Bugzilla – Attachment 108161 Details for
Bug 25360
Use secure flag for CGISESSID cookie
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS
Bug-25360-Use-secure-flag-for-CGISESSID-cookie-whe.patch (text/plain), 7.39 KB, created by
Martin Renvoize
on 2020-08-13 08:02:00 UTC
(
hide
)
Description:
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS
Filename:
MIME Type:
Creator:
Martin Renvoize
Created:
2020-08-13 08:02:00 UTC
Size:
7.39 KB
patch
obsolete
>From 1c1ac15a1d0bb07be5f161c5a373a3b97c94295b Mon Sep 17 00:00:00 2001 >From: David Cook <dcook@prosentient.com.au> >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 <m.de.rooy@rijksmuseum.nl> >[EDIT] Amended number of tests in Context.t >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25360
:
104244
|
104469
|
104470
|
104642
|
106176
|
106177
|
106178
|
107833
|
107834
| 108161 |
108162