fix(security): BLOCK-38 — gate template writes on a dedicated cap, not edit_posts - #65
Conversation
check_template_edit_permissions() currently passes on toggle-on AND
(edit_posts OR edit_theme_options). Because the plugin performs the
wp_insert_post()/wp_update_post() on wp_template/wp_template_part itself
regardless of the caller's own caps, with the toggle on ANY
contributor-or-above account (or a leaked low-privilege Application
Password) can rewrite sitewide template chrome — header, footer, 404,
archive, search.
Primary red pin: test_update_template_route_403_for_contributor_even_with_toggle_on
in TemplatesRestTest.php. Confirmed genuinely red against current shipped
code via `git stash` of the (not-yet-written) implementation:
1) TemplatesRestTest::test_update_template_route_403_for_contributor_even_with_toggle_on
Failed asserting that 200 is identical to 403.
Companion pins added alongside it:
- TemplatesRestTest.php: an editor (same edit_posts-only shape as a
contributor) is equally denied; the dedicated agent role, after
register_role() runs with the toggle on, holds the new
Agent_Provisioner::TEMPLATE_EDIT_CAP and POST /template succeeds;
toggling back off and re-running register_role() revokes the cap
(asserted at the capability level on the role, not just behaviorally)
and the route 403s again. The existing edit_theme_options-alone success
test and the reset-template round-trip test are updated to use an
edit_theme_options actor instead of editor/contributor, since those
roles no longer have template-write permission — this is intentional,
not incidental breakage.
- TemplateAbilitiesTest.php: the same editor-denied case is pinned via
the Abilities path (gk-block-mcp/update-template), proving the shared
check_template_edit_permissions() callback can't be bypassed there.
The persist/round-trip tests switch their actor to edit_theme_options
for the same reason as above.
- AgentProvisionerTest.php: register_role() grants TEMPLATE_EDIT_CAP on
a fresh role when the toggle is on, and revokes it from an existing
role when the toggle is later switched off — the one exception to the
existing "additive only" re-assert loop, proven not to weaken
test_register_role_strips_forbidden_caps_from_existing_role() (still
green, unmodified).
Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
…p [green] Fixes BLOCK-38, a security-review finding that blocks the develop→main 2.2.0 release (#64). Fable's design: a plugin-owned primitive cap managed by the existing toggle, not a raw grant of edit_theme_options. - Agent_Provisioner::TEMPLATE_EDIT_CAP ('gk_block_mcp_edit_templates') is a new constant. Deliberately NOT added to forbidden_capabilities() — that denylist strips caps this class never grants; this one it grants and revokes on purpose. - register_role() computes the cap's value from Template_Manager::edits_enabled() before the existing gk/block-mcp/agent/caps filter runs, so operators can still override it. On an existing role, it's the one cap both ADDED (already covered by the additive re-assert loop) and — new — REMOVED when the toggle is off, scoped to only this single cap; the loop otherwise stays additive- only, and the forbidden-cap strip loop is untouched. Self-heals on every `init` (priority 99); a new `update_option_gk_block_api_template_edits` hook re-asserts immediately on settings save so grant/revoke doesn't wait for the next request. - check_template_edit_permissions() now passes on current_user_can('gk_block_mcp_edit_templates') OR current_user_can('edit_theme_options'), replacing the edit_posts clause. The toggle check still runs first (403 template_edits_disabled before any capability check). Docblock rewritten to document the actual vulnerability this closes, not the old (incorrect) edit_posts rationale. - Consent copy (class-settings-page.php) reworded to disclose the mechanism: granting the toggle changes what the Block MCP agent account itself can do to the theme layer, not just "let the assistant edit templates." Never grants raw edit_theme_options to the agent: that cap also opens core's /wp/v2/templates, /wp/v2/template-parts, /wp/v2/navigation, /wp/v2/global-styles, the Customizer, menus, and widgets, none of which the toggle is meant to reach — and it would be stripped by the forbidden-capabilities re-assert loop anyway. Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
WalkthroughThe plugin adds a dedicated template-edit capability to the agent role, synchronizes it with the template-editing option, and requires it for template write routes. Tests cover capability changes, authorized agent requests, denied editor/contributor requests, and template reset behavior. ChangesTemplate edit capability gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant Template_Manager
participant Agent_Provisioner
participant REST_Controller
participant AgentRole
Admin->>Template_Manager: save template-editing toggle
Template_Manager->>Agent_Provisioner: update option action
Agent_Provisioner->>AgentRole: register_role()
Agent_Provisioner->>AgentRole: grant or revoke TEMPLATE_EDIT_CAP
AgentRole->>REST_Controller: POST /template
REST_Controller->>AgentRole: check TEMPLATE_EDIT_CAP
REST_Controller-->>AgentRole: allow or return 403
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wordpress-plugin/gk-block-mcp/gk-block-mcp.php`:
- Around line 338-342: The option lifecycle currently reasserts the agent role
only through the update hook, so deletion during settings reset leaves stale
TEMPLATE_EDIT_CAP until init. In the hook registration near
Agent_Provisioner::register_role, add a post-delete handler scoped to
ALLOW_TEMPLATE_EDITS_OPTION that invokes the same role reassertion callback, and
add a regression test using delete_option() or the reset flow to verify the
capability is revoked immediately.
In `@wordpress-plugin/gk-block-mcp/includes/class-agent-provisioner.php`:
- Around line 178-184: In the capability reconciliation logic around
TEMPLATE_EDIT_CAP, safely handle the public gk/block-mcp/agent/caps filter
removing that key by capturing the filtered capability value with an appropriate
default and the existing capability state in named variables before the
condition. Update the if statement to use those variables, preserving the
explicit remove_cap behavior and complying with the compound-check naming
guideline.
In `@wordpress-plugin/gk-block-mcp/tests/Abilities/TemplateAbilitiesTest.php`:
- Around line 248-269: The Abilities tests lack coverage for the dedicated
template-edit capability allow path. In TemplateAbilitiesTest, add a test for
gk-block-mcp/update-template that enables the template-edit toggle, invokes
Agent_Provisioner::register_role(), assigns the provisioned role to the current
user, executes the ability with valid template data, and asserts successful
execution rather than a WP_Error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6cd63ba-6a9d-4ed9-9b99-ff61d8ce3808
📒 Files selected for processing (7)
wordpress-plugin/gk-block-mcp/gk-block-mcp.phpwordpress-plugin/gk-block-mcp/includes/class-agent-provisioner.phpwordpress-plugin/gk-block-mcp/includes/class-rest-controller.phpwordpress-plugin/gk-block-mcp/includes/class-settings-page.phpwordpress-plugin/gk-block-mcp/tests/Abilities/TemplateAbilitiesTest.phpwordpress-plugin/gk-block-mcp/tests/Connect/AgentProvisionerTest.phpwordpress-plugin/gk-block-mcp/tests/Templates/TemplatesRestTest.php
| // register_role() derives Agent_Provisioner::TEMPLATE_EDIT_CAP from this | ||
| // toggle; re-assert on save so grant/revoke is immediate rather than | ||
| // waiting for the next `init`. register_role() takes no required args, | ||
| // so WordPress's extra ($old_value) argument here is simply unused. | ||
| add_action( 'update_option_' . \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, array( __NAMESPACE__ . '\\Agent_Provisioner', 'register_role' ) ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reassert the role after toggle deletion too.
Settings reset deletes gk_block_api_template_edits, so this update-only hook does not run. The agent role can retain TEMPLATE_EDIT_CAP until the next request’s init, despite the effective toggle being off. Add a post-delete deleted_option handler scoped to this option, and cover delete_option()/reset with a regression test.
Proposed fix
add_action( 'update_option_' . \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, array( __NAMESPACE__ . '\\Agent_Provisioner', 'register_role' ) );
+add_action(
+ 'deleted_option',
+ static function ( $option ) {
+ $is_template_edits_option = \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION === $option;
+ if ( ! $is_template_edits_option ) {
+ return;
+ }
+ Agent_Provisioner::register_role();
+ },
+ 10,
+ 1
+);As per coding guidelines, every bug fix requires a regression test exercising the real mechanism.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // register_role() derives Agent_Provisioner::TEMPLATE_EDIT_CAP from this | |
| // toggle; re-assert on save so grant/revoke is immediate rather than | |
| // waiting for the next `init`. register_role() takes no required args, | |
| // so WordPress's extra ($old_value) argument here is simply unused. | |
| add_action( 'update_option_' . \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, array( __NAMESPACE__ . '\\Agent_Provisioner', 'register_role' ) ); | |
| // register_role() derives Agent_Provisioner::TEMPLATE_EDIT_CAP from this | |
| // toggle; re-assert on save so grant/revoke is immediate rather than | |
| // waiting for the next `init`. register_role() takes no required args, | |
| // so WordPress's extra ($old_value) argument here is simply unused. | |
| add_action( 'update_option_' . \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, array( __NAMESPACE__ . '\\Agent_Provisioner', 'register_role' ) ); | |
| add_action( | |
| 'deleted_option', | |
| static function ( $option ) { | |
| $is_template_edits_option = \GravityKit\BlockMCP\Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION === $option; | |
| if ( ! $is_template_edits_option ) { | |
| return; | |
| } | |
| Agent_Provisioner::register_role(); | |
| }, | |
| 10, | |
| 1 | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/gk-block-mcp.php` around lines 338 - 342, The
option lifecycle currently reasserts the agent role only through the update
hook, so deletion during settings reset leaves stale TEMPLATE_EDIT_CAP until
init. In the hook registration near Agent_Provisioner::register_role, add a
post-delete handler scoped to ALLOW_TEMPLATE_EDITS_OPTION that invokes the same
role reassertion callback, and add a regression test using delete_option() or
the reset flow to verify the capability is revoked immediately.
Source: Coding guidelines
| // TEMPLATE_EDIT_CAP is the one cap this class both adds and | ||
| // removes: the additive loop above never takes it away, so a | ||
| // toggle flipped off needs this explicit revoke or the grant | ||
| // would outlive the setting that authorized it. | ||
| if ( ! $caps[ self::TEMPLATE_EDIT_CAP ] && $existing->has_cap( self::TEMPLATE_EDIT_CAP ) ) { | ||
| $existing->remove_cap( self::TEMPLATE_EDIT_CAP ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid an undefined capability-map offset.
The public gk/block-mcp/agent/caps filter can unset TEMPLATE_EDIT_CAP; line 182 then reads a missing key. Capture both values before the condition.
Proposed fix
- if ( ! $caps[ self::TEMPLATE_EDIT_CAP ] && $existing->has_cap( self::TEMPLATE_EDIT_CAP ) ) {
+ $template_edit_cap_granted = ! empty( $caps[ self::TEMPLATE_EDIT_CAP ] );
+ $role_has_template_edit_cap = $existing->has_cap( self::TEMPLATE_EDIT_CAP );
+ if ( ! $template_edit_cap_granted && $role_has_template_edit_cap ) {
$existing->remove_cap( self::TEMPLATE_EDIT_CAP );
}As per coding guidelines, assign compound checks to named variables before using them in if conditions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TEMPLATE_EDIT_CAP is the one cap this class both adds and | |
| // removes: the additive loop above never takes it away, so a | |
| // toggle flipped off needs this explicit revoke or the grant | |
| // would outlive the setting that authorized it. | |
| if ( ! $caps[ self::TEMPLATE_EDIT_CAP ] && $existing->has_cap( self::TEMPLATE_EDIT_CAP ) ) { | |
| $existing->remove_cap( self::TEMPLATE_EDIT_CAP ); | |
| } | |
| // TEMPLATE_EDIT_CAP is the one cap this class both adds and | |
| // removes: the additive loop above never takes it away, so a | |
| // toggle flipped off needs this explicit revoke or the grant | |
| // would outlive the setting that authorized it. | |
| $template_edit_cap_granted = ! empty( $caps[ self::TEMPLATE_EDIT_CAP ] ); | |
| $role_has_template_edit_cap = $existing->has_cap( self::TEMPLATE_EDIT_CAP ); | |
| if ( ! $template_edit_cap_granted && $role_has_template_edit_cap ) { | |
| $existing->remove_cap( self::TEMPLATE_EDIT_CAP ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/includes/class-agent-provisioner.php` around
lines 178 - 184, In the capability reconciliation logic around
TEMPLATE_EDIT_CAP, safely handle the public gk/block-mcp/agent/caps filter
removing that key by capturing the filtered capability value with an appropriate
default and the existing capability state in named variables before the
condition. Update the if statement to use those variables, preserving the
explicit remove_cap behavior and complying with the compound-check naming
guideline.
Source: Coding guidelines
| /** | ||
| * With the toggle on, an editor (edit_posts, no dedicated cap, no | ||
| * edit_theme_options) is denied via the ability, matching REST-level | ||
| * coverage of the same fix — the Abilities surface delegates to the | ||
| * same check_template_edit_permissions() callback, so it can't be used | ||
| * to bypass the capability gate REST enforces. | ||
| */ | ||
| public function test_update_template_ability_denies_editor_even_with_gate_on() { | ||
| update_option( Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, '1' ); | ||
| wp_set_current_user( self::factory()->user->create( array( 'role' => 'editor' ) ) ); | ||
|
|
||
| $this->setExpectedIncorrectUsage( 'WP_Ability::execute' ); | ||
| $result = wp_get_ability( 'gk-block-mcp/update-template' )->execute( | ||
| array( | ||
| 'id' => $this->theme . '//index', | ||
| 'content' => '<!-- wp:paragraph --><p>x</p><!-- /wp:paragraph -->', | ||
| ) | ||
| ); | ||
|
|
||
| $this->assertWPError( $result ); | ||
| $this->assertSame( 'ability_invalid_permissions', $result->get_error_code() ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the dedicated-capability allow path through Abilities.
These tests prove the edit_theme_options allow branch and editor denial, but not that an agent role granted TEMPLATE_EDIT_CAP can execute gk-block-mcp/update-template. Add an Ability-level test that enables the toggle, calls Agent_Provisioner::register_role(), assigns that role, and asserts successful execution.
As per coding guidelines, regression tests must exercise relevant capability and API facets.
🧰 Tools
🪛 PHPStan (2.2.5)
[error] 257-257: Call to an undefined static method TemplateAbilitiesTest::factory().
(staticMethod.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wordpress-plugin/gk-block-mcp/tests/Abilities/TemplateAbilitiesTest.php`
around lines 248 - 269, The Abilities tests lack coverage for the dedicated
template-edit capability allow path. In TemplateAbilitiesTest, add a test for
gk-block-mcp/update-template that enables the template-edit toggle, invokes
Agent_Provisioner::register_role(), assigns the provisioned role to the current
user, executes the ability with valid template data, and asserts successful
execution rather than a WP_Error.
Source: Coding guidelines
…38) (#66) * fix(security): BLOCK-38 CodeRabbit follow-ups — option lifecycle, cap-map safety, coverage Three follow-ups from PR #65's review: - Reassert the agent role on add/delete of gk_block_api_template_edits, not just update. delete_option() fires the generic `deleted_option` action (no `delete_option_{$option}` hook exists), and a fresh site's first enable routes through add_option() rather than update_option() — both previously left TEMPLATE_EDIT_CAP stale until the next `init`, so a settings reset didn't immediately revoke the write capability. - Guard the TEMPLATE_EDIT_CAP read in Agent_Provisioner::register_role() against the public gk/block-mcp/agent/caps filter unsetting the key entirely (rather than setting it false), which previously raised an undefined-array-key warning. Unset now revokes the cap, the safe default. - Add ability-layer coverage proving the dedicated block_mcp_agent role (holding TEMPLATE_EDIT_CAP, not edit_theme_options) can write templates through the Abilities API — the actual allow branch BLOCK-38 added, previously exercised only via edit_theme_options in this test file. Each of the first two fixes ships with a test that failed pre-fix (deleting the option left the cap granted; the filter unset triggered a PHP warning) and passes post-fix. composer test: 1488 tests green across all four suite configs (main, yoast, adapter, multisite). Claude-Session: https://claude.ai/code/session_01EddiNE6Q2mqXo5koeiVTpT * fix(settings): Release 2.2.0 CodeRabbit follow-ups — toggles persist stored value; template-cap docs Address the CodeRabbit review on PR #64 (Release 2.2.0). Behavioral fix (security): the Media uploads, Move-to-trash, template-editing, and MCP Adapter toggles rendered their checkbox from the post-filter effective value (*_enabled()), not the stored option. Because the checkbox is the form control, a gk/block-mcp/* filter overriding the stored value meant saving the settings page silently wrote the filtered value back into the option, flipping the admin's persisted security setting. Each checkbox now renders from the stored option; the divergence is surfaced by the existing "Heads up" override notice, which now reuses the already-computed *_enabled value instead of re-applying the filter. CodeRabbit flagged only template editing; the identical bug existed in all four toggles (uploads/trash shipped since 2.0.0/2.1.0), so all four are fixed. Regression test drives the real render_page() across every toggle (fails pre-fix, has teeth), and confirmed e2e against a live site. Docs/comments: - Template-write authorization is the dedicated gk_block_mcp_edit_templates capability or edit_theme_options (not edit_posts): corrected the stale rest-controller.php comment and the AGENTS.md/README.md permission docs. - README: reconcile per-block-tool applicability on templates (theme-file-only wp_id:null not writable; a numeric override is an ordinary post). - AGENTS.md: list create_pattern in the patterns MCP-tool inventory. - template-manager.php: assign the compound area condition to a named variable. Claude-Session: https://claude.ai/code/session_01EddiNE6Q2mqXo5koeiVTpT
Summary
Fixes BLOCK-38 — a security-review finding confirmed by Fable that blocks the develop→main 2.2.0 release (#64).
Finding:
check_template_edit_permissions()(class-rest-controller.php:1195) passed when the template-edit toggle was on and the caller hadedit_postsoredit_theme_options. The plugin performs the underlyingwp_insert_post()/wp_update_post()onwp_template/wp_template_partitself — those don't enforce capabilities — so with the toggle on, any contributor-or-above account, or any leaked low-privilege Application Password, could rewrite sitewide template chrome (header, footer, 404, archive, search).Fix (Fable's design): a plugin-owned primitive cap,
gk_block_mcp_edit_templates, managed by the existing toggle checkbox — never a raw grant ofedit_theme_options(which would also open core's/wp/v2/templates,/wp/v2/template-parts,/wp/v2/navigation,/wp/v2/global-styles, the Customizer, menus, and widgets to the agent's Application Password, and would be stripped by the forbidden-capabilities re-assert loop anyway).Changes
Agent_Provisioner::TEMPLATE_EDIT_CAP(new constant) — deliberately not added toforbidden_capabilities(); that denylist strips caps this class never grants, this one it grants and revokes on purpose.register_role()computes the cap fromTemplate_Manager::edits_enabled()before the existinggk/block-mcp/agent/capsfilter runs (operators can still override it). On an existing role it's the one cap both added (via the existing additive loop) and — new — removed when the toggle is off, scoped to only this cap; everything else about the additive-only re-assert and the forbidden-cap strip loop is untouched.update_option_gk_block_api_template_editshook (new, ingk-block-mcp.php) re-assertsregister_role()immediately on settings save, so grant/revoke doesn't wait for the nextinit(priority-99 self-heal is still the fallback for e.g. "Reset to defaults", which usesdelete_option()).check_template_edit_permissions()now passes oncurrent_user_can('gk_block_mcp_edit_templates') || current_user_can('edit_theme_options'), replacing theedit_postsclause. The toggle check still runs first. Docblock rewritten to document the actual vulnerability, not the old (incorrect)edit_postsrationale.class-settings-page.php) reworded to disclose the mechanism — granting the toggle changes what the Block MCP agent account itself can do to the theme layer.Strict TDD — visible red/green, unsquashed
ea825c1test(security): pin BLOCK-38 template-write cap gate [red]f1238cffix(security): gate template writes on gk_block_mcp_edit_templates cap [green]Red proof —
git stashof only the implementation files (keeping the new tests), then the primary pin run alone against pre-fix code:A plain contributor (
edit_posts, no dedicated cap) got 200 against current shipped code — the vulnerability, proven. Restored the implementation (git stash pop), same test now passes.Companion pins (all in the red commit, all confirmed failing pre-fix, passing post-fix): an editor is equally denied (same
edit_posts-only shape); the agent role, afterregister_role()runs with the toggle on, holdsTEMPLATE_EDIT_CAP(capability-level assertion) andPOST /templatesucceeds; toggling back off and re-runningregister_role()revokes the cap (capability-level) and the route 403s again; the existingedit_theme_options-alone success path is unaffected; the existing forbidden-cap-strip test (test_register_role_strips_forbidden_caps_from_existing_role) is unmodified and still green, proving the new managed-cap removal branch doesn't weaken that denylist. The Abilities path (gk-block-mcp/update-template) is pinned separately since it delegates to the samecheck_template_edit_permissions()callback.Acceptance criteria
register_role(); not inforbidden_capabilities()Gate
Siteminter smoke
Fresh site (
block-38-authz, WordPress 7.0.2,twentytwentyfiveactive — a real FSE block theme),gk-block-mcp2.2.0 active.1. Toggle ON, contributor denied:
2. Toggle ON, admin (
edit_theme_options) succeeds:3. Toggle ON, the agent role itself (dedicated cap, confirmed NOT
edit_theme_options) succeeds:4. Toggle OFF — cap revoked immediately (no extra request, via the new
update_optionhook) and route 403s again:Site will be destroyed after this PR merges.
Claude-Session: https://claude.ai/code/session_013YcSbKroBJjPanX3okQrT3
💾 Build file (f1238cf).
Summary by CodeRabbit