From b42d57984bda809f2d8904781b9e7b70120ddf77 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: Jonathan Druart --- 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 0dfc5ee664..c521d693ab 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -230,6 +230,7 @@ sub get_template_and_user { -value => '', -expires => '', -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $template->param( @@ -826,6 +827,7 @@ sub checkauth { -value => '', -expires => '', -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; } @@ -926,7 +928,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... @@ -955,7 +958,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) ) { @@ -1126,7 +1130,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 ), ); } @@ -1474,6 +1480,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 ); @@ -1528,6 +1535,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 e30b2104e4..2ab22636f7 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -1072,6 +1072,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; =head3 needs_install diff --git a/C4/InstallAuth.pm b/C4/InstallAuth.pm index ddc923d353..a753754462 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -261,6 +261,7 @@ sub checkauth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; $userid = $session->param('cardnumber'); @@ -300,6 +301,7 @@ sub checkauth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 2 ) { @@ -345,7 +347,8 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -expires => '' + -expires => '', + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } if ($envcookie) { @@ -387,7 +390,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 c2dbbfddff..3ecdb31724 100755 --- a/t/Context.t +++ b/t/Context.t @@ -18,7 +18,7 @@ use Modern::Perl; use DBI; -use Test::More tests => 29; +use Test::More tests => 35; use Test::MockModule; use Test::Warn; use YAML; @@ -117,3 +117,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