Opened 12 months ago
Closed 9 months ago
#34830 closed Bug (fixed)
csrf_failure and bad_request views missing context processors
Reported by: | Alex Henman | Owned by: | yushan |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
The default csrf_failure
view does not pass the request to the template rendering engine which means that all context processors are missing.
This is problematic if you override the default 403_csrf.html
template without customising the view and are expecting the same default context you would get access to in other templates.
I think the most straight forward way to replicate on a default Django deployment would be to add a custom 403_csrf.html
template to your templates dir and attempt to access from some of Django's built-in context processors e.g. request
or TIME_ZONE
The fix should be very straight forward unless there's a good reason not to pass the request to the template engine in this view. The view currently looks like this:
def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME): """ Default view used when request fails CSRF protection """ from django.middleware.csrf import REASON_NO_CSRF_COOKIE, REASON_NO_REFERER c = { "title": _("Forbidden"), ... } try: t = loader.get_template(template_name) except TemplateDoesNotExist: if template_name == CSRF_FAILURE_TEMPLATE_NAME: # If the default template doesn't exist, use the fallback template. with builtin_template_path("csrf_403.html").open(encoding="utf-8") as fh: t = Engine().from_string(fh.read()) c = Context(c) else: # Raise if a developer-specified template doesn't exist. raise return HttpResponseForbidden(t.render(c))
So it just needs modifying to t.render(c, request)
Change History (28)
follow-up: 6 comment:1 by , 12 months ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 4.2 → dev |
comment:2 by , 12 months ago
Hello, please assign me this issue. I am working on django for about 3 years, I would love to get started contributing to this amazing repository.
comment:3 by , 12 months ago
Hello faizan2700, you can assign the ticket yourself once you are ready to start working on it. You can use the "assign to" box in this page. If you haven't already, please go over the contributing documentation for submitting patches.
Thank you for your interest in contributing!
comment:4 by , 12 months ago
Hey @faizan2700
As you didn't pick up this issue, if you don't mind, I assign it to myself.
comment:5 by , 12 months ago
I think based on the issue description, in addition to the request, maybe settings need to be provided to get the timezone. Something like that:
"more": _("More information is available with DEBUG=True."), "request": request, "settings": reporter_filter.get_safe_settings(), }
Not sure, just curious!
follow-up: 7 comment:6 by , 12 months ago
Replying to Natalia Bidart:
Accepting since it's easily reproducible and the proposed fix makes sense. As far as I see, the change should not be backwards compatible.
Do note that the request should be pass in the context and not as an extra param:
django/views/csrf.py
a b def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME): 64 64 "DEBUG": settings.DEBUG, 65 65 "docs_version": get_docs_version(), 66 66 "more": _("More information is available with DEBUG=True."), 67 "request": request, 67 68 } 68 69 try: 69 70 t = loader.get_template(template_name)
Sorry I had a slightly different understanding of the issue here but I'm not super familiar with the internals of Django's template rendering so tell me if I'm wrong.
The render method takes an extra request argument as well as the context:
def render(self, context=None, request=None): context = make_context( context, request, autoescape=self.backend.engine.autoescape ) try: return self.template.render(context) except TemplateDoesNotExist as exc: reraise(exc, self.backend)
And that make_context
does:
def make_context(context, request=None, **kwargs): """ Create a suitable Context from a plain dict and optionally an HttpRequest. """ if context is not None and not isinstance(context, dict): raise TypeError( "context must be a dict rather than %s." % context.__class__.__name__ ) if request is None: context = Context(context, **kwargs) else: # The following pattern is required to ensure values from # context override those from template context processors. original_context = context context = RequestContext(request, **kwargs) if original_context: context.push(original_context) return context
And it is inside RequestContext
rather than Context
that the context processor magic happens:
def bind_template(self, template): if self.template is not None: raise RuntimeError("Context is already bound to a template") self.template = template # Set context processors according to the template engine's settings. processors = template.engine.template_context_processors + self._processors updates = {} for processor in processors: context = processor(self.request)
So I thought the fix was to explicitly pass the request
rather than add it to the context dict
follow-up: 8 comment:7 by , 12 months ago
Replying to Alex Henman:
So I thought the fix was to explicitly pass the
request
rather than add it to the context dict
My advice would be to try your patch and run the tests :-) (this is what I did when reproducing/accepting the ticket). Spoiler alert, some tests fail with:
TypeError: Template.render() got an unexpected keyword argument 'request'
This is why the Template
class that is being used is the one defined in django/template/base.py
which render
method is defined as def render(self, context)
. I hope this helps!
comment:8 by , 12 months ago
Replying to Natalia Bidart:
Replying to Alex Henman:
So I thought the fix was to explicitly pass the
request
rather than add it to the context dict
My advice would be to try your patch and run the tests :-) (this is what I did when reproducing/accepting the ticket). Spoiler alert, some tests fail with:
TypeError: Template.render() got an unexpected keyword argument 'request'This is why the
Template
class that is being used is the one defined indjango/template/base.py
whichrender
method is defined asdef render(self, context)
. I hope this helps!
Ahh I see: sorry I was just trying to help out those who were keen to take on working on a fix. I don't really have a working Django development environment set up so haven't been able to test out any of my suggested changes here.
I think the key thing is that just passing request
in to the context might not be enough as for my use case what I want is the context processors in my configured template backend. That is perhaps not as simple as I'd hoped then
comment:9 by , 11 months ago
Cc: | added |
---|
comment:10 by , 10 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:11 by , 10 months ago
Patch needs improvement: | set |
---|
comment:12 by , 10 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
follow-up: 15 comment:14 by , 10 months ago
As Alex stated in comment 8, I don't think passing the request
like this causes the template to be rendered with RequestContext
(which makes the values from context processors available in the template). The ticket summary should at least be retitled to reflect what was actually changed, e.g. "Add request to csrf_failure view context."
comment:15 by , 10 months ago
Replying to Tim Graham:
As Alex stated in comment 8, I don't think passing the
request
like this causes the template to be rendered withRequestContext
(which makes the values from context processors available in the template). The ticket summary should at least be retitled to reflect what was actually changed, e.g. "Add request to csrf_failure view context."
Thank you Tim for the comment, indeed your have a valid point. I started drafting a possible solution so a RequestContext
is used, I ended up with this diff. It still needs tests and some validation that this is an acceptable solution:
-
django/views/csrf.py
diff --git a/django/views/csrf.py b/django/views/csrf.py index e282ebb2b6..8da5f2b082 100644
a b def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME): 64 64 "DEBUG": settings.DEBUG, 65 65 "docs_version": get_docs_version(), 66 66 "more": _("More information is available with DEBUG=True."), 67 "request": request,68 67 } 69 68 try: 70 69 t = loader.get_template(template_name) … … def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME): 73 72 # If the default template doesn't exist, use the fallback template. 74 73 with builtin_template_path("csrf_403.html").open(encoding="utf-8") as fh: 75 74 t = Engine().from_string(fh.read()) 76 c = Context(c)77 75 else: 78 76 # Raise if a developer-specified template doesn't exist. 79 77 raise 80 return HttpResponseForbidden(t.render(c)) 78 try: 79 response = t.render(c, request=request) 80 except TypeError: 81 c["request"] = request 82 response = t.render(Context(c)) 83 return HttpResponseForbidden(response)}}}
comment:16 by , 10 months ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
I think this might be the way to go:
-
django/views/csrf.py
diff --git a/django/views/csrf.py b/django/views/csrf.py index 3c572a621a..60c564b809 100644
a b def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME): 72 72 # If the default template doesn't exist, use the fallback template. 73 73 with builtin_template_path("csrf_403.html").open(encoding="utf-8") as fh: 74 74 t = Engine().from_string(fh.read()) 75 c = Context(c)75 body = t.render(Context(c)) 76 76 else: 77 77 # Raise if a developer-specified template doesn't exist. 78 78 raise 79 return HttpResponseForbidden(t.render(c)) 79 else: 80 body = t.render(c, request) 81 return HttpResponseForbidden(body)
See similar code in django.views.default.page_not_found
.
comment:17 by , 10 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:18 by , 10 months ago
Thank you Tim for the pointer. After some investigation, I see that server_error
and bad_request
suffer from the same issue (request
is not passed when rendering the loaded/custom template).
I did some git history analysis and both page_not_found
and server_error
rendering got a RequestContext
added in #688 (commit), but server_error
got it quickly replaced by a Context
in this commit to "lessen the chance that the 500 view would raise an error in itself".
OTOH, permission_denied
was built with a RequestContext
from the start when fixing #9847 in this commit.
Then, bad_request and csrf_failure views were "born" without getting the request passed when rendering the template, and one could argue that these should be doing a similar template handling to what page_not_found
is doing.
So, at this point, I'm guessing we should leave server_error
as is, but I'm inclined to fix both bad_request
and csrf_failure
views, effectively matching what page_not_found
provides. I'm unclear on whether we should re-title this ticket as you originally proposed, and create a new one to "fix" the mentioned views; or re-title this ticket to something like "Missing context processors in bad_request and csrf_failure views" and tackle the same conceptual fix in one (follow up) PR.
Any preference/guidance? Thanks again! I have TIL a lot today :partying_face:
comment:19 by , 10 months ago
Summary: | csrf_failure view missing context processors → csrf_failure and bad_request views missing context processors |
---|
Sounds good. I think I would revert the original commit here, then add a commit or two to fix the two views. It's simple enough that those commits could all be in the same PR.
comment:21 by , 10 months ago
Resolution: | fixed |
---|---|
Status: | closed → new |
comment:22 by , 10 months ago
Component: | CSRF → Core (Other) |
---|---|
Easy pickings: | set |
Owner: | removed |
Status: | new → assigned |
De-assigning myself in case someone else can grab this before I get to it. The commentary explains in details what should be done to properly solve the ticket.
comment:23 by , 10 months ago
Status: | assigned → new |
---|
comment:24 by , 10 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:26 by , 9 months ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:27 by , 9 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
Accepting since it's easily reproducible and the proposed fix makes sense. As far as I see, the change should not be backwards incompatible.
Do note that the request should be pass in the context and not as an extra param:
django/views/csrf.py