528693 - HTML in new version release notes show up unparsed in the add-ons manager.

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: API, defect, P3)

Description

[Kim]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091107 Gentoo Firefox/3.5.5
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091107 Gentoo Firefox/3.5.5

Unparsed as in the actual tags (<tagname>) are displayed.

Reproducible: Always

Steps to Reproduce:
1. Click "Find updates" in the add-ons manager.
2. Select an update were the release notes use HTML, for example Personas 1.4.
3. Click Show Information.
Actual Results:  
For some reason I'm not allowed to select the text in the add-ons manager.
Anyway, the tags for the <ul> list and its <li> items are neither rendered nor removed from the displayed text. There's also an URL which would require more than simple removal.

Expected Results:  
Either properly rendered content or at the very least all HTML tags removed with a visible href.

"<a href="example.com">example</a>" turning into "example (example.com)" ought to do it.

Comment 1

[Justin Scott [:fligtar]]

I think the release notes of an add-on can contain some HTML tags, but not links. Dave, do you have a list of the tags it can contain somewhere?

Comment 2

[Dave Townsend [:mossop]]

We currently only support h1, h2, h3, p, ul, ol, li, b, strong, i, em tags.

https://developer.mozilla.org/en/extension_versioning,_update_and_compatibility#Providing_Details_about_Updates

Comment 3

[Matthew N. [:MattN]]

Attachment: Screenshot of Greasemonkey update

Here is the URL for an affect update:
https://addons.mozilla.org/en-US/firefox/versions/updateInfo/86982/en-US/

and the resulting display in Firefox 3.6 Nightlies is attached (screenshot)

AMO is escaping the HTML and adding <p>...</p> around it

Comment 4

[Wil Clouser [:clouserw]]

Attachment: patch

Here is a patch that fixes this.  I'm running it through nl2br() in the hopes that the client will support <br/> soon (I filed bug 534646).  The current code to use <p> broke horribly with <li>'s.

Comment 5

[Wil Clouser [:clouserw]]

(In reply to comment #2)
> We currently only support h1, h2, h3, p, ul, ol, li, b, strong, i, em tags.
> 
> https://developer.mozilla.org/en/extension_versioning,_update_and_compatibility#Providing_Details_about_Updates

The docs say other tags "are ignored" - does that mean they are stripped or escaped?  If they are escaped I should probably remove the nl2br() from the patch.

Comment 6

[Fred Wenzel [:wenzel]]

Comment on attachment 417493 [details] [diff] [review]
patch

r+ with a few notes:

- Please don't commit this until your question is answered (will the <br>s show up verbatim?).
- Why did you change this part in the first place? What wasn't good about the <p> tags which are supported by the browser?

Comment 7

[Wil Clouser [:clouserw]]

> - Why did you change this part in the first place? What wasn't good about the
> <p> tags which are supported by the browser?

Because you end up with invalid stuff like:
<p>
<ul></p><p>
<li>something</li></p><p>
<li>something</li></p><p>
</ul></p><p>
</p>

Comment 8

[Fred Wenzel [:wenzel]]

Oh. Makes sense!

Comment 9

[Wil Clouser [:clouserw]]

Committed in r57897

stephend - can you help me figure out how to show this in the browser so we can see what "ignore" means?

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (In reply to comment #2)
> > We currently only support h1, h2, h3, p, ul, ol, li, b, strong, i, em tags.
> > 
> > https://developer.mozilla.org/en/extension_versioning,_update_and_compatibility#Providing_Details_about_Updates
> 
> The docs say other tags "are ignored" - does that mean they are stripped or
> escaped?  If they are escaped I should probably remove the nl2br() from the
> patch.

I'm pretty sure that the tags are essentially stripped, so <br /> would get dropped, "<foo>bar</foo>" would become "bar", but I intend to put up a test page later today to verify this.

Comment 11

[Dave Townsend [:mossop]]

So, using <br /> is fine (currently ignored though) but wrapping everything in <p></p> is a bad thing as the client currently only looks for <ul>, <ol> and the various heading tags when outside of a <p> tag. This is arguably correct according to the html spec which is probably why I went this way in the first place, but clearly that is difficult to work with. I could look at making the code less constrained which we could potentially get into a branch too, but that doesn't help you right now I guess.

If you need to experiment I put up http://www.oxymoronical.com/experiments/updateinfo/ which will take some html and show you what the client would display for that.

Comment 12

[Wil Clouser [:clouserw]]

Thanks Mossop.  I switched the <p>s to <div>s in r57913.  I think we're done here.  For authors; note that AMO supports more HTML than the client in this case so be aware of that when you make your notes.

Comment 13

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

Verified FIXED, as best I can tell:

* no more XSS on https://preview.addons.mozilla.org/en-US/developers/versions/edit/79336
* see forthcoming screenshot for the in-client update window

Comment 14

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

Attachment: Screenshot of client