From 8e645670aecd6858069f33bc092485d09ee9f734 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 1 Nov 2023 10:57:30 +1300 Subject: [PATCH] [CVE-2023-44401] Ensure canView() checks are run Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> --- .../DataObject/Plugin/CanViewPermission.php | 27 ++++++++----- tests/Fake/FakeDataObjectWithCanView.php | 20 ++++++++++ tests/Schema/IntegrationTest.php | 40 +++++++++++++++++++ .../Schema/_testCanViewPagination/schema.yml | 8 ++++ 4 files changed, 84 insertions(+), 11 deletions(-) create mode 100644 tests/Fake/FakeDataObjectWithCanView.php create mode 100644 tests/Schema/_testCanViewPagination/schema.yml diff --git a/src/Schema/DataObject/Plugin/CanViewPermission.php b/src/Schema/DataObject/Plugin/CanViewPermission.php index 106089c3..a8adfcf1 100644 --- a/src/Schema/DataObject/Plugin/CanViewPermission.php +++ b/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 = []; diff --git a/tests/Fake/FakeDataObjectWithCanView.php b/tests/Fake/FakeDataObjectWithCanView.php new file mode 100644 index 00000000..7db00639 --- /dev/null +++ b/tests/Fake/FakeDataObjectWithCanView.php @@ -0,0 +1,20 @@ + 'Varchar', + ]; + + private static $table_name = 'FakeDataObjectWithCanView_Test'; + + public function canView($member = null) + { + return $this->ID % 2; + } +} diff --git a/tests/Schema/IntegrationTest.php b/tests/Schema/IntegrationTest.php index 987cfb58..c3cab186 100644 --- a/tests/Schema/IntegrationTest.php +++ b/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 = <<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 diff --git a/tests/Schema/_testCanViewPagination/schema.yml b/tests/Schema/_testCanViewPagination/schema.yml new file mode 100644 index 00000000..d59343a2 --- /dev/null +++ b/tests/Schema/_testCanViewPagination/schema.yml @@ -0,0 +1,8 @@ +models: + SilverStripe\GraphQL\Tests\Fake\FakeDataObjectWithCanView: + fields: '*' + operations: + read: + plugins: + paginateList: true + canView: true