Opened 9 years ago

Closed 8 years ago

#24977 closed Cleanup/optimization (wontfix)

Template variables with a value of None are considered to be == to non-existent properties

Reported by: Daniel Quinn Owned by: Tim Martin
Component: Template system Version: 1.7
Severity: Normal Keywords: string, if, equivalence
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This bit me today and I thought I'd point it out since I would consider this a bug

{% if user.pk == some_undefined_value %}
  This is rendered if user is not logged in
{% endif %}

{% if user.pk == some_object.some_invalid_property %}
  This is also rendered if user is not logged in
{% endif %}

It's understood that the template shouldn't flip out with an exception in the event that we're trying to access an undefined value, but when testing against these in an {% if %} block, some very scary stuff can happen.

In my case in particular, I had something like this:

{% if user.pk == product.product_owner_id %}
  This is private data
{% endif %}

Changing the attribute name product_owner_id to something like owner_id, now accidentally leaks private data to unauthenticated users because the templating engine considers None equal to what is effectively an AttributeError.

What's worse, if you try to render these two values, you get None and "", so they're not even equivalent when cast as a string.

Change History (13)

comment:1 by Baptiste Mispelon, 9 years ago

Hi,

I think I was bitten by this in the past as well.

However, I'm not quite sure if this is intended to be a feature or if it's an actual bug.
And if it's a bug, fixing it in a backwards-compatible way will be tricky.

I spent some time digging around the implementation of {% if %} and I think the key to this issue is this line: https://github.com/django/django/blob/master/django/template/defaulttags.py#L945
If you remove ignore_failures=True, then {% if %} behaves the way you're suggesting it should.

But that change also brings several failures in the test suite, which is not a good sign :(

We might have to resolve to just document this, unless someone has a better idea...

comment:2 by Daniel Quinn, 9 years ago

I'm not familiar with the inner workings of the template engine, but I would think that this could be done in a backward-friendly way by defining failed lookups as something other than None:

class NotAThing(object):
    def __str__(self):
        return ""

That way you could test if something evaluating to None is equal to an instance of NotAThing and hopefully it'd pan out as not-equal, right?

comment:3 by Moritz Sichert, 9 years ago

That's actually how jinja2 does it.
All undefined variables are an instance of jinja2.Undefined and {% if foo.undefined_attr is none %} evaluates to false.

comment:4 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:5 by Tim Martin, 8 years ago

Owner: changed from nobody to Tim Martin
Status: newassigned

comment:6 by Tim Martin, 8 years ago

Has patch: set

comment:7 by Tim Martin, 8 years ago

I've addressed the comments on the pull request aside from the question of whether we need a deprecation path for the ignore_failures parameter. I'm assuming it's OK to change an internal API without a deprecation path; if this is controversial then it's probably best to discuss in the Django developers list.

comment:8 by Tim Martin, 8 years ago

Patch needs improvement: set

comment:9 by Tim Martin, 8 years ago

Patch needs improvement: unset

I've fixed up the UT and flakes failures.

comment:10 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:11 by Tim Martin, 8 years ago

Patch needs improvement: unset

Thanks for the feedback, I've improved the pull request.

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:13 by Tim Graham, 8 years ago

Resolution: wontfix
Status: assignedclosed

Per the discussion on django-developers, it seems we can't make the change due to backwards compatibility (see the documentation of the current behavior).

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