52 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
Update the mozillathimbelivepreview.net previewloader with the content of https://raw.github.com/mozilla/friendlycode/iframed/templates/previewloader.html
39 bytes, text/plain
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.
I want to suggest that this is post-June 15, and doesn't need to block what we'll put out for that release.
Created attachment 772747 [details] [review] https://github.com/mozilla/thimble.webmaker.org/pull/164 changes in thimble.webmaker.org
Created attachment 772748 [details] [review] https://github.com/Pomax/friendlycode/pull/1 friendlycode changes
Component: General → Thimble
Summary: [Meta] Allow <script> in user generated content → Allow <script> in user generated content in Thimble
morphing to thimble bug for now.
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.
Attachment #772748 - Flags: review?(david.humphrey) → review+
Attachment #772747 - Flags: review?(david.humphrey) → review+
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
Oops, fixed the permissions issue. Sorry!
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?
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 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.
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.
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.
Created attachment 773434 [details] [review] 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.
Created attachment 773435 [details] 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 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.
Attachment #773434 - Flags: review?(david.humphrey) → review?(scott)
split the pull request up into iframe-script only, and metadata sanitization only (https://bugzilla.mozilla.org/show_bug.cgi?id=892066).
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.
Attachment #773435 - Flags: review?(jon) → review?
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.
Attachment #773434 - Flags: review?(scott) → review-
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.
Attachment #773434 - Flags: review- → review?(scott)
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.
Attachment #773434 - Flags: review?(scott) → review+
[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 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.
Attachment #773434 - Flags: review?(jon) → review+
switch to https was made today.
are we good to land this?
We are indeed.
Flags: sec-review?(mgoodwin) → sec-review+
the eagle has landed.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain text/plain → text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.