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.

Sign up for GitHub

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

Jump to bottom

fix(extension-link): make the javascript link detection case insensitive #5153

Closed
wants to merge 3 commits into from
Closed

fix(extension-link): make the javascript link detection case insensitive #5153

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3afb49e
fix(extension-link): make the javascript link detection case insensitive
May 15, 2024
b79de07
fix(extension-link): trim the URL before checking the scheme
May 15, 2024
0ac4249
fix(extension-link): extract the scheme from the URL before checking …
May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/extension-link/src/link.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oof good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@julmud julmud May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for chiming in @chroth we are looking into the right approach for this sensitive topic and your contribution made us re-think some things

There are three open PRs for XSS vuln fixes:
#5157
#5160
#5153

// strip out the href
return ['a', mergeAttributes(this.options.HTMLAttributes, { ...HTMLAttributes, href: '' }), 0]
}
Expand Down
82 changes: 45 additions & 37 deletions tests/cypress/integration/extensions/link.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,45 +20,53 @@ describe('extension-link', () => {
}
const getEditorEl = () => document.querySelector(`.${editorElClass}`)

it('does not output src tag for javascript schema', () => {
editor = new Editor({
element: createEditorEl(),
extensions: [
Document,
Text,
Paragraph,
Link,
],
content: {
type: 'doc',
content: [
{
type: 'paragraph',
content: [
{
type: 'text',
text: 'hello world!',
marks: [
{
type: 'link',
attrs: {
// We have to disable the eslint rule here because we're trying to purposely test eval urls
// eslint-disable-next-line no-script-url
href: 'javascript:alert(window.origin)',
},
},
],
},
],
},
const invalidUrls = [
// We have to disable the eslint rule here because we're trying to purposely test eval urls
// eslint-disable-next-line no-script-url
'javascript:alert(window.origin)',
// eslint-disable-next-line no-script-url
'jAvAsCrIpT:alert(window.origin)',
]

invalidUrls.forEach(url => {
it('does not output src tag for javascript schema', () => {
editor = new Editor({
element: createEditorEl(),
extensions: [
Document,
Text,
Paragraph,
Link,
],
},
})
content: {
type: 'doc',
content: [
{
type: 'paragraph',
content: [
{
type: 'text',
text: 'hello world!',
marks: [
{
type: 'link',
attrs: {
href: url,
},
},
],
},
],
},
],
},
})

// eslint-disable-next-line no-script-url
expect(editor.getHTML()).to.not.include('javascript:alert(window.origin)')
// eslint-disable-next-line no-script-url
expect(editor.getHTML()).to.not.include('javascript:alert(window.origin)')

editor?.destroy()
getEditorEl()?.remove()
editor?.destroy()
getEditorEl()?.remove()
})
})
})
2 changes: 2 additions & 0 deletions tests/cypress/integration/extensions/youtube.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ describe('extension-youtube', () => {
// We have to disable the eslint rule here because we're trying to purposely test eval urls
// eslint-disable-next-line no-script-url
'javascript:alert(window.origin)//embed/',
// eslint-disable-next-line no-script-url
'jAvAsCrIpT:alert(window.origin)//embed/',
'https://youtube.google.com/embed/fdsafsdf',
'https://youtube.com.bad/embed',
]
Expand Down