From 0fdd9aa991da6f76333077a3a93244035c02cc86 Mon Sep 17 00:00:00 2001
From: Kevin Decherf <kevin@kdecherf.com>
Date: Thu, 12 Jan 2023 23:40:16 +0100
Subject: [PATCH] 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>
---
 .../CoreBundle/Controller/ExportController.php  | 17 ++++++++++++++---
 .../Controller/ExportControllerTest.php         | 14 +++++++++++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/Wallabag/CoreBundle/Controller/ExportController.php b/src/Wallabag/CoreBundle/Controller/ExportController.php
index 2db335527c..0599c401d7 100644
--- a/src/Wallabag/CoreBundle/Controller/ExportController.php
+++ b/src/Wallabag/CoreBundle/Controller/ExportController.php
@@ -6,7 +6,6 @@
 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')
diff --git a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php
index 41b995efaa..9d983200ca 100644
--- a/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php
+++ b/tests/Wallabag/CoreBundle/Controller/ExportControllerTest.php
@@ -57,7 +57,7 @@ public function testUnsupportedFormatExport()
         $this->assertSame(404, $client->getResponse()->getStatusCode());
     }
 
-    public function testBadEntryId()
+    public function testNonExistingEntryId()
     {
         $this->logInAs('admin');
         $client = $this->getClient();
@@ -67,6 +67,18 @@ public function testBadEntryId()
         $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');