Skip to content

allow explicit SameSite=None cookies #1282

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

Merged
merged 1 commit into from
Jan 23, 2020
Merged

allow explicit SameSite=None cookies #1282

merged 1 commit into from
Jan 23, 2020

Conversation

robjtede
Copy link
Member

@robjtede robjtede commented Jan 16, 2020

Given the upcoming changes in Chrome to default SameSite-less cookies to Lax, we should allow the option to explicity set None cookies.

This could be considered non-breaking while Chrome's changes are still in beta.

It's also possible this change should be packported to 0.x and 1.x branches.

fixes #1035

@robjtede robjtede force-pushed the fix/same-site-none branch 2 times, most recently from 3f1600f to fb2c36a Compare January 16, 2020 13:54
@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #1282 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   80.47%   80.46%   -0.02%     
==========================================
  Files         159      159              
  Lines       18541    18540       -1     
==========================================
- Hits        14921    14918       -3     
- Misses       3620     3622       +2
Impacted Files Coverage Δ
actix-http/src/cookie/mod.rs 92.57% <100%> (-0.04%) ⬇️
actix-http/src/cookie/draft.rs 23.52% <100%> (-11.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 412e54c...464db0c. Read the comment docs.

@fafhrd91
Copy link
Member

Could you add entry to changes.md
Also we should mention chia change in migration.md, technically this is breaking change

@robjtede
Copy link
Member Author

IMHO this should not be considered a breaking change, but a bug fix. Anyone using .same_site(SameSite::None) and expecting the same behavior as not setting it has been relying on a bug.

@fafhrd91
Copy link
Member

this is breaking change even if original behavior is wrong. we have to mention change in default behavior in migration.md

@robjtede
Copy link
Member Author

the migration.md file mentions the change now

@robjtede robjtede closed this Jan 20, 2020
@robjtede robjtede reopened this Jan 20, 2020
@codecov-io
Copy link

codecov-io commented Jan 20, 2020

Codecov Report

Merging #1282 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   80.47%   80.46%   -0.02%     
==========================================
  Files         159      159              
  Lines       18541    18540       -1     
==========================================
- Hits        14921    14918       -3     
- Misses       3620     3622       +2
Impacted Files Coverage Δ
actix-http/src/cookie/mod.rs 92.57% <100%> (-0.04%) ⬇️
actix-http/src/cookie/draft.rs 23.52% <100%> (-11.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e9ab06...8182fa0. Read the comment docs.

@cetra3
Copy link
Contributor

cetra3 commented Jan 20, 2020

There are more browsers than just chrome & there should be a way to have no samesite (existing behaviour) or a samesite with a value of None.

@robjtede
Copy link
Member Author

robjtede commented Jan 20, 2020

@cetra3 omitting the call to set samesite will result in no samesite flag being sent in the set-cookie header

eg. most of the tests dont set it and pass without it being asserted

the issue is that None is being treated the same as Some(SameSite::None) which will be incorrect

@robjtede robjtede requested a review from a team January 20, 2020 22:02
@cetra3
Copy link
Contributor

cetra3 commented Jan 20, 2020

Ah right, I don't have an issue with it if that is the case (I.e, if it's not set there is no change in behaviour)

@JohnTitor JohnTitor merged commit a328794 into master Jan 23, 2020
@JohnTitor
Copy link
Member

Thanks!

@robjtede robjtede deleted the fix/same-site-none branch January 23, 2020 01:14
theikkila pushed a commit to theikkila/actix-web that referenced this pull request Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SameSite value "None" cookie attribute
5 participants