Sanitizing filter broken in 0.90

http://code.google.com/p/html5lib/issues/detail?id=162

Reported by [email protected], Oct 10, 2010

DESCRIPTION

Consider the following interaction with html5lib 0.90:
    >>> from html5lib import html5parser, serializer, treebuilders, treewalkers
    >>> p = html5parser.HTMLParser(tree = treebuilders.getTreeBuilder('dom'))
    >>> dom = p.parse("""<body onload="sucker()">""") 
    >>> s = serializer.htmlserializer.HTMLSerializer(sanitize = True)
    >>> ''.join(s.serialize(treewalkers.getTreeWalker('dom')(dom)))
    u'<body onload=sucker()>'
This is clearly incorrect: the onload attribute should have been removed by the sanitizer during the serialization.

ANALYSIS

The problem is that there are two sanitizers: a tokenizing sanitizer in html5lib.sanitizer, and a sanitizing filter in html5lib.filter.sanitizer. To avoid duplication of code, these two sanitizers inherit from the class HTMLSanitizerMixin and both call that class's function sanitize_token.

Unfortunately, the format of tokens differs between tokenization and filtering. During tokenization, a token looks like this:
    >>> from html5lib import tokenizer
    >>> next(iter(tokenizer.HTMLTokenizer("""<body onload="sucker()">""")))
    {'selfClosing': False, 'data': [[u'onload', u'sucker()']], 'type': 3, 'name': u'body', 'selfClosingAcknowledged': False}
But during filtering, tokens look like this:
    >>> list(iter(treewalkers.getTreeWalker('dom')(dom)))[3]
    {'namespace': u'http://www.w3.org/1999/xhtml', 'type': 'StartTag', 'name': u'body', 'data': [(u'onload', u'sucker()')]}
When the sanitizing filter passes its token to the sanitize_token method of HTMLSanitizerMixin, nothing happens, because sanitize_token is expecting 'type' to be an integer.

OBSERVATION

Having two very similar but subtly different data formats for the same data type is dangerous: how many other incompatibilities are there?

WORKAROUND

I am working around this problem as follows: when I need to apply a sanitizing filter to a DOM tree, instead I do the following:

Serialize the DOM to HTML without sanitization.

Re-parse the HTML from step 1, using the sanitizing tokenizer.

@jgraham

So the status is:

We only support sanitizing at the tokenizer level.
We support and are totally broken sanitizing at the filter level (and sometimes die, sometimes don't).

My gut says the filter level is the right level for the sanitizer to be operating (the middle of the parser doesn't make much sense, as what you really want to do is post-process the tree to remove what you dislike). I think we should probably goes as far as to remove the ability to change the tokenizer. The big downside of that, obviously, is that we go from the sanitizer only working as a tokenizer in 1.0b1 to that being unsupported and only working as a filter in 1.0b2…

Thoughts, @jgraham, @garethrees, @jsocol?

#24 has some relevance, but given duck-typing doesn't help much.

@jgraham

Thoughts, @jgraham, @garethrees, @jsocol?

I'm garethrees.co.uk. GL!

@gareth-rees

Oops! Sorry! Attempt two: Thoughts, @gareth-rees, on the above?

Ostensibly I agree that the filters are the more "correct" place to do sanitization, even if it means huge changes in bleach for 1.0, but I haven't really done it that way so my one question is: do filters enable both dropping the tag completely (with or without any content) and replacing it with an escaped version (e.g. <script>)?

It's much easier to do with filters, as you're guaranteed a matching start tag and an end tag for each node, so you can maintain a simple stack to drop content. Obviously with tokenizers you either have to reimplement half the parser or accept you'll never get it quite right.

Fix html5lib#72: Move the sanitizer to purely be a filter.

Adjust to removal of XHTMLSerializer from html5lib and fix and workar…

Fix html5lib#72: rewrite the sanitizer to be a treewalker filter only.

Adjust to removal of XHTMLSerializer from html5lib and fix and workar…

@gsnedders

Howdy! I'm working to migrate the HTML sanitizer in feedparser to rely on html5lib. However, some of the feedparser unit tests are triggering the TypeError bug referenced in #68. If @gsnedders has written viable code to resolve this, would it be possible to coordinate feedparser's migration with the integration of the fix for this?

@kurtmckee

@kurtmckee the fix for that is for currently to use the sanitizer as a filter when tokenizing and not when serializing; this will change once #72 gets fixed (which is PR #110) as then you'll use the sanitizer as a filter when serializing and not when tokenizing.

Fix html5lib#72: rewrite the sanitizer to be a treewalker filter only.

Fix #72: rewrite the sanitizer to be a treewalker filter only.

squash! Fix #72: rewrite the sanitizer to be a treewalker filter only.

fixup! squash! Fix #72: rewrite the sanitizer to be a treewalker filt…

Fix html5lib#72: rewrite the sanitizer to be a treewalker filter only.

fixup! Fix html5lib#72: rewrite the sanitizer to be a treewalker filt…

Fix html5lib#72: rewrite the sanitizer to be a treewalker filter only.

Any chance someone could write replacement code for the following?

from html5lib import sanitizer

sanitizer.HTMLSanitizer.acceptable_elements.extend(settings.TEXT_ADDITIONAL_TAGS)

This would really help me out. Thanks.

Navigation Menu

Repository navigation

Sanitizing filter broken in 0.90 #72

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Sanitizing filter broken in 0.90 #72

Description

Activity

gsnedders commented on Jun 26, 2013

gsnedders commented on Jun 26, 2013

garethrees commented on Jun 27, 2013

gsnedders commented on Jun 27, 2013

jsocol commented on Jun 27, 2013

gsnedders commented on Jun 27, 2013

kurtmckee commented on May 4, 2015

gsnedders commented on May 5, 2015

rando305 commented on Nov 18, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions