Restrict to (top-level site, embedded origin) keying (fixes #39) #123
Conversation
) This is what Firefox ships and Chrome intends to ship, so it makes sense to align the spec. Some of our rationale for why we prefer origin as the embedded choice is outlined in privacycg#113. These security concerns don't apply to top-level site and indeed we've seen compatibility cases in Firefox that justify keeping top-level site.
storage-access.bs
Outdated
|
|
||
| <div class=example> | ||
|
|
||
| `(("https", "news.example"), ("https", "social.example"))` is a [=partitioned storage key=] whose [=top-level site=] is `("https", "news.example")` and whose [=embedded site=] is `("https", "social.example")`. | ||
| `(("https", "news.example"), ("https", "social.example"))` is a [=partitioned storage key=] whose [=top-level site=] is `("https", "news.example")` and whose [=embedded origin=] is `("https", "social.example", null, null)`. |
There was a problem hiding this comment.
You would need to update the partitioned storage key itself as well I think as ("https", "social.example") is not a valid origin representation.
There was a problem hiding this comment.
Right, thanks for catching that :)
|
Agreed that this seems worth revisiting depending on known fallout/breakage. It's also worth mentioning that per-frame (#122) storage access reduces the attack potential that we're trying to mitigate with this change. |
|
So one thing that we should try to do before we land this is to have adequate testing for this. Trying to use https://whatwg.org/working-mode#changes as we make changes will make our lives a lot easier as we eventually migrate this document. |
|
Hmm that's a good point, though testing infra for SAA is a bit needing and so we'd have to evaluate this on a case-by-case basis, I think. We should invest more in better infra after deciding on #122. Should we have a checklist for those things on new PRs? |
|
Retroactively filled out the checklist in the initial comment, @annevk does it make sense to file a bug about this with WebKit? |
|
If you can construct a test that WebKit fails, yes. |
|
So, yeah, having looked further into this I'm pretty sure that we can't test this through WPT right now. The |
|
Well, we could add manual tests and automate those later. |
|
@johannhof I believe the set_permission command should be working in Chrome/Edge impls when running the WPT tests as part of a Chromium enlistment. Looking at the public webdriver test runs you linked the error seems to be coming from that WebDriver instance hitting a different permissions path and throwing the descriptor error vs how it is hooked up and run via blink web tests for all WPT test instances (where it succeeds). That seems more like a potential impl bug vs a test blocker in this case. FWIW there was some work done to the WebDriver spec a while back to help support testability of this spec: w3c/webdriver#1437 Including the addition of a set_storage_access API in testdriver (impl'd in Chromium) to allow us to set storage access (e.g. blocked) then verify the API will unblock access. example usage: https://github.com/web-platform-tests/wpt/blob/master/storage-access-api/storageAccess.testdriver.sub.html |
This is what Firefox ships and Chrome intends to ship, so it makes sense to align the spec. Some of our rationale for why we prefer origin as the embedded choice is outlined in #113.
These security concerns don't apply to top-level site and indeed we've seen compatibility cases in Firefox that justify keeping top-level site.
cc @arturjanc @cfredric
Preview | Diff