Skip to content

fix(security): harden CSS override pipeline and fix frontend wiring - #83

Merged
jackgranatowski merged 4 commits into
mainfrom
claude/plugin-security-review-5pmukt
Jun 28, 2026
Merged

fix(security): harden CSS override pipeline and fix frontend wiring#83
jackgranatowski merged 4 commits into
mainfrom
claude/plugin-security-review-5pmukt

Conversation

@jackgranatowski

@jackgranatowski jackgranatowski commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Close injection path in the override validation pipeline. is_css_safe() now rejects { } ; globally, and valid_font_family()'s var() branch is anchored to a full match (no prefix-only bypass). Together these ensure no validator can return a value containing CSS declaration delimiters.
  • Tighten the flat override map at both read and emit time. The REST endpoint (sanitize_overrides) and the CSS generator (generate_flat_override_declarations) both now enforce --sf-[a-z0-9-]+ so only Slashed-namespaced custom properties can be persisted or rendered.
  • Accept numeric scalar values from JSON. sanitize_overrides() previously dropped int/float values before validation; they now reach validate_override_value().
  • Wire the configurator's saves to the frontend. Tokens saved from the embedded Design Settings SPA (POST /tokens/overridesslashed_overrides option) were never rendered on the public site. get_override_css() / generate_flat_override_declarations() now read and emit that flat map, re-validating every entry at emission time.
  • Sanitize $_POST['rebemer_map'] at point of read. Plugin Check flagged the raw wp_unslash() without an explicit sanitizer. Values are now run through sanitize_text_field() via array_map() before processing; downstream sanitize_rebemer_element_map() still validates with its regex.
  • Fix stale readme claims. Removed references to the "Manual CSS page" (removed in 0.3.8) and a "planned for 0.8.0+" note that predated the now-shipping embedded configurator.

Type

  • fix
  • feat
  • docs
  • chore / tooling

Checklist

  • Conventional Commit messages (feat:, fix:, docs:, …)
  • npm test passes
  • npm run lint passes (stylelint + php -l)
  • npm run verify passes (version metadata in sync)
  • Generated artifacts not hand-edited
  • CHANGELOG.md updated under ## [Unreleased] (for user-facing changes)
  • Built SPA assets committed if editor-app/admin-app source changed

Notes

The configurator wiring fix (flat slashed_overridesget_override_css()) is the only user-visible behaviour change. All other commits are hardening of the sanitization pipeline with no observable effect for clean inputs.

Plugin Check report (2026-06-28): 1 finding resolved (InputNotSanitized on $_POST['rebemer_map']). No remaining findings.


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation for saved map settings so only valid array input is accepted and each entry is cleaned before saving.
    • Tightened acceptance rules for generated CSS custom properties so only the expected --sf- style variables are allowed.

claude added 2 commits June 28, 2026 20:58
The REST write gate already requires --sf-[a-z0-9-]+ (tightened in the
previous commit), but generate_flat_override_declarations() still matched
any --[a-z0-9-]+ key at emission time. Align the two so pre-existing
DB rows with non-namespaced keys are also silently dropped at render.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CE6esDyehWhYtjFK2jiADG
Plugin Check flags $_POST['rebemer_map'] as InputNotSanitized because
wp_unslash() alone is not a sanitizer. Wrap each value with
sanitize_text_field() via array_map so the sniff sees explicit
sanitization at the point of input. Values were already safe downstream
(sanitize_rebemer_element_map() re-validates via sanitize_key + regex)
but the early sanitize makes the intent explicit and silences the warning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CE6esDyehWhYtjFK2jiADG
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jackgranatowski, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 52 minutes and 42 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 027cb038-85e1-4a1c-8912-4a21291a469c

📥 Commits

Reviewing files that changed from the base of the PR and between 555d7ea and 2efd968.

📒 Files selected for processing (3)
  • SLASHED-for-WP/includes/class-bricks-settings-page.php
  • SLASHED-for-WP/includes/class-css-generator.php
  • SLASHED-for-WP/includes/class-rest-controller.php
📝 Walkthrough

Walkthrough

Two single-line hardening changes: handle_save_rebemer() now validates $_POST['rebemer_map'] is an array and sanitizes each entry with sanitize_text_field; generate_flat_override_declarations() restricts flat override map keys to the --sf- prefix pattern.

Changes

Input Sanitization Hardening

Layer / File(s) Summary
POST array validation and CSS key allowlist
SLASHED-for-WP/includes/class-bricks-settings-page.php, SLASHED-for-WP/includes/class-css-generator.php
handle_save_rebemer() adds array type check and per-entry sanitize_text_field on rebemer_map; generate_flat_override_declarations() narrows the key regex from --[a-z0-9-]+ to --sf-[a-z0-9-]+.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • codeslash-dev/SLASHED-Plugins#81: Also hardens flat CSS token override handling in class-css-generator.php, strengthening override value/key validation for the flat map.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main security hardening and frontend wiring fixes in the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/plugin-security-review-5pmukt

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Harden CSS override sanitization and fix override emission namespace
🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

Description

• Sanitize rebemer_map POSTed values at input to satisfy Plugin Check and reduce risk.
• Restrict emitted CSS override keys to the --sf- namespace to prevent rendering legacy/untrusted
 keys.
Diagram

graph TD
A["Admin settings form"] --> B["class-bricks-settings-page.php"] --> C[("WP options")]
D["Slashed overrides map"] --> E["Slashed_Token_Store"] --> C
C --> E --> F["class-css-generator.php"] --> G["Public site CSS"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize override-key allowlist in Slashed_Token_Store
  • ➕ Single source of truth for key validation (read/write/emit all share it).
  • ➕ Reduces risk of future mismatch between REST sanitization and CSS emission.
  • ➖ Requires refactor and touchpoints beyond the immediate fixes.
  • ➖ May be harder to land quickly as a security hardening patch.
2. Introduce a dedicated 'CustomPropertyName' value object
  • ➕ Enforces invariants (namespace + charset) at construction time.
  • ➕ Makes it difficult to accidentally bypass key validation elsewhere.
  • ➖ Adds additional abstraction and boilerplate for a small codebase.
  • ➖ Not idiomatic in many WordPress plugin codebases.

Recommendation: Current approach is appropriate for a targeted hardening fix: sanitize at input for rebemer_map, and align emission-time filtering with the intended --sf- namespace. If override handling grows further, consider centralizing the allowlist in Slashed_Token_Store to prevent future read/write/emit drift.

Files changed (2) +2 / -2

Bug fix (2) +2 / -2
class-bricks-settings-page.phpSanitize 'rebemer_map' POST values at read time +1/-1

Sanitize 'rebemer_map' POST values at read time

• Wraps 'wp_unslash($_POST['rebemer_map'])' with 'array_map('sanitize_text_field', ...)' before further processing. This makes input sanitization explicit at the ingestion point and resolves the Plugin Check 'InputNotSanitized' finding.

SLASHED-for-WP/includes/class-bricks-settings-page.php

class-css-generator.phpRestrict emitted override keys to '--sf-' custom properties +1/-1

Restrict emitted override keys to '--sf-' custom properties

• Tightens the emission-side regex from '^--[a-z0-9-]+$' to '^--sf-[a-z0-9-]+$' when iterating overrides. This aligns rendering with the storage/REST allowlist and prevents legacy/non-namespaced keys from being output as CSS declarations.

SLASHED-for-WP/includes/class-css-generator.php

@qodo-code-review

qodo-code-review Bot commented Jun 28, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. rebemer_map type not guarded ✓ Resolved 🐞 Bug ☼ Reliability
Description
handle_save_rebemer() runs array_map('sanitize_text_field', ...) across $_POST['rebemer_map']
without ensuring each entry is a scalar/string, so malformed nested values can be coerced to empty
strings and then treated as legitimate inputs by the save loop. This can unintentionally clear or
change stored element overrides during a save when submitted data is malformed.
Code

SLASHED-for-WP/includes/class-bricks-settings-page.php[232]

+		$posted_map                  = ( isset( $_POST['rebemer_map'] ) && is_array( $_POST['rebemer_map'] ) ) ? array_map( 'sanitize_text_field', wp_unslash( $_POST['rebemer_map'] ) ) : array();
Relevance

⭐⭐ Medium

No prior reviews on rebemer_map scalar-guarding; closest history focuses on Plugin Check
sanitization, not nested input handling.

PR-#69
PR-#68

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new code sanitizes the entire posted map in one shot, but the later save loop uses empty string
to clear overrides; without per-entry type checks, malformed values can be transformed into empty
strings and trigger clearing logic.

SLASHED-for-WP/includes/class-bricks-settings-page.php[220-255]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handle_save_rebemer()` applies `array_map('sanitize_text_field', ...)` to the whole `rebemer_map` array without per-entry type checks. If a client submits a malformed payload where some entries are arrays/objects, sanitization may collapse them to empty strings, and the subsequent save loop can interpret that as “clear override”, unintentionally changing persisted settings.

## Issue Context
The save loop treats empty string as a signal to `unset( $overrides[$type] )`, so collapsing malformed values to `''` can have side effects.

## Fix Focus Areas
- SLASHED-for-WP/includes/class-bricks-settings-page.php[230-255]

## Suggested fix
- Replace the `array_map('sanitize_text_field', ...)` with a small loop (or an `array_map` callback) that sanitizes only scalar/string values and drops anything else.
- Example approach:
 - Read `$raw_map = wp_unslash( $_POST['rebemer_map'] );`
 - Build `$posted_map = array();`
 - For each `$raw_map[$k] = $v`, if `is_string($v)` (or `is_scalar($v)`), store `sanitize_text_field($v)`; otherwise skip.
This keeps behavior stable for valid form posts while making the handler resilient to malformed input.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Override detection mismatch ✓ Resolved 🐞 Bug ≡ Correctness
Description
Slashed_CSS_Generator::has_overrides() considers any non-empty value in the flat overrides map as
“overrides exist”, but generate_flat_override_declarations() now drops any entry whose key does not
match /^--sf-[a-z0-9-]+$/i. This can cause override injection to run (and potentially add empty
inline CSS) even when no stored overrides will actually be emitted under the stricter name rule.
Code

SLASHED-for-WP/includes/class-css-generator.php[154]

+			if ( ! is_string( $name ) || ! preg_match( '/^--sf-[a-z0-9-]+$/i', $name ) ) {
Relevance

⭐⭐⭐ High

Team accepted tightening override validation/allowlists in CSS generator & overrides pipeline;
likely align has_overrides with stricter key regex.

PR-#81

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The current detection checks values only, while emission now filters by a stricter key regex;
therefore the two functions can disagree about whether any overrides exist.

SLASHED-for-WP/includes/class-css-generator.php[37-66]
SLASHED-for-WP/includes/class-css-generator.php[151-163]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`has_overrides()` scans only override values and ignores the key validation that the emission path enforces. After tightening `generate_flat_override_declarations()` to require `--sf-` names, `has_overrides()` can return true even though the emitter will skip every entry.

## Issue Context
This primarily creates inconsistent internal state ("overrides exist" but nothing is emitted) and can lead to unnecessary work or empty inline style injection.

## Fix Focus Areas
- SLASHED-for-WP/includes/class-css-generator.php[37-66]
- SLASHED-for-WP/includes/class-css-generator.php[151-163]

## Suggested fix
Update `has_overrides()` to apply the same criteria as `generate_flat_override_declarations()` when checking the flat overrides map, e.g.:
- Validate the key with the same `/^--sf-[a-z0-9-]+$/i` regex, and
- Optionally run `validate_override_value()` (or at least ensure the value is non-empty and would pass validation).
This keeps detection consistent with what will actually be emitted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

1. rebemer_map POST handling: replace array_map('sanitize_text_field')
   with an explicit loop that skips non-string entries. array_map would
   coerce nested arrays to empty strings, which the save loop reads as
   "clear this override" — an unintended side-effect on malformed input.

2. has_overrides(): apply the same --sf- key filter as
   generate_flat_override_declarations() when scanning the flat override
   map. Without this the two functions could disagree: has_overrides()
   returning true while the emitter silently skips every entry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CE6esDyehWhYtjFK2jiADG

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@SLASHED-for-WP/includes/class-css-generator.php`:
- Line 154: The emit gate in class-css-generator still allows uppercase
custom-property names because the preg_match in the CSS name check uses a
case-insensitive flag. Update the validation in the relevant method to match the
save gate exactly by removing the /i behavior from the `--sf-[a-z0-9-]+`
pattern, so stored data outside the allowed contract is not rendered.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6f188287-045f-4d3e-a9d0-c373aef5765c

📥 Commits

Reviewing files that changed from the base of the PR and between 29a7aef and 555d7ea.

📒 Files selected for processing (2)
  • SLASHED-for-WP/includes/class-bricks-settings-page.php
  • SLASHED-for-WP/includes/class-css-generator.php

Comment thread SLASHED-for-WP/includes/class-css-generator.php Outdated
…sensitive)

All three occurrences of /^--sf-[a-z0-9-]+$/i — in has_overrides(),
generate_flat_override_declarations(), and sanitize_overrides() — used a
case-insensitive match. CSS custom properties are case-sensitive and SLASHED
tokens are always lowercase, so /i was accepting --SF-* keys it should not.
Remove the flag to enforce exact lowercase matching.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CE6esDyehWhYtjFK2jiADG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants