Refs #36532 - Added CSP view decorators. #19680
Conversation
robhudson
force-pushed
the
add-csp-decorators
branch
4 times, most recently
from
e8b5379 to
a12f683
Compare
robhudson
force-pushed
the
add-csp-decorators
branch
from
a12f683 to
901752b
Compare
robhudson
changed the title
There was a problem hiding this comment.
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.
| for header, config in [ | ||
| (CSP.HEADER_ENFORCE, settings.SECURE_CSP), | ||
| (CSP.HEADER_REPORT_ONLY, settings.SECURE_CSP_REPORT_ONLY), | ||
| for config, header, disabled in [ |
There was a problem hiding this comment.
Small nitpick, could we keep the previous order, adding the disabled at the end?
| for config, header, disabled in [ | |
| for header, config, disabled in [ |
There was a problem hiding this comment.
I switched it to match the order it is being used in the conditional within this loop. This wasn't an arbitrary change I just felt it read better with the 3 conditionals after following the same order.
That order could be discussed too, but I feel like config makes sense first. The header not in response and the not disabled could be swapped with the assumption that using the decorators would happen more often than a custom header set in the view.
I'm open to however you'd like to tweak these but thought I'd share my thought process.
| from asgiref.sync import iscoroutinefunction | ||
|
|
||
|
|
||
| def csp_override(config, enforced=True, report_only=True): |
There was a problem hiding this comment.
Did you consider providing two independent decorators? perhaps this is a nicer API?
What I like is that there is a clear separation of concerns, and if both config need tweak, both decorators can be used. What's unclear is whether we could reuse code between the two definitions... I'm torn about code dup with the benefit of clear separation vs DRY and an arguably less clean API.
I'm inclined to split them given that we have two separated settings for these. What do you think?
| def csp_override(config, enforced=True, report_only=True): | |
| def csp_override(config): | |
| def decorator(view_func): | |
| @wraps(view_func) | |
| async def _wrapped_async_view(request, *args, **kwargs): | |
| response = await view_func(request, *args, **kwargs) | |
| response._csp_config = config | |
| return response | |
| @wraps(view_func) | |
| def _wrapped_sync_view(request, *args, **kwargs): | |
| response = view_func(request, *args, **kwargs) | |
| response._csp_config = config | |
| return response | |
| # Determine whether to wrap as async or sync function | |
| if iscoroutinefunction(view_func): | |
| return _wrapped_async_view | |
| return _wrapped_sync_view | |
| return decorator | |
| def csp_override_report_only(config): | |
| def decorator(view_func): | |
| @wraps(view_func) | |
| async def _wrapped_async_view(request, *args, **kwargs): | |
| response = await view_func(request, *args, **kwargs) | |
| response._csp_config_ro = config | |
| return response | |
| @wraps(view_func) | |
| def _wrapped_sync_view(request, *args, **kwargs): | |
| response = view_func(request, *args, **kwargs) | |
| response._csp_config_ro = config | |
| return response | |
| # Determine whether to wrap as async or sync function | |
| if iscoroutinefunction(view_func): | |
| return _wrapped_async_view | |
| return _wrapped_sync_view | |
| return decorator |
There was a problem hiding this comment.
It was slightly mentioned in the original PR here:
#18215 (review)
It's hard to think of what the most common use case would be. For disabling the CSP on a view altogether I would imagine it would be more common to want to disable both the enforced and report-only. So having a single decorator makes that nice, otherwise you'd have to stack 2 decorators.
For the overrides I'm not sure. You may be overriding the CSP for the report-only header perhaps, but then why not use the setting? It's likely you'd want to override both as well I imagine since there's something in the view + template that needs the override and needs to override both.
I do think there's a valid argument that they are separate headers so they should be separate decorators. But I also think the context of the view + template typically sharing the same needs for both headers also sways the argument a bit towards being a single decorator.
Curious what your further thoughts are.
| @@ -70,6 +70,10 @@ The resulting ``Content-Security-Policy`` header would be set to: | |||
|
|
|||
| default-src 'self'; script-src 'self' 'nonce-SECRET'; img-src 'self' https: | |||
|
|
|||
| Per-view policy customization with can be achieved via the | |||
There was a problem hiding this comment.
| Per-view policy customization with can be achieved via the | |
| Per-view policy customization can be achieved via the |
Trac ticket number
ticket-36532
Branch description
This adds view decorators for adjusting the CSP configuration per-view.
Checklist
mainbranch.