-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
net/http: SameSiteDefaultMode adds an incorrect 'Set-Cookie' attribute #36990
Comments
I've reverted the title change because No matter what version of the spec we're going off, |
I took a look at https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-3.1
As you can see a single "SameSite" was okay. But you are right it seems as if this was changed in https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.1 :
So it does matter which spec we try to implement. Is far as I understand the Chromium sources and the spec 6265bis-05 a "naked" So we a) should document the issue with SameSiteDefaultMode but b) we do not need to change how SameSiteDefaultMode is serialized (as it will be treated as None). |
Yeah, that version sorta implies it's okay, but then section 4.1 makes it clear that it expects a I think overall what you've said makes sense, though with two additional caveats:
The reason I became aware of this issue in the first place is that we had users that ran into cookie issues with our site due to an incidental use of Because of the above, it seems like serializing it as nothing, rather than as I think the best thing to do is add a comment indicating you should never use that const, and should rather use I'm happy to open a CR for that if it sounds good. |
…o cookie at all. See golang/go#36990
…o cookie at all. See golang/go#36990
Drive-by comment: I would also be inclined to make it serialize as nothing. There is not benefit in adding (I got here because I found this issue myself and was about to open a duplicate) |
In packages that target a moving platform, like net/http and the Web Platform or crypto/x509 and the Web PKI, we should interpret the backwards compatibility promise in terms of behaviour, not wire output. So agreed that if the naked option was dropped and the way to get the default is to serialise nothing, then that option should be changed to serialise nothing. |
Change https://golang.org/cl/256498 mentions this issue: |
@empijei @FiloSottile @euank I disagree. This would downgrade security for older browsers like iOS Safari, as you can see they interpret missing attribute value as I would suggest to keep the old behavior (revert this patch), despite the name, it is not a real default so it was used deliberately and let the browsers interpret the attribute. Maybe consider #39610 to have a name for the real default (zero value) too. |
@pjediny I filed this issue because gorilla/csrf assumed the 'Default' value was the right value for backwards compatibility, which led to a regression in some code using it when we upgraded go. I don't think people were deliberately using the 'Default' value to get 'Lax' on some browsers (newer chrome/ff I think), 'None' on other browsers (older chrome/ff), 'strict' on other browsers (apparently ios), and silently drop the cookie on yet other browsers (some older chrome/android). That was the previous behavior of 'Default' if I understand all this correctly. If people deliberately wanted to set 'Strict', they would have set 'Strict'. |
@euank So instead of fixing gorilla/csrf you would rather change the behavior for all and instead of browser default interpretation you would enforce behavior that suits your case with no regard to backward compatibility. If the people didn't want SameSite attribute without a value to be set, they should have not set it. While I agree that the situation is quite a mess and the name is unfortunate and confusing, changing the behavior to something else adds to this mess too. Now we need to keep track of not only browser versions/implementations but the go versions too.
On the other hand, it's old draft vs new draft... |
I think we're on a similar page overall, @pjediny. gorilla's use was fixed, I'm just pointing out that the value 'Default' as it was implemented before has caused actual breakages for people. I agree there's some bad cases with either option. You're right that this is a subtly backwards incompatible change, though for most cases it seems like it'll be a strict improvement, and other parts of the http stack make technically-backwards-incompatible-changes all the time (like To me, the most compelling reason is that |
I see. We agree, but we disagree on how a solution to such problem should look like. The biggest issue is the confusing mode name, but maybe there were some other solutions to this problem:
Default is probably now closer to what one would expect, but I find it in some ways still a little confusing. I'm not sure if it can revert the decision or not, but I probably cannot do much more than to state these impacts and propose other solutions. Thanks for patience anyway. |
My 2c on this:
|
Safari&webviews on iOS 12 (iPhone6 latest iOS). The 'none' attribute value is not supported and the browser handles it as 'strict' instead...
I was thinking about this a little bit more, currently I have something close to proposal: Cookie write mode
Cookie read mode
What do you think about it?
If there is an agreement, I'm willing to create the CL. |
I don't see why anyone would ever want this behavior, and I don't think we need to support it. We should definitely update the spec reference in the docs. Deprecating SameSiteDefaultMode is also a good idea, but we are not going to remove it. Deprecation is how we tell applications they need to actively move away from something. A name for the zero value also sounds good. I'd call it SameSiteUnset, without Mode. When parsing we should do what modern browsers do. What is that? |
Modern browsers ignore any +1 on Deprecating If this is good enough for you, you might want to try and take a stab at this. |
This is a little bit unfair, it is mixing standard evolution steps (that will stay) and browser bugs.
I do see a little bit of a value for compliance/pen testing tools. I understand it is not a common use, so you will probably not want to support it, but it was possible to use it before and now there is no clear path to replace it. Maybe there are some other use cases, that we don't know about.
Maybe the https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.3.7 is now the appropriate reference.
This will need a clarification
I think this is a good idea. I was thinking about it, but the all the names are of the
Standard says unknown values should enforce 'None' behavior. There are multiple issues with this:
I believe this is incorrect. See above, default for unknown is
I strongly disagree. As example can be Google making this mess with While I agree with the goal, the way it was forced feels just wrong. It was in the situation when the standard draft was hastily edited (and now it shows how ambiguous and underspecified it is), the problem it was handling had already known mitigations deployed. I'm not sure what makes this inconsiderate behavior so seductive in Google, it may be the mono repo development style or the Chrome marketshare of the web platform, but it is causing problems for external vendors.
If there is no rush, let's wait for replies for few days, maybe new draft. I'm now thinking about enforcement and attribute value separation in read/write modes and still want to review how the cookie jars are handled. How much do we hurry/can we get this in go1.16? |
The fact that on Apple browsers SameSite=None behaved as SameSite=Lax was a bug, it's not really about backporting but more like Apple not really caring about what FF and Chrome do or what the spec says (aka: unknown values should be ignored). Nowadays Safari enforces something similar to SameSite=Lax by default and they pushed it to all users in a completely careless way, not putting much thought about breakages and bragging about having solved CSRF (which they didn't). They can because the user base is small and people can always count on compliant, backup browsers in case stuff doesn't work.
Yes, it was because the breakage would have been too big since Apple was lagging too much behind (two full years) the current spec and, unlike the expectation towards Safari, people rely on Chrome to always work.
My personal guess: trying to protect users from CSRF in a widespread way, since the issue has been there for very long it would just be inconsiderate to leave that dangerous issue there when there is a way for user agents to fix it (exactly like Safari did, please see the the links above). Please note this is a personal guess and not an official Google stance.
I am fine with rolling back my change and keep discussing this here, I am not in a rush to get this fixed and I would like to find something that works for everyone. If I get this right your proposal is:
My question would be: if someone wrote a server 3 years ago and used |
@FiloSottile @empijei I am confused about the state of this issue and want to make sure we don't make a change in Go 1.16 that we do not intend to make. Should https://go-review.googlesource.com/c/go/+/256498 be rolled back? |
We could roll that back until we reach a consensus or fix-forward. This is not a big issue either way. |
There is a new development regarding this. New rfc6265bis draft (07) introduces a |
(The next paragraph says the algorithm maps unknown values to "None" but it clearly maps to "Default", so I am going to assume it's an editing mistake.) Awesome. This looks like exactly our post-CL 256498 behavior, and the fact that the spec is calling this Default probably saves us renaming/deprecating it. So in theory we are now spec compliant. In practice, the current behavior also seems to match what applications would want (as opposed to risking Strict on iOS, dropped on some browsers, and Default on the rest). I think CL 256498 is fine, and there is no more work left. Please correct me if I'm wrong. |
Unknown should map to
|
httpwg/http-extensions#1325 looks very clear to me that unknown maps to Default, and they just omitted to change the paragraph below the algorithm. Opened httpwg/http-extensions#1339 to clarify. |
Yop, you are probably right about this one. Citing the https://tools.ietf.org/html/draft-west-cookie-incrementalism-01#section-3.1
Still, it's a user agent perspective, the server side could be more flexible. |
I'd rather err on the side of least surprise and of better security. If the application uses |
For what it's worth, I think the already-merged CL 256498 is my preferred solution. Thanks for the help getting this fixed, @FiloSottile! |
I have this feeling we misunderstand each other, I'm afraid we somehow mix the mode request and user agent enforcement and IMO you lean too much on the UA side interpretation. I'm not aware I was requesting We only have three modes of UA behavior: I still disagree, but it looks like you converged on already merged solution and the SameSite w/o value is not supported by current specs and usecase is kind of marginal so I will stop nagging people on this issue. Thank you and I hope I did not waste too much of your time. |
You definitely didn't waste anyone's time, thank you for the discussion, it helped make sure we all understood what was going on, even if we did not end up agreeing on the exact same outcome. |
Change https://golang.org/cl/288274 mentions this issue: |
Problem
Currently,
http.SameSite
offers thehttp.SameSiteDefaultMode
option.This is handled by code here:
go/src/net/http/cookie.go
Lines 220 to 229 in 07ccdeb
For
SameSiteDefaultMode
, it addsSameSite
(as a bare key, no=
, no value) to the cookie.On older versions of chrome, this results in the cookie being dropped entirely.
The draft specification for same-site makes it clear now that dropping the entire cookie is incorrect, but previously it implied this was correct behavior, so I wouldn't be surprised if this issue existed elsewhere too.
If the current draft spec is implemented correctly,
SameSite
would be parsed as an unrecognised value, and would then result in the samesite attribute it being ignored entirely (that is to say, being treated the same ashttp.SameSite(0)
where nothing is added to theSet-Cookie
header).It seems a bit silly to include an option in the http library to generate an invalid cookie attribute which the browser should just ignore.
Possible Solutions
The 2 solutions that make sense to me are the following:
SameSiteDefaultMode
and recommend not using it at all.SameSiteDefaultMode
be the same ashttp.SameSite(0)
, where noSameSite
attribute is included at all (which causes the browser to default to None or Lax, depending on version/browser).I think option 2 ends up most closely matching what people would expect
SameSiteDefaultMode
to do. Technically it's a backwards incompatible change, which is the only reason I think we may wish to go with option 1 instead.The text was updated successfully, but these errors were encountered: