Added support for Content Security Policy #330

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

gavinwahl commented Oct 9, 2012

In order to use debug toolbar with a content security policy enabled, no
inline scripts or styles can be used. This commit replaces all style
attributes with CSS or javascript. It uses data attributes to handle
the dynamic styles, like width and color.

In addition, this required updating jQuery to 1.8, because 1.7 is not
CSP compatible.

See also #307, which began implementing CSP compatibility.

Contributor

graingert commented Oct 14, 2012

why not use hidden="hidden" rather than class="hidden"

Contributor

gavinwahl commented Oct 14, 2012

Neither HTML 4.01 nor XHTML 1 have a hidden attribute. I suppose now it doesn't matter either way.

Contributor

graingert commented Oct 14, 2012

data- attributes are invalid for HTML 4.01 and XHTML 1

Contributor

aaugustin commented Oct 26, 2013

Thanks for the proposal. I'd like to make this improvement.

Maintenance efforts on the debug toolbar were extremely limited back when you made this pull request. The situation has improved since then.

If you're still interested in making this happen, since it's a fairly large patch, we should coordinate. Here's what I suggest.

  1. Start with a PR to upgrade to the latest jQuery (2.x). I don't know if it will require updating the rest of the JS code.

  2. Once it's merged, create a new PR with the remainder of the changes.

I'm far from a frontend expert but I'll merge these changes provided they seem reasonable.

data- attributes are a great way to deal with this kind of thing and is also safer as everything is just html-escaped. As for the jQuery - I'd suggest perhaps sticking to 1.X for a while to maintain compatibility with old IEs

Contributor

aaugustin commented Oct 26, 2013

Given the target audience of the debug toolbar, I'm not sure we need to maintain support for IE ≤ 8.

I don't care much either way — whoever makes the PR decides :)

Contributor

gavinwahl commented Dec 13, 2013

#471 did the jquery part of this. I don't have time right now to do the rest but I am still interested.

Can someone take a look at this? It would be really useful for the project to be as conservative as possible in supported CSPs, so that development environment can differ as little as possible from production.

Contributor

gavinwahl commented Jan 11, 2015

I started working on this again and updated the pull request. I have it working with RENDER_PANELS=True, but with RENDER_PANELS=False it's tricky.

When rendering panels lazily, debug toolbar fetches their content over xhr and then inserts them with $.html. When you do $.html('<script src="http://foo"></script>') in jquery, it doesn't actually insert a script tag into the dom -- it retrieves the script source with xhr and then evals it (see _evalUrl), which of course doesn't work with CSP. I'm not sure how to fix this.

With RENDER_PANELS=True, this pull request is compatible with the CSP policy default-src 'none'; script-src 'self' http://ajax.googleapis.com/; img-src 'self' data:; style-src 'self'; connect-src 'self';.

Contributor

graingert commented Jan 12, 2015

@gavinwahl nice one, thanks for taking another look at this 2yo PR!

What is the Google api Ajax URL for, and why is it not https?

Contributor

gavinwahl commented Jan 12, 2015

It's for debug toolbar to load it's jquery (JQUERY_URL). It should actually be ajax.googleapis.com to allow https.

Unless we want to bundle jQuery (which I'm pretty sure we don't!) then we will have to have that exception, so I suggest we document it.

With regards to the other parts, allowing self makes sense, and is pretty reasonable, but it would be nice to remove the dependency on img-src data:;. Is that possible to remove?

Contributor

graingert commented Jan 12, 2015

@gavinwahl I'm right in thinking that JQUERY_URL can point to a self hosted jQuery JS.

Contributor

gavinwahl commented Jan 12, 2015

data: is used for some icons: https://github.com/django-debug-toolbar/django-debug-toolbar/blob/master/debug_toolbar/static/debug_toolbar/css/toolbar.css#L150. I suppose they could be put into files instead. I'm not sure if there's any security problem with allow data urls for images.

Regarding the rest of the policy, this is not the purview of debug toolbar. It doesn't set the header itself, that's up to you and you can use whatever policy you like. If you set JQUERY_URL to something local, feel free to remove ajax.googleapis.com from script-src.

I only mentioned the policy I used so we could document a policy that works with the default toolbar config.

Contributor

graingert commented Jan 12, 2015

@gavinwahl With data-uri images an attacker can, in principle, place a transparant gif over the top of say a bank balance and trick a user into sending the incorrect amount to the benifit of the attacker. While this is a bit far fetched, the purpose of CSP is to make cross site attacks completely impossible.

They can also be slower than multiple files in lots of places (especially with HTTP/2).

Please put these in separate files if you have time.

Contributor

graingert commented Jan 12, 2015

Some of these might also look better as :after{content:'&#XXX'} Unicode glyphs, which are generally faster, safer and pretter.

Contributor

aaugustin commented Jan 13, 2015

Since the debug toolbar is intended for local use, it shouldn't be optimized for speed, only for ease of maintenance and development.

Contributor

gavinwahl commented Jan 13, 2015

I opened jquery/jquery#2012 for the script thing

Contributor

gavinwahl commented Jan 14, 2015

Looks like the jquery thing isn't going to be fixed until jquery 3.0 is released. I'm not aware of a workaround, so we'll just have to document that you must either use RENDER_PANELS=False or jquery>=3.0. What section of the docs can these notes about CSP support go?

Contributor

graingert commented Jan 14, 2015

@gavinwahl you can get this working in jQuery 2.x by triggering:

if ( code.indexOf("use strict") === 1 ) {
    script = document.createElement("script");
    script.text = code;
    document.head.appendChild( script ).parentNode.removeChild( script );
} else {
Contributor

gavinwahl commented Jan 14, 2015

No, I saw that too and tried it. Notice that it's putting the source code of the script in script.text -- this is an inline script, which CSP blocks.

Contributor

graingert commented Jan 14, 2015

urgh yeah I didn't spot that.

Contributor

graingert commented Jan 14, 2015

@gavinwahl can you use:

inner.html(''); // empty and unbind jQuery handlers
inner[0].innerHTML = data;

instead of:

inner.html(data);
Contributor

gavinwahl commented Jan 14, 2015

Script tags inserted with innerHTML are not executed. That's the reason why jQuery is doing this stuff in the first place.

Contributor

graingert commented Jan 14, 2015

Can we load the scripts statically and the content dynamically?
On 14 Jan 2015 18:29, "Gavin Wahl" notifications@github.com wrote:

Script tags inserted with innerHTML are not executed. That's the reason
why jQuery is doing this stuff in the first place.

Reply to this email directly or view it on GitHub
django-debug-toolbar#330 (comment)
.

Contributor

gavinwahl commented Jan 14, 2015

Yes, we could load the scripts for all the builtin panels in base.html, where toolbar.js is loaded. Third-party panels would be unable to load scripts then.

Contributor

graingert commented Jan 14, 2015

How about getting a copy of the jQuery 3.0 .html function and including it
here?
On 14 Jan 2015 18:31, "Gavin Wahl" notifications@github.com wrote:

Yes, we could load the scripts for all the builtin panels in base.html,
where toolbar.js is loaded. Third-party panels would be unable to load
scripts then.

Reply to this email directly or view it on GitHub
django-debug-toolbar#330 (comment)
.

Contributor

gavinwahl commented Jan 17, 2015

That doesn't seem feasible, it's actually functions that are indirectly used by .html(). We'd have to monkey patch jquery's _evalUrl function to do it, but we reuse a jquery that's already on the page. It'd be rude to go about monkey patching some else's copy of jquery.

Contributor

gavinwahl commented Feb 26, 2015

I don't think there's going to be any workaround for the jquery thing, so we'll just have to document that you must either use RENDER_PANELS=False or jquery>=3.0 for CSP compatibility. What section of the docs can these notes about CSP support go?

Contributor

graingert commented Apr 10, 2015

I've rebased and fixed the tests in #704

Contributor

tim-schilling commented Jul 18, 2015

Closing this since #704 was merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment