From 89e671b094cf94e09e089e846952579ca6e3d3b1 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 95c03e4312..20388051ff 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -244,7 +244,7 @@ sub get_template_and_user { -value => '', -expires => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $template->param( @@ -863,7 +863,7 @@ sub checkauth { -value => '', -expires => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $loggedin = 1; } @@ -996,7 +996,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... @@ -1026,7 +1026,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) ) { @@ -1220,7 +1220,7 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); $info{'wrongip'} = 1; } @@ -1310,7 +1310,7 @@ sub checkauth { -name => 'CGISESSID', -value => '', -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } @@ -1726,7 +1726,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 ); @@ -1781,7 +1781,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 d68d5c1ae8..2302b09d72 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 223ed2cb32..82fd9b7cc6 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -267,7 +267,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'); @@ -308,7 +308,7 @@ sub checkauth { -name => 'CGISESSID', -value => $sessionID, -HttpOnly => 1, - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); if ( $return == 2 ) { @@ -355,7 +355,7 @@ sub checkauth { -value => '', -HttpOnly => 1, -expires => '', - -secure => ( $ENV{HTTPS} ? 1 : 0 ), + -secure => ( C4::Context->https_enabled() ? 1 : 0 ), ); } if ($envcookie) { @@ -398,7 +398,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 ea26ac755e..de57fa8386 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.16.4