[Django] #34830: csrf_failure view missing context processors
Django
---------------------------------------+------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: 4.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+------------------------
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:
{{{
#!python
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)`
--
Ticket URL: <https://code.djangoproject.com/ticket/34830>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* version: 4.2 => dev
* stage: Unreviewed => Accepted
Comment:
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:
{{{#!diff
--- a/django/views/csrf.py
+++ b/django/views/csrf.py
@@ -64,6 +64,7 @@ def csrf_failure(request, reason="",
template_name=CSRF_FAILURE_TEMPLATE_NAME):
"DEBUG": settings.DEBUG,
"docs_version": get_docs_version(),
"more": _("More information is available with DEBUG=True."),
+ "request": request,
}
try:
t = loader.get_template(template_name)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:1>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by faizan2700):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:2>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Natalia Bidart):
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
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/submitting-patches/ contributing documentation for submitting patches].
Thank you for your interest in contributing!
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:3>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Amir Karimi):
Hey @faizan2700
As you didn't pick up this issue, if you don't mind, I assign it to
myself.
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:4>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Amir Karimi):
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:5>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Alex Henman):
Replying to [comment:1 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:
>
> {{{#!diff
> --- a/django/views/csrf.py
> +++ b/django/views/csrf.py
> @@ -64,6 +64,7 @@ def csrf_failure(request, reason="",
template_name=CSRF_FAILURE_TEMPLATE_NAME):
> "DEBUG": settings.DEBUG,
> "docs_version": get_docs_version(),
> "more": _("More information is available with DEBUG=True."),
> + "request": request,
> }
> try:
> 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:
{{{#!python
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:
{{{#!python
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:
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:6>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:6 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:7>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by Alex Henman):
Replying to [comment:7 Natalia Bidart]:
> Replying to [comment:6 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!
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
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:8>
Django
-----------------------------+------------------------------------
Reporter: Alex Henman | Owner: nobody
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:9>
Django
Reporter: Alex Henman | Owner: Prakhar Parashari
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* owner: nobody => Prakhar Parashari
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:10>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
Changes (by Natalia Bidart):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:11>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
Changes (by Natalia Bidart):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:12>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"535f7b5c6cea54a0796d85bbe213183d50002689" 535f7b5]:
{{{
#!CommitTicketReference repository=""
revision="535f7b5c6cea54a0796d85bbe213183d50002689"
Fixed #34830 -- Added request to csrf_failure view's template context.
Co-authored-by: nessita <124304+...@users.noreply.github.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:13>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Type: Bug | Status: closed
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
Comment (by Tim Graham):
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."
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:14>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Type: Bug | Status: closed
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:14 Tim Graham]:
> 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."
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:
{{{
#!diff
diff --git a/django/views/csrf.py b/django/views/csrf.py
index e282ebb2b6..8da5f2b082 100644
--- a/django/views/csrf.py
+++ b/django/views/csrf.py
@@ -64,7 +64,6 @@ def csrf_failure(request, reason="",
template_name=CSRF_FAILURE_TEMPLATE_NAME):
"DEBUG": settings.DEBUG,
"docs_version": get_docs_version(),
"more": _("More information is available with DEBUG=True."),
- "request": request,
}
try:
t = loader.get_template(template_name)
@@ -73,8 +72,12 @@ def csrf_failure(request, reason="",
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))
+ try:
+ response = t.render(c, request=request)
+ except TypeError:
+ c["request"] = request
+ response = t.render(Context(c))
+ return HttpResponseForbidden(response)}}}
which would need some unit tests and perhaps some further analysis of how
robust this solution is. Do you have an opinion?
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:15>
Django
-----------------------------+---------------------------------------------
Reporter: Alex Henman | Owner: Prakhar Parashari
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
I think this might be the way to go:
{{{ #!diff
diff --git a/django/views/csrf.py b/django/views/csrf.py
index 3c572a621a..60c564b809 100644
--- a/django/views/csrf.py
+++ b/django/views/csrf.py
@@ -72,8 +72,10 @@ def csrf_failure(request, reason="",
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)
+ body = t.render(Context(c))
else:
# Raise if a developer-specified template doesn't exist.
raise
- return HttpResponseForbidden(t.render(c))
+ else:
+ body = t.render(c, request)
+ return HttpResponseForbidden(body)
}}}
See
[https://github.com/django/django/blob/535f7b5c6cea54a0796d85bbe213183d50002689/django/views/defaults.py#L64-L78
similar code] in `django.views.default.page_not_found`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:16>
Django
-----------------------------+------------------------------------------
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------------
* owner: Prakhar Parashari => Natalia Bidart
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:17>
Django
-----------------------------+------------------------------------------
Reporter: Alex Henman | Owner: Natalia Bidart
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------------
Comment (by Natalia Bidart):
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
([https://github.com/django/django/commit/dc4f726df9 commit]), but
`server_error` got it quickly replaced by a `Context` in
[https://github.com/django/django/commit/10229c4a1e8532e31310a6e2a74fe12ed2b3b26b
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
[https://github.com/django/django/commit/1ca6e9b9e24240033349c93b05902c79c0a25bbb
this commit].
Then,
[https://github.com/django/django/commit/d228c1192ed59ab0114d9eba82ac99df611652d2
bad_request] and
[https://github.com/django/django/commit/16945f0e9c57aeabadb6f2e2f150a2687455be40
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:
--
Ticket URL: <https://code.djangoproject.com/ticket/34830#comment:18>