-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added support for Content Security Policy, Rebase #330 #704
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
Conversation
This is a rebase of #330 |
@@ -35,7 +35,7 @@ | |||
<dt><strong>{{ key|escape }}</strong></dt> | |||
<dd> | |||
<div class="djTemplateShowContextDiv"><a class="djTemplateShowContext"><span class="toggleArrow">▶</span> {% trans "Toggle context" %}</a></div> | |||
<div class="djTemplateHideContextDiv" style="display:none;"><code>{{ value|escape }}</code></div> | |||
<div class="djTemplateHideContextDiv" hidden><code>{{ value|escape }}</code></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you didn't use hidden="hidden"
here and on line 22?
This looks pretty good. Could you please add a |
60f57c3
to
a966351
Compare
Done, I've also prefixed the CSS class 'highlighted' |
Conflicts: debug_toolbar/templates/debug_toolbar/panels/profiling.html
re-based, also killed that other "hidden" |
@tim-schilling @thedrow poke |
I tried setting up a test application with a content security policy enabled and I had to add |
Yes that's correct. Although if you set a custom JQuery URL it will be whatever the origin of that URL is. |
I was doing some more testing and whenever I click on the SQL tab, it generates the unsafe-eval error. Can you duplicate that issue? |
@tim-schilling Can you paste me the full error? Do you have your test project on GitHub? |
I've repeated the issue. We need to allow |
Yes, I think we should merge this PR before it needs rebasing again. @graingert Fortunately there isn't a lot of activity, only @tim-schilling and myself merge patches. I think it would be helpful to add guidelines to the contributing documentation so future changes don't break CSP inadventently. I'm not sure if it could be tested automatically e.g. with Selenium? @tim-schilling As far as I am concerned, I don't mind merging the patch as is and iterating. |
I meant to respond earlier. I decided that I'd merge it but not document it
until the unsafe-eval directive isn't needed. I don't think we can say the
toolbar supports a content security policy until that directive is no
longer needed.
|
Closes PR #704. This is the first part of making the toolbar support a content security policy. With this change a user would need to set the script-src directive to contain 'self' 'unsafe-eval' and ajax.googleapis.com, set the img-src directive to contain 'self' and data:. In the future the unsafe-eval value will no longer be needed.
I rebased your changes on master, and merged them in with commit 8b29127. Thank you for the PR @graingert! |
Closes PR django-commons#704. This is the first part of making the toolbar support a content security policy. With this change a user would need to set the script-src directive to contain 'self' 'unsafe-eval' and ajax.googleapis.com, set the img-src directive to contain 'self' and data:. In the future the unsafe-eval value will no longer be needed.
I'm deploying the experimental `Feature-Policy` on my site. I tried disabling the `sync-script` policy, which blocks HTML-parse-blocking JavaScript, and django-debug-toolbar broke because its script tags aren't async or deferred. There's a bit more information about this feature, and a demo at https://feature-policy-demos.appspot.com/sync-script.html?on I've added `defer` to all the `<script>` tags, which makes them execute after HTML parsing has finished, in order. There was also one inline script for redirects which I've extracted - this would break for `sync-script` but also for use of `Content-Security-Policy` blocking inline JavaScript, which has been supported since django-commons#704 but it seems this was missed. Tested by using the `example` app, redirect interception worked and every panel loaded and looked right.
No description provided.