Skip to content

fix(log): authorize the change history against its objects (#150) - #160

Merged
ifahimreza merged 1 commit into
mainfrom
fix/150-recall-changes-read-filter
Aug 26, 2026
Merged

fix(log): authorize the change history against its objects (#150)#160
ifahimreza merged 1 commit into
mainfrom
fix/150-recall-changes-read-filter

Conversation

@ifahimreza

Copy link
Copy Markdown
Contributor

Closes #150

What

saddle/recall-changes and the "recent changes" section of the system context both
return Saddle_Log::recent_executed() unfiltered. Each row's summary carries the
title of the thing that changed, so a connection made with a low-permission account
could enumerate the titles of drafts and private posts it can never open. This filters
each row against the object it names.

Why

Found while sweeping the read surface for #148 and deliberately left out of that PR,
which was a review blocker that needed to stay reviewable. It is the same class of gap
the WordPress.org reviewer raised — the read tier's capability is read, which every
logged-in user holds — and #148's fix does not reach it, because the log is its own CPT
and never passes through collection().

How

  • The filter lives in Saddle_Log::recent_executed(), not in the ability. Both callers
    share it, and the context path is the one that reaches every connected session at
    whatever tier — fixing only the ability would have left it disclosing the same titles.
  • entry_is_visible() judges a row the way Saddle_Abilities::collection() judges a
    listing: read_post against the target.

Two edges, both deliberate and both pinned by tests:

  • A row naming no post stays. Settings changes, plugin activations and cache flushes
    have no object to authorize against, and the action itself is admin tier. Term ids are
    numeric and are judged as post ids; the worst that costs is hiding a public taxonomy
    row from a low-privilege connection, never disclosing anything.
  • A row whose post no longer exists stays for an account that can delete content.
    That row is the record of a deletion, which is what this log exists for; dropping it
    would erase deletion history from the owner's own record. A trashed post is still a
    post, so it follows read_post normally — which for trash falls through to the edit
    capabilities.

Every post the log names is primed in one query before the per-row check, so this is not
a query in a loop.

Saddle_Context_Test now runs as an administrator. It never set a current user, and the
system context is only ever assembled for a resolved one; without a user
recent_executed() correctly withholds every row naming a post, which is right for user
0 and wrong for what those tests measure.

Testing

  • composer test653 tests, 2229 assertions, 0 failures (was 645). The eight
    new cases are in tests/read-authorization-test.php, written red first: the three
    disclosure cases failed naming the exact symptom before the fix landed.
  • composer lint — 0 errors (3 pre-existing warnings, none in the changed files).
  • Live-install check with two Application Passwords — to run before the .org zip is
    rebuilt.
  • No notices or warnings with WP_DEBUG on (the suite runs with it enabled).

Screenshots

None — no admin UI change. The admin Activity screen reads Saddle_Log::query(), which
is manage_options-gated and untouched.

recall-changes and the system context's "recent changes" section are both
read tier, and the read tier's capability is `read` — which every logged-in
user holds, a Subscriber included. Each row's `summary` carries the title of
the thing that changed, so an account that could never open a draft was
being handed its title anyway. Same class as #148; different mechanism,
because the log is its own CPT and never passes through collection().

The filter goes in Saddle_Log::recent_executed(), not in the ability: the
same rows are assembled into the context every connected session receives,
and fixing only the ability would leave that path disclosing the same
titles.

Two edges, both deliberate and both pinned:

- A row naming no post — a settings change, a plugin activation — has no
  object to authorize against and is an admin-tier action in the first
  place. It stays. Term ids are numeric and get judged as post ids; the
  worst that costs is hiding a public taxonomy row from a low-privilege
  connection, never disclosing anything.
- A row whose post no longer exists is the record of a deletion, which is
  what this log is for. Dropping it would erase deletion history from the
  owner's own record, so it survives for an account that can delete content
  and is withheld from one that cannot.

Every named post is primed in one query before the per-row check, so this
is not a query in a loop.

Saddle_Context_Test now runs as an administrator. It never set a current
user, and the system context is only ever assembled for a resolved one —
without a user, recent_executed() correctly withholds every row naming a
post, which is right for user 0 and wrong for what those tests measure.

Closes #150
@ifahimreza ifahimreza linked an issue Aug 26, 2026 that may be closed by this pull request
@ifahimreza
ifahimreza merged commit b83ae2f into main Aug 26, 2026
1 of 7 checks passed
@ifahimreza
ifahimreza deleted the fix/150-recall-changes-read-filter branch August 26, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

recall-changes returns the mutation log unfiltered at read tier

1 participant