ExportController: fix improper authorization vulnerability

We fix the improper authorization by duplicating the check done by
the private method EntryController::checkUserAction().

We also replace the ParamConverter used to get the requested Entry with
an explicit call to EntryRepository in order to prevent a resource
enumeration through response discrepancy. Thus, we get the same
exception whether the requested resource does not exist or is not owned
by the requester.

Fixes GHSA-qwx8-mxxx-mg96

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

src/Wallabag/CoreBundle/Controller/ExportController.php

@@ -6,7 +6,6 @@ use Symfony\Bundle\FrameworkBundle\Controller\Controller;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Annotation\Route;
-use Wallabag\CoreBundle\Entity\Entry;
 
 /**
  * The try/catch can be removed once all formats will be implemented.
@@ -26,9 +25,21 @@ class ExportController extends Controller
      *
      * @return \Symfony\Component\HttpFoundation\Response
      */
-    public function downloadEntryAction(Entry $entry, $format)
+    public function downloadEntryAction(Request $request, $format)
     {
-        try {
+         try {
+            $entry = $this->get('wallabag_core.entry_repository')
+                ->find((int) $request->query->get('id'));
+
+            /**
+             * We duplicate EntryController::checkUserAction here as a quick fix for an improper authorization vulnerability
+             *
+             * This should be eventually rewritten
+            */
+            if (null === $entry || null === $this->getUser() || $this->getUser()->getId() !== $entry->getUser()->getId()) {
+                throw new NotFoundHttpException();
+            }
+
             return $this->get('wallabag_core.helper.entries_export')
                 ->setEntries($entry)
                 ->updateTitle('entry')

tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php

@@ -57,7 +57,7 @@ class ExportControllerTest extends WallabagCoreTestCase
         $this->assertSame(404, $client->getResponse()->getStatusCode());
     }
 
-    public function testBadEntryId()
+    public function testNonExistingEntryId()
     {
         $this->logInAs('admin');
         $client = $this->getClient();
@@ -67,6 +67,18 @@ class ExportControllerTest extends WallabagCoreTestCase
         $this->assertSame(404, $client->getResponse()->getStatusCode());
     }
 
+    public function testForbiddenEntryId()
+    {
+        $this->logInAs('admin');
+        $client = $this->getClient();
+
+        // Entry with id 3 is owned by the user bob
+        // See EntryFixtures
+        $client->request('GET', '/export/3.mobi');
+
+        $this->assertSame(404, $client->getResponse()->getStatusCode());
+    }
+
     public function testEpubExport()
     {
         $this->logInAs('admin');