-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
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
Fix ci after location patch #5552
Conversation
86f60dd
to
2135b33
Compare
2135b33
to
499a8c1
Compare
New habits are hard to break. I used template literal strings which obviously didn't work in old versions. Should be fixed now. |
@@ -58,7 +58,7 @@ describe('res', function(){ | |||
}); | |||
|
|||
request(app) | |||
.get('/?q=http://google.com\\@apple.com') | |||
.get('/?q=http://google.com' + encodeURIComponent('\\@apple.com')) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I am not sure why appveyor and coveralls did not trigger here but I am going to merge anyway. |
# [1.26.0](v1.25.2...v1.26.0) (2024-04-02) ### Upgrade * Bump express from 4.18.2 to 4.19.2 (#552) ([bd280ba](bd280ba)), closes [#552](#552) [expressjs/express#5552](expressjs/express#5552) [expressjs/express#5556](expressjs/express#5556) [expressjs/express#5527](expressjs/express#5527) [expressjs/express#5511](expressjs/express#5511) [expressjs/express#5510](expressjs/express#5510) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5551](expressjs/express#5551) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5032](expressjs/express#5032) [expressjs/express#5034](expressjs/express#5034) [expressjs/express#5027](expressjs/express#5027) [expressjs/express#5124](expressjs/express#5124) [expressjs/express#5119](expressjs/express#5119) [expressjs/express#5117](expressjs/express#5117) [expressjs/express#5113](expressjs/express#5113) [expressjs/express#5130](expressjs/express#5130) [expressjs/express#5131](expressjs/express#5131) [expressjs/express#5028](expressjs/express#5028) [expressjs/express#5137](expressjs/express#5137) [#5541](https://github.com/brafdlog/caspion/issues/5541) * Bump express from 4.19.2 in /ui-react (#551) ([8d6032d](8d6032d)), closes [#551](#551) [expressjs/express#5552](expressjs/express#5552) [expressjs/express#5556](expressjs/express#5556) [expressjs/express#5527](expressjs/express#5527) [expressjs/express#5511](expressjs/express#5511) [expressjs/express#5510](expressjs/express#5510) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5551](expressjs/express#5551) [expressjs/express#5541](expressjs/express#5541) [expressjs/express#5032](expressjs/express#5032) [expressjs/express#5034](expressjs/express#5034) [expressjs/express#5027](expressjs/express#5027) [expressjs/express#5124](expressjs/express#5124) [expressjs/express#5119](expressjs/express#5119) [expressjs/express#5117](expressjs/express#5117) [expressjs/express#5113](expressjs/express#5113) [expressjs/express#5130](expressjs/express#5130) [expressjs/express#5131](expressjs/express#5131) [expressjs/express#5028](expressjs/express#5028) [expressjs/express#5137](expressjs/express#5137) [#5541](https://github.com/brafdlog/caspion/issues/5541)
CI started failing with the previous push due to old
supertest
versions installed only in ci not correctly url encoding before making the request. By encoding it ourselves in the testsupertest
works as expected in both versions.