-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
Refs #36532 - Added CSP view decorators. #19680
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
a12f683
to
901752b
Compare
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.
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.
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 Pros of the unified decorator:
Cons/concerns:
Other points: We intentionally do not validate CSP directives in middleware or decorators. An invalid report_only config, like missing 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 Thanks again for your work on this! Happy to discuss further or help with refactoring if desired. |
08105f0
to
b9cd404
Compare
b9cd404
to
b6ef4e9
Compare
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.
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.
89ba68d
to
562ed52
Compare
After some further though I now wonder if we should remove the @csp_override(None)
@csp_report_only_override(None) (any falsey value would do, empty dict, False, etc) |
My only concern with this is that the intention is a bit less clear. Seeing |
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 ( |
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. 👍 |
Yep, I think that's really clear. |
d6d596a
to
05c1369
Compare
…ride or disable policies. Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
05c1369
to
e55e220
Compare
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.
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.)
Trac ticket number
ticket-36532
Branch description
This adds view decorators for adjusting the CSP configuration per-view.
Checklist
main
branch.