775377 - nsIContentPermissionRequest should use nsIPrincipal instead of URI

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

This is using the permission manager to check for the permission and that should use a principal instead of an URI.

Doug, Jonas told me you wrote that code. Do you think you could change that?

Comment 1

[Doug Turner (:dougt)]

I could, sure.  Won't happen this week though.  Feel free to reassign if you want me to look at it.  wchen could also take a look too.

Comment 2

[Mounir Lamouri (:mounir)]

wchen, can you look at this? We need that to happen ASAP. It's a major blocker to switch the permission manager to be "app-aware".

Comment 3

[William Chen [:wchen]]

Yeah, I'll take a look at this. It was bugging me that we were using URIs for permissions.

Comment 4

[William Chen [:wchen]]

Attachment: Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.

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

chris, can you also review?

::: browser/components/nsBrowserGlue.js
@@ +1560,4 @@
>          return;
>      }
>  
> +    var requestingPrincipal = request.principal;

Can a request here have a null principal?

::: dom/base/nsContentPermissionHelper.cpp
@@ +69,1 @@
>    if (mParent == nullptr)

style nit:

if (..) {
  stmt;
}

@@ +69,4 @@
>    if (mParent == nullptr)
>      return NS_ERROR_FAILURE;
>  
> +  NS_ADDREF(*aRequestingPrincipal = mParent->mPrincipal);

is mParent->mPrincipal always non-null?

::: dom/ipc/PermissionMessageUtils.cpp
@@ +18,5 @@
> +  }
> +
> +  bool isSerialized = false;
> +  nsCString principalString;
> +  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);

We should assert/die if we find a principal that doesn't serialize.

@@ +29,5 @@
> +  WriteParam(aMsg, isSerialized);
> +
> +  if (isSerialized) {
> +    WriteParam(aMsg, principalString);
> +  } else {

no need for else here.

if (isSerialized) {
  WriteParam(aMsg, principalString);
}

::: dom/ipc/PermissionMessageUtils.h
@@ +21,5 @@
> +  operator nsIPrincipal*() const { return mPrincipal.get(); }
> +
> +private:
> +  // Unimplemented
> +  Principal& operator=(Principal&);

are you worried about assignment?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.


>diff -r 1cb91ed25dd0 -r 8abeec1f1787 b2g/components/ContentPermissionPrompt.js

>       "type": "permission-prompt",
>       "permission": request.type,
>       "id": requestId,
>-      "url": request.uri.spec
>+      "url": request.principal.URI.spec

This needs to be changed too.  Please file a followup on sending this
information to the system app too.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/base/nsContentPermissionHelper.cpp

> nsresult
> nsContentPermissionRequestProxy::Init(const nsACString & type,
>-				                      mozilla::dom::ContentPermissionRequestParent* parent)
>+                                      mozilla::dom::ContentPermissionRequestParent* parent)

|using namespace mozilla::dom| and remove the qualification here.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/devicestorage/nsDeviceStorage.cpp

I didn't look at this.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PBrowser.ipdl

>+    PContentPermissionRequest(nsCString aType, Principal principal);

We need to document these things.  Explain that this initiates an
asynchronous request asking for permission |aType| for the object
|principal|.  In the parent process, |principal| is "untrusted", and
requests are restricted to the principals that can live in the content
process.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PermissionMessageUtils.cpp

>+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
>+  if (serializable) {

If we try to serialize an unserializable URI, then the content process
will just end up being killed, but confusingly because it'll be
asynchronously killed after the parent realizes it didn't get a URI.

Let's just preemptively NS_RUNTIMEABORT() here.

>+  if (isSerialized) {

Won't need the if/else here.

>+bool
>+ParamTraits<Principal>::Read(const Message* aMsg, void** aIter, paramType* aResult)
>+{
>+  bool isNull;
>+  if (!ReadParam(aMsg, aIter, &isNull)) {

What's the meaning of a null URI in this context?  I don't know.

We don't need to resolve that here, since I think the old URI
serialization did this too, but please file a followup and CC bz /
sicking / jlebar / mounir to comment on this.

>+  bool isSerialized;

We won't need this logic now.

I'm not a good reviewer for the non-IPC parts.

Would like to see the next one.

Comment 7

[William Chen [:wchen]]

(In reply to Doug Turner (:dougt) from comment #5)
> Comment on attachment 647186 [details] [diff] [review]
> Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
> 
> Review of attachment 647186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> chris, can you also review?
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1560,4 @@
> >          return;
> >      }
> >  
> > +    var requestingPrincipal = request.principal;
> 
> Can a request here have a null principal?
A request should not have a null principal. I have looked through the callers and this should be the case.
> 
> ::: dom/base/nsContentPermissionHelper.cpp
> @@ +69,1 @@
> >    if (mParent == nullptr)
> 
> style nit:
> 
> if (..) {
>   stmt;
> }
> 
done.
> @@ +69,4 @@
> >    if (mParent == nullptr)
> >      return NS_ERROR_FAILURE;
> >  
> > +  NS_ADDREF(*aRequestingPrincipal = mParent->mPrincipal);
> 
> is mParent->mPrincipal always non-null?
>
Yes
> ::: dom/ipc/PermissionMessageUtils.cpp
> @@ +18,5 @@
> > +  }
> > +
> > +  bool isSerialized = false;
> > +  nsCString principalString;
> > +  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
> 
> We should assert/die if we find a principal that doesn't serialize.
> 
done.
> @@ +29,5 @@
> > +  WriteParam(aMsg, isSerialized);
> > +
> > +  if (isSerialized) {
> > +    WriteParam(aMsg, principalString);
> > +  } else {
> 
> no need for else here.
> 
done
> if (isSerialized) {
>   WriteParam(aMsg, principalString);
> }
> 
> ::: dom/ipc/PermissionMessageUtils.h
> @@ +21,5 @@
> > +  operator nsIPrincipal*() const { return mPrincipal.get(); }
> > +
> > +private:
> > +  // Unimplemented
> > +  Principal& operator=(Principal&);
> 
> are you worried about assignment?
No, but the serialization wrapper for URI (IPC::URI) did this so I am guessing there is a reason. The class should only be used for IPC and I don't see a reason to allow users to do much with IPC::Principal.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Comment on attachment 647186 [details] [diff] [review]
> Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
> 
> 
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 b2g/components/ContentPermissionPrompt.js
> 
> >       "type": "permission-prompt",
> >       "permission": request.type,
> >       "id": requestId,
> >-      "url": request.uri.spec
> >+      "url": request.principal.URI.spec
> 
> This needs to be changed too.  Please file a followup on sending this
> information to the system app too.
> 
ok
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/base/nsContentPermissionHelper.cpp
> 
> > nsresult
> > nsContentPermissionRequestProxy::Init(const nsACString & type,
> >-				                      mozilla::dom::ContentPermissionRequestParent* parent)
> >+                                      mozilla::dom::ContentPermissionRequestParent* parent)
> 
> |using namespace mozilla::dom| and remove the qualification here.
> 
done
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/devicestorage/nsDeviceStorage.cpp
> 
> I didn't look at this.
> 
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PBrowser.ipdl
> 
> >+    PContentPermissionRequest(nsCString aType, Principal principal);
> 
> We need to document these things.  Explain that this initiates an
> asynchronous request asking for permission |aType| for the object
> |principal|.  In the parent process, |principal| is "untrusted", and
> requests are restricted to the principals that can live in the content
> process.
> 
done
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PermissionMessageUtils.cpp
> 
> >+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
> >+  if (serializable) {
> 
> If we try to serialize an unserializable URI, then the content process
> will just end up being killed, but confusingly because it'll be
> asynchronously killed after the parent realizes it didn't get a URI.
> 
> Let's just preemptively NS_RUNTIMEABORT() here.
> 
done
> >+  if (isSerialized) {
> 
> Won't need the if/else here.
> 
done
> >+bool
> >+ParamTraits<Principal>::Read(const Message* aMsg, void** aIter, paramType* aResult)
> >+{
> >+  bool isNull;
> >+  if (!ReadParam(aMsg, aIter, &isNull)) {
> 
> What's the meaning of a null URI in this context?  I don't know.
> 
> We don't need to resolve that here, since I think the old URI
> serialization did this too, but please file a followup and CC bz /
> sicking / jlebar / mounir to comment on this.
> 
I think it is reasonable that we are able to serialize null pointers for the sake of completeness and leave it up to the consumers to decide what they want to do with a null pointer. I'm not sure I see the problem, but sure, I'll cc someone to comment.
> >+  bool isSerialized;
> 
> We won't need this logic now.
> 
done.
> I'm not a good reviewer for the non-IPC parts.
> 
> Would like to see the next one.

Comment 8

[William Chen [:wchen]]

Attachment: Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2

Comment 9

[William Chen [:wchen]]

Attachment: v1 diff v2

Comment 10

[Doug Turner (:dougt)]

What are the two follow up bugs (one for sending the request.principal information to the system app, serialize null uri pointers)?

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 648149 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2

You'll have to rebase this patch as it doesn't apply cleanly. Please post a new patch.  Also, please send this new patch to try so that there are no surprises.

Comment 12

[William Chen [:wchen]]

Attachment: Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v3

Rebased. https://tbpl.mozilla.org/?tree=Try&rev=5efc48cfb7fd

I added one of the followup bugs (system app) to this one. The other follow up (uri serialization) is bug 781028.

Comment 13

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37694e6f8df0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37694e6f8df0

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 649940 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v3

this should have been r+'ed when I pushed.