fix: add guard before sanitizing #9
Conversation
📝 WalkthroughWalkthroughAdds input sanitization and HTML validation across interaction saving and action types, updates versioning and packaging behavior, enhances editor UI to surface restricted notices, refactors license modal async flows, updates documentation and CI triggers, and adds tooling to adjust plugin header for premium builds. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as Editor UI
participant Interaction as Interact_Interaction
participant ActionType as Action Type Handler
participant Sanitizer as Sanitizer Utilities
participant DB as Database
User->>Editor: Save interaction (with actions)
Editor->>Interaction: update(interaction_data)
rect rgb(220,230,255)
Note over Interaction,Sanitizer: Secure & Sanitize Phase
Interaction->>Interaction: secure_interaction_data()
loop per action
Interaction->>Sanitizer: sanitize_interaction_value(value)
Sanitizer-->>Interaction: sanitized value
Interaction->>ActionType: sanitize_data_for_saving(value)
alt dangerous content detected
ActionType->>Sanitizer: validate_html_for_saving(...)
Sanitizer-->>ActionType: WP_Error
ActionType-->>Interaction: WP_Error (early return)
else safe
ActionType-->>Interaction: sanitized value
end
end
end
rect rgb(220,255,230)
Note over Interaction,DB: Persist Phase
Interaction->>DB: save post_content (sanitized)
Interaction->>Interaction: clear_cache()
Interaction-->>Editor: success
end
Editor-->>User: Confirm save
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/freemius.php (1)
604-676: License modal JS refactor looks correct; consider dropping debug logs.The new IIFE-based dialog handling, busy-state toggling, and AJAX flows for activate/deactivate are well-structured and safely use
textContentfor server messages. Only minor nit: theconsole.log('result', result);calls in both handlers can be removed once debugging is done to avoid noisy consoles in production.src/action-types/class-action-type-update-attribute.php (2)
28-35: Align restricted notice wording with actual capability checkThe notice mentions “administrator with unfiltered_html capability”, but the enforcement uses
current_user_can( 'unfiltered_html' ), which may apply to non‑admin roles (custom roles, multisite super admins, etc.). I’d suggest wording this around the capability itself to avoid confusing users and translators.Example tweak:
- 'Some attribute names and values are disallowed unless you are an administrator with unfiltered_html capability for security reasons.' + 'Some attribute names and values are disallowed unless you have the unfiltered_html capability (typically administrator-level users) for security reasons.'
62-87: Dangerous-attribute detection looks good; consider minor refinementsThe helper covers the key high‑risk cases (event handlers and URL/FORM‑related attributes) and normalizes casing, which is good. Two small refinements you may want to consider:
- Anchor the regex with
$to avoid over‑matching attributes that merely start withonbut aren’t event handlers (e.g.,once):'/^on[a-z0-9]+$/'.- Re‑evaluate whether
styleshould be included in$dangerous_attributesgiven the separate CSS sanitization; if you keep it commented out, a short comment clarifying thatsanitize_style_value()is expected to mitigate style‑based payloads would help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.vscode/settings.json(1 hunks)interactions.php(3 hunks)package.json(1 hunks)readme.txt(4 hunks)scripts/package.js(1 hunks)src/action-types/abstract-action-type.php(1 hunks)src/action-types/class-action-type-background-color.php(1 hunks)src/action-types/class-action-type-background-image.php(1 hunks)src/action-types/class-action-type-css-rule.php(1 hunks)src/action-types/class-action-type-display.php(1 hunks)src/action-types/class-action-type-move.php(1 hunks)src/action-types/class-action-type-opacity.php(1 hunks)src/action-types/class-action-type-redirect-to-url.php(1 hunks)src/action-types/class-action-type-rotate.php(1 hunks)src/action-types/class-action-type-scale.php(1 hunks)src/action-types/class-action-type-skew.php(1 hunks)src/action-types/class-action-type-text-color.php(1 hunks)src/action-types/class-action-type-toggle-class.php(1 hunks)src/action-types/class-action-type-toggle-video.php(1 hunks)src/action-types/class-action-type-update-attribute.php(2 hunks)src/action-types/class-action-type-visibility.php(1 hunks)src/class-interaction.php(3 hunks)src/editor/components/timeline/property-control.js(2 hunks)src/editor/editor.php(1 hunks)src/freemius.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
src/action-types/class-action-type-toggle-class.php (15)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-move.php (15)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-rotate.php (12)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (2)
sanitize_data_for_saving(268-271)sanitize_style_value(279-290)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-background-color.php (15)
src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (2)
sanitize_data_for_saving(268-271)sanitize_style_value(279-290)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-visibility.php (3)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)
src/action-types/class-action-type-opacity.php (8)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)
src/action-types/class-action-type-skew.php (2)
src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)
src/action-types/class-action-type-update-attribute.php (8)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (2)
sanitize_data_for_saving(268-271)sanitize_style_value(279-290)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)
src/action-types/class-action-type-css-rule.php (6)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/abstract-action-type.php (2)
sanitize_data_for_saving(268-271)sanitize_style_value(279-290)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)
src/action-types/class-action-type-display.php (3)
src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-redirect-to-url.php (2)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)
src/action-types/abstract-action-type.php (15)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-text-color.php (2)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/abstract-action-type.php (2)
sanitize_data_for_saving(268-271)sanitize_style_value(279-290)
src/class-interaction.php (16)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
src/action-types/class-action-type-toggle-video.php (15)
src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/abstract-action-type.php (1)
sanitize_data_for_saving(268-271)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-96)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(89-133)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)
🔇 Additional comments (20)
package.json (1)
3-3: Version bump to 1.3.2 looks consistent.Matches interactions.php and readme.txt; no other behavioral changes here.
readme.txt (1)
1-8: Readme versioning and 1.3.2 notes align with code changes.The updated stable tag (1.3.2), WP version fields, and the 1.3.2 changelog entries about unfiltered_html restrictions and extra sanitization correctly reflect the implementation changes elsewhere in the plugin.
Also applies to: 152-156
interactions.php (1)
10-11: Version bump and activation version-tracking logic look correct.Updating
INTERACT_VERSIONto1.3.2and callingupdate_option( 'interact_plugin_version', INTERACT_VERSION )unconditionally on activation ensures the stored version reliably matches the running plugin, while migrations still only run when upgrading ($saved_version<INTERACT_VERSION). This is a clean, predictable activation path.Also applies to: 21-22, 31-36
src/editor/editor.php (1)
65-80: ExposingcurrentUserCanUnfilteredHtmlto the editor is appropriate.Using
current_user_can( 'unfiltered_html' )here and passing it through the existing localized config cleanly enables the UI to reflect capability-based restrictions without changing server-side checks. The flag is properly consumed in the editor to gate restricted notices insrc/editor/components/timeline/property-control.js.src/action-types/class-action-type-css-rule.php (1)
44-49: CSS Rule value sanitization is good; the current approach viasanitize_style_value()is appropriate.The new
sanitize_data_for_saving()correctly runs thevaluethroughsanitize_style_value(), which removesexpression(...)andjavascript:patterns, bringing this action in line with other style-related actions and preventing common XSS vectors.The suggestion to additionally apply
safecss_filter_attr()is not recommended. That function is designed specifically for inlinestyle=""attributes and uses a property allowlist that may be overly restrictive for CSS rule values in custom contexts. The current sanitization strategy is sufficient and appropriate for this use case.scripts/package.js (1)
315-323: Rename logic is safe but assumption about premium expectations is unverified.The conditional rename from
interactions.phptoplugin.phpfor premium builds is guarded properly withfs.existsSync()and won't cause runtime errors. However, the justification that "the premium main plugin file and Freemius config expectplugin.phpas the entry point" cannot be verified since thepro__premium_onlydirectory and premium code are not present in this repository. Confirm that the premium distribution actually expectsplugin.phpas the main entry point.src/action-types/class-action-type-skew.php (1)
50-66: Numeric sanitization for skew x/y looks correctThe method safely coerces
x/yto numeric values and nulls out non‑numeric input, matching the scale pattern and preventing unsafe transform values from slipping through.src/action-types/class-action-type-background-image.php (1)
37-42: Background image value is correctly sanitized via shared helperUsing
sanitize_style_value()on theimagefield centralizes stripping ofexpression()/javascript:patterns and keeps the implementation consistent with other style-related action types.src/action-types/class-action-type-visibility.php (1)
44-51: Visibility sanitizer safely whitelists allowed valuesRestricting
visibilitytotoggle/hide/showand defaulting totoggleon invalid input is a good, simple guard that matches the configured options.src/action-types/class-action-type-display.php (1)
59-80: Display value whitelist aligns with configured optionsThe sanitizer’s allowed values list matches the select options and safely normalizes unexpected input to
'block', which is a sensible default for this action type.src/action-types/class-action-type-scale.php (1)
50-66: Scale x/y sanitization is consistent and safeCoercing
x/yto numeric values and nulling non‑numeric input is appropriate here and keeps scale data in a clean, predictable shape.src/action-types/class-action-type-opacity.php (1)
40-49: Opacity sanitizer correctly normalizes numeric inputThe method enforces that
opacityis numeric (or null), which is enough to prevent injection via crafted strings while keeping existing range enforcement in the UI.src/action-types/class-action-type-background-color.php (1)
56-61: Background color is sanitized using shared style helperReusing
sanitize_style_value()for thecolorfield keeps background color in line with other style sanitization and helps strip dangerous patterns from any unexpected input.src/action-types/class-action-type-text-color.php (1)
37-43: LGTM!The sanitization method correctly follows the established pattern from other action types (e.g.,
background-color), properly checking for array structure and sanitizing thecolorfield using the parent'ssanitize_style_value()method.src/action-types/class-action-type-redirect-to-url.php (1)
40-50: LGTM!The URL sanitization using
esc_url()is the appropriate WordPress function for this purpose. The type check and null fallback for non-string values is correctly implemented.src/action-types/class-action-type-toggle-video.php (1)
57-73: LGTM!The sanitization correctly validates the
modefield against the allowed values and properly coercesstartTimeto a numeric value, consistent with the pattern used in other action types.src/action-types/class-action-type-move.php (1)
61-75: LGTM!The implementation efficiently handles all three coordinate fields using a loop, correctly preserving blank values (as noted in the property defaults) while sanitizing numeric inputs. Good use of the comment to clarify the intentional blank preservation.
src/class-interaction.php (2)
92-108: LGTM - Well-structured recursive sanitization.The implementation correctly:
- Short-circuits for users with
unfiltered_htmlcapability- Recursively processes nested arrays
- Applies
wp_kses_post()to strings for safe HTML filteringThe logic order ensures arrays are fully traversed before any string check, preventing unintended behavior.
118-147: Good integration of per-action-type sanitization with proper error propagation.The flow correctly applies generic sanitization first via
sanitize_interaction_value(), then delegates to action-type-specific sanitization viasanitize_data_for_saving(). The early return onWP_Errorensures validation failures (like fromupdate-attributewith dangerous attributes) properly abort the save operation.src/action-types/abstract-action-type.php (1)
279-290: Current implementation appropriately targets historical attack vectors; additional patterns are not recommended by modern CSS injection best practices.The
sanitize_style_value()method correctly removesexpression()andjavascript:patterns, which address the primary legacy IE-specific injection vectors. Modern CSS injection mitigation (2025) prioritizes Content-Security-Policy (CSP) with strictstyle-srccontrols and removal of style sinks over pattern-based blocking. The suggested additions (-moz-binding,behavior,@import) follow an outdated approach:
-moz-binding: Mozilla removed XBL support from Firefox; blocking it is unnecessary in modern browsers.behavior: IE-only property, obsolete in modern contexts.@import: Standard CSS at-rule controlled via CSP, not typically blocked by sanitizers; pattern blocking is not recommended practice.The current implementation is solid for backward-compatibility scenarios. For modern security, rely on CSP enforcement and sanitizing user-controlled style sinks rather than expanding regex-based pattern exclusions.
| }, | ||
| "search.exclude": { | ||
| "**/pro__premium_only": false | ||
| } |
There was a problem hiding this comment.
search.exclude value looks inverted for the described behavior.
In VS Code, "search.exclude": { "**/pro__premium_only": false } explicitly includes that folder in search. If your intention is to hide pro__premium_only from search results, this needs to be true instead:
- "search.exclude": {
- "**/pro__premium_only": false
- }
+ "search.exclude": {
+ "**/pro__premium_only": true
+ }If the goal is indeed to keep it searchable, the current setting is fine.
📝 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.
| }, | |
| "search.exclude": { | |
| "**/pro__premium_only": false | |
| } | |
| }, | |
| "search.exclude": { | |
| "**/pro__premium_only": true | |
| } |
🤖 Prompt for AI Agents
.vscode/settings.json lines 43-46: the search.exclude entry currently sets
"**/pro__premium_only": false which explicitly includes that folder in search;
if the intent is to hide/exclude pro__premium_only from searches change the
value to true so VS Code will exclude it; if the intent is to keep it
searchable, leave as-is.
| public function sanitize_data_for_saving( $value ) { | ||
| // Sanitize action value: ensure $value is an array and attribute/value are strings. | ||
| if ( ! is_array( $value ) ) { | ||
| return new WP_Error( | ||
| 'invalid_structure', | ||
| __( 'Value must be an array containing attribute and value keys.', 'interactions' ) | ||
| ); | ||
| } | ||
|
|
||
| // Sanitize attribute name | ||
| if ( isset( $value['attribute'] ) && is_string( $value['attribute'] ) ) { | ||
| $value['attribute'] = sanitize_key( $value['attribute'] ); | ||
| } | ||
|
|
||
| // Sanitize value if present, and convert to string. | ||
| if ( isset( $value['value'] ) ) { | ||
| $value['value'] = $this->sanitize_style_value( $value['value'] ); | ||
| } | ||
|
|
||
| // Sanitize action field for select option. | ||
| if ( isset( $value['action'] ) ) { | ||
| $allowed_actions = [ 'update', 'remove', 'toggle' ]; | ||
| if ( ! in_array( $value['action'], $allowed_actions, true ) ) { | ||
| $value['action'] = 'update'; | ||
| } | ||
| } | ||
|
|
||
| if ( current_user_can( 'unfiltered_html' ) ) { | ||
| return $value; | ||
| } | ||
|
|
||
| if ( ! empty( $value['attribute'] ) && $this->is_dangerous_attribute( $value['attribute'] ) ) { | ||
| // Only allow dangerous attributes if user has unfiltered_html capability | ||
| return new WP_Error( | ||
| 'invalid_attribute', | ||
| sprintf( | ||
| // Translators: %s is the attribute name. | ||
| __( 'The attribute "%s" requires administrator privileges with unfiltered_html capability to prevent security vulnerabilities.', 'interactions' ), | ||
| esc_html( $value['attribute'] ) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| return $value; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all usages of sanitize_data_for_saving() method
rg -n "sanitize_data_for_saving" -C 3Repository: gambitph/Interactions
Length of output: 10402
🏁 Script executed:
# Find other action-type files to compare implementation patterns
fd "class-action-type-" src/action-types/Repository: gambitph/Interactions
Length of output: 905
🏁 Script executed:
# Check where the sanitize_data_for_saving result is used in the current file
cat -n src/action-types/class-action-type-update-attribute.php | head -150Repository: gambitph/Interactions
Length of output: 5592
🏁 Script executed:
# Search more comprehensively for all calls to sanitize_data_for_saving
rg "sanitize_data_for_saving" -n --type phpRepository: gambitph/Interactions
Length of output: 1893
🏁 Script executed:
# Check what happens in the call site when WP_Error is returned
cat -n src/class-interaction.php | sed -n '120,145p'Repository: gambitph/Interactions
Length of output: 1226
🏁 Script executed:
# Verify if required keys being missing causes actual issues by looking at how the value is used
rg "is_dangerous_attribute\|sanitize_data_for_saving" -A 5 -B 5 src/class-interaction.phpRepository: gambitph/Interactions
Length of output: 47
Structure validation message doesn't match actual checks, and 'remove' action should bypass dangerous attribute restrictions
The method handles WP_Error returns correctly (the call site in src/class-interaction.php:131 checks is_wp_error() before persisting), so that aspect is sound. However, two items merit refinement:
-
Error message misleads about required keys.
The'invalid_structure'message claims the array must contain "attribute and value keys", but the code only checksis_array(). Missing keys don't cause errors; they're only validated withisset()afterward. Relax the message to say "Value must be an array" to match the actual validation logic. -
Allow 'remove' action on dangerous attributes for all users.
Removing a dangerous attribute (e.g.,onclick,href) poses no security risk and could even reduce it. The current logic blocks all operations on dangerous attributes unless the user hasunfiltered_html, butremoveshould be allowed regardless:if ( ! empty( $value['attribute'] ) && $this->is_dangerous_attribute( $value['attribute'] ) ) {
-
// Only allow dangerous attributes if user has unfiltered_html capability
-
// Allow non-privileged users to remove dangerous attributes, but not add/update/toggle them. -
if ( isset( $value['action'] ) && 'remove' === $value['action'] ) { -
return $value; -
} -
}
// Only allow other operations on dangerous attributes if user has unfiltered_html capability. return new WP_Error( 'invalid_attribute', sprintf( __( 'The attribute "%s" requires administrator privileges with unfiltered_html capability to prevent security vulnerabilities.', 'interactions' ), esc_html( $value['attribute'] ) ) );
🤖 Prompt for AI Agents
In src/action-types/class-action-type-update-attribute.php around lines 89 to
133, update the structure validation message and allow the 'remove' action to
bypass dangerous-attribute restriction: change the WP_Error message for
'invalid_structure' to state only "Value must be an array." (remove the
"containing attribute and value keys" wording) and modify the
dangerous-attribute check so that it only returns WP_Error when the attribute is
dangerous AND the action is not 'remove' (i.e., if action === 'remove' do not
block even for users without unfiltered_html; continue to block for other
actions unless current_user_can('unfiltered_html')).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/action-types/class-action-type-rotate.php(1 hunks)src/action-types/class-action-type-toggle-class.php(1 hunks)src/editor/components/timeline/property-control.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/action-types/class-action-type-rotate.php
- src/action-types/class-action-type-toggle-class.php
🔇 Additional comments (2)
src/editor/components/timeline/property-control.js (2)
13-13: LGTM! Imports support the new restriction notice feature.The new imports are necessary for displaying the restriction notice tooltip and checking the user's unfiltered HTML capability.
Also applies to: 21-21
175-175: LGTM! Fix properly preserves computed help content.The change from
if ( property.help )toif ( help )correctly preserves the dynamically computed help content (including the restriction notice added above) when rendering the id-type help message. This addresses the concern raised in the previous review.
| // Add a restriction notice. | ||
| if ( property.restrictedNotice && ! currentUserCanUnfilteredHtml ) { | ||
| help = ( | ||
| <> | ||
| { help }{ ' ' } | ||
| <Tooltip | ||
| delay={ 0 } | ||
| text={ <span style={ { maxWidth: 300, display: 'block' } }>{ property.restrictedNotice.replace( /<[^>]+>/g, '' ) }</span> } | ||
| > | ||
| <span | ||
| style={ { | ||
| color: '#FFA500', | ||
| borderBottom: '1px dotted rgb(208, 135, 0)', | ||
| cursor: 'help', | ||
| } } | ||
| > | ||
| { __( 'Has restrictions', 'interactions' ) } | ||
| </span> | ||
| </Tooltip> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add type guard before calling .replace() on restrictedNotice.
The code calls .replace() directly on property.restrictedNotice without verifying it's a string. If restrictedNotice is defined but not a string (e.g., an object, number, or array), this will throw a runtime error.
Apply this diff to add a type check:
- if ( property.restrictedNotice && ! currentUserCanUnfilteredHtml ) {
+ if ( typeof property.restrictedNotice === 'string' && property.restrictedNotice && ! currentUserCanUnfilteredHtml ) {
help = (
<>
{ help }{ ' ' }
<Tooltip
delay={ 0 }
- text={ <span style={ { maxWidth: 300, display: 'block' } }>{ property.restrictedNotice.replace( /<[^>]+>/g, '' ) }</span> }
+ text={ <span style={ { maxWidth: 300, display: 'block' } }>{ property.restrictedNotice.replace( /<[^>]+>/g, '' ) }</span> }
>Additional observations:
- The HTML tag stripping is appropriate for security, though HTML entities (like
<or&) won't be decoded and will display as-is in the tooltip. This is acceptable for developer-defined restriction messages. - The hardcoded inline styles (
'#FFA500','rgb(208, 135, 0)',300) could be extracted to constants or CSS classes for better maintainability.
📝 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.
| // Add a restriction notice. | |
| if ( property.restrictedNotice && ! currentUserCanUnfilteredHtml ) { | |
| help = ( | |
| <> | |
| { help }{ ' ' } | |
| <Tooltip | |
| delay={ 0 } | |
| text={ <span style={ { maxWidth: 300, display: 'block' } }>{ property.restrictedNotice.replace( /<[^>]+>/g, '' ) }</span> } | |
| > | |
| <span | |
| style={ { | |
| color: '#FFA500', | |
| borderBottom: '1px dotted rgb(208, 135, 0)', | |
| cursor: 'help', | |
| } } | |
| > | |
| { __( 'Has restrictions', 'interactions' ) } | |
| </span> | |
| </Tooltip> | |
| </> | |
| ) | |
| } | |
| // Add a restriction notice. | |
| if ( typeof property.restrictedNotice === 'string' && property.restrictedNotice && ! currentUserCanUnfilteredHtml ) { | |
| help = ( | |
| <> | |
| { help }{ ' ' } | |
| <Tooltip | |
| delay={ 0 } | |
| text={ <span style={ { maxWidth: 300, display: 'block' } }>{ property.restrictedNotice.replace( /<[^>]+>/g, '' ) }</span> } | |
| > | |
| <span | |
| style={ { | |
| color: '#FFA500', | |
| borderBottom: '1px dotted rgb(208, 135, 0)', | |
| cursor: 'help', | |
| } } | |
| > | |
| { __( 'Has restrictions', 'interactions' ) } | |
| </span> | |
| </Tooltip> | |
| </> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/editor/components/timeline/property-control.js around lines 53-74, the
code calls property.restrictedNotice.replace(...) without ensuring
restrictedNotice is a string; add a type guard (e.g., check typeof
property.restrictedNotice === 'string') before calling .replace and fall back to
an empty string (or String(property.restrictedNotice) sanitized) if it isn't,
then use that safe value in the Tooltip text; keep the existing regex for
stripping tags.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/action-types/class-action-type-update-attribute.php (1)
93-103: Allow 'remove' action on dangerous attributes for all users.The past review comment still applies: removing a dangerous attribute (e.g.,
onclick,href) poses no security risk and could even improve it. The current logic blocks all operations on dangerous attributes, butremoveshould be allowed regardless ofunfiltered_htmlcapability.Suggested fix:
if ( ! empty( $value['attribute'] ) && $this->is_dangerous_attribute( $value['attribute'] ) ) { - // Only allow dangerous attributes if user has unfiltered_html capability + // Allow non-privileged users to remove dangerous attributes, but not add/update/toggle them. + if ( isset( $value['action'] ) && 'remove' === $value['action'] ) { + return $value; + } + + // Only allow other operations on dangerous attributes if user has unfiltered_html capability. return new WP_Error(
🧹 Nitpick comments (2)
src/action-types/abstract-action-type.php (2)
343-354: Consider addingsrcdocanddatato the dangerous attributes list.The current list covers common vectors, but these additional attributes can execute scripts:
srcdoc- allows embedding full HTML documents in iframesdata- used by<object>elements to load external contentSuggested addition:
$dangerous_attributes = [ 'href', 'src', + 'srcdoc', + 'data', 'action', 'formaction', 'form', 'formmethod', 'formtarget', ];
399-405: Broaden thedata:protocol check.The current check
data:\s*text\/is too narrow. Dangerous payloads can use variations likedata:text/html,data:application/javascript, or base64-encoded variants. Consider blocking alldata:URIs or at minimum expanding the pattern.Suggested fix:
-if ( preg_match( '/javascript:\s*/i', $html ) || preg_match( '/data:\s*text\//i', $html ) ) { +if ( preg_match( '/javascript:\s*/i', $html ) || preg_match( '/data:\s*/i', $html ) || preg_match( '/vbscript:\s*/i', $html ) ) { return new WP_Error( 'invalid_protocol', - __( 'The HTML contains disallowed protocols (javascript: or data:).', 'interactions' ) + __( 'The HTML contains disallowed protocols (javascript:, data:, or vbscript:).', 'interactions' ) ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/action-types/abstract-action-type.php(1 hunks)src/action-types/class-action-type-update-attribute.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/action-types/abstract-action-type.php (15)
src/action-types/class-action-type-update-attribute.php (1)
sanitize_data_for_saving(62-106)src/action-types/class-action-type-rotate.php (1)
sanitize_data_for_saving(67-98)src/action-types/class-action-type-background-color.php (1)
sanitize_data_for_saving(56-61)src/action-types/class-action-type-toggle-class.php (1)
sanitize_data_for_saving(54-67)src/action-types/class-action-type-background-image.php (1)
sanitize_data_for_saving(37-42)src/action-types/class-action-type-css-rule.php (1)
sanitize_data_for_saving(44-49)src/action-types/class-action-type-display.php (1)
sanitize_data_for_saving(59-80)src/action-types/class-action-type-move.php (1)
sanitize_data_for_saving(62-75)src/action-types/class-action-type-opacity.php (1)
sanitize_data_for_saving(40-49)src/action-types/class-action-type-redirect-to-url.php (1)
sanitize_data_for_saving(41-50)src/action-types/class-action-type-scale.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-text-color.php (1)
sanitize_data_for_saving(38-43)src/action-types/class-action-type-skew.php (1)
sanitize_data_for_saving(50-66)src/action-types/class-action-type-visibility.php (1)
sanitize_data_for_saving(44-52)src/action-types/class-action-type-toggle-video.php (1)
sanitize_data_for_saving(58-73)
🔇 Additional comments (6)
src/action-types/class-action-type-update-attribute.php (2)
33-33: LGTM!The
restrictedNoticeprovides good UX by informing users about security restrictions upfront in the editor.
62-106: Approve the sanitization implementation with a minor note.The sanitization flow is well-structured: array validation, field-level sanitization, allowlist for action types, and capability-gated dangerous attribute restrictions. The implementation correctly returns
WP_Errorfor security violations, which is handled by the caller.src/action-types/abstract-action-type.php (4)
268-271: LGTM!Good template method pattern allowing child classes to override with specific sanitization logic while providing a safe default.
279-290: LGTM with optional enhancement.The recursive regex for
expression(...)handles nested parentheses well. Consider also strippingvbscript:for completeness, though it's primarily a legacy IE concern.
363-408: Note on regex-based HTML validation.Regex-based HTML parsing can potentially be bypassed with malformed markup or encoding tricks. This is a known limitation, but given that:
- This serves as defense-in-depth behind the
unfiltered_htmlcapability check- WordPress's
wp_ksesprovides additional sanitization at output timeThe approach is reasonable for this use case. For higher-risk scenarios, consider using
DOMDocumentfor more robust parsing.
299-321: LGTM!Comprehensive list of dangerous tags covering script execution, external content embedding, and page behavior modification vectors.
…nteractions into fix/7-extend-security
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/package.js (1)
295-321: Well-structured version suffix logic!The function correctly handles premium vs. free builds, includes proper existence checks, and prevents duplicate suffix application. The regex pattern appropriately targets WordPress plugin header format.
One optional enhancement: consider validating the suffix format to ensure it doesn't contain problematic characters (spaces, special chars) that could break version string conventions.
♻️ Optional: Add suffix validation
function updatePluginHeaderVersion( buildDir, suffix ) { if ( ! suffix ) { return } + // Validate suffix contains only safe characters + if ( ! /^[a-zA-Z0-9_-]+$/.test( suffix ) ) { + console.warn( `⚠️ Suffix contains potentially problematic characters: ${ suffix }` ) + } const pluginFileName = IS_PREMIUM_BUILD ? 'plugin.php' : 'interactions.php'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/package.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
scripts/package.js (3)
343-351: LGTM! Idempotent premium build rename.The existence check ensures this operation is safe to run multiple times, and the conditional scoping to premium builds is correct.
363-364: Correct placement for version update.The call occurs after the premium rename and security files, ensuring the version suffix is applied to the correct file with all content in place.
4-4: Update archiver to latest version (7.0.1) for currency, though this will not resolve the detected security vulnerabilities which come from other dependencies.The project has 16 known vulnerabilities (6 high, 10 moderate), but they originate from
@babel/runtime,js-yaml,qs,webpack-dev-server,playwright, andnode-forge—not from archiver itself. Archiver@6.0.1 has no known security vulnerabilities and is not deprecated. While upgrading to 7.0.1 is recommended for general dependency maintenance, addressing the security issues requires updating the vulnerable dependencies listed above.Likely an incorrect or invalid review comment.
fixes #7
Summary by CodeRabbit
New Features
Documentation
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.