Repository navigation
Improved security for dynamic content, stored CSS, and block conditions. - #3110
girishpanchal30 wants to merge 11 commits into
Conversation
|
Plugin build for ff9f009 is ready 🛎️!
|
Bundle Size Diff
|
E2E TestsPlaywright Test Status: See serial and parallel matrix jobs Performance ResultsserverResponse: {"q25":461.59,"q50":500.07,"q75":512.24,"cnt":10}, firstPaint: {"q25":1507.28,"q50":1608.48,"q75":1952.8,"cnt":10}, domContentLoaded: {"q25":4318.39,"q50":4392.75,"q75":4427.34,"cnt":10}, loaded: {"q25":4321.72,"q50":4396.11,"q75":4430.6,"cnt":10}, firstContentfulPaint: {"q25":2841.4,"q50":3232,"q75":3295.68,"cnt":10}, firstBlock: {"q25":19761.29,"q50":19889.46,"q75":20073.76,"cnt":10}, type: {"q25":33.57,"q50":37.73,"q75":42.01,"cnt":10}, typeWithoutInspector: {"q25":27.38,"q50":32.19,"q75":37.45,"cnt":10}, typeWithTopToolbar: {"q25":44.55,"q50":49.84,"q75":53.51,"cnt":10}, typeContainer: {"q25":19.65,"q50":21.46,"q75":22.77,"cnt":10}, focus: {"q25":180.28,"q50":186.24,"q75":211.47,"cnt":10}, inserterOpen: {"q25":52.53,"q50":55.42,"q75":62.19,"cnt":10}, inserterSearch: {"q25":35.91,"q50":36.83,"q75":39.62,"cnt":10}, inserterHover: {"q25":5.75,"q50":6.4,"q75":7.45,"cnt":20}, loadPatterns: {"q25":2213.02,"q50":2257.43,"q75":2304.61,"cnt":10}, listViewOpen: {"q25":289.51,"q50":304.13,"q75":327.6,"cnt":10} |
There was a problem hiding this comment.
🟡 Changes recommended
The new CSS escapes are stripped during persistence and file writing, corrupting affected CSS values.
2 open findings
What changed in this PR
Hardens dynamic content, stored CSS, Atomic Wind styles, and conditional block rendering against XSS and unauthorized disclosure.
Changes:
- Restricts dynamic-content matching and REST access.
- Escapes inline CSS and Atomic Wind caches.
- Enforces conditions in REST/admin-ajax contexts with regression tests.
| File | Description |
|---|---|
tests/test-woo-conditions.php |
Tests missing WooCommerce carts. |
tests/test-dynamic-content.php |
Tests token boundaries and URL escaping. |
tests/test-dynamic-content-server.php |
Tests dynamic REST permissions. |
tests/test-css-handler.php |
Tests stored CSS neutralization. |
tests/test-block-conditions-rest.php |
Tests REST and admin-ajax conditions. |
tests/test-atomic-wind-blocks.php |
Tests Atomic Wind CSS escaping. |
plugins/otter-pro/inc/plugins/class-dynamic-content.php |
Handles invalid product IDs safely. |
plugins/otter-pro/inc/plugins/class-block-conditions.php |
Handles unavailable carts safely. |
inc/server/class-dynamic-content-server.php |
Tightens dynamic route authorization. |
inc/plugins/class-dynamic-content.php |
Hardens token matching and URLs. |
inc/plugins/class-block-conditions.php |
Applies conditions outside editor rendering. |
inc/plugins/class-atomic-wind-blocks.php |
Escapes stored Atomic Wind CSS. |
inc/css/class-css-handler.php |
Adds style-content escaping. |
inc/css/class-block-frontend.php |
Escapes CSS before inline output. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes requested
The new link and image token patterns cut meta keys at spaces, and three changed branches need tests.
Validation details
- Files reviewed: 14/14 changed files. Two questions stayed unchecked: "consumer: Dynamic_Content_Server::check_permission() return changed" and "consumer: REST route '/dynamic': register_rest_route() call changed".
- PHPUnit ran on PHP 8.3 with the WordPress 7.1.3 test library. The 22 new tests passed on HEAD and failed on the base tree.
- A kses check on WordPress 7.1.3 kept the space in a saved link:
mkdir -p /workspace/checks/F1; cd /var/www/html; wp eval '$h = "<a href=\"#otterDynamicLink?type=postMetaURL&metaKey…printedHEAD token: #otterDynamicLink?type=postMetaURL&metaKey=my(run R4). - Source checks found no unescaped inline CSS print, a balanced REST condition stack and guarded WooCommerce conditions.
- Question for the author: The description says the escape runs after the decode in
compress(). The code escapes at the print sites instead. Please update the description.
Untested areas
- "consumer: Dynamic_Content_Server::check_permission() return changed" and "consumer: REST route '/dynamic': register_rest_route() call changed": open
inc/plugins/class-dynamic-content.php:283. Check whether a Pro ACF repeater image with one item keeps the REST URL insrc. If it does, visitors now get a 403 inside synced patterns and templates. - Editor and browser screens did not run. JavaScript dependencies and a Pro build were absent.
- The base test run used a stub animation asset because the build folder is absent.
🤖 Automated review · run code-review-agent_6acf2ee8168ca2.61911910.
🤖 Review agent — review posted ✅ on 00d6a730 · changes requested · 5 findings · 40 min
Run code-review-agent_6acf2ee8168ca2.61911910 · trail
| /** | ||
| * Resolve a text Magic Tag, or attribute-escape every Magic Tag inside a start tag. | ||
| * | ||
| * @param array<int|string, string> $matches Start tag in `tag`, or the Magic Tag query in group 2. |
There was a problem hiding this comment.
types The pattern always yields keys 0, tag and 1. Key 2 exists only for a text Magic Tag. Declare that shape: @param array{0: string, tag: string, 1: string, 2?: string} $matches.
There was a problem hiding this comment.
Still open at 40e112d. The description now names key 2, but the type is still array<int|string, string>. The pattern always yields 0, tag and 1, and 2 only for a text Magic Tag. Declare @param array{0: string, tag: string, 1: string, 2?: string} $matches.
There was a problem hiding this comment.
Not applied: this method is the callback for preg_replace_callback(), whose contract is callable(array<string>): string. Declaring array{0: string, tag: string, 1: string, 2?: string} narrows the callable's parameter below that contract. On 40e112d, PHPStan 2.2.0 then fails at line 130 with argument.type ("expects callable(array): string"). The type stays array<int|string, string>, with the keys documented in the description, and the code branches on isset( $matches[2] ).
There was a problem hiding this comment.
Conceded. This method is a preg_replace_callback() callback, so the callback contract dictates the parameter type. A narrower shape would break that contract. The current array<int|string, string> type plus its description of the keys is enough here.
There was a problem hiding this comment.
🟡 Changes requested
Four earlier findings are fixed. The $matches type in this thread is still too broad.
Validation details
- Files reviewed: 3/3 files changed since the prior review. Two questions stayed unchecked: "consumer: Dynamic_Content_Server::check_permission() return changed" and "consumer: REST route '/dynamic': register_rest_route() call changed".
- PHPUnit 9.6 ran on PHP 8.3. The four new tests passed on HEAD:
cd /repo; set -o pipefail; phpunit --do-not-cache-result --filter 'test_dynamic_tokens_keep_spaced_meta_keys|test_dynami…printedOK (4 tests, 12 assertions)(run R4) - Three of them failed on the base tree:
cd /workspace/checks/AB/basetree; set -o pipefail; phpunit --do-not-cache-result --filter 'test_dynamic_tokens_keep_spac…printedTests: 4, Assertions: 5, Failures: 3.(run R3) - The spaced-key test failed with the earlier file restored:
cd /workspace/checks/AB/priortree; set -o pipefail; phpunit --do-not-cache-result --filter 'test_dynamic_tokens_keep_spa…printedTests: 1, Assertions: 1, Failures: 1.(run R5)
Untested areas
- "consumer: Dynamic_Content_Server::check_permission() return changed" and "consumer: REST route '/dynamic': register_rest_route() call changed": the Pro image producer was not confirmed.
- Three Post Content tests errored because the build folder is absent.
- Editor screens did not run.
🤖 Automated review · run code-review-agent_6acf40214df198.51803395.
🤖 Review agent — review posted ✅ on 40e112d7 · changes requested · 0 findings · 12 min
Run code-review-agent_6acf40214df198.51803395 · trail
40e112d to
f3624fe
Compare
There was a problem hiding this comment.
🟡 Changes requested
The editor's Post Content preview now hides conditioned blocks, a Pro image path still fatals, and two REST stack cases need tests.
plugins/otter-pro/inc/plugins/class-dynamic-content.php:765—correctnessThe PR says Pro no longer fatals whenidis not a product. Only the REST path got the guard. The front-endevaluate_content_media_content()still calls$product->get_image_id()on the result ofwc_get_product(). That result isfalsefor a deleted product or a non-product ID. A post whose dynamic product image points to a deleted product then fatals on every view. A direct call returnedError: Call to a member function get_image_id() on falseon HEAD. Apply the same guard:$image = $product ? $product->get_image_id() : 0;. Add a case next totest_product_image_ignores_non_product_id.
Validation details
- Files reviewed: 14/14 changed files. Four questions stayed unchecked: the
get_cached_css()return change, issue #2937, issue #2929 and issue #2696. - Changed PHPUnit tests ran on HEAD and on a base tree with PHP 8.3 and WordPress 7.1.3. All pass on HEAD. Twenty of 21 fail on the base tree.
- A probe test fetched an editor's Post Content preview. HEAD returned only
PUBLIC. Base returnedSECRETandPUBLIC.
Untested areas
Atomic_Wind_Blocks::get_cached_css()return change: check that the callers at lines 336, 363 and 445 still get the CSS they expect.- Issue #2937, widget CSS writability: check that
save_widgets_styles()still works withoutWP_Filesystem(). - Issue #2929, post-loop warning: check that Magic Tags still resolve against the loop's context post.
- Issue #2696, template part assets: check that
get_post_css()still prints CSS for template parts. - Magic Tags inside a malformed start tag resolve without escaping. No producer of such markup was found after kses.
🤖 Automated review · run code-review-agent_6ad0596eca38c4.37487895.
🤖 Review agent — review posted ✅ on f3624fed · changes requested · 3 findings · 49 min
Run code-review-agent_6ad0596eca38c4.37487895 · trail
There was a problem hiding this comment.
🟡 Changes requested
The earlier findings are fixed, but two comments are now stale and the new tests repeat cleanup code.
Validation details
- Files reviewed: 4/14 changed files since the prior review. The rest reuse run code-review-agent_6ad0596eca38c4.37487895, not rerun. Unchecked: the
get_cached_css()return change, issue #2929, issue #2696 and the FSE CSS print path. - Changed PHPUnit tests pass on HEAD with PHP 8.3:
cd /repo; grep -n "Requires PHP" readme.txt otter-blocks.php | head -3; mkdir -p /workspace/checks/T1; set -o pipefail; …printedOK (10 tests, 26 assertions)(run R1). - The f3624fe Pro file fails the new test:
D=/workspace/checks/T1/tree; P=plugins/otter-pro/inc/plugins/class-dynamic-content.php; cd /repo; git show f3624fed302f3…printedCall to a member function get_image_id() on false(run R5).
Untested areas
get_cached_css()return change: callers at lines 336, 363 and 445 were not rechecked.- Issue #2929: Magic Tags against the loop's context post were not rechecked.
- Issue #2696: template part CSS printing was not rechecked.
- FSE CSS print path:
enqueue_fse_css()prints CSS withoutescape_style_contents(). Template edits neededit_theme_options.
🤖 Automated review · run code-review-agent_6ad0678f05afa8.84083586.
🤖 Review agent — review posted ✅ on ec3a08a5 · changes requested · 3 findings · 28 min
Run code-review-agent_6ad0678f05afa8.84083586 · trail
There was a problem hiding this comment.
🟢 Approved
The PR passes. Commit ff9f009 fixes all three earlier findings, and the checks found no new problem.
Validation details
- Files reviewed: 2/14 changed files since the prior review. The other files reuse run code-review-agent_6ad0678f05afa8.84083586, not rerun.
- The REST condition tests pass on HEAD with PHP 8.3:
cd /repo; set -o pipefail; timeout 300 vendor/bin/phpunit --do-not-cache-result --filter 'Test_Block_Conditions_Rest::' …printedOK (5 tests, 14 assertions)(run R1). - The same tests fail on the base code:
cd /repo; B=/workspace/checks/T1/base; rm -rf $B; mkdir -p $B; set -o pipefail; rsync -a --exclude node_modules --exclud…printedTests: 5, Assertions: 11, Failures: 4.(run R2). - A focused check covered cached Atomic Wind CSS, Magic Tags in a loop and template part CSS:
cd /repo; sed -n 285,296p inc/class-blocks-css.php; python3 - <<'EOF' p='/workspace/checks/T2/Test_Review_Rows.php' s=op…printedOK (3 tests, 18 assertions)(run R6). Loop and template part output match the base code in run R7.
Untested areas
- Full Site Editing and Customizer CSS printing were not rechecked. That code is unchanged.
🤖 Automated review · run code-review-agent_6ad0728f60be71.41682677.
🤖 Review agent — review posted ✅ on ff9f0098 · approved · 0 findings · 11 min
Run code-review-agent_6ad0728f60be71.41682677 · trail
Summary
1. Unauthenticated stored XSS via dynamic-content rewrites
The
render_blockfilters that rewrite#otterDynamicLink,/otter/v1/dynamicimage URLs and{otterDynamic?…}magic tags run on every block, including core blocks. Their regex tail (\/?.[^"]*) could consume the quote that closes an HTML attribute.Text printed straight after a token then became live attributes. A visitor's comment author name in the Latest Comments block was enough, with no account needed. Contributors could do the same in their own content.
Fix:
",', whitespace,<and>; magic tags require?and stop at}.esc_url(), as the siblingapply_link()already did. Pro's post-meta link returned raw meta.2. Contributor stored XSS via block style values
save_css_file()strips tags with kses, thencompress()runshtmlspecialchars_decode(). That turned entity-encoded markup in any style attribute, on any block, back into a real closing</style>, which was then printed raw inside<style>.Fix: new
CSS_Handler::escape_style_contents()rewrites</styleto the CSS escape\3c\2fstyle, the same escape core's HTML API applies, inlined because it is WP 6.7+ only. It runs:compress();The decode stays: kses encodes
>,&and<that valid CSS needs (child combinators, URLs, media ranges), and a test guards that.3. Unauthenticated disclosure via
otter/v1/dynamicroutescheck_permission()treated any post withpublishstatus as public. That status is also used by password-protected posts and non-viewable post types (synced patterns, templates, navigation, WooCommerce coupons).The image route also loaded any product by its
idparameter without checking it.Fix:
edit_postusers keep full access.is_post_type_viewable()and! post_password_required(). A visitor who has entered the post password still passes.idgets the same check.idisn't a product.4. Unauthenticated disclosure of condition-restricted blocks
Block Conditions skipped evaluation whenever
REST_REQUESTwas set oris_admin()was true. SoGET /wp/v2/posts/<id>returned every restricted block incontent.renderedandexcerpt.rendered. Front-end admin-ajax "load more" requests did the same, becauseis_admin()is true there too.Fix: conditions are now skipped only on:
is_admin() && ! wp_doing_ajax());context=editand come from a user withedit_posts.The REST state is a stack pushed and popped in
rest_request_before_callbacks/rest_request_after_callbacks. The capability check is required because those filters run before the route's own permission check.5. Contributor stored XSS via Atomic Wind CSS
POST otter/v1/atomic-wind/stylestores CSS for anyone who can edit the post. Its sanitizer stripped style and script tags in a single pass, so nested tags reassembled into a closing</style>.WordPress 6.6–6.9 print
wp_add_inline_style()content raw, so this was exploitable there. Newer cores escape it.Fix:
escape_style_contents()runs on save and whenever cached CSS is read.Behaviour changes to be aware of
content.renderedfiltered per user, matching the front end. Editor flows (edit context) are unchanged.global $product, which REST doesn't set. Their negated forms may now show a block in REST that the front end hides.Closes https://github.com/Codeinwp/otter-internals/issues/316
Checklist before the final review