Attachment 107833 Details for Bug 25360 – Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS

[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.36 KB, created by Marcel de Rooy on 2020-08-05 14:08:56 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2020-08-05 14:08:56 UTC

Size: 7.36 KB

patch

obsolete

>From 550df0f8e716a2a076f2cbab3af4f7d828883dea 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
>Content-Type: text/plain; charset=utf-8
>
>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
>---
> 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 259b6d6063..e45fb21efb 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -229,6 +229,7 @@ sub get_template_and_user {
>                 -value    => '',
>                 -expires  => '',
>                 -HttpOnly => 1,
>+                -secure => ( C4::Context->https_enabled() ? 1 : 0 ),
>             );
> 
>             $template->param(
>@@ -825,6 +826,7 @@ sub checkauth {
>             -value    => '',
>             -expires  => '',
>             -HttpOnly => 1,
>+            -secure => ( C4::Context->https_enabled() ? 1 : 0 ),
>         );
>         $loggedin = 1;
>     }
>@@ -925,7 +927,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...
>@@ -954,7 +957,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) ) {
>@@ -1120,7 +1124,8 @@ sub checkauth {
>                             $cookie = $query->cookie(
>                                 -name     => 'CGISESSID',
>                                 -value    => '',
>-                                -HttpOnly => 1
>+                                -HttpOnly => 1,
>+                                -secure => ( C4::Context->https_enabled() ? 1 : 0 ),
>                             );
>                             $info{'wrongip'} = 1;
>                         }
>@@ -1198,7 +1203,8 @@ sub checkauth {
>             $cookie = $query->cookie(
>                 -name     => 'CGISESSID',
>                 -value    => '',
>-                -HttpOnly => 1
>+                -HttpOnly => 1,
>+                -secure => ( C4::Context->https_enabled() ? 1 : 0 ),
>             );
>         }
> 
>@@ -1464,6 +1470,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 );
>@@ -1518,6 +1525,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 8c621e8576..14c60db555 100644
>--- a/C4/Context.pm
>+++ b/C4/Context.pm
>@@ -1069,6 +1069,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.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 25360: 104244 | 104469 | 104470 | 104642 | 106176 | 106177 | 106178 | 107833 | 107834 | 108161 | 108162