Bugzilla – Attachment 182758 Details for
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)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 39206: Add whitelist to Koha::CookieManager
Bug-39206-Add-whitelist-to-KohaCookieManager.patch (text/plain), 16.69 KB, created by
Marcel de Rooy
on 2025-05-23 09:46:24 UTC
(
hide
)
Description:
Bug 39206: Add whitelist to Koha::CookieManager
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2025-05-23 09:46:24 UTC
Size:
16.69 KB
patch
obsolete
>From 7b12302b7f44dcd03481ffd571098b442989f1f1 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Thu, 22 May 2025 13:44:52 +0200 >Subject: [PATCH] Bug 39206: Add whitelist to Koha::CookieManager >Content-Type: text/plain; charset=utf-8 > >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> >--- > Koha/CookieManager.pm | 130 +++++++++++++++++++++++++++++------------- > t/CookieManager.t | 97 +++++++++++++++---------------- > 2 files changed, 136 insertions(+), 91 deletions(-) > >diff --git a/Koha/CookieManager.pm b/Koha/CookieManager.pm >index 3b655fa12c..2c5ae33516 100644 >--- a/Koha/CookieManager.pm >+++ b/Koha/CookieManager.pm >@@ -25,7 +25,28 @@ use CGI::Cookie; > > use C4::Context; > >-use constant DENY_LIST_VAR => 'do_not_remove_cookie'; >+# The cookies on the following list are removed unless koha-conf.xml indicates otherwise. >+# koha-conf.xml may also contain additional cookies to be removed. >+# Not including here: always_show_holds, catalogue_editor_, issues-table-load-immediately-circulation, >+# and ItemEditorSessionTemplateId. >+# TODO Not sure about branch (rotatingcollections), patronSessionConfirmation (circulation.pl) >+use constant MANAGED_COOKIE_PREFIXES => qw{ >+ bib_list >+ CGISESSID >+ holdfor holdforclub >+ intranet_bib_list >+ JWT >+ KohaOpacLanguage >+ LastCreatedItem >+ marctagstructure_selectdisplay >+ search_path_code >+ searchToOrder >+}; >+use constant PATH_EXCEPTIONS => { >+ always_show_holds => '/cgi-bin/koha/reserve', >+}; >+use constant KEEP_COOKIE_CONF_VAR => 'do_not_remove_cookie'; >+use constant REMOVE_COOKIE_CONF_VAR => 'remove_cookie'; > > our $cookies; > >@@ -41,18 +62,22 @@ Koha::CookieManager - Object for unified handling of cookies in Koha > # Replace cookies > $cookie_list = $mgr->replace_in_list( [ $cookie1, $cookie2_old ], $cookie2_new ); > >- # Clear cookies (governed by deny list entries in koha-conf) >+ # Clear cookies > $cookie_list = $mgr->clear_unless( $cookie1, $cookie2, $cookie3_name ); > > =head1 DESCRIPTION > >-The current object allows you to clear cookies in a list based on the deny list >-in koha-conf.xml. It also offers a method to replace the old version of a cookie >+The current object allows you to remove cookies on the hardcoded list >+in this module, refined by 'keep' or 'remove' entries in koha-conf.xml. >+Note that a keep entry overrules a remove. >+ >+This module also offers a method to replace the old version of a cookie > by a new one. > >-It could be extended by (gradually) routing cookie creation through it in order >-to consistently fill cookie parameters like httponly, secure and samesite flag, >-etc. And could serve to register all our cookies in a central location. >+The module could be extended by (gradually) routing cookie creation >+through it in order to consistently fill cookie parameters like httponly, >+secure and samesite flag, etc. And could serve to register all our cookies >+in a central location. > > =head1 METHODS > >@@ -64,11 +89,15 @@ etc. And could serve to register all our cookies in a central location. > > sub new { > my ( $class, $params ) = @_; >- my $self = bless $params // {}, $class; >- my $denied = C4::Context->config(DENY_LIST_VAR) || []; # expecting scalar or arrayref >- $denied = [$denied] if ref($denied) eq q{}; >- $self->{_remove_unless} = { map { $_ => 1 } @$denied }; >- $self->{_secure} = C4::Context->https_enabled; >+ my $self = bless $params // {}, $class; >+ >+ # Get keep and remove list from koha-conf (scalar or arrayref) >+ my $keep_list = C4::Context->config(KEEP_COOKIE_CONF_VAR) || []; >+ $self->{_keep_list} = ref($keep_list) ? $keep_list : [$keep_list]; >+ my $remove_list = C4::Context->config(REMOVE_COOKIE_CONF_VAR) || []; >+ $self->{_remove_list} = ref($remove_list) ? $remove_list : [$remove_list]; >+ >+ $self->{_secure} = C4::Context->https_enabled; > return $self; > } > >@@ -80,9 +109,9 @@ sub new { > Note: in the example above $query->cookie is a list of cookie names as returned > by the CGI object. > >- Returns an arrayref of cookie objects: empty, expired cookies for those passed >- by name or objects that are not on the deny list, together with the remaining >- (untouched) cookie objects that are on the deny list. >+ Returns an arrayref of cookie objects: empty, expired cookies for >+ cookies on the remove list, together with the remaining (untouched) >+ cookie objects. > > =cut > >@@ -102,17 +131,15 @@ sub clear_unless { > } > next if !$name; > >- if ( $self->_should_be_cleared($name) ) { >+ if ( $self->_should_be_removed($name) ) { > next if $seen->{$name}; >- push @rv, CGI::Cookie->new( >- >- # -expires explicitly omitted to create shortlived 'session' cookie >- # -HttpOnly explicitly set to 0: not really needed here for the >- # cleared httponly cookies, while the js cookies should be 0 >- -name => $name, -value => q{}, -HttpOnly => 0, >- $self->{_secure} ? ( -secure => 1 ) : (), >- ); > $seen->{$name} = 1; # prevent duplicates >+ if ($type) { >+ $c->max_age(0); >+ push @rv, _correct_path($c); >+ } else { >+ push @rv, _correct_path( CGI::Cookie->new( -name => $name, -value => q{}, '-max-age' => 0 ) ); >+ } > } elsif ( $type eq 'CGI::Cookie' ) { # keep the last occurrence > @rv = @{ $self->replace_in_list( \@rv, $c ) }; > } >@@ -120,22 +147,6 @@ sub clear_unless { > return \@rv; > } > >-sub _should_be_cleared { # when it is not on the deny list in koha-conf >- my ( $self, $name ) = @_; >- >- return if $self->{_remove_unless}->{$name}; # exact match >- >- # Now try the entries as regex >- foreach my $k ( keys %{ $self->{_remove_unless} } ) { >- my $reg = $self->{_remove_unless}->{$k}; >- >- # The entry in koha-conf should match the complete string >- # So adding a ^ and $ >- return if $name =~ qr/^${k}$/; >- } >- return 1; >-} >- > =head2 replace_in_list > > $list2 = $mgr->replace_in_list( $list1, $cookie ); >@@ -162,7 +173,44 @@ sub replace_in_list { > return \@result; > } > >-=head1 INTERNAL ROUTINES >+# INTERNAL ROUTINES >+ >+sub _should_be_removed { >+ my ( $self, $name ) = @_; >+ >+ # Is this a controlled cookie? Or is it added as 'keep' or 'remove' in koha-conf.xml? >+ # The conf entries are treated as prefix (no longer as regex). >+ return unless grep { $name =~ /^$_/ } MANAGED_COOKIE_PREFIXES(), @{ $self->{_remove_list} }; >+ return !grep { $name =~ /^$_/ } @{ $self->{_keep_list} }; >+} >+ >+sub _correct_path { >+ my $cookie_object = shift; >+ my $path = PATH_EXCEPTIONS->{ $cookie_object->name } or return $cookie_object; >+ $cookie_object->path($path); >+ return $cookie_object; >+} >+ >+=head1 ADDITIONAL COMMENTS >+ >+ How do the keep or remove lines in koha-conf.xml work? >+ >+ <do_not_remove_cookie>some_cookie</do_not_remove_cookie> >+ The name some_cookie should refer here to a cookie that is on the >+ hardcoded list in this module. If you do not want it to be cleared >+ (removed) on logout, include this line. >+ You might want to do this e.g. for KohaOpacLanguage. >+ >+ <remove_cookie>another_cookie</remove_cookie> >+ The name another_cookie refers here to a cookie that is not on the >+ hardcoded list but you want this cookie to be cleared/removed on logout. >+ It could be a custom cookie. >+ >+ Note that both directives use the cookie name as a prefix. So if you >+ add a remove line for cookie1, it also affects cookie12, etc. >+ Since a keep line overrules a remove line, this allows you to add >+ lines for removing cookie1 and not removing cookie12 in order to >+ remove cookie1, cookie11, cookie13 but not cookie12, etc. > > =cut > >diff --git a/t/CookieManager.t b/t/CookieManager.t >index b4d3604bff..639b681d12 100755 >--- a/t/CookieManager.t >+++ b/t/CookieManager.t >@@ -19,9 +19,10 @@ > > use Modern::Perl; > use CGI; >-use Data::Dumper qw(Dumper); >+ >+#use Data::Dumper qw(Dumper); > use Test::NoWarnings; >-use Test::More tests => 4; >+use Test::More tests => 5; > > use t::lib::Mocks; > >@@ -29,29 +30,32 @@ use C4::Context; > use Koha::CookieManager; > > subtest 'new' => sub { >- plan tests => 3; >+ plan tests => 4; > >- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, 'just_one' ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, 'just_one' ); > my $cmgr = Koha::CookieManager->new; >- is( scalar keys %{ $cmgr->{_remove_unless} }, 1, 'one entry' ); >- is( exists $cmgr->{_secure}, 1, 'secure key found' ); >+ is( scalar @{ $cmgr->{_keep_list} }, 1, 'one entry to keep' ); >+ is( exists $cmgr->{_secure}, 1, 'secure key found' ); > >- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'two', 'entries' ] ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, [ 'two', 'entries' ] ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['test'] ); > $cmgr = Koha::CookieManager->new; >- is( scalar keys %{ $cmgr->{_remove_unless} }, 2, 'two entries' ); >+ is( scalar @{ $cmgr->{_keep_list} }, 2, 'two entries to keep' ); >+ is( scalar @{ $cmgr->{_remove_list} }, 1, 'one entry to remove' ); > }; > > subtest 'clear_unless' => sub { >- plan tests => 17; >+ plan tests => 14; > >- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'aap', 'noot' ] ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, [ 'aap', 'noot' ] ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['mies'] ); > > my $q = CGI->new; > my $cmgr = Koha::CookieManager->new; > > my $cookie1 = $q->cookie( -name => 'aap', -value => 'aap', -expires => '+1d' ); > my $cookie2 = $q->cookie( -name => 'noot', -value => 'noot' ); >- my $cookie3 = $q->cookie( -name => 'wim', -value => q{wim}, -HttpOnly => 1 ); >+ my $cookie3 = $q->cookie( -name => 'wim', -value => q{wim}, -HttpOnly => 0 ); > my $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2}, -HttpOnly => 1 ); > my $list = [ $cookie1, $cookie2, $cookie3, $cookie4, 'mies', 'zus' ]; # 4 cookies, 2 names > >@@ -59,54 +63,47 @@ subtest 'clear_unless' => sub { > is( @{ $cmgr->clear_unless }, 0, 'Empty list' ); > is( @{ $cmgr->clear_unless( { hash => 1 }, ['array'], $q ) }, 0, 'Empty list for invalid arguments' ); > >- # Pass list, expect 5 cookies (3 cleared, last aap kept) >+ # Pass list, expecting 4 cookies (2 kept, 1 untouched, 1 cleared); duplicate aap and zus discarded > my @rv = @{ $cmgr->clear_unless(@$list) }; >- is( @rv, 5, '5 expected' ); >+ is( @rv, 4, '4 expected' ); > is( $rv[0]->name, 'noot', '1st cookie' ); > is( $rv[1]->name, 'wim', '2nd cookie' ); > is( $rv[2]->name, 'aap', '3rd cookie' ); > is( $rv[3]->name, 'mies', '4th cookie' ); >- is( $rv[4]->name, 'zus', '5th cookie' ); >- is( $rv[0]->value, q{noot}, 'noot not empty' ); >- is( $rv[1]->value, q{}, 'wim empty' ); >- is( $rv[2]->value, q{aap2}, 'aap not empty' ); >- is( $rv[3]->value, q{}, 'mies empty' ); >- is( $rv[4]->value, q{}, 'zus empty' ); >- is( $rv[1]->httponly, 0, 'cleared wim is not httponly' ); >+ is( $rv[0]->value, q{noot}, 'noot kept' ); >+ is( $rv[1]->value, q{wim}, 'wim untouched' ); >+ is( $rv[2]->value, q{aap2}, 'aap kept, last entry' ); >+ is( $rv[3]->value, q{}, 'mies cleared' ); >+ is( $rv[1]->httponly, undef, 'wim still not httponly' ); > is( $rv[2]->httponly, 1, 'aap httponly' ); > >- # Test with numeric suffix (via regex) >- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, ['catalogue_editor_\d+'] ); >+ # Test with prefix (note trailing underscore) >+ t::lib::Mocks::mock_config( Koha::CookieManager::KEEP_COOKIE_CONF_VAR, 'catalogue_editor_' ); >+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, 'catalogue_editor' ); > $cmgr = Koha::CookieManager->new; >- $cookie1 = $q->cookie( -name => 'catalogue_editor_abc', -value => '1', -expires => '+1y' ); >- $cookie2 = $q->cookie( -name => 'catalogue_editor_345', -value => '1', -expires => '+1y' ); >- $cookie3 = $q->cookie( -name => 'catalogue_editor_', -value => '1', -expires => '+1y' ); >- $cookie4 = $q->cookie( -name => 'catalogue_editor_123x', -value => '1', -expires => '+1y' ); >- >- $list = [ $cookie1, $cookie2, $cookie3, $cookie4 ]; >- @rv = @{ $cmgr->clear_unless(@$list) }; >- is_deeply( >- [ map { $_->value ? $_->name : () } @rv ], >- ['catalogue_editor_345'], >- 'Cookie2 should be found only' >- ); >- >- # Test with another regex (yes, highly realistic examples :) >- t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, ['next_\w+_number\d{2}_(now|never)'] ); >- $cmgr = Koha::CookieManager->new; >- my $cookie5; >- $cookie1 = $q->cookie( -name => 'next_mynewword_number99_never', -value => '1', -expires => '+1y' ); #fine >- $cookie2 = $q->cookie( -name => 'prefixed_next_mynewword_number99_never', -value => '1', -expires => '+1y' ) >- ; # wrong prefix >- $cookie3 = $q->cookie( -name => 'next_mynew-word_number99_never', -value => '1', -expires => '+1y' ) >- ; # wrong: hyphen in word >- $cookie4 = >- $q->cookie( -name => 'mynewword_number999_never', -value => '1', -expires => '+1y' ); # wrong: three digits >- $cookie5 = >- $q->cookie( -name => 'next_mynewword_number99_always', -value => '1', -expires => '+1y' ); # wrong: always >- @rv = @{ $cmgr->clear_unless( $cookie1, $cookie2, $cookie3, $cookie4, $cookie5 ) }; >- is_deeply( [ map { $_->value ? $_->name : () } @rv ], ['next_mynewword_number99_never'], 'Only cookie1 matched' ); >+ $cookie1 = $q->cookie( -name => 'catalogue_editor', -value => '1' ); >+ $cookie2 = $q->cookie( -name => 'catalogue_editor2', -value => '2' ); >+ $cookie3 = $q->cookie( -name => 'catalogue_editor_3', -value => '3' ); >+ >+ $list = [ $cookie1, $cookie2, $cookie3, 'catalogue_editor4' ]; >+ my $result = [ map { defined( $_->max_age ) ? () : $_->name } @{ $cmgr->clear_unless(@$list) } ]; >+ is_deeply( $result, ['catalogue_editor_3'], 'Only cookie3 is kept (not expired)' ); >+}; > >+subtest 'path exception' => sub { >+ plan tests => 4; >+ >+ t::lib::Mocks::mock_config( Koha::CookieManager::REMOVE_COOKIE_CONF_VAR, ['always_show_holds'] ); >+ my $q = CGI->new; >+ my $cmgr = Koha::CookieManager->new; >+ my $cookie1 = $q->cookie( -name => 'always_show_holds', -value => 'DO', path => '/cgi-bin/koha/reserve' ); >+ my @rv = @{ $cmgr->clear_unless($cookie1) }; >+ is( $rv[0]->name, 'always_show_holds', 'Check name' ); >+ is( $rv[0]->path, '/cgi-bin/koha/reserve', 'Check path' ); >+ is( $rv[0]->max_age, 0, 'Check max_age' ); >+ my $cookie2 = $q->cookie( -name => 'always_show_holds', -value => 'DONT' ); # default path >+ @rv = @{ $cmgr->clear_unless($cookie2) }; >+ is( $rv[0]->path, '/cgi-bin/koha/reserve', 'Check path cookie2, corrected here' ); > }; > > subtest 'replace_in_list' => sub { >-- >2.39.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 39206
:
182758
|
182759
|
182760
|
182761