689749 - Feed Preview Should Render Inline HTML Partly

Status: RESOLVED INACTIVE

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

Description

[Felix Fung (:felix)]

Attachment: RSS Atom Feed Showing Bug

In RSS Feeds, we will strip all tags when the type of any field happens to be "html" or "xhtml", as in the example RSS Atom attachment. Note that while the 'type' attribute may appear in any feed, only the Atom spec defines it.

Sometimes, stripping tags from entries will result in the message change the meaning of the message. As a result, we should try to (conservatively) render some HTML, such as (<b>, <i>, or <s>).

Comment 1

[Felix Fung (:felix)]

Attachment: Feed Preview Should Render Some HTML in Titles

Summary of Changes:
- Created new interface function createSimpleDocumentFragment
- Run titles through createDocumentFragment which parses the HTML safely into a DOM representation
- Reparse the DOM representation stripping out everything outside a whitelist

This code might (probably) need some refactoring. Hoping review could help with that.

Comment 2

[Dão Gottwald [:dao]]

As far as I remember, feed preview used to be a security nightmare. I don't think we want to (re)open this can of worms unless there's a very compelling reason.

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 562934 [details] [diff] [review]
Feed Preview Should Render Some HTML in Titles

According to https://wiki.mozilla.org/Modules/Firefox#Feed_Preview I'd guess you're looking for Mano as a reviewer.

Comment 4

[Felix Fung (:felix)]

This patch doesn't parse anymore HTML than it already does. The current feed preview already calls createDocumentFragment once on the unfiltered body of an RSS story. We're just calling it on the title now. I'm not really in any position to debate the past security concerns with FeedPreview but I just wanted to make of note of it.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 562934 [details] [diff] [review]
Feed Preview Should Render Some HTML in Titles

It seems to me that the right criteria would be "all inline elements allowed". The way this is written now, you're encouraging the use of deprecated tags (b/i/u/strong) rather than the recommended way of styling (in short, span).

Also note that any element you're accepting could come with some attributes. That could be problematic for the class attribute.

All that said, the FeedProcessor is going to be rewritten sometime soon (or not so soon).  Thus I'm not so sure it's worth spending time on minor issues like this.

Comment 6

[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.