Bug 217077 – Storage Access API: Enable per-page storage access scope and align test cases

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

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.

[John Wilander]

<rdar://problem/69017878>

[John Wilander]

Created attachment 409955 [details]
Patch

[Brent Fulgham]

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

[John Wilander]

Created attachment 410184 [details]
Patch for landing

[John Wilander]

Created attachment 410185 [details]
Patch for landing

[John Wilander]

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

[John Wilander]

Also, thanks for the review, Brent!

[EWS]

Committed r267817: <https://trac.webkit.org/changeset/267817>

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

[Truitt Savell]

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

[John Wilander]

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.

[John Wilander]

I can reproduce the failures when not running with the -f flag.

[Truitt Savell]

Reverted r267817 for reason:

Broke two tests on Mac

Committed r267897: <https://trac.webkit.org/changeset/267897>

[John Wilander]

Created attachment 410500 [details]
Patch

[John Wilander]

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!

[EWS]

Committed r267973: <https://trac.webkit.org/changeset/267973>

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