Opened 8 months ago

Closed 6 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)

comment:1 by Natalia Bidart, 8 months ago

Triage Stage: UnreviewedAccepted
Version: 4.2dev

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

    a b def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME):  
    6464        "DEBUG": settings.DEBUG,
    6565        "docs_version": get_docs_version(),
    6666        "more": _("More information is available with DEBUG=True."),
     67        "request": request,
    6768    }
    6869    try:
    6970        t = loader.get_template(template_name)
Last edited 8 months ago by Natalia Bidart (previous) (diff)

comment:2 by faizan2700, 8 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 Natalia Bidart, 8 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 Amir Karimi, 8 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 Amir Karimi, 8 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(),
    }

Like HttpResponseNotFound. Not sure, just curious!

Last edited 8 months ago by Amir Karimi (previous) (diff)

in reply to:  1 ; comment:6 by Alex Henman, 8 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):  
    6464        "DEBUG": settings.DEBUG,
    6565        "docs_version": get_docs_version(),
    6666        "more": _("More information is available with DEBUG=True."),
     67        "request": request,
    6768    }
    6869    try:
    6970        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

in reply to:  6 ; comment:7 by Natalia Bidart, 8 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!

in reply to:  7 comment:8 by Alex Henman, 8 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 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

comment:9 by David Wobrock, 8 months ago

Cc: David Wobrock added

comment:10 by Natalia Bidart, 6 months ago

Has patch: set
Owner: changed from nobody to Prakhar Parashari
Status: newassigned

comment:11 by Natalia Bidart, 6 months ago

Patch needs improvement: set

comment:12 by Natalia Bidart, 6 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Natalia <124304+nessita@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 535f7b5:

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

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

comment:14 by Tim Graham, 6 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."

in reply to:  14 comment:15 by Natalia Bidart, 6 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 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

    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):  
    6464        "DEBUG": settings.DEBUG,
    6565        "docs_version": get_docs_version(),
    6666        "more": _("More information is available with DEBUG=True."),
    67         "request": request,
    6867    }
    6968    try:
    7069        t = loader.get_template(template_name)
    def csrf_failure(request, reason="", template_name=CSRF_FAILURE_TEMPLATE_NAME):  
    7372            # If the default template doesn't exist, use the fallback template.
    7473            with builtin_template_path("csrf_403.html").open(encoding="utf-8") as fh:
    7574                t = Engine().from_string(fh.read())
    76             c = Context(c)
    7775        else:
    7876            # Raise if a developer-specified template doesn't exist.
    7977            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)}}}
Last edited 6 months ago by Natalia Bidart (previous) (diff)

comment:16 by Tim Graham, 6 months ago

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

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):  
    7272            # If the default template doesn't exist, use the fallback template.
    7373            with builtin_template_path("csrf_403.html").open(encoding="utf-8") as fh:
    7474                t = Engine().from_string(fh.read())
    75             c = Context(c)
     75            body = t.render(Context(c))
    7676        else:
    7777            # Raise if a developer-specified template doesn't exist.
    7878            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 Natalia Bidart, 6 months ago

Owner: changed from Prakhar Parashari to Natalia Bidart
Status: newassigned

comment:18 by Natalia Bidart, 6 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 Tim Graham, 6 months ago

Summary: csrf_failure view missing context processorscsrf_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:20 by Natalia <124304+nessita@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 5f2f12f6:

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

This reverts commit 535f7b5c6cea54a0796d85bbe213183d50002689.

comment:21 by Natalia Bidart, 6 months ago

Resolution: fixed
Status: closednew

comment:22 by Natalia Bidart, 6 months ago

Component: CSRFCore (Other)
Easy pickings: set
Owner: Natalia Bidart removed
Status: newassigned

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 Natalia Bidart, 6 months ago

Status: assignednew

comment:24 by yushan, 6 months ago

Owner: set to yushan
Status: newassigned

comment:25 by yushan, 6 months ago

Has patch: set

comment:26 by Mariusz Felisiak, 6 months ago

Needs tests: set
Patch needs improvement: set

comment:27 by Mariusz Felisiak, 6 months ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 14b0132:

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

Note: See TracTickets for help on using tickets.
Back to Top