BaseObject and Component Magic Methods do not conform to PHP Visibility: protected setters accessible publicly

[belisoful]

What steps will reproduce the problem?

Make a subclass of Component with a private property and protected getter and protected setter.

$obj->getMyProperty();    // will fail
$obj->setMyProperty("new value");   //will fail

$value = $obj->MyProperty;   //is successful and does not fail
$obj->MyProperty = "new value";   //is successful and does not fail

The problem is that method_exists disregards the public/protected/private status of methods and only says if they exist. A further step must be done to determine if the method is public or not.

What is the expected result?

To conform to OOP principles where the property is inaccessible when the getter and setter methods are protected.

What do you get instead?

rather than failure, errors, or NOOP (calling protected methods is NOOP), it produces the MyProperty and sets MyProperty on protected methods by magic method.

Additional info

We found this bug in PRADO, the parent of Yii. I thought I'd be a good Human Being(tm) and report that this could be a serious bug.

Publicly accessing protected and private properties by magic methods could be a "CRITICAL" security issue for some people.

[belisoful]

The price of making all properties accessible by magic method to one's own class, is making them publicly available to all. Contrary, Limiting the public to only public getters/setters limits the class itself being able to read its own protected and private properties.

unless the calling object can be found. then when the calling object is $this, then allow the protected/private method call.

This restores OOP principles.

[belisoful]

pradosoft/prado#898 This is the issue over on the PRADO side. I have a new method_exists that will fix the issue all ways. public access only for external objects and protected/private access for ones own object. Proper access for each:

public static function method_visible($object_or_class, string $method): bool
{
	if (method_exists($object_or_class, $method)) {
		$trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT | DEBUG_BACKTRACE_IGNORE_ARGS, 3);
		$reflection = new \ReflectionMethod($object_or_class, $method);
		if (empty($trace[2]) || empty($trace[1]['object']) || empty($trace[2]['object']) || $trace[1]['object'] !== $trace[2]['object']) {
			return $reflection->isPublic();
		} elseif ($reflection->isPrivate()) {
			return $trace[2]['class'] === $reflection->class;
		} 
		return true;
	}
	return false;
}

Edit - "it was method_exists"

[belisoful]

pradosoft/prado@6296b7e

We fixed it.

[bizley]

Is there a noticeable impact on the performance on your side?

[belisoful]

it was about 10 ms slower on an 8.81s full run test. is that margin of error?

[bizley]

@yiisoft/reviewers @samdark please take a look.

[diego-mathis]

method_exists from php docu, does say it checks if the method exists. Imho it does what it says.

[belisoful]

@My6UoT9 right, and there should be checks on access for public vs protected vs private via magic method.

[diego-mathis]

@belisoful maybe it should be named differently then, method_accessible or something like that?

[belisoful]

All we had to do was change "method_exists" to "Prado::method_exists". It was assumed that there was some visibility validation when there was none. Our method does the visibility validation. simple switch the method to a static object call with the updated code.