-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Conversation
Also contains an update for the cookie library dependency
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, 👍 |
Only one thought: is there any place where we could add a unit test to confirm that the cookie returns samesite=lax as expected? |
Wouldn't that break some use-cases for https://github.com/flarum/embed? |
[ci skip] [skip ci]
I'd still like to see a unit test for this, but otherwise looks great! |
@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. |
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. |
Co-authored-by: Daniël Klabbers <luceos@users.noreply.github.com>
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 |
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:
Confirmed
- [ ] Frontend changes: tested on a local Flarum installation.composer test
).