-
Notifications
You must be signed in to change notification settings - Fork 5
fix(security): BLOCK-38 — gate template writes on a dedicated cap, not edit_posts #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,22 @@ class Agent_Provisioner { | |||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| const USER_ID_OPTION = 'gk_block_api_agent_user_id'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Plugin-owned primitive cap that gates template writes (POST /template, | ||||||||||||||||||||||||||||||||||
| * POST /template/reset), managed here rather than granted via core's | ||||||||||||||||||||||||||||||||||
| * `edit_theme_options` — that cap also opens core's own | ||||||||||||||||||||||||||||||||||
| * `/wp/v2/templates`, `/wp/v2/template-parts`, `/wp/v2/navigation`, | ||||||||||||||||||||||||||||||||||
| * `/wp/v2/global-styles`, the Customizer, menus, and widgets, none of | ||||||||||||||||||||||||||||||||||
| * which the agent should ever reach. Deliberately NOT in | ||||||||||||||||||||||||||||||||||
| * forbidden_capabilities() — that denylist exists to strip caps this | ||||||||||||||||||||||||||||||||||
| * class does not grant; this one it grants and revokes on purpose, | ||||||||||||||||||||||||||||||||||
| * following the site's `gk_block_api_template_edits` toggle. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * @since 2.2.0 | ||||||||||||||||||||||||||||||||||
| * @var string | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| const TEMPLATE_EDIT_CAP = 'gk_block_mcp_edit_templates'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Register the minimal block_mcp_agent role idempotently. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
|
|
@@ -104,10 +120,12 @@ public static function register_role(): string { | |||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * @param array<string,bool> $caps Map of capability name => granted, for the agent role. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| $caps = apply_filters( | ||||||||||||||||||||||||||||||||||
| 'gk/block-mcp/agent/caps', | ||||||||||||||||||||||||||||||||||
| self::derive_capabilities() | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| $caps = self::derive_capabilities(); | ||||||||||||||||||||||||||||||||||
| // The one entry in this map that register_role() below both adds AND | ||||||||||||||||||||||||||||||||||
| // removes on an existing role, tracking the toggle live rather than | ||||||||||||||||||||||||||||||||||
| // whatever was true when the role was first created. | ||||||||||||||||||||||||||||||||||
| $caps[ self::TEMPLATE_EDIT_CAP ] = Template_Manager::edits_enabled(); | ||||||||||||||||||||||||||||||||||
| $caps = apply_filters( 'gk/block-mcp/agent/caps', $caps ); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Run the AI agent on a role you control instead of the built-in one. | ||||||||||||||||||||||||||||||||||
|
|
@@ -157,6 +175,13 @@ public static function register_role(): string { | |||||||||||||||||||||||||||||||||
| $existing->remove_cap( $forbidden ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| // 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 ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+178
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Avoid an undefined capability-map offset. The public 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 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,14 +210,19 @@ public function test_update_template_ability_denies_when_filter_forces_off() { | |
| } | ||
|
|
||
| /** | ||
| * With the toggle on, an editor (edit_posts, no edit_theme_options) can | ||
| * write via the ability — matching the REST route's two-part permission | ||
| * callback (toggle ON and edit_posts OR edit_theme_options) — and the | ||
| * change round-trips through get-template. | ||
| * With the toggle on, an actor holding edit_theme_options can write via | ||
| * the ability — matching the REST route's permission callback (toggle | ||
| * ON and the dedicated cap OR edit_theme_options) — and the change | ||
| * round-trips through get-template. | ||
| */ | ||
| public function test_update_template_ability_persists_change_when_gate_on() { | ||
| update_option( Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, '1' ); | ||
| wp_set_current_user( self::factory()->user->create( array( 'role' => 'editor' ) ) ); | ||
|
|
||
| // edit_posts too, unlike the write-only test below: the get-template | ||
| // read-back is gated on the 'read' bucket, which checks edit_posts. | ||
| $role_name = 'gk_test_ability_persist_theme_options_only'; | ||
| add_role( $role_name, 'Theme Options Only', array( 'read' => true, 'edit_posts' => true, 'edit_theme_options' => true ) ); | ||
| wp_set_current_user( self::factory()->user->create( array( 'role' => $role_name ) ) ); | ||
|
|
||
| $result = wp_get_ability( 'gk-block-mcp/update-template' )->execute( | ||
| array( | ||
|
|
@@ -232,11 +237,37 @@ public function test_update_template_ability_persists_change_when_gate_on() { | |
| $this->assertGreaterThan( 0, $result['wp_id'] ); | ||
|
|
||
| $read = wp_get_ability( 'gk-block-mcp/get-template' )->execute( array( 'id' => $this->theme . '//index' ) ); | ||
|
|
||
| remove_role( $role_name ); | ||
|
|
||
| $this->assertNotWPError( $read ); | ||
| $this->assertSame( 'custom', $read['source'] ); | ||
| $this->assertStringContainsString( 'ABILITY-MARKER', $read['content'] ); | ||
| } | ||
|
|
||
| /** | ||
| * 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() ); | ||
| } | ||
|
Comment on lines
+248
to
+269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Cover the dedicated-capability allow path through Abilities. These tests prove the 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 AgentsSource: Coding guidelines |
||
|
|
||
| /** | ||
| * With the toggle on, an actor holding neither edit_posts nor | ||
| * edit_theme_options (a subscriber) is still denied — the toggle widens | ||
|
|
@@ -292,7 +323,12 @@ public function test_update_template_ability_succeeds_via_edit_theme_options_alo | |
| */ | ||
| public function test_reset_template_ability_deletes_override_when_gate_on() { | ||
| update_option( Template_Manager::ALLOW_TEMPLATE_EDITS_OPTION, '1' ); | ||
| wp_set_current_user( self::factory()->user->create( array( 'role' => 'editor' ) ) ); | ||
|
|
||
| // edit_posts too: the get-template read-back is gated on the 'read' | ||
| // bucket, which checks edit_posts, not edit_theme_options. | ||
| $role_name = 'gk_test_ability_reset_theme_options_only'; | ||
| add_role( $role_name, 'Theme Options Only', array( 'read' => true, 'edit_posts' => true, 'edit_theme_options' => true ) ); | ||
| wp_set_current_user( self::factory()->user->create( array( 'role' => $role_name ) ) ); | ||
|
|
||
| $created = wp_get_ability( 'gk-block-mcp/update-template' )->execute( | ||
| array( | ||
|
|
@@ -310,6 +346,9 @@ public function test_reset_template_ability_deletes_override_when_gate_on() { | |
| $this->assertNull( get_post( $created['wp_id'] ) ); | ||
|
|
||
| $read = wp_get_ability( 'gk-block-mcp/get-template' )->execute( array( 'id' => $this->theme . '//index' ) ); | ||
|
|
||
| remove_role( $role_name ); | ||
|
|
||
| $this->assertNotWPError( $read ); | ||
| $this->assertSame( 'theme', $read['source'] ); | ||
| $this->assertNull( $read['wp_id'] ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 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 retainTEMPLATE_EDIT_CAPuntil the next request’sinit, despite the effective toggle being off. Add a post-deletedeleted_optionhandler scoped to this option, and coverdelete_option()/reset with a regression test.Proposed fix
As per coding guidelines, every bug fix requires a regression test exercising the real mechanism.
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines