Skip to content

fix: parse comma-separated X-Forwarded-Proto in Scheme#3055

Closed
sonnemusk wants to merge 1 commit into
labstack:masterfrom
sonnemusk:fix/scheme-multi-value-forwarded-proto
Closed

fix: parse comma-separated X-Forwarded-Proto in Scheme#3055
sonnemusk wants to merge 1 commit into
labstack:masterfrom
sonnemusk:fix/scheme-multi-value-forwarded-proto

Conversation

@sonnemusk

Copy link
Copy Markdown
Contributor

Summary

Context.Scheme() treated the entire X-Forwarded-Proto header as one token. Multi-proxy values like https, http failed validation and were ignored.

Change

  • Introduce firstValidProto to take the left-most valid scheme token.
  • Apply to X-Forwarded-Proto, X-Forwarded-Protocol, and X-Url-Scheme.
  • Add unit tests for comma-separated headers.

Test plan

  • go test . -run TestContext_Scheme

Proxies may send multi-value X-Forwarded-Proto (e.g. "https, http").
The whole string failed isValidProto, so Scheme fell through incorrectly.
Use the left-most valid token for X-Forwarded-Proto/Protocol and X-Url-Scheme.
@aldas

aldas commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

in which case that header has comma separated values? When I read https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Forwarded-Proto it only seems to refer single value

@sonnemusk

Copy link
Copy Markdown
Contributor Author

Agreed that MDN describes a single token. Multi-value X-Forwarded-Proto is non-spec but does show up in the wild when:

  1. Proxy chains append — some L7 hops (certain nginx/ingress/service-mesh configs) append instead of replace, producing values like https, http.
  2. Multiple header linesHeader.Get joins with ", " for multi-value headers in Go, so two X-Forwarded-Proto lines become a comma-separated string even if each was single-valued.
  3. Other frameworks already defensive-parse — e.g. similar left-most-token handling exists in common Go HTTP stacks for forwarded headers.

If you’d rather not support the multi-value case, I can close this and leave Scheme as strict single-token. The multi-value path was meant as defensive parsing, not a claim that MDN documents commas.

@sonnemusk
sonnemusk force-pushed the fix/scheme-multi-value-forwarded-proto branch from 1305e35 to 4f38bea Compare July 22, 2026 07:56
@aldas

aldas commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

after asking this from LLM it seems that there are few things that do that. but when we start to choosing one from list - which one to trust? When proxies append these values - which proxy in that chain was fist to be trusted and which ones before that are/can be probably spoofed?

@sonnemusk are you having actual problem in production - is there Proxy application that causes this problem?

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.35%. Comparing base (dcb05f0) to head (4f38bea).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3055   +/-   ##
=======================================
  Coverage   93.34%   93.35%           
=======================================
  Files          43       43           
  Lines        4735     4741    +6     
=======================================
+ Hits         4420     4426    +6     
  Misses        192      192           
  Partials      123      123           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sonnemusk

Copy link
Copy Markdown
Contributor Author

No production issue — I was just being defensive about multi-value headers.

On the trust question: you're right that picking left vs right isn't well-defined without knowing the proxy chain, so baking a choice into Scheme() is the wrong place.

Closing this. Happy to revisit if someone hits a concrete proxy that appends and needs documented behavior.

@sonnemusk sonnemusk closed this Jul 23, 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.

2 participants