Re: custom function for autoescape

[Will Hardy]

Hi Luke,

> First, you depend only on the name of the function - so one that shadows
> a builtin filter won't be treated correctly (as you noted on the
> ticket).

This is true, I really hated this bit. The only thing that might work
is "libraryname.filtername" if it's possible to identify exactly which
filter was meant at runtime (the libraryname would be the same as that
used in {% load %} ). Even then it's functionality could depend on
the installed apps, which is just wrong.

> Second, when you are defining an autoescape function you have to know
> all the filters it will ever be used with. This is just as ugly a
> constraint as the problem it is trying to fix.

Not quite. The 'proper' way to do this is to have the filter define a
matching "autoescape context". The whitelist is only really there for
convenience, to let you use filters from other contexts (ie the
default django ones, external ones), without writing new ones.

If you wrote the filter, you can define the "autoescape context". If
you only wrote the escape function, you can define the whitelist. If
you wrote neither and they are for different "autoescape contexts",
you can use the "|safe" filter in the template.

> Third, although we can fix the filters in your 'group 2', you can't fix
> 3rd party filters like this. If we don't fix them, we still have caveats
> that we have to add to the docs - "Be aware that any filters/tag not
> bundled with Django may be broken with respect to autoescape" etc.

This is true, I don't see any way around this.

> Overall, with this addition, the whole thing actually gets harder to
> understand and document, and just feels like a hack. That kind of thing
> is acceptable to fix existing bugs, but not for new features IMO.

Agreed. I'll leave the ticket be for now, I don't really want to
distract anyone from 1.3 work. If I can think of a cleaner way of
doing this, I'll have another look at it after 1.3 is out.

Cheers and thanks for your time,

Will

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Luke Plant]

On Wed, 2010-11-10 at 13:04 +0100, Will Hardy wrote:
> > Reading over the discussion, I'm in the same camp as Luke. I can see
> > the use case, but I see a bunch of sharp edges that will end up biting
> > the user in unexpected ways.
> 
> Thanks for dropping by :-) I think I've managed to remove the sharp edges.

The proposed solution isn't compelling IMO.  Specifying a set of filters
as a whitelist (or as a blacklist) is a very brittle mechanism.

First, you depend only on the name of the function - so one that shadows
a builtin filter won't be treated correctly (as you noted on the
ticket). 

Second, when you are defining an autoescape function you have to know
all the filters it will ever be used with. This is just as ugly a
constraint as the problem it is trying to fix.

Third, although we can fix the filters in your 'group 2', you can't fix
3rd party filters like this. If we don't fix them, we still have caveats
that we have to add to the docs - "Be aware that any filters/tag not
bundled with Django may be broken with respect to autoescape" etc.

Overall, with this addition, the whole thing actually gets harder to
understand and document, and just feels like a hack. That kind of thing
is acceptable to fix existing bugs, but not for new features IMO.

Luke

-- 
I never hated a man enough to give him his diamonds back. (Zsa Zsa 
Gabor)

Luke Plant || http://lukeplant.me.uk/

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Will Hardy]

> Reading over the discussion, I'm in the same camp as Luke. I can see
> the use case, but I see a bunch of sharp edges that will end up biting
> the user in unexpected ways.

Thanks for dropping by :-) I think I've managed to remove the sharp edges.

The main problem in this thread is that the default filters were
written in the html context and their interaction with autoescaping
breaks down when a different escaping context is required.

I believe the solution would be to simply declare the html filters as
"unsafe" and I've uploaded a patch [1] to the ticket that would do
that. I've included a detailed description of the approach on the
ticket [2]. All filters appear to be correctly handled in the new
approach described there.

If you think this approach is sensible, I'll edit the group 2 filters
to use the custom escape function (they mostly take the autoescape
argument anyway). There are a small number of tests on the ticket, but
if this is something that might be in core, then I'll write some more
comprehensive tests and documentation.

Cheers,

Will

[1] 

[2] 

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Russell Keith-Magee]

On Sat, Nov 6, 2010 at 11:16 PM, Luke Plant  wrote:
> On Thu, 2010-11-04 at 19:06 -0700, SmileyChris wrote:
>
>> I too would like to know other's thoughts.
>
> Is there any other core dev who would like to weigh in on this?

Reading over the discussion, I'm in the same camp as Luke. I can see
the use case, but I see a bunch of sharp edges that will end up biting
the user in unexpected ways.

I think Luke's point (on the ticket) about XSS also bears repeating. Quoting:

"""
HTML autoescaping serves a very important security function —
preventing XSS — I think it is OK to leave it as being an HTML only
feature. Of course, there might be other types of output that would
benefit in the same way, but they are not as important for a web
framework.
"""

On top of that, alternate escaping schemes can be implemented as a
template tag or filter. Doing it this way isn't quite as elegant as a
systemic change to the rendering process, but with the edge cases
described, the change wouldn't be an elegant systemic anyway.

At the end of the day, it strikes me as a little too messy to be a
core feature of the template language.

Yours,
Russ Magee %-)

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Luke Plant]

On Thu, 2010-11-04 at 19:06 -0700, SmileyChris wrote:
> Thanks for following up, Luke.
> 
> I understand your point of view, but personally, I'm fine with an "all
> bets are off using built-in filters/tags" clause on a custom escape
> method.
> While you'd expect that addslashes would just work, I'd take the
> opposite expectation and assume that any filter / tag used would have
> to be custom built for a custom escape method.

I think that you and I probably see the problem immediately, but someone
less familiar with the template system is much more likely to be
confused and caught out. We are about level 2 on the Rusty API's Design
Manifesto [1] - you have to read the docs to see that there is a
problem, and then read the implementation of the filters (and think
quite hard) to know whether they can be used safely or not.

> Just clarifying my point of view (in that I had thought of this
> caveat).
> I too would like to know other's thoughts.

Thanks for that clarification.

It occurs to me that one technique that could improve the situation is
loading a custom template tag library that overrides the builtin filters
and tags. Such a library should be fairly straightforward to build  do
'from django.template.defaultfilters import *' or equivalent, override
the ones that need fixing, and register your new filters under a new
library. But identifying the ones that need fixing for your case is
perhaps a lot of work, and it would be easiest not to bother, so I can
imagine people getting stung.

In some cases the builtin filters are actually HTML specific with
respect to what they add e.g. linebreaks, unordered_list.  These are
also obvious candidates for shadowing with a custom template tag library
that does the right thing.

There also remain some usage warts. To use a different escaping, I have
to remember to pass in the autoescape function to Context *and* adjust
the template by adding a {% load whatever %} to get the filters that are
fixed for my use case. I could also easily forget the second, and find
that my template works most of the time.

Part of my reluctance is that if we started from scratch with support
for custom escaping in mind, we probably wouldn't do it this way. We
already have enough things in Django where we are stuck supporting
something due to an old design decision, and I would like to avoid
adding another wart when we have the option of just not adding it :-)

Regards,

Luke

[1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto


-- 
"I imagine bugs and girls have a dim suspicion that nature played a 
cruel trick on them, but they lack the intelligence to really 
comprehend the magnitude of it." (Calvin and Hobbes)

Luke Plant || http://lukeplant.me.uk/

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Will Hardy]

Thanks for the explanation and perfect example Luke.

I thought I would try to be helpful and went through the source code
for all the filters and categorised them (see below). There are 12
filters that can't be used (8 are for html only anyway), 24 filters
may fail unpredictably (like your example did), 2 of those can be
easily rewritten to avoid that and 23 filters that should be fine with
custom escape functions.

I'm personally happy with the "all bets are off" disclaimer, as it
should be clear that the default filters are generally HTML-oriented.
Many other filters are still useful, so developers will be fine as
long as they know which ones to avoid. I guess adding a list of
working/not working/incompatible filters may be "documentation noise"
for those that don't need it, but it could have its own home somewhere
else.

I don't imagine the default tags will pose any problems, as they have
access to Context.autoescape. In any case, it seems that only {%
csrf_token %} marks its (HTML) output as safe, and the {% filter %}
tag would of course behave like the filters it uses.

Cheers,

Will


DIFFICULT FILTERS (4+8)
These would be difficult to make work with custom escape, because the
escaping needs to be handled by the filter itself:
* cut (marks safe when change won't create new HTML tags)
* force_escape (unlike escape, it won't use the custom escape function)
* join (escapes each joined item individually)
* linenumbers (but autoescape argument, allowing it to be turned off)

These have the same problem, but they would probably only be used in
HTML documents.
* unordered_list
* linebreaks
* linebreaksbr
* urlize (but takes autoescape argument)
* urlizetrunc (but takes autoescape argument)
* markup.textile
* markup.markdown
* markup.restructuredtext


FILTERS THAT DON'T TAINT/UNSAFE DATA (24)
These could be potentially dangerous, when they alter the "safe" data
in a way that would make it unsafe for a given context (like Luke's
example). Many however have a clear functionality, but the problem
might be so subtle that the developer wouldn't think of it. I imagine
that they don't taint the output because someone has carefully
analysed their behaviour in the HTML context.
* addslashes
* capfirst
* iri_to_uri
* lower
* stringformat
* title
* truncatewords
* truncatewords_html
* wordwrap
* ljust
* rjust
* center
* removetags
* striptags
* last (oddly, the "first" filter does taint/unsafe the data, but this doesn't)
* random (ditto, the "first" filter does taint/unsafe the data, but
this doesn't)
* slice
* filesizeformat
* phone2numeric
* pprint
* fix_ampersands
* humanize: ordinal
* humanize: intcomma
* humanize: apnumber


FILTERS UNNECESSARILY MARKED SAFE (2)
These filters are marked as safe although there is apparently no need
to do so. It does however save a bit of processing. Removing
mark_safe() shouldn't have an effect on their operation, other than
being less efficient.
* slugify (output can't contain &, <, >, " or ' anyway)
* floatformat (marks format output as safe, none of the default
formats would be affected by escaping (I checked). This would however
prevent developers from writing formats with html markup, ie
DECIMAL_SEPARATOR = '.', which might be
very useful for some)


SAFE FILTERS (23)
The following filters should be ok, as they don't prevent the custom
escape from being run
* escape
* urlencode
* wordcount
* escapejs
* safe (well it does prevent custom escape, but that's the idea)
* safeseq
* dictsort
* dictsortreversed
* first
* length_is
* add
* get_digit
* date
* time
* timesince
* timeuntil
* default
* default_if_none
* divisibleby
* yesno
* pluralize
* humanize: intword
* humanize: naturalday

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[SmileyChris]

Thanks for following up, Luke.

I understand your point of view, but personally, I'm fine with an "all
bets are off using built-in filters/tags" clause on a custom escape
method.
While you'd expect that addslashes would just work, I'd take the
opposite expectation and assume that any filter / tag used would have
to be custom built for a custom escape method.

Just clarifying my point of view (in that I had thought of this
caveat).
I too would like to know other's thoughts.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[Luke Plant]

On Thu, 2010-11-04 at 17:24 +0100, Will Hardy wrote:

> I've attached a simple patch to the ticket [2] and would be happy to
> write tests and documentation if this approach is enough to overcome
> the "wontfix". Have I overlooked something?

I think the devil is in the details. If we go for a simple
implementation like this, the ugliness moves to trying to document it.

The basic objection I have is that all the builtin filters become
unreliable.  It's difficult to come up with examples that don't
ridiculously sound contrived, but I'll try.

Suppose I am outputting RTF text.  I have an escape function like this:

def rtfescape(value):
return value.replace('\\', '')
.replace('{','\\{')
.replace('}', '\\}')

i.e. backslashes and curly braces need escaping.

Suppose I have an input string "Joe's string".  I should be able to mark
this as safe - it doesn't need escaping either for HTML or RTF. Or leave
it unmarked - it should make no difference.

Suppose also I have a template that uses 'addslashes' (for the sake of
argument, because I'm writing a RTF document that shows what the
'addslashes' filter does). That filter is marked 'is_safe', because it
only introduces slashes, which are not dangerous characters for HTML.
But they *are* dangerous for RTF.

Logically I would expect the following 3 to produce the same output:

1) I use mark_safe on my safe input string and use addslashes to add
the slashes

Template("{{ val|addslashes }}").render(
   Context({'val': mark_safe("Joe's string")}, 
   autoescape=rtfescape)
)

2) I don't use mark_safe on my safe input string and use addslashes to
add the slashes

Template("{{ val|addslashes }}").render(
   Context({'val': "Joe's string"}, 
   autoescape=rtfescape)
)

3) I manually 'apply' addslashes.

Template("{{ val }}").render(
   Context({'val': "Joe\\'s string"}, 
   autoescape=rtfescape)
)

But these do not produce the same output - 1 is different from 2 and 3,
and is not what I intend.

I know this still seems pretty contrived, but it is certainly possible
that people will want to use 'safe' or 'mark_safe' in conjunction with
custom escaping — I can think of plenty of examples of wanting to do
this with RTF, with custom filters or template tags or other code that
outputs RTF. And it would appear to work most of the time. But it would
break at some points, so when it comes to documenting this, you would
have to say something like:

  If you use this functionality, please note that all of the builtin
  filters may be subtly broken with respect to escaping, depending on
  what your escaping function does, if you ever use 'safe' in your
  templates or 'mark_safe' in your code.

With that caveat, I'm not convinced this is still a beautiful patch! As
Chris and I seem to have a difference of opinion about this, what do
other people think?

(BTW, the patch needs to be extended to alter DebugVariableNode,
otherwise it produces different output with DEBUG=True)

Luke


-- 

Luke Plant || http://lukeplant.me.uk/

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: custom function for autoescape

[SmileyChris]

Hi Will,

I've reopened the ticket, because that's elegant enough for me.

I remember having this discussion in IRC either with you or someone
else a while back and couldn't come up with any negatives to providing
this, as long as obvious caveats of tags/filters potentially relying
on the original escaping method are documented.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

custom function for autoescape

[Will Hardy]

Hi all,

While using templates to produce something other than HTML (latex by
the way), I wanted to use my own escape function, instead of the
HTML-oriented default in autoescape.

This is of course not too difficult with filters and turning of
autoescape, but it would be very nice if I could get the autoescape
framework to use my own custom escape function. I found a ticket
(#14057, [1]) that was requesting the same thing, but was closed as
wontfix. A reason for doing this was that the change would a) require
lots of difficult changes and b) not be backwards compatible. Luke
left the door open for a "beautiful patch" to rescue the ticket and I
wonder if what I'm about to suggest would be enough.

Could a callable be passed to Context() in the autoescape parameter,
in order to define a custom escape function?

I've attached a simple patch to the ticket [2] and would be happy to
write tests and documentation if this approach is enough to overcome
the "wontfix". Have I overlooked something?

Cheers,

Will

[1] http://code.djangoproject.com/ticket/14057
[2] 
http://code.djangoproject.com/attachment/ticket/14057/14057-context-parameter.diff

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.