docs: Etappe 2b rulings, security audit and trial feedback

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

docs/superpowers/notes/2026-09-26-etappe-2b-security-audit.md

@@ -0,0 +1,336 @@
+# WP-LIT-MCP: whole-plugin security audit (S42 step 2)
+
+- **Scope:** branch etappe-2, HEAD 314b3d0.
+- **Environment:** WordPress 7.1.2, Divi 5.2.1 on DDEV.
+- **Method:**
+  - Read all of `src/`, plus the relevant parts of `vendor/wp-media/mcp-oauth` and `vendor/wordpress/mcp-adapter`.
+  - Ran read-only DDEV `wp eval-file` probes.
+  - Ran one live, self-cleaning run of the real `lit/content-create` ability as a temporary Author:
+    - user 67 and post 861 were created, then deleted;
+    - the deletion was verified afterwards: no `lit_audit_*` user and no probe post remain.
+  - No code was changed. The option `lit_mcp_allowed_roles` was widened in-process only, through the filter, never in the database.
+
+No Critical or High findings. The Divi value pipeline itself held.
+The two most relevant problems are ways *around* it:
+- the raw `content` field;
+- the tool-group switch.
+
+---
+
+## Findings (most severe first)
+
+### M1 – Medium: raw `content` lets users without unfiltered_html write the "builder-only" Divi values (the S7/S26 gates don't apply)
+
+**Where**
+- `src/Integrations/WordPress/Content/PostWriter.php:28` (content → post_content) and `:58`.
+- `ContentCreateAbility.php:127-133`.
+- `ContentUpdateAbility.php:124` refuses `content` only when the post *already* uses Divi. Nothing refuses content that *introduces* Divi markup.
+- Compare `src/Integrations/Divi/Compose/ValueCleaner.php:57` (`BUILDER_ONLY`).
+
+**Scenario**
+1. An Author, or an Editor on multisite or with `DISALLOW_UNFILTERED_HTML`, calls `lit/content-create` (or `lit/content-update` on a non-Divi post).
+2. The `content` is Divi 5 block markup that sets:
+   - a module's `css.desktop.value.freeForm` (e.g. `body{…}`);
+   - `module.decoration.attributes` (custom HTML attributes).
+3. kses (`filter_block_content`) and Divi's own `wp_insert_post_data` security filters keep both. Only `on*` attributes are stripped, by Divi.
+4. The page is now a Divi layout. From then on every `lit-divi-*` tool works on it inside the kses window.
+
+The same values are refused with "nur im Divi-Builder" (only in the Divi builder) when sent through `lit-divi-insert`/`-update` (rulings S7/S26).
+
+A prompt-injected Claude session can therefore do CSS injection or attribute injection on the live site. This is exactly what S26 was meant to prevent.
+
+**Evidence**
+- Live run of `wp_get_ability('lit/content-create')->execute()` as a temporary Author (unfiltered_html=false):
+  - `freeForm CSS stored: true`;
+  - `data-probe stored: true`;
+  - rendered `<div class="et_pb_text_0 … et_pb_module et_flex_module" data-probe="1">`.
+- Read-only kses probe (`kses6.php`): `onmouseover` is removed by Divi and never rendered. No script execution found. Divi renders a `javascript:` `linkUrl` as `href=""`.
+
+**Parity note**
+The same user can store the same markup through core REST (`/wp/v2/posts`) or the code editor. So this is no privilege escalation beyond WordPress core. It is a bypass of the plugin's own, explicitly decided gate. The pipeline guarantee "every path from ability input to post_content goes through ValueCleaner/Composer/TreeEditor" does not hold for `content`.
+
+**Fix proposal**
+- In `PostWriter::prepare()` (create and update), refuse a `content` that contains `<!-- wp:divi/` (and `[et_pb_`), with a hint to use `divi` / the Divi tools.
+- Alternative: parse it with `Parser` and run every Divi block through `Composer`/`ValueCleaner` (heavier).
+- At minimum, do this for users without `unfiltered_html`.
+
+### M2 – Medium: "nur lesen" / disabled Divi group does not stop Divi writes (content-create `divi`)
+
+**Where**
+- `src/Integrations/WordPress/WordPressIntegration.php:83` passes `$this->divi` (the ContentComposer) unconditionally.
+- `ContentCreateAbility.php:113-119, 145` write the Divi layout whenever Divi 5 is loaded.
+- `ExposurePolicy::disabledByReadonly()` and `enabled()` only look at the ability's own group ("wordpress").
+
+**Scenario**
+1. The admin sets group Divi to "nur lesen", or disables `lit-divi-insert`, or disables the whole group.
+2. The tools tab, readme ("schaltet alle schreibenden Tools einer Gruppe … ab", switches off all writing tools of a group) and acceptance note §7 all promise that no Divi writes happen.
+3. Claude can still create complete Divi layouts via `lit-content-create` with `divi: [...]`. That goes through the same kses window.
+
+Related cross-group writes that still work with Divi "nur lesen":
+- `lit/post-duplicate`, `lit/post-replace-with-copy`, `lit/revision-restore` on Divi pages. These use stored content, so the impact is lower.
+
+**Evidence:** by reading. The wiring is unconditional; ExposurePolicy has no cross-group notion.
+
+**Fix proposal**
+- Give `ContentCreateAbility` a callable `diviAllowed()` that is true only when `ExposurePolicy::mode()` of `lit/divi-insert` is not `disabled`. That mode already includes the Divi group's read-only switch.
+- If Divi writes are not allowed, answer `divi` with `unsupported`, and drop the `divi` property from the input schema.
+- Document in the Tools tab that duplicate/replace/restore copy stored Divi content regardless of the Divi group.
+
+### M3 – Medium: URL import SSRF – DNS rebinding TOCTOU, core-version-dependent blocklist, error oracle
+
+**Where**
+- `src/Integrations/Media/MediaImporter.php:55-62` (`wp_http_validate_url` pre-check).
+- `:119-139` (`wp_safe_remote_get` + `CURLOPT_IPRESOLVE_V4`).
+- `:142` / `:339-346` (the raw `WP_Error` message is returned).
+
+**Scenario 1: DNS rebinding**
+- A prompt-injected Claude (e.g. a web page Claude read says "import this image") calls `lit-media-upload-from-url` with a hostname.
+- `wp_http_validate_url()` resolves the host with `gethostbyname()` and sees a public IP. cURL then resolves again; with TTL 0 it gets 127.0.0.1 or another internal host.
+- Redirects are re-validated by core (`WP_Http::validate_redirects`, verified in 7.1.2), but each redirect has the same TOCTOU gap.
+- Impact:
+  - blind GET requests to internal services on ports 80/443/8080;
+  - internal *images/PDFs* (e.g. internal dashboards rendering PNG) get copied into the public media library.
+
+**Scenario 2: version-dependent blocklist**
+- On DDEV (core 7.1.2), `wp_http_validate_url()` blocks 169.254/16, 100.64/10, TEST-NETs, multicast etc. Verified: `ssrf1.php`, all 17 internal probes return false.
+- The plugin declares "Requires at least: 6.9". Older cores only block 0/8, 10/8, 127/8, 172.16/12 and 192.168/16.
+- If 6.9 lacks the newer ranges, `http://169.254.169.254/` (cloud metadata) is reachable. Not verified on 6.9.
+
+**Scenario 3: error oracle**
+- `Download fehlgeschlagen: <cURL/WP message>` and `HTTP-Status %d` let the caller tell apart:
+  - refused;
+  - timeout;
+  - 404;
+  - "wrong file type".
+
+**Evidence:** by reading, plus the DDEV validation probe.
+
+**Fix proposal**
+- In the existing `http_api_curl` hook:
+  - resolve the host once;
+  - check the IP with the plugin's own full private/reserved list (`filter_var(..., FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)`, plus 100.64/10);
+  - pin it with `CURLOPT_RESOLVE` (`host:port:ip`).
+- Set `'redirection' => 0` and follow at most 3 redirects manually, applying the same resolve, check and pin to each.
+- Return a generic "Download fehlgeschlagen" message and log the detail.
+
+### M4 – Medium (only when Pods field restrictions are used): Pods field access restrictions are ignored
+
+**Where**
+- `src/Integrations/Pods/PodsFieldSource.php:73-114` (read/write through `pods()->field()` and `pods()->save()`).
+- `PostWriter.php:226` (`META_SOURCES` excludes pods, so there is no per-field capability check).
+- `PostPresenter::isHiddenField()` only hides the `password` type.
+
+**Scenario**
+- Pods fields can be restricted per field:
+  - `admin_only`;
+  - `restrict_role`;
+  - `restrict_capability`;
+  - `read_only`;
+  - `hidden`.
+- Pods enforces these only in its forms (`PodsForm::permission` → `pods_permission`).
+- Its PHP API does not: `PodsAPI::save_pod_item` defaults `from = 'save'`, and `'save' === $params->from` skips `pods_permission` (pods/classes/PodsAPI.php:4868, 4994).
+- An Editor or Author with `edit_post` on the entry can therefore read (`lit-content-get`, `lit-content-query` field filter) and write (`lit-content-update/create`) fields the site owner restricted to admins or made read-only.
+
+**Evidence:** by reading the plugin and the Pods source in `wp-content/plugins/pods`. Pods is not active on DDEV, so there was no live test.
+
+**Fix proposal**
+- In `PodsFieldSource::definition()`:
+  - mark fields `READONLY` when `pods_has_permissions($field) && ! pods_permission($field)` or when `read_only` is set;
+  - hide them (as `isHiddenField` does) when `hidden` is set or permission fails without `read_only_restricted`.
+- Re-check in `write()` before `save()`.
+
+### L1 – Low: any logged-in user can complete OAuth and get a connection (application password + tokens)
+
+**Where**
+- The vendor flow (`mcp-oauth` `AuthorizeEndpoint`/`ConsentEndpoint`: only `is_user_logged_in()`).
+- The plugin adds no gate: `Plugin.php:110` boots it as-is.
+- The `lit_mcp_use` gate only applies at tool execution (`AbilityRegistrar.php:56-63`) and tool listing (`VisibilityFilter`).
+
+**Scenario**
+- On sites with customer or subscriber accounts (WooCommerce, memberships), any customer can connect Claude.
+- mcp-oauth then creates an application password for them, even where core would not offer app passwords to that user.
+- It also issues access and refresh tokens, and mcp-adapter sessions in user meta.
+- The customers can't run tools. But the connections appear in "Zugriff", and the site accumulates credentials and sessions for users who were never meant to have them.
+
+**Evidence:** by reading.
+
+**Fix proposal:** before mcp-oauth's authorize/consent handler, refuse with a clear message when `! current_user_can('lit_mcp_use')`. A hook such as `template_redirect` on its rewrite query var would work, and so would a filter if mcp-oauth offers one.
+
+### L2 – Low: release/build hygiene – dev vendor, tests namespace, direct file access
+
+**Where**
+- `vendor/` (dev install).
+- `composer.json` `autoload-dev`.
+- All `src/**/*.php` (no `ABSPATH` guard).
+- `tests/Integration/divi-convert-fixture.php` (the only integration script without a `WP_CLI` guard).
+- `.gitattributes` (only relevant if the zip is made with `git archive`, and `vendor/` is git-ignored).
+
+**Scenario and evidence**
+
+1. **Dev vendor loaded at runtime.** The current tree's Jetpack autoloader loads these files on every WP request:
+   - `mockery/library/helpers.php`;
+   - `Mockery.php`;
+   - `brain/monkey/inc/api.php`;
+   - `phpunit/.../Assert/Functions.php`.
+
+   Source: `vendor/composer/jetpack_autoload_filemap.php`. No release script exists (spec §U is still open).
+
+2. **Tests namespace survives `--no-dev`.** Reproduced: `composer install --no-dev` in a temp dir still gives `LIT\MCP\Tests\ → tests/` in `vendor/composer/jetpack_autoload_psr4.php:21`. The Jetpack autoloader keeps root `autoload-dev`. If `tests/` ships, test classes are autoloadable in production.
+
+3. **Direct HTTP access leaks paths and versions.**
+   - `curl https://…/wp-content/plugins/WP-LIT-MCP/src/Admin/AccessTab.php` returns a PHP fatal error with the full server path (display_errors on in DDEV).
+   - `…/tests/Integration/divi-convert-fixture.php` → HTTP 200.
+   - `…/vendor/composer/installed.json` → HTTP 200 (full dependency and version list).
+
+**Fix proposal**
+- Add a build script:
+  - `composer install --no-dev -o`;
+  - copy only `wp-lit-mcp.php`, `uninstall.php`, `readme.txt`, `src/`, `assets/`, `docs/guides/`, `vendor/`;
+  - no `tests/`, `docs/superpowers`, `.superpowers`, `composer.*`, `phpunit/phpcs` configs.
+- Add a CI check that the zip contains no `vendor/phpunit`, `vendor/mockery` or `tests/`.
+- Add `defined('ABSPATH') || exit;` to the src files (Plugin Check flags its absence), and the `WP_CLI` guard to `divi-convert-fixture.php`.
+
+### L3 – Low: `tree_rev` / `rev` lock is check-then-write (lost update)
+
+**Where**
+- `DiviWriter.php:116` (check) → `:161` (save).
+- `ContentUpdateAbility.php:115` → `:156`.
+- `MediaUpdateAbility` `modified`.
+
+**Scenario:** two parallel writes on the same page (two Claude sessions, or Claude plus the Visual Builder) both pass the rev check. The later `wp_update_post()` silently overwrites the earlier change. The restore-point revision limits the damage.
+
+**Evidence:** by reading. There is no lock or conditional update.
+
+**Fix proposal:**
+- Take a short per-post lock around check and save (`wp_cache_add`/`add_option('lit_mcp_lock_<id>')` with expiry, or MySQL `GET_LOCK`).
+- Re-read `post_content` inside the lock and compare `Rev` again right before `wp_update_post()`.
+
+### L4 – Low: deactivation and uninstall leave OAuth sessions valid
+
+**Where**
+- `Lifecycle.php:93-101`.
+- `Uninstaller.php:28-61`. This is documented as intentional.
+
+**Scenario**
+- Deactivating or deleting the plugin keeps these:
+  - the mcp-oauth application passwords;
+  - `mcp_refresh_jti_*` meta;
+  - `mcp_jwt_secret`.
+- Reactivating it within 30 days revives every refresh token. The admin may believe that removing the plugin cut Claude off.
+- Plugin-owned post meta `_lit_mcp_copy_of` / `_lit_mcp_backup_of` also remains.
+
+**Evidence:** by reading.
+
+**Fix proposal:**
+- On uninstall, delete the connections `ConnectionRepository` identifies (marker meta plus https `app_id`).
+- Either delete `_lit_mcp_*` meta, or mention in readme and deactivation notice that connections survive.
+- Offer "alle Verbindungen widerrufen" (revoke all connections) in the Access tab.
+
+### I1 – Info: mcp-oauth logs MCP request bodies when `WP_DEBUG` and `WP_DEBUG_LOG` are on
+
+**Where:** `vendor/wp-media/mcp-oauth/inc/Transport/OAuthHttpTransport.php:128-137` (`McpLogger::safe_request_body()`, 8 KB).
+
+Tool arguments (page content, private drafts, Base64 uploads) go to `wp-content/debug.log`, which is often web-reachable on customer staging sites.
+
+**Fix proposal:** mention this in the readme, or turn it off (e.g. filter or constant, if mcp-oauth offers one; otherwise ask upstream).
+
+### I2 – Info: non-Divi writes re-kses Divi pages for users without unfiltered_html
+
+These tools save through plain `wp_update_post`/`wp_insert_post`, so `content_save_pre` kses re-serializes the whole Divi layout:
+- `lit/content-update` (title/excerpt/fields);
+- publish/unpublish/schedule;
+- `revision-restore`;
+- `post-duplicate`;
+- `post-replace-with-copy`.
+
+The byte-identity promise of spec §1.3/§5.2 does not hold for these tools. There is no security impact: it only ever sanitizes more.
+
+**Evidence:** read-only probe `kses2.php`. All 7 stored Divi pages changed under editor-kses (e.g. an empty block becomes `/-->`).
+
+### I3 – Info: smaller observations
+
+1. **Multisite super admin bypasses the role gate.** A super admin always has `lit_mcp_use` (`WP_User::has_cap`), even if "Zugriff" allows no role. `AccessTab::currentUserLosesAccess` knows this; the UI text doesn't say it.
+2. **Media usage lists unreadable posts.** `MediaUsageFinder.php:455-461` returns post ID and post type (not the title) for posts the user can't read. This lets a user learn that private posts exist. It is intentional, to block deletes.
+3. **All post types with a UI are exposed.** Every `show_ui` post type, including non-public ones (on DDEV: `path`), is exposed; only core, Divi internals and `lit_mcp_excluded_post_types` are excluded. Consider documenting the filter for data-protection purposes (form submissions, personal data as CPTs).
+4. **Nested REST requests can block later calls.** A `rest_do_request()` to another route during a tool call makes `RequestContext` false for the rest of that HTTP request (`RequestContext.php:281`). This fails closed, so later JSON-RPC calls in the same batch get `forbidden`. Functional only.
+5. **`confirm: true` is not a human confirmation.** Claude sets it itself; under prompt injection it is no safeguard. Keep the destructive tools indirect by default.
+
+---
+
+## Checked and found sound
+
+- **RequestContext:** recomputed on every matched REST request, cleared after our route, and required by every ability. WP-CLI/STDIO and other routes fail closed.
+- **Default mcp-adapter server:** disabled; the only MCP route is `/mcp/mcp-oauth-server` (verified via `rest_get_server()->get_routes()`).
+- **Core Abilities REST:** our abilities have `show_in_rest=false` (only the 3 `core/*` abilities are in REST).
+- **Transport and JWT:** `OAuthHttpTransport` requires a Bearer JWT (HS256 pinned, exp, aud = our route, iss = `home_url()`, app password must still exist). The current user is set only from the JWT; cookie or Basic auth can't reach the tools.
+- **Revocation:**
+  - AccessTab revoke deletes the app password, which the next request's check honours;
+  - refresh-token rotation and reuse detection live in mcp-oauth;
+  - revoke is limited to `edit_user`-able users and, on multisite, to site members.
+- **ConnectionUsage:** records only after a <400 response on our route, and only for `sub === current user`.
+- **`lit_mcp_use` gate:** dynamic cap via `user_has_cap` (no role DB changes); the option is sanitized to existing roles; the filter output is type-checked; the meta-tools' capability filters are set to `lit_mcp_use`.
+- **Exposure:**
+  - disabled and "nur lesen" abilities are never registered, so `execute-ability` answers "not found";
+  - DIRECT_ONLY abilities are `mcp.public=false`, so `execute-ability` refuses them;
+  - `lit/guide` is always direct.
+- **Annotations:** every tool annotated `readonly` really doesn't write (render-summary runs `the_content` side effects only).
+- **Per-object checks:**
+  - `PostGuard::editable` (`edit_post`, managed type, trash rules) on every content, status, revision, render and Divi ability, in both permission and execute;
+  - revisions are checked via the parent and `post_type === 'revision'`;
+  - attachments via `MediaGuard` (`edit_post`/`delete_post`, type check);
+  - menu items via item ID membership in the menu.
+- **Nested IDs:**
+  - featured image, parent and relation fields: `read_post` plus type checks;
+  - menu `object_id`: `read_post`, no private, unpublished only with a flag, taxonomy must be viewable;
+  - Divi media/gallery and `select_post`: `read_post`;
+  - terms: `assign_terms`/`edit_terms`;
+  - meta: `edit_post_meta`/`add_post_meta` per key.
+- **Query and search:**
+  - `content-query`/`media-search`/`replace-text` limit the query to the user's own posts without `edit_others_posts` and gate statuses;
+  - hidden fields can't be used as a query filter.
+- **Settings:**
+  - `manage_options`;
+  - strict allow-list;
+  - `admin_email`/`users_can_register`/`default_role`/`siteurl`/`home` explicitly blocked;
+  - per-key validation (installed languages, valid timezones, published pages, image attachment).
+- **Meta writes:**
+  - `update_post_meta` double-serializes a serialized string, so there is no object injection;
+  - protected keys are refused through the meta caps;
+  - clone and replace copy raw stored meta via `$wpdb` (no unserialize).
+- **Menu URLs:** only http/https/mailto/tel-style schemes, `/path` (not `//` or `/\`) or `#`; control characters and spaces are refused; `esc_url_raw` is applied.
+- **Media files:**
+  - MIME allow-list intersected with the core-allowed types for the user, checked against the real content (`wp_check_filetype_and_ext`);
+  - no SVG by default;
+  - `sanitize_file_name` (no traversal, double extensions defused);
+  - streamed size cap;
+  - Base64 size checked before decoding;
+  - temp files removed on every path;
+  - download and preview only read files `realpath`-inside uploads;
+  - caption and description go through `wp_kses_post`.
+- **Error handling:**
+  - Throwables are mapped to `internal_error`, details only go to the PHP log;
+  - the sideload error is hidden;
+  - no SQL or path text reaches the client (apart from the M3 download messages).
+- **SQL:** every query is `$wpdb->prepare()`d. The only interpolations are table names and the constant status list; the log filters are whitelisted.
+- **Change log:** holds titles and references only, no content. Admin list output is escaped; revision links are gated by `edit_post` on the parent.
+- **Admin UI:**
+  - every admin-post action has `manage_options` plus `check_admin_referer` (revoke nonce per UUID, clear-log);
+  - settings go through `options.php` with `settings_fields` nonces and sanitize callbacks;
+  - `closedpostboxes` is core AJAX with its nonce;
+  - notices and tabs come from fixed maps;
+  - everything is escaped (`esc_js`+`esc_attr` for confirm handlers).
+- **Site Health and admin notices:** static text only, escaped; the loopback check is cached and only runs on demand.
+- **Guides:** topic slug regex plus the glob'd file list (no traversal); filtered guides are site-owned.
+- **Divi pipeline integration:**
+  - the kses window covers only `RevisionPoint::capture` plus the one `wp_update_post` and is restored in `finally`, only when kses was active;
+  - Divi's own `wp_insert_post_data` security filters stay (verified: they strip `on*` attributes);
+  - the insert, update, move, delete and duplicate-node tools only write through Composer/TreeEditor;
+  - duplicate-node adds only the stored label plus a constant;
+  - replace-text re-checks targets per post;
+  - DynamicContentMap drops `loop_*` options and types come from the registry; the fixed rules (enable_html, meta keys, markers) remain.
+- **Uninstall and deactivation:** both remove their own options, transient, cron and table on every site of a network, never posts.
+- **Limits and DoS:**
+  - cursor offset capped at 9 digits, page size ≤100;
+  - batch caps (media-update 50, divi-update 50, insert/delete 20, replace-text 20 targets / dry run ≤100 pages, content-create `divi` 30 sections);
+  - render summary strings bounded;
+  - text search limited to 200 characters.