From 6f039cfb96059f26afd715ce336825c65fc3626c Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 7 May 2020 10:24:44 +1000 Subject: [PATCH] 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. --- C4/Auth.pm | 16 ++++++++-------- C4/Context.pm | 26 ++++++++++++++++++++++++++ C4/InstallAuth.pm | 8 ++++---- t/Context.t | 18 +++++++++++++++++- 4 files changed, 55 insertions(+), 13 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 7b8c522edb..166bfa6596 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -229,7 +229,7 @@ sub get_template_and_user { -value => '', -expires => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $template->param( @@ -830,7 +830,7 @@ sub checkauth { -value => '', -expires => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; } @@ -932,7 +932,7 @@ sub checkauth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -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... @@ -962,7 +962,7 @@ sub checkauth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); my $pki_field = C4::Context->preference('AllowPKIAuth'); if ( !defined($pki_field) ) { @@ -1129,7 +1129,7 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $info{'wrongip'} = 1; } @@ -1208,7 +1208,7 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } @@ -1475,7 +1475,7 @@ sub check_api_auth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $session->param( 'lasttime', time() ); my $flags = haspermission( $userid, $flagsrequired ); @@ -1530,7 +1530,7 @@ sub check_api_auth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 1 ) { my ( diff --git a/C4/Context.pm b/C4/Context.pm index 6f7baeaf2d..9fe3206653 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -1071,6 +1071,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 8d4d95377a..dcbfdce4c4 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -266,7 +266,7 @@ sub checkauth { -name => 'CGISESSID', -value => $session->id, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() } ? 1 : 0 ), ); $loggedin = 1; $userid = $session->param('cardnumber'); @@ -307,7 +307,7 @@ sub checkauth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 2 ) { @@ -354,7 +354,7 @@ sub checkauth { -value => '', -HttpOnly => 1, -expires => '', - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } if ($envcookie) { @@ -397,7 +397,7 @@ sub checkauth { -value => $sessionID, -HttpOnly => 1, -expires => '', - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -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..8f2c137cd0 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 => 34; 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.16.4