Skip to content

Reject repeated keys in strict urlencoded_params_matcher - #828

Open
Atishyy27 wants to merge 1 commit into
getsentry:masterfrom
Atishyy27:fix-urlencoded-strict-repeated-keys
Open

Atishyy27 wants to merge 1 commit into
getsentry:masterfrom
Atishyy27:fix-urlencoded-strict-repeated-keys

Conversation

@Atishyy27

Copy link
Copy Markdown

urlencoded_params_matcher parses the body with dict(parse_qsl(...)), which collapses repeated keys (last value wins). Under the default strict_match=True a body carrying a duplicate key then compares equal to a params dict that cannot represent it:

>>> from urllib.parse import parse_qsl
>>> dict(parse_qsl("a=1&a=2"))
{'a': '2'}          # a=1 is gone, so this equals the strict spec {"a": "2"}

So a strict matcher for {"a": "2"} wrongly matches a body of a=1&a=2 (for example data=[("a", "1"), ("a", "2")]), even though it sent an a=1 the spec never allowed. Same lossy-parse_qsl family as #804; query_string_matcher avoids it by comparing the parsed pair lists rather than dicts.

Fix: in the strict path, reject the body when the parsed pairs outnumber the collapsed dict. The non-strict path (meant to allow extra params) is untouched. Added test_urlencoded_params_matcher_strict_repeated_keys (fails on master, passes here) and a CHANGES entry. Prepared with AI assistance under my direction; I reviewed and tested it myself.

dict(parse_qsl(...)) collapses repeated keys, so a strict match accepted a
request body carrying data the params dict cannot represent (e.g. a body
"a=1&a=2" against {"a": "2"}). Fail the strict match when the parsed
pairs outnumber the collapsed dict, and add a regression test.
@Atishyy27

Copy link
Copy Markdown
Author

All six workflow runs on this branch sit at conclusion: action_required, so no CI has run and there is nothing for a reviewer to look at yet. Could someone approve the run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant