From bug 796857 comment 10: require('api-utils/window/utils').open() creates chrome windows by default, even if passed a content URI (it just forwards the parameters to the windowWatcher). This is pretty nuts, since I've heard that the AMO reviewers deny review for addons that load untrusted content in chrome iframes. It seems like we shouldn't be making that so easy.

Should restrict to `chrome://` and `resource://` URIs

Sunny asked to me to review the PR, but I think we need to define better what "untrusted content" is in this context, and what it means "shouldn't be making that so easy". If the problem is about "content UI" then also "resource:" and "data:" shouldn't be allowed, but just "chrome:"; we also had problem with unexpected behaviors passing directly "content URI" (HTML document, via "data:") to `open` method – events not fired etc. Dave, could you clarify a bit more the specs of this bug?

We shouldn't be allowing external content (i.e. something that could be remotely compromised) to display in a top-level window. So http, https and ftp are definitely bad. resource and data could potentially include things generated from remote content, but I suspect it's rare and mostly what I think we want to avoid here is a footgun, an easy way to accidentally do something bad. So I think allowing resource and data is fine for this.

I see. My only concern here is that loading a HTML page in a window opened by `window/utils` open method, brings to some unexpected behaviors, that are leading to problems, as we had during our unit tests. It seems works fine only when the browser's URI is specified, or one with XUL content (at least on OS X).

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5) > I see. My only concern here is that loading a HTML page in a window opened > by `window/utils` open method, brings to some unexpected behaviors, that are > leading to problems, as we had during our unit tests. It seems works fine > only when the browser's URI is specified, or one with XUL content (at least > on OS X). Well that's not something we can identify immediately from the URI anyway, it could point to a XUL or HTML file regardless of the protocol. I'd be interested to know what behaviours you're talking about but for a separate bug I suspect.

(In reply to Dave Townsend (:Mossop) from comment #6) > Well that's not something we can identify immediately from the URI anyway, > it could point to a XUL or HTML file regardless of the protocol. I'd be > interested to know what behaviours you're talking about but for a separate > bug I suspect. We (me and Erik) had some issues with `open` method during the writing of some unit test. Some events are not fired, as I mentioned, plus the window looks weird (totally transparent background, for example). As consequences some tests were stuck, because waiting for events that never happens. We ended up to open the window, load the browser's URI, and then load the URI we wanted to as content of the browser. That works, and I also suspect that that's what most of the people expects from a `open` method. They really don't want to deal with all this stuff, they want to just open a HTML page from data folder maybe, that is displayed in a dialog window without toolbar maybe or address bar.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7) > (In reply to Dave Townsend (:Mossop) from comment #6) > > > Well that's not something we can identify immediately from the URI anyway, > > it could point to a XUL or HTML file regardless of the protocol. I'd be > > interested to know what behaviours you're talking about but for a separate > > bug I suspect. > > We (me and Erik) had some issues with `open` method during the writing of > some unit test. Some events are not fired, as I mentioned, plus the window > looks weird (totally transparent background, for example). As consequences > some tests were stuck, because waiting for events that never happens. We > ended up to open the window, load the browser's URI, and then load the URI > we wanted to as content of the browser. That works, and I also suspect that > that's what most of the people expects from a `open` method. They really > don't want to deal with all this stuff, they want to just open a HTML page > from data folder maybe, that is displayed in a dialog window without toolbar > maybe or address bar. I like this idea Matteo, I think it should be in a higher level dialog window module, separate from our browser window module though. It could be made to work on Fennec with tabs too. This patch is merely meant to be protection for our lowest level api for opening windows, something that we don't want to encourage people to use.

Comment on attachment 743863 [details] Link to pull request 983 We should get Irakli's approval.

What I'm tried to say is: what's the purpose of this method? We really need it? What's are the use case for us or add-on devs to use that? More in details, it seems that we basically create a 'security hole', where we let the add-on dev invoke `windowWatcher.openWindow` directly without requires the "chrome" privileges; because everything happens here it's just forward the parameters to the `windowWatcher`. So I was wondering what are exactly the use cases that let decide us to implement this such "proxy method", because to me I can see only two use cases: 1. Add-on devs wants to open a content UI (therefore they need browser URI upfront, and then load the content UI) 2. Add-on devs wants to open a XUL document (therefore they need chrome privilege, because remote XUL is disabled, or passing a chrome registered URI) This method is not suitable for content URI, as I described, we can cover it better with another method to do so. The only reason I can see this method useful for add-on devs (and us), is if they want to open a XUL document; and that's could be interesting only if it's registered (chrome://) otherwise they need in any case chrome privileges, and they could use `windowWatcher` directly – as said it's just a proxy method at the end. And that seems brings more issues than it solves: it just forward the parameters to the windowWatcher. It's useful only if you're passing a XUL document, or if you load the browser URI and then load a content URI. We can cover the second case with another functionality, as I described above. For the first case, remote XUL is disabled so you need to pass a registered chrome URI (or using chrome privileges anyway). Because bug 559306 is fixed, I believe that leaving the capability to open dialog window for registered `chrome://` URL without requires `chrome` module could be a good point. Sure you can't determined the document's type just by the URI schema, but it's already a step forward to me. So my proposal here is: 1. Limit the `open` method of `window/utils` to only "chrome://" URL; that would be much secure and will simplify the reviews too. If a developer wants so badly open a non chrome content in a chrome window can do so using chrome privileges, so the reviewer have an additional indication (require('chrome')) during the review process; however a content URI should be opened by a browser's dialog. 2. Add a way to open content URI – so via a browser's dialog. I already have a couple of discussion in IRC about why `open` method doesn't really works with HTML document in data folder, this scenario is not uncommon: require('sdk/window/utils').open(data.url('foo.html'), options); But it doesn't works as they expects – because the reasons said before, it messed up with events and look & feel, etc – and they're lost because they don't know how to proceed. I totally agreed that we shouldn't encourage people to open windows, especially chrome window like that, that's why I think is good limit this method to "chrome://" schema instead of leave it available to both "data:" and "resource:" specially. Sure, as Mossop said we can't determined if a document is a XUL document or HTML document just from the URI, but limit that method to the registered chrome documents it's already better than what we have now.

BTW, block the remote content is a good security improvements in any case, that is worthy to be landed, I just tried to go further.

Comment on attachment 743863 [details] Link to pull request 983 Looks good to me. Although if we don't actually use this function I would just go ahead and remove it all together. I believe I added it back in a time as we thought it was a best way to do addon/window which turned out to be false. I suspect we just use it in tests and even there I think we could just use regular open from existing browser window.

Ahh we didn't land this yet? Should I squash this branch? or may I leave it?

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #13) > Ahh we didn't land this yet? > > Should I squash this branch? or may I leave it? Just land it.

Thanks :)

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/2635e340fc97ee27eb9299ffb8d6b3b34bcf05e1 Merge pull request #983 from darkowlzz/bug840314 Bug 840314 - Don't open untrusted content in top-level chrome windows r=@erikvold

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/2ab29dfb6a58705e56b7dba8a2bf80c1d376ea71 Revert "Merge pull request #983 from darkowlzz/bug840314" This reverts commit 2635e340fc97ee27eb9299ffb8d6b3b34bcf05e1, reversing changes made to b3e20357526463d2d572a42d4609121a359d62f8.

It looks like some other tests that use this api need to be updated, they appear to use data uris though so I would think they would have passed.. need to investigate this..

Commits pushed to firefox25 at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/2635e340fc97ee27eb9299ffb8d6b3b34bcf05e1 Merge pull request #983 from darkowlzz/bug840314 https://github.com/mozilla/addon-sdk/commit/2ab29dfb6a58705e56b7dba8a2bf80c1d376ea71 Revert "Merge pull request #983 from darkowlzz/bug840314"

(In reply to [github robot] from comment #19) > Commits pushed to firefox25 at https://github.com/mozilla/addon-sdk > > https://github.com/mozilla/addon-sdk/commit/ > 2635e340fc97ee27eb9299ffb8d6b3b34bcf05e1 > Merge pull request #983 from darkowlzz/bug840314 > > https://github.com/mozilla/addon-sdk/commit/ > 2ab29dfb6a58705e56b7dba8a2bf80c1d376ea71 > Revert "Merge pull request #983 from darkowlzz/bug840314" This was reverted from master, so hopefully it's not in firefox25

Yep, this uplift uplifted both the original landing and the revert, so we should be fine.

(In reply to Wes Kocher (:KWierso) from comment #21) > Yep, this uplift uplifted both the original landing and the revert, so we > should be fine. k cool, thanks for confirming.

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/5add1141b87bc103b6b38fa3add41f6fba14b7c0 Revert "Revert "Merge pull request #983 from darkowlzz/bug840314"" This reverts commit 2ab29dfb6a58705e56b7dba8a2bf80c1d376ea71. https://github.com/mozilla/addon-sdk/commit/d6911a134f035616b6854b32f8d9cb9cdc77e65b Bug 840314 - test the uri scheme after the default uri is applied

I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Erik?

This is resolved. https://github.com/mozilla/addon-sdk/commit/5add1141b87bc103b6b38fa3add41f6fba14b7c0 https://github.com/mozilla/addon-sdk/commit/d6911a134f035616b6854b32f8d9cb9cdc77e65b

Changing ASSIGNED to myself to make is easy to refer later.