From 6768cd0b77e4543fac69663b0e99582bdfc84d52 Mon Sep 17 00:00:00 2001 From: fahimreza-dev Date: Mon, 7 Sep 2026 03:28:45 +0600 Subject: [PATCH] fix(oauth): advertise offline_access, and treat it as asking for nothing ChatGPT checks discovery metadata for offline_access before trusting refresh-token renewal. Advertise it in both documents, and strip it at authorize and refresh before the scope decision, so a client sending only that word is neither pinned to read nor narrowed on renewal. Closes #159 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01ApYKEKcBnpuz8w6BX2agfH --- .../oauth/class-saddle-oauth-discovery.php | 6 ++- .../oauth/class-saddle-oauth-endpoints.php | 15 ++++-- includes/oauth/class-saddle-oauth.php | 49 +++++++++++++++++++ tests/oauth-discovery-test.php | 18 +++++++ tests/oauth-flow-test.php | 29 +++++++++++ tests/oauth-scope-test.php | 26 ++++++++++ 6 files changed, 138 insertions(+), 5 deletions(-) diff --git a/includes/oauth/class-saddle-oauth-discovery.php b/includes/oauth/class-saddle-oauth-discovery.php index 5b4c4ac..f01cc2a 100644 --- a/includes/oauth/class-saddle-oauth-discovery.php +++ b/includes/oauth/class-saddle-oauth-discovery.php @@ -330,7 +330,7 @@ public static function protected_resource_metadata() { $document = array( 'resource' => Saddle_OAuth::resource_id(), 'authorization_servers' => array( Saddle_OAuth::issuer() ), - 'scopes_supported' => Saddle_OAuth::SCOPES, + 'scopes_supported' => Saddle_OAuth::advertised_scopes(), 'bearer_methods_supported' => array( 'header' ), 'resource_name' => Saddle_MCP::server_name(), 'resource_documentation' => 'https://wordpress.org/plugins/saddle/', @@ -358,7 +358,9 @@ public static function authorization_server_metadata() { 'authorization_endpoint' => Saddle_OAuth::endpoint( 'authorize' ), 'token_endpoint' => Saddle_OAuth::endpoint( 'token' ), 'revocation_endpoint' => Saddle_OAuth::endpoint( 'revoke' ), - 'scopes_supported' => Saddle_OAuth::SCOPES, + // Includes `offline_access`: ChatGPT checks this list before trusting + // refresh-token renewal (#159). Advertised, never granted as access. + 'scopes_supported' => Saddle_OAuth::advertised_scopes(), 'response_types_supported' => array( 'code' ), 'response_modes_supported' => array( 'query' ), 'grant_types_supported' => array( 'authorization_code', 'refresh_token' ), diff --git a/includes/oauth/class-saddle-oauth-endpoints.php b/includes/oauth/class-saddle-oauth-endpoints.php index 08018b0..b3e64cf 100644 --- a/includes/oauth/class-saddle-oauth-endpoints.php +++ b/includes/oauth/class-saddle-oauth-endpoints.php @@ -154,7 +154,10 @@ public static function authorize( WP_REST_Request $request ) { return self::bounce( $redirect_uri, 'server_error', $request_id->get_error_message(), $state ); } - $asked = trim( (string) $request->get_param( 'scope' ) ); + // `offline_access` is a request for a refresh token, which every grant + // gets anyway — it says nothing about access, so it must not count as + // having "named a scope" below. See Saddle_OAuth::strip_refresh_scope(). + $asked = Saddle_OAuth::strip_refresh_scope( $request->get_param( 'scope' ) ); // A client that named its scopes is taken at its word — widening a // deliberate `saddle:read` request would be both a spec violation and a @@ -300,8 +303,14 @@ private static function grant_refresh_token( array $params ) { // A refresh may narrow the scope but never widen it — otherwise a // read-only grant could quietly promote itself on renewal. - if ( isset( $params['scope'] ) && '' !== $params['scope'] ) { - $requested = Saddle_OAuth::normalize_scope( (string) $params['scope'] ); + // + // `offline_access` is stripped first, same as at authorize: a refresh + // that names ONLY that word has not asked to narrow anything, and + // normalizing it alone would fall through to read — silently shrinking + // an admin grant on renewal, which is the opposite failure. + $asked = isset( $params['scope'] ) ? Saddle_OAuth::strip_refresh_scope( (string) $params['scope'] ) : ''; + if ( '' !== $asked ) { + $requested = Saddle_OAuth::normalize_scope( $asked ); if ( ! self::scope_is_subset( $requested, $scope ) ) { return Saddle_OAuth::error_response( 'invalid_scope', __( 'A refresh cannot ask for more access than was originally granted.', 'saddle' ) ); } diff --git a/includes/oauth/class-saddle-oauth.php b/includes/oauth/class-saddle-oauth.php index 69b867f..f6308ea 100644 --- a/includes/oauth/class-saddle-oauth.php +++ b/includes/oauth/class-saddle-oauth.php @@ -48,6 +48,22 @@ class Saddle_OAuth { */ const SCOPES = array( 'saddle:read', 'saddle:write', 'saddle:admin' ); + /** + * The OpenID Connect scope a client asks for to be issued a refresh token. + * + * Saddle issues refresh tokens unconditionally, so this scope changes + * nothing about what a grant can do — it is NOT a tier and never reaches + * {@see self::scope_to_tier()}. It exists because ChatGPT reads a provider's + * discovery metadata for it before trusting refresh-token renewal (OpenAI, + * "Developer mode and MCP apps in ChatGPT", 2026-08): a server that never + * advertises it may be treated as one that never renews, and the connector + * dies at ACCESS_TTL with a reconnect as the only remedy. See #159. + * + * Kept out of SCOPES on purpose: that constant is the tier-clamp set and + * must stay exactly three. + */ + const REFRESH_SCOPE = 'offline_access'; + /** * The scope challenged on an unauthenticated request. Least privilege: a * client that knows nothing else asks for read, and steps up if it needs to. @@ -326,6 +342,39 @@ public static function authorize_capability() { return (string) apply_filters( 'saddle_oauth_authorize_capability', 'manage_options' ); } + /** + * Every scope the discovery documents list: the three tier scopes plus the + * refresh scope. Discovery-only — {@see self::normalize_scope()} still grants + * from SCOPES alone, so advertising the extra word cannot widen a token. + * + * @return string[] + */ + public static function advertised_scopes() { + return array_merge( self::SCOPES, array( self::REFRESH_SCOPE ) ); + } + + /** + * Drop the refresh scope from a requested scope string, returning the rest. + * + * The authorize endpoint decides "did this client express a preference?" by + * whether `scope` is empty, and a client that asked for nothing is offered + * the site's own level. `offline_access` is not a preference about access: + * a client that sends ONLY that word has still asked for nothing, and must + * not be pinned to read-only for it (that is precisely the pre-#98 outcome + * for ChatGPT). Strip it BEFORE that decision, not inside normalize_scope(), + * whose "unrecognized words are a preference" rule is right for `openid + * profile` and wrong for this one word. + * + * @param string $requested Space-delimited scope string. + * @return string The same string without the refresh scope, trimmed. + */ + public static function strip_refresh_scope( $requested ) { + $asked = preg_split( '/\s+/', trim( (string) $requested ), -1, PREG_SPLIT_NO_EMPTY ); + $asked = is_array( $asked ) ? $asked : array(); + + return implode( ' ', array_values( array_diff( $asked, array( self::REFRESH_SCOPE ) ) ) ); + } + /** * Normalize a requested scope string to the scopes Saddle actually grants. * diff --git a/tests/oauth-discovery-test.php b/tests/oauth-discovery-test.php index 4388ba2..db831af 100644 --- a/tests/oauth-discovery-test.php +++ b/tests/oauth-discovery-test.php @@ -62,6 +62,24 @@ public function test_protected_resource_names_the_mcp_endpoint_and_its_server() $this->assertNotEmpty( $doc['scopes_supported'] ); } + /** + * ChatGPT reads `scopes_supported` for `offline_access` before trusting + * refresh-token renewal; a server that never lists it may be treated as one + * that never renews, and the connector dies at the access-token TTL (#159). + * Both documents carry it — and the tier-clamp set does not. + */ + public function test_offline_access_is_advertised_without_becoming_a_tier() { + $resource = Saddle_OAuth_Discovery::protected_resource_metadata(); + $server = Saddle_OAuth_Discovery::authorization_server_metadata(); + + $this->assertContains( 'offline_access', $resource['scopes_supported'] ); + $this->assertContains( 'offline_access', $server['scopes_supported'] ); + $this->assertContains( 'refresh_token', $server['grant_types_supported'], 'Advertising the scope only makes sense alongside the grant.' ); + + $this->assertSame( array( 'saddle:read', 'saddle:write', 'saddle:admin' ), Saddle_OAuth::SCOPES, 'SCOPES is the tier clamp and must stay exactly three.' ); + $this->assertSame( 'read', Saddle_OAuth::scope_to_tier( 'offline_access' ), 'The refresh scope must never resolve above the least tier.' ); + } + public function test_the_resource_id_has_no_trailing_slash() { // RFC 8707 canonical form. A client sends this back verbatim as // `resource=`, and the audience check is a string comparison. diff --git a/tests/oauth-flow-test.php b/tests/oauth-flow-test.php index 149c687..b1f6b33 100644 --- a/tests/oauth-flow-test.php +++ b/tests/oauth-flow-test.php @@ -482,6 +482,35 @@ public function test_refresh_cannot_widen_scope() { $this->assertSame( 'invalid_scope', $response->get_data()['error'] ); } + /** + * The refresh-side half of #159. A refresh naming only `offline_access` + * has not asked to narrow anything; before the strip it normalized to read + * and silently shrank the grant on renewal. + */ + public function test_refresh_asking_only_for_offline_access_keeps_the_granted_scope() { + Saddle_Capabilities::set_tier( 'admin' ); + + $client = $this->register_client(); + $verifier = $this->verifier(); + $code = $this->authorize( $client['client_id'], $this->challenge_for( $verifier ) ); + $tokens = $this->exchange( $client['client_id'], $code, $verifier )->get_data(); + + $request = new WP_REST_Request( 'POST', '/saddle/v1/oauth/token' ); + $request->set_body_params( + array( + 'grant_type' => 'refresh_token', + 'refresh_token' => $tokens['refresh_token'], + 'client_id' => $client['client_id'], + 'scope' => 'offline_access', + ) + ); + + $response = rest_get_server()->dispatch( $request ); + + $this->assertSame( 200, $response->get_status() ); + $this->assertSame( $tokens['scope'], $response->get_data()['scope'], 'The refresh scope alone must neither widen nor narrow the grant.' ); + } + public function test_unsupported_grant_type_is_named_as_such() { $request = new WP_REST_Request( 'POST', '/saddle/v1/oauth/token' ); $request->set_body_params( array( 'grant_type' => 'password' ) ); diff --git a/tests/oauth-scope-test.php b/tests/oauth-scope-test.php index be85044..565bd89 100644 --- a/tests/oauth-scope-test.php +++ b/tests/oauth-scope-test.php @@ -279,6 +279,32 @@ public function test_scopes_saddle_does_not_grant_are_dropped_without_widening() ); } + /** + * `offline_access` is the one unrecognized word that is NOT a preference + * about access: it asks for a refresh token, which every grant gets. Now + * that discovery advertises it (#159), a client that sends only that word + * has still asked for nothing and must reach consent proposing the site's + * own level — not be pinned to read the way `openid profile` is. + */ + public function test_asking_only_for_offline_access_is_asking_for_nothing() { + Saddle_Capabilities::set_tier( 'admin' ); + + $this->assertSame( + 'saddle:read saddle:write saddle:admin', + $this->pending_scope( array( 'scope' => 'offline_access' ) ) + ); + } + + public function test_offline_access_never_widens_an_explicit_request() { + Saddle_Capabilities::set_tier( 'admin' ); + + $this->assertSame( + 'saddle:read', + $this->pending_scope( array( 'scope' => 'saddle:read offline_access' ) ), + 'The refresh scope rides along; it must neither widen the request nor survive into the grant.' + ); + } + /* ------------------------------------------------------------------ * What the consent screen grants * --------------------------------------------------------------- */