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.

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

Fix ci after location patch #5552

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Fix ci after location patch #5552

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/res.location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('res', function(){
});

request(app)
.get('/?q=http://google.com\\@apple.com')
.get('/?q=http://google.com' + encodeURIComponent('\\@apple.com'))
Copy link
Member

@jonchurch jonchurch Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I know you tested locally, but I have no idea why this works, I guess we are converting the encoded @ (%40) back to a @ before location setting? A percent encoded @ should not trigger this vuln Im pretty sure.

encodeURIComponent('\\@apple.com') //  => "%5C%40apple.com"

This comment is specifically about being unsure how the input ends up matching the expected Location in the test. Who converts that %40 back into the @ we assert against? maybe it's supertest when doing the string compare? Is it Node.js itself doing that before express receives the request.query property? I don't know

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea, do we need to know? It fixes it afaict in all supported versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I shouldn't say no idea. I mean that I looked and it appears old supertest does not encode correctly (not at all IIRC). Encoding ourselves does it and new super test doesnt double escape or anything. This is not something I can spend more time so if you think we need to debug supertest to figure out why then go for it and approve when you are done.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a bug report because we did not have test coverage of accepting URL instances. You requested changes on this. Can you approve it instead? I would like to merge this so we can also put out #5554

Copy link
Member

Choose a reason for hiding this comment

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

I do want to understand why this still passes the test, but I wont block on it bc it is test only changes.

.expect(200)
.expect('Location', 'http://google.com\\@apple.com')
.end(function (err) {
Expand All @@ -68,7 +68,7 @@ describe('res', function(){

// This ensures that our protocol check is case insensitive
request(app)
.get('/?q=HTTP://google.com\\@apple.com')
.get('/?q=HTTP://google.com' + encodeURIComponent('\\@apple.com'))
.expect(200)
.expect('Location', 'HTTP://google.com\\@apple.com')
.end(done)
Expand Down
Loading