test(providers): pin EG-5 and EG-6 on the redirect hop (trimmed from #213) - #215
Merged
Merged
Conversation
Closes #206. First slice of #118. Three PCP-1 MUST assertions were implemented in `src/providers/egress.ts` but never asserted by a test, so deleting any of the guards would have landed green. No production code changes. - EG-4: the redirect chain is bounded. The test asserts both the error and that the request count stays finite -- without the second half, an unbounded loop that happened to error for another reason would pass. - EG-5: a non-http(s) scheme is rejected. - EG-6: a URL carrying embedded credentials is rejected. EG-5 and EG-6 get two tests each: one on the configured base URL and one on a redirect target. The redirect case is the one that matters, since a `Location` header is attacker-influenced in a way a configured base URL is not, and a guard applied only at entry would pass the first test. Every test asserts the mock transport was not called for the rejected hop, so the rejection is proven to happen before any bytes are sent rather than merely being reported afterwards. Fully offline. Bypass-proof, verified by hand: disabling each guard in turn turns exactly its own tests red -- EG-4 one, EG-5 two, EG-6 two -- and nothing else.
#211 covered the same issue and merged first, so three of the five tests here now duplicate what is on main: the EG-4 bound, and the base-URL cases for EG-5 and EG-6. Removed, leaving the two this PR alone contributes. Those two are not redundant with anything. `assertProviderUrlAllowed` runs on every hop of the redirect loop, and the redirect hop is the one a `Location` header can steer. Downgrading that call to first-hop-only leaves every base-URL test on main green and turns exactly these two red, along with the pre-existing metadata-redirect test — which is the whole argument for keeping them, verified rather than assumed. The header comment was written for five tests across three assertions; retargeted at the two that remain. Trim only. The tests, their reasoning, and the per-hop insight are dchaudhari7177's work, kept unmodified in the commit below this one. Co-Authored-By: dchaudhari7177 <111210939+dchaudhari7177@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW
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
3 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.
Summary
The tests in this PR are @dchaudhari7177's work, from #213. Their commit
207f837is preserved unmodified in this branch's history; the only commit of mine removes what #211 had already landed.#213 and #211 both answered #206. #211 merged first — my error, I checked the open-PR list before #213 existed and did not re-check before merging. #213 was the stronger submission, and this is the part of it that #211 does not cover.
I could not push the trim to #213's branch (the fork is outside this session's authorized repositories), so it lands here instead. #213 is being closed with a pointer to this PR.
What survives the trim
Two tests, both covering the redirect hop:
EG-5: rejects a non-http(s) scheme reached through a redirect—Location: file:///etc/passwdEG-6: rejects embedded credentials reached through a redirect—Location: https://user:pass@…Removed as duplicates of what #211 landed: the EG-4 bound test and the base-URL cases for EG-5 and EG-6.
Why these two are not redundant
@dchaudhari7177's argument, quoted from #213: "
assertProviderUrlAllowedis called on every hop, and the redirect hop is the one that matters — aLocationheader is attacker-influenced in a way a configured base URL is not."That is correct, and I verified it rather than taking it on reasoning alone. Simulating the regression a refactor would plausibly introduce — validating only the first hop:
exactly three tests fail:
Every base-URL test, #211's included, stays green. A change hoisting that check out of the loop or moving it to construction time would land clean on
mainas it stands and be caught only by these two.Verification
main+ 2).tsc --noEmitclean.fetch, no real network call (PCP-1 T-1).Note
#211's EG-4 test, now on
main, assertstoHaveBeenCalledTimes(6). #213's version usedtoBeLessThanOrEqual(6), which does not need editing ifMAX_REDIRECTSchanges. Not changed here — out of scope for a trim — but worth doing when that area is next touched.Related: #214, the AU-1 audit gap on the redirect-bound path, found while reviewing these.
🤖 Generated with Claude Code
https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW
Generated by Claude Code