From 8d5e54857a2d5000b98927dd2dc1fb1a6631c618 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Wed, 14 Oct 2026 16:08:41 -0400 Subject: [PATCH] fix(kernel): reload XOBJ_DTYPE_UNICODE_ARRAY values intact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects remained after #269 made the array_walk() encode and decode calls act on the array: XoopsObject::assignVar() url-decoded the stored value as a whole string before getVar() unserialized it. Any element containing a %-sequence (café, a+b, 100%) changed length, unserialize() warned and getVar() returned []. The serialized string is now stored as is; getVar() unserializes it and decodes the elements. XoopsModelWrite::cleanVars() still ran the addslashes() loop that predates $db->quote() before url-encoding the elements, so the backslashes became stored data (O'Reilly became O\'Reilly). The loop is removed; the whole value is SQL-quoted once. A new test stores an array through both write paths in a subprocess with the real functions.encoding.php, reloads it through assignVar() and asserts the original array. It fails on master and passes here. --- docs/changelog.270.txt | 1 + htdocs/class/model/write.php | 5 +- htdocs/kernel/object.php | 4 +- .../kernel/XoopsObjectUnicodeArrayTest.php | 119 ++++++++++++++++++ 4 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.php diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index 9aec456d6..c918e8215 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -8,6 +8,7 @@ Bug fixes: - LDAP and Active Directory logins on PHP 8.3+ no longer emit a deprecation: ldap_connect() receives one ldap:// URI built from the configured server and port, a bare IPv6 address is bracketed, a server already written with an ldap:// or ldaps:// scheme is used as is, and STARTTLS is not attempted on an ldaps:// connection (class/auth/auth_ldap.php, class/auth/auth_ads.php) - a cache engine without read() or write() throws LogicException instead of trigger_error(E_USER_ERROR), which PHP 8.4 deprecates (class/cache/xoopscache.php) - xoops_aw_encode() and xoops_aw_decode() take their element by reference, so the array_walk() calls that encode and decode XOBJ_DTYPE_UNICODE_ARRAY values act on the array; write.php and XoopsObject::setVar() serialize the walked array instead of array_walk()'s boolean, and XoopsObject::getVar() returns the decoded array instead of true (include/functions.encoding.php, class/model/write.php, kernel/object.php) +- a XOBJ_DTYPE_UNICODE_ARRAY value reloads as the array that was stored: assignVar() keeps the serialized string as is instead of url-decoding it, which broke the string lengths of elements such as café or a+b and made getVar() return [], and XoopsModelWrite::cleanVars() no longer addslashes() the elements before url-encoding them, which stored the backslashes (kernel/object.php, class/model/write.php, tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.php) - xoops_getUrlDomain() checks the host key only when parse_url() returned an array (include/functions.php) - templates can use |sprintf without a Smarty 4 deprecation notice: XoopsTpl registers sprintf as a modifier, as it already does for strstr, trim and file_exists (class/template.php) (#264) diff --git a/htdocs/class/model/write.php b/htdocs/class/model/write.php index 95cc18df1..de1a2623e 100644 --- a/htdocs/class/model/write.php +++ b/htdocs/class/model/write.php @@ -193,10 +193,7 @@ public function cleanVars($object) if (!$v['not_gpc']) { $cleanv = array_map([&$myts, 'stripSlashesGPC'], $cleanv); } - foreach (array_keys($cleanv) as $key) { - $cleanv[$key] = str_replace('\\"', '"', addslashes($cleanv[$key])); - } - // TODO: Not encoding safe, should try base64_encode -- phppp + // Elements are url-encoded, then the whole value is SQL-quoted below: no addslashes() pass. array_walk($cleanv, 'xoops_aw_encode'); $cleanv = $this->handler->db->quote(serialize($cleanv)); break; diff --git a/htdocs/kernel/object.php b/htdocs/kernel/object.php index 5e16b347b..33eb3d099 100644 --- a/htdocs/kernel/object.php +++ b/htdocs/kernel/object.php @@ -263,7 +263,9 @@ public function assignVar($key, $value) $value = $temp; $this->vars[$key]['value'] = $value; } else { - $this->vars[$key]['value'] = xoops_convert_decode($value); + // A stored value is a serialized array of encoded elements; getVar() unserializes + // and decodes it. Decoding the serialized string here would break its lengths. + $this->vars[$key]['value'] = $value; } break; case /** @scrutinizer ignore-deprecated */ XOBJ_DTYPE_UNICODE_URL: diff --git a/tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.php b/tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.php new file mode 100644 index 000000000..8c8efb462 --- /dev/null +++ b/tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.php @@ -0,0 +1,119 @@ +initVar('tags', XOBJ_DTYPE_UNICODE_ARRAY, []); + } +}; +\$object->setVar('tags', \$values); + +// XoopsObject::cleanVars(): what the legacy handlers store. +\$object->cleanVars(); +\$storedByObject = \$object->cleanVars['tags']; + +// XoopsModelWrite::cleanVars(): what XoopsPersistableObjectHandler stores, SQL-quoted. +\$db = new class () { + public function quote(\$v) + { + return "'" . addslashes((string) \$v) . "'"; + } +}; +\$write = new XoopsModelWrite(); +\$write->handler = (object) ['db' => \$db]; +\$write->cleanVars(\$object); +\$storedByModel = stripslashes(substr(\$object->cleanVars['tags'], 1, -1)); + +\$r = ['object' => \$storedByObject, 'model' => \$storedByModel, 'loaded' => []]; +foreach (['object' => \$storedByObject, 'model' => \$storedByModel] as \$path => \$column) { + \$loaded = clone \$object; + \$loaded->assignVar('tags', \$column); // what the handler does on a database read + \$r['loaded'][\$path] = @\$loaded->getVar('tags'); +} +echo json_encode(\$r); +PHP; + file_put_contents($directory . '/run.php', $script); + try { + $process = proc_open( + [PHP_BINARY, '-d', 'output_buffering=0', $directory . '/run.php'], + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes + ); + self::assertIsResource($process); + $output = stream_get_contents($pipes[1]); + $errors = stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + self::assertSame(0, proc_close($process), $errors . $output); + $r = json_decode((string) $output, true, 512, JSON_THROW_ON_ERROR); + } finally { + foreach (glob($directory . '/*') ?: [] as $file) { + unlink($file); + } + rmdir($directory); + } + + $values = ['café', 'a+b', "O'Reilly", 'back\\slash', '100%']; + self::assertSame($r['object'], $r['model'], 'both write paths store the same serialized value'); + self::assertSame($values, $r['loaded']['object'], 'XoopsObject::cleanVars() value reloads intact'); + self::assertSame($values, $r['loaded']['model'], 'XoopsModelWrite::cleanVars() value reloads intact'); + } +}