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

Support the SameSite field in Cookies #255

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@dstufft
Copy link
Contributor

dstufft commented Jun 13, 2016

See https://tools.ietf.org/html/draft-west-first-party-cookies-07 and https://www.chromestatus.com/feature/4672634709082112 for more information.

This is shipping in Chrome since version 51, Opera since 39, positive support in Firefox.

@dstufft

This comment has been minimized.

Copy link
Contributor

dstufft commented Jun 13, 2016

If this is something WebOb is interested in having, let me know and I can write tests for it.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jun 13, 2016

I added an open ticket here: #246

@dstufft

This comment has been minimized.

Copy link
Contributor

dstufft commented Jun 13, 2016

hurf durf, I am good at computer.

So I guess the question is, are you happy shipping a draft or do you want to wait until the RFC is finalized? If you're OK shipping a draft, is there any sort of remark we should make about it being a draft?

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jun 13, 2016

Hit comment too soon:

So yes I am interested in having it in WebOb, but in a capacity whereby it is optional, and marked as a draft spec. I tend to avoid adding draft's to libraries that are widely used because they can change too often and could potentially break things in the future.

If we do add it, it should be commented as such, and should link back to the draft version that is implemented. We should also add a way to verify the values passed so that cookies sent are valid, and you can't set SameSite to an invalid value.

If added to make_cookie() it should also be exposed in the CookieProfile, so that if you use the newer API for handling cookies within WebOb you get access to the same capabilities as using the set_cookie() API.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jun 13, 2016

I would also want to add a page to documentation that documents what if any draft specs are implemented.

@dstufft dstufft force-pushed the dstufft:same-site-cookies branch from b6806ef to caf229c Jun 15, 2016

@dstufft

This comment has been minimized.

Copy link
Contributor

dstufft commented Jun 15, 2016

This now has tests and was added to CookieProfile and invalid values raise a ValueError instead of silently working.

I still need to surface the documentation you wanted, but I'm not sure where you want the comment that this is a draft and the link to be put. Looking at the documentation, I'd guess you want it as part of the docstrings in the relevant methods where the samesite kwarg has been added, is that correct?

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jun 15, 2016

I think this is a new page that should be added to the docs.

There are some other patches that have been languishing because I tend to not want to include anything that is not official or complete (especially RFC wise), because it can become a maintenance nightmare, but at the same time I would like to include them in a future release because they allow for some neat features.

For example: #180 (although it needs some more work to be functional within WebOb...)

I'd like it documented in the doc string too, of course, but a seperate page named "Experimental" or something along those lines that I can point people to would be fantastic.

@bertjwregeer bertjwregeer added this to the 1.7.0 milestone Jun 27, 2016

@bertjwregeer bertjwregeer removed this from the 1.7.0 milestone Nov 16, 2016

@bertjwregeer bertjwregeer referenced this pull request May 22, 2017

Merged

Replacement for #255 #316

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented May 22, 2017

Closing for #316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment