fix(everything): re-check GZIP_ALLOWED_DOMAINS on redirect hops - #4622
Open
SashaMIT wants to merge 2 commits into
Open
fix(everything): re-check GZIP_ALLOWED_DOMAINS on redirect hops#4622SashaMIT wants to merge 2 commits into
SashaMIT wants to merge 2 commits into
Conversation
The gzip-file-as-resource allowlist only validated the initial URL, while fetch followed redirects by default, so an allowed host could redirect to a host outside GZIP_ALLOWED_DOMAINS. Follow redirects manually and apply the same protocol/domain checks on each hop.
This comment was marked as spam.
This comment was marked as spam.
Per review: exercise chains where each hop re-validation must hold even though the destination's textual form changes: hostname to IPv4/IPv6 literal, permitted-to-non-permitted chains, relative Location then absolute cross-origin, and userinfo authority tricks. Positive control: relative redirect staying on the allowed host still succeeds. Each scenario asserts the forbidden destination is never fetched. Fails when redirect mode is reverted to follow.
Author
|
Great callouts, thank you. Added coverage in 9791fdb for the representation-change cases: hostname to IPv4 and IPv6 literals, permitted-to-non-permitted chains, relative Location then absolute cross-origin, and the userinfo authority trick. Each scenario asserts the non-allowed destination is never fetched, and there is a positive control that a relative redirect staying on the allowed host still succeeds. On DNS resolution changing between validation and connection: agreed, that is egress-layer territory. URL-layer validation alone is not complete SSRF containment there; I have noted it as out of scope in the PR description. |
This comment was marked as spam.
This comment was marked as spam.
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
2 participants
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
gzip-file-as-resourcevalidatedGZIP_ALLOWED_DOMAINSonly against the initial URL, then calledfetchwith the default redirect policy (follow). An allowlisted host could respond with a3xxto a host outside the allowlist, and the tool would still download and gzip that body.This change follows redirects manually (
redirect: "manual"), re-applies the same protocol and domain checks on every hop, and caps redirect depth.Server Details
gzip-file-as-resource)Motivation and Context
GZIP_ALLOWED_DOMAINSis documented as the domain allowlist for this tool. Without hop re-checks, the control does not hold whenever an allowed origin redirects.Local confirmation (pre-fix): with allowlist
127.0.0.1, an origin on127.0.0.1that302s tohttp://[::1]:.../secretreturned the IPv6 target body under defaultfetchredirect following. Post-fix, the same setup rejects[::1]before the second request.How Has This Been Tested?
302from an allowed host to127.0.0.1and asserts rejection + that the redirect target is never fetchednpm testinsrc/everything(108 tests) greenBreaking Changes
None for the default (empty allowlist). Operators with
GZIP_ALLOWED_DOMAINSset get the documented allowlist behavior on redirect chains as well as on the first URL.Types of changes
Checklist
Additional context
Framed as a product bug in the existing allowlist control (reference
everythingserver).SECURITY.mdnotes these servers are reference implementations and not a security-reporting surface; happy to adjust if maintainers prefer a different lane.Made with Cursor