ljharb / qs

A querystring parser with nesting support
BSD 3-Clause "New" or "Revised" License
8.47k stars 731 forks source link

Nested Brackets #494

Open gronke opened 5 months ago

gronke commented 5 months ago
# 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 commented 5 months ago

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

ljharb commented 5 months ago

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[]"?

gronke commented 5 months ago

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 commented 5 months ago

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.