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: strip sensitive headers on redirect to different origin #273

Merged
merged 3 commits into from
May 12, 2022

Conversation

rexxars
Copy link
Member

@rexxars rexxars commented May 11, 2022

I originally sent a PR for this as #271, but it fixed a number of issues related to the redirect handling which could be considered breaking - and also added a dependency. After giving @joeybaker's review some thought, I am really eager to get a patch release out for this issue, as I don't want to leave the entire 2.x range with a security issue.

This PR attempts to address only the security issue, and does so in a way that should be backwards compatible. I still think we should improve the redirect handling overall, so I have renamed #271 to be more descriptive about this.

In other words, I want to merge this PR first, release a patch release on the 2.x range (potentially also backporting it to 1.x). Later on, I want to merge #271 and do a major release for that - but there is no rush with that one.

Copy link
Contributor

@joeybaker joeybaker left a comment

Choose a reason for hiding this comment

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

A few minor suggestions for you, but this seems to do the thing. Ironic how much more code it is :P

lib/eventsource.js Outdated Show resolved Hide resolved
lib/eventsource.js Show resolved Hide resolved
@rexxars rexxars merged commit e411e73 into master May 12, 2022
@rexxars rexxars deleted the fix/redirect-drop-sensitive branch May 12, 2022 04:31
@MatanBobi
Copy link

Hi @rexxars! Thanks for the hard work on this one.
I saw you published v1.1.1 also. Does that mean you backported this fix to support 1.x too? I'm asking because Snyk and the vulnerabilities database say to upgrade to version 2.

Thanks again :)

@rexxars
Copy link
Member Author

rexxars commented May 16, 2022

Hi @rexxars! Thanks for the hard work on this one. I saw you published v1.1.1 also. Does that mean you backported this fix to support 1.x too? I'm asking because Snyk and the vulnerabilities database say to upgrade to version 2.

Thanks again :)

Yes, I backported it to 1.1.1 in case people are still on 1.x and cannot upgrade for some reason. Generally we don't maintain the 1.x range any longer, but felt like patching this was worthwhile.

@MatanBobi
Copy link

Yes, I backported it to 1.1.1 in case people are still on 1.x and cannot upgrade for some reason. Generally we don't maintain the 1.x range any longer, but felt like patching this was worthwhile.

Thanks for thinking about that :)
I reached out to both Snyk and the vulnerabilities database to update that 1.1.1 also contains a fix.

This was referenced Jun 10, 2022
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.

6 participants