Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
improve csrf handling (#2936)
  • Loading branch information
kevinpapst committed Nov 16, 2021
1 parent a199249 commit 95796ab
Show file tree
Hide file tree
Showing 15 changed files with 119 additions and 31 deletions.
30 changes: 25 additions & 5 deletions src/Controller/CustomerController.php
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -196,19 +206,29 @@ 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);
} catch (\Exception $ex) {
$this->flashUpdateException($ex);
}

return $this->redirectToRoute('customer_details', ['id' => $comment->getCustomer()->getId()]);
return $this->redirectToRoute('customer_details', ['id' => $customerId]);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Controller/DoctorController.php
Expand Up @@ -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');
}
Expand Down
14 changes: 11 additions & 3 deletions src/Controller/InvoiceController.php
Expand Up @@ -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');
}
Expand Down Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion src/Controller/PermissionController.php
Expand Up @@ -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');
}
Expand Down
30 changes: 25 additions & 5 deletions src/Controller/ProjectController.php
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -218,19 +228,29 @@ 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);
} catch (\Exception $ex) {
$this->flashUpdateException($ex);
}

return $this->redirectToRoute('project_details', ['id' => $comment->getProject()->getId()]);
return $this->redirectToRoute('project_details', ['id' => $projectId]);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/EventSubscriber/Actions/InvoiceTemplateSubscriber.php
Expand Up @@ -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);
}
}
}
2 changes: 1 addition & 1 deletion templates/customer/details.html.twig
Expand Up @@ -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 %}
Expand Down
6 changes: 3 additions & 3 deletions 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 %}
Expand All @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion templates/invoice/actions.html.twig
Expand Up @@ -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 %}
2 changes: 1 addition & 1 deletion templates/project/details.html.twig
Expand Up @@ -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 %}
Expand Down
34 changes: 30 additions & 4 deletions tests/Controller/CustomerControllerTest.php
Expand Up @@ -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);
Expand All @@ -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()
Expand Down
4 changes: 3 additions & 1 deletion tests/Controller/InvoiceControllerTest.php
Expand Up @@ -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();

Expand Down
12 changes: 8 additions & 4 deletions tests/Controller/ProjectControllerTest.php
Expand Up @@ -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');
Expand All @@ -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()
Expand Down
4 changes: 4 additions & 0 deletions translations/flashmessages.de.xlf
Expand Up @@ -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>
4 changes: 4 additions & 0 deletions translations/flashmessages.en.xlf
Expand Up @@ -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>

0 comments on commit 95796ab

Please sign in to comment.