Making the admin compatible with CSP

[James Bligh]

Ticket #15727, dealing with adding support for the Content Security Policy header, was last updated with the suggestion that the one thing should be done is to make the admin site compatible by removing inline scripts.

I'd love to see this done, especially with the new design. I have done a similar job on my own django site so had a quick look at the admin. And it seems manageable, but I might be missing things due to naivety.

I'm just looking for opinions before I start. I'm gonna try get a pull request together in the next day or two.

There is not a huge amount of inline scripts and in general I see three problems to solve.

1) inline scripts to autofoucs a field when the page loads

2) inline scripts that pass data from django to javascript

3) scripts that use href="javascript"

I'd propose solving as follows 

1) use the autofocus attribute when creating the form. This will work for IE10+ http://caniuse.com/#feat=autofocus It can also be done by including a creating a few targeted one line js files and including instead of the inline. It just seems a lot of http overhead to save users of two old browsers having to click into a text input.

2) use <script type="application/json" technique instead. https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#HTML_entity_encoding

Quick example from popup_response.html

<script type="text/javascript">

      {% if action == 'change' %}

        opener.dismissChangeRelatedObjectPopup(window, "{{ value|escapejs }}", "{{ obj|escapejs }}", "{{ new_value|escapejs }}");

      {% elif action == 'delete' %}

        opener.dismissDeleteRelatedObjectPopup(window, "{{ value|escapejs }}");

      {% else %}

        opener.dismissAddRelatedObjectPopup(window, "{{ value|escapejs }}", "{{ obj|escapejs }}");

      {% endif %}

    </script>

becomes

    <script type="application/json" id="popup_response_data">

      {

      {% if action == 'change' %}

        "action": "change",

        "value": "{{ value|escapejs }}", 

        "obj": "{{ obj|escapejs }}",

        "new_value": "{{ new_value|escapejs }}"

      {% elif action == 'delete' %}

        "action": "delete",

        "value": "{{ value|escapejs }}"

      {% else %}

        "action": "add",

        "value": "{{ value|escapejs }}",

        "obj": "{{ obj|escapejs }}"

      {% endif %}

      }

    </script>

    <script type="text/javascript" src="{% static "admin/js/popup_response.js" %}"></script>

-popup_response.js

/*global opener*/

(function() {

    var dataElement = document.getElementById('popup_response_data');

    var jsonText = dataElement.textContent || dataElement.innerText;

    var initData = JSON.parse(jsonText);

    if (initData.action == 'change') {

        opener.dismissChangeRelatedObjectPopup(window, initData.value, initData.obj, initData.new_value);

    } else if (data.action == 'delete') {

        opener.dismissDeleteRelatedObjectPopup(window, initData.value);

    } else {

        opener.dismissAddRelatedObjectPopup(window, initData.value, initData.obj);

    }

})();

3) instead of hrefs pointing to javascript:void(0) change these to simply #javascriptvoid. Other places use code inline code in the javascript: href. These could be changed to use #javascriptvoid too and the code moved to a function that is linked to the element using jQuery or the addEvent method from core.js as appropriate.

James

[Florian Apolloner]

On Monday, September 14, 2015 at 1:17:37 AM UTC+2, jasbligh wrote:

I'd propose solving as follows 

1) use the autofocus attribute when creating the form. This will work for IE10+ http://caniuse.com/#feat=autofocus It can also be done by including a creating a few targeted one line js files and including instead of the inline. It just seems a lot of http overhead to save users of two old browsers having to click into a text input.

Why would we need one-line js files? If we decide against the autofocus attribute (this might get a little bit hard with the forms framework), we can still put a data-autofocus="#someif" into the body element and then add a handler for that.
 

2) use <script type="application/json" technique instead. https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#HTML_entity_encoding

Makes sense, depending on the actual parameters data attributes could make sense too for a few things. Oh, btw please do not handwrite JSON in templates, either do it in the view or use a filter to actually create JSON which then only needs to go through the autoescape filter I think (In that sense there is also not really a need for escapejs as far as I see it, but I might be wrong).

3) instead of hrefs pointing to javascript:void(0) change these to simply #javascriptvoid. Other places use code inline code in the javascript: href. These could be changed to use #javascriptvoid too and the code moved to a function that is linked to the element using jQuery or the addEvent method from core.js as appropriate.

Sounds good, do we need a target for the hrefs at all though?

Cheers,
Florian

[James Bligh]

On 14 September 2015 at 11:52, Florian Apolloner <f.apo...@gmail.com> wrote:

On Monday, September 14, 2015 at 1:17:37 AM UTC+2, jasbligh wrote:

I'd propose solving as follows 

1) use the autofocus attribute when creating the form. This will work for IE10+ http://caniuse.com/#feat=autofocus It can also be done by including a creating a few targeted one line js files and including instead of the inline. It just seems a lot of http overhead to save users of two old browsers having to click into a text input.

Why would we need one-line js files? If we decide against the autofocus attribute (this might get a little bit hard with the forms framework), we can still put a data-autofocus="#someif" into the body element and then add a handler for that.

Perfect, would you know where would be best to place the js in that case? (I've got the attribute working with the forms framework though)

 

2) use <script type="application/json" technique instead. https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#HTML_entity_encoding

Makes sense, depending on the actual parameters data attributes could make sense too for a few things. Oh, btw please do not handwrite JSON in templates, either do it in the view or use a filter to actually create JSON which then only needs to go through the autoescape filter I think (In that sense there is also not really a need for escapejs as far as I see it, but I might be wrong).

I'll put the json in views or a filter. There was a very relevant discussion on this before with a gist by David Evans https://groups.google.com/forum/#!msg/django-developers/RNMs5YbKeRY/ZtewE89xu_4J

 

3) instead of hrefs pointing to javascript:void(0) change these to simply #javascriptvoid. Other places use code inline code in the javascript: href. These could be changed to use #javascriptvoid too and the code moved to a function that is linked to the element using jQuery or the addEvent method from core.js as appropriate.

Sounds good, do we need a target for the hrefs at all though?

If it was my project I wouldn't have a target, that's my preferred style. But felt if the code already have javascript:void(0) someone felt the need for a href attribute and as my first contribution I didn't want to be changing too much.

 

Cheers,
Florian

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/a10e49d9-32c8-475a-ae13-f07dcc8d3b5e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Florian Apolloner]

On Monday, September 14, 2015 at 1:54:06 PM UTC+2, jasbligh wrote:

Perfect, would you know where would be best to place the js in that case? (I've got the attribute working with the forms framework though)

Dunno, any existing file which has a few functions should do ;) If the form attribute code is nice, then by all means we should use that…
 

I'll put the json in views or a filter. There was a very relevant discussion on this before with a gist by David Evans https://groups.google.com/forum/#!msg/django-developers/RNMs5YbKeRY/ZtewE89xu_4J

Yes, that library marks the output as safe, which is quote dangerous is not used properly.
 

If it was my project I wouldn't have a target, that's my preferred style. But felt if the code already have javascript:void(0) someone felt the need for a href attribute and as my first contribution I didn't want to be changing too much.

Ok, removing it should be fine then -- that is unless someone with more frontend experience speaks up :D

Cheers,
Florian

[James Bligh]

On 14 Sep 2015, at 11:52, Florian Apolloner <f.apo...@gmail.com> wrote:

Makes sense, depending on the actual parameters data attributes could make sense too for a few things. Oh, btw please do not handwrite JSON in templates, either do it in the view or use a filter to actually create JSON which then only needs to go through the autoescape filter I think (In that sense there is also not really a need for escapejs as far as I see it, but I might be wrong).

So I’ve had a go at this and I’ve come up against the how do you safely output the json to the template without calling mark_safe problem. Having looked around  my solution is to output the json string with {{ json_string|escapejs }}. 

Then in the javascript call JSON.parse twice

JSON.parse(JSON.parse('"' + jsonString + '"'));

First parse wraps the json in quotes and returns the original unescaped string which we can then turn into our data.

Because escapejs turns things into HEX codes which JSON.parse can easily understand it is easy to decode but nice and safe to include in html. No chance of closing script tags in the html output.

I’ve all of Florian’s other suggestions addressed, so next stage for me is to create a pull request so someone can look at actual code and give feedback.

Give me another evening to figure that out.

James

[Gavin Wahl]

I'm very interested in getting this into 1.10. I can devote some time to it to help.

When I looked at it before, based on the time I had available, it didn't seem feasible for me to remove every single inline script. Especially with form widgets that include templated javascript. Instead I was looking at the two ways to whitelist scripts with CSP, namely script-nonce and script hash sources. The disadvantage with either of these approaches is that they need to be integrated with the middleware adding the CSP header, to communicate the current page nonce or the list of hashes. script-nonces also totally destroy caching, because each response has to have a unique nonce that's referenced by each inline script. 

Ideally django admin would just be compatible with whatever CSP header the user wants, without any specific integration, so removing all inline scripts and styles is certainly preferable if you have the time.

>  Oh, btw please do not handwrite JSON in templates, 

Absolutely, the view should build a data structure representing the data to be encoded as JSON rather than templating it.

>  which then only needs to go through the autoescape filter I think

This is actually incorrect. <script> tags in HTML5 are Raw Text elements, so Django's autoescaping doesn't work because HTML entities are not decoded inside Raw Text elements [1]. I use the json filter from django-argonauts[2] in all my projects to do json encoding.

[1]: http://www.w3.org/TR/html5/syntax.html#raw-text-elements

[2]: https://github.com/fusionbox/django-argonauts#filter

[Florian Apolloner]

On Saturday, September 26, 2015 at 4:41:55 AM UTC+2, Gavin Wahl wrote:

>  which then only needs to go through the autoescape filter I think

This is actually incorrect. <script> tags in HTML5 are Raw Text elements, so Django's autoescaping doesn't work because HTML entities are not decoded inside Raw Text elements [1]. I use the json filter from django-argonauts[2] in all my projects to do json encoding.

Good thing I said "I think" :D It apparently has been a long time since I did put JSON into templates (I usually put them into data attributes if at all where normal escaping applies, right?)

[James Bligh]

Help would be greatly appreciated, first time contributor, not 100% sure what I’m doing.

Got stuck with the test suite over the week but back on track now.

There is a github repo here with my work [1]. The tests seem to be passing but I haven’t created any new ones.

What is the best approach a simple regression test against the views I’ve changed or would it be possible to have 

a test that checked any potential view in the admin and made sure there was no inline javascript

ie no script without a src tag unless it had a different type. The logic I can do, the all admin views is the bit I’m not sure about.

I also haven’t done the escaping the way you suggested, just coz the suggestion came in after I’d done the work but it’s an easy enough change.

Also haven’t removed all anchor links with javascript event.preventDefault calls which would me and Florian agreed would be nicer.

Haven’t done inline styles either, much less of them so should be easier task.

So work to do but would appreciate a review from someone who knows what they are doing.

James

[1] https://github.com/blighj/django/commit/ffc40d0cd904840ea77a34f640df2a3512a349db

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/087a439b-0933-43a3-a4ae-e55182d33ca2%40googlegroups.com.

[Gavin Wahl]

> Having looked around  my solution is to output the json string with {{ json_string|escapejs }}. 

> Then in the javascript call JSON.parse twice

> JSON.parse(JSON.parse('"' + jsonString + '"'));

That doesn't seem nice at all. Is there any objection to putting the json filter from django-argonauts into django?

[James Bligh]

I know there was previous ticket about a json filter which came to the conclusion it was too hard to do a generic one that could be used everywhere [1]

For the use case where the json is going to be in a script tag that won’t be executed it seems to me like the approach in django-argonauts is perfect.

Perhaps if we came up with a name that specified the context |escape_json_script or is that too much of a mouthful?

[1] https://code.djangoproject.com/ticket/17419

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CACPudh203NvC8SY8wiDi5knpxyeOe9LB6HRn1s0P%3Dd%2BfQYyHkA%40mail.gmail.com.

[Gavin Wahl]

>  it was too hard to do a generic one that could be used everywhere

The tag in argonauts works in HTML4, HTML5, and XHTML content, with or without CDATA tags. The only place you cant use it is inside an [X]HTML attribute.

[Florian Apolloner]

On Tuesday, September 15, 2015 at 12:16:03 AM UTC+2, jasbligh wrote:

Then in the javascript call JSON.parse twice

No idea why you'd ever need to use that; var json = JSON.parse('{{ json_dumps|escapejs }}'); should be fine already, or do I miss anything? That said, we should get rid of inline js anyways, so either go with a data attribute or "<script type=application/json>" + the filter from argonauts