Skip to content

Commit b83ae2f

Browse files
ifahimrezaifahimreza
authored
fix(log): authorize the change history against its objects (#160)
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
1 parent 43d30ed commit b83ae2f

4 files changed

Lines changed: 220 additions & 2 deletions

File tree

‎includes/class-saddle-log.php‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,10 @@ public static function query( $per_page = 20, $page = 1, $type = '' ) {
180180
* noise, not orientation an agent needs. Recency-bounded so a dormant site
181181
* serves nothing stale.
182182
*
183+
* Rows are filtered against the caller — see entry_is_visible(). Both
184+
* consumers are read tier, so the filter belongs here rather than in either
185+
* of them.
186+
*
183187
* @param int $limit Maximum entries (1–50).
184188
* @param int $days Recency window in days.
185189
* @return array[] Entries: date, action, target, summary. Newest first.
@@ -214,15 +218,71 @@ public static function recent_executed( $limit = 15, $days = 30 ) {
214218
);
215219

216220
$entries = array();
221+
$targets = array();
217222
foreach ( $q->posts as $post ) {
223+
$target = (string) get_post_meta( $post->ID, '_saddle_target', true );
218224
$entries[] = array(
219225
'date' => $post->post_date_gmt,
220226
'action' => (string) get_post_meta( $post->ID, '_saddle_action', true ),
221-
'target' => (string) get_post_meta( $post->ID, '_saddle_target', true ),
227+
'target' => $target,
222228
'summary' => $post->post_title,
223229
);
230+
if ( is_numeric( $target ) ) {
231+
$targets[ (int) $target ] = true;
232+
}
224233
}
225-
return $entries;
234+
235+
// Prime every post the log names in one query, so the per-row check
236+
// below is not a query in a loop. read_post resolves an attachment's
237+
// status through post_parent, so those get primed too.
238+
if ( $targets ) {
239+
$ids = array_keys( $targets );
240+
_prime_post_caches( $ids, false, false );
241+
$primed = array_filter( array_map( 'get_post', $ids ) );
242+
if ( $primed ) {
243+
update_post_parent_caches( $primed );
244+
}
245+
}
246+
247+
return array_values( array_filter( $entries, array( __CLASS__, 'entry_is_visible' ) ) );
248+
}
249+
250+
/**
251+
* Whether one log row may be shown to the current user.
252+
*
253+
* `summary` carries the title of the thing that changed, so a row about a
254+
* post discloses that post. Both consumers are read tier, and the read
255+
* tier's capability is `read` — which every logged-in user holds, a
256+
* Subscriber included. So each row is judged against its own object, the
257+
* same way a listing is judged in Saddle_Abilities::collection().
258+
*
259+
* Two edges, both deliberate:
260+
*
261+
* - A row naming no post — a settings change, a plugin activation, a cache
262+
* flush — has no object to authorize against, and the action itself is
263+
* admin tier. It stays. Term ids are numeric and are judged as post ids;
264+
* the worst that costs is hiding a public taxonomy row from a
265+
* low-privilege connection, never disclosing anything.
266+
* - A row whose post no longer exists is the record of a deletion, which is
267+
* the thing this log exists for. Dropping it would erase deletion history
268+
* from the owner's own record, so it survives for an account that could
269+
* have deleted content and is withheld from one that could not.
270+
*
271+
* @param array $entry One entry assembled by recent_executed().
272+
* @return bool
273+
*/
274+
private static function entry_is_visible( array $entry ) {
275+
$target = isset( $entry['target'] ) ? $entry['target'] : '';
276+
if ( ! is_numeric( $target ) ) {
277+
return true;
278+
}
279+
280+
$id = (int) $target;
281+
if ( ! get_post( $id ) ) {
282+
return current_user_can( 'delete_posts' );
283+
}
284+
285+
return current_user_can( 'read_post', $id );
226286
}
227287

228288
/**

‎readme.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ If you happen to have the separate MCP Adapter plugin active, Saddle notices and
148148
* The Dashboard opens with a sentence telling you what your AI can do right now, instead of four boxes of numbers. The counts moved to one quiet line underneath, and the box that used to show a dash when there was nothing to report is gone.
149149
* Removed the Cookbook screen.
150150
* Security: read tools now check whether the connected account is actually allowed to see each item, not just that it is signed in. A connection made with a low-permission WordPress account could previously read any draft, private or password-protected post, any media item's details and any post's revision history — and could list and search all of it — even though the account could never see any of it in wp-admin. Nothing changes for the usual setup, where you connect as an administrator.
151+
* Security: the list of recent changes — both the one an assistant can ask for and the one it reads when it connects — now hides entries about items the connected account cannot see. It was naming the titles of drafts and private posts to accounts that could never open them. Entries about something that has since been deleted stay visible to accounts that can delete content, so you do not lose that history.
151152
* Fixed: on sites where another plugin checks who is signed in very early in the request — several SEO plugins do — every ChatGPT request crashed before Saddle could examine its token, so the connection failed with a sign-in error forever while the same build worked elsewhere. The token check now works no matter how early in the request it runs.
152153
* Fixed: the connection check could report that sign-ins were working on a server that was actually blocking half of them. Apps you connect with a pasted key send one kind of sign-in header and apps that sign in through Saddle — ChatGPT is the one that can only connect that way — send another, and some servers pass the first and drop the second. The check now tests both, says which one is being blocked, and offers the same one-click fix, which always covered both.
153154
* Connection details and health now shows how each request signed in, so "the key was rejected" and "no key ever arrived" stop looking identical. They are the same error message and they need opposite fixes — one is reconnecting the app, the other is a word with your host.

‎tests/context-test.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,19 @@
88

99
class Saddle_Context_Test extends WP_UnitTestCase {
1010

11+
/**
12+
* The system context is only ever assembled for a resolved WordPress user —
13+
* the MCP route requires one before any of this runs. Test as the normal
14+
* Saddle setup, an owner's administrator credential, so the recent-changes
15+
* section is judged the way it is in the field. Without a user,
16+
* Saddle_Log::recent_executed() correctly withholds every row that names a
17+
* post, which is right for user 0 and wrong for what these tests measure.
18+
*/
19+
public function set_up() {
20+
parent::set_up();
21+
wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) );
22+
}
23+
1124
public function tear_down() {
1225
delete_option( Saddle_Capabilities::OPTION );
1326
delete_option( 'active_plugins' );

‎tests/read-authorization-test.php‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,150 @@ public function test_get_preview_url_still_works_for_the_owner() {
482482
$this->assertNotEmpty( $result['url'] );
483483
}
484484

485+
486+
/* -------- recall-changes and the recent-changes context (#150) -------- */
487+
488+
/**
489+
* The mutation log's rows carry a human-readable summary — usually the
490+
* title of the thing that changed. `recall-changes` is read tier, and the
491+
* read tier's capability is `read`, so without a filter a subscriber's
492+
* credential can enumerate the titles of drafts and private posts it can
493+
* never open. Same class as the rest of this file; different mechanism,
494+
* because the log is its own CPT and never passes through collection().
495+
*/
496+
private function log_change_on( $post_id, $summary ) {
497+
Saddle_Log::record_action( 'update-post', $post_id, $summary );
498+
}
499+
500+
private function summaries( array $result ) {
501+
return wp_list_pluck( $result['changes'], 'summary' );
502+
}
503+
504+
public function test_subscriber_does_not_see_log_entries_for_another_authors_draft() {
505+
$draft = $this->other_post( 'draft' );
506+
$this->log_change_on( $draft, 'Updated "Confidential draft post"' );
507+
$this->as_subscriber();
508+
509+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
510+
511+
$this->assertNotContains(
512+
'Updated "Confidential draft post"',
513+
$this->summaries( $result ),
514+
'recall-changes must not disclose the title of a draft the caller cannot read.'
515+
);
516+
}
517+
518+
public function test_subscriber_does_not_see_log_entries_for_another_authors_private_post() {
519+
$private = $this->other_post( 'private' );
520+
$this->log_change_on( $private, 'Updated "Confidential private post"' );
521+
$this->as_subscriber();
522+
523+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
524+
525+
$this->assertNotContains( 'Updated "Confidential private post"', $this->summaries( $result ) );
526+
}
527+
528+
public function test_subscriber_still_sees_log_entries_for_published_content() {
529+
$published = $this->other_post( 'publish' );
530+
$this->log_change_on( $published, 'Updated "Confidential publish post"' );
531+
$this->as_subscriber();
532+
533+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
534+
535+
$this->assertContains(
536+
'Updated "Confidential publish post"',
537+
$this->summaries( $result ),
538+
'A change to public content is not a disclosure, and the ability has to stay useful at read tier.'
539+
);
540+
}
541+
542+
/**
543+
* Settings changes, plugin activations and the like name no post. They are
544+
* already admin-tier actions — nothing below admin can perform one — so the
545+
* row itself is the record of something the owner did, and there is no
546+
* per-object capability to consult. They stay.
547+
*/
548+
public function test_a_log_entry_naming_no_post_survives_the_filter() {
549+
Saddle_Log::record_action( 'update-option', '', 'Changed the site tagline' );
550+
$this->as_subscriber();
551+
552+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
553+
554+
$this->assertContains( 'Changed the site tagline', $this->summaries( $result ) );
555+
}
556+
557+
public function test_administrator_sees_every_log_entry() {
558+
$draft = $this->other_post( 'draft' );
559+
$this->log_change_on( $draft, 'Updated "Confidential draft post"' );
560+
561+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
562+
563+
$this->assertContains(
564+
'Updated "Confidential draft post"',
565+
$this->summaries( $result ),
566+
'The owner\'s administrator credential must be unaffected.'
567+
);
568+
$this->assertSame( 1, $result['count'] );
569+
}
570+
571+
/**
572+
* The record of a deletion is the thing the log exists for, and the object
573+
* it names is gone, so there is nothing left to authorize against. It
574+
* survives for an account that could have deleted content, and is withheld
575+
* from one that could not — an owner never loses deletion history, and a
576+
* read-only connection never gains it.
577+
*/
578+
public function test_a_log_entry_whose_post_is_gone_survives_for_an_account_that_can_delete() {
579+
$gone = $this->other_post( 'publish' );
580+
$this->log_change_on( $gone, 'Deleted "Confidential publish post"' );
581+
wp_delete_post( $gone, true );
582+
583+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
584+
$this->assertContains( 'Deleted "Confidential publish post"', $this->summaries( $result ) );
585+
586+
$this->as_subscriber();
587+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
588+
$this->assertNotContains( 'Deleted "Confidential publish post"', $this->summaries( $result ) );
589+
}
590+
591+
/**
592+
* A trashed post is still a post, so the ordinary read_post mapping decides
593+
* — which for `trash` falls through to the edit capabilities. Pinned
594+
* because it is the common shape: delete-post trashes by default.
595+
*/
596+
public function test_a_trashed_posts_entry_follows_read_post() {
597+
$trashed = $this->other_post( 'publish' );
598+
$this->log_change_on( $trashed, 'Trashed "Confidential publish post"' );
599+
wp_trash_post( $trashed );
600+
601+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
602+
$this->assertContains( 'Trashed "Confidential publish post"', $this->summaries( $result ) );
603+
604+
$this->as_subscriber();
605+
$result = $this->ability( 'saddle/recall-changes' )->execute( array() );
606+
$this->assertNotContains( 'Trashed "Confidential publish post"', $this->summaries( $result ) );
607+
}
608+
609+
/**
610+
* The filter belongs in Saddle_Log::recent_executed(), not in the ability:
611+
* the same rows are assembled into the "recent changes" section of the
612+
* system context that every connected session receives, at whatever tier.
613+
* Fixing only the ability would leave that path disclosing the same titles.
614+
*/
615+
public function test_the_recent_changes_context_section_is_filtered_too() {
616+
$draft = $this->other_post( 'draft' );
617+
$this->log_change_on( $draft, 'Updated "Confidential draft post"' );
618+
$this->as_subscriber();
619+
620+
$context = Saddle_Context::system_context();
621+
622+
$this->assertStringNotContainsString(
623+
'Confidential draft post',
624+
$context,
625+
'The system context is assembled from the same log rows and must be filtered at the source.'
626+
);
627+
}
628+
485629
/* -------- the tier itself must not have moved -------- */
486630

487631
/**

0 commit comments

Comments
 (0)