Merge pull request #568 from creative-commoners/pulls/4.3/cve-2023-44401

[CVE-2023-44401] Ensure canView() checks are run

src/Schema/DataObject/Plugin/CanViewPermission.php

@@ -13,6 +13,7 @@
 use InvalidArgumentException;
 use SilverStripe\ORM\SS_List;
 use SilverStripe\View\ArrayData;
+use SilverStripe\ORM\DataList;
 
 /**
  * A permission checking plugin for DataLists
@@ -83,19 +84,9 @@ public static function permissionCheck($obj, array $args, array $context, Resolv
     public static function paginatedPermissionCheck(array $obj, array $args, array $context, ResolveInfo $info): array
     {
         $list = $obj['nodes'];
-        $originalCount = count($list ?? []);
         $filteredList = static::permissionCheck($list, $args, $context, $info);
-        $newCount = $filteredList->count();
-        if ($originalCount === $newCount) {
-            return $obj;
-        }
         $obj['nodes'] = $filteredList;
-        $edges = [];
-        foreach ($filteredList as $record) {
-            $edges[] = ['node' => $record];
-        }
-        $obj['edges'] = $edges;
-
+        $obj['edges'] = $filteredList;
         return $obj;
     }
 
@@ -123,6 +114,20 @@ public static function itemPermissionCheck($obj, array $args, array $context, Re
      */
     public static function listPermissionCheck(Filterable $obj, array $args, array $context, ResolveInfo $info): Filterable
     {
+        // Use an ArrayList rather than a DataList to ensure items returns all have had a canView() check run on them.
+        // Converting to an ArrayList will run a query and mean we start with a fixed number of items before
+        // running the canView() check on them e.g. 10 results. from there we remove items that fail a canView() check.
+        // This means the paginated results may only return say 6 out of 10 results. This is consistent with
+        // Silverstripe pagination and canView() checks.
+        // If we don't convert run the query immediately by converting to an ArrayList, then the resulting SQL will
+        // be SELECT ... WHERE "ID" NOT IN (1,2,...) ... LIMIT 10, which will result in 10 results
+        // however there will be 4 additional results that have NOT had a canView() check run on them
+        if ($obj instanceof DataList) {
+            $new = ArrayList::create();
+            $new->merge($obj);
+            $obj = $new;
+        }
+
         $member = UserContextProvider::get($context);
         $excludes = [];
 

tests/Fake/FakeDataObjectWithCanView.php

@@ -0,0 +1,20 @@
+<?php
+
+namespace SilverStripe\GraphQL\Tests\Fake;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class FakeDataObjectWithCanView extends DataObject implements TestOnly
+{
+    private static $db = [
+        'Title' => 'Varchar',
+    ];
+
+    private static $table_name = 'FakeDataObjectWithCanView_Test';
+
+    public function canView($member = null)
+    {
+        return $this->ID % 2;
+    }
+}

tests/Schema/IntegrationTest.php

@@ -24,6 +24,7 @@
 use SilverStripe\GraphQL\Schema\Storage\HashNameObfuscator;
 use SilverStripe\GraphQL\Schema\Storage\NameObfuscator;
 use SilverStripe\GraphQL\Tests\Fake\DataObjectFake;
+use SilverStripe\GraphQL\Tests\Fake\FakeDataObjectWithCanView;
 use SilverStripe\GraphQL\Tests\Fake\FakePage;
 use SilverStripe\GraphQL\Tests\Fake\FakeProduct;
 use SilverStripe\GraphQL\Tests\Fake\FakeProductPage;
@@ -55,6 +56,7 @@ class IntegrationTest extends SapphireTest
         FakeProduct::class,
         FakeReview::class,
         Member::class,
+        FakeDataObjectWithCanView::class,
     ];
 
     protected function setUp(): void
@@ -70,6 +72,7 @@ protected function tearDown(): void
         DataObjectFake::get()->removeAll();
         File::get()->removeAll();
         Member::get()->removeAll();
+        FakeDataObjectWithCanView::get()->removeAll();
     }
 
     public function testSimpleType()
@@ -1008,6 +1011,43 @@ public function testBasicPaginator()
         ], $records);
     }
 
+    public function testCanViewPagination()
+    {
+        // FakeDataObjectWithCanView has a canView() check of `return $this->ID % 2` i.e. half the records are viewable
+        // This test will:
+        // - Create 20 records
+        // - Query 10 records
+        // - Assert that 5 records were returned
+        for ($i = 0; $i < 20; $i++) {
+            $obj = FakeDataObjectWithCanView::create(['Title' => "obj$i"]);
+            $obj->write();
+        }
+        $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__]));
+        $query = <<<GRAPHQL
+            query {
+              readFakeDataObjectWithCanViews(limit: 10) {
+                nodes {
+                  id
+                  title
+                }
+                edges {
+                  node {
+                    id
+                    title
+                  }
+                }
+              }
+            }
+        GRAPHQL;
+        $result = $this->querySchema($schema, $query);
+        $this->assertSuccess($result);
+        $records = $result['data']['readFakeDataObjectWithCanViews']['nodes'];
+        $this->assertCount(5, $records);
+        $ids = array_map(fn($record) => $record['id'], $records);
+        $filteredIDs = array_filter($ids, fn($id) => $id % 2);
+        $this->assertSame(count($ids), count($filteredIDs));
+    }
+
     /**
      * @throws SchemaBuilderException
      * @throws SchemaNotFoundException

tests/Schema/_testCanViewPagination/schema.yml

@@ -0,0 +1,8 @@
+models:
+  SilverStripe\GraphQL\Tests\Fake\FakeDataObjectWithCanView:
+    fields: '*'
+    operations:
+      read:
+        plugins:
+          paginateList: true
+          canView: true