fix(proxy): forwardHeaders must not override framing headers by pi0x · Pull Request #1467 · h3js/h3

pi0x · 2026-07-14T12:37:02Z

forwardHeaders is documented as an escape hatch for the "soft" denylist drops (host, accept-encoding), but it previously won over the entire hop-by-hop denylist and Connection nominations. That let a caller force-forward true framing headers (connection, keep-alive, transfer-encoding, te, trailer, upgrade, proxy-authorization, proxy-connection) upstream, which can desync request framing or leak the inbound proxy's credentials.

This partitions the denylist into a dedicated framingHeaders set: forwardHeaders may now only override the soft drops, never the framing set or Connection-nominated tokens. The forwardHeaders JSDoc is updated to document the constraint, and regression tests cover both that framing headers stay dropped and that soft drops (e.g. accept-encoding) still forward.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved proxy header forwarding to prevent unsafe hop-by-hop framing headers from being forwarded.
- Preserved the ability to explicitly forward permitted headers, such as Accept-Encoding.
- Added safeguards for Transfer-Encoding, Connection, and Keep-Alive while continuing to forward valid headers.

`forwardHeaders` is meant as an escape hatch for the "soft" denylist drops (`host`, `accept-encoding`), but it previously won over the entire hop-by-hop denylist and `Connection` nominations. That let a caller force-forward true framing headers (`connection`, `transfer-encoding`, `te`, ...) upstream, which can desync request framing or leak the inbound proxy's credentials. Partition the denylist: `forwardHeaders` may only override soft drops, never the framing set or `Connection`-nominated tokens. Co-Authored-By: Claude Fable 5 <[email protected]>

coderabbitai · 2026-07-14T12:39:10Z

Warning

Review limit reached

@pi0x, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f21b6743-91aa-41fe-bfc3-65cc3035d93d

📥 Commits

Reviewing files that changed from the base of the PR and between 0b0042a and ebe4157.

📒 Files selected for processing (2)

src/utils/internal/proxy.ts
src/utils/proxy.ts

📝 Walkthrough

Walkthrough

The proxy adds a dedicated set of non-overridable framing headers, restricts forwardHeaders accordingly, documents the distinction from soft drops, and adds tests for accept-encoding and hop-by-hop headers.

Changes

Proxy header forwarding

Layer / File(s)	Summary
Framing header contract `src/utils/internal/proxy.ts`, `src/utils/proxy.ts`	Defines exported framing headers and documents which dropped headers `forwardHeaders` may override.
Forwarding filter and coverage `src/utils/proxy.ts`, `test/proxy.test.ts`	Prevents forwarding framing or `Connection`-nominated headers while testing permitted soft-drop overrides and protected headers.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

h3js/h3#1423: Changes proxy handling of accept-encoding, which is covered by this forwarding behavior.

Suggested reviewers: pi0, iainsproat

Poem

I’m a rabbit guarding the wire,
Keeping framing headers from climbing higher.
Soft drops may hop when invited through,
But connection tricks stay out of view.
The proxy now thumps its paws: “Headers behave!”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: preventing forwardHeaders from overriding framing headers in the proxy.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/proxy-forward-headers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pi0x

Verdict: APPROVE (one minor, non-blocking doc nit)

Reviewed independently. Fetched the branch, pnpm install, pnpm vitest run test/proxy.test.ts (104 passed / 24 skipped), and pnpm lint all green.

Correctness — looks right:

The framing guard is effective: framingHeaders is a strict subset of ignoredHeaders, so a blocked framing header still falls through to the ignoredHeaders drop rather than leaking. (Worth keeping in mind: if a future entry is added to framingHeaders but not ignoredHeaders, the !framingHeaders.has(name) block would be a no-op since the header would forward via the !ignoredHeaders.has(name) branch. A brief invariant comment could help, but current state is fine.)
Case-insensitivity holds end to end: forwardHeaders is lowercased up front, framingHeaders entries are lowercase, Headers.entries() yields lowercase names, and connectionTokens() lowercases. All comparisons align.
Connection-nominated tokens are now correctly excluded from the escape hatch (!connectionNominated.has(name)) and still dropped below.
No behavior change when forwardHeaders is unset: forwardHeaders?.includes(name) short-circuits to falsy, so the added && clauses never run.

Tests — solid:

Confirmed the framing regression test genuinely fails without the fix (expected 'chunked' to be undefined → received 'chunked'), so it's a real guard, not a tautology.
The web-only skip on the framing test is justified: on the node target t.fetch uses real fetch, which strips forbidden request headers (connection/transfer-encoding/keep-alive) before they ever reach the proxy, so the assertion can't be exercised there. The soft-drop (accept-encoding) test correctly runs on both targets.

Minor nit (non-blocking): the partition leaves three soft drops overridable by forwardHeaders — host, accept-encoding, and also expect (it's in ignoredHeaders but not framingHeaders). The ProxyOptions JSDoc says "Only the 'soft' drops (host, accept-encoding) can be overridden this way", which reads as exhaustive and omits expect. Leaving expect soft is defensible (it isn't in the classic RFC 9110 §7.6.1 hop-by-hop set), but consider either mentioning expect in the doc or moving it into framingHeaders for consistency. Behavior for expect is unchanged by this PR either way.

Style matches AGENTS.md conventions (/* @__PURE__ */ new Set, Set<string> for isolatedDeclarations, RFC-referenced JSDoc).

🤖 Generated with Claude Code

`expect` is in `ignoredHeaders` but not in `framingHeaders`, so it remains overridable via `forwardHeaders` — the soft-drop list read as exhaustive without it. Co-Authored-By: Claude Fable 5 <[email protected]>

pi0x · 2026-07-14T12:41:40Z

Addressed the reviewer's doc nit in ebe4157: the forwardHeaders JSDoc (and the matching framingHeaders comment) now lists expect in the soft-drop set alongside host and accept-encoding. expect stays soft — it was not moved into framingHeaders.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/proxy.test.ts`:
- Around line 348-382: Replace the runtime-gated test declaration around “does
not force-forward hop-by-hop framing headers via forwardHeaders” with skipIf,
skipping when the target is node. Preserve the test body and its existing
web-target behavior unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e68e6ca5-3209-4bfb-aad1-54c7be0a3a3b

📥 Commits

Reviewing files that changed from the base of the PR and between 2264316 and 0b0042a.

📒 Files selected for processing (3)

src/utils/internal/proxy.ts
src/utils/proxy.ts
test/proxy.test.ts

pi0x requested a review from pi0 as a code owner July 14, 2026 12:37

pi0x commented Jul 14, 2026

View reviewed changes

docs(proxy): include expect in the soft-drop list for forwardHeaders

ebe4157

`expect` is in `ignoredHeaders` but not in `framingHeaders`, so it remains overridable via `forwardHeaders` — the soft-drop list read as exhaustive without it. Co-Authored-By: Claude Fable 5 <[email protected]>

coderabbitai Bot reviewed Jul 14, 2026

View reviewed changes

Comment thread test/proxy.test.ts

pi0 approved these changes Jul 15, 2026

View reviewed changes

pi0 merged commit 12b6efe into main Jul 15, 2026
9 checks passed

pi0 deleted the fix/proxy-forward-headers branch July 15, 2026 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(proxy): forwardHeaders must not override framing headers#1467

fix(proxy): forwardHeaders must not override framing headers#1467
pi0 merged 2 commits into
mainfrom
fix/proxy-forward-headers

pi0x commented Jul 14, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 14, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

pi0x left a comment

Uh oh!

pi0x commented Jul 14, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pi0x commented Jul 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

pi0x left a comment

Choose a reason for hiding this comment

Uh oh!

pi0x commented Jul 14, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pi0x commented Jul 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 14, 2026 •

edited

Loading