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

Fixed #15727 -- Add support for Content-Security-Policy header support for SecurityMiddleware #3550

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@rudolphfroger

rudolphfroger commented Nov 15, 2014

No description provided.

@@ -4,6 +4,24 @@
from django.http import HttpResponsePermanentRedirect
def construct_csp_header(policy, report_only=False):
"""
Construct Content Security Policy header from a dictonay with all the

This comment has been minimized.

@carljm

carljm Nov 20, 2014

Member

dictionary

to log all violations::
import json
import logging

This comment has been minimized.

@manfre

manfre Nov 20, 2014

Contributor

indentation.

@timgraham

This comment has been minimized.

Member

timgraham commented Nov 21, 2014

I will try to review this in detail next week or so, in the meantime I am going to trigger the build to see if flake8 detection works as I notice there are some warnings in this patch. If you could please rebase your patch instead of merging master into your branch, that's ideal when it comes to commit, thanks.

@timgraham

This comment has been minimized.

Member

timgraham commented Nov 21, 2014

buildbot, test this please.

def csp_report(request):
try:
data = json.loads(request.body)
except json.JSONDecodeError:

This comment has been minimized.

@wrwrwr

wrwrwr Nov 24, 2014

Contributor

I believe json.loads raises ValueError on parsing errors, or possibly a RuntimeError (recursion depth) if given something like: '{"":' * 995 + '""' + '}' * 995.

That's just an example, but how about limiting the body size before trying to parse it and a pprint.pformat(data, depth=3) before logging?

@timgraham

This comment has been minimized.

Member

timgraham commented Apr 17, 2015

As noted on the ticket, " The important thing for django to support is to make the admin compatible with CSP. Otherwise, most installations can't use CSP anyway." Closing due to inactivity. Please send a new PR if you want to revive this.

I guess we should discuss the comment on the ticket, "I don't see any value in adding the ability for django to set the CSP header for you. I can easily do that myself with a middleware." on the django-developers mailing list as well, before putting more time into this.

@timgraham timgraham closed this Apr 17, 2015

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