Bug 26019 - Koha should set SameSite attribute on cookies
Summary: Koha should set SameSite attribute on cookies
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 29940
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-18 12:28 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 26019: Koha should set SameSite attribute on cookies (18.79 KB, patch)
2022-02-14 18:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26019: Koha should set SameSite attribute on cookies (18.79 KB, patch)
2022-02-14 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26019: Koha should set SameSite attribute on cookies (18.88 KB, patch)
2022-03-17 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26019: Add two other js cookies (1.99 KB, patch)
2022-03-17 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26019: Koha should set SameSite attribute on cookies (18.86 KB, patch)
2022-03-31 18:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26019: Add two other js cookies (2.01 KB, patch)
2022-03-31 18:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26019: Koha should set SameSite attribute on cookies (18.90 KB, patch)
2022-04-12 12:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26019: Add two other js cookies (2.01 KB, patch)
2022-04-12 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26019: (follow-up) Set SameSite attribute on Cart cookies (1001 bytes, patch)
2022-04-12 12:23 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-07-18 12:28:17 UTC
Browsers are starting to show warnings about this attribute missing.

https://web.dev/samesite-cookies-explained/
Comment 1 David Cook 2020-08-04 00:23:29 UTC
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...
Comment 2 Marcel de Rooy 2020-08-05 14:50:47 UTC
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 ?
Comment 3 David Cook 2020-08-06 00:35:56 UTC
(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?
Comment 4 Marcel de Rooy 2020-08-06 11:58:50 UTC
(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..
Comment 5 Tomás Cohen Arazi 2020-08-06 12:38:59 UTC
(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'.
Comment 6 David Cook 2020-08-06 23:17:17 UTC
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.
Comment 7 David Cook 2020-08-06 23:25:09 UTC
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.
Comment 8 David Cook 2020-08-06 23:38:43 UTC
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.
Comment 9 David Cook 2020-08-06 23:40:52 UTC
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).
Comment 10 David Cook 2020-08-06 23:43:16 UTC
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.
Comment 11 Owen Leonard 2022-01-19 14:24:50 UTC
Would it be a logical first step to explicitly set SameSite to Lax for the cookies we set without a SameSite attribute?
Comment 12 Owen Leonard 2022-02-14 18:43:55 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2022-02-14 18:52:14 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2022-03-17 12:37:30 UTC
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.
Comment 15 Marcel de Rooy 2022-03-17 12:47:28 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2022-03-17 12:47:31 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2022-03-31 18:01:25 UTC
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>
Comment 18 Kyle M Hall 2022-03-31 18:01:45 UTC
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>
Comment 19 Fridolin Somers 2022-04-12 09:37:39 UTC
Sorry does not apply anymore, maybe because of Bug 29957.
Please rebase
Comment 20 Owen Leonard 2022-04-12 12:22:56 UTC
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>
Comment 21 Owen Leonard 2022-04-12 12:23:00 UTC
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>
Comment 22 Owen Leonard 2022-04-12 12:23:05 UTC
Created attachment 133214 [details] [review]
Bug 26019: (follow-up) Set SameSite attribute on Cart cookies
Comment 23 Owen Leonard 2022-04-12 12:23:56 UTC
I've added an additional followup, so if RM would like another signoff please reset the status.
Comment 24 Fridolin Somers 2022-04-13 13:56:51 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄