Skip to content

fix(deps): update version lib qs #6969

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.

Sign up for GitHub

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

Jump to bottom
Open
gabrieel1007 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(deps): update version lib qs #6969

gabrieel1007 wants to merge 4 commits into from
+1 −1

Conversation

@gabrieel1007
Copy link

@gabrieel1007 gabrieel1007 commented Jan 2, 2026

  • Issue created about this dependency vulnerability:
  • The lib qs has a vulnerability.
  • what has been done:
  • Update of lib qs to version 6.14.1 (fixed).
  • creation of tests covering whether the failure has been remedied and whether functionality has been compromised

UlisesGascon
UlisesGascon reviewed Jan 3, 2026
View reviewed changes
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Hey @gabrieel1007! Thanks for contributing to Express.

We likely do not need to add additional tests here. This behavior is already well covered in qs (see: ljharb/qs@3086902), and Express does not define an arrayLimit by default.

As a result, customization is left to the user. Any related testing should ideally be handled at the application level, according to their specific configuration and requirements.

Additionally, we will probably want to open a separate PR targeting the 4.x branch to upgrade the qs version as well, since this PR is intended to apply only to the Express 5 line.

@gabrieel1007
Copy link
Author

gabrieel1007 commented Jan 3, 2026

@UlisesGascon
Thank you for your response.
The requested adjustments have been made.

@UlisesGascon
Copy link
Member

UlisesGascon commented Jan 4, 2026

The tests are failing due the changes on qs (see discussion on body-parser too expressjs/body-parser#687 (comment)).

  1) express.urlencoded()
       with extended option
         when false
           should parse multiple key instances:
     Error: expected '{"user":["Tobi","Loki"]}' response body, got '{"user":{"0":"Tobi","1":"Loki"}}'
      at Context.<anonymous> (test/express.urlencoded.js:106:12)
      at process.processImmediate (node:internal/timers:476:21)
      at process.callbackTrampoline (node:internal/async_hooks:128:17)
  ----
      at error (node_modules/supertest/lib/test.js:335:15)
      at Test._assertBody (node_modules/supertest/lib/test.js:206:16)
      at /home/runner/work/express/express/node_modules/supertest/lib/test.js:308:13
      at Test._assertFunction (node_modules/supertest/lib/test.js:285:13)
      at Test.assert (node_modules/supertest/lib/test.js:164:23)
      at Server.localAssert (node_modules/supertest/lib/test.js:120:14)
      at Object.onceWrapper (node:events:631:28)
      at Server.emit (node:events:517:28)
      at emitCloseNT (node:net:2221:8)
      at process.processTicksAndRejections (node:internal/process/task_queues:81:21)

cc: @expressjs/express-tc

@UlisesGascon UlisesGascon mentioned this pull request Jan 4, 2026
Merged
@gabrieel1007
Copy link
Author

gabrieel1007 commented Jan 4, 2026

Os testes estão falhando devido às alterações no qs (veja também a discussão sobre body-parser expressjs/body-parser#687 (comentário) ).

  1) express.urlencoded()
       with extended option
         when false
           should parse multiple key instances:
     Error: expected '{"user":["Tobi","Loki"]}' response body, got '{"user":{"0":"Tobi","1":"Loki"}}'
      at Context.<anonymous> (test/express.urlencoded.js:106:12)
      at process.processImmediate (node:internal/timers:476:21)
      at process.callbackTrampoline (node:internal/async_hooks:128:17)
  ----
      at error (node_modules/supertest/lib/test.js:335:15)
      at Test._assertBody (node_modules/supertest/lib/test.js:206:16)
      at /home/runner/work/express/express/node_modules/supertest/lib/test.js:308:13
      at Test._assertFunction (node_modules/supertest/lib/test.js:285:13)
      at Test.assert (node_modules/supertest/lib/test.js:164:23)
      at Server.localAssert (node_modules/supertest/lib/test.js:120:14)
      at Object.onceWrapper (node:events:631:28)
      at Server.emit (node:events:517:28)
      at emitCloseNT (node:net:2221:8)
      at process.processTicksAndRejections (node:internal/process/task_queues:81:21)

cc: @expressjs/express-tc

@UlisesGascon
Okay, based on this test failure, should we:

  • align the test with the new behavior?
  • or maintain compatibility with the current behavior?

@jonchurch
Copy link
Member

jonchurch commented Jan 5, 2026

These tests are basically just asserting on body-parser behavior due to the pass through, so we fix it in body-parser and take both updates at the same time

we have a PR to update body-parser #6972

@gabrieel1007
Copy link
Author

gabrieel1007 commented Jan 5, 2026

@jonchurch
All right, I'll be waiting,
Thanks for your response.

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

Reviewers

@UlisesGascon UlisesGascon Awaiting requested review from UlisesGascon

1 more reviewer

@efekrskl efekrskl efekrskl approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

5.x deps

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gabrieel1007 @UlisesGascon @jonchurch @efekrskl