Summary: | Koha improperly tries to remove foreign cookies on logout (and in general the cookies aren't actually removed, but set to empty values) | ||
---|---|---|---|
Product: | Koha | Reporter: | Michał <schodkowy.omegi-0r> |
Component: | Authentication | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | Pushed to stable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | minor | ||
Priority: | P5 - low | CC: | dcook, dpavlin, jonathan.druart, lucas, m.de.rooy, martin.renvoize |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29957 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29956 |
||
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: |
25.11.00,25.05.02
|
Circulation function: | |
Attachments: |
Bug 39206: Add whitelist to Koha::CookieManager
Bug 39206: Modify koha-conf templates Bug 39206: Add whitelist to Koha::CookieManager Bug 39206: Modify koha-conf templates Bug 39206: Add whitelist to Koha::CookieManager Bug 39206: Modify koha-conf templates Bug 39206: Add an allowlist to Koha::CookieManager Bug 39206: Modify koha-conf templates |
Description
Michał
2025-02-26 19:21:29 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... Taking a look here (In reply to Marcel de Rooy from comment #2) > Taking a look here <3 (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. (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. Created attachment 182758 [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. Logout (OPAC or staff). 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> Created attachment 182759 [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> (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. 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> 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> Created attachment 183234 [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> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Created attachment 183235 [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> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Created attachment 183497 [details] [review] Bug 39206: Add an allowlist to Koha::CookieManager This patch adds a bit more control to what CookieManager does by adding a hardcoded allowlist 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> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 183498 [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> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Taking Paul's SO line as a signoff.. this all appears to work as expected to me and I can't see any security risks this exposes.. No regressions found and the QA scripts are happy. Passing QA Note, I updated the commit message of the first commit.. 'allowlist' is the modern term, we tend to about 'white' or 'black' lists now ;) Nice work everyone! Pushed to main for 25.11 Nice work everyone! Pushed to 25.05.x |