Skip to content

Commit

Permalink
more csrf protection for invoice and search (#2984)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevinpapst
kevinpapst committed Dec 2, 2021
1 parent b0045a9 commit 4e42911
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 20 deletions.
11 changes: 11 additions & 0 deletions src/Controller/AbstractController.php
View file
Expand Up @@ -207,6 +207,17 @@ protected function handleSearch(FormInterface $form, Request $request): bool
throw new \InvalidArgumentException('handleSearchForm() requires an instanceof BaseQuery as form data');
}

$actions = ['resetSearchFilter', 'removeDefaultQuery', 'setDefaultQuery'];
foreach ($actions as $action) {
if ($request->query->has($action)) {
if (!$this->isCsrfTokenValid('search', $request->query->get('_token'))) {
$this->flashError('action.csrf.error');

return false;
}
}
}

if ($request->query->has('resetSearchFilter')) {
$data->resetFilter();
$this->removeLastSearch($data);
Expand Down
47 changes: 40 additions & 7 deletions src/Controller/InvoiceController.php
View file
Expand Up @@ -76,6 +76,13 @@ public function indexAction(Request $request, SystemConfiguration $configuration
}

$query = $this->getDefaultQuery();

$token = null;
if ($request->query->has('token')) {
$token = $request->query->get('token');
$request->query->remove('token');
}

$form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
if ($this->handleSearch($form, $request)) {
return $this->redirectToRoute('invoice');
Expand All @@ -87,6 +94,12 @@ public function indexAction(Request $request, SystemConfiguration $configuration

if ($form->isValid() && $this->isGranted('create_invoice')) {
if ($request->query->has('createInvoice')) {
if (!$this->isCsrfTokenValid('invoice.create', $token)) {
$this->flashError('action.csrf.error');

return $this->redirectToRoute('invoice');
}

try {
return $this->renderInvoice($query, $request);
} catch (Exception $ex) {
Expand Down Expand Up @@ -160,6 +173,18 @@ public function createInvoiceAction(Customer $customer, InvoiceTemplate $templat
return $this->redirectToRoute('invoice');
}

$token = null;
if ($request->query->has('token')) {
$token = $request->query->get('token');
$request->query->remove('token');
}

if (!$this->isCsrfTokenValid('invoice.create', $token)) {
$this->flashError('action.csrf.error');

return $this->redirectToRoute('invoice');
}

$query = $this->getDefaultQuery();
$form = $this->getToolbarForm($query, $configuration->find('invoice.simple_form'));
$form->submit($request->query->all(), false);
Expand All @@ -177,14 +202,22 @@ public function createInvoiceAction(Customer $customer, InvoiceTemplate $templat
}

/**
* @Route(path="/change-status/{id}/{status}", name="admin_invoice_status", methods={"GET", "POST"})
* @Route(path="/change-status/{id}/{status}/{token}", name="admin_invoice_status", methods={"GET", "POST"})
* @Security("is_granted('access', invoice.getCustomer())")
* @Security("is_granted('create_invoice')")
*/
public function changeStatusAction(Invoice $invoice, string $status, Request $request): Response
public function changeStatusAction(Invoice $invoice, string $status, string $token, Request $request, CsrfTokenManagerInterface $csrfTokenManager): Response
{
if (!$csrfTokenManager->isTokenValid(new CsrfToken('invoice.status', $token))) {
$this->flashError('action.csrf.error');

return $this->redirectToRoute('admin_invoice_list');
}

$token = $csrfTokenManager->refreshToken('invoice.status');

if ($status === Invoice::STATUS_PAID) {
$form = $this->createPaymentDateForm($invoice, $status);
$form = $this->createPaymentDateForm($invoice, $status, $token->getValue());
$form->handleRequest($request);

if (!$form->isSubmitted() || !$form->isValid()) {
Expand Down Expand Up @@ -212,13 +245,13 @@ 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))) {
if (!$csrfTokenManager->isTokenValid(new CsrfToken('invoice.status', $token))) {
$this->flashError('action.csrf.error');

return $this->redirectToRoute('admin_invoice_list');
}

$csrfTokenManager->refreshToken('invoice.delete');
$csrfTokenManager->refreshToken('invoice.status');

try {
$this->service->deleteInvoice($invoice);
Expand Down Expand Up @@ -624,13 +657,13 @@ private function createEditForm(InvoiceTemplate $template): FormInterface
]);
}

private function createPaymentDateForm(Invoice $invoice, string $status): FormInterface
private function createPaymentDateForm(Invoice $invoice, string $status, string $token): FormInterface
{
if (null === $invoice->getPaymentDate()) {
$invoice->setPaymentDate($this->getDateTimeFactory()->createDateTime());
}

$url = $this->generateUrl('admin_invoice_status', ['id' => $invoice->getId(), 'status' => $status]);
$url = $this->generateUrl('admin_invoice_status', ['id' => $invoice->getId(), 'status' => $status, 'token' => $token]);

return $this->createForm(InvoicePaymentDateForm::class, $invoice, [
'action' => $url,
Expand Down
6 changes: 3 additions & 3 deletions src/EventSubscriber/Actions/InvoiceSubscriber.php
View file
Expand Up @@ -31,15 +31,15 @@ public function onActions(PageActionsEvent $event): void
}

if (!$invoice->isPending()) {
$event->addAction('invoice.pending', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'pending'])]);
$event->addAction('invoice.pending', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'pending', 'token' => $payload['token']])]);
} else {
$event->addAction('invoice.paid', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'paid']), 'class' => 'modal-ajax-form']);
$event->addAction('invoice.paid', ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'paid', 'token' => $payload['token']]), 'class' => 'modal-ajax-form']);
}

$allowDelete = $this->isGranted('delete_invoice');
if (!$invoice->isCanceled()) {
$id = $allowDelete ? 'invoice.cancel' : 'trash';
$event->addAction($id, ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'canceled']), 'title' => 'invoice.cancel', 'translation_domain' => 'actions']);
$event->addAction($id, ['url' => $this->path('admin_invoice_status', ['id' => $invoice->getId(), 'status' => 'canceled', 'token' => $payload['token']]), 'title' => 'invoice.cancel', 'translation_domain' => 'actions']);
}

$event->addDivider();
Expand Down
2 changes: 1 addition & 1 deletion templates/invoice/actions.html.twig
View file
Expand Up @@ -6,7 +6,7 @@

{% macro invoice(invoice, view) %}
{% import "macros/widgets.html.twig" as widgets %}
{% set event = actions(app.user, 'invoice', view, {'invoice': invoice, 'token': csrf_token('invoice.delete')}) %}
{% set event = actions(app.user, 'invoice', view, {'invoice': invoice, 'token': csrf_token('invoice.status')}) %}
{{ widgets.table_actions(event.actions) }}
{% endmacro %}

Expand Down
4 changes: 2 additions & 2 deletions templates/invoice/index.html.twig
View file
Expand Up @@ -218,7 +218,7 @@
const overwrites = {'customers[]': link.dataset['customer'], 'template': link.dataset['template']};
const uri = formPlugin.convertFormDataToQueryString(document.getElementById('{{ formId }}'), overwrites);
link.href = link.dataset['href'] + '?' + uri;
link.href = link.dataset['href'] + '?token={{ csrf_token('invoice.create') }}&' + uri;
return true;
}
Expand All @@ -228,7 +228,7 @@
const formPlugin = kimai.getPlugin('form');
const uri = formPlugin.convertFormDataToQueryString(document.getElementById('{{ formId }}'));
link.href = '{{ path('invoice') }}?createInvoice=true&' + uri;
link.href = '{{ path('invoice') }}?token={{ csrf_token('invoice.create') }}&createInvoice=true&' + uri;
return true;
}
Expand Down
3 changes: 2 additions & 1 deletion templates/macros/search.html.twig
View file
Expand Up @@ -40,6 +40,7 @@
{% endmacro %}

{% macro searchButton(form) %}
<input type="hidden" name="_token" value="{{ csrf_token('search') }}">
<div class="btn-group">
<button type="submit" name="performSearch" value="performSearch" class="btn btn-primary pull-left" data-type="submit">{{ 'search'|trans }}</button>
</div>
Expand All @@ -56,7 +57,7 @@
</button>
<ul class="dropdown-menu">
<li>
<a href="?removeDefaultQuery={{ form.vars.data.bookmark.name }}" id="removeDefaultQuery">{{ 'label.remove_default'|trans }}</a>
<a href="?_token={{ csrf_token('search') }}&removeDefaultQuery={{ form.vars.data.bookmark.name }}" id="removeDefaultQuery">{{ 'label.remove_default'|trans }}</a>
</li>
</ul>
{% else %}
Expand Down
20 changes: 14 additions & 6 deletions tests/Controller/InvoiceControllerTest.php
View file
Expand Up @@ -190,7 +190,9 @@ public function testCreateAction()
'markAsExported' => 1,
];

$action = '/invoice/save-invoice/1/' . $template->getId() . '?' . http_build_query($urlParams);
$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.create');

$action = '/invoice/save-invoice/1/' . $template->getId() . '?token=' . $token->getValue() . '&' . http_build_query($urlParams);
$this->request($client, $action);
$this->assertIsRedirect($client);
$this->assertRedirectUrl($client, '/invoice/show?id=', false);
Expand Down Expand Up @@ -286,9 +288,11 @@ public function testCreateActionAsAdminWithDownloadAndStatusChange()
// but the datatable with all timesheets
$this->assertDataTableRowCount($client, 'datatable_invoice', 20);

$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.create');

$form = $client->getCrawler()->filter('#invoice-print-form')->form();
$node = $form->getFormNode();
$node->setAttribute('action', $this->createUrl('/invoice/?createInvoice=true'));
$node->setAttribute('action', $this->createUrl('/invoice/?createInvoice=true&token=' . $token->getValue()));
$node->setAttribute('method', 'GET');
$client->submit($form, [
'template' => $template->getId(),
Expand Down Expand Up @@ -317,17 +321,20 @@ public function testCreateActionAsAdminWithDownloadAndStatusChange()
self::assertInstanceOf(BinaryFileResponse::class, $response);
self::assertFileExists($response->getFile());

$this->request($client, '/invoice/change-status/' . $id . '/pending');
$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.status');
$this->request($client, '/invoice/change-status/' . $id . '/pending/' . $token->getValue());
$this->assertIsRedirect($client, '/invoice/show');
$client->followRedirect();
$this->assertTrue($client->getResponse()->isSuccessful());

$this->request($client, '/invoice/change-status/' . $id . '/paid');
$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.status');
$this->request($client, '/invoice/change-status/' . $id . '/paid/' . $token->getValue());
$this->assertTrue($client->getResponse()->isSuccessful());

$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.status');
$this->assertHasValidationError(
$client,
'/invoice/change-status/' . $id . '/paid',
'/invoice/change-status/' . $id . '/paid/' . $token->getValue(),
'form[name=invoice_payment_date_form]',
[
'invoice_payment_date_form' => [
Expand All @@ -350,7 +357,8 @@ public function testCreateActionAsAdminWithDownloadAndStatusChange()
$client->followRedirect();
$this->assertTrue($client->getResponse()->isSuccessful());

$this->request($client, '/invoice/change-status/' . $id . '/new');
$token = self::$container->get('security.csrf.token_manager')->getToken('invoice.status');
$this->request($client, '/invoice/change-status/' . $id . '/new/' . $token->getValue());
$this->assertIsRedirect($client, '/invoice/show');
$client->followRedirect();
$this->assertTrue($client->getResponse()->isSuccessful());
Expand Down

0 comments on commit 4e42911

Please sign in to comment.