Opened 3 years ago

Closed 20 months ago

#22232 closed Bug (fixed)

Template extends tag can cause recursion

Reported by: galedragon Owned by: Sambhav Satija
Component: Template system Version: 1.6
Severity: Normal Keywords:
Cc: nathanrandal@…, areski@…, f.ssat95@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I just wanted to bring this to the community's attention.

If someone has this in "somepage.html":
{% extends "somepage.html" %}

So basically you try to extend the page you're working on.

It causes a Recursion error. It's an easy fix and can be fairly obvious, but I was wondering if this was the intentional effect.

Filing under optimization because it's not exactly a bug.

First ticket, so be gentle :)

Change History (25)

comment:1 Changed 3 years ago by Russell Keith-Magee

Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

I'd call it a bug; hitting a system level recursion error as a result of accidentally naming the wrong parent in an extends should't be the expected behaviour, if only because it's computationally expensive to hit maximum stack depth.

It's definitely an error though - I can't think of any way an internally recursive template could render in a meaningful way.

So - yes, this should be caught at the template load level -- a simple "if parent == self, raise error" check should do the trick.

comment:2 Changed 3 years ago by Nathan Smith

Owner: changed from nobody to Nathan Smith
Status: newassigned

comment:3 Changed 3 years ago by Nathan Smith

Pull Request: https://github.com/django/django/pull/2431

This patch checks the template node list for self extension.

During the exploration of this issue we noticed that there may be another case when a recursion error could occur. This issue addresses the case when a template extends itself, but a circular extension could occur further up the ancestry. Should I handle this case as well? It is not immediately clear how I could do this efficiently.

comment:4 Changed 3 years ago by Russell Keith-Magee

Hrm. This is an interesting problem. On the one hand, it would be nice to raise a humane error message, but if it's only going to catch the one-level recursion case, I'm starting to wonder if it's worth the effort. Adding a potentially expensive check to the top of every template load so that you can present a nicer error message in the edge case of recursive templates seems like too high a price to pay.

If we can't find an efficient way to catch the complex circular case, I'm inclined to turn this into a documentation fix (ie. noting in the discussion around extends that circular loops may cause stack overflow errors).

One option might be to handle this at render time, rather than at load time. If you put something in the context that tracks which templates have been used, and you try to re-use a template name, that indicates a circular loop, and you can raise an error. This should be relatively cheap (a set lookup & insertion on each new template file used); as long as we can find a clean place to track the data, it might suffice as a solution.

comment:5 Changed 3 years ago by Nathan Smith

Cc: nathanrandal@… added

comment:6 Changed 3 years ago by Nathan Smith

I updated the pull request (https://github.com/django/django/pull/2431) with your recommended solution. It keeps a list of the extended templates in the render_context if there are any ExtendsNode nodes. I have removed the previous attempt as this new approach catches both the old and the new. Let me know what you think.

comment:7 Changed 3 years ago by Nathan Smith

Has patch: set

comment:8 Changed 3 years ago by Nathan Smith

Any thoughts on this?

comment:9 Changed 3 years ago by Marc Egli

Why not add the current template to the list during the instantiation? Then you would catch a recursion error one step earlier?

comment:10 Changed 2 years ago by Areski Belaid

To be noted that this PR is being refactored https://github.com/django/django/pull/3014

comment:11 Changed 2 years ago by Areski Belaid

Cc: areski@… added

comment:12 Changed 2 years ago by anonymous

Apologies, looks like my comment from yesterday didn't go though. I refactored Nathan's pull request at the PyConAu sprints yesterday. Was my first time working with the Django source. Thanks for the suggestions areski, will look into them now.

comment:13 Changed 2 years ago by kateseamer

Note that the message above was from me, looks like the login form that is presented after registration does not work. I have created a new ticket here: https://code.djangoproject.com/ticket/23204

comment:14 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Please uncheck "Patch needs improvement" when the comments on the PR have been updated, thanks.

comment:15 Changed 2 years ago by Anand Reddy Pandikunta

Owner: changed from Nathan Smith to Anand Reddy Pandikunta

comment:16 Changed 23 months ago by Sambhav Satija

Cc: f.ssat95@… added

If it's still pending, I'd like to work on it from scratch. Should I ?

comment:17 Changed 23 months ago by Sambhav Satija

Owner: changed from Anand Reddy Pandikunta to Sambhav Satija
Last edited 23 months ago by Sambhav Satija (previous) (diff)

comment:18 Changed 23 months ago by Markus Holtermann

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:19 Changed 23 months ago by Preston Timmons

FYI - This behavior is also addressed as part of #15053. If this patch is committed, it's implementation via a context variable may be obsoleted by the new origin api discussed here:

https://groups.google.com/forum/#!topic/django-developers/VFBLAoPSplI

comment:20 in reply to:  19 Changed 23 months ago by Sambhav Satija

Replying to prestontimmons:

FYI - This behavior is also addressed as part of #15053. If this patch is committed, it's implementation via a context variable may be obsoleted by the new origin api discussed here:

https://groups.google.com/forum/#!topic/django-developers/VFBLAoPSplI

Since this was my first attempt at contributing, I have no clue what the logical next step should be. Should this ticket be put on hold or is there any other way to proceed? And if the Loader API is implemented, I guess that fixes this ticket as well.

comment:21 Changed 23 months ago by Preston Timmons

Your patch was marked RFC, so you don't need to change anything unless you get feedback otherwise, especially if this patch is considered a bug fix that can land in Django 1.8. My note above is more directed toward potential committers if this only goes into the 1.9 release cycle.

comment:22 Changed 21 months ago by Tim Graham

Easy pickings: unset
Has patch: unset
Triage Stage: Ready for checkinAccepted

Preston, let's assume your work will be committed for 1.9. Should we close this ticket or is there anything remaining to address the goals of this ticket that isn't addressed in your current PR for #15053?

comment:23 Changed 21 months ago by Preston Timmons

There's one edge case not strictly fixed by #15053: 3rd party loaders that haven't updated to the recursive api through the deprecation period. The PR does have an explicit check for this, but it's something that's easy to overlook. Therefore, I don't feel strongly about closing this ticket yet.

comment:24 Changed 20 months ago by Preston Timmons

This is fixed now that #15053 is merged.

comment:25 Changed 20 months ago by Preston Timmons

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top