Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Add CSRF-check to /logout #632
  • Loading branch information
Jan Henning Thorsen committed Dec 13, 2021
1 parent d4609e4 commit a2d2651
Show file tree
Hide file tree
Showing 12 changed files with 32 additions and 10 deletions.
4 changes: 4 additions & 0 deletions 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
Expand Down
4 changes: 2 additions & 2 deletions assets/components/ChatSidebar.svelte
Expand Up @@ -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';
Expand Down Expand Up @@ -230,7 +230,7 @@ function renderUnread(conversation, max = 60) {
<Icon name="question-circle"/>
<span>{$l('Help')}</span>
</Link>
<a href="{route.urlFor('/logout')}" target="_self">
<a href="{route.urlFor('/logout')}?csrf={settings('csrf')}" target="_self">
<Icon name="power-off"/>
<span>{$l('Log out')}</span>
</a>
Expand Down
3 changes: 3 additions & 0 deletions lib/Convos/Controller/User.pm
Expand Up @@ -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');
Expand Down
5 changes: 5 additions & 0 deletions public/convos-api.yaml
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions t/Helper.pm
Expand Up @@ -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;
Expand Down
6 changes: 4 additions & 2 deletions t/web-ldap.t
Expand Up @@ -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',
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion t/web-login.t
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion t/web-recover-password.t
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion t/web-register-invite-only.t
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion t/web-user.t
Expand Up @@ -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);
};

Expand Down
5 changes: 3 additions & 2 deletions t/web-users.t
Expand Up @@ -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 {
Expand All @@ -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};
Expand Down
1 change: 1 addition & 0 deletions templates/layouts/convos.html.ep
Expand Up @@ -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

<!-- base style + themes -->
Expand Down

0 comments on commit a2d2651

Please sign in to comment.