t.w.template does not always properly escape comments

[twisted-trac]

	@bitglue reported
Trac ID	trac#5275
Type	defect
Created	2011-09-26 17:43:15Z
Branch	https://github.com/twisted/twisted/tree/template-comment-fix-5275

>>> from twisted.web.template import flattenString, Comment
>>> from twisted.python.util import println
>>> flattenString(None, Comment('the quick---brown fox')).addCallback(println)
<!--the quick- --brown fox-->
<Deferred at 0x1020d6a28 current result: None>

Which of course is malformed HTML, because HTML is SGML, and comments in SGML begin and end with '--', and are between '<!' and '>'. Or something like that.

What this generates is a comment declaration with one comment, 'the quick- ', and some illegal characters 'brown fox--'.

http://htmlhelp.com/reference/wilbur/misc/comment.html

Attachments:

• template-comments-5275.patch (2065 bytes) - added by indigo on 2011-09-26 18:13:56Z - proposed fix
• template-comments-5275.2.patch (2342 bytes) - added by indigo on 2011-09-26 18:18:03Z -
• template-comments-5275.3.patch (2654 bytes) - added by indigo on 2011-09-26 18:37:31Z -

Searchable metadata

trac-id__5275 5275
type__defect defect
reporter__indigo indigo
priority__normal normal
milestone__ 
branch__branches_template_comment_fix_5275 branches/template-comment-fix-5275
branch_author__indigo indigo
status__closed closed
resolution__fixed fixed
component__web web
keywords__ 
time__1317058995000000 1317058995000000
changetime__1317242625000000 1317242625000000
version__None None
owner__exarkun exarkun
cc__indigo

[twisted-trac]

	@bitglue commented

I couldn't think of an elegant way to fix this without changing the way comments are serialized sometimes. Not sure anyone cares.

Also, the proposed patch escapes things which strictly need not be escaped, at least by SGML syntax rules. My reasoning was that it's better to be conservative and generate something that won't confuse a browser's parser and will be recognized as a comment than it is to follow SGML to the letter. There are plenty of valid SGML comments that confuse browsers anyway, so this seems reasonable.

[twisted-trac]

	@bitglue commented

Seriously, comment syntax is complicated. Did you know they can't end in '-'? I didn't. http://www.w3.org/TR/REC-xml/#sec-comments

[twisted-trac]

	@exarkun commented

(In [32674]) Branching to 'template-comment-fix-5275'

[twisted-trac]

	@exarkun commented

(In [32675]) Apply template-comments-5275.3.patch

refs #5275

[twisted-trac]

	@exarkun set owner to @exarkun

Thanks a lot! Some simple coding standard points:

1. test_CommentEscaping should be test_commentEscaping.
2. Test method docstrings should not start with "Test ...".
3. Two blank lines between methods.

Some others:

4. It might be nice if when this test failed it reported what kind of input it failed on and what bogus output it got.
5. [ReleaseProcess#Newsfiles News fragments] are handy. Committers will often write them for you, but it's nice when you write them. :) This is a nice bug fix, it should definitely have a nice news fragment.

These points are pretty minor. I'll address them and merge, if everything looks good on buildbot.

[twisted-trac]

	@exarkun commented

(In [32676]) Address some review feedback for parts of the new test

refs #5275

[twisted-trac]

	@exarkun commented

(In [32677]) news fragment

refs #5275

[twisted-trac]

	@exarkun set status to closed

(In [32678]) Merge template-comment-fix-5275

Author: indigo
Reviewer: exarkun
Fixes: #5275

Add comment value mangling to twisted.web.template so that comments are not
emitted which will break an SGML-ish or XML parser on the receiving end. Since
there is no real escaping rule for comments in an HTML document, the value is
mangled irreversibly.