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.

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

Fix XSS potential #5157

Closed
wants to merge 5 commits into from
Closed

Fix XSS potential #5157

wants to merge 5 commits into from

Conversation

Nantris
Copy link
Contributor

@Nantris Nantris commented May 15, 2024

Changes Overview

Fix XSS potential

Implementation Approach

Trim and lowercase href before comparing. Rely on getAttrs instead of CSS selector.

Testing Done

None

Verification Steps

Additional Notes

Given that this potential was already overlooked, it's essential that these changes be tested and examined more vigorously by the core team than they were in the past.

Sample case which previously allowed XSS (due to leading spaces and due to differences in capitalization): https://codesandbox.io/p/sandbox/sharp-hill-6k846v?file=%2Fsrc%2FApp.js%3A21%2C28

Checklist

  • I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
  • My changes do not break the library.
  • I have added tests where applicable.
  • I have followed the project guidelines.
  • I have fixed any lint issues.

Related Issues

#3673

trim and lowercase hrefs
Copy link

netlify bot commented May 15, 2024

Deploy Preview for tiptap-embed failed.

Name Link
🔨 Latest commit 911cf67
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/664542a03f928e00084ce4a2

@Nantris
Copy link
Contributor Author

Nantris commented May 15, 2024

@Nantris
Copy link
Contributor Author

Nantris commented May 15, 2024

@svenadlung @bdbch it's paramount that these changes be integrated as quickly as feasible to resolve XSS potential.

I didn't see any way to prevent parsing javascript: hrefs (note leading spaces) in the parseHTML without abandoning the CSS-selector approach. We could filter for javascript: anywhere in the href but I'm not certain that won't exclude potentially valid hrefs.

@Nantris
Copy link
Contributor Author

Nantris commented May 16, 2024

Not sure what mistake I've made in this PR that has it failing. Can anyone advise? Despite the high priority of this issue, our editor isn't even ready to ship, so I won't have much more time to work on this issue.

@bdbch
Copy link
Contributor

bdbch commented May 16, 2024

@Nantris happy to get his merged and released ASAP. The test failing is because the dom on getAttr can be a string OR dom element so .getAttribute is not working on a string.

I think you'd need to do a typecheck beforehand.

return [{
tag: 'a[href]',
getAttrs: dom => {
const href = dom.getAttribute('href')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here we need a typecheck as dom apparently can also be a string (at least typescript says so) so you can't get the href directly from a string object as getAttribute does not exist

@nperez0111
Copy link
Contributor

We resolved this with #5160

@nperez0111 nperez0111 closed this May 16, 2024
@nperez0111
Copy link
Contributor

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

@Nantris
Copy link
Contributor Author

Nantris commented May 16, 2024

Glad it's taken care of! Thanks for the quick action and words of appreciation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants