Skip to content

Restructure CSP Configuration with Streamlined Settings (backwards incompatible) - #219

Merged
robhudson merged 1 commit into
mozilla:mainfrom
robhudson:two-dict-based-settings
Jun 6, 2024
Merged

robhudson merged 1 commit into
mozilla:mainfrom
robhudson:two-dict-based-settings

Conversation

@robhudson

@robhudson robhudson commented May 1, 2024

Copy link
Copy Markdown
Collaborator

This PR introduces a significant update to the django-csp project, focusing on enhancing the coherence between configuration and headers, and aligning with Django's common practices for settings. The CSP settings have been restructured, consolidating them into two primary options for enforced and report-only policies.

Key Changes:

  • Consolidation of Settings: Simplified the settings config by transitioning to a two-setting approach:
    • CONTENT_SECURITY_POLICY: settings for the enforced policy.
    • CONTENT_SECURITY_POLICY_REPORT_ONLY: settings for the report-only policy.

This is a backwards-incompatible change.

While this change may require refactoring for existing users...

  • it consolidates and simplifies django-csp configuration,
  • it ties the CSP configuration more closely with the CSP response headers, simplifying the comprehension of content-security policies,
  • it unlocks the capability to provide both enforceable and report-only headers (discussed in issue Allow enforced CSP and Report-Only at the same time #36).

Feedback and Contributions

We invite our community members to test and provide feedback on the updated settings structure. Your input will help refine django-csp and ensure its compatibility with Django best practices. Documentation enhancements or suggestions are greatly appreciated.

Comment thread docs/migration-guide.rst
By following this migration guide, you should be able to successfully update your Django project to
use the new dict-based CSP settings format introduced in the latest version of `django-csp`. This
change aligns the package with the latest CSP specification and provides a more organized and
flexible way to configure your Content Security Policy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 💋

@robhudson
robhudson force-pushed the two-dict-based-settings branch from 1da985f to b09c116 Compare May 7, 2024 21:07

@willkg willkg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much excited about this overhaul. The new configuration model is vastly more ergonomic and easier to work with. Thank you for working on this!

Comment thread docs/configuration.rst
'upgrade-insecure-requests': True,
'report-uri': "/csp-report/",
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I <3 this soooo much more than django-csp 3 configuration.

Comment thread csp/contrib/rate_limiting.py Outdated
Comment thread csp/contrib/rate_limiting.py Outdated

@diox diox left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not a full review, but some comments around the backwards-incompatibility this introduces)

Comment thread CHANGES Outdated
Comment thread docs/configuration.rst Outdated
Comment thread docs/migration-guide.rst Outdated
@robhudson
robhudson force-pushed the two-dict-based-settings branch 2 times, most recently from d2cd3e2 to 4c12388 Compare May 28, 2024 19:10
@robhudson

Copy link
Copy Markdown
Collaborator Author

Does anyone also feel like we shouldn't have a top level csp import path? I think current practices for Django apps would suggest something like django_csp instead.

I'm considering changing the import path since we're breaking backwards compatibility anyway but wanted to try to get a poll:

  • 👍 if you think we should do this
  • 👎 if you think this is too much change

Comment thread csp/checks.py
warning = (
"You are using django-csp < 4.0 settings. Please update your settings to use the new format.\n"
"See https://django-csp.readthedocs.io/en/latest/migration-guide.html for more information.\n\n"
"We have attempted to build the new CSP config for you based on your current settings:\n\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good

@stevejalim stevejalim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with v v minor comments

Very excited about this - thank you for all the hard (and smart!) work on it 🚀

Comment thread csp/decorators.py Outdated
Comment thread docs/configuration.rst Outdated
@robhudson
robhudson force-pushed the two-dict-based-settings branch from 93f1eb3 to bbfc8bb Compare June 6, 2024 16:41
This is a backwards incompatible change.

Also fixes mozilla#139, mozilla#191
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants