From c7e14d883e18f6280e404361f366bbb1eafa822e Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Sun, 25 Aug 2024 12:37:59 +0200 Subject: [PATCH 1/3] Differentiate between domain cookies and portal cookies (#3228) --- .../lib/Lemonldap/NG/Portal/Auth/CAS.pm | 3 +- .../lib/Lemonldap/NG/Portal/Auth/Twitter.pm | 8 ++--- .../NG/Portal/Lib/Notifications/JSON.pm | 2 +- .../NG/Portal/Lib/Notifications/XML.pm | 2 +- .../lib/Lemonldap/NG/Portal/Main/Process.pm | 10 +++--- .../lib/Lemonldap/NG/Portal/Main/Run.pm | 31 +++++++++++-------- .../NG/Portal/Plugins/RememberAuthChoice.pm | 6 ++-- .../NG/Portal/Plugins/TrustedBrowser.pm | 6 ++-- 8 files changed, 33 insertions(+), 35 deletions(-) diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/CAS.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/CAS.pm index 16a724c3a6..47dfd8908e 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/CAS.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/CAS.pm @@ -198,8 +198,7 @@ sub extractFormInfo { $req->{urldc} = $login_url; $req->steps( [] ); $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => 'llngcasserver', value => $srv, secure => $self->conf->{securedCookie}, diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/Twitter.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/Twitter.pm index 205a646680..6a71d682f8 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/Twitter.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Auth/Twitter.pm @@ -126,8 +126,7 @@ sub extractFormInfo { # 1.2 Store token key and secret in cookies (available 180s) $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => '_twitSec', value => $response->token_secret, max_age => 180, @@ -170,7 +169,7 @@ sub extractFormInfo { signature_method => 'HMAC-SHA1', verifier => $verifier, token => $request_token, - token_secret => $self->p->genCookie( $req, name => '_twitSec' ), + token_secret => $self->p->cookie( name => '_twitSec' ), timestamp => time, nonce => $nonce, ); @@ -211,8 +210,7 @@ sub extractFormInfo { # Clean temporaries cookies $req->addCookie( - $self->p->genCookie( - $req, + $self->cookie( name => '_twitSec', value => 0, expires => 'Wed, 21 Oct 2015 00:00:00 GMT' diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/JSON.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/JSON.pm index 87a92fa702..81e8a9ab7f 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/JSON.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/JSON.pm @@ -184,7 +184,7 @@ sub getNotifBack { if ( $req->param('cancel') ) { $self->logger->debug('Cancel called -> remove ciphered cookie'); $req->addCookie( - $self->p->genCookie( + $self->p->genDomainCookie( $req, name => $self->conf->{cookieName}, value => 0, diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/XML.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/XML.pm index b93b8aa31e..65d4db15aa 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/XML.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Lib/Notifications/XML.pm @@ -242,7 +242,7 @@ sub getNotifBack { if ( $req->param('cancel') ) { $self->logger->debug('Cancel called -> remove ciphered cookie'); $req->addCookie( - $self->p->genCookie( + $self->p->genDomainCookie( $req, name => $self->conf->{cookieName}, value => 0, diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Process.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Process.pm index 59eac87bac..4d83ec3b06 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Process.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Process.pm @@ -409,7 +409,7 @@ sub extractFormInfo { and $req->cookies->{ $self->conf->{cookieName} } ) { $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName}, value => 0, @@ -601,8 +601,8 @@ sub store { my $session = $self->getApacheSession( $req->id, hashStore => $req->data->{hashStore}, - force => $req->{force}, - info => $infos + force => $req->{force}, + info => $infos ); return PE_APACHESESSIONERROR unless $session; @@ -630,7 +630,7 @@ sub buildCookie { my ( $self, $req ) = @_; if ( $req->id ) { $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName}, value => $req->id, @@ -639,7 +639,7 @@ sub buildCookie { ); if ( $self->conf->{securedCookie} >= 2 ) { $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName} . "http", value => $req->{sessionInfo}->{_httpSession}, diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm index 1174e1ef29..0627bf3bb6 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm @@ -107,7 +107,7 @@ sub handler { : () ), ); - push @{ $res->[1] }, 'Set-Cookie', $self->genCookie( $req, %v ); + push @{ $res->[1] }, 'Set-Cookie', $self->cookie(%v); } return $res; } @@ -240,7 +240,7 @@ sub processRefreshSession { # Avoid interferences when refresh is run on multiple sessions # in the same request - $req->sessionInfo({}); + $req->sessionInfo( {} ); $req->steps( [ 'getUser', @{ $self->betweenAuthAndData }, @@ -313,7 +313,7 @@ sub _unauthLogout { $self->logger->debug("Removing $self->{conf}->{cookieName} cookie"); $req->pdata( {} ); $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName}, secure => $self->conf->{securedCookie}, @@ -732,7 +732,7 @@ sub _deleteSession { # Create an obsolete cookie to remove it $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName} . 'http', value => 0, @@ -747,7 +747,7 @@ sub _deleteSession { # Create an obsolete cookie to remove it $req->addCookie( - $self->genCookie( + $self->genDomainCookie( $req, name => $self->conf->{cookieName}, value => 0, @@ -926,18 +926,23 @@ sub fullUrl { } # Generates a cookie header which can depend on the request +# If no domain was explicitely specified, use the default SSO domain +sub genDomainCookie { + my ( $self, $req, %h ) = @_; + + $h{domain} ||= $self->getCookieDomain($req); + return $self->cookie(%h); +} + +# DEPRECATED, #3228 sub genCookie { my ( $self, $req, %h ) = @_; - $h{path} ||= '/'; - $h{HttpOnly} //= $self->conf->{httpOnly}; - $h{max_age} //= $self->conf->{cookieExpiration} - if ( $self->conf->{cookieExpiration} ); - $h{SameSite} ||= $self->cookieSameSite; - $h{domain} ||= $self->getCookieDomain($req); - return $self->assemble_cookie(%h); + return $self->cookie(%h); } -# Deprecated method, does not handle dynamic portal URL +# Generate a cookie header +# If no domain was explicitely specified, only the portal will see +# this cookie sub cookie { my ( $self, %h ) = @_; $h{path} ||= '/'; diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm index b864e27a74..6eb7a2fd36 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm @@ -76,8 +76,7 @@ sub storeRememberedAuthChoice { . " with authentication choice lmAuth=" . $lmAuth ); $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => $self->rememberCookieName, value => $lmAuth, max_age => $self->rememberCookieTimeout, @@ -94,8 +93,7 @@ sub storeRememberedAuthChoice { . $self->rememberCookieName ); $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => $self->rememberCookieName, value => 0, expires => 'Wed, 21 Oct 2015 00:00:00 GMT', diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm index 1cf1ee710f..276b71e734 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm @@ -194,8 +194,7 @@ sub storeBrowser { # Cookie available 30 days by default $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => $self->cookieName, value => $ps->id, max_age => $self->timeout, @@ -404,8 +403,7 @@ sub removeCookie { my ( $self, $req ) = @_; $req->addCookie( - $self->p->genCookie( - $req, + $self->p->cookie( name => $self->cookieName, value => 0, expires => 'Wed, 21 Oct 2015 00:00:00 GMT', -- GitLab From 3ce22c66156ccff91d31347381f3113686a73d2b Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Sun, 25 Aug 2024 12:47:42 +0200 Subject: [PATCH 2/3] Unit test for #3228 --- lemonldap-ng-portal/t/01-Cookie-Domain.t | 2 +- lemonldap-ng-portal/t/01-pdata.t | 24 ++++++++++++++++++++++-- lemonldap-ng-portal/t/64-StayConnected.t | 14 ++++++++++++-- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lemonldap-ng-portal/t/01-Cookie-Domain.t b/lemonldap-ng-portal/t/01-Cookie-Domain.t index 502ba2f43c..da10a2c75e 100644 --- a/lemonldap-ng-portal/t/01-Cookie-Domain.t +++ b/lemonldap-ng-portal/t/01-Cookie-Domain.t @@ -18,7 +18,7 @@ sub assertCookieValue { sub assertGenCookieValue { local $Test::Builder::Level = $Test::Builder::Level + 1; my ( $client, $req, $opts, $expected ) = @_; - my $result = $client->p->genCookie( $req, %$opts ); + my $result = $client->p->genDomainCookie( $req, %$opts ); my $str = join( ',', map { "$_=$opts->{$_}" } sort keys %$opts ); is( $result, $expected, "Correct cookie result for $str" ); } diff --git a/lemonldap-ng-portal/t/01-pdata.t b/lemonldap-ng-portal/t/01-pdata.t index 78f5217bde..ce4b48e288 100644 --- a/lemonldap-ng-portal/t/01-pdata.t +++ b/lemonldap-ng-portal/t/01-pdata.t @@ -3,14 +3,14 @@ use Test::More; use strict; use IO::String; use URI::Escape; +use Plack::Response; require 't/test-lib.pm'; my $res; my $tmp; -my $client = LLNG::Manager::Test->new( - { +my $client = LLNG::Manager::Test->new( { ini => { logLevel => 'error', customPlugins => 't::pdata', @@ -21,6 +21,14 @@ my $client = LLNG::Manager::Test->new( # Two simple access to see if pdata is set and restored ok( $res = $client->_get( '/', ), 'Simple access' ); $tmp = expectCookie( $res, 'lemonldappdata' ); + +unlike( + Plack::Response->new(@$res)->headers->header('Set-Cookie'), + qr/lemonldappdata=[^,]*domain=/, + "Domain not set in pdata cookie" +); +count(1); + ok( $tmp eq uri_escape('{"mytest":1}'), 'Pdata is {"mytest":1}' ) or explain( $tmp, uri_escape('{"mytest":1}') ); count(2); @@ -28,6 +36,12 @@ count(2); ok( $res = $client->_get( '/', cookie => 'lemonldappdata=' . $tmp, ), 'Second simple access' ); $tmp = expectCookie( $res, 'lemonldappdata' ); +unlike( + Plack::Response->new(@$res)->headers->header('Set-Cookie'), + qr/lemonldappdata=[^,]*domain=/, + "Domain not set in pdata cookie" +); +count(1); ok( $tmp eq uri_escape('{"mytest":2}'), 'Pdata is {"mytest":2}' ) or explain( $tmp, uri_escape('{"mytest":2}') ); count(2); @@ -47,6 +61,12 @@ count(1); expectOK($res); $tmp = expectCookie( $res, 'lemonldappdata' ); +unlike( + Plack::Response->new(@$res)->headers->header('Set-Cookie'), + qr/lemonldappdata=[^,]*domain=/, + "Domain not set in pdata cookie" +); +count(1); ok( $tmp eq '', 'Pdata is ""' ) or explain( $res, 'lemonldappdata=;' ); count(1); diff --git a/lemonldap-ng-portal/t/64-StayConnected.t b/lemonldap-ng-portal/t/64-StayConnected.t index 6aeac8c470..a82975ec4f 100644 --- a/lemonldap-ng-portal/t/64-StayConnected.t +++ b/lemonldap-ng-portal/t/64-StayConnected.t @@ -2,11 +2,11 @@ use warnings; use Test::More; use strict; use IO::String; +use Plack::Response; require 't/test-lib.pm'; -my $client = LLNG::Manager::Test->new( - { +my $client = LLNG::Manager::Test->new( { ini => { logLevel => 'error', useSafeJail => 1, @@ -48,6 +48,11 @@ subtest "Register session, use it, then logout" => sub { my $id = expectCookie($res); expectRedirection( $res, 'http://auth.example.com/' ); my $cid = expectCookie( $res, 'llngpersistent' ); + unlike( + Plack::Response->new(@$res)->headers->header('Set-Cookie'), + qr/llngpersistent=[^,]*domain=/, + "Domain not set in stayconnected cookie" + ); ok( $res->[1]->[5] =~ /\bsecure\b/, ' Secure cookie found' ) or explain( $res->[1]->[5], 'Secure cookie found' ); @@ -115,6 +120,11 @@ subtest "Make sure connection ID is saved on first login too" => sub { my $id = expectCookie($res); expectRedirection( $res, 'http://auth.example.com/' ); my $cid = expectCookie( $res, 'llngpersistent' ); + unlike( + Plack::Response->new(@$res)->headers->header('Set-Cookie'), + qr/llngpersistent=[^,]*domain=/, + "Domain not set in stayconnected cookie" + ); ok( $res->[1]->[5] =~ /\bsecure\b/, ' Secure cookie found' ) or explain( $res->[1]->[5], 'Secure cookie found' ); -- GitLab From 209f7f6eb15f41ea2eef72e47e23c0206993f150 Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Mon, 26 Aug 2024 20:46:41 +0200 Subject: [PATCH 3/3] Avoid regressions in #3228 In order to avoid interference by cookies set with a too-wide domain we unset faulty cookies twice: on the portal scope itself, and on the SSO domain scope --- .../lib/Lemonldap/NG/Portal/Main/Run.pm | 50 +++++++++++++------ .../NG/Portal/Plugins/RememberAuthChoice.pm | 11 ++++ .../NG/Portal/Plugins/TrustedBrowser.pm | 11 ++++ lemonldap-ng-portal/t/01-pdata.t | 6 --- 4 files changed, 57 insertions(+), 21 deletions(-) diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm index 0627bf3bb6..02f294a59f 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Main/Run.pm @@ -93,21 +93,41 @@ sub handler { # Save pdata if ( $sp or %{ $req->pdata } ) { - my %v = ( - name => $self->conf->{cookieName} . 'pdata', - secure => $self->conf->{securedCookie}, - ( - %{ $req->pdata } - ? ( value => uri_escape( JSON::to_json( $req->pdata ) ) ) - : ( value => '', expires => 'Wed, 21 Oct 2015 00:00:00 GMT' ) - ), - ( - $self->conf->{pdataDomain} - ? ( domain => $self->conf->{pdataDomain}, ) - : () - ), - ); - push @{ $res->[1] }, 'Set-Cookie', $self->cookie(%v); + my %pdata_options = (); + $pdata_options{domain} = $self->conf->{pdataDomain} + if $self->conf->{pdataDomain}; + + if ( %{ $req->pdata } ) { + push @{ $res->[1] }, 'Set-Cookie', + $self->cookie( + name => $self->conf->{cookieName} . 'pdata', + secure => $self->conf->{securedCookie}, + value => uri_escape( JSON::to_json( $req->pdata ) ), + %pdata_options, + ); + + } + else { + push @{ $res->[1] }, 'Set-Cookie', + $self->cookie( + name => $self->conf->{cookieName} . 'pdata', + secure => $self->conf->{securedCookie}, + value => '', + expires => 'Wed, 21 Oct 2015 00:00:00 GMT', + %pdata_options, + ); + + # Avoid regressions with #3228 + push @{ $res->[1] }, 'Set-Cookie', + $self->genDomainCookie( + $req, + name => $self->conf->{cookieName} . 'pdata', + secure => $self->conf->{securedCookie}, + value => '', + expires => 'Wed, 21 Oct 2015 00:00:00 GMT', + %pdata_options, + ); + } } return $res; } diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm index 6eb7a2fd36..0f5cab00f5 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/RememberAuthChoice.pm @@ -100,6 +100,17 @@ sub storeRememberedAuthChoice { secure => $self->conf->{securedCookie}, ) ); + + # Avoid regressions with #3228 + $req->addCookie( + $self->p->genDomainCookie( + $req, + name => $self->rememberCookieName, + value => 0, + expires => 'Wed, 21 Oct 2015 00:00:00 GMT', + secure => $self->conf->{securedCookie}, + ) + ); } } diff --git a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm index 276b71e734..6a9e716f7d 100644 --- a/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm +++ b/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Plugins/TrustedBrowser.pm @@ -410,6 +410,17 @@ sub removeCookie { secure => $self->conf->{securedCookie}, ) ); + + # Avoid regressions with #3228 + $req->addCookie( + $self->p->genDomainCookie( + $req, + name => $self->cookieName, + value => 0, + expires => 'Wed, 21 Oct 2015 00:00:00 GMT', + secure => $self->conf->{securedCookie}, + ) + ); } sub removeExistingSessions { diff --git a/lemonldap-ng-portal/t/01-pdata.t b/lemonldap-ng-portal/t/01-pdata.t index ce4b48e288..03762d46e8 100644 --- a/lemonldap-ng-portal/t/01-pdata.t +++ b/lemonldap-ng-portal/t/01-pdata.t @@ -61,12 +61,6 @@ count(1); expectOK($res); $tmp = expectCookie( $res, 'lemonldappdata' ); -unlike( - Plack::Response->new(@$res)->headers->header('Set-Cookie'), - qr/lemonldappdata=[^,]*domain=/, - "Domain not set in pdata cookie" -); -count(1); ok( $tmp eq '', 'Pdata is ""' ) or explain( $res, 'lemonldappdata=;' ); count(1); -- GitLab