feat: add block defaults for heading theme margin and icon for icon l…#3676
feat: add block defaults for heading theme margin and icon for icon l…#3676Arukuen wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adds admin-facing defaults for Heading margins and Icon List defaults, initializes those defaults in block edit code, introduces an AdminIconSetting component and styles, updates IconControl behavior, and registers/sanitizes new editor settings in PHP. Changes
Sequence Diagram(s)sequenceDiagram
participant Block as Heading Block Edit
participant Effect as useEffect Hook
participant Select as useSelect (core/editor)
participant Settings as Stackable Settings
participant Attr as setAttributes
Block->>Effect: Component mounts
Effect->>Select: Query current post type
Select-->>Effect: Return post type
Effect->>Settings: Fetch useThemeTextMargins default based on post type
Settings-->>Effect: Return default margin setting
alt useThemeTextMargins is undefined/empty
Effect->>Attr: Set useThemeTextMargins to determined default
Attr-->>Block: Update block attributes
else Already set
Effect-->>Block: No changes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
|
![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 }
| 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.
|
These failed checks should pass after this PR is merged. |
1 participant
…ist block
fixes #3673
fixes #3674
Summary by CodeRabbit
New Features
Chores