Do not output invalid css vars#2296
Conversation
…condition to key check
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant O as output_vars
participant K as css_key_is_valid
participant V as css_value_is_valid
C->>O: Request CSS output generation
O->>K: Validate CSS key
K-->>O: Return key validation result
O->>V: Validate CSS value
V-->>O: Return value validation result
O-->>C: Return processed CSS output
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
classes/helpers/FrmStylesHelper.php (1)
467-495: Good security practice for preventing potential JS injection.This method enhances security by checking CSS values for potentially harmful JavaScript code patterns. The implementation is concise and effective at blocking common attack vectors.
However, consider using a more comprehensive approach in the future:
- You could consider using a regular expression to match a broader range of JavaScript-like patterns
- Consider adding more patterns like
eval(,document., etc. to the invalid substrings list- For increased maintainability, you might want to add comments explaining why each pattern is considered harmful
Example of additional patterns to consider:
$invalid_substrings = array( 'function(', ';userAgent', ';stopPropagation', '{const', 'window[', 'navigator[', 'Array;', 'eval(', // Prevents eval execution 'document.', // Prevents DOM access 'setTimeout(', // Prevents timing attacks 'setInterval(', // Prevents timing attacks );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmStylesHelper.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Spell Check with Typos
classes/helpers/FrmStylesHelper.php
[warning] 462-462:
"abnormaly" should be "abnormally".
🪛 GitHub Actions: Typo Checks
classes/helpers/FrmStylesHelper.php
[warning] 462-462: "abnormaly" should be "abnormally".
[error] 462-462: Process completed with exit code 2.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (3)
classes/helpers/FrmStylesHelper.php (3)
436-436: LGTM: Added 'use_base_font_size' to the excluded variables list.The addition of 'use_base_font_size' to the $remove array ensures this variable won't be output as a CSS variable, which is appropriate since it's used for internal logic rather than styling.
440-440: Enhanced validation for CSS variable keys.The addition of
self::css_key_is_valid($var)check helps prevent potentially invalid CSS keys from being output, improving security.
447-447: Added CSS value validation to prevent harmful code.The addition of
self::css_value_is_valid($settings[$var])check helps prevent potentially harmful JavaScript code from being output as CSS values, enhancing security.
1 participant
Related ticket https://secure.helpscout.net/conversation/2873552726/224955
This update adds some additional validation before adding CSS vars.
I also noticed that the
use_base_font_sizesetting is here, when it isn't used as a CSS var.Confirmed by customer