Repository navigation
Conversation
Reviewer's GuideThe data-plane binary now runs a static, environment-configurable HTTP reverse proxy with streaming, cancellation, hop-by-hop header filtering, structured outcomes, server timeouts, and bounded graceful shutdown; comprehensive M1 tests and documentation capture the implemented behavior and verification evidence. Sequence diagram for streaming proxy requests and cancellationsequenceDiagram
participant Client as HTTP Client
participant Server as HTTP Server
participant Proxy as ReverseProxy
participant Upstream as Static Upstream
Client->>Server: HTTP request
Server->>Proxy: ServeHTTP
Proxy->>Upstream: Forward request
Upstream-->>Proxy: Response headers
Proxy-->>Client: Response headers
loop response chunks
Upstream-->>Proxy: Response body chunk
Proxy-->>Client: Stream chunk
end
Client--xServer: Cancel request
Server--xProxy: Request context cancellation
Proxy--xUpstream: Cancel upstream request
Sequence diagram for graceful data-plane shutdownsequenceDiagram
participant Signal as SIGINT or SIGTERM
participant Lifecycle as Lifecycle
participant Server as HTTP Server
participant Proxy as Reverse Proxy
participant Upstream as Static Upstream
Signal->>Lifecycle: NotifyContext cancellation
Lifecycle->>Server: Shutdown timeout
Server-->>Server: Stop accepting requests
Server->>Proxy: Drain in-flight request
Proxy->>Upstream: Complete active request
Upstream-->>Proxy: Final response
Proxy-->>Server: Request finished
Server->>Proxy: CloseIdleConnections
Server-->>Lifecycle: Shutdown result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="cmd/gatehouse-data/main.go" line_range="38" />
<code_context>
+ }
+ server, err := dataplane.NewServer(proxy, dataplane.ServerConfig{
+ Address: envOrDefault("GATEHOUSE_LISTEN_ADDR", "127.0.0.1:8080"),
+ })
+ if err != nil {
+ return fmt.Errorf("configure data-plane listener: %w", err)
</code_context>
<issue_to_address>
**Protected upstream exposed without authentication**
When the operator sets `GATEHOUSE_LISTEN_ADDR` to a non-loopback interface and the upstream has protected endpoints, `NewServer` listens on the configured non-loopback address, and `Proxy.ServeHTTP` forwards every method and path without authenticating callers, so external users can reach protected upstream endpoints.
Keep unauthenticated mode loopback-only, or require authentication before accepting requests on an externally reachable listener.
Also at `internal/dataplane/proxy.go:102`.
</issue_to_address>
### Comment 2
<location path="internal/dataplane/server.go" line_range="73-76" />
<code_context>
+ s.listener = listener
+ s.server = &http.Server{
+ Handler: s.handler,
+ ReadHeaderTimeout: s.cfg.ReadHeaderTimeout,
+ IdleTimeout: s.cfg.IdleTimeout,
+ MaxHeaderBytes: s.cfg.MaxHeaderBytes,
+ }
+ s.serveErr = make(chan error, 1)
+ s.started = true
</code_context>
<issue_to_address>
**Slow requests exhaust proxy resources**
When an attacker can reach the listener and sends a body slowly or causes upstream requests to stall, `http.Server` bounds header reading but sets no request-body read timeout, while the cloned upstream transport has no response-header timeout; slow clients or stalled upstreams therefore keep requests and connections active indefinitely, exhausting resources under concurrency.
Add bounded request/body and upstream-response waiting policies plus concurrency protection that preserve intended streaming behavior.
Also at `internal/dataplane/proxy.go:47-55`.
</issue_to_address>
### Comment 3
<location path="internal/dataplane/proxy.go" line_range="106-110" />
<code_context>
+}
+
+type statusRecorder struct {
+ http.ResponseWriter
+ status int
+}
+
+func (w *statusRecorder) Unwrap() http.ResponseWriter { return w.ResponseWriter }
+
+func (w *statusRecorder) WriteHeader(status int) {
</code_context>
<issue_to_address>
**Protocol upgrades fail**
When a client requests an HTTP protocol upgrade and the upstream returns 101 Switching Protocols, `statusRecorder` does not implement or forward `http.Hijacker`, so `httputil.ReverseProxy` cannot take over the client connection and establish the tunnel; the client sees a failed upgrade.
Forward `http.Hijacker` through `statusRecorder` so `ReverseProxy` can establish upgraded connections.
Also at `internal/dataplane/proxy.go:75`.
</issue_to_address>
### Comment 4
<location path="internal/dataplane/proxy.go" line_range="98" />
<code_context>
+ p.logger.Info("proxy request complete",
+ "method", r.Method,
+ "status", status,
+ "outcome", "success",
+ "duration", time.Since(started),
+ )
</code_context>
<issue_to_address>
**Failed requests logged as successful**
When the upstream request fails or its response body aborts, the `ErrorHandler` writes a 502 and returns normally, and `httputil.ReverseProxy` also returns normally when response copying fails. The deferred completion log still sets `outcome` to `success` (only the panic branch records failure), so operators see failed proxy requests counted as successful.
Track proxy errors, including transport failures and response-copy failures, and log `outcome=failure` instead of unconditionally logging success.
Also at `internal/dataplane/proxy.go:56-59`, `internal/dataplane/proxy.go:95`, `internal/dataplane/proxy.go:99`.
</issue_to_address>
### Comment 5
<location path="internal/dataplane/proxy_test.go" line_range="487" />
<code_context>
+ if err != nil {
+ t.Fatal(err)
+ }
+ req.Header.Set("Connection", "X-Hop-By-Hop")
+ req.Header.Set("X-Hop-By-Hop", "must-not-forward")
+ resp, err := http.DefaultClient.Do(req)
</code_context>
<issue_to_address>
**Connection header leaks go undetected**
When a proxy regression forwards `Connection` while still stripping its nominated header, `TestProxyStripsHopByHopRequestHeaders` records only `X-Hop-By-Hop` from the request (lines 469, 487–497) and checks only `X-Upstream-Hop` in the response (lines 470–471, 497–499), so a proxy that forwards `Connection` but drops its nominated header passes both checks.
Also assert that `Connection` is absent from the upstream request and the client response.
Also at `internal/dataplane/proxy_test.go:470`.
</issue_to_address>
### Comment 6
<location path="internal/dataplane/server_test.go" line_range="73-74" />
<code_context>
+ select {
+ case err := <-shutdownResult:
+ t.Fatalf("server shutdown returned before in-flight request drained: %v", err)
+ case <-time.After(25 * time.Millisecond):
+ }
+ releaseOnce.Do(func() { close(releaseUpstream) })
+
</code_context>
<issue_to_address>
**Shutdown test can miss early return**
When the shutdown goroutine is delayed past the 25 ms timeout, the test releases the upstream request after the timer fires, so an implementation that returns early without draining can pass if shutdown was not scheduled before the release.
Synchronize on shutdown actually starting or use another deterministic signal before releasing the upstream request.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 6 findings to address first, and if the proxy routes requests to the wrong upstream or mishandles HTTP boundaries, it could expose request data or cause an outage for traffic passing through the new listener. Reverting stops future proxying, but it cannot undo requests already served or data already exposed.
Blocking findings: cmd/gatehouse-data/main.go:38, internal/dataplane/server.go:76, internal/dataplane/proxy.go:110, internal/dataplane/proxy.go:98, internal/dataplane/proxy_test.go:487, and 1 more
| } | ||
| server, err := dataplane.NewServer(proxy, dataplane.ServerConfig{ | ||
| Address: envOrDefault("GATEHOUSE_LISTEN_ADDR", "127.0.0.1:8080"), | ||
| }) |
There was a problem hiding this comment.
🟠 High · Protected upstream exposed without authentication
When the operator sets GATEHOUSE_LISTEN_ADDR to a non-loopback interface and the upstream has protected endpoints, NewServer listens on the configured non-loopback address, and Proxy.ServeHTTP forwards every method and path without authenticating callers, so external users can reach protected upstream endpoints.
Keep unauthenticated mode loopback-only, or require authentication before accepting requests on an externally reachable listener.
Also at internal/dataplane/proxy.go:102.
Prompt for AI agents
In `cmd/gatehouse-data/main.go` at line 38:
**Protected upstream exposed without authentication**
When the operator sets `GATEHOUSE_LISTEN_ADDR` to a non-loopback interface and the upstream has protected endpoints, `NewServer` listens on the configured non-loopback address, and `Proxy.ServeHTTP` forwards every method and path without authenticating callers, so external users can reach protected upstream endpoints.
Keep unauthenticated mode loopback-only, or require authentication before accepting requests on an externally reachable listener.
Also at `internal/dataplane/proxy.go:102`.
Comment on lines
+73
to
+76
| ReadHeaderTimeout: s.cfg.ReadHeaderTimeout, | ||
| IdleTimeout: s.cfg.IdleTimeout, | ||
| MaxHeaderBytes: s.cfg.MaxHeaderBytes, | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Slow requests exhaust proxy resources
When an attacker can reach the listener and sends a body slowly or causes upstream requests to stall, http.Server bounds header reading but sets no request-body read timeout, while the cloned upstream transport has no response-header timeout; slow clients or stalled upstreams therefore keep requests and connections active indefinitely, exhausting resources under concurrency.
Add bounded request/body and upstream-response waiting policies plus concurrency protection that preserve intended streaming behavior.
Also at internal/dataplane/proxy.go:47-55.
Prompt for AI agents
In `internal/dataplane/server.go` at lines 73-76:
**Slow requests exhaust proxy resources**
When an attacker can reach the listener and sends a body slowly or causes upstream requests to stall, `http.Server` bounds header reading but sets no request-body read timeout, while the cloned upstream transport has no response-header timeout; slow clients or stalled upstreams therefore keep requests and connections active indefinitely, exhausting resources under concurrency.
Add bounded request/body and upstream-response waiting policies plus concurrency protection that preserve intended streaming behavior.
Also at `internal/dataplane/proxy.go:47-55`.
Comment on lines
+106
to
+110
| http.ResponseWriter | ||
| status int | ||
| } | ||
|
|
||
| func (w *statusRecorder) Unwrap() http.ResponseWriter { return w.ResponseWriter } |
There was a problem hiding this comment.
🟡 Medium · Protocol upgrades fail
When a client requests an HTTP protocol upgrade and the upstream returns 101 Switching Protocols, statusRecorder does not implement or forward http.Hijacker, so httputil.ReverseProxy cannot take over the client connection and establish the tunnel; the client sees a failed upgrade.
Forward http.Hijacker through statusRecorder so ReverseProxy can establish upgraded connections.
Also at internal/dataplane/proxy.go:75.
Prompt for AI agents
In `internal/dataplane/proxy.go` at lines 106-110:
**Protocol upgrades fail**
When a client requests an HTTP protocol upgrade and the upstream returns 101 Switching Protocols, `statusRecorder` does not implement or forward `http.Hijacker`, so `httputil.ReverseProxy` cannot take over the client connection and establish the tunnel; the client sees a failed upgrade.
Forward `http.Hijacker` through `statusRecorder` so `ReverseProxy` can establish upgraded connections.
Also at `internal/dataplane/proxy.go:75`.| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| req.Header.Set("Connection", "X-Hop-By-Hop") |
There was a problem hiding this comment.
🟡 Medium · Connection header leaks go undetected
When a proxy regression forwards Connection while still stripping its nominated header, TestProxyStripsHopByHopRequestHeaders records only X-Hop-By-Hop from the request (lines 469, 487–497) and checks only X-Upstream-Hop in the response (lines 470–471, 497–499), so a proxy that forwards Connection but drops its nominated header passes both checks.
Also assert that Connection is absent from the upstream request and the client response.
Also at internal/dataplane/proxy_test.go:470.
Prompt for AI agents
In `internal/dataplane/proxy_test.go` at line 487:
**Connection header leaks go undetected**
When a proxy regression forwards `Connection` while still stripping its nominated header, `TestProxyStripsHopByHopRequestHeaders` records only `X-Hop-By-Hop` from the request (lines 469, 487–497) and checks only `X-Upstream-Hop` in the response (lines 470–471, 497–499), so a proxy that forwards `Connection` but drops its nominated header passes both checks.
Also assert that `Connection` is absent from the upstream request and the client response.
Also at `internal/dataplane/proxy_test.go:470`.
Comment on lines
+73
to
+74
| case <-time.After(25 * time.Millisecond): | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Shutdown test can miss early return
When the shutdown goroutine is delayed past the 25 ms timeout, the test releases the upstream request after the timer fires, so an implementation that returns early without draining can pass if shutdown was not scheduled before the release.
Synchronize on shutdown actually starting or use another deterministic signal before releasing the upstream request.
Prompt for AI agents
In `internal/dataplane/server_test.go` at lines 73-74:
**Shutdown test can miss early return**
When the shutdown goroutine is delayed past the 25 ms timeout, the test releases the upstream request after the timer fires, so an implementation that returns early without draining can pass if shutdown was not scheduled before the release.
Synchronize on shutdown actually starting or use another deterministic signal before releasing the upstream request.
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.
Summary
Verification
make verifypassed locally.git diff --checkpassed.Scope
M1 proves the static HTTP request lifecycle. Dynamic route/cluster configuration and control-plane publication remain later milestones.