Could we find a better name for the unserialise function?
I almost had a heart-attck looking at
one of the recent commits, featuring this diff:
diff --git a/Zotlabs/Lib/Config.php b/Zotlabs/Lib/Config.php
index cd8b08991..139affa09 100644
--- a/Zotlabs/Lib/Config.php
+++ b/Zotlabs/Lib/Config.php
@@ -132,8 +132,8 @@ class Config {
$value = App::$config[$family][$key];
if (! is_array($value)) {
- if (substr($value, 0, 5) == 'json:') {
- return json_decode(substr($value, 5), true);
+ if (str_starts_with($value, 'json:')) {
+ return unserialise($value);
} else if (preg_match('|^a:[0-9]+:{.*}$|s', $value)) {
// Unserialize in inherently unsafe. Try to mitigate by not
// allowing unserializing objects. Only kept for backwards
This seems like it reverts from the safe function
json_decode() to the unsafe, and
potentially dangerous unserialize() function. Something that would also break deserializing json payloads, which seems to contradict the purpose of the patch. (We luckily have tests that would have caught this, but nevertheless, the vulnerability analyst in me got a jump!)
The keen eye will discover that it's not changing to use
unserialize(), but
unserialise(). That's a
one letter difference! While easy to misread, it's also easy to mistype, which very likely would introduce exploitable vulnerabilities.
I would suggest that if we are to keep the
unserialise() function, we should at least give it a better name.
safe_unserialize_json() perhaps?
In this particular case, I don't really see the point of using it over just
json_decode() anyways. The only thing it gives us is that it checks that the value to unserialize is not an array, and whether it starts with the text "json:". We have already checked both of these conditions before calling the function here.
But at least let's give it a less confusing name!
