fix: scrub late cover response trailers without buffering - #23
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Only a non-blocking test-coverage nit remains; no approval-blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Extends reverse-proxy credential filtering to response trailers discovered during streaming and body closure, without buffering responses.
Changes:
- Filters late trailers while preserving streaming, cancellation, and 101 body behavior.
- Adds wire-level, unit, concurrency, and cancellation tests.
- Documents response trailer filtering.
| File | Summary |
|---|---|
internal/cover/response_trailer_test.go |
Tests trailer filtering and streaming behavior. |
internal/cover/response_cancel_test.go |
Verifies cancellation propagation. |
internal/cover/handler.go |
Implements late trailer sanitization while preserving response behavior. |
docs/WEB_COVER.md |
Documents trailer filtering guarantees. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a2fa15cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| b.active-- | ||
| b.pending = b.pending || terminal | ||
| if b.active == 0 && b.pending { | ||
| removeUnsafeHeaders(b.response.Trailer) |
There was a problem hiding this comment.
Preserve late ordinary trailers when filtering replacement maps
When the supplied RoundTripper replaces Response.Trailer, this cleanup can make the final map's length equal the number of initially announced trailers even though its keys changed—for example, {X-Old} becomes {Authorization, X-New}, then filtering leaves {X-New}. httputil.ReverseProxy uses that length equality to assume the declarations are unchanged and copies X-New as an ordinary header after WriteHeader, so it is discarded on the wire; the safe late trailer is therefore lost. Preserve the original safe declarations or otherwise ensure changed trailer key sets take the late-trailer path.
Useful? React with 👍 / 👎.
Summary
Extend the existing cover response credential filtering to response trailers, including metadata a transport discovers only at body EOF or Close.
No request-trailer behavior, tunnel authentication, wire protocol, default configuration, dependency, version or release change.
Verification
49b15b9fails the new on-wire regression for undeclared credential trailers; the fixed handler passes.make checkandmake racepassed.Evidence limits
This is defensive handling of an upstream sending authentication-related fields in prohibited trailers. The local diagnostic does not establish client tunnel-token leakage, authentication bypass, arbitrary-origin access or SSRF. No separate remote-host or passive-traffic experiment is claimed.
Exact PR head
27a2fa15cb9db0ffc0fbf51340b7b01cfa9dda69passed CI, CodeQL and Linux namespace/netem integration. CI includes Linux/macOS tests, race detection, reachable vulnerability scanning, four-platform builds, actual Docker integration and the multi-platform OCI image build. No review change requests or open review threads were present at merge preparation. No tag or release is created.Post-merge verification: main commit
f6a931b7156ff0e57aae2e7c2f4a532ef9557022has the identical source tree016bf4324b42311038b2e3260f10bf0ea15be814and passed its own push-triggered CI, CodeQL and Linux namespace/netem integration.