592875 - Sanitize feed content safely in FeedProcessor and handle XHTML

Status: RESOLVED INACTIVE

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Nicholas Wilson]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

At the moment, the FeedProcessor library used by the internal feed previewer and several extensions has a problem in the way it handles text fields. The text construct suffers from at least these shortcomings:

1) Field content is not thoroughly sanitized. It is safe from JavaScript execution, but does not have all undesirable elements and attributes stripped. It would be desirable for extensions using the FeedProcessor to have this handled. I have implemented a first try at this using the WHATWG whitelists.
2) XHTML content is not name-space safely handled. There may be a better way of doing this, but my patches use the innerHTML parser instead once all dangerous elements have been stripped. This means that SVG and MathML content in feeds (see Sam Ruby's feed above) can be properly displayed. This is a free upgrade for any extensions using the FeedProcessor.
3) atomInlineOtherContent allows for base64 encoded embedded content like images. This is now preliminarily supported.

I am new to Gecko XPCOM, so a few lines of code took a while to figure out. There may well be better ways to implement some of this. In particular, I am bothered by having to implement the sanitizer rules separately for HTML content and XHTML content, but there is no way to stop the XHTML from being parsed along with the whole feed, in which case dangerous things (onerror, onload, script) have to be taken away right there before the content is inserted into a DOM; on the other hand, there is no way to call the methods from nsIScriptableUnescapeHTML before we get a DOM. In other words, there is probably a cleaner solution possible if the call made in nsScriptableUnescapeHTML.cpp to nsParser.cpp's parseFragment could be replaced by a call to nsHTML5Parser.cpp's parseFragment, but that is currently unimplemented.

In any case, it works, though I would welcome any comments.

The interface to nsIFeedTextConstruct has been kept backwards compatible, with the following changes: it is no longer possible for extensions to read 'dangerous' content out; another attribute has had to be added for use setting a sanitized XML string as content.

What these patches do not achieve:
*) Add a CSS Parser (bug 359316)
*) Overhaul the nsIFeedResult interface, which currently has several problematic points (eg bug 525655)

I have some patch queues on changes in this file, but am a new contributor to Firefox and am unfamiliar with the code, so I am uploading these as relatively uncontroversial initial changes hoping for comment. Thanks for bearing with me.

Reproducible: Always

Comment 1

[Nicholas Wilson]

Attachment: First step: decide what content is safe

Comment 2

[Nicholas Wilson]

Attachment: Second patch: handle XHTML content preserving namespaces

Comment 3

[Nicholas Wilson]

Attachment: Third step: use the sanitizer to strip nasties

Comment 4

[Nicholas Wilson]

Attachment: Finally: refactor HTML handling so interface only presents safe content to users of FeedProcessor

Comment 5

[Nicholas Wilson]

Note: patches 1-3 make sense to apply together and give improved handling on XHTML with no regressions (as far I can test); 4 makes further separate changes.

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

You should ask for Feedback/Review on your Patches using the given Attachment Flags and this List http://www.mozilla.org/projects/firefox/review.html.

Comment 7

[Nicholas Wilson]

Thanks. I added reviewers for patches on the other bugs, but was not sure who to ask for this. Asaf Romano is listed as the head honcho for the component, so I have added him.

Comment 8

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.