fix(redirect): compare X-Forwarded-Proto case-insensitively - #13865
Open
nic-6443 wants to merge 1 commit into
Open
fix(redirect): compare X-Forwarded-Proto case-insensitively#13865nic-6443 wants to merge 1 commit into
nic-6443 wants to merge 1 commit into
Conversation
A URI scheme is case-insensitive per RFC 3986, Section 3.1, and RFC 7239 requires the `proto` parameter to conform to that definition. A proxy forwarding `X-Forwarded-Proto: HTTPS` is therefore stating exactly what one forwarding `https` states, but `http_to_https` compared the raw value against the lowercase literal and redirected the request anyway. Behind a TLS-terminating proxy that does not lowercase the value this becomes a redirect loop: the client follows the 301 back over HTTPS, the proxy forwards over HTTP again, and the plugin redirects again. Also document how the scheme is determined, including the fact that an inbound X-Forwarded-Proto is only preserved for peers in `apisix.trusted_addresses` -- the plugin docs did not mention the header at all.
dosubot
Bot
added
the
size:M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
redirect'shttp_to_httpscomparesX-Forwarded-Protoagainst the lowercase literalhttps, so a proxy that forwardsHTTPSgets redirected as if the request had arrived over plaintext HTTP.That comparison is wrong. A URI scheme is case-insensitive per RFC 3986, Section 3.1, and RFC 7239 requires the
protoparameter to "conform to the URI scheme name as defined in Section 3.1 in [RFC3986]". A proxy forwardingHTTPSis stating exactly what one forwardinghttpsstates.Behind a TLS-terminating proxy that does not lowercase the value, this turns into a redirect loop: the client follows the 301 back over HTTPS, the proxy terminates TLS and forwards over HTTP again, and the plugin redirects again. That is the same loop #6242 originally fixed, reintroduced for anyone whose proxy happens to send a different case.
The fix lowercases the value before the comparison.
core.request.header()already collapses a duplicated header down to its first value, so there is no table to guard against here.I also documented how the scheme is determined, including the fact that an inbound
X-Forwarded-Protois only preserved for peers listed inapisix.trusted_addresses— theredirectdocs did not mention the header at all, which makeshttp_to_httpsbehind a load balancer hard to reason about.Deliberately out of scope: a multi-valued
X-Forwarded-Protosuch ashttps, httpstill does not match.X-Forwarded-Protois not part of any specification (RFC 7239'sForwardedis the standardized form, and there the chain is expressed as multiple forwarded-elements, each with its ownproto), so there is no normative basis for deciding which element of a list is authoritative. That deserves its own discussion rather than riding along with a comparison fix.Which issue(s) this PR fixes:
N/A
Checklist