866704 - Refine the element and attribute whitelists for Thimble

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Pomax]

The element and element attribute whitelists we lifted from webpagemaker for the rewrite of Thimble were never optimized. We have an opportunity to do that now, so let's refine the list of permitted elements, and permitted attributes per element.

Comment 3

[Pomax]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/37

Comment 4

[Pomax]

CCing mgoodwin; pretty sure none of these are more permissive than what we already allow (some cleared elements have src attributes but are spec-wise restricted to only allow real URLs, rather than things like javascript:..., such as the source element) but an extra pair of eyes is always a good idea.

Comment 5

[Scott [:thecount] Downe]

Comment on attachment 743215 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/37

One nit.

I believe you can remove the bleachData in middleware.js now, correct? No where else uses it?

Anyway, R+ with that or a confirmation that we still need it. Things still work and passes tests.

(I was going to do this as a pull request comment, but github seems to be down)

Comment 6

[Pomax]

that's not a nit, that's a massive oversight and that should have been an r-

fixed (and rebased).

Comment 7

[Scott [:thecount] Downe]

Comment on attachment 743215 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/37

Looks good.

Comment 8

[Pomax]

any movement on this?

Comment 9

[Pomax]

landing this, we can tighten it up after sec-review since it's not deployed in any way yet.

Comment 10

[Mark Goodwin [:mgoodwin]]

I see this is closed but I've spent some time looking at it so I'll drop my thoughts in here:

The whitelist here looks good provided a) we're using the latest version of bleach in the REST service and b) we don't want to protect users on older (pre- IE8) versions of Internet Explorer (the issue here being CSS expressions).

I would still be keen to see per-user subdomains of a webmaker content domain as a primary defense over sanitising user content; if we can do this in a robust fashion we can maybe even relax this list a bit.

Comment 11

[David Humphrey (:humph)]

Thanks Mark.  One note, we are indeed doing per-user subdomains for all published Thimble content with all Webmaker tools in the upcoming release (it's been implemented already by Jon, Pomax, and others).