fix: prevent DecodeOptions.listLimit bypass in bracket notation to mitigate potential DoS via memory exhaustion
#46
Conversation
…tial DoS via memory exhaustion
WalkthroughDecoder and utility code now propagate a configurable Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 97.91% 97.90% -0.01%
==========================================
Files 14 14
Lines 910 954 +44
==========================================
+ Hits 891 934 +43
- Misses 19 20 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements a security fix to prevent potential DoS attacks via memory exhaustion by enforcing the listLimit when decoding query strings. Instead of allowing lists to grow beyond the limit, the implementation now converts them to special "overflow" objects (maps with numeric string keys).
Changes:
- Introduces overflow object tracking using
Expandoto mark maps that represent arrays exceedinglistLimit - Updates
Utils.combineandUtils.mergeto handle overflow objects by maintaining numeric indices and preventing unbounded list growth - Integrates overflow handling into the decode logic to ensure consistent behavior across all decode paths
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/src/utils.dart | Adds overflow tracking infrastructure (Expando, helper methods) and updates combine/merge to handle overflow objects |
| lib/src/extensions/decode.dart | Integrates listLimit parameter in combine calls and preserves overflow objects during decode |
| test/unit/decode_test.dart | Updates expectations for listLimit: 0 behavior and adds comprehensive tests for overflow scenarios and DOS prevention |
| test/unit/uri_extension_test.dart | Updates test expectations to reflect overflow object behavior with listLimit: 0 |
| test/unit/utils_test.dart | Adds tests for overflow object merging and combining, including boundary conditions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/utils.dart (2)
160-180: Potential merge semantic bug: overflow target + Iterable source is inserted as a single element.At Line 161-166, if
targetis overflow andsourceis anIterable(e.g., a list produced by another key path), the current logic appends the entire iterable as one value at the next numeric key. This is likely to produce surprising shapes for mixed query notations, and may break qs parity for cases like combininga[]=witha[0]=...under smalllistLimit.Proposed fix (spread iterables when appending into overflow)
if (isOverflow(target)) { - final int newIndex = _getOverflowIndex(target) + 1; - target[newIndex.toString()] = source; - _setOverflowIndex(target, newIndex); - return target; + int newIndex = _getOverflowIndex(target); + if (source is Iterable) { + for (final item in source.whereNotType<Undefined>()) { + newIndex++; + target[newIndex.toString()] = item; + } + } else { + newIndex++; + target[newIndex.toString()] = source; + } + _setOverflowIndex(target, newIndex); + return target; }
194-254: Add tests for mixed bracket-array and numeric-index sequences under tight listLimits, with overflow-status assertions.Lines 223–253 correctly propagate overflow when merging map-with-map. However, test coverage does not explicitly verify the mixed notation patterns mentioned in the review:
a[]=b&a[0]=canda[0]=b&a[]=cwithlistLimit: 0orlistLimit: 1.Current tests cover structurally similar cases (
a[]=b&a=cwithlistLimit: 0→{'0': 'b', '1': 'c'}, anda[]=1&a[]=2withlistLimit: 1), but do not assert onUtils.isOverflow()status in decode results. Add tests for the exact sequences above and verify that the returned map is marked as overflow when expected.Suggested test additions
test('a[]=b&a[0]=c with listLimit: 0 produces overflow map', () { final result = QS.decode('a[]=b&a[0]=c', const DecodeOptions(listLimit: 0)); expect(result['a'], isA<Map<String, dynamic>>()); expect(result['a'], {'0': 'b', '1': 'c'}); expect(Utils.isOverflow(result['a']), isTrue); }); test('a[0]=b&a[]=c with listLimit: 0 produces overflow map', () { final result = QS.decode('a[0]=b&a[]=c', const DecodeOptions(listLimit: 0)); expect(result['a'], isA<Map<String, dynamic>>()); expect(result['a'], {'0': 'b', '1': 'c'}); expect(Utils.isOverflow(result['a']), isTrue); });
🧹 Nitpick comments (4)
lib/src/extensions/decode.dart (1)
263-275: Preserving overflow leaves avoids accidentally re-materialising lists.Line 266-274 short-circuits when
leafis already an overflow object, preventing it being wrapped/combined back into a list during[]handling.Consider a small comment clarifying when
leafcan already be overflow here (e.g., earliercombineon duplicates), to make the invariant easier to follow.test/unit/utils_test.dart (1)
1010-1058: Nice: listLimit boundary + “existing overflow” behaviour is now pinned by tests.One gap worth considering: add a test for
Utils.merge(overflowMap, ['x','y'])(and/orUtils.merge(['x'], overflowMap)) to lock down whether iterables should be appended/spread vs inserted as a single value. That case is easy to regress and is relevant to mixed query notations.test/unit/decode_test.dart (1)
2027-2111: Good regression suite for the[]listLimit bypass; consider asserting overflow tagging too.If “overflow-ness” (not just “is a Map”) is intended to be stable/observable across merges, add
expect(Utils.isOverflow(aValue), isTrue)in the new tests to catch future cases where a plain numeric-key map is produced without being treated as overflow.lib/src/utils.dart (1)
31-61: Overflow tagging viaExpandois a solid approach.This avoids mutating map contents while still allowing “overflow” to be tracked across merges/combines.
markOverflow(...)is used in production code paths; consider annotating it@internalas well (in addition to@visibleForTesting) to better reflect intended usage.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/decode.dartlib/src/utils.darttest/unit/decode_test.darttest/unit/uri_extension_test.darttest/unit/utils_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
lib/src/extensions/decode.dart (1)
193-199: PropagatelistLimitinto duplicate-key combining (good hardening).Line 195 ensures
Duplicates.combinecan’t grow lists pastoptions.listLimitvia repeated keys, which is the core bypass described in the PR objective.test/unit/uri_extension_test.dart (1)
439-479: Test updates correctly reflect listLimit=0 now producing numeric-key maps for[]growth.These assertions align with the new “overflow object” behaviour and help prevent regressions across
Uri.queryParametersQs().Also applies to: 828-853
test/unit/utils_test.dart (2)
935-961: Good targeted coverage for merging overflow objects.These cases validate overflow retention and index progression in
Utils.merge(...).
966-1008: Test refactor (type inference) is fine.No loss of clarity here, and it keeps the tests consistent with other groups.
test/unit/decode_test.dart (1)
511-542: Updated listLimit=0 expectations look consistent with the new overflow/map semantics.These changes should prevent accidental reversion to list materialisation in the limit=0 paths.
Also applies to: 913-941, 1994-2004
…ure proper handling of empty-bracket pushes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/utils.dart (1)
653-680: Consider documenting the in-place mutation of overflow objects.When
ais already an overflow object (lines 654–667), the code mutates it directly by adding new keys. This is efficient but could surprise callers if the same overflow object were reused across multiplecombinecalls.In the current decode flow this appears safe since structures are freshly created, but adding a brief doc note about the mutation behaviour would aid future maintainability.
📝 Suggested documentation addition
/// Concatenates two values, spreading iterables. /// /// When [listLimit] is provided and exceeded, returns a map with string keys. + /// + /// **Note:** If [a] is already an overflow object, this method mutates [a] + /// in place by appending entries from [b]. /// /// Examples:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/extensions/decode.dartlib/src/models/decode_options.dartlib/src/utils.darttest/comparison/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
lib/src/models/decode_options.dart (1)
101-117: LGTM!The updated documentation clearly explains the revised
listLimitsemantics, including:
- The list-to-map conversion when limits are exceeded (matching Node
qsarrayLimitbehaviour)- Negative value behaviour disabling numeric-index parsing
- Interaction with
throwOnLimitExceededThis provides good clarity for users of the API.
lib/src/extensions/decode.dart (2)
195-195: LGTM!Correctly propagates
listLimittoUtils.combinewhen handling duplicate key combining, ensuring overflow semantics are applied during the tokenisation phase.
267-275: LGTM!The overflow-aware handling for anonymous list segments (
[]) is well-structured:
- Preserves existing overflow objects to avoid re-wrapping
- Falls back to
Utils.combinewithlistLimitfor normal leaves- Maintains the existing
allowEmptyListsbehaviour for empty/null leaveslib/src/utils.dart (4)
31-62: Overflow tracking infrastructure looks well-designed.Using
Expando<int>is an appropriate choice for attaching metadata without mutating the underlying map structure. The helper methods provide a clean internal API for overflow management.One minor observation:
_getOverflowIndexreturns-1when no overflow is set, which works correctly with the+1logic in callers. Consider adding a brief inline comment explaining this sentinel value for future maintainers.
161-166: LGTM!Correctly handles appending a non-Map source to an overflow target by inserting at the next available index and updating the overflow marker.
194-209: LGTM!The logic for merging an overflow source into a non-map target correctly offsets numeric keys to accommodate the target at index
'0'. The null-check on line 197 properly avoids inserting a null value.
223-252: LGTM!Overflow status is correctly preserved and propagated through the recursive merge operation. The
_updateOverflowMaxcall ensures the max index is tracked as new keys are merged in.test/comparison/package.json (1)
8-8: LGTM!The dependency bump to
qs@^6.14.1aligns the test comparison environment with the upstream Nodeqsversion that includes the criticalarrayLimitsemantics fix (CVE-2025-15284) that this PR is matching.
… bypass and enhance memory safety
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
lib/src/utils.dart:86
- The merge function's documentation does not mention that it mutates overflow objects in place (lines 169-182). When
targetis an overflow object, the function modifies it directly and returns it, which is inconsistent with the general principle stated in the class documentation that functions are pure except forcompact. Either update the function documentation to explicitly state that overflow objects may be mutated, or update the class-level documentation to listmergeas another exception to the purity principle.
/// Deeply merges `source` into `target` while preserving insertion order
/// and list semantics used by `qs`.
///
/// Rules of thumb:
/// - If `source == null`, returns `target` unchanged.
/// - When **both** sides are maps, keys are stringified and values are merged
/// recursively.
/// - When `target` is an **Iterable** and `source` is **not** a map:
/// - If either side is a list/set of maps, items are merged **by index**.
/// - Otherwise values are **appended** (keeping iteration order).
/// - Presence of [Undefined] acts like a hole; if `options.parseLists == false`
/// and any `Undefined` remain after merging, the result is normalized to
/// a map with string indices (`"0"`, `"1"`, …) to force object-shape.
/// - When `target` is a **map** and `source` is an **Iterable**, the iterable
/// is promoted to an object using string indices and merged in.
/// - If neither side is a map/iterable, the two values are wrapped into a
/// two-element list `[target, source]`.
///
/// Ordering guarantees:
/// - Uses `SplayTreeMap` for temporary index maps to keep keys predictable.
///
/// This mirrors the behavior of the original Node.js `qs` merge routine,
/// including treatment of `Undefined` sentinels.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/src/utils.dart:
- Around line 165-185: The isOverflow(target) branch contains an unreachable
nested check for "if (source is Iterable)" because an earlier "if (source is
Iterable)" returned; remove that inner conditional and its loop, and always
treat the overflow case as the non-Iterable branch: call
_getOverflowIndex(target), increment once, assign target[newIndex.toString()] =
source, call _setOverflowIndex(target, newIndex) and return target; update the
code in the same function where merge(...), isOverflow(target),
_getOverflowIndex, and _setOverflowIndex are used to reflect this simplified
overflow handling.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/decode.dartlib/src/qs.dartlib/src/utils.darttest/unit/decode_test.darttest/unit/utils_test.dart
💤 Files with no reviewable changes (1)
- lib/src/qs.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
test/unit/utils_test.dart (2)
934-984: Comprehensive overflow merge tests look good.The test group covers key scenarios for overflow object handling:
- Merging primitives into overflow objects at next index
- Merging overflow objects into primitives (with offset adjustment)
- Merging lists with overflow objects
- Preservation of overflow status through merges
The assertions correctly verify both the resulting structure and the overflow status via
Utils.isOverflow.
1032-1081: Good coverage forcombinewithlistLimitand overflow propagation.The tests thoroughly validate:
- Under/at/over limit boundary conditions
listLimit: 0edge case converting immediately to map- Appending to existing overflow objects preserves identity (
same(overflow))- Plain objects with numeric keys are correctly distinguished from overflow objects
One minor observation: the test at line 1067 uses
same(overflow)to verify mutation-in-place, which correctly documents the API's behaviour thatcombinemutates existing overflow objects rather than creating copies.test/unit/decode_test.dart (3)
518-541: Updated expectations correctly reflect overflow semantics.When
listLimit: 0, the decoder now produces maps with numeric string keys instead of lists. The change from:'a': ['b', 'c']to:
'a': {'0': 'b', '1': 'c'}aligns with the DoS mitigation strategy where exceeding the limit converts arrays to object notation.
2027-2061: Good DoS prevention and boundary condition tests.The "array limit parity" group effectively validates:
- DoS prevention with 105 values against
listLimit: 100producing an overflow map- Boundary at limit (3 items with
listLimit: 3) stays as list- Over limit (4 items with
listLimit: 3) converts to map with overflow marker- Overflow tagging via
Utils.isOverflowassertionsThis addresses the security advisory GHSA-6rw7-vpxm-498p mentioned in the PR objectives.
2114-2144: Valuable tests for order-dependent overflow tagging.These tests document an important behaviour: overflow tagging depends on parsing order:
a[]=b&a[0]=cwithlistLimit: 0→ overflow tagged (bracket notation seen first)a[0]=b&a[]=cwithlistLimit: 0→ not overflow tagged (explicit index seen first)The comment "overflow tagging is order-dependent in Node qs; we mirror that" provides helpful context for maintainers.
lib/src/extensions/decode.dart (2)
195-195: Correct propagation oflistLimitduring duplicate key combining.Passing
listLimittoUtils.combineensures that when duplicate keys are combined, the resulting structure respects the configured limit and converts to an overflow map when exceeded.
267-276: Sound overflow preservation for anonymous list segments.The logic correctly handles the case where
leafis already an overflow object (e.g., from earlier duplicate combining or listLimit enforcement):
- If overflow → preserve as-is to avoid double-wrapping
- Otherwise → use
Utils.combine([], leaf, listLimit: ...)which respects the limitThis ensures consistent behaviour when parsing patterns like
a[]=b&a[]=c&a[]=dunder tight limits.lib/src/utils.dart (3)
31-65: Well-designed overflow tracking using Expando.Using
Expando<int>to track overflow state is a sound approach:
- Avoids mutating user-provided data structures
- Allows GC to clean up when objects are no longer referenced
- Provides O(1) lookup for overflow detection
The helper functions (
markOverflow,isOverflow,_getOverflowIndex,_setOverflowIndex,_updateOverflowMax) provide a clean internal API.
225-239: Correct overflow source handling with index offsetting.When merging an overflow source into a non-map/non-iterable target, the logic correctly:
- Places the target at index
'0'- Offsets all numeric keys from the source by +1
- Preserves non-numeric keys as-is
- Marks the result as overflow with
sourceMax + 1This maintains proper ordering when prepending a scalar to an overflow collection.
688-715:combinecorrectly implements overflow-aware merging.The implementation handles three cases:
- Existing overflow: Mutates in place, appending items with incremented indices
- Under limit: Returns a normal list
- Over limit: Creates an overflow map via
createIndexMapand marks itThe in-place mutation for existing overflow objects is documented in the docstring (line 681), which is important for callers to understand.
One consideration: the mutation of
awhen it's an overflow object means callers should not expect immutability. This is acceptable given the documented behaviour and the internal nature of this API.
…o ensure correct behavior
techouse
changed the title
listLimit bypass in bracket notation to mitigate potential DoS via memory exhaustionDecodeOptions.listLimit bypass in bracket notation to mitigate potential DoS via memory exhaustion
techouse
deleted the
fix/qs-array-limit-bypass-in-bracket-notation-allows-dos-via-memory-exhaustion
branch
This pull request significantly improves the handling of array limits (
listLimit) in the query string decoding logic, aligning the behavior more closely with the Node.jsqslibrary. The main changes introduce robust overflow handling to prevent denial-of-service (DoS) scenarios from large lists, ensure correct type conversion when limits are exceeded, and update tests and documentation accordingly.Key changes include:
Core decoding logic and overflow handling
listLimit, converting them into maps with string indices and marking them using anExpandofor internal tracking. This prevents unbounded memory growth and matches Node.jsqsarrayLimit semantics. (lib/src/utils.dart[1] [2] [3] [4] [5] [6] [7]a[0],a[1]) and anonymous pushes (a[]) respect thelistLimitand convert to maps as needed. (lib/src/extensions/decode.dart[1] [2]Documentation and option semantics
DecodeOptions.listLimit, specifying its effect on both explicit indices and anonymous pushes, and describing the new overflow-to-map conversion behavior. (lib/src/models/decode_options.dartlib/src/models/decode_options.dartL101-R107)QS.decodeAPI and internal methods to reflect the new overflow handling and removed outdated notes about temporary list parsing disablement. (lib/src/qs.dart[1] [2]Testing and compatibility
test/unit/decode_test.dart[1] [2] [3] [4] [5] [6]test/unit/uri_extension_test.dart[1] [2] [3] [4]qsdependency version in the comparison test package to ensure compatibility with the latest Node.js behavior. (test/comparison/package.jsontest/comparison/package.jsonL8-R8)These changes make the query string decoding safer and more predictable under extreme input, and ensure strong compatibility with established
qsbehavior in the JavaScript ecosystem.Relates to GHSA-6rw7-vpxm-498p