1422056 - Isolate site permissions by OriginAttributes

Status: RESOLVED FIXED

(Core :: Permission Manager, enhancement, P2)

Comment 1

[Tom Ritter [:tjr]]

The FPI-only version of this bug is 1330467, which we intend to land 'sometime soonish'

Comment 2

[Thorin [:thorin]]

Hi Tom. Would this mean that all existing permissions (no origin attribute key), would no longer apply, and users would have to re-add them? By permissions I assume you mean site exceptions, right?

Comment 5

[Tanvi Vyas[:tanvi]]

This now works for FPI per the work done in bug 1330467.

Comment 6

[Emma Zühlcke [:emz]]

As discussed with Johann, I'll update the permission manager to put origin attribute stripping for user context and private browsing behind prefs and disable stripping by default.

Comment 7

[Emma Zühlcke [:emz]]

Attachment: Bug 1422056 - nsPermissionManager: Disabled OA stripping for private browsing and added OA strip prefs. r=johannh

Comment 8

[Emma Zühlcke [:emz]]

Attachment: Bug 1422056 - Updated tests to account for permission manager OA stripping. r=johannh

Depends on D48664

Comment 9

[Tanvi Vyas[:tanvi]]

Thanks Paul for your work on this!

How does this effect the work done in https://bugzilla.mozilla.org/show_bug.cgi?id=1330467 for First Party Isolation? Does it rely on the many patches in that bug, or can FPI permission implementation be simplified now that this bug is almost complete?

For multi-account Containers and Facebook Container, we should consider whether we want to keep permissions.isolateBy.userContext on or turn it off.

Comment 10

[Emma Zühlcke [:emz]]

(In reply to Tanvi Vyas[:tanvi] from comment #9)

How does this effect the work done in https://bugzilla.mozilla.org/show_bug.cgi?id=1330467 for First Party Isolation? Does it rely on the many patches in that bug, or can FPI permission implementation be simplified now that this bug is almost complete?

The FPI patch removed the origin stripping for first party domain. My patch disables the remaining OriginAttributes strip code for user context and private browsing. From what I've seen, the other FPI changes involved refactoring permission access to use principals, which my patch is based on. Removing the remaining URI based permission checks was a prerequisite to this patch, see Bug 1402957.

Comment 11

[Emma Zühlcke [:emz]]

Attachment: Bug 1422056 - Added permission manager OA strip test. r=johannh

Depends on D48665

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61ea36deed37
nsPermissionManager: Disabled OA stripping for private browsing and added OA strip prefs. r=johannh,Ehsan
https://hg.mozilla.org/integration/autoland/rev/a44806b8a838
Updated tests to account for permission manager OA stripping. r=johannh
https://hg.mozilla.org/integration/autoland/rev/297502522753
Added permission manager OA strip test. r=johannh

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/61ea36deed37
https://hg.mozilla.org/mozilla-central/rev/a44806b8a838
https://hg.mozilla.org/mozilla-central/rev/297502522753

Comment 14

[Jonathan Kingston [:jkt] he/him]

(In reply to Tanvi Vyas[:tanvi] from comment #9)

For multi-account Containers and Facebook Container, we should consider whether we want to keep permissions.isolateBy.userContext on or turn it off.

This strikes me as it should be a default, once it's tested for a few releases.
This is a really exciting patch :).

Comment 15

[Johann Hofmann [:johannh]]

I wonder if this needs to be leave-open or if we can defer containers to a follow-up bug.

Comment 16

[Emma Zühlcke [:emz]]

Attachment: Bug 1422056 - Put remaining permission userContextId OA stripping behind pref. r=johannh,ehsan

Comment 17

[Tanvi Vyas[:tanvi]]

(In reply to Jonathan Kingston [:jkt] from comment #14)

(In reply to Tanvi Vyas[:tanvi] from comment #9)

For multi-account Containers and Facebook Container, we should consider whether we want to keep permissions.isolateBy.userContext on or turn it off.

This strikes me as it should be a default, once it's tested for a few releases.
This is a really exciting patch :).

Yes, after thinking about it more, lets leave it on by default for Containers (in Nightly) and the Multi-Account Containers (MAC) extension.

How do we migrate existing permissions? Do we assume they are all in the default container? That might be odd if MAC is configured to only visit a site in a specific container. Could this be exploited in anyway? (I hypothesize that its not more exploitable than already possible without isolated permissions).

We could also throw away all permissions and start over for users who have containers enabled, but that is also poor user experience.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a50e8246828d
Put remaining permission userContextId OA stripping behind pref. r=johannh

Comment 19

[Andrei Ciure[:aciure]]

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

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a50e8246828d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Hi Paul, does this bug still need to be open?

Comment 22

[Emma Zühlcke [:emz]]

Hi Ryan, the bug is still open because we have container permission isolation preffed off by default. However, we could file that as a follow up and close this one.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I think that'd be a very good idea.

Comment 24

[Tanvi Vyas[:tanvi]]

(In reply to Paul Zühlcke [:pbz] from comment #22)

Hi Ryan, the bug is still open because we have container permission isolation preffed off by default. However, we could file that as a follow up and close this one.

Was a followup for this ever filed?

Comment 25

[Emma Zühlcke [:emz]]

Oh, sorry it seems I didn't file a follow up. I've filed one now: Bug 1641584