From ac291c229a924e91c86f8e80c3cd5f40f4fdfa76 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 23 Mar 2022 07:41:54 +0000 Subject: [PATCH] Bug 29957: (follow-up) Turn allow list into deny list See the discussion on the Bugzilla report. It might be easier to work with a deny list. Test plan: Run t/CookieManager.t Signed-off-by: Marcel de Rooy Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Fridolin Somers --- Koha/CookieManager.pm | 39 +++++++++++++-------------- t/CookieManager.t | 62 ++++++++++++++++++------------------------- 2 files changed, 45 insertions(+), 56 deletions(-) diff --git a/Koha/CookieManager.pm b/Koha/CookieManager.pm index cba829a196..d7680d8cc0 100644 --- a/Koha/CookieManager.pm +++ b/Koha/CookieManager.pm @@ -24,7 +24,7 @@ use CGI::Cookie; use C4::Context; -use constant ALLOW_LIST_VAR => 'removable_cookie'; +use constant DENY_LIST_VAR => 'do_not_remove_cookie'; our $cookies; @@ -40,15 +40,14 @@ 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 koha-conf removable_cookie lines) - $cookie_list = $mgr->clear_if_allowed( $cookie1, $cookie2, $cookie3_name ); - + # Clear cookies (governed by deny list entries in koha-conf) + $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 allow -list in koha-conf.xml. It also offers a method to replace the old version of -a cookie by a new one. +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 +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, @@ -65,28 +64,28 @@ etc. And could serve to register all our cookies in a central location. sub new { my ( $class, $params ) = @_; my $self = bless $params//{}, $class; - my $allowed = C4::Context->config(ALLOW_LIST_VAR) || []; # expecting scalar or arrayref - $allowed = [ $allowed ] if ref($allowed) eq q{}; - $self->{_remove_allowed} = { map { $_ => 1 } @$allowed }; + 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; return $self; } -=head2 clear_if_allowed +=head2 clear_unless - $cookies = $self->clear_if_allowed( $query->cookie, @$cookies ); + $cookies = $self->clear_unless( $query->cookie, @$cookies ); Arguments: either cookie names or cookie objects (CGI::Cookie). 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 object that are on the allow list, together with the remaining - (untouched) cookie objects not on that list. + by name or objects that are not on the deny list, together with the remaining + (untouched) cookie objects that are on the deny list. =cut -sub clear_if_allowed { +sub clear_unless { my ( $self, @cookies ) = @_; my @rv; my $seen = {}; @@ -101,8 +100,8 @@ sub clear_if_allowed { $name = $c; } - if( $self->{_remove_allowed}->{$name} ) { - next if $seen->{ $name }; + if( !$self->{_remove_unless}->{$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 @@ -110,9 +109,9 @@ sub clear_if_allowed { -name => $name, -value => q{}, -HttpOnly => 0, $self->{_secure} ? ( -secure => 1 ) : (), ); - $seen->{ $name } = 1; # prevent duplicates - } elsif( $type eq 'CGI::Cookie' ) { - push @rv, $c; + $seen->{$name} = 1; # prevent duplicates + } elsif( $type eq 'CGI::Cookie' ) { # keep the last occurrence + @rv = @{ $self->replace_in_list( \@rv, $c ) }; } } return \@rv; diff --git a/t/CookieManager.t b/t/CookieManager.t index c7c46e35af..3c49035451 100755 --- a/t/CookieManager.t +++ b/t/CookieManager.t @@ -30,59 +30,49 @@ use Koha::CookieManager; subtest 'new' => sub { plan tests => 3; - t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, 'just_one' ); + t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, 'just_one' ); my $cmgr = Koha::CookieManager->new; - is( scalar keys %{$cmgr->{_remove_allowed}}, 1, 'one entry' ); + is( scalar keys %{$cmgr->{_remove_unless}}, 1, 'one entry' ); is( exists $cmgr->{_secure}, 1, 'secure key found' ); - t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, [ 'two', 'entries' ] ); + t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'two', 'entries' ] ); $cmgr = Koha::CookieManager->new; - is( scalar keys %{$cmgr->{_remove_allowed}}, 2, 'two entries' ); + is( scalar keys %{$cmgr->{_remove_unless}}, 2, 'two entries' ); }; -subtest 'clear_if_allowed' => sub { - plan tests => 13; +subtest 'clear_unless' => sub { + plan tests => 15; - t::lib::Mocks::mock_config( Koha::CookieManager::ALLOW_LIST_VAR, [ 'aap', 'noot', 'mies' ] ); + t::lib::Mocks::mock_config( Koha::CookieManager::DENY_LIST_VAR, [ 'aap', 'noot' ] ); my $q = CGI->new; my $cmgr = Koha::CookieManager->new; - my $cookie1 = $q->cookie( - -name => 'aap', - -value => 'aap', - -expires => '+1d', - -HttpOnly => 1, - -secure => 1, - ); - my $cookie2 = $q->cookie( - -name => 'noot', - -value => 'noot', - -expires => '+1d', - -HttpOnly => 1, - -secure => 1, - ); + 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 $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2} ); + my $cookie4 = $q->cookie( -name => 'aap', -value => q{aap2}, -HttpOnly => 1 ); my $list = [ $cookie1, $cookie2, $cookie3, $cookie4, 'mies', 'zus' ]; # 4 cookies, 2 names # No results expected - is( @{$cmgr->clear_if_allowed}, 0, 'Empty list' ); - is( @{$cmgr->clear_if_allowed( 'scalar', [], $q )}, 0, 'Empty list for invalid arguments' ); - - # Pass list, expect 4 cookies (3 cleared) - my @rv = @{$cmgr->clear_if_allowed( @$list )}; - is( @rv, 4, 'Four expected' ); - is( $rv[0]->name, 'aap', 'First cookie' ); - is( $rv[1]->name, 'noot', '2nd cookie' ); - is( $rv[2]->name, 'wim', '3rd cookie' ); + 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) + my @rv = @{$cmgr->clear_unless( @$list )}; + is( @rv, 5, '5 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[0]->value, q{}, 'aap should be empty' ); - is( $rv[1]->value, q{}, 'noot should be empty' ); - is( $rv[2]->value, 'wim', 'wim not empty' ); + 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[0]->httponly, 0, 'cleared aap isnt httponly' ); - is( $rv[2]->httponly, 1, 'wim still httponly' ); + is( $rv[4]->value, q{}, 'zus empty' ); + is( $rv[1]->httponly, 0, 'cleared wim isnt httponly' ); + is( $rv[2]->httponly, 1, 'aap httponly' ); }; subtest 'replace_in_list' => sub { -- 2.39.5