Skip to content
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: net/http: add constant for samesite cookie mode #39609

Open
pjediny opened this issue Jun 16, 2020 · 9 comments · May be fixed by #39610
Open

proposal: net/http: add constant for samesite cookie mode #39609

pjediny opened this issue Jun 16, 2020 · 9 comments · May be fixed by #39610

Comments

@pjediny
Copy link
Contributor

@pjediny pjediny commented Jun 16, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.4 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

not relevant

What did you do?

I wanted to specify cookie samesite mode that would indicate cookie without a samesite attribute while implementing ory/hydra#1908 that requires cookie without any samesite attribute.

What did you expect to see?

I expected to find the samesite mode const with such behavior in the net/http package

What did you see instead?

I had to use unnamed magic constant (0)

@pjediny pjediny linked a pull request that will close this issue Jun 16, 2020
@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 16, 2020

Why do we need a constant that has the same value as the zero value for that type? Why does explicitly saying "the thing isn't set" improve over just not setting it?

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 16, 2020

Change https://golang.org/cl/237998 mentions this issue: net/http: add SameSiteUnsetMode const

@pjediny
Copy link
Contributor Author

@pjediny pjediny commented Jun 16, 2020

@davecheney In my opinion it mainly improves code readability in some situations. The behavior is already there, but the name is not, so to invoke such behavior I need to pass 0.

For example you can have a configuration with SameSite mode and you don't want to have multiple ways to construct a cookie (with or without the samesite attribute). For example see the ory/hydra#1908 commit.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 16, 2020

Thanks for the example. That API is pretty hard to use correctly, especially as there is no safe default for that parameter. Changing Go might be a solution to that, but there might be others which would lead to a more usable API.

@pjediny
Copy link
Contributor Author

@pjediny pjediny commented Jun 17, 2020

@davecheney Agreed, the cookie samesite API is hard to use and one of the small problems is that there is behavior that does not have a name, yet it is used. The other is that SameSiteDefaultMode is a name confusing people, as the mode is not a safe default, nor default, nor safe. The first one is easy to fix, but I'm not sure how to fix the other one without breaking the compatibility. You suggest there might be other ways to enhance the API, can you be more specific?

@andybons andybons changed the title net/http: missing constant for samesite cookie mode proposal: net/http: add constant for samesite cookie mode Jun 17, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 17, 2020
@gopherbot gopherbot added the Proposal label Jun 17, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 11, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 11, 2020

@rsc
Copy link
Contributor

@rsc rsc commented Nov 11, 2020

I'm a bit confused about SameSiteUnsetMode vs SameSiteDefaultMode vs SameSiteNoneMode.
(I know what they mean, I just find the names confusing.)
It seems like leaving 0 for "there's just no SameSite at all" might be clearer.

Also, based on the change made for #36990, the proposed SameSiteUnsetMode would be exactly the same as SameSiteDefaultMode, which is even more confusing.

It's unfortunate that we now have 0 and 1 meaning the same thing, but since 1 has a name, it seems like we should leave it there and not name 0 as well. If you want to be explicit about nothing happening, you can use the named 1 instead of a named 0.

@rsc rsc moved this from Incoming to Active in Proposals Nov 11, 2020
@pjediny
Copy link
Contributor Author

@pjediny pjediny commented Nov 11, 2020

@rsc Yes, the names are confusing, I'm currently discussing the changes for #36990. @FiloSottile proposed instead of SameSiteUnset instead of SameSiteUnsetMode and I think it's a step in the right direction, helping to distinguish the zero value from samesite modes.

The idea is to revert the #36990 change and deprecate the SameSiteDefaultMode instead so the user who rely on the old spec behavior are not surprised by the change. Default, None and Unset would be different thing.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

I'm still confused about the state of #36990. Until that gets reverted, it doesn't make sense to have two different named constants with the same behavior. Let's wait on this until #36990 settles.

@rsc rsc moved this from Active to Hold in Proposals Nov 18, 2020
@rsc rsc added the Proposal-Hold label Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants