679633 - Support html styling tags in add-on descriptions

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, enhancement)

Description

[Arivald]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

My extension for Thunderbird, Stationery, use some of allowed HTML tags in description.
But in addon manager, when I open my extension description, i can't see effects of this tags.
in detail:
1. <b> and <strong> are not bold.
2. <code> is not monospace.
3. Lists have no numbers or bullets




Expected results:

As I check, extension description in addon manager is just single <description> tag. Some code transform HTML into plain text.

But this should not happen. HTML tags should be preserved, or converted to some XUL equivalents. Description should be rendered similarly like in AMO (when they fix new AMO skin)

Or using HTML tags in extension desctiption on AMO should be disabled.

Or AMO should allow two versions of descriptions, one for AMO (html), and one for addon manager (plain text).

Comment 1

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

The description on AMO also looks a bit weird:
https://addons.mozilla.org/en-US/thunderbird/addon/stationery/

Could that be the reason why we have this issue in the AOM?

Comment 2

[Dave Townsend [:mossop]]

No the issue is that we strip all HTML from the descriptions right now

Comment 3

[Arivald]

Attachment: How it should look

(In reply to Henrik Skupin (:whimboo) from comment #1)
> The description on AMO also looks a bit weird:
> https://addons.mozilla.org/en-US/thunderbird/addon/stationery/
> 
> Could that be the reason why we have this issue in the AOM?

AMO have this problem as well, but on AMO tags are present, but CSS is broken.
There is separate bug 678415 for this issue.

Basically description in AMO and in addon manager should look like in AMO devlopers area (picture in attachment).

Comment 4

[Ben Bucksch (:BenB)]

Please note that HTML is obviously much more dangerous than plaintext. You can try to sanitize (and you must), but it's still dangerous, because there can be cases you didn't think of.

I recommend against this bug, or to at most do what Firefox does.

Comment 5

[Arivald]

HTML is already sanitized by AMO. Allowed is only sub-set of tags (<a href title> <abbr title> <acronym title> <b> <blockquote> <code> <em> <i> <li> <ol> <strong> <ul>), without any custom styling.

By the way, AMO is fixed now, and look correctly. 
It will be good to have similar look in addon manager.

Comment 6

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

CC'ing Jorge to get more input from the AMO side.

Comment 7

[Ben Bucksch (:BenB)]

> HTML is already sanitized by AMO.

Irrelevant. For security purposes of the client, the server is hostile by definition. That's the whole point.

Comment 8

[Arivald]

(In reply to Ben Bucksch (:BenB) from comment #7)
> > HTML is already sanitized by AMO.
>
> Irrelevant. For security purposes of the client, the server is hostile by
> definition. That's the whole point.

I do not say to accept any HMTL from server. I just point out that AMO already have HMTL sanity rules. It is quite simple to write filter, to remove any unwanted tags and CSS. In fact such procedure already exists on AMO.

For displaying purposes in addons manager it even do not need to be HTML, it may be XUL labels just styled to look like HML on AMO. Or even image rendered on <canvas>.

Second thing, "dangerous/hostile server" assume that it is third party server, or communications channel is not secure. But AMO is not third party, it should be considered friendly. And communication channel, I believe addons manager use SSL to communicate with AMO? 


But, if properly styled description in addon manager is too much risk, then at least allow on AMO to write alternative version of description, for plain text only.

Comment 9

[Ben Bucksch (:BenB)]

> It is quite simple to write filter

Not at all.

> AMO is not third party

Please see the XUL security primer.

<http://www.mozilla.org/projects/security/components/design.html> defines as a ground rule: "No web-based XUL". Given that HTML injected into XUL is essentially the same as XUL, you are violating a fundamental security assumption.

We had the same thing in Firefox. See bug 595280. Please just do the same as Firefox did.

Comment 10

[Ben Bucksch (:BenB)]

(all servers are third party. We do warn before installing XPIs from AMO.)

Comment 11

[Arivald]

(In reply to Ben Bucksch (:BenB) from comment #9)
> > It is quite simple to write filter
>
> Not at all.

It is not very difficult. First parse it in iframe, then walk dom an serialize what we need.
nsIDocumentEncoder plus proper node fixer make it even simpler.

> > AMO is not third party
>
> Please see the XUL security primer.
>
> <http://www.mozilla.org/projects/security/components/design.html> defines as
> a ground rule: "No web-based XUL". Given that HTML injected into XUL is
> essentially the same as XUL, you are violating a fundamental security
> assumption.

I read this document. "No web-based XUL" it says... But it are limits applied to "web-based code".

Addon manager is not web based code. 
It just loads some text from AMO - no security risk.
Currently it converted it to plain text - no security risk.
So why do not convert to simple, sanitized HTML? What risk in such conversion? Or conversion result?


If You want apply "No web-based XUL" rule to addon manager, You should limit it to information from XPI only, and never query AMO.

>
> We had the same thing in Firefox. See bug 595280. Please just do the same as
> Firefox did.

Bug 595280 is exactly problem here. Somebody choose simple, lazy solution "just strip all HMTL, we already have function for this".

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> CC'ing Jorge to get more input from the AMO side.

It'd be nice for the AOM to support text formatting. It looks specially bad for <ul> and <ol> tags, where all structure is lost. However, this is not a trivial task given the security concerns, as commented previously.

Comment 13

[Ben Bucksch (:BenB)]

> Bug 595280 is exactly problem here. Somebody choose simple, lazy solution

No, it was not the simple, lazy solution, but the secure solution.

FWIW, we already have an HTML sanitizer in Mozilla and Thunderbird (see Message Body as | Simple HTML). I wrote it.

I do not believe that it is sufficient to use it to include it in a chrome document. It was meant as additional protection on top of being a sandboxed HTML document. As the author of just such a filter that you propose, I maintain that it's way too dangerous to include HTML sniplets from a website into a chrome (!) document.

Comment 14

[Ben Bucksch (:BenB)]

> It'd be nice for the AOM to support text formatting.
> It looks specially bad for <ul> and <ol> tags

FWIW, the HTML -> plaintext converter has a mode where it nicely formats such block-level elements. This is what's used in the mail display, too.

Comment 15

[Ben Bucksch (:BenB)]

Also, there's progress on bug 80713. With this, we could stuff the foreign HTML into a sandboxed iframe. That would solve the security problem.