fix(isURL): improve protocol detection. Resolves CVE-2025-56200

[theofidry]

This PR is a fix for GHSA-9965-vmph-33xx.

Cherry-picked the change from #2603 but a test first to capture the vulnerability was created first. It also modifies the original patch as it was making any URL with authentication information no longer valid.

Closes #2603 and #2600.

[WikiRik]

Thanks for working on this! I was also looking into this, and I came up with this set of invalid URLs that we probably should test for with the default settings. (Disclaimer; these were partially generated by AI.)

        // the following tests are because of CVE-2025-56200
        /* eslint-disable no-script-url */
        "javascript:alert(1);a=';@example.com/alert(1)'",
        'JaVaScRiPt:alert(1)@example.com',
        'javascript:%61%6c%65%72%74%28%31%29@example.com',
        'javascript:/* comment */alert(1)@example.com',
        'javascript:var a=1; alert(a);@example.com',
        'javascript:alert(1)@user@example.com',
        'javascript:alert(1)@example.com?q=safe',
        'data:text/html,<script>alert(1)</script>@example.com',
        'vbscript:msgbox("XSS")@example.com',
        '//evil-site.com/path@example.com',
        'http://evil-site.com@example.com/',
        'ｊａｖａｓｃｒｉｐｔ:alert(1)@example.com',
        /* eslint-enable no-script-url */

Could you add these (or a variation of these) to this PR?

[theofidry]

@WikiRik from what I can see the following are marked as valid:

javascript:%61%6c%65%72%74%28%31%29@example.com
//evil-site.com/path@example.com
http://evil-site.com@example.com/
ｊａｖａｓｃｒｉｐｔ:alert(1)@example.com

I do not know if it's correct or not

[WikiRik]

Thanks for checking that!

Why is javascript:%61%6c%65%72%74%28%31%29@example.com passing? That's strange to me.
The three others can be removed for now. We'll look into those (or variations of those) in future rework.

To get your CI passing; you can remove Node 6 from the versions we test on, as I did in #2609

[theofidry]

The flow of the current for javascript:%61%6c%65%72%74%28%31%29@example.com is:

• detects javascript as a potential protocol
• checks what is after the colon, which is %61%6c%65%72%74%28%31%29@example.com
• checks what is before the @, which is %61%6c%65%72%74%28%31%29
• %61%6c%65%72%74%28%31%29 is a valid authentication (it only contains alphanumeric and %)
• Because the above is valid authentication, it does not treat javascript: as a protocol but instead treats it as if it was username:password@example.com

Not sure what is the best way to fix this.

[WikiRik]

Let's skip that one as well for now. Running the original CVE in a browser console gives me the alert and this one doesn't.

We don't do XSS sanitization in this library, so that shouldn't be a big issue

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6f436be) to head (376c84f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2608   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          114       114           
  Lines         2536      2562   +26     
  Branches       642       649    +7     
=========================================
+ Hits          2536      2562   +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[theofidry]

@WikiRik updated.

Also worth mentioning it is only a risk when the protocol is not required, as if it is then it is checked against the allowed protocols.

So may be worth adding a strong recommendation in the docs or making the protocol required the default

[WikiRik]

Yes, we will make the protocol required by default but that will be a breaking change so we do that in the next major release. A strong recommendation might be good. Is that something you can add to the README?

[benedery]

Hey @theofidry @WikiRik , any ETA on when its going to be merged & released?

[WikiRik]

@theofidry codecov is complaining about some missing and partially covered lines. Can you check if those branches should be touched, and do one of the following;

• add additional test cases
• remove the branch of code

I hope to be able to review this PR properly this weekend, suggestions from the community are appreciated on this

[scottgigante-hubflow]

Some suggested test cases that might help with coverage

[theofidry]

I can't check it out locally atm, but if you do a PR against my changes I can merge it

[scottgigante-hubflow]

theofidry#2

[a-safadi]

any ETA on when its going to be merged & released?

[IdanAdar]

What else is this PR missing?

[heatxsink]

Does this PR need a bit more test coverage to pass?

[WikiRik]

There is still 1 line partially covered, and I haven't had the time to properly review the proposed changes

[henri-extravagant]

@theofidry Here's a PR to handle the partially covered line

[GodsonLeigh]

Thank you everyone for your work on getting this ready, I have been following the progress here since the vulnerability was found. Do we know how long this will now take to be released as this transient dependency is blocking a number of work items on our (admittedly rigid) pipeline.
@WikiRik @theofidry

[WikiRik]

Two suggestions, apart from that I think this is fine to merge. After release we can look into incorporating the native URL interface, but that's more work than needs to be done to fix this CVE

[WikiRik]

I'll prepare a release later today that includes this fix

[dmonserr]

@WikiRik any update on this? I see it was merged into the release branch but not yet approved by @profnandaa.

[profnandaa]

@WikiRik any update on this? I see it was merged into the release branch but not yet approved by @profnandaa.

Sorry, this is implicitly approved, since we were on some email discussions about it.

[gboisvert-beyondtrust]

@WikiRik @profnandaa Is there an ETA for distribution of this security fix?

[Moumouls]

Hi all i sent the PR to mark the 13.15.20 as patched: github/advisory-database#6355 to update the GH advisory database

[typenoob]

Hi, could this security fix backport to v13.12.x?

[profnandaa]

Hi, could this security fix backport to v13.12.x?

Any reasons you have locked on v13.12.x, where there any breaking changes after? /cc. @WikiRik

[typenoob]

#2421 This is the breaking change. @profnandaa

I know I could use loose mode to handle it but class-validator has not exposed the option yet.
typestack/class-validator#2648

[WikiRik]

@typenoob I still need to reply in depth to the issue that was created, but in short; it was not the best idea to publish that change in 13.x.x. I didn't know that you weren't able to use the loose mode in class-validator. I'll see if we want to make the loose mode the default setting so that people can update.

Security wise; if you require protocols and make them mandatory (possibly with preprocessing to add "https://" in the application if the user did not provide it), you are safe from this vulnerability.

[WikiRik]

@typenoob I still need to reply in depth to the issue that was created, but in short; it was not the best idea to publish that change in 13.x.x. I didn't know that you weren't able to use the loose mode in class-validator. I'll see if we want to make the loose mode the default setting so that people can update.

Security wise; if you require protocols and make them mandatory (possibly with preprocessing to add "https://" in the application if the user did not provide it), you are safe from this vulnerability.