fix(isURL): block dangerous schemes like javascript: to prevent XSS #2618
Conversation
|
Although I like the change, I feel like this results in a breaking change if done by default. This should be an option that's turned off by default now and enabled by default in the next major release |
Im update PR for a new option (e.g. allow_unsafe_protocol) that defaults to true for now, preserving current behavior. |
|
You are not preserving current behavior, you are removing the existing Aren't we going to get a new CVE targeting this use-case if we don't release it with the other isURL fix? |
I agree that this behaviour should indeed be kept with the default options.
What would be the real security implications of parsing a |
|
Overall, this PR meaningfully hardens
Once these are resolved and tests pass, this will be a solid security improvement. |
There was a problem hiding this comment.
Overall, this is a valuable security improvement for isURL.
Key notes:
- Static analysis warnings caused by
'javascript:'literals need to be handled. - Check if blocking
mailto:is intentional — it was previously allowed. - The new
allow_unsafe_protocoloption makes sense but should default to preserving existing behavior.
Once tests are fixed, this will be a strong enhancement.
src/lib/isURL.js
Outdated
| return false; | ||
| if (!options.allow_unsafe_protocol) { | ||
| const lowerUrl = url.trim().toLowerCase(); | ||
| const dangerousSchemes = ['javascript:', 'data:', 'vbscript:', 'file:', 'blob:', 'mailto:']; |
There was a problem hiding this comment.
Nice addition to harden the URL validation
Two suggestions:
- The inclusion of
'javascript:'and similar literals seems to trigger CI’s static analysis ("Script URL is a form of eval"). You might avoid this by dynamically constructing the strings (e.g.'java' + 'script:') or pulling them from a constants file. - Consider moving
dangerousSchemesoutside the function scope or converting this logic into a single regex to improve clarity and performance.
- Introduce `allow_unsafe_protocol` (default: true) to preserve backward compatibility - Block javascript:, data:, vbscript:, file:, blob:, and mailto: when disabled - Move dangerous schemes to constant - Avoid static analysis warnings by splitting scheme literals - Add comprehensive tests
|
Thanks for the feedback! I’ve updated the PR: mailto: is allowed by default (allow_unsafe_protocol: true) to preserve existing behavior |
4 participants
This PR hardens the isURL validator by explicitly rejecting URLs that start with dangerous schemes such as javascript:, data:, vbscript:, file:, blob:, and mailto:—even when they lack ://. This closes a security gap where such URLs could bypass protocol validation and lead to XSS or open redirect vulnerabilities due to inconsistent parsing compared to browser behavior.
Checklist