Allow [bf]g colors for style attrib #610

lukebarnard1 · 2017-01-11T16:39:40Z

Instead of dropping the style attribute on  tags entirely, sanitise aggressively and only keep background-color and color keys, and also sanitise the values to prevent url(XXXXXX) and expression(XXXXXX) type XSS attacks.

Fix for element-hq/element-web#2460.

Instead of dropping the style attribute on `` tags entirely, sanitise aggressively and only keep `background-color` and `color` keys, and also sanitise the values to prevent `url(XXXXXX)` and `expression(XXXXXX)` type XSS attacks.

Half-Shot · 2017-02-09T14:17:59Z

Is there a reason why it's limited to font? Historically it was just because that was the only way of making colours, but I don't see why we can't do this for p, span and other tags?

lukebarnard1 · 2017-02-09T15:27:14Z

@Half-Shot there's no reason really, but I don't see any reason to use p or span. AFAIK p would look slightly different but I'm not sure that's useful.

Half-Shot · 2017-02-09T17:00:18Z

Fair enough. p is probably a bit useless, but I can see myself using span. font is technically not supported anymore and span is the closest thing to a replacement.

lukebarnard1 · 2017-02-09T17:10:03Z

ah.. I see what you mean

Half-Shot · 2017-02-09T17:11:58Z

Mm. What I was saying is that font was only used because it was a convenient way of avoiding CSS while doing styling. I don't think it should be used now we can avoid it :P

lukebarnard1 · 2017-02-09T17:17:42Z

Using  seems sensible!

lukebarnard1 · 2017-02-22T17:02:42Z

IRL @ara4n and @kegsay have said that we should stick with . And I can't remember why...

Half-Shot · 2017-02-22T17:05:26Z

I assume there are good reasons, but that kinda sucks 🙁

ara4n · 2017-02-22T17:16:33Z

i can't remember why either tbh. perhaps we just shift to span for both (or is that going to run up against HTML sanitizer problems?)

ara4n · 2017-02-22T17:25:24Z

@kegsay do you remember?

kegsay · 2017-02-22T17:46:16Z

Previously, clients could send colours in one of two ways,  or CSS. CSS would get sanitized out by most clients, so  was preferable. If we are now not sanitizing some CSS, then there's no reason to restrict the tag to  only.

Half-Shot · 2017-02-22T18:12:24Z

Previously, clients could send colours in one of two ways, or CSS. CSS would get sanitized out by most clients, so was preferable. If we are now not sanitizing some CSS, then there's no reason to restrict the tag to only.

Mm, I remember that was the reason a long time back. This should hopefully fix all that and I can haz ? 😇

lukebarnard1 · 2017-02-27T10:19:59Z

@kegsay so I suppose this can be done for all tags then?

kegsay · 2017-02-27T11:14:52Z

Yes.

lukebarnard1 · 2017-02-27T11:20:18Z

\o/ /me implements..

ara4n · 2017-03-02T01:09:05Z

src/HtmlUtils.js

+
+            const pairs = attribs.style.split(';');
+            let sanitisedStyle = "";
+            for (let i = 0; i < pairs.length; i++) {


hand-sanitizing CSS like this scares me quite a lot. In this instance it feels quite fragile, and will fail if there's any white-space in the expression? which feels wrong, given whitespace is valid in CSS so we're defining our own custom derivative of CSS here which is generally bad karma.

However, in the longer term, we probably do want users to be able to enter CSS into their HTML messages in order to get pixel-perfect layout (and hopefully sandboxing tech will have caught up by then to ensure we can render the contents in some kind of sandbox to protect the rest of the app from it). So i guess it's not that bad here... as long as the sanitizer is neither to lax nor too strict.

hand-sanitizing CSS like this scares me quite a lot

Should we look into a library that does this instead?

will fail if there's any white-space in the expression
defining our own custom derivative of CSS

Agreed that we shouldn't reinvent the wheel here, but the rationale was to be super strict such as not to allow any possibility of url(...) or expression(...). This will inevitably end up being are own version of CSS but are we not already doing that with HTML?

lukebarnard1 · 2017-03-02T10:58:01Z

So IRL, we discussed that we shouldn't be writing our own CSS sanitizer and using a custom data attribute (data-mx[-bg]-color) would be better because then we only have to spec that and not the slimmed-down CSS that we would inevitably have to spec otherwise.

This keeps things simple: remove style and transform the custom data attributes to CSS. Then we only allow hex #RRGGBB style colours.

This has the benefit of not needing a spec for custom CSS. Instead we rigourously sanitise the values for custom data attributes that are transformed to CSS equivalents. `data-mx-color` translates to CSS `color` for example.

dbkr · 2017-03-02T17:41:00Z

src/HtmlUtils.js

@@ -136,6 +138,37 @@ var sanitizeHtmlParams = {
            attribs.rel = 'noopener'; // https://mathiasbynens.github.io/rel-noopener/
            return { tagName: tagName, attribs : attribs };
        },
+        '*': function(tagName, attribs) {
+            // Delete any style previously assigned
+            delete attribs.style;


Doesn't this make allowing style above pointless? If not, probably needs a comment as to why not :)

Transforming is done before attributes are stripped (sensibly). I shall comment.

kegsay · 2017-03-03T15:40:37Z

src/HtmlUtils.js

@@ -28,6 +28,7 @@ emojione.imagePathSVG = 'emojione/svg/';
 emojione.imageType = 'svg';

 const EMOJI_REGEX = new RegExp(emojione.unicodeRegexp+"+", "gi");
+const COLOR_REGEX = /#[0-9a-fA-F]{6}/;


Woah there! Careful!

> r = /#[0-9a-fA-F]{6}/ /#[0-9a-fA-F]{6}/ > r.test("url(https://evil.com)/*#ffffff*/") true

You need to use start and end symbols!

> r = /^#[0-9a-fA-F]{6}$/ /^#[0-9a-fA-F]{6}$/ > r.test("url(https://evil.com);/*#ffffff*/") false > r.test("#ffffff") true

Luke Barnard and others added 3 commits January 11, 2017 16:35

Allow [bf]g colors for style attrib

8e3f2eb

Instead of dropping the style attribute on `` tags entirely, sanitise aggressively and only keep `background-color` and `color` keys, and also sanitise the values to prevent `url(XXXXXX)` and `expression(XXXXXX)` type XSS attacks.

Only transform 

32185be

Merge branch 'develop' into luke/feature-css-msg-colors

ae03244

Luke Barnard added 2 commits February 27, 2017 11:23

Sanitise for *, fix style issues

886b0a3

Allow span, and only allow style attrib

5fc828f

lukebarnard1 requested a review from dbkr February 27, 2017 11:33

ara4n reviewed Mar 2, 2017

View reviewed changes

Luke Barnard added 2 commits March 2, 2017 11:36

Use data-mx[-bg]-color instead of stripping style

36795fa

This has the benefit of not needing a spec for custom CSS. Instead we rigourously sanitise the values for custom data attributes that are transformed to CSS equivalents. `data-mx-color` translates to CSS `color` for example.

Remove custom attribs as consumed

b951713

lukebarnard1 assigned dbkr Mar 2, 2017

Have COLOR_REGEX constant

0f8ab99

dbkr reviewed Mar 2, 2017

View reviewed changes

dbkr assigned lukebarnard1 and unassigned dbkr Mar 2, 2017

Update comment

f4278b6

lukebarnard1 assigned dbkr and unassigned lukebarnard1 Mar 2, 2017

dbkr approved these changes Mar 2, 2017

View reviewed changes

dbkr assigned lukebarnard1 and unassigned dbkr Mar 2, 2017

lukebarnard1 merged commit 6a007d0 into develop Mar 3, 2017

kegsay reviewed Mar 3, 2017

View reviewed changes

dkasak mentioned this pull request Mar 2, 2018

Support data-mx[-bg]-color attributes element-hq/riot-android#2023

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow [bf]g colors for <font> style attrib #610

Allow [bf]g colors for <font> style attrib #610

lukebarnard1 commented Jan 11, 2017 •

edited

Loading

Half-Shot commented Feb 9, 2017

lukebarnard1 commented Feb 9, 2017

Half-Shot commented Feb 9, 2017 •

edited

Loading

lukebarnard1 commented Feb 9, 2017

Half-Shot commented Feb 9, 2017

lukebarnard1 commented Feb 9, 2017

lukebarnard1 commented Feb 22, 2017 •

edited

Loading

Half-Shot commented Feb 22, 2017

ara4n commented Feb 22, 2017

ara4n commented Feb 22, 2017 •

edited by lukebarnard1

Loading

kegsay commented Feb 22, 2017

Half-Shot commented Feb 22, 2017

lukebarnard1 commented Feb 27, 2017

kegsay commented Feb 27, 2017

lukebarnard1 commented Feb 27, 2017

ara4n Mar 2, 2017

lukebarnard1 Mar 2, 2017

lukebarnard1 commented Mar 2, 2017

dbkr Mar 2, 2017

lukebarnard1 Mar 2, 2017

kegsay Mar 3, 2017

lukebarnard1 Mar 3, 2017

Allow [bf]g colors for <font> style attrib #610

Allow [bf]g colors for <font> style attrib #610

Conversation

lukebarnard1 commented Jan 11, 2017 • edited Loading

Half-Shot commented Feb 9, 2017

lukebarnard1 commented Feb 9, 2017

Half-Shot commented Feb 9, 2017 • edited Loading

lukebarnard1 commented Feb 9, 2017

Half-Shot commented Feb 9, 2017

lukebarnard1 commented Feb 9, 2017

lukebarnard1 commented Feb 22, 2017 • edited Loading

Half-Shot commented Feb 22, 2017

ara4n commented Feb 22, 2017

ara4n commented Feb 22, 2017 • edited by lukebarnard1 Loading

kegsay commented Feb 22, 2017

Half-Shot commented Feb 22, 2017

lukebarnard1 commented Feb 27, 2017

kegsay commented Feb 27, 2017

lukebarnard1 commented Feb 27, 2017

ara4n Mar 2, 2017

Choose a reason for hiding this comment

lukebarnard1 Mar 2, 2017

Choose a reason for hiding this comment

lukebarnard1 commented Mar 2, 2017

dbkr Mar 2, 2017

Choose a reason for hiding this comment

lukebarnard1 Mar 2, 2017

Choose a reason for hiding this comment

kegsay Mar 3, 2017

Choose a reason for hiding this comment

lukebarnard1 Mar 3, 2017

Choose a reason for hiding this comment

lukebarnard1 commented Jan 11, 2017 •

edited

Loading

Half-Shot commented Feb 9, 2017 •

edited

Loading

lukebarnard1 commented Feb 22, 2017 •

edited

Loading

ara4n commented Feb 22, 2017 •

edited by lukebarnard1

Loading