CVE-2024-29885: Insufficient Information (NVD-CWE-noinfo)
value: silverstripe/reports is an API for creating backend reports in the Silverstripe Framework. In affected versions reports can be accessed by their direct URL by any user who has access to view the reports admin section, even if the `canView()` method for that report returns `false`. This issue has been addressed in version 5.2.3. All users are advised to upgrade. There are no known workarounds for this vulnerability.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-07-17T20:15Z
Repository: https://github.com/silverstripe/silverstripe-reports
Language: PHP
Files changed: code/ReportAdmin.php, tests/ReportAdminTest.php, tests/ReportAdminTest/CannotViewReport.php, tests/ReportAdminTest/FakeReport.php
Commit date: 2024-07-17 11:12:24 +1200
Commit message: [CVE-2024-29885] Respect canView permissions for viewing reports

Diff: diff --git a/code/ReportAdmin.php b/code/ReportAdmin.php
index 3522bd68..397037ed 100644
--- a/code/ReportAdmin.php
+++ b/code/ReportAdmin.php
@@ -122,10 +122,13 @@ class ReportAdmin extends LeftAndMain implements PermissionProvider
             $allReports = Report::get_reports();
             if (empty($allReports[$this->reportClass])) {
                 return $this->httpError(404);
             }
             $this->reportObject = $allReports[$this->reportClass];
+            if (!$this->reportObject->canView()) {
+                return Security::permissionFailure($this);
+            }
         }
 
         // Delegate to sub-form
         return parent::handleAction($request, $action);
     }
diff --git a/tests/ReportAdminTest.php b/tests/ReportAdminTest.php
index fd448bc0..36776c6d 100644
--- a/tests/ReportAdminTest.php
+++ b/tests/ReportAdminTest.php
@@ -2,18 +2,18 @@
 
 namespace SilverStripe\Reports\Tests;
 
 use ReflectionClass;
 use SilverStripe\Control\Controller;
-use SilverStripe\Control\HTTPRequest;
-use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Reports\Report;
 use SilverStripe\Reports\ReportAdmin;
+use SilverStripe\Reports\Tests\ReportAdminTest\CannotViewReport;
 use SilverStripe\Reports\Tests\ReportAdminTest\FakeReport;
 use SilverStripe\Reports\Tests\ReportAdminTest\FakeReport2;
 
-class ReportAdminTest extends SapphireTest
+class ReportAdminTest extends FunctionalTest
 {
     public function testBreadcrumbsAreGenerated()
     {
         $noExtraCrumbs = FakeReport::create();
 
@@ -44,10 +44,38 @@ class ReportAdminTest extends SapphireTest
 
         $map = $breadcrumbs[2]->toMap();
         $this->assertSame('Fake report two', $map['Title']);
     }
 
+    public function provideShowReport(): array
+    {
+        return [
+            'cannot view' => [
+                'reportClass' => CannotViewReport::class,
+                'expected' => 403,
+            ],
+            'can view' => [
+                'reportClass' => FakeReport::class,
+                'expected' => 200,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideShowReport
+     */
+    public function testShowReport(string $reportClass, int $expected): void
+    {
+        $this->logInWithPermission('ADMIN');
+        $report = new $reportClass();
+        $controller = $this->mockController($report);
+        $breadcrumbs = $controller->BreadCrumbs();
+        $response = $this->get($breadcrumbs[1]->Link);
+
+        $this->assertSame($expected, $response->getStatusCode());
+    }
+
     /**
      * @param Report $report
      * @return ReportAdmin
      * @throws \ReflectionException
      */
diff --git a/tests/ReportAdminTest/CannotViewReport.php b/tests/ReportAdminTest/CannotViewReport.php
new file mode 100644
index 00000000..344d5b7e
--- /dev/null
+++ b/tests/ReportAdminTest/CannotViewReport.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace SilverStripe\Reports\Tests\ReportAdminTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Reports\Report;
+
+class CannotViewReport extends Report implements TestOnly
+{
+    public function title()
+    {
+        return 'Cannot View report';
+    }
+
+    public function canView($member = null)
+    {
+        return false;
+    }
+}
diff --git a/tests/ReportAdminTest/FakeReport.php b/tests/ReportAdminTest/FakeReport.php
index d571404d..72242343 100644
--- a/tests/ReportAdminTest/FakeReport.php
+++ b/tests/ReportAdminTest/FakeReport.php
@@ -1,14 +1,23 @@
 <?php
 
 namespace SilverStripe\Reports\Tests\ReportAdminTest;
 
 use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\ArrayList;
 use SilverStripe\Reports\Report;
+use SilverStripe\Security\Member;
 
 class FakeReport extends Report implements TestOnly
 {
     public function title()
     {
         return 'Fake report';
     }
+
+    public function sourceRecords($params = [], $sort = null, $limit = null)
+    {
+        $list = new ArrayList();
+        $list->setDataClass(Member::class);
+        return $list;
+    }
 }

Vulnerable code: use SilverStripe\Control\HTTPRequest;
use SilverStripe\Dev\SapphireTest;
class ReportAdminTest extends SapphireTest
Fixed code:             if (!$this->reportObject->canView()) {
                return Security::permissionFailure($this);
            }
use SilverStripe\Dev\FunctionalTest;
use SilverStripe\Reports\Tests\ReportAdminTest\CannotViewReport;
class ReportAdminTest extends FunctionalTest
    public function provideShowReport(): array
    {
        return [
            'cannot view' => [
                'reportClass' => CannotViewReport::class,
                'expected' => 403,
            ],
            'can view' => [
                'reportClass' => FakeReport::class,
                'expected' => 200,
            ],
        ];
    }

    /**
     * @dataProvider provideShowReport
     */
    public function testShowReport(string $reportClass, int $expected): void
    {
        $this->logInWithPermission('ADMIN');
        $report = new $reportClass();
        $controller = $this->mockController($report);
        $breadcrumbs = $controller->BreadCrumbs();
        $response = $this->get($breadcrumbs[1]->Link);

        $this->assertSame($expected, $response->getStatusCode());
    }

<?php

namespace SilverStripe\Reports\Tests\ReportAdminTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\Reports\Report;

class CannotViewReport extends Report implements TestOnly
{
    public function title()
    {
        return 'Cannot View report';
    }

    public function canView($member = null)
    {
        return false;
    }
}
use SilverStripe\ORM\ArrayList;
use SilverStripe\Security\Member;

    public function sourceRecords($params = [], $sort = null, $limit = null)
    {
        $list = new ArrayList();
        $list->setDataClass(Member::class);
        return $list;
    }