csrf_failure and bad_request views missing context processors

[Alex Henman]

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)

[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 incompatible.

Do note that the request should be pass in the context and not as an extra param:

django/views/csrf.py

@@ -64 +64 @@
         "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)

[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.

[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 ​contributing documentation for submitting patches.

Thank you for your interest in contributing!

[Amir Karimi]

Hey @faizan2700
As you didn't pick up this issue, if you don't mind, I assign it to myself.

[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!

[Alex Henman]

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

@@ -64 +64 @@
         "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:

    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

[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 in django/template/base.py which render method is defined as def render(self, context). I hope this helps!

[Alex Henman]

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 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

[Natalia <124304+nessita@…>]

In 535f7b5:

Fixed #34830 -- Added request to csrf_failure view's template context.

Co-authored-by: nessita <124304+nessita@…>

[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."

[Natalia Bidart]

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 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. It still needs tests and some validation that this is an acceptable solution:

django/views/csrf.py

@@ -64 +64 @@
         "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 +72 @@
             # 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)}}}

[Tim Graham]

I think this might be the way to go:

django/views/csrf.py

@@ -72 +72 @@
             # 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 ​similar code in django.views.default.page_not_found.

[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 (​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:

[Tim Graham]

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.

[Natalia <124304+nessita@…>]

In 5f2f12f6:

Reverted "Fixed #34830 -- Added request to csrf_failure view's template context."

This reverts commit 535f7b5c6cea54a0796d85bbe213183d50002689.

[Natalia Bidart]

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.

[yushan]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 14b0132:

Fixed #34830 -- Added request to bad_request/csrf_failure view template contexts.