Bug 217077 - Storage Access API: Enable per-page storage access scope and align test cases
Summary: Storage Access API: Enable per-page storage access scope and align test cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-28 19:28 PDT by John Wilander
Modified: 2022-12-15 06:59 PST (History)
7 users (show)

See Also:


Attachments
Patch (22.61 KB, patch)
2020-09-28 19:48 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (15.74 KB, patch)
2020-09-30 17:59 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (15.75 KB, patch)
2020-09-30 18:01 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (18.13 KB, patch)
2020-10-04 21:17 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2020-09-28 19:28:32 PDT
It was decided in https://github.com/privacycg/storage-access/issues/3 that browsers should grant storage access for all same-site resources on the whole page, not just the requesting iframe.
Comment 1 John Wilander 2020-09-28 19:28:47 PDT
<rdar://problem/69017878>
Comment 2 John Wilander 2020-09-28 19:48:57 PDT
Created attachment 409955 [details]
Patch
Comment 3 Brent Fulgham 2020-09-30 09:55:14 PDT
Comment on attachment 409955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409955&action=review

> Source/WebCore/ChangeLog:21
> +            Changed the default setting for m_storageAccessScope.

"to per-page."

> Source/WebCore/page/Settings.yaml:705
> +  initial: true

Does this setting only exist to support test cases? I wonder if we should have some place to collect these kinds of settings (not relevant to this patch today, just an idea).
Comment 4 John Wilander 2020-09-30 17:59:38 PDT
Created attachment 410184 [details]
Patch for landing
Comment 5 John Wilander 2020-09-30 18:01:16 PDT
Created attachment 410185 [details]
Patch for landing
Comment 6 John Wilander 2020-09-30 18:03:35 PDT
(In reply to Brent Fulgham from comment #3)
> Comment on attachment 409955 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409955&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +            Changed the default setting for m_storageAccessScope.
> 
> "to per-page."

Fixed.

> > Source/WebCore/page/Settings.yaml:705
> > +  initial: true
> 
> Does this setting only exist to support test cases? I wonder if we should
> have some place to collect these kinds of settings (not relevant to this
> patch today, just an idea).

It is only for test cases, yes.
Comment 7 John Wilander 2020-09-30 18:03:46 PDT
Also, thanks for the review, Brent!
Comment 8 EWS 2020-09-30 20:21:12 PDT
Committed r267817: <https://trac.webkit.org/changeset/267817>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410185 [details].
Comment 9 Truitt Savell 2020-10-01 08:15:22 PDT
It looks like the changes in https://trac.webkit.org/changeset/267817/webkit

broke these two tests 
http/tests/storageAccess/request-and-grant-access-cross-origin-sandboxed-iframe-from-prevalent-domain-without-user-interaction.html 
http/tests/storageAccess/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2FstorageAccess%2Frequest-and-grant-access-cross-origin-sandboxed-iframe-from-prevalent-domain-without-user-interaction.html&test=http%2Ftests%2FstorageAccess%2Frequest-storage-access-cross-origin-sandboxed-iframe-without-allow-token.html

Diffs

--- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-and-grant-access-cross-origin-sandboxed-iframe-from-prevalent-domain-without-user-interaction-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-and-grant-access-cross-origin-sandboxed-iframe-from-prevalent-domain-without-user-interaction-actual.txt
@@ -3,8 +3,9 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS Storage access was denied. document.cookie == , cookies seen server-side == "No cookies"
+FAIL FAIL Storage access was granted and requestStorageAccessResolved was not granted but should not have been granted. document.cookie ==
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
 

--- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token-actual.txt
@@ -3,8 +3,9 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS Storage access was denied. document.cookie == , cookies seen server-side == "No cookies"
+FAIL Storage access was granted and requestStorageAccessResolved was not granted but should not have been granted. document.cookie ==
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
Comment 10 John Wilander 2020-10-01 08:24:50 PDT
All tests passed on my system and the previous patch was all green on EWS. You can revert my patch if you want. I’ll investigate.
Comment 11 John Wilander 2020-10-01 10:43:23 PDT
I can reproduce the failures when not running with the -f flag.
Comment 12 Truitt Savell 2020-10-02 14:53:14 PDT
Reverted r267817 for reason:

Broke two tests on Mac

Committed r267897: <https://trac.webkit.org/changeset/267897>
Comment 13 John Wilander 2020-10-04 21:17:25 PDT
Created attachment 410500 [details]
Patch
Comment 14 John Wilander 2020-10-04 21:19:11 PDT
Comment on attachment 410500 [details]
Patch

Brent, I fixed the two failing tests by adding WebPage::clearPageLevelStorageAccess() and calling it in WebProcess::clearResourceLoadStatistics(). Your name is still as reviewer in the change log. If it looks good to you, please cq+. Thanks!
Comment 15 EWS 2020-10-05 09:42:12 PDT
Committed r267973: <https://trac.webkit.org/changeset/267973>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410500 [details].