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

Proposal: add hasCrossSiteAncestor value to partitionKey in Cookies API #581

Merged
merged 56 commits into from
Sep 12, 2024

Conversation

aselya
Copy link
Contributor

@aselya aselya commented Apr 1, 2024

To allow for correct interaction with partitioned cookies that include the cross-site ancestor chain bit, the Cookies API should be updated to include a parameter that corresponds to the value of the cross-site ancestor chain bit.

Add initial proposal
Updates for clarity
Update formatting of url
Updates in phrasing and document organization.
Additional content and formatting
@Rob--W
Copy link
Member

Rob--W commented Apr 1, 2024

I'm adding myself as a reviewer because I designed the partitionKey part of the cookies API that has been implemented in Firefox a few years ago, before the WECG existed (at privacycg/CHIPS#6, which is already linked from the proposal doc).

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Some comments with suggestions can be applied to the PR with Github's UI, others are discussion topics.

proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
aselya and others added 9 commits April 2, 2024 09:17
Update pr to incorporate reviewer feedback.

Co-authored-by: Rob Wu <[email protected]>
Update to reflect reviewer suggestion.

Co-authored-by: Rob Wu <[email protected]>
Update to remove reference to Chrome

Co-authored-by: Rob Wu <[email protected]>
Added background information on x-site ancestor chain bit
Update language for cookies.remove()
Copy link
Member

@oliverdunk oliverdunk left a comment

Choose a reason for hiding this comment

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

Thanks for the work here - did another pass after reading some more about the context to try and make the proposal a bit clearer.

proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
Add description for getPartitionKey() api.
Updating language for getAll() to align it with the other APIs, where a partitionKey with no topLevelSite and a value for hasCrossSiteAncestor returns an error.
Update language to specify that `{hasCrossSiteAncestor: false}` and `{hasCrossSiteAncestor:true}` are invalid keys.
Add table containing valid partitionKeys. Update language in the background section to be more consistent.
Clarify language surrounding the empty partitionKey
aselya and others added 3 commits August 21, 2024 10:24
Co-authored-by: bvandersloot-mozilla <[email protected]>
Update language to specify the domain of the cookie's url
Return {} to the table of valid partition keys
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
> `frameId`
integer. The ID of the frame in the given tab.

##### Return value
Copy link
Member

Choose a reason for hiding this comment

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

Could you specify:

  • Does this method always return a partition key? If not, when?
  • Are there any (origin) permission requirements? Cautiously we could opt for requiring host permissions for the target frame, because that permission is also required to execute a script there, which in turn could query the top level origin via location.ancestorOrigins. I am also willing to consider not requiring origins if we deem the return value to not be too sensitive. After all, extensions can already observe partitioned cookies for non-document subresources in frames through the cookies API.
  • Are there error conditions? E.g. frame does not exist, permission denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this method always return a partition key? If not, when?

It should always return a partitionKey unless an error occurs.

Are there any (origin) permission requirements?

In the chromium implementation, cookies.get(), cookies.set() and cookies.remove() all require host permissions so I think it makes sense to be consistent here as well.

Are there error conditions?

Both the frame does not exist and permission denied would be an error.
Do you have any thoughts on if a mismatch between the documentId and any tabId/frameId combination should be treated as an error?

Copy link
Member

Choose a reason for hiding this comment

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

Does this method always return a partition key? If not, when?

It should always return a partitionKey unless an error occurs.

SGTM.

Are there any (origin) permission requirements?

In the chromium implementation, cookies.get(), cookies.set() and cookies.remove() all require host permissions so I think it makes sense to be consistent here as well.

cookies.get, set, etc. only require host permissions for the URL where the cookie is set. Not for the partition key. An extension can therefore try to set cookies for a subresource (img, etc.) without having host permissions for the iframe. In the interest of getting to a resolution quicker, let's require host permissions for the document whose partitionkey is getting queried. If there is a compelling use case we could consider relaxing this, and that would be backwards-compatible since it is always possible to go from strict to less strict.

Are there error conditions?

Both the frame does not exist and permission denied would be an error. Do you have any thoughts on if a mismatch between the documentId and any tabId/frameId combination should be treated as an error?

Yes, this should be an error. When extensions use all combinations to describe a target, and there is no target matching it, then it should be an error.

Copy link
Member

Choose a reason for hiding this comment

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

This thread was marked as resolved, but the updated PR does not mention anything about permission requirements or errors. Did you forget to push a commit with the updated text?

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, pushed the commit

proposals/hasCrossSiteAncestor.md Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
@aselya
Copy link
Contributor Author

aselya commented Aug 27, 2024

Friendly ping: with the AncestorChainBit being enabled by default in Chrome, we're beginning to get reports of breakage related to extensions not being able to handle partitioned cookies.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

LGTM, with getPartitionKey semantics clarified.

proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
> `frameId`
integer. The ID of the frame in the given tab.

##### Return value
Copy link
Member

Choose a reason for hiding this comment

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

Does this method always return a partition key? If not, when?

It should always return a partitionKey unless an error occurs.

SGTM.

Are there any (origin) permission requirements?

In the chromium implementation, cookies.get(), cookies.set() and cookies.remove() all require host permissions so I think it makes sense to be consistent here as well.

cookies.get, set, etc. only require host permissions for the URL where the cookie is set. Not for the partition key. An extension can therefore try to set cookies for a subresource (img, etc.) without having host permissions for the iframe. In the interest of getting to a resolution quicker, let's require host permissions for the document whose partitionkey is getting queried. If there is a compelling use case we could consider relaxing this, and that would be backwards-compatible since it is always possible to go from strict to less strict.

Are there error conditions?

Both the frame does not exist and permission denied would be an error. Do you have any thoughts on if a mismatch between the documentId and any tabId/frameId combination should be treated as an error?

Yes, this should be an error. When extensions use all combinations to describe a target, and there is no target matching it, then it should be an error.

proposals/hasCrossSiteAncestor.md Show resolved Hide resolved
proposals/hasCrossSiteAncestor.md Outdated Show resolved Hide resolved
describe error conditions of new api
@Rob--W Rob--W merged commit d18ade9 into w3c:main Sep 12, 2024
3 checks passed
github-actions bot added a commit that referenced this pull request Sep 12, 2024
…PI (#581)

SHA: d18ade9
Reason: push, by Rob--W

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Sep 12, 2024
…PI (#581)

SHA: d18ade9
Reason: push, by Rob--W

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Rob--W
Copy link
Member

Rob--W commented Sep 16, 2024

FYI I opened a bug to track the implementation of cookies.getPartitionKey at https://bugzilla.mozilla.org/show_bug.cgi?id=1919153

@Rob--W Rob--W mentioned this pull request Sep 16, 2024
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.

7 participants