Refine the element and attribute whitelists for Thimble

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: pomax, Assigned: pomax)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 759765
(Assignee)

Updated

6 years ago
Duplicate of this bug: 765829
(Assignee)

Updated

6 years ago
Assignee: nobody → pomax
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
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.
Flags: needinfo?(mgoodwin)
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)
Attachment #743215 - Flags: review?(scott) → review+
(Assignee)

Comment 6

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

fixed (and rebased).
(Assignee)

Updated

6 years ago
Attachment #743215 - Flags: review+ → review?(scott)
(Assignee)

Comment 8

6 years ago
any movement on this?
(Assignee)

Comment 9

6 years ago
landing this, we can tighten it up after sec-review since it's not deployed in any way yet.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(mgoodwin)
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).
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.