Repository navigation
fix: critical and major issues from code audit - #254
Merged
Merged
Conversation
… return cancel the discovery context on every run() return path and await a bounded discoveryDone before exit so provider watcher goroutines don't outlive run. register signal.Stop and a catch-all defer cancel so the signal goroutine can't leak across run() calls.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses audited stability, shutdown, resource leak, routing duplication, and documentation issues across discovery, plugin middleware, Consul discovery, and CLI/docs.
Changes:
- Adds synchronization and tests for discovery mapper cache access and health ping response cleanup.
- Fixes plugin middleware lock handling and Consul service deduplication.
- Propagates shutdown context into discovery/health checks and documents trusted-proxy requirements for remote lookup headers.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents spoofing/trust requirements for remote lookup headers. |
| docs/plans/20260530-fix-critical-major-audit-findings.md | Adds implementation plan and verification notes. |
| app/plugin/conductor.go | Snapshots plugins under lock before invoking RPC handlers. |
| app/plugin/conductor_test.go | Adds regression coverage for plugin lock leak. |
| app/main.go | Propagates cancellation to discovery/health checks and updates flag help. |
| app/main_test.go | Adds early-error shutdown regression test. |
| app/discovery/provider/consulcatalog/client.go | Prevents duplicate service names from multiple reproxy tags. |
| app/discovery/provider/consulcatalog/client_test.go | Adds filterServices deduplication tests. |
| app/discovery/discovery.go | Guards mapper cache access and closes ping response bodies. |
| app/discovery/discovery_test.go | Adds concurrent match and ping behavior coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go func() { | ||
| if e := svc.Run(context.Background()); e != nil { | ||
| defer close(discoveryDone) | ||
| if e := svc.Run(ctx); e != nil && !errors.Is(e, context.Canceled) { |
the provider watcher goroutines did a bare send on the events channel, so on shutdown a producer blocked mid-send (once the consumer stopped on ctx cancel) could leak. guard both sends with a select on ctx.Done so the discovery shutdown wait can't be left hanging on a stuck producer.
…e godoc - add TestConductor_MiddlewareReleasesLockDuringCall: a blocking plugin RPC, asserting a write-lock op succeeds while the call is in flight (proves the read lock is released before the RPC, not held across it) - assert run() returns well under the 5s shutdown bound on the error path, proving discovery's ctx is cancelled before the wait - correct Conductor.Middleware godoc: a call error returns HTTP 500 and stops the chain, a reply status >= 400 stops with that status
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes a set of bugs found by a code audit of the proxy, discovery, and plugin layers. One is a remote-triggerable crash, the rest are resource/lock leaks and a routing dup.
fixed
discovery.go).Service.Matchruns per request underRLock, butfindMatchingMapperslazily wrotemappersCacheon the read path. Two parallel requests for the same uncached wildcard/regex host triggeredfatal error: concurrent map writesand killed the proxy. Now the cache is guarded by a dedicatedcacheLock, andfindMatchingMappersis a method onService. Reachable by an unauthenticated caller whenever wildcard (*.example.com) or regex server patterns are configured.discovery.go).URLMapper.pingnever closed the response body, leaking a connection per interval tick. Addeddefer resp.Body.Close().consulcatalog/client.go).filterServicesappended a service once perreproxy.-prefixed tag, so a multi-tagged service produced N duplicateURLMappers and skewed load-balancing.breakafter the first match.conductor.go).Conductor.MiddlewareheldRLockacross the blocking plugin RPC and returned withoutRUnlockon the call-error path, deadlocking later plugin (un)register. Now snapshots the alive plugins under the lock and calls RPC outside it.main.go).run()launched discovery withcontext.Background()and never awaited it, so provider-watcher goroutines outlivedrun()(this surfaced as a flaky-racefailure in the logger). Discovery now gets the cancellable ctx andrun()waits for it on exit, with a 5s bound so a context-less providerList()can't hang process exit. The signal goroutine and registration no longer leak on early error returns.docs
--remote-lookup-headersnow documents thatX-Real-IP/X-Forwarded-Forare client-supplied and spoofable, so it must only be enabled behind a trusted proxy that overwrites those headers (the per-route IP allowlist trusts them when this is on).all changes have tests (race test for the crash, deadlock test for the plugin lock, dedup/ping/error-path coverage). Full
-racesuite green, golangci-lint clean.one related issue found but left out of scope: a latent data race in
proxy.goHttp.Run()on thehttpServer/httpsServerlocals between setup and thectx.Done()close goroutine. Pre-existing and masked by timing, worth a separate PR.