Add CSRF-check to /logout #632

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

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) {
       <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>

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');

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.

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;

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 {

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);
 

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 {

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)

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);
 };
 

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};

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
 
     <!-- base style + themes -->