Skip to content
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.

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

Write specification #20

Merged
merged 21 commits into from
Nov 22, 2024
Merged

Write specification #20

merged 21 commits into from
Nov 22, 2024

Conversation

cfredric
Copy link
Collaborator

@cfredric cfredric commented Nov 7, 2024

@johannhof johannhof self-requested a review November 8, 2024 21:19
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

Partial pass

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
cfredric and others added 3 commits November 12, 2024 16:45
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 13, 2024
When drafting the spec for SAH
(privacycg/storage-access-headers#20), I
realized that Chrome is currently overly restrictive when computing
the StorageAccessStatus of a request. This prevents the
`Activate-Storage-Access: load` response header from being used in a
response to a request whose credentials mode was not "include", which
is not intended.

This CL fixes that by moving the checks of the request's credentials
mode to the appropriate places (i.e. when appending the header value,
and when processing a `Activate-Storage-Access: retry; ...` response
header.) It also splits and renames the correspond histogram into two
histograms, to measure relative incidences of the StorageAccessStatus
and Sec-Fetch-Storage-Access header values separately.

OBSOLETE_HISTOGRAM[API.StorageAccessHeader.SecFetchStorageAccessValueOutcome]=Replaced by API.StorageAccessHeader.SecFetchStorageAccessOutcome and API.StorageAccessHeader.StorageAccessStatusOutcome

Bug: 378077855
Change-Id: Ia25354d14458fd78d97ba24d43bc4806c65c81d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6004331
Reviewed-by: Ayu Ishii <[email protected]>
Reviewed-by: mmenke <[email protected]>
Commit-Queue: Ayu Ishii <[email protected]>
Auto-Submit: Chris Fredrickson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1382523}
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.

Great work Chris! Had some thoughts, but nothing major, I think.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@cfredric cfredric marked this pull request as ready for review November 20, 2024 21:04
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
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 LGTM as an initial spec draft, with the acknowledgement that there are still a few rough edges we will iron out over time.

@cfredric cfredric merged commit 7b02c6a into main Nov 22, 2024
1 check passed
@cfredric cfredric deleted the spec branch November 22, 2024 14:56
github-actions bot added a commit that referenced this pull request Nov 22, 2024
SHA: 7b02c6a
Reason: push, by cfredric

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

5 participants