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

Fixes #2157, Explicitly set SameSite value for cookies #2159

Merged
merged 5 commits into from
Jun 4, 2020

Conversation

tankerkiller125
Copy link
Contributor

Also contains an update for the cookie library dependency

**Fixes #2157 **

Changes proposed in this pull request:

Updates the cookie library and explicitly sets the same site value

Reviewers should focus on:

  • Is Lax the value we want to set? None requires HTTPs and Strict would prevent cookie use on a primary/sub-domain

Confirmed

- [ ] Frontend changes: tested on a local Flarum installation.

  • Backend changes: tests are green (run composer test).

Also contains an update for the cookie library dependency
@askvortsov1
Copy link
Member

As per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite, Lax seems to be the default value, so I think lax is best for now (this way we just avoid browsers complaining about no explicit setting).

We can consider hardening this, as well as the stuff in #353 and https://discuss.flarum.org/d/22642-security-roadmap, later on. For now, 👍

@askvortsov1
Copy link
Member

Only one thought: is there any place where we could add a unit test to confirm that the cookie returns samesite=lax as expected?

@rob006
Copy link
Contributor

rob006 commented May 9, 2020

Wouldn't that break some use-cases for https://github.com/flarum/embed?

@askvortsov1
Copy link
Member

I'd still like to see a unit test for this, but otherwise looks great!

@tankerkiller125
Copy link
Contributor Author

@askvortsov1 Any suggestions as to where in the testing this would fit? Not sure if we would classify it as a unit test or integration test.

@askvortsov1
Copy link
Member

It seems from the cookie library code that the actual cookies are resolved by calling toString on them ( (string) $cookie)), so perhaps in a unit test, we could assert that the proper samesite value is present in the resolved cookie string? I wouldn't mind an integration test as well, but I'd hold off on that until we have more of other stuff we can test.

src/Http/CookieFactory.php Outdated Show resolved Hide resolved
Co-authored-by: Daniël Klabbers <[email protected]>
@tankerkiller125
Copy link
Contributor Author

Got an error with the example, code from luceos. Actually found an even nicer way of doing it I think but I need to get my unit test working properly first on my laptop. If it does work it will be SameSite::{$this->samesite ?? 'lax'}() which I think is much easier to understand and looks better. (Supported syntax in 7.0 and up)

@askvortsov1 askvortsov1 merged commit 293e225 into master Jun 4, 2020
@askvortsov1 askvortsov1 deleted the mk/2157-fix-cookies branch June 4, 2020 02:53
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.

Flarum gives a cookie warning in console
4 participants