nsIContentPermissionRequest should use nsIPrincipal instead of URI

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: wchen)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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?
(Reporter)

Updated

5 years ago
Blocks: 756644

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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".
(Assignee)

Comment 3

5 years ago
Yeah, I'll take a look at this. It was bugging me that we were using URIs for permissions.
(Reporter)

Updated

5 years ago
Assignee: nobody → wchen
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
(Assignee)

Comment 4

5 years ago
Created attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
Attachment #647186 - Flags: review?(doug.turner)

Comment 5

5 years ago
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?
Attachment #647186 - Flags: review?(jones.chris.g)
Attachment #647186 - Flags: review?(doug.turner)
Attachment #647186 - Flags: review-
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.
Attachment #647186 - Flags: review?(jones.chris.g)
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 648149 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2
Attachment #647186 - Attachment is obsolete: true
Attachment #648149 - Flags: review?(doug.turner)
(Assignee)

Comment 9

5 years ago
Created attachment 648150 [details] [diff] [review]
v1 diff v2
What are the two follow up bugs (one for sending the request.principal information to the system app, serialize null uri pointers)?
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.
Attachment #648149 - Flags: review?(doug.turner) → review+
(Assignee)

Updated

5 years ago
Blocks: 781011
(Assignee)

Comment 12

5 years ago
Created attachment 649940 [details] [diff] [review]
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.
Attachment #648149 - Attachment is obsolete: true
Attachment #648150 - Attachment is obsolete: true
Attachment #649940 - Flags: review?(doug.turner)
https://hg.mozilla.org/integration/mozilla-inbound/rev/37694e6f8df0
https://hg.mozilla.org/mozilla-central/rev/37694e6f8df0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
Attachment #649940 - Flags: review?(doug.turner) → review+

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.