Merge pull request #10670 from creative-commoners/pulls/5/no-private-viewabledata

FIX Don't try to access private properties/methods

src/View/ViewableData.php

@@ -7,7 +7,9 @@ use Exception;
 use InvalidArgumentException;
 use IteratorAggregate;
 use LogicException;
+use ReflectionMethod;
 use ReflectionObject;
+use ReflectionProperty;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Config\Configurable;
@@ -111,7 +113,7 @@ class ViewableData implements IteratorAggregate
     public function __isset($property)
     {
         // getField() isn't a field-specific getter and shouldn't be treated as such
-        if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method = "get$property")) {
+        if (strtolower($property ?? '') !== 'field' && $this->hasMethod("get$property")) {
             return true;
         }
         if ($this->hasField($property)) {
@@ -134,7 +136,8 @@ class ViewableData implements IteratorAggregate
     public function __get($property)
     {
         // getField() isn't a field-specific getter and shouldn't be treated as such
-        if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method = "get$property")) {
+        $method = "get$property";
+        if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method) && $this->isAccessibleMethod($method)) {
             return $this->$method();
         }
         if ($this->hasField($property)) {
@@ -159,20 +162,13 @@ class ViewableData implements IteratorAggregate
         $this->objCacheClear();
         $method = "set$property";
 
-        if ($this->hasMethod($method) && !$this->isPrivate($this, $method)) {
+        if ($this->hasMethod($method) && $this->isAccessibleMethod($method)) {
             $this->$method($value);
         } else {
             $this->setField($property, $value);
         }
     }
 
-    private function isPrivate(object $class, string $method): bool
-    {
-        $class = new ReflectionObject($class);
-        
-        return $class->getMethod($method)->isPrivate();
-    }
-
     /**
      * Set a failover object to attempt to get data from if it is not present on this object.
      *
@@ -218,7 +214,7 @@ class ViewableData implements IteratorAggregate
      */
     public function getField($field)
     {
-        if (property_exists($this, $field)) {
+        if ($this->isAccessibleProperty($field)) {
             return $this->$field;
         }
         return $this->data[$field];
@@ -237,13 +233,45 @@ class ViewableData implements IteratorAggregate
         // prior to PHP 8.2 support ViewableData::setField() simply used `$this->field = $value;`
         // so the following logic essentially mimics this behaviour, though without the use
         // of now deprecated dynamic properties
-        if (property_exists($this, $field)) {
+        if ($this->isAccessibleProperty($field)) {
             $this->$field = $value;
         }
         $this->data[$field] = $value;
         return $this;
     }
 
+    /**
+     * Returns true if a method exists for the current class which isn't private.
+     * Also returns true for private methods if $this is ViewableData (not a subclass)
+     */
+    private function isAccessibleMethod(string $method): bool
+    {
+        if (!method_exists($this, $method)) {
+            return false;
+        }
+        if (static::class === self::class) {
+            return true;
+        }
+        $reflectionMethod = new ReflectionMethod($this, $method);
+        return !$reflectionMethod->isPrivate();
+    }
+
+    /**
+     * Returns true if a property exists for the current class which isn't private.
+     * Also returns true for private properties if $this is ViewableData (not a subclass)
+     */
+    private function isAccessibleProperty(string $property): bool
+    {
+        if (!property_exists($this, $property)) {
+            return false;
+        }
+        if (static::class === self::class) {
+            return true;
+        }
+        $reflectionProperty = new ReflectionProperty($this, $property);
+        return !$reflectionProperty->isPrivate();
+    }
+
     // -----------------------------------------------------------------------------------------------------------------
 
     /**

tests/php/View/ViewableDataTest.php

@@ -208,16 +208,47 @@ class ViewableDataTest extends SapphireTest
         $this->assertFalse($container->hasMethod('testMethod'), 'testMethod() incorrectly reported as existing');
     }
 
-    public function testIsPrivate()
+    public function testIsAccessibleMethod()
     {
-        $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isPrivate');
+        $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isAccessibleMethod');
         $reflectionMethod->setAccessible(true);
         $object = new ViewableDataTestObject();
-        
-        $output = $reflectionMethod->invokeArgs($object, [$object, 'privateMethod']);
-        $this->assertTrue($output, 'Method is not private');
-        
-        $output = $reflectionMethod->invokeArgs($object, [$object, 'publicMethod']);
-        $this->assertFalse($output, 'Method is private');
+
+        $output = $reflectionMethod->invokeArgs($object, ['privateMethod']);
+        $this->assertFalse($output, 'Method should not be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['protectedMethod']);
+        $this->assertTrue($output, 'Method should be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['publicMethod']);
+        $this->assertTrue($output, 'Method should be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['missingMethod']);
+        $this->assertFalse($output, 'Method should not be accessible');
+
+        $output = $reflectionMethod->invokeArgs(new ViewableData(), ['isAccessibleProperty']);
+        $this->assertTrue($output, 'Method should be accessible');
+    }
+
+    public function testIsAccessibleProperty()
+    {
+        $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isAccessibleProperty');
+        $reflectionMethod->setAccessible(true);
+        $object = new ViewableDataTestObject();
+
+        $output = $reflectionMethod->invokeArgs($object, ['privateProperty']);
+        $this->assertFalse($output, 'Property should not be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['protectedProperty']);
+        $this->assertTrue($output, 'Property should be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['publicProperty']);
+        $this->assertTrue($output, 'Property should be accessible');
+
+        $output = $reflectionMethod->invokeArgs($object, ['missingProperty']);
+        $this->assertFalse($output, 'Property should not be accessible');
+
+        $output = $reflectionMethod->invokeArgs(new ViewableData(), ['objCache']);
+        $this->assertTrue($output, 'Property should be accessible');
     }
 }

tests/php/View/ViewableDataTestObject.php

@@ -7,11 +7,22 @@ use SilverStripe\ORM\DataObject;
 
 class ViewableDataTestObject extends DataObject implements TestOnly
 {
+    private string $privateProperty = 'private property';
+
+    protected string $protectedProperty = 'protected property';
+
+    public string $publicProperty = 'public property';
+
     private function privateMethod(): string
     {
         return 'Private function';
     }
 
+    protected function protectedMethod(): string
+    {
+        return 'Protected function';
+    }
+
     public function publicMethod(): string
     {
         return 'Public function';