Opened 5 months ago
Closed 18 hours ago
#36784 closed New feature (fixed)
Add CSP support to Django's script object and media objects
| Reported by: | Johannes Maron | Owned by: | Natalia Bidart |
|---|---|---|---|
| Component: | Forms | Version: | 6.0 |
| Severity: | Normal | Keywords: | |
| Cc: | Johannes Maron, Rob Hudson, Tobias Kunze, David Smith, Carsten Fuchs | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Django 5.2 added official support for a script object in media classes #35886
However, the introduction of CSP's nonce-function in Django 6.0 seems to have overlooked both old form media rendering and the script-object.
Furthermore, the template processor-based approach currently doesn't provide practical solution of object based media assets.
I'd suggest updating the media class and tag-rendering to include a nonce values by default, or the least an opt-in that doesn't require the request context in during form definition.
If there already is an easy way to add nonce-values to form media, I'd suggest that we add a few lines of documentation for the next developer looking for it.
Change History (45)
comment:1 by , 5 months ago
| Type: | Uncategorized → New feature |
|---|
comment:3 by , 5 months ago
| Cc: | added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
I've been thinking about this and I see a couple of options:
- A decent workaround would be to define a template filter that would take the nonce and include it in the tag. We could perhaps write a how-to to backport and include in 6.0.
- For
main, I agree that we should ideally have something more "first class citizen" in the objects. I'm adding a few folks as cc to see what they think.
Given the above, I'll accept pending a design discussion for the "new feature" part for 6.1. In any case, Johannes it would be super helpful if you could attach a minimal sample project showing the use cases.
comment:4 by , 5 months ago
Any thoughts on below failing testcase for above behaviour ?
from django.test import SimpleTestCase, override_settings
from django.forms import Form
from django.template import Context, Template
from django.utils.csp import CSP
class FormWithJsMedia(Form):
class Media:
js = ["path/to/js_file.js"]
@override_settings(
STATIC_URL="/static/",
MIDDLEWARE=[
"django.middleware.security.SecurityMiddleware",
"django.middleware.csp.ContentSecurityPolicyMiddleware",
],
TEMPLATES=[{
"BACKEND": "django.template.backends.django.DjangoTemplates",
"APP_DIRS": True,
"OPTIONS": {
"context_processors": [
"django.template.context_processors.request",
"django.template.context_processors.csp",
],
},
}],
SECURE_CSP={
"default-src": [CSP.SELF],
"script-src": [CSP.SELF, CSP.NONCE],
}
)
class CSPMediaTest(SimpleTestCase):
def test_form_media_js_missing_nonce(self):
form = FormWithJsMedia()
tpl = Template("{% load static %}{{ form.media }}")
rendered = tpl.render(Context({"form": form}))
self.assertIn('<script src="/static/path/to/js_file.js">', rendered)
self.assertIn('nonce="', rendered)
comment:5 by , 5 months ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:6 by , 5 months ago
The challenge seems to be that form.media does not have access to the context and is stateless. I assume this is by design. Changing this seems like a big architectural shift so I looked for other options.
One idea that I liked has two parts to it:
- Extend the
Scriptclass to add awith_nonce: bool = Falseparameter.
Example:
class MyWidget(forms.TextInput):
class Media:
js = [
"already-in-policy.js", # No nonce needed
Script("inline-script.js", with_nonce=True), # Opt-in to nonce
]
This would render the script tag with a data attribute - something harmless if the next step is forgotten (vs something like a nonce attribute with a sentinel):
<script src="..." data-csp-nonce></script>
I like the opt-in nature of this vs outputting all tags with a data attribute since, if the media is self served you likely don't need the nonce.
- Use a template filter to replace data attribute with the actual nonce
{{ form.media|with_nonce }}
The filter:
- finds and replaces the
data-csp-nonceattribute with the actual nonce from template context. - if no nonce in the context, removes the data attribute.
follow-up: 8 comment:7 by , 5 months ago
@Rish, if you don't mind, I was hoping to solve this myself. Did you make any progress yet, you'd care to share?
@Rob, I was thinking to use template nodes, instead of HTML-safe strings. So the asset objects would be rendered with the full temple context, including a nouce.
If the template includes it, we render it. Otherwise we don't. Of course, this could be added explicitly with a keyword, as you suggested.
comment:8 by , 5 months ago
Replying to Johannes Maron:
@Rish, if you don't mind, I was hoping to solve this myself. Did you make any progress yet, you'd care to share?
@Rob, I was thinking to use template nodes, instead of HTML-safe strings. So the asset objects would be rendered with the full temple context, including a nouce.
If the template includes it, we render it. Otherwise we don't. Of course, this could be added explicitly with a keyword, as you suggested.
Sorry for blocking you, I am new to this. You can have the ticket.
comment:9 by , 5 months ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
follow-up: 11 comment:10 by , 4 months ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
follow-up: 12 comment:11 by , 4 months ago
Replying to Nilesh Pahari: Hi, I noticed this ticket is currently assigned.
I’m interested in working on this issue and have started reviewing the Media
and script rendering internals. Please let me know if anyone is actively
working on it; otherwise I’d be happy to take this forward.
comment:12 by , 4 months ago
Replying to Laharyy:
Replying to Nilesh Pahari: Hi, I noticed this ticket is currently assigned.
I’m interested in working on this issue and have started reviewing the Media
and script rendering internals. Please let me know if anyone is actively
working on it; otherwise I’d be happy to take this forward.
Hi, thanks for your interest. I’m currently working on this, but I’ll definitely let you know if I need any help.
comment:13 by , 4 months ago
Hi @Laharyy, this is taking a bit longer than I initially anticipated. If you’re particularly interested, you’re welcome to take this up. I’m still working on it and may open a PR in the coming days otherwise.
comment:14 by , 2 months ago
| Owner: | changed from to |
|---|
Hi there,
Since I opened it and am fairly familiar with this part of the code base, I am taking the liberty to submit a patch myself. I hope y'all don't mind <3
Cheers!
Joe
comment:15 by , 2 months ago
| Has patch: | set |
|---|---|
| Patch needs improvement: | set |
comment:16 by , 2 months ago
| Patch needs improvement: | unset |
|---|
comment:17 by , 2 months ago
| Patch needs improvement: | set |
|---|
comment:18 by , 2 months ago
| Patch needs improvement: | unset |
|---|
comment:19 by , 8 weeks ago
| Patch needs improvement: | set |
|---|
comment:20 by , 8 weeks ago
I've left a note on the PR, but wanted to expand here on the design question. My recommendation is to go with a more explicit approach: I've been brainstorming with some LLMs and investigating different paths. The one I settled on is having a render(attrs=None) method on Script and Media classes, paired with a template filter that passes the nonce explicitly. This keeps CSP and form media as independent concerns (which I think it's important), the rendering machinery stays generic, and the filter is the only place that knows a nonce is involved and connects the two worlds.
My rationale is that both CSP and form media are opt-in features, and I think they should be combined explicitly and not behind the scenes, this is why I propose a filter since it makes the intent visible at the call site. Rob's suggestion in comment:6 points in the right direction, though I'd build on it as follows: rather than a boolean flag on Script, let's add a generic attrs dict parameter in render, since this is more consistent with Widget.render(attrs=...) elsewhere in django.forms, and is more extensible (and nonce-agnostic). Then the filter bridges both sides:
{{ form.media|with_nonce:csp_nonce }}
On the filter vs. tag question: yes, a filter cannot access the template context directly, so the nonce must be passed explicitly as the filter argument. For me, that's actually a (required) feature: it makes the machinery explicit and works regardless of what the variable is named in the context (think about an alternative implementation of CSP or a different nonce generator).
Rough sketch (names and logic to be polished):
-
django/forms/widgets.py
diff --git a/django/forms/widgets.py b/django/forms/widgets.py index 1bcfeba288..db47f0f1a2 100644
a b class MediaAsset: 82 82 return hash(self._path) 83 83 84 84 def __str__(self): 85 return self.render() 86 87 def __repr__(self): 88 return f"{type(self).__qualname__}({self._path!r})" 89 90 def render(self, *, attrs=None): 85 91 return format_html( 86 92 self.element_template, 87 93 path=self.path, 88 attributes=flatatt( self.attributes),94 attributes=flatatt({**(attrs or {}), **self.attributes}), 89 95 ) 90 96 91 def __repr__(self):92 return f"{type(self).__qualname__}({self._path!r})"93 94 97 @property 95 98 def path(self): 96 99 """ … … class Media: 142 145 def _js(self): 143 146 return self.merge(*self._js_lists) 144 147 145 def render(self ):148 def render(self, *, attrs=None): 146 149 return mark_safe( 147 150 "\n".join( 148 151 chain.from_iterable( 149 getattr(self, "render_" + name)( ) for name in MEDIA_TYPES152 getattr(self, "render_" + name)(attrs=attrs) for name in MEDIA_TYPES 150 153 ) 151 154 ) 152 155 ) 153 156 154 def render_js(self ):157 def render_js(self, *, attrs=None): 155 158 return [ 156 159 ( 157 path.__html__() 158 if hasattr(path, "__html__") 159 else format_html('<script src="{}"></script>', self.absolute_path(path)) 160 path.render(attrs=attrs) 161 if isinstance(path, MediaAsset) 162 else ( 163 path.__html__() 164 if hasattr(path, "__html__") 165 else Script(self.absolute_path(path)).render(attrs=attrs) 166 ) 160 167 ) 161 168 for path in self._js 162 169 ] 163 170 164 def render_css(self ):171 def render_css(self, *, attrs=None): 165 172 # To keep rendering order consistent, we can't just iterate over 166 173 # items(). We need to sort the keys, and iterate over the sorted list. 167 174 media = sorted(self._css) 168 175 return chain.from_iterable( 169 176 [ 170 177 ( 171 path.__html__() 172 if hasattr(path, "__html__") 173 else format_html( 174 '<link href="{}" media="{}" rel="stylesheet">', 175 self.absolute_path(path), 176 medium, 178 path.render(attrs=attrs) 179 if isinstance(path, MediaAsset) 180 else ( 181 path.__html__() 182 if hasattr(path, "__html__") 183 else format_html( 184 '<link href="{}" media="{}" {} rel="stylesheet">', 185 self.absolute_path(path), 186 medium, 187 flatatt(attrs or {}), 188 ) 177 189 ) 178 190 ) 179 191 for path in self._css[medium] -
new file django/templatetags/media.py
diff --git a/django/templatetags/media.py b/django/templatetags/media.py new file mode 100644 index 0000000000..c9c84e9042
- + 1 from django import template 2 3 register = template.Library() 4 5 6 @register.filter 7 def with_nonce(media, nonce): 8 """ 9 Render a Media object with a CSP nonce applied to all script and link tags. 10 11 Usage:: 12 13 {% load media %} 14 {{ form.media|with_nonce:csp_nonce }} 15 """ 16 return media.render(attrs={"nonce": nonce} if nonce else None)
comment:21 by , 7 weeks ago
I find the reasoning behind Natalia's proposal persuasive, and I also like the parallel explicitness with the explicit provision of {{ csp_nonce }} in <script> tags.
comment:22 by , 6 weeks ago
| Cc: | added |
|---|
comment:23 by , 6 weeks ago
| Owner: | changed from to |
|---|
Given that I have a draft branch for the solution I proposed above, I may take ownership of the ticket and polish the PR. @codingjoe I would appreciate if you would be able to provide reviews!
comment:24 by , 6 weeks ago
| Owner: | changed from to |
|---|
Hi there,
Thank you for your patience. I have a newborn at home and don't get to work on Django every day.
A separate tag was my initial solution before changing it to a more "magical" solution based on community feedback.
Since I only need to revert the changes that I already have, I would very much appreciate continuing to work on this myself. Natalia, please reopen my initial PR so we don't lose the history for any reviewer.
Lastly, I don't feel comfortable moving forward without a decision from the steering committee. There has been too much back and forth on potential solutions.
Especially a filter solution has sizable architectural implications, which I will try to outline for the committee later today.
Best
Joe
comment:25 by , 6 weeks ago
Request for Comment: CSP nonce in form media
Goal: We want to provide a way to render form media with CSP nonce values (currently impossible).
There are multiple ways to go about this, all with their pros and cons. We should put a strong emphasis on dev usability (for people with deadlines) while weighing the security considerations (marked with an (S)). Importantly, any approach (other than a tag) may set a precedent and must be considered beyond the scope of this ticket.
I would kindly ask the steering council and the security team for an official comment, including a short explanation for future reference.
Tag-based
{% with_nonce form.media %}
| Pros | Cons |
|---|---|
| No changes to the template engine are required. | Requires updates to admin templates & 3rd-party packages. |
Consistent with current logic, that rendering is handled in tags, like csrf. | Fairly code-intense in Django. |
| (S) Explicit choice to add nonce values to form media. |
Filter-based w/ explicit nonce
{% if csp_nonce %}
{{ form.media|with_nonce:csp_nonce }}
{% endif %}
<!-- or -->
{% if csp_nonce %}
{{ form.media|context:csp_nonce }}
{% endif %}
| Pros | Cons |
|---|---|
| No changes to the template engine are required. | Requires updates to admin templates & 3rd-party packages. |
| (S) Explicit choice to add nonce values to form media. | Requires case handling to prevent VariableDoesNotExist in Django's admin and 3rd-party packages. (The context processor needs to be added to a project.)
|
| Fairly complex template syntax, including branching. |
Filter-based w/ implicit context
{{ form.media|with_nonce }}
<!-- or -->
{{ form.media|context:"csp_nonce" }}
| Pros | Cons |
|---|---|
| (S) Explicit choice to add nonce values to form media. | Requires updates to admin templates & 3rd-party packages. |
A context filter would be reusable. | A context filter would be reusable and requires some form of a public interface.
|
| (S) Requires changes to the template engines. Filters are currently not context-aware. |
Automatic injection (if nonce in context)
{{ form.media }}
| Pros | Cons |
|---|---|
| No updates to existing templates required (incl. 3rd party). | (S) No explicit opt-in nonce in form media. However, the context processor and the nonce option in CSP need to be enabled.
|
| Simple (as is) syntax. | (S) Requires a new "special" object that gets rendered with a nonce. |
Author's note
I have implemented all versions and don't have a strong preference. Tag-based seems the most in line with Django's current idioms, yet I do like the simplicity of the automatic approach. Both filter approaches come with a fair bit of complexity for either Django or its users, which is why I, personally, wouldn't endorse them. The tag-based implementation was my first approach; however, it does require more new code than any other solution (similar to CSRF).
comment:26 by , 6 weeks ago
Can I ask for some clarification on the security angle? It seems all of the "cons" reduce to the proposition that we trust at least some portion of the request.context. (Either potentially *any* key with a reusable context filter, or just a hard-coded key for csp nonces.) I thought request.context was already trusted, so I'm having to grasp at straws to imagine the concern: is it that in a stack of reusable apps not totally under your control, you cannot reason about whether or not the contents of request.context have been sanitized if user-controlled? Or something else?
I'm glad to have the Steering Council's eyes here to ratify an approach, as I'm very keen to advance this for 6.1 (feature freeze mid-May).
comment:27 by , 6 weeks ago
I think for most Django devs who are not super familiar with CSP, but still want to build out robust applications, avoiding {{ form.media|with_nonce:csp_nonce }} would be ideal. On the Django Discord, there were several long-time django devs who agreed that the automatic injection of the nonce would be a preferable API, with nobody preferring the explicit template tag.
comment:28 by , 6 weeks ago
Hi Jacob,
hm… I hoped to point to more cons than just security considerations in my previous comment. If you are referring to my deliberation on the PR, yes, that one is a little trickier.
I believe my point isn't "can we trust the context" (We don't; that's why we're escaping template variables.) – It's rather, can we trust everyone with the context? A radical example would be changing __html__ to always ingest the full context. I would consider this VERY unsafe (to an injection attack). We should always explicitly limit the context, but there are multiple ways to do it.
Unrelated to the context, my other concern was that you might not trust a 3rd party to use safe script sources. I may trust the package, but not its supply chain. E.G., Django's GeoAdmin uses scripts from a CDN, which opens the door to a supply chain attack. The automatic solution would be to unknowingly trust those resources. It does go against the idea of CSP a little, where you want to explicitly review and whitelist browser resources.
My main concern with any solution is making any changes to the template engine. Both Django and Jinja had their fair share of security vulnerabilities in their early days. New code simply means a new opportunity to screw up. I consider myself experienced enough to know that I make mistakes :P
Lastly, yes, 6.1 would be great, since this has been a bit of a pain point in multiple issues. But Django has thought of patience. ;)
Hi Tim,
Yes, that's why I emphasized the "people with deadlines" part. It should be easy to do the right (secure) thing. Now we just need to figure out what that is.
Cheers,
Joe
comment:29 by , 6 weeks ago
Thank you everyone for the further commentary.
Given that there is a closely related ticket (#36825) covering the same problem space from the admin's angle, and that Trac is not the best venue to gather broad community input on a design question of this scope (audience here is very limited), I will open a Django Forum post to consolidate and continue the discussion. A link will be added here once it is up, and I would kindly ask everyone to follow up there. The Security team and SC are present on that platform and more "pingeable", while the broader community can also contribute to the conversation.
comment:30 by , 6 weeks ago
comment:31 by , 9 days ago
I just made an initial review of the PR that was announced on that forum thread. It follows option 1, the template-tag approach. The code is not very involved. IMO, among the "cons" from Joe's table in comment:25, all that leaves are the updates needed in 3rd party packages, which is spun as a "pro" (manual review) by some on the forum thread.
In other words, I think the PR looks very good. I left small questions.
Returning to this earlier aside:
I believe my point isn't "can we trust the context" (We don't; that's why we're escaping template variables.)
My point was that we should be able to trust that the context does not contain unexpected, arbitrary key-value pairs. (That is to say, we would not expect an attacker would be able to get {"csp_nonce": "evil"} into the context somehow.) That's different from acknowledging that the DTL auto-HTML-escapes template variables. Does that sound right?
(I'd also be very happy to see additional reviews on the above PR!)
follow-up: 33 comment:32 by , 8 days ago
Natalia, I think we reached somewhat of a consensus on the form, right? So even without the steering council, I'd be ok to push forward.
I'll continue my work later today, to be mindful of your time, and add you as a co-author.
comment:33 by , 3 days ago
| Owner: | changed from to |
|---|
Replying to Johannes Maron:
Natalia, I think we reached somewhat of a consensus on the form, right? So even without the steering council, I'd be ok to push forward.
That was my understanding also!
I'll continue my work later today, to be mindful of your time, and add you as a co-author.
I mentioned reviewing Natalia's PR in comment:31. The feature freeze is in two weeks, so for me, the most practical thing would be to iterate on that PR, as it's already had one thorough review. We will be mindful as always to credit co-authors and reviewers, of course. Would that work? I would very much appreciate having your review before we land anything!
comment:34 by , 3 days ago
I'll work on incorporating the PR feedback today. Johannes, your review is very much appreciated!
comment:35 by , 2 days ago
| Owner: | changed from to |
|---|---|
| Patch needs improvement: | unset |
Don't get this the wrong way, but I already FINISHED the work and @-mentioned Natalia over a week ago.
Just review my PR. I have time until the feature freeze to move this over the finish line.
I didn't update Trac because I cared about Natalia's feedback before getting more fellows involved again.
follow-up: 37 comment:36 by , 2 days ago
Please don't take this the wrong way, and I didn't say this before, but since we're here again, it feels fairly discouraging to have a contribution hijacked that people committed a lot of time and effort to. The support is applaudable, but I also see how first-time contributors might be discouraged by this.
follow-up: 39 comment:37 by , 2 days ago
Replying to Johannes Maron:
Please don't take this the wrong way, and I didn't say this before, but since we're here again, it feels fairly discouraging to have a contribution hijacked that people committed a lot of time and effort to. The support is applaudable, but I also see how first-time contributors might be discouraged by this.
Thank you for raising this, I appreciate your honesty. From my view, I think it would be important to consider the earlier comments and proposals, as the current framing does not seem to fully reflect that. As it stands, it comes across as somewhat incomplete, which I feel is a bit unfair. My PR has been available and ready for review since March 24, as part of the earlier proposal and follow-up discussion. That work involved revisiting the initial design direction and refining it toward what I believe is a more Django-idiomatic approach.
Timeline wise, on March 24th, I indicated on comment:23 that I had a draft branch for an alternative approach and that I would take ownership to polish it, explicitly inviting reviews. On March 25, I created a forum post including a polished PR implementing that option. Given that sequence, my PR was not intended as taking over someone else's work, but rather continuing along the process of proposing, validating, and refining solutions in the open, with concrete implementations to support discussion.
Looking at the current PRs, there is clear alignment in direction (explicit opt-in via template tag usage), but a notable difference in architectural approach. One of the approaches on PR #20763 in particular:
- Introduces tight coupling between forms/media and CSP, pulling a request-specific concern (the nonce) into what is otherwise a static, declarative API.
- Makes
Mediarendering nonce-aware withrender(nonce=...), making the nonce receive special treatment in forms definitions, rather than following Django's existing, generic attribute injection pattern. - Bounds the solution to DTL internals and does not keep the path open for other template engines.
From a process perspective, you are already a co-author on the PR #21010, and your work done so far has directly contributed to moving this forward. The goal here is to converge on the right design rather than competing implementations. I would lean towards the option that minimizes coupling and keeps concerns clearly separated.
Lastly, it would be very valuable to have your review on my PR, since it would help ensure alignment on the current state, progress, and next steps.
comment:38 by , 2 days ago
| Owner: | changed from to |
|---|
comment:39 by , 2 days ago
Replying to Natalia Bidart:
Timeline wise, on March 24th, I indicated on comment:23 that I had a draft branch for an alternative approach and that I would take ownership to polish it, explicitly inviting reviews. On March 25, I created a forum post including a polished PR implementing that option. Given that sequence, my PR was not intended as taking over someone else's work, but rather continuing along the process of proposing, validating, and refining solutions in the open, with concrete implementations to support discussion.
Very kindly, I don't believe this is a healthy discussion that will lead to a meaningful outcome.
I have reviewed your PR prior to posting the RFC to the steering council and the security team but didn't leave any comments to be mindful of your time. I will now do another thorough round.
Introduces tight coupling between forms/media and CSP, pulling a request-specific concern (the nonce) into what is otherwise a static, declarative API.
I was aiming for specificity deliberately to minimize vulnerability vectors. It is a security feature, and the ability to inject attributes to static assets in the template is new.
Makes Media rendering nonce-aware with render(nonce=...), making the nonce receive special treatment in forms definitions, rather than following Django's existing, generic attribute injection pattern.
Same as above.
Bounds the solution to DTL internals and does not keep the path open for other template engines.
Where though? Maybe we are talking about different code?
Lastly, it would be very valuable to have your review on my PR, since it would help ensure alignment on the current state, progress, and next steps.
Thank you, I would have appreciated a review from you too.
comment:40 by , 2 days ago
| Needs tests: | set |
|---|---|
| Patch needs improvement: | set |
comment:41 by , 25 hours ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
Isolated the work solving this issue alone in this PR.
comment:42 by , 24 hours ago
| Needs tests: | set |
|---|---|
| Patch needs improvement: | set |
comment:43 by , 21 hours ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:44 by , 21 hours ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Hello Johannes! Thank you for this report. Could you please share a small project sample that would highlight how CSP is lacking support for the cases you listed? That would help me greatly to properly triage this ticket.