Currently content scripts can access window.controllers. They can't retrieve a controller - no permission to create a wrapper fro nsIController, that's fine. But they still can remove controllers and install their own controllers. The controller list isn't reset when the window's location is changed so this way a content script can create effects that will survive a location change. Same-origin policy still applies so the damage is limited but this still can be abused in several ways. First of all a content script can call controllers.removeControllerAt(0) several times - this will break keyboard scrolling and copying in the current tab. The only way to fix this is to close the tab and to open a new one. You can also misuse this e.g. to forbid copying of text by installing your own controller as the first in the list. You could also make sure a certain part of the document is selected every time supportsCommand for cmd_copy is called while still returning false - that way the buffer will always contain the text you want. You can spy on the user though this isn't very useful - you only get the commands and you can't get any information on their context unless they are called inside your domain. Finally you could trap the user inside your domain, any supportsCommand call executed outside the domain will bring you back (it seems this call is always executed when the page starts loading). Three test cases are coming. Solutions: Best solution IMHO would be to limit access to window.controllers to privileged scripts. I don't know why unprivileged scripts should be able to change the controller list. It might be reasonable to make controllers.commandDispatcher an exception, XUL pages might need it (do they?). Other solution would be to reset the controller list on every location change. I don't think this is desirable though.

So... can we block access to controllers altogether? If not, which parts of the interface do we need to block off? That is, what can we lock down without breaking remote XUL any more than it already is, and what do we actually need to lock down? Once we have that info, we should make some decisions.

(In reply to comment #0) >It might be reasonable to make controllers.commandDispatcher >an exception, XUL pages might need it (do they?). XUL elements set it but it's only a copy of document.commandDispatcher anyway.

(In reply to comment #4) >So... can we block access to controllers altogether? Sounds good to me.

Yes, given that controllers.commandDispatcher isn't really usable anyway (always null in my tests, unlike document.commandDispatcher) - I think access to the whole thing should be blocked.

Actually, it looks like window.controllers is replaceable, so we can't just block it. So we get to do it the hard way. ;)

Comment on attachment 210208 [details] [diff] [review] Proposed patch A better alternative would IMO be to give the XULControllers a classinfo object that doesn't claim the class has the flag nsIClassInfo::DOM_OBJECT, that way XPConnect wouldn't even let content code wrap the object. This way we don't need to make sure the caps prefs are in sync with the interface etc. sr- based on that, but if others think it's useful to have this contollable through prefs, then I could rethink this...

No, the no-prefs approach sounds great. I was worried about the sync thing myself. I assume chrome script won't be affected?

I don't know how to do the classinfo stuff offhand and won't have time this week to figure it out. On the other hand we should probably get this fixed for 1.0.8 and such... Can someone take a stab at it, please?

Not quite sure about this, and I also don't have a possibility to test right now. But this should be it?

This is trev's patch with a change to nsIDOMClasInfo.idl to make it run. I guess that only the nsXULControllers.cpp change is necessary for branches. I also used diff -u8 for ease of review ;-)

Comment on attachment 210242 [details] [diff] [review] Patch (tested) Stupid bugzilla ate my superreview request :-( See previous comment for details.

Right, I mixed up the paths, that change to nsIDOMClassInfo.h is necessary as well of course. Neil, you can merge the #ifdef blocks (remove #endif + #ifdev MOZ_XUL) for both changes in nsDOMClassInfo.cpp as well.

Comment on attachment 210242 [details] [diff] [review] Patch (tested) Yeah, that'd do it. sr=jst (with Wladimir's suggested changes).

Comment on attachment 210242 [details] [diff] [review] Patch (tested) r=bzbarsky. On branch we should probably just take the nsXULControllers.cpp change, yeah.

reassigning to Boris since he's done the patches for this. Are you going to land this on the trunk?

Actually, that's Neil and Trev's patch... But I'm happy to land it on the trunk.

Fixed on trunk.

Comment on attachment 210546 [details] [diff] [review] Branch-safe patch a=timr for drivers.

Comment on attachment 210546 [details] [diff] [review] Branch-safe patch a=timr for drivers

*** Committing to MOZILLA_1_7_BRANCH... new revision: 1.27.16.1; previous revision: 1.27 *** Committing content/xul/document/src/nsXULControllers.cpp on AVIARY_1_0_1_20050124_BRANCH new revision: 1.27.30.1; previous revision: 1.27 *** Committing content/xul/document/src/nsXULControllers.cpp on MOZILLA_1_8_0_BRANCH new revision: 1.28.36.1; previous revision: 1.28

This caused bug 326248. :(

Comment on attachment 210208 [details] [diff] [review] Proposed patch This is what we want for the old branches, a=dveditz

Comment on attachment 210546 [details] [diff] [review] Branch-safe patch Turned out to be not so branch-safe (bug 326248)

Checked-in the original CAPS fix ("proposed patch" attachment 210208 [details] [diff] [review]) to the old branches (aviary101/moz17/moz180) and backed out the "branch-safe patch".

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060220 Firefox/1.0.8, testcases all appear to work ok.

Can we get a patch for the 1.8 branch here, e.g., at least whatever we took on 1.8.0?

This is fixed on the 1.8 branch, by the checkin for bug 326248.