Skip to content

restrict link protocols in safe mode #230

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

Merged
merged 3 commits into from
Nov 8, 2016

Conversation

alanhamlett
Copy link
Contributor

@alanhamlett alanhamlett commented Nov 7, 2016

Related to #51 and #174.

When safe_mode is truthy, only allows http, https, and ftp protocols in links to prevent XSS.

@nicholasserra
Copy link
Collaborator

Nice, thank you

@nicholasserra
Copy link
Collaborator

@trentm We could probably use a release and new version. I think we're past due ha

@nicholasserra
Copy link
Collaborator

@alanhamlett I may revert this as #233 was brought to my attention. Should urls be unescaped with safe mode on? I think that this patch may need to be more specific in keeping urls unescaped.

@alanhamlett
Copy link
Contributor Author

Ok, we shouldn't escape urls when safe mode is on? I can submit a new PR with only the necessary changes, or all if it's reverted?

@nicholasserra
Copy link
Collaborator

Bringing @lopopolo in on this conversation as he made the report. I don't actually know what is the most "correct" way to handle this. aka either:

  1. Safe mode escapes everything including stuff in links
  2. Safe mode escapes everything except for what we want to exclude.

@lopopolo
Copy link

lopopolo commented Dec 15, 2016

I'd consider the way django handles autoescaping

Escapes a string’s HTML. Specifically, it makes these replacements:

< is converted to &lt;
> is converted to &gt;
' (single quote) is converted to &#39;
" (double quote) is converted to &quot;
& is converted to &amp;

This patch doesn't escape input; it urlencodes it. URL encoding should be applied to URL parameters. Not HTML content.

@strindhaug
Copy link
Contributor

strindhaug commented Dec 19, 2016

What is "safe_mode" supposed to do? I can't find any documentation about it.

If safe mode actually is supposed to break all links (as it does now), why bother converting the link syntax [a link](http://example.com) into an invalid link <a href="http://my-forum-domain.com/forum/forum/sometopic/http%3A%2F%2Fexample.com">a link</a> in the first place. Why not just strip the links completely, and replace it with the text [links not allowed] or something.

The same applies to images. Why not just remove images instead of leaving a broken image tag, for perfectly innocent image urls, if "safe_mode" is supposed to break all urls.

@alanhamlett
Copy link
Contributor Author

@strindhaug safe mode shouldn't break links or images, it should just urlencode them. I htmlencoded them to prevent injecting javascript into elements, but I'll switch to urlencoding to fix this.

@strindhaug
Copy link
Contributor

strindhaug commented Dec 19, 2016

@alanhamlett But url'encoding the whole url does break images and links, because changing http:// to http%3A%2F%2F changes the url from an absolute url to (almost guaranteed to be invalid) relative url.

Try adding this to test/tm-cases/basic_safe_mode.text

![ok img](http://example.com/image.gif?h=200&w=500)

[link ok](http://example.com)

and this to test/tm-cases/basic_safe_mode.html:

<p><img src="http://example.com/image.gif?h=200&amp;w=500" alt="ok img" /></p>

<p><a href="http://example.com">link ok</a></p>

@nicholasserra
Copy link
Collaborator

Fixed in #236 thanks everyone!

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