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

Autoescape does not escape javascript in a href #2623

Closed
scootstah opened this issue Feb 5, 2018 · 9 comments
Closed

Autoescape does not escape javascript in a href #2623

scootstah opened this issue Feb 5, 2018 · 9 comments

Comments

@scootstah
Copy link

scootstah commented Feb 5, 2018

I stumbled upon this, and I'm not sure if it's a bug or not.

It seems that Twig will not autoescape a "javascript:" URI in a href attribute. It will escape if you explicitly tell it to escape js, however.

I have only tested this from Symfony, but I assume that shouldn't matter. Here's an example:

From a controller:

return $this->render('default/index.html.twig', [
    'foo' => 'javascript:alert("hello")',
]);

The view:

<a href="{{ foo }}">foo</a>

This will not be escaped, and clicking the link will open an alert.

This does not work either:

{% autoescape %}
<a href="{{ foo }}">foo</a>
{% endautoescape %}

However, these will escape properly:

<a href="{{ foo|e('js') }}">foo</a>
{% autoescape 'js' %}
<a href="{{ foo }}">foo</a>
{% endautoescape %}
@umulmrum
Copy link

I stumbled across the same problem and am thankful for a solution.

Using "e('js')" will break legitimate/non-JS URLs, so this can't be used in my context. What is the best solution here? I could imagine that it's hard to solve in a transparent way, so introduce another escaper for URL-but-not-JS? Or an argument for html_attr to allow/disallow JS?

@stof
Copy link
Member

stof commented Feb 20, 2018

I think the html_attr escaper might escape things enough to cover this case (and without breaking things like the js escaper, which is not meant to be used in a HTML context at all)

@umulmrum
Copy link

Please excuse my impatience, but I think it needs to be made clear that this is a security issue which opens space for XSS attacks. Supposedly a lot of developers might not be aware of this, as they assume that Twig handles output escaping completely, so a rather quick solution would be very appreciated.

I cannot see what the best solution is (don't know if people rely on JS NOT being escaped by html_attr so that simply adding escaping will break their use case), but can try an implementation if someone tells me what to do.

@stof
Copy link
Member

stof commented Feb 27, 2018

@umulmrum the default autoescaping strategy is html, not html_attr.

And escaping things entirely all the time would require doing contextual escaping (i.e. changing the escaping based on where the output is performed), which is not something done by Twig (it would be really hard to do that while keeping Twig agnostic of the output format)

@umulmrum
Copy link

umulmrum commented Feb 27, 2018

As said before, I am not able to propose the correct/best solution (and did not try). The problem is that "javascript:" calls are not escaped automatically and I am not aware of any way to tell Twig to escape those. So let me ask a few auxiliary questions:

  1. Can Twig escape "javascript:" URLs?
  2. If yes, how?
  3. If no, is the project willing to add such capability?
  4. If yes, how should it work?

Thanks!

edit: Maybe I misunderstood your post above. html_attr does not escape "javascript:" URLs.

@stof
Copy link
Member

stof commented Feb 27, 2018

Using the html_attr escaping is escaping it. Here is the proof: https://twigfiddle.com/sg1sf2

@umulmrum
Copy link

Well yes, again I'm unclear :-)

It is escaping, so that the JS code does not break the attribute. But the code still stays executable, which is the security problem. Would you see this outside of the scope of html_attr?

@mikkelrd
Copy link

I, too, would love for this to be built-in.

@fabpot
Copy link
Contributor

fabpot commented Apr 22, 2019

Escaping is hard and not everything can be easily automated. I've added a note in the docs to warn users (see #2950), but I don't want to add more specific information about escaping as this out of the scope of Twig, which only provide tools to help escape elements.

@fabpot fabpot closed this as completed Apr 22, 2019
fabpot added a commit that referenced this issue Apr 22, 2019
This PR was merged into the 1.x branch.

Discussion
----------

Add a note about escaping

closes #2623, closes #2661

Commits
-------

a83f7e8 added a note about escaping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants