Opened 16 months ago

Closed 2 months ago

#22232 closed Bug (fixed)

Template extends tag can cause recursion

Reported by: galedragon Owned by: darkryder
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 16 months ago by russellm

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Cleanup/optimization to Bug

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 16 months ago by Saidin7979

  • Owner changed from nobody to Saidin7979
  • Status changed from new to assigned

comment:3 Changed 16 months ago by Saidin7979

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 16 months ago by russellm

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 16 months ago by Saidin7979

  • Cc nathanrandal@… added

comment:6 Changed 16 months ago by Saidin7979

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 16 months ago by Saidin7979

  • Has patch set

comment:8 Changed 15 months ago by Saidin7979

Any thoughts on this?

comment:9 Changed 15 months ago by frog32

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 11 months ago by areski

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

comment:11 Changed 11 months ago by areski

  • Cc areski@… added

comment:12 Changed 11 months 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 11 months 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 11 months ago by timgraham

  • Patch needs improvement set

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

comment:15 Changed 6 months ago by ChillarAnand

  • Owner changed from Saidin7979 to ChillarAnand

comment:16 Changed 6 months ago by darkryder

  • Cc f.ssat95@… added

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

comment:17 Changed 6 months ago by darkryder

  • Owner changed from ChillarAnand to darkryder
Last edited 6 months ago by darkryder (previous) (diff)

comment:18 Changed 6 months ago by MarkusH

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:19 follow-up: Changed 6 months ago by 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

comment:20 in reply to: ↑ 19 Changed 6 months ago by darkryder

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 6 months ago by prestontimmons

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 4 months ago by timgraham

  • Easy pickings unset
  • Has patch unset
  • Triage Stage changed from Ready for checkin to Accepted

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 4 months ago by prestontimmons

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 2 months ago by prestontimmons

This is fixed now that #15053 is merged.

comment:25 Changed 2 months ago by prestontimmons

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top