fix: stop truncating proxied requests and harden proxy auth surface - #1
Merged
Merged
Conversation
The audit body cap was doing double duty as the limit on the request body actually forwarded upstream. Any request over GAP_AUDIT_MAX_BODY_BYTES (default 64 KiB) was silently cut mid-JSON and relayed to xAI, which answered with an unexplainable parse error. The two limits are now separate: GAP_SERVER_MAX_REQUEST_BYTES (default 10 MiB) bounds what is proxied and returns 413 above it, while the audit cap only clips what is persisted. As a side effect the audit row's model/stream fields are now parsed from the complete body instead of the clipped copy. Security hardening on the same paths: - AdminAuth compared the admin key with a byte-wise !=, which returns on the first differing byte and leaks the key through response latency. Both sides are now SHA-256'd and compared with crypto/subtle.ConstantTimeCompare, so the compare is constant time and length independent. An unset or whitespace-only admin key now rejects every caller instead of authenticating one that echoes it back. - The proxy relayed X-Admin-Key, Proxy-Authorization and Cookie to xAI. Those are credentials scoped to this proxy and are now stripped, along with the remaining RFC 9110 hop-by-hop headers. - http.Server had no ReadTimeout and no IdleTimeout, so a client could dribble out a body or park idle keep-alive connections forever. Both are now set and configurable. WriteTimeout stays zero on purpose for SSE streams. - config rejects a non-HTTPS auth.upstream_base or auth.issuer for non-loopback hosts; those requests carry the Grok access token and refresh token, which must not cross the network in cleartext. - The metrics middleware labelled unmatched routes with the raw URL path, letting any caller mint one Prometheus series per 404'd URL and exhaust process memory. Unmatched requests now share one label value. - CORS sets Vary: Origin on every response when the origin list is restricted, not just on a match, so a shared cache cannot replay an allowed origin's Access-Control-Allow-Origin to a disallowed one. Adds middleware tests (none existed) plus proxy and config tests; each new test was confirmed to fail with its fix reverted. Adds a CI workflow running gofmt/tidy/build/vet/test/test -race, since only the Docker image build ran on PRs before, and gofmt-formats the three files that gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moveeeax
added a commit
that referenced
this pull request
Jul 26, 2026
The audit body cap was doing double duty as the limit on the request body actually forwarded upstream. Any request over GAP_AUDIT_MAX_BODY_BYTES (default 64 KiB) was silently cut mid-JSON and relayed to xAI, which answered with an unexplainable parse error. The two limits are now separate: GAP_SERVER_MAX_REQUEST_BYTES (default 10 MiB) bounds what is proxied and returns 413 above it, while the audit cap only clips what is persisted. As a side effect the audit row's model/stream fields are now parsed from the complete body instead of the clipped copy. Security hardening on the same paths: - AdminAuth compared the admin key with a byte-wise !=, which returns on the first differing byte and leaks the key through response latency. Both sides are now SHA-256'd and compared with crypto/subtle.ConstantTimeCompare, so the compare is constant time and length independent. An unset or whitespace-only admin key now rejects every caller instead of authenticating one that echoes it back. - The proxy relayed X-Admin-Key, Proxy-Authorization and Cookie to xAI. Those are credentials scoped to this proxy and are now stripped, along with the remaining RFC 9110 hop-by-hop headers. - http.Server had no ReadTimeout and no IdleTimeout, so a client could dribble out a body or park idle keep-alive connections forever. Both are now set and configurable. WriteTimeout stays zero on purpose for SSE streams. - config rejects a non-HTTPS auth.upstream_base or auth.issuer for non-loopback hosts; those requests carry the Grok access token and refresh token, which must not cross the network in cleartext. - The metrics middleware labelled unmatched routes with the raw URL path, letting any caller mint one Prometheus series per 404'd URL and exhaust process memory. Unmatched requests now share one label value. - CORS sets Vary: Origin on every response when the origin list is restricted, not just on a match, so a shared cache cannot replay an allowed origin's Access-Control-Allow-Origin to a disallowed one. Adds middleware tests (none existed) plus proxy and config tests; each new test was confirmed to fail with its fix reverted. Adds a CI workflow running gofmt/tidy/build/vet/test/test -race, since only the Docker image build ran on PRs before, and gofmt-formats the three files that gate.
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.
This is an authenticating proxy, so the auth and forwarding paths are the product. Reading them turned up one outright bug and a set of hardening gaps on the same code.
The bug: the audit cap was truncating real traffic
audit.max_body_bytesis documented as "max body size stored per side". It was also being used as the limit on the request body actually forwarded to xAI:Any request over 64 KiB — a long conversation, a pasted document — was silently cut mid-JSON and relayed upstream, which answered with a parse error the client had no way to explain. It happened even with
audit.enabled=false.The two limits are now separate:
GAP_SERVER_MAX_REQUEST_BYTES(default 10 MiB) bounds what is proxied, and oversized requests get413instead of being truncated and forwarded.GAP_AUDIT_MAX_BODY_BYTESclips only what is persisted, and setsrequest_truncatedas before.Side effect worth noting: the audit row now parses
modelandstreamfrom the complete body rather than the clipped copy, so those columns stop coming back empty for large requests.Hardening on the same paths
got != adminKeyreturns at the first differing byte, leaking the key through response latency. Both sides are now SHA-256'd and compared withcrypto/subtle.ConstantTimeCompare— constant time and independent of key length. An unset or whitespace-only admin key now rejects everyone, where before a caller echoing that same whitespace back was authenticated.X-Admin-Key,Proxy-AuthorizationandCookiewere forwarded to xAI. They are credentials scoped to this proxy; now stripped, along with the remaining RFC 9110 hop-by-hop headers (keep-alive,te,trailer,upgrade).http.Serverset onlyReadHeaderTimeout, so a client could dribble out a request body or park idle keep-alive connections indefinitely — a trivial slowloris.ReadTimeout(60s) andIdleTimeout(120s) are now set and configurable, plus an explicitMaxHeaderBytes.WriteTimeoutstays zero on purpose; a write deadline would cut SSE streams off mid-response, and that is now commented rather than implied.auth.upstream_baseandauth.issuercarry the Grok access token and refresh token respectively. Config now rejects non-HTTPS values for non-loopback hosts. Loopback stays permitted so local mock upstreams keep working.Vary: Originwas set only when the origin matched the allowlist, letting a shared cache replay an allowed origin'sAccess-Control-Allow-Originto a disallowed one. It is now set on every response when the origin list is restricted.TLS itself was already fine — no server-side TLS by design (ingress terminates), and the outbound client uses Go defaults with a floor of TLS 1.2.
Tests
internal/middlewarehad no tests at all; it now has the admin-auth accept/reject matrix and the CORS cases. Added proxy tests for the truncation regression, the 413 path, the audit-clipping split and header stripping, and config tests for the HTTPS enforcement and the new defaults.Each new test was confirmed to fail with only its fix reverted:
TestProxyRejectsOversizedRequestBody: status=200, want 413TestProxyStripsClientCredentialHeaders: X-Admin-Key leaked upstream as "admin-secret"==admin compareTestAdminAuthRejectsEverythingWhenKeyUnset: admin_key=" " sent=" ": expected 401, got 200VaryTestCORSAlwaysVariesOnOriginWhenRestricted: origin="https://evil.example.com": Vary="", want OriginTestValidateRejectsCleartextCredentialEndpoints: http_upstream_remote, bad_scheme, relative, http_issuer_remoteCI
Only the Docker image build ran on PRs — nothing executed the test suite. Added
.github/workflows/ci.ymlrunning gofmt,go mod tidyverification, build, vet, test andtest -race. Three files were already not gofmt-clean and are formatted here so the gate passes (whitespace-only; struct field alignment instore.go,store_test.go,metrics.go).Verified locally
Go 1.26.5, all in the foreground:
No breaking changes: the three new config keys are additive with defaults matching prior behaviour, and no exported function signature changed except
proxy.Options, which gains an optional field.The one behaviour change a consumer could notice is the HTTPS requirement on
auth.upstream_base/auth.issuer— a deployment pointing at a remote plaintext HTTP upstream will now fail to start rather than leak its token on the wire. Loopback is exempt.🤖 Generated with Claude Code