additional context for included templates can override current context

[clime7@…]

Hey,

here is how:

base.html

{{ foo }} <!-- outputs "foo" as defined in a view -->
{% include evil_snippet.html foo='bar' %}
{{ foo }} <!-- outputs "bar" cause the value has been overriden-->

evil_snippet.html

{% context_updater %}

@register.simple_tag(takes_context=True)
def context_updater(context, template):

context.update({'resistance': 'is futile'}) # adds new context layer that is not accounted for

[Baptiste Mispelon]

Hi,

The problem comes from the fact that when using takes_context=True, you're given the actual instance of the Context that's being used to render the "parent" template (the one containing the {% include %} tag) and if you're not careful, you can end up in a situation like the one you describe.

Here, IncludeNode uses a context manager (ie a with statement) to render its template.
The context manager is quite simple: it just adds a new layer when entered and removes the last layer when exiting. In particular, no effort is made to try and determine if the layer that's being removed at the end is the one that had been added at the beginning.

This has the effect that if you add a new layer from inside the context manager (which is what your update() call does), that's the one that ends up being removed when you exit the context manager, leaving you with one more layer than when you entered it.

With that said, I'm not sure how to proceed here.

On the one hand, your code example is incorrect in its usage: you should be using with context.push(resitance='is_futile') or at least context.pop to leave the context is the same state when your function exits.

On the other hand, leaving the consistency of the context object at the mercy of the person who wrote the custom tags used in the included template seems quite risky, seeing how easy it is to shoot yourself in the foot (not to mention the fact that this is not documented anywhere).

I suppose one overkill approach would be to do a copy of the context object before passing it to the included template but I'm worried about performance costs for that.

Another approach might be to build some logic in the Context's context manager to check if some extra layers might have been added between __enter__ and __exit__. This would prevent a custom tag from leaving the context in a broken state but it might be tricky to implement (and there will be backwards-compatibility issues to deal with as well).

I'll mark the ticket as accepted because there's clearly a problem here.
Suggestions on how to approach the problem and/or patches are welcome, as always.

[clime]

I suggest passing just the top most context layer to template tags instead of the whole context.

[Baptiste Mispelon]

Replying to clime:

I suggest passing just the top most context layer to template tags instead of the whole context.

I don't think that would work.
A Context is basically a list of dicts and when looking up a variable in the context, Django just goes through that list in order, looking for the variable in each dict and raising a KeyError at the end if not found.
This means that if you only use the last "layer" of the context, you're probably going to have some variables missing.

[clime]

This means that if you only use the last "layer" of the context, you're probably going to have some variables missing.

You are, of course, right. I was thinking how to make it so that updating context in a template tag does not add a whole new layer but only modifies the last one. But I didn't realize that you actually want to have read access to all context variables in a template tag so you need to pass the whole context. Would there be a problem if update would update just the last layer? And for adding new layers push would need to be used explicitly... Sorry if this is yet another bad idea. Just came to as an alternative to the previously suggested solutions.

[Baptiste Mispelon]

Can't do that either I'm afraid.

Context.update() currently adds a new layer, it doesn't actually update any dictionary (I've actually tried to change this and it breaks a lot of tests).

The core issue is that since we're passing the actual context object to the custom tag, it's free to change it in all sorts of broken ways.

[anonymous]

Replying to bmispelon:

Can't do that either I'm afraid.

Context.update() currently adds a new layer, it doesn't actually update any dictionary (I've actually tried to change this and it breaks a lot of tests).

I believe, these errors would disappear if push was called before update (update being modified to update just the top dictionary in the context stack). Optionally push could be modified to accept dictionary so that it would essentially become the current version of update. Then it would be matter of substituting update calls for push calls in Django code. From the point of template tags it should not matter if update adds a layer or extends the top most layer so backward compatibility should be ok. I favor this solution for its simplicity.