Restrict to (top-level site, embedded origin) keying (fixes #39)

privacycg / storage-access

The Storage Access API

https://privacycg.github.io/storage-access/

199 stars 26 forks source link

Restrict to (top-level site, embedded origin) keying (fixes #39) #123

Closed johannhof closed 1 year ago

johannhof commented 1 year ago

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

[ ] At least two implementers are interested (and none opposed):
- Firefox is shipping this
- Chrome plans to ship this
[ ] Tests are written and can be reviewed and commented upon at:
- I'm not sure we have the infrastructure to test this but I'll investigate further before continuing this PR
[ ] Implementation bugs are filed:
- Chromium: N/A
- Gecko: N/A
- WebKit: Not yet

Preview | Diff

johannhof commented 1 year ago

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.

annevk commented 1 year ago

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.

johannhof commented 1 year ago

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?

johannhof commented 1 year ago

Retroactively filled out the checklist in the initial comment, @annevk does it make sense to file a bug about this with WebKit?

annevk commented 1 year ago

If you can construct a test that WebKit fails, yes.

johannhof commented 1 year ago

So, yeah, having looked further into this I'm pretty sure that we can't test this through WPT right now. The test_driver.set_permission command for storage-access seems to fail in all browsers, so it doesn't seem possible to actually grant storage access in WPT.

annevk commented 1 year ago

Well, we could add manual tests and automate those later.

Brandr0id commented 1 year ago

@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: https://github.com/w3c/webdriver/issues/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