1798493 - Tool operating as 3rd party iframe unable to use LockManager

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[pfranza]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:106.0) Gecko/20100101 Firefox/106.0

Steps to reproduce:

Tool is a 3rd party iframe with total cookie protection turned on, the cookies/localstorage are being partitioned correctly, however when I goto use a LockManager.request it throws an error that

Uncaught (in promise) DOMException: LockManager.request: request() is not allowed in this context

Actual results:

Uncaught (in promise) DOMException: LockManager.request: request() is not allowed in this context

Expected results:

I would have expected the call to either just be partition the locks like everything else is partitioned, or to just allow the locks and make the lock local to the page only, or in the worst case just remove LockManager.request from the page so I it clear that it isn't supported and I can degrade.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Privacy: Anti-Tracking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Andrew McCreight [:mccr8]]

saschanaz, could you take a look at this bug? Thanks.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

It's the GetStorageAccess API that returns ePartition{}OrDeny and currently the implementation always denies such situation.

I'm not 100% sure partitioned context can just be allowed here. Does ContentPrincipalInfoHashKey correctly handle such partitioning?

I'll write some tests and see what happens there.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Kagami :saschanaz from comment #3)

It's the GetStorageAccess API that returns ePartition{}OrDeny and currently the implementation always denies such situation.

I'm not 100% sure partitioned context can just be allowed here. Does ContentPrincipalInfoHashKey correctly handle such partitioning?

This is a case where if we had the StorageKey abstraction (Bug 1776271 for the formal abstraction, but the OPFS patch stack is adding a GetStorageKey method that will return a PrincipalInfo for now at least) and the appropriate policy machinery integrated into the concept of a StorageKey, we could map ePartitionOrDeny to a null principal to provide for partitioning/isolation. Of course, a null principal is not a content principal, so ContentPrincipalInfoHashKey would not work for that.

Note however that there is a bit of messiness where the algorithm to obtain a storage key explicitly says "If key’s origin is an opaque origin, then return failure.", but in this case we would be using the null principal internally to correspond to a partitioned origin. GetStorageKey for an explicit opaque origin with a null principal origin would fail.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #4)

Note however that there is a bit of messiness where the algorithm to obtain a storage key explicitly says "If key’s origin is an opaque origin, then return failure.", but in this case we would be using the null principal internally to correspond to a partitioned origin. GetStorageKey for an explicit opaque origin with a null principal origin would fail.

Indeed, and https://github.com/w3c/web-locks/pull/75 wants to fix that. I'm not sure how exactly to obtain a storage bottle map algorithm maps to our implementation, though.

In any case, we might want to fix the spec first before fixing the implementation.

Edit: Oh wait, I somehow thought you pointed to https://w3c.github.io/web-locks/#obtain-a-lock-manager which has the same step. Meh.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

but the OPFS patch stack is adding a GetStorageKey method that will return a PrincipalInfo for now at least

Seems that's bug 1790207.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Of course, a null principal is not a content principal, so ContentPrincipalInfoHashKey would not work for that.

I used ContentPrincipalInfoHashKey only because every relevant principal was content principal, that should be able to change.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1798493 - Part 1: Use nsIPrincipal instead of ContentPrincipalInfo for PLockManager r=asuth

Depends on D163336

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Note for me: Migrate to nsIPrincipal from PrincipalInfo? https://groups.google.com/a/mozilla.org/g/dev-platform/c/A721Mg-SFFw

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Note for me 2: No real need to use mochitest when https://wpt.fyi/results/web-locks/partitioned-web-locks.tentative.https.html?label=experimental&label=master&aligned exists?

Comment 12

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

The following field has been copied from a duplicate bug:

Field	Value	Source
Status	NEW	bug 1764547

For more information, please visit auto_nag documentation.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Just experimented a bit and it seems nsIGlobalObject::GetStorageAccess returns eAllow for first-party iframe nested in a third-party iframe. Is it expected? Maybe Paul knows:

(Context: An existing WPT test assumes such case should be partitioned too: https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/testing/web-platform/tests/web-locks/partitioned-web-locks.tentative.https.html#101-103)

Comment 14

[Paul Zühlcke [:pbz]]

AFAIC currently we only partition by top level site without any ancestor bits so it would make sense for storage to be unpartitioned in this case. Tim, what do you think? Should we additionally take ancestors into account?
Also see https://github.com/web-platform-tests/wpt/pull/33460

Comment 15

[Tim Huang[:timhuang]]

From the privacy perspective, we only consider if the iframe domain is considered same-party as the top-level domain to determine storage access; we don't consider the ancestor chain. So, the first-party nested iframe is still considered first-party in this case.

There were some discussions for HTTP cache before regarding whether we should consider the ancestors for partitioning caches. No conclusion was made at the time because taking ancestors into account could regress performance. Still, it does provide certain benefits, like preventing cache attacks in a nested first-party iframe.

Regarding whether we should consider ancestors when partitioning Weblock, I am leaning more toward considering the ancestor chain if it doesn't regress performance. Also, the WPT test implies that we should do this. However, I am unsure whether we should apply the same thing to Storage Access because it affects other storage and is likely to have a negative impact. We should have a consensus about the behavior of storage access across browser vendors before we make any changes.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1798493 - Part 2: Use ShouldPartitionStorage in LockManager::Request r=asuth

Depends on D163337

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hello Kagami,
a gentle ping here for updates to ensure this S2 is still on track, thank you. :)

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Still waiting for review.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f65876226f15
Part 1: Use nsIPrincipal instead of ContentPrincipalInfo for PLockManager r=asuth
https://hg.mozilla.org/integration/autoland/rev/c8c8ab2aa130
Part 2: Use ShouldPartitionStorage in LockManager::Request r=asuth,anti-tracking-reviewers,pbz

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40574 for changes under testing/web-platform/tests

Comment 21

[Cosmin Sabou [:CosminS]]

Backed out for causing wpt failures on partitioned-web-locks.tentative.https.html

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=bNswrIulSPS5qQR-bIsBtQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&fromchange=c8c8ab2aa130322aef2e13d1cb87681471344af1&tochange=f9cb00c2cdc956ea9c3083d9ba66cd9848cd26db&searchStr=wpt

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=419506196&repo=autoland
• TV https://treeherder.mozilla.org/logviewer?job_id=419486726&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/f9cb00c2cdc956ea9c3083d9ba66cd9848cd26db

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 23

[Kagami Rosylight [:saschanaz] (they/them)]

The TV failure looks like an existing issue since the failure happens inside PartitionedStorageHelper.

The Android WPT failure is weird, is dFPI not enabled on Android? It seems the relevant flags does not filter Android out, not sure what's the issue here.

Comment 24

[Paul Zühlcke [:pbz]]

As discussed on Slack, it looks like dFPI isn't enabled by default on Android yet. We should consider changing the default in StaticPrefList.yaml https://searchfox.org/mozilla-central/rev/c0adc2160976e2c118e2e5709d08aac071fddce9/modules/libpref/init/StaticPrefList.yaml#11335,11347 . Before we do that we should check with the Fenix folks though. For now you can enable dFPI via pref in the test config and see if it fixes the test failure.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f602589f22e5
Part 1: Use nsIPrincipal instead of ContentPrincipalInfo for PLockManager r=asuth
https://hg.mozilla.org/integration/autoland/rev/4f4e44d1026b
Part 2: Use ShouldPartitionStorage in LockManager::Request r=asuth,anti-tracking-reviewers,pbz

Comment 26

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f602589f22e5
https://hg.mozilla.org/mozilla-central/rev/4f4e44d1026b

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot