From 6a274ce64b49dde0bb5f00aa8adb0e45ba0b3d82 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 (cherry picked from commit b9d92bdc6c43e8d242c274682b16b6f45af35f86) Signed-off-by: Victor Grousset/tuxayo --- 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 45d97bcbde..0dc7cd45a9 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( @@ -843,6 +844,7 @@ sub checkauth { -value => '', -expires => '', -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; } @@ -943,7 +945,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... @@ -972,7 +975,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) ) { @@ -1140,7 +1144,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => '', - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $info{'wrongip'} = 1; } @@ -1220,7 +1225,8 @@ sub checkauth { $cookie = $query->cookie( -name => 'CGISESSID', -value => '', - -HttpOnly => 1 + -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } @@ -1488,6 +1494,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 ); @@ -1542,6 +1549,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 fa612472f7..86271161fb 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -1079,6 +1079,32 @@ sub temporary_directory { } +=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 70d9ac637b..df0974dbc2 100755 --- a/t/Context.t +++ b/t/Context.t @@ -2,7 +2,7 @@ use Modern::Perl; use DBI; -use Test::More tests => 27; +use Test::More tests => 33; use Test::MockModule; BEGIN { @@ -63,3 +63,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