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: stronger enforcement of normalizeLink #6728

Merged
merged 7 commits into from Jun 29, 2023

Conversation

vjeux
Copy link
Contributor

@vjeux vjeux commented Jun 29, 2023

We should only accept links that start with https?:// and if it doesn't then add it. Do not allow for relative links.

Test Plan:
On excalidraw.com create a rectangle with a link javascript://%0aalert(document.domain) and save it: javas.excalidraw.zip

In this PR, load the file above and see that the link has https:// added before:
image

In this PR, create a rectangle with the above link. Make sure that https:// is added before.

We should only accept links that start with https?:// and if it doesn't then add it.
@vercel
Copy link

vercel bot commented Jun 29, 2023

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

Name Status Preview Updated (UTC)
excalidraw ✅ Ready (Inspect) Visit Preview Jun 29, 2023 10:11am
excalidraw-package-example ✅ Ready (Inspect) Visit Preview Jun 29, 2023 10:11am
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs ⬜️ Ignored (Inspect) Jun 29, 2023 10:11am

@vjeux vjeux requested review from dwelle and ad1992 June 29, 2023 05:10
@vjeux vjeux changed the title Stronger enforcement of normalizeLink fix: stronger enforcement of normalizeLink Jun 29, 2023
@ad1992
Copy link
Member

ad1992 commented Jun 29, 2023

@vjeux we were earlier allowing only https but users might want to add different protocols eg ftp://, file:// and may be some custom as mentioned in #4620 (comment) so we should instead whitelist these protocols instead of disallowing all except https, does that make sense ?

@zsviczian
Copy link
Collaborator

I did not check the code change in details, but please make sure you continue to allow markdown and [[wiki]] links.

@dwelle
Copy link
Member

dwelle commented Jun 29, 2023

I did not check the code change in details, but please make sure you continue to allow markdown and [[wiki]] links.

we'll need to add a props.normalizeLink so host apps can do what they want, else it's outside the scope of the package to figure out and make decisions for host apps like this.

As for ftp:// and file:// protocols, I'm now not sure if it's actually a good idea to allow this as it make be another attack vector.

@dwelle
Copy link
Member

dwelle commented Jun 29, 2023

we'll need to add a props.normalizeLink so host apps can do what they want, else it's outside the scope of the package to figure out and make decisions for host apps like this.

but the issue is how to propagate props.normalizeLink into util helpers such as restore() — we have the same issue in the iframe PR.

@dwelle
Copy link
Member

dwelle commented Jun 29, 2023

Ok, so instead I've used a sanitizer from Braintree (https://github.com/braintree/sanitize-url), which is permissive enough to allow custom formats such as markdown (@zsviczian you'll need to sanitize the url yourself once you parse it and render, unless you're using a markdown renderer that does it for you). If needed, we export our link normalize from the package as normalizeLink.

This way we don't have to support props.normalizeLink which in turn allows us to keep doing the normalization on restore().

That said, restore() is not guaranteed to be run in every case, so from security perspective we still want to normalize in runtime when rendering the link or passing it upstream.

Further, we still want to support relative links & retain opening relative links in the same tab.

@dwelle
Copy link
Member

dwelle commented Jun 29, 2023

Thanks @vjeux!

@dwelle dwelle merged commit b33fa6d into excalidraw:master Jun 29, 2023
7 checks passed
dwelle added a commit that referenced this pull request Aug 16, 2023
Co-authored-by: dwelle <luzar.david@gmail.com>
@azu azu mentioned this pull request Aug 17, 2023
alswl pushed a commit to alswl/excalidraw that referenced this pull request Nov 15, 2023
Co-authored-by: dwelle <luzar.david@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants