Protect delete_entry with a CSRF token

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();
     });
   }
 });

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

src/Wallabag/CoreBundle/Resources/views/Entry/_card_actions.html.twig

@@ -32,7 +32,13 @@
             </form>
         </li>
         <li>
-            <a title="{{ 'entry.list.delete'|trans }}" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')" data-action-confirm="{{ 'entry.confirm.delete'|trans }}" class="tool grey-text delete" href="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" data-action="delete" data-entry-id="{{ entry.id }}"><i class="material-icons">delete</i></a>
+            <form action="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" method="post" class="inline-block">
+                <input type="hidden" name="token" value="{{ csrf_token('delete-entry') }}"/>
+
+                <button type="submit" class="btn-link tool grey-text" title="{{ 'entry.list.delete'|trans }}" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')">
+                    <i class="material-icons">delete</i>
+                </button>
+            </form>
         </li>
     </ul>
 </div>

src/Wallabag/CoreBundle/Resources/views/Entry/_card_list.html.twig

@@ -29,7 +29,13 @@
                     <i class="material-icons">{% if entry.isStarred == 0 %}star_border{% else %}star{% endif %}</i>
                 </button>
             </form>
-            <a title="{{ 'entry.list.delete'|trans }}" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')" class="tool grey-text delete" href="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}"><i class="material-icons">delete</i></a>
+            <form action="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" method="post" class="inline-block">
+                <input type="hidden" name="token" value="{{ csrf_token('delete-entry') }}"/>
+
+                <button type="submit" class="btn-link tool grey-text" title="{{ 'entry.list.delete'|trans }}" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')">
+                    <i class="material-icons">delete</i>
+                </button>
+            </form>
         </li>
     </ul>
 </div>

src/Wallabag/CoreBundle/Resources/views/Entry/entry.html.twig

@@ -104,10 +104,14 @@
             <div class="collapsible-body"></div>
         </li>
         <li class="bold border-bottom">
-            <a class="waves-effect collapsible-header delete" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')" title="{{ 'entry.view.left_menu.delete'|trans }}" href="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}">
+            <form action="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" method="post">
-                <i class="material-icons small">delete</i>
+                <input type="hidden" name="token" value="{{ csrf_token('delete-entry') }}"/>
-                <span>{{ 'entry.view.left_menu.delete'|trans }}</span>
+
-            </a>
+                <button type="submit" class="waves-effect collapsible-header delete" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')" title="{{ 'entry.view.left_menu.delete'|trans }}">
+                    <i class="material-icons small">delete</i>
+                    <span>{{ 'entry.view.left_menu.delete'|trans }}</span>
+                </button>
+            </form>
             <div class="collapsible-body"></div>
         </li>
 
@@ -338,7 +342,15 @@
                       </button>
                   </form>
               </li>
-              <li><a class="btn-floating" href="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')"><i class="material-icons">delete</i></a></li>
+              <li>
+                  <form action="{{ path('delete_entry', {'id': entry.id, redirect: current_path}) }}" method="post" class="inline-block">
+                      <input type="hidden" name="token" value="{{ csrf_token('delete-entry') }}"/>
+
+                      <button type="submit" class="btn-floating" onclick="return confirm('{{ 'entry.confirm.delete'|trans|escape('js') }}')">
+                          <i class="material-icons">delete</i>
+                      </button>
+                  </form>
+              </li>
             </ul>
         </div>
     </div>

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());
+        $this->assertSame(400, $client->getResponse()->getStatusCode());
-
+        $this->assertGreaterThan(1, $body = $crawler->filter('body')->extract(['_text']));
-        $client->request('GET', '/delete/' . $entry->getId());
+        $this->assertStringContainsString('Bad CSRF token.', $body[0]);
-
-        $this->assertSame(404, $client->getResponse()->getStatusCode());
     }
 
     /**
@@ -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();
+        $form = $crawler->filter('.tools, .tools-list')->selectButton('delete')->form();
-        $client->click($element);
+        $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[] = $entry1Id = $entry1->getId();
-        $entries[] = $entry2->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());
+        $res = $client->getContainer()
-        $this->assertSame(404, $client->getResponse()->getStatusCode());
+            ->get(EntityManagerInterface::class)
+            ->getRepository(Entry::class)
+            ->find($entry1Id);
 
-        $client->request('GET', '/delete/' . $entry2->getId());
+        $this->assertNull($res);
-        $this->assertSame(404, $client->getResponse()->getStatusCode());
+
+        $res = $client->getContainer()
+            ->get(EntityManagerInterface::class)
+            ->getRepository(Entry::class)
+            ->find($entry2Id);
+
+        $this->assertNull($res);
     }
 
     public function testGetSameDomainEntries()