Skip to content

Commit 59b2003

Browse files
ifahimrezaclaude
andauthored
Phase 2: security hardening — token user-binding, credential marker, log caps, domain enforcement (supersedes #67) (#99)
* fix: audit-log failed confirmed-destructive calls; warn on wrapper collisions - Saddle_Approval::gate now logs a confirmed destructive execution even when the executor returns WP_Error — a partial mutation with no audit trail is worse than a noisy one. The entry carries a "FAILED after confirmation" marker plus the error message. - Saddle_Integrations::wrap surfaces genuine wrapper-name collisions via _doing_it_wrong instead of silently dropping the partner tool; its own wrappers are tracked so idempotent re-runs stay silent. Part of Phase 1 (correctness) of the 2026-07 architecture review (plugpressco/saddle-pro#39). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133CZhoFPY6BBChDDGEa22Q * feat: security hardening — token user-binding, credential marker, split log caps, domain enforcement - Approval tokens are bound to the previewing user: with several agents on one site, agent A's preview can no longer be confirmed by agent B (saddle_token_user_mismatch). - Saddle-issued app passwords are recorded by UUID in user meta (saddle_issued_credentials); credential scoping, the XML-RPC block, and client revoke/rotate/list key on that immutable marker instead of the user-editable display name. Legacy prefix-named keys migrate on sight. - Saddle_Log GC caps denials (300) and executed mutations (1000) as separate filterable buckets, batch raised to 500, so denial noise can never evict change history. - Opt-in domain-drift enforcement (saddle_enforce_tier_domain, default off): write/admin abilities refuse on a migrated/cloned domain until the owner re-confirms the tier; exposed via GET/POST /settings. Phase 2 of the 2026-07 architecture review (#63). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133CZhoFPY6BBChDDGEa22Q * style: Yoda condition on the token user-binding check phpcs flagged it after the merge; WPCS wants the call on the left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GkZr73cqaSesHRDG89Yy8S --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent c2bdf52 commit 59b2003

8 files changed

Lines changed: 294 additions & 22 deletions

includes/admin/class-saddle-rest.php

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,7 @@ public static function get_settings() {
512512
'domain' => array(
513513
'current' => Saddle_Capabilities::current_domain(),
514514
'recorded' => Saddle_Capabilities::recorded_tier_domain(),
515+
'enforced' => Saddle_Capabilities::is_domain_enforced(),
515516
),
516517
// The key itself never leaves the server — only whether one is
517518
// set, plus a last-4 hint so the owner can recognize it.
@@ -561,6 +562,10 @@ public static function update_settings( WP_REST_Request $request ) {
561562
Saddle_Capabilities::set_paused( (bool) $request->get_param( 'paused' ) );
562563
}
563564

565+
if ( array_key_exists( 'domain_enforced', $params ) ) {
566+
Saddle_Capabilities::set_domain_enforcement( (bool) $request->get_param( 'domain_enforced' ) );
567+
}
568+
564569
// Key absent from the body ⇒ untouched; '' or null ⇒ cleared;
565570
// non-empty ⇒ validated and saved.
566571
if ( array_key_exists( 'unsplash_access_key', $params ) ) {
@@ -988,6 +993,10 @@ public static function create_client( WP_REST_Request $request ) {
988993
$hints[ $item['uuid'] ] = $hint;
989994
update_user_meta( $user->ID, 'saddle_client_hints', $hints );
990995

996+
// The immutable ownership marker credential scoping keys on — the
997+
// display name alone is user-editable and can't be trusted for it.
998+
Saddle_Connection::mark_issued( $user->ID, $item['uuid'] );
999+
9911000
return new WP_REST_Response(
9921001
array(
9931002
'uuid' => $item['uuid'],
@@ -1017,13 +1026,15 @@ public static function get_clients() {
10171026

10181027
$passwords = WP_Application_Passwords::get_user_application_passwords( $user_id );
10191028
foreach ( (array) $passwords as $item ) {
1020-
if ( empty( $item['name'] ) || 0 !== strpos( $item['name'], self::CLIENT_PREFIX ) ) {
1029+
// Marker first (rename-proof), name prefix for legacy keys.
1030+
if ( empty( $item['uuid'] ) || ! Saddle_Connection::is_saddle_issued( $user_id, $item['uuid'] ) ) {
10211031
continue;
10221032
}
1033+
$name = isset( $item['name'] ) ? (string) $item['name'] : '';
10231034
$clients[] = array(
10241035
'uuid' => $item['uuid'],
1025-
'name' => $item['name'],
1026-
'label' => trim( substr( $item['name'], strlen( self::CLIENT_PREFIX ) ) ),
1036+
'name' => $name,
1037+
'label' => 0 === strpos( $name, self::CLIENT_PREFIX ) ? trim( substr( $name, strlen( self::CLIENT_PREFIX ) ) ) : $name,
10271038
'created' => isset( $item['created'] ) ? (int) $item['created'] : 0,
10281039
'last_used' => isset( $item['last_used'] ) ? $item['last_used'] : null,
10291040
'last_ip' => isset( $item['last_ip'] ) ? $item['last_ip'] : null,
@@ -1051,17 +1062,20 @@ public static function revoke_client( WP_REST_Request $request ) {
10511062
return new WP_Error( 'saddle_app_passwords_unavailable', __( 'Application Passwords are not available on this site.', 'saddle' ), array( 'status' => 500 ) );
10521063
}
10531064

1054-
// Only allow revoking a Saddle-prefixed password, so this endpoint can't
1055-
// be used to delete unrelated credentials.
1065+
// Only allow revoking a Saddle-issued password, so this endpoint can't
1066+
// be used to delete unrelated credentials. Checked via the immutable
1067+
// marker (with legacy name-prefix fallback), so a renamed key can
1068+
// still be revoked here.
10561069
$item = WP_Application_Passwords::get_user_application_password( $user_id, $uuid );
1057-
if ( ! $item || empty( $item['name'] ) || 0 !== strpos( $item['name'], self::CLIENT_PREFIX ) ) {
1070+
if ( ! $item || ! Saddle_Connection::is_saddle_issued( $user_id, $uuid ) ) {
10581071
return new WP_Error( 'saddle_client_not_found', __( 'No Saddle client with that ID.', 'saddle' ), array( 'status' => 404 ) );
10591072
}
10601073

10611074
$result = WP_Application_Passwords::delete_application_password( $user_id, $uuid );
10621075
if ( is_wp_error( $result ) ) {
10631076
return $result;
10641077
}
1078+
Saddle_Connection::unmark_issued( $user_id, $uuid );
10651079

10661080
// Drop the stored last-4 hint along with the credential.
10671081
$hints = get_user_meta( $user_id, 'saddle_client_hints', true );
@@ -1105,7 +1119,7 @@ public static function rotate_client( WP_REST_Request $request ) {
11051119

11061120
// Same guard as revoke_client: only Saddle-issued credentials.
11071121
$item = WP_Application_Passwords::get_user_application_password( $user->ID, $uuid );
1108-
if ( ! $item || empty( $item['name'] ) || 0 !== strpos( $item['name'], self::CLIENT_PREFIX ) ) {
1122+
if ( ! $item || ! Saddle_Connection::is_saddle_issued( $user->ID, $uuid ) ) {
11091123
return new WP_Error( 'saddle_client_not_found', __( 'No Saddle client with that ID.', 'saddle' ), array( 'status' => 404 ) );
11101124
}
11111125

@@ -1132,13 +1146,15 @@ public static function rotate_client( WP_REST_Request $request ) {
11321146

11331147
list( $raw_password, $new_item ) = $created;
11341148

1135-
// Swap the last-4 hint: old uuid out, new one in.
1149+
// Swap the last-4 hint and the issued marker: old uuid out, new one in.
11361150
$hint = substr( str_replace( ' ', '', $raw_password ), -4 );
11371151
$hints = get_user_meta( $user->ID, 'saddle_client_hints', true );
11381152
$hints = is_array( $hints ) ? $hints : array();
11391153
unset( $hints[ $uuid ] );
11401154
$hints[ $new_item['uuid'] ] = $hint;
11411155
update_user_meta( $user->ID, 'saddle_client_hints', $hints );
1156+
Saddle_Connection::unmark_issued( $user->ID, $uuid );
1157+
Saddle_Connection::mark_issued( $user->ID, $new_item['uuid'] );
11421158

11431159
return new WP_REST_Response(
11441160
array(

includes/class-saddle-approval.php

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,8 @@ public static function gate( array $args ) {
156156
}
157157

158158
/**
159-
* Create and persist a single-use token bound to an action and target.
159+
* Create and persist a single-use token bound to an action, a target, and
160+
* the previewing user.
160161
*
161162
* @param string $action Action identifier.
162163
* @param string $target Target identifier the token is bound to (e.g. post id).
@@ -197,6 +198,10 @@ private static function issue_token( $action, $target = '', $bind = '' ) {
197198
update_post_meta( $post_id, '_saddle_action', $action );
198199
update_post_meta( $post_id, '_saddle_target', $target );
199200
update_post_meta( $post_id, '_saddle_bind', $bind );
201+
// The token belongs to whoever saw the preview: with several agents on
202+
// one site (separate app passwords / users), agent A's preview must
203+
// not be confirmable by agent B.
204+
update_post_meta( $post_id, '_saddle_user', get_current_user_id() );
200205
update_post_meta( $post_id, '_saddle_expires', time() + self::TOKEN_TTL );
201206

202207
return $token;
@@ -218,7 +223,7 @@ private static function hash_token( $token ) {
218223
/**
219224
* Validate and consume a token. Single-use: the token record is deleted on
220225
* lookup regardless of outcome, so even a mismatched/expired token cannot be
221-
* retried.
226+
* retried. The consumer must be the same user the preview was issued to.
222227
*
223228
* @param string $token Candidate token.
224229
* @param string $action Action the token must be bound to.
@@ -257,11 +262,20 @@ public static function consume_token( $token, $action, $target = '', $bind = ''
257262
$stored_action = get_post_meta( $post_id, '_saddle_action', true );
258263
$stored_target = (string) get_post_meta( $post_id, '_saddle_target', true );
259264
$stored_bind = (string) get_post_meta( $post_id, '_saddle_bind', true );
265+
$stored_user = (int) get_post_meta( $post_id, '_saddle_user', true );
260266
$expires = (int) get_post_meta( $post_id, '_saddle_expires', true );
261267

262268
// Single-use: burn the token now, before any further branching.
263269
wp_delete_post( $post_id, true );
264270

271+
if ( get_current_user_id() !== $stored_user ) {
272+
return new WP_Error(
273+
'saddle_token_user_mismatch',
274+
__( 'This confirmation token was issued to a different user. Preview the action yourself, then confirm with the token it returns.', 'saddle' ),
275+
array( 'status' => 403 )
276+
);
277+
}
278+
265279
if ( $stored_action !== $action ) {
266280
return new WP_Error(
267281
'saddle_token_mismatch',

includes/class-saddle-capabilities.php

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,15 @@ class Saddle_Capabilities {
6060
*/
6161
const TIER_DOMAIN_OPTION = 'saddle_tier_domain';
6262

63+
/**
64+
* Option key for opt-in domain-drift enforcement. Off by default (warn
65+
* only). When on, write- and admin-tier abilities are refused while the
66+
* current domain differs from the one the tier was granted on — a cloned
67+
* or migrated database then lands read-only until the owner re-confirms
68+
* the access level (set_tier re-records the domain).
69+
*/
70+
const ENFORCE_DOMAIN_OPTION = 'saddle_enforce_tier_domain';
71+
6372
/**
6473
* Tier name => numeric rank. Higher rank = more power.
6574
*
@@ -265,6 +274,11 @@ public static function permission( $level, $cap = 'read', $short_name = null ) {
265274
return false;
266275
}
267276

277+
if ( 'read' !== $level && self::is_domain_enforced() && ! self::domain_matches_recorded() ) {
278+
self::log_denial( $short_name, 'domain' );
279+
return false;
280+
}
281+
268282
if ( ! self::tier_allows( $level ) ) {
269283
self::log_denial( $short_name, 'tier' );
270284
return false;
@@ -441,6 +455,15 @@ public static function denial_reason( $ability_name ) {
441455
),
442456
);
443457
}
458+
459+
// Domain enforcement sits between the capability and the tier here,
460+
// mirroring the order permission() checks them in.
461+
if ( 'read' !== $required && '' !== $required && self::is_domain_enforced() && ! self::domain_matches_recorded() ) {
462+
return array(
463+
'code' => 'saddle_domain_drift',
464+
'message' => __( 'This site\'s domain changed since write access was granted, and the owner has domain enforcement on — write tools are refused until they re-confirm the access level (Saddle → Permissions). Do not retry; tell the user.', 'saddle' ),
465+
);
466+
}
444467
if ( '' !== $required && ! self::tier_allows( $required ) ) {
445468
// The site allows this, but the credential in hand doesn't — the
446469
// app was granted a narrower scope when it was authorized. That
@@ -486,7 +509,7 @@ public static function denial_reason( $ability_name ) {
486509
* flood the log, and it never fires for anonymous requests.
487510
*
488511
* @param string|null $short_name Ability id without the 'saddle/' prefix.
489-
* @param string $reason 'paused' | 'capability' | 'disabled' | 'tier'.
512+
* @param string $reason 'paused' | 'capability' | 'disabled' | 'domain' | 'tier'.
490513
*/
491514
private static function log_denial( $short_name, $reason ) {
492515
if ( ! class_exists( 'Saddle_Log' ) ) {
@@ -515,6 +538,10 @@ private static function log_denial( $short_name, $reason ) {
515538
/* translators: %s: tool name. */
516539
$summary = sprintf( __( 'Blocked: the tool "%s" is turned off.', 'saddle' ), $tool );
517540
break;
541+
case 'domain':
542+
/* translators: %s: tool name. */
543+
$summary = sprintf( __( 'Blocked: the site domain changed since write access was granted — "%s" is refused until the access level is re-confirmed.', 'saddle' ), $tool );
544+
break;
518545
case 'tier':
519546
default:
520547
/* translators: %s: tool name. */
@@ -552,6 +579,25 @@ public static function set_paused( $paused ) {
552579
return update_option( self::PAUSED_OPTION, (bool) $paused );
553580
}
554581

582+
/**
583+
* Whether domain-drift enforcement is on (see ENFORCE_DOMAIN_OPTION).
584+
*
585+
* @return bool
586+
*/
587+
public static function is_domain_enforced() {
588+
return (bool) get_option( self::ENFORCE_DOMAIN_OPTION, false );
589+
}
590+
591+
/**
592+
* Turn domain-drift enforcement on or off.
593+
*
594+
* @param bool $enforce Whether write/admin abilities require the recorded domain.
595+
* @return bool
596+
*/
597+
public static function set_domain_enforcement( $enforce ) {
598+
return update_option( self::ENFORCE_DOMAIN_OPTION, (bool) $enforce );
599+
}
600+
555601
/**
556602
* The site's current hostname, for comparison against the recorded one.
557603
*

includes/class-saddle-connection.php

Lines changed: 69 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,15 @@ class Saddle_Connection {
3333
*/
3434
const HTACCESS_MARKER = 'Saddle Authorization Header';
3535

36+
/**
37+
* User-meta key recording the UUIDs of app passwords Saddle issued.
38+
*
39+
* The immutable identity credential scoping keys on: the display name
40+
* ("Saddle: …") is user-editable in wp-admin, and scoping that silently
41+
* stops applying after a rename would hand the key full wp/v2 access.
42+
*/
43+
const ISSUED_META = 'saddle_issued_credentials';
44+
3645
/**
3746
* Scope Saddle-issued credentials to Saddle's own REST surface.
3847
*
@@ -75,7 +84,7 @@ public static function scope_credentials( $response, $handler, $request ) {
7584
$uuid = function_exists( 'rest_get_authenticated_app_password' )
7685
? rest_get_authenticated_app_password()
7786
: null;
78-
if ( ! $uuid || ! self::is_saddle_credential( get_current_user_id(), $uuid ) ) {
87+
if ( ! $uuid || ! self::is_saddle_issued( get_current_user_id(), $uuid ) ) {
7988
return $response;
8089
}
8190

@@ -134,8 +143,10 @@ public static function block_xmlrpc_credentials( $error, $user, $item ) {
134143
if ( ! apply_filters( 'saddle_scope_credentials', true ) ) {
135144
return;
136145
}
137-
$prefix = class_exists( 'Saddle_REST_Admin' ) ? Saddle_REST_Admin::CLIENT_PREFIX : 'Saddle: ';
138-
if ( isset( $item['name'] ) && 0 === strpos( (string) $item['name'], $prefix ) ) {
146+
$is_saddle = isset( $item['uuid'] ) && $user instanceof WP_User
147+
? self::is_saddle_issued( $user->ID, (string) $item['uuid'] )
148+
: ( isset( $item['name'] ) && 0 === strpos( (string) $item['name'], class_exists( 'Saddle_REST_Admin' ) ? Saddle_REST_Admin::CLIENT_PREFIX : 'Saddle: ' ) );
149+
if ( $is_saddle ) {
139150
$error->add(
140151
'saddle_credential_scope',
141152
__( 'Saddle sign-in keys cannot be used over XML-RPC.', 'saddle' )
@@ -144,23 +155,73 @@ public static function block_xmlrpc_credentials( $error, $user, $item ) {
144155
}
145156

146157
/**
147-
* Whether the given application password UUID is one Saddle issued
148-
* (identified by the `Saddle: ` name prefix) for this user.
158+
* Record an app-password UUID as Saddle-issued for a user.
159+
*
160+
* @param int $user_id User the credential belongs to.
161+
* @param string $uuid Application password UUID.
162+
*/
163+
public static function mark_issued( $user_id, $uuid ) {
164+
$uuids = get_user_meta( (int) $user_id, self::ISSUED_META, true );
165+
$uuids = is_array( $uuids ) ? $uuids : array();
166+
if ( ! in_array( (string) $uuid, $uuids, true ) ) {
167+
$uuids[] = (string) $uuid;
168+
update_user_meta( (int) $user_id, self::ISSUED_META, $uuids );
169+
}
170+
}
171+
172+
/**
173+
* Forget a revoked credential's UUID.
174+
*
175+
* @param int $user_id User the credential belonged to.
176+
* @param string $uuid Application password UUID.
177+
*/
178+
public static function unmark_issued( $user_id, $uuid ) {
179+
$uuids = get_user_meta( (int) $user_id, self::ISSUED_META, true );
180+
if ( ! is_array( $uuids ) || ! in_array( (string) $uuid, $uuids, true ) ) {
181+
return;
182+
}
183+
$uuids = array_values( array_diff( $uuids, array( (string) $uuid ) ) );
184+
if ( $uuids ) {
185+
update_user_meta( (int) $user_id, self::ISSUED_META, $uuids );
186+
} else {
187+
delete_user_meta( (int) $user_id, self::ISSUED_META );
188+
}
189+
}
190+
191+
/**
192+
* Whether the given application password UUID is one Saddle issued for
193+
* this user.
194+
*
195+
* Keyed on the stored UUID marker, which survives a rename in wp-admin →
196+
* Application Passwords. Keys issued before the marker existed are only
197+
* recognizable by their `Saddle: ` name prefix — those migrate into the
198+
* marker on first sight, so a later rename can no longer un-scope them.
149199
*
150200
* @param int $user_id User the request authenticated as.
151201
* @param string $uuid Application password UUID.
152202
* @return bool
153203
*/
154-
private static function is_saddle_credential( $user_id, $uuid ) {
155-
if ( ! $user_id || ! class_exists( 'WP_Application_Passwords' ) ) {
204+
public static function is_saddle_issued( $user_id, $uuid ) {
205+
if ( ! $user_id || '' === (string) $uuid || ! class_exists( 'WP_Application_Passwords' ) ) {
156206
return false;
157207
}
208+
209+
$uuids = get_user_meta( (int) $user_id, self::ISSUED_META, true );
210+
if ( is_array( $uuids ) && in_array( (string) $uuid, $uuids, true ) ) {
211+
return true;
212+
}
213+
158214
$item = WP_Application_Passwords::get_user_application_password( (int) $user_id, (string) $uuid );
159215
if ( ! $item || empty( $item['name'] ) ) {
160216
return false;
161217
}
162218
$prefix = class_exists( 'Saddle_REST_Admin' ) ? Saddle_REST_Admin::CLIENT_PREFIX : 'Saddle: ';
163-
return 0 === strpos( (string) $item['name'], $prefix );
219+
if ( 0 === strpos( (string) $item['name'], $prefix ) ) {
220+
self::mark_issued( (int) $user_id, (string) $uuid );
221+
return true;
222+
}
223+
224+
return false;
164225
}
165226

166227
/**

0 commit comments

Comments
 (0)