fix: harden deprecated block defaults management endpoints#3725
Open
bfintal wants to merge 1 commit into
Open
Conversation
Sanitize block default style data on save and read, and restrict update/delete REST routes to users who can manage theme options. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds centralized capability checks and comprehensive sanitization for stored block styles, REST parameters, REST mutations, admin redirects, and temporary editor content. ChangesBlock Style Security
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Pull request artifacts
|
|
Size Change: +1.41 kB (+0.05%) Total Size: 2.63 MB 📦 View Changed
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/deprecated/block-defaults/custom-block-styles.php`:
- Around line 429-435: Update the docblocks for the affected REST callback
methods, including the method containing the invalid block/style validation and
delete_block_style, from `@return` void to `@return` WP_REST_Response|WP_Error to
match their actual return values.
- Around line 186-192: Replace the case-insensitive prefix checks using stripos
with case-sensitive checks in sanitize_block_name, sanitize_block_name_param,
and sanitize_parsed_blocks, ensuring only names beginning with the exact
lowercase "stackable/" prefix are accepted and stored.
- Around line 617-628: Protect edit_default_block_redirect() with an admin nonce
check before processing request parameters, using a dedicated action and
rejecting invalid requests via check_admin_referer() or wp_verify_nonce().
Update the generated link that invokes this redirect to include the matching
nonce.
- Around line 302-304: Update sanitize_parsed_blocks() so the stackable/
block-name filter applies only to top-level blocks, preserving nested core
blocks such as core/paragraph; ensure the option_stackable_block_styles read
path retains these descendants while still filtering unrelated top-level blocks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6027c176-3b5d-4a58-af7d-6d1dd099c9c7
📒 Files selected for processing (1)
src/deprecated/block-defaults/custom-block-styles.php
Comment on lines
+186
to
+192
| public function sanitize_block_name( $block ) { | ||
| $block = sanitize_text_field( $block ); | ||
| if ( stripos( $block, 'stackable/' ) !== 0 ) { | ||
| return ''; | ||
| } | ||
| return $block; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
stripos makes the prefix check case-insensitive.
stripos( $block, 'stackable/' ) !== 0 accepts values like Stackable/heading, which then get stored verbatim and won't match the real (lowercase) block name during lookups in update_block_style/redirect_to_block_style_editor. Use a case-sensitive check to keep stored names canonical. Same applies to sanitize_block_name_param (Line 385) and sanitize_parsed_blocks (Line 302).
Proposed change
- if ( stripos( $block, 'stackable/' ) !== 0 ) {
+ if ( strpos( $block, 'stackable/' ) !== 0 ) {
return '';
}📝 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.
Suggested change
| public function sanitize_block_name( $block ) { | |
| $block = sanitize_text_field( $block ); | |
| if ( stripos( $block, 'stackable/' ) !== 0 ) { | |
| return ''; | |
| } | |
| return $block; | |
| } | |
| public function sanitize_block_name( $block ) { | |
| $block = sanitize_text_field( $block ); | |
| if ( strpos( $block, 'stackable/' ) !== 0 ) { | |
| return ''; | |
| } | |
| return $block; | |
| } |
🤖 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 `@src/deprecated/block-defaults/custom-block-styles.php` around lines 186 -
192, Replace the case-insensitive prefix checks using stripos with
case-sensitive checks in sanitize_block_name, sanitize_block_name_param, and
sanitize_parsed_blocks, ensuring only names beginning with the exact lowercase
"stackable/" prefix are accepted and stored.
Comment on lines
+302
to
+304
| if ( stripos( $block['blockName'], 'stackable/' ) !== 0 ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline src/deprecated/block-defaults/custom-block-styles.php --view expanded || true
echo
echo "== Relevant lines around 260-340 =="
sed -n '260,340p' src/deprecated/block-defaults/custom-block-styles.php | cat -n
echo
echo "== Search for sanitize_parsed_blocks usage and related parsers =="
rg -n "sanitize_parsed_blocks|block-defaults|option_.*block" src . -g '!**/node_modules/**' || true
echo
echo "== Search for save markup/default templates that may include inner blocks =="
rg -n "innerBlocks|parse_blocks|serialize_blocks|save markup|blockName" src/deprecated src -g '!**/node_modules/**' || trueRepository: gambitph/Stackable
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== save-block editor path =="
sed -n '1,220p' src/deprecated/block-defaults/save-block/custom-block-styles-editor.js | cat -n
echo
echo "== save-block application path =="
sed -n '1,180p' src/deprecated/block-defaults/save-block/use-saved-default-block-style.js | cat -n
echo
echo "== block templates / examples that define innerBlocks =="
sed -n '1,220p' src/deprecated/v2/block/accordion/example.js | cat -n
echo
sed -n '1,220p' src/deprecated/v2/block/testimonial/example.js | cat -n 2>/dev/null || true
echo
sed -n '1,220p' src/deprecated/v2/block/columns/example.js | cat -n 2>/dev/null || true
echo
echo "== any allowedBlocks or inserter restrictions in the saved-style editor =="
rg -n "allowedBlocks|templateLock|InnerBlocks|createBlocksFromInnerBlocksTemplate|getBlocks\\(|insertBlock|useInnerBlocksProps" src/deprecated/block-defaults/save-block src/deprecated/v2/block -g '!**/node_modules/**' || trueRepository: gambitph/Stackable
Length of output: 29707
Nested core blocks are being stripped from saved styles. sanitize_parsed_blocks() drops every non-stackable/ block at any depth, but saved block-style markup can include core inner blocks (for example, stackable/accordion includes a core/paragraph). That removes valid content from the serialized style and from the option_stackable_block_styles read path. Allow non-Stackable descendants or scope the filter to the top-level block only.
🤖 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 `@src/deprecated/block-defaults/custom-block-styles.php` around lines 302 -
304, Update sanitize_parsed_blocks() so the stackable/ block-name filter applies
only to top-level blocks, preserving nested core blocks such as core/paragraph;
ensure the option_stackable_block_styles read path retains these descendants
while still filtering unrelated top-level blocks.
Comment on lines
+429
to
+435
| if ( empty( $block ) || empty( $slug ) ) { | ||
| return new WP_Error( | ||
| 'invalid_block_style', | ||
| __( 'Invalid block or style slug.', STACKABLE_I18N ), | ||
| array( 'status' => 400 ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the @return void docblock.
This method now returns WP_Error (and rest_ensure_response(...)), but the docblock still declares @return void, which is what PHPStan flags. Update it to @return WP_REST_Response|WP_Error. delete_block_style (Line 494) has the same mismatch.
🧰 Tools
🪛 PHPStan (2.2.2)
[error] 430-430: Method Stackable_Custom_Block_Styles::update_block_style() with return type void returns WP_Error but should not return anything.
(return.void)
🤖 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 `@src/deprecated/block-defaults/custom-block-styles.php` around lines 429 -
435, Update the docblocks for the affected REST callback methods, including the
method containing the invalid block/style validation and delete_block_style,
from `@return` void to `@return` WP_REST_Response|WP_Error to match their actual
return values.
Source: Linters/SAST tools
Comment on lines
617
to
+628
| public function edit_default_block_redirect() { | ||
| $block_name = sanitize_text_field( $_REQUEST['stk_edit_block'] ); | ||
| $style_slug = sanitize_text_field( $_REQUEST['stk_edit_block_style'] ); | ||
| $block_title = sanitize_text_field( $_REQUEST['stk_edit_block_title'] ); | ||
| if ( ! self::can_manage_block_styles() ) { | ||
| wp_die( esc_html__( 'You do not have permission to edit block default styles.', STACKABLE_I18N ) ); | ||
| } | ||
|
|
||
| $block_name = $this->sanitize_block_name( sanitize_text_field( wp_unslash( $_REQUEST['stk_edit_block'] ) ) ); | ||
| $style_slug = $this->sanitize_style_slug( sanitize_text_field( wp_unslash( $_REQUEST['stk_edit_block_style'] ) ) ); | ||
| $block_title = sanitize_text_field( wp_unslash( $_REQUEST['stk_edit_block_title'] ) ); | ||
|
|
||
| if ( empty( $block_name ) || empty( $style_slug ) ) { | ||
| wp_die( esc_html__( 'Invalid block default style.', STACKABLE_I18N ) ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and find the relevant functions/callers.
ast-grep outline src/deprecated/block-defaults/custom-block-styles.php --view expanded || true
printf '\n--- function matches ---\n'
rg -n "edit_default_block_redirect|redirect_to_block_style_editor|can_manage_block_styles|admin_init|wp_verify_nonce|check_admin_referer|wp_nonce_url" src/deprecated/block-defaults/custom-block-styles.php
printf '\n--- surrounding lines ---\n'
sed -n '560,720p' src/deprecated/block-defaults/custom-block-styles.phpRepository: gambitph/Stackable
Length of output: 7765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- nonce / request-param search ---\n'
rg -n "stk_edit_block|stk_edit_block_style|stk_edit_block_title|wp_nonce_url|wp_create_nonce|wp_verify_nonce|check_admin_referer|check_ajax_referer" src
printf '\n--- edit_default_block_redirect + redirect_to_block_style_editor (full) ---\n'
sed -n '617,760p' src/deprecated/block-defaults/custom-block-styles.php
printf '\n--- constructor / hook setup ---\n'
sed -n '1,90p' src/deprecated/block-defaults/custom-block-styles.phpRepository: gambitph/Stackable
Length of output: 10347
Add a nonce to this admin redirect
edit_default_block_redirect() accepts request params on admin_init and passes them to redirect_to_block_style_editor(), which creates/updates the temp post and option state. Without a nonce, a crafted link can trigger that change for any user with edit_theme_options. Add check_admin_referer/wp_verify_nonce here and include the nonce in the generated link.
🤖 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 `@src/deprecated/block-defaults/custom-block-styles.php` around lines 617 -
628, Protect edit_default_block_redirect() with an admin nonce check before
processing request parameters, using a dedicated action and rejecting invalid
requests via check_admin_referer() or wp_verify_nonce(). Update the generated
link that invokes this redirect to include the matching nonce.
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
1 participant
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.
Summary
block,slug,name,data,save) before storage and on readupdate_block_style/delete_block_styleREST endpoints to users withedit_theme_optionsThis only affects the deprecated Block Defaults feature when
stackable_enable_block_defaultsis enabled. The newer Block Styles feature is unchanged.Test plan
Prerequisites
edit_theme_options)Block Defaults panel (Global Settings)
Save from block menu
Regression
Release
Target: 3.19.10
@Arukuen — please review when you have a moment.
Made with Cursor
Summary by CodeRabbit