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(extension-link): use whitelist for allowed href values #5160

Merged

Conversation

chroth
Copy link
Contributor

@chroth chroth commented May 16, 2024

Changes Overview

Link href values needs to be sanitized to not result in XSS vulnerabilities.

Implementation Approach

Link href values only allows whitelisted patterns for url, that are known to not be dangerous.

Testing Done

The link extension spec has been updated to cover both positive and negative tests.

Verification Steps

Link extension still functions as expected when testing with valid links, such as relative, absolute, with different schemes etc.

Additional Notes

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

Copy link

netlify bot commented May 16, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 738c436
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/664621edbbeefb000876bcd3
😎 Deploy Preview https://deploy-preview-5160--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nperez0111 nperez0111 marked this pull request as ready for review May 16, 2024 15:10
@nperez0111
Copy link
Contributor

Thanks for your contribution @chroth I've went ahead and added a couple more tests and merged some of the changes from #5157

Thank you as well @Nantris for your contribution here.

@nperez0111 nperez0111 merged commit 9df8737 into ueberdosis:main May 16, 2024
14 checks passed
@nperez0111 nperez0111 mentioned this pull request May 16, 2024
5 tasks
@Nantris
Copy link
Contributor

Nantris commented May 16, 2024

Thanks to all for their work on this. Based on the code I imagine that this properly prevents javascript: hrefs. I'd submit a PR to add that test case but I'm not 100% clear that simply adding that line would work properly (eg after this line https://github.com/chroth/tiptap/blob/738c436a9ff4af39d1d6abd52208eb7d46616106/tests/cypress/integration/extensions/link.spec.ts#L186)

Is this (or will this be) backported to older versions?

@Nantris
Copy link
Contributor

Nantris commented May 25, 2024

Can anyone advise whether this will be backported? I understand the desire is probably not to continue maintaining older versions, but with this being a security issue and a number of outstanding issues blocking upgrade paths, I think it should be planned to be backported at least a few versions.

f2c-ci-robot bot pushed a commit to halo-dev/halo that referenced this pull request Jun 26, 2024
#### What type of PR is this?

/kind improvement
/area editor
/milestone 2.17.x

#### What this PR does / why we need it:

在用户设置 iframe 相关的 src 时,检测设置的链接是否符合白名单。如果不符合则不允许设置。

see ueberdosis/tiptap#5160

#### How to test it?

测试在 iframe 中的 src 输入 `javascript: alert("1")` 时是否会触发 javascript

#### Does this PR introduce a user-facing change?
```release-note
处理默认编辑器中 iframe 标签的 src 属性可能存在的风险
```
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

4 participants