fix(extension-link): make the javascript link detection case insensitive #5153
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/extension-link/src/link.ts
Outdated
| @@ -163,7 +163,7 @@ export const Link = Mark.create<LinkOptions>({ | |||
| renderHTML({ HTMLAttributes }) { | |||
| // False positive; we're explicitly checking for javascript: links to ignore them | |||
| // eslint-disable-next-line no-script-url | |||
| if (HTMLAttributes.href?.startsWith('javascript:')) { | |||
| if (HTMLAttributes.href?.toLowerCase().startsWith('javascript:')) { | |||
There was a problem hiding this comment.
I just tested this locally and it looks like you can fool it with leading white space too. I think that besides the protocol (e.g. https://) a colon is invalid so may this should be an .includes instead of a .startsWith
There was a problem hiding this comment.
It gets tricky if the URL uses basic-auth and includes the username and password (i.e., https://javascript:my_password@example.com/ for a user named javascript would be a false positive when using .includes). I think it'd be better to .trim() before calling .startsWith()
Edit: and a colon is valid in the path part of an URL, as long as it's not the first part of a relative path according to RFC 3986. (BTW, it's also used as a delimiter for the port number if needed.)
There was a problem hiding this comment.
I think the correct way to do this would be to extract the scheme (everything up to the first colon), trim it, and then do a case insensitive match for javascript. I'll push something later today.
There was a problem hiding this comment.
Hello! Happy to see that you're looking into fixing this. ❤️ However I think there are a few issues with your approach here.
- it would allow javascript protocol when it includes \x01-\x20 characters inside the
javascript: - it would allow javascript protocol when it includes whitespace characters before the colon
javascript\n:.
It's really hard to get the blacklisting approach right. I made a PR where I try to use a whitelist approach instead, see here: #5160 It's still draft/WIP though.
I've taken inspiration from DOMPurify (using it might in general be a good approach for tiptap).
I've also created test cases from PortSwiggers XSS cheat sheet: https://portswigger.net/web-security/cross-site-scripting/cheat-sheet#protocols
|
will this be released as soon as merged? |
…whether it's javascript
julmud
force-pushed
the
bugfix/case-insensitive-sanitization
branch
from
f95429c
to
0ac4249
Compare
|
We ended up resolving it with this #5160 But we appreciate your contribution, your work was helpful in the final fix. We did this a bit strangely because of the multiple PRs and the urgency of it |
Changes Overview
The fix for the XSS vulnerability in #4602 does not make the check on javascript URL in a case insentive way.
Implementation Approach
Convert the href attribute to lowercase before testing if it starts with "javascript:"
Testing Done
Increased the Cypress test coverage and ran it.
Verification Steps
Verify non-breaking on link and youtube extension, and ensure the new tests pass.
Checklist
feat: Implement new featureorchore(deps): Update dependencies)Related Issues
#3673
#4600
#4602
#5157