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

Update alert banner cookie to set samesite="lax" #240

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

rgcarr
Copy link
Contributor

@rgcarr rgcarr commented Mar 3, 2023

Added SameSite default setting to Cookie

Added SameSite default setting to Cookie
@andybroomfield andybroomfield self-requested a review March 6, 2023 11:34
Copy link
Contributor

@andybroomfield andybroomfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @rgcarr

I've tested and the banner still hides, and I'm no longer seeing a warning in firefox consiole.
We should review at some point if 'lax' is the right value here, and if secure (which EU cookie complicance uses) would be better. Also need to look at if we should send secure if under https. These can be looked at as seperate issues though.

@andybroomfield
Copy link
Contributor

@rgcarr
Copy link
Contributor Author

rgcarr commented Mar 6, 2023

I did some testing with SameSite: None (which seemed most flexible option), but you have to implement Secure... and didn't think we can assume that a site is always running HTTPS (errors if Cookie is "Secure' but running HTTP). Lax seemed to be path of least resistance.

@andybroomfield andybroomfield changed the title Update alert_banner.js Update alert banner cookie to set samesite="lax" Mar 6, 2023
@andybroomfield andybroomfield merged commit 183c2d9 into localgovdrupal:1.x Mar 6, 2023
@ekes
Copy link
Member

ekes commented Mar 7, 2023

And on the same day https://www.drupal.org/node/3275352 "Drupal now defaults to "Lax" for the SameSite session cookie attribute, and checks for valid values. Introduced in version:
10.1.0" 😄

@rgcarr rgcarr deleted the patch-2 branch March 7, 2023 10:00
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.

3 participants