Skip to content

Conversation

@manuelMarkDenver
Copy link
Contributor

🛠️ Fix: Prevent URL validation bypass in isURL

Summary
This PR addresses GHSA-9965-vmph-33xx, a vulnerability in the isURL function where the existing logic used split("://") to detect the protocol. This approach fails to correctly identify schemes that do not include // (e.g., javascript:), allowing malicious URLs to bypass validation checks.

What’s Changed

  • Replaced the split("://") parsing logic with a regex-based approach that correctly matches URI schemes per RFC 3986
  • Tracks whether a protocol was explicitly present and correctly handles // only as a protocol-relative indicator when no protocol is present.
  • Preserves existing behavior for options like require_protocol and allow_protocol_relative_urls.

Why This Fix Matters

  • Prevents bypasses where URLs like javascript:alert(1) or other non-// schemes could previously be accepted.
  • Strengthens URL validation security without introducing breaking changes.
  • Maintains compatibility with existing valid URL patterns (e.g., http://example.com, ftp://host, etc.).

Additional Notes

  • All lint checks pass (npm run lint).
  • Existing test cases continue to pass.
  • Behavior remains backward-compatible with previous valid URLs.

✅ This PR helps mitigate the URL validation bypass vulnerability and improves the security of the isURL utility without breaking existing implementations.

const protocol_match = url.match(/^([a-z][a-z0-9+\-.]*):/i);
const hadExplicitProtocol = !!protocol_match;

if (protocol_match) {
Copy link

Choose a reason for hiding this comment

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

why not hadExplicitProtocol ?

Choose a reason for hiding this comment

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

To go with this suggestion and the one above to retain naming consistency

Suggested change
if (protocol_match) {
if (had_explicit_protocol) {

@WikiRik
Copy link
Member

WikiRik commented Oct 14, 2025

CI failure is unrelated to your PR. We're not using a package-lock.json in this project, and one of the underlying dependencies is not working with Node 6 anymore. That should also be fixed, but in a different PR

@fredericfalliere
Copy link

Nice work @manuelMarkDenver
I think a unit test could be a great addition to this PR, to demonstrate 1/ no regression 2/ CVE is adressed

// Replaced the 'split("://")' logic with a regex to match the protocol.
// This correctly identifies schemes like `javascript:` which don't use `//`.
const protocol_match = url.match(/^([a-z][a-z0-9+\-.]*):/i);
const hadExplicitProtocol = !!protocol_match;
Copy link

Choose a reason for hiding this comment

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

To maintain consistency in naming: had_explicit_protocol

Choose a reason for hiding this comment

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

Suggested change
const hadExplicitProtocol = !!protocol_match;
const had_explicit_protocol = !!protocol_match;

@Fafly
Copy link

Fafly commented Oct 14, 2025

Link to this issue: #2600

@qwertychouskie
Copy link

This looks (at least partially) LLM-written. While LLMs are useful, using them for security-sensitive code can often result in subtle bugs. Perhaps there should be a bit extra thorough review of this PR to make sure that a. it fully fixes the issue, and b. doesn't introduce any other issues.

@DysektAI
Copy link

This looks (at least partially) LLM-written. While LLMs are useful, using them for security-sensitive code can often result in subtle bugs. Perhaps there should be a bit extra thorough review of this PR to make sure that a. it fully fixes the issue, and b. doesn't introduce any other issues.

While I agree with you, LLM's do make mistakes and aren't perfect. I am sure the maintainers know this and also know people make mistakes. That's where we help reviewing, testing, and typically use extensive CI/CD testing.

My point: All PR's should be handled without bias, extensively tested + reviewed thoroughly.

@colekerr
Copy link

CI failure is unrelated to your PR. We're not using a package-lock.json in this project, and one of the underlying dependencies is not working with Node 6 anymore. That should also be fixed, but in a different PR

@WikiRik Would it be appropriate for said PR to, say, drop semver for a subsequent minor release and pin the versions of @babel/* to 7.12.7, before Node 6 support was dropped by them?

I figured it would be a quicker solution to maintaining node 6 support for validator.jsv13.x.x, unless you're already planning on immediately dropping Node 6 yourselves.

@WikiRik
Copy link
Member

WikiRik commented Oct 15, 2025

CI failure is unrelated to your PR. We're not using a package-lock.json in this project, and one of the underlying dependencies is not working with Node 6 anymore. That should also be fixed, but in a different PR

@WikiRik Would it be appropriate for said PR to, say, drop semver for a subsequent minor release and pin the versions of @babel/* to 7.12.7, before Node 6 support was dropped by them?

I figured it would be a quicker solution to maintaining node 6 support for validator.jsv13.x.x, unless you're already planning on immediately dropping Node 6 yourselves.

Pinning @babel/* to 7.12.7 would be an option, but since the actual issue is more downstream I opened #2609 to remove Node 6 from testing. That does not change the build itself.

With these issues occuring more and more I want to release a new major release soon that just drops support for EoL Node versions.

@WikiRik
Copy link
Member

WikiRik commented Oct 15, 2025

I will close this in favour of #2608. If people want to review, feel free to do so on that PR

@WikiRik WikiRik closed this Oct 15, 2025
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.

9 participants