🐛 enhance comma-split behavior to enforce listLimit and handle overflow#48
🐛 enhance comma-split behavior to enforce listLimit and handle overflow#48
listLimit and handle overflow#48Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughReworks comma-split handling during query-string decoding: comma-splits are returned in full during decoding and comma-split overflow enforcement is applied later (throw or convert to an overflow map) based on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 97.27% 97.31% +0.04%
==========================================
Files 16 16
Lines 1136 1153 +17
==========================================
+ Hits 1105 1122 +17
Misses 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: fcf3c20189
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/extensions/decode.dart`:
- Around line 63-66: The branch computing `remaining` is dead because it's
entered only when `options.listLimit < 0` and `currentListLength >= 0`, so
`remaining = options.listLimit - currentListLength` is always ≤ 0; remove the
intermediate `remaining` variable and the redundant `if (remaining <= 0)` check
and simply return an empty list directly when `options.listLimit < 0` (replace
the whole block that references `remaining` with `return const <String>[];`),
keeping references to `options.listLimit` and `currentListLength` for context.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart`: - Around line 63-66: The branch computing `remaining` is dead because it's entered only when `options.listLimit < 0` and `currentListLength >= 0`, so `remaining = options.listLimit - currentListLength` is always ≤ 0; remove the intermediate `remaining` variable and the redundant `if (remaining <= 0)` check and simply return an empty list directly when `options.listLimit < 0` (replace the whole block that references `remaining` with `return const <String>[];`), keeping references to `options.listLimit` and `currentListLength` for context.lib/src/extensions/decode.dart (1)
63-66: Dead computation:remainingis always ≤ 0 whenlistLimit < 0.Since this block is only entered when
options.listLimit < 0, andcurrentListLength >= 0, the expressionoptions.listLimit - currentListLengthis always negative. Theif (remaining <= 0)check is always true, making the intermediate variable unnecessary.♻️ Simplify to a direct return
if (options.listLimit < 0) { - final int remaining = options.listLimit - currentListLength; - if (remaining <= 0) return const <String>[]; + return const <String>[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart` around lines 63 - 66, The branch computing `remaining` is dead because it's entered only when `options.listLimit < 0` and `currentListLength >= 0`, so `remaining = options.listLimit - currentListLength` is always ≤ 0; remove the intermediate `remaining` variable and the redundant `if (remaining <= 0)` check and simply return an empty list directly when `options.listLimit < 0` (replace the whole block that references `remaining` with `return const <String>[];`), keeping references to `options.listLimit` and `currentListLength` for context.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the comma-split list limit enforcement mechanism to match Node.js qs behavior more accurately. The main change moves overflow enforcement from _parseListValue to _parseQueryStringValues, improving separation of concerns and ensuring correct handling of list limits in comma-separated values.
Changes:
- Refactored
_parseListValueto return full comma-split results for non-negativelistLimit, deferring overflow enforcement to_parseQueryStringValues - Added overflow enforcement logic in
_parseQueryStringValuesthat either throws in strict mode or converts to an overflow map - Added comprehensive tests for comma-split list limit behavior, including edge cases and GHSA security payloads
- Updated
qsdependency from 6.14.1 to 6.14.2 for compatibility with upstream
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/src/extensions/decode.dart | Refactored _parseListValue to return full split for non-negative limits; added overflow enforcement in _parseQueryStringValues for comma-splits |
| test/unit/decode_test.dart | Updated and added tests for comma-split list limits, overflow conversion, strict mode errors, and GHSA payloads |
| test/comparison/package.json | Updated qs dependency from 6.14.1 to 6.14.2 |
|
@codex review |
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/extensions/decode.dart`:
- Around line 193-207: The RangeError message in the comma-split overflow branch
is duplicated; replace the hard-coded string used when
options.throwOnLimitExceeded is true with the existing helper
_listLimitExceededMessage so all three occurrences (this block handling
options.comma, the branch using Utils.combine, and the _parseListValue function)
reuse the same message generator; update the throwing site (the if
(options.throwOnLimitExceeded) { throw RangeError(...); } in the comma overflow
block) to call _listLimitExceededMessage(options.listLimit) (or the helper's
exact name/signature) instead of inlining the string, ensuring consistency
across _parseListValue and other places.
- Around line 239-255: The _listLikeCount function currently computes overflow
map size by scanning keys and returning maxIndex + 1 which over-counts sparse
overflow maps; change _listLikeCount (and its overflow branch that checks
Utils.isOverflow) to return the actual entry count (value.length) instead of
maxIndex + 1, and add a short comment noting that if Utils.combine guarantees
dense sequential keys this could be an alternative behavior; ensure references
to Utils.isOverflow remain and keep behavior for Iterable/other types unchanged.
In `@test/unit/decode_test.dart`:
- Around line 3047-3077: Add a brief inline comment next to the payload
expression "','.padLeft(25, ',')" in the tests "GHSA payload throws when limit
exceeded in strict mode" and "GHSA payload converts to overflow map without
throw" stating that padLeft(25) yields 25 commas which produce 26 elements after
splitting (expected element count = 26); place the comment immediately beside
the payload variable to clarify intent for future readers.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart`: - Around line 193-207: The RangeError message in the comma-split overflow branch is duplicated; replace the hard-coded string used when options.throwOnLimitExceeded is true with the existing helper _listLimitExceededMessage so all three occurrences (this block handling options.comma, the branch using Utils.combine, and the _parseListValue function) reuse the same message generator; update the throwing site (the if (options.throwOnLimitExceeded) { throw RangeError(...); } in the comma overflow block) to call _listLimitExceededMessage(options.listLimit) (or the helper's exact name/signature) instead of inlining the string, ensuring consistency across _parseListValue and other places. - Around line 239-255: The _listLikeCount function currently computes overflow map size by scanning keys and returning maxIndex + 1 which over-counts sparse overflow maps; change _listLikeCount (and its overflow branch that checks Utils.isOverflow) to return the actual entry count (value.length) instead of maxIndex + 1, and add a short comment noting that if Utils.combine guarantees dense sequential keys this could be an alternative behavior; ensure references to Utils.isOverflow remain and keep behavior for Iterable/other types unchanged. In `@test/unit/decode_test.dart`: - Around line 3047-3077: Add a brief inline comment next to the payload expression "','.padLeft(25, ',')" in the tests "GHSA payload throws when limit exceeded in strict mode" and "GHSA payload converts to overflow map without throw" stating that padLeft(25) yields 25 commas which produce 26 elements after splitting (expected element count = 26); place the comment immediately beside the payload variable to clarify intent for future readers.test/unit/decode_test.dart (1)
3047-3077: GHSA payload tests look correct and valuable.Minor note:
','.padLeft(25, ',')produces exactly 25 commas (hence 26 elements after split). The intent is clear, but a brief inline comment stating the expected element count could aid future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/decode_test.dart` around lines 3047 - 3077, Add a brief inline comment next to the payload expression "','.padLeft(25, ',')" in the tests "GHSA payload throws when limit exceeded in strict mode" and "GHSA payload converts to overflow map without throw" stating that padLeft(25) yields 25 commas which produce 26 elements after splitting (expected element count = 26); place the comment immediately beside the payload variable to clarify intent for future readers.lib/src/extensions/decode.dart (2)
193-207: Duplicated error message — consider reusing_listLimitExceededMessage.The message string on lines 202–203 is identical to the helper introduced at line 231. The same pattern also appears at lines 72–73 inside
_parseListValue. Using the helper in all three sites would eliminate the duplication.♻️ Suggested diff
if (options.throwOnLimitExceeded) { - throw RangeError( - 'List limit exceeded. Only ${options.listLimit} ' - 'element${options.listLimit == 1 ? '' : 's'} allowed in a list.', - ); + throw RangeError(_listLimitExceededMessage(options.listLimit)); }And similarly in
_parseListValue(line 72–73):: 'List limit exceeded. Only ${options.listLimit} ' - 'element${options.listLimit == 1 ? '' : 's'} allowed in a list.'; + '${_listLimitExceededMessage(options.listLimit)}';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart` around lines 193 - 207, The RangeError message in the comma-split overflow branch is duplicated; replace the hard-coded string used when options.throwOnLimitExceeded is true with the existing helper _listLimitExceededMessage so all three occurrences (this block handling options.comma, the branch using Utils.combine, and the _parseListValue function) reuse the same message generator; update the throwing site (the if (options.throwOnLimitExceeded) { throw RangeError(...); } in the comma overflow block) to call _listLimitExceededMessage(options.listLimit) (or the helper's exact name/signature) instead of inlining the string, ensuring consistency across _parseListValue and other places.
239-255:_listLikeCountrelies on max-index rather than entry count for overflow maps.For overflow maps with sequential
"0","1", … keys (the normal case from comma-split conversion),maxIndex + 1equals the entry count. However, if an overflow map ever contains gaps (e.g. keys"0"and"5"), this returns 6 instead of 2, which could make the strict-mode guard at lines 215–218 over-count and throw prematurely.If overflow maps are guaranteed to have dense sequential keys from
Utils.combine, this is fine — but worth a brief comment to document that assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart` around lines 239 - 255, The _listLikeCount function currently computes overflow map size by scanning keys and returning maxIndex + 1 which over-counts sparse overflow maps; change _listLikeCount (and its overflow branch that checks Utils.isOverflow) to return the actual entry count (value.length) instead of maxIndex + 1, and add a short comment noting that if Utils.combine guarantees dense sequential keys this could be an alternative behavior; ensure references to Utils.isOverflow remain and keep behavior for Iterable/other types unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 788cfe2f33
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
lib/src/extensions/decode.dart:74
- The strict list-growth guard
currentListLength >= options.listLimitbecomes unconditional whenlistLimitis negative (e.g.,0 >= -1), which makesQS.decode('a=b', DecodeOptions(listLimit: -1, throwOnLimitExceeded: true))throw even though no list growth is happening. This contradictsDecodeOptions.listLimitdocs (negative disables numeric indexing; strict mode should only throw when an operation would grow a list, likea[]/comma/duplicates). Adjust the condition so negativelistLimitonly throws on actual list-growth paths (and ensure duplicates/[] are still enforced appropriately).
// Guard incremental growth of an existing list as we parse additional items.
if (options.throwOnLimitExceeded &&
currentListLength >= options.listLimit) {
final String msg = options.listLimit < 0
? 'List parsing is disabled (listLimit < 0).'
: 'List limit exceeded. Only ${options.listLimit} '
'element${options.listLimit == 1 ? '' : 's'} allowed in a list.';
throw RangeError(msg);
…prove error management
…numeric-entity interpretation
…in sparse overflow maps
…ove list limit checks
…values decode correctly
|
@codex review |
| // Enforce comma-split overflow behavior before any normalization that can | ||
| // collapse iterables into scalars (e.g. numeric entity interpretation). | ||
| if (options.comma && | ||
| options.listLimit >= 0 && | ||
| val is Iterable && | ||
| val.length > options.listLimit) { | ||
| if (options.throwOnLimitExceeded) { | ||
| throw RangeError(_listLimitExceededMessage(options.listLimit)); | ||
| } | ||
| val = Utils.combine([], val, listLimit: options.listLimit); | ||
| } |
There was a problem hiding this comment.
The comma-overflow enforcement triggers for any Iterable val when options.comma is true, including Iterables returned by a custom DecodeOptions.decoder even when the raw value was not comma-split. This can cause unexpected RangeErrors or overflow-map conversions for callers using custom decoders. Consider restricting this check to values that actually came from comma-splitting (e.g., track a wasCommaSplit flag from _parseListValue, or check the raw substring for , before decoding) so non-comma Iterables from custom decoders aren’t treated as comma lists.
| // Quirk: a literal `[]=` suffix forces an array container (qs behavior). | ||
| if (options.parseLists && part.contains('[]=')) { | ||
| if (options.throwOnLimitExceeded && options.listLimit < 0) { | ||
| throw RangeError('List parsing is disabled (listLimit < 0).'); | ||
| } | ||
| val = [val]; | ||
| } |
There was a problem hiding this comment.
The new strict-mode guard for negative listLimit only checks part.contains('[]='). A bare key like a[] (no '=') or a[]&... still reaches _parseObject and will create a list even though strict mode is treating listLimit < 0 as “list parsing disabled” elsewhere. To keep behavior consistent, enforce the same rule for the pos == -1 path when the key contains [], or enforce it centrally in _parseObject when encountering a [] segment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb40ce9eb0
ℹ️ 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.parseLists && part.contains('[]=')) { | ||
| if (options.throwOnLimitExceeded && options.listLimit < 0) { | ||
| throw RangeError('List parsing is disabled (listLimit < 0).'); |
There was a problem hiding this comment.
In _parseQueryStringValues, the new strict negative-limit guard only runs inside the part.contains('[]=') branch, so list-growth shapes that do not include that literal substring (for example a[][b]=1) bypass the throw and still create arrays later in _parseObject. With DecodeOptions(listLimit: -1, throwOnLimitExceeded: true), this now permits list construction in a mode documented to disable list growth, so untrusted nested [] inputs can evade the limit enforcement.
Useful? React with 👍 / 👎.
Summary
This PR aligns
qs_dartdecode behavior with upstreamqsv6.14.2for comma-separated value parsing and list-limit overflow handling.Important context: the previous Dart implementation did not exhibit the same bypass pattern as the Node advisory (
GHSA-w7fw-mjwx-w883). It already enforced limits for comma parsing (throw in strict mode, truncate in non-strict mode). This PR is primarily a parity/behavior alignment update with upstream patched semantics.Motivation
qsv6.14.2.Changes
lib/src/extensions/decode.dart.comma: true,listLimit >= 0,throwOnLimitExceeded: false) now converts to an overflow map and preserves all values.RangeErroron overflow.listLimit < 0), as intended.test/unit/decode_test.dart:test/comparison/package.jsonfromqs:^6.14.1toqs:^6.14.2.Behavioral Impact
listLimit:Validation
dart test test/unit/decode_test.dartpassed.dart test test/unit/uri_extension_test.dartpassed.dart analyze lib test --fatal-infospassed.test/comparison/compare_outputs.shpassed after upgrading comparison dependency.References
parse: fix error message to reflect arrayLimit as max index; remove extraneous comments ljharb/qs#545qsv6.14.2