I'm more interested in this one now after replying to your comment on Bug 25360.

I figure CGISESSID should be SameSite=Lax, but maybe other cookies could be SameSite=Strict.

That being said... even things like "intranet_bib_list" it would be good to send as SameSite=Lax, because they affect the display of the landing page if you're already authenticated and you're going to /cgi-bin/koha/catalogue/search.pl?q=test (in say a new tab for instance).

I imagine this one will need a lot of thinking...

Why wouldnt we add a preference like SameSiteCookie to include cookie names that do not want to default to Lax ?

So e.g. SameSiteCookie = cookieA:None, cookieB:Strict
CookieA and CookieB should respond to the pref and the other ones default to Lax? Which is becoming the behavior in most browsers? Or even add the fallback in the pref itself?

We could add a wrapper around CGI->cookie to set it.
Koha::Cookie->new({ attributes })->generate ?

(In reply to Marcel de Rooy from comment #2)
> Why wouldnt we add a preference like SameSiteCookie to include cookie names
> that do not want to default to Lax ?

Why should we let librarians determine cookie settings? It seems to me that we as developers are best suited to making those choices?

(In reply to David Cook from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > Why wouldnt we add a preference like SameSiteCookie to include cookie names
> > that do not want to default to Lax ?
> 
> Why should we let librarians determine cookie settings? It seems to me that
> we as developers are best suited to making those choices?

I agree that most librarians would not. But we have more preferences that should be set by sysadmins. But surely, we could move them to koha-conf.xml. Risking the argument that people want to change them and have no access..

(In reply to David Cook from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > Why wouldnt we add a preference like SameSiteCookie to include cookie names
> > that do not want to default to Lax ?
> 
> Why should we let librarians determine cookie settings? It seems to me that
> we as developers are best suited to making those choices?

I think we should provide a sane default, and probably have a separate tab for 'Risky area'.

But what's the use case for a Koha staff user changing the SameSite value for a cookie?

Due to deep linking (e.g. linking to a search result page and visiting it as an authenticated user), I can't think of a case off the top of my head that shoulnd't be SameSite=Lax. 

With SameSite=None, we'd be letting any site send that cookie. I can't see any reason to do that. We wouldn't be creating tracking cookies, and I don't know why we'd let another site send a cookie to Koha via a background call.

SameSite=Strict sounds good in theory for internal cookie usage, but - due to that deep linking I mentioned - every cookie I can think of should be sendable when externally navigating to the site. That said, I'd be willing to test this theory to try to prove it wrong. I have a feeling that using SameSite=Strict would break a lot of Koha functionality when navigating directly to a page (like search results), but I'm happy to be proven wrong.

So I'd argue it's not just a case of people creating a security risk by using SameSite=None, but also a case of people breaking things by using SameSite=Strict, which really just leaves SameSite=Lax, which is why that's the default in browsers I imagine. 

SameSite=Lax is a sane default.

SameSite=None has no obvious use to me for Koha.

SameSite=Strict... maybe but probably not. The exception being if we re-routed people through a login page for new navigations, even when authenticated. If someone clicked a link in their email for http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=test, and they already had a valid Koha session for localhost:8081, we could bounce them through http://localhost:8081/cgi-bin/koha/mainpage.pl, which would then allow you to use SameSite=Strict. 

But I doubt we're going to change how we handle authentication and navigation just to use SameSite=Strict.

I've actually been looking for cookies on sites I use, and for the most part I don't see any actually setting SameSite. (Of course, many of the sites are using ServiceWorker, and at a glance it's not obvious what it's doing in the background.)

One exception is a load balancer stickness CORS cookie, AWSALBCORS, which has "SameSite=None". (More info available at https://forums.aws.amazon.com/ann.jspa?annID=7413)

Noticing a "SameSite=None" cookie for www.google.com which is a tracking/targeting/advertising cookie.

I notice some console errors saying "A cookie associated with a cross-site resource at http://youtube.com/ was set without the `SameSite` attribute. A future release of Chrome will only deliver cookies with cross-site requests if they are set... with `SameSite=None` and `Secure`."

Is *this* the warning mentioned in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26019#c0? 

If so, I think we can safely ignore it, except in cases where we're doing Cross-Origin Resource Sharing (CORS) requests, but I don't see why we'd be passing a cookie with a CORS request (unless you were doing a CORS request to the API with a cookie but that seems problematic and unnecessary since you can use OAuth2 or Basic Auth for the API).

Ahhh yes I assume that must be what this is about?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#Fixing_common_warnings

If the browser is showing this warning for Koha, then we should look at that particular cookie and its usage. Otherwise, I don't argue that no changes are necessary.

Would it be a logical first step to explicitly set SameSite to Lax for the cookies we set without a SameSite attribute?

Just a faint remark on:

+            -sameSite => 'Lax'

I always tend to end parameter lines with a comma. Easier to add another one.. Does not mean that it needs correction now.

Created attachment 132804 [details] [review]
Bug 26019: Koha should set SameSite attribute on cookies

This patch modifies the way Koha sets cookies so that the "sameSite"
attribute is explicitly set to "Lax." This option is chosen because it
is the value which is currently assumed by browsers when the sameSite
attribute is not set.

To test, apply the patch and restart services.

- Log in to the staff interface and open your browser's developer tools.
  - In Firefox, look for a "Storage" tab.
  - In Chrome, look for an "Application" tab.
- Under "Cookies," click the URL of the staff interface.
- You should see all the cookies which are set for that domain.
- The CGISESSID cookie should have sameSite set to "Lax."

- Go to Cataloging -> New record.
  - Check the "marcdocs" and "marctags" cookies.
- Switch to the Advanced MARC editor (you may need to enable
  theEnableAdvancedCatalogingEditor preference).
  - Check the "catalogue_editor" cookie.
- Add a new item to an existing bibliographic record.
  - Check the "LastCreatedItem" cookie which is set after you save the
    new item.
- Go to Authorities -> Authority search.
  - In authority search results, click "Merge" from the "Actions" menu
    next to one of the results..
    - Check the "auth_to_merge" cookie.
- Go to Administration -> MARC bibliographic framework
  - Choose "MARC structure" from the menu corresponding to one of the
    frameworks.
  - Check the "Display only used tags/subfields" checkbox.
    - Check the "marctagstructure_selectdisplay" cookie.
- Go to Circulation -> Check out to a patron with checkouts.
  - Check the "Always show checkouts immediately" checkbox.
    - Check the "issues-table-load-immediately-circulation" cookie.
- Go to Tools -> Patron clubs. You will need at least one active club
  with one or more patrons enrolled.
  - From the list of clubs, click Actions -> Search to hold.
    - Check the "holdforclub" cookie.
- Go to Tools -> Batch item modification and submit a batch of items.
  - Uncheck one or more checkboxes in the "Show/hide columns" area.
    - Check the "showColumns" cookie.
- View a patron -> Search to hold.
  - Check the 'holdfor' cookie.
- With WebBasedSelfCheck enabled, log in to the self-checkout page.
  - Check the "JWT" cookie.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 132805 [details] [review]
Bug 26019: Add two other js cookies

Test plan:
Same as former patch. Use OPAC adv search here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Sorry does not apply anymore, maybe because of Bug 29957.
Please rebase

Created attachment 133212 [details] [review]
Bug 26019: Koha should set SameSite attribute on cookies

This patch modifies the way Koha sets cookies so that the "sameSite"
attribute is explicitly set to "Lax." This option is chosen because it
is the value which is currently assumed by browsers when the sameSite
attribute is not set.

To test, apply the patch and restart services.

- Log in to the staff interface and open your browser's developer tools.
  - In Firefox, look for a "Storage" tab.
  - In Chrome, look for an "Application" tab.
- Under "Cookies," click the URL of the staff interface.
- You should see all the cookies which are set for that domain.
- The CGISESSID cookie should have sameSite set to "Lax."

- Go to Cataloging -> New record.
  - Check the "marcdocs" and "marctags" cookies.
- Switch to the Advanced MARC editor (you may need to enable
  theEnableAdvancedCatalogingEditor preference).
  - Check the "catalogue_editor" cookie.
- Add a new item to an existing bibliographic record.
  - Check the "LastCreatedItem" cookie which is set after you save the
    new item.
- Go to Authorities -> Authority search.
  - In authority search results, click "Merge" from the "Actions" menu
    next to one of the results..
    - Check the "auth_to_merge" cookie.
- Go to Administration -> MARC bibliographic framework
  - Choose "MARC structure" from the menu corresponding to one of the
    frameworks.
  - Check the "Display only used tags/subfields" checkbox.
    - Check the "marctagstructure_selectdisplay" cookie.
- Go to Circulation -> Check out to a patron with checkouts.
  - Check the "Always show checkouts immediately" checkbox.
    - Check the "issues-table-load-immediately-circulation" cookie.
- Go to Tools -> Patron clubs. You will need at least one active club
  with one or more patrons enrolled.
  - From the list of clubs, click Actions -> Search to hold.
    - Check the "holdforclub" cookie.
- Go to Tools -> Batch item modification and submit a batch of items.
  - Uncheck one or more checkboxes in the "Show/hide columns" area.
    - Check the "showColumns" cookie.
- View a patron -> Search to hold.
  - Check the 'holdfor' cookie.
- With WebBasedSelfCheck enabled, log in to the self-checkout page.
  - Check the "JWT" cookie.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 133213 [details] [review]
Bug 26019: Add two other js cookies

Test plan:
Same as former patch. Use OPAC adv search here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 133214 [details] [review]
Bug 26019: (follow-up) Set SameSite attribute on Cart cookies

I've added an additional followup, so if RM would like another signoff please reset the status.

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑