Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Don't try to access private properties/methods #10670

Merged
merged 1 commit into from
Jan 31, 2023
Merged

FIX Don't try to access private properties/methods #10670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 40 additions & 12 deletions src/View/ViewableData.php
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may (probably am) misunderstanding this, though won't self::class always be ViewableData::class?

Is this what we're trying to do here?

if (!$reflectionMethod->isPrivate()) {
  return true;
} else {
  // method is private
  if (<method is defined on the class that called isAccessibleMethod(), or a trait that's been applied to that class>) {
    return true;
  } else {
    return false;
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't self::class always be ViewableData::class?

Yes. So we're checking if the class of the object is ViewableData (and not some subclass).

What we're doing is:

  1. Returning false if the method just simply doesn't exist. It's not accessible if it doesn't exist.
  2. Returning true if the method exists and the static class is ViewableData - it can always access its own methods.
  3. Returning false if the method exists, but the static class is not ViewableData, and the method is private. ViewableData can't access methods which are private and defined in subclasses.
  4. Returning true if the method exists, the static class is not ViewableData, but the method is not private. ViewableData can access protected and public methods of its subclasses.

The unit tests cover all of these bases, I believe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my bad, forgot that isAccessibleMethod() is a private method itself so doesn't need to work on subclasses

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
View file
Open in desktop
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 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');
}
}
11 changes: 11 additions & 0 deletions tests/php/View/ViewableDataTestObject.php
View file
Open in desktop
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