feat: add block defaults for heading theme margin and icon for icon l… - #3676
Open
Arukuen wants to merge 5 commits into
Open
feat: add block defaults for heading theme margin and icon for icon l…#3676Arukuen wants to merge 5 commits into
Arukuen wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds Block Defaults settings for Heading theme margins and the Icon List default icon. The settings UI, REST exposure, SVG sanitization, editor context, and block inserter variations now use these values. ChangesBlock default settings
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminSettings
participant StackableEditorSettings
participant HeadingVariation
participant IconListVariation
participant BlockInserter
AdminSettings->>StackableEditorSettings: Save Heading margin and Icon List icon settings
StackableEditorSettings-->>BlockInserter: Provide settings and current post type
BlockInserter->>HeadingVariation: Create Heading variation
HeadingVariation-->>BlockInserter: Set post-specific theme margin attribute
BlockInserter->>IconListVariation: Create Icon List variation
IconListVariation-->>BlockInserter: Set configured SVG or default SVG
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/editor-settings.php`:
- Around line 286-298: sanitize_svg_setting currently uses brittle regexes that
miss unquoted event handlers, data: URLs, xlink/href javascript targets, <use>
references and animation elements, and does not handle preg_replace returning
null; update it to perform robust sanitization by either (A) replacing the
ad-hoc regex approach in sanitize_svg_setting with WordPress's wp_kses using a
restrictive SVG-specific allowed tags/attributes whitelist (including removal of
on* attributes regardless of quoting, stripping href/xlink:href values that
start with javascript: or data:, and disallowing <use>, <animate>, <set>), or
(B) if you keep regexes, add patterns to remove unquoted on* attributes, strip
any href/xlink:href attributes whose values begin with javascript: or data:,
remove <use>, <animate>, <set> elements, and after each preg_replace check for
null and handle by returning an empty string or logging and returning safe
output; make these changes inside the sanitize_svg_setting function to ensure
all dangerous patterns are covered.
🧹 Nitpick comments (4)
src/block/icon-list/edit.js (1)
208-213: Missing dependencies inuseEffectmay cause lint warnings.The effect references
attributes.icon,setAttributes, andsettingsbut has an empty dependency array. While the intent is to run once on mount, this pattern typically triggers React'sexhaustive-depslint rule.If this is intentional (and linting allows it), consider adding a suppression comment to clarify intent. Otherwise, the pattern used in
heading/edit.js(also with empty deps) suggests this is acceptable in this codebase.Optional: Add lint suppression for clarity
// Set icon default value from setting on first load useEffect( () => { if ( attributes.icon === undefined || attributes.icon === '' ) { setAttributes( { icon: settings.stackable_icon_list_block_default_icon || DEFAULT_SVG } ) } + // eslint-disable-next-line react-hooks/exhaustive-deps }, [] )src/block/heading/edit.js (1)
91-102:postTypeis not in the dependency array but is used in the effect.The effect uses
postTypefromuseSelectto determine the default margin setting, butpostTypeis not in the dependency array. While post type rarely changes during editing, if it ever does (e.g., in some custom editor scenarios), the effect won't recalculate.Given this is an initialization effect meant to run once, and post type switching is extremely rare, this is likely acceptable. Consider adding a comment or lint suppression for clarity.
Optional: Add lint suppression for clarity
// Set useThemeTextMargins default value from setting on first load useEffect( () => { if ( attributes.useThemeTextMargins === undefined || attributes.useThemeTextMargins === '' ) { const isPost = postType === 'post' const defaultThemeMargins = isPost ? !! settings.stackable_enable_heading_default_theme_margins_posts : !! settings.stackable_enable_heading_default_theme_margins_non_posts setAttributes( { useThemeTextMargins: defaultThemeMargins } ) } + // eslint-disable-next-line react-hooks/exhaustive-deps }, [] )src/editor-settings.php (1)
244-267: Consider usingrest_sanitize_booleanfor boolean settings.The new boolean settings use
sanitize_text_fieldas the sanitize callback. While this follows the pattern used elsewhere in this file (lines 84, 132, etc.), usingrest_sanitize_boolean(as on line 39) would be more semantically correct for boolean types.This is a minor consistency issue and doesn't affect functionality since the REST API handles boolean coercion.
Optional: Use rest_sanitize_boolean for boolean settings
register_setting( 'stackable_editor_settings', 'stackable_enable_heading_default_theme_margins_posts', array( 'type' => 'boolean', 'description' => __( "When enabled, newly added Stackable Heading blocks in Posts will use the theme's default margins automatically.", STACKABLE_I18N ), - 'sanitize_callback' => 'sanitize_text_field', + 'sanitize_callback' => 'rest_sanitize_boolean', 'show_in_rest' => true, 'default' => false, ) ); register_setting( 'stackable_editor_settings', 'stackable_enable_heading_default_theme_margins_non_posts', array( 'type' => 'boolean', 'description' => __( "When enabled, newly added Stackable Heading blocks in non-Post content (Pages and custom post types) will use the theme's default margins automatically.", STACKABLE_I18N ), - 'sanitize_callback' => 'sanitize_text_field', + 'sanitize_callback' => 'rest_sanitize_boolean', 'show_in_rest' => true, 'default' => false, ) );src/components/admin-icon-setting/index.js (1)
16-18: Consider simplifying theonChangehandler.The inline arrow function can be replaced with a direct reference to
props.onChangesince it just passes the argument through without transformation.♻️ Suggested simplification
- onChange={ icon => { - props.onChange( icon ) - } } + onChange={ props.onChange }
Comment on lines
+286
to
+298
| public function sanitize_svg_setting( $input ) { | ||
| if ( empty( $input ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| // Remove scripts, event handlers, foreignObject, iframe, embeds | ||
| $input = preg_replace( '/<\s*(script|iframe|embed|object|foreignObject)[^>]*>.*?<\s*\/\s*\1\s*>/is', '', $input ); | ||
| $input = preg_replace( '/on\w+\s*=\s*"[^"]*"/i', '', $input ); | ||
| $input = preg_replace( "/on\w+\s*=\s*'[^']*'/i", '', $input ); | ||
| $input = preg_replace( '/javascript:/i', '', $input ); | ||
|
|
||
| return $input; | ||
| } |
There was a problem hiding this comment.
SVG sanitization may be incomplete for security-critical use.
The current implementation provides basic protection but has gaps that could allow XSS:
-
Unquoted event handlers: The regex only handles quoted attributes (
on\w+=\s*"..."and'...'), missing unquoted values likeonclick=alert(1). -
Missing dangerous patterns:
data:URLs (e.g.,xlink:href="data:text/html,<script>...")xlink:hrefandhrefattributes pointing tojavascript:<use>elements referencing external content<animate>,<set>elements that can trigger scripts
-
Error handling:
preg_replacereturnsnullon error; this should be handled.
Proposed improvements for more robust sanitization
public function sanitize_svg_setting( $input ) {
if ( empty( $input ) ) {
return '';
}
// Remove scripts, event handlers, foreignObject, iframe, embeds
$input = preg_replace( '/<\s*(script|iframe|embed|object|foreignObject)[^>]*>.*?<\s*\/\s*\1\s*>/is', '', $input );
+ // Remove potentially dangerous elements
+ $input = preg_replace( '/<\s*(use|animate|set|animateTransform)[^>]*\/?>/is', '', $input );
$input = preg_replace( '/on\w+\s*=\s*"[^"]*"/i', '', $input );
$input = preg_replace( "/on\w+\s*=\s*'[^']*'/i", '', $input );
+ // Handle unquoted event handlers
+ $input = preg_replace( '/on\w+\s*=\s*[^\s>]+/i', '', $input );
$input = preg_replace( '/javascript:/i', '', $input );
+ // Remove data: URLs and xlink:href with dangerous protocols
+ $input = preg_replace( '/xlink:href\s*=\s*["\'][^"\']*(?:javascript:|data:)[^"\']*["\']/i', '', $input );
+ $input = preg_replace( '/href\s*=\s*["\'][^"\']*(?:javascript:|data:)[^"\']*["\']/i', '', $input );
+ // Handle preg_replace errors
+ if ( $input === null ) {
+ return '';
+ }
+
return $input;
}Alternatively, consider using WordPress's built-in wp_kses with an SVG-specific allowed tags/attributes list, or a dedicated SVG sanitization library for more comprehensive protection.
🤖 Prompt for AI Agents
In `@src/editor-settings.php` around lines 286 - 298, sanitize_svg_setting
currently uses brittle regexes that miss unquoted event handlers, data: URLs,
xlink/href javascript targets, <use> references and animation elements, and does
not handle preg_replace returning null; update it to perform robust sanitization
by either (A) replacing the ad-hoc regex approach in sanitize_svg_setting with
WordPress's wp_kses using a restrictive SVG-specific allowed tags/attributes
whitelist (including removal of on* attributes regardless of quoting, stripping
href/xlink:href values that start with javascript: or data:, and disallowing
<use>, <animate>, <set>), or (B) if you keep regexes, add patterns to remove
unquoted on* attributes, strip any href/xlink:href attributes whose values begin
with javascript: or data:, remove <use>, <animate>, <set> elements, and after
each preg_replace check for null and handle by returning an empty string or
logging and returning safe output; make these changes inside the
sanitize_svg_setting function to ensure all dangerous patterns are covered.
Contributor
Author
|
These failed checks should pass after this PR is merged. |
bfintal
requested changes
Jul 29, 2026
bfintal
left a comment
bfintal
left a comment
Contributor
There was a problem hiding this comment.
Critical: defaults applied on Edit mount — existing blocks & Design Library affected
Both Heading (#3673) and Icon List (#3674) use the same pattern: a mount useEffect that treats the schema default '' as “unset” and then setAttributes(...). That does not mean “newly added only.”
Most saved blocks omit the attribute when it matched the previous default, so on load it becomes ''. Opening the editor then rewrites the attribute from the admin setting.
Heading (src/block/heading/edit.js) — #3673
useEffect( () => {
if ( attributes.useThemeTextMargins === undefined || attributes.useThemeTextMargins === '' ) {
// ...
setAttributes( { useThemeTextMargins: defaultThemeMargins } )
}
}, [] )- Existing blocks are affected — against the issue requirement and the settings help text (“Existing blocks are not affected”). With the Posts/Non-Posts setting on, opening old content can flip Headings to theme margins.
- Design Library inserts can be rewritten — library Headings that omit
useThemeTextMarginsget the admin default on mount. Designs should stay as authored. - Even with the setting off, writing
falsecan dirty the document (no__unstableMarkNextChangeAsNotPersistent).
Icon List (src/block/icon-list/edit.js + schema) — same class of bug (#3674)
Same mount useEffect after changing icon default from DEFAULT_SVG → '':
useEffect( () => {
if ( attributes.icon === undefined || attributes.icon === '' ) {
setAttributes( { icon: settings.stackable_icon_list_block_default_icon || DEFAULT_SVG } )
}
}, [] )Existing Icon Lists that never serialized icon (because it matched the old schema default) now look “unset” and can pick up the admin default icon (or get rewritten on open). Design Library Icon Lists with empty/omitted icons are at the same risk.
Suggested fix
Apply admin defaults only on new block insert, not on every Edit mount for '' attributes. Do not treat loaded empty defaults from old content / library markup as “apply setting.”
Admin UI / register_setting placement and defaults look fine — the problem is the Edit-time application path for both blocks.
|
Size Change: +1.5 kB (+0.06%) Total Size: 2.63 MB 📦 View Changed
ℹ️ View Unchanged
|
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
2 participants
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.
…ist block
fixes #3673
fixes #3674
Summary by CodeRabbit
Test plan
Settings UI
Heading theme margins (#3673)
useThemeTextMarginsis onIcon List default icon (#3674)
Regression / insertion paths
/heading,/icon list) applies the same defaults as the block inserter panel