56 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
friendlycode does no iframe sandboxing (even if it did, a disheartening number of browsers won't do anything with that), so if we allow script publication for authenticated users, user X can create a page with cookie-stealing scripts, and publish that legally; if an authenticated user Y then hits "remix", I'm pretty sure that would lead to a compromised session. we could set up a safety mechanism so that if someone remixes a page with script, they first get a warning saying "this page has scripts, you better know what you're doing", but people click throught that anyway; Another option is to load a fully sanitized version of a page on remix (i.e. if a not-owning user loads it in the editor) and then for each <script> element ask "do you know what this does?" and put it back as a normal script element into the page source if they indicate they know what they're doing. Then again, for a page with 10 or 20 scripts, that gets quite tedious. Short of a sandboxed iframe, I'm not sure what the safe solution is, here. Is there a way to create an iframe with a template from another domain, and then modify its content so that scripts inside that iframe run on a different domain, and can't steal editor session cookies?
Created attachment 768317 [details] [review] https://github.com/mozilla/thimble.webmaker.org/pull/164 This is a stub pull request to enable scripts in Thimble, by making the preview pane an actually loader.html file that lives on a different domain, updating the preview using postmessages. This is not so much a final code suggestion, but a proposal on how we can do this, locking down the preview in a way that allows scripts.
Comment on attachment 768317 [details] [review] https://github.com/mozilla/thimble.webmaker.org/pull/164 diff for friendlycode: https://github.com/Pomax/friendlycode/pull/1
This looks pretty sane as-is. We could move forward by enabling the preview loader to live within Friendly Code for starters, then look at deploying it somewhere else on the web.
I made some comments in both PRs, and I agree with Jon that this looks like a solid plan. It might be useful to show it to mgoodwin too? I can't really evaluate it from a security POV well enough to say "yes", and that's really what this patch is about.
I'm marking this as a duplicate, since at this point it pretty much is.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 863737
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.