Skip to content

Commit

Permalink
FIX Don't try to access private properties/methods
Browse files Browse the repository at this point in the history
  • Loading branch information
GuySartorelli committed Jan 31, 2023
1 parent 0f40146 commit 934cd75
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 20 deletions.
52 changes: 40 additions & 12 deletions src/View/ViewableData.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
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;
Expand Down Expand Up @@ -111,7 +113,7 @@ public function __construct()
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)) {
Expand All @@ -134,7 +136,8 @@ public function __isset($property)
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)) {
Expand All @@ -159,20 +162,13 @@ public function __set($property, $value)
$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.
*
Expand Down Expand Up @@ -218,7 +214,7 @@ public function hasField($field)
*/
public function getField($field)
{
if (property_exists($this, $field)) {
if ($this->isAccessibleProperty($field)) {
return $this->$field;
}
return $this->data[$field];
Expand All @@ -237,13 +233,45 @@ public function setField($field, $value)
// 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();
}

// -----------------------------------------------------------------------------------------------------------------

/**
Expand Down
47 changes: 39 additions & 8 deletions tests/php/View/ViewableDataTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -208,16 +208,47 @@ public function testSetFailover()
$this->assertFalse($container->hasMethod('testMethod'), 'testMethod() incorrectly reported as existing');
}

public function testIsPrivate()
public function testIsPrivateMethod()
{
$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 testIsPrivateProperty()
{
$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');
}
}
11 changes: 11 additions & 0 deletions tests/php/View/ViewableDataTestObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,22 @@

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';
Expand Down

0 comments on commit 934cd75

Please sign in to comment.