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.

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

merged 1 commit into from
Mar 20, 2024

Conversation

wesleytodd
Copy link
Member

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 test supertest works as expected in both versions.

@wesleytodd wesleytodd force-pushed the fix-ci-after-location-patch branch from 86f60dd to 2135b33 Compare March 20, 2024 16:37
@wesleytodd wesleytodd force-pushed the fix-ci-after-location-patch branch from 2135b33 to 499a8c1 Compare March 20, 2024 17:34
@wesleytodd
Copy link
Member Author

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'))
Copy link
Member

@jonchurch jonchurch Mar 20, 2024

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.

@wesleytodd
Copy link
Member Author

I am not sure why appveyor and coveralls did not trigger here but I am going to merge anyway.

@wesleytodd wesleytodd merged commit 11f2b1d into master Mar 20, 2024
48 checks passed
@wesleytodd wesleytodd deleted the fix-ci-after-location-patch branch March 20, 2024 22:11
github-actions bot pushed a commit to brafdlog/caspion that referenced this pull request Apr 2, 2024
# [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants