From d703fa6a3a75f7c3b433e8caf618bfb0a9a0ba63 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Tue, 18 Mar 2025 23:38:48 +0100 Subject: [PATCH 01/21] Protect generate_token with a CSRF token --- .../static/themes/material/css/various.scss | 15 ++++ .../Controller/ConfigController.php | 11 +-- .../Resources/views/Config/index.html.twig | 88 +++++++++++-------- .../Controller/ConfigControllerTest.php | 21 +---- 4 files changed, 72 insertions(+), 63 deletions(-) diff --git a/app/Resources/static/themes/material/css/various.scss b/app/Resources/static/themes/material/css/various.scss index ad0703afa..94bb95bd0 100644 --- a/app/Resources/static/themes/material/css/various.scss +++ b/app/Resources/static/themes/material/css/various.scss @@ -38,3 +38,18 @@ nav .input-field input { .tab { flex: 1; } + +.btn-link { + background: none; + border: 0; + padding: 0; + color: $blue-accent-color; + + &:focus { + background: none; + } +} + +.inline-block { + display: inline-block; +} diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index 4709a4d2e..24e9c07e4 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -16,6 +16,7 @@ use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; +use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Validator\Constraints\Locale as LocaleConstraint; @@ -429,22 +430,22 @@ class ConfigController extends AbstractController } /** - * @Route("/generate-token", name="generate_token") + * @Route("/generate-token", name="generate_token", methods={"POST"}) * * @return RedirectResponse|JsonResponse */ public function generateTokenAction(Request $request) { + if (!$this->isCsrfTokenValid('generate-token', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $config = $this->getConfig(); $config->setFeedToken(Utils::generateToken()); $this->entityManager->persist($config); $this->entityManager->flush(); - if ($request->isXmlHttpRequest()) { - return new JsonResponse(['token' => $config->getFeedToken()]); - } - $this->addFlash( 'notice', 'flashes.config.notice.feed_token_updated' diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig index d52aa4c51..981c53fe4 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig @@ -123,48 +123,58 @@
+
+
+ {{ 'config.form_feed.description'|trans }} +
+
+ +
+
+
{{ 'config.form_feed.token_label'|trans }}
+
+ {% if feed.token %} + {{ feed.token }} + {% else %} + {{ 'config.form_feed.no_token'|trans }} + {% endif %} + + {% if feed.token %} + – +
+ + + +
+ – {{ 'config.form_feed.token_revoke'|trans }} + {% else %} + – +
+ + + +
+ {% endif %} +
+
+
+ {% if feed.token %} +
+
+
{{ 'config.form_feed.feed_links'|trans }}
+ +
+
+ {% endif %} + {{ form_start(form.feed) }} {{ form_errors(form.feed) }} -
-
- {{ 'config.form_feed.description'|trans }} -
-
- -
-
-
{{ 'config.form_feed.token_label'|trans }}
-
- {% if feed.token %} - {{ feed.token }} - {% else %} - {{ 'config.form_feed.no_token'|trans }} - {% endif %} - - {% if feed.token %} - – {{ 'config.form_feed.token_reset'|trans }} - – {{ 'config.form_feed.token_revoke'|trans }} - {% else %} - – {{ 'config.form_feed.token_create'|trans }} - {% endif %} -
-
-
- {% if feed.token %} -
-
-
{{ 'config.form_feed.feed_links'|trans }}
- -
-
- {% endif %} -
{{ form_label(form.feed.feed_limit) }} diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index 6c049e1dc..7dd5cdf1b 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -328,7 +328,8 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); $this->assertStringContainsString('config.form_feed.no_token', $body[0]); - $client->request('GET', '/generate-token'); + $client->submit($crawler->selectButton('config.form_feed.token_create')->form()); + $this->assertSame(302, $client->getResponse()->getStatusCode()); $crawler = $client->followRedirect(); @@ -337,24 +338,6 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->assertStringContainsString('config.form_feed.token_reset', $body[0]); } - public function testGenerateTokenAjax() - { - $this->logInAs('admin'); - $client = $this->getTestClient(); - - $client->request( - 'GET', - '/generate-token', - [], - [], - ['HTTP_X-Requested-With' => 'XMLHttpRequest'] - ); - - $this->assertSame(200, $client->getResponse()->getStatusCode()); - $content = json_decode($client->getResponse()->getContent(), true); - $this->assertArrayHasKey('token', $content); - } - public function testRevokeTokenAjax() { $this->logInAs('admin'); From ac5b5fb379233d6e96ea14ae21b7f88761d5fa3f Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Tue, 18 Mar 2025 23:42:51 +0100 Subject: [PATCH 02/21] Protect revoke_token with a CSRF token --- .../Controller/ConfigController.php | 10 +++---- .../Resources/views/Config/index.html.twig | 7 ++++- .../Controller/ConfigControllerTest.php | 30 ++++++++++++++----- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index 24e9c07e4..e103e4010 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -455,22 +455,22 @@ class ConfigController extends AbstractController } /** - * @Route("/revoke-token", name="revoke_token") + * @Route("/revoke-token", name="revoke_token", methods={"POST"}) * * @return RedirectResponse|JsonResponse */ public function revokeTokenAction(Request $request) { + if (!$this->isCsrfTokenValid('revoke-token', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $config = $this->getConfig(); $config->setFeedToken(null); $this->entityManager->persist($config); $this->entityManager->flush(); - if ($request->isXmlHttpRequest()) { - return new JsonResponse(); - } - $this->addFlash( 'notice', 'flashes.config.notice.feed_token_revoked' diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig index 981c53fe4..ac2b9ab38 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig @@ -146,7 +146,12 @@ - – {{ 'config.form_feed.token_revoke'|trans }} + – +
+ + + +
{% else %} –
diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index 7dd5cdf1b..1594b603d 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -343,15 +343,29 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->logInAs('admin'); $client = $this->getTestClient(); - $client->request( - 'GET', - '/revoke-token', - [], - [], - ['HTTP_X-Requested-With' => 'XMLHttpRequest'] - ); + // set the token + $em = $client->getContainer()->get(EntityManagerInterface::class); + $user = $em + ->getRepository(User::class) + ->findOneByUsername('admin'); - $this->assertSame(200, $client->getResponse()->getStatusCode()); + if (!$user) { + $this->markTestSkipped('No user found in db.'); + } + + $config = $user->getConfig(); + $config->setFeedToken('abcd1234'); + $em->persist($config); + $em->flush(); + + $crawler = $client->request('GET', '/config'); + + $client->submit($crawler->selectButton('config.form_feed.token_revoke')->form()); + + $crawler = $client->followRedirect(); + + $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); + $this->assertStringContainsString('config.form_feed.token_create', $body[0]); } public function testFeedUpdate() From 264f91126e2c42188b80848c881264da743b4dc1 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Tue, 18 Mar 2025 23:52:10 +0100 Subject: [PATCH 03/21] Protect delete_tagging_rule with a CSRF token --- .../CoreBundle/Controller/ConfigController.php | 8 ++++++-- .../CoreBundle/Resources/views/Config/index.html.twig | 10 +++++++--- .../CoreBundle/Controller/ConfigControllerTest.php | 9 ++++----- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index e103e4010..3b951f4a6 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -482,12 +482,16 @@ class ConfigController extends AbstractController /** * Deletes a tagging rule and redirect to the config homepage. * - * @Route("/tagging-rule/delete/{id}", requirements={"id" = "\d+"}, name="delete_tagging_rule") + * @Route("/tagging-rule/delete/{id}", name="delete_tagging_rule", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ - public function deleteTaggingRuleAction(TaggingRule $rule) + public function deleteTaggingRuleAction(Request $request, TaggingRule $rule) { + if (!$this->isCsrfTokenValid('delete-tagging-rule', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->validateRuleAction($rule); $this->entityManager->remove($rule); diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig index ac2b9ab38..c3d94f5e5 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig @@ -340,9 +340,13 @@ mode_edit - - delete - + + + + + {% endfor %} diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index 1594b603d..74813b519 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -481,9 +481,8 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->assertStringContainsString('readingTime <= 30', $crawler->filter('body')->extract(['_text'])[0]); - $deleteLink = $crawler->filter('.delete_tagging_rule')->last()->link(); + $crawler = $client->submit($crawler->filter('#set5')->selectButton('delete')->form()); - $crawler = $client->click($deleteLink); $this->assertSame(302, $client->getResponse()->getStatusCode()); $crawler = $client->followRedirect(); @@ -573,11 +572,11 @@ class ConfigControllerTest extends WallabagCoreTestCase ->getRepository(TaggingRule::class) ->findAll()[0]; - $crawler = $client->request('GET', '/tagging-rule/delete/' . $rule->getId()); + $crawler = $client->request('POST', '/tagging-rule/delete/' . $rule->getId()); - $this->assertSame(403, $client->getResponse()->getStatusCode()); + $this->assertSame(400, $client->getResponse()->getStatusCode()); $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); - $this->assertStringContainsString('You can not access this rule', $body[0]); + $this->assertStringContainsString('Bad CSRF token.', $body[0]); } public function testEditingTaggingRuleFromAnOtherUser() From 6fa61c0f9c48d37625c92a8913b487230761fb47 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Tue, 18 Mar 2025 23:57:18 +0100 Subject: [PATCH 04/21] Protect delete_ignore_origin_rule with a CSRF token --- .../CoreBundle/Controller/ConfigController.php | 8 ++++++-- .../CoreBundle/Resources/views/Config/index.html.twig | 10 +++++++--- .../CoreBundle/Controller/ConfigControllerTest.php | 10 +++++----- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index 3b951f4a6..81bea532a 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -522,12 +522,16 @@ class ConfigController extends AbstractController /** * Deletes an ignore origin rule and redirect to the config homepage. * - * @Route("/ignore-origin-user-rule/delete/{id}", requirements={"id" = "\d+"}, name="delete_ignore_origin_rule") + * @Route("/ignore-origin-user-rule/delete/{id}", name="delete_ignore_origin_rule", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ - public function deleteIgnoreOriginRuleAction(IgnoreOriginUserRule $rule) + public function deleteIgnoreOriginRuleAction(Request $request, IgnoreOriginUserRule $rule) { + if (!$this->isCsrfTokenValid('delete-ignore-origin-rule', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->validateRuleAction($rule); $this->entityManager->remove($rule); diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig index c3d94f5e5..59031054d 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig @@ -524,9 +524,13 @@ mode_edit - - delete - +
+ + + +
{% endfor %} diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index 74813b519..0a5a7da46 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -642,9 +642,9 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->assertStringContainsString('host = "example.org"', $crawler->filter('body')->extract(['_text'])[0]); - $deleteLink = $crawler->filter('div[id=set6] a.delete')->last()->link(); + $form = $crawler->filter('#set6')->selectButton('delete')->form(); - $crawler = $client->click($deleteLink); + $crawler = $client->submit($form); $this->assertSame(302, $client->getResponse()->getStatusCode()); $crawler = $client->followRedirect(); @@ -709,11 +709,11 @@ class ConfigControllerTest extends WallabagCoreTestCase ->getRepository(IgnoreOriginUserRule::class) ->findAll()[0]; - $crawler = $client->request('GET', '/ignore-origin-user-rule/edit/' . $rule->getId()); + $crawler = $client->request('POST', '/ignore-origin-user-rule/delete/' . $rule->getId()); - $this->assertSame(403, $client->getResponse()->getStatusCode()); + $this->assertSame(400, $client->getResponse()->getStatusCode()); $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); - $this->assertStringContainsString('You can not access this rule', $body[0]); + $this->assertStringContainsString('Bad CSRF token.', $body[0]); } public function testEditingIgnoreOriginRuleFromAnOtherUser() From e162408139ac9bb12e69f4d49de45ade49369c21 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Wed, 19 Mar 2025 00:28:34 +0100 Subject: [PATCH 05/21] Protect switch_view_mode with a CSRF token --- app/Resources/static/themes/material/index.js | 4 ++-- .../Controller/ConfigController.php | 6 ++++- .../views/Entry/Card/_mass_checkbox.html.twig | 2 +- .../Resources/views/Entry/entries.html.twig | 23 +++++++++++-------- .../Controller/ConfigControllerTest.php | 13 ++++------- 5 files changed, 27 insertions(+), 21 deletions(-) diff --git a/app/Resources/static/themes/material/index.js b/app/Resources/static/themes/material/index.js index 704a9ea11..24adf8aab 100755 --- a/app/Resources/static/themes/material/index.js +++ b/app/Resources/static/themes/material/index.js @@ -228,10 +228,10 @@ $(document).ready(() => { }); }); } - $('form[name="form_mass_action"] input[name="tags"]').on('keydown', (e) => { + $('input[name="tags"][form="form_mass_action"]').on('keydown', (e) => { if (e.key === 'Enter') { e.preventDefault(); - $('form[name="form_mass_action"] button[name="tag"]').trigger('click'); + $('button[name="tag"][form="form_mass_action"]').trigger('click'); } }); }); diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index 81bea532a..5aae8122b 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -646,12 +646,16 @@ class ConfigController extends AbstractController /** * Switch view mode for current user. * - * @Route("/config/view-mode", name="switch_view_mode") + * @Route("/config/view-mode", name="switch_view_mode", methods={"POST"}) * * @return RedirectResponse */ public function changeViewModeAction(Request $request) { + if (!$this->isCsrfTokenValid('switch-view-mode', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $user = $this->getUser(); $user->getConfig()->setListMode(!$user->getConfig()->getListMode()); diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/Card/_mass_checkbox.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/Card/_mass_checkbox.html.twig index 5e4fe8f6d..b4bd1e94b 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/Card/_mass_checkbox.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/Card/_mass_checkbox.html.twig @@ -1,3 +1,3 @@ diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/entries.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/entries.html.twig index 2c26b24af..93d5a82d1 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/entries.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/entries.html.twig @@ -26,12 +26,18 @@ {% if current_route == 'homepage' %} {% set current_route = 'unread' %} {% endif %} -
+
{{ 'entry.list.number_on_the_page'|trans({'%count%': entries.count}) }} {% if entries.count > 0 %} - {% if list_mode == 0 %}view_list{% else %}view_module{% endif %} +
+ + + +
{% endif %} {% if entries.count > 0 %} @@ -50,15 +56,15 @@
- - - - + + + +
- - + +
@@ -77,7 +83,6 @@ {% endfor %} {% endif %} - {% if entries.getNbPages > 1 %}
diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index 0a5a7da46..a9e51f3a8 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -1116,18 +1116,17 @@ class ConfigControllerTest extends WallabagCoreTestCase $this->logInAs('admin'); $client = $this->getTestClient(); - $client->request('GET', '/unread/list'); + $crawler = $client->request('GET', '/unread/list'); $this->assertStringContainsString('row data', $client->getResponse()->getContent()); - $client->request('GET', '/config/view-mode'); - $crawler = $client->followRedirect(); + $form = $crawler->filter('.nb-results')->selectButton('view_list')->form(); - $client->request('GET', '/unread/list'); + $client->submit($form); + + $client->followRedirect(); $this->assertStringContainsString('collection', $client->getResponse()->getContent()); - - $client->request('GET', '/config/view-mode'); } public function testChangeLocaleWithoutReferer() @@ -1378,7 +1377,5 @@ class ConfigControllerTest extends WallabagCoreTestCase $client->request('GET', '/unread/list'); $this->assertStringNotContainsString('class="preview"', $client->getResponse()->getContent()); - - $client->request('GET', '/config/view-mode'); } } From ed1acf59e166a2a6bb81c52baaeabd6196feae98 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Wed, 19 Mar 2025 00:40:30 +0100 Subject: [PATCH 06/21] Protect changeLocale with a CSRF token --- .../Controller/ConfigController.php | 6 ++++- .../bundles/FOSUserBundle/layout.html.twig | 20 ++++++++++++++--- .../Controller/ConfigControllerTest.php | 22 ++++++++++++------- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index 5aae8122b..21f1f990d 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -672,12 +672,16 @@ class ConfigController extends AbstractController * * @param string $language * - * @Route("/locale/{language}", name="changeLocale") + * @Route("/locale/{language}", name="changeLocale", methods={"POST"}) * * @return RedirectResponse */ public function setLocaleAction(Request $request, ValidatorInterface $validator, $language = null) { + if (!$this->isCsrfTokenValid('change-locale', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $errors = $validator->validate($language, (new LocaleConstraint())); if (0 === \count($errors)) { diff --git a/templates/bundles/FOSUserBundle/layout.html.twig b/templates/bundles/FOSUserBundle/layout.html.twig index 937fd5cb0..191b492e5 100644 --- a/templates/bundles/FOSUserBundle/layout.html.twig +++ b/templates/bundles/FOSUserBundle/layout.html.twig @@ -16,9 +16,23 @@ {% endblock fos_user_content %}
- Deutsch – - English – - Français +
+ + + +
+ – +
+ + + +
+ – +
+ + + +
diff --git a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php index a9e51f3a8..9c6965ff6 100644 --- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php @@ -1133,19 +1133,21 @@ class ConfigControllerTest extends WallabagCoreTestCase { $client = $this->getTestClient(); - $client->request('GET', '/locale/de'); - $client->followRedirect(); + $crawler = $client->request('POST', '/locale/de'); - $this->assertSame('de', $client->getRequest()->getLocale()); - $this->assertSame('de', $client->getContainer()->get(SessionInterface::class)->get('_locale')); + $this->assertSame(400, $client->getResponse()->getStatusCode()); + $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); + $this->assertStringContainsString('Bad CSRF token.', $body[0]); } public function testChangeLocaleWithReferer() { $client = $this->getTestClient(); - $client->request('GET', '/login'); - $client->request('GET', '/locale/de'); + $crawler = $client->request('GET', '/login'); + + $client->submit($crawler->selectButton('Deutsch')->form()); + $client->followRedirect(); $this->assertSame('de', $client->getRequest()->getLocale()); @@ -1156,8 +1158,12 @@ class ConfigControllerTest extends WallabagCoreTestCase { $client = $this->getTestClient(); - $client->request('GET', '/login'); - $client->request('GET', '/locale/yuyuyuyu'); + $crawler = $client->request('GET', '/login'); + $token = $crawler->filter('form[action="/locale/de"] input[name=token]')->attr('value'); + + $client->request('POST', '/locale/yuyuyuyu', [ + 'token' => $token, + ]); $client->followRedirect(); $this->assertNotSame('yuyuyuyu', $client->getRequest()->getLocale()); From 3817010e29ed368df271cdd11ec71a46a341c673 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Wed, 19 Mar 2025 01:31:35 +0100 Subject: [PATCH 07/21] Protect reload_entry with a CSRF token --- .../themes/material/css/dark_theme.scss | 4 ++- .../static/themes/material/css/sidenav.scss | 32 ++++++++++++++++++- .../CoreBundle/Controller/EntryController.php | 9 ++++-- .../Resources/views/Entry/entry.html.twig | 12 ++++--- .../Controller/EntryControllerTest.php | 8 +++-- 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/app/Resources/static/themes/material/css/dark_theme.scss b/app/Resources/static/themes/material/css/dark_theme.scss index ac38b566d..9e4aec806 100644 --- a/app/Resources/static/themes/material/css/dark_theme.scss +++ b/app/Resources/static/themes/material/css/dark_theme.scss @@ -62,7 +62,9 @@ .nav-panels .input-field input:focus, .results-item, .side-nav li > a, - .side-nav li > a > i.material-icons { + .side-nav li > a > i.material-icons, + .side-nav li button, + .side-nav li button > i.material-icons { color: #dfdfdf; } diff --git a/app/Resources/static/themes/material/css/sidenav.scss b/app/Resources/static/themes/material/css/sidenav.scss index 00e4c5c2a..a7e50851e 100644 --- a/app/Resources/static/themes/material/css/sidenav.scss +++ b/app/Resources/static/themes/material/css/sidenav.scss @@ -12,6 +12,7 @@ background: initial; } + & button > i.material-icons.theme-toggle-icon, & > a > i.material-icons.theme-toggle-icon { float: none; margin-left: 0; @@ -22,6 +23,7 @@ margin: 0; } + &.fixed button, &.fixed a { font-size: 13px; line-height: 44px; @@ -41,7 +43,35 @@ } } -.bold > a { +// adapted from anchor styles from node_modules/materialize-css/sass/components/_sideNav.scss +.side-nav li button { + color: rgba(0 0 0 / 87%); + display: block; + font-size: 14px; + font-weight: 500; + height: 48px; + line-height: 48px; + padding: 0 (16px * 2); + width: 100%; + text-align: left; + + &:hover { + background-color: rgba(0 0 0 / 5%); + } + + & > i, + & > i.material-icons { + float: left; + height: 48px; + line-height: 48px; + margin: 0 (16px * 2) 0 0; + width: 24px; + color: rgba(0 0 0 / 54%); + } +} + +.bold > a, +.bold > button { font-weight: bold; } diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 3b624dd1f..66cfec10c 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -14,6 +14,7 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\Routing\Annotation\Route; use Symfony\Contracts\Translation\TranslatorInterface; use Wallabag\CoreBundle\Entity\Entry; @@ -400,12 +401,16 @@ class EntryController extends AbstractController * Reload an entry. * Refetch content from the website and make it readable again. * - * @Route("/reload/{id}", requirements={"id" = "\d+"}, name="reload_entry") + * @Route("/reload/{id}", name="reload_entry", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ - public function reloadAction(Entry $entry) + public function reloadAction(Request $request, Entry $entry) { + if (!$this->isCsrfTokenValid('reload-entry', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->checkUserAction($entry); $this->updateEntry($entry, 'entry_reloaded'); diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig index 9ce783bbf..0517f0baf 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig @@ -56,10 +56,14 @@
  • - - refresh - {{ 'entry.view.left_menu.re_fetch_content'|trans }} - +
    + + + +
    • diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 78143770e..128ad3657 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -509,7 +509,9 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->flush(); $this->getEntityManager()->clear(); - $client->request('GET', '/reload/' . $entry->getId()); + $crawler = $client->request('GET', '/view/' . $entry->getId()); + + $client->submit($crawler->selectButton('entry.view.left_menu.re_fetch_content')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); @@ -530,7 +532,9 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->persist($entry); $this->getEntityManager()->flush(); - $client->request('GET', '/reload/' . $entry->getId()); + $crawler = $client->request('GET', '/view/' . $entry->getId()); + + $client->submit($crawler->selectButton('entry.view.left_menu.re_fetch_content')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); From edffef837598355c9bec433c469f1e04c35b27cb Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Fri, 21 Mar 2025 23:21:25 +0100 Subject: [PATCH 08/21] Protect archive_entry with a CSRF token --- .../static/themes/material/css/nav.scss | 22 +++++++++++++ .../themes/material/js/shortcuts/entry.js | 2 +- .../CoreBundle/Controller/EntryController.php | 6 +++- .../views/Entry/_card_actions.html.twig | 8 ++++- .../views/Entry/_card_list.html.twig | 8 ++++- .../Resources/views/Entry/entry.html.twig | 32 ++++++++++++++----- .../Controller/EntryControllerTest.php | 12 ++++--- 7 files changed, 73 insertions(+), 17 deletions(-) diff --git a/app/Resources/static/themes/material/css/nav.scss b/app/Resources/static/themes/material/css/nav.scss index a085febd9..8a83596ff 100644 --- a/app/Resources/static/themes/material/css/nav.scss +++ b/app/Resources/static/themes/material/css/nav.scss @@ -6,11 +6,32 @@ nav { line-height: initial; } +// adapted from anchor styles from node_modules/materialize-css/sass/components/_navbar.scss +nav ul button { + transition: background-color .3s; + font-size: 1rem; + color: #fff; + display: block; + padding: 0 15px; + cursor: pointer; + background: none; + border: 0; + + &:focus { + background: none; + } + + &:hover { + background-color: rgba(0 0 0 / 10%); + } +} + nav { input { color: #aaa; } + ul button:hover, ul a:hover { background-color: initial; } @@ -34,6 +55,7 @@ nav { justify-content: space-between; align-items: center; + button, a { padding: 10px 15px; } diff --git a/app/Resources/static/themes/material/js/shortcuts/entry.js b/app/Resources/static/themes/material/js/shortcuts/entry.js index e19800bd3..a8cbd787a 100644 --- a/app/Resources/static/themes/material/js/shortcuts/entry.js +++ b/app/Resources/static/themes/material/js/shortcuts/entry.js @@ -15,7 +15,7 @@ $(document).ready(() => { /* mark as read */ Mousetrap.bind('a', () => { - $('ul.side-nav a.markasread i')[0].click(); + $('ul.side-nav button.markasread i')[0].click(); }); /* delete */ diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 66cfec10c..10c50a071 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -434,12 +434,16 @@ class EntryController extends AbstractController /** * Changes read status for an entry. * - * @Route("/archive/{id}", requirements={"id" = "\d+"}, name="archive_entry") + * @Route("/archive/{id}", name="archive_entry", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ public function toggleArchiveAction(Request $request, Entry $entry) { + if (!$this->isCsrfTokenValid('archive-entry', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->checkUserAction($entry); $entry->toggleArchive(); diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig index 8102409ee..376d7875c 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig @@ -14,7 +14,13 @@ language
  • - {% if entry.isArchived == 0 %}done{% else %}unarchive{% endif %} +
    + + + +
  • {% if entry.isStarred == 0 %}star_border{% else %}star{% endif %} diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig index 78b918912..e39d54cf8 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig @@ -15,7 +15,13 @@
    • - - {% if entry.isArchived == 0 %}done{% else %}unarchive{% endif %} - +
      + + + +
    • @@ -73,10 +77,14 @@ {% endif %}
    • - - {% if entry.isArchived == 0 %}done{% else %}unarchive{% endif %} - {{ mark_as_read_label|trans }} - +
      + + + +
      • @@ -304,7 +312,15 @@ menu diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 128ad3657..83b481560 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -645,7 +645,9 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->flush(); $this->getEntityManager()->clear(); - $client->request('GET', '/archive/' . $entry->getId()); + $crawler = $client->request('GET', '/view/' . $entry->getId()); + + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.set_as_read')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); @@ -1283,8 +1285,9 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->flush(); - $client->request('GET', '/view/' . $entry->getId()); - $client->request('GET', '/archive/' . $entry->getId()); + $crawler = $client->request('GET', '/view/' . $entry->getId()); + + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.set_as_read')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); $this->assertSame('/', $client->getResponse()->headers->get('location')); @@ -1308,8 +1311,7 @@ class EntryControllerTest extends WallabagCoreTestCase $crawler = $client->request('GET', '/view/' . $entry->getId()); - $link = $crawler->filter('a[id="markAsRead"]')->link(); - $client->click($link); + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.set_as_read')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); $this->assertStringContainsString('/view/' . $entry->getId(), $client->getResponse()->headers->get('location')); From 00d0e6f951927434039465b4d3ae3dd661911172 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Fri, 21 Mar 2025 23:28:08 +0100 Subject: [PATCH 09/21] Protect star_entry with a CSRF token --- .../themes/material/js/shortcuts/entry.js | 2 +- .../CoreBundle/Controller/EntryController.php | 6 +++- .../views/Entry/_card_actions.html.twig | 8 ++++- .../views/Entry/_card_list.html.twig | 8 ++++- .../Resources/views/Entry/entry.html.twig | 32 ++++++++++++++----- .../Controller/EntryControllerTest.php | 4 ++- 6 files changed, 47 insertions(+), 13 deletions(-) diff --git a/app/Resources/static/themes/material/js/shortcuts/entry.js b/app/Resources/static/themes/material/js/shortcuts/entry.js index a8cbd787a..3c84e4c6b 100644 --- a/app/Resources/static/themes/material/js/shortcuts/entry.js +++ b/app/Resources/static/themes/material/js/shortcuts/entry.js @@ -10,7 +10,7 @@ $(document).ready(() => { /* mark as favorite */ Mousetrap.bind('f', () => { - $('ul.side-nav a.favorite i')[0].click(); + $('ul.side-nav button.favorite i')[0].click(); }); /* mark as read */ diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 10c50a071..4032c5c73 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -467,12 +467,16 @@ class EntryController extends AbstractController /** * Changes starred status for an entry. * - * @Route("/star/{id}", requirements={"id" = "\d+"}, name="star_entry") + * @Route("/star/{id}", name="star_entry", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ public function toggleStarAction(Request $request, Entry $entry) { + if (!$this->isCsrfTokenValid('star-entry', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->checkUserAction($entry); $entry->toggleStar(); diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig index 376d7875c..bd6f7f2b9 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig @@ -23,7 +23,13 @@
    • - {% if entry.isStarred == 0 %}star_border{% else %}star{% endif %} +
      + + + +
    • delete diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig index e39d54cf8..cd1ff7df6 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig @@ -22,7 +22,13 @@ {% if entry.isArchived == 0 %}done{% else %}unarchive{% endif %} - {% if entry.isStarred == 0 %}star_border{% else %}star{% endif %} +
      + + + +
      delete
    diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig index cec681957..4aaa006bf 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig @@ -35,9 +35,13 @@
  • - - {% if entry.isStarred == 0 %}star_outline{% else %}star{% endif %} - +
    + + + +
    • @@ -89,10 +93,14 @@
  • - - {% if entry.isStarred == 0 %}star_outline{% else %}star{% endif %} - {{ 'entry.view.left_menu.set_as_starred'|trans }} - +
    + + + +
  • @@ -321,7 +329,15 @@
    • -
  • {% if entry.isStarred == 0 %}star_outline{% else %}star{% endif %}
    • +
  • +
    + + + +
    +
  • delete
    • diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 83b481560..5d1c74b46 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -670,7 +670,9 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->flush(); $this->getEntityManager()->clear(); - $client->request('GET', '/star/' . $entry->getId()); + $crawler = $client->request('GET', '/view/' . $entry->getId()); + + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.set_as_starred')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); From eb8408b22fbaa6b3d78047d6203b23b7f52bbf03 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Sun, 23 Mar 2025 03:09:42 +0100 Subject: [PATCH 10/21] Protect delete_entry with a CSRF token --- .../themes/material/js/shortcuts/entry.js | 2 +- .../CoreBundle/Controller/EntryController.php | 6 ++- .../views/Entry/_card_actions.html.twig | 8 +++- .../views/Entry/_card_list.html.twig | 8 +++- .../Resources/views/Entry/entry.html.twig | 22 +++++++-- .../Controller/EntryControllerTest.php | 46 +++++++++++-------- 6 files changed, 65 insertions(+), 27 deletions(-) diff --git a/app/Resources/static/themes/material/js/shortcuts/entry.js b/app/Resources/static/themes/material/js/shortcuts/entry.js index 3c84e4c6b..1c916b200 100644 --- a/app/Resources/static/themes/material/js/shortcuts/entry.js +++ b/app/Resources/static/themes/material/js/shortcuts/entry.js @@ -20,7 +20,7 @@ $(document).ready(() => { /* delete */ Mousetrap.bind('del', () => { - $('ul.side-nav a.delete i')[0].click(); + $('ul.side-nav button.delete i')[0].click(); }); } }); diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 4032c5c73..1f82cf9f0 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -501,12 +501,16 @@ class EntryController extends AbstractController /** * Deletes entry and redirect to the homepage or the last viewed page. * - * @Route("/delete/{id}", requirements={"id" = "\d+"}, name="delete_entry") + * @Route("/delete/{id}", name="delete_entry", methods={"POST"}, requirements={"id" = "\d+"}) * * @return RedirectResponse */ public function deleteEntryAction(Request $request, Entry $entry) { + if (!$this->isCsrfTokenValid('delete-entry', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->checkUserAction($entry); // generates the view url for this entry to check for redirection later diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig index bd6f7f2b9..140c283df 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig @@ -32,7 +32,13 @@
  • - delete +
    + + + +
    • diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig index cd1ff7df6..41877a0f5 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig @@ -29,7 +29,13 @@ {% if entry.isStarred == 0 %}star_border{% else %}star{% endif %} - delete +
    + + + +
    diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig index 4aaa006bf..c5f1600de 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig @@ -104,10 +104,14 @@
  • - - delete - {{ 'entry.view.left_menu.delete'|trans }} - +
    + + + +
    • @@ -338,7 +342,15 @@ -
  • delete
    • +
  • +
    + + + +
    +
    • diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 5d1c74b46..e3ffbafd1 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -694,13 +694,11 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->persist($entry); $this->getEntityManager()->flush(); - $client->request('GET', '/delete/' . $entry->getId()); + $crawler = $client->request('POST', '/delete/' . $entry->getId()); - $this->assertSame(302, $client->getResponse()->getStatusCode()); - - $client->request('GET', '/delete/' . $entry->getId()); - - $this->assertSame(404, $client->getResponse()->getStatusCode()); + $this->assertSame(400, $client->getResponse()->getStatusCode()); + $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text'])); + $this->assertStringContainsString('Bad CSRF token.', $body[0]); } /** @@ -736,10 +734,11 @@ class EntryControllerTest extends WallabagCoreTestCase $em->persist($content); $em->flush(); - $client->request('GET', '/view/' . $content->getId()); + $crawler = $client->request('GET', '/view/' . $content->getId()); $this->assertSame(200, $client->getResponse()->getStatusCode()); - $client->request('GET', '/delete/' . $content->getId()); + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.delete')->form()); + $this->assertSame(302, $client->getResponse()->getStatusCode()); $client->followRedirect(); @@ -1264,7 +1263,9 @@ class EntryControllerTest extends WallabagCoreTestCase ->getRepository(Entry::class) ->findByUrlAndUserId($url, $this->getLoggedInUserId()); - $client->request('GET', '/delete/' . $content->getId()); + $crawler = $client->request('GET', '/view/' . $content->getId()); + + $client->submit($crawler->filter('.left-bar')->selectButton('entry.view.left_menu.delete')->form()); $this->assertSame(302, $client->getResponse()->getStatusCode()); @@ -1437,7 +1438,8 @@ class EntryControllerTest extends WallabagCoreTestCase $crawler = $client->submit($form, $data); $this->assertCount(1, $crawler->filter($this->entryDataTestAttribute)); - $client->request('GET', '/delete/' . $entry->getId()); + + $client->submit($crawler->filter('.tools, .tools-list')->selectButton('delete')->form()); // test on list of all articles $crawler = $client->request('GET', '/all/list'); @@ -1508,8 +1510,8 @@ class EntryControllerTest extends WallabagCoreTestCase $crawler = $client->submit($form, $data); $currentUrl = $client->getRequest()->getUri(); - $element = $crawler->filter('a[data-action="delete"]')->link(); - $client->click($element); + $form = $crawler->filter('.tools, .tools-list')->selectButton('delete')->form(); + $client->submit($form); $client->followRedirect(); $nextUrl = $client->getRequest()->getUri(); $this->assertSame($currentUrl, $nextUrl); @@ -1752,8 +1754,8 @@ class EntryControllerTest extends WallabagCoreTestCase $this->getEntityManager()->clear(); $entries = []; - $entries[] = $entry1->getId(); - $entries[] = $entry2->getId(); + $entries[] = $entry1Id = $entry1->getId(); + $entries[] = $entry2Id = $entry2->getId(); // Mass actions : archive $client->request('POST', '/mass', [ @@ -1835,11 +1837,19 @@ class EntryControllerTest extends WallabagCoreTestCase 'entry-checkbox' => $entries, ]); - $client->request('GET', '/delete/' . $entry1->getId()); - $this->assertSame(404, $client->getResponse()->getStatusCode()); + $res = $client->getContainer() + ->get(EntityManagerInterface::class) + ->getRepository(Entry::class) + ->find($entry1Id); - $client->request('GET', '/delete/' . $entry2->getId()); - $this->assertSame(404, $client->getResponse()->getStatusCode()); + $this->assertNull($res); + + $res = $client->getContainer() + ->get(EntityManagerInterface::class) + ->getRepository(Entry::class) + ->find($entry2Id); + + $this->assertNull($res); } public function testGetSameDomainEntries() From 0d8429dfc77b84f50060b253fd84f1c09b892226 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Sun, 23 Mar 2025 12:52:16 +0100 Subject: [PATCH 11/21] Protect share with a CSRF token --- .../static/themes/material/css/dark_theme.scss | 1 + src/Wallabag/CoreBundle/Controller/EntryController.php | 10 +++++++--- .../CoreBundle/Resources/views/Entry/entry.html.twig | 10 +++++++--- .../CoreBundle/Controller/EntryControllerTest.php | 5 ++++- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/Resources/static/themes/material/css/dark_theme.scss b/app/Resources/static/themes/material/css/dark_theme.scss index 9e4aec806..532442360 100644 --- a/app/Resources/static/themes/material/css/dark_theme.scss +++ b/app/Resources/static/themes/material/css/dark_theme.scss @@ -89,6 +89,7 @@ .mass-action-tags .mass-action-tags-input.mass-action-tags-input, .side-nav li:not(.logo) > a:hover, + .side-nav li:not(.logo) button:hover, .side-nav .collapsible-header:hover, .side-nav.fixed .collapsible-header:hover { background-color: #1d1d1d; diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 1f82cf9f0..1efc81991 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -543,12 +543,16 @@ class EntryController extends AbstractController /** * Get public URL for entry (and generate it if necessary). * - * @Route("/share/{id}", requirements={"id" = "\d+"}, name="share") + * @Route("/share/{id}", name="share", methods={"POST"}, requirements={"id" = "\d+"}) * * @return Response */ - public function shareAction(Entry $entry) + public function shareAction(Request $request, Entry $entry) { + if (!$this->isCsrfTokenValid('share-entry', $request->request->get('token'))) { + throw new BadRequestHttpException('Bad CSRF token.'); + } + $this->checkUserAction($entry); if (null === $entry->getUid()) { @@ -587,7 +591,7 @@ class EntryController extends AbstractController /** * Ability to view a content publicly. * - * @Route("/share/{uid}", requirements={"uid" = ".+"}, name="share_entry") + * @Route("/share/{uid}", name="share_entry", methods={"GET"}, requirements={"uid" = ".+"}) * @Cache(maxage="25200", smaxage="25200", public=true) * * @return Response diff --git a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig index c5f1600de..9f2154766 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig @@ -159,9 +159,13 @@