-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(link): respect custom protocols #5468 #5470
Conversation
🦋 Changeset detectedLatest commit: d766bd5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@nperez0111 We've finally integrated the latest Tiptap version with our product, and unfortunately this change (actually, the main culprit is the XSS vulnerability fix change) is causing issues for us. We've started getting loads of support tickets of people complaining that custom links copied from the most various apps could no longer be used to link text in the editor. To give a few examples, apps such as Airmail, Bear, Obsidian, give you links with custom protocols (i.e. I'm wondering if it would make sense to rethink the approach here, and introduce a disallow list (with sane defaults) instead of an allow list? The downside is that
What do you think? Alternatively, here are a few more ideas (mutually exclusive):
As an aside, I think there's a bit of a disconnect between the default |
Hey @rfgamaral can you make a new issue for this? I think it warrants some discussion on the tradeoffs & I'm not sure that this is the best place for it. I don't have the bandwidth right now to give my specific thoughts on it but I know it is important. My not very well thought out thought is that we should just expose an API to let you choose how you want to handle this sort of a thing (I think there is an existing validate method that we could probably re-use for this). But I do think that the default behavior is quite valid & safe for a web-based editor that isn't really going to deal with custom protocols as much, but I of course understand your concern & it should be accounted for. |
@nperez0111 Done: #5564. Let's continue the discussion there. |
Changes Overview
When we fixed a XSS vuln, we inadvertently broke the ability to use custom protocols, this PR resolves that by allowing additional custom protocols to be considered valid and not stripped out
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
#5468