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

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - nsIURIClassifier.classify() takes a principal

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - nsIUrlClassifierDBService.lookup() takes principal

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am not a toolkit reviewer, so CCing dolske so he's aware I'm playing in his yard.

Comment 6

[Dave Townsend [:mossop]]

I'm fine with Brian reviewing this stuff

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77fff33d19aa
https://hg.mozilla.org/mozilla-central/rev/88af948f119e