Merge pull request from GHSA-mrqx-mjc4-vfh3

AnnotationController: fix improper authorization vulnerability
2024-11-26 02:51:04 +00:00 · 2023-02-01 09:32:22 +01:00 · 2023-02-01 09:32:22 +01:00 · 5ac6b6bff9
commit 5ac6b6bff9
parent 0f7460dbab 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,13 +72,14 @@ 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());
+
+            $data = json_decode($request->getContent(), true, 512, \JSON_THROW_ON_ERROR);

            $form = $this->get('form.factory')->createNamed('', EditAnnotationType::class, $annotation, [
                'csrf_protection' => false,
@ -97,6 +98,9 @@ class WallabagAnnotationController extends FOSRestController
            }

            return $form;
+        } catch (\InvalidArgumentException $e) {
+            throw new NotFoundHttpException($e);
+        }
    }

    /**
@ -104,12 +108,13 @@ 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();
@ -117,5 +122,21 @@ class WallabagAnnotationController extends FOSRestController
            $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();
+
+        $annotation = $em->getRepository('WallabagAnnotationBundle:Annotation')->findOneByIdAndUserId($annotationId, $userId);
+
+        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');
    }