Merge pull request #189 from creative-commoners/pulls/5.2/cve-2024-29885

[CVE-2024-29885] Respect canView permissions for viewing reports

code/ReportAdmin.php

@@ -124,6 +124,9 @@ public function handleAction($request, $action)
                 return $this->httpError(404);
             }
             $this->reportObject = $allReports[$this->reportClass];
+            if (!$this->reportObject->canView()) {
+                return Security::permissionFailure($this);
+            }
         }
 
         // Delegate to sub-form

tests/ReportAdminTest.php

@@ -4,14 +4,14 @@
 
 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()
     {
@@ -46,6 +46,34 @@ public function testBreadcrumbsAreGenerated()
         $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

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;
+    }
+}

tests/ReportAdminTest/FakeReport.php

@@ -3,12 +3,21 @@
 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;
+    }
 }