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 45 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.
Could you specify:
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.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.
It should always return a partitionKey unless an error occurs.
In the chromium implementation,
cookies.get()
,cookies.set()
andcookies.remove()
all require host permissions so I think it makes sense to be consistent here as well.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?
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.
SGTM.
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.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.
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.
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?
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.
Yes, pushed the commit