Skip to content

Fix parsing of keys containing literal [] inside bracket groups#530

Open
techouse wants to merge 1 commit intoljharb:mainfrom
techouse:fix/qs-493-parse-nested-brackets
Open

Fix parsing of keys containing literal [] inside bracket groups#530
techouse wants to merge 1 commit intoljharb:mainfrom
techouse:fix/qs-493-parse-nested-brackets

Conversation

@techouse
Copy link
Copy Markdown
Contributor

@techouse techouse commented Aug 26, 2025

Problem

qs splits bracket groups with a regex that forbids [] inside a group:

var child = /(\[[^[][^\]]*])/g; // original: /(\[[^[\]]*])/g

This causes keys like search[withbracket[]] to split into "[withbracket]", then "[]", leaving the inner [] detached from the literal segment. Downstream, it can no longer be treated as a single literal key.

Repro

// expected:
qs.parse('search[withbracket[]]=foobar');
// => { search: { 'withbracket[]': 'foobar' } }

// actual (today):
// inner [] is split out, so the key can’t be treated as one literal segment

Fix approach

Backport splitKeyIntoSegments from qs_dart and replace the regex child matcher with a balanced bracket splitter that treats [] inside a bracket group as literal content (only outer [] act as array push). This preserves withbracket[] as a single segment.

Notes

  • Does not change existing semantics for [] at the outermost level (array push).
  • Honors existing options: allowDots, depth, strictDepth, allowPrototypes, etc.

Test snippet

t.test('parses keys with literal [] inside a bracket group (#493)', function (st) {
    // A bracket pair inside a bracket group should be treated literally as part of the key
    st.deepEqual(
        qs.parse('search[withbracket[]]=foobar'),
        { search: { 'withbracket[]': 'foobar' } },
        'treats inner [] literally when inside a bracket group'
    );

    // Single-level variant
    st.deepEqual(
        qs.parse('a[b[]]=c'),
        { a: { 'b[]': 'c' } },
        'keeps "b[]" as a literal key'
    );

    // Nested with an array push on the outer level
    st.deepEqual(
        qs.parse('list[][x[]]=y'),
        { list: [{ 'x[]': 'y' }] },
        'preserves inner [] while still treating outer [] as array push'
    );

    // Multiple nested bracket pairs: inner [] remains literal as part of the key
    st.deepEqual(
        qs.parse('a[b[c[]]]=d'),
        { a: { 'b[c[]]': 'd' } },
        'treats "b[c[]]" as a literal key inside the bracket group'
    );

    // Depth limits with literal brackets: preserve inner [] while limiting bracket-group parsing
    st.deepEqual(
        qs.parse('a[b[c[]]][d]=e', { depth: 1 }),
        { a: { 'b[c[]]': { '[d]': 'e' } } },
        'respects depth: 1 and preserves literal inner [] in the parsed key'
    );

    // Unterminated inner bracket group is wrapped as a literal remainder segment
    st.deepEqual(
        qs.parse('a[[]b=c'),
        { a: { '[[]b': 'c' } },
        'handles unterminated inner bracket groups without throwing'
    );

    st.end();
});

Fixes #493

@techouse techouse changed the title Fix incorrect parsing of nested params with closing square bracket ] in property name Fix parsing of keys containing literal [] inside bracket groups Aug 27, 2025
@techouse techouse changed the title Fix parsing of keys containing literal [] inside bracket groups Fix parsing of keys containing literal [] inside bracket groups Aug 27, 2025
@techouse
Copy link
Copy Markdown
Contributor Author

techouse commented Aug 28, 2025

@ljharb, it seems some of the tests fail because the runners are unable to download a specific Node version, i.e.

curl: (56) OpenSSL SSL_read: Connection reset by peer, errno 104
download from https://nodejs.org/dist/v5.3.0/node-v5.3.0-linux-x64.tar.xz failed
grep: /home/runner/.nvm/.cache/bin/node-v5.3.0-linux-x64/node-v5.3.0-linux-x64.tar.xz: No such file or directory
curl: (92) HTTP/2 stream 0 was not closed cleanly: INTERNAL_ERROR (err 2)
download from https://nodejs.org/dist/v13.13.0/node-v13.13.0-linux-x64.tar.xz failed
grep: /home/runner/.nvm/.cache/bin/node-v13.13.0-linux-x64/node-v13.13.0-linux-x64.tar.xz: No such file or directory

@techouse
Copy link
Copy Markdown
Contributor Author

@ljharb, have you had a chance to look at this yet? 😊

@ljharb ljharb requested a review from Copilot November 4, 2025 06:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements bracket balancing in the query string parser to correctly handle literal [] characters inside bracket groups. Previously, bracket pairs within a bracket group could cause parsing issues. The new implementation uses a level counter to properly balance opening and closing brackets, treating inner bracket pairs as literal parts of keys.

Key Changes

  • Refactored parseKeys function to extract bracket parsing logic into a new splitKeyIntoSegments helper function
  • Implemented bracket-balancing algorithm that uses a level counter to track nested brackets
  • Added test cases validating that [] inside bracket groups are treated as literal key characters

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/parse.js Replaced regex-based bracket parsing with bracket-balancing algorithm that correctly handles nested [] within bracket groups
test/parse.js Added test case covering three scenarios: basic nested brackets, single-level variant, and array push with nested brackets

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty hard to review. Could you perhaps make a refactoring commit that makes the majority of the changes, and then a fix commit with the test and the thing that makes the test pass?

@techouse techouse force-pushed the fix/qs-493-parse-nested-brackets branch from 243b1f5 to bb1b7c3 Compare November 4, 2025 08:41
@techouse
Copy link
Copy Markdown
Contributor Author

techouse commented Nov 4, 2025

This is pretty hard to review. Could you perhaps make a refactoring commit that makes the majority of the changes, and then a fix commit with the test and the thing that makes the test pass?

@ljharb done. 😇

I have split the parse key processing into a refactor-only commit followed by the nested-bracket fix plus its regression tests so it now has the structure you have asked for.

@techouse techouse requested a review from Copilot November 4, 2025 08:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ljharb ljharb marked this pull request as draft February 11, 2026 21:23
@techouse
Copy link
Copy Markdown
Contributor Author

techouse commented Feb 17, 2026

Thanks for the review, @ljharb 😇

I have pushed updates to align with main semantics and addressed each thread below:

  • restored depth <= 0 behavior (including prototype guard) in splitKeyIntoSegments
  • switched strict depth checks to options.strictDepth === true
  • removed the remainder !== '.' special-case and simplified tail handling
  • applied the naming/style suggestions

I have also added regression coverage for depth-0 behavior (allowDots normalization and prototype guarding).

The merge conflicts were quite the challenge (this PR is from August!), so I hope I didn't mess up somewhere 😅

@techouse techouse requested review from Copilot and ljharb February 17, 2026 21:17
@techouse techouse marked this pull request as ready for review February 17, 2026 21:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@techouse techouse force-pushed the fix/qs-493-parse-nested-brackets branch from 98bc892 to 172525a Compare February 17, 2026 22:13
@techouse techouse marked this pull request as ready for review February 17, 2026 22:14
@techouse techouse requested a review from Copilot February 17, 2026 22:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@techouse techouse force-pushed the fix/qs-493-parse-nested-brackets branch from 9eb32ca to bc25549 Compare February 17, 2026 22:33
@techouse techouse requested a review from Copilot February 17, 2026 22:33
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect parsing of nested params with closing square bracket ] in property name

3 participants