Fedik
commented
1 year ago The fix is there https://github.com/joomla/joomla-cms/pull/41342 please test
Open tampe125 opened 1 year ago
Fedik
commented
1 year ago The fix is there https://github.com/joomla/joomla-cms/pull/41342 please test
Fedik
commented
1 year ago Hm, the fix is for Form::bindLevel
\Joomla\CMS\Serializer\JoomlaSerializer::getAttributes still need to be fixed. But I think it can be very similar.
Just loop the data in foreach, and collect public properties.
Fedik
commented
1 year ago Okay, this should work for JoomlaSerializer::getAttributes #41511, please test
tampe125
commented
1 year ago mhm... I think this is not going to truly fix the problem.
The data is no longer an array when it reaches the iteration which expects an array.
Your solution might work because PHP can iterate through objects; however, I would prefer it if everything went explicitly through getProperties because we might have objects with private/protected variables and magic getters/setters.
The way this was handled before was extending them from CMSObject and overriding getProperties.
It seems that what you are doing now is a b/c break you have not documented anywhere, not even in the PR which implemented this change.
Fedik
commented
1 year ago The data is no longer an array when it reaches the iteration which expects an array.
The PHP will handle it.
because we might have objects with private/protected variables
If someone want to expose protected properties, then can implement Iterator on his object.
We cannot blindly use getProperties on any object, we should have an interface for it. But that another question.
I don't mind if someone suggest a better fix.
wilsonge
commented
1 year ago I’m on holiday and back on the 8th. If you remind me then ill try and take a look. In the meantime
@HLeithner as release manager
HLeithner
commented
1 year ago I'm not sure about this, removing CMSObject from the Table make sense to me. Blindly detecting a getProperties method is not optimal. We have the Registry::toArray() for this for special conversation.
On the other hand I think it also make sense to filter all private (including pseudo private _ prefixed) and protected variables from the converted array.... but not sure what impact we have in this case...
What needs to be fixed
Whilst working towards Joomla 5 compatibility of our extensions we found out that the removal of
\Joomla\CMS\Object\CMSObjectfrom the inheritance path of\Joomla\CMS\Table\Tablehas unintended consequences in at least two places:\Joomla\CMS\Form\Form::bindLevelcan experience an infinite recursion if the Table object has a private or protected property which references itself, or the table object. We observed it at least once with the database object (_db) having a non-nullsqlproperty which, of course, has a reference back to the database object, thus causing an infinite loop. Please see below for an explanation.\Joomla\CMS\Serializer\JoomlaSerializer::getAttributesreturns all properties (public, protected, and private). By itself not a problem, since they are by default filtered by$fieldsToRenderList/$fieldsToRenderItem, but could be an issue on API views which assume that Joomla returns only the public properties of an object. This change in behaviour can lead to security issues, i.e. the disclosure of privileged information such as the database access information of the site.The debug backtrace of our observation of the first problem is as follows:
The infinite recursion happens at line 182 of the Form class. It is caused by line 164.
$datais a Table object. However, it is no longer a CMSObject. Therefore, Joomla! skips over the if-block on lines 164-166 and executes line 169. Casting an object to an array includes private and protected properties, with their names prefixed by a NULL byte (\x00) plus some other stuff, depending on whether they are private or protected. Using thegetPropertiesmethod, however, only returns public properties i.e. it excludes things like_db.While this is a pretty rare occurrence, debugging this is hard. It requires having prior knowledge of Table no longer extending CMSObject while it previously did. The current code base offers no clues to that. We only discovered the issue because our lead developer had a conversation about CMSObject with your maintainers several months ago.
Why this should be fixed
Looking at #40999 we can plainly see that the observed behaviour is unintended, i.e. it is a bug.
Moreover, iterating through private and protected properties while binding data to a form is at best a waste of time, at worst the source of a hard to troubleshoot bug.
Finally, returning protected and private properties in the serializer is unexpected and can lead to serious vulnerabilities (information disclosure, including database connection information).
How would you fix it
Change
\Joomla\CMS\Form\Formline 164 fromto
Temporary mitigation in 3PD code: In the model's
loadFormDatawe replaced lines previously looking like this:with
Change
\Joomla\CMS\Serializer\JoomlaSerializerline 71 fromto
Temporary mitigation in 3PD code: We strip the returned properties of any protected and private members with a simple
array_filter:Our mitigation is based on the fact that PHP object property names cannot start with a null byte, and the observation that PHP prefixes the keys corresponding to protected and private properties with a null byte when you cast an object to an array, see https://3v4l.org/SNTvf
Side Effects expected
None. Instead, it removes the unintended side effects added to Joomla! accidentally.
@wilsonge @laoneo