If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Feed Preview Should Render Inline HTML Partly

NEW
Assigned to

Status

()

Firefox
RSS Discovery and Preview
6 years ago
6 years ago

People

(Reporter: felix, Assigned: felix)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 562872 [details]
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>).
(Assignee)

Updated

6 years ago
Assignee: nobody → ffung
(Assignee)

Updated

6 years ago
Summary: Feed Preview Should Render Inline HTML Party → Feed Preview Should Render Inline HTML Partly
(Assignee)

Comment 1

6 years ago
Created attachment 562934 [details] [diff] [review]
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.
Attachment #562934 - Flags: review?(philringnalda)

Updated

6 years ago
Component: Bookmarks & History → RSS Discovery and Preview
QA Contact: bookmarks → rss.preview
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 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.
Attachment #562934 - Flags: review?(philringnalda) → review?(mano)
(Assignee)

Comment 4

6 years ago
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 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.
Attachment #562934 - Flags: review?(mano) → review-
You need to log in before you can comment on or make changes to this bug.