From 5240684be9706bb418de1b02584b17664e018f1c Mon Sep 17 00:00:00 2001
From: Kevin Decherf <kevin@kdecherf.com>
Date: Wed, 27 Sep 2023 11:08:08 +0200
Subject: [PATCH 1/2] ConfigController: move OTP endpoints to POST method only

Fixes GHSA-56fm-hfp3-x3w3

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
---
 .../Controller/ConfigController.php           | 38 +++++++++---
 .../Form/Type/UserInformationType.php         |  9 ---
 .../Resources/views/Config/index.html.twig    | 36 ++++++++++--
 .../Resources/views/Config/otp_app.html.twig  |  1 +
 .../Controller/ConfigControllerTest.php       | 58 +++++++++++++------
 5 files changed, 101 insertions(+), 41 deletions(-)

diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php
index 03db7390e..db318dddf 100644
--- a/src/Wallabag/CoreBundle/Controller/ConfigController.php
+++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php
@@ -254,10 +254,14 @@ class ConfigController extends AbstractController
     /**
      * Disable 2FA using email.
      *
-     * @Route("/config/otp/email/disable", name="disable_otp_email")
+     * @Route("/config/otp/email/disable", name="disable_otp_email", methods={"POST"})
      */
-    public function disableOtpEmailAction()
+    public function disableOtpEmailAction(Request $request)
     {
+        if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
+            throw $this->createAccessDeniedException('Bad CSRF token.');
+        }
+
         $user = $this->getUser();
         $user->setEmailTwoFactor(false);
 
@@ -274,10 +278,14 @@ class ConfigController extends AbstractController
     /**
      * Enable 2FA using email.
      *
-     * @Route("/config/otp/email", name="config_otp_email")
+     * @Route("/config/otp/email", name="config_otp_email", methods={"POST"})
      */
-    public function otpEmailAction()
+    public function otpEmailAction(Request $request)
     {
+        if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
+            throw $this->createAccessDeniedException('Bad CSRF token.');
+        }
+
         $user = $this->getUser();
 
         $user->setGoogleAuthenticatorSecret(null);
@@ -297,10 +305,14 @@ class ConfigController extends AbstractController
     /**
      * Disable 2FA using OTP app.
      *
-     * @Route("/config/otp/app/disable", name="disable_otp_app")
+     * @Route("/config/otp/app/disable", name="disable_otp_app", methods={"POST"})
      */
-    public function disableOtpAppAction()
+    public function disableOtpAppAction(Request $request)
     {
+        if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
+            throw $this->createAccessDeniedException('Bad CSRF token.');
+        }
+
         $user = $this->getUser();
 
         $user->setGoogleAuthenticatorSecret('');
@@ -319,10 +331,14 @@ class ConfigController extends AbstractController
     /**
      * Enable 2FA using OTP app, user will need to confirm the generated code from the app.
      *
-     * @Route("/config/otp/app", name="config_otp_app")
+     * @Route("/config/otp/app", name="config_otp_app", methods={"POST"})
      */
-    public function otpAppAction(GoogleAuthenticatorInterface $googleAuthenticator)
+    public function otpAppAction(Request $request, GoogleAuthenticatorInterface $googleAuthenticator)
     {
+        if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
+            throw $this->createAccessDeniedException('Bad CSRF token.');
+        }
+
         $user = $this->getUser();
         $secret = $googleAuthenticator->generateSecret();
 
@@ -372,10 +388,14 @@ class ConfigController extends AbstractController
     /**
      * Validate OTP code.
      *
-     * @Route("/config/otp/app/check", name="config_otp_app_check")
+     * @Route("/config/otp/app/check", name="config_otp_app_check", methods={"POST"})
      */
     public function otpAppCheckAction(Request $request, GoogleAuthenticatorInterface $googleAuthenticator)
     {
+        if (!$this->isCsrfTokenValid('otp', $request->request->get('token'))) {
+            throw $this->createAccessDeniedException('Bad CSRF token.');
+        }
+
         $isValid = $googleAuthenticator->checkCode(
             $this->getUser(),
             $request->get('_auth_code')
diff --git a/src/Wallabag/CoreBundle/Form/Type/UserInformationType.php b/src/Wallabag/CoreBundle/Form/Type/UserInformationType.php
index c8544e428..cd5148db6 100644
--- a/src/Wallabag/CoreBundle/Form/Type/UserInformationType.php
+++ b/src/Wallabag/CoreBundle/Form/Type/UserInformationType.php
@@ -23,15 +23,6 @@ class UserInformationType extends AbstractType
             ->add('email', EmailType::class, [
                 'label' => 'config.form_user.email_label',
             ])
-            ->add('emailTwoFactor', CheckboxType::class, [
-                'required' => false,
-                'label' => 'config.form_user.emailTwoFactor_label',
-            ])
-            ->add('googleTwoFactor', CheckboxType::class, [
-                'required' => false,
-                'label' => 'config.form_user.googleTwoFactor_label',
-                'mapped' => false,
-            ])
             ->add('save', SubmitType::class, [
                 'label' => 'config.form.save',
             ])
diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig
index c0d57c067..4d65e82ed 100644
--- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig
+++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig
@@ -209,6 +209,10 @@
 
                             {{ form_widget(form.user.save, {'attr': {'class': 'btn waves-effect waves-light'}}) }}
 
+                            {{ form_widget(form.user._token) }}
+                        
+                        {{ form_end(form.user) }}
+
                             <br/>
                             <br/>
                             <div class="row">
@@ -229,18 +233,42 @@
                                         <tr>
                                             <td>{{ 'config.form_user.two_factor.emailTwoFactor_label'|trans }}</td>
                                             <td>{% if app.user.isEmailTwoFactor %}<b>{{ 'config.form_user.two_factor.state_enabled'|trans }}</b>{% else %}{{ 'config.form_user.two_factor.state_disabled'|trans }}{% endif %}</td>
-                                            <td><a href="{{ path('config_otp_email') }}" class="waves-effect waves-light btn{% if app.user.isEmailTwoFactor %} disabled{% endif %}">{{ 'config.form_user.two_factor.action_email'|trans }}</a> {% if app.user.isEmailTwoFactor %}<a href="{{ path('disable_otp_email') }}" class="waves-effect waves-light btn red">Disable</a>{% endif %}</td>
+                                            <td>
+                                                <form action="{{ path('config_otp_email') }}" method="post" name="config_otp_email">
+                                                    <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
+                
+                                                    <button class="waves-effect waves-light btn{% if app.user.isEmailTwoFactor %} disabled{% endif %}" type="submit">{{ 'config.form_user.two_factor.action_email'|trans }}</button>
+                                                </form>
+                                                {% if app.user.isEmailTwoFactor %}
+                                                <form action="{{ path('disable_otp_email') }}" method="post" name="disable_otp_email">
+                                                    <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
+                
+                                                    <button class="waves-effect waves-light btn red" type="submit">Disable</button>
+                                                </form>
+                                                {% endif %}
+                                            </td>
                                         </tr>
                                         <tr>
                                             <td>{{ 'config.form_user.two_factor.googleTwoFactor_label'|trans }}</td>
                                             <td>{% if app.user.isGoogleTwoFactor %}<b>{{ 'config.form_user.two_factor.state_enabled'|trans }}</b>{% else %}{{ 'config.form_user.two_factor.state_disabled'|trans }}{% endif %}</td>
-                                            <td><a href="{{ path('config_otp_app') }}" class="waves-effect waves-light btn{% if app.user.isGoogleTwoFactor %} disabled{% endif %}">{{ 'config.form_user.two_factor.action_app'|trans }}</a> {% if app.user.isGoogleTwoFactor %}<a href="{{ path('disable_otp_app') }}" class="waves-effect waves-light btn red">Disable</a>{% endif %}</td>
+                                            <td>
+                                                <form action="{{ path('config_otp_app') }}" method="post" name="config_otp_app">
+                                                    <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
+                
+                                                    <button class="waves-effect waves-light btn{% if app.user.isGoogleTwoFactor %} disabled{% endif %}" type="submit">{{ 'config.form_user.two_factor.action_app'|trans }}</button>
+                                                </form>
+                                                {% if app.user.isGoogleTwoFactor %}
+                                                <form action="{{ path('disable_otp_app') }}" method="post" name="disable_otp_app">
+                                                    <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
+                
+                                                    <button class="waves-effect waves-light btn red" type="submit">Disable</button>
+                                                </form>
+                                                {% endif %}
+                                            </td>
                                         </tr>
                                     </tbody>
                                 </table>
                             </div>
-                            {{ form_widget(form.user._token) }}
-                        </form>
                     </div>
 
                     <div id="set4" class="col s12">
diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
index 3beaf241f..f042fd500 100644
--- a/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
+++ b/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
@@ -40,6 +40,7 @@
                             {% endfor %}
 
                             <form class="form" action="{{ path("config_otp_app_check") }}" method="post">
+                                <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
                                 <div class="card-content">
                                     <div class="row">
                                         <div class="input-field col s12">
diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
index 9174daf85..46f5c232e 100644
--- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
+++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
@@ -1174,14 +1174,13 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $this->logInAs('admin');
         $client = $this->getTestClient();
 
-        $crawler = $client->request('GET', '/config/otp/email');
+        $crawler = $client->request('GET', '/config');
+
+        $form = $crawler->filter('form[name=config_otp_email]')->form();
+        $client->submit($form);
 
         $this->assertSame(302, $client->getResponse()->getStatusCode());
-
-        $crawler = $client->followRedirect();
-
-        $this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
-        $this->assertStringContainsString('flashes.config.notice.otp_enabled', $alert[0]);
+        $this->assertStringContainsString('flashes.config.notice.otp_enabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);
 
         // restore user
         $em = $this->getEntityManager();
@@ -1201,14 +1200,23 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $this->logInAs('admin');
         $client = $this->getTestClient();
 
-        $crawler = $client->request('GET', '/config/otp/email/disable');
+        $em = $this->getEntityManager();
+        $user = $em
+            ->getRepository(User::class)
+            ->findOneByUsername('admin');
+
+        $user->setEmailTwoFactor(true);
+        $em->persist($user);
+        $em->flush();
+
+        $crawler = $client->request('GET', '/config');
+
+        $form = $crawler->filter('form[name=disable_otp_email]')->form();
+        $client->submit($form);
 
         $this->assertSame(302, $client->getResponse()->getStatusCode());
-
-        $crawler = $client->followRedirect();
-
-        $this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
-        $this->assertStringContainsString('flashes.config.notice.otp_disabled', $alert[0]);
+        
+        $this->assertStringContainsString('flashes.config.notice.otp_disabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);
 
         // restore user
         $em = $this->getEntityManager();
@@ -1224,7 +1232,10 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $this->logInAs('admin');
         $client = $this->getTestClient();
 
-        $crawler = $client->request('GET', '/config/otp/app');
+        $crawler = $client->request('GET', '/config');
+
+        $form = $crawler->filter('form[name=config_otp_app]')->form();
+        $client->submit($form);
 
         $this->assertSame(200, $client->getResponse()->getStatusCode());
 
@@ -1278,14 +1289,23 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $this->logInAs('admin');
         $client = $this->getTestClient();
 
-        $crawler = $client->request('GET', '/config/otp/app/disable');
+        $em = $this->getEntityManager();
+        $user = $em
+            ->getRepository(User::class)
+            ->findOneByUsername('admin');
+
+        $user->setGoogleAuthenticatorSecret("Google2FA");
+        $em->persist($user);
+        $em->flush();
+
+        $crawler = $client->request('GET', '/config');
+
+        $form = $crawler->filter('form[name=disable_otp_app]')->form();
+        $client->submit($form);
 
         $this->assertSame(302, $client->getResponse()->getStatusCode());
-
-        $crawler = $client->followRedirect();
-
-        $this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(['_text']));
-        $this->assertStringContainsString('flashes.config.notice.otp_disabled', $alert[0]);
+        
+        $this->assertStringContainsString('flashes.config.notice.otp_disabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]);
 
         // restore user
         $em = $this->getEntityManager();

From aa06e8328eaac5cbdc7ac02d57a6d3a9f7c63b18 Mon Sep 17 00:00:00 2001
From: Kevin Decherf <kevin@kdecherf.com>
Date: Sat, 30 Sep 2023 00:46:19 +0200
Subject: [PATCH 2/2] ConfigController: remove 2fa cancel step

This change annoys me, however this endpoint was anyway problematic:
- it was vulnerable to a CSRF attack, see GHSA-56fm-hfp3-x3w3
- it is useless as we don't really handle a two-steps validation

Still, if you send an incorrect code during the "activation" phase a
flash error will pop up but the 2fa will stay enabled. This need rework
when possible.

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
---
 .../Controller/ConfigController.php           | 13 ++++++--
 .../Resources/views/Config/otp_app.html.twig  |  3 --
 .../Controller/ConfigControllerTest.php       | 30 -------------------
 3 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php
index db318dddf..cfe992f59 100644
--- a/src/Wallabag/CoreBundle/Controller/ConfigController.php
+++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php
@@ -373,8 +373,10 @@ class ConfigController extends AbstractController
      * Cancelling 2FA using OTP app.
      *
      * @Route("/config/otp/app/cancel", name="config_otp_app_cancel")
+     * 
+     * XXX: commented until we rewrite 2fa with a real two-steps activation
      */
-    public function otpAppCancelAction()
+    /*public function otpAppCancelAction()
     {
         $user = $this->getUser();
         $user->setGoogleAuthenticatorSecret(null);
@@ -383,7 +385,7 @@ class ConfigController extends AbstractController
         $this->userManager->updateUser($user, true);
 
         return $this->redirect($this->generateUrl('config') . '#set3');
-    }
+    }*/
 
     /**
      * Validate OTP code.
@@ -415,7 +417,12 @@ class ConfigController extends AbstractController
             'scheb_two_factor.code_invalid'
         );
 
-        return $this->redirect($this->generateUrl('config_otp_app'));
+        $this->addFlash(
+            'notice',
+            'scheb_two_factor.code_invalid'
+        );
+
+        return $this->redirect($this->generateUrl('config') . '#set3');
     }
 
     /**
diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
index f042fd500..6d5d402b1 100644
--- a/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
+++ b/src/Wallabag/CoreBundle/Resources/views/Config/otp_app.html.twig
@@ -50,9 +50,6 @@
                                     </div>
                                 </div>
                                 <div class="card-action">
-                                    <a href="{{ path('config_otp_app_cancel') }}" class="waves-effect waves-light grey btn">
-                                        {{ 'config.otp.app.cancel'|trans }}
-                                    </a>
                                     <button class="btn waves-effect waves-light" type="submit" name="send">
                                         {{ 'config.otp.app.enable'|trans }}
                                         <i class="material-icons right">send</i>
diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
index 46f5c232e..c0d138dfa 100644
--- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
+++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
@@ -1254,36 +1254,6 @@ class ConfigControllerTest extends WallabagCoreTestCase
         $em->flush();
     }
 
-    public function testUserEnable2faGoogleCancel()
-    {
-        $this->logInAs('admin');
-        $client = $this->getTestClient();
-
-        $crawler = $client->request('GET', '/config/otp/app');
-
-        $this->assertSame(200, $client->getResponse()->getStatusCode());
-
-        // restore user
-        $em = $this->getEntityManager();
-        $user = $em
-            ->getRepository(User::class)
-            ->findOneByUsername('admin');
-
-        $this->assertTrue($user->isGoogleTwoFactor());
-        $this->assertGreaterThan(0, $user->getBackupCodes());
-
-        $crawler = $client->request('GET', '/config/otp/app/cancel');
-
-        $this->assertSame(302, $client->getResponse()->getStatusCode());
-
-        $user = $em
-            ->getRepository(User::class)
-            ->findOneByUsername('admin');
-
-        $this->assertFalse($user->isGoogleTwoFactor());
-        $this->assertEmpty($user->getBackupCodes());
-    }
-
     public function testUserDisable2faGoogle()
     {
         $this->logInAs('admin');