From 7190593d9dd38001c2d101bcad5cddc222a45ebe Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 16 Feb 2017 11:59:12 +0100 Subject: [PATCH] Bug 18124: [Follow-up] Handle default parameters in a sub Adds a internal routine to handle default values for the parameters id and secret. Also adds a parameter session_id for generate_csrf and check_csrf. This session parameter is combined with the id parameter when generating or checking a token. Test plan: Run t/Token.t Signed-off-by: Marcel de Rooy Signed-off-by: Julian Maurice Signed-off-by: Kyle M Hall --- Koha/Token.pm | 47 +++++++++++++++++++++++++++++++++-------------- t/Token.t | 24 ++++++++++++------------ 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/Koha/Token.pm b/Koha/Token.pm index c560fce06f..d0f7a54516 100644 --- a/Koha/Token.pm +++ b/Koha/Token.pm @@ -34,9 +34,10 @@ Koha::Token - Tokenizer type => 'CSRF', id => $id, secret => $secret, }); - # or check a CSRF token + # generate/check CSRF token with defaults and session id + my $csrf_token = $tokenizer->generate_csrf({ session_id => $x }); my $result = $tokenizer->check_csrf({ - id => $id, secret => $secret, token => $token, + session_id => $x, token => $token, }); =head1 DESCRIPTION @@ -74,7 +75,7 @@ sub new { my $token = $tokenizer->generate({ length => 20 }); my $csrf_token = $tokenizer->generate({ - type => 'CSRF', id => $id, + type => 'CSRF', id => $id, secret => $secret, }); Generate several types of tokens. Now includes CSRF. @@ -85,10 +86,7 @@ sub new { sub generate { my ( $self, $params ) = @_; if( $params->{type} && $params->{type} eq 'CSRF' ) { - $self->{lasttoken} = _gen_csrf( { - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - }); + $self->{lasttoken} = _gen_csrf( $params ); } else { $self->{lasttoken} = _gen_rand( $params ); } @@ -97,13 +95,16 @@ sub generate { =head2 generate_csrf - Shortcut for: generate({ type => 'CSRF', ... }) + Like: generate({ type => 'CSRF', ... }) + Note: id defaults to userid from context, secret to database password. + session_id is mandatory; it is combined with id. =cut sub generate_csrf { my ( $self, $params ) = @_; - return unless $params->{id}; + return if !$params->{session_id}; + $params = _add_default_csrf_params( $params ); return $self->generate({ %$params, type => 'CSRF' }); } @@ -128,17 +129,35 @@ sub check { =head2 check_csrf - Shortcut for: check({ type => 'CSRF', ... }) + Like: check({ type => 'CSRF', ... }) + Note: id defaults to userid from context, secret to database password. + session_id is mandatory; it is combined with id. =cut sub check_csrf { my ( $self, $params ) = @_; + return if !$params->{session_id}; + $params = _add_default_csrf_params( $params ); return $self->check({ %$params, type => 'CSRF' }); } # --- Internal routines --- +sub _add_default_csrf_params { + my ( $params ) = @_; + $params->{session_id} //= ''; + if( !$params->{id} ) { + $params->{id} = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{session_id} ); + } else { + $params->{id} .= $params->{session_id}; + } + $params->{id} //= Encode::encode( 'UTF-8', C4::Context->userenv->{id} ); + my $pw = C4::Context->config('pass'); + $params->{secret} //= md5_base64( Encode::encode( 'UTF-8', $pw ) ), + return $params; +} + sub _gen_csrf { # Since WWW::CSRF::generate_csrf_token does not use the NonBlocking @@ -162,12 +181,12 @@ sub _gen_csrf { sub _chk_csrf { my ( $params ) = @_; - return if !$params->{id} || !$params->{token}; + return if !$params->{id} || !$params->{secret} || !$params->{token}; - my $id = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ); - my $secret = md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ); my $csrf_status = WWW::CSRF::check_csrf_token( - $id, $secret, $params->{token}, + $params->{id}, + $params->{secret}, + $params->{token}, { MaxAge => $params->{MaxAge} // ( CSRF_EXPIRY_HOURS * 3600 ) }, ); return $csrf_status == WWW::CSRF::CSRF_OK(); diff --git a/t/Token.t b/t/Token.t index 3a971a577d..2314d2ebba 100644 --- a/t/Token.t +++ b/t/Token.t @@ -33,13 +33,13 @@ is( length( $tokenizer->generate ), 1, "Generate without parameters" ); my $token = $tokenizer->generate({ length => 20 }); is( length($token), 20, "Token $token has 20 chars" ); -my $id = $tokenizer->generate({ lyyGength => 8 }); -my $csrftoken = $tokenizer->generate_csrf({ id => $id }); +my $id = $tokenizer->generate({ length => 8 }); +my $csrftoken = $tokenizer->generate_csrf({ session_id => $id }); isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" ); is( $tokenizer->check, undef, "Check without any parameters" ); my $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, + session_id => $id, token => $csrftoken, }); is( $result, 1, "CSRF token verified" ); @@ -51,25 +51,25 @@ isnt( $result, 1, "This token is no CSRF token" ); # Test MaxAge parameter my $age = 1; # 1 second $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, MaxAge => $age, + session_id => $id, token => $csrftoken, MaxAge => $age, }); is( $result, 1, "CSRF token still valid within one second" ); usleep $age * 1000000 * 2; # micro (millionth) seconds + 100% $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, MaxAge => $age, + session_id => $id, token => $csrftoken, MaxAge => $age, }); isnt( $result, 1, "CSRF token expired after one second" ); subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub { plan tests => 2; - $csrftoken = $tokenizer->generate_csrf({ id => $id }); + $csrftoken = $tokenizer->generate_csrf({ session_id => $id }); $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, + session_id => $id, token => $csrftoken, }); is( $result, 1, "CSRF token verified" ); C4::Context->set_userenv(0,43,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, + session_id => $id, token => $csrftoken, }); is( $result, '', "CSRF token is not verified if another logged in user is using the same id" ); }; @@ -77,15 +77,15 @@ subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub { subtest 'Same logged in user with another session (cookie CGISESSID)' => sub { plan tests => 2; C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); - $csrftoken = $tokenizer->generate_csrf({ id => $id }); + $csrftoken = $tokenizer->generate_csrf({ session_id => $id }); $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, + session_id => $id, token => $csrftoken, }); is( $result, 1, "CSRF token verified" ); # Get another session id - $id = $tokenizer->generate({ lyyGength => 8 }); + $id = $tokenizer->generate({ length => 8 }); $result = $tokenizer->check_csrf({ - id => $id, token => $csrftoken, + session_id => $id, token => $csrftoken, }); is( $result, '', "CSRF token is not verified if another session is used" ); }; -- 2.39.5