From 0351106c18ad4246d983b5f4e082c09c382121f4 Mon Sep 17 00:00:00 2001
From: Guy Sartorelli <guy.sartorelli@silverstripe.com>
Date: Wed, 27 Mar 2024 14:06:39 +1300
Subject: [PATCH] [CVE-2024-29885] Respect canView permissions for viewing
 reports

---
 code/ReportAdmin.php                       |  3 ++
 tests/ReportAdminTest.php                  | 34 ++++++++++++++++++++--
 tests/ReportAdminTest/CannotViewReport.php | 19 ++++++++++++
 tests/ReportAdminTest/FakeReport.php       |  9 ++++++
 4 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 tests/ReportAdminTest/CannotViewReport.php

diff --git a/code/ReportAdmin.php b/code/ReportAdmin.php
index 3522bd68..397037ed 100644
--- a/code/ReportAdmin.php
+++ b/code/ReportAdmin.php
@@ -124,6 +124,9 @@ class ReportAdmin extends LeftAndMain implements PermissionProvider
                 return $this->httpError(404);
             }
             $this->reportObject = $allReports[$this->reportClass];
+            if (!$this->reportObject->canView()) {
+                return Security::permissionFailure($this);
+            }
         }
 
         // Delegate to sub-form
diff --git a/tests/ReportAdminTest.php b/tests/ReportAdminTest.php
index fd448bc0..36776c6d 100644
--- a/tests/ReportAdminTest.php
+++ b/tests/ReportAdminTest.php
@@ -4,14 +4,14 @@ 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()
     {
@@ -46,6 +46,34 @@ class ReportAdminTest extends SapphireTest
         $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
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
@@ -3,7 +3,9 @@
 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
 {
@@ -11,4 +13,11 @@ class FakeReport extends Report implements TestOnly
     {
         return 'Fake report';
     }
+
+    public function sourceRecords($params = [], $sort = null, $limit = null)
+    {
+        $list = new ArrayList();
+        $list->setDataClass(Member::class);
+        return $list;
+    }
 }