fix: authorize every read against its target object (#148) (#149)

* test(security): pin read-tier disclosure of unreadable content

Red on purpose. Every read-tier ability passes `read` to
Saddle_Capabilities::permission(), a capability every logged-in
Subscriber holds, and half the read surface never re-checks the target.
These 15 failures are the disclosure itself:

- get-media returns attachments on draft and private parents
- get-post/get-page return raw content for drafts, private posts and
  password-protected posts
- list-post-revisions returns the edit history of any post
- list-posts/list-pages/search-content enumerate every author's drafts
- list-media enumerates attachments of private posts

The 16 passing cases are the other half of the contract: an
administrator credential — the normal Saddle setup — must be unaffected,
and read_post on an attachment must keep following post_parent.

Refs #148

* fix(abilities): authorize the object on every single-item read

The read tier's capability is `read`, which every logged-in Subscriber
holds, so the permission callback proves the caller may read something
and never that they may read this. get-post, get-page, get-media and
list-post-revisions resolved their target and returned it unchecked.

require_readable_post() already did this job for lint-page, render-node
and verify-page. It now takes the input key and the accepted post types
as arguments, so those four route through the same funnel instead of
gaining a fifth copy of the check, and the byte-identical inline clone in
get_blocks() collapses into it too. Defaults match the old hardcoded
values, so the three original callers are unchanged.

Two things the shared check gains:

- A password clause. map_meta_cap never consults post_password, so
  read_post alone waves a protected post through. Saddle returns raw
  post_content, which core only hands out in the edit context, so the
  threshold is core's own edit_post. It refuses rather than blanking:
  this reader feeds a writer on the same id, and an agent handed an
  empty body concludes the page needs rebuilding.
- require_id(), so a malformed post_id is a 400 naming the field rather
  than a 404 claiming the post does not exist.

list-post-revisions additionally requires edit_post on the parent, which
is what WP_REST_Revisions_Controller requires — being able to read a post
is not being able to read the drafts it went through.

get-preview-url stays deliberately outside the helper: it mints an
unauthenticated URL, so unpublished content needs a higher bar than "may
read". Its docblock now says so, so nobody collapses it later.

Refs #148

* fix(abilities): stop the list paths enumerating unreadable content

list-posts, list-pages, search-content and list-media returned every
author's drafts and private posts, and every attachment hanging off one,
to any read-tier connection.

WP_Query does not gate this by itself. `post_status => 'any'` excludes
only the two internal statuses, because register_post_status() derives
exclude_from_search from `internal` and not from `protected` — so draft,
pending, future and private all come back. And `'perm' => 'readable'` is
not the fix people assume: it is consulted in one branch, applies only to
an explicitly requested `private`, and is a complete no-op against `any`,
where the arrays it filters are both empty. It is deliberately not used
here; a query var that looks like the control but isn't is worse than
none, and there is a comment saying so.

So the two controls core's own REST layer uses, both of them:

- status_filter() gates the requested status on the post type's
  edit_posts, as sanitize_post_statuses() does. An explicit forbidden
  status is refused by name rather than silently emptied; the default
  `any` narrows to `publish` instead, because refusing the default would
  break read-only listing entirely for a legitimate connection.
- collection() — already the single chokepoint for all three listings —
  drops rows failing read_post, as get_items() does. This is the only
  control that reaches list-media at all, since attachments carry
  `inherit` and the status gate cannot see through it.

Both are needed: an author holds edit_posts and may legitimately ask for
drafts, and must still not receive another author's. There is a test for
exactly that case.

Dropped rows leave `total` counting items that were not returned — the
same inconsistency core accepts, because recounting means a second
unbounded query. What Saddle adds is a note on the response, because an
agent handed a short page with no explanation retries it. It never fires
for an administrator, so that response shape is unchanged.

Refs #148

* docs(oauth): say why each public route is public

Four routes carry permission_callback => '__return_true' with no comment
saying why, and they are exactly what a reviewer grepping for that string
lands on. The reasons were already written down in the wp-security-rules
skill; this moves them into the code, in the style /auth-probe already
uses. No behaviour change.

Refs #148

* docs: unsplash links that resolve, changelog, security rule, CI note

readme: unsplash.com sits behind bot protection that answers 401 to any
user agent containing "Mozilla" and 200 to anything else, which is why
the review flagged the API Terms URL as dead. It is not — but their
checker cannot see it, and https://unsplash.com/privacy behaves the same
way and was not flagged, so swapping one URL would leave a second
landmine. Both canonical links stay, because the external-services
disclosure needs terms and privacy and no mirror of the privacy policy
exists. Unsplash's own help-centre guidelines URL, which answers 200 to
everything, is added alongside.

Also a changelog entry for the read-authorization fix, in the same
plain-language voice as its neighbours.

wp-security-rules gains rule 12, the read-side mirror of rule 4: read
tier means current_user_can('read'), which a Subscriber holds, so the
permission callback proves the caller is signed in and nothing else. It
names require_readable_post() as the single funnel and records the two
core behaviours this leans on — read_post resolving an attachment's
status through post_parent, and map_meta_cap never consulting
post_password — plus why 'perm' => 'readable' is not a control.

CLAUDE.md said this repo has no CI workflows. It has had
.github/workflows/ci.yml running composer lint and composer test for a
while, so the instruction to never claim CI passed was telling agents to
ignore a real signal. Corrected, and pointed at the harder case: report
a red check even when the failure predates the branch.

.pot regenerated — 6 new msgids, 0 removed, and it was stale again.

Refs #148

* docs: session log for the WordPress.org review round 2

Refs #148

Author: ifahimreza

.claude/skills/wp-security-rules/SKILL.md

@@ -32,7 +32,8 @@ this table and re-deriving every example.
 | Effective tier (decide) | `Saddle_Capabilities::get_tier()` |
 | Configured tier (report) | `Saddle_Capabilities::get_site_tier()` |
 | Destructive gate | `Saddle_Approval::gate()` |
-| Object-level authz | `Saddle_Abilities::authorize_write()` · `Saddle_Pro_Divi::editable_divi5_post()` |
+| Object-level authz (write) | `Saddle_Abilities::authorize_write()` · `Saddle_Pro_Divi::editable_divi5_post()` |
+| Object-level authz (read) | `Saddle_Abilities::require_readable_post()` |
 | SSRF guard | `Saddle_HTTP::url_is_safe()` |
 | Admin REST gate | `Saddle_REST_Admin::can_manage` |
 | Option allowlist | `Saddle_Abilities::guard_option()` |
@@ -141,6 +142,8 @@ route through `Saddle_Pro_Divi::editable_divi5_post()`
 allows building an empty non-Divi post. Any other route to `post_content` is a
 finding.
 
+**The read side has the same rule and a different mechanism — see rule 12.**
+
 ## 5. Agent-facing payloads must NOT be HTML-escaped — HIGH
 
 Ability return values are JSON for an AI agent, not HTML for a browser.
@@ -261,6 +264,68 @@ discloses.
 'permission_callback' => '__return_true',
 ```
 
+## 12. A read ability must authorize the OBJECT, not just the tier — CRITICAL
+
+The mirror of rule 4, and the easier one to miss. Nearly every read-tier ability
+passes `$cap = 'read'` — a capability every logged-in Subscriber holds. Any
+logged-in user can mint a core Application Password, and the MCP route requires
+only `is_user_logged_in()` (`class-saddle-mcp.php:168`). So on the read side the
+permission callback proves the caller is signed in and *nothing else*. Anything
+that resolves a caller-supplied post id, or runs a `WP_Query` whose `post_status`
+is not `publish`, is deciding disclosure entirely by itself.
+
+Two things `current_user_can( 'read' )` does not know, both load-bearing.
+`read_post` is the meta capability that actually consults the object's status,
+and it resolves an attachment's status through `get_post_status()`, so it follows
+`post_parent` on its own — a parentless attachment reads as `publish`, matching
+core's own REST controller. And `map_meta_cap` never looks at `post_password` at
+all; core blanks that content at render time instead
+(`WP_REST_Posts_Controller::can_access_password_content()`).
+
+```php
+// BAD (constructed) — permission_callback passed 'read'; nothing checks THIS post
+public static function get_media( $input ) {
+    $post = get_post( (int) $input['id'] );
+    return array( 'url' => wp_get_attachment_url( $post->ID ), 'alt' => …, 'attached_to' => … );
+}
+// GOOD — includes/abilities/core-content.php:1250
+$post = self::require_readable_post( $input, 'id', array( 'attachment' ) );
+if ( is_wp_error( $post ) ) { return $post; }
+```
+
+`require_readable_post()` (`core-content.php:1785`) is the single funnel, and it
+takes the input key and the accepted post types as arguments so there is never a
+second copy: it resolves the id, restricts the type, checks `read_post` on the
+target, and refuses a password-protected item to a caller without `edit_post` —
+Saddle returns *raw* `post_content`, which core only ever hands out in the edit
+context. Callers: `get-post`, `get-page`, `get-media`, `list-post-revisions`
+(all `core-content.php`), `get-blocks` (`blocks.php:803`), `lint-page`
+(`lint.php:57`), `render-node` (`render.php:92`), `verify-page` (`verify.php:57`).
+`get-preview-url` (`render.php:148`) is deliberately stricter — `edit_post` for
+anything unpublished, because it mints an unauthenticated URL. A new read ability
+that reaches a specific object by any other route is a finding.
+
+Listing paths need the same rule in **two** places, exactly as core does:
+
+```php
+// BAD (constructed) — 'any' is every author's drafts, and perm does not save you
+'post_status' => 'any', 'perm' => 'readable',
+// GOOD — the status the caller may QUERY, then the rows the caller may READ
+$status = self::status_filter( $input, $type );                   // core: sanitize_post_statuses()
+if ( ! current_user_can( 'read_post', $post->ID ) ) { continue; } // core: get_items()
+```
+
+Neither alone is enough: an Author holds `edit_posts` and may legitimately query
+`draft`, but must not receive another author's draft. And **`'perm' => 'readable'`
+is not a substitute for either** — it narrows only an explicitly requested
+`private` status and is a complete no-op against `post_status => 'any'`, which
+excludes only the two `internal` statuses (`register_post_status()` derives
+`exclude_from_search` from `internal`, not from `protected`). Filtering rows after
+the query means `total` counts items that were not returned; core accepts that
+silently, Saddle narrates it in a `note`, because an agent handed a short page
+with no explanation retries it.
+
+
 ---
 
 ## Known-safe patterns — do not flag
@@ -271,7 +336,8 @@ discloses.
 - **`/auth-probe`** — `class-saddle-connection.php:366`. Returns booleans about
   the caller's own headers; reads no credential.
 - **MCP transport gated only on `is_user_logged_in()`** — `class-saddle-mcp.php:168`.
-  Per-tool authorization is each ability's `permission_callback`.
+  Per-tool authorization is each ability's `permission_callback`, and per-*object*
+  authorization is the check inside its execute callback (rules 4 and 12).
 - **Unescaped echo of captured admin notices** — `includes/admin/class-saddle-settings.php:134`,
   standing `phpcs:ignore`. Other plugins' rendered HTML moved to a hidden
   container; escaping breaks their dismiss buttons.

CLAUDE.md

@@ -595,7 +595,7 @@ The problem from the issue.
 `Closes #N` is mandatory — it auto-closes the issue and keeps the board honest. Use
 `Refs #N` only when the PR genuinely doesn't finish the issue.
 
-**6. Verify before merge.** This repo has **no CI workflows**, so `gh pr checks` reports nothing. "Green" means you ran the repo's own build, lint and test commands locally and said so in the PR body. Do not claim CI passed when there is no CI.
+**6. Verify before merge.** CI is `.github/workflows/ci.yml` — `composer lint` and `composer test`, on every PR. Read `gh pr checks` rather than assuming, and run the same commands locally too: the suite is environment-sensitive and has failed in CI while green locally. **Report a red check even when the failure predates your branch** — say which test and which issue tracks it. Never write "green" over an inherited failure.
 
 **7. Merge — solo fast mode.** Once checks are green and the description is
 complete, merge it yourself:

STATUS.md

@@ -1,4 +1,25 @@
 ## Last session
+2026-08-25 (**WordPress.org review round 2 — they were right about `get-media`, and it was five abilities wider; fixed, swept, verified on a live install, `.org` zip rebuilt**) — Issue [#148](https://github.com/plugpressco/saddle/issues/148), PR [#149](https://github.com/plugpressco/saddle/pull/149) **ready, deliberately not merged**. **643 tests** (was 612), 0 lint errors. Plugin Check against the built zip: **0 errors**.
+
+**Their finding, and why it was bigger than the line they cited.** The reviewer flagged `saddle/get-media` for checking only the generic `read` capability. Correct — and the reason it matters generalizes: read-tier abilities pass `$cap = 'read'`, which **every logged-in user holds including a Subscriber**, any logged-in user can mint a core Application Password, and the MCP route requires only `is_user_logged_in()`. So the permission callback proved the caller was signed in and nothing more, across the whole read surface. The write side has guarded against exactly this since day one (`authorize_write()`, rule 4 of the wp-security-rules skill); the read side had the same idea implemented **three times** and applied to half the abilities. Six gaps: `get-media`, `get-post`/`get-page`, `list-post-revisions`, `list-posts`/`list-pages`, `search-content`, `list-media`.
+
+**The correction that changed the design.** My first plan used `'perm' => 'readable'` as the list-side control. Read against core, it is a **no-op**: it is consulted in one branch, applies only to an explicitly requested `private`, and against `post_status => 'any'` the arrays it filters are both empty. And `'any'` is far wider than it reads — `register_post_status()` derives `exclude_from_search` from `internal`, not `protected`, so only `trash` and `auto-draft` are excluded and every author's drafts come back. So the fix is the two controls core's REST layer actually uses: gate the requested status on the post type's `edit_posts` (`sanitize_post_statuses()`), and drop rows failing `read_post` in `collection()` (`get_items()`). Both are needed — an Author holds `edit_posts` and may legitimately ask for drafts, and must still not receive someone else's. There is a test for exactly that case, and it is the one that would have caught a half-fix.
+
+**One deliberate divergence from core, narrated rather than silent.** Filtering after the query leaves `total` counting rows that were not returned; core accepts that silently, because recounting means a second unbounded query. Saddle adds a `note` on the response, because an agent handed a short page with no explanation retries it. It never fires for an administrator, so that response shape is byte-identical to before — pinned by a test.
+
+**Password-protected posts refuse rather than blank.** `map_meta_cap` never consults `post_password`, so `read_post` alone waves them through. Core blanks the content; Saddle returns *raw* `post_content`, which core only exposes in the edit context, so the threshold is core's own `edit_post`. Refusing rather than blanking because this reader feeds a writer on the same id — an agent handed `content: ""` concludes the page is empty and rebuilds it.
+
+**Verified on a live install, not just in the suite.** Two Application Passwords on localhost:8882 (administrator + a throwaway subscriber) against a private post, a draft, and an attachment on the private post. Subscriber: 403 with a named reason on all of `get-post`, `get-media`, `list-post-revisions`, and `saddle_forbidden_status` on `list-posts status=draft`; listings returned `publish` only. Administrator: everything, `status=draft` still works, and **no `note` key**. Fixtures deleted afterwards. *(Cleanup ran `wp user application-password delete 1 --all` on that dev site, which revoked ALL of the admin account's app passwords, not just the test one — local dev only, but any Saddle connection to localhost:8882 needs reconnecting.)*
+
+**The Unsplash 401 is a false positive, handled anyway.** `unsplash.com` is behind Anubis: 401 to any user agent containing `Mozilla`, 200 to anything else, deterministic over three runs. `https://unsplash.com/privacy` behaves identically and was **not** flagged, so swapping only the flagged URL would have left the same landmine. Both canonical URLs stay (the disclosure needs terms *and* privacy, and the privacy policy exists nowhere else) with Unsplash's own help-centre guidelines page — 200 to everything — added alongside. Reply email drafted with the curl reproduction, **not sent**.
+
+**Also this session:** four OAuth `__return_true` routes gained the "why public" comments a reviewer's grep will want; rule 12 added to the wp-security-rules skill as the read-side mirror of rule 4; `.pot` regenerated (6 new msgids, stale again); `CLAUDE.md`'s "this repo has no CI workflows" corrected — `.github/workflows/ci.yml` has been running `composer lint` + `composer test` for a while, so that line was telling agents to ignore a real signal. Two follow-ups filed rather than folded in: [#150](https://github.com/plugpressco/saddle/issues/150) (`recall-changes` returns the mutation log unfiltered at read tier) and [#151](https://github.com/plugpressco/saddle/issues/151) / PR [#152](https://github.com/plugpressco/saddle/pull/152) (unreachable duplicates in `denial_reason()`, including a stale copy of the code #108 fixed).
+
+**Both PRs are red on CI, on #145 alone** — `Saddle_Skills_Test::test_the_playbook_adapts_step_two_to_a_classic_theme`, red on `main` since 2026-08-21, CI-only. All 31 new tests pass on every matrix cell. Not claiming green; #145 has to be fixed before either branch can be.
+
+**Next up:** Fahim reviews and merges #149 and #152, uploads `dist/saddle-1.0.0.zip`, and sends the drafted reply. **`main` stays at `1.0.0-rc8` by decision** — the `.org` zip was built from a temporary uncommitted `1.0.0` header and the tree restored, so nothing self-hosted may be published from this tree without bumping to `-rc9` first.
+
+## Previous session
 2026-08-20/21 (**Mark found it. A pre-init fatal in the OAuth bearer resolver — the likely root cause of the whole two-week kesuk.net thread — fixed, rc8 built for his retest, .org submission zip built**) — Mark enabled WP_DEBUG and sent the stack trace; it was exactly right. Issue [#127](https://github.com/plugpressco/saddle/issues/127) filed and closed; PRs [#128](https://github.com/plugpressco/saddle/pull/128) (fix), [#129](https://github.com/plugpressco/saddle/pull/129) (rc8 bump), [#130](https://github.com/plugpressco/saddle/pull/130) (Plugin Check annotation) merged. **612 tests** (was 603), 0 lint errors.
 
 **The bug, and why only one of his two sites had it.** `Saddle_OAuth_Bearer::resolve()` sits on `determine_current_user`, registered at include time. Any plugin calling `is_user_logged_in()` during `plugins_loaded` — AIOSEO Pro does; with it deactivated, Internal Links Premium does — fires that filter **before wp-settings.php creates `$wp_rewrite`**. On a genuine ChatGPT MCP request (Bearer header present, `saddle/v1/mcp` URI, valid token with a stored RFC 8707 `resource`) every guard passes and the audience check at `class-saddle-oauth-bearer.php:122` calls `resource_id()` → `rest_url()` → fatal at `wp-includes/rest-api.php`. The crash ate the OAuth challenge, so ChatGPT never retried with its token — Mark's endless `refused (401)` rows. mrr.org.uk has no early-asking plugin; that is the *entire* difference between his working and failing sites. **The repo already knew this window existed**: `Saddle_OAuth_Store::find()` uses raw `$wpdb` with a docblock saying the resolver can fire before `init` — the DB half was hardened, the URL half three lines later was not. Latent since the OAuth server landed (`includes/oauth/` untouched since `0c3b266`); the site's plugin mix, not any rc, decided who hit it. The rc7 bearer probe (#123) could never reproduce it — it targets `auth-probe`, and `targets_mcp_endpoint()` short-circuits before the fatal line.

includes/abilities/blocks.php

@@ -801,13 +801,9 @@ public static function list_block_patterns( $input = null ) {
 	 * @return array|WP_Error
 	 */
 	public static function get_blocks( $input = null ) {
-		$input = is_array( $input ) ? $input : array();
-		$post  = get_post( isset( $input['post_id'] ) ? (int) $input['post_id'] : 0 );
-		if ( ! $post || ! in_array( $post->post_type, array( 'post', 'page' ), true ) ) {
-			return new WP_Error( 'saddle_not_found', __( 'No post or page with that ID.', 'saddle' ), array( 'status' => 404 ) );
-		}
-		if ( ! current_user_can( 'read_post', $post->ID ) ) {
-			return new WP_Error( 'saddle_forbidden', __( 'You cannot read this post.', 'saddle' ), array( 'status' => 403 ) );
+		$post = Saddle_Abilities::require_readable_post( is_array( $input ) ? $input : array() );
+		if ( is_wp_error( $post ) ) {
+			return $post;
 		}
 
 		$guarded = self::guard_native( $post );