AnnotationController: fix improper authorization vulnerability

This PR is based on 2.5.x branch. We fix the improper authorization by retrieving the annotation using id and user id. We also replace the ParamConverter used to get the requested Annotation on put and delete actions with an explicit call to AnnotationRepository in order to prevent a resource enumeration through response discrepancy. Fixes GHSA-mrqx-mjc4-vfh3 Co-authored-by: Jeremy Benoist <jeremy.benoist@gmail.com> Signed-off-by: Kevin Decherf <kevin@kdecherf.com>
2024-06-09 08:49:27 +00:00 · 2023-01-23 12:16:09 +01:00 · 2023-01-23 12:16:09 +01:00 · 3ed7f2b751
parent 9e9aedee94
commit 3ed7f2b751
6 changed files with 173 additions and 62 deletions
--- a/src/Wallabag/AnnotationBundle/Controller/WallabagAnnotationController.php
+++ b/src/Wallabag/AnnotationBundle/Controller/WallabagAnnotationController.php
@ -3,9 +3,9 @@
 namespace Wallabag\AnnotationBundle\Controller;

 use FOS\RestBundle\Controller\FOSRestController;
-use Sensio\Bundle\FrameworkExtraBundle\Configuration\ParamConverter;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Wallabag\AnnotationBundle\Entity\Annotation;
 use Wallabag\AnnotationBundle\Form\EditAnnotationType;
 use Wallabag\AnnotationBundle\Form\NewAnnotationType;
@ -25,7 +25,7 @@ class WallabagAnnotationController extends FOSRestController
        $annotationRows = $this
            ->getDoctrine()
            ->getRepository('WallabagAnnotationBundle:Annotation')
-            ->findAnnotationsByPageId($entry->getId(), $this->getUser()->getId());
+            ->findByEntryIdAndUserId($entry->getId(), $this->getUser()->getId());
        $total = \count($annotationRows);
        $annotations = ['total' => $total, 'rows' => $annotationRows];

@ -72,31 +72,35 @@ class WallabagAnnotationController extends FOSRestController
     *
     * @see Wallabag\ApiBundle\Controller\WallabagRestController
     *
-     * @ParamConverter("annotation", class="WallabagAnnotationBundle:Annotation")
-     *
     * @return JsonResponse
     */
-    public function putAnnotationAction(Annotation $annotation, Request $request)
+    public function putAnnotationAction(Request $request, int $annotation)
    {
-        $data = json_decode($request->getContent(), true);
+        try {
+            $annotation = $this->validateAnnotation($annotation, $this->getUser()->getId());

-        $form = $this->get('form.factory')->createNamed('', EditAnnotationType::class, $annotation, [
-            'csrf_protection' => false,
-            'allow_extra_fields' => true,
-        ]);
-        $form->submit($data);
+            $data = json_decode($request->getContent(), true, 512, \JSON_THROW_ON_ERROR);

-        if ($form->isValid()) {
-            $em = $this->getDoctrine()->getManager();
-            $em->persist($annotation);
-            $em->flush();
+            $form = $this->get('form.factory')->createNamed('', EditAnnotationType::class, $annotation, [
+                'csrf_protection' => false,
+                'allow_extra_fields' => true,
+            ]);
+            $form->submit($data);

-            $json = $this->get('jms_serializer')->serialize($annotation, 'json');
+            if ($form->isValid()) {
+                $em = $this->getDoctrine()->getManager();
+                $em->persist($annotation);
+                $em->flush();

-            return JsonResponse::fromJsonString($json);
+                $json = $this->get('jms_serializer')->serialize($annotation, 'json');
+
+                return JsonResponse::fromJsonString($json);
+            }
+
+            return $form;
+        } catch (\InvalidArgumentException $e) {
+            throw new NotFoundHttpException($e);
        }
-
-        return $form;
    }

    /**
@ -104,18 +108,35 @@ class WallabagAnnotationController extends FOSRestController
     *
     * @see Wallabag\ApiBundle\Controller\WallabagRestController
     *
-     * @ParamConverter("annotation", class="WallabagAnnotationBundle:Annotation")
-     *
     * @return JsonResponse
     */
-    public function deleteAnnotationAction(Annotation $annotation)
+    public function deleteAnnotationAction(int $annotation)
+    {
+        try {
+            $annotation = $this->validateAnnotation($annotation, $this->getUser()->getId());
+
+            $em = $this->getDoctrine()->getManager();
+            $em->remove($annotation);
+            $em->flush();
+
+            $json = $this->get('jms_serializer')->serialize($annotation, 'json');
+
+            return (new JsonResponse())->setJson($json);
+        } catch (\InvalidArgumentException $e) {
+            throw new NotFoundHttpException($e);
+        }
+    }
+
+    private function validateAnnotation(int $annotationId, int $userId)
    {
        $em = $this->getDoctrine()->getManager();
-        $em->remove($annotation);
-        $em->flush();

-        $json = $this->get('jms_serializer')->serialize($annotation, 'json');
+        $annotation = $em->getRepository('WallabagAnnotationBundle:Annotation')->findOneByIdAndUserId($annotationId, $userId);

-        return (new JsonResponse())->setJson($json);
+        if (null === $annotation) {
+            throw new NotFoundHttpException();
+        }
+
+        return $annotation;
    }
 }
--- a/src/Wallabag/AnnotationBundle/DataFixtures/AnnotationFixtures.php
+++ b/src/Wallabag/AnnotationBundle/DataFixtures/AnnotationFixtures.php
@ -34,6 +34,15 @@ class AnnotationFixtures extends Fixture implements DependentFixtureInterface

        $this->addReference('annotation2', $annotation2);

+        $annotation3 = new Annotation($this->getReference('bob-user'));
+        $annotation3->setEntry($this->getReference('entry3'));
+        $annotation3->setText('This is my first annotation !');
+        $annotation3->setQuote('content');
+
+        $manager->persist($annotation3);
+
+        $this->addReference('annotation3', $annotation3);
+
        $manager->flush();
    }

--- a/src/Wallabag/AnnotationBundle/Repository/AnnotationRepository.php
+++ b/src/Wallabag/AnnotationBundle/Repository/AnnotationRepository.php
@ -41,6 +41,24 @@ class AnnotationRepository extends EntityRepository
        ;
    }

+    /**
+     * Find annotation by id and user.
+     *
+     * @param int $annotationId
+     * @param int $userId
+     *
+     * @return Annotation
+     */
+    public function findOneByIdAndUserId($annotationId, $userId)
+    {
+        return $this->createQueryBuilder('a')
+                    ->where('a.id = :annotationId')->setParameter('annotationId', $annotationId)
+                    ->andWhere('a.user = :userId')->setParameter('userId', $userId)
+                    ->setMaxResults(1)
+                    ->getQuery()
+                    ->getOneOrNullResult();
+    }
+
    /**
     * Find annotations for entry id.
     *
@ -49,7 +67,7 @@ class AnnotationRepository extends EntityRepository
     *
     * @return array
     */
-    public function findAnnotationsByPageId($entryId, $userId)
+    public function findByEntryIdAndUserId($entryId, $userId)
    {
        return $this->createQueryBuilder('a')
            ->where('a.entry = :entryId')->setParameter('entryId', $entryId)
@ -66,7 +84,7 @@ class AnnotationRepository extends EntityRepository
     *
     * @return array
     */
-    public function findLastAnnotationByPageId($entryId, $userId)
+    public function findLastAnnotationByUserId($entryId, $userId)
    {
        return $this->createQueryBuilder('a')
            ->where('a.entry = :entryId')->setParameter('entryId', $entryId)
--- a/src/Wallabag/ApiBundle/Controller/AnnotationRestController.php
+++ b/src/Wallabag/ApiBundle/Controller/AnnotationRestController.php
@ -3,7 +3,6 @@
 namespace Wallabag\ApiBundle\Controller;

 use Nelmio\ApiDocBundle\Annotation\ApiDoc;
-use Sensio\Bundle\FrameworkExtraBundle\Configuration\ParamConverter;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Wallabag\AnnotationBundle\Entity\Annotation;
@ -63,11 +62,9 @@ class AnnotationRestController extends WallabagRestController
     *      }
     * )
     *
-     * @ParamConverter("annotation", class="WallabagAnnotationBundle:Annotation")
-     *
     * @return JsonResponse
     */
-    public function putAnnotationAction(Annotation $annotation, Request $request)
+    public function putAnnotationAction(int $annotation, Request $request)
    {
        $this->validateAuthentication();

@ -86,11 +83,9 @@ class AnnotationRestController extends WallabagRestController
     *      }
     * )
     *
-     * @ParamConverter("annotation", class="WallabagAnnotationBundle:Annotation")
-     *
     * @return JsonResponse
     */
-    public function deleteAnnotationAction(Annotation $annotation)
+    public function deleteAnnotationAction(int $annotation)
    {
        $this->validateAuthentication();

--- a/tests/Wallabag/AnnotationBundle/Controller/AnnotationControllerTest.php
+++ b/tests/Wallabag/AnnotationBundle/Controller/AnnotationControllerTest.php
@ -22,8 +22,6 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase
    }

    /**
-     * Test fetching annotations for an entry.
-     *
     * @dataProvider dataForEachAnnotations
     */
    public function testGetAnnotations($prefixUrl)
@ -35,15 +33,7 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase
            ->findOneByUserName('admin');
        $entry = $em
            ->getRepository('WallabagCoreBundle:Entry')
-            ->findOneByUsernameAndNotArchived('admin');
-
-        $annotation = new Annotation($user);
-        $annotation->setEntry($entry);
-        $annotation->setText('This is my annotation /o/');
-        $annotation->setQuote('content');
-
-        $em->persist($annotation);
-        $em->flush();
+            ->findByUrlAndUserId('http://0.0.0.0/entry1', $user->getId());

        if ('annotations' === $prefixUrl) {
            $this->logInAs('admin');
@ -54,23 +44,44 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase

        $content = json_decode($this->client->getResponse()->getContent(), true);
        $this->assertGreaterThanOrEqual(1, $content['total']);
-        $this->assertSame($annotation->getText(), $content['rows'][0]['text']);
-
-        // we need to re-fetch the annotation becase after the flush, it has been "detached" from the entity manager
-        $annotation = $em->getRepository('WallabagAnnotationBundle:Annotation')->findAnnotationById($annotation->getId());
-        $em->remove($annotation);
-        $em->flush();
    }

    /**
-     * Test creating an annotation for an entry.
-     *
+     * @dataProvider dataForEachAnnotations
+     */
+    public function testGetAnnotationsFromAnOtherUser($prefixUrl)
+    {
+        $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
+
+        $otherUser = $em
+            ->getRepository('WallabagUserBundle:User')
+            ->findOneByUserName('bob');
+        $entry = $em
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findByUrlAndUserId('http://0.0.0.0/entry3', $otherUser->getId());
+
+        if ('annotations' === $prefixUrl) {
+            $this->logInAs('admin');
+        }
+
+        $this->client->request('GET', $prefixUrl . '/' . $entry->getId() . '.json');
+        $this->assertSame(200, $this->client->getResponse()->getStatusCode());
+
+        $content = json_decode($this->client->getResponse()->getContent(), true);
+        $this->assertGreaterThanOrEqual(0, $content['total']);
+    }
+
+    /**
     * @dataProvider dataForEachAnnotations
     */
    public function testSetAnnotation($prefixUrl)
    {
        $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');

+        $user = $em
+            ->getRepository('WallabagUserBundle:User')
+            ->findOneByUserName('admin');
+
        if ('annotations' === $prefixUrl) {
            $this->logInAs('admin');
        }
@ -102,7 +113,7 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase
        /** @var Annotation $annotation */
        $annotation = $em
            ->getRepository('WallabagAnnotationBundle:Annotation')
-            ->findLastAnnotationByPageId($entry->getId(), 1);
+            ->findLastAnnotationByUserId($entry->getId(), $user->getId());

        $this->assertSame('my annotation', $annotation->getText());
    }
@ -195,8 +206,6 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase
    }

    /**
-     * Test editing an existing annotation.
-     *
     * @dataProvider dataForEachAnnotations
     */
    public function testEditAnnotation($prefixUrl)
@ -243,8 +252,31 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase
    }

    /**
-     * Test deleting an annotation.
-     *
+     * @dataProvider dataForEachAnnotations
+     */
+    public function testEditAnnotationFromAnOtherUser($prefixUrl)
+    {
+        $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
+
+        $otherUser = $em
+            ->getRepository('WallabagUserBundle:User')
+            ->findOneByUserName('bob');
+        $entry = $em
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findByUrlAndUserId('http://0.0.0.0/entry3', $otherUser->getId());
+        $annotation = $em
+            ->getRepository('WallabagAnnotationBundle:Annotation')
+            ->findLastAnnotationByUserId($entry->getId(), $otherUser->getId());
+
+        $headers = ['CONTENT_TYPE' => 'application/json'];
+        $content = json_encode([
+            'text' => 'a modified annotation',
+        ]);
+        $this->client->request('PUT', $prefixUrl . '/' . $annotation->getId() . '.json', [], [], $headers, $content);
+        $this->assertSame(404, $this->client->getResponse()->getStatusCode());
+    }
+
+    /**
     * @dataProvider dataForEachAnnotations
     */
    public function testDeleteAnnotation($prefixUrl)
@ -287,4 +319,40 @@ class AnnotationControllerTest extends WallabagAnnotationTestCase

        $this->assertNull($annotationDeleted);
    }
+
+    /**
+     * @dataProvider dataForEachAnnotations
+     */
+    public function testDeleteAnnotationFromAnOtherUser($prefixUrl)
+    {
+        $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
+
+        $otherUser = $em
+            ->getRepository('WallabagUserBundle:User')
+            ->findOneByUserName('bob');
+        $entry = $em
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findByUrlAndUserId('http://0.0.0.0/entry3', $otherUser->getId());
+        $annotation = $em
+            ->getRepository('WallabagAnnotationBundle:Annotation')
+            ->findLastAnnotationByUserId($entry->getId(), $otherUser->getId());
+
+        $user = $em
+            ->getRepository('WallabagUserBundle:User')
+            ->findOneByUserName('admin');
+        $entry = $em
+            ->getRepository('WallabagCoreBundle:Entry')
+            ->findOneByUsernameAndNotArchived('admin');
+
+        if ('annotations' === $prefixUrl) {
+            $this->logInAs('admin');
+        }
+
+        $headers = ['CONTENT_TYPE' => 'application/json'];
+        $content = json_encode([
+            'text' => 'a modified annotation',
+        ]);
+        $this->client->request('DELETE', $prefixUrl . '/' . $annotation->getId() . '.json', [], [], $headers, $content);
+        $this->assertSame(404, $this->client->getResponse()->getStatusCode());
+    }
 }
--- a/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
+++ b/tests/Wallabag/CoreBundle/Controller/ConfigControllerTest.php
@ -932,7 +932,7 @@ class ConfigControllerTest extends WallabagCoreTestCase

        $annotationsReset = $em
            ->getRepository('WallabagAnnotationBundle:Annotation')
-            ->findAnnotationsByPageId($entry->getId(), $user->getId());
+            ->findByEntryIdAndUserId($entry->getId(), $user->getId());

        $this->assertEmpty($annotationsReset, 'Annotations were reset');

@ -1040,7 +1040,7 @@ class ConfigControllerTest extends WallabagCoreTestCase

        $annotationsReset = $em
            ->getRepository('WallabagAnnotationBundle:Annotation')
-            ->findAnnotationsByPageId($annotationArchived->getId(), $user->getId());
+            ->findByEntryIdAndUserId($annotationArchived->getId(), $user->getId());

        $this->assertEmpty($annotationsReset, 'Annotations were reset');
    }
@ -1097,7 +1097,7 @@ class ConfigControllerTest extends WallabagCoreTestCase

        $annotationsReset = $em
            ->getRepository('WallabagAnnotationBundle:Annotation')
-            ->findAnnotationsByPageId($entry->getId(), $user->getId());
+            ->findByEntryIdAndUserId($entry->getId(), $user->getId());

        $this->assertEmpty($annotationsReset, 'Annotations were reset');
    }