Whitelist more CSS3 options in skins

Bug #1264098 reported by Melissa Newman
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Mahara
Fix Released
Medium
Son Nguyen
16.10
Fix Released
Medium
Unassigned

Bug Description

I tried to create a new skin with custom CSS code added to the "Advanced" tab.

collection-nav ul {
  columns: 2;
  -webkit-columns: 2;
  -moz-columns: 2;
}

When I save it, and then try to edit the skin, everything is deleted except:

ul {
}

Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote :

It seems that some of the code is stripped out due to purifying the CSS.

Changed in mahara:
status: New → Confirmed
milestone: none → 1.9.0
importance: Undecided → Medium
Changed in mahara:
assignee: nobody → Son Nguyen (ngson2000)
Revision history for this message
Son Nguyen (ngson2000) wrote :

The css property 'columns' is not allowed by HTMLPurifier.
I think we need to discuss more about what css styles and properties should be allowed in Mahara.

Revision history for this message
Melissa Newman (melissa-l) wrote :

The discussion is simple ... everything that is a valid css property should be allowed. CSS does not cause malicious code, so there is absolutely no reason why it needs to be filtered. And if somebody does something that causes an affect the user does not want (white text on a white background), simply include a button that either allows the page to be be temporarily set to default settings or revert to the last saved settings.

Mahara's main problem is that Mahara tries to hold the hand of the user too much. There is a point in trying to help the user by making things easier, but when the help frustrates the user, the help is of no help.

Revision history for this message
Son Nguyen (ngson2000) wrote :

Some css properties and their values need to be sanitized to prevent injections or phishing
For example,

background-image: url(javascript:alert('Injected'));
-moz-binding: url('http://virus.com/htmlBindings.xml');
position: absolute;

See more at https://code.google.com/p/browsersec/wiki/Part1#Cascading_stylesheets

Revision history for this message
Son Nguyen (ngson2000) wrote :

https://reviews.mahara.org/2872 add some popular CSS3 properties

See http://www.quackit.com/css/css3/properties for more info

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Hi Son,

Would it make sense to upstream some of those allowed properties back to HTMLPurifier? I notice in their 4.5.0 release ( http://repo.or.cz/w/htmlpurifier.git/blob/20eff0a3a07b2bbb6d42e4867293b119508b2f08:/NEWS ) they added some additional CSS3 properties, so they seem to be interested in expanding the list.

Cheers,
Aaron

Revision history for this message
Robert Lyon (robertl-9) wrote :

I did some testing with this chunk of css, which according to w3schools.com are valid css rules.

/* tests for column styles */
.test1 {
 column-width: 10px;
 column-rule-width: 5px;
 column-rule-style: double;
 column-rule-color: #deface;
 column-count: 3;
 column-fill: balance;
 column-gap: 10px;
 column-span: 1;
}
.test2 {
 column-width: auto;
 column-rule-width: medium;
 column-rule-style: none;
 column-rule-color: black;
 column-count: auto;
 column-fill: auto;
 column-gap: normal;
 column-span: all;
}
.test3 {
 columns: 10px 3;
 column-rule: 5px double #deface;
}
.test4 {
 columns: auto auto;
 column-rule: medium none black;
}

/* tests for border styles */
.test5 {
 border-top-left-radius: 5px;
 border-top-right-radius: 5px;
 border-bottom-left-radius: 5px 3px;
 border-bottom-right-radius: 5px;
 border-image-source: url(cats.jpg);
 border-image-slice: fill;
 border-image-width: auto;
 border-image-outset: 30 30;
 border-image-repeat: round;
}
.test6 {
 border-top-left-radius: 0;
 border-top-right-radius: 0;
 border-bottom-left-radius: 0;
 border-bottom-right-radius: 0;
 border-image-source: none;
 border-image-slice: 100%;
 border-image-width: 1;
 border-image-outset: 0;
 border-image-repeat: stretch;
}
.test7 {
 border-radius: 5px 3px;
 border-image: url(cats.jpg) fill auto 30 30 round;
}
.test8 {
 border-radius: 0 0;
 border-image: none 100% 1 0 stretch;
}

/* tests for transition styles */
.test9 {
 transition-property: width;
 transition-duration: 5s;
 transition-timing-function: cubic-bezier(0.2,0.3,0.4,0.5);
 transition-delay: 2s;
}
.test10 {
 transition-property: all;
 transition-duration: 0;
 transition-timing-function: ease;
 transition-delay: 0;
}
.test11 {
 transition: width 5s cubic-bezier(0.2,0.3,0.4,0.5) 2s;
}
.test12 {
 transition: all 0 ease 0;
}
/* tests for transform properties */
.test13 {
 transform: rotate(-12deg);
 transform-style: preserve-3d;
 transform-origin: left bottom 5px;
}
.test14 {
 transform: none;
 transform-style: flat;
 transform-origin: 50% 50% 0;
}

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Robert, what was the result of your test?

Revision history for this message
Robert Lyon (robertl-9) wrote :

Ah the results of the test are on the patch 2872

Changed in mahara:
milestone: 1.9.0 → 1.10.0
Changed in mahara:
status: Confirmed → In Progress
Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "master" branch: https://reviews.mahara.org/3679

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "master" branch: https://reviews.mahara.org/3682

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "master" branch: https://reviews.mahara.org/3681

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "master" branch: https://reviews.mahara.org/3683

Aaron Wells (u-aaronw)
summary: - skins not saving properly
+ Whitelist more CSS3 options in skins
Revision history for this message
Aaron Wells (u-aaronw) wrote :

The rgba concern from bug 1243004 is handled by patch 3679.

Revision history for this message
Robert Lyon (robertl-9) wrote :

CSS comments get stripped out and this is a pain if you are trying to comment your css to remember what the styles are doing.

See: https://mahara.org/interaction/forum/topic.php?id=6443&offset=0&limit=10

Revision history for this message
Aaron Wells (aaronwells) wrote :

A Mahara community member posted a separate bug about the CSS comments: https://bugs.launchpad.net/mahara/+bug/1369830

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/3679
Committed: http://gitorious.org/mahara/mahara/commit/9694c3a2965a8d9cc5789879cd4a6f0960423544
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 9694c3a2965a8d9cc5789879cd4a6f0960423544
Author: Son Nguyen <email address hidden>
Date: Tue Sep 2 09:04:57 2014 +1200

Add more legal color formats (Bug 1264098)

- rgba()
- hsl()
- hsla()

Change-Id: I0aedf72eac821e9a8f126ac91c098f4ad091c046
Signed-off-by: Son Nguyen <email address hidden>

Revision history for this message
Robert Lyon (robertl-9) wrote :

I was testing which html tags are allowed to have css rules.
If I try to add display: none to all the html tags (see www.w3schools.com/tags/ for list of valid tags and what they do).

a,
abbr,
acronym,
address,
applet,
area,
article,
aside,
audio,
b,
base,
basefont,
bdi,
bdo,
big,
blockquote,
body,
br,
button,
canvas,
caption,
center,
cite,
code,
col,
colgroup,
datalist,
dd,
del,
details,
dfn,
dialog,
dir,
div,
dl,
dt,
em,
embed,
fieldset,
figcaption,
figure,
font,
footer,
form,
frame,
frameset,
head,
header,
hgroup,
h1,
h2,
h3,
h4,
h5,
h6,
hr,
html,
i,
iframe,
img,
input,
ins,
kbd,
keygen,
label,
legend,
li,
link,
main,
map,
mark,
menu,
menuitem,
meta,
meter,
nav,
noframes,
noscript,
object,
ol,
optgroup,
option,
output,
p,
param,
pre,
progress,
q,
rp,
rt,
ruby,
s,
samp,
script,
section,
select,
small,
source,
span,
strike,
strong,
style,
sub,
summary,
sup,
table,
tbody,
td,
textarea,
tfoot,
th,
thead,
time,
title,
tr,
track,
tt,
u,
ul,
var,
video,
wbr {
  display: none;
}

It only leaves me with:

a, abbr, acronym, address, b, basefont, bdo, big, blockquote, br, caption, center, cite, code, col, colgroup, dd, del, dfn, dir, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, img, ins, kbd, li, menu, ol, p, pre, q, s, samp, small, span, strike, strong, sub, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, var {
display:none
}

Ok most of the tags it strips out are of no/very little use when it comes to css and mahara - but some that I think maybe worth keeping are:
body,
button,
form,
input,
label,
legend,
option,
select,
textarea,

The 'body' one is especially useful for setting the default font-size, font-family, color, background-colour etc.

What do others think, allow more html tags to accept css styling?

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Hi Robert,

Be careful about using w3schools as your source of canonical HTML information. See http://www.w3fools.com/ for some links to more rigorous sites.

Before tinkering with which tags are allowed to accept direct CSS styling, I'd want to see if we can find out why the folks behind HTMLPurifier blocked them in the first place. Presumably they know more about browser security than we do.

Revision history for this message
Jinelle Foley-Barnes (jinelleb) wrote :

Hi Sonn,

The bot loves this, so post me some testing instructions eg (Specific area of Mahara this is effecting)

Cheers,
Jinelle

no longer affects: mahara/1.10
Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/6853

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6853
Committed: https://git.mahara.org/mahara/mahara/commit/1f6e6d022b1ff743858fa98983753f3931049a13
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit 1f6e6d022b1ff743858fa98983753f3931049a13
Author: Son Nguyen <email address hidden>
Date: Tue Sep 2 09:04:57 2014 +1200

Add more legal color formats (Bug 1264098)

- rgba()
- hsl()
- hsla()

Change-Id: I51742b629d10a8f7b0d07478d3c2c67affcea8ac

no longer affects: mahara/16.04
no longer affects: mahara/15.10
no longer affects: mahara/15.04
tags: added: nominatedfeature
Robert Lyon (robertl-9)
Changed in mahara:
milestone: 16.04.1 → none
status: Fix Committed → Fix Released
Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/8739

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/8739
Committed: https://git.mahara.org/mahara/mahara/commit/ca09675611efac6b0f153871e8a7ee35ca3e6562
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit ca09675611efac6b0f153871e8a7ee35ca3e6562
Author: Cecilia Vela Gurovic <email address hidden>
Date: Wed Mar 28 11:19:01 2018 +1300

Bug 1759367: remove customization for color formats, not needed

Customization: Add more legal color formats such as rgba, hsl, and hsla
(Bug 1264098)
The library added support for the color formats in file
htmlpurifier/HTMLPurifier/AttrDef/CSS/Color.php

behatnotneeded

Change-Id: I60a59b25e54a6ad584518a1629450fdf37fbbaab

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "18.04_STABLE" branch: https://reviews.mahara.org/8761

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/8761
Committed: https://git.mahara.org/mahara/mahara/commit/1094e48814f0850aa3fba3bbefa9405dc7acece2
Submitter: Robert Lyon (<email address hidden>)
Branch: 18.04_STABLE

commit 1094e48814f0850aa3fba3bbefa9405dc7acece2
Author: Cecilia Vela Gurovic <email address hidden>
Date: Wed Mar 28 11:19:01 2018 +1300

Bug 1759367: remove customization for color formats, not needed

Customization: Add more legal color formats such as rgba, hsl, and hsla
(Bug 1264098)
The library added support for the color formats in file
htmlpurifier/HTMLPurifier/AttrDef/CSS/Color.php

behatnotneeded

Change-Id: I60a59b25e54a6ad584518a1629450fdf37fbbaab

Revision history for this message
Lena Stackhouse (lenastackhouse) wrote :

HTML purifier is stripping out border styles, could this be added as an exception in the future?
To make images appear in a circle crop <img style="border-radius:50%;">

Revision history for this message
Liam Lynch (284533k) wrote :

Great to see from July 2019 to the next patch (10/21) that the border radius will be resolved.

Would love to be able to play with the rest of the css3 features so we can customise/create more within the platform. Grids, flex etc.

Unfortunately HTMLPurifier, as good as it was in it's time, is ancient, neglected and only kept alive by yearly contributions from the wider community, not the creator himself. Would be nice to be able to use similar CSS layout styles we are using elsewhere/ on other platforms.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.