feat: Add allowInsecureRedirect option #28
Conversation
legobeat
force-pushed
the
ssrf-allow-insecure-redirect
branch
5 times, most recently
from
06d32fc
to
bfd8422
Compare
There was a problem hiding this comment.
I have not unit tested, but changes are simple/minimal and look good.
Note: I don't see this change as breaking. If apps using this package start to break, it's because of incorrect protocol provided OR a potentially malicious redirection. Most concerning is https to http redirect. I'm not sure why http to https is a concern in the security advisory. In any case, this fix plugs the hole and will surface the problem if there is one.
|
Is there anything we can do to assist with getting this PR merged and a new version of |
|
I second S100's suggestion. Note: we have been running the version with this patch (made manually) without any issues in some of our products. |
- Addresses CVE-2023-28155 - Existing behavior allows malicios redirects between protocols - Set default behavior to disable this vector (breaking) - Add new option `allowInsecureRedirect` where `true` reverts to old behavior - Ported from request#3444
legobeat
force-pushed
the
ssrf-allow-insecure-redirect
branch
from
bfd8422
to
8625fb6
Compare
|
@nagash77 Have you had a chance to consider this? |
|
note that to properly resolve GHSA-p8p7-x288-28g6 this behaviour needs to be enabled by default - given the usecase of this library and that it's a security issue I think it would be fair to argue this is a bug and so valid to change in a patch version with it enabled by default (i.e. this PR, rather than #30) |
|
Hi @legobeat , we have not had a chance to review this PR. We are currently in a push to get a new major feature out the door but I will try and get some eyes on this in the next couple of weeks. I know that is probably longer than you were hoping for but it is the best we can do at the moment. This library is at the core of Cypress so changes here do have the potential to have wide ranging impacts so this will take awhile to ensure it is safe to merge this change. I will make sure to keep an eye on this PR so it does not get lost and we can get to a resolution one way or another. |
There was a problem hiding this comment.
Thanks for this contribution and sorry it's taken so long for us to get around to reviewing it. I discussed this with the team and we decided we want to go forward with this PR, with the more secure option as the default. Since it's breaking, we'll follow semantic versioning and release it as a new major version of this package.
chrisbreiding
changed the title
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Will there be a new release of cypress itself that bumps this requirement? https://github.com/cypress-io/cypress/blob/b17043102bcb7bb653efb6526238468c74b4d564/package.json#L75 |
|
Confusingly the Line 10 in c5bcf21 |
@MikeMcC399 That's true. Thanks for pointing that out. Since semantic-release doesn't commit a version change back to the repo after a release, we usually set the version to something that can't be mistaken or the real version, but looks like we didn't do that when this repo was converted to use semantic-release. I'm addressing this in #41. |
@lukehsiao Yes. I'm addressing that in this PR. |
PR Checklist:
npm testlocally and all tests are passing.PR Description
allowInsecureRedirectwheretruereverts to old behaviorallowInsecureRedirect: truefor applicable casesA non-breaking (and therefore not-safe-by-default) alternative in #30.