571547 - HTML in add-on summary/descriptions

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Tony Chung [:tchung]]

Attachment: personas description screenshot

When installing the themes on firefoxcup site, the descriptions aren't being rendered properly in the new addons manager.

See screenshot

Repro:
1) install nightly build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100611 Minefield/3.7a6pre
2) install a firefox cup theme from the URL
3) open addons manager, and verify descriptions aren't rendered

Expected:
- descriptions rendered properly

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As far as Dave mentioned on IRC we don't wanna do that due to possible security holes.

It is also present in older builds like Firefox 3.6 inside the about addon dialog. So it's not a regression.

Comment 2

[Dave Townsend [:mossop]]

The bug is that the Firefox cup personas shouldn't contain html in their descriptions.

Comment 3

[Jeff Balogh (:jbalogh)]

Mossop: AMO allows links in add-on summaries and limited HTML in descriptions.  Is that a problem for the add-ons manager?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Mossop: AMO allows links in add-on summaries and limited HTML in descriptions. 
> Is that a problem for the add-ons manager?

There are two different bugs here. This bug was originally about how html was included in the persona install object. Another bug is that the API is returning html in the description fields, the add-ons manager has never supported that.

Comment 5

[Justin Scott [:fligtar]]

It would be nice if the Add-ons Manager would strip out the stuff it doesn't like so that we don't have to limit the information provided through our API for all its consumers, but I suppose we can default to stripping HTML unless some flag is passed.

The release notes service should still provide HTML but I don't think that's part of the API.

Comment 6

[Wil Clouser [:clouserw]]

(In reply to comment #5)
> It would be nice if the Add-ons Manager would strip out the stuff it doesn't
> like so that we don't have to limit the information provided through our API
> for all its consumers, but I suppose we can default to stripping HTML unless
> some flag is passed.

This isn't good.  The consumer should be modifying data if it needs modified.

Comment 7

[Justin Scott [:fligtar]]

True, but we also changed the way the API works by allowing HTML. I don't think it's the Add-ons Manager's job to make all the input it gets pretty. If people want to throw in HTML or XML or bbcode they can, but it doesn't mean it should have to recognize all that and strip it out. Since we are the ones that started allowing that for our purposes, I think this is our bug, not theirs.

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #6)
> (In reply to comment #5)
> > It would be nice if the Add-ons Manager would strip out the stuff it doesn't
> > like so that we don't have to limit the information provided through our API
> > for all its consumers, but I suppose we can default to stripping HTML unless
> > some flag is passed.
> 
> This isn't good.  The consumer should be modifying data if it needs modified.

In general I agree, but we never have done it this way around for the life of the API, nor do we have code immediately available in the client right now to be able to do this easily but I understand that AMO does.

Comment 9

[Wil Clouser [:clouserw]]

(In reply to comment #7)
> True, but we also changed the way the API works by allowing HTML. I don't think
> it's the Add-ons Manager's job to make all the input it gets pretty. If people
> want to throw in HTML or XML or bbcode they can, but it doesn't mean it should
> have to recognize all that and strip it out. Since we are the ones that started
> allowing that for our purposes, I think this is our bug, not theirs.

We're going to have different flags to strip out html, xml, or bbcode?  If people put it in, we send it out.  We shouldn't care if bbcode shows up in the API or the add-on manager.

> In general I agree, but we never have done it this way around for the life of
> the API, nor do we have code immediately available in the client right now to
> be able to do this easily but I understand that AMO does.

How much work is it to change on your end?  I wouldn't call it "easy" on our end either, and if it takes a little extra time to do it right, I'd rather do that.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> > In general I agree, but we never have done it this way around for the life of
> > the API, nor do we have code immediately available in the client right now to
> > be able to do this easily but I understand that AMO does.
> 
> How much work is it to change on your end?  I wouldn't call it "easy" on our
> end either, and if it takes a little extra time to do it right, I'd rather do
> that.

I guess it depends what level you want to go to. Stripping out anything between < and > characters is pretty simple. Actually rendering the html is extremely difficult. A middle ground of trying to do something sane with the html tags is also difficult. Ironically we don't have any sane way to parse html fragments in the client so we'd have to find some code that does that, that has a good license and that we can trust and integrate it into the API handling code.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have talked to Ben Bucksch and he mentioned the nsPlainTextSerializer class, which could be used to transform HTML content into plain text. Here is an example test:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestPlainTextSerializer.cpp

Would that help us?

Comment 12

[Ben Bucksch (:BenB)]

Wil Clouser, whether a field contains plaintext or HTML or bbcode or XML is very much part of its definition. You need to nail that down, otherwise the consumer can't properly process it (how is it to know whether it's plaintext or bbcode or HTML?).
If the field used to return plaintext, you need to continue to return plaintext.

You can also define it as HTML - then the client has the problem of processing it. Apparently that is a problem here, so chose well. Either way, if you define as plaintext or HTML, it won't be bbcode, then.

whimboo asked me. There is a HTML to TXT converter in Gecko - it's called nsPlainTextSerializer. It's used both for Thunderbird mail send as well as copy&paste in Firefox. You could use it, but its use is not entirely trivial.
You can see its use in
http://mxr.mozilla.org/comm-central/source/mozilla/parser/htmlparser/tests/outsinks/Convert.cpp#111 (function HTML2text())
I don't know of any JS user in the tree, off-hand.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #11)
> I have talked to Ben Bucksch and he mentioned the nsPlainTextSerializer class,
> which could be used to transform HTML content into plain text. Here is an
> example test:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/TestPlainTextSerializer.cpp
> 
> Would that help us?

Huh, not come across this before. It looks like we might be able to construct something useful out of it though on the surface it may not be easy since it looks like it can't be accessed from JavaScript.

Comment 15

[Dave Townsend [:mossop]]

I've filed bug 595280 to make the current add-ons manager able to cope with this, however it's unlikely to be something that we backport so I suggest API versions < 1.5 should strip the HTML out appropriately.

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #15)
> I've filed bug 595280 to make the current add-ons manager able to cope with
> this, however it's unlikely to be something that we backport so I suggest API
> versions < 1.5 should strip the HTML out appropriately.

Filed bug 595339.

Comment 17

[Wil Clouser [:clouserw]]

The two bugs mentioned in comments 15 and 16 are closed, so....I guess this is fixed?