Skip to content

Cookie same-site#2630

Merged
Alkarex merged 3 commits intoFreshRSS:devfrom
Alkarex:cookie-same-site
Nov 5, 2019
Merged

Cookie same-site#2630
Alkarex merged 3 commits intoFreshRSS:devfrom
Alkarex:cookie-same-site

Conversation

@Alkarex Alkarex added this to the 1.15.1 milestone Nov 4, 2019
@Alkarex
Copy link
Member Author

Alkarex commented Nov 4, 2019

Assuming we use form-based login, with a cookie:

  • With Lax , we can link from another domain to any FreshRSS page, e.g. direct link to a category. It is a relatively minor change compared to what we have now. I believe it will be the default from Chrome 80.
  • With Strict, we can only link to FreshRSS root (otherwise one has to log-in again), but it is a safer policy. Being able to link to the root at all is made possible thanks to the cookie isolation we have with /i/ and its redirection

Thoughts?

@Alkarex
Copy link
Member Author

Alkarex commented Nov 4, 2019

After thinking a bit, I believe that Lax would be safer for a x.1 release, and we can discuss the possibly upgrade to Strict for the next bigger release, e.g. 1.16.0

@marienfressinaud
Copy link
Member

I don't have the motivation to think about it tonight… but it is important to have for the next 1.15.1?

@Alkarex
Copy link
Member Author

Alkarex commented Nov 4, 2019

Lax should be quite safe, especially since it is becoming the default value anyway.
But it can wait a bit 🙂

@marienfressinaud
Copy link
Member

Ok, I gave a thought about this. If I understand, in Strict mode, if any external site link to, let's say the configuration page, the user will have to log in again. If so, I'm not really for the Strict mode and I would stay with Lax.

Besides this opinion, Lax looks safe indeed and I'm now inclined to merge this PR for the 1.15.1 ;)

@Frenzie
Copy link
Member

Frenzie commented Nov 5, 2019

If I understand @Alkarex's summary correctly (don't have time to read the provided links atm) then strict would break functionality like the bookmarklet?

@Alkarex
Copy link
Member Author

Alkarex commented Nov 5, 2019

@Frenzie That is correct, I did not think about it

@Alkarex Alkarex merged commit 8b0f9fa into FreshRSS:dev Nov 5, 2019
@Alkarex Alkarex deleted the cookie-same-site branch November 5, 2019 17:11
@Alkarex Alkarex mentioned this pull request Nov 6, 2019
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Jan 18, 2026
Follow-up of FreshRSS#2630 now that we have PHP 7.3+ (even PHP 8.1+).
* The new syntax natively supports `samesite`, and also avoids the need of re-setting all parameters.
* Use automatic path instead of own function `getCookieDir()`.

Follow-up of FreshRSS#8446
* Sanitize lifetime of session cookies from PHP ini to avoid likely invalid/misunderstood values
Alkarex added a commit that referenced this pull request Jan 28, 2026
Follow-up of #2630 now that we have PHP 7.3+ (even PHP 8.1+).
* The new syntax natively supports `samesite`, and also avoids the need of re-setting all parameters.
* Use automatic path instead of own function `getCookieDir()`.

Follow-up of #8446
* Sanitize lifetime of session cookies from PHP ini to avoid likely invalid/misunderstood values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants