docs: Etappe 3a rulings and security audit
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,210 @@
|
||||
# Etappe 3a (Divi-Bibliothek): security audit
|
||||
|
||||
- **Scope:** branch etappe-3, cfc81dd..186f7af. Focus: what the per-task reviews covered least (object authorization, global edits, stored-content injection, disclosure, DoS).
|
||||
- **Environment:** DDEV (WordPress 7.1.x, Divi 5.2.1), single site, no editors/authors present, `DISALLOW_UNFILTERED_HTML` off.
|
||||
- **Method:** read all new/changed `src/` files of the range plus the relevant Divi 5 render code (`BlockParser`, `BlockParserStore`, `GlobalLayout`, `DynamicContentOptions*`, `LinkUtils`, MultiView, Divi 4 `class-et-builder-element.php`), scanned all `script-library-*.js` for DOM sinks, and ran four self-cleaning `wp eval-file` probes (temp posts 1906–1914, temp user `litaudit_author`; all deleted, verified: 0 `LITAUDIT` posts, 0 postmeta/term rows for those IDs, no temp user, no probe files, `git status` clean). No [Fixture]/[Test] content was touched; no code was changed.
|
||||
|
||||
**Result:** no Critical, High or Medium findings. The rulings P3/P5/P8a/P11/P12/P15–P19 hold. Six Low findings and three Info/hardening notes follow.
|
||||
|
||||
---
|
||||
|
||||
## Findings (most severe first)
|
||||
|
||||
### L1 – Low (DoS): the P8a check loads every live usage with full content, and each dry run scans `wp_posts` 3×
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/GlobalUsages.php:68-79` (`liveIds()`: no LIMIT).
|
||||
- `src/Integrations/Divi/Library/GlobalEdits.php:158-171` (`_prime_post_caches($live, …)`, then `current_user_can('edit_post')` per ID).
|
||||
- `GlobalUsages.php:29-47`: COUNT plus SELECT, each an unindexed `post_content LIKE '%…%'`.
|
||||
- Also reached from `GlobalEdits::notice()` (every delete dry run) and `LibraryListAbility::detail()`.
|
||||
|
||||
**Scenario**
|
||||
1. A global module (newsletter box, CTA) sits in thousands of posts. That is a common Divi pattern.
|
||||
2. Any user with library rights and `edit_post` on the entry calls a Divi tool on it without `confirm` (dry run), or `lit/library-list` with `id`.
|
||||
3. `_prime_post_caches()` fetches `posts.*` for every live ID in one query, `post_content` included. At 5,000 posts × ~50 KB of Divi markup that is about 250 MB, which exceeds the usual `memory_limit` and ends the request in a fatal error.
|
||||
4. Each call also runs 3 full-table LIKE scans (6 on confirm, because dry run and confirm both call `of()` and `blocked()`). A loop of calls ties up the database on large sites.
|
||||
5. The effect stops when the requests stop, and it needs an authenticated MCP user.
|
||||
|
||||
**Evidence:** by reading. The DDEV data set is too small to reproduce the memory limit.
|
||||
|
||||
**Fix**
|
||||
- Use one query: `SELECT ID, post_status, post_type, post_author, post_title … LIKE …`. Derive `count`, the first 50 items and the live list in PHP, instead of COUNT + SELECT + `liveIds()`.
|
||||
- Evaluate `edit_post` in chunks of about 200 IDs, calling `_prime_post_caches()` per chunk and `clean_post_cache()` or `wp_cache_flush_runtime()` between chunks.
|
||||
- Or fail closed: above a hard cap (e.g. 1,000 live usages), refuse the confirmed edit with a hint ("im Divi-Builder bzw. von einem Administrator bearbeiten").
|
||||
|
||||
### L2 – Low (global edits): "global" is decided by the scope term, but Divi renders every referenced entry, so edits to referenced non-global entries skip the dry run and P8a
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/GlobalEdits.php:71` and `:88-90`: `if (! $global) return LibraryItem`.
|
||||
- `src/Integrations/Divi/Library/DiviLibraryStore.php:37` (`isGlobal()` → `GlobalLayout::is_global_layout_template()`, which checks the scope term, with a meta fallback for modules only).
|
||||
- Divi: `BlockParser.php:838-880` renders any `divi/global-layout` whose `globalModule` names a *published `et_pb_layout`*. It checks only `post_type` and `post_status`, not the scope.
|
||||
|
||||
**Scenario**
|
||||
1. A live page that user U cannot edit holds an instance of library entry N, but N has no `global` scope term. This can happen when:
|
||||
- the entry comes from an import (Divi's D4 fallback covers modules only, not sections or rows);
|
||||
- the scope was changed later;
|
||||
- the markup was written outside the Divi builder.
|
||||
2. U has `edit_post` on N (as its author, or as an editor) and changes N with any Divi tool.
|
||||
3. `GlobalEdits` treats N as a normal entry: no dry run, no `affected` list, no P8a check, no site-wide CSS clear.
|
||||
4. The page U cannot edit now shows the changed content.
|
||||
|
||||
**Evidence (live)**
|
||||
- Temp entry 1910 (`scope = non_global`) and temp published page 1911 with `<!-- wp:divi/global-layout {"globalModule":"1910","blockName":"divi/text"} /-->`.
|
||||
- `isGlobal(1910) = false`, while `GlobalUsages::liveIds(1910) = [1911]`.
|
||||
- The frontend HTTP render of 1911 shows the marker from 1910 (`LITAUDIT_NORMAL_ENTRY`).
|
||||
- Both posts were deleted afterwards.
|
||||
|
||||
**Fix:** in `GlobalEdits::prepare()` and `notice()`, apply the global rules (confirm, `affected`, P8a, `GlobalItem` scope) whenever `isGlobal()` is true **or** `GlobalUsages` finds at least one usage. For the check, one `LIMIT 1` query is enough.
|
||||
|
||||
### L3 – Low (disclosure): the `lit/library-list` cursor is an oracle over the *content* of entries the caller may not read
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/LibraryListAbility.php:176-194`: rows the caller may not read still advance `$position`, and `next_cursor = Cursor::encode($position)` is `base64("o:N")`.
|
||||
- `src/Integrations/Divi/Library/DiviLibraryStore.php:103`: `'s' => search` makes WP search title, excerpt **and content**, although the schema at `LibraryListAbility.php:68` says "Suche im Titel".
|
||||
|
||||
**Scenario**
|
||||
1. An author with the Divi library right creates two entries of their own that contain candidate words.
|
||||
2. They then search with `limit: 1`.
|
||||
3. The decoded cursor tells how many hidden entries match the search and sort before their own. That lets them guess words from other users' library content (codes, internal notes).
|
||||
4. Plugin rule E3 / P12 is that authors may not read those entries: `lit/library-list id=…` returns `lit_forbidden`.
|
||||
5. Mitigation: Divi's own Visual Builder library lists all entries to every user with the library right. Relative to Divi this is parity; relative to the plugin's own stricter rule it is a bypass. It is the same defect class that `PostGuard::ownPostsOnly()` already documents.
|
||||
|
||||
**Evidence (live)**
|
||||
- Temp author and 3 temp entries: an admin entry "aaa" containing `LITAUDITSECRET pin 4711`, and author entries "zzz1" and "zzz2" containing `LITAUDITSECRET`.
|
||||
- As the author, `search=LITAUDITSECRET, limit=1` returns items `["LITAUDIT zzz1"]` with `next_cursor=bzoy` (= `o:2`, which reveals one hidden match).
|
||||
- A direct read of the admin entry returns `lit_forbidden`.
|
||||
- All temp data was deleted and the deletion verified.
|
||||
|
||||
**Fix**
|
||||
- Restrict the query in `find()`:
|
||||
- add `'author' => PostGuard::ownPostsOnly(DiviLibraryStore::POST_TYPE)` when it is non-null, so the cursor counts only rows the caller can see;
|
||||
- set `'search_columns' => ['post_title']` (WP ≥ 6.2) to match the documented title-only search.
|
||||
- Keep the per-row `canRead()` filter as defence in depth.
|
||||
|
||||
### L4 – Low (disclosure): `affected`, `blocked_ids` and the P8a refusal expose IDs, status and type of posts the caller may not read
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/GlobalUsages.php:48-56`: title masked, but `post_id`, `status` and `type` are always returned.
|
||||
- `src/Integrations/Divi/Library/GlobalEdits.php:184-203`: IDs in message and details.
|
||||
- Also `lit/library-list id=…` (`affected`).
|
||||
|
||||
**Scenario**
|
||||
1. An author owns a global entry. An administrator used it on private pages and on other users' drafts.
|
||||
2. The author's dry run lists those posts' IDs, their status (`private`, `draft`, `future`) and their post type.
|
||||
3. Only the titles are hidden.
|
||||
|
||||
**Evidence:** by reading. The masking applies to `post_title` only.
|
||||
|
||||
**Fix**
|
||||
- For rows without `read_post`, return only an aggregate: `hidden_count`, plus the count of blocking hidden posts.
|
||||
- Name blocked IDs only where `read_post` holds; otherwise say "N Einträge, die dieses Konto nicht lesen darf".
|
||||
|
||||
### L5 – Low (authorization): `lit/divi-node-copy` from a library entry into a page skips Divi's role option `add_library`
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Pages/DiviNodeCopyAbility.php:78-96`: the source goes through `DiviPageLoader::load(…, false)`, i.e. `LibraryAccess::item()` / `canRead()` (`edit_posts`, `divi_library`, `edit_post`).
|
||||
- Compare `LibraryAccess::canInsert()` (`add_library`), which `lit/library-insert` requires.
|
||||
|
||||
**Scenario**
|
||||
1. The site owner turns off "Aus der Bibliothek hinzufügen" (add from library) for a role in Divi → Rollen-Editor.
|
||||
2. A user of that role can still copy any node from an entry they can edit into a page, via `lit/divi-node-copy` with `source_post_id = <library entry>`.
|
||||
3. Security impact is small, because content still passes StoredNodeGuard. But it breaks E3 ("nie laxer als Divi", never laxer than Divi).
|
||||
|
||||
**Evidence:** by reading.
|
||||
|
||||
**Fix:** in `precheck()`, when the loaded source `isLibraryItem()`, additionally return `$access->canInsert()`. The target side is already covered by `canEdit()` / `save_library`.
|
||||
|
||||
### L6 – Low (global edits): P8a accepts Theme Builder layouts on WordPress `edit_post` alone
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Library/GlobalEdits.php:158-171`.
|
||||
- On DDEV, `et_header_layout`, `et_template`, `et_theme_builder` and `et_pb_layout` all use capability type `post` with `map_meta_cap`. So an editor has `edit_post` on the site header and footer layouts.
|
||||
- Divi gates the Theme Builder with the role option `theme_builder`, which is off by default for every role but administrator.
|
||||
|
||||
**Scenario**
|
||||
1. A global module sits in the Theme Builder header, footer or body.
|
||||
2. An editor with `edit_global_library` passes P8a, because they have `edit_post` on the TB layout.
|
||||
3. The confirmed edit changes the site-wide header, although Divi does not let this editor use the Theme Builder.
|
||||
4. Divi's own Visual Builder allows the same global edit from any page (parity with Divi). But P8a's intent, "changes only on posts this account may edit", is not met.
|
||||
|
||||
**Evidence:** live, post type objects and caps (probe p1). The rest is by reading.
|
||||
|
||||
**Fix:** in `blocked()`, also treat IDs whose post type is one of `et_header_layout`, `et_body_layout`, `et_footer_layout` or `et_template` as blocked unless `DiviRoles::allows('theme_builder')`.
|
||||
|
||||
---
|
||||
|
||||
## Info / hardening
|
||||
|
||||
### I1 – StoredNodeGuard trusts the instance's own `blockName` and `globalModule`; Divi renders the real entry
|
||||
|
||||
**Where**
|
||||
- `src/Integrations/Divi/Compose/StoredNodeGuard.php:90-91` and `:114-138`. Routing and the code-module check use `GlobalInstance::effectiveName()`, i.e. the stored `blockName`.
|
||||
- `Json::decodeObject()`: the last duplicate key wins.
|
||||
|
||||
**What Divi does**
|
||||
- `BlockParser.php:838` takes the *first textual* `"globalModule":"(\d+)"`. That can be a nested one inside `localAttrs`, or the first of two duplicate keys.
|
||||
- `BlockParserStore.php:1113ff` merges `localAttrs` (filtered by the entry's `localAttrsMap`) onto the entry's *actual* root block.
|
||||
|
||||
**Consequence**
|
||||
- A forged instance can be routed as `divi/text` (kses-checked richtext) while it is rendered onto a `divi/code` entry, or onto another entry than the guard saw.
|
||||
- Exploiting this needs a forged instance in a source the user can edit. The user can already write that into their own page through core REST (parity, cf. 2b audit M1). The new part is that node-copy can carry it into a library entry (`et_pb_layout` has `show_in_rest = false`).
|
||||
|
||||
**Hardening**
|
||||
- Refuse instances whose JSON contains more than one `"globalModule"` occurrence.
|
||||
- Resolve `globalModule` to a published `et_pb_layout` the user may read.
|
||||
- Require its root block name to equal `blockName`, and route `localAttrs` by that real name.
|
||||
|
||||
### I2 – Confirm is not bound to the dry run
|
||||
|
||||
- `GlobalEdits.php:95-117`: between the dry run and `confirm: true`, the `affected` set can grow; the user agreed to N pages, and M are changed.
|
||||
- Rights are safe, because P8a is recomputed at confirm (verified by reading; a live check exists from T9).
|
||||
- Optional: accept `expected_affected_count` (or a hash of the affected IDs) and refuse on mismatch.
|
||||
|
||||
### I3 – GlobalUsages misses non-canonical references Divi still renders
|
||||
|
||||
- `"globalModule":"0123"` renders entry 123: Divi's `([0-9]+)` capture is cast to int by `get_post()`.
|
||||
- The LIKE on `"globalModule":"123"` does not match it.
|
||||
- Only someone who can edit the containing post can write it, so there is no attacker gain.
|
||||
- Optional: LIKE on `'"globalModule":"%'` plus a PHP regex with `ltrim($id, '0')`.
|
||||
|
||||
**Not a gap (checked live):** Divi 4 shortcode pages with `global_module="<id>"`. GlobalUsages does not find them (probe p2/p3: `liveIds = []`), but Divi 5 does not render the D5 entry content there either. The frontend showed the page's own local D4 content, not the entry's. The plugin refuses to edit D4-format entries, so no unlisted page changes.
|
||||
|
||||
---
|
||||
|
||||
## Areas checked and found sound
|
||||
|
||||
- **IDOR via `library_id`:** `LibraryInsertAbility::item()` goes through `LibraryAccess::item(false)` (P5/P12): same `not_found` for non-library IDs and revisions, `forbidden` only for users with the library right.
|
||||
- **IDOR via `source_post_id`:** PostGuard (`edit_post`, managed types only; `revision`, TB and library types excluded) or LibraryAccess, re-checked in `execute()`, with its own `source_tree_rev`.
|
||||
- **IDOR via `post_id` in `$variable` settings / `{{link:ID}}`:** `select_post` → `read_post` (`DynamicContentRules.php:246-259`), re-checked by StoredNodeGuard through `canonical()` against the target post. Only `post_link_url_*` uses `select_post` in Divi 5.2.1. `{{link:}}` to `et_pb_layout` is refused by the registry, with a uniform message.
|
||||
- **Divi role options:**
|
||||
- `divi_library`, `save_library`, `add_library` and `edit_global_library` are combined correctly with `edit_posts`, `edit_post` and `publish_posts` in `LibraryAccess` (except L5).
|
||||
- The P9 group gates (writable/visible) are applied in `item()`, `canEdit()`, `canSave()` and `canInsert()`.
|
||||
- **Global edits through replace-text, node-copy into, insert into, and delete in a global entry:**
|
||||
- All go through `DiviWriter::commit()` → `GlobalEdits` (shape, dry run, P8a, backup, `GlobalItem` scope).
|
||||
- replace-text never sends `confirm`.
|
||||
- An instance inside a global entry is refused (P11/`shapeViolation`).
|
||||
- Generic WP tools (`content-update`, `post-trash`, `revision-restore`, `duplicate`) cannot reach `et_pb_layout` (not managed).
|
||||
- **Dry run/confirm race for rights:** P8a is recomputed at confirm from a fresh query. The `tree_rev` lock covers entry content changes.
|
||||
- **Content injection:**
|
||||
- `localAttrs` and the snapshot children of instances are walked, and so are local children, because the parser keeps inner blocks of opaque blocks.
|
||||
- `deglobalize` runs before the guard.
|
||||
- CRLF unification touches whitespace-only text between blocks only.
|
||||
- Invalid or non-`divi/` `blockName` instances are refused as opaque.
|
||||
- Library titles go through `sanitize_text_field` (plus core title kses).
|
||||
- Terms are limited to existing names.
|
||||
- **Divi frontend sinks beyond interactions and Lottie** (scan of all `script-library-*.js`):
|
||||
- module link → `diviElementLinkData`, where `LinkUtils::generate_data` applies `esc_url` before `window.location`/`window.open`;
|
||||
- multi-view `innerHTML`/`setAttribute`: data via `wp_localize_script` (nested values are not entity-decoded), with fixed attribute names and per-attribute sanitizers;
|
||||
- `new Function` hits are bundler global shims;
|
||||
- magnific-popup, slider and menu read `src`/`href` from escaped markup.
|
||||
- Together with the T11 live URL matrix, no new sink was found.
|
||||
- **Disclosure:**
|
||||
- `dynamic-sources` samples only an editable post, uses the `edit` context (custom meta hidden without `read_dynamic_content_custom_fields`) and returns no option defaults.
|
||||
- Render-summary notes list only IDs already present in content the caller may read.
|
||||
- site-status `problems` are labels of missing Divi internals only.
|
||||
- Error texts are uniform for missing and unreadable IDs (P12; `{{link:}}` from T2). The known P13 leftovers are left to the final wave.
|
||||
- **Multisite:** everything is per-blog (`$wpdb->posts`, Divi role options, `current_user_can`). No network-level path exists. Without `unfiltered_html` (site admins), the guard and the P9 backup refusal only get stricter.
|
||||
- **DoS (other):**
|
||||
- `lit/library-list` scan is capped (`MAX_SCAN` 500, `limit` ≤ `Cursor::MAX_LIMIT`).
|
||||
- Guide `topic=all` is capped at 100 KB of static files.
|
||||
- `affected` is capped at 50 rows (the cost is in L1, not the output).
|
||||
Reference in New Issue
Block a user