Closed
Bug 713747
Opened 12 years ago
Closed 12 years ago
Rip out non-UniversalXPConnect privilege manager functionality
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: dev-doc-needed)
Attachments
(5 files, 1 obsolete file)
31.35 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
19.86 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.74 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We're aiming to get rid of the privilege manager entirely, so a good first step would be to kill things like UniversalBrowserRead, UniversalFileWrite, etc. This has a few advantages: 1 - I need tests to stop using them as a dependency for bug 622301. 2 - It simplifies the process later when we remove the API entirely. 3 - It simplifies the SpecialPowers conversion, since post-this-bug the only use of enablePrivilege should be |enablePrivilege("UniversalXPConnect")|, rather than one of the many variants we have now. 4 - It gives intranets etc a mild wakeup call that the API is going away soon. They'll have an easy temporary fix ("switch to UniversalXPConnect"), but hopefully recognize that they need to change things. Patches coming right up.
Assignee | ||
Comment 1•12 years ago
|
||
Pushed some patches to try for this: https://tbpl.mozilla.org/?tree=Try&rev=b8b1e8e4892e Once they go green I'll flag for review.
Comment 2•12 years ago
|
||
Can we land a warning on use of these into beta or aurora to give people more heads-up?
Keywords: dev-doc-needed
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #2) > Can we land a warning on use of these into beta or aurora to give people > more heads-up? I thought you landed a warning for all uses of enablePrivilege?
Comment 4•12 years ago
|
||
Er, so I did. Nevermind, then!
Assignee | ||
Comment 5•12 years ago
|
||
There were some test failures, which I've rectified hopefully. Attaching patches and flagging bz for review.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #584698 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•12 years ago
|
||
The conversion of checks for UniversalBrowserWrite to UniversalXPConnect caused these test fail, because they acquire UniversalXPConnect and actually doesn't want the results. In particular, they pass the security check in nsWindowWatcher::CalculateChromeFlags, which causes them to fail. The first test doesn't actually need UniversalXPConnect, so we can remove it for free. The second one needs it, so we add some SpecialPowers goop to make it possible. It's not ideal, but it can go away when the wrapper in bug 702353 lands.
Attachment #584699 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•12 years ago
|
||
I don't think we need this stuff at all anymore, since the security policy stuff is now burned into c++ in xpconnect/wrappers. However, I'm too afraid to do the removal myself.
Attachment #584700 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #584701 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #584702 -
Flags: review?(bzbarsky)
Comment 11•12 years ago
|
||
Comment on attachment 584700 [details] [diff] [review] part 3 - Remove UniversalFoo from all.js. v1 r=me
Attachment #584700 -
Flags: review?(bzbarsky) → review+
Comment 12•12 years ago
|
||
Comment on attachment 584698 [details] [diff] [review] part 1 - Use UniversalXPConnect and UniversalXPConnect only in test coverage. v1 r=me
Attachment #584698 -
Flags: review?(bzbarsky) → review+
Comment 13•12 years ago
|
||
Comment on attachment 584699 [details] [diff] [review] part 2 - Fix funky tests. v1 r=me Make sure there's a followup on removing the new goop?
Attachment #584699 -
Flags: review?(bzbarsky) → review+
Comment 14•12 years ago
|
||
Comment on attachment 584702 [details] [diff] [review] part 5 - Remove UniversalFoo from caps (and thus, the tree). v1 Shouldn't CheckSameOriginDOMProp still check for UniversalXPConnect? Or is that handled elsewhere?
Comment 15•12 years ago
|
||
Comment on attachment 584701 [details] [diff] [review] part 4 - Remove usage of UniversalFoo in gecko. v1 Why not just nix the useless arg from IsCallerTrustedForCapability (and perhaps rename it to CallerHasUniversalXPConnectPrivs or something)? r=me with something like that.
Attachment #584701 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #14) > Comment on attachment 584702 [details] [diff] [review] > part 5 - Remove UniversalFoo from caps (and thus, the tree). v1 > > Shouldn't CheckSameOriginDOMProp still check for UniversalXPConnect? Or is > that handled elsewhere? This comment suggests that we check it later: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1085 And I believe it's correct. In the only caller of CheckSameOriginDOMProp, we subsequently call CheckXPCPermissions: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#873 And we must, because within caps UniversalXPConnect doesn't imply the rest. So the current call to IsCapabilityEnabled would fail for just UniversalXPConnect.
Comment 17•12 years ago
|
||
Comment on attachment 584702 [details] [diff] [review] part 5 - Remove UniversalFoo from caps (and thus, the tree). v1 r=me
Attachment #584702 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #13) > Make sure there's a followup on removing the new goop? Filed bug 714012.
Assignee | ||
Comment 19•12 years ago
|
||
Addressed bz's comments, and gave this one last push to try. Once it goes green, this can land: https://tbpl.mozilla.org/?tree=Try&rev=281a08aa5c58
Assignee | ||
Comment 20•12 years ago
|
||
Arg, looks like I missed one other modal dialog test. Given that bz's on PTO, I'm flagging jdm for review on the changes to test_bug437361.html. bz's review carries on everything else. I've pushed this to try for another run of the mochitest-2 suite only: https://tbpl.mozilla.org/?tree=Try&rev=741e3c3a5d35
Attachment #584699 -
Attachment is obsolete: true
Attachment #584909 -
Flags: review?(josh)
Assignee | ||
Updated•12 years ago
|
Attachment #584909 -
Attachment description: Bug 713747 - Fix funky tests. v2 r=bz → part 2 - Fix funky tests. v2 r=bz
Assignee | ||
Comment 21•12 years ago
|
||
Also, given that this was the patch that introduced mozprefs.js and I'd never heard of it before, I'd wager a guess it was the only mochitest that used it. Given that we're all for SpecialPowers now, it should probably be removed from the tree, with any unique functionality being merged into SpecialPowers.
Comment 22•12 years ago
|
||
Comment on attachment 584909 [details] [diff] [review] part 2 - Fix funky tests. v2 r=bz r=me, but do you still need to include mozprefs.js?
Attachment #584909 -
Flags: review?(josh) → review+
Assignee | ||
Comment 23•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #22) > Comment on attachment 584909 [details] [diff] [review] > part 2 - Fix funky tests. v2 r=bz > > r=me, but do you still need to include mozprefs.js? Oh, good catch. I don't! And it looks like there's one other user in the tree.
Assignee | ||
Comment 24•12 years ago
|
||
Landed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/c85e6c648d8f http://hg.mozilla.org/integration/mozilla-inbound/rev/c991b7a9810a http://hg.mozilla.org/integration/mozilla-inbound/rev/3efc3a946a7f http://hg.mozilla.org/integration/mozilla-inbound/rev/3aa338192b8a http://hg.mozilla.org/integration/mozilla-inbound/rev/e1b00c29bf03
Target Milestone: --- → mozilla12
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c85e6c648d8f https://hg.mozilla.org/mozilla-central/rev/c991b7a9810a https://hg.mozilla.org/mozilla-central/rev/3efc3a946a7f https://hg.mozilla.org/mozilla-central/rev/3aa338192b8a https://hg.mozilla.org/mozilla-central/rev/e1b00c29bf03
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
Comment on attachment 584909 [details] [diff] [review] part 2 - Fix funky tests. v2 r=bz Review of attachment 584909 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/bugs/test_bug504862.html @@ -24,4 @@ > window.addEventListener("message", onMsgRcv, false); > > - var subsequentDlg = "data:text/html,<html><body onload='opener.is(window.dialogArguments, \'my args\', \'subsequent dialog document did not get the right arguments.\'); close();'>"; > - Just to confirm, was the removal (= not replacement) of this line intended? (As in, was it not part of the test? Or is that part obsolete now?)
Comment 27•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #23) > it looks like there's one other user in the tree. I filed bug 714632.
Assignee | ||
Comment 28•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #26) > Just to confirm, was the removal (= not replacement) of this line intended? > (As in, was it not part of the test? Or is that part obsolete now?) Yeah, it was a dead variable.
Updated•12 years ago
|
Flags: in-testsuite+
Version: unspecified → Trunk
Comment 29•12 years ago
|
||
For those of us who relied on UniversalBrowserRead and UniversalBrowserWrite, what is the alternative? I need to load a page from another domain in an iFrame, populate the form, and submit the form, using Javascript. The purpose is to automate an online shipping tool with no API.
Comment 30•12 years ago
|
||
Short-term, use UniversalXPConnect. Longer-term, enablePrivilege is going away. We've been saying so for several years now. Code that needs extra privileges should move into an extension....
Comment 31•12 years ago
|
||
Thanks. UniversalXPConnect was not a drop-in replacement and required some rewriting. If anyone has a tutorial that will allow me to convert my current code to an extension, I would love to see it.
You need to log in
before you can comment on or make changes to this bug.
Description
•