nic-6443 opened a new pull request, #13865:
URL: https://github.com/apache/apisix/pull/13865

   ### Description
   
   `redirect`'s `http_to_https` compares `X-Forwarded-Proto` against the 
lowercase literal `https`, so a proxy that forwards `HTTPS` gets 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](https://datatracker.ietf.org/doc/html/rfc3986#section-3.1), and 
RFC 7239 requires the `proto` parameter to "conform to the URI scheme name as 
defined in Section 3.1 in [RFC3986]". A proxy forwarding `HTTPS` is stating 
exactly what one forwarding `https` states.
   
   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-Proto` is only preserved for peers listed in 
`apisix.trusted_addresses` — the `redirect` docs did not mention the header at 
all, which makes `http_to_https` behind a load balancer hard to reason about.
   
   Deliberately out of scope: a multi-valued `X-Forwarded-Proto` such as 
`https, http` still does not match. `X-Forwarded-Proto` is not part of any 
specification (RFC 7239's `Forwarded` is the standardized form, and there the 
chain is expressed as multiple forwarded-elements, each with its own `proto`), 
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
   
   - [x] I have explained the need for this PR and the problem it solves
   - [x] I have explained the changes or the new features added to this PR
   - [x] I have added tests corresponding to this change
   - [x] I have updated the documentation to reflect this change
   - [x] I have verified that this change is backward compatible (If not, 
please discuss on the [APISIX mailing 
list](https://github.com/apache/apisix/tree/master#community) first)
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to