Bug 39206 - Koha improperly tries to remove foreign cookies on logout (and in general the cookies aren't actually removed, but set to empty values)
Summary: Koha improperly tries to remove foreign cookies on logout (and in general the...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-02-26 19:21 UTC by Michał
Modified: 2025-05-23 10:06 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch adds more control to Koha::CookieManager by allowing to refine its list of managed cookies with keep or remove entries in koha-conf.xml. IMPORTANT NOTE: The former (probably widely unused) feature of putting a regex in the do_not_remove_cookie lines is replaced by interpreting its value as a prefix. (So you should e.g. replace catalogue_editor_\d+ by just catalogue_editor_
Version(s) released in:
Circulation function:


Attachments
Bug 39206: Add whitelist to Koha::CookieManager (16.69 KB, patch)
2025-05-23 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39206: Modify koha-conf templates (2.11 KB, patch)
2025-05-23 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39206: Add whitelist to Koha::CookieManager (16.70 KB, patch)
2025-05-23 09:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39206: Modify koha-conf templates (2.11 KB, patch)
2025-05-23 09:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michał 2025-02-26 19:21:29 UTC
So Koha tries to remove all cookies on logout apparently, even if they don't belong to her. It does so improperly though, only resulting in a new cookie being created.

So we have Koha's CGISESSID on opac.library.com and then WEB360SESSID on .library.com (ie. library's main site, that set the cookie on subdomains, because the main site also has some sub-domains).

With the above, removing such a cookie is NOT EXPECTED, because logging out from Koha will interfere with independent unrelated sites on other subdomains. Koha ideally SHOULD NOT try to remove cookies from a different domain. The problem is, the cookies are innocently sent like this:

Cookie: WEB360SESSID=ab2e1sngdb7odgfh68h1vp4j8l; CGISESSID=f4b8837f0bb7040ac96c2c9361cdcc02

So how can we even tell if a cookie is to be removed or not???

Anyway what Koha tries to do is:
set-cookie: WEB360SESSID=; path=/; secure
set-cookie: CGISESSID=7b5138e06ed10fd6c11e4c8d5de73c73; path=/; secure; HttpOnly; SameSite=Lax

Well, this simply ends up creating a brand new new empty WEB360SESSID cookie on opac.library.com, while the cookie at .library.com remains intact, which is obviously a broken behavior.

The actual removal attempt happens here it seems, just as some initial pointer:
https://github.com/Koha-Community/Koha/blob/a524d94abb9b3634ed92186e94d68fdb7db1d50e/Koha/CookieManager.pm#L107-L114

But we should abuse the fact that Set-Cookie defaults to the current subdomain, and use Max-Age=0 to immediately delete the cookie (this is the other side of the coin, current behavior won't remove the cookie, it will just set it to empty value with indefinite expiration!). This way we'd both improve the behavior by actually cleaning up the cookies, and still not making Koha end up overreaching what it might remove, given there's no way to tell from Cookie header what subdomain the cookie was set for.

Of course if someone wanted to push another avenue of removing cookies of higher-level domain, they'd need to use JavaScript to do that probably. But they shouldn't, since Koha shouldn't touch other sites, period.

Also the comment in linked code is: "-expires explicitly omitted to create shortlived 'session' cookie"; and that's wrong because a session could last for months or years. It's not said that the browser will clear it on its restart either.

Another cherry on top: path also isn't set on clearout. This will also miss clearing out some cookies, even ones on the same subdomain. See: https://stackoverflow.com/questions/20320549/how-can-you-delete-a-cookie-in-an-http-response

RFC 6265:
> Finally, to remove a cookie, the server returns a Set-Cookie header with an expiration date in the past. The server will be successful in removing the cookie only if the Path and the Domain attribute in the Set-Cookie header match the values used when the cookie was created.

But now that I think about it, you don't get path in Cookie header either. Uh, so I guess JavaScript cookie cleaning it is then?

Okay some short summary TLDR:
Koha tries to "remove" a cookie by setting it to an empty value, but it fails in that:
- no Max-Age=0 is set, meaning a cookie with empty value is created instead, indefinitely (possibly a new redundant one, as in next point)
- no Path or Domain are passed, meaning that the cookies with these set to non-default values will not be cleared out (and a new empty cookie will be created instead, without touching the cookies that we want to remove)
  - different Domain is expected to NOT be cleared out, as that's another site than Koha and we cannot interfere with that
  - different Path is within Koha subdomain, so not clearing it is problematic in regards to something like Bug 29956
Comment 1 David Cook 2025-02-26 22:14:59 UTC
That's interesting, Michał!

I hadn't bumped into this issue, but I just tried it out on a live site with Google Analytics, and I can see on Koha logout that Koha is taking the GA cookie names and setting new cookies with those GA names for "koha.domain.com.au". 

Although they're session cookies rather than persistent cookies (as you describe - if I understand what you've said correctly), so they'll be cleared out on a browser restart. 

But yeah... definitely a bug. 

--

CCing in Jonathan and Marcel as I think they worked on the original code.

From my perspective, I'd say we should only try to manipulate cookies set by Koha, but that would mean keeping an authoritative list somewhere. Koha sets more cookies than just CGISESSID although I don't think they're well documented at the moment...
Comment 2 Marcel de Rooy 2025-05-19 08:24:03 UTC
Taking a look here
Comment 3 David Cook 2025-05-19 23:42:34 UTC
(In reply to Marcel de Rooy from comment #2)
> Taking a look here

<3
Comment 4 Marcel de Rooy 2025-05-20 14:12:40 UTC
(In reply to Michał from comment #0)

> So we have Koha's CGISESSID on opac.library.com and then WEB360SESSID on
> .library.com (ie. library's main site, that set the cookie on subdomains,
> because the main site also has some sub-domains).

Some preliminary remarks after reading:
The construction here makes that you share WEB360SESSID with Koha. You may not be able to change that (or do not want a "long" list of (sub)domain names in that cookie). You could add the cookie name to koha-conf.xml to tell Koha to leave it alone. (Look for do_not_remove_cookie in koha-conf.)

Coming back to other points soon.
Comment 5 David Cook 2025-05-20 23:20:19 UTC
(In reply to Marcel de Rooy from comment #4)
> names in that cookie). You could add the cookie name to koha-conf.xml to
> tell Koha to leave it alone. (Look for do_not_remove_cookie in koha-conf.)

That's interesting. One of those configs I think that I've seen but never paid any attention to...

Looking at bug 31250 I can see why we went with "do_not_remove_cookie" to create a "keep cookie list", although I suspect the idea was based on the idea that all existing cookies would be set/managed by Koha, which isn't the real world scenario, unfortunately. 

The "do_not_remove_cookie" list could work well in conjunction with a hard-coded list of Koha cookies.
Comment 6 Marcel de Rooy 2025-05-23 09:46:24 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2025-05-23 09:46:26 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2025-05-23 09:55:25 UTC
(In reply to Michał from comment #0)

> Okay some short summary TLDR:
> Koha tries to "remove" a cookie by setting it to an empty value, but it
> fails in that:
> - no Max-Age=0 is set, meaning a cookie with empty value is created instead,
> indefinitely (possibly a new redundant one, as in next point)

Good point. Fixed in this patch set.

> - no Path or Domain are passed, meaning that the cookies with these set to
> non-default values will not be cleared out (and a new empty cookie will be
> created instead, without touching the cookies that we want to remove)
>   - different Domain is expected to NOT be cleared out, as that's another
> site than Koha and we cannot interfere with that
>   - different Path is within Koha subdomain, so not clearing it is
> problematic in regards to something like Bug 29956

The patch now uses a whitelist that can be refined with entries from koha-conf.
So should no longer touch other ones.
Path defaults to /. No domain means current host. There seems to be only one HttpOnly cookie with a longer path in Koha.
If there is a cookie with another path than expected, this patch cannot remove it since the Cookie header line only contains names and values. So we just dont know. But that seems to be a more theoretical issue.
Comment 9 Marcel de Rooy 2025-05-23 09:59:13 UTC
Created attachment 182760 [details] [review]
Bug 39206: Add whitelist to Koha::CookieManager

This patch adds a bit more control to what CookieManager does by
adding a hardcoded whitelist of cookie names that are cleared at
logout. Allowing at the same time to add entries to that list by
using koha-conf <remove_cookie> lines or removing entries from the
hardcoded list by using <do_not_remove_cookie> lines.

The patch fixes the expiration of cookies that should be removed
by passing max-age 0.

Also it adds a theoretical path correction for always_show_holds but
since we do not clear that cookie, it is currently unused. This seems
to be the only Koha cookie where we use a longer path.

Test plan:
Run t/CookieManager.t

Go to OPAC, login, select a few OPAC search results and send them
to cart. This would create cookie bib_list. (Check dev tools.)

Logout from OPAC and check cookie in your browser dev tools. What
you see, depends on the browser. But the cookie should be either
gone or empty and expired (FF: Session).

Now add a <do_not_remove_cookie> line for bib_list in koha-conf.
Restart all. Repeat search, add to cart. Logout. Check again in dev
tools that bib_list is not empty, not expired.

Check out an item. And click on 'Always show checkouts...' on
the patron checkout form. This should create the cookie with
value DO. Logout from intranet. Check that cookie was not affected.
Now add a <remove_cookie> line for the following cookie:
issues-table-load-immediately-circulation. Restart all.
Login and logout from staff again. Check that cookie is empty
and expired, or just gone.

Bonus for devs: Create some custom cookie, and test keeping or
removing it similar as above.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2025-05-23 09:59:16 UTC
Created attachment 182761 [details] [review]
Bug 39206: Modify koha-conf templates

Test plan:
Just read the patch.

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