Closed Bug 1110546 Opened 7 years ago Closed 7 years ago

Passing null to Sandbox constructor should create a null principal

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bholley, Assigned: bholley)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

The current state of the art for getting a null principal is:

Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);

Which is sucky, and encourages people to do things like passing "about:blank" to the sandbox constructor.

The C++ API to creating sandboxes actually does the right thing when you pass it null. We just need to make the Cu API not barf.
Keywords: dev-doc-needed
Attachment #8535317 - Flags: superreview?(mrbkap)
Attachment #8535317 - Flags: review?(gkrizsanits)
Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

Review of attachment 8535317 [details] [diff] [review]:
-----------------------------------------------------------------

Not a big fun of using js Null as an argument in general, but the current alternative is indeed painfully complicated for such a simple thing... Ideally the API would only take the options object and then we could just default the nullprincipal, but we are too late for that change. Another option would be to change the string parsing function and let it parse "null-principal" and "system-principal" (or just "null"/"system") strings, but since system is always easy in practice (we just pass in |this|) I'm fine with this. (little bit still leaning toward the string based approach because I believe passing Null in JS is a lot less conventional than in C++ but I leave this up to you to decide)
Attachment #8535317 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

Irakli, any thoughts on this as an API?
Attachment #8535317 - Flags: feedback?(rFobic)
Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

Review of attachment 8535317 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable to me & I don't expect that to break anything on SDK side. Although I have to say I would much rather have API like `Sandbox({ wantComponents: false, principal: window })` that way if `principal` omitted it can default to null principal (I presume number of passed args can be used to support legacy API as well).

Another option to make gabro happy would be to built upon expanded principal and treat `Sandbox([])` as null principal.

Either way all of the options seem good enough to me.
Attachment #8535317 - Flags: feedback?(rFobic) → feedback+
Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

sr timeout. Pushing to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36f9d3196503
Attachment #8535317 - Flags: superreview?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/36f9d3196503
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
I've added this, and cleaned up the stuff on specifying principals while I'm about it: https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#principal.

Gabor, would you mind taking a look?
Flags: needinfo?(gkrizsanits)
(In reply to Will Bamberg [:wbamberg] from comment #8)
> I've added this, and cleaned up the stuff on specifying principals while I'm
> about it:
> https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#principal.
> 
> Gabor, would you mind taking a look?

This looks perfect, thanks Will.
Flags: needinfo?(gkrizsanits)
You need to log in before you can comment on or make changes to this bug.