From 4b846961530f87d891d254a388e15fce7fab5721 Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Wed, 6 May 2026 14:58:22 +0200 Subject: [PATCH 1/5] Add /theme-settings REST endpoint and CBT_Theme_Settings_Save service Adds the server-side foundation for the upcoming Edit Theme Settings modal. The new endpoint accepts a partial-theme.json payload, validates its shape, deep-merges it into the active theme's theme.json, and writes the result. Payload shape: - settings: deep-merged into existing settings (creates missing parent keys; empty arrays clear leaves; lists replace wholesale) - customTemplates / templateParts: array-replace top-level - removedShadowDefaults: operational key reified into settings.shadow.defaultPresets: false plus the kept WP core shadow defaults re-registered as customs (idempotent; preserves user-defined presets whose slugs do not collide) Capability gate: edit_theme_options (matches existing endpoints). Part of #836. Closes #837 once merged. --- includes/class-create-block-theme-api.php | 34 ++ includes/create-theme/theme-settings-save.php | 294 ++++++++++++++++++ tests/test-theme-settings-save.php | 246 +++++++++++++++ 3 files changed, 574 insertions(+) create mode 100644 includes/create-theme/theme-settings-save.php create mode 100644 tests/test-theme-settings-save.php diff --git a/includes/class-create-block-theme-api.php b/includes/class-create-block-theme-api.php index 12c42522..b988452d 100644 --- a/includes/class-create-block-theme-api.php +++ b/includes/class-create-block-theme-api.php @@ -13,6 +13,7 @@ require_once __DIR__ . '/create-theme/theme-readme.php'; require_once __DIR__ . '/create-theme/theme-fonts.php'; require_once __DIR__ . '/create-theme/theme-create.php'; +require_once __DIR__ . '/create-theme/theme-settings-save.php'; /** * The api functionality of the plugin leveraged by the site editor UI. @@ -68,6 +69,17 @@ public function register_rest_routes() { }, ) ); + register_rest_route( + 'create-block-theme/v1', + '/theme-settings', + array( + 'methods' => 'POST', + 'callback' => array( $this, 'rest_save_theme_settings' ), + 'permission_callback' => function () { + return current_user_can( 'edit_theme_options' ); + }, + ) + ); register_rest_route( 'create-block-theme/v1', '/clone', @@ -394,6 +406,28 @@ function rest_save_theme( $request ) { ); } + /** + * Persist a partial theme.json payload from the Edit Theme Settings modal. + * + * Accepts the keys `settings`, `customTemplates`, `templateParts`, and + * `removedShadowDefaults`. Only keys present in the payload are written; + * missing keys leave the existing theme.json untouched. + */ + function rest_save_theme_settings( $request ) { + $result = CBT_Theme_Settings_Save::run( $request->get_json_params() ); + + if ( is_wp_error( $result ) ) { + return $result; + } + + return new WP_REST_Response( + array( + 'status' => 'SUCCESS', + 'theme_json' => $result, + ) + ); + } + /** * Get a list of all the font families used in the theme. * diff --git a/includes/create-theme/theme-settings-save.php b/includes/create-theme/theme-settings-save.php new file mode 100644 index 00000000..c4d12309 --- /dev/null +++ b/includes/create-theme/theme-settings-save.php @@ -0,0 +1,294 @@ + $value ) { + if ( ! in_array( $key, self::ALLOWED_TOP_LEVEL_KEYS, true ) ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: %s: unknown payload key */ + __( 'Unknown top-level key: %s', 'create-block-theme' ), + $key + ), + array( 'status' => 400 ) + ); + } + } + + if ( isset( $payload['settings'] ) && ! is_array( $payload['settings'] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( '"settings" must be an object.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } + + foreach ( array( 'customTemplates', 'templateParts' ) as $list_key ) { + if ( ! isset( $payload[ $list_key ] ) ) { + continue; + } + if ( ! is_array( $payload[ $list_key ] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: %s: payload key */ + __( '"%s" must be an array.', 'create-block-theme' ), + $list_key + ), + array( 'status' => 400 ) + ); + } + foreach ( $payload[ $list_key ] as $entry ) { + if ( ! is_array( $entry ) ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: %s: payload key */ + __( 'Entries of "%s" must be objects.', 'create-block-theme' ), + $list_key + ), + array( 'status' => 400 ) + ); + } + } + } + + if ( isset( $payload['removedShadowDefaults'] ) ) { + if ( ! is_array( $payload['removedShadowDefaults'] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( '"removedShadowDefaults" must be an array of slugs.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } + foreach ( $payload['removedShadowDefaults'] as $slug ) { + if ( ! is_string( $slug ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( 'Entries of "removedShadowDefaults" must be strings.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } + } + } + + return $payload; + } + + /** + * Recursively sanitize string leaves in the payload. Booleans and numbers + * pass through; arrays recurse; strings are run through `sanitize_text_field`. + * + * @param mixed $value + * @return mixed + */ + public static function sanitize( $value ) { + if ( is_array( $value ) ) { + $out = array(); + foreach ( $value as $k => $v ) { + $out[ $k ] = self::sanitize( $v ); + } + return $out; + } + if ( is_string( $value ) ) { + return sanitize_text_field( $value ); + } + return $value; + } + + /** + * Deep-merge $payload into $current. At every level, associative-array + * values are merged recursively; lists, scalars, and empty arrays replace + * the existing value. Missing parent keys are created. + * + * The operational key `removedShadowDefaults` is dropped here — it's + * handled separately by `reify_shadow_removals()`. + * + * @param array $current + * @param array $payload + * @return array + */ + public static function merge( array $current, array $payload ) { + $result = $current; + foreach ( $payload as $key => $value ) { + if ( 'removedShadowDefaults' === $key ) { + continue; + } + if ( + is_array( $value ) && + ! self::is_list( $value ) && + isset( $result[ $key ] ) && + is_array( $result[ $key ] ) && + ! self::is_list( $result[ $key ] ) + ) { + $result[ $key ] = self::merge( $result[ $key ], $value ); + } else { + $result[ $key ] = $value; + } + } + return $result; + } + + /** + * Translate `removedShadowDefaults: [slug, ...]` into the theme.json shape: + * `settings.shadow.defaultPresets: false` plus the kept core shadow + * defaults re-registered under `settings.shadow.presets`. User-defined + * presets (slugs that are not core defaults) are preserved. + * + * Idempotent: running the same removal twice produces the same output. + * + * @param array $merged Merged theme.json (post `merge()`). + * @param string[] $removed_slugs Slugs of core defaults to remove. + * @return array + */ + public static function reify_shadow_removals( array $merged, array $removed_slugs ) { + $core_presets = self::get_core_shadow_presets(); + if ( empty( $core_presets ) ) { + return $merged; + } + + $core_slugs = array_column( $core_presets, 'slug' ); + $kept = array_values( + array_filter( + $core_presets, + static function ( $preset ) use ( $removed_slugs ) { + return ! in_array( $preset['slug'], $removed_slugs, true ); + } + ) + ); + + $existing = isset( $merged['settings']['shadow']['presets'] ) && is_array( $merged['settings']['shadow']['presets'] ) + ? $merged['settings']['shadow']['presets'] + : array(); + + // Strip any existing presets whose slug matches a core default slug — + // we re-register the kept defaults below, so this prevents duplication. + $user_customs = array_values( + array_filter( + $existing, + static function ( $preset ) use ( $core_slugs ) { + return isset( $preset['slug'] ) && ! in_array( $preset['slug'], $core_slugs, true ); + } + ) + ); + + if ( ! isset( $merged['settings'] ) || ! is_array( $merged['settings'] ) ) { + $merged['settings'] = array(); + } + if ( ! isset( $merged['settings']['shadow'] ) || ! is_array( $merged['settings']['shadow'] ) ) { + $merged['settings']['shadow'] = array(); + } + + $merged['settings']['shadow']['defaultPresets'] = false; + $merged['settings']['shadow']['presets'] = array_merge( $user_customs, $kept ); + + return $merged; + } + + /** + * Fetch the core shadow defaults via `wp_get_global_settings`. Returns an + * empty array if core does not expose shadow defaults (older WP versions + * or unusual environments) — in which case shadow reification is a no-op + * and the caller's `defaultPresets` flag round-trips literally. + * + * `wp_get_global_settings` returns presets keyed by origin + * (`['default' => [...]]`); we extract the `default` slot. + * + * @return array + */ + public static function get_core_shadow_presets() { + if ( ! function_exists( 'wp_get_global_settings' ) ) { + return array(); + } + $presets = wp_get_global_settings( array( 'shadow', 'presets' ) ); + if ( is_array( $presets ) && isset( $presets['default'] ) && is_array( $presets['default'] ) ) { + return $presets['default']; + } + return array(); + } + + /** + * Detect whether an array is a list (sequential integer keys starting at 0). + * Mirrors PHP 8.1+ `array_is_list()`. + * + * @param array $arr + * @return bool + */ + private static function is_list( array $arr ) { + if ( function_exists( 'array_is_list' ) ) { + return array_is_list( $arr ); + } + if ( array() === $arr ) { + return true; + } + $expected = 0; + foreach ( $arr as $key => $_v ) { + if ( $key !== $expected++ ) { + return false; + } + } + return true; + } +} diff --git a/tests/test-theme-settings-save.php b/tests/test-theme-settings-save.php new file mode 100644 index 00000000..c10b2785 --- /dev/null +++ b/tests/test-theme-settings-save.php @@ -0,0 +1,246 @@ + array( 'color' => array( 'custom' => true ) ), + 'customTemplates' => array(), + 'templateParts' => array(), + 'removedShadowDefaults' => array( 'natural' ), + ); + $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); + } + + public function test_validate_rejects_unknown_top_level_key() { + $result = CBT_Theme_Settings_Save::validate( array( 'unexpected' => 1 ) ); + $this->assertWPError( $result ); + $this->assertSame( 'cbt_invalid_payload', $result->get_error_code() ); + $this->assertSame( 400, $result->get_error_data()['status'] ); + } + + public function test_validate_rejects_settings_not_array() { + $result = CBT_Theme_Settings_Save::validate( array( 'settings' => 'oops' ) ); + $this->assertWPError( $result ); + } + + public function test_validate_rejects_custom_templates_entries_not_objects() { + $result = CBT_Theme_Settings_Save::validate( + array( 'customTemplates' => array( 'not-an-object' ) ) + ); + $this->assertWPError( $result ); + } + + public function test_validate_rejects_removed_shadow_defaults_entries_not_strings() { + $result = CBT_Theme_Settings_Save::validate( + array( 'removedShadowDefaults' => array( 1 ) ) + ); + $this->assertWPError( $result ); + } + + /* ---------------------------------------------------------------- * + * sanitize() + * ---------------------------------------------------------------- */ + + public function test_sanitize_strips_tags_from_string_values() { + $out = CBT_Theme_Settings_Save::sanitize( + array( + 'settings' => array( + 'color' => array( + 'palette' => array( + array( + 'slug' => 'brand', + 'name' => 'Brand', + 'color' => '#fff', + ), + ), + ), + ), + ) + ); + $this->assertSame( 'Brand', $out['settings']['color']['palette'][0]['name'] ); + } + + public function test_sanitize_preserves_booleans_and_numbers() { + $out = CBT_Theme_Settings_Save::sanitize( + array( + 'settings' => array( + 'color' => array( + 'custom' => false, + 'defaultPalette' => true, + ), + 'spacing' => array( 'padding' => 16 ), + ), + ) + ); + $this->assertSame( false, $out['settings']['color']['custom'] ); + $this->assertSame( true, $out['settings']['color']['defaultPalette'] ); + $this->assertSame( 16, $out['settings']['spacing']['padding'] ); + } + + public function test_sanitize_preserves_camel_case_keys() { + $out = CBT_Theme_Settings_Save::sanitize( + array( 'settings' => array( 'color' => array( 'defaultPalette' => true ) ) ) + ); + $this->assertArrayHasKey( 'defaultPalette', $out['settings']['color'] ); + } + + /* ---------------------------------------------------------------- * + * merge() + * ---------------------------------------------------------------- */ + + public function test_merge_partial_settings_color_palette_leaves_other_keys_untouched() { + $current = array( + 'settings' => array( + 'color' => array( + 'palette' => array( + array( + 'slug' => 'old', + 'color' => '#000', + ), + ), + 'gradients' => array( + array( + 'slug' => 'g1', + 'gradient' => 'linear-gradient(red,blue)', + ), + ), + 'custom' => true, + ), + 'spacing' => array( 'padding' => true ), + ), + ); + $payload = array( + 'settings' => array( + 'color' => array( + 'palette' => array( + array( + 'slug' => 'new', + 'color' => '#fff', + ), + ), + ), + ), + ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( 'new', $out['settings']['color']['palette'][0]['slug'] ); + $this->assertSame( 'g1', $out['settings']['color']['gradients'][0]['slug'] ); + $this->assertTrue( $out['settings']['color']['custom'] ); + $this->assertTrue( $out['settings']['spacing']['padding'] ); + } + + public function test_merge_empty_palette_array_clears_palette() { + $current = array( + 'settings' => array( + 'color' => array( 'palette' => array( array( 'slug' => 'a' ) ) ), + ), + ); + $payload = array( 'settings' => array( 'color' => array( 'palette' => array() ) ) ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( array(), $out['settings']['color']['palette'] ); + } + + public function test_merge_creates_missing_parent_keys() { + $current = array(); // theme.json without any settings + $payload = array( + 'settings' => array( + 'color' => array( + 'palette' => array( + array( + 'slug' => 'brand', + 'color' => '#fff', + ), + ), + ), + ), + ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( 'brand', $out['settings']['color']['palette'][0]['slug'] ); + } + + public function test_merge_replaces_template_parts_array_wholesale() { + $current = array( + 'templateParts' => array( + array( + 'name' => 'header', + 'area' => 'header', + ), + array( + 'name' => 'footer', + 'area' => 'footer', + ), + ), + ); + $payload = array( + 'templateParts' => array( + array( + 'name' => 'sidebar', + 'area' => 'uncategorized', + ), + ), + ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertCount( 1, $out['templateParts'] ); + $this->assertSame( 'sidebar', $out['templateParts'][0]['name'] ); + } + + public function test_merge_does_not_emit_removed_shadow_defaults_key() { + $out = CBT_Theme_Settings_Save::merge( + array(), + array( 'removedShadowDefaults' => array( 'natural' ) ) + ); + $this->assertArrayNotHasKey( 'removedShadowDefaults', $out ); + } + + /* ---------------------------------------------------------------- * + * reify_shadow_removals() + * ---------------------------------------------------------------- */ + + public function test_reify_shadow_removals_sets_default_presets_false_and_re_registers_kept() { + $core_presets = CBT_Theme_Settings_Save::get_core_shadow_presets(); + if ( empty( $core_presets ) ) { + $this->markTestSkipped( 'WP core does not expose shadow defaults in this environment.' ); + } + $out = CBT_Theme_Settings_Save::reify_shadow_removals( array(), array( $core_presets[0]['slug'] ) ); + $this->assertFalse( $out['settings']['shadow']['defaultPresets'] ); + $kept_slugs = array_column( $out['settings']['shadow']['presets'], 'slug' ); + $this->assertNotContains( $core_presets[0]['slug'], $kept_slugs ); + $this->assertCount( count( $core_presets ) - 1, $kept_slugs ); + } + + public function test_reify_shadow_removals_is_idempotent() { + $core_presets = CBT_Theme_Settings_Save::get_core_shadow_presets(); + if ( empty( $core_presets ) ) { + $this->markTestSkipped( 'WP core does not expose shadow defaults in this environment.' ); + } + $once = CBT_Theme_Settings_Save::reify_shadow_removals( array(), array( $core_presets[0]['slug'] ) ); + $twice = CBT_Theme_Settings_Save::reify_shadow_removals( $once, array( $core_presets[0]['slug'] ) ); + $this->assertSame( $once, $twice ); + } + + public function test_reify_shadow_removals_preserves_user_custom_presets() { + $core_presets = CBT_Theme_Settings_Save::get_core_shadow_presets(); + if ( empty( $core_presets ) ) { + $this->markTestSkipped( 'WP core does not expose shadow defaults in this environment.' ); + } + $user_custom = array( + 'slug' => 'my-custom-shadow', + 'name' => 'Mine', + 'shadow' => '0 0 5px #000', + ); + $current = array( + 'settings' => array( + 'shadow' => array( 'presets' => array( $user_custom ) ), + ), + ); + $out = CBT_Theme_Settings_Save::reify_shadow_removals( $current, array( $core_presets[0]['slug'] ) ); + $slugs = array_column( $out['settings']['shadow']['presets'], 'slug' ); + $this->assertContains( 'my-custom-shadow', $slugs ); + } +} From edf4431a44b54fd08f31f373cbd8b3a8329b4482 Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Thu, 7 May 2026 11:20:11 +0200 Subject: [PATCH 2/5] Address review: context-aware sanitize, slug validation, write-failure handling - sanitize() is now context-aware: text fields (name/title/label/description) go through sanitize_text_field; slug fields (slug/area, postTypes entries) through sanitize_key; everything else (CSS values like shadow, color, gradient) through wp_kses_no_null. Fixes silent corruption of multi-segment shadow values, gradients, and other CSS that contained whitespace or commas. - validate() now rejects malformed slugs in removedShadowDefaults and the name field on customTemplates/templateParts (must survive sanitize_key unchanged and be non-empty). Catches client-side typos and bad input before the merge step. - run() now returns WP_Error('cbt_write_failed', 500) when the theme.json write fails. Previously a write failure (permissions, disk full, read-only filesystem) would silently swallow the error and return SUCCESS with the merged-but-not-persisted payload. write_theme_file_contents() now returns bool to support this; @ silences a permission-denied warning so the boolean check is the source of truth. Tests: 7 new cases (3 sanitization round-trips for shadow/gradient/postTypes, 3 validation rejections for invalid slugs, 1 integration test that proves the write-failure path returns WP_Error). 23/23 in the new suite, 117/117 overall, lint clean. --- includes/create-theme/resolver_additions.php | 7 +- includes/create-theme/theme-settings-save.php | 96 ++++++++++- tests/test-theme-settings-save.php | 149 ++++++++++++++++++ 3 files changed, 244 insertions(+), 8 deletions(-) diff --git a/includes/create-theme/resolver_additions.php b/includes/create-theme/resolver_additions.php index 21117cf9..b5ee4675 100644 --- a/includes/create-theme/resolver_additions.php +++ b/includes/create-theme/resolver_additions.php @@ -135,8 +135,13 @@ public static function get_theme_file_contents() { public static function write_theme_file_contents( $theme_json_data ) { $theme_json = wp_json_encode( $theme_json_data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE ); - file_put_contents( static::get_file_path_from_theme( 'theme.json' ), $theme_json ); + // Suppress warnings so a permission/disk error returns false cleanly + // rather than emitting a PHP warning that may be promoted to an + // exception. Callers must check the boolean return value. + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + $bytes = @file_put_contents( static::get_file_path_from_theme( 'theme.json' ), $theme_json ); static::clean_cached_data(); + return false !== $bytes; } public static function write_user_settings( $user_settings ) { diff --git a/includes/create-theme/theme-settings-save.php b/includes/create-theme/theme-settings-save.php index c4d12309..92639e4e 100644 --- a/includes/create-theme/theme-settings-save.php +++ b/includes/create-theme/theme-settings-save.php @@ -23,6 +23,23 @@ class CBT_Theme_Settings_Save { 'removedShadowDefaults', ); + /** + * Keys whose string values are user-facing labels and benefit from + * `sanitize_text_field()` (HTML stripping, whitespace normalization). + */ + const TEXT_FIELD_KEYS = array( 'name', 'title', 'label', 'description' ); + + /** + * Keys whose string values are slug-formatted and pass through `sanitize_key()`. + * Used for both leaf scalars and entries of slug-list keys (e.g., `postTypes`). + */ + const SLUG_FIELD_KEYS = array( 'slug', 'area' ); + + /** + * Keys whose values are lists of slug strings (the entries — not the key — are slugs). + */ + const SLUG_LIST_KEYS = array( 'postTypes' ); + /** * Persist a partial theme.json payload to the active theme's theme.json. * @@ -49,7 +66,14 @@ public static function run( array $payload ) { $merged = self::reify_shadow_removals( $merged, $sanitized['removedShadowDefaults'] ); } - CBT_Theme_JSON_Resolver::write_theme_file_contents( $merged ); + $wrote = CBT_Theme_JSON_Resolver::write_theme_file_contents( $merged ); + if ( true !== $wrote ) { + return new WP_Error( + 'cbt_write_failed', + __( 'Failed to write theme.json. Check filesystem permissions on the active theme directory.', 'create-block-theme' ), + array( 'status' => 500 ) + ); + } return $merged; } @@ -131,6 +155,42 @@ public static function validate( array $payload ) { array( 'status' => 400 ) ); } + if ( sanitize_key( $slug ) !== $slug || '' === $slug ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: %s: invalid slug */ + __( 'Invalid shadow slug "%s". Slugs must be lowercase alphanumeric with dashes or underscores.', 'create-block-theme' ), + $slug + ), + array( 'status' => 400 ) + ); + } + } + } + + // Validate `name` slugs on customTemplates and templateParts entries + // (the `name` field is used as the file-system slug, e.g. templates/.html). + foreach ( array( 'customTemplates', 'templateParts' ) as $list_key ) { + if ( ! isset( $payload[ $list_key ] ) ) { + continue; + } + foreach ( $payload[ $list_key ] as $entry ) { + if ( ! isset( $entry['name'] ) ) { + continue; + } + if ( ! is_string( $entry['name'] ) || sanitize_key( $entry['name'] ) !== $entry['name'] || '' === $entry['name'] ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: 1: list key, 2: invalid name */ + __( 'Invalid "%1$s" entry name "%2$s". Names must be lowercase alphanumeric with dashes or underscores.', 'create-block-theme' ), + $list_key, + is_scalar( $entry['name'] ) ? (string) $entry['name'] : '?' + ), + array( 'status' => 400 ) + ); + } } } @@ -138,22 +198,44 @@ public static function validate( array $payload ) { } /** - * Recursively sanitize string leaves in the payload. Booleans and numbers - * pass through; arrays recurse; strings are run through `sanitize_text_field`. + * Recursively sanitize the payload, applying a context-aware sanitizer per + * leaf based on the parent key. + * + * - `name`/`title`/`label`/`description` → `sanitize_text_field()` (HTML-stripped labels). + * - `slug`/`area` → `sanitize_key()` (already validated to be slug-safe; this is belt-and-braces). + * - Entries inside `postTypes` lists → `sanitize_key()`. + * - Everything else (CSS values like `shadow`, `color`, `gradient`, `fontFamily`) + * → `wp_kses_no_null()` (strip NULL bytes only; preserve whitespace, parens, commas). * - * @param mixed $value + * Booleans and numbers pass through unchanged. + * + * @param mixed $value Value to sanitize. + * @param string $parent_key The key under which `$value` lives (empty for the root). + * For list entries, the list's key name. * @return mixed */ - public static function sanitize( $value ) { + public static function sanitize( $value, $parent_key = '' ) { if ( is_array( $value ) ) { $out = array(); foreach ( $value as $k => $v ) { - $out[ $k ] = self::sanitize( $v ); + // Associative-key children inherit their own key as context. + // List-entry children inherit the list's key as context. + $child_context = is_string( $k ) ? $k : $parent_key; + $out[ $k ] = self::sanitize( $v, $child_context ); } return $out; } if ( is_string( $value ) ) { - return sanitize_text_field( $value ); + if ( in_array( $parent_key, self::TEXT_FIELD_KEYS, true ) ) { + return sanitize_text_field( $value ); + } + if ( + in_array( $parent_key, self::SLUG_FIELD_KEYS, true ) || + in_array( $parent_key, self::SLUG_LIST_KEYS, true ) + ) { + return sanitize_key( $value ); + } + return wp_kses_no_null( $value ); } return $value; } diff --git a/tests/test-theme-settings-save.php b/tests/test-theme-settings-save.php index c10b2785..70e8fa17 100644 --- a/tests/test-theme-settings-save.php +++ b/tests/test-theme-settings-save.php @@ -243,4 +243,153 @@ public function test_reify_shadow_removals_preserves_user_custom_presets() { $slugs = array_column( $out['settings']['shadow']['presets'], 'slug' ); $this->assertContains( 'my-custom-shadow', $slugs ); } + + /* ---------------------------------------------------------------- * + * sanitize() — context-aware (CSS values must round-trip) + * ---------------------------------------------------------------- */ + + public function test_sanitize_preserves_complex_shadow_value() { + $shadow = '6px 6px 0px -3px rgb(255, 255, 255), 6px 6px rgb(0, 0, 0)'; + $out = CBT_Theme_Settings_Save::sanitize( + array( + 'settings' => array( + 'shadow' => array( + 'presets' => array( + array( + 'slug' => 'outlined', + 'name' => 'Outlined', + 'shadow' => $shadow, + ), + ), + ), + ), + ) + ); + $this->assertSame( $shadow, $out['settings']['shadow']['presets'][0]['shadow'] ); + } + + public function test_sanitize_preserves_gradient_value() { + $gradient = 'linear-gradient(135deg, rgb(6, 147, 227) 0%, rgb(155, 81, 224) 100%)'; + $out = CBT_Theme_Settings_Save::sanitize( + array( + 'settings' => array( + 'color' => array( + 'gradients' => array( + array( + 'slug' => 'vivid', + 'name' => 'Vivid', + 'gradient' => $gradient, + ), + ), + ), + ), + ) + ); + $this->assertSame( $gradient, $out['settings']['color']['gradients'][0]['gradient'] ); + } + + public function test_sanitize_post_types_entries_are_slug_normalized() { + $out = CBT_Theme_Settings_Save::sanitize( + array( + 'customTemplates' => array( + array( + 'name' => 'page-wide', + 'title' => 'Wide Page', + 'postTypes' => array( 'Page', 'POST' ), + ), + ), + ) + ); + // `sanitize_key` lowercases. + $this->assertSame( array( 'page', 'post' ), $out['customTemplates'][0]['postTypes'] ); + } + + /* ---------------------------------------------------------------- * + * validate() — slug rejection + * ---------------------------------------------------------------- */ + + public function test_validate_rejects_removed_shadow_slug_with_invalid_chars() { + $result = CBT_Theme_Settings_Save::validate( + array( 'removedShadowDefaults' => array( 'natural', 'has spaces' ) ) + ); + $this->assertWPError( $result ); + $this->assertSame( 'cbt_invalid_payload', $result->get_error_code() ); + } + + public function test_validate_rejects_custom_template_name_with_invalid_chars() { + $result = CBT_Theme_Settings_Save::validate( + array( + 'customTemplates' => array( + array( + 'name' => 'Has Caps', + 'title' => 'Caps Page', + ), + ), + ) + ); + $this->assertWPError( $result ); + } + + public function test_validate_accepts_well_formed_slugs() { + $payload = array( + 'removedShadowDefaults' => array( 'natural', 'sharp_2' ), + 'customTemplates' => array( + array( + 'name' => 'page-wide-2', + 'title' => 'Wide Page', + ), + ), + ); + $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); + } + + /* ---------------------------------------------------------------- * + * run() — write-failure path + * + * Verified via integration: a hardened service should surface a write + * failure as WP_Error rather than returning the merged payload as if it + * had been persisted. Smoke tests the unhappy path of + * `CBT_Theme_JSON_Resolver::write_theme_file_contents`. + * ---------------------------------------------------------------- */ + + public function test_run_returns_wp_error_on_write_failure() { + // Create a temp theme directory with a read-only theme.json. Point the + // resolver at it via the stylesheet_directory filter; file_put_contents + // will fail because the file is not writable, which the service must + // surface as WP_Error rather than reporting SUCCESS. + $tmp_dir = sys_get_temp_dir() . '/cbt-test-' . uniqid(); + $theme_json = $tmp_dir . '/theme.json'; + mkdir( $tmp_dir ); + file_put_contents( $theme_json, '{}' ); + chmod( $theme_json, 0444 ); + // Best-effort guard: skip if running as root (chmod is meaningless). + if ( is_writable( $theme_json ) ) { + chmod( $theme_json, 0644 ); + unlink( $theme_json ); + rmdir( $tmp_dir ); + $this->markTestSkipped( 'Cannot make file read-only in this environment.' ); + } + + $filter = static function () use ( $tmp_dir ) { + return $tmp_dir; + }; + add_filter( 'stylesheet_directory', $filter ); + add_filter( 'template_directory', $filter ); + + $result = CBT_Theme_Settings_Save::run( + array( 'settings' => array( 'color' => array( 'custom' => true ) ) ) + ); + + remove_filter( 'stylesheet_directory', $filter ); + remove_filter( 'template_directory', $filter ); + + // Cleanup. + chmod( $theme_json, 0644 ); + unlink( $theme_json ); + rmdir( $tmp_dir ); + + $this->assertWPError( $result ); + $this->assertSame( 'cbt_write_failed', $result->get_error_code() ); + $this->assertSame( 500, $result->get_error_data()['status'] ); + } } From 2e6b5ba26d6c09d9997d81afc1c7908575f0e705 Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Thu, 7 May 2026 11:28:34 +0200 Subject: [PATCH 3/5] Validate required keys on customTemplates and templateParts entries customTemplates entries must have non-empty 'name' and 'title' fields. templateParts entries must have non-empty 'name' and 'area' fields. Catches malformed entries (missing required keys, empty strings) at the REST boundary rather than letting them land verbatim in theme.json. The 'name' field continues to require slug format since it's used as a filename. --- includes/create-theme/theme-settings-save.php | 32 +++++++++--- tests/test-theme-settings-save.php | 50 +++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/includes/create-theme/theme-settings-save.php b/includes/create-theme/theme-settings-save.php index 92639e4e..f1ec525c 100644 --- a/includes/create-theme/theme-settings-save.php +++ b/includes/create-theme/theme-settings-save.php @@ -169,24 +169,42 @@ public static function validate( array $payload ) { } } - // Validate `name` slugs on customTemplates and templateParts entries - // (the `name` field is used as the file-system slug, e.g. templates/.html). - foreach ( array( 'customTemplates', 'templateParts' ) as $list_key ) { + // Validate required keys + slug format on customTemplates and templateParts + // entries. `name` is required on both (used as the file-system slug, + // e.g. templates/.html). `area` is required on templateParts; + // `title` is required on customTemplates. + $entry_required_keys = array( + 'customTemplates' => array( 'name', 'title' ), + 'templateParts' => array( 'name', 'area' ), + ); + foreach ( $entry_required_keys as $list_key => $required_keys ) { if ( ! isset( $payload[ $list_key ] ) ) { continue; } foreach ( $payload[ $list_key ] as $entry ) { - if ( ! isset( $entry['name'] ) ) { - continue; + foreach ( $required_keys as $required_key ) { + if ( ! isset( $entry[ $required_key ] ) || ! is_string( $entry[ $required_key ] ) || '' === $entry[ $required_key ] ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: 1: list key, 2: required key */ + __( 'Entries of "%1$s" must have a non-empty string "%2$s" field.', 'create-block-theme' ), + $list_key, + $required_key + ), + array( 'status' => 400 ) + ); + } } - if ( ! is_string( $entry['name'] ) || sanitize_key( $entry['name'] ) !== $entry['name'] || '' === $entry['name'] ) { + // `name` must be a valid slug (used as filename). + if ( sanitize_key( $entry['name'] ) !== $entry['name'] ) { return new WP_Error( 'cbt_invalid_payload', sprintf( /* translators: 1: list key, 2: invalid name */ __( 'Invalid "%1$s" entry name "%2$s". Names must be lowercase alphanumeric with dashes or underscores.', 'create-block-theme' ), $list_key, - is_scalar( $entry['name'] ) ? (string) $entry['name'] : '?' + $entry['name'] ), array( 'status' => 400 ) ); diff --git a/tests/test-theme-settings-save.php b/tests/test-theme-settings-save.php index 70e8fa17..43f02fda 100644 --- a/tests/test-theme-settings-save.php +++ b/tests/test-theme-settings-save.php @@ -343,6 +343,56 @@ public function test_validate_accepts_well_formed_slugs() { $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); } + public function test_validate_rejects_custom_template_entry_missing_title() { + $result = CBT_Theme_Settings_Save::validate( + array( 'customTemplates' => array( array( 'name' => 'page-wide' ) ) ) + ); + $this->assertWPError( $result ); + $this->assertStringContainsString( 'title', $result->get_error_message() ); + } + + public function test_validate_rejects_template_part_entry_missing_area() { + $result = CBT_Theme_Settings_Save::validate( + array( 'templateParts' => array( array( 'name' => 'sidebar' ) ) ) + ); + $this->assertWPError( $result ); + $this->assertStringContainsString( 'area', $result->get_error_message() ); + } + + public function test_validate_rejects_entry_with_empty_required_key() { + $result = CBT_Theme_Settings_Save::validate( + array( + 'customTemplates' => array( + array( + 'name' => 'page-wide', + 'title' => '', + ), + ), + ) + ); + $this->assertWPError( $result ); + } + + public function test_validate_rejects_template_part_missing_name() { + $result = CBT_Theme_Settings_Save::validate( + array( 'templateParts' => array( array( 'area' => 'header' ) ) ) + ); + $this->assertWPError( $result ); + $this->assertStringContainsString( 'name', $result->get_error_message() ); + } + + public function test_validate_accepts_complete_template_part_entry() { + $payload = array( + 'templateParts' => array( + array( + 'name' => 'sidebar', + 'area' => 'uncategorized', + ), + ), + ); + $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); + } + /* ---------------------------------------------------------------- * * run() — write-failure path * From 980c89d5bdf8deb38da9f8439512dda3f23f245f Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Tue, 12 May 2026 14:13:55 +0200 Subject: [PATCH 4/5] Adopt RFC 7396 merge semantics + atomic write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on PR. JSON Merge Patch (RFC 7396) semantics: - null in the payload deletes the corresponding key on disk. - Empty object (`{}`) is a no-op. PHP cannot distinguish `{}` from `[]` after json_decode(true), so we use a heuristic: if the existing value at the key is a list, an empty payload value clears the list; otherwise it's a no-op. This closes the `{settings: {}}`-wipes-everything footgun flagged in review while preserving the explicit-clear semantics for lists like `palette: []`. - Lists still replace wholesale — RFC 7396 doesn't define per-element list patching. To drop one palette entry, send the new full palette. Atomic write + theme cache: - write_theme_file_contents now writes to a .tmp sibling and renames into place. If the write or rename fails partway, the original theme.json is untouched. Previously file_put_contents would truncate-then-write, leaving a corrupt file on interruption. - After a successful write, wp_get_theme()->cache_delete() is also called (matching rest_save_theme); clean_cached_data alone only clears the resolver's caches, not the WP theme cache. Maintenance note on TEXT_FIELD_KEYS: spelled out the trade-off so future contributors know what to update when adding new label-class fields. Concurrent edits are still last-write-wins; documented as a known limitation. ETag/If-Match handling can be a follow-up. Tests: 6 new merge cases (null deletes, null-on-missing no-op, {} at top level no-op, {} nested no-op, empty-for-existing-list clears, empty-for- missing-list no-op). Write-failure test updated to chmod the directory rather than the file (the atomic write fails at the temp-file creation, not the rename). 34/34 in this suite, 128/128 overall, lint clean. --- includes/create-theme/resolver_additions.php | 25 ++++- includes/create-theme/theme-settings-save.php | 63 ++++++++++-- tests/test-theme-settings-save.php | 97 +++++++++++++++++-- 3 files changed, 167 insertions(+), 18 deletions(-) diff --git a/includes/create-theme/resolver_additions.php b/includes/create-theme/resolver_additions.php index b5ee4675..7b4e35e1 100644 --- a/includes/create-theme/resolver_additions.php +++ b/includes/create-theme/resolver_additions.php @@ -135,13 +135,34 @@ public static function get_theme_file_contents() { public static function write_theme_file_contents( $theme_json_data ) { $theme_json = wp_json_encode( $theme_json_data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE ); + $target = static::get_file_path_from_theme( 'theme.json' ); + + // Atomic write: write to a sibling temp file, then rename into place. + // If the write or rename fails partway, the original theme.json is + // untouched. `file_put_contents` would truncate-then-write, leaving + // a corrupt file on interruption. + $tmp = $target . '.tmp'; // Suppress warnings so a permission/disk error returns false cleanly // rather than emitting a PHP warning that may be promoted to an // exception. Callers must check the boolean return value. // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged - $bytes = @file_put_contents( static::get_file_path_from_theme( 'theme.json' ), $theme_json ); + $bytes = @file_put_contents( $tmp, $theme_json ); + if ( false === $bytes ) { + return false; + } + + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + if ( ! @rename( $tmp, $target ) ) { + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + @unlink( $tmp ); + return false; + } + static::clean_cached_data(); - return false !== $bytes; + // Bust the WP-level theme cache too — `clean_cached_data()` only + // clears the JSON resolver's caches, not `wp_get_theme()`. + wp_get_theme()->cache_delete(); + return true; } public static function write_user_settings( $user_settings ) { diff --git a/includes/create-theme/theme-settings-save.php b/includes/create-theme/theme-settings-save.php index f1ec525c..d6f2a72a 100644 --- a/includes/create-theme/theme-settings-save.php +++ b/includes/create-theme/theme-settings-save.php @@ -4,15 +4,21 @@ * * Persists Edit Theme Settings modal payloads to the active theme's theme.json. * Accepts a partial-`theme.json` payload (only the keys the user edited), - * deep-merges it into the existing file, and writes the result. Reifies the - * `removedShadowDefaults` operational key into the standard - * `settings.shadow.defaultPresets` + `settings.shadow.presets` shape. + * deep-merges it into the existing file using JSON Merge Patch semantics + * (RFC 7396), and writes the result. Reifies the `removedShadowDefaults` + * operational key into the standard `settings.shadow.defaultPresets` + + * `settings.shadow.presets` shape. * * Callers must enforce capability checks (`edit_theme_options`) before * invoking `run()`. The service performs payload-shape validation and * sanitization but does not authenticate. * + * Known limitation: concurrent edits are last-write-wins. Two clients with + * the modal open simultaneously will silently overwrite each other. ETag / + * If-Match handling can be added if this becomes a problem in practice. + * * @package Create_Block_Theme + * @see https://datatracker.ietf.org/doc/html/rfc7396 JSON Merge Patch */ class CBT_Theme_Settings_Save { @@ -26,6 +32,13 @@ class CBT_Theme_Settings_Save { /** * Keys whose string values are user-facing labels and benefit from * `sanitize_text_field()` (HTML stripping, whitespace normalization). + * + * This is an allowlist. Adding a new label-class field to theme.json + * upstream means adding it here — otherwise it falls through to + * `wp_kses_no_null` and HTML will not be stripped. Trade-off accepted: + * the inverse rule ("strip HTML by default, allowlist CSS-value fields") + * silently corrupts CSS for new fields, which is worse than missing a + * label sanitization update. */ const TEXT_FIELD_KEYS = array( 'name', 'title', 'label', 'description' ); @@ -259,11 +272,28 @@ public static function sanitize( $value, $parent_key = '' ) { } /** - * Deep-merge $payload into $current. At every level, associative-array - * values are merged recursively; lists, scalars, and empty arrays replace - * the existing value. Missing parent keys are created. + * Deep-merge $payload into $current using JSON Merge Patch (RFC 7396) + * semantics, with one PHP-imposed accommodation. + * + * Rules: * - * The operational key `removedShadowDefaults` is dropped here — it's + * - **`null` deletes the key.** Sending `{"settings":{"color":{"custom":null}}}` + * removes `settings.color.custom` from the result. Deleting a missing + * key is a no-op. + * - **Empty object (`{}`) is a no-op.** Per RFC 7396, an empty object means + * "no change at this key." PHP cannot distinguish a JSON `{}` from a JSON + * `[]` after `json_decode(..., true)` (both become empty PHP arrays), so + * we use a heuristic: if the existing value at this key is a list, an + * empty payload value clears that list; otherwise it is treated as a + * no-op. This means clients that intend to clear a list MUST already + * know the field is list-typed (which is how the modal is built). + * - **Associative-object values are merged recursively.** Leaves replace. + * - **List values replace wholesale** — RFC 7396 does not support + * per-element list patching. To remove one palette entry, send the full + * new palette. + * - **Missing parent keys are created** when assigning into them. + * + * The operational key `removedShadowDefaults` is skipped here — it's * handled separately by `reify_shadow_removals()`. * * @param array $current @@ -276,6 +306,25 @@ public static function merge( array $current, array $payload ) { if ( 'removedShadowDefaults' === $key ) { continue; } + + // RFC 7396: null deletes the key. + if ( null === $value ) { + unset( $result[ $key ] ); + continue; + } + + // Empty array: RFC 7396 says `{}` is a no-op. PHP can't tell `{}` + // from `[]`, so we infer intent from the existing value: + // - existing value is a list → caller intends to clear it. + // - otherwise (existing is assoc, missing, or scalar) → no-op. + if ( is_array( $value ) && empty( $value ) ) { + if ( isset( $result[ $key ] ) && is_array( $result[ $key ] ) && self::is_list( $result[ $key ] ) ) { + $result[ $key ] = array(); + } + continue; + } + + // Deep merge associative-object → associative-object. if ( is_array( $value ) && ! self::is_list( $value ) && diff --git a/tests/test-theme-settings-save.php b/tests/test-theme-settings-save.php index 43f02fda..909f14db 100644 --- a/tests/test-theme-settings-save.php +++ b/tests/test-theme-settings-save.php @@ -198,6 +198,85 @@ public function test_merge_does_not_emit_removed_shadow_defaults_key() { $this->assertArrayNotHasKey( 'removedShadowDefaults', $out ); } + /* ---------------------------------------------------------------- * + * merge() — RFC 7396 semantics + * ---------------------------------------------------------------- */ + + public function test_merge_null_deletes_existing_key() { + $current = array( + 'settings' => array( + 'color' => array( + 'custom' => true, + 'defaultPalette' => true, + ), + ), + ); + $payload = array( + 'settings' => array( 'color' => array( 'custom' => null ) ), + ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertArrayNotHasKey( 'custom', $out['settings']['color'] ); + $this->assertTrue( $out['settings']['color']['defaultPalette'] ); + } + + public function test_merge_null_for_missing_key_is_no_op() { + $current = array( 'settings' => array( 'color' => array( 'custom' => true ) ) ); + $payload = array( 'settings' => array( 'color' => array( 'doesNotExist' => null ) ) ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertTrue( $out['settings']['color']['custom'] ); + $this->assertArrayNotHasKey( 'doesNotExist', $out['settings']['color'] ); + } + + public function test_merge_empty_object_at_top_level_is_no_op() { + // The footgun the JSON-Merge-Patch contract closes: `settings: {}` must + // not wipe everything in the existing settings tree. + $current = array( + 'settings' => array( + 'color' => array( 'custom' => true ), + 'spacing' => array( 'padding' => true ), + ), + ); + $payload = array( 'settings' => array() ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( $current, $out ); + } + + public function test_merge_empty_object_nested_is_no_op() { + $current = array( + 'settings' => array( + 'color' => array( + 'custom' => true, + 'palette' => array( array( 'slug' => 'a' ) ), + ), + ), + ); + $payload = array( 'settings' => array( 'color' => array() ) ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( $current, $out ); + } + + public function test_merge_empty_for_existing_list_still_clears() { + // `palette: []` should still clear the palette, because palette is a + // list (its current value is a sequential array). + $current = array( + 'settings' => array( + 'color' => array( 'palette' => array( array( 'slug' => 'a' ) ) ), + ), + ); + $payload = array( 'settings' => array( 'color' => array( 'palette' => array() ) ) ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( array(), $out['settings']['color']['palette'] ); + } + + public function test_merge_empty_for_missing_list_is_no_op() { + // payload `customTemplates: []` against a theme.json that doesn't have + // a customTemplates key at all → no-op (don't create an empty list). + $current = array( 'settings' => array() ); + $payload = array( 'customTemplates' => array() ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertArrayNotHasKey( 'customTemplates', $out ); + } + /* ---------------------------------------------------------------- * * reify_shadow_removals() * ---------------------------------------------------------------- */ @@ -403,21 +482,21 @@ public function test_validate_accepts_complete_template_part_entry() { * ---------------------------------------------------------------- */ public function test_run_returns_wp_error_on_write_failure() { - // Create a temp theme directory with a read-only theme.json. Point the - // resolver at it via the stylesheet_directory filter; file_put_contents - // will fail because the file is not writable, which the service must - // surface as WP_Error rather than reporting SUCCESS. + // Create a temp theme directory with a theme.json, then make the + // directory read-only so the atomic write (write-temp-then-rename) + // can't create its `theme.json.tmp` sibling. The service must surface + // the failure as WP_Error rather than reporting SUCCESS. $tmp_dir = sys_get_temp_dir() . '/cbt-test-' . uniqid(); $theme_json = $tmp_dir . '/theme.json'; mkdir( $tmp_dir ); file_put_contents( $theme_json, '{}' ); - chmod( $theme_json, 0444 ); + chmod( $tmp_dir, 0555 ); // Best-effort guard: skip if running as root (chmod is meaningless). - if ( is_writable( $theme_json ) ) { - chmod( $theme_json, 0644 ); + if ( is_writable( $tmp_dir ) ) { + chmod( $tmp_dir, 0755 ); unlink( $theme_json ); rmdir( $tmp_dir ); - $this->markTestSkipped( 'Cannot make file read-only in this environment.' ); + $this->markTestSkipped( 'Cannot make directory read-only in this environment.' ); } $filter = static function () use ( $tmp_dir ) { @@ -434,7 +513,7 @@ public function test_run_returns_wp_error_on_write_failure() { remove_filter( 'template_directory', $filter ); // Cleanup. - chmod( $theme_json, 0644 ); + chmod( $tmp_dir, 0755 ); unlink( $theme_json ); rmdir( $tmp_dir ); From dd93c0e25ac133388b69d093720dabd4d4066fda Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Tue, 12 May 2026 14:58:34 +0200 Subject: [PATCH 5/5] Harden concurrency + tighten payload shape validation Addresses three review items together since they share code paths. Concurrency: - write_theme_file_contents() now uses a per-request temp filename (uniqid suffix) so two concurrent saves can no longer collide on a shared staging path. The previous shared 'theme.json.tmp' allowed one request to rename another's truncated payload. - run() now holds an exclusive flock() on a sibling 'theme.json.lock' around the read-merge-write block. Disjoint edits from two clients no longer overwrite each other; the second request sees the first's saved state before merging its own payload on top. Remaining limitation: conflicting same-key edits are still last-write-wins; ETag/If-Match is the right follow-up if that becomes painful. - flock() acquisition failure surfaces as WP_Error('cbt_lock_failed', status 503), so the client can retry rather than seeing silent success. Payload shape: - validate() rejects JSON lists in the 'settings' object position and JSON objects in the 'customTemplates' / 'templateParts' / 'removedShadowDefaults' list positions. Uses the internal is_list() helper that merge() was already using internally; the helper now doubles as a contract assertion at the validation boundary. - Empty arrays remain accepted for either shape (PHP cannot distinguish {} from [] post json_decode(true); merge() handles them correctly). - merge() defensively normalizes lists via array_values() so any sparse-keyed array can't slip through and serialize as a JSON object. Docblock updated to document the flock-based serialization and its distributed-host caveat. Tests: 4 new (settings-as-list rejection, customTemplates-as-object rejection, removed-shadow-defaults-as-object rejection, empty-arrays-accepted) + 1 list-normalization smoke test. Write-failure test broadened to accept either cbt_lock_failed or cbt_write_failed since a read-only theme directory blocks both. 39/39 in this suite, 133/133 overall, lint clean. --- includes/create-theme/resolver_additions.php | 13 ++- includes/create-theme/theme-settings-save.php | 107 +++++++++++++++--- tests/test-theme-settings-save.php | 90 ++++++++++++++- 3 files changed, 187 insertions(+), 23 deletions(-) diff --git a/includes/create-theme/resolver_additions.php b/includes/create-theme/resolver_additions.php index 7b4e35e1..ca60b29f 100644 --- a/includes/create-theme/resolver_additions.php +++ b/includes/create-theme/resolver_additions.php @@ -137,11 +137,14 @@ public static function write_theme_file_contents( $theme_json_data ) { $theme_json = wp_json_encode( $theme_json_data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE ); $target = static::get_file_path_from_theme( 'theme.json' ); - // Atomic write: write to a sibling temp file, then rename into place. - // If the write or rename fails partway, the original theme.json is - // untouched. `file_put_contents` would truncate-then-write, leaving - // a corrupt file on interruption. - $tmp = $target . '.tmp'; + // Atomic write with a request-unique temp file: write to a + // per-request sibling temp file, then rename into place. A + // per-request name prevents two concurrent saves from clobbering + // each other's staging payload (a shared `theme.json.tmp` is unsafe + // — request A could rename request B's truncated contents, or one + // could unlink the other's temp file mid-write). Each request + // cleans up only its own temp file on failure. + $tmp = $target . '.' . uniqid( '', true ) . '.tmp'; // Suppress warnings so a permission/disk error returns false cleanly // rather than emitting a PHP warning that may be promoted to an // exception. Callers must check the boolean return value. diff --git a/includes/create-theme/theme-settings-save.php b/includes/create-theme/theme-settings-save.php index d6f2a72a..37a3c42a 100644 --- a/includes/create-theme/theme-settings-save.php +++ b/includes/create-theme/theme-settings-save.php @@ -13,9 +13,15 @@ * invoking `run()`. The service performs payload-shape validation and * sanitization but does not authenticate. * - * Known limitation: concurrent edits are last-write-wins. Two clients with - * the modal open simultaneously will silently overwrite each other. ETag / - * If-Match handling can be added if this becomes a problem in practice. + * Concurrent saves on a single host are serialized via `flock()` on a + * sibling lockfile (`theme.json.lock`), so disjoint edits from two clients + * compose correctly. The remaining limitation is conflicting same-key + * edits, which are still last-write-wins (the second save sees the first + * save's state after taking the lock, merges its payload on top, and the + * conflicting field gets the second client's value). Distributed hosts + * where `flock()` is not honored (some NFS configurations) degrade to + * best-effort. ETag / If-Match handling is a candidate follow-up if the + * remaining conflict case becomes a problem in practice. * * @package Create_Block_Theme * @see https://datatracker.ietf.org/doc/html/rfc7396 JSON Merge Patch @@ -68,18 +74,51 @@ public static function run( array $payload ) { $sanitized = self::sanitize( $validated ); - $current = CBT_Theme_JSON_Resolver::get_theme_file_contents(); - if ( ! is_array( $current ) ) { - $current = array(); + // Serialize concurrent saves on a single host: hold an exclusive lock + // on a sibling lockfile around the read-merge-write sequence. This + // prevents two requests from reading stale state, merging in + // parallel, and one overwriting the other's disjoint changes. On + // distributed hosts where flock isn't honored (some NFS configs) + // this degrades to best-effort, hence the documented limitation. + $lock_path = get_stylesheet_directory() . '/theme.json.lock'; + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fopen + $lock_handle = @fopen( $lock_path, 'c' ); + if ( false === $lock_handle ) { + return new WP_Error( + 'cbt_lock_failed', + __( 'Could not acquire theme.json save lock. Check filesystem permissions on the active theme directory.', 'create-block-theme' ), + array( 'status' => 503 ) + ); + } + if ( ! flock( $lock_handle, LOCK_EX ) ) { + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fclose + fclose( $lock_handle ); + return new WP_Error( + 'cbt_lock_failed', + __( 'Could not acquire theme.json save lock.', 'create-block-theme' ), + array( 'status' => 503 ) + ); } - $merged = self::merge( $current, $sanitized ); + try { + $current = CBT_Theme_JSON_Resolver::get_theme_file_contents(); + if ( ! is_array( $current ) ) { + $current = array(); + } + + $merged = self::merge( $current, $sanitized ); + + if ( array_key_exists( 'removedShadowDefaults', $sanitized ) ) { + $merged = self::reify_shadow_removals( $merged, $sanitized['removedShadowDefaults'] ); + } - if ( array_key_exists( 'removedShadowDefaults', $sanitized ) ) { - $merged = self::reify_shadow_removals( $merged, $sanitized['removedShadowDefaults'] ); + $wrote = CBT_Theme_JSON_Resolver::write_theme_file_contents( $merged ); + } finally { + flock( $lock_handle, LOCK_UN ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fclose + fclose( $lock_handle ); } - $wrote = CBT_Theme_JSON_Resolver::write_theme_file_contents( $merged ); if ( true !== $wrote ) { return new WP_Error( 'cbt_write_failed', @@ -114,12 +153,24 @@ public static function validate( array $payload ) { } } - if ( isset( $payload['settings'] ) && ! is_array( $payload['settings'] ) ) { - return new WP_Error( - 'cbt_invalid_payload', - __( '"settings" must be an object.', 'create-block-theme' ), - array( 'status' => 400 ) - ); + if ( isset( $payload['settings'] ) ) { + if ( ! is_array( $payload['settings'] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( '"settings" must be an object.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } + // Reject JSON lists in an object position. Empty array is + // ambiguous in PHP (both `{}` and `[]` decode to `[]`) and is + // allowed — `merge()` handles it as a no-op. + if ( ! empty( $payload['settings'] ) && self::is_list( $payload['settings'] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( '"settings" must be an object, not a list.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } } foreach ( array( 'customTemplates', 'templateParts' ) as $list_key ) { @@ -137,6 +188,19 @@ public static function validate( array $payload ) { array( 'status' => 400 ) ); } + // Require a JSON list, not a JSON object. Empty array is ambiguous + // in PHP and is allowed. + if ( ! empty( $payload[ $list_key ] ) && ! self::is_list( $payload[ $list_key ] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + sprintf( + /* translators: %s: payload key */ + __( '"%s" must be a list, not an object.', 'create-block-theme' ), + $list_key + ), + array( 'status' => 400 ) + ); + } foreach ( $payload[ $list_key ] as $entry ) { if ( ! is_array( $entry ) ) { return new WP_Error( @@ -160,6 +224,13 @@ public static function validate( array $payload ) { array( 'status' => 400 ) ); } + if ( ! empty( $payload['removedShadowDefaults'] ) && ! self::is_list( $payload['removedShadowDefaults'] ) ) { + return new WP_Error( + 'cbt_invalid_payload', + __( '"removedShadowDefaults" must be a list, not an object.', 'create-block-theme' ), + array( 'status' => 400 ) + ); + } foreach ( $payload['removedShadowDefaults'] as $slug ) { if ( ! is_string( $slug ) ) { return new WP_Error( @@ -333,6 +404,10 @@ public static function merge( array $current, array $payload ) { ! self::is_list( $result[ $key ] ) ) { $result[ $key ] = self::merge( $result[ $key ], $value ); + } elseif ( is_array( $value ) && self::is_list( $value ) ) { + // Normalize lists to a contiguous index. Defends against any + // sparse-keyed array slipping past validation. + $result[ $key ] = array_values( $value ); } else { $result[ $key ] = $value; } diff --git a/tests/test-theme-settings-save.php b/tests/test-theme-settings-save.php index 909f14db..73bddc26 100644 --- a/tests/test-theme-settings-save.php +++ b/tests/test-theme-settings-save.php @@ -472,6 +472,82 @@ public function test_validate_accepts_complete_template_part_entry() { $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); } + /* ---------------------------------------------------------------- * + * validate() — JSON shape (object vs list) enforcement + * + * PHP's `json_decode(..., true)` flattens `{}` and `[]` to the same empty + * array, but a non-empty JSON list passed where an object is expected + * (or vice versa) is detectable and should be rejected. + * ---------------------------------------------------------------- */ + + public function test_validate_rejects_settings_passed_as_non_empty_list() { + $result = CBT_Theme_Settings_Save::validate( + array( 'settings' => array( array( 'foo' => 'bar' ) ) ) + ); + $this->assertWPError( $result ); + $this->assertStringContainsString( 'object', $result->get_error_message() ); + } + + public function test_validate_rejects_custom_templates_passed_as_object() { + $result = CBT_Theme_Settings_Save::validate( + array( + 'customTemplates' => array( + 'page-wide' => array( + 'name' => 'page-wide', + 'title' => 'Wide Page', + ), + ), + ) + ); + $this->assertWPError( $result ); + $this->assertStringContainsString( 'list', $result->get_error_message() ); + } + + public function test_validate_rejects_removed_shadow_defaults_passed_as_object() { + $result = CBT_Theme_Settings_Save::validate( + array( 'removedShadowDefaults' => array( 'natural' => true ) ) + ); + $this->assertWPError( $result ); + } + + public function test_validate_accepts_empty_arrays_for_either_shape() { + // Empty arrays are intentionally permitted regardless of expected + // shape — they're handled in merge() (no-op for object positions, + // clear for list positions where the existing value is a list). + $payload = array( + 'settings' => array(), + 'customTemplates' => array(), + 'templateParts' => array(), + 'removedShadowDefaults' => array(), + ); + $this->assertSame( $payload, CBT_Theme_Settings_Save::validate( $payload ) ); + } + + /* ---------------------------------------------------------------- * + * merge() — list normalization + * ---------------------------------------------------------------- */ + + public function test_merge_writes_lists_with_sequential_keys() { + // Routes a proper list payload through the `is_list`-true branch and + // confirms it lands with sequential integer keys (i.e., emits as a + // JSON list, not an object, when serialized). + $current = array(); + $payload = array( + 'customTemplates' => array( + array( + 'name' => 'a', + 'title' => 'A', + ), + array( + 'name' => 'b', + 'title' => 'B', + ), + ), + ); + $out = CBT_Theme_Settings_Save::merge( $current, $payload ); + $this->assertSame( array( 0, 1 ), array_keys( $out['customTemplates'] ) ); + } + /* ---------------------------------------------------------------- * * run() — write-failure path * @@ -518,7 +594,17 @@ public function test_run_returns_wp_error_on_write_failure() { rmdir( $tmp_dir ); $this->assertWPError( $result ); - $this->assertSame( 'cbt_write_failed', $result->get_error_code() ); - $this->assertSame( 500, $result->get_error_data()['status'] ); + // A read-only theme directory blocks both lockfile creation and the + // atomic write. Accept either failure code; both are correct surfaces + // for "the filesystem said no" and both protect against the original + // silent-success bug. + $this->assertContains( + $result->get_error_code(), + array( 'cbt_lock_failed', 'cbt_write_failed' ) + ); + $this->assertContains( + (int) $result->get_error_data()['status'], + array( 500, 503 ) + ); } }