Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: silverstripe/silverstripe-graphql
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 4.3.6
Choose a base ref
...
head repository: silverstripe/silverstripe-graphql
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 4.3.7
Choose a head ref
  • 2 commits
  • 4 files changed
  • 3 contributors

Commits on Jan 22, 2024

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

    Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>
    emteknetnz and GuySartorelli committed Jan 22, 2024

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    Copy the full SHA
    8e64567 View commit details
  2. Merge pull request #568 from creative-commoners/pulls/4.3/cve-2023-44401

    
    
    [CVE-2023-44401] Ensure canView() checks are run
    sabina-talipova authored Jan 22, 2024
    Copy the full SHA
    2aabfb0 View commit details
27 changes: 16 additions & 11 deletions src/Schema/DataObject/Plugin/CanViewPermission.php
Original file line number Diff line number Diff line change
@@ -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 = [];

20 changes: 20 additions & 0 deletions tests/Fake/FakeDataObjectWithCanView.php
Original file line number Diff line number Diff line change
@@ -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;
}
}
40 changes: 40 additions & 0 deletions tests/Schema/IntegrationTest.php
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions tests/Schema/_testCanViewPagination/schema.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
models:
SilverStripe\GraphQL\Tests\Fake\FakeDataObjectWithCanView:
fields: '*'
operations:
read:
plugins:
paginateList: true
canView: true