Skip to content

fix(proxy): strip hop-by-hop request headers (RFC 7230) - #804

Merged
harlan-zw merged 4 commits into
mainfrom
fix/proxy-hop-by-hop-headers-791
May 27, 2026
Merged

fix(proxy): strip hop-by-hop request headers (RFC 7230)#804
harlan-zw merged 4 commits into
mainfrom
fix/proxy-hop-by-hop-headers-791

Conversation

@harlan-zw

@harlan-zwharlan-zw commented May 27, 2026

Copy link
Copy Markdown
Collaborator

🔗 Linked issue

Resolves#791

❓ Type of change

  • 📖 Documentation
  • 🐞 Bug fix
  • 👌 Enhancement
  • ✨ New feature
  • 🧹 Chore
  • ⚠️ Breaking change

📚 Description

The script proxy handler forwarded incoming request headers verbatim, including hop-by-hop headers that RFC 7230 §6.1 says must not be forwarded by intermediaries (connection, keep-alive, proxy-authenticate, proxy-authorization, te, trailer, transfer-encoding, upgrade), nor any headers named in the Connection header value. Mirrors the existing SKIP_RESPONSE_HEADERS filter on the response side. New unit test boots the actual proxy handler and asserts both static and Connection-named headers are stripped.

Per RFC 7230 §6.1, a proxy must not forward connection-specific
hop-by-hop headers upstream. The handler already filters them on
the response side via SKIP_RESPONSE_HEADERS; mirror that with a
SKIP_REQUEST_HEADERS set covering connection, keep-alive,
proxy-authenticate, proxy-authorization, te, trailer,
transfer-encoding, upgrade. Also strip any header listed by name
in the incoming Connection header value.
Forwarding these can corrupt the upstream exchange (mis-framed
bodies from a stale transfer-encoding, broken keep-alive
negotiation) and has been observed to break Sentry proxying and
native webview signals.
Closes#791
@vercel

vercelBot commented May 27, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
scripts-playgroundReadyReadyPreview, CommentMay 27, 2026 7:22am

@coderabbitai

coderabbitaiBot commented May 27, 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: 2724ee84-4e24-445d-befd-f2fcf25e2676

📥 Commits

Reviewing files that changed from the base of the PR and between a609570 and 1f927fd.

📒 Files selected for processing (1)
  • test/unit/proxy-handler-hop-by-hop.test.ts
✅ Files skipped from review due to trivial changes (1)
  • test/unit/proxy-handler-hop-by-hop.test.ts

📝 Walkthrough

Walkthrough

This PR adds RFC 7230 hop-by-hop request header filtering to the proxy handler. It introduces a SKIP_REQUEST_HEADERS set containing the standard hop-by-hop headers (connection, keep-alive, proxy-authenticate, proxy-authorization, te, trailer, transfer-encoding, upgrade) and applies this filter during request forwarding. The implementation also parses the incoming Connection header to identify connection-declared custom headers and filters those as well. A Vitest unit test validates that the constant contains all required headers and confirms hop-by-hop headers are stripped while non-hop-by-hop headers pass through.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(proxy): strip hop-by-hop request headers (RFC 7230)' accurately and concisely describes the main change—adding RFC 7230-compliant header filtering to the proxy handler.
Description check✅ PassedThe description clearly explains the bug fix, references the linked issue #791, specifies the hop-by-hop headers being filtered, mentions the test coverage, and notes the RFC 7230 compliance requirement.
Linked Issues check✅ PassedThe PR fully addresses all coding requirements from issue #791: defines SKIP_REQUEST_HEADERS with the exact hop-by-hop header set, strips Connection-named headers, mirrors SKIP_RESPONSE_HEADERS, and includes comprehensive unit tests.
Out of Scope Changes check✅ PassedAll changes are directly scoped to fixing the proxy hop-by-hop header filtering issue; no unrelated modifications are present in proxy-handler.ts or the new test file.
Docstring Coverage✅ PassedDocstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/proxy-hop-by-hop-headers-791

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 and usage tips.

@pkg-pr-new

pkg-pr-newBot commented May 27, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@nuxt/scripts@804

commit: 1f927fd

@coderabbitaicoderabbitaiBot 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.

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/unit/proxy-handler-hop-by-hop.test.ts`:
- Around line 52-64: The test overrides globalThis.fetch (see the realFetch
assignment and the async replacement function) but never restores it, which can
leak into other tests; add a teardown that restores globalThis.fetch to
realFetch in an afterAll/afterEach block (matching the test lifecycle used) so
that the original fetch is reinstated after the tests complete, and ensure any
other similar overrides around lines 80-83 are also restored the same way.
🪄 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: 233d3a8e-6fa6-4fc1-81d6-048a4318a042

📥 Commits

Reviewing files that changed from the base of the PR and between 4ed9c62 and a609570.

📒 Files selected for processing (2)
  • packages/script/src/runtime/server/proxy-handler.ts
  • test/unit/proxy-handler-hop-by-hop.test.ts

Comment threadtest/unit/proxy-handler-hop-by-hop.test.ts Outdated
@harlan-zw
harlan-zw merged commit da57b6e into mainMay 27, 2026
22 of 23 checks passed
@harlan-zw
harlan-zw deleted the fix/proxy-hop-by-hop-headers-791 branch May 27, 2026 07:49
Sign up for freeto 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.

Proxy forwards hop-by-hop request headers to upstream (RFC 7230 §6.1)

1 participant

@harlan-zw