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 -- Added django-csp to contrib. #5776

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@graingert
Contributor

graingert commented Dec 5, 2015

This is copied from https://github.com/mozilla/django-csp

It's licensed under the MPL, but that's compatible with BSD https://www.mozilla.org/en-US/MPL/2.0/FAQ/#mpl-bsd-and-apache

pinging @jsocol who last committed to the project

@timgraham

This comment has been minimized.

Member

timgraham commented Dec 5, 2015

Please make proposals like this on the django-developers mailing list. Thanks!

@timgraham timgraham changed the title from Proposal to merge django-csp into contrib to Fixed #15727 -- Added django-csp to contrib. Dec 5, 2015

@timgraham

This comment has been minimized.

Member

timgraham commented Dec 5, 2015

p.s. every commit you push triggers a build on Jenkins, so please try to avoid pushing in small increments.

@jsocol

This comment has been minimized.

jsocol commented Dec 6, 2015

I'm not at Mozilla or maintaining django-csp anymore. @mozmark is the last contact I know for it.

ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

It would be nice to avoid proliferation of licenses within Django, even if they're compatible with Django's own license. Could the copyright holders to let us distribute the code under Django's license?

@aaugustin

This comment has been minimized.

Member

aaugustin commented Dec 6, 2015

It isn't obvious to me that making this a third-party app is the right choice. Security-related features tend to be implemented in core and not to be considered optional. However that argument may not apply to CSP as it is non-trivial and not every site will want it.

from django.utils.six.moves import http_client
class CSPMiddleware(object):

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

Did you consider wrapping this functionality into the SecurityMiddleware?

@apollo13

This comment has been minimized.

Member

apollo13 commented Dec 6, 2015

I'd prefer this to be part of SecurityMiddleware and in core.

'connect-src': getattr(settings, 'CSP_CONNECT_SRC', None),
'sandbox': getattr(settings, 'CSP_SANDBOX', None),
'report-uri': getattr(settings, 'CSP_REPORT_URI', None),
}

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

The fact that you're immediately wrapping all CSP_* settings into a dict suggests that it should have been a dict in the first place.

.. note::
Note when a setting requires a tuple or list. Since Python strings
are iterable, you may get very strange policies and errors.

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

Django detects and automatically fixes this sort of sloppiness is various places. Automatically fixing or at least providing a clear error in such cases is better than warning "hey you'll get confusing errors".

===============
Content-Security-Policy_ is a complicated header. There are many values
you may need to tweak here.

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

I don't this we can start a security-related documentation page with "errr it's complicated". The point of the documentation is to teach non-experts, not to scare them.

Set the ``sandbox`` directive. A tuple or list. *None*
``CSP_REPORT_URI``
Set the ``report-uri`` directive. A **string** with a full or
relative URI.

This comment has been minimized.

@aaugustin

aaugustin Dec 6, 2015

Member

This should go in ref/settings -- and perhaps be grouped into a single dict, as mentioned above.

@aaugustin

This comment has been minimized.

Member

aaugustin commented Dec 6, 2015

(This isn't a full review, I gave up and wrote to django-developers instead.)

@graingert

This comment has been minimized.

Contributor

graingert commented Dec 6, 2015

I'm not going to make these CR changes until the discussion with Mozilla and on the mailing list has been hashed out

@timgraham

This comment has been minimized.

Member

timgraham commented Dec 8, 2015

Closing in favor of adding this to SecurityMiddleware as discussed on the mailing list.

@timgraham timgraham closed this Dec 8, 2015

@jezdez

This comment has been minimized.

Contributor

jezdez commented Feb 16, 2017

For the record, @EnTeQuAk is the current maintainer of django-csp in case you need input from him.

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