Right now if I need to know the subject principal of my called in a webidl function defined in C++, I need to call `nsContentUtils::SubjectPrincipal()` somewhere in order to get it. This makes this function unsafe to call from C++, because SubjectPrincipal() MOZ_CRASHes if called with no JS on the stack. I have been bitten by this twice, where C++ code is consuming an API intended for JS, and calls the webidl endpoint with no JS on the stack. It would be nice if we had some way to make the fact that the endpoint cares about the subject principal more explicit through something like an explicit parameter on the C++ side. This could take the form of a [explicit_principal] attribute or similar, which would pass the Subject principal as the second last argument, immediately before the ErrorResult argument.

Making it explicit and having the binding layer pass it sounds great to me.

bz, how difficult would something like this be to implement, and is it something which we would want? The idea is that the attribute would cause the generated function calls in the binding code to also pass the subject principal explicitly.

My guess is that the binding work is very easy, and the hard part is adding the attribute and updating the signatures for all the consumers that need it.

The binding bits are very simple. Whether we want it, I'm torn. The main pro is making the subject principal explicit and maybe allowing decoupling it from the current compartment. The main con is decoupling it from the current compartment and whether that violates someone's assumptions... I think on balance this is probably worth doing.

(In reply to Boris Zbarsky [:bz] from comment #4) > The binding bits are very simple. > > Whether we want it, I'm torn. The main pro is making the subject principal > explicit and maybe allowing decoupling it from the current compartment. The > main con is decoupling it from the current compartment and whether that > violates someone's assumptions... Well, the caller will still need to explicitly pass it everywhere that it is used. So I'm not really sure what the threat model is. It's not like it will persist across script execution or anything.

Sure. I'm not saying I have a coherent worry here, just a general vague one and one specific implementation concern: we'll need to change which principal we pass on codepaths where we right now change the compartment to affect the principal. This shouldn't be hard to do; we just have to not blindly pass aSubjectPrincipal when there was a current compartment modification in our stackframe.

Baku said he'll help with this!

> This could take the form of a [explicit_principal] attribute or similar, > which would pass the Subject principal as the second last argument, > immediately before the ErrorResult argument. nsIPrincipal is main-thread only. We should pass a nullptr if a [ExplicitPrincipal] method is called in workers.

(In reply to Andrea Marchesini [:baku] from comment #8) > nsIPrincipal is main-thread only. We should pass a nullptr if a > [ExplicitPrincipal] method is called in workers. Hmm, I'm not quite sure about that. Someone could easily start to interpret nullptr as if C++ had called the method with nullptr. Are there other options here? Something like Maybe<nsIPrincipal*> , where not-initialized value means the callee must do something sane? Or something else ?

Comment on attachment 8791616 [details] [diff] [review] part 4 - location pre explicitPrincipal Sorry, I'm not quite happy with this approach. This makes it even more harder to fix the issue that we may throw Gecko-internal errors to the web. I would hope either: some code uses only error types defined in webidl and dom and in other specs and then passing ErrorResult everywhere is fine, or use nsresult for Gecko internal stuff and ErrorResult for the case when we're about to pass the value to bindings layer. (and we should then just fix layer closest to bindings to not use gecko-only errors, and not fix the issue everywhere.) If you disagree, please explain why and ask review again. I do think we should either start doing some automatic conversion from gecko errors to web exposed errors in ErrorResult, or MOZ_ASSERT in ErrorResult that only web exposed error types are used with it. Probably both, so that even if debug builds wouldn't catch all the cases using MOZ_ASSERT, we wouldn't accidentally expose anything Gecko internal error values to the web in opt builds but rather some (possibly wrong) web errors.

There are some options, yes. Offhand: 1) Pass a Maybe<nsIPrincipal*>. This requires an NS_IsMainThread check. Or maybe we can get away with a branch on the principal value (that is, in the binding code convert null to empty Maybe). 2) Forbid this annotation on APIs exposed in workers. Not clear yet whether this would restrict its use in practice or not... We don't actually have _that_ maybe SubjectPrincipal() calls in our code, and none are in worker-exposed stuff at first glance. We have more IsCallerChrome() callers, but I think they're all mainthread-only stuff (I mean, the function itself is; I mean the calling code in general). ThreadsafeIsCallerChrome could be a problem, if we want to replace it with this API. But that wouldn't work anyway, because on workers the principal is not what represents chromeness. We may need a separate annotation for "tell me if my caller is chrome" or something.

Comment on attachment 8792550 [details] [diff] [review] part 4 - location pre explicitPrincipal I don't understand why *Internal stuff starts to use ErrorResult. Am I misunderstanding something here?

Comment on attachment 8793643 [details] [diff] [review] part 1 - WebIDL [ExplicitPrincipal] Review of attachment 8793643 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +7424,5 @@ > idlNode.identifier.name)) > else: > cgThings.append(CGCallGenerator( > self.isFallible(), > + self.explicitPrincipal(), Can't this use idlNode.getExtendedAttribute('ExplicitPrincipal')? @@ +13831,5 @@ > elif returnType.isObject() or returnType.isSpiderMonkeyInterface(): > args.append(Argument("JS::MutableHandle<JSObject*>", "aRetVal")) > > + # And the nsIPrincipal > + if 'explicitPrincipal' in self.extendedAttrs: And here self.member.getExtendedAttribute('ExplicitPrincipal')? ::: dom/bindings/Configuration.py @@ +582,5 @@ > attrs = self.extendedAttributes['all'].get(name, []) > maybeAppendInfallibleToAttrs(attrs, throws) > + > + if member.getExtendedAttribute("ExplicitPrincipal"): > + attrs.append("explicitPrincipal") And then you don't need these changes in Configuration.py

This is no longer needed for bug 1297687.

Comment on attachment 8793643 [details] [diff] [review] part 1 - WebIDL [ExplicitPrincipal] ehsan said he could review this. I'm rather overloaded with reviews right now.

Comment on attachment 8793643 [details] [diff] [review] part 1 - WebIDL [ExplicitPrincipal] Review of attachment 8793643 [details] [diff] [review]: ----------------------------------------------------------------- r=me with Peter's comments addressed.

Comment on attachment 8793644 [details] [diff] [review] part 2 - DataTransferItem Review of attachment 8793644 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/DataTransferItem.webidl @@ +14,2 @@ > void getAsString(FunctionStringCallback? _callback); > + [Throws,ExplicitPrincipal] Actually, I'm not a fan of the name ExplicitPrincipal. How about [NeedsSubjectPrincipal]? Also, as a nit, space after comma!

Comment on attachment 8793645 [details] [diff] [review] part 3 - DataTransferItemList Review of attachment 8793645 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/DataTransfer.cpp @@ +476,5 @@ > { > Optional<nsAString> format; > format = &aFormat; > ErrorResult rv; > + ClearData(format, Some(nsContentUtils::SubjectPrincipal()), rv); It's quite sad that you have to do this to support nsIDOMDataTransfer. :( But it turns out that there's no consumers for the XPCOM versions of clearData() or mozClearDataAt(). Therefore, I think it's better to remove both of these XPCOM methods from nsIDOMDataTransfer. What do you think? (This can obviously be done in a follow-up!)

Comment on attachment 8793646 [details] [diff] [review] part 4 - location Review of attachment 8793646 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Location.cpp @@ +914,4 @@ > { > + if (!aSubjectPrincipal) { > + return false; > + } This should never happen, so please MOZ_ASSERT instead.

Comment on attachment 8793647 [details] [diff] [review] part 5 - CSSStyleSheet Review of attachment 8793647 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/CSSStyleSheet.cpp @@ +1820,5 @@ > CSSStyleSheet::GetCssRules(nsIDOMCSSRuleList** aCssRules) > { > ErrorResult rv; > + nsCOMPtr<nsIDOMCSSRuleList> rules = > + GetCssRules(Some(nsContentUtils::SubjectPrincipal()), rv); Same comment for these ones as well.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e716d57f4b2 Make passing of subject principals to webidl entry points explicit - part 1 - WebIDL [NeedsSubjectPrincipal], r=ehsan, r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5ddd3a3758 Make passing of subject principals to webidl entry points explicit - part 2 - DataTransferItem, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d03e3a73d3 Make passing of subject principals to webidl entry points explicit - part 3 - DataTransferItemList, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5925a07224 Make passing of subject principals to webidl entry points explicit - part 4 - Location, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/68898ebb4390 Make passing of subject principals to webidl entry points explicit - part 5 - CSSStyleSheet, r=ehsan

> > CSSStyleSheet::GetCssRules(nsIDOMCSSRuleList** aCssRules) > Same comment for these ones as well. They are used by addons.

https://hg.mozilla.org/mozilla-central/rev/0e716d57f4b2 https://hg.mozilla.org/mozilla-central/rev/6a5ddd3a3758 https://hg.mozilla.org/mozilla-central/rev/a3d03e3a73d3 https://hg.mozilla.org/mozilla-central/rev/cc5925a07224 https://hg.mozilla.org/mozilla-central/rev/68898ebb4390