776603 - Inline styling: hsl(), rgba() stripped

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: Editing, defect)

Description

[Jean-Yves Perrier [:teoli]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120722030555

Steps to reproduce:

https://developer-new.mozilla.org/en-US/docs/CSS/color_value has numerous examples of the form background: hsl(…) or rgba(…).




Actual results:

They are stripped on the real Web site.


Expected results:

They shouldn't. (live example problem)

(maybe opacity related — security sensitive, but not worse than what we have for the moment — stop-gap until we have a system for live example)

Priority: pretty high, like with all the live examples broken, we have to find a solution before going live with Kuma.

Comment 1

[Jean-Yves Perrier [:teoli]]

linear-gradient() and such are also stripped out.

Given the importance of these pages for Mobile Evangelism (and Kilimanjaro), we can't go live without a fix for this.

Comment 2

[Jean-Yves Perrier [:teoli]]

So that I remember to check them once fixed, I will list (some) affected pages.
https://developer-new.mozilla.org/en-US/docs/CSS/color_value
https://developer-new.mozilla.org/en-US/docs/CSS/Multiple_backgrounds
https://developer-new.mozilla.org/en-US/docs/CSS/clip
https://developer-new.mozilla.org/en-US/docs/CSS/gradient
https://developer-new.mozilla.org/en-US/docs/CSS/linear-gradient
https://developer-new.mozilla.org/en-US/docs/CSS/opacity
https://developer-new.mozilla.org/en-US/docs/CSS/radial-gradient
https://developer-new.mozilla.org/en-US/docs/CSS/repeating-linear-gradient
https://developer-new.mozilla.org/en-US/docs/CSS/repeating-radial-gradient
https://developer-new.mozilla.org/en-US/docs/CSS/Using_CSS_gradients
https://developer-new.mozilla.org/en-US/docs/CSS/transform
https://developer-new.mozilla.org/en-US/docs/CSS/transform-origin


Strangely neither -moz-element() does sees to be stripped, nor rgb()! But other functional notation like url(), rgba(), hsl, *-gradient(), rect(), alpha()… are.

Comment 3

[Les Orchard [:lorchard]]

Initial tinkering seems to point to a bug in Bleach. There's a regex that attempts to validate the value of a CSS property, and it seems to dislike "%" or "." inside a parenthesized sequence of numbers. That is:

* rgba(1, 2, 3, 4) is okay
* rgba(0.1, 0.2, 0.3, 0.4) is not
* rgba(10%, 20%, 30%, 40%) is not

Comment 4

[Les Orchard [:lorchard]]

Submitted PR for fix & test to upstream Bleach project:
https://github.com/jsocol/bleach/pull/72

Need that to be accepted, then we can update our own copy of Bleach.

Comment 5

[Jean-Yves Perrier [:teoli]]

If it is not too late, could you add the character ' to this list.

It is used by text-overflow which accept a quoted characters. It borks this page: https://developer-new.mozilla.org/en-US/docs/CSS/text-overflow 

This is also used by other properties, like quotes, but we don't have live example for those.

(Sorry I missed it until now).

If it is too late, I'll open another bug for it and I'll do the change and PR.

Comment 6

[Les Orchard [:lorchard]]

(In reply to Jean-Yves Perrier [:teoli] from comment #5)
> If it is too late, I'll open another bug for it and I'll do the change and
> PR.

This sounds like a separate bug, maybe in a different part in the Bleach code?

Comment 7

[Les Orchard [:lorchard]]

(In reply to Les Orchard [:lorchard] from comment #6)
> (In reply to Jean-Yves Perrier [:teoli] from comment #5)
> > If it is too late, I'll open another bug for it and I'll do the change and
> > PR.
> 
> This sounds like a separate bug, maybe in a different part in the Bleach
> code?

Actually, I think I manages to just roll the fix in with the others in the same PR:
https://github.com/jsocol/bleach/pull/72

Comment 8

[Les Orchard [:lorchard]]

Worth noting that this PR isn't a fix for Kuma - it's a fix for *Bleach*.

To fix Kuma, we have 2 choices:

1) Wait until the upstream Bleach PR is accepted, then update Kuma's vendor library.

2) Use my fork of Bleach in Kuma's vendor library while we wait for the Bleach PR to be accepted.

I'm more comfortable with #1, because that gets the changes reviewed and keeps us from needing to keep track of a fork of the upstream project

Comment 9

[Les Orchard [:lorchard]]

cc'ing :jsocol, since Bleach is his bailiwick

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/054ed79496ae5a37c3cf6a3e30ac7fd714e340bc
fix bug 776603 - bleach update for css value bugs

Comment 11

[Jean-Yves Perrier [:teoli]]

Is this supposed to be fixed on developer-new? (Because it doesn't look so right now).

Comment 12

[John Karahalis [:openjck]]

This should be on -new. Les, and idea?

Comment 13

[Les Orchard [:lorchard]]

Looks like it's working to me, after a re-save:
    https://developer-new.mozilla.org/en-US/docs/CSS/text-overflow

Remember you have to re-save any docs affected by Bleach changes.

Comment 14

[Les Orchard [:lorchard]]

Looks like this one is working for me, too:
    https://developer-new.mozilla.org/en-US/docs/CSS/color_value

Comment 15

[Jean-Yves Perrier [:teoli]]

It looks like some are fixed and some not (I tried to edit/save).
https://developer-new.mozilla.org/en-US/docs/CSS/clip
https://developer-new.mozilla.org/en-US/docs/CSS/gradient
are not working. (Still going through the list)

Comment 16

[Jean-Yves Perrier [:teoli]]

I'm not sure if the fix was enough. % was one part of the problem, but the content of a function can have a lot more different content.

For example gradients will have keywords ('at' or 'to') but also CSS dimensions, <angle>, that is things like 10deg, 10rad, 10grad or 2turn.

Clip is using rect() that takes four <length> (10px, 10em, ...)

For opacity, that's the IE 5.5 version filter() that uses an '='. We could eventually remove this one, though.

Comment 17

[Jean-Yves Perrier [:teoli]]

Reopen as it is not fixed.

It is a launch blocker.

Comment 18

[Les Orchard [:lorchard]]

(In reply to Jean-Yves Perrier [:teoli] from comment #17)
> Reopen as it is not fixed.

Going to try to look at it, but my dev environment is pretty crippled at the moment because my usual laptop is dead.

> It is a launch blocker.

I don't think it should be... IMO at this point, we should only really hold up the launch for big things that affect 50-80% of pages. This is something we can continue to work on even after getting the new site launched.

Comment 19

[Jean-Yves Perrier [:teoli]]

It *is* a launch blocker. It blocks all evangelism effort.

Comment 20

[Les Orchard [:lorchard]]

(In reply to Jean-Yves Perrier [:teoli] from comment #19)
> It *is* a launch blocker. It blocks all evangelism effort.

So... launch is currently in less than 12 hours. I don't think it will be fixed by then.

Comment 21

[Les Orchard [:lorchard]]

(In reply to Les Orchard [:lorchard] from comment #20)
> (In reply to Jean-Yves Perrier [:teoli] from comment #19)
> > It *is* a launch blocker. It blocks all evangelism effort.
> 
> So... launch is currently in less than 12 hours. I don't think it will be
> fixed by then.

Or, actually, a little over 13 hours that is. (10am Pacific, tomorrow)

Comment 22

[John Karahalis [:openjck]]

Ali, Sheppy: Thoughts on whether this should block the launch?

We can make this our highest priority and fix it soon whether or not it blocks launch. To put it another way, making this a launch blocker would not get it done faster, it would just make launch later.

However, making this a launch blocker would ensure that this bug does not appear under the developer.mozilla.org domain. I will let Ali and Sheppy decide whether we should do that.

Comment 23

[Jean-Yves Perrier [:teoli]]

If we go live without this fixed, we have to notify everybody to stop linking to the MDN and gradients until this is fixed.

This was clear in comment 1 and we were unable to test the fix before yesterday.

It means that all evangelism related to Kilimanjaro (both Mobile and B2G) has to be stopped.

Just remove the test on parameters in bleach, that's security theater anyway as we have to allow everything which is valid in CSS.

Comment 24

[Les Orchard [:lorchard]]

I managed to get enough of a dev environment going to hack on Bleach, at least. I submitted these PRs for Bleach and Kuma, respectively:

https://github.com/jsocol/bleach/pull/73
https://github.com/mozilla/kuma/pull/485

But, since my laptop is hosed and my calendar shows :jsocol (ie. the Bleach maintainer) traveling tomorrow, I need help from another dev on the team:

1) Review my Bleach PR. and if looks good, update kuma-lib to use my fork as the submodule and this specific commit:

https://github.com/lmorchard/bleach/commit/2906726af82ed0063a559124cee610bcd2e5859c

Oh yeah, and be sure to push a change to Kuma to update the ref to kuma-lib

2) Then, review the PR for Kuma that turns on the switch to skip the CSS "gauntlet" that seems to be causing most of these issues

Comment 25

[Les Orchard [:lorchard]]

Oh, just as a caveat: I haven't been able to test my code in the Kuma PR at all, and can't troubleshoot it yet myself until I get a dev VM again.

Comment 26

[Jean-Yves Perrier [:teoli]]

Les, you don't know how much this is appreciated.

Comment 27

[James Socol [:jsocol, :james]]

(In reply to Jean-Yves Perrier [:teoli] from comment #23)
> Just remove the test on parameters in bleach, that's security theater anyway
> as we have to allow everything which is valid in CSS.

Bleach will never accept this patch. I'm sorry.

The CSS sanitization may need work--may need complete reworking, in fact--but the option to skip it completely is not part of the 99% use case Bleach targets. In fact, the volume of whitelisted CSS MDN requires is already well beyond what Bleach is designed to do.

MDN's use case is too unique. It is my serious opinion that you fork Bleach and live on a fork. We can't seriously compromise the project for one website, even if that website is MDN.

MDN has a number of additional, unique requirements that have come up that do not belong in Bleach itself. This statement is a reaction not to this particular case but to the latest in a series of "well MDN needs...." requests.

Comment 28

[[github robot]]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/aad11b3a83436fa71c108a3b66e1cf831062f9ab
fix bug 776603: Skip the CSS gauntlet for Kuma

https://github.com/mozilla/kuma/commit/f6eab901f5c64301574d4c897638190327a9344f
Merge pull request #485 from lmorchard/less-style-filtering-776603

fix bug 776603: Skip the CSS gauntlet for Kuma

Comment 29

[John Karahalis [:openjck]]

I think GitHub robot made a mistake here. Still requires our change to Bleach. Correct me if I'm wrong.

Comment 30

[John Karahalis [:openjck]]

Fixed on allizom.

Comment 31

[Les Orchard [:lorchard]]

(In reply to James Socol [:jsocol, :james] from comment #27)
> (In reply to Jean-Yves Perrier [:teoli] from comment #23)
> > Just remove the test on parameters in bleach, that's security theater anyway
> > as we have to allow everything which is valid in CSS.
> 
> Bleach will never accept this patch. I'm sorry.

Thanks for drawing the line in the sand on this one. :) If anything my patch is a 3am reaction to the phrases "launch blocker" and "Kilimanjaro"

> The CSS sanitization may need work--may need complete reworking, in
> fact--but the option to skip it completely is not part of the 99% use case
> Bleach targets. 

I think the thing I keep banging my head against is that "gauntlet" regex. I think it wants to be a CSS validator, but keeps failing on valid CSS constructs. If anything, the reworking there might be to introduce a formal CSS validator, if only optionally.

My "skip" flag in my PR only skips that regex, but leaves the rest of the checks in place.

> In fact, the volume of whitelisted CSS MDN requires is
> already well beyond what Bleach is designed to do.
> 
> MDN's use case is too unique. It is my serious opinion that you fork Bleach
> and live on a fork. We can't seriously compromise the project for one
> website, even if that website is MDN.
> 
> MDN has a number of additional, unique requirements that have come up that
> do not belong in Bleach itself. This statement is a reaction not to this
> particular case but to the latest in a series of "well MDN needs...."
> requests.

Yeah, what I think we've arrived at in IRC is that eventually we need a separate feature set for live code samples, and to back down from whitelisting all the things for content. But, that will take a long time and require a lot of content retooling

Comment 32

[Jean-Yves Perrier [:teoli]]

Just to confirm, this is only a stop-gap for parity and absolutely not something I want to keep forever. 

The CSS gauntlet workaround, the much-too-long-list of CSS properties and the long list of HTML <elements> that we allow make me uncomfortable to0. 

Except for live examples (where we need to use them to show their usage, but in a safe way), I want to completely strip their use from the whole wiki.

The Live Examples Tool and the Find & Replace Tool are the two elements that we'll need so that we can make major advances in the editorial work that is a prerequisite to tighten again the Bleach constraints (which will be done step by step as we progress). 

The needed editorial changes themselves have already started: we are moving little by little most inline styling into rules in CustomCSS (for everything but Live Examples), which has the security of templates right now (editors-only). But that's a huge amount of work, because they have spread on thousands of pages during years.

I don't want people playing with gradients, <font> or font: (and others) in the wiki itself (even I don't want people playing w/ color opacity like rgba() allow them). For security, but also for coherence through the web site, and maintainability.

Comment 33

[Les Orchard [:lorchard]]

Hmm, I never meant to reopen this. We can file a future bug as part of live examples to change back to mainline bleach