diff --git a/Changes b/Changes index a75bee035..da273b2b0 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,9 @@ Revision history for perl distribution Convos +6.40 Not Released + - Add CSRF-check to /logout #632 + Contributors: Devendra Bhatla and Jamie Slome + 6.39 2021-11-27T08:50:00+0900 - Add file management to frontend #426 - Add GET /files endpoint #426 diff --git a/assets/components/ChatSidebar.svelte b/assets/components/ChatSidebar.svelte index e735e0ce2..59a02ef9d 100644 --- a/assets/components/ChatSidebar.svelte +++ b/assets/components/ChatSidebar.svelte @@ -2,7 +2,7 @@ import Icon from './Icon.svelte'; import Link from './Link.svelte'; import {activeMenu} from '../store/writable'; -import {closestEl, q, regexpEscape, tagNameIs} from '../js/util'; +import {closestEl, q, regexpEscape, settings, tagNameIs} from '../js/util'; import {fly} from 'svelte/transition'; import {getContext} from 'svelte'; import {l} from '../store/I18N'; @@ -230,7 +230,7 @@ function renderUnread(conversation, max = 60) { {$l('Help')} - + {$l('Log out')} diff --git a/lib/Convos/Controller/User.pm b/lib/Convos/Controller/User.pm index 24e504bbf..09e425d9c 100644 --- a/lib/Convos/Controller/User.pm +++ b/lib/Convos/Controller/User.pm @@ -99,6 +99,9 @@ sub login { sub logout { my $self = shift; # Not a big deal if it's ->openapi->valid_input or not + return $self->reply->exception('Invalid csrf token') + unless +($self->param('csrf') // 'does_not_match') eq $self->csrf_token; + return $self->auth->logout_p({})->then(sub { $self->session({expires => 1}); return $self->redirect_to('/login'); diff --git a/public/convos-api.yaml b/public/convos-api.yaml index 6791b54f8..eb764aba6 100644 --- a/public/convos-api.yaml +++ b/public/convos-api.yaml @@ -625,6 +625,11 @@ paths: summary: Logout a user. tags: [ user ] x-mojo-to: ["user#logout", ["format", ["html", "json"]]] + parameters: + - in: query + name: csrf + required: true + type: string responses: '302': description: Successfully logged out. diff --git a/t/Helper.pm b/t/Helper.pm index 380557c7b..eefeea5f8 100644 --- a/t/Helper.pm +++ b/t/Helper.pm @@ -30,6 +30,12 @@ sub subprocess_in_main_process { ); } +sub with_csrf { + my ($t, $path) = @_; + my $token = $t->get_ok('/chat')->tx->res->dom->at('meta[name=csrf]')->{content} // 'undef'; + return $t->get_ok("$path?csrf=$token"); +} + sub messages { my $class = shift; my $ts = shift || time; diff --git a/t/web-ldap.t b/t/web-ldap.t index 0eff9a516..4ba28e0e1 100644 --- a/t/web-ldap.t +++ b/t/web-ldap.t @@ -8,7 +8,9 @@ $ENV{CONVOS_PLUGINS} = 'Convos::Plugin::Auth::LDAP'; $ENV{CONVOS_BACKEND} = 'Convos::Core::Backend'; my $t = t::Helper->t; -$t->get_ok('/api/user')->status_is(401); +subtest initial => sub { + $t->get_ok('/api/user')->status_is(401); +}; subtest 'not authorized' => sub { $t->post_ok('/api/user/login', @@ -22,7 +24,7 @@ subtest 'authorized' => sub { ok $t->app->core->get_user('superman@example.com'), 'superman account was created'; $t->get_ok('/api/user')->status_is(200); - $t->get_ok('/api/user/logout')->status_is(302); + $t->t::Helper::with_csrf('/logout')->status_is(302); }; subtest 'fallback to local user' => sub { diff --git a/t/web-login.t b/t/web-login.t index 5a5e8b627..fba9a2402 100644 --- a/t/web-login.t +++ b/t/web-login.t @@ -37,7 +37,7 @@ $t->websocket_ok('/events') $t->get_ok('/api/user')->status_is(200); $t->get_ok('/login')->status_is(200); -$t->get_ok('/logout')->status_is(302)->header_is(Location => '/login'); +$t->t::Helper::with_csrf('/logout')->status_is(302)->header_is(Location => '/login'); $t->get_ok('/login')->status_is(200); $t->get_ok('/api/user')->status_is(401); diff --git a/t/web-recover-password.t b/t/web-recover-password.t index f48ec8f6d..7acc66d45 100644 --- a/t/web-recover-password.t +++ b/t/web-recover-password.t @@ -21,7 +21,7 @@ subtest 'Create recover user and recover url' => sub { }; subtest 'Log out admin' => sub { - $t->get_ok('/api/user/logout')->status_is(302); + $t->t::Helper::with_csrf('/logout')->status_is(302); }; subtest 'Let recover user do the rest' => sub { diff --git a/t/web-register-invite-only.t b/t/web-register-invite-only.t index ca5a51876..322137eb1 100644 --- a/t/web-register-invite-only.t +++ b/t/web-register-invite-only.t @@ -18,7 +18,7 @@ $t->get_ok('/register')->status_is(200) ->element_exists(qq(meta[name="convos:first_user"][content="yes"])); my %register = (email => 'first@convos.chat', password => 'firstpassword'); $t->post_ok('/api/user/register', json => \%register)->status_is(200); -$t->get_ok('/api/user/logout')->status_is(302)->header_is(Location => '/login'); +$t->t::Helper::with_csrf('/logout')->status_is(302)->header_is(Location => '/login'); note 'second user needs invite link'; $t->get_ok('/register')->status_is(200) diff --git a/t/web-user.t b/t/web-user.t index 2742ee0e2..60d6140fd 100644 --- a/t/web-user.t +++ b/t/web-user.t @@ -94,7 +94,7 @@ subtest 'Delete' => sub { $t->delete_ok('/api/user/superman@example.com')->status_is(400) ->json_is('/errors/0/message', 'You are the only user left.'); - $t->get_ok('/api/user/logout')->status_is(302); + $t->t::Helper::with_csrf('/logout')->status_is(302); $t->get_ok('/api/user')->status_is(401); }; diff --git a/t/web-users.t b/t/web-users.t index 2a671034e..4407e5650 100644 --- a/t/web-users.t +++ b/t/web-users.t @@ -29,7 +29,8 @@ subtest 'delete / update other users as admin' => sub { }; subtest 'logout first user' => sub { - $t->get_ok('/api/user/logout')->status_is(302); + $t->get_ok('/api/user/logout')->status_is(500); + $t->t::Helper::with_csrf('/api/user/logout')->status_is(302); }; subtest 'second user' => sub { @@ -52,7 +53,7 @@ subtest 'delete / update other users as normal user' => sub { }; subtest 'logout second user' => sub { - $t->get_ok('/api/user/logout')->status_is(302); + $t->t::Helper::with_csrf('/api/user/logout')->status_is(302); $server->client($t->app->core->get_user('superwoman@example.com')->connections->[0]) ->server_event_ok('_irc_event_nick')->process_ok; delete $server->{client}; diff --git a/templates/layouts/convos.html.ep b/templates/layouts/convos.html.ep index c4d0dbe8e..146f36744 100644 --- a/templates/layouts/convos.html.ep +++ b/templates/layouts/convos.html.ep @@ -55,6 +55,7 @@ %= tag 'meta', name => 'convos:open_to_public', content => $open_to_public ? 'yes' : 'no' %= tag 'meta', name => 'convos:start_app', content => $start_app %= tag 'meta', name => 'convos:status', content => stash('status') || 200 + %= tag 'meta', name => 'csrf', content => csrf_token %= tag 'meta', name => 'version', content => Convos->VERSION