Skip to content

fix: scrub late cover response trailers without buffering - #23

Merged
cppla merged 1 commit into
mainfrom
codex/cover-response-trailer-scrubbing
Sep 22, 2026
Merged

cppla merged 1 commit into
mainfrom
codex/cover-response-trailer-scrubbing

Conversation

@cppla

@cppla cppla commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

Extend the existing cover response credential filtering to response trailers, including metadata a transport discovers only at body EOF or Close.

  • Filter unsafe trailer declarations before forwarding response headers, and reapply the same filtering after terminal body reads and closes.
  • Follow the response's current Trailer map so replacement maps and late fields are handled.
  • Preserve ordinary trailers, streaming reads, original n+error and Close results. No full-body buffering is added.
  • Keep locks outside underlying I/O and defer map cleanup until concurrent body operations have returned, so Close does not wait on a blocked reader.
  • Preserve the body type for 101 responses; this does not add upgrade support.

No request-trailer behavior, tunnel authentication, wire protocol, default configuration, dependency, version or release change.

Verification

  • Real two-hop HTTP/1.1 tests cover declared and undeclared upstream credential trailers, initial response-header scrubbing and retention of an ordinary X-End trailer.
  • A negative-control overlay using the original handler from main 49b15b9 fails the new on-wire regression for undeclared credential trailers; the fixed handler passes.
  • Unit tests cover lazy streaming, terminal n+EOF/read-error/cancellation, replacement Trailer maps, Close-populated metadata, original/repeated Close results and concurrent Close/Read.
  • A real two-hop request-cancellation regression verifies an already-streamed prefix, client read termination, upstream request cancellation, upstream body Close and frontend handler exit.
  • Local cover-package race tests and the real cancellation test each passed for 20 repetitions.
  • Final local make check and make race passed.

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 27a2fa15cb9db0ffc0fbf51340b7b01cfa9dda69 passed 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 f6a931b7156ff0e57aae2e7c2f4a532ef9557022 has the identical source tree 016bf4324b42311038b2e3260f10bf0ea15be814 and passed its own push-triggered CI, CodeQL and Linux namespace/netem integration.

Copilot AI lite review requested due to automatic review settings September 22, 2026 14:36
@cppla
cppla merged commit f6a931b into main Sep 22, 2026
14 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread internal/cover/handler.go
b.active--
b.pending = b.pending || terminal
if b.active == 0 && b.pending {
removeUnsafeHeaders(b.response.Trailer)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants