URL-Classifier code should use nsIPrincipal when calling the permission manager

RESOLVED FIXED in mozilla17

Status

()

Toolkit
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 644096 [details] [diff] [review]
Part 1 - nsIURIClassifier.classify() takes a principal
Attachment #644096 - Flags: review?(bsmith)
(Assignee)

Comment 2

6 years ago
Created attachment 644098 [details] [diff] [review]
Part 2 - nsIUrlClassifierDBService.lookup() takes principal
Attachment #644098 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #644098 - Flags: review? → review?(bsmith)
Comment on attachment 644098 [details] [diff] [review]
Part 2 - nsIUrlClassifierDBService.lookup() takes principal

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

Seems reasonable. It is not good that we do not have any tests that actually check to ensure that the data is being isolated on a per-app-ID basis though. I guess that is the case for the other related changes too. What's the plan for testing that?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +4285,5 @@
>    NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
> +  NS_ENSURE_ARG(aPrincipal);
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));

This needs to use the innermost URI.

@@ +4311,5 @@
>          do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>  
>        if (permissionManager) {
>          PRUint32 perm;
> +        permissionManager->TestPermissionFromPrincipal(aPrincipal,

Why are we not checking the return value here.

I bet this is a common pattern in callers, so TestPermissionFromPrincipal and related Test* functions should be made to set the value of *perm to the safest possible value (permission not granted).

::: toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ +29,5 @@
>  
>  NS_IMETHODIMP
>  UrlClassifierDBServiceWorkerProxy::LookupRunnable::Run()
>  {
> +  mTarget->Lookup(mPrincipal, mCB);

nit: (void) since you are ignoring the return value.
Attachment #644098 - Flags: review?(bsmith) → review+
Comment on attachment 644096 [details] [diff] [review]
Part 1 - nsIURIClassifier.classify() takes a principal

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

Again, seems reasonable but we're not actually testing the intended effect of this change; we're just testing that we didn't break the existing functionality.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +4255,5 @@
>      new nsUrlClassifierClassifyCallback(c, mCheckMalware, mCheckPhishing);
>    if (!callback) return NS_ERROR_OUT_OF_MEMORY;
>  
> +  nsCOMPtr<nsIURI> uri;
> +  aPrincipal->GetURI(getter_AddRefs(uri));

check return value or (void) if GetURI is infallable.
Attachment #644096 - Flags: review?(bsmith) → review+
I am not a toolkit reviewer, so CCing dolske so he's aware I'm playing in his yard.
I'm fine with Brian reviewing this stuff
(Assignee)

Updated

6 years ago
Summary: URL-Classifier code should use → URL-Classifier code should use nsIPrincipal when calling the permission manager
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Attachment #644096 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #644098 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/77fff33d19aa
https://hg.mozilla.org/mozilla-central/rev/88af948f119e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [qa-]

Updated

5 years ago
Depends on: 811193
(Assignee)

Updated

5 years ago
Depends on: 813897
You need to log in before you can comment on or make changes to this bug.