863737 - Allow <script> in user generated content in Thimble

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, enhancement)

Description

[David Humphrey (:humph)]

We're working on moving publishing to user subdomains, and when that's done, we're going to be in a good place to figure out a strategy for allowing <script> in user generated content created with our tools.

Doing this properly will involve a bunch of things, including decisions about how we do remixing, what we allow (everything, whitelisted things, using CSP, ...), what our education strategy is around this (i.e., how to help our users understand the implications of including/remixing scripts).

Having the full web stack be part of what a webmaker can author and remix is going to amazing, and will open the door for a lot of new content and projects.

Please file dependent bugs as you think of them.  Because we have so much to do with user domains first, I'm not going to worry about full specifying this yet.

I'm provisionally assigning this to Jon as owner, and expecting that he, Pomax, myself and others will all help get this done.

Comment 1

[David Humphrey (:humph)]

I want to suggest that this is post-June 15, and doesn't need to block what we'll put out for that release.

Comment 2

[Pomax]

agreed.

Comment 3

[Pomax]

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

changes in thimble.webmaker.org

Comment 4

[Pomax]

Attachment: https://github.com/Pomax/friendlycode/pull/1

friendlycode changes

Comment 5

[Pomax]

morphing to thimble bug for now.

Comment 6

[David Humphrey (:humph)]

Comment on attachment 772748 [details] [review]
https://github.com/Pomax/friendlycode/pull/1

A few style and other nits, some questions, but mostly this looks good.  r+ with that stuff addressed.

Comment 7

[Pomax]

I've spun up some heroku instances for testing:

Thimble: http://calm-headland-1764.herokuapp.com
login: http://obscure-fjord-5429.herokuapp.com
webmaker.org: http://quiet-fortress-6677.herokuapp.com

The first problem is that our previewloader is apparently set to not be accessible anymore, so AWS throws a nice 403 when you try to access http://mozillathimblelivepreview.net/loader.html

Comment 8

[Jon Buckley [:jbuck]]

Oops, fixed the permissions issue. Sorry!

Comment 9

[Pomax]

With our iframe isolation, we should be able to publish an unbleached page (since we have iframe isolation both in thimble now, and when the page is served in our embed shell). Do we want to turn off bleaching and request a security review to see how this holds up, or are there more steps we should take before we get this sec.reviewed?

Comment 10

[Mark Goodwin [:mgoodwin]]

We should be in a position to review here. I'd like to have a (good) play in a non-production environment. Can I use the heroku instances above? What are your timescales for review?

Comment 11

[Jon Buckley [:jbuck]]

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

Looks good, but what's allowJS used for? Doesn't seem to be used for anything in friendlycode.

Comment 12

[Pomax]

if we switch over to unsanitized data, our schema is now too full: we currently store rawData + sanitizedData, but after the switch we technically don't have to store the sanitizedData column anymore. This may need a cleanup ticket after we're happy that this solution is good enough to merge into master.

Comment 13

[Pomax]

allowJS is picked up by Slowparse, and detemines whether or not to throw errors if <script> is found in the token stream. I've updated the code to no-bleach, allow-scripts, so I'll be filing need attachments for rereview.

Comment 14

[Pomax]

Attachment: https://github.com/Pomax/thimble.webmaker.org/pull/1

replaces the moz/thimble pull request - updated to master, with bleach taken out of the picture. Heroku instance has been updated to this.

Comment 15

[Pomax]

Attachment: Update the mozillathimbelivepreview.net previewloader with the content of https://raw.github.com/mozilla/friendlycode/iframed/templates/previewloader.html

sneakily using the attachment system to get jbuck to update the previewloader file

Comment 16

[Pomax]

Comment on attachment 773434 [details] [review]
https://github.com/Pomax/thimble.webmaker.org/pull/1

moved review to scott instead, since he'll have opinions on the saveCode changes.

Comment 17

[Pomax]

split the pull request up into iframe-script only, and metadata sanitization only (https://bugzilla.mozilla.org/show_bug.cgi?id=892066).

Comment 18

[Pomax]

Comment on attachment 773435 [details]
Update the mozillathimbelivepreview.net previewloader with the content of https://raw.github.com/mozilla/friendlycode/iframed/templates/previewloader.html

updated.

Comment 19

[Scott [:thecount] Downe]

Comment on attachment 773434 [details] [review]
https://github.com/Pomax/thimble.webmaker.org/pull/1

Just one small nit in the pull request. Not sure if it was intended or not.

Comment 20

[Pomax]

Comment on attachment 773434 [details] [review]
https://github.com/Pomax/thimble.webmaker.org/pull/1

removed console.log, filed https://bugzilla.mozilla.org/show_bug.cgi?id=892999 for it instead.

Comment 21

[Scott [:thecount] Downe]

Comment on attachment 773434 [details] [review]
https://github.com/Pomax/thimble.webmaker.org/pull/1

Not sure if we want to leave this on Jon still.

Your call, but R+ from me.

Comment 22

[Pomax]

[Sec.Review request]

Running down the sec.review list:

1. Who is/are the point of contact(s) for this review?

I will be the that person for this bug.

2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):

we're getting Thimble updated so that users can safely add script in their page without it causes session sniffing and the like, so in this bug several code paths have been changed to effect user code always loading in an iframe from a different domain, to put up a hard cross-origin boundary.

There are two project changes involved in this sec.review request.

The first is in the "friendlycode" code editor component that is used in Thimble. In the current version, a user's html+css is "previewed" by building an on-page iframe and setting its content to what the user has in the code editor. This iframe is same-domain, which means that session information is technically accesible to the code running in the iframe. This bug changes that so that friendlycode instead loads a "preview loader" from a different domain in the same iframe (see https://github.com/mozilla/friendlycode/blob/iframed/js/fc/ui/live-preview.js#L9..L25), setting up an explicit cross-origin boundary. This preview loader, https://github.com/mozilla/friendlycode/blob/iframed/templates/previewloader.html, sets up its own iframe into which it can load content, and to preview a user's page, the loader is passed the code through a postMessage, after which it is responsible for setting its own iframe content to effect a preview. We are running this previewloader.html, on its own, on mozillathimblelivepreview.net

The second change is to the Thimble project. As content is now explicitly cross-origin sandboxed, bleaching for user data has been removed (https://github.com/Pomax/thimble.webmaker.org/pull/1/files#L1L8) and the friendlycode component is now invoked in a way that tells it that scripts are allowed, and that it should use the previewloader from mozillathimblelivepreview.net: https://github.com/Pomax/thimble.webmaker.org/pull/1/files#L7R208

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description.

no additional links.

4. Does this request block another bug? If so, please indicate the bug number

No, this is an independent enhancement.

5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

Ideally this can be looked at as soon as possible so we can switch over to supporting full webmaking on webmaker.org rather than the current html+css only solution. However, this code does not patch any critical vulnerabilities, nor does it have any stakeholders explicitly waiting for it to land, so it probably has a normal priority.

6. To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?

no.

7a. Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?

no.

7b. Are there any portions of the project that interact with 3rd party services?

Thimble.webmaker.org relies on webmaker.org SSO, which is based on Persona. The major concern is that this changeover does not make it possible to sniff persona session information.

7c. Will your application/service collect user data? If so, please describe

thimble itself does not, but you cannot publish thimble pages without being logged in to webmaker.org, which uses persona. As such, a logged in user has an active session that contains their email address, tied to their webmaker username. No personal information is collected or stored beyond these identifiers.

8. If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):

nothing comes to mind at the moment.

9. Desired Date of review (if known) and whom to invite.

No real preference. The sooner the better, within reason? In terms of people invested in the security review, probably :mgoodwin, :jbuck, and :humph

Comment 23

[Jon Buckley [:jbuck]]

Comment on attachment 773434 [details] [review]
https://github.com/Pomax/thimble.webmaker.org/pull/1

r+ with the note that we need https version of mtlp.net running before we can test this on staging.

Comment 24

[Pomax]

switch to https was made today.

Comment 26

[Pomax]

are we good to land this?

Comment 27

[Mark Goodwin [:mgoodwin]]

We are indeed.

Comment 28

[Pomax]

the eagle has landed.