Skip to content

Add "has storage access" boolean to environment #10990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add "has storage access" boolean to environment #10990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8ccc8d
Add "has storage access" boolean to environment
bvandersloot-mozilla Jan 31, 2025
1f84a6c
fix style issues
annevk May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108066,6 +108066,11 @@ new PaymentRequest(…); // Allowed to use
for="environment">execution ready flag</dfn></dt>
<dd><p>A flag that indicates whether the environment setup is done. It is initially
unset.</p></dd>

<dt>A <dfn data-x="concept-environment-has-storage-access" export for="environment">has storage
access</dfn></dt>
Comment on lines +108070 to +108071
Copy link

Choose a reason for hiding this comment

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

As the person who originally picked has storage access as the name for this bool, I'd now advocate for something like eligible for storage-access instead 😅

IMO, has storage access turned out to be a bit of a misnomer, since it's not authoritative for whether the environment really has access to cookies and other unpartitioned storage; the bool just indicates whether the environment has "opted in" or not. The UA's cookie store still has the ability to deny access due to user settings, for example. (See https://privacycg.github.io/storage-access/#cookies.)

Naming this eligible for storage-access would match the corresponding field on request, which conveys the same opt-in signal and is initialized based on environment's has storage access bool.

(We could also reuse the storage access eligibility enum instead of a bool, which was Anne's suggestion when I added the field to request.)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, while I think for requests it's accurate to say they are eligible, as other factors are in play, that's less clear to me for environments. They have the storage access bit.

What end user setting would come into play after they get the bit, but wouldn't have come into play while obtaining the bit? If anything it seems like it should have come into play when obtaining the bit.

Copy link

Choose a reason for hiding this comment

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

The UA should check end user settings both when setting the bit, and when "using" the bit. Failure to check the settings when using the bit is a TOCTOU bug, since the end user might have changed the settings after the bit was set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check-to-use within the same page lifetime seems very fair and like it could cause unexpected bugs in websites if their cookie access flip-flops while a user toggles a checkbox in a browser settings tab.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think the bug would be not reloading the website when such consequential choices are made.

Copy link

Choose a reason for hiding this comment

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

Forcing a reload when settings change is one solution; re-checking settings at time-of-use is another option that doesn't require reloads.

Do implementations today reload the affected pages when end user settings are changed?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of implementations reloading, but they should. Re-checking at time-of-use is not an actual solution as the website already had access to the relevant state before the end user changed the setting. Without reload the end user will either make the website non-functional or think they are preventing something that they're actually not.

Copy link

Choose a reason for hiding this comment

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

Strictly speaking, simply reloading the page isn't a full solution either because sites can store identifiers in partitioned storage. Among other things, the UA would really have to unload all documents, clear all data for all sites (both partitioned and unpartitioned), and then let the user resume browsing.

But that would be disruptive and annoying to users, and would be a powerful incentive against changing the UA's cookie settings to make them more private. I'd rather avoid that disincentive, since it doesn't seem like a practical way to increase user privacy, IMO. My intuition is that we can get most of the privacy benefits (without most of the annoyance) by simply blocking cookies when user settings say to.

But either way, we're getting into product-level settings behavior decisions now, which aren't standardized; and as far as standards go, you're the editors of the SAA spec and HTML standard. If you prefer to keep the old has storage access name, that's fine by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firefox currently reloads along the most common UX path users disable third-party cookie blocking.

Copy link
Member

Choose a reason for hiding this comment

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

"They should reload" feels like a weak argument to me when it's not normatively required. I'm pretty sure that no user agent (including Firefox, IIRC) forces a reload in all cases where cookie settings or permissions change.

With that said, this feels like the bikesheddiest conversation I've seen in a long time, and I'm glad to support either of these two names :P

<dd><p>A boolean that indicates whether the environment has access to unpartitioned cookies. It
is initially false.</p></dd>
Copy link
Member

Choose a reason for hiding this comment

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

Can I ask why this is on environment instead of environment settings object? Fetch only seems to use it on request clients, which are ESOs.

Copy link

Choose a reason for hiding this comment

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

If I remember correctly, I put it on environment because Storage Access API's changes to set up a window environment settings object need to access the has storage access bool on reservedEnvironment which is an environment (rather than environment settings object).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks, I only checked the Fetch situation, not the SAA situation.

However, I'm still confused, because I can't see any spec that accesses the reserved client's has storage access.

Copy link

Choose a reason for hiding this comment

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

I might be missing what you're saying, but the intent was for this part of SAA to set the reserved client's has storage access: https://privacycg.github.io/storage-access/#navigation:~:text=When%20creating%20request,the%20following%20hold%3A

Copy link
Member

Choose a reason for hiding this comment

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

Right, I'm saying that there is a spec which sets the bit. But there is no spec that reads the bit. So setting the bit is just dead code, from what I can tell.

Copy link

Choose a reason for hiding this comment

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

As far as I can tell, the request's reserved client is assigned to the navigation params's reserved environment in the final step of create navigation params by fetching. That's called from step 4.2 of attempt to populate the history entry's document (as long as no other navigation params was provided). Step 5.6 of that algorithm calls load a document, which calls load an HTML document, which calls create and initialize a Document object. Step 7.10 of that algorithm is to set up the window environment settings object using the navigation params' reserved environment, which is the other algorithm that SAA modified to read the has storage access bit. So I think the plumbing from write-site to read-site is intact.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So it seems like this information is being used to populate from the fetching, to the new document creation.

Such information is best stored in navigation params, not on the client. The client represents a window or worker that needs to be shown to the service worker APIs. Whereas the navigation params represents information used to create new documents.

Furthermore, I think this means the current PR has a pretty bad a bug. Since you are putting this information on the client (which is an environment settings object), you are failing to capture this information for cases where you are navigating from the initial about:blank. Because navigations from the initial about:blank reuse the same Window/environment settings object as the initial about:blank page. It's only the Document that is new.

It seems to me that "has storage access" should live on document. And it can be put there by also tracking it in navigation params, to capture from source document -> new document. A good model to follow is policy container.

I wonder, actually, if "has storage access" should live in the policy container? The main impact of that would be to automatically propagate it correctly to tricky cases like about:blank, srcdoc, blob URL documents, data: URL documents, workers, etc. per https://html.spec.whatwg.org/#determining-navigation-params-policy-container and https://html.spec.whatwg.org/#initialize-worker-policy-container

What do you think?

Copy link

Choose a reason for hiding this comment

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

Using navigation params makes sense to me. I'm unsure about the practical impact of not propagating the bool from the initial about:blank page (is it possible for the has storage access bool to become true on the initial about:blank page?), but in principle SAA ought to use the right mechanisms for things, so updating that seems good.

I don't know enough about the differences between environment vs document lifetimes, nor what policy container is or how it could be used, so I'll go back to lurking and defer to the SAA editors to manage this PR.

</dl>

<p>Specifications may define <dfn export>environment discarding steps</dfn> for environments. The
Expand Down