🐛 fix decode: enforce list_limit on comma-parsed values (qs v6.14.2 parity)#41
🐛 fix decode: enforce list_limit on comma-parsed values (qs v6.14.2 parity)#41
list_limit on comma-parsed values (qs v6.14.2 parity)#41Conversation
📝 WalkthroughWalkthroughThe changes add enforcement of list limits to comma-split values in query string decoding. When comma-splitting produces a list exceeding the limit, it either raises an error (if raise_on_limit_exceeded=True) or normalizes via Utils.combine(). New unit tests validate this behavior across various parameter combinations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
1 similar comment
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 1247 1251 +4
=========================================
+ Hits 1247 1251 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qs_codec/decode.py (1)
292-298: Duplicate raise path for comma-split values — consider removing the earlier one.When
raise_on_limit_exceeded=Trueand a comma-split in_parse_array_value(line 152) produces an oversized list, the error is raised there before this block is reached. This block's raise path (lines 294–297) therefore only fires for lists produced by a custom decoder (not by comma-splitting in_parse_array_value). That's correct and the tests confirm it.However, the redundancy between line 152 and line 294 for the comma-split + raise case means there are now two places defining the same error message for the same semantic check. If the message format ever changes, both sites must be updated in lockstep.
Consider consolidating: let
_parse_array_valuealways return the split list (drop lines 152–155) and let this single enforcement block handle both raise and overflow-conversion for all list sources uniformly. This would also simplify the_parse_array_valuedocstring.♻️ Sketch: consolidate the raise into the single enforcement block
In
_parse_array_value(lines 150–156), remove the raise guard so it always returns the split list:if isinstance(value, str) and value and options.comma and "," in value: split_val: t.List[str] = value.split(",") - if options.raise_on_limit_exceeded and len(split_val) > options.list_limit: - raise ValueError( - f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." - ) return split_valThe existing block at lines 292–298 already handles both raise and overflow-conversion for any list that exceeds
list_limit, so no other changes are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qs_codec/decode.py` around lines 292 - 298, Remove the duplicate raise in _parse_array_value so comma-split always returns the split list and let the single enforcement block (the if options.comma and isinstance(val, list) ... using options.list_limit, options.raise_on_limit_exceeded and Utils.combine) be the sole place that enforces list_limit and either raises or combines; specifically, delete the early raise in _parse_array_value and rely on the existing enforcement in the block that checks options.comma/len(val) against options.list_limit to handle both comma-split and custom-decoder lists uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/qs_codec/decode.py`:
- Around line 292-298: Remove the duplicate raise in _parse_array_value so
comma-split always returns the split list and let the single enforcement block
(the if options.comma and isinstance(val, list) ... using options.list_limit,
options.raise_on_limit_exceeded and Utils.combine) be the sole place that
enforces list_limit and either raises or combines; specifically, delete the
early raise in _parse_array_value and rely on the existing enforcement in the
block that checks options.comma/len(val) against options.list_limit to handle
both comma-split and custom-decoder lists uniformly.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR updates qs_codec.decode to match upstream qs v6.14.2 behavior by enforcing list_limit for comma-parsed values, converting over-limit comma-split arrays into overflow-object semantics (or raising when configured), and adds unit tests to lock in the parity behavior.
Changes:
- Enforce
list_limitwhencomma=Trueproduces a list value, raising or normalizing to overflow-object semantics. - Update decoder docstrings/comments to describe the new comma/list-limit handling.
- Add unit tests covering comma + list_limit within-limit, at-limit, overflow-normalization, and raise-on-overflow paths (plus a decoder-regression test).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/qs_codec/decode.py |
Adds comma-split list-limit enforcement/normalization during query tokenization. |
tests/unit/decode_test.py |
Adds parity/regression tests for comma + list_limit behaviors. |
| # If the pair used empty brackets syntax and list parsing is enabled, force an array container. | ||
| # Always wrap exactly once to preserve list-of-lists semantics when comma splitting applies. | ||
| if options.parse_lists and "[]=" in part: | ||
| val = [val] | ||
|
|
||
| # Enforce comma-split list limits consistently with other list construction paths. | ||
| if options.comma and isinstance(val, list) and len(val) > options.list_limit: | ||
| if options.raise_on_limit_exceeded: | ||
| raise ValueError( | ||
| f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." | ||
| ) | ||
| val = Utils.combine([], val, options) |
There was a problem hiding this comment.
The new comma list-limit enforcement runs after the []= wrapping (val = [val]). For inputs like a[]=1,2,3,4 with comma=True, list_limit=3, raise_on_limit_exceeded=False, the comma split creates an inner list of 4 items but wrapping turns it into an outer list of length 1, so this check won’t trigger and the over-limit comma-parsed list will still be returned (nested) without overflow normalization. To enforce list_limit on comma-parsed values consistently, apply the over-limit normalization/raise to the comma-split result before wrapping (or explicitly detect and handle the wrapped-list-of-list case).
| if options.comma and isinstance(val, list) and len(val) > options.list_limit: | ||
| if options.raise_on_limit_exceeded: | ||
| raise ValueError( | ||
| f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." | ||
| ) | ||
| val = Utils.combine([], val, options) |
There was a problem hiding this comment.
This enforcement only checks isinstance(val, list). Custom decoders (or other internal paths) can yield tuples as well (the rest of this function already treats (list, tuple) as list-like), which would bypass the comma list-limit enforcement. Consider checking isinstance(val, (list, tuple)) (and normalize tuples the same way) so oversized tuple values are handled consistently.
| if options.comma and isinstance(val, list) and len(val) > options.list_limit: | |
| if options.raise_on_limit_exceeded: | |
| raise ValueError( | |
| f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." | |
| ) | |
| val = Utils.combine([], val, options) | |
| if options.comma and isinstance(val, (list, tuple)): | |
| val_sequence = list(val) | |
| if len(val_sequence) > options.list_limit: | |
| if options.raise_on_limit_exceeded: | |
| raise ValueError( | |
| f"List limit exceeded: Only {options.list_limit} element{'' if options.list_limit == 1 else 's'} allowed in a list." | |
| ) | |
| val = Utils.combine([], val_sequence, options) |
| @pytest.mark.parametrize( | ||
| "query, options, expected, raises_error, expect_overflow", | ||
| [ | ||
| pytest.param( | ||
| "a=1,2,3", | ||
| DecodeOptions(comma=True, list_limit=5), | ||
| {"a": ["1", "2", "3"]}, | ||
| False, | ||
| False, | ||
| id="comma-within-list-limit", | ||
| ), | ||
| pytest.param( | ||
| "a=1,2,3,4", | ||
| DecodeOptions(comma=True, list_limit=3), | ||
| {"a": {"0": "1", "1": "2", "2": "3", "3": "4"}}, | ||
| False, | ||
| True, | ||
| id="comma-over-list-limit-converts-to-overflow", | ||
| ), | ||
| pytest.param( | ||
| "a=1,2,3,4", | ||
| DecodeOptions(comma=True, list_limit=3, raise_on_limit_exceeded=True), | ||
| None, | ||
| True, | ||
| False, | ||
| id="comma-over-list-limit-raises", | ||
| ), | ||
| pytest.param( | ||
| "a=1,2,3", | ||
| DecodeOptions(comma=True, list_limit=3), | ||
| {"a": ["1", "2", "3"]}, | ||
| False, | ||
| False, | ||
| id="comma-at-list-limit-stays-list", | ||
| ), |
There was a problem hiding this comment.
The new comma/list_limit parity tests don’t cover the []-wrapping case (e.g. a[]=1,2,3,4 with comma=True, list_limit=3, raise_on_limit_exceeded=False). That path is important because the tokenizer wraps the comma-split list (val = [val]), which can mask an over-limit inner list unless explicitly enforced. Adding this case would prevent regressions and ensure the intended CVE/parity fix applies when comma-splitting is combined with empty-bracket list markers.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62357cb311
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if options.parse_lists and "[]=" in part: | ||
| val = [val] | ||
|
|
||
| # Enforce comma-split list limits consistently with other list construction paths. | ||
| if options.comma and isinstance(val, list) and len(val) > options.list_limit: |
There was a problem hiding this comment.
With inputs like a[]=1,2,3,4 and DecodeOptions(comma=True, list_limit=3, raise_on_limit_exceeded=False), the overflow check is bypassed because val is wrapped to [val] before the new limit guard runs, making len(val) equal to 1. This means over-limit comma-parsed payloads that use []= still return oversized inner lists instead of being normalized to overflow-object semantics, so the list-limit enforcement added here is incomplete for a common query form.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR aligns
qs_codec.decodewith upstreamqsbehavior for comma-parsed values and list limits (GHSA-w7fw-mjwx-w883 / CVE-2026-2391 parity).Previously, when
comma=Trueandlist_limitwas exceeded, the parser could still return a Python list whenraise_on_limit_exceeded=False.Now, over-limit comma-split values are normalized to overflow-object semantics (numeric-key dict/
OverflowDict) instead of remaining oversized lists.Motivation
qsfixed this class of issue inv6.14.2by enforcingarrayLimiton comma-parsed values.As this project is a port of
qs, this change keeps behavior and safety expectations in sync.Changes
src/qs_codec/decode.pyAdded comma list-limit enforcement in
_parse_query_string_valuesafter value decoding and[]wrapping.If
options.commaand decodedvalis a list andlen(val) > options.list_limit:raise_on_limit_exceeded=True-> raiseValueError(existing message format).raise_on_limit_exceeded=False-> normalize withUtils.combine([], val, options)to produce overflow-object semantics.Updated related docstrings/comments to reflect the new normalization behavior.
tests/unit/decode_test.pyAdded parameterized parity tests for
comma + list_limit:within limit stays list
over limit converts to overflow object
over limit with
raise_on_limit_exceeded=Trueraisesexactly at limit stays list
Added targeted regression test to hit the decoder-produced oversized list raise path (restores 100% coverage).
Behavioral example
a=1,2,3,4DecodeOptions(comma=True, list_limit=3, raise_on_limit_exceeded=False){"a": ["1", "2", "3", "4"]}{"a": {"0": "1", "1": "2", "2": "3", "3": "4"}}(OverflowDictsemantics)Validation
pytest -q tests/unit/decode_test.py -k "comma and list_limit"pytest -q tests/unit/decode_test.pypytest -qpytest -q --cov=src/qs_codec --cov-report=term-missingResults:
926 passedTOTAL ... 100%coverageCompatibility
References
Summary by CodeRabbit
Bug Fixes
Tests