From 2e544f127c142d017b0bc2d3fe875091245b8780 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 4 May 2020 11:12:26 +1000 Subject: [PATCH] Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS This patch adds the secure flag to the CGISESSID cookie when using HTTPS. This prevents the cookie being used again over a normal HTTP request. Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var This patch tests for HTTPS "on" or "ON" before setting the secure cookie. Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm Signed-off-by: Marcel de Rooy [EDIT] Amended number of tests in Context.t Signed-off-by: Martin Renvoize Signed-off-by: Aleisha Amohia Signed-off-by: Aleisha Amohia --- C4/Auth.pm | 16 ++++++++++++---- C4/Context.pm | 26 ++++++++++++++++++++++++++ C4/InstallAuth.pm | 8 ++++++-- t/Context.t | 18 +++++++++++++++++- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index d431d5a59d..d60dbc02e7 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -228,6 +228,7 @@ sub get_template_and_user { -value => '', -expires => '', -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $template->param( @@ -830,6 +831,7 @@ sub checkauth { -value => '', -expires => '', -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; } @@ -930,7 +932,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => $session->id, - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $session->param( 'lasttime', time() ); unless ( $sessiontype && $sessiontype eq 'anon' ) { #if this is an anonymous session, we want to update the session, but not behave as if they are logged in... @@ -959,7 +962,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => $session->id, - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); my $pki_field = C4::Context->preference('AllowPKIAuth'); if ( !defined($pki_field) ) { @@ -1127,7 +1131,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => '', - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $info{'wrongip'} = 1; } @@ -1207,7 +1212,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => '', - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } @@ -1475,6 +1481,7 @@ sub check_api_auth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $session->param( 'lasttime', time() ); my $flags = haspermission( $userid, $flagsrequired ); @@ -1529,6 +1536,7 @@ sub check_api_auth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 1 ) { my ( diff --git a/C4/Context.pm b/C4/Context.pm index ea281ee800..cfd0b553fe 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -1119,6 +1119,32 @@ sub set_remote_address { } } +=head3 https_enabled + +https_enabled should be called when checking if a HTTPS connection +is used. + +Note that this depends on a HTTPS environmental variable being defined +by the web server. This function may not return the expected result, +if your web server or reverse proxies are not setting the correct +X-Forwarded-Proto headers and HTTPS environmental variable. + +Note too that the HTTPS value can vary from web server to web server. +We are relying on the convention of the value being "on" or "ON" here. + +=cut + +sub https_enabled { + my $https_enabled = 0; + my $env_https = $ENV{HTTPS}; + if ($env_https){ + if ($env_https =~ /^ON$/i){ + $https_enabled = 1; + } + } + return $https_enabled; +} + 1; __END__ diff --git a/C4/InstallAuth.pm b/C4/InstallAuth.pm index 84817ce8ad..c08460582b 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -267,6 +267,7 @@ sub checkauth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; $userid = $session->param('cardnumber'); @@ -307,6 +308,7 @@ sub checkauth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 2 ) { @@ -352,7 +354,8 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -expires => '' + -expires => '', + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } if ($envcookie) { @@ -394,7 +397,8 @@ sub checkauth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, - -expires => '' + -expires => '', + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); print $query->header( -type => 'text/html; charset=utf-8', diff --git a/t/Context.t b/t/Context.t index ea26ac755e..125ea35a5b 100755 --- a/t/Context.t +++ b/t/Context.t @@ -18,7 +18,7 @@ use Modern::Perl; use DBI; -use Test::More tests => 28; +use Test::More tests => 34; use Test::MockModule; use Test::Warn; use YAML; @@ -104,3 +104,19 @@ is(C4::Context->interface, 'opac', 'interface still opac'); is( C4::Context->interface( 'SiP' ), 'sip', 'interface SiP' ); is( C4::Context->interface( 'COMMANDLINE' ), 'commandline', 'interface commandline uc' ); is( C4::Context->interface( 'CRON' ), 'cron', 'interface cron uc' ); + +{ + local %ENV = %ENV; + delete $ENV{HTTPS}; + is( C4::Context->https_enabled, 0, "Undefined HTTPS env returns 0"); + $ENV{HTTPS} = '1'; + is( C4::Context->https_enabled, 0, "Invalid 1 HTTPS env returns 0"); + $ENV{HTTPS} = 'off'; + is( C4::Context->https_enabled, 0, "off HTTPS env returns 0"); + $ENV{HTTPS} = 'OFF'; + is( C4::Context->https_enabled, 0, "OFF HTTPS env returns 0"); + $ENV{HTTPS} = 'on'; + is( C4::Context->https_enabled, 1, "on HTTPS env returns 1"); + $ENV{HTTPS} = 'ON'; + is( C4::Context->https_enabled, 1, "ON HTTPS env returns 1"); +} -- 2.39.5