From cdd0bf66efbdb8c89ab292f07bbf28ce7478f585 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy 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 lines or removing entries from the hardcoded list by using 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 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 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 --- 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? + + some_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. + + another_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