Skip to content

fix(proxy): ignore incoming accept-encoding header#1423

Merged
pi0 merged 1 commit into
h3js:mainfrom
mahirhir:fix/proxy-accept-encoding
Jul 2, 2026
Merged

fix(proxy): ignore incoming accept-encoding header#1423
pi0 merged 1 commit into
h3js:mainfrom
mahirhir:fix/proxy-accept-encoding

Conversation

@mahirhir

@mahirhir mahirhir commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

v1 strips the incoming accept-encoding header before proxying (#914). The v2 ignoredHeaders set in src/utils/internal/proxy.ts added accept but dropped accept-encoding, so getProxyRequestHeaders forwards the client's accept-encoding to the target.

proxy() removes content-encoding from the upstream response, relying on the runtime's fetch having already decoded the body. Forwarding the client's accept-encoding lets the target pick an encoding the runtime's fetch may not decode (the zstd-on-undici case from nitrojs/nitro#2880 that #914 addressed); the still-encoded body would then be passed on without a content-encoding header. Since h3 v2 runs across runtimes whose fetch clients support different encoding sets, this restores the v1 behavior by adding accept-encoding back to the set so the runtime fetch negotiates and decodes the encoding itself.

Added a regression test: proxying a request that sends accept-encoding: zstd, br, gzip now asserts the upstream does not receive it. Without the change the header is forwarded (web and node test targets); with it the header is dropped. Full suite passes.

Ref: #914

Summary by CodeRabbit

  • Bug Fixes
    • Updated proxy handling so incoming accept-encoding headers are no longer forwarded upstream.
    • This helps ensure proxied requests use the expected header behavior.
  • Tests
    • Added coverage verifying that accept-encoding is stripped before reaching the upstream request.

@mahirhir
mahirhir requested a review from pi0 as a code owner June 30, 2026 05:44
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a3b51ca7-4b5c-4497-af41-ab4094539d4e

📥 Commits

Reviewing files that changed from the base of the PR and between f1155fc and 63449bb.

📒 Files selected for processing (2)
  • src/utils/internal/proxy.ts
  • test/proxy.test.ts

📝 Walkthrough

Walkthrough

Adds "accept-encoding" to the ignoredHeaders set in the proxy utility so it is not forwarded to upstream targets. A corresponding test verifies the header is absent in the upstream-observed request headers.

Changes

Ignore accept-encoding in proxy

Layer / File(s) Summary
Add accept-encoding to ignoredHeaders and test
src/utils/internal/proxy.ts, test/proxy.test.ts
"accept-encoding" is added to the ignoredHeaders set, and a test asserts the header is undefined in headers received by the proxied upstream.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested reviewers

  • pi0

Poem

🐇 A header once slipped through the gate,
encoding its way to some upstream fate.
But the rabbit said "No, you shall not pass!"
and added it firmly to the ignored class.
Now proxies hop clean, no encoding to see — 🎉

🚥 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: the proxy now ignores incoming accept-encoding headers.
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

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 pi0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: this review was performed by AI agents (Claude Code) on behalf of the maintainer, in two independent passes.

Pass 1 reviewed the diff for correctness, edge cases, and test coverage. Pass 2 (a fresh agent, adversarial by design) independently re-read the diff, checked for merge conflicts/CI status, and tried to find a reason to block approval before this review was posted. Both passes agree this change is correct, adequately tested, and safe to merge.

Caveat: this repo's real CI workflow (lint + build + vitest) requires maintainer approval to run on fork PRs and has not executed on this PR yet — only automated third-party checks (CodeRabbit, Socket Security) have run. Please trigger/approve the CI workflow before merging.

@pi0 pi0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@pi0
pi0 merged commit 14dd9b2 into h3js:main Jul 2, 2026
7 checks passed
pi0x pushed a commit to mixelburg/h3 that referenced this pull request Jul 2, 2026
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.

3 participants