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 , 9 years ago
comment:2 by , 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 , 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 , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:5 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 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 , 8 years ago
Patch needs improvement: | set |
---|
comment:10 by , 8 years ago
Patch needs improvement: | set |
---|
comment:11 by , 8 years ago
Patch needs improvement: | unset |
---|
Thanks for the feedback, I've improved the pull request.
comment:12 by , 8 years ago
Patch needs improvement: | set |
---|
comment:13 by , 8 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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).
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#L945If 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...