fix: URLAllowList inspects URLs nested inside containers - #30
Merged
Conversation
0.3.2 taught URLAllowList to skip non-string values so it could coexist on a
multi-argument tool like http_post(url, body). It never taught the rule to look
*inside* those values, so a URL hidden one level down passed unchecked:
http_post(url="https://api.internal.com/v1",
payload={"redirect": "http://evil.com"}) -> executed
A redirect or callback field is as much an exfiltration route as the url
parameter itself, and SensitiveDataFilter already recursed into containers, so
the two shipped rules disagreed about how deep to look.
URLAllowList now walks dict, list, tuple, set and frozenset, checking every
string it reaches. Dict keys are checked as well as values, since an endpoint
map can carry a URL as a key. A seen-set guards against self-referential
containers, which a hand-built argument can contain even though a JSON-derived
one cannot.
Verified the 0.3.2 behaviour is preserved: a legitimate call with a prose body,
an int timeout, a benign dict, or an allowlisted nested URL all still pass;
evil.com blocks whether it appears at the top level, in a dict value, in a dict
key, or in a nested list; file:// still blocks inside a container.
Tests 60 -> 71.
Owner
Author
|
Added two Covers local scratch scripts and notes so they can't be swept into a commit. The sdist was already protected by the explicit allowlist added in 0.3.2, so this is about accidental Still 71 tests passing. |
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.
Version →
0.3.4.The gap
0.3.2 taught
URLAllowListto returnNonefor non-string values, so it could coexist on a multi-argument tool likehttp_post(url, body)without flaggingbody. That fix was right, but incomplete: it taught the rule to skip non-strings without teaching it to look inside them.A
redirectorcallbackfield is as much an exfiltration route as theurlparameter. AndSensitiveDataFilteralready recursed into containers, so the two shipped rules disagreed about how deep to look.This was a regression I introduced in 0.3.2. For the record, comparing versions on the same input:
0.3.1 blocked the nested case only because
urlparse()raised on a dict and its bareexcept ExceptionreturnedInvalid URL— it was blocking everything non-string, which is why it also blocked legitimate calls. It looked secure because it was uniformly broken.The change
URLAllowListnow walksdict,list,tuple,setandfrozenset, applying the existing URL check to every string it reaches. The per-URL logic is unchanged — it moved into_check_urluntouched.Two decisions worth flagging, both slightly beyond "recurse into containers":
URLAllowListis not subject to the dict-key blind spot thatSensitiveDataFilterstill has (tracked in the local audit backlog).payload={"http://evil.com"}is the same gap, and leaving it out would have meant filing a follow-up issue immediately.A
seen-set of container ids guards against self-referential arguments, so a cyclic payload can't hang the check.Verification
71 tests pass (was 60),
rules.pyat 97%. New tests cover the dict value/key cases, arbitrary nesting depth, a parametrized sweep over all five container types, cyclic containers, and — importantly — that an allowlisted URL inside a container is not turned into a violation, plus that a benign structured payload still passes. The existing multi-argument integration test was extended rather than replaced.Note
README.md's Limitations section says "onlystr,list,tuple, anddictvalues are inspected". That remains accurate forSensitiveDataFilterbut now understatesURLAllowList, which also reaches sets and dict keys. Understating protection is the safe direction so I've left it, but it's worth a wording pass next time the README is touched.