Skip to content

fix(request): parse first entry of comma-list x-forwarded-proto header#1413

Open
LeSingh1 wants to merge 2 commits into
h3js:mainfrom
LeSingh1:fix/x-forwarded-proto-comma-list
Open

fix(request): parse first entry of comma-list x-forwarded-proto header#1413
LeSingh1 wants to merge 2 commits into
h3js:mainfrom
LeSingh1:fix/x-forwarded-proto-comma-list

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Jun 12, 2026

Copy link
Copy Markdown

Proxies can stack x-forwarded-proto as a comma-separated list, for example "https,http" where the leftmost entry is the scheme the original client used. getRequestProtocol compared 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. toRequest had the same problem on the same header.

The fix applies the same split(",")[0].trim() pattern already used by getRequestHost and getRequestIP for their respective forwarded headers.

Two failing tests were added before the fix and confirmed broken: one in test/unit/request.test.ts that exercises getRequestProtocol directly with a fake event, and two in test/utils.test.ts under getRequestURL that 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 and oxfmt are clean.

Summary by CodeRabbit

  • Bug Fixes

    • Improved protocol detection for proxied requests: when proxy headers contain comma-separated values or extra whitespace the first entry is used, and an option to ignore proxy headers is respected—resulting in more accurate HTTP vs HTTPS determination.
  • Tests

    • Added tests covering comma-separated proxy header parsing, whitespace handling, and the disable-proxy-header option.

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.
@LeSingh1 LeSingh1 requested a review from pi0 as a code owner June 12, 2026 06:50
@coderabbitai

coderabbitai Bot commented Jun 12, 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: 2c88fb2c-c4cc-4efb-a3f0-40819a376151

📥 Commits

Reviewing files that changed from the base of the PR and between 400993d and df725b5.

📒 Files selected for processing (1)
  • src/utils/request.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/request.ts

📝 Walkthrough

Walkthrough

The PR changes protocol detection to parse comma-separated x-forwarded-proto headers by taking the first trimmed entry in toRequest() and getRequestProtocol(), and adds unit/integration tests covering comma-separated and whitespace-padded values and the disable option.

Changes

x-forwarded-proto comma-separated parsing

Layer / File(s) Summary
x-forwarded-proto parsing implementation
src/utils/request.ts
toRequest() and getRequestProtocol() now split x-forwarded-proto on commas, take the first trimmed value, and use it to determine scheme/protocol.
getRequestProtocol unit tests
test/unit/request.test.ts
Adds a makeEvent test helper and tests validating getRequestProtocol() for plain, comma-separated, and whitespace-padded x-forwarded-proto values, and the { xForwardedProto: false } option.
getRequestURL integration tests
test/utils.test.ts
Adds tests ensuring getRequestURL() uses the first comma-separated x-forwarded-proto entry and trims whitespace when constructing the URL.

🎯 3 (Moderate) | ⏱️ ~20 minutes


Suggested reviewers:

  • pi0

"I nibble headers in the night,
I split the commas, trim them right,
The first proto leads the way,
Through proxies, hop and sway,
Happy routes, in moonlit byte." 🐰✨

🚥 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 summarizes the main change: parsing the first entry of comma-separated x-forwarded-proto header values. It is specific, concise, and directly reflects the core fix across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

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

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

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

🧹 Nitpick comments (1)
src/utils/request.ts (1)

377-388: ⚡ Quick win

Update JSDoc to document comma-separated header handling.

The JSDoc comment states "If x-forwarded-proto header 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb018e and 400993d.

📒 Files selected for processing (3)
  • src/utils/request.ts
  • test/unit/request.test.ts
  • test/utils.test.ts

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.

1 participant