From 95796ab2560ad93f44068a88f0fad758c2053514 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Tue, 16 Nov 2021 10:17:26 +0100 Subject: [PATCH] improve csrf handling (#2936) --- src/Controller/CustomerController.php | 30 +++++++++++++--- src/Controller/DoctorController.php | 2 +- src/Controller/InvoiceController.php | 14 ++++++-- src/Controller/PermissionController.php | 2 +- src/Controller/ProjectController.php | 30 +++++++++++++--- .../Actions/InvoiceTemplateSubscriber.php | 2 +- templates/customer/details.html.twig | 2 +- templates/embeds/comments.html.twig | 6 ++-- templates/invoice/actions.html.twig | 2 +- templates/project/details.html.twig | 2 +- tests/Controller/CustomerControllerTest.php | 34 ++++++++++++++++--- tests/Controller/InvoiceControllerTest.php | 4 ++- tests/Controller/ProjectControllerTest.php | 12 ++++--- translations/flashmessages.de.xlf | 4 +++ translations/flashmessages.en.xlf | 4 +++ 15 files changed, 119 insertions(+), 31 deletions(-) diff --git a/src/Controller/CustomerController.php b/src/Controller/CustomerController.php index 837884b06e..c9f5b76ca5 100644 --- a/src/Controller/CustomerController.php +++ b/src/Controller/CustomerController.php @@ -41,6 +41,8 @@ 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 @@ public function teamPermissionsAction(Customer $customer, Request $request) } /** - * @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 @@ public function addCommentAction(Customer $customer, Request $request) } /** - * @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 @@ public function pinCommentAction(CustomerComment $comment) $this->flashUpdateException($ex); } - return $this->redirectToRoute('customer_details', ['id' => $comment->getCustomer()->getId()]); + return $this->redirectToRoute('customer_details', ['id' => $customerId]); } /** diff --git a/src/Controller/DoctorController.php b/src/Controller/DoctorController.php index 7e1b56254a..83e723554d 100644 --- a/src/Controller/DoctorController.php +++ b/src/Controller/DoctorController.php @@ -64,7 +64,7 @@ public function __construct(string $projectDirectory, string $kernelEnvironment, 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'); } diff --git a/src/Controller/InvoiceController.php b/src/Controller/InvoiceController.php index 1d160217b1..03d53949f5 100644 --- a/src/Controller/InvoiceController.php +++ b/src/Controller/InvoiceController.php @@ -260,7 +260,7 @@ public function changeStatusAction(Invoice $invoice, string $status, Request $re 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 @@ public function createTemplateAction(Request $request, ?InvoiceTemplate $copyFro } /** - * @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'); diff --git a/src/Controller/PermissionController.php b/src/Controller/PermissionController.php index a0ba581da4..3d7df9e29f 100644 --- a/src/Controller/PermissionController.php +++ b/src/Controller/PermissionController.php @@ -209,7 +209,7 @@ public function createRole(Request $request): Response 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'); } diff --git a/src/Controller/ProjectController.php b/src/Controller/ProjectController.php index 39d07e708d..c2800c6aee 100644 --- a/src/Controller/ProjectController.php +++ b/src/Controller/ProjectController.php @@ -43,6 +43,8 @@ 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 @@ public function createAction(Request $request, ?Customer $customer = null) } /** - * @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 @@ public function addCommentAction(Project $project, Request $request) } /** - * @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 @@ public function pinCommentAction(ProjectComment $comment) $this->flashUpdateException($ex); } - return $this->redirectToRoute('project_details', ['id' => $comment->getProject()->getId()]); + return $this->redirectToRoute('project_details', ['id' => $projectId]); } /** diff --git a/src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php b/src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php index 0ccf3868ea..f7a8281b06 100644 --- a/src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php +++ b/src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php @@ -36,7 +36,7 @@ public function onActions(PageActionsEvent $event): void } $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); } } } diff --git a/templates/customer/details.html.twig b/templates/customer/details.html.twig index f200d09cfc..097c444501 100644 --- a/templates/customer/details.html.twig +++ b/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 %} diff --git a/templates/embeds/comments.html.twig b/templates/embeds/comments.html.twig index 25084b82cf..99d97d433c 100644 --- a/templates/embeds/comments.html.twig +++ b/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 @@ {% if route_pin is not null %} - + {% elseif comment.pinned %} {% endif %} {% if route_delete is not null and ((not delete_by_user) or (delete_by_user and comment.createdBy.id == app.user.id)) %} - + {% endif %} diff --git a/templates/invoice/actions.html.twig b/templates/invoice/actions.html.twig index d1fa98c4b4..e654110c3b 100644 --- a/templates/invoice/actions.html.twig +++ b/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 %} diff --git a/templates/project/details.html.twig b/templates/project/details.html.twig index 1936551c34..a4d2ec4b2b 100644 --- a/templates/project/details.html.twig +++ b/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 %} diff --git a/tests/Controller/CustomerControllerTest.php b/tests/Controller/CustomerControllerTest.php index f1d9f2cb69..2d319e229c 100644 --- a/tests/Controller/CustomerControllerTest.php +++ b/tests/Controller/CustomerControllerTest.php @@ -176,21 +176,45 @@ public function testDeleteCommentAction() ]); $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'); + $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->assertRouteNotFound($client); + } + public function testPinCommentAction() { $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); @@ -211,12 +235,14 @@ public function testPinCommentAction() $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() diff --git a/tests/Controller/InvoiceControllerTest.php b/tests/Controller/InvoiceControllerTest.php index 56cdafd9fa..442f18c383 100644 --- a/tests/Controller/InvoiceControllerTest.php +++ b/tests/Controller/InvoiceControllerTest.php @@ -395,7 +395,9 @@ public function testDeleteTemplateAction() $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(); diff --git a/tests/Controller/ProjectControllerTest.php b/tests/Controller/ProjectControllerTest.php index 98dd58823d..a14a34ba28 100644 --- a/tests/Controller/ProjectControllerTest.php +++ b/tests/Controller/ProjectControllerTest.php @@ -261,8 +261,10 @@ public function testDeleteCommentAction() $comments = $this->getEntityManager()->getRepository(ProjectComment::class)->findAll(); $id = $comments[0]->getId(); - self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_delete'), $node->attr('href')); - $this->request($client, '/admin/project/' . $id . '/comment_delete'); + $token = self::$container->get('security.csrf.token_manager')->getToken('project.delete_comment'); + + 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 @@ public function testPinCommentAction() $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() diff --git a/translations/flashmessages.de.xlf b/translations/flashmessages.de.xlf index 55bbd83ef5..dbe4f2f129 100644 --- a/translations/flashmessages.de.xlf +++ b/translations/flashmessages.de.xlf @@ -54,6 +54,10 @@ action.upload.error Die Datei konnte nicht hochgeladen bzw. gespeichert werden: %reason% + + action.csrf.error + Die Aktion konnte nicht durchgeführt werden: ungültiges Sicherheitstoken. + diff --git a/translations/flashmessages.en.xlf b/translations/flashmessages.en.xlf index 3eee9f3c67..8a86bf9944 100644 --- a/translations/flashmessages.en.xlf +++ b/translations/flashmessages.en.xlf @@ -54,6 +54,10 @@ action.upload.error The file could not be uploaded or saved: %reason% + + action.csrf.error + The action could not be performed: invalid security token. +