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

URLs fall victim to the default escape_html function's overagressive escaping #368

Open
iirelu opened this issue Dec 9, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@iirelu
Copy link

commented Dec 9, 2018

When setting a URL via a template variable, the obvious answer is to use <a href="{{ url }}">, but this is subtly wrong in a way that arguably doesn't really matter. Thanks to tera::escape_html escaping even / (as recommended by OWASP), what would otherwise look like <a href="/foo/bar">Example</a> instead looks like <a href="&#x2F;foo&#x2F;bar">Example</a>. This continues to work, at least on all modern browsers, but needlessly escaping unreserved characters in URLs is recommended against by everything I can find.

The obvious answer to this is to use {{ url | safe }}, but that's overkill, and potentially serves as another vector for bugs and attacks by letting all unescaped characters through. A filter like "url_safe" might work, but I don't know the answer.

@Keats

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2018

Hmm we could maybe add a boolean url param to the safe filter to only escape parts of it or have a url_safe as you mention would be nice.

@iirelu

This comment has been minimized.

Copy link
Author

commented Dec 11, 2018

Thinking more, I think an attr_escape would work best, something that marks it as safe and only escapes " and '.

@Keats

This comment has been minimized.

Copy link
Owner

commented Dec 12, 2018

I kind of like the idea of having a single escape personally, so we could add something like escape(type="js") if we want to have built-in support for escaping other format than HTML. It's already possible to give Tera your own escape fn but it means you cannot mix html/js rendering for example. Not sure how common this is.

@iirelu

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

Honestly, unless any evidence of lone forward slashes being able to single-handedly cause malformed HTML or be a possible attack vector can be found, I think it might just be best to not escape /s.

While looking for information about it I stumbled across this stackexchange post which offers a good rationale for escaping forward slashes. That does make things tougher.

Still, OWASP itself recommends a different set of escaping rules for "complex attributes like href, src, style, or any of the event handlers like onmouseover". Reading more, I see it actually explicitly states that URLs should be escaped differently from other attributes, meaning that once again I think a simple url filter could do the job. Sure, it could be longer like escape(type="url") but that's not the kind of thing that's fun to type dozens of times in a row, and this is a pretty safety-critical thing.

Another argument for a url filter is that it could possibly perform some URL validation, and print out warnings (or errors) when the given input is an invalid URL.

@Keats

This comment has been minimized.

Copy link
Owner

commented Jan 2, 2019

Can you write the potential doc of the url filter in the issue to make sure we agree on the design? Seems like it can/should be added with the next release.

@Keats Keats added the enhancement label Jan 3, 2019

@iirelu

This comment has been minimized.

Copy link
Author

commented Jan 8, 2019

Sorry, a bout of leukemia has knocked me out of action. I can't really help with this any more.

@Keats

This comment has been minimized.

Copy link
Owner

commented Jan 8, 2019

:o Sorry to hear that, hope you get better

I'll add it to the v1 later

@Keats

This comment has been minimized.

Copy link
Owner

commented Feb 4, 2019

Looks like the URL filter could use the https://docs.rs/percent-encoding/1.0.1/percent_encoding/ crate

@Keats Keats referenced this issue Jun 6, 2019

Open

v1 release #331

11 of 16 tasks complete
@Keats

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2019

I think I completely forgot that there already is a urlencode filter? So we can just do urlencode | safe for urls and it should be fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.