fix(system): hydrate module fields when cloning a block (#73)

docs/changelog.270.txt

@@ -25,6 +25,9 @@ Tests:
 Forms and templates:
 - fix TinyMCE 7 language handling to preserve case-sensitive locale codes such as zh_TW
 
+System:
+- fix cloning a module block: the save handler now hydrates the module fields (mid, func_num, func_file, show_func, edit_func, template, dirname, name) from the clone form when bid=0, so the clone keeps its module binding and passes name validation; the path/function fields are validated (no traversal, identifier-only) before persisting (issue #73)
+
 ===================================
 2.7.0 RC5                    2026-04-22
 ===================================

docs/lang_diff.txt

@@ -10,6 +10,9 @@ Below are language differences from a version to next version.
 - added define('MISSING_REQUIRED_EXTENSIONS', 'Required PHP extensions are missing');
 - added define('MISSING_REQUIRED_EXTENSIONS_MSG', 'XOOPS cannot be installed because the following mandatory PHP extension(s) are not available: %s. Enable them in your PHP configuration (php.ini) and restart your web server, then reload this page.');
 
+/htdocs/modules/system/language/english/admin/blocksadmin.php
+- added define('_AM_SYSTEM_BLOCKS_INVALIDCLONE', 'Invalid block parameters.');
+
 /htdocs/modules/system/language/english/admin.php
 - added define('_AM_SYSTEM_MENUS', 'Menus');
 - added define('_AM_SYSTEM_MENUS_DESC', 'Manage site navigation menus');

htdocs/modules/system/admin/blocksadmin/main.php

@@ -287,6 +287,66 @@
             $block = $block_handler->get($block_id);
         } else {
             $block = $block_handler->create();
+            // Clone (bid=0): a fresh object has no module metadata. The
+            // source block's binding only survives in the hidden fields
+            // emitted by SystemBlock::getForm('clone'); read them back so
+            // the clone keeps its module association and passes the
+            // not-null name validation. Normal new custom blocks post these
+            // empty/0, which is harmless (isCustom() path ignores them).
+            $clone_dirname   = Request::getString('dirname', '', 'POST');
+            $clone_func_file = Request::getString('func_file', '', 'POST');
+            $clone_template  = Request::getString('template', '', 'POST');
+            $clone_show_func = Request::getString('show_func', '', 'POST');
+            $clone_edit_func = Request::getString('edit_func', '', 'POST');
+
+            // Reject tampered hidden inputs. dirname/func_file are later
+            // used to locate and include_once the module block PHP file,
+            // template is persisted as the block's Smarty template name,
+            // and show/edit_func are called as functions when the block
+            // renders or exposes options. Legitimate clone values come
+            // from a trusted DB row, so these checks never reject real
+            // clones but block path traversal / code injection via a
+            // forged POST.
+            //
+            // dirname is a single module directory segment (never contains
+            // a separator).
+            if ($clone_dirname !== ''
+                && (false !== strpos($clone_dirname, '/')
+                    || false !== strpos($clone_dirname, '\\')
+                    || false !== strpos($clone_dirname, '..')
+                    || false !== strpos($clone_dirname, "\0"))) {
+                redirect_header('admin.php?fct=blocksadmin', 3, _AM_SYSTEM_BLOCKS_INVALIDCLONE);
+            }
+            // func_file is the block PHP file under modules/<dirname>/blocks/
+            // (a subdirectory is allowed - some modules nest block files);
+            // template is a Smarty template name. Neither legitimately
+            // contains traversal, backslashes, NUL or an absolute path, so
+            // reject those while still permitting an internal '/'.
+            foreach ([$clone_func_file, $clone_template] as $clone_path) {
+                if ($clone_path !== ''
+                    && (false !== strpos($clone_path, '..')
+                        || false !== strpos($clone_path, '\\')
+                        || false !== strpos($clone_path, "\0")
+                        || str_starts_with($clone_path, '/'))) {
+                    redirect_header('admin.php?fct=blocksadmin', 3, _AM_SYSTEM_BLOCKS_INVALIDCLONE);
+                }
+            }
+            foreach ([$clone_show_func, $clone_edit_func] as $clone_func) {
+                if ($clone_func !== '' && !preg_match('/^[A-Za-z_]\w*$/', $clone_func)) {
+                    redirect_header('admin.php?fct=blocksadmin', 3, _AM_SYSTEM_BLOCKS_INVALIDCLONE);
+                }
+            }
+
+            $block->setVars([
+                'mid'       => Request::getInt('mid', 0, 'POST'),
+                'func_num'  => Request::getInt('func_num', 0, 'POST'),
+                'func_file' => $clone_func_file,
+                'show_func' => $clone_show_func,
+                'edit_func' => $clone_edit_func,
+                'template'  => $clone_template,
+                'dirname'   => $clone_dirname,
+                'name'      => Request::getString('name', '', 'POST'),
+            ]);
         }
         $block_type = Request::getString('block_type', '');
         $block->setVar('block_type', $block_type);

htdocs/modules/system/language/english/admin/blocksadmin.php

@@ -88,3 +88,4 @@
 define('_AM_SYSTEM_BLOCKS_FOOTER_LEFT', 'Footer Left');
 define('_AM_SYSTEM_BLOCKS_FOOTER_CENTER', 'Footer Center');
 define('_AM_SYSTEM_BLOCKS_FOOTER_RIGHT', 'Footer Right');
+define('_AM_SYSTEM_BLOCKS_INVALIDCLONE', 'Invalid block parameters.');

tests/unit/htdocs/modules/system/BlocksAdminCloneTest.php

@@ -0,0 +1,178 @@
+<?php
+/**
+ * Regression tests for modules/system/admin/blocksadmin/main.php — issue #73.
+ *
+ * Cloning a module block submits bid=0, so the save handler creates a
+ * fresh empty block. Before the fix it never read the module metadata
+ * back from the hidden clone-form fields, so the clone failed the
+ * not-null name validation and lost its module association.
+ *
+ * blocksadmin/main.php is a procedural request handler (redirects, CSRF,
+ * globals) and cannot be executed in isolation, so — like ModulesAdminTest
+ * — this verifies the fix by static analysis of the source.
+ *
+ * @copyright    2000-2026 XOOPS Project (https://xoops.org)
+ * @license      GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html)
+ * @package      Tests\Unit\System\BlocksAdmin
+ */
+
+declare(strict_types=1);
+
+namespace Tests\Unit\System\BlocksAdmin;
+
+use PHPUnit\Framework\TestCase;
+
+require_once __DIR__ . '/SourceFileTestTrait.php';
+
+use Tests\Unit\System\SourceFileTestTrait;
+
+class BlocksAdminCloneTest extends TestCase
+{
+    use SourceFileTestTrait;
+
+    /** Module-binding fields the clone form round-trips as hidden inputs. */
+    private const CLONE_FIELDS = [
+        'mid',
+        'func_num',
+        'func_file',
+        'show_func',
+        'edit_func',
+        'template',
+        'dirname',
+        'name',
+    ];
+
+    protected function setUp(): void
+    {
+        $this->loadSourceFile('htdocs/modules/system/admin/blocksadmin/main.php');
+    }
+
+    /**
+     * Byte offset of the save handler's clone branch. Anchored on the
+     * unique "get($block_id); } else { ... create();" shape so the inner
+     * c_type switch (case 'H'/default) can't throw off detection.
+     */
+    private function cloneBranchOffset(): int
+    {
+        $matched = preg_match(
+            '/\$block_handler->get\(\$block_id\);\s*\}\s*else\s*\{\s*\$block\s*=\s*\$block_handler->create\(\);/',
+            $this->sourceContent,
+            $m,
+            PREG_OFFSET_CAPTURE
+        );
+        self::assertSame(
+            1,
+            $matched,
+            "save handler must branch: bid>0 -> get(\$block_id), else create()"
+        );
+
+        return $m[0][1];
+    }
+
+    /**
+     * Capture the clone-branch hydration region: from the clone create()
+     * through the end of the $block->setVars([...]); call. This spans both
+     * the $clone_* = Request::get*() reads and the setVars() array, so it
+     * works whether a field is read inline or via a local. Window-free, so
+     * it cannot under/over-read like a fixed substring length would.
+     */
+    private function cloneHydrationRegion(): string
+    {
+        $matched = preg_match(
+            '/\$block_handler->get\(\$block_id\);\s*\}\s*else\s*\{\s*'
+            . '\$block\s*=\s*\$block_handler->create\(\);'
+            . '(.*?\$block->setVars\(\[.*?\]\);)/s',
+            $this->sourceContent,
+            $m
+        );
+        self::assertSame(
+            1,
+            $matched,
+            'clone branch must hydrate the new block via $block->setVars([...])'
+        );
+
+        return $m[1];
+    }
+
+    /**
+     * The clone branch (bid == 0 -> create()) must hydrate every
+     * module-binding field from POST — this is the #73 fix.
+     */
+    public function testCloneBranchHydratesModuleFieldsFromPost(): void
+    {
+        $region = $this->cloneHydrationRegion();
+
+        foreach (self::CLONE_FIELDS as $field) {
+            self::assertMatchesRegularExpression(
+                "/Request::get\w+\(\s*'" . preg_quote($field, '/') . "'.*'POST'\s*\)/",
+                $region,
+                "Clone branch must read '{$field}' from POST so the cloned block keeps its module binding (issue #73)"
+            );
+        }
+    }
+
+    /**
+     * Hardening for issue #73: the clone branch must reject tampered
+     * hidden inputs before persisting, since dirname/func_file/template
+     * feed include_once paths and show/edit_func are called as functions.
+     */
+    public function testCloneBranchRejectsTamperedPathAndFunctionFields(): void
+    {
+        $region = $this->cloneHydrationRegion();
+
+        // Path-traversal rejection on the path-building fields.
+        self::assertStringContainsString(
+            "'..'",
+            $region,
+            'Clone branch must reject ".." in dirname/func_file/template'
+        );
+        self::assertStringContainsString(
+            "redirect_header('admin.php?fct=blocksadmin'",
+            $region,
+            'A tampered clone must be rejected via redirect_header(), not persisted'
+        );
+        // Function-name allowlist for show_func / edit_func.
+        self::assertStringContainsString(
+            '^[A-Za-z_]\w*$',
+            $region,
+            'show_func/edit_func must be validated against a PHP-identifier allowlist'
+        );
+    }
+
+    /**
+     * Failure mode #1: a cloned block must carry a name before insert(),
+     * otherwise the not-null DB validation rejects it.
+     */
+    public function testCloneHydratesNameBeforeInsert(): void
+    {
+        $cloneOffset = $this->cloneBranchOffset();
+
+        $nameRead = strpos($this->sourceContent, "Request::getString('name'", $cloneOffset);
+        self::assertNotFalse($nameRead, "clone branch must read 'name' from POST");
+
+        $insertPos = strpos($this->sourceContent, '$block_handler->insert(', $cloneOffset);
+        self::assertNotFalse($insertPos, 'save handler must insert() the block');
+
+        // assertGreaterThan($expected, $actual) passes when $actual >
+        // $expected, i.e. insert() appears after the name read. Phrased
+        // this way (vs the equivalent assertLessThan) to read naturally.
+        self::assertGreaterThan(
+            $nameRead,
+            $insertPos,
+            "'name' must be hydrated before insert() so a clone passes name validation (issue #73)"
+        );
+    }
+
+    /**
+     * Regression guard: the normal edit path (bid > 0) must still load
+     * the block from the handler, unchanged by the fix.
+     */
+    public function testEditPathStillLoadsViaHandlerGet(): void
+    {
+        self::assertMatchesRegularExpression(
+            '/if \(\$block_id > 0\) \{\s*\$block\s*=\s*\$block_handler->get\(\$block_id\);/',
+            $this->sourceContent,
+            'Edit path (bid > 0) must still load the existing block via $block_handler->get()'
+        );
+    }
+}