Once the DOMParser can parse HTML, the microsummary service should use DOMParser instead of hacky hidden iframes to parse HTML.

I found nsScriptableUnescapeHTML::ParseFragment is calling Layout's html DOM parser, and that is scriptable, literally. http://lxr.mozilla.org/mozilla/source/toolkit/components/feeds/src/nsScriptableUnescapeHTML.cpp http://lxr.mozilla.org/mozilla/source/toolkit/components/feeds/public/nsIScriptableUnescapeHTML.idl So bug 102699 is not blocking this bug any longer, is it?

bug 102699 is blocking because nsScriptableUnescapeHTML::ParseFragment is calling a content sink written for it, which avoids the problems with scripts, meta elements, and frames because it doesn't process them.

(In reply to comment #2) > bug 102699 is blocking because nsScriptableUnescapeHTML::ParseFragment is > calling a content sink written for it, which avoids the problems with scripts, > meta elements, and frames because it doesn't process them. Hmm, I wonder if nsScriptableUnescapeHTML::ParseFragment is sufficient, given that the microsummary service already turns off scripts and frames in the hidden iframe it uses to load content. It doesn't turn off meta elements, though. What are the problems with those elements that prompted us to avoid them for nsScriptableUnescapeHTML::ParseFragment?

> It doesn't turn off meta elements, though. Yes, it does (and does not create a document head anyway). To see the list of elements and attributes allowed by the ParanoidFragmentSinks used by nsScriptableUnescapeHTML::ParseFragment, see http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentSink.h#131 this is used by the feed preview to disallow all sorts of things that don't belong in feeds, and present security risks in privileged contexts.

Aha. Then do you think it is possible to switch the list at the constructor into, say, >static nsIAtom** const kAllowedTagsForMicroSummary [] = { > &nsGkAtoms::head, > &nsGkAtoms::link, > ... >}; etc. to create another DOM parser, sharing most of codebase of feeds?

(In reply to comment #4) > > It doesn't turn off meta elements, though. > > Yes, it does (and does not create a document head anyway). I meant that the microsummary service doesn't turn off meta elements. > this is used by the feed preview to disallow all sorts of things that don't > belong in feeds, and present security risks in privileged contexts. The microsummary service doesn't need to load pages in privileged contexts, but it nevertheless turns off unnecessary elements to minimize risk in the event of privilege escalation bugs. It seems like there's significant overlap between the HTML parsing needs of feeds and microsummaries, so if there's a way for the microsummary service to use nsScriptableUnescapeHTML::ParseFragment, that would be very helpful, since the alternative is significantly more complicated (although it's also wanted for a number of other cases besides microsummaries, so it might happen regardless).

(In reply to comment #6) > (In reply to comment #4) > > > It doesn't turn off meta elements, though. > > > > Yes, it does (and does not create a document head anyway). > > I meant that the microsummary service doesn't turn off meta elements. Ah, gotcha. > > > this is used by the feed preview to disallow all sorts of things that don't > > belong in feeds, and present security risks in privileged contexts. > > The microsummary service doesn't need to load pages in privileged contexts, but > it nevertheless turns off unnecessary elements to minimize risk in the event of > privilege escalation bugs. How does it turn them off? > > It seems like there's significant overlap between the HTML parsing needs of > feeds and microsummaries, so if there's a way for the microsummary service to > use nsScriptableUnescapeHTML::ParseFragment, that would be very helpful, since > the alternative is significantly more complicated (although it's also wanted > for a number of other cases besides microsummaries, so it might happen > regardless). I think it would be better to go this route, and generalize anything useful from the content sinks I wrote for feeds.

> > The microsummary service doesn't need to load pages in privileged contexts, > > but it nevertheless turns off unnecessary elements to minimize risk in the > > event of privilege escalation bugs. > > How does it turn them off? It twiddles these docshell attributes: allowJavascript allowAuth allowPlugins allowMetaRedirects allowSubframes allowImages > I think it would be better to go this route, and generalize anything useful > from the content sinks I wrote for feeds. That seems reasonable to me.

this patch is messy and incomplete -- no drive-by reviews, please. :)

Doesn't that patch belong in bug 102699? I thought bug 102699 was about making DOMParser support text/html and this bug was about making Microsummaries use that capability once it was fixed. (And there's a good bit of stuff in there about what needs to be done, e.g., regarding charset detection.)

(In reply to comment #10) > Doesn't that patch belong in bug 102699? I thought bug 102699 was about making > DOMParser support text/html and this bug was about making Microsummaries use > that capability once it was fixed. Oh, I missed that distinction. Will move it over there.

Comment on attachment 257988 [details] [diff] [review] WIP -- it's alive patch obsoleted

- BUGSPAM - Wontfixing all Microsummaries bugs, since the feature has been removed from the core product and previous versions won't get further fixes for it. If interested in supporting Microsummaries in your add-on, you're free to use our old microsummaries code and to search all previously open bugs by looking for [microsummaries-feature-removal] in the status whiteboard field.