Switch to (top-level site, embedded site) keying (closes #147, #156) #159

johannhof · 2023-01-23T10:29:51Z

This updates the permission key for storage-access to (site, site), and also removes the concept of the "partitioned storage key", which was origin-keyed as well. The storage key was only used for running the implementation-defined steps that are supposed to be removed as of #156.

At least two implementers are interested (and none opposed)
Tests are written and can be reviewed and commented upon at:
- https://chromium-review.googlesource.com/c/chromium/src/+/4307251
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1418470
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1825216
- WebKit: Shipping behavior

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

privacycg#156) This updates the permission key for storage-access to (site, site), and also removes the concept of the "partitioned storage key", which was origin-keyed as well. The storage key was only used for running the implementation-defined steps that are supposed to be removed as of privacycg#156.

annevk · 2023-01-24T13:28:29Z

For future context:

This is the first time a permission is scoped wider than an origin. Though at the same time it's constrained by the top-level site, which is a thing that other permissions don't need as they are restricted to the top-level origin.
This satisfies an important aspect of Improving the Storage Access API security model #113 as this will continue to require each document to call requestStorageAccess(). So one document cannot directly impact another.

annevk

This looks good to me, modulo example and tests. I'd prefer we follow https://github.com/whatwg/meta/blob/main/COMMITTING.md for our commit messages. E.g., issues that get closed should go in the commit body, not the title.

storage-access.bs

(closes privacycg#156) This was originally part of privacycg#159 but I'm submitting it separately since this is a rather editorial change that we can probably fast-track without including it in the other PR which still needs WPT etc.

(closes #156) This was originally part of #159 but I'm submitting it separately since this is a rather editorial change that we can probably fast-track without including it in the other PR which still needs WPT etc.

johannhof · 2023-01-27T12:30:19Z

Merged with #161 but I still need to write tests for this and update the example for the permission key.

This updates storage-access permissions to apply on an (embedded site, top-level site) scope as detailed in privacycg/storage-access#159. For adding WPT tests, I patched up the web_test_permission_manager to special-case the storage-access permission to (site, site) scope. I kept things intentionally simple for now but we could consider abstracting out the scope into a "permission key" concept in that file, as it is done in the permissions spec now. There's follow-up work here to make this (and the existing top-level site scope) sync up with the scope that is defined in content settings, see https://crbug.com/1422971 Bug: 1418470 Change-Id: I316f682d24d962f79fcac332365790bca383c6ee

This updates storage-access permissions to apply on an (embedded site, top-level site) scope as detailed in privacycg/storage-access#159. For adding WPT tests, I patched up the web_test_permission_manager to special-case the storage-access permission to (site, site) scope. I kept things intentionally simple for now but we could consider abstracting out the scope into a "permission key" concept in that file, as it is done in the permissions spec now. There's follow-up work here to make this (and the existing top-level site scope) sync up with the scope that is defined in content settings, see https://crbug.com/1422971 Bug: 1418470 Change-Id: I316f682d24d962f79fcac332365790bca383c6ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4307251 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Johann Hofmann <johannhof@chromium.org> Reviewed-by: Christian Dullweber <dullweber@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120269}

johannhof · 2023-03-29T12:18:17Z

We have WPT for this now, I filed a bug with Firefox and I added back the example, so merging this now.

This updates storage-access permissions to apply on an (embedded site, top-level site) scope as detailed in privacycg/storage-access#159. For adding WPT tests, I patched up the web_test_permission_manager to special-case the storage-access permission to (site, site) scope. I kept things intentionally simple for now but we could consider abstracting out the scope into a "permission key" concept in that file, as it is done in the permissions spec now. There's follow-up work here to make this (and the existing top-level site scope) sync up with the scope that is defined in content settings, see https://crbug.com/1422971 Bug: 1418470 Change-Id: I316f682d24d962f79fcac332365790bca383c6ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4307251 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Johann Hofmann <johannhof@chromium.org> Reviewed-by: Christian Dullweber <dullweber@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120269}

Automatic update from web-platform-tests Use (site, site) for SAA grants This updates storage-access permissions to apply on an (embedded site, top-level site) scope as detailed in privacycg/storage-access#159. For adding WPT tests, I patched up the web_test_permission_manager to special-case the storage-access permission to (site, site) scope. I kept things intentionally simple for now but we could consider abstracting out the scope into a "permission key" concept in that file, as it is done in the permissions spec now. There's follow-up work here to make this (and the existing top-level site scope) sync up with the scope that is defined in content settings, see https://crbug.com/1422971 Bug: 1418470 Change-Id: I316f682d24d962f79fcac332365790bca383c6ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4307251 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Johann Hofmann <johannhof@chromium.org> Reviewed-by: Christian Dullweber <dullweber@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120269} -- wpt-commits: b070171e29a18891540d1a600ac649098b5571a2 wpt-pr: 38908

johannhof requested a review from annevk January 23, 2023 10:29

annevk approved these changes Jan 24, 2023

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

johannhof mentioned this pull request Jan 26, 2023

Editorial: Remove partitioned storage key and implementation-defined steps #161

Merged

Merge branch 'main' into site-site

152d09f

chromium-wpt-export-bot mentioned this pull request Mar 9, 2023

Use (site, site) for SAA grants web-platform-tests/wpt#38908

Merged

Add example for permission key

52a6cc2

johannhof merged commit ceca554 into privacycg:main Mar 29, 2023

johannhof deleted the site-site branch March 29, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to (top-level site, embedded site) keying (closes #147, #156) #159

Switch to (top-level site, embedded site) keying (closes #147, #156) #159

johannhof commented Jan 23, 2023 •

edited by pr-preview bot

Loading

annevk commented Jan 24, 2023

annevk left a comment

johannhof commented Jan 27, 2023

johannhof commented Mar 29, 2023

Switch to (top-level site, embedded site) keying (closes #147, #156) #159

Switch to (top-level site, embedded site) keying (closes #147, #156) #159

Conversation

johannhof commented Jan 23, 2023 • edited by pr-preview bot Loading

annevk commented Jan 24, 2023

annevk left a comment

Choose a reason for hiding this comment

johannhof commented Jan 27, 2023

johannhof commented Mar 29, 2023

johannhof commented Jan 23, 2023 •

edited by pr-preview bot

Loading