From 56d02673eddacf617e15f617af39db51bb840eb3 Mon Sep 17 00:00:00 2001 From: tdozbun-reno <94628229+tdozbun-reno@users.noreply.github.com> Date: Thu, 18 Nov 2021 11:32:00 -0600 Subject: [PATCH] Update CSRF Token IDs for Issue kevinpapst/kimai2#2947 (#2948) * bump version * removed not needed token, as it is already contained in the form Co-authored-by: Kevin Papst --- src/Constants.php | 4 +-- src/Controller/CustomerController.php | 4 +-- src/Controller/DoctorController.php | 2 +- src/Controller/InvoiceController.php | 4 +-- src/Controller/ProjectController.php | 6 ++-- src/Controller/TeamController.php | 2 +- .../TimesheetAbstractController.php | 6 ++-- src/Controller/TimesheetController.php | 20 +++-------- src/Controller/TimesheetTeamController.php | 20 +++-------- .../Actions/AbstractTimesheetSubscriber.php | 2 +- templates/timesheet-team/actions.html.twig | 2 +- templates/timesheet/actions.html.twig | 2 +- tests/Controller/CustomerControllerTest.php | 4 ++- tests/Controller/ProjectControllerTest.php | 4 ++- tests/Controller/TimesheetControllerTest.php | 33 +------------------ .../TimesheetTeamControllerTest.php | 33 +------------------ 16 files changed, 35 insertions(+), 113 deletions(-) diff --git a/src/Constants.php b/src/Constants.php index 379a31d4..8338ea63 100644 --- a/src/Constants.php +++ b/src/Constants.php @@ -17,11 +17,11 @@ class Constants /** * The current release version */ - public const VERSION = '1.16.2'; + public const VERSION = '1.16.3'; /** * The current release: major * 10000 + minor * 100 + patch */ - public const VERSION_ID = 11602; + public const VERSION_ID = 11603; /** * The current release status, either "stable" or "dev" */ diff --git a/src/Controller/CustomerController.php b/src/Controller/CustomerController.php index c9f5b76c..faefbeb7 100644 --- a/src/Controller/CustomerController.php +++ b/src/Controller/CustomerController.php @@ -172,7 +172,7 @@ final class CustomerController extends AbstractController return $this->redirectToRoute('customer_details', ['id' => $customerId]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('customer.delete_comment'); try { $this->repository->deleteComment($comment); @@ -219,7 +219,7 @@ final class CustomerController extends AbstractController return $this->redirectToRoute('customer_details', ['id' => $customerId]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('customer.pin_comment'); $comment->setPinned(!$comment->isPinned()); try { diff --git a/src/Controller/DoctorController.php b/src/Controller/DoctorController.php index 83e72355..bc84740a 100644 --- a/src/Controller/DoctorController.php +++ b/src/Controller/DoctorController.php @@ -69,7 +69,7 @@ class DoctorController extends AbstractController return $this->redirectToRoute('doctor'); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('doctor.flush_log'); $logfile = $this->getLogFilename(); diff --git a/src/Controller/InvoiceController.php b/src/Controller/InvoiceController.php index 03d53949..6d721f8b 100644 --- a/src/Controller/InvoiceController.php +++ b/src/Controller/InvoiceController.php @@ -265,7 +265,7 @@ final class InvoiceController extends AbstractController return $this->redirectToRoute('admin_invoice_list'); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('invoice.delete'); try { $this->service->deleteInvoice($invoice); @@ -462,7 +462,7 @@ final class InvoiceController extends AbstractController return $this->redirectToRoute('admin_invoice_template'); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('invoice.delete_template'); try { $this->templateRepository->removeTemplate($template); diff --git a/src/Controller/ProjectController.php b/src/Controller/ProjectController.php index 50f35d87..75e21560 100644 --- a/src/Controller/ProjectController.php +++ b/src/Controller/ProjectController.php @@ -194,7 +194,7 @@ final class ProjectController extends AbstractController return $this->redirectToRoute('project_details', ['id' => $projectId]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('project.delete_comment'); try { $this->repository->deleteComment($comment); @@ -241,7 +241,7 @@ final class ProjectController extends AbstractController return $this->redirectToRoute('project_details', ['id' => $projectId]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('project.pin_comment'); $comment->setPinned(!$comment->isPinned()); try { @@ -432,7 +432,7 @@ final class ProjectController extends AbstractController return $this->redirectToRoute('project_details', ['id' => $project->getId()]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('project.duplicate'); $newProject = $projectDuplicationService->duplicate($project, $project->getName() . ' [COPY]'); diff --git a/src/Controller/TeamController.php b/src/Controller/TeamController.php index 3181f753..62754b94 100644 --- a/src/Controller/TeamController.php +++ b/src/Controller/TeamController.php @@ -94,7 +94,7 @@ final class TeamController extends AbstractController return $this->redirectToRoute('admin_team_edit', ['id' => $team->getId()]); } - $csrfTokenManager->refreshToken($token); + $csrfTokenManager->refreshToken('team.duplicate'); $newTeam = clone $team; $newTeam->setName($team->getName() . ' [COPY]'); diff --git a/src/Controller/TimesheetAbstractController.php b/src/Controller/TimesheetAbstractController.php index 2677a586..a536a1f2 100644 --- a/src/Controller/TimesheetAbstractController.php +++ b/src/Controller/TimesheetAbstractController.php @@ -211,14 +211,14 @@ abstract class TimesheetAbstractController extends AbstractController ]); } - protected function duplicate(Timesheet $timesheet, Request $request, string $renderTemplate, string $token): Response + protected function duplicate(Timesheet $timesheet, Request $request, string $renderTemplate): Response { $copyTimesheet = clone $timesheet; $event = new TimesheetMetaDefinitionEvent($copyTimesheet); $this->dispatcher->dispatch($event); - $form = $this->getDuplicateForm($copyTimesheet, $timesheet, $token); + $form = $this->getDuplicateForm($copyTimesheet, $timesheet); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { @@ -612,7 +612,7 @@ abstract class TimesheetAbstractController extends AbstractController return $query; } - abstract protected function getDuplicateForm(Timesheet $entry, Timesheet $original, string $token): FormInterface; + abstract protected function getDuplicateForm(Timesheet $entry, Timesheet $original): FormInterface; abstract protected function getCreateForm(Timesheet $entry): FormInterface; } diff --git a/src/Controller/TimesheetController.php b/src/Controller/TimesheetController.php index 4f5bbad4..df14b2cc 100644 --- a/src/Controller/TimesheetController.php +++ b/src/Controller/TimesheetController.php @@ -21,8 +21,6 @@ use Symfony\Component\Form\FormInterface; 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; /** * @Route(path="/timesheet") @@ -62,20 +60,12 @@ class TimesheetController extends TimesheetAbstractController } /** - * @Route(path="/{id}/duplicate/{token}", name="timesheet_duplicate", methods={"GET", "POST"}) + * @Route(path="/{id}/duplicate", name="timesheet_duplicate", methods={"GET", "POST"}) * @Security("is_granted('duplicate', entry)") */ - public function duplicateAction(Timesheet $entry, Request $request, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response + public function duplicateAction(Timesheet $entry, Request $request): Response { - if (!$csrfTokenManager->isTokenValid(new CsrfToken('timesheet.duplicate', $token))) { - $this->flashError('action.csrf.error'); - - return $this->redirectToRoute('timesheet'); - } - - $csrfTokenManager->refreshToken($token); - - return $this->duplicate($entry, $request, 'timesheet/edit.html.twig', $token); + return $this->duplicate($entry, $request, 'timesheet/edit.html.twig'); } /** @@ -110,8 +100,8 @@ class TimesheetController extends TimesheetAbstractController return $this->generateCreateForm($entry, TimesheetEditForm::class, $this->generateUrl('timesheet_create')); } - protected function getDuplicateForm(Timesheet $entry, Timesheet $original, string $token): FormInterface + protected function getDuplicateForm(Timesheet $entry, Timesheet $original): FormInterface { - return $this->generateCreateForm($entry, TimesheetEditForm::class, $this->generateUrl('timesheet_duplicate', ['id' => $original->getId(), 'token' => $token])); + return $this->generateCreateForm($entry, TimesheetEditForm::class, $this->generateUrl('timesheet_duplicate', ['id' => $original->getId()])); } } diff --git a/src/Controller/TimesheetTeamController.php b/src/Controller/TimesheetTeamController.php index 3a073315..9cd14726 100644 --- a/src/Controller/TimesheetTeamController.php +++ b/src/Controller/TimesheetTeamController.php @@ -28,8 +28,6 @@ use Symfony\Component\Form\FormInterface; 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; /** * @Route(path="/team/timesheet") @@ -73,20 +71,12 @@ class TimesheetTeamController extends TimesheetAbstractController } /** - * @Route(path="/{id}/duplicate/{token}", name="admin_timesheet_duplicate", methods={"GET", "POST"}) + * @Route(path="/{id}/duplicate", name="admin_timesheet_duplicate", methods={"GET", "POST"}) * @Security("is_granted('duplicate', entry)") */ - public function duplicateAction(Timesheet $entry, Request $request, string $token, CsrfTokenManagerInterface $csrfTokenManager): Response + public function duplicateAction(Timesheet $entry, Request $request): Response { - if (!$csrfTokenManager->isTokenValid(new CsrfToken('admin_timesheet.duplicate', $token))) { - $this->flashError('action.csrf.error'); - - return $this->redirectToRoute('admin_timesheet'); - } - - $csrfTokenManager->refreshToken($token); - - return $this->duplicate($entry, $request, 'timesheet-team/edit.html.twig', $token); + return $this->duplicate($entry, $request, 'timesheet-team/edit.html.twig'); } /** @@ -205,9 +195,9 @@ class TimesheetTeamController extends TimesheetAbstractController return $this->generateCreateForm($entry, TimesheetAdminEditForm::class, $this->generateUrl('admin_timesheet_create')); } - protected function getDuplicateForm(Timesheet $entry, Timesheet $original, string $token): FormInterface + protected function getDuplicateForm(Timesheet $entry, Timesheet $original): FormInterface { - return $this->generateCreateForm($entry, TimesheetAdminEditForm::class, $this->generateUrl('admin_timesheet_duplicate', ['id' => $original->getId(), 'token' => $token])); + return $this->generateCreateForm($entry, TimesheetAdminEditForm::class, $this->generateUrl('admin_timesheet_duplicate', ['id' => $original->getId()])); } protected function getPermissionEditExport(): string diff --git a/src/EventSubscriber/Actions/AbstractTimesheetSubscriber.php b/src/EventSubscriber/Actions/AbstractTimesheetSubscriber.php index 47eb8202..24b96ed8 100644 --- a/src/EventSubscriber/Actions/AbstractTimesheetSubscriber.php +++ b/src/EventSubscriber/Actions/AbstractTimesheetSubscriber.php @@ -39,7 +39,7 @@ abstract class AbstractTimesheetSubscriber extends AbstractActionsSubscriber if ($this->isGranted('duplicate', $timesheet)) { $class = $event->isView('edit') ? '' : 'modal-ajax-form'; - $event->addAction('copy', ['url' => $this->path($routeDuplicate, ['id' => $timesheet->getId(), 'token' => $payload['token']]), 'class' => $class]); + $event->addAction('copy', ['url' => $this->path($routeDuplicate, ['id' => $timesheet->getId()]), 'class' => $class]); } if ($event->countActions() > 0) { diff --git a/templates/timesheet-team/actions.html.twig b/templates/timesheet-team/actions.html.twig index a4b0aef3..406a9c87 100644 --- a/templates/timesheet-team/actions.html.twig +++ b/templates/timesheet-team/actions.html.twig @@ -6,7 +6,7 @@ {% macro timesheet_team(timesheet, view) %} {% import "macros/widgets.html.twig" as widgets %} - {% set event = actions(app.user, 'timesheet_team', view, {'timesheet': timesheet, 'token': csrf_token('admin_timesheet.duplicate')}) %} + {% set event = actions(app.user, 'timesheet_team', view, {'timesheet': timesheet}) %} {% if view == 'index' or view == 'custom' %} {{ widgets.table_actions(event.actions) }} {% else %} diff --git a/templates/timesheet/actions.html.twig b/templates/timesheet/actions.html.twig index 525ec5e8..521bda16 100644 --- a/templates/timesheet/actions.html.twig +++ b/templates/timesheet/actions.html.twig @@ -6,7 +6,7 @@ {% macro timesheet(timesheet, view, options) %} {% import "macros/widgets.html.twig" as widgets %} - {% set event = actions(app.user, 'timesheet', view, {'timesheet': timesheet, 'token': csrf_token('timesheet.duplicate')}) %} + {% set event = actions(app.user, 'timesheet', view, {'timesheet': timesheet}) %} {% if view == 'index' or view == 'custom' %} {{ widgets.table_actions(event.actions) }} {% else %} diff --git a/tests/Controller/CustomerControllerTest.php b/tests/Controller/CustomerControllerTest.php index 2d319e22..92f5c9a5 100644 --- a/tests/Controller/CustomerControllerTest.php +++ b/tests/Controller/CustomerControllerTest.php @@ -241,8 +241,10 @@ class CustomerControllerTest extends ControllerBaseTest $this->assertIsRedirect($client, $this->createUrl('/admin/customer/1/details')); $client->followRedirect(); $node = $client->getCrawler()->filter('div.box#comments_box .box-body a.btn.active'); + $token2 = self::$container->get('security.csrf.token_manager')->getToken('customer.pin_comment'); self::assertEquals(1, $node->count()); - self::assertEquals($this->createUrl('/admin/customer/' . $id . '/comment_pin/' . $token), $node->attr('href')); + self::assertEquals($this->createUrl('/admin/customer/' . $id . '/comment_pin/' . $token2), $node->attr('href')); + self::assertNotEquals($token, $token2); } public function testCreateDefaultTeamAction() diff --git a/tests/Controller/ProjectControllerTest.php b/tests/Controller/ProjectControllerTest.php index 6a0eb488..c6f01b80 100644 --- a/tests/Controller/ProjectControllerTest.php +++ b/tests/Controller/ProjectControllerTest.php @@ -318,8 +318,10 @@ class ProjectControllerTest extends ControllerBaseTest $this->assertIsRedirect($client, $this->createUrl('/admin/project/1/details')); $client->followRedirect(); $node = $client->getCrawler()->filter('div.box#comments_box .box-body a.btn.active'); + $token2 = self::$container->get('security.csrf.token_manager')->getToken('project.pin_comment'); self::assertEquals(1, $node->count()); - self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_pin/' . $token), $node->attr('href')); + self::assertEquals($this->createUrl('/admin/project/' . $id . '/comment_pin/' . $token2), $node->attr('href')); + self::assertNotEquals($token, $token2); } public function testCreateDefaultTeamAction() diff --git a/tests/Controller/TimesheetControllerTest.php b/tests/Controller/TimesheetControllerTest.php index 02d65efc..2dede1e8 100644 --- a/tests/Controller/TimesheetControllerTest.php +++ b/tests/Controller/TimesheetControllerTest.php @@ -708,9 +708,7 @@ class TimesheetControllerTest extends ControllerBaseTest $ids = $this->importFixture($fixture); $newId = $ids[0]->getId(); - $token = self::$container->get('security.csrf.token_manager')->getToken('timesheet.duplicate'); - - $this->request($client, '/timesheet/' . $newId . '/duplicate/' . $token); + $this->request($client, '/timesheet/' . $newId . '/duplicate'); $this->assertTrue($client->getResponse()->isSuccessful()); $form = $client->getCrawler()->filter('form[name=timesheet_edit_form]')->form(); @@ -732,33 +730,4 @@ class TimesheetControllerTest extends ControllerBaseTest $this->assertEquals(2016, $timesheet->getFixedRate()); $this->assertEquals(2016, $timesheet->getRate()); } - - public function testDuplicateActionWithInvalidCsrf() - { - $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); - $dateTime = new DateTimeFactory(new \DateTimeZone('Europe/London')); - - $fixture = new TimesheetFixtures(); - $fixture->setAmount(1); - $fixture->setAmountRunning(0); - $fixture->setUser($this->getUserByRole(User::ROLE_USER)); - $fixture->setStartDate($dateTime->createDateTime()); - $fixture->setCallback(function (Timesheet $timesheet) { - $timesheet->setDescription('Testing is fun!'); - $begin = clone $timesheet->getBegin(); - $begin->setTime(0, 0, 0); - $timesheet->setBegin($begin); - $end = clone $timesheet->getBegin(); - $end->modify('+ 8 hours'); - $timesheet->setEnd($end); - $timesheet->setFixedRate(2016); - $timesheet->setHourlyRate(127); - }); - - /** @var Timesheet[] $ids */ - $ids = $this->importFixture($fixture); - $newId = $ids[0]->getId(); - - $this->assertInvalidCsrfToken($client, '/timesheet/' . $newId . '/duplicate/dfghdfghdfghdfghdfgh', $this->createUrl('/timesheet/')); - } } diff --git a/tests/Controller/TimesheetTeamControllerTest.php b/tests/Controller/TimesheetTeamControllerTest.php index c4b21743..44232638 100644 --- a/tests/Controller/TimesheetTeamControllerTest.php +++ b/tests/Controller/TimesheetTeamControllerTest.php @@ -428,9 +428,7 @@ class TimesheetTeamControllerTest extends ControllerBaseTest $ids = $this->importFixture($fixture); $newId = $ids[0]->getId(); - $token = self::$container->get('security.csrf.token_manager')->getToken('admin_timesheet.duplicate'); - - $this->request($client, '/team/timesheet/' . $newId . '/duplicate/' . $token); + $this->request($client, '/team/timesheet/' . $newId . '/duplicate'); $this->assertTrue($client->getResponse()->isSuccessful()); $form = $client->getCrawler()->filter('form[name=timesheet_admin_edit_form]')->form(); @@ -452,33 +450,4 @@ class TimesheetTeamControllerTest extends ControllerBaseTest $this->assertEquals(2016, $timesheet->getFixedRate()); $this->assertEquals(2016, $timesheet->getRate()); } - - public function testDuplicateActionWithInvalidCsrf() - { - $client = $this->getClientForAuthenticatedUser(User::ROLE_ADMIN); - $dateTime = new DateTimeFactory(new \DateTimeZone('Europe/London')); - - $fixture = new TimesheetFixtures(); - $fixture->setAmount(1); - $fixture->setAmountRunning(0); - $fixture->setUser($this->getUserByRole(User::ROLE_USER)); - $fixture->setStartDate($dateTime->createDateTime()); - $fixture->setCallback(function (Timesheet $timesheet) { - $timesheet->setDescription('Testing is fun!'); - $begin = clone $timesheet->getBegin(); - $begin->setTime(0, 0, 0); - $timesheet->setBegin($begin); - $end = clone $timesheet->getBegin(); - $end->modify('+ 8 hours'); - $timesheet->setEnd($end); - $timesheet->setFixedRate(2016); - $timesheet->setHourlyRate(127); - }); - - /** @var Timesheet[] $ids */ - $ids = $this->importFixture($fixture); - $newId = $ids[0]->getId(); - - $this->assertInvalidCsrfToken($client, '/team/timesheet/' . $newId . '/duplicate/dfghdfghdfghdfghdfgh', $this->createUrl('/team/timesheet/')); - } }