1583790 - SitePermissions should validate principal arguments

Status: RESOLVED FIXED

(Firefox :: Site Permissions, enhancement, P3)

Description

[Emma Zühlcke [:emz]]

SitePermissions methods should throw if principal arguments aren't of type nsIPrincipal. Currently they silently fail to default state.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 2

[Johann Hofmann [:johannh]]

To fix this, you will need to modify all functions in SitePermissions.jsm (only in the exposed SitePermissions object) that handle principals to return an error when the given principal is not instanceof Ci.nsIPrincipal.

However, note that some functions also take a browser argument. In those cases, it's okay to have either a browser or a principal or both, but one of them has to be there.

Please also add a test for the new errors here: https://searchfox.org/mozilla-central/rev/557a0e222dd104c5d805ba344c45d6abc27d3db0/browser/modules/SitePermissions.jsm#258

Jayati, would you like to take this bug?

Note that with this change we might break some stuff (which would be good!), so it's important to do a run on our Try server. You can find instructions about how to get access to Try on our contribution document.

Comment 3

[Jayati Shrivastava]

Thanks Johann . Will begin working on this straightaway.

Comment 4

[Jayati Shrivastava]

Attachment: Bug 1583790 - Ensure that we are passing valid principal values as arguments to functions in SitePermissions.jsm. r=prathiksha

Comment 5

[Jayati Shrivastava]

Attachment: Bug 1583790- Ensure that we are passing valid principal values as arguments to functions in SitePermissions.jsm. r=prathiksha

Depends on D66371

Comment 6

[Jayati Shrivastava]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10ca1becca39a7d61a218395056d9562562067e2

Comment 7

[Jayati Shrivastava]

Ignore the above link ,posted accidently.

This is the try server run for the patch- https://treeherder.mozilla.org/#/jobs?repo=try&revision=94fda26b68cee453f6b4ee0b775708cc150d37df

Comment 8

[Jayati Shrivastava]

(In reply to Johann Hofmann [:johannh] from comment #2)

Note that with this change we might break some stuff (which would be good!), so it's important to do a run on our Try server.

Several uncaught exceptions were thrown.We are passing invalid arguments like null as principal at many places....What do you suggest ?

Comment 9

[Johann Hofmann [:johannh]]

The reason for the failures seems to be that the patch is missing this detail from comment 2:

However, note that some functions also take a browser argument. In those cases, it's okay to have either a browser or a principal or both, but one of them has to be there.

e.g. here we're calling the API with null as the principal, but there's a browser argument, so that would be valid: https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/browser/base/content/test/permissions/browser_permission_delegate_geo.js#176

You'll need to update your patch to handle that case.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d93b4a6d9bd4
Ensure that we are passing valid principal values as arguments to functions in SitePermissions.jsm. r=johannh,prathiksha

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d93b4a6d9bd4