Skip to content

Conversation

robhudson
Copy link
Contributor

@robhudson robhudson commented Jul 28, 2025

Trac ticket number

ticket-36532

Branch description

This adds view decorators for adjusting the CSP configuration per-view.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@robhudson robhudson force-pushed the add-csp-decorators branch 5 times, most recently from a12f683 to 901752b Compare July 30, 2025 17:19
@robhudson robhudson changed the title Refs #15727 - Added CSP view decorators. Refs #36532 - Added CSP view decorators. Jul 30, 2025
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Amazing start, thank you @robhudson! 🌟

I added an initial comment regarding API design, once we settled that I'll go deeper in tests and docs.

@nessita
Copy link
Contributor

nessita commented Aug 8, 2025

Notes about CSP decorators design.

(This summary has been written with the assistance of AI to ensure all POVs are included and phrased with as much clarity as possible.)

There have been ongoing conversations about whether to use combined or separate decorators for CSP enforcement and report_only overrides. Rob shared his perspective, which I carefully considered, and also consulted with Sarah to gather a Fellows-wide perspective. The summary of that discussion and conclusions is detailed below.

Rob pointed out that disabling CSP entirely on a view likely involves both enforced and report_only headers, so a single decorator simplifies that use case. For overriding policies, he feels that since view and template contexts usually share the same needs for both headers, a single decorator is often more practical. However, he acknowledged that the headers are separate concerns, which supports having distinct decorators in alignment with having two separated settings.

After discussing with Sarah, we considered the trade-offs between having a single csp_override decorator with boolean flags for enforced and report_only, versus separate decorators for enforced and report_only overrides.

Pros of the unified decorator:

  • Fewer decorators to remember or import.
  • Single decorator call can override both headers when they share the same config.

Cons/concerns:

  • The default behavior considering both booleans being True means a simple call might unintentionally override both enforced and report_only policies, which could lead to subtle security or debugging issues if the user only intended to override one.
  • When a user wants to override only the report_only policy (which is common for testing new policies or policy changes), they must explicitly set enforced=False to avoid overriding the enforced header, adding some cognitive overhead.
  • Since enforced and report_only policies are configured independently in settings, having separate decorators better reflects this separation and reduces the chance of user error.

Other points:

We intentionally do not validate CSP directives in middleware or decorators. An invalid report_only config, like missing report-uri, won't error in Django but may cause browser issues or defeat the purpose of the report_only CSP. Similarly, using the same config for both headers can also add report_only-only directives (e.g., report-uri) to the enforced header, potentially causing problems or unexpected results.

This makes it important to keep overrides clear and explicit by using separate decorators for enforced and report_only policies, avoiding misconfiguration.

Using two decorators can still allows for simpler and safer usage in typical cases, e.g.:

@csp_override(config1)
@csp_override_report_only(config2)
def my_view(request):
    ...

even if the config is the same:

my_policy = {...}

@csp_override(my_policy)
@csp_override_report_only({**my_policy, "report-uri": "/csp-report-endpoint/"})
def my_view(request):
    ...

Because of the above, the suggestion is to split csp_override into two decorators: one for enforced and one for report_only overrides, for clarity, reduced chance of accidental overrides, and better alignment with the independent settings and typical usage patterns. The boolean flags approach is workable but has subtle usability risks.

Thanks again for your work on this! Happy to discuss further or help with refactoring if desired.

@robhudson robhudson force-pushed the add-csp-decorators branch 2 times, most recently from 08105f0 to b9cd404 Compare August 23, 2025 19:24
@nessita nessita force-pushed the add-csp-decorators branch from b9cd404 to b6ef4e9 Compare August 27, 2025 01:56
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you @robhudson for this work! 🌟

I have pushed a simplification of the decorators definition, using some sort of "factory", and the same response attribute for override/disable (but still two attributes, one for enforced and one for report-only).

Let me know what you think, if you don't object I will give the final review tomorrow.

@nessita nessita force-pushed the add-csp-decorators branch 2 times, most recently from 89ba68d to 562ed52 Compare August 27, 2025 02:16
@nessita
Copy link
Contributor

nessita commented Aug 27, 2025

After some further though I now wonder if we should remove the _disabled variants of the decorator... people needing to disable CSP for a view would now do:

@csp_override(None)
@csp_report_only_override(None)

(any falsey value would do, empty dict, False, etc)

@robhudson
Copy link
Contributor Author

After some further though I now wonder if we should remove the _disabled variants of the decorator... people needing to disable CSP for a view would now do:

@csp_override(None)
@csp_report_only_override(None)

My only concern with this is that the intention is a bit less clear. Seeing @csp_disabled is pretty clear and self documenting what is intended. @csp_override(None) seems less so - it almost reads as "I'm not overriding anything". Due to that I'd vote a -0.

@nessita
Copy link
Contributor

nessita commented Aug 27, 2025

My only concern with this is that the intention is a bit less clear. Seeing @csp_disabled is pretty clear and self documenting what is intended. @csp_override(None) seems less so - it almost reads as "I'm not overriding anything". Due to that I'd vote a -0.

Thanks @robhudson, that's a valid point and it makes sense.

With that in mind, other option could be to make the decorator stricter and only accept dicts for the config. In that case, passing an empty dict ({}) would mean CSP is disabled, which would line up with the SECURE_CSP/SECURE_CSP_REPORT_ONLY settings default. That might make the intent clearer than allowing any falsey value, and it would avoid the somewhat ambiguous None.

@robhudson
Copy link
Contributor Author

With that in mind, other option could be to make the decorator stricter and only accept dicts for the config. In that case, passing an empty dict ({}) would mean CSP is disabled, which would line up with the SECURE_CSP/SECURE_CSP_REPORT_ONLY settings default. That might make the intent clearer than allowing any falsey value, and it would avoid the somewhat ambiguous None.

Yes, I agree that would help make the intention more clear if the preference is leaning towards a single decorator -- basically, override with an empty policy, meaning, no policy. 👍

@jacobtylerwalls
Copy link
Member

Yep, I think that's really clear.

@nessita nessita force-pushed the add-csp-decorators branch 2 times, most recently from d6d596a to 05c1369 Compare August 28, 2025 18:50
…ride or disable policies.

Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
@nessita nessita force-pushed the add-csp-decorators branch from 05c1369 to e55e220 Compare August 28, 2025 18:53
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Great work @robhudson! 🌟

I've pushed the latest suggestion on reducing the decorators to two, ad adjusted tests and docs accordingly. Let me know what you think!

(EDIT: I won't be around tomorrow so I merged this work today.)

@nessita nessita merged commit 550822b into django:main Aug 28, 2025
35 checks passed
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.

3 participants