fix(request): parse first entry of comma-list x-forwarded-proto header#1413
Conversation
Proxies can stack x-forwarded-proto as a comma-separated list
(e.g. "https,http"). getRequestProtocol compared the raw header
value with strict equality, so any comma-list value fell through
and the underlying request URL scheme was used instead of the
client-facing protocol. toRequest had the same issue.
Apply the same split(",")[0].trim() pattern already used by
getRequestHost and getRequestIP for their respective headers.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates request protocol detection to parse comma-separated Changesx-forwarded-proto comma-separated parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/request.ts (1)
377-388: ⚡ Quick winUpdate JSDoc to document comma-separated header handling.
The JSDoc comment states "If
x-forwarded-protoheader is set to 'https'", but with the new implementation (lines 394-395), comma-separated values are now supported and the first entry is used. Consider updating the documentation to clarify this behavior for API consumers.Example:
* Get the request protocol. * - * If `x-forwarded-proto` header is set to "https", it will return "https". You can disable this behavior by setting `xForwardedProto` to `false`. + * If the first entry of the `x-forwarded-proto` header (comma-separated list supported) is "https", it will return "https". You can disable this behavior by setting `xForwardedProto` to `false`. *🤖 Prompt for 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. In `@src/utils/request.ts` around lines 377 - 388, Update the JSDoc for getRequestProtocol to document that the function supports comma-separated x-forwarded-proto values: when the header contains multiple comma-separated protocols the function will use the first entry (trimmed) and compare it to "https"; also note that this behavior can be disabled via the xForwardedProto parameter and that the fallback remains "http" when protocol cannot be determined. Include a short example showing a comma-separated header and that the first value is used.
🤖 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.
Nitpick comments:
In `@src/utils/request.ts`:
- Around line 377-388: Update the JSDoc for getRequestProtocol to document that
the function supports comma-separated x-forwarded-proto values: when the header
contains multiple comma-separated protocols the function will use the first
entry (trimmed) and compare it to "https"; also note that this behavior can be
disabled via the xForwardedProto parameter and that the fallback remains "http"
when protocol cannot be determined. Include a short example showing a
comma-separated header and that the first value is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4403885f-6b47-41a5-b678-206f6d063250
📒 Files selected for processing (3)
src/utils/request.tstest/unit/request.test.tstest/utils.test.ts
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3 participants
Proxies can stack
x-forwarded-protoas a comma-separated list, for example"https,http"where the leftmost entry is the scheme the original client used.getRequestProtocolcompared the raw header value with strict equality, so any comma-list value fell through and the function returned the scheme from the underlying request URL instead of the client-facing protocol.toRequesthad the same problem on the same header.The fix applies the same
split(",")[0].trim()pattern already used bygetRequestHostandgetRequestIPfor their respective forwarded headers.Two failing tests were added before the fix and confirmed broken: one in
test/unit/request.test.tsthat exercisesgetRequestProtocoldirectly with a fake event, and two intest/utils.test.tsundergetRequestURLthat test the comma-list and comma-with-space cases end-to-end through a live app handler. All 48 test files and 1159 tests pass after the fix. Lint andoxfmtare clean.Summary by CodeRabbit
x-forwarded-protowhen it contains a comma-separated list (using the first value with whitespace trimmed), while respecting the option to ignore proxy headers.x-forwarded-protoparsing, including comma-separated values, whitespace handling, and the disable-proxy-header option.x-forwarded-protobehavior and thexForwardedProto: falseoption.