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

Ssrf fix #3444

Closed
wants to merge 5 commits into from
Closed

Ssrf fix #3444

wants to merge 5 commits into from

Conversation

SzymonDrosdzol
Copy link

PR Checklist:

  • [DONE] I have run npm test locally and all tests are passing.
  • [DONE] I have added/updated tests for any new behavior.
  • [DONE] If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

This pull request is a fix to CVE-2023-28155.

It introduces a new configuration option allowInsecureRedirects, turned off by default. The default configuration leaves library users protected from exploiting CVE-2023-28155. When the option gets turned on, the cross-protocol redirects will be allowed if library user decides it's safe and required in their case.

@phawxby
Copy link

phawxby commented Mar 16, 2023

@mikeal @jabrena @mmalecki @eiriksm @reconbot any possibility one of you is available to review and merge this? Our build process is currently broken because of audit failures. Thanks.

lib/redirect.js Outdated Show resolved Hide resolved
legobeat added a commit to legobeat/metamask-extension that referenced this pull request Mar 17, 2023
legobeat added a commit to legobeat/metamask-extension that referenced this pull request Mar 17, 2023
SzymonDrosdzol and others added 3 commits March 17, 2023 10:09
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Co-authored-by: Kevin van Rijn <6368561+kevinvanrijn@users.noreply.github.com>
Gudahtt pushed a commit to MetaMask/metamask-extension that referenced this pull request Mar 17, 2023
* security: patch request for CVE-2023-28155

GHSA-p8p7-x288-28g6

Ported from request/request#3444

* add iyarc exclusion
danjm pushed a commit to MetaMask/metamask-extension that referenced this pull request Mar 17, 2023
* security: patch request for CVE-2023-28155

GHSA-p8p7-x288-28g6

Ported from request/request#3444

* add iyarc exclusion
NidhiKJha pushed a commit to MetaMask/metamask-extension that referenced this pull request Mar 20, 2023
* security: patch request for CVE-2023-28155

GHSA-p8p7-x288-28g6

Ported from request/request#3444

* add iyarc exclusion
@tcherel
Copy link

tcherel commented Mar 22, 2023

Can we expect an updated request release with this fix soon?
If yes, how soon?
Sorry, just trying to figure out how fast we need to move out of request completely.
Thanks.

@khitrenovich
Copy link

khitrenovich commented Mar 22, 2023

There were no non-doc/tools changes in the repo since 2018 and no commits at all for the last 3 years.
I wouldn't expect any fix coming from here.

Our own dependency on request came from jsforce (which is not gonna get fixed soon, as it looks like), so we migrated to the still-maintained fork from Cypress via Yarn forced resolutions mechanism, and it worked like a charm.


Update: in case somebody missed that, request is deprecated since 2020.

@danilogco
Copy link

danilogco commented Mar 22, 2023

Yeah... It seems like it's not being maintained/updated anymore a long time.

@tcherel
Copy link

tcherel commented Mar 22, 2023

Thanks.
Yes, I understand that request is not maintained any longer but since there was this PR created and it got reviewed and approved, I though that, may be, something was going to be released.

@suside
Copy link

suside commented Mar 23, 2023

@khitrenovich CVE-2023-28155 was reported on Mar 16, 2023 and the last commit in fork from Cypress is from Jan 11, 2023. It doesn't look like this has been fixed in their fork 🤨

@khitrenovich
Copy link

@khitrenovich CVE-2023-28155 was reported on Mar 16, 2023 and the last commit in fork from Cypress is from Jan 11, 2023. It doesn't look like this has been fixed in their fork 🤨

@suside You are right... Just opened an issue there, let's see how they respond.

@phawxby
Copy link

phawxby commented Mar 24, 2023

@SzymonDrosdzol it might be worth you opening the same CVE against the cypress fork too

legobeat pushed a commit to legobeat/request that referenced this pull request Mar 25, 2023
- 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
Copy link

I have made two PRs on cypress:

It seems they have an extended test suite handling more complex redirects so if anyone feels up for extending my PRs addressing those cases to get them up to standard, feel free.

@karanr1990
Copy link

is there any alternate library of sforcejs where we do not have dependency on request library ?

@jeremieSTC
Copy link

Can we expect an updated request release with this fix soon?

If yes, how soon?

Sorry, just trying to figure out how fast we need to move out of request completely.

Thanks.

It would be great if a new version could be released including the fix. I see there was a new version a couple of days ago with the new version of xml2js so I have hope.

@legobeat
Copy link

legobeat commented Apr 23, 2023

@jeremieSTC A couple of years, you mean? See above for alternatives - request itself is unmaintained and I wouldn't be hoping to see further updates.

@reconbot
Copy link
Contributor

reconbot commented Apr 23, 2023

Per the readme and npm deprecation warning.

As of Feb 11th 2020, request is fully deprecated. No new changes are expected to land. In fact, none have landed for some time.

Please use alternative libraries.

@reconbot reconbot closed this Apr 23, 2023
legobeat pushed a commit to legobeat/request that referenced this pull request Aug 1, 2023
- 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 pushed a commit to legobeat/request that referenced this pull request Aug 1, 2023
- 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
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.