Closed Bug 1422056 Opened 3 years ago Closed 1 year ago

Isolate site permissions by OriginAttributes

Categories

(Core :: Permission Manager, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: groovecoder, Assigned: pbz)

References

(Depends on 2 open bugs, Blocks 4 open bugs, Regressed 1 open bug)

Details

Attachments

(4 files)

No description provided.
The FPI-only version of this bug is 1330467, which we intend to land 'sometime soonish'
See Also: → 1330467
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?
Duplicate of this bug: 1519881
Component: Security → Permission Manager
Depends on: 1330467
Priority: -- → P2
See Also: 1330467
Type: defect → enhancement
Duplicate of this bug: 1546653

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

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.

Assignee: nobody → pbz
Status: NEW → ASSIGNED
Blocks: 1588457
Blocks: 1588461
Depends on: 1589162

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.

(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.

Depends on: 1589608
Attachment #9099821 - Attachment description: Bug 1422056 - nsPermissionManager: Disabled OA stripping for private browsing and user context and added prefs. r=johannh → Bug 1422056 - nsPermissionManager: Disabled OA stripping for private browsing and added OA strip prefs. r=johannh
Keywords: leave-open
Pushed by pzuhlcke@mozilla.com:
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
Regressions: 1591695

(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 :).

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

Regressions: 1591748

(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.

Regressions: 1592028
Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a50e8246828d
Put remaining permission userContextId OA stripping behind pref. r=johannh
Regressions: 1597751
Regressions: 1601256

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

Flags: needinfo?(pbz)

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.

Flags: needinfo?(pbz)

I think that'd be a very good idea.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
See Also: → 1617386

(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?

Flags: needinfo?(pbz)
Blocks: 1641584

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

Flags: needinfo?(pbz)
You need to log in before you can comment on or make changes to this bug.