fix(system): hydrate module fields when cloning a block (#73)#77
Merged
Conversation
Cloning a module block posts bid=0, so case 'save' creates a fresh
empty block. The handler 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 (empty mid/func_*/dirname →
renders nothing, no options panel).
In the bid==0 branch, hydrate mid, func_num, func_file, show_func,
edit_func, template, dirname and name from POST (the exact set
SystemBlock::getForm('clone') round-trips). Edits (bid>0) are
unchanged — get() still loads from the DB; custom-block clones are
unaffected (isCustom() path ignores these and posts them empty).
Adds BlocksAdminCloneTest (static source analysis, like ModulesAdminTest
— the handler is procedural and not executable in isolation) covering
both failure modes and guarding the edit path.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes module block cloning in the XOOPS Block Admin save flow: when ChangesModule Block Clone Fix
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #77 +/- ##
============================================
- Coverage 18.13% 18.12% -0.02%
Complexity 7854 7854
============================================
Files 666 666
Lines 43208 43240 +32
============================================
Hits 7837 7837
- Misses 35371 35403 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes system Blocks Admin cloning for module blocks (block_type=D) by ensuring that when a clone is submitted with bid=0, the save handler re-hydrates the module-binding fields from the clone form so the cloned block retains its module association and passes name validation (issue #73).
Changes:
- Hydrate module block metadata (mid/func/file/functions/template/dirname/name) from POST when
bid=0in the blocks save handler. - Add a static source-analysis regression test to ensure the clone branch hydrates the expected fields and the edit path remains unchanged.
- Document the fix in the 2.7.0 changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
htdocs/modules/system/admin/blocksadmin/main.php |
Hydrates module-binding fields in the bid=0 (clone/create) save branch via $block->setVars([...]). |
tests/unit/htdocs/modules/system/BlocksAdminCloneTest.php |
Adds a static regression test that asserts the clone branch reads all expected fields from POST and that edit path still uses get($block_id). |
docs/changelog.270.txt |
Notes the module-block clone hydration fix for issue #73. |
Comment on lines
+296
to
+305
| $block->setVars([ | ||
| 'mid' => Request::getInt('mid', 0, 'POST'), | ||
| 'func_num' => Request::getInt('func_num', 0, 'POST'), | ||
| 'func_file' => Request::getString('func_file', '', 'POST'), | ||
| 'show_func' => Request::getString('show_func', '', 'POST'), | ||
| 'edit_func' => Request::getString('edit_func', '', 'POST'), | ||
| 'template' => Request::getString('template', '', 'POST'), | ||
| 'dirname' => Request::getString('dirname', '', 'POST'), | ||
| 'name' => Request::getString('name', '', 'POST'), | ||
| ]); |
…review) PR XOOPS#77 review: the clone branch persists dirname/func_file/template (later concatenated into include_once paths) and show_func/edit_func (called as functions) from hidden POST inputs. Legitimate clone values are plain module identifiers copied from a trusted DB row, so add a strict reject-on-tamper guard that never rejects real clones but blocks path traversal / code injection via a forged POST: - dirname/func_file/template: reject if they contain / \ .. or NUL - show_func/edit_func: must match ^[A-Za-z_][A-Za-z0-9_]*$ (or empty) - on violation: redirect_header(), do not persist Not using Request::getCmd() (it lowercases/strips and would silently corrupt valid mixed-case identifiers — the same broken-clone class this issue fixes). Test extended to cover the guard; all assertions verified against source. Scoped to the bid==0 clone path; edits (bid>0) load from the trusted DB and are untouched.
Comment on lines
+314
to
+320
| redirect_header('admin.php?fct=blocksadmin', 3, 'Invalid block parameters.'); | ||
| } | ||
| } | ||
| foreach ([$clone_show_func, $clone_edit_func] as $clone_func) { | ||
| if ($clone_func !== '' && !preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $clone_func)) { | ||
| redirect_header('admin.php?fct=blocksadmin', 3, 'Invalid block parameters.'); | ||
| } |
| } | ||
|
|
||
| /** | ||
| * Hardening (PR #77 review): the clone branch must reject tampered |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Without the /u flag, PCRE \w is exactly [A-Za-z0-9_], so /^[A-Za-z_][A-Za-z0-9_]*$/ -> /^[A-Za-z_]\w*$/ is identical (leading class keeps no-digit-start). Coupled test assertion updated in lockstep; regex behaviour re-verified (b_News_show matches, ../evil rejected).
Comment on lines
+314
to
+319
| redirect_header('admin.php?fct=blocksadmin', 3, 'Invalid block parameters.'); | ||
| } | ||
| } | ||
| 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, 'Invalid block parameters.'); |
| } | ||
| 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, 'Invalid block parameters.'); |
PR XOOPS#77 review (both valid): - replace the hard-coded 'Invalid block parameters.' redirect with a new _AM_SYSTEM_BLOCKS_INVALIDCLONE language constant, matching this handler's i18n convention; constant added to the english admin blocksadmin language file and recorded in docs/lang_diff.txt. - reword the regression-test docblock to reference issue XOOPS#73 instead of a transient PR number (clearer in blame/history). Stale: the original "sanitize these fields" comment (commit 368bc926) is already addressed by the a1c260a hardening. CodeRabbit approved.
Comment on lines
+302
to
+316
| // Reject tampered hidden inputs. dirname/func_file/template are | ||
| // later concatenated into include_once paths and show/edit_func | ||
| // are called as functions when the block renders. Legitimate | ||
| // clone values are plain module identifiers copied from a | ||
| // trusted DB row, so a strict check never rejects real clones | ||
| // but blocks path traversal / code injection via forged POST. | ||
| foreach ([$clone_dirname, $clone_func_file, $clone_template] as $clone_path) { | ||
| if ($clone_path !== '' | ||
| && (false !== strpos($clone_path, '/') | ||
| || false !== strpos($clone_path, '\\') | ||
| || false !== strpos($clone_path, '..') | ||
| || false !== strpos($clone_path, "\0"))) { | ||
| redirect_header('admin.php?fct=blocksadmin', 3, _AM_SYSTEM_BLOCKS_INVALIDCLONE); | ||
| } | ||
| } |
Copilot review (verified valid): core builds module-block paths by plain concatenation modules/<dirname>/blocks/<func_file> with no enforcement that func_file is flat, so a module may legitimately nest a block file (sub/foo.php). The previous guard rejected any '/' in func_file/template, which would break cloning such blocks — a real BC regression for the ~200-module ecosystem. Relax: dirname stays single-segment-strict (no / \ .. NUL); func_file and template now allow an internal '/' but still reject '..', backslash, NUL and absolute paths. Equally secure against the actual threat (a forged value cannot escape modules/<dirname>/blocks/, and a system admin can already run PHP via custom blocks); no BC regression. Behaviour verified: system_blocks.php and sub/foo.php accepted; ../../x, /abs, a\b rejected.
Comment on lines
+157
to
+158
| $insertPos, | ||
| $nameRead, |
Comment on lines
+302
to
+305
| // Reject tampered hidden inputs. dirname/func_file/template are | ||
| // later concatenated into include_once paths and show/edit_func | ||
| // are called as functions when the block renders. Legitimate | ||
| // clone values come from a trusted DB row, so these checks never |
PR XOOPS#77 review: - Copilot (valid): the guard comment claimed `template` is concatenated into an include_once path. Only dirname/func_file locate the block PHP file; `template` is persisted as the block's Smarty template name. Reworded both comments accordingly. Validation is unchanged (defense-in-depth still applies to a Smarty template name). - Copilot (incorrect): claimed the assertLessThan args were reversed. PHPUnit assertLessThan($expected,$actual) passes when $actual<$expected, so assertLessThan($insertPos,$nameRead) already enforced nameRead<insertPos. Not inverting it (that would break the test); rewrote as the equivalent assertGreaterThan($nameRead,$insertPos) so it reads naturally and stops recurring review confusion.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #73.
Problem
Cloning a module block (
block_type=D) submitsbid=0, socase 'save'inmodules/system/admin/blocksadmin/main.phpbuilds a fresh empty block viacreate(). The handler never read the module metadata back from the hidden clone-form fields, so:getVar('name')was''→ "name is required" DB validation error.mid/func_file/show_func/edit_func/template/dirnamesaved empty → clone renders nothing on the front end and shows no options panel in admin.Edits (
bid>0) were unaffected becauseget()loads everything from the DB.Fix
In the
bid==0branch, hydrate the module fields from POST — exactly the setSystemBlock::getForm('clone')round-trips (mid, func_num, func_file, show_func, edit_func, template, dirname, name). Verified againstblock.php::getForm()(hidden fields, lines 188–196).Scope / regression safety: change is fully contained in the clone branch. Normal edits still use
get(). Custom-block clones are unaffected —isCustom()derivesnamefromc_typeand ignores these (posted empty).Trust model unchanged: these fields are admin-only + CSRF-gated and already round-trip through the clone form; this restores intended behavior, introduces no new exposure. A stronger design (carry a
source_bid, re-fetch + validatedirname/funcserver-side) is a larger, separate hardening — not required to fix #73; can file as a follow-up if wanted.Test
tests/unit/htdocs/modules/system/BlocksAdminCloneTest.php— static source analysis, consistent with the codebase's existingModulesAdminTest/SourceFileTestTraitpattern (this handler is procedural — redirects/CSRF/globals — and not executable in isolation). Covers: clone branch reads all 8 fields from POST;nameis hydrated beforeinsert()(failure mode #1); the edit path still usesget().Honest coverage note: this is source-level verification, not an executed end-to-end handler test (same approach/limitation as
ModulesAdminTest). It is a genuine regression guard — thesetVars([...])capture does not match the pre-fix code, so it fails without the fix. I validated every assertion deterministically against the patched source locally; I could not run the actual PHPUnit suite in this environment.Test plan
BlocksAdminCloneTest)Summary by CodeRabbit
Bug Fixes
Tests
Documentation