Fix server_scheme leak, HTTPS value validation and X-Forwarded-Host list - #6
Merged
Merged
Conversation
Three further fixes found while reviewing the proxy-header handling: - RPAFsethttps set r->server->server_scheme to "https" but never reset it, so once any https-forwarded request was seen the shared server_rec stayed "https" and later plain-HTTP requests generated https self-referential URLs (e.g. mod_dir redirects). The scheme is now reset to the original per request -- the same class of shared-state leak already fixed for setport. - RPAFsethttps trusted the X-Forwarded-HTTPS / X-HTTPS value literally, so a value like "off" still flagged the request as https. The value is now validated (on/1/true/yes/https). - RPAFsethostname now uses the last entry of a comma-separated X-Forwarded-Host instead of the raw header value. CI gains regression cases for all three (the server_scheme test pins the server to a single child process so the leak is observable via the /dir redirect scheme). README and CHANGES updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Three further bug fixes found while reviewing the proxy-header handling (follow-up to #5). No configuration changes are required.
Fixes
server_scheme leaks across requests (same class as the old setport bug)
RPAFsethttpssetr->server->server_schemetohttpson an https-forwarded request but never reset it. Becauseserver_recis shared, once any https request was seen the server stayedhttps, and later plain-HTTP requests producedhttps://self-referential URLs (e.g.mod_dirtrailing-slash redirects). The scheme is now reset to the original value per request —cfg->orig_schemewas already captured for this but unused.Forwarded HTTPS value trusted literally
X-Forwarded-HTTPS/X-HTTPSwere used as-is, so a value likeoffstill flagged the request as HTTPS. The value is now validated (on/1/true/yes/https, case-insensitive);X-Forwarded-Protois matched case-insensitively too.Comma-separated X-Forwarded-Host
RPAFsethostnameused the raw header value. A list such asa.example, b.examplenow resolves to the last entry (b.example), the one added by the nearest proxy.Testing
The CI suite (Apache 2.2 / CentOS 6) gains three regression cases, all passing:
/dirredirect stayshttp://(no scheme leak)X-Forwarded-HTTPS: offRewriteCond %{HTTPS}isoffX-Forwarded-Host: first.example, second.examplesecond.exampleThe server_scheme test pins Apache to a single child process (
ServerLimit 1/MaxClients 1) so the shared-state leak is observable. README and CHANGES are updated.🤖 Generated with Claude Code