docs: Security-Audit Etappe 4 mit Status nach der Fix-Welle
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Jan-Niclas Loosen
co-authored by
Claude Sonnet 5.5
Claude Sonnet 5.5
parent
99c7aab238
commit
1fdcb2e075
@@ -0,0 +1,293 @@
|
||||
# Etappe 4 (Designsystem: Presets, Zuweisen, Farben, Variablen, Theme-Optionen, Schriften, Schleifen): security audit
|
||||
|
||||
- **Scope:** branch etappe-4, `d391cb5^..5220876` (HEAD, 153 commits; `d391cb5` is the Etappe 4 spec, so this is the whole Etappe 4 and not only `d73144a^..HEAD`, which starts at plan 4c and would miss 4a/4b). 177 `src/` files changed. Focus per spec §12 and plan task 8:
|
||||
- (a) preset `attrs`/`from_node`;
|
||||
- (b) font upload and `RemoteFileFetcher`;
|
||||
- (c) mass writes (`PageRewriter`, `lit/divi-presets-assign`, `lit/divi-design-detach`, P8/P8a, `HARD_CAP`, `post_ids`);
|
||||
- (d) loop options (type boundary, S29/S30 for `loop_*_meta_key_`, X1 context, `StoredNodeGuard`, move warning, Z2);
|
||||
- (e) snapshot restore (rights per area, `MAX_BYTES`, FIFO, `DesignLock`, ID parsing, restoring stored data);
|
||||
- (f) theme option whitelist and sanitizers, `et_update_option`, color and variable adapters, `ItemKeys`/`GroupKeys`, rights in `permission()` and `execute()`, release gate, error oracles.
|
||||
- **Environment:** DDEV (WordPress 7.1.3, PHP 8.4, Divi 5.15.1), single site, one user (`administrator`), `DISALLOW_UNFILTERED_HTML` undefined, `et_pb_role_settings` empty (Divi defaults). Plugins `postlos` and `asynclistry` inactive before and after (left as found).
|
||||
- **Method:**
|
||||
- Read every new or changed `src/` file of the range in five slices (loops/dynamic content, fonts/fetch, presets/rewriter, snapshots/options/colors/variables, registrar/gate) and traced each write route to its checks.
|
||||
- Read the Divi 5.15.1 code wherever the plugin relies on Divi behaviour: `LoopUtils` (query args, `parse_and_duplicate_loop_blocks`, `_create_duplicated_block`, no-results path), `DynamicContentLoopOptions`, `DynamicContentOptionLoopPostMetaKey`, `DynamicContentACFUtils` (user meta), `DynamicData`/`DynamicContentUtils` (loop_id passing), `BlockParser` (global layout expansion), `GlobalData` (colors, variables, sanitizers), `FrontEnd/Module/Style.php` (variable CSS output), `epanel/custom_functions.php` (`et_update_option`), `PageResource`, `LoopPresetAttrsMap`, `GlobalPreset::prepare_data`, font upload (`et_pb_add_font`, `check_filetype_and_ext_font`); core `class-wp-query.php` (private status handling), `option.php` (`add_option`), Requests redirect hooks.
|
||||
- Ran read-only `wp eval-file` probes. Probe files were deleted afterwards. Covered: `et_divi` layout and key count, Divi role defaults for an in-memory editor, snapshot counts, the live loop option registry (34 options), `RemoteFileFetcher::allows()` against 20 host tricks, and `LoopUtils::parse_and_duplicate_loop_blocks` on in-memory block markup with an in-memory `get_post_metadata` marker filter (M1, L2).
|
||||
- Simulated roles with in-memory `WP_User` objects (never saved): editor without `edit_theme_options`, administrator without `unfiltered_html`, without `upload_files`, without the Divi role options `custom_fonts_management`/`variables_manager`/`theme_customizer`.
|
||||
- Nothing was written to the site. md5 before and after, identical: `et_divi` 771d6532…, `et_divi_global_variables` 191a4ce9…, `et_uploaded_fonts` a2072c8a… (empty), `et_divi_builder_global_presets_d5` 53586deb…, content hash of all pages/posts/library/TB posts f28463a0…; the `lit_mcp_snapshots_*` options unchanged. `git status` clean except this note.
|
||||
- `vendor/bin/phpunit`: 3449 tests, 21498 assertions, OK.
|
||||
|
||||
**Result:** no Critical or High findings. One Medium, nine Low, ten Info.
|
||||
- M1 is the one finding that defeats the type boundary of the Global Constraints silently: an empty inner loop hands its `loop_post_meta_*` content to an outer term/user loop.
|
||||
- The ruling Z2 (two-call route stays a warning) is acceptable only together with M1 and L2: today the warning is wrong for alias/case query types (L2), and M1 reaches the same result with no warning at all.
|
||||
- Ruling X7(d) (presets enabling loops) turned out harmless on render: Divi's loop pass reads raw block attrs, never presets. A preset-based loop switch is therefore not a finding.
|
||||
- W13 items hold as recorded except two: the `et_update_option` lost update also happens between two LIT tools (L5), and `DesignLock` is not atomic at all, not only racy on takeover (L6).
|
||||
|
||||
---
|
||||
|
||||
## Findings (most severe first)
|
||||
|
||||
### M1 – Medium (type boundary bypass): an empty inner loop lets the outer loop of another kind resolve `loop_post_meta_*`, with no refusal and no warning
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Compose/LoopContext.php:171-181` (`fromChain()`: the innermost enabled loop wins) and `src/Integrations/Divi/Compose/DynamicContentRules.php:142-165` (checks only that innermost scope).
|
||||
- Divi `includes/builder-5/server/Packages/Module/Options/Loop/LoopUtils.php:4451-4456`: a loop with no results returns its block unchanged, still holding its `$variable(…)$` references.
|
||||
- `LoopUtils.php:4572-4598`: the outer loop then runs `replace_loop_variables_in_attrs` / `process_loop_block_inner_blocks` with its own `query_type`/`query_item` over the nested block.
|
||||
- `DynamicContentLoopOptions.php:150-158`: post meta is read with `object_id = loop_id`, which is the term (or user) ID.
|
||||
|
||||
**Scenario**
|
||||
1. A page has a term loop (built in the Visual Builder, or switched on through `attrs`, X7a).
|
||||
2. Call 1: `lit-divi-insert` puts a Text module inside it with `attrs` `module.advanced.loop` = `{enable: on, queryType: post_types}` and a query that never matches (`includeSpecificPosts: [999999]`, a large offset, a type without posts). No loop content is in the call, so it passes.
|
||||
3. Call 2: `lit-divi-update` sets the module's text to `loop_post_meta_key_manual_custom_field` with any `loop_meta_key` that passes S29/S30 (no leading `_`). The node's own scope is post, so `fits()` holds and no warning is produced.
|
||||
4. On the frontend the inner loop has no results; the outer term loop resolves the variable: the meta of the post whose ID equals each term ID, whatever its status or type (drafts, private posts, ACF fields of other types).
|
||||
5. The account needs only `edit_post` on the page and dynamic-content rights. It is the result the Global Constraints call "keine Komfortmeldung", and unlike Z2 nothing tells the user.
|
||||
|
||||
**Evidence:** probe (in memory): `LoopUtils::parse_and_duplicate_loop_blocks` inside `the_content` on markup "category term loop > post loop with no hits > text with `loop_post_meta_key_manual_custom_field`", with a `get_post_metadata` filter that marks reads. The inner block kept `__loop_no_results: true`, and the text resolved to the marker for post 1 (= term ID 1). A stack trace showed the read coming from the outer loop's `_create_duplicated_block` (`:4591`).
|
||||
|
||||
**Fix (smallest)**
|
||||
- For meta-key options only (`str_contains($name, '_meta_key_')`), require every enabled loop in the ancestor chain to have the option's kind, not only the innermost one.
|
||||
- `LoopContext::fromChain()` collects the kinds of all enabled ancestors-or-self into `LoopScope` (e.g. `kinds(): list<string>`); `LoopScope::fits()` checks meta options against all of them, other `loop_*` options keep the innermost rule (their Divi resolvers check `instanceof WP_Post`/`WP_Term`/`WP_User` and return '' for a foreign object, `LoopUtils.php:3162+`, `3553+`, `3599+`).
|
||||
- The same chain rule must hold in the update/move warnings (`DiviWriteAbility::loopWarning()`).
|
||||
- Tests: post meta option in a post loop nested in a term loop is refused on insert and update; in a post loop nested in a post loop it is accepted.
|
||||
|
||||
### L1 – Low (disclosure, Divi parity): loop settings through `attrs` let any account with `edit_post` publish private posts of others and user e-mails/logins
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Compose/ValueCleaner.php:63-64`: `module.advanced.loop` is not in the builder-only paths, so any writer may set it (X7a; test `DiviLoopWriteTest.php:206-228`).
|
||||
- Divi `LoopUtils.php:1005-1037`: an empty `subTypes`, or one whose types are all invalid, falls back to `post_type = any`; `any` or `attachment` sets `post_status = [publish, inherit, private]`.
|
||||
- Core `wp-includes/class-wp-query.php:2685-2687`: without `perm => readable`, explicit `private` is merged into the visible statuses for every visitor. Divi never sets `perm`.
|
||||
- Divi `LoopUtils.php:922-990`: the user query has no capability check, `role__in` is free.
|
||||
|
||||
**Scenario**
|
||||
1. An author with dynamic-content rights builds a post loop with empty `subTypes` on their own page and uses `loop_post_title`/`loop_post_excerpt`. The published page lists other authors' private posts, although the author lacks `read_private_posts`.
|
||||
2. Or a user loop for role `administrator` with `loop_user_email`/`loop_user_username`: the page publishes admin e-mails and logins; `list_users` is bypassed. A contributor sees the same in preview.
|
||||
3. The Visual Builder allows the same to any account with builder access, so this is Divi parity. LIT adds no new effect but opens it to every MCP account with `edit_post`, while the guide says "Abfrage und Anzahl gehören in den Divi-Builder".
|
||||
|
||||
**Evidence:** by reading Divi and core (paths above). No private posts or second account exist on DDEV, so no end-to-end probe was run.
|
||||
|
||||
**Fix (product decision, behaviour change)**
|
||||
- Option A (one line, also closes step 2 of M1 and the Z2 update route for lower roles): add `advanced\.loop` to `ValueCleaner::BUILDER_ONLY` (needs `unfiltered_html`), like the other builder-only paths.
|
||||
- Option B (narrower): refuse a loop with `queryType` `user_roles` without `list_users`, and a post loop whose `subTypes` is empty, `any` or contains `attachment` without `read_private_posts`.
|
||||
- Recommendation: B, because it keeps loop editing for editors and closes only the two disclosures.
|
||||
|
||||
### L2 – Low (wrong warning, normalisation): `LoopScope` reads `queryType` raw, but Divi `sanitize_key()`s it and knows the aliases `terms`/`users`
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Compose/LoopScope.php:32-38`, `:76-79`; `LoopContext.php:162-165`.
|
||||
- Divi `LoopUtils.php:729-731` (`sanitize_key`), `:881-898` (`users` runs the user query, `terms` the terms query), `:4111-4128` (loop_id = term_id / user ID).
|
||||
- `src/Integrations/Divi/Pages/DiviWriteAbility.php:141-160` (warning text), `DiviUpdateAbility.php:207` (none → unsupported: both kinds null, no warning).
|
||||
|
||||
**Scenario**
|
||||
1. For LIT, `"Post_Taxonomies"`, `" post_taxonomies"`, `"terms"` and `"users"` are unsupported, so new `loop_*` content under them is refused (fail closed, good).
|
||||
2. Z2 route: post loop with `loop_post_meta_*` children, then `lit-divi-update` sets `queryType` to `terms` or `Post_Taxonomies`. The warning says the content is "jetzt außerhalb jeder Divi-Schleife; Divi zeigt sie dort leer".
|
||||
3. Divi in fact runs a term loop and prints post meta by term ID. The warning ruling Z2 relies on misleads the user into thinking the change is harmless.
|
||||
|
||||
**Evidence:** probes "queryType `terms`, flat" and "`Post_Taxonomies` > empty post loop" both resolved the post 1 marker (term ID 1).
|
||||
|
||||
**Fix**
|
||||
- In `LoopContext::ownScope()`, normalise like `sanitize_key`: `preg_replace('/[^a-z0-9_\-]/', '', strtolower($q))`.
|
||||
- Add `'terms' => TERM` and `'users' => USER` to `LoopScope::KINDS`.
|
||||
- In the update/move warning, word an unsupported target as "kann Daten eines anderen Objekts zeigen", not "leer".
|
||||
- Tests: `Post_Taxonomies` and `terms` are term scope; the update warning names the term kind.
|
||||
|
||||
### L3 – Low (type boundary, no check or warning): global instances and copies carry `loop_*` content across loop kinds
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/LibraryInsertAbility.php:152-163`: in mode `global`, `StoredNodeGuard` sees only the instance's local attrs; the entry content is referenced and Divi expands it before the loop pass (`BlockParser.php:917-924`, then `:947`).
|
||||
- `src/Integrations/Divi/Compose/StoredNodeGuard.php:64-66`: no checks at all for `unfiltered_html` accounts.
|
||||
- `DiviNodeCopyAbility.php:172`, `DiviMoveAbility.php:93`: `LoopContext::optionsIn()` skips opaque nodes (`LoopContext.php:192-194`), so moving a global instance never warns.
|
||||
|
||||
**Scenario**
|
||||
1. A library entry holds `loop_post_meta_key_*` (built in the VB, or saved by an `unfiltered_html` account with `lit/library-save`).
|
||||
2. Any account with library and page rights inserts it as a global instance into a term loop: post meta by term ID, no warning.
|
||||
3. `unfiltered_html` accounts can also `lit/divi-node-copy` or `lit/library-insert` (copy mode) loop content into any loop kind, again without the warning that move and update give.
|
||||
|
||||
**Fix**
|
||||
- In `library-insert` (global and copy) and `node-copy`, collect `optionsIn()` of the content that will render (entry blocks or the copy) for `_meta_key_` options and compare with the target's `loopAt()` under M1's chain rule; refuse, or at least add `loopWarning()`, for every account including `unfiltered_html`.
|
||||
- In move, enter global instances through their entry, or warn whenever an opaque global instance changes loop kind.
|
||||
|
||||
### L4 – Low (authorization, P8/P8a): `lit/divi-design-restore` never computes affected pages, so it skips `blocked_ids` and `in_use`
|
||||
|
||||
**Where**
|
||||
- `PresetRestorePlanner.php:64-72`, `ColorRestorePlanner.php:67`, `VariableRestorePlanner.php:54-60`, `ThemeOptionsRestorePlanner.php:62-69` build `DesignPlan(..., usages: null)`.
|
||||
- So `DesignChange::blocked()` (`DesignChange.php:200-219`) is always null on restore, and the dry run shows `affected: []`.
|
||||
- Compare the set tools, which run `ReferenceUsageQuery`: `DiviColorsSetAbility.php:202-218`, `DiviVariablesSetAbility.php:179-195`, `DiviPresetsDeleteAbility.php:174-176` (`in_use`), `:196`. Spec §4 step 4 requires `affected`/`blocked_ids` for every design write tool.
|
||||
|
||||
**Scenario**
|
||||
1. An account with `edit_theme_options` and Visual Builder rights lacks `edit_post` on live page X, which uses color C.
|
||||
2. `lit/divi-colors-set` on C is refused (P8a).
|
||||
3. The same account restores an older colors snapshot in which C had another value or did not exist: page X changes without a check. The same works for variables, and for presets (restore removes presets in use without `in_use`).
|
||||
4. Limits: only earlier states are reachable, the account is administrator-level, and Divi's builder lets it change globals anyway.
|
||||
|
||||
**Fix:** in each planner except theme options, collect IDs whose diff action is `changed`, `removed` or `deactivated` and pass `$usages->reportFor(new ReferenceUsageQuery($ids))` (presets: `PresetUsageQuery::withGroups`) as the fourth `DesignPlan` argument; inject `GlobalUsages` through the constructors (`DiviIntegration`). Keep the preset orphan warning (spec §5) but let P8a block. Test: a colors restore that changes a color used on an uneditable live page answers `forbidden` with `blocked_ids`.
|
||||
|
||||
### L5 – Low (integrity, W13 extended): colors and theme options take different locks but both rewrite the whole `et_divi` array from Divi's request global
|
||||
|
||||
**Where**
|
||||
- Divi `GlobalData::set_global_colors()` (`GlobalData.php:682-727`) and `ThemeOptionsAdapter::write()` (`ThemeOptionsAdapter.php:398-403`) both end in `et_update_option()`, which writes `$GLOBALS['et_theme_options']`, loaded once per request (`epanel/custom_functions.php:276-312`). Probe: `et_divi` has 153 keys, `et_global_data` lives inside it.
|
||||
- `DesignLock.php:184-191`: `lit_mcp_lock_colors` and `lit_mcp_lock_theme_options` are different locks; each `design_rev` covers only its own keys.
|
||||
|
||||
**Scenario:** a confirmed `lit/divi-colors-set` and a confirmed `lit/divi-theme-options-update` overlap, or the theme options request loaded `et_divi` before the colors write. The later `et_update_option` writes its stale copy back and silently reverts the global and customizer colors. Both read-backs pass because each checks only its own keys; the colors snapshot and log describe a state that is gone. Same principal, integrity only.
|
||||
|
||||
**Fix:** map `colors` and `theme_options` to one lock name in `DesignLock::option()`, and under the lock refresh `$GLOBALS['et_theme_options'] = get_option('et_divi');` before writing (`ThemeOptionsAdapter::write()`, `GlobalColorsAdapter::save()`).
|
||||
|
||||
### L6 – Low (concurrency): `DesignLock::acquire()` is not atomic, and a stale takeover lets the old holder release the new holder's lock
|
||||
|
||||
**Where**
|
||||
- `DesignLock.php:172-176` (docblock "add_option() is atomic") and `:193-207`.
|
||||
- Core `add_option()` (`wp-includes/option.php:1119-1125`, `:1142`) checks with `get_option` first and then runs `INSERT … ON DUPLICATE KEY UPDATE`; two concurrent requests can both get `true`.
|
||||
- The plugin already knows the safe pattern: `src/Integrations/Divi/Release/DiviCheckRoute.php:19-22` (`INSERT IGNORE` plus compare-and-swap).
|
||||
- Stale takeover after 120 s: a long confirmed run (`PageRewriter` with 50 pages) loses its lock to B; A's `finally release()` (`DesignChange.php:97-99`) deletes B's lock, so C enters too.
|
||||
|
||||
**Scenario:** two confirmed changes of one area both pass `design_rev` and write; last write wins, and snapshots and change log disagree with the stored state. Same principal, integrity only. This also widens the recorded `attachLog`/RMW and takeover items.
|
||||
|
||||
**Fix:** reuse `DiviCheckRoute`'s pattern: `INSERT IGNORE` with a token `time() . ':' . wp_generate_password(8, false)`, success only on `rows_affected === 1`; take over a stale lock with `UPDATE … WHERE option_value = <read value>`; release only while the token still matches.
|
||||
|
||||
### L7 – Low (CSS injection without `unfiltered_html`): image variable values can leave the `:root` rule
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Design/VariableValue.php:36` (`WEB_URL = /^https?:\/\/[^\s<>"]+$/iD`) allows `; { } ( ) '`.
|
||||
- Divi `sanitize_global_variables_data` applies `sanitize_text_field` to images (`GlobalData.php:836-878`), which keeps them.
|
||||
- Divi `FrontEnd/Module/Style.php:1503-1505` wraps images as `"url({$result})"` without escaping, inside `:root{--gvid-…: …;}`.
|
||||
- `numbers` and `fonts` already exclude `;{}<>` (`VariableValue.php:62`, `:101`); `images` is the gap.
|
||||
|
||||
**Scenario:** an administrator without `unfiltered_html` (multisite site admin, or `DISALLOW_UNFILTERED_HTML`) holding `variables_manager` sets an image variable to `https://x.test/a.png);}body{display:none}a{b:url(x`. Every page that uses the variable gets the injected CSS (hide content, overlays, tracking `url()`). No `<`, so no `</style>` and no script. Spec §4 step 2 reserves raw CSS for `unfiltered_html`. Divi's own variable manager accepts the same value (parity), hence Low.
|
||||
|
||||
**Evidence:** `preg_match(WEB_URL, 'https://x.test/a.png);}body{display:none}a{b:url(x')` returns 1; the Divi output line was read.
|
||||
|
||||
**Fix:** `WEB_URL = '/^https?:\/\/[^\s<>"\'(){};\\\\]+$/iD'` (and the same for a `/`-relative branch if images accept it). Test: the value above is refused.
|
||||
|
||||
### L8 – Low (SSRF, transport): the IPv4-only rule and the redirect checks around cURL do not apply to the fsockopen transport
|
||||
|
||||
**Where:** `src/Integrations/Media/RemoteFileFetcher.php:59-64`, `:72`. The `http_api_curl` hook does not fire on the Requests fsockopen transport; `PublicAddress::dnsResolver()` and `wp_http_validate_url` check IPv4 only.
|
||||
|
||||
**Scenario:** on a host without ext-curl, `stream_socket_client` may connect over AAAA. A name with a public A record and an AAAA record of `::1` or `fd00::…` reaches an internal IPv6 target (fonts: blind oracle only; media import: an image/PDF answer becomes a readable attachment). Needs a server without cURL, which is rare; DDEV has cURL.
|
||||
|
||||
**Fix:** in `fetch()`, return `failed(…)` when `! function_exists('curl_init')`, or restrict this request to cURL with a scoped `http_api_transports` filter (`fn () => ['curl']`).
|
||||
|
||||
### L9 – Low (DoS): the page rewriter parses every candidate post on each dry run and confirm, with no cap on drafts
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/GlobalUsages.php:101-116` (`rowsFor()`: up to `PARSE_CAP` 5000 parsed, the rest appended).
|
||||
- `src/Integrations/Divi/Design/PageRewriter.php:184-216` (`changes()`: `loadWritable()`, parse and `TreeRewrite` for every row, for `preview()` and `apply()`, beyond `PAGE_LIMIT` 50 because the remainder is counted); `:89`, `:140` (`reportFor()` first, so candidates are scanned twice).
|
||||
- `HARD_CAP` (2000) counts live posts only.
|
||||
|
||||
**Scenario:** a site with thousands of drafts holding `divi/text`; every `lit/divi-presets-assign module=text` dry run parses them all, up to `max_execution_time`. Memory stays bounded by chunks of 50. A timeout during `apply()` keeps the posts saved so far (each with a revision) but returns no `partial_write`. Administrator-level actor, effect ends with the requests.
|
||||
|
||||
**Fix:** stop the in-memory rewrite after `PAGE_LIMIT` changed posts plus a bounded counting window (e.g. 500 candidates) and report `remaining` as "mindestens N"; or refuse above a total candidate cap (all statuses) unless `post_ids` is given. Test: 600 candidate rows load at most `PAGE_LIMIT` plus the window.
|
||||
|
||||
---
|
||||
|
||||
## Info / hardening
|
||||
|
||||
### I1 – Preset-side variable detach inlines `before`/`after` without the marker check of the page-side detach
|
||||
- `PresetBook.php:366-378` (`resolved()` concatenates affixes raw) vs `VariableDetachRewrite::hasUnsafeAffix()`/`unsafe()` (`VariableDetachRewrite.php:41-45`, `120-147`).
|
||||
- `markupOutside([])` (`DiviDesignDetachAbility.php:429`) and Divi's `prepare_data()` catch `<`/`>`, but a `$variable(…)$` stored in an affix by an `unfiltered_html` account becomes a live, never-canonicalised reference once inlined. Fix (about 5 lines): skip or refuse a reference whose affixes are `unsafe($text, markupAllowed: true)` and list it in the "eingebettet" note.
|
||||
|
||||
### I2 – A detached variable value is inlined without cleaning for the target field type
|
||||
- `VariableDetachRewrite::replacement()` (`:51-61`); only `<`/`>` and markers are checked (`DiviDesignDetachAbility.php:277-291`). A strings value `javascript:…` lands raw in a link field. Divi escapes `href`/`src` with `esc_url` on render (`HTMLUtility.php:791-793`, `1054-1068`), so not exploitable. Optional: `ValueSanitizer::url()` when `kindAt()` is Url.
|
||||
|
||||
### I3 – For `unfiltered_html` accounts, `from_node` copies `loop_*`/`custom_meta_*` references unchecked into a preset
|
||||
- `PresetComposer.php:75-82`; `StoredNodeGuard::problems()` returns `[]` for them (`StoredNodeGuard.php:64-66`). The `attrs` path refuses every `loop_*` (no scope). A preset can then carry a loop reference into modules of any loop kind. Administrator actor. Optional: run the taken attrs through the composition's `references()` for the loop rule regardless of `unfiltered_html`.
|
||||
|
||||
### I4 – Read tools check rights only in `permission()`
|
||||
- `DiviPresetGetAbility.php:104-109`, `DiviDesignSystemAbility`, `DiviDesignSnapshotsAbility`. `AbilityRegistrar.php:66-82` always runs `permission_callback` first, so unreachable; inconsistent with the write tools and `DiviThemeOptionsGetAbility.php:114-118`.
|
||||
|
||||
### I5 – Shortcodes and custom-field conditions inside term/user/menu loops run against the post with the term/user ID
|
||||
- Divi `LoopUtils.php:4507-4510` (`__loop_post_id` = term/user ID), `DynamicContentUtils.php:669-690`, `ModuleElements.php:1718-1771` (`override_post_data()` → `get_post(term_id)`), `Conditions.php:928-940`. `[acf]`, `[pods field=…]` and similar in a text module in a term loop read the unrelated post. Outside the `loop_*` check and Divi behaviour; document in the guide. Optional: refuse shortcodes under a non-post loop without `unfiltered_html`.
|
||||
|
||||
### I6 – Font dry run reveals whether a host name resolves privately
|
||||
- `DiviFontsUploadAbility.php:216` ("nur öffentlich erreichbare http(s)-Adressen") vs "nicht abrufbar": internal DNS enumeration for an administrator-level principal. Media import behaved so before Etappe 4.
|
||||
|
||||
### I7 – Font confirm is not bound to the dry run
|
||||
- Confirm downloads again (`DiviFontsUploadAbility.php:288` ff., `download()` `:320`) without comparing the SHA-256 shown in the dry run. The content is re-checked, so nothing unsafe gets in. Optional `expected_sha256` input.
|
||||
|
||||
### I8 – Font download time and leftovers
|
||||
- Up to 4 files × 5 MB × 30 s (`TIMEOUT`, `:24`), both in dry run and confirm; a slow server holds a worker about 120 s per call. If PHP is killed mid-download, `wp_tempnam` files stay (cleanup only in `finally`). Administrator-only.
|
||||
|
||||
### I9 – Lost update on `et_uploaded_fonts`
|
||||
- `font_add` reads and writes the whole option without a lock (fonts have no pipeline, A2). Concurrent uploads, or an upload next to a Divi UI upload, can drop an entry and leave orphan files in `et-fonts`. Same class as W13.
|
||||
|
||||
### I10 – `numbers` variables accept `url(https://…)`
|
||||
- No breakout (`esc_html`'d, `;{}<>` excluded), but a tracking request from every page is possible. Acceptable.
|
||||
|
||||
---
|
||||
|
||||
## Deferred items re-checked
|
||||
|
||||
| Item | Verdict |
|
||||
|---|---|
|
||||
| W13 lost update of `et_update_option` | Confirmed, and wider than recorded: also between `colors-set` and `theme-options-update` (L5). **Zu beheben** (L5); against Divi's own saves acceptable. |
|
||||
| W13 `body_header_style` | One of italic/uppercase/underline only; Divi can combine. Functional, not security. **Akzeptabel.** |
|
||||
| W13 missing range check in Divi | `ThemeOptionSpec::check()` enforces min/max/step/choices before Divi's sanitizer; a sanitizer change is an error (`DiviThemeOptionsUpdateAbility.php:183-193`). **Akzeptabel.** |
|
||||
| W13 `font_add` partial success | `FontInstaller::install()` (`:402-450`) compares the formats Divi stored (`FontInstaller.php:122-124`) and calls `font_remove($name)` on a shortfall; the name rules guarantee Divi's stripped name equals `$name`; a failed removal is `WriteFailed::partial` with a hint. **Akzeptabel.** |
|
||||
| W13 `remove_static_resources` only with a logged-in user | Divi checks `et_core_security_check_passed('edit_posts')` (`PageResource.php:1404`); MCP requests are always authenticated. Only CLI/cron would miss the CSS clear. **Akzeptabel.** |
|
||||
| W13 tablet/phone 14/30 quirk | Display defaults and inheritance only. **Akzeptabel.** |
|
||||
| W13 DNS-rebinding window between `allows()` and cURL | Confirmed (`RemoteFileFetcher.php:37-42` vs `:51-84`, redirects `:67-71`; nothing pins the checked IP; ports 80/443/8080). For fonts a blind boolean oracle only (magic bytes + finfo, nothing stored): Low. For media import (authors with `upload_files`) an internal image/PDF answer becomes a readable attachment; that is the 3a/3b "DNS pinning" item, carried over unchanged. **Zu beheben** (about 15 lines): resolve once in `fetch()`, check, and pin with `CURLOPT_RESOLVE` in the existing `http_api_curl` hook, redirect hosts likewise. |
|
||||
| W13 schema errors without lit field paths | UX only; `ThemeOptionSpec` errors carry `set.<key>`. **Akzeptabel.** |
|
||||
| Ruling R9 (rewriters without `unfiltered_html`) | No client string enters page content: assign writes preset IDs through `PresetAssignment` → `keepsId`, preset detach removes IDs, variable detach refuses `<`/`>`/markers and unsafe affixes per block without `unfiltered_html`. `DiviSaver`'s kses window keeps existing markup byte-identical. **Akzeptabel.** |
|
||||
| P8a with `post_ids` | Only named posts are rewritten (`PageRewriter.php:187-189`), each through `loadWritable()` (`edit_post`, Library/TB access and group gate); a named library entry gets its own usage report (`:282-292`). **Akzeptabel.** |
|
||||
| `SnapshotStore::attachLog` without error check, RMW without lock | Inside the area lock, but the lock is not atomic (L6). Worst case a missing `log_id` link. **Akzeptabel**, resolves with L6. |
|
||||
| `DesignLock` takeover race | Worse than recorded: `acquire()` itself is not atomic (L6). **Zu beheben** (L6). |
|
||||
| X7(a) loops switchable through `attrs` | Holds as designed for the type boundary, but enables L1 (private posts, user data). **Produktentscheidung** (L1, options A/B). |
|
||||
| X7(b) global modules without context | `loop_*` in library entries is refused (no scope). The inbound direction via global instances is L3. **Akzeptabel** for entry edits; L3 zu beheben. |
|
||||
| X7(c) copying fail closed | Holds for accounts without `unfiltered_html` (`StoredNodeGuard` composes with scope none, every `loop_*` refused). `unfiltered_html` accounts get no check and no warning (L3). **Akzeptabel** with L3. |
|
||||
| X7(d) presets with loop | No render effect: `LoopUtils::parse_and_duplicate_loop_blocks` parses the raw document with `WP_Block_Parser` and reads raw `attrs` (`LoopUtils.php:4219-4229`, `718-733`); presets never start, stop or retype a loop on the frontend. X1 matches Divi's render. **Akzeptabel** (fail-closed limit only). |
|
||||
| X7(e) ACF repeater fields | `loop_acf_*` has no kind, `repeater_*` is unsupported, all refused. **Akzeptabel.** |
|
||||
| Ruling Z2 (two-call route stays a warning) | Impact: non-protected post meta of posts whose IDs equal term/user IDs. Acceptable as Low **only** while the warning is accurate and no silent route exists; today L2 makes the warning wrong and M1 is silent. Recommendation: fix M1 and L2; for `_meta_key_` options refuse (instead of warn) on update/move, ordinary loop editing stays possible. |
|
||||
| Mirror `customizer-typography` | Fingerprints all of `et_divi_customizer_theme_settings`; any customizer edit locks ThemeOptionRead/Write (fail closed, noisy). **Akzeptabel** as documented shortcut. |
|
||||
| Ledger lead: `LoopContext::of` skips an opaque node's own loop | Used only by dynamic-sources (a listing); write tools never write onto opaque blocks; `loopAt()` does not skip. Info. **Akzeptabel.** |
|
||||
| Ledger lead: `kindOfOption` defaults to POST | `loopFieldsFor` needs both `fits()` and a map entry from `loopFields(kind)` (Divi group; meta options by name, matching Divi's own prefix rule, `DynamicContentOptionLoopPostMetaKey.php:334-345`). An unknown name never passes. **Akzeptabel.** |
|
||||
| Ledger lead: user meta via manual `loop_meta_key` in user loops | Divi's user branch of `get_meta_value_by_type()` requires the **viewer** to hold `manage_users` (non-ACF) or `list_users` (ACF) (`DynamicContentACFUtils.php`). No visitor leak. **Akzeptabel**, documented as parity; the real user exposure is L1. |
|
||||
| Ledger lead: Divi passes `loop_id`/`loop_query_type`/`loop_object` to every content variable | Non-loop options resolve `post_id` from `$value['post_id'] ?? get_dynamic_content_post_id()` (the page); `VariableRef::canonical()` allows only `name`/`settings`, so `post_id` cannot be injected. Of the non-loop resolvers only the repeater branch of `DynamicContentOptionPostMetaKey` and `get_archive_term_thumbnail_id` (returns 0) read loop data. No crossing; the real crossings are M1 and I5. **Akzeptabel.** |
|
||||
| Ledger lead: none → unsupported loop switch without warning | Confirmed, but no `loop_*` content can exist under none. The real gap is L2. **Akzeptabel** with L2. |
|
||||
|
||||
---
|
||||
|
||||
## Areas checked and found sound
|
||||
|
||||
- **Rights matrix** (`permission()` and `execute()` both check, unless noted in I4):
|
||||
|
||||
| Tool | Checks | Divi features |
|
||||
|---|---|---|
|
||||
| `lit/divi-colors-set` | `DesignRights::colors`: `edit_theme_options`, Visual Builder | DesignRead, ColorWrite |
|
||||
| `lit/divi-variables-set` | `edit_theme_options`, Visual Builder, `variables_manager` | DesignRead, VariableWrite |
|
||||
| `lit/divi-presets-set`, `-delete`, `-assign` | `DesignRights::presets` (`edit_theme_options`, Visual Builder); `markupOutside()` without `unfiltered_html` | assign/detach also PageRead/Write, Library, Theme Builder |
|
||||
| `lit/divi-design-detach` | rights per preset or variable | as above |
|
||||
| `lit/divi-theme-options-update` | `edit_theme_options`, `theme_customizer` | ThemeOptionRead, ThemeOptionWrite |
|
||||
| `lit/divi-theme-options-get` | `edit_posts` | ThemeOptionRead |
|
||||
| `lit/divi-design-restore` | the area planner's rights; an invalid ID needs all areas' rights | per area, or all |
|
||||
| `lit/divi-design-snapshots` | `edit_theme_options` (`permission()` only) | none |
|
||||
| `lit-divi-design-system`, `lit/divi-preset-get` | `edit_posts` (`permission()` only) | DesignRead (+ Catalog) |
|
||||
| `lit/divi-fonts-upload` | `upload_files`, `edit_theme_options`, `custom_fonts_management`, fail closed without Divi | FontUpload |
|
||||
|
||||
`AbilityRegistrar` runs access, OAuth, the guards (`DiviFeatureGate`) and then `permission()` in `permission_callback`, and the guards again in `execute_callback`. `DesignChange::apply()` asserts the feature before the snapshot, each adapter again before writing, `FontInstaller::install()` before any write. In-memory editor: refused everywhere for lack of `edit_theme_options`, although Divi's defaults grant `theme_customizer`, `variables_manager` and `custom_fonts_management`.
|
||||
- **Preset attrs (XSS):** `set`/`attrs` go through `Composer::update()` (ValueCleaner S7/S26, URL/HTML rules); `from_node` through `StoredNodeGuard` on the whole subtree; every path must be a Divi preset attribute, so text, links and images are refused (`PresetComposer.php:94-102`). A changed preset holding markup is refused without `unfiltered_html` (`DiviPresetsSetAbility.php:541`, same in delete/detach). Divi's `prepare_data()` sanitizes again (`DesignSystemWriter.php:103`). CSS values without `<` cannot leave the `<style>` block.
|
||||
- **Mass writes:** usages and P8 in presets-set/delete with `withGroups`; delete refuses `in_use` on explicit usages including drafts; TB layouts need ThemeBuilderAccess (hidden ones read as not found and are never named); blocked library entries refuse (P8a), site-wide entries and layouts without revisions are skipped; `read_post` before naming, one `NO_CHANGE` text for every reason (`PageRewriter.php:60-61`, `230-241`); global instances opaque, only listed in `skipped`. `Serializer` escapes `--` and `<`, so an inlined value cannot close the block comment.
|
||||
- **`ItemKeys`/`GroupKeys`:** `strval` on numeric keys; groups resolved only against Divi's groupId paths; `PresetBook` casts IDs to string at every lookup and writes with `array_values`; `PresetDiff` merges key lists only. No renumbering of option arrays found.
|
||||
- **Loop type boundary (bypass attempts, all held):** control characters (stripped by `ControlChars` before `canonical`), case or leading space in names (not `loop_`-prefixed → unknown), JSON `\u` escapes (decoded before the checks), `{{loop:…}}` (through `Placeholders` → `canonical`), same-call loop attrs (Insert refuses on the composed blocks, `DiviInsertAbility.php:107`; Update re-composes in the new scope, `DiviUpdateAbility.php:240-246`), tablet/hover/`true` enable (neither LIT nor Divi counts them), integer/bool `queryType` (unsupported → refused), `loop_position` (changes only `loop_object`; post meta uses `loop_id`, term/user branches check `instanceof`). S29/S30 cover `select_loop_meta_key` and `loop_meta_key`; prefix stripping matches Divi's `get_meta_key_from_name`. Registry unavailable → default map → every `loop_*` refused. Live registry: 34 loop options, groups as assumed, `enable_html` refused.
|
||||
- **Theme options:** 20 typography and layout keys, no custom CSS or head code; unknown keys refused at write (`ThemeOptionsAdapter.php:393-396`); font names without `<>"'\`` and control characters, exact Divi font required (`FontCheck`).
|
||||
- **Colors and variables:** colors only hex, keywords or functions without `( ) ;` or quotes; string variables rendered through `wp_kses_post` (`DynamicContentGlobalVariableOptions.php:149-153`); fonts and numbers without breakout characters (images: L7).
|
||||
- **Snapshots:** strict ID regex `^(colors|variables|presets|theme_options)-\d{8}T\d{6}Z-[0-9a-f]{6}$/D`; area taken from the ID, lookup by area and `===`; not found says nothing about other areas. `MAX_BYTES` 2 MB (413 before writing), FIFO 20, `TOTAL_BYTES` 8 MB keeping the newest, not autoloaded. The list shows `user_id`, `ability`, `log_id`, never `data`; data appears only as a restore diff behind the area's rights. Stored data is not re-validated on restore, but it is written only by `SnapshotStore::save()` from Divi's own inventory and no tool writes `lit_mcp_snapshots_*` (excluded from the settings whitelist), so changing it needs database or option access; partial re-checks still run (presets `markupOutside` both ways, Divi colors/variables sanitizers and `validate_global_variable_ids`, theme option whitelist). Rollback restores the snapshot on failure; a failed restore becomes `partial_write` naming the snapshot.
|
||||
- **Font upload:** `allows()` denied `2130706433`, `0x7f.1`, `017700000001`, `127.1`, `[::ffff:127.0.0.1]`, `[::ffff:7f00:1]`, `localhost`, `localhost.`, `0`, `[::]`, `[fd00::1]`, `169.254.169.254`, `[64:ff9b::a9fe:a9fe]`, port 22, `ftp://`, `user:pw@`, the site's own host and public IPv6 literals (cURL is IPv4-only). Redirect hook name matches `WP_HTTP_Requests_Hooks::dispatch`; relative locations are absolutised first; both hooks removed in `finally`. Size `limit_response_size = max+1`, `max = min(upload limit, 5 MB)`, at most 4 files, empty files refused. Type: own magic bytes (`FontSignature`) must equal the format, then Divi's finfo check (fails closed without fileinfo), then `wp_handle_upload` with `test_type`. Name `[A-Za-z0-9 _-]`, length per Divi, case-insensitive collision check against Google, web-safe and uploaded fonts before any download; `sanitize_file_name`, Divi's `upload_dir_font`, `wp_unique_filename`: no traversal. Temp files deleted in `finally` on every path. Network reasons go to the log only (URL without credentials, query, fragment). The media import extraction behaves as before Etappe 4.
|
||||
|
||||
## Status nach der Fix-Welle (2026-10-20)
|
||||
|
||||
Basis: Audit-HEAD `5220876`, Stand danach `99c7aab` (`composer test`: 3496 Tests grün, `composer lint` sauber; Smokes `divi-loop`, `divi-colors-variables`, `divi-theme-fonts`, `divi-presets` grün).
|
||||
|
||||
| Befund | Status |
|
||||
|---|---|
|
||||
| M1 | behoben in `522fc8f` (Kettenregel für `_meta_key_`-Optionen), `3a89b15` (Warnung nur bei Inhalten, die vorher passten) |
|
||||
| L1 | Produktentscheidung Option B, behoben in `4a8a889`, `71c2e03` (Prüfung auf dem bereinigten Wert), `6e84138` (`queryType: null` wie Divi) |
|
||||
| L2 | behoben in `522fc8f` |
|
||||
| L3 | **offen**: globale Instanzen und Kopien (`unfiltered_html`) tragen `loop_*`-Inhalt über Schleifen-Arten ohne Prüfung. Eigener kleiner Plan nötig; bis dahin als Grenze zu behandeln. |
|
||||
| L4 | behoben in `bd80ab1`, `bb54870` (Restore berechnet Fundstellen; P8a `blocked_ids`, `affected` im Probelauf). Bewusst kein `in_use` bei Presets (Spec §5: verwaiste Presets erlaubt, Warnung bleibt). |
|
||||
| L5 | behoben in `b59a0c0`, `1b3ef55`, `c4ee195`, `99c7aab`, `f754772` (ein Lock für Farben und Theme-Optionen; unter der Sperre Divis Laufzeit-Caches verwerfen, gegen echtes Divi für Farben, Variablen und Presets nachgewiesen; Uninstaller räumt alte Lock-Namen) |
|
||||
| L6 | behoben in `b59a0c0` |
|
||||
| L7 | behoben in `06b798a` |
|
||||
| L8 | teilweise behoben in `20e0216` (kein Abruf ohne cURL). Rest: bei https mit einem cURL ohne SSL kann Requests auf fsockopen ausweichen; selten, **bewusst belassen**. |
|
||||
| L9 | **bewusst belassen**: Administrator-Niveau, Wirkung endet mit dem Request; ein Cap ändert das Verhalten und ist eine eigene Entscheidung. |
|
||||
| I1–I10 | unverändert (Hinweise, kein Fix in dieser Welle) |
|
||||
|
||||
Rest-Risiken aus „Deferred items re-checked": DNS-Rebinding behoben in `20e0216` (`CURLOPT_RESOLVE`, auch für Weiterleitungsziele); `DesignLock`-Übernahmerennen behoben (L6); Lost Update zwischen Farben und Theme-Optionen behoben (L5); alle übrigen Punkte wie oben als „Akzeptabel" bewertet. Ruling Z2 (Zwei-Aufruf-Weg nur Warnung) hält jetzt, weil M1 und L2 geschlossen sind.
|
||||
|
||||
Re-Reviews: Opus-Re-Review (Wellen 1+2) fand L1-Umgehung und unvollständiges L5; Sonnet-Verifikation fand die `queryType: null`-Regression und das Cache-Problem bei Farben/Variablen/Presets. Beide Funde sind behoben. Die letzten beiden Commits (`6e84138`, `99c7aab`) wurden nicht noch einmal reviewt; L5 ist stattdessen per Probe gegen echtes Divi belegt.
|
||||
Reference in New Issue
Block a user