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.
Integrate with new draft cookie spec (draft-annevk-johannhof-httpbis-cookies/00+ε) #1807
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
base: main
Are you sure you want to change the base?
Integrate with new draft cookie spec (draft-annevk-johannhof-httpbis-cookies/00+ε) #1807
Changes from 1 commit
aef5c9d
7a6a1fe
c79ead3
853595b
fb5a26a
fea955e
0b38de1
f6d0076
72d8ecf
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.
Should this be "same-site" or "same-site-cross-origin"?
Also, you overwrite a variable with "set" and "to". "Let ... be" is only for initializing.
Single dot at the end here too.
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'm going to redefine the enum to match
Sec-Fetch-Site
, omittingnone
. That would make thissame-site
, yes.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.
Hmm, it strikes me that we should turn these two into a "redirect taint" enum for responses?
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.
That is a bit cleaner. I thought it would have the side effect of being a bit harder to read, unless we define algorithms for these two booleans which do the two obvious operations. But now that I type an example out, I like the enum even better.
e.g. compare "If response's has-cross-origin-redirects is false" to "If response's redirect taint is not 'cross-origin'".
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 think we need a dedicated Cookies section and the the Cookie header can be one part of that.
The other part will need to tackle johannhof/draft-annevk-johannhof-httpbis-cookies#13. In particular defining requirements for user agents around clearing cookies and how those changes are propagated across documents. At least I think we want all of the architecture to be handled by Fetch and then have HTML, Cookie Store API, and Service Workers (although maybe no Service Workers changes are required? I don't recall) call into that.
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.
Shuffled. That's how I had it in my first draft anyway.
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 think it's appropriate to
<dfn>
it. We should do this more akin toContent-Type
.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.
👍
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.
Indentation is wrong here.
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.
Isn't it strictly based on the URL scheme?
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 just tested on Chrome, Safari, and Firefox. Chrome and Firefox allow write-then-read of
Secure
attribute having cookies on localhost.I have no idea when this happened because I thought the same. Maybe I didn't test what I wanted?
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" here refers to the word "should" in the line below? I'm not a huge fan of that, both because it's a bit subtle and also because it doesn't account for the opposite case: All major browsers have settings / overrides that will allow cross-site cookies to be sent, so I think it would be preferable to insert a step that allows user agents to make an additional implementation defined choice about whether to include cookies or not. It might have to live in the
append a request Cookie header
algorithm.Obviously this should be reserved for truly implementation-specific settings and anything else should be standardized here.
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.
Yeah, we can move the user-configuration logic into
append a request Cookie header
.Since you left this comment I split out the partitioning bits, so that may just resolve it.