Skip to content

[Fix] parse: throw on unbalanced bracket groups#560

Draft
pupuking723 wants to merge 4 commits into
ljharb:mainfrom
pupuking723:fix/unbalanced-bracket-parse-error
Draft

[Fix] parse: throw on unbalanced bracket groups#560
pupuking723 wants to merge 4 commits into
ljharb:mainfrom
pupuking723:fix/unbalanced-bracket-parse-error

Conversation

@pupuking723

@pupuking723 pupuking723 commented Jun 9, 2026

Copy link
Copy Markdown

Closes #558.

This changes parser behavior for malformed nested keys that start a bracket group after a parent key but never close it. Instead of treating the rest of the key as a literal segment, qs.parse now throws a RangeError, which matches the maintainer direction in the issue discussion.

Existing behavior is preserved for bracket-prefixed keys and for keys with unterminated inner bracket groups that are already handled as literal segments.

Verification:

  • npx tape test/parse.js
  • npm run tests-only
  • npm run lint
  • npm test

ljharb added a commit that referenced this pull request Jun 23, 2026
…bracket keys (#558)

Pin the current (lenient) parse output for every malformed/unbalanced bracket-key shape,
so any future change to that behavior
(e.g. the proposed "throw on unbalanced brackets" in #560)
is explicit in the test diff rather than silent.

Each case asserts both that parsing does not throw and the exact output,
via a small `characterizeParse` helper, so a change that introduces a throw fails cleanly per case instead of aborting the run.

Covers the cases #560 would make throw
(`a[bc`, `a[`, `a[b][c`, `a[b]c[d`, the issue reproduction, stray-close variants),
the ones it would leave as literals
(inner-bracket and bracket-prefixed keys),
the depth- and allowDots-dependent shapes,
and the unaffected stray-close/trailing-text keys.
@ljharb ljharb force-pushed the fix/unbalanced-bracket-parse-error branch from 2518024 to 4f2c349 Compare June 23, 2026 04:59
@ljharb

ljharb commented Jun 23, 2026

Copy link
Copy Markdown
Owner

I've added some tests to main, and rebased this PR on top of it, to make sure any breaking changes are surfaced.

@ljharb ljharb changed the title fix(parse): throw on unbalanced bracket groups [Fix] parse: throw on unbalanced bracket groups Jun 23, 2026
@ljharb ljharb added the parse label Jun 23, 2026
@ljharb ljharb marked this pull request as draft June 23, 2026 05:29
@pupuking723 pupuking723 force-pushed the fix/unbalanced-bracket-parse-error branch from 4f2c349 to 8490add Compare June 24, 2026 10:31
@pupuking723

Copy link
Copy Markdown
Author

Updated the branch after the new characterization tests were added on main.\n\nThe PR now keeps the new throwing behavior for unbalanced bracket groups after a parent key, including the #558 reproduction, while preserving the existing lenient behavior for bracket-prefixed keys, inner-bracket cases, and depth-limited literal remainders.\n\nVerification:\n- HOME=/private/tmp/qs-test-home npm run tests-only\n- npm run lint

@pupuking723 pupuking723 force-pushed the fix/unbalanced-bracket-parse-error branch from 8490add to 8105b0c Compare June 24, 2026 10:38
@pupuking723 pupuking723 marked this pull request as ready for review June 24, 2026 10:39
@ljharb

ljharb commented Jun 24, 2026

Copy link
Copy Markdown
Owner

The \n in your message strongly implies you're an LLM - can you confirm you're a human and have substantially contributed to the content in this contribution? If not, your contribution is not copyrightable and thus is not viable for OSS.

Comment thread lib/parse.js
}

if (close < 0) {
if (parent && key.indexOf('[', open + 1) < 0) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If we're going to throw on unbalanced brackets, I think we should throw on all of them rather than this narrow subset. As written, the guard only fires when there's a parent and no later [, which leaves three inconsistent escape hatches:

  1. Depth-dependent. a[b]c[d]e[f=v throws at depth: 5 but silently falls through to the lenient literal path (below, ~line 312) at depth: 1. Whether malformed input throws shouldn't depend on the depth option
  2. Later-[ escape. key.indexOf('[', open + 1) < 0 means a[bc[=v stays lenient even though it's just as unbalanced - only a trailing unclosed bracket throws
  3. Bracket-prefixed keys never throw (the parent && guard), so [a[b=v stays lenient too

So the effective rule is hard to describe: "unbalanced brackets throw, unless the key starts with [, or has another [ after the unclosed one, or the unclosed group is past the depth budget." If the intent is "malformed key → throw," I'd rather detect an unbalanced bracket count for the whole key up front and throw unconditionally, so the behavior is consistent regardless of parent, later brackets, or depth.

@ljharb ljharb marked this pull request as draft June 24, 2026 19:29
@pupuking723

Copy link
Copy Markdown
Author

你在消息中的 \n 一词显然表明你是一名大型语言模型。你能确认你是人类吗?而且,你确实对这篇内容做出了实质性的贡献吧?如果不是这样,那么你的贡献就不属于版权保护的范围,因此不适合用于开源项目中。

Yes, I am a human contributor and I substantially reviewed and contributed to this change.

I used coding assistance while working on it, but I read the issue, reviewed the implementation and tests, made the final decisions, and take responsibility for the contribution.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6.15.2 parse behavioral change breaks nested bracket query string parsing

2 participants