improve csrf handling (#2936)

src/Controller/CustomerController.php

@@ -41,6 +41,8 @@ use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Annotation\Route;
+use Symfony\Component\Security\Csrf\CsrfToken;
+use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
 
 /**
  * Controller used to manage customer in the admin part of the site.
@@ -157,13 +159,21 @@ final class CustomerController extends AbstractController
     }
 
     /**
-     * @Route(path="/{id}/comment_delete", name="customer_comment_delete", methods={"GET"})
+     * @Route(path="/{id}/comment_delete/{token}", name="customer_comment_delete", methods={"GET"})
      * @Security("is_granted('edit', comment.getCustomer()) and is_granted('comments', comment.getCustomer())")
      */
-    public function deleteCommentAction(CustomerComment $comment)
+    public function deleteCommentAction(CustomerComment $comment, string $token, CsrfTokenManagerInterface $csrfTokenManager)
     {
         $customerId = $comment->getCustomer()->getId();
 
+        if (!$csrfTokenManager->isTokenValid(new CsrfToken('customer.delete_comment', $token))) {
+            $this->flashError('action.csrf.error');
+
+            return $this->redirectToRoute('customer_details', ['id' => $customerId]);
+        }
+
+        $csrfTokenManager->refreshToken($token);
+
         try {
             $this->repository->deleteComment($comment);
         } catch (\Exception $ex) {
@@ -196,11 +206,21 @@ final class CustomerController extends AbstractController
     }
 
     /**
-     * @Route(path="/{id}/comment_pin", name="customer_comment_pin", methods={"GET"})
+     * @Route(path="/{id}/comment_pin/{token}", name="customer_comment_pin", methods={"GET"})
      * @Security("is_granted('edit', comment.getCustomer()) and is_granted('comments', comment.getCustomer())")
      */
-    public function pinCommentAction(CustomerComment $comment)
+    public function pinCommentAction(CustomerComment $comment, string $token, CsrfTokenManagerInterface $csrfTokenManager)
     {
+        $customerId = $comment->getCustomer()->getId();
+
+        if (!$csrfTokenManager->isTokenValid(new CsrfToken('customer.pin_comment', $token))) {
+            $this->flashError('action.csrf.error');
+
+            return $this->redirectToRoute('customer_details', ['id' => $customerId]);
+        }
+
+        $csrfTokenManager->refreshToken($token);
+
         $comment->setPinned(!$comment->isPinned());
         try {
             $this->repository->saveComment($comment);
@@ -208,7 +228,7 @@ final class CustomerController extends AbstractController
             $this->flashUpdateException($ex);
         }
 
-        return $this->redirectToRoute('customer_details', ['id' => $comment->getCustomer()->getId()]);
+        return $this->redirectToRoute('customer_details', ['id' => $customerId]);
     }
 
     /**

src/Controller/DoctorController.php

@@ -64,7 +64,7 @@ class DoctorController extends AbstractController
     public function deleteLogfileAction(string $token, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
         if (!$csrfTokenManager->isTokenValid(new CsrfToken('doctor.flush_log', $token))) {
-            $this->flashError('action.delete.error');
+            $this->flashError('action.csrf.error');
 
             return $this->redirectToRoute('doctor');
         }

src/Controller/InvoiceController.php

@@ -260,7 +260,7 @@ final class InvoiceController extends AbstractController
     public function deleteInvoiceAction(Invoice $invoice, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
         if (!$csrfTokenManager->isTokenValid(new CsrfToken('invoice.delete', $token))) {
-            $this->flashError('action.delete.error');
+            $this->flashError('action.csrf.error');
 
             return $this->redirectToRoute('admin_invoice_list');
         }
@@ -451,11 +451,19 @@ final class InvoiceController extends AbstractController
     }
 
     /**
-     * @Route(path="/template/{id}/delete", name="admin_invoice_template_delete", methods={"GET", "POST"})
+     * @Route(path="/template/{id}/delete/{token}", name="admin_invoice_template_delete", methods={"GET", "POST"})
      * @Security("is_granted('manage_invoice_template')")
      */
-    public function deleteTemplate(InvoiceTemplate $template): Response
+    public function deleteTemplate(InvoiceTemplate $template, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
+        if (!$csrfTokenManager->isTokenValid(new CsrfToken('invoice.delete_template', $token))) {
+            $this->flashError('action.csrf.error');
+
+            return $this->redirectToRoute('admin_invoice_template');
+        }
+
+        $csrfTokenManager->refreshToken($token);
+
         try {
             $this->templateRepository->removeTemplate($template);
             $this->flashSuccess('action.delete.success');

src/Controller/PermissionController.php

@@ -209,7 +209,7 @@ final class PermissionController extends AbstractController
     public function deleteRole(Role $role, string $csrfToken, UserRepository $userRepository, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
         if (!$this->isCsrfTokenValid(self::TOKEN_NAME, $csrfToken)) {
-            $this->flashUpdateException(new \Exception('Invalid CSRF token'));
+            $this->flashError('action.csrf.error');
 
             return $this->redirectToRoute('admin_user_permissions');
         }

src/Controller/ProjectController.php

@@ -43,6 +43,8 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Annotation\Route;
+use Symfony\Component\Security\Csrf\CsrfToken;
+use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
 
 /**
  * Controller used to manage projects.
@@ -179,13 +181,21 @@ final class ProjectController extends AbstractController
     }
 
     /**
-     * @Route(path="/{id}/comment_delete", name="project_comment_delete", methods={"GET"})
+     * @Route(path="/{id}/comment_delete/{token}", name="project_comment_delete", methods={"GET"})
      * @Security("is_granted('edit', comment.getProject()) and is_granted('comments', comment.getProject())")
      */
-    public function deleteCommentAction(ProjectComment $comment)
+    public function deleteCommentAction(ProjectComment $comment, string $token, CsrfTokenManagerInterface $csrfTokenManager)
     {
         $projectId = $comment->getProject()->getId();
 
+        if (!$csrfTokenManager->isTokenValid(new CsrfToken('project.delete_comment', $token))) {
+            $this->flashError('action.csrf.error');
+
+            return $this->redirectToRoute('project_details', ['id' => $projectId]);
+        }
+
+        $csrfTokenManager->refreshToken($token);
+
         try {
             $this->repository->deleteComment($comment);
         } catch (\Exception $ex) {
@@ -218,11 +228,21 @@ final class ProjectController extends AbstractController
     }
 
     /**
-     * @Route(path="/{id}/comment_pin", name="project_comment_pin", methods={"GET"})
+     * @Route(path="/{id}/comment_pin/{token}", name="project_comment_pin", methods={"GET"})
      * @Security("is_granted('edit', comment.getProject()) and is_granted('comments', comment.getProject())")
      */
-    public function pinCommentAction(ProjectComment $comment)
+    public function pinCommentAction(ProjectComment $comment, string $token, CsrfTokenManagerInterface $csrfTokenManager)
     {
+        $projectId = $comment->getProject()->getId();
+
+        if (!$csrfTokenManager->isTokenValid(new CsrfToken('project.pin_comment', $token))) {
+            $this->flashError('action.csrf.error');
+
+            return $this->redirectToRoute('project_details', ['id' => $projectId]);
+        }
+
+        $csrfTokenManager->refreshToken($token);
+
         $comment->setPinned(!$comment->isPinned());
         try {
             $this->repository->saveComment($comment);
@@ -230,7 +250,7 @@ final class ProjectController extends AbstractController
             $this->flashUpdateException($ex);
         }
 
-        return $this->redirectToRoute('project_details', ['id' => $comment->getProject()->getId()]);
+        return $this->redirectToRoute('project_details', ['id' => $projectId]);
     }
 
     /**

src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php

@@ -36,7 +36,7 @@ class InvoiceTemplateSubscriber extends AbstractActionsSubscriber
             }
             $event->addAction('edit', ['url' => $this->path('admin_invoice_template_edit', ['id' => $template->getId()]), 'class' => 'modal-ajax-form']);
             $event->addAction('copy', ['url' => $this->path('admin_invoice_template_copy', ['id' => $template->getId()])]);
-            $event->addDelete($this->path('admin_invoice_template_delete', ['id' => $template->getId()]), false);
+            $event->addDelete($this->path('admin_invoice_template_delete', ['id' => $template->getId(), 'token' => $payload['token']]), false);
         }
     }
 }

templates/customer/details.html.twig

@@ -139,7 +139,7 @@
     {% if comments is not null %}
         {% set options = {'form': commentForm, 'comments': comments} %}
         {% if can_edit %}
-            {% set options = options|merge({'route_pin': 'customer_comment_pin', 'route_delete': 'customer_comment_delete'}) %}
+            {% set options = options|merge({'route_pin': 'customer_comment_pin', 'route_delete': 'customer_comment_delete', 'csrf_delete': 'customer.delete_comment', 'csrf_pin': 'customer.pin_comment'}) %}
         {% endif %}
         {{ include('embeds/comments.html.twig', options) }}
     {% endif %}

templates/embeds/comments.html.twig

@@ -1,4 +1,4 @@
-{% embed '@AdminLTE/Widgets/box-widget.html.twig' with {'form': form, 'comments': comments, 'route_pin': route_pin|default(null), 'route_delete': route_delete|default(null), 'delete_by_user': delete_by_user|default(false)} %}
+{% embed '@AdminLTE/Widgets/box-widget.html.twig' with {'form': form, 'comments': comments, 'route_pin': route_pin|default(null), 'route_delete': route_delete|default(null), 'delete_by_user': delete_by_user|default(false), 'csrf_delete': csrf_token(csrf_delete), 'csrf_pin': csrf_token(csrf_pin)} %}
     {% import "macros/widgets.html.twig" as widgets %}
     {% block box_title %}{{ 'label.comment'|trans }}{% endblock %}
     {% block box_attributes %}id="comments_box"{% endblock %}
@@ -24,12 +24,12 @@
                         </span>
                         <span class="pull-right">
                         {% if route_pin is not null %}
-                            <a href="{{ path(route_pin, {'id': comment.id}) }}" class="btn btn-default btn-xs {% if comment.pinned %}active{% endif %}"><i class="{{ 'pin'|icon }}"></i></a>
+                            <a href="{{ path(route_pin, {'id': comment.id, 'token': csrf_pin}) }}" class="btn btn-default btn-xs {% if comment.pinned %}active{% endif %}"><i class="{{ 'pin'|icon }}"></i></a>
                         {% elseif comment.pinned %}
                             <i class="{{ 'pin'|icon }}"></i>
                         {% endif %}
                         {% if route_delete is not null and ((not delete_by_user) or (delete_by_user and comment.createdBy.id == app.user.id)) %}
-                            <a href="{{ path(route_delete, {'id': comment.id}) }}" class="confirmation-link btn btn-default btn-xs" data-question="confirm.delete"><i class="{{ 'delete'|icon }}"></i></a>
+                            <a href="{{ path(route_delete, {'id': comment.id, 'token': csrf_delete}) }}" class="confirmation-link btn btn-default btn-xs" data-question="confirm.delete"><i class="{{ 'delete'|icon }}"></i></a>
                         {% endif %}
                         </span>
                     </div>

templates/invoice/actions.html.twig

@@ -30,6 +30,6 @@
 
 {% macro invoice_template(template, view) %}
     {% import "macros/widgets.html.twig" as widgets %}
-    {% set event = actions(app.user, 'invoice_template', view, {'template': template}) %}
+    {% set event = actions(app.user, 'invoice_template', view, {'template': template, 'token': csrf_token('invoice.delete_template')}) %}
     {{ widgets.table_actions(event.actions) }}
 {% endmacro %}

templates/project/details.html.twig

@@ -144,7 +144,7 @@
     {% if comments is not null %}
         {% set options = {'form': commentForm, 'comments': comments} %}
         {% if can_edit %}
-            {% set options = options|merge({'route_pin': 'project_comment_pin', 'route_delete': 'project_comment_delete'}) %}
+            {% set options = options|merge({'route_pin': 'project_comment_pin', 'route_delete': 'project_comment_delete', 'csrf_delete': 'project.delete_comment', 'csrf_pin': 'project.pin_comment'}) %}
         {% endif %}
         {{ include('embeds/comments.html.twig', options) }}
     {% endif %}

tests/Controller/CustomerControllerTest.php

@@ -176,19 +176,43 @@ class CustomerControllerTest extends ControllerBaseTest
         ]);
         $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details'));
         $client->followRedirect();
+
+        $token = self::$container->get('security.csrf.token_manager')->getToken('customer.delete_comment');
+
         $node = $client->getCrawler()->filter('div.box#comments_box .direct-chat-msg');
         self::assertStringContainsString('Blah foo bar', $node->html());
         $node = $client->getCrawler()->filter('div.box#comments_box .box-body a.confirmation-link');
-        self::assertStringEndsWith('/comment_delete', $node->attr('href'));
+        self::assertStringEndsWith('/comment_delete/' . $token, $node->attr('href'));
+
+        $comments = $this->getEntityManager()->getRepository(CustomerComment::class)->findAll();
+        $id = $comments[0]->getId();
+
+        $this->request($client, '/admin/customer/' . $id . '/comment_delete/' . $token);
+        $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details'));
+        $client->followRedirect();
+        $node = $client->getCrawler()->filter('div.box#comments_box .box-body');
+        self::assertStringContainsString('There were no comments posted yet', $node->html());
+    }
+
+    public function testDeleteCommentActionWithoutToken()
+    {
+        $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN);
+        $this->assertAccessIsGranted($client, '/admin/customer/1/details');
+        $form = $client->getCrawler()->filter('form[name=customer_comment_form]')->form();
+        $client->submit($form, [
+            'customer_comment_form' => [
+                'message' => 'Blah foo bar',
+            ]
+        ]);
+        $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details'));
+        $client->followRedirect();
 
         $comments = $this->getEntityManager()->getRepository(CustomerComment::class)->findAll();
         $id = $comments[0]->getId();
 
         $this->request($client, '/admin/customer/' . $id . '/comment_delete');
-        $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details'));
+
-        $client->followRedirect();
+        $this->assertRouteNotFound($client);
-        $node = $client->getCrawler()->filter('div.box#comments_box .box-body');
-        self::assertStringContainsString('There were no comments posted yet', $node->html());
     }
 
     public function testPinCommentAction()
@@ -211,12 +235,14 @@ class CustomerControllerTest extends ControllerBaseTest
         $comments = $this->getEntityManager()->getRepository(CustomerComment::class)->findAll();
         $id = $comments[0]->getId();
 
-        $this->request($client, '/admin/customer/' . $id . '/comment_pin');
+        $token = self::$container->get('security.csrf.token_manager')->getToken('customer.pin_comment');
+
+        $this->request($client, '/admin/customer/' . $id . '/comment_pin/' . $token);
         $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details'));
         $client->followRedirect();
         $node = $client->getCrawler()->filter('div.box#comments_box .box-body a.btn.active');
         self::assertEquals(1, $node->count());
-        self::assertEquals($this->createUrl('/admin/customer/' . $id . '/comment_pin'), $node->attr('href'));
+        self::assertEquals($this->createUrl('/admin/customer/' . $id . '/comment_pin/' . $token), $node->attr('href'));
     }
 
     public function testCreateDefaultTeamAction()

tests/Controller/InvoiceControllerTest.php

@@ -395,7 +395,9 @@ class InvoiceControllerTest extends ControllerBaseTest
         $template = $this->importFixture($fixture);
         $id = $template[0]->getId();
 
-        $this->request($client, '/invoice/template/' . $id . '/delete');
+        $token = self::$container->get('security.csrf.token_manager')->getToken('invoice.delete_template');
+
+        $this->request($client, '/invoice/template/' . $id . '/delete/' . $token);
         $this->assertIsRedirect($client, '/invoice/template');
         $client->followRedirect();
 

tests/Controller/ProjectControllerTest.php

@@ -261,8 +261,10 @@ class ProjectControllerTest extends ControllerBaseTest
         $comments = $this->getEntityManager()->getRepository(ProjectComment::class)->findAll();
         $id = $comments[0]->getId();
 
-        self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_delete'), $node->attr('href'));
+        $token = self::$container->get('security.csrf.token_manager')->getToken('project.delete_comment');
-        $this->request($client, '/admin/project/' . $id . '/comment_delete');
+
+        self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_delete/' . $token), $node->attr('href'));
+        $this->request($client, '/admin/project/' . $id . '/comment_delete/' . $token);
         $this->assertIsRedirect($client, $this->createUrl('/admin/project/1/details'));
         $client->followRedirect();
         $node = $client->getCrawler()->filter('div.box#comments_box .box-body');
@@ -289,12 +291,14 @@ class ProjectControllerTest extends ControllerBaseTest
         $comments = $this->getEntityManager()->getRepository(ProjectComment::class)->findAll();
         $id = $comments[0]->getId();
 
-        $this->request($client, '/admin/project/' . $id . '/comment_pin');
+        $token = self::$container->get('security.csrf.token_manager')->getToken('project.pin_comment');
+
+        $this->request($client, '/admin/project/' . $id . '/comment_pin/' . $token);
         $this->assertIsRedirect($client, $this->createUrl('/admin/project/1/details'));
         $client->followRedirect();
         $node = $client->getCrawler()->filter('div.box#comments_box .box-body a.btn.active');
         self::assertEquals(1, $node->count());
-        self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_pin'), $node->attr('href'));
+        self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_pin/' . $token), $node->attr('href'));
     }
 
     public function testCreateDefaultTeamAction()

translations/flashmessages.de.xlf

@@ -54,6 +54,10 @@
         <source>action.upload.error</source>
         <target>Die Datei konnte nicht hochgeladen bzw. gespeichert werden: %reason%</target>
       </trans-unit>
+      <trans-unit resname="action.csrf.error" id="bOE_q5R">
+        <source>action.csrf.error</source>
+        <target>Die Aktion konnte nicht durchgeführt werden: ungültiges Sicherheitstoken.</target>
+      </trans-unit>
     </body>
   </file>
 </xliff>

translations/flashmessages.en.xlf

@@ -54,6 +54,10 @@
         <source>action.upload.error</source>
         <target>The file could not be uploaded or saved: %reason%</target>
       </trans-unit>
+      <trans-unit resname="action.csrf.error" id="bOE_q5R">
+        <source>action.csrf.error</source>
+        <target>The action could not be performed: invalid security token.</target>
+      </trans-unit>
     </body>
   </file>
 </xliff>