Repository navigation
Conversation
Since superagent 9.0.2, the library uses `new URL()` instead of the deprecated `url.parse()` for URL handling. The `URL` class automatically normalizes paths containing `/../` sequences, which prevented tests from verifying Express's security behavior for path traversal attempts. This change modifies affected tests to use Node's `http.request()` directly instead of supertest, bypassing client-side URL normalization and allowing proper verification of server-side path traversal protection. Changes: - Add `http` module import to test files - Modify 4 tests in express.static.js to use http.request() - Modify 1 test in acceptance/downloads.js to use http.request() - Add missing test fixture directory "snow ☃" for redirect encoding test - Update package.json to use supertest ^7.1.4 and superagent ^10.2.3 Tests modified: - express.static.js: "should fall-through when traversing past root" - express.static.js: "should 403 when traversing past root" - express.static.js: "should catch urlencoded ../" - express.static.js: "should not allow root path disclosure" - acceptance/downloads.js: "should respond with 403" Fixes compatibility with supertest 7.x and superagent 10.x while maintaining proper security validation.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
To be clear, the changes are only relevant upon upgrading to supertest v7, which is where we get superagent 9.0.2 It is not clear from the PR that this is both a dep bump, and then a code change to address test breakage in said dep bump elsewhere we have intentionally not taken supertest 7, in order to avoid specifically this issue ala pillarjs/send#231 |
jonchurch
left a comment
jonchurch
left a comment
There was a problem hiding this comment.
Idk if upgrading supertest is worth the additional complexity demonstrated in this PR
It's something worth thinking about though, we haven't thought too deeply about using something else as a test client, but we have been concerned in the past about this exact class of bug/drift in supertest
| }, function (res) { | ||
| assert.strictEqual(res.statusCode, 404) | ||
| var body = '' | ||
| res.on('data', function (chunk) { body += chunk }) | ||
| res.on('end', function () { | ||
| assert.strictEqual(body, 'Not Found') | ||
| server.close(done) | ||
| }) |
There was a problem hiding this comment.
When the assertions throw, they prevent cleanup (server.close) so the suite hangs forever with an open server.
They're also throwing in callbacks, so mocha only sees them due to its global uncaught exception handler.
They'd need to be try/catch'd, otherwise we can never clean up or reliaby associate the failure w/ the test.
(ideally we assert in only one place, so moved the first one)
This feedback applies to all of the updated test cases in the PR
| }, function (res) { | |
| assert.strictEqual(res.statusCode, 404) | |
| var body = '' | |
| res.on('data', function (chunk) { body += chunk }) | |
| res.on('end', function () { | |
| assert.strictEqual(body, 'Not Found') | |
| server.close(done) | |
| }) | |
| }, function (res) { | |
| var body = '' | |
| res.on('data', function (chunk) { body += chunk }) | |
| res.on('end', function () { | |
| try { | |
| assert.strictEqual(res.statusCode, 404) | |
| assert.strictEqual(body, 'Not Found') | |
| server.close(done) | |
| } catch(err) { | |
| server.close(() => done(err)) | |
| } | |
| }) |
| "nyc": "^17.1.0", | ||
| "pbkdf2-password": "1.2.1", | ||
| "supertest": "^6.3.0", | ||
| "superagent": "^10.2.3", |
There was a problem hiding this comment.
| "superagent": "^10.2.3", |
Unused?
This comment was marked as spam.
This comment was marked as spam.
|
Hi, Some new Severity: remediation recommended | Category: reliability How to fix: Call res.resume() before close Agent prompt to fix - you can give this to your LLM of choice:
Spotted by Qodo code review - free for open-source projects. |
4 participants
Since superagent 9.0.2, the library uses
new URL()instead of the deprecatedurl.parse()for URL handling. TheURLclass automatically normalizes paths containing/../sequences, which prevented tests from verifying Express's security behavior for path traversal attempts.This change modifies affected tests to use Node's
http.request()directly instead of supertest, bypassing client-side URL normalization and allowing proper verification of server-side path traversal protection.Changes:
httpmodule import to test filesTests modified:
Fixes compatibility with supertest 7.x and superagent 10.x while maintaining proper security validation.
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.