Skip to content

Conversation

@cfredric
Copy link
Contributor

@cfredric cfredric commented Feb 7, 2025

This is meant to address the ambiguities pointed out in #210 by defining Storage Access's integration with Fetch more rigorously. This PR codifies the behavior I'd like to Storage Access implementations to align on (namely stricter adherence to the Same Origin Policy).


Preview | Diff

Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the tentative WPT had more coverage of the cases Anne outlined here, although I admit I didn't update them either.


1. If |request|'s [=request/client=] is null, return false.
1. If |request|'s [=request/client=]'s [=environment/has storage access=] is false, return false.
1. If |request|'s [=url/origin=] is not [=same origin=] with |request|'s [=request/url=]'s [=url/origin=], return false.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this makes a storage access grant only apply to a single origin, rather than the entire site?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. This makes the storage access grant be initially "active" only for the origin of the document that called document.requestStorageAccess(), rather than the entire site. But if another origin needs to access cookies, it can use document.requestStorageAccess() itself (which will resolve without a user gesture or a prompt), or activate the permission a different way (e.g. Storage Access Headers).

The effect of this is to keep the privacy boundary the same (i.e. keyed on <site, site>), but restrict the security model to be origin-based rather than site-based. So, one origin's use of SAA doesn't make the other origins of the site vulnerable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me clarify: it makes document.requestStorageAccess() only apply to the origin of the document?

I'd be more comfortable with this if there were a positive signal from Apple around Storage Access Headers, because this further leans on that feature for subresources.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should fix the cross-origin-same-site navigation fetch to not send unpartitioned cookies, however the subresource fetch inside of the document is the part that leans on Storage Access Headers.

Copy link
Contributor Author

@cfredric cfredric Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me clarify: it makes document.requestStorageAccess() only apply to the origin of the document?

document.requestStorageAccess() will still request a permission that applies to the whole site, but only the document's origin is opted-in and receives cookie access as a result of the call. Other origins have to explicitly activate the permission by calling document.requestStorageAccess() themselves, if they also need cookie access.

if there were a positive signal from Apple around Storage Access Headers

FWIW, Anne wrote that SAH seems like a reasonable extension of Storage Access, and opened one issue about worker support (specifically for the Activate-Storage-Access: load header). He didn't mention any concerns about the Activate-Storage-Access: retry; ... part, which is what this would lean on. That's not a formal positive signal, though, to be fair.

I agree that we should fix the cross-origin-same-site navigation fetch to not send unpartitioned cookies, however the subresource fetch inside of the document is the part that leans on Storage Access Headers.

Are you saying that you'd expect SAA's Fetch integration to treat navigation fetches and subresource fetches differently? My assumption was that SAA's usage of Fetch would be agnostic to the request's destination, so that fetches of different kinds of resources get the same kind of CSRF protection (whether it's a navigation or not); i.e., whether cookies are included wouldn't depend on what exactly the page is fetching. WDYT?

Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other origins have to explicitly activate the permission by calling document.requestStorageAccess() themselves, if they also need cookie access.

So they would need to create another iframe to do that (excepting SAH for subresources)?

Are you saying that you'd expect SAA's Fetch integration to treat navigation fetches and subresource fetches differently?

Yeah, that is the way I was thinking. In my mind you are fetching resources that are going into a different context than the current one, so it is natural to treat them as the destination context rather than the source context.

The way I'm thinking about this is like this. The storage access API breaks open the (sub)document to use its unpartitioned cookies. An iframe provider controls security by dictating what Origins can request the API. We permit the same-origin self-initiated navigations to persist the openness to allow document fetches out of convenience to developers without relaxing the security model.

Copy link
Contributor Author

@cfredric cfredric Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So they would need to create another iframe to do that (excepting SAH for subresources)?

That's right. The principle is that if an origin didn't opt into cross-site cookie access, then the browser doesn't send cookies to it. And the two ways of opting in are:

  • Call document.requestStorageAccess() in a document which is same-origin with the server endpoint that wants to receive cookies.
  • Use SAH.

Yeah, that is the way I was thinking. In my mind you are fetching resources that are going into a different context than the current one, so it is natural to treat them as the destination context rather than the source context.

I think that model might be okay from a privacy POV, but not from a web security POV. From a security POV, we need to consider the source of the fetch too.

IMO this is easiest to see if we start by thinking about cross-site fetches: we know that when a malicious site can send unfettered credentialed requests to another site, it can carry out CSRF attacks. Whether the fetch is a navigation, or just a subresource fetch, doesn't matter w.r.t. CSRF.

But the web security model is the Same-Origin Policy, so the web's security principals are origins, not sites. So we can modify the CSRF attack (s/site/origin/g): a malicious origin should not be able to send credentialed requests to another origin. (At least not without that origin's prior go-ahead -- CORS' ACAC header is prior art here.)

This PR aims to achieve that protection -- at least whenever the SAA is the mechanism by which cookies are accessible.

@cfredric
Copy link
Contributor Author

cfredric commented Feb 11, 2025

Re: the WPT test cases Anne listed, AFAIK the WPT framework only provides two distinct domains, so we'd have to add a new one if we wanted to write the tests that involve all of A, B, and C.

The semantics in this PR don't make a distinction between navigations vs subresources vs non-subresource requests (i.e. this PR doesn't involve the request's destination or "navigation" at all), so I chose not to do all the navigation-based variations. The cases I wrote were:

  • A with a nested document B. Where B subresources B2 (different origin)
  • A with a nested document B. Where B subresources B which redirects to B2

Combined with the rest of the WPT test coverage we have, I think that's a good start. (We already have "A has a nested document B, which subresources B" and similar, I think.)

Copy link
Member

@johannhof johannhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me, but I want to be mindful that this could be a big change in web-observable behavior for some embedded third parties. It would change how SAA inherits storage access today in Firefox and Safari, and also how developers got to try it out in our initial 1% experiment in Chrome.

From a Chrome perspective I think we'd like to try and ship this security improvement (pending approval by Blink API Owners etc.), especially given that the old behavior should be easy to restore with SAH. But I'd leave it to @bvandersloot-mozilla and @annevk whether they're comfortable merging this behavior into the spec.

An alternative would be to factor the backwards-incompatible change out of the PR and merge this PR without it.

@bvandersloot-mozilla
Copy link
Collaborator

I think it may make sense to hold off merging this until we are sure the change to only effect the requesting Origin it is something that we can ship for compat reasons. The other part of this is something we should probably do though.

@cfredric
Copy link
Contributor Author

I've split this PR into two: #214 adds the request infrastructure so that we can define the current behavior in Chrome/Firefox, and this PR is now stacked on top of #214 and includes the behavior change (s/site/origin/ in a few places).

I also incorporated @annevk's feedback about using a tri-state instead of a boolean.

@cfredric
Copy link
Contributor Author

Since #214 has now been merged, I've rebased this.

Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to try. We should be prepared to revert course on this if we run into issues however. We also want to exclude tests for this from Interop 2025.

(You can consider WebKit interested.)

Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto Anne, but s/Webkit/Gecko/

@cfredric cfredric changed the title Add request integration with stricter Same Origin Policy adherence Adhere to Same Origin Policy more strictly Apr 7, 2025
@johannhof
Copy link
Member

So, should we merge this PR, despite not having multiple implementers yet?

@annevk
Copy link
Collaborator

annevk commented Apr 8, 2025

The template in OP should be completed, but I don't see why we wouldn't merge it? To merge you need implementer interest, not concrete implementations.

@johannhof
Copy link
Member

Ok, yeah, I agree, that makes sense. @cfredric can you complete the remaining tasks? Then I'm happy to merge this.

@cfredric
Copy link
Contributor Author

Thanks all, I filed bugs and added links to them.

@johannhof johannhof merged commit b569d1e into privacycg:main Apr 12, 2025
1 check was pending
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 14, 2025
privacycg/storage-access#213 has been merged
into the spec, so this behavior is no longer tentative.

Bug: 379030052
Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445
Reviewed-by: Aaron Selya <selya@google.com>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Aaron Selya <selya@google.com>
Cr-Commit-Position: refs/heads/main@{#1446536}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2025
privacycg/storage-access#213 has been merged
into the spec, so this behavior is no longer tentative.

Bug: 379030052
Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445
Reviewed-by: Aaron Selya <selya@google.com>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Aaron Selya <selya@google.com>
Cr-Commit-Position: refs/heads/main@{#1446536}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2025
privacycg/storage-access#213 has been merged
into the spec, so this behavior is no longer tentative.

Bug: 379030052
Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445
Reviewed-by: Aaron Selya <selya@google.com>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Aaron Selya <selya@google.com>
Cr-Commit-Position: refs/heads/main@{#1446536}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 23, 2025
…ive", a=testonly

Automatic update from web-platform-tests
[SAA] Rename SOP tests to remove "tentative"

privacycg/storage-access#213 has been merged
into the spec, so this behavior is no longer tentative.

Bug: 379030052
Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445
Reviewed-by: Aaron Selya <selya@google.com>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Aaron Selya <selya@google.com>
Cr-Commit-Position: refs/heads/main@{#1446536}

--

wpt-commits: 84067a927d3b9d8430a0abbc91bdf1370e7084d8
wpt-pr: 51972
shtrom pushed a commit to mozilla-conduit/ff-test that referenced this pull request Apr 28, 2025
…ive", a=testonly

Automatic update from web-platform-tests
[SAA] Rename SOP tests to remove "tentative"

privacycg/storage-access#213 has been merged
into the spec, so this behavior is no longer tentative.

Bug: 379030052
Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445
Reviewed-by: Aaron Selya <selya@google.com>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Aaron Selya <selya@google.com>
Cr-Commit-Position: refs/heads/main@{#1446536}

--

wpt-commits: 84067a927d3b9d8430a0abbc91bdf1370e7084d8
wpt-pr: 51972
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 8, 2025
This ships https://chromestatus.com/feature/5169937372676096, which was
approved in
https://groups.google.com/a/chromium.org/g/blink-dev/c/jot2b6e0ewo/m/iDXIfzNJAgAJ

Note that this feature improves security of the API (by adhering to the
SOP), but does not change any privacy properties.

Spec change: privacycg/storage-access#213

Fixed: 379030052
Change-Id: I7d8d97b1cffe131f01c279df03a7b11bd6616c1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6281077
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1498795}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants