django
django copied to clipboard
django
Fixed #33405 -- Documentation for template filter 'escapejs' is extremely unclear
This makes the string safe for use in HTML and JavaScript or JSON string literals
I don't think it's OK. It's unsafe not just in ES6 template literals, but also in javascript URL protocol. For example:
<a href="javascript:console.log('{{ request.GET.q | escapejs }}')">click me</a>
Although the user input is in the "JavaScript string literals", it still can be attacked by this payload:
?q=%27%2Balert%281%29%2B%27
Update on 20 Jun: I made a mistake. The correct payload is
?q=%2527%252Balert%25281%2529%252B%2527
@phith0n I cannot reproduce your example, it amounts to:
In [1]: from django.template import Template, Context
In [2]: t = Template("""<a href="javascript:console.log('{{ q | escapejs }}')">click me</a>""")
In [3]: print(t.render(Context({"q": "'+alert(1)+'"})))
<a href="javascript:console.log('\u0027+alert(1)+\u0027')">click me</a>
escapejs clearly maps the single quote to \u0027, as can be seen in the source:
https://github.com/django/django/blob/d19a53d8e3640e8f937048bf7826c7e2229a9665/django/utils/html.py#L30-L43
@adamchainz Hi, try this one:
In [1]: from django.template import Template, Context
In [2]: t = Template("""<a href="javascript:console.log('{{ q | escapejs }}')">click me</a>""")
In [3]: print(t.render(Context({"q": "%27%2Balert%281%29%2B%27"})))
<a href="javascript:console.log('%27%2Balert%281%29%2B%27')">click me</a>
alert(1) will be execute when you click the link: https://codepen.io/phithon/pen/MWQRZqO
@adamchainz Hi, try this one:
In [2]: t = Template("""<a href="javascript:console.log('{{ q | escapejs }}')">click me</a>""")
But that's due to a bug in your template, not a problem with escapejs. q is embedded within JavaScript code, which escapejs has correctly dealt with, but then that is itself embedded within a URL, and you've forgotten to escape around that. The correct template would be:
<a href="javascript:console.log('{{ q | escapejs | urlencode }}')">
@adamchainz Hi, try this one:
In [2]: t = Template("""<a href="javascript:console.log('{{ q | escapejs }}')">click me</a>""")But that's due to a bug in your template, not a problem with
escapejs.qis embedded within JavaScript code, whichescapejshas correctly dealt with, but then that is itself embedded within a URL, and you've forgotten to escape around that. The correct template would be:<a href="javascript:console.log('{{ q | escapejs | urlencode }}')">
There is no problem with escapejs, but it can mislead developers to trust the escapejs if you say "this makes the string safe
for use in HTML and JavaScript or JSON string literals".
There is no problem with
escapejs, but it can mislead developers to trust theescapejsif you say "this makes the string safe for use in HTML and JavaScript or JSON string literals".
But none of those things are "javascript: URLs". It doesn't say either that you shouldn't do eval({{ q | escapejs}}), but you clearly shouldn't and it's your own fault if you do.
There is no problem with
escapejs, but it can mislead developers to trust theescapejsif you say "this makes the string safe for use in HTML and JavaScript or JSON string literals".But none of those things are "javascript: URLs". It doesn't say either that you shouldn't do
eval({{ q | escapejs}}), but you clearly shouldn't and it's your own fault if you do.
I don't think javascript:console.log('{{ q | escapejs }}') is equal to eval({{ q | escapejs}}). The first q is put in JavaScript string literals (although it also in the URL context), not all the developers know that they have to quote it.
Due to the "secure by default" philosophy, I think it's not a good idea to update the document and say it is "safe to use in HTML".
Another way is to add % to the escape list in escapejs:
_js_escapes = {
ord("\\"): "\\u005C",
ord("'"): "\\u0027",
ord('"'): "\\u0022",
ord(">"): "\\u003E",
ord("<"): "\\u003C",
ord("&"): "\\u0026",
ord("="): "\\u003D",
ord("-"): "\\u002D",
ord(";"): "\\u003B",
ord("`"): "\\u0060",
ord("%"): "\\u0025",
ord("\u2028"): "\\u2028",
ord("\u2029"): "\\u2029",
}
To add some context, from my perspective this is still sitting here because the feedback from the security team was more or less, "If we can phrase it correctly, ..." but that conceals a lot. It's not at all clear (to me at least) what we should say, for the exact cases it's OK, and how we don't encourage people into adopting a foot-gun.
Due to the "secure as default" philosophy, I think it's not a good idea to update the document and say it is "safe to use in HTML".
I more or less agree with this.
@carltongibson What is the problem with saying it's "safe to use in HTML"? It's true, and it doesn't violate "secure by default". What scenario is being imagined whereby a problem might arise?
The documentation really can't stay as it is - it's flat-out wrong, and so confused as to be almost meaningless. What on earth would "it isn't safe but it protects you from syntax errors" be supposed to mean even if it were true? And I guess "when using templates to generate JavaScript/JSON" is supposed to mean "when using templates to generate JavaScript/JSON files", but that isn't what it says (obviously you're generating JavaScript/JSON, since this filter has no other purpose).
The filter output is safe to use:
- in HTML content and attribute values
- in JavaScript/JSON
'and"string literals in JavaScript/JSON files - in JavaScript/JSON
'and"string literals inside HTML in-line<script>content and attribute values
The things that aren't safe to do include:
- using it in JavaScript
`template literals - using it in a string literal but putting a
\immediately before the variable part - using it in a
javascript:URL without usingurlencode
but the latter two of these seem so contrived as to constitute writing a caveat "do not load gun, take careful aim at own foot, and pull trigger".
The problem is there's a lot of talk but not a clear demonstration that it's actually safe.
And then there are the examples where suddenly it's not, which would need documentation.
If don't think "it's safe unless you do this" is a very good thing to be saying. People don't read the caveats, use it badly, and then say "but you said it was safe". Better the current text.
But that's what I was just saying, the current text is not "better", the current text is false, and worse than that, it's gibberish. It has no discernible meaning. There is no way that leaving it unchanged is a good idea.
I don't know what you mean by "there are the examples where suddenly it's not [safe]". The "examples" (apart from backtick template strings, which according to ticket-29055 is a deliberate decision by the security list to keep as unsafe and yet the current documentation does not clearly describe as unsafe - which my patch fixes) are of the category "if the developer writes buggy code then there will be bugs". If you wanted to document every place in Django where it is possible to write buggy code then obviously the documentation would be of infinite size.
... it's gibberish. It has no discernible meaning...
It seems pretty clear TBH, much clearer than trying to specify a exact set of safe usages. (Recalling that safe means not subject to injection attacks)
It seems pretty clear TBH, much clearer than trying to specify a exact set of safe usages. (Recalling that safe means not subject to injection attacks)
My patch specifies the safe usages, and is very clear.
I think you're failing to appreciate how the current documentation reads to someone who doesn't already know what it means. It's like someone's winning entry for an "ambiguous phrasing" competition. It says:
Escapes characters for use in JavaScript strings.
This sentence isn't untrue, but there is vital unmentioned information here that the filter doesn't turn a Python string into a JavaScript string, it turns it into a sequence of characters that can be inserted into a JavaScript string - i.e. it doesn't add leading and trailing quotes. The only way to discover this is by examining the source or through experimentation. My patch fixes this by making it clear with an example.
This does not make the string safe for use in HTML
This part is false, and incredibly confusing: "If it doesn't make it safe for HTML, why on earth does this filter exist, what is it for?" - bear in mind, 99% of the usage of Django templates is to create HTML files, not other sorts of files. My patch fixes this by removing the "not".
or JavaScript template literals,
This part is extremely easy to misunderstand - it means "the ES6 JavaScript feature of template literals" but given this whole document is describing Django templates, the more obvious interpretation of it is "Django templates that output JavaScript literals". My patch fixes this by making it explicit what we're talking about.
but does protect you from syntax errors
This part is meaningless gibberish. What bizarre meaning of the word "protect" is the documentation using if we can be simultaneously "protected" and yet also "unsafe"? Why would anyone want to generate syntactically-valid but unsafe code!? My patch fixes this by removing it.
when using templates to generate JavaScript/JSON.
I guess this means "when generating JavaScript/JSON files" but it doesn't say that. Anyone looking for something to generate JavaScript/JSON that is embedded within HTML will think this part is talking about them, which it isn't. My patch fixes this by removing it.
Hi @jribbens.
We seem to be going round in circles... 🤔
The point about the output being unsafe is (I take it) that you can't pass arbitrary data from an untrusted source without risk of an injection. The filter is still useful because I can use it with data that is trusted (that I created) to construct syntactically valid JavaScript.
Maybe there's a case for clarification, adding the usage example, and specifying "JavaScript template literals", and so on.
What I'm objecting to here is making the strong claim that, in fact, despite previous judgements to the contrary, escapejs is safe to use with arbitrary data from an untrusted source. I don't see either the argument or the test cases to support that.
We frequently see security reports in areas otherwise thought safe, and am not keen to make this change unless we can show that it's correct. Even in the simple case, is there really no injection vulnerability? Are we sure about that? (I'm not.) Then, for me, the it's safe in this case, but not that one claim is just a sign that the guidance should be, as the existing, it's not safe, i.e. users should use escapejs with untrusted data.
We seem to be going round in circles... 🤔
The point about the output being unsafe is (I take it) that you can't pass arbitrary data from an untrusted source without risk of an injection. The filter is still useful because I can use it with data that is trusted (that I created) to construct syntactically valid JavaScript.
Maybe there's a case for clarification, adding the usage example, and specifying "JavaScript template literals", and so on.
Yes, I mean I don't like this solution but it is still an improvement on "change nothing", because as explained above, if this is the meaning the documentation is supposed to be conveying, it most certainly does not convey that meaning at present.
What I'm objecting to here is making the strong claim that, in fact, despite previous judgements to the contrary,
escapejsis safe to use with arbitrary data from an untrusted source. I don't see either the argument or the test cases to support that.
I don't know what you mean by "previous judgements to the contrary". As far as I can tell, no such judgements exist. What has happened is that in ticket-7177 the filter was significantly modified to make it safe, and they simply forgot to update the documentation at the same time. Nobody has ever actually decided the filter (in the version that has existed for a great many years) is unsafe.
"Test cases" to prove security obviously makes no sense, but I can provide argument as to why it is safe if you wish. It will obviously be slightly lengthy and I am aware this thread has already ceased being short some time ago, so please let me know if you think it would be helpful, rather than annoying, for me to provide a proof of safety.
I can also update the patch as suggested above by @phith0n to make it safe for the extremely obscure case of javascript: URLs where the developer forgets to call urlencode, if that would be helpful. I could also make it safe for use in ES6 template literals, but as mentioned in ticket-29055, it is apparently a deliberate decision by the security list that the filter be unsafe in that situation.
so please let me know if you think it would be helpful, rather than annoying, for me to provide a proof of safety.
If you can make it plain, then yes, I'd be very happy to read it! Thanks.
Ok. So, firstly, what does escapejs do exactly? We can see from django/utils/html.py that it transforms the following characters:
\x00-\x1f \ ' " > < & = - ; ` \u2028 \u2029
and all of them are replaced with sequences of the form \uXXXX where XXXX is 4 hexadecimal digits.
The most fundamental and important question is, of course, is the output from this safe to be passed to something that will execute it as JavaScript? We can inspect the ECMAScript specification to see that:
All code points may appear literally in a string literal except for the closing quote code points, U+005C (REVERSE SOLIDUS), U+000D (CARRIAGE RETURN), and U+000A (LINE FEED). Any code points may appear in the form of an escape sequence.
We can immediately see that all of those five characters are indeed encoded by escapejs, and that the \uXXXX escapes they are replaced with are allowed and are valid instances of the JavaScript UnicodeEscapeSequence.
So next let's look at the JSON grammar to see if the escapejs output is ok there. That says we can have all characters \u0020 - \u10ffff except " and \, and that the \uXXXX escapes are valid. escapejs is encoding ", \ and \x00-\x1f so we're all good for JSON.
It is important to note that all of the above assumes that you are using the filter in the way I have documented it, i.e. one of "{{ foo|escapejs }}" or '{{ foo|escapejs }}'. If you were to omit the quotes, or if you put a partial escape sequence immediately before the inserted value, then the output can be unsafe. The current documentation does not mention this, so my patch improves this situation. If you think this should be made even more explicit, I can of course easily do this. But note, this is unlikely to end up as a security problem as the code will simply not work as expected even when presented with non-malicious input, so the developer is going to notice before the code is deployed.
As previously mentioned, escapejs output is not safe for inclusion in ES6 backtick template string literals. I think this could probably be remedied by adding $ to the list of encoded characters, but the fact that it is unsafe is apparently a deliberate decision of the Django security list so I have not proposed to do this. My patch however does improve the situation by documenting clearly that you should not use escapejs in this context.
So the next question is: is the output safe for use as HTML text and HTML attribute values? The HTML specification would be an extremely verbose source to use for this, but we can presumably agree that if the escapejs output is at least as safe as all the rest of the output from Django templates then we're doing well.
Django delegates the escaping of HTML text and attributes to Python's html.escape function. We can see from its source that it encodes the characters &, <, >, ", and '. From this we can see that (a) escapejs does indeed prevent any of those characters from occurring in its output, and (b) the characters that escapejs adds to its output (i.e. \, u and hexadecimal digits) are all treated as plain text by HTML. So not only is the output from escapejs safe to embed in HTML, but the HTML parser will leave it unchanged so the JavaScript/JSON parser will still understand it.
The above suffices to prove that it is correct to say, as my patch does, that the output of escapejs is "safe for use in HTML and JavaScript or JSON string literals".
The only other point mentioned is that of javascript: URLs. Personally I think this is not particularly relevant, because (a) the documentation does not say that the output is safe to use in this situation, either before or after my patch, and (b) the developer should know that if they're encoding any sort of value into a URL then they must call the urlencode filter.
However, as @phith0n mentioned, perhaps even this unwise and obscure situation could be made safe by adding % to the list of characters that escapejs encodes. We can see from the HTML specification that the URL is processed by percent-decoding and then UTF-8 decoding its contents. "Percent decoding", as the name suggests, only affects sequences of the form %XX, and as above, escapejs will never introduce a % character as part of its encoding, so making it encode % would indeed make this situation safe as well. I can add this to my patch if that would be thought helpful.
Hi @jribbens — sorry for the delay I had COVID. I will pick this up again to review.
Hi there, I just ran over this ticket and those are my thoughts towards escapejs:
Independent of what the filter does, I do not want it to be used widely as it sends a bad usage signal. The main intended usage is the one that this PR adds an example for, namely:
<script>
let myValue = '{{ value|escapejs }}'
let myValue2 = "{{ value|escapejs }}"
</script>
And in my opinion supporting even that is going further than I want nowadays. I personally do not see any reason to include JS literally in HTML like this. Javascript should be loaded externally and required data for it should come via data-* attributes on HTML elements (or via |json_script).
Even if this filter is usable for JSON generation, I would not want anyone to use it for JSON. Manually constructing JSON like that sets a bad precedent imo; next thing people start constructing XML manually.
So from a security (strict CSP) and best practices POV there is no reason to have this filter. Having this filter at all encourages "bad" practices. Mind you, this filter is old, so I understand why we have it. Can we simply deprecate and remove it? ;)
In that sense I would trim & simplify the docs to say something the lines of (obviously needs some refinements but you should get the gist):
Escapes characters for use in JavaScript strings. This makes the string safe for use in
<script>tags of a HTML document or in a javascript template file. Note that it does not make the string safe for use in "JavaScript template literals" (i.e. the JavaScript backtick syntax). Any other use is not supported, seejson_scriptfor an html data attributes for alternatives.
What do you think?
@apollo13 I'm not sure that everyone in the world should have to write code according to your personal opinions ;-) I would not want to see this filter removed. Having said that, I've no problem with disrecommending this filter - my main issue with the documentation as it stands is that it is meaningless gibberish that leaves you with no clue when to use it, or for that matter when not to use it.
What would you think to:
Escapes characters for use in JavaScript strings. This makes the string safe for use in JavaScript string literals, including when embedded in HTML. Note that it does not make the string safe for use in "JavaScript template literals" (i.e. the JavaScript backtick syntax). Any other use is not supported. It is generally recommended however that data should be passed using HTML
data-attributes, or thejson_scriptfilter, rather than in embedded JavaScript.
(And keeping the example, which is vital as without it there's nothing to tell you that you have to include the quote characters yourself.)
If that's OK I'll update the patch.
@jribbens
@apollo13 I'm not sure that everyone in the world should have to write code according to your personal opinions ;-)
Ha, I do not care how people write code as long as we don't encourage bad patterns :)
I would not want to see this filter removed.
May I ask what you are using it for and why you think there are no better alternatives?
including when embedded in HTML.
Not sure about how I feel about that on; embedded in HMTL can mean many things. "embedded in a HTML script tag" would be clear as far as the intent is concerned.
And keeping the example, which is vital as without it there's nothing to tell you that you have to include the quote characters yourself.
Absolutely, the example is really needed (that said one usually notices during dev that hell break loose if you don't add the quotes -- unless you test solely with numbers or so ;))
@apollo13
I would not want to see this filter removed.
May I ask what you are using it for and why you think there are no better alternatives?
It's not necessarily the case that there are no better alternatives, but I certainly wouldn't want to be prevented from upgrading to newer versions of Django on existing projects without having to significantly refactor code.
Having said that, personally I see almost no value at all in using CSP or being able to use it without script-src unsafe-inline. Being strict with CSP disallows all style attributes for god's sake. It's ridiculous. For any real-world site you have to allow so much stuff it just adds enormous overhead to every HTTP request with absolutely no benefit.
I also think json_script non-script-script-tags-that-must-then-be-JSON.parse'd are very long-winded and convoluted and have disadvantages and no advantages over simply putting the equivalent string in a normal script tag.
including when embedded in HTML.
Not sure about how I feel about that on; embedded in HTML can mean many things. "embedded in a HTML script tag" would be clear as far as the intent is concerned.
The reason I'm avoiding saying that is because of on... attributes. I'm not sure "embedded in HTML" can mean anything that would be wrong or misleading?
And keeping the example, which is vital as without it there's nothing to tell you that you have to include the quote characters yourself.
Absolutely, the example is really needed (that said one usually notices during dev that hell break loose if you don't add the quotes -- unless you test solely with numbers or so ;))
Excellent :-)
Having said that, personally I see almost no value at all in using CSP or being able to use it without script-src unsafe-inline. Being strict with CSP disallows all style attributes for god's sake. It's ridiculous. For any real-world site you have to allow so much stuff it just adds enormous overhead to every HTTP request with absolutely no benefit.
I also think json_script non-script-script-tags-that-must-then-be-JSON.parse'd are very long-winded and convoluted and have disadvantages and no advantages over simply putting the equivalent string in a normal script tag.
Agree, we must make it possible for both demanding and casual people to develop web applications quickly, no need to follow to a strict design pattern.
escapejs is a very simple and useful filter for me.
@nessita I'm afraid you've completely misunderstood this patch and indeed fundamentally what escapejs is and does. Your code is insecure whether this patch is accepted or not. It is fundamentally misusing the escapejs filter, which has one use and one use only - in some JavaScript code in a template you write "{{ foo|escapejs }}" with the quotes. My patch makes this more clear, rather than less clear, by adding the example code to show developers how to use it.
Yes, if we could turn back time then I would certainly recommend that this filter output string values with the quotes included, but we can't do that now as it would break backwards compatibility.
This patch is not about fixing the behaviour of the escapejs filter, which although not ideal is not broken if used correctly, it is about fixing its documentation, which as it stands is frankly just nonsensical gibberish.
@jribbens Thanks for the clarification, I will re-read all the comments with that in mind and re-reply next week.
First of all, I would like to kindly ask @jribbens to please tone down their responses. We might all be experiencing some frustration due to a lack of understanding, and while certain points may seem obvious to oneself, they may not be as clear to others. Nevertheless, it's important that we continue the conversation with a friendly tone and make an effort to meet halfway, to be able to find a compromise that works for everyone. Above all we should follow the Django Code of Conduct: let's be friendly and patient, welcoming, considerate, respectful, and careful in the words that we choose.
Having said that, and after many hours of reading related tickets and the comments in this PR, before making a recommendation, I'll try to summarize what I think are the key points around this work:
1- The author of this PR would like the escapejs documentation to be changed so it's more explicit about the intended use of the filter. I don't think this is controversial and we all agree it can be improved.
2- The author would like the docs to be explicit about the level of "safeness" that the filter guarantees, when used in a specific way. I think this is the controversial bit, since I (and from the comments, other reviewers as well) believe that when Django users read the qualifier "safe", they tend to expect a lot of that and think something along the lines of "safe in almost every context and prevents XSS attacks".
With that in mind, and 1- using the already suggested paragraphs, and 2- expanding on the bits that (also) caused me confusion (I'm not a JS expert), I would propose that we resort to the "less is more" approach while staying on the secure-by-default side, and using something like:
Escapes characters for use inside JavaScript strings (text written inside single or double quotes). Using this filter only makes the string safe when used in JavaScript string literals (please see examples below). Note that it does not make the string safe for use in "JavaScript template literals" (i.e. the JavaScript backtick syntax), and any other use is not supported. It is generally recommended however that data should be passed using HTML
data-attributes, or thejson_scriptfilter, rather than in embedded JavaScript.
Then, and following the original proposal, I think we could include examples for both "embed in HTML" cases: using it with <script> tag and with the on directive.
Thoughts? Thank you! Natalia.
(Just a small clarification: I want to mention that since English is not my native language (and I have also discussed this with other individuals who are in the same situation), when reading the term "JavaScript string," an initial interpretation could be "a string containing JavaScript code" rather than a "text enclosed in quotes." Therefore, I personally believe that providing further clarification about what is meant by a "JavaScript string" could greatly help in understanding.)
First of all, I would like to kindly ask @jribbens to please tone down their responses.
Sorry, I don't understand what you are referring to here.
1- The author of this PR would like the
escapejsdocumentation to be changed so it's more explicit about the intended use of the filter. I don't think this is controversial and we all agree it can be improved.2- The author would like the docs to be explicit about the level of "safeness" that the filter guarantees, when used in a specific way. I think this is the controversial bit, since I (and from the comments, other reviewers as well) believe that when Django users read the qualifier "safe", they tend to expect a lot of that and think something along the lines of "safe in almost every context and prevents XSS attacks".
That is indeed what the filter does, when used correctly. Part of the issue here is that the filter used to work a different way, which wasn't "safe" - but then it was completely re-written to make it safe, but when that change happened they forgot to update the documentation. It is now just as safe as any of the other Django filters, e.g. escape.
With that in mind, and 1- using the already suggested paragraphs, and 2- expanding on the bits that (also) caused me confusion (I'm not a JS expert), I would propose that we resort to the "less is more" approach while staying on the secure-by-default side, and using something like:
Escapes characters for use inside JavaScript strings (text written inside single or double quotes). Using this filter only makes the string safe when used in JavaScript string literals (please see examples below). Note that it does not make the string safe for use in "JavaScript template literals" (i.e. the JavaScript backtick syntax), and any other use is not supported. It is generally recommended however that data should be passed using HTML
data-attributes, or thejson_scriptfilter, rather than in embedded JavaScript.Then, and following the original proposal, I think we could include examples for both "embed in HTML" cases: using it with
<script>tag and with theondirective.Thoughts? Thank you! Natalia.
I generally like your suggestion, but note that the correct usage is more restricted that your words explain. In particular, the following is not going to work properly:
const foo = "lorem ipsum \{{ foo|escapejs }}"
The filter must be used only as literally "{{ or '{{ and the value to be output and then |escapejs }}" or |escapejs }}'. (Hence why I said above it is a great shame they didn't originally make it so that it output the quotes itself.)
(Just a small clarification: I want to mention that since English is not my native language (and I have also discussed this with other individuals who are in the same situation), when reading the term "JavaScript string," an initial interpretation could be "a string containing JavaScript code" rather than a "text enclosed in quotes." Therefore, I personally believe that providing further clarification about what is meant by a "JavaScript string" could greatly help in understanding.)
Indeed. And also the existing documentation's reference to "JavaScript template literals", which I (and presumably other people) took to mean "JavaScript code which is inside a Django template" rather than what it actually means, "JavaScript backtick syntax".
I've updated the PR to reflect your wording plus my comments above. Unfortunately this required asking git to do something more complicated than just commit something to HEAD, and hence inevitably went horribly wrong and I've had to delete the whole repository and start again from scratch in PR #16897.