Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested Brackets #494

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Nested Brackets #494

wants to merge 1 commit into from

Conversation

gronke
Copy link

@gronke gronke commented Feb 21, 2024

  • Add unit test for nested brackets (?foo[bar[]]=baz)
# params containing brackets within brackets
not ok 211 should be deeply equivalent
  ---
    operator: deepEqual
    expected: |-
      { a: { 'b[]': 'c' } }
    actual: |-
      { 'a[b': [ 'c' ] }
    at: Test.<anonymous> (/home/runner/work/qs/qs/test/parse.js:6:1420)
    stack: |-
      Error: should be deeply equivalent
          at Test.assert (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:493:48)
          at Test.tapeDeepEqual (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:748:7)
          at Test.<anonymous> (/home/runner/work/qs/qs/test/parse.js:6:1420)
          at Test.run (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:127:28)
          at Test._end (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:399:5)
          at Test.<anonymous> (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:398:34)
          at Test.emit (events.js:92:17)
          at completeEnd (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:404:27)
          at next (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:418:4)
          at Test._end (/home/runner/work/qs/qs/node_modules/tape/lib/test.js:438:2)

@ljharb
Copy link
Owner

ljharb commented Feb 28, 2024

I'm confused; you're parsing an object, not a string?

@ljharb
Copy link
Owner

ljharb commented Feb 28, 2024

Certainly the test fails in the same way if a string is passed - but what do you expect to happen here?

brackets in a query string represent either an array item (with or without a numeric index) or an object key. is the object key here literally "b[]"?

@ljharb ljharb marked this pull request as draft February 28, 2024 05:59
@gronke
Copy link
Author

gronke commented Feb 28, 2024

Certainly the test fails in the same way if a string is passed - but what do you expect to happen here?

brackets in a query string represent either an array item (with or without a numeric index) or an object key. is the object key here literally "b[]"?

I came across this bug after having the "great" idea to give some SQL column names a [] suffix to indicate it may be interpreted as a list. Got me interested how URLs are parsed in Express, which brought me here.

Interestingly this differences in parsing data could have a security impact, for instance when a WAF is expecting the one format, but the application is reading it in another way.

Query parameters in string notation do not appear to be a regular language and can not be parsed with a RegExp (similar to HTML). I would imagine walking over the string and counting the unescaped opening and closing [ brackets can do the job.

Thought it would be nice to leave a unit test here to have an example for one query param name that does not parse.

@ljharb
Copy link
Owner

ljharb commented Feb 28, 2024

I appreciate the test case! Something certainly needs fixing regardless, since 'a[b' is clearly wrong. However, to know what the right fix is, I need to understand the use case.

It sounds like you're saying that the object property name should be the literal string 'b[]'?

if so, then what i'd expect (without looking at any code, just off the top of my head) is that an object of { a: { 'b[]': 1 } } would stringify to something like a[b%5B%5D]=1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants