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.
Created attachment 743215 [details] [review] https://github.com/mozilla/thimble.webmaker.org/pull/37
Attachment #743215 - Flags: review?(scott)
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)
that's not a nit, that's a massive oversight and that should have been an r- fixed (and rebased).
Attachment #743215 - Flags: review+ → review?(scott)
Comment on attachment 743215 [details] [review] https://github.com/mozilla/thimble.webmaker.org/pull/37 Looks good.
any movement on this?
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.
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.