Allows samesite='none' in set_cookie

[marc1n]

#406

[stevepiercy]

Please do not change the case of values.

>>> b"Strict" == b"strict"
False

For consistency, I recommend retaining the original casing.

[stevepiercy]

Black is required to format code in WebOb.

[marc1n]

I've changed case in doc for better visual distinction between "none" and None.

According to the cookie spec SameSite attribute value is case-insensitive.

[digitalresistor]

According to the specs if the value "None" is used for the same site attribute then the cookie must be marked as secure or it will be ignored by browsers.

Do we want to handle that invariant?

[digitalresistor]

@marc1n would you also kindly remove the merge commit by rebasing this work on top of master?

[marc1n]

I tried but I failed. My knowledge of git is too limited. What steps should I do to remove the merge commit?

[digitalresistor]

@marc1n you should be able to run:

git rebase -i a5712d73498be29b03d4d15dfcee21c2ed45ce1a

when the editor appears, exit the editor without modifying anything.

Then force push the results to your Github repo:

git push --force-with-lease

This will overwrite the copy on Github with your fixed local copy, this PR will then automatically update. Thanks!

[marc1n]

@bertjwregeer Done, thanks for help!

[marc1n]

According to the specs if the value "None" is used for the same site attribute then the cookie must be marked as secure or it will be ignored by browsers.

Do we want to handle that invariant?

I think "yes", but I am not sure which is better: to raise exception if secure=True is not set, or automatically set it to True.

[mmerickel]

to raise exception if secure=True is not set, or automatically set it to True.

I would raise a ValueError explaining the incompatible arguments.

[digitalresistor]

I would be happy to accept this PR with the suggestion that @mmerickel made. I think that is the best way moving forward to make sure that people get the behavior they are expecting.

[jvanasco]

This change in Chrome caused a series of issues for us.

While I support this PR, and am very eager for it to be merged/released ASAP, I want to suggest that a future version does not raise an exception after checking the SameSite arguments or their compatibility with the secure flag - as that could suddenly change again.

Perhaps an environment variable could be used to toggle between warning with a deprecation/log or raising an exception. The default can absolutely be raising an exception, but there should be an option to easily avoid these checks if the cookie spec changes again or a major browser vendor decides to suddenly force a new behavior or keyword value.

I love that WebOb/Pyramid are helping us "do the right thing", but I am worried about future sudden changes by browser vendors.

[digitalresistor]

There's a reason why I marked this as experimental in WebOb. I am all for security improvements, but the fact remains that Google and Chrome are steamrolling the various communities and forcing them to adapt ever changing standards.

The draft that was linked above expired "November 8, 2019" and the fact that Chrome is steam-rolling ahead with the change without further consent from the internet community is absolutely terrible.

[digitalresistor]

I highly doubt that Google is going to relax the requirement for SameSite=None to not also be Secure, so having that check in the codebase is fine by me.

I will not be adding environment checks to WebOb, I may provide a flag in the future that can be set on a module, but no environment variables. That sort of behaviour has bitten me far too many times in the past (looking at oauthlib...) in other libraries that changed core behaviour depending on environment variables that are not well documented/not well known.

[jvanasco]

I will not be adding environment checks to WebOb, I may provide a flag in the future that can be set on a module, but no environment variables. That sort of behaviour has bitten me far too many times in the past (looking at oauthlib...) in other libraries that changed core behaviour depending on environment variables that are not well documented/not well known.

That's a great point. I prefer toggling a flag in a module too, but I've seen more people advocate for environment variables so went with that (also, probably oauthlib). I'd be happy to submit a draft PR that uses that approach.

I have a bunch of whitelabel sites that communicate with a centralized domain via javascript + iframes (like facebook and twitter buttons). The Chrome change completely broke this functionality and our overall user experience over a very large chunk of traffic. This was hotfixed with a patch in the deployment routine, but I really hate this approach.

[digitalresistor]

I'd be open to a draft PR that adds that, although I am not sure it is worth adding the change for something that I think is very unlikely to change in the future. We should be moving towards a more HTTPS web anyway, and the Secure flag on cookies should be the norm.

[jvanasco]

I'm just going to use the flag to disable checking the value matches a b

…

Sent from my iPhone

On Dec 19, 2019, at 8:14 AM, Bert JW Regeer ***@***.***> wrote: I'd be open to a draft PR that adds that, although I am not sure it is worth adding the change for something that I think is very unlikely to change in the future. We should be moving towards a more HTTPS web anyway, and the Secure flag on cookies should be the norm. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[jvanasco]

Understood. I was just going to draft a PR to support disabling the check on the samesite value , in case a new one is added or they start tests again (there were some other values tested at one point).

[digitalresistor]

Oh... I guess that could make sense. Sure, start a PR for that change.