From 67382986bd2948e231427f0e4975b74a9370f3f1 Mon Sep 17 00:00:00 2001 From: AnnoyingTechnology Date: Sun, 19 Jul 2026 22:28:09 +0200 Subject: [PATCH 1/2] Protect admin mutations with POST and CSRF --- config/packages/framework.yaml | 2 +- config/packages/security.yaml | 1 + public/js/app.js | 46 +++------------ .../Admin/AddressBookController.php | 10 ++-- src/Controller/Admin/CalendarController.php | 24 ++++---- src/Controller/Admin/DashboardController.php | 2 +- src/Controller/Admin/UserController.php | 29 +++++---- src/Controller/SecurityController.php | 4 +- .../_partials/add_delegate_modal.html.twig | 15 ++--- templates/_partials/delegate_row.html.twig | 2 +- templates/_partials/delete_modal.html.twig | 7 ++- templates/_partials/navigation.html.twig | 7 ++- templates/_partials/share_modal.html.twig | 20 ++++--- templates/security/login.html.twig | 6 +- templates/users/delegates.html.twig | 13 +++- .../Controllers/AddressBookControllerTest.php | 7 ++- .../Controllers/AdminMutationSecurityTest.php | 38 ++++++++++++ .../Controllers/CalendarControllerTest.php | 45 +++++++++++++- .../Functional/Controllers/DashboardTest.php | 20 +++++++ .../Controllers/UserControllerTest.php | 59 ++++++++++++++++--- 20 files changed, 256 insertions(+), 101 deletions(-) create mode 100644 tests/Functional/Controllers/AdminMutationSecurityTest.php diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 575e2f16..90b3f3d7 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -2,7 +2,7 @@ framework: secret: '%env(APP_SECRET)%' handle_all_throwables: true - #csrf_protection: true + csrf_protection: true http_method_override: false # Enables session support. Note that the session will ONLY be started if you read or write from it. diff --git a/config/packages/security.yaml b/config/packages/security.yaml index f229488d..030f4588 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -21,6 +21,7 @@ security: logout: path: app_logout target: dashboard + enable_csrf: true access_control: diff --git a/public/js/app.js b/public/js/app.js index dac7c60e..6643a79c 100644 --- a/public/js/app.js +++ b/public/js/app.js @@ -7,19 +7,10 @@ if (shareModal) { // Button that triggered the modal const button = event.relatedTarget - // Grab calendar shares url and add url + // Grab calendar shares URL and share target. let shareesUrl = button.getAttribute('data-sharees-href'); let targetUrl = button.getAttribute('data-href'); - - // When adding the sharee, catch the click to add the query parameter - const addShareeButton = document.getElementById('shareModal-addSharee'); - addShareeButton.addEventListener("click", function(e) { - const writeAccess = document.getElementById('shareModal-writeAccess').checked ? 'true' : 'false'; - const principalId = document.getElementById('shareModal-member').value; - - e.preventDefault() - window.location = targetUrl + "?principalId=" + principalId + "&write=" + writeAccess - }); + document.getElementById('shareModal-addForm').setAttribute('action', targetUrl) const noneElement = document.getElementById('shareModal-none') @@ -53,8 +44,8 @@ if (shareModal) { badge[0].classList.add('bg-success') badge[0].classList.remove('bg-info') } - let revokeButton = clone.querySelectorAll("a.revoke"); - revokeButton[0].href = element.revokeUrl; + let revokeForm = clone.querySelectorAll("form.revoke"); + revokeForm[0].setAttribute('action', element.revokeUrl); shares.appendChild(clone); }); @@ -71,37 +62,16 @@ deleteModals.forEach(element => { // Button that triggered the modal const button = event.relatedTarget - // Grab real target url for deletion + // Grab the target URL for deletion. let targetUrl = button.getAttribute('data-href'); let modalFlavour = button.getAttribute('data-flavour'); - // Put it into the modal's OK button - const deleteCTA = document.getElementById(`deleteModal-${modalFlavour}-cta`); - console.log("setting href to " + targetUrl) - deleteCTA.setAttribute('href', targetUrl); + // Put it into the modal's confirmation form. + const deleteForm = document.getElementById(`deleteModal-${modalFlavour}-form`); + deleteForm.setAttribute('action', targetUrl); }) }) - - -// Global account delegation modal -const addDelegateModal = document.getElementById('addDelegateModal') -if (addDelegateModal) { - addDelegateModal.addEventListener('show.bs.modal', event => { - // When adding the sharee, catch the click to add the query parameter - const addDelegateButton = document.getElementById('addDelegateModal-cta'); - addDelegateButton.addEventListener("click", function(e) { - const targetUrl = addDelegateButton.getAttribute('data-href'); - const writeAccess = document.getElementById('addDelegateModal-writeAccess').checked ? 'true' : 'false'; - const principalId = document.getElementById('addDelegateModal-member').value; - - e.preventDefault() - window.location = targetUrl + "?principalId=" + principalId + "&write=" + writeAccess - }); - - }) -} - // Color swatch: update it live (not working in IE ¯\_(ツ)_/¯ but it's just a nice to have) const colorPicker = document.getElementById('calendar_instance_calendarColor'); if (colorPicker) { diff --git a/src/Controller/Admin/AddressBookController.php b/src/Controller/Admin/AddressBookController.php index 7711b983..e0a12a94 100644 --- a/src/Controller/Admin/AddressBookController.php +++ b/src/Controller/Admin/AddressBookController.php @@ -12,12 +12,13 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Annotation\Route; +use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid; use Symfony\Contracts\Translation\TranslatorInterface; #[Route('/addressbooks', name: 'addressbook_')] class AddressBookController extends AbstractController { - #[Route('/{userId}', name: 'index')] + #[Route('/{userId}', name: 'index', methods: ['GET'])] public function addressBooks(ManagerRegistry $doctrine, int $userId): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -38,8 +39,8 @@ public function addressBooks(ManagerRegistry $doctrine, int $userId): Response ]); } - #[Route('/{userId}/new', name: 'create')] - #[Route('/{userId}/edit/{id}', name: 'edit', requirements: ['id' => "\d+"])] + #[Route('/{userId}/new', name: 'create', methods: ['GET', 'POST'])] + #[Route('/{userId}/edit/{id}', name: 'edit', methods: ['GET', 'POST'], requirements: ['id' => "\d+"])] public function addressbookCreate(ManagerRegistry $doctrine, Request $request, int $userId, ?int $id, TranslatorInterface $trans, BirthdayService $birthdayService): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -106,7 +107,8 @@ public function addressbookCreate(ManagerRegistry $doctrine, Request $request, i ]); } - #[Route('/{userId}/delete/{id}', name: 'delete', requirements: ['id' => "\d+"])] + #[Route('/{userId}/delete/{id}', name: 'delete', methods: ['POST'], requirements: ['id' => "\d+"])] + #[IsCsrfTokenValid('delete-addressbooks')] public function addressbookDelete(ManagerRegistry $doctrine, int $userId, string $id, TranslatorInterface $trans, BirthdayService $birthdayService): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); diff --git a/src/Controller/Admin/CalendarController.php b/src/Controller/Admin/CalendarController.php index 80b1cede..5046b708 100644 --- a/src/Controller/Admin/CalendarController.php +++ b/src/Controller/Admin/CalendarController.php @@ -17,12 +17,13 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid; use Symfony\Contracts\Translation\TranslatorInterface; #[Route('/calendars', name: 'calendar_')] class CalendarController extends AbstractController { - #[Route('/{userId}', name: 'index')] + #[Route('/{userId}', name: 'index', methods: ['GET'])] public function calendars(ManagerRegistry $doctrine, UrlGeneratorInterface $router, int $userId): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -75,8 +76,8 @@ public function calendars(ManagerRegistry $doctrine, UrlGeneratorInterface $rout ]); } - #[Route('/{userId}/new', name: 'create')] - #[Route('/{userId}/edit/{id}', name: 'edit', requirements: ['id' => "\d+"])] + #[Route('/{userId}/new', name: 'create', methods: ['GET', 'POST'])] + #[Route('/{userId}/edit/{id}', name: 'edit', methods: ['GET', 'POST'], requirements: ['id' => "\d+"])] public function calendarEdit(ManagerRegistry $doctrine, Request $request, int $userId, ?int $id, TranslatorInterface $trans): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -170,7 +171,7 @@ public function calendarEdit(ManagerRegistry $doctrine, Request $request, int $u ]); } - #[Route('/{userId}/shares/{calendarid}', name: 'shares', requirements: ['calendarid' => "\d+"])] + #[Route('/{userId}/shares/{calendarid}', name: 'shares', methods: ['GET'], requirements: ['calendarid' => "\d+"])] public function calendarShares(ManagerRegistry $doctrine, int $userId, string $calendarid, TranslatorInterface $trans): Response { $instances = $doctrine->getRepository(CalendarInstance::class)->findSharedInstancesOfInstance($calendarid, true); @@ -190,7 +191,8 @@ public function calendarShares(ManagerRegistry $doctrine, int $userId, string $c return new JsonResponse($response); } - #[Route('/{userId}/share/{instanceid}', name: 'share_add', requirements: ['instanceid' => "\d+"])] + #[Route('/{userId}/share/{instanceid}', name: 'share_add', methods: ['POST'], requirements: ['instanceid' => "\d+"])] + #[IsCsrfTokenValid('calendar-share')] public function calendarShareAdd(ManagerRegistry $doctrine, Request $request, int $userId, string $instanceid, TranslatorInterface $trans): Response { $instance = $doctrine->getRepository(CalendarInstance::class)->findOneById($instanceid); @@ -198,11 +200,11 @@ public function calendarShareAdd(ManagerRegistry $doctrine, Request $request, in throw $this->createNotFoundException('Calendar not found'); } - if (!is_numeric($request->get('principalId'))) { + if (!is_numeric($request->request->get('principalId'))) { throw new BadRequestHttpException(); } - $newShareeToAdd = $doctrine->getRepository(Principal::class)->findOneById($request->get('principalId')); + $newShareeToAdd = $doctrine->getRepository(Principal::class)->findOneById($request->request->get('principalId')); if (!$newShareeToAdd) { throw $this->createNotFoundException('Member not found'); } @@ -211,7 +213,7 @@ public function calendarShareAdd(ManagerRegistry $doctrine, Request $request, in // already existing first, so we can update it: $existingSharedInstance = $doctrine->getRepository(CalendarInstance::class)->findSharedInstanceOfInstanceFor($instance->getCalendar()->getId(), $newShareeToAdd->getUri()); - $writeAccess = ('true' === $request->get('write') ? SharingPlugin::ACCESS_READWRITE : SharingPlugin::ACCESS_READ); + $writeAccess = ('true' === $request->request->get('write') ? SharingPlugin::ACCESS_READWRITE : SharingPlugin::ACCESS_READ); $entityManager = $doctrine->getManager(); @@ -237,7 +239,8 @@ public function calendarShareAdd(ManagerRegistry $doctrine, Request $request, in return $this->redirectToRoute('calendar_index', ['userId' => $userId]); } - #[Route('/{userId}/delete/{id}', name: 'delete', requirements: ['id' => "\d+"])] + #[Route('/{userId}/delete/{id}', name: 'delete', methods: ['POST'], requirements: ['id' => "\d+"])] + #[IsCsrfTokenValid('delete-calendars')] public function calendarDelete(ManagerRegistry $doctrine, int $userId, string $id, TranslatorInterface $trans): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -283,7 +286,8 @@ public function calendarDelete(ManagerRegistry $doctrine, int $userId, string $i return $this->redirectToRoute('calendar_index', ['userId' => $userId]); } - #[Route('/{userId}/revoke/{id}', name: 'revoke', requirements: ['id' => "\d+"])] + #[Route('/{userId}/revoke/{id}', name: 'revoke', methods: ['POST'], requirements: ['id' => "\d+"])] + #[IsCsrfTokenValid('delete-revoke')] public function calendarRevoke(ManagerRegistry $doctrine, int $userId, string $id, TranslatorInterface $trans): Response { $instance = $doctrine->getRepository(CalendarInstance::class)->findOneById($id); diff --git a/src/Controller/Admin/DashboardController.php b/src/Controller/Admin/DashboardController.php index 57aa1549..29219a36 100644 --- a/src/Controller/Admin/DashboardController.php +++ b/src/Controller/Admin/DashboardController.php @@ -14,7 +14,7 @@ class DashboardController extends AbstractController { - #[Route('/dashboard', name: 'dashboard')] + #[Route('/dashboard', name: 'dashboard', methods: ['GET'])] public function dashboard(ManagerRegistry $doctrine): Response { $usersCount = $doctrine->getRepository(User::class)->count([]); diff --git a/src/Controller/Admin/UserController.php b/src/Controller/Admin/UserController.php index cba5fd98..2018b80c 100644 --- a/src/Controller/Admin/UserController.php +++ b/src/Controller/Admin/UserController.php @@ -17,12 +17,13 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\Routing\Annotation\Route; +use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid; use Symfony\Contracts\Translation\TranslatorInterface; #[Route('/users', name: 'user_')] class UserController extends AbstractController { - #[Route('/', name: 'index')] + #[Route('/', name: 'index', methods: ['GET'])] public function users(ManagerRegistry $doctrine): Response { $results = $doctrine->getRepository(Principal::class)->findAllMainPrincipalsWithUserIds(); @@ -32,8 +33,8 @@ public function users(ManagerRegistry $doctrine): Response ]); } - #[Route('/new', name: 'create')] - #[Route('/edit/{userId}', name: 'edit')] + #[Route('/new', name: 'create', methods: ['GET', 'POST'])] + #[Route('/edit/{userId}', name: 'edit', methods: ['GET', 'POST'])] public function userCreate(ManagerRegistry $doctrine, Utils $utils, Request $request, ?int $userId, TranslatorInterface $trans): Response { if ($userId) { @@ -124,7 +125,8 @@ public function userCreate(ManagerRegistry $doctrine, Utils $utils, Request $req ]); } - #[Route('/delete/{userId}', name: 'delete')] + #[Route('/delete/{userId}', name: 'delete', methods: ['POST'])] + #[IsCsrfTokenValid('delete-users')] public function userDelete(ManagerRegistry $doctrine, int $userId, TranslatorInterface $trans): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -198,7 +200,7 @@ public function userDelete(ManagerRegistry $doctrine, int $userId, TranslatorInt return $this->redirectToRoute('user_index'); } - #[Route('/delegates/{userId}', name: 'delegates')] + #[Route('/delegates/{userId}', name: 'delegates', methods: ['GET'])] public function userDelegates(ManagerRegistry $doctrine, int $userId): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -226,7 +228,8 @@ public function userDelegates(ManagerRegistry $doctrine, int $userId): Response ]); } - #[Route('/delegation/{userId}/{toggle}', name: 'delegation_toggle', requirements: ['toggle' => '(on|off)'])] + #[Route('/delegation/{userId}/{toggle}', name: 'delegation_toggle', methods: ['POST'], requirements: ['toggle' => '(on|off)'])] + #[IsCsrfTokenValid('delegation-toggle')] public function userToggleDelegation(ManagerRegistry $doctrine, int $userId, string $toggle): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); @@ -270,10 +273,11 @@ public function userToggleDelegation(ManagerRegistry $doctrine, int $userId, str return $this->redirectToRoute('user_delegates', ['userId' => $userId]); } - #[Route('/delegates/{userId}/add', name: 'delegate_add')] + #[Route('/delegates/{userId}/add', name: 'delegate_add', methods: ['POST'])] + #[IsCsrfTokenValid('delegate-add')] public function userDelegateAdd(ManagerRegistry $doctrine, Request $request, int $userId): Response { - if (!is_numeric($request->get('principalId'))) { + if (!is_numeric($request->request->get('principalId'))) { throw new BadRequestHttpException(); } @@ -284,14 +288,14 @@ public function userDelegateAdd(ManagerRegistry $doctrine, Request $request, int $principalUri = Principal::PREFIX.$user->getUsername(); - $newMemberToAdd = $doctrine->getRepository(Principal::class)->findOneById($request->get('principalId')); + $newMemberToAdd = $doctrine->getRepository(Principal::class)->findOneById($request->request->get('principalId')); if (!$newMemberToAdd) { throw $this->createNotFoundException('Member not found'); } // Depending on write access or not, attach to the correct principal - if ('true' === $request->get('write')) { + if ('true' === $request->request->get('write')) { // Let's check that there wasn't a read proxy first $principalProxyRead = $doctrine->getRepository(Principal::class)->findOneByUri($principalUri.Principal::READ_PROXY_SUFFIX); if (!$principalProxyRead) { @@ -315,8 +319,9 @@ public function userDelegateAdd(ManagerRegistry $doctrine, Request $request, int return $this->redirectToRoute('user_delegates', ['userId' => $userId]); } - #[Route('/delegates/{userId}/remove/{principalProxyId}/{delegateId}', name: 'delegate_remove', requirements: ['principalProxyId' => "\d+", 'delegateId' => "\d+"])] - public function userDelegateRemove(ManagerRegistry $doctrine, Request $request, int $userId, int $principalProxyId, int $delegateId): Response + #[Route('/delegates/{userId}/remove/{principalProxyId}/{delegateId}', name: 'delegate_remove', methods: ['POST'], requirements: ['principalProxyId' => "\d+", 'delegateId' => "\d+"])] + #[IsCsrfTokenValid('delete-delegates')] + public function userDelegateRemove(ManagerRegistry $doctrine, int $userId, int $principalProxyId, int $delegateId): Response { $user = $doctrine->getRepository(User::class)->findOneById($userId); if (!$user) { diff --git a/src/Controller/SecurityController.php b/src/Controller/SecurityController.php index a0166265..4ea7ed92 100644 --- a/src/Controller/SecurityController.php +++ b/src/Controller/SecurityController.php @@ -9,7 +9,7 @@ class SecurityController extends AbstractController { - #[Route('/login', name: 'app_login')] + #[Route('/login', name: 'app_login', methods: ['GET', 'POST'])] public function login(AuthenticationUtils $authenticationUtils): Response { // if ($this->getUser()) { @@ -24,7 +24,7 @@ public function login(AuthenticationUtils $authenticationUtils): Response return $this->render('security/login.html.twig', ['last_username' => $lastUsername, 'error' => $error]); } - #[Route('/logout', name: 'app_logout')] + #[Route('/logout', name: 'app_logout', methods: ['POST'])] public function logout() { throw new \Exception('This method can be blank - it will be intercepted by the logout key on your firewall'); diff --git a/templates/_partials/add_delegate_modal.html.twig b/templates/_partials/add_delegate_modal.html.twig index 6eaba06e..094b0e37 100644 --- a/templates/_partials/add_delegate_modal.html.twig +++ b/templates/_partials/add_delegate_modal.html.twig @@ -6,10 +6,11 @@
-
+ +
- - {% for principal in principals %} {% endfor %} @@ -17,8 +18,8 @@ {{ "delegates.member.help"|trans }}
- -
@@ -26,8 +27,8 @@
- {{ "add"|trans }} +
- \ No newline at end of file + diff --git a/templates/_partials/delegate_row.html.twig b/templates/_partials/delegate_row.html.twig index de55e46f..48d6c280 100644 --- a/templates/_partials/delegate_row.html.twig +++ b/templates/_partials/delegate_row.html.twig @@ -21,6 +21,6 @@

{{ "users.username"|trans }} : {{ delegate.username }}

{{ "users.uri"|trans }} : {{ delegate.uri }}
- ⚠ {{ "remove"|trans }} + ⚠ {{ "remove"|trans }}
diff --git a/templates/_partials/delete_modal.html.twig b/templates/_partials/delete_modal.html.twig index 1d136ebe..783b8d1a 100644 --- a/templates/_partials/delete_modal.html.twig +++ b/templates/_partials/delete_modal.html.twig @@ -10,8 +10,11 @@
- {{ "yes"|trans }} + + + +
- \ No newline at end of file + diff --git a/templates/_partials/navigation.html.twig b/templates/_partials/navigation.html.twig index 63f4e913..12c01ceb 100644 --- a/templates/_partials/navigation.html.twig +++ b/templates/_partials/navigation.html.twig @@ -22,7 +22,10 @@ 👤 {{ app.user.username }}
- {{ "logout"|trans }} +
+ + +
  • @@ -51,4 +54,4 @@ {% endif %} - \ No newline at end of file + diff --git a/templates/_partials/share_modal.html.twig b/templates/_partials/share_modal.html.twig index 3c68d2d5..58164605 100644 --- a/templates/_partials/share_modal.html.twig +++ b/templates/_partials/share_modal.html.twig @@ -13,15 +13,19 @@ - {{ "revoke"|trans }} +
    + + +
    {{ "calendars.delegates.none"|trans }} -
    + +
    - - {% for principal in principals %} {% endfor %} @@ -35,8 +39,8 @@
    - -
    @@ -44,8 +48,8 @@
    - {{ "add"|trans }} +
    - \ No newline at end of file + diff --git a/templates/security/login.html.twig b/templates/security/login.html.twig index b76dac61..0d7eca9d 100644 --- a/templates/security/login.html.twig +++ b/templates/security/login.html.twig @@ -5,7 +5,11 @@ {% if app.user %}
    - {{ "login.already"|trans({username: app.user.username}) }}, {{ "logout"|trans }} + {{ "login.already"|trans({username: app.user.username}) }}, + + + +
    {% else %}
    diff --git a/templates/users/delegates.html.twig b/templates/users/delegates.html.twig index d3a05698..abaa0090 100644 --- a/templates/users/delegates.html.twig +++ b/templates/users/delegates.html.twig @@ -17,7 +17,10 @@ {% if delegation %} {% for delegate in principalProxyRead.delegees %} {% include '_partials/delegate_row.html.twig' with {has_write: false} %} @@ -28,7 +31,11 @@ {% else %} {% endif %} @@ -37,4 +44,4 @@ {% include '_partials/delete_modal.html.twig' with {flavour: 'delegates'} %} {% include '_partials/add_delegate_modal.html.twig' with {principals: allPrincipals} %} -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/tests/Functional/Controllers/AddressBookControllerTest.php b/tests/Functional/Controllers/AddressBookControllerTest.php index 1642fdd8..bb189795 100644 --- a/tests/Functional/Controllers/AddressBookControllerTest.php +++ b/tests/Functional/Controllers/AddressBookControllerTest.php @@ -107,7 +107,12 @@ public function testAddressBookDelete(): void $addressbookRepository = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(AddressBook::class); $addressbook = $addressbookRepository->findOneByDisplayName('default.addressbook.title'); - $client->request('GET', '/addressbooks/'.$userId.'/delete/'.$addressbook->getId()); + $crawler = $client->request('GET', '/addressbooks/'.$userId); + $csrfToken = $crawler->filter('#deleteModal-addressbooks-form input[name="_token"]')->attr('value'); + + $client->request('POST', '/addressbooks/'.$userId.'/delete/'.$addressbook->getId(), [ + '_token' => $csrfToken, + ]); $this->assertResponseRedirects('/addressbooks/'.$userId); $client->followRedirect(); diff --git a/tests/Functional/Controllers/AdminMutationSecurityTest.php b/tests/Functional/Controllers/AdminMutationSecurityTest.php new file mode 100644 index 00000000..2e1e657d --- /dev/null +++ b/tests/Functional/Controllers/AdminMutationSecurityTest.php @@ -0,0 +1,38 @@ + '/addressbooks/1/delete/1', + 'calendar_share_add' => '/calendars/1/share/1', + 'calendar_delete' => '/calendars/1/delete/1', + 'calendar_revoke' => '/calendars/1/revoke/1', + 'user_delete' => '/users/delete/1', + 'user_delegation_toggle' => '/users/delegation/1/off', + 'user_delegate_add' => '/users/delegates/1/add', + 'user_delegate_remove' => '/users/delegates/1/remove/1/1', + ]; + + $routes = static::getContainer()->get(RouterInterface::class)->getRouteCollection(); + foreach ($mutations as $routeName => $path) { + $this->assertSame(['POST'], $routes->get($routeName)->getMethods(), $routeName.' is not POST-only'); + + $client->loginUser(new AdminUser('admin', 'test')); + $client->request('GET', '/dashboard'); + $this->assertResponseIsSuccessful(); + + $client->request('POST', $path); + $this->assertResponseRedirects('/login', 302, $path.' accepted a POST request without a CSRF token'); + } + } +} diff --git a/tests/Functional/Controllers/CalendarControllerTest.php b/tests/Functional/Controllers/CalendarControllerTest.php index a7a001dc..18443199 100644 --- a/tests/Functional/Controllers/CalendarControllerTest.php +++ b/tests/Functional/Controllers/CalendarControllerTest.php @@ -2,9 +2,11 @@ namespace App\Tests\Functional; +use App\Entity\Principal; use App\Entity\User; use App\Repository\CalendarInstanceRepository; use App\Security\AdminUser; +use Sabre\DAV\Sharing\Plugin as SharingPlugin; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; class CalendarControllerTest extends WebTestCase @@ -108,11 +110,52 @@ public function testCalendarDelete(): void $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class); $calendar = $calendarRepository->findOneByDisplayName('default.calendar.title'); - $client->request('GET', '/calendars/'.$userId.'/delete/'.$calendar->getId()); + $crawler = $client->request('GET', '/calendars/'.$userId); + $csrfToken = $crawler->filter('#deleteModal-calendars-form input[name="_token"]')->attr('value'); + + $client->request('POST', '/calendars/'.$userId.'/delete/'.$calendar->getId(), [ + '_token' => $csrfToken, + ]); $this->assertResponseRedirects('/calendars/'.$userId); $client->followRedirect(); $this->assertSelectorTextNotContains('h5', 'default.calendar.title'); } + + public function testCalendarSharingAndRevocation(): void + { + $client = static::createClient(); + $client->loginUser(new AdminUser('admin', 'test')); + + $userId = $this->getUserId($client, 'test_user'); + $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class); + $calendar = $calendarRepository->findOneByDisplayName('default.calendar.title'); + $sharee = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(Principal::class)->findOneByUri(Principal::PREFIX.'test_user2'); + + $crawler = $client->request('GET', '/calendars/'.$userId); + $shareToken = $crawler->filter('#shareModal-addForm input[name="_token"]')->attr('value'); + $revokeToken = $crawler->filter('#shareModal-shareeTemplate input[name="_token"]')->attr('value'); + + $client->request('POST', '/calendars/'.$userId.'/share/'.$calendar->getId(), [ + '_token' => $shareToken, + 'principalId' => $sharee->getId(), + 'write' => 'true', + ]); + $this->assertResponseRedirects('/calendars/'.$userId); + + $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class); + $sharedInstance = $calendarRepository->findSharedInstanceOfInstanceFor($calendar->getCalendar()->getId(), $sharee->getUri()); + $this->assertNotNull($sharedInstance); + $this->assertSame(SharingPlugin::ACCESS_READWRITE, $sharedInstance->getAccess()); + + $sharedInstanceId = $sharedInstance->getId(); + $client->request('POST', '/calendars/'.$userId.'/revoke/'.$sharedInstanceId, [ + '_token' => $revokeToken, + ]); + $this->assertResponseRedirects('/calendars/'.$userId); + + $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class); + $this->assertNull($calendarRepository->find($sharedInstanceId)); + } } diff --git a/tests/Functional/Controllers/DashboardTest.php b/tests/Functional/Controllers/DashboardTest.php index c3371ef0..12678d72 100644 --- a/tests/Functional/Controllers/DashboardTest.php +++ b/tests/Functional/Controllers/DashboardTest.php @@ -2,7 +2,9 @@ namespace App\Tests\Functional; +use App\Security\AdminUser; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; +use Symfony\Component\Routing\RouterInterface; class DashboardTest extends WebTestCase { @@ -91,4 +93,22 @@ public function testLoginCorrect(): void $this->assertSelectorTextContains('h3.environment', 'Configured environment'); $this->assertSelectorExists('nav.navbar'); } + + public function testLogoutRequiresPostAndCsrfProtection(): void + { + $client = static::createClient(); + $client->loginUser(new AdminUser('admin', 'test')); + + $route = static::getContainer()->get(RouterInterface::class)->getRouteCollection()->get('app_logout'); + $this->assertSame(['POST'], $route->getMethods()); + + $crawler = $client->request('GET', '/dashboard'); + $csrfToken = $crawler->filter('form[action="/logout"] input[name="_csrf_token"]')->attr('value'); + + $client->request('POST', '/logout'); + $this->assertResponseStatusCodeSame(403); + + $client->request('POST', '/logout', ['_csrf_token' => $csrfToken]); + $this->assertResponseRedirects('/dashboard'); + } } diff --git a/tests/Functional/Controllers/UserControllerTest.php b/tests/Functional/Controllers/UserControllerTest.php index b6643fda..7cfe3434 100644 --- a/tests/Functional/Controllers/UserControllerTest.php +++ b/tests/Functional/Controllers/UserControllerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Functional; +use App\Entity\Principal; use App\Entity\User; use App\Security\AdminUser; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; @@ -98,14 +99,21 @@ public function testUserDelete(): void $userId1 = $this->getUserId($client, 'test_user'); $userId2 = $this->getUserId($client, 'test_user2'); - $client->request('GET', '/users/delete/'.$userId1); + $crawler = $client->request('GET', '/users/'); + $csrfToken = $crawler->filter('#deleteModal-users-form input[name="_token"]')->attr('value'); + + $client->request('POST', '/users/delete/'.$userId1, [ + '_token' => $csrfToken, + ]); $this->assertResponseRedirects('/users/'); $client->followRedirect(); $this->assertAnySelectorTextContains('h5', 'Test User 2'); - $client->request('GET', '/users/delete/'.$userId2); + $client->request('POST', '/users/delete/'.$userId2, [ + '_token' => $csrfToken, + ]); $this->assertResponseRedirects('/users/'); $client->followRedirect(); @@ -128,19 +136,56 @@ public function testUserDelegates(): void $this->assertSelectorExists('nav.navbar'); $this->assertSelectorTextContains('h1', 'Delegates for Test User'); - $this->assertSelectorTextContains('a.btn', '+ Add a delegate'); + $this->assertSelectorTextContains('a[data-bs-target="#addDelegateModal"]', '+ Add a delegate'); $this->assertAnySelectorTextContains('div', 'Delegation is enabled for this account.'); - $this->assertAnySelectorTextContains('a.btn', 'Disable it'); + $this->assertAnySelectorTextContains('button.btn', 'Disable it'); - $client->clickLink('Disable it'); + $client->submitForm('Disable it'); $this->assertResponseRedirects('/users/delegates/'.$userId); $client->followRedirect(); $this->assertSelectorExists('nav.navbar'); $this->assertSelectorTextContains('h1', 'Delegates for Test User'); - $this->assertSelectorTextNotContains('a.btn', '+ Add a delegate'); + $this->assertSelectorNotExists('a[data-bs-target="#addDelegateModal"]'); $this->assertAnySelectorTextContains('div', 'Delegation is not enabled for this account.'); - $this->assertAnySelectorTextContains('a.btn', 'Enable it'); + $this->assertAnySelectorTextContains('button.btn', 'Enable it'); + } + + public function testDelegateAdditionAndRemoval(): void + { + $client = static::createClient(); + $client->loginUser(new AdminUser('admin', 'test')); + + $userId = $this->getUserId($client, 'test_user'); + $principalRepository = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(Principal::class); + $principal = $principalRepository->findOneByUri(Principal::PREFIX.'test_user'); + $delegate = $principalRepository->findOneByUri(Principal::PREFIX.'test_user2'); + $readProxy = $principalRepository->findOneByUri($principal->getUri().Principal::READ_PROXY_SUFFIX); + + $crawler = $client->request('GET', '/users/delegates/'.$userId); + $addToken = $crawler->filter('#addDelegateModal-form input[name="_token"]')->attr('value'); + $removeToken = $crawler->filter('#deleteModal-delegates-form input[name="_token"]')->attr('value'); + + $client->request('POST', '/users/delegates/'.$userId.'/add', [ + '_token' => $addToken, + 'principalId' => $delegate->getId(), + ]); + $this->assertResponseRedirects('/users/delegates/'.$userId); + + $principalRepository = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(Principal::class); + $readProxy = $principalRepository->find($readProxy->getId()); + $delegate = $principalRepository->find($delegate->getId()); + $this->assertTrue($readProxy->getDelegees()->contains($delegate)); + + $client->request('POST', '/users/delegates/'.$userId.'/remove/'.$readProxy->getId().'/'.$delegate->getId(), [ + '_token' => $removeToken, + ]); + $this->assertResponseRedirects('/users/delegates/'.$userId); + + $principalRepository = static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(Principal::class); + $readProxy = $principalRepository->find($readProxy->getId()); + $delegate = $principalRepository->find($delegate->getId()); + $this->assertFalse($readProxy->getDelegees()->contains($delegate)); } } From 472e65ad2d98c70543d5c754feefb90adc9289b5 Mon Sep 17 00:00:00 2001 From: AnnoyingTechnology Date: Mon, 20 Jul 2026 07:47:03 +0200 Subject: [PATCH 2/2] Reject unsafe calendar colors at render time Calendar colors written through DAV bypass Symfony validation, and SQLite does not enforce the declared VARCHAR length. Rendering those stored values directly in an inline style therefore permits CSS declaration injection in the admin interface. Allow only exact #RRGGBB and #RRGGBBAA values at the rendering boundary and omit the style attribute otherwise. This also protects existing records and all current write paths. A follow-up should validate calendar colors on each write path so malformed values are rejected instead of being stored inertly. --- templates/calendars/index.html.twig | 12 ++++--- .../Controllers/CalendarControllerTest.php | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/templates/calendars/index.html.twig b/templates/calendars/index.html.twig index 822dc5ce..7d5e9003 100644 --- a/templates/calendars/index.html.twig +++ b/templates/calendars/index.html.twig @@ -1,6 +1,10 @@ {% extends 'base.html.twig' %} {% set menu = 'resources' %} +{% macro color_badge(color) %} +   +{% endmacro %} + {% block body %} {% include '_partials/back_button.html.twig' with { url: path('user_index'), text: "users.back"|trans } %} @@ -19,7 +23,7 @@ {{ ('calendar.public')|trans }} {% endif %} ⓘ -   + {{ _self.color_badge(calendar.calendarColor) }}
    {% if not calendar.isPublic() %} @@ -85,7 +89,7 @@ {{ ('calendar.share_access.' ~ calendar.access)|trans }} {% endif %} ⓘ -   + {{ _self.color_badge(calendar.calendarColor) }}
    ✎ {{ "edit"|trans }} @@ -135,7 +139,7 @@ {{ calendar.displayName }} {{ ('calendar.auto')|trans }} ⓘ -   + {{ _self.color_badge(calendar.calendarColor) }}
    ✎ {{ "edit"|trans }} @@ -168,7 +172,7 @@ {{ subscription.displayName }} {{ ('calendar.subscription')|trans }} ⓘ -   + {{ _self.color_badge(subscription.calendarColor) }}
    {{ subscription.source }} diff --git a/tests/Functional/Controllers/CalendarControllerTest.php b/tests/Functional/Controllers/CalendarControllerTest.php index 18443199..78db0cb0 100644 --- a/tests/Functional/Controllers/CalendarControllerTest.php +++ b/tests/Functional/Controllers/CalendarControllerTest.php @@ -2,6 +2,8 @@ namespace App\Tests\Functional; +use App\Entity\Calendar; +use App\Entity\CalendarInstance; use App\Entity\Principal; use App\Entity\User; use App\Repository\CalendarInstanceRepository; @@ -38,6 +40,35 @@ public function testCalendarIndex(): void $this->assertSelectorTextContains('h5', 'default.calendar.title'); } + public function testCalendarColorsAreSafelyRendered(): void + { + $client = static::createClient(); + $client->loginUser(new AdminUser('admin', 'test')); + + $userId = $this->getUserId($client, 'test_user'); + $entityManager = static::getContainer()->get('doctrine.orm.entity_manager'); + $calendarRepository = $entityManager->getRepository(CalendarInstance::class); + + $calendarRepository->findOneByDisplayName('default.calendar.title') + ->setCalendarColor('#00112233'); + + $unsafeCalendar = (new CalendarInstance()) + ->setPrincipalUri(Principal::PREFIX.'test_user') + ->setUri('unsafe-color') + ->setDisplayName('Unsafe color') + ->setCalendarColor('#000;top:0') + ->setCalendar(new Calendar()); + $entityManager->persist($unsafeCalendar); + $entityManager->flush(); + + $client->request('GET', '/calendars/'.$userId); + + $this->assertResponseIsSuccessful(); + $this->assertSelectorExists('span.badge-indicator[style="background-color: #00112233"]'); + $this->assertSelectorCount(1, 'span.badge-indicator[style]'); + $this->assertStringNotContainsString('#000;top:0', $client->getResponse()->getContent()); + } + public function testCalendarEdit(): void { $user = new AdminUser('admin', 'test');