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

[0.10] Ban javascript URLs in @lexical/link #4342

Merged
merged 6 commits into from
Apr 18, 2023
Merged

[0.10] Ban javascript URLs in @lexical/link #4342

merged 6 commits into from
Apr 18, 2023

Conversation

acywatson
Copy link
Contributor

More targeted approach to #4329

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 17, 2023
@vercel
Copy link

vercel bot commented Apr 17, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2023 11:47pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2023 11:47pm

@acywatson acywatson mentioned this pull request Apr 17, 2023
Closed
@github-actions
Copy link

github-actions bot commented Apr 17, 2023 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.94 KB (0%) 539 ms (0%) 353 ms (+103.91% 🔺) 891 ms
packages/lexical-rich-text/dist/LexicalRichText.js 37.8 KB (0%) 757 ms (0%) 490 ms (-14.07% 🔽) 1.3 s
packages/lexical-plain-text/dist/LexicalPlainText.js 37.78 KB (0%) 756 ms (0%) 317 ms (-19.66% 🔽) 1.1 s

zurfyx
zurfyx reviewed Apr 17, 2023
View reviewed changes
packages/lexical-link/src/index.ts
Comment on lines +169 to +171
if (!SUPPORTED_URL_PROTOCOLS.has(parsedUrl.protocol)) {
return 'about:blank';
}
Copy link
Member

Choose a reason for hiding this comment

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

What's your opinion on the file: protocol? Is it bad to support it? I understand the concerns around javascript: but not sure how strict we want to be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have an opinion, tbh. This copies Quill, which seems like a sensible place to start since people presumably have been living with that for a while without any serious problems. My thought is to start with this and then expand. The only issue I have is that there's not really a good way to notify a user when this happens. Like, theoretically the URL could pass the user-supplied validation at the plugin level, and then fail here, which is not ideal, but probably pretty uncommon.

fantactuka
fantactuka approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@fantactuka fantactuka left a comment

Choose a reason for hiding this comment

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

👍

@acywatson acywatson changed the title Ban javascript URLs in @lexical/link [0.10] Ban javascript URLs in @lexical/link Apr 18, 2023
@acywatson acywatson merged commit 6017410 into main Apr 18, 2023
@acywatson acywatson mentioned this pull request Apr 19, 2023
Merged
@fantactuka fantactuka deleted the sani-links-2 branch July 6, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zurfyx zurfyx zurfyx approved these changes

@fantactuka fantactuka fantactuka approved these changes

@tylerjbainbridge tylerjbainbridge Awaiting requested review from tylerjbainbridge

@thegreatercurve thegreatercurve Awaiting requested review from thegreatercurve

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@acywatson @fantactuka @zurfyx @facebook-github-bot