diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | perllib/Catalyst/Plugin/FixMyStreet/Session/RotateSession.pm | 26 | ||||
| -rw-r--r-- | perllib/FixMyStreet/App.pm | 1 | ||||
| -rw-r--r-- | perllib/FixMyStreet/App/Controller/Report/New.pm | 8 | ||||
| -rw-r--r-- | t/app/controller/alert_new.t | 23 |
5 files changed, 44 insertions, 15 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 95514c3b5..03d6a83f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ * Unreleased - Security: - Fix XSS vulnerability in pagination page number. + - Rotate session ID after successful login. - Front end improvements: - Improved 403 message, especially for private reports. #2511 - Mobile users can now filter the pins on the `/around` map view. #2366 diff --git a/perllib/Catalyst/Plugin/FixMyStreet/Session/RotateSession.pm b/perllib/Catalyst/Plugin/FixMyStreet/Session/RotateSession.pm new file mode 100644 index 000000000..8da88721f --- /dev/null +++ b/perllib/Catalyst/Plugin/FixMyStreet/Session/RotateSession.pm @@ -0,0 +1,26 @@ +package Catalyst::Plugin::FixMyStreet::Session::RotateSession; +use Moose::Role; +use namespace::autoclean; + +# After successful authentication, rotate the session ID +after set_authenticated => sub { + my $c = shift; + $c->change_session_id; +}; + +# The below is necessary otherwise the rotation fails due to the delegate +# holding on to the now-deleted old session. See +# https://rt.cpan.org/Public/Bug/Display.html?id=112679 + +after delete_session_data => sub { + my ($c, $key) = @_; + + my ($field) = split(':', $key); + if ($field eq 'session') { + $c->_session_store_delegate->_session_row(undef); + } elsif ($field eq 'flash') { + $c->_session_store_delegate->_flash_row(undef); + } +}; + +1; diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm index 1173523bc..3e8c07fb0 100644 --- a/perllib/FixMyStreet/App.pm +++ b/perllib/FixMyStreet/App.pm @@ -27,6 +27,7 @@ use Catalyst ( 'Session::State::Cookie', # FIXME - we're using our own override atm 'Authentication', 'SmartURI', + 'FixMyStreet::Session::RotateSession', 'FixMyStreet::Session::StoreSessions', ); diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm index 585337d8b..b748a34e0 100644 --- a/perllib/FixMyStreet/App/Controller/Report/New.pm +++ b/perllib/FixMyStreet/App/Controller/Report/New.pm @@ -514,9 +514,11 @@ sub initialize_report : Private { ->first; if ($report) { - # log the problem creation user in to the site - $c->authenticate( { email => $report->user->email, email_verified => 1 }, - 'no_password' ); + # log the problem creation user in to the site, if not already logged in + if (!$c->user_exists || $c->user->email ne $report->user->email) { + $c->authenticate( { email => $report->user->email, email_verified => 1 }, + 'no_password' ); + } # save the token to delete at the end $c->stash->{partial_token} = $token if $report; diff --git a/t/app/controller/alert_new.t b/t/app/controller/alert_new.t index 90f043935..a5a2a52b3 100644 --- a/t/app/controller/alert_new.t +++ b/t/app/controller/alert_new.t @@ -3,9 +3,8 @@ use FixMyStreet::Script::Alerts; my $mech = FixMyStreet::TestMech->new; -my $user = $mech->log_in_ok('test@example.com'); -$mech->get_ok('/alert/subscribe?id=1'); -my ($csrf) = $mech->content =~ /name="token" value="([^"]*)"/; +my $user = FixMyStreet::App->model('DB::User') + ->new( { email => 'test@example.com' } ); foreach my $test ( { @@ -62,19 +61,13 @@ foreach my $test ( my $type = $test->{type}; - my $user = - FixMyStreet::DB->resultset('User') - ->find( { email => $test->{email} } ); - - # we don't want an alert - if ($user) { - $mech->delete_user($user); - } + $mech->get_ok('/alert/subscribe?id=1'); + my ($csrf) = $mech->content =~ /name="token" value="([^"]*)"/; $mech->get_ok( $test->{uri} . "&token=$csrf" ); $mech->content_contains( $test->{content} ); - $user = + my $user = FixMyStreet::DB->resultset('User') ->find( { email => $test->{email} } ); @@ -162,6 +155,9 @@ foreach my $test ( # clear existing data so we can be sure we're creating it ok $alert->delete() if $alert && !$test->{exist}; + $mech->get_ok('/alert/subscribe?id=1'); + my ($csrf) = $mech->content =~ /name="token" value="([^"]*)"/; + $mech->get_ok( '/alert/subscribe?type=local&rznvy=' . $user->email . '&feed=area:1000:A_Location&token=' . $csrf ); $alert = FixMyStreet::DB->resultset('Alert')->find( @@ -259,6 +255,9 @@ for my $test ( FixMyStreet::DB->resultset('Abuse') ->find_or_create( { email => $test->{email} } ); + $mech->get_ok('/alert/subscribe?id=1'); + my ($csrf) = $mech->content =~ /name="token" value="([^"]*)"/; + $mech->get_ok( $test->{uri} . "&token=$csrf" ); $mech->content_contains( $test->{content} ); |