From 50bf7b4c8971d7c591a2001cb7c4aa0ffd88384b 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 Content-Type: text/plain; charset=utf-8 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 --- Koha/Token.pm | 36 ++++++++++++++++++++++++------------ t/Token.t | 16 ++++++++-------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/Koha/Token.pm b/Koha/Token.pm index c560fce..002237e 100644 --- a/Koha/Token.pm +++ b/Koha/Token.pm @@ -74,7 +74,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 +85,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 +94,15 @@ 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. + If session_id is provided, it is combined with id. =cut sub generate_csrf { my ( $self, $params ) = @_; - return unless $params->{id}; + $params = _add_default_csrf_params( $params ); return $self->generate({ %$params, type => 'CSRF' }); } @@ -128,17 +127,30 @@ 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. + If session_id is provided, it is combined with id. =cut sub check_csrf { my ( $self, $params ) = @_; + $params = _add_default_csrf_params( $params ); return $self->check({ %$params, type => 'CSRF' }); } # --- Internal routines --- +sub _add_default_csrf_params { + my ( $params ) = @_; + $params->{session_id} //= ''; + $params->{id} //= Encode::encode( 'UTF-8', C4::Context->userenv->{id} ); + $params->{id} .= $params->{session_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 +174,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 3a971a5..b0d8303 100644 --- a/t/Token.t +++ b/t/Token.t @@ -33,7 +33,7 @@ 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 $id = $tokenizer->generate({ length => 8 }); my $csrftoken = $tokenizer->generate_csrf({ id => $id }); isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" ); @@ -62,14 +62,14 @@ 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.1.4