Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal: add hasCrossSiteAncestor value to partitionKey in Cookies API #581
Changes from 34 commits
18ce5e0
deb0dd7
9d579c2
33d24cc
e991178
5ce6b56
04e4a6e
327ea24
150ffbf
0c88b2f
f064146
c341071
769663c
70a3280
ffb23e1
582b880
b209dd4
bce3162
b71a7a7
1bcf678
845f868
b2ef5e7
ab5eaaa
cdf260f
c522cdc
85428ac
86f0e7f
e244fef
64a15c5
cd75fce
5a3d145
131b666
a831db7
44a8e80
04a9fee
3faee04
a6bf397
bdf8077
f83b18f
3a8e2ba
8e05e8d
0d83efa
8328697
9edb236
13ca34f
d76d4a1
95f4753
0447b92
ac34784
f21810e
589dab8
39aefc9
7d42807
0aed648
cfe1825
92b747d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Privacy risk: "incorrect" use of the
cookies
API can result in cookies being inadvertently associated with the wrong cookie partition. See my comment atgetAll
.Regardless of whether that issue is fixed, we need to call out the risk of inadvertent mixing cookies of cookies from different partitions by extensions that are unaware of partitioning.
Note that it is currently near-impossible for an extension to "correctly" use the
cookies
API in combination with actual web pages. For the simple use case of "create cookie for this tab" or "remove cookie for this tab" ("this tab" = top-level document), the documented examples simply take the URL of the current tab to feed to the cookies API:With the introduction of the cross-site ancestor bit, that logic is unreliable. At best it removes/edits the right cookie. It is also probably for it to not remove/modify any cookie. At worst it removes/modifies the wrong cookie.
At the bare minimum, we'd need a way to query the hasCrossSiteAncestor bit for a given tab (tabId), frame (frameId) and/or document (documentId). This can be a dedicated
cookies.getHasCrossSiteAncestor({ tabId, frameId, documentId })
.I also considered a generic
cookies.getPartitionKey
method (which also takes a URL in order to determine whether the cookie would be cross-site or same-site). The advantage of this is that extension developers can then treat the return value as a black box and pass it to thecookies
API to always correctly manage the desired cookie, and that the method can account forrequestStorageAccess
calls in the document if desired.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it makes sense to provide a way to help developers obtain the expected
HasCrossSiteAncestor
value. I'd be in support of acookies.getPartitionKey
method and don't mind drafting a proposal for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added language that describes the preexisting risk, in the privacy risk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to add getPartitionKey to this proposal or would it be a separate document?
On the one hand it is not strictly necessary for extensions that only use the cookies API to get a view of the partitioned cookies. On the other hand it would be nice if the introduction of the concept of "cross-site ancestor chain bit" also includes a first-class method for extensions to accurately determine the correct bit to use, so they can update their extension and immediately use the right mechanism from the start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference, is there one the you prefer over the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How it works internally in Firefox is explained at https://searchfox.org/mozilla-central/rev/c4966e1c1e6a8cb38c15a27693e8ecd63082055e/toolkit/components/antitracking/StoragePrincipalHelper.h#15
The short version is that a document may initially have access to partitioned storage. Then after granted access through the SSA, it may from then on have access to unpartitioned cookies, and no access to partitioned cookies.
Good question. From the extension API perspective, I would expect "whatever the effective access" is, and that is something that you should raise in the Privacy CG.
Even without SSA, I have already observed that with CHIPS through the Partitioned attribute, that it is possible for the top-level document to have cookies from two partitions. So perhaps we should shape the new getPartitionKey API such that it recognizes that there are multiple partition keys, and return all of them that may potentially appear, with the default one clearly identifiable (e.g. the first in the list, if the return value is a list).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brought up the question in today's privacy CG meeting, nobody brought up any initial concerns with the designs. However, I think we should ask for additional feedback once the API we finalize the language for the API to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given it some more thought and I think that the API we need is
cookies.getPartitionKey()
, notcookies.getHasCrossSiteAncestor()
. By getting the full key, we simplify the flow for the developer, since they will not need to create the key after callingcookies.getHasCrossSiteAncestor()
. This also allows the browser to make the decision on what key it prefers for a given frame, if there is more than one valid one. In the event the key provided by the browser is not the one needed by the developer, the developer will have enough information from the initial call tocookies.getPartitionKey()
to calculate the key they require.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added language to the PR to describe the API in more detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W, any concerns with adding this api to the proposal?