-
Notifications
You must be signed in to change notification settings - Fork 9.1k
v3.2: Provide guidance for the Set-Cookie
response header
#4748
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
Conversation
With apologies to @philsturgeon for clearing his approval (its' set to automatically do that if anything changes) I realized we don't need |
OK apparently I was more out-of-it yesterday with this cold than I thought. Now I have added the commit that does things I mentioned in the last comment! 🤦 |
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 mostly in favour, with some minor editorial comments. Do we need to wait for the dataValue/serializedValue elements to resolve though? I see them in an example here.
@lornajane I have rewritten this to (hopefully) focus more on examples and less on explanations. This should also go well with PR #4800 which defines Meh... now there are conflicts, though, I will rebase and force-push, but it will be the same commits. It's probably best to just look at it fresh anyway as the rewrite commit on its own is noisier than just looking at it as one thing. |
The Set-Cookie response header breaks the normal rules for headers with multiple values and requires special handling.
Co-authored-by: Phil Sturgeon <[email protected]>
Also remove a stray line from an example that didn't really hurt but wasn't needed and could have been confusing.
Co-authored-by: Lorna Jane Mitchell <[email protected]>
Explain when Set-Cookie workaround is needed Also remove a stray line from an example that didn't really hurt but wasn't needed and could have been confusing.
I commented this somewhere else too: I don't think we have the serializedValue PR merged yet and I see this one uses the feature (and it's a good use of it!) so let's get that one in before we approve this one. |
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.
+1 with one nit and two questions
Co-authored-by: Ralf Handl <[email protected]>
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.
Thanks!
Partially addresses issue #1237
The Set-Cookie response header breaks the normal rules for headers with multiple values and requires special handling.
There are two options here:
Set-Cookie
in a consistent mannerSet-Cookie
as a special case and define how it relates to normal behaviorSince RFC9110 §5.3 advises clients to treat
Set-Cookie
as a special case, I went with special-casing it. This assumes the approach to header serialization that is described in PR #4648 and discussed in one of its comment threads (specifically, that it does not include the header name which is required for it to be consistent with howstyle: "simple"
andexplode
are treated elsewhere and are defined in RFC6570).