PluginProbe
ThinkRank AI SEO – AI SEO Plugin for WordPress: Schema, XML Sitemaps, Meta Tags, Search Console & Local SEO / 2.9.0
ThinkRank AI SEO – AI SEO Plugin for WordPress: Schema, XML Sitemaps, Meta Tags, Search Console & Local SEO v2.9.0
2.9.0 2.8.0 2.7.0 2.6.0 2.5.0 2.4.0 2.3.0 2.2.0 2.1.1 2.1.0 2.0.2 2.0.1 2.0.0 1.32.0 1.31.0 1.30.0 1.29.0 1.28.0 1.27.0 1.26.0 1.25.0 trunk 1.0.0 1.0.1 1.0.2 All 50 releases
← All changes | includes/mcp/class-mcp-oauth.php +341 -76 1.32.0 → 2.9.0 View file →
@@ -16,10 +16,11 @@
16 16 * Security contract:
17 17 * - PKCE S256 REQUIRED (OAuth 2.1 public clients); codes are single-use,
18 18 * 60 s TTL, bound to client_id + redirect_uri + challenge.
19 19 * - /authorize gates on manage_options — only an admin can grant access.
20 - * - Access/refresh tokens stored only as SHA-256 hashes; the raw value
21 - * exists solely in the /token response. Constant-time comparison.
20 + * - Authorization codes and access/refresh tokens stored only as SHA-256
21 + * hashes; the raw value exists solely in the response that hands it out.
22 + * Constant-time comparison.
22 23 * - Tokens carry the read/write scope model; a read-only grant refuses
23 24 * every write tool, exactly like a read-only pairing token.
24 25 *
25 26 * State lives in the `thinkrank_mcp_oauth` option (clients, codes, tokens,
@@ -47,8 +48,25 @@
47 48 */
48 49 public const OPTION = 'thinkrank_mcp_oauth';
49 50
50 51 /**
52 + * Per-client "last used" stamps, kept OUT of self::OPTION.
53 + *
54 + * Every authenticated MCP call used to stamp this inside the credential
55 + * option, which meant ordinary tool traffic did a read-modify-write of the
56 + * whole client/code/token/refresh store. A tool call overlapping a token
57 + * refresh could write back its stale snapshot and erase a token the server
58 + * had just minted — the client then holds an access token the server has
59 + * no record of, and every later call 401s (#485).
60 + *
61 + * A cosmetic timestamp has no business sharing a store with credentials,
62 + * so it lives in its own option. Losing a race here costs one stamp.
63 + *
64 + * @since 2.1.0
65 + */
66 + public const LAST_USED_OPTION = 'thinkrank_mcp_oauth_last_used';
67 +
68 + /**
51 69 * Authorization-code lifetime (seconds). Deliberately short.
52 70 */
53 71 private const CODE_TTL = 60;
54 72
@@ -74,8 +92,26 @@
74 92 */
75 93 private const LAST_USED_THROTTLE = 60;
76 94
77 95 /**
96 + * Seconds to wait for the advisory lock before giving up and proceeding
97 + * unguarded. Short: these are user-facing OAuth endpoints, and waiting is
98 + * worse than the small race we are narrowing.
99 + *
100 + * @since 2.1.0
101 + */
102 + private const LOCK_TIMEOUT = 3;
103 +
104 + /**
105 + * Nesting depth of mutate() on this request, so a mutation that calls
106 + * another (grant -> mint) releases the lock once, at the outermost exit.
107 + *
108 + * @since 2.1.0
109 + * @var int
110 + */
111 + private static int $lock_depth = 0;
112 +
113 + /**
78 114 * How many registered clients to keep. RFC 7591 registration is open by
79 115 * necessity — a client must register BEFORE it can hold any credential —
80 116 * so without a cap anyone on the internet can grow this option without
81 117 * bound, and every state() read pays for it. Clients holding a live token
@@ -239,16 +275,18 @@
239 275
240 276 $name = isset( $body['client_name'] ) ? sanitize_text_field( (string) $body['client_name'] ) : 'MCP Client';
241 277 $client_id = 'trk_' . bin2hex( random_bytes( 16 ) );
242 278
243 - $state = self::state();
244 - $state['clients'][ $client_id ] = [
245 - 'redirect_uris' => $redirect_uris,
246 - 'name' => $name,
247 - 'created' => time(),
248 - ];
249 - $state['clients'] = self::prune_clients( $state );
250 - self::save( $state );
279 + self::mutate(
280 + static function ( array &$state ) use ( $client_id, $redirect_uris, $name ): void {
281 + $state['clients'][ $client_id ] = [
282 + 'redirect_uris' => $redirect_uris,
283 + 'name' => $name,
284 + 'created' => time(),
285 + ];
286 + $state['clients'] = self::prune_clients( $state );
287 + }
288 + );
251 289
252 290 return [
253 291 'client_id' => $client_id,
254 292 'client_id_issued_at' => time(),
@@ -308,8 +346,22 @@
308 346 'redirectable' => true,
309 347 ]
310 348 );
311 349 }
350 + // The challenge reaches us verbatim now (#487), so it is checked
351 + // against its own character set rather than cleaned as display text.
352 + // RFC 7636 unreserved base64url; an S256 challenge is 43 characters,
353 + // the wider bound leaves room for a client that pads.
354 + if ( ! preg_match( '/^[A-Za-z0-9\-._~]{43,128}$/', $challenge ) ) {
355 + return new \WP_Error(
356 + 'invalid_request',
357 + __( 'code_challenge is not a valid S256 challenge.', 'thinkrank' ),
358 + [
359 + 'status' => 400,
360 + 'redirectable' => true,
361 + ]
362 + );
363 + }
312 364
313 365 return [
314 366 'client_id' => $client_id,
315 367 'client_name' => $client['name'],
@@ -329,19 +381,29 @@
329 381 * @param int $user_id Approving admin user id.
330 382 * @return string The authorization code.
331 383 */
332 384 public static function issue_code( array $req, int $user_id ): string {
333 - $code = bin2hex( random_bytes( 32 ) );
334 - $state = self::state();
335 - $state['codes'][ $code ] = [
336 - 'client_id' => $req['client_id'],
337 - 'redirect_uri' => $req['redirect_uri'],
338 - 'challenge' => $req['code_challenge'],
339 - 'scope' => $req['scope'],
340 - 'user_id' => $user_id,
341 - 'expires' => time() + self::CODE_TTL,
342 - ];
343 - self::save( $state );
385 + $code = bin2hex( random_bytes( 32 ) );
386 +
387 + // Keyed by hash, like access and refresh tokens. The authorization
388 + // code is a bearer credential too, and this file's own contract says
389 + // the raw value exists solely in the response that hands it out — the
390 + // code was the one exception (#488). The exposure is small (60 s TTL,
391 + // single use, bound to client_id + redirect_uri + PKCE) but #396 made
392 + // exactly that argument about the pairing token and still hashed it.
393 + self::mutate(
394 + static function ( array &$state ) use ( $code, $req, $user_id ): void {
395 + $state['codes'][ self::hash( $code ) ] = [
396 + 'client_id' => $req['client_id'],
397 + 'redirect_uri' => $req['redirect_uri'],
398 + 'challenge' => $req['code_challenge'],
399 + 'scope' => $req['scope'],
400 + 'user_id' => $user_id,
401 + 'expires' => time() + self::CODE_TTL,
402 + ];
403 + }
404 + );
405 +
344 406 return $code;
345 407 }
346 408
347 409 // -- Token endpoint --------------------------------------------------
@@ -376,18 +438,34 @@
376 438 $client_id = isset( $body['client_id'] ) ? (string) $body['client_id'] : '';
377 439 $redirect_uri = isset( $body['redirect_uri'] ) ? (string) $body['redirect_uri'] : '';
378 440 $verifier = isset( $body['code_verifier'] ) ? (string) $body['code_verifier'] : '';
379 441
380 - $state = self::state();
381 - if ( '' === $code || ! isset( $state['codes'][ $code ] ) ) {
442 + // Claim the code and remove it in one guarded read-modify-write.
443 + // Single-use has to mean single-use: looking it up, saving the removal,
444 + // and letting a concurrent writer restore its pre-removal snapshot put
445 + // a spent code back in the store (#485). Looked up by hash, because
446 + // that is how issue_code() stores it (#488).
447 + $entry = self::mutate(
448 + static function ( array &$state ) use ( $code ) {
449 + $chash = self::hash( $code );
450 +
451 + if ( '' === $code || ! isset( $state['codes'][ $chash ] ) ) {
452 + return null;
453 + }
454 +
455 + $claimed = $state['codes'][ $chash ];
456 +
457 + // Removed whether or not verification below passes.
458 + unset( $state['codes'][ $chash ] );
459 +
460 + return $claimed;
461 + }
462 + );
463 +
464 + if ( null === $entry ) {
382 465 return self::oauth_error( 'invalid_grant', 'Unknown or expired authorization code.' );
383 466 }
384 - $entry = $state['codes'][ $code ];
385 467
386 - // Single-use: remove immediately whether or not verification passes.
387 - unset( $state['codes'][ $code ] );
388 - self::save( $state );
389 -
390 468 if ( $entry['expires'] < time() ) {
391 469 return self::oauth_error( 'invalid_grant', 'Authorization code expired.' );
392 470 }
393 471 if ( ! hash_equals( (string) $entry['client_id'], $client_id ) ) {
@@ -414,24 +492,39 @@
414 492 private static function grant_refresh_token( array $body ) {
415 493 $refresh = isset( $body['refresh_token'] ) ? (string) $body['refresh_token'] : '';
416 494 $client_id = isset( $body['client_id'] ) ? (string) $body['client_id'] : '';
417 495
418 - $state = self::state();
419 496 $rhash = self::hash( $refresh );
420 - if ( '' === $refresh || ! isset( $state['refresh'][ $rhash ] ) ) {
421 - return self::oauth_error( 'invalid_grant', 'Unknown refresh token.' );
497 +
498 + // Look up and rotate under one guard. A mismatched client_id must not
499 + // consume the token, so the check happens inside the mutation.
500 + $claim = self::mutate(
501 + static function ( array &$state ) use ( $refresh, $rhash, $client_id ): array {
502 + if ( '' === $refresh || ! isset( $state['refresh'][ $rhash ] ) ) {
503 + return [ 'error' => 'Unknown refresh token.' ];
504 + }
505 +
506 + $entry = $state['refresh'][ $rhash ];
507 +
508 + if ( '' !== $client_id && ! hash_equals( (string) $entry['client_id'], $client_id ) ) {
509 + return [ 'error' => 'client_id mismatch.' ];
510 + }
511 +
512 + // Rotate: drop old refresh + its access token.
513 + unset( $state['refresh'][ $rhash ] );
514 + if ( isset( $entry['access_hash'] ) ) {
515 + unset( $state['tokens'][ $entry['access_hash'] ] );
516 + }
517 +
518 + return [ 'entry' => $entry ];
519 + }
520 + );
521 +
522 + if ( isset( $claim['error'] ) ) {
523 + return self::oauth_error( 'invalid_grant', (string) $claim['error'] );
422 524 }
423 - $entry = $state['refresh'][ $rhash ];
424 - if ( '' !== $client_id && ! hash_equals( (string) $entry['client_id'], $client_id ) ) {
425 - return self::oauth_error( 'invalid_grant', 'client_id mismatch.' );
426 - }
427 525
428 - // Rotate: drop old refresh + its access token.
429 - unset( $state['refresh'][ $rhash ] );
430 - if ( isset( $entry['access_hash'] ) ) {
431 - unset( $state['tokens'][ $entry['access_hash'] ] );
432 - }
433 - self::save( $state );
526 + $entry = $claim['entry'];
434 527
435 528 return self::mint_tokens( (string) $entry['client_id'], (string) $entry['scope'], (int) $entry['user_id'] );
436 529 }
437 530
@@ -449,24 +542,26 @@
449 542 $refresh = bin2hex( random_bytes( 32 ) );
450 543 $ahash = self::hash( $access );
451 544 $rhash = self::hash( $refresh );
452 545
453 - $state = self::state();
454 - $state['tokens'][ $ahash ] = [
455 - 'client_id' => $client_id,
456 - 'scope' => $scope,
457 - 'user_id' => $user_id,
458 - 'expires' => time() + self::ACCESS_TTL,
459 - 'refresh' => $rhash,
460 - ];
461 - $state['refresh'][ $rhash ] = [
462 - 'access_hash' => $ahash,
463 - 'client_id' => $client_id,
464 - 'scope' => $scope,
465 - 'user_id' => $user_id,
466 - 'expires' => time() + self::REFRESH_TTL,
467 - ];
468 - self::save( $state );
546 + self::mutate(
547 + static function ( array &$state ) use ( $ahash, $rhash, $client_id, $scope, $user_id ): void {
548 + $state['tokens'][ $ahash ] = [
549 + 'client_id' => $client_id,
550 + 'scope' => $scope,
551 + 'user_id' => $user_id,
552 + 'expires' => time() + self::ACCESS_TTL,
553 + 'refresh' => $rhash,
554 + ];
555 + $state['refresh'][ $rhash ] = [
556 + 'access_hash' => $ahash,
557 + 'client_id' => $client_id,
558 + 'scope' => $scope,
559 + 'user_id' => $user_id,
560 + 'expires' => time() + self::REFRESH_TTL,
561 + ];
562 + }
563 + );
469 564
470 565 return [
471 566 'access_token' => $access,
472 567 'token_type' => 'Bearer',
@@ -500,18 +595,15 @@
500 595 return null;
501 596 }
502 597
503 598 // Record activity against the owning client so the "Connected AI apps"
504 - // list can show a last-used date. Throttled + stored on the client
505 - // record so it survives access-token rotation.
599 + // list can show a last-used date. Throttled, and written to its own
600 + // option: this runs on every authenticated MCP call, and writing it
601 + // back into the credential store meant ordinary tool traffic could
602 + // erase a token minted by an overlapping refresh (#485).
506 603 $client_id = (string) $entry['client_id'];
507 - $now = time();
508 604 if ( isset( $state['clients'][ $client_id ] ) && is_array( $state['clients'][ $client_id ] ) ) {
509 - $last = isset( $state['clients'][ $client_id ]['last_used'] ) ? (int) $state['clients'][ $client_id ]['last_used'] : 0;
510 - if ( $now - $last >= self::LAST_USED_THROTTLE ) {
511 - $state['clients'][ $client_id ]['last_used'] = $now;
512 - self::save( $state );
513 - }
605 + self::touch_last_used( $client_id, array_keys( $state['clients'] ) );
514 606 }
515 607
516 608 return [
517 609 'client_id' => $client_id,
@@ -540,8 +632,9 @@
540 632 * @return void
541 633 */
542 634 public static function revoke_all(): void {
543 635 delete_option( self::OPTION );
636 + delete_option( self::LAST_USED_OPTION );
544 637 }
545 638
546 639 /**
547 640 * The OAuth clients currently holding a live grant, for the "Connected AI
@@ -552,9 +645,10 @@
552 645 *
553 646 * @return array<int,array{client_id:string,name:string,scope:string,read_only:bool,user_id:int,connected_at:int,last_used:int}>
554 647 */
555 648 public static function connected_apps(): array {
556 - $state = self::state();
649 + $state = self::state();
650 + $last_used = self::last_used_map();
557 651
558 652 // Collect the scope + approving user per active client. Refresh tokens
559 653 // are the durable grant, so prefer them; fall back to access tokens.
560 654 $active = [];
@@ -580,9 +674,13 @@
580 674 'scope' => $info['scope'],
581 675 'read_only' => self::scope_is_read_only( $info['scope'] ),
582 676 'user_id' => $info['user_id'],
583 677 'connected_at' => isset( $client['created'] ) ? (int) $client['created'] : 0,
584 - 'last_used' => isset( $client['last_used'] ) ? (int) $client['last_used'] : 0,
678 + // Legacy fallback: stamps written before #485 still sit on the
679 + // client record, so an existing install keeps its dates.
680 + 'last_used' => isset( $last_used[ $cid ] )
681 + ? (int) $last_used[ $cid ]
682 + : ( isset( $client['last_used'] ) ? (int) $client['last_used'] : 0 ),
585 683 ];
586 684 }
587 685
588 686 // Newest connection first.
@@ -616,23 +714,31 @@
616 714 public static function revoke_client( string $client_id ): bool {
617 715 if ( '' === $client_id ) {
618 716 return false;
619 717 }
620 - $state = self::state();
621 - $removed = false;
718 + $removed = self::mutate(
719 + static function ( array &$state ) use ( $client_id ): bool {
720 + $found = false;
622 721
623 - foreach ( [ 'tokens', 'refresh', 'codes' ] as $bucket ) {
624 - foreach ( $state[ $bucket ] as $key => $entry ) {
625 - if ( isset( $entry['client_id'] ) && (string) $entry['client_id'] === $client_id ) {
626 - unset( $state[ $bucket ][ $key ] );
627 - $removed = true;
722 + foreach ( [ 'tokens', 'refresh', 'codes' ] as $bucket ) {
723 + foreach ( $state[ $bucket ] as $key => $entry ) {
724 + if ( isset( $entry['client_id'] ) && (string) $entry['client_id'] === $client_id ) {
725 + unset( $state[ $bucket ][ $key ] );
726 + $found = true;
727 + }
728 + }
628 729 }
730 +
731 + return $found;
629 732 }
630 - }
733 + );
631 734
632 735 if ( $removed ) {
633 - self::save( $state );
736 + $map = self::last_used_map();
737 + unset( $map[ $client_id ] );
738 + update_option( self::LAST_USED_OPTION, $map, false );
634 739 }
740 +
635 741 return $removed;
636 742 }
637 743
638 744 // -- State + helpers -------------------------------------------------
@@ -676,13 +782,172 @@
676 782
677 783 /**
678 784 * Persist state (autoload off — hot-write, request-scoped option).
679 785 *
786 + * Private on purpose: every mutation goes through mutate(), so that the
787 + * state being written was read inside the same guard.
788 + *
680 789 * @param array<string,mixed> $state State to persist.
681 790 * @return void
682 791 */
683 792 private static function save( array $state ): void {
684 793 update_option( self::OPTION, $state, false );
794 + }
795 +
796 + /**
797 + * Read-modify-write the OAuth state under a guard, re-reading inside it.
798 + *
799 + * Clients, codes, access tokens and refresh tokens share one option, and
800 + * every mutation used to read a snapshot at the top of the request and
801 + * write the whole thing back later. Two overlapping requests therefore had
802 + * one silently erase the other's work — the damaging order being a tool
803 + * call writing back a pre-refresh snapshot over a token pair that had just
804 + * been minted, leaving the client holding an access token the server has no
805 + * record of (#485).
806 + *
807 + * The mutator receives the state by reference and may return a value, which
808 + * is handed back to the caller — so a caller can claim-and-remove (a
809 + * single-use code, a rotating refresh token) without the lookup and the
810 + * removal being separate writes.
811 + *
812 + * @param callable $mutator function ( array &$state ): mixed
813 + * @return mixed Whatever the mutator returned.
814 + */
815 + private static function mutate( callable $mutator ) {
816 + $locked = self::lock();
817 +
818 + try {
819 + $state = self::state();
820 + $result = $mutator( $state );
821 + self::save( $state );
822 + } finally {
823 + if ( $locked ) {
824 + self::unlock();
825 + }
826 + }
827 +
828 + return $result;
829 + }
830 +
831 + /**
832 + * Take the cross-request advisory lock guarding self::OPTION.
833 + *
834 + * MySQL GET_LOCK is what WordPress gives us that actually holds ACROSS
835 + * processes — wp_cache_add() is per-request without a persistent object
836 + * cache, which is exactly the configuration this bug bites hardest on.
837 + * The name is namespaced by database + table prefix because GET_LOCK names
838 + * are server-wide and shared MySQL hosts are the common case.
839 + *
840 + * Best-effort by design: a host where the lock cannot be taken (SQLite
841 + * drop-in, a proxy that does not support session locks, contention past
842 + * the timeout) proceeds unguarded, which is exactly today's behaviour
843 + * rather than a new failure.
844 + *
845 + * @return bool Whether the lock is held.
846 + */
847 + private static function lock(): bool {
848 + global $wpdb;
849 +
850 + // Already inside a guarded mutation on this request (grant -> mint).
851 + // MySQL's lock is re-entrant per session; the depth counter is what
852 + // keeps the release paired with the outermost acquire.
853 + if ( self::$lock_depth > 0 ) {
854 + ++self::$lock_depth;
855 + return true;
856 + }
857 +
858 + if ( ! isset( $wpdb ) || ! is_object( $wpdb ) ) {
859 + return false;
860 + }
861 +
862 + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- advisory lock, not cacheable data.
863 + $got = $wpdb->get_var( $wpdb->prepare( 'SELECT GET_LOCK(%s, %d)', self::lock_name(), self::LOCK_TIMEOUT ) );
864 +
865 + if ( '1' !== (string) $got ) {
866 + return false;
867 + }
868 +
869 + self::$lock_depth = 1;
870 +
871 + return true;
872 + }
873 +
874 + /**
875 + * Release the advisory lock taken by lock(). Only the outermost mutation
876 + * actually releases it.
877 + *
878 + * @return void
879 + */
880 + private static function unlock(): void {
881 + global $wpdb;
882 +
883 + if ( self::$lock_depth <= 0 ) {
884 + return;
885 + }
886 +
887 + --self::$lock_depth;
888 +
889 + if ( self::$lock_depth > 0 || ! isset( $wpdb ) || ! is_object( $wpdb ) ) {
890 + return;
891 + }
892 +
893 + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- advisory lock, not cacheable data.
894 + $wpdb->get_var( $wpdb->prepare( 'SELECT RELEASE_LOCK(%s)', self::lock_name() ) );
895 + }
896 +
897 + /**
898 + * Lock name, inside MySQL's 64-character limit and unique per install.
899 + *
900 + * @return string
901 + */
902 + private static function lock_name(): string {
903 + global $wpdb;
904 +
905 + $prefix = isset( $wpdb ) && is_object( $wpdb ) ? (string) $wpdb->prefix : '';
906 +
907 + return 'trk_mcp_oauth_' . md5( ( defined( 'DB_NAME' ) ? (string) DB_NAME : '' ) . '|' . $prefix );
908 + }
909 +
910 + /**
911 + * Per-client last-used stamps, client_id => unix timestamp.
912 + *
913 + * @return array<string,int>
914 + */
915 + private static function last_used_map(): array {
916 + $stored = get_option( self::LAST_USED_OPTION, [] );
917 +
918 + return is_array( $stored ) ? $stored : [];
919 + }
920 +
921 + /**
922 + * Stamp a client as having just been used, at most once per throttle
923 + * window. Writes its own option, never the credential store.
924 + *
925 + * @param string $client_id Client to stamp.
926 + * @param string[] $known_clients Client ids that still exist, so the map
927 + * cannot outgrow the store it describes.
928 + * @return void
929 + */
930 + private static function touch_last_used( string $client_id, array $known_clients ): void {
931 + $map = self::last_used_map();
932 + $now = time();
933 + $last = isset( $map[ $client_id ] ) ? (int) $map[ $client_id ] : 0;
934 +
935 + if ( $now - $last < self::LAST_USED_THROTTLE ) {
936 + return;
937 + }
938 +
939 + $map[ $client_id ] = $now;
940 +
941 + // Drop stamps for clients that are gone (revoked, pruned, expired).
942 + $known = array_flip( $known_clients );
943 + foreach ( array_keys( $map ) as $id ) {
944 + if ( ! isset( $known[ $id ] ) ) {
945 + unset( $map[ $id ] );
946 + }
947 + }
948 +
949 + update_option( self::LAST_USED_OPTION, $map, false );
685 950 }
686 951
687 952 /**
688 953 * Look up a registered client.