Redis storage rehydrates sessions past the CIMDStorageDecorator, so CIMD clients fail every token exchange with invalid_grant

[alex-feel]

Bug description

With the embedded OAuth 2.0 authorization server running on the Redis storage backend, any client registered via CIMD (Client ID Metadata Document, i.e. a URL-shaped client_id) completes the authorization flow and receives an authorization code, but then every token exchange fails with invalid_grant. The ChatGPT connector is a real-world client that hits this: it registers via CIMD with client_id = https://chatgpt.com/oauth/<id>/client.json, signs in successfully, and then can never redeem its code.

Facts observed in source (at tag v0.41.0, commit d722304; all cited paths re-checked and unchanged on current main as of 2026-08-04):

• The server wraps its storage in the CIMD decorator: pkg/authserver/server_impl.go:234 calls storage.NewCIMDStorageDecorator(stor, ...).
• CIMDStorageDecorator.GetClient (pkg/authserver/storage/cimd_decorator.go:88-93) intercepts URL-shaped client_id values (oauthproto.IsClientIDMetadataDocumentURL) and resolves them by fetching the metadata document, with an in-memory LRU cache. Nothing on this path ever persists the resolved client into the wrapped storage's client store — a CIMD client exists only as this on-the-fly resolution.
• Redis session rehydration bypasses the decorator: unmarshalRequester (pkg/authserver/storage/redis.go:1854) resolves the stored session's client with client, err := s.GetClient(ctx, stored.ClientID) on the bare *RedisStorage receiver (line 1861), wrapping failures as failed to get client for session: %w (line 1863). It is invoked from GetAuthorizeCodeSession (line 332), GetAccessTokenSession (line 423), GetRefreshTokenSession (line 499), and GetPKCERequestSession (line 650).
• RedisStorage.GetClient (pkg/authserver/storage/redis.go:224) looks up the persisted client key and, for a client that was never registered, returns ErrNotFound (storage: item not found, pkg/authserver/storage/types.go:39) wrapped with fosite.ErrNotFound (line 230).
• MemoryStorage does not have this problem: GetAuthorizeCodeSession (pkg/authserver/storage/memory.go:435) returns the stored live fosite.Requester (entry.value, line 451) with its client pointer intact — the client is never re-resolved.
• Diagnosability: on token-exchange failure the handler logs only slog.Error("failed to create access request", "error", err) (pkg/authserver/server/handlers/token.go:30), and the fosite error stringifies as just invalid_grant. Neither the client_id nor the wrapped storage error (failed to get client for session: ...) reaches the log.

Interpretation: the authorize endpoint resolves the CIMD client through the decorated storage and succeeds, but the token endpoint's session read-back re-resolves the client through the inner *RedisStorage, which has never seen it. The defect therefore exists only in the CIMD × Redis combination — which is exactly what a production multi-replica deployment runs — while the memory backend (what unit tests exercise) keeps the live requester and cannot surface it.

Steps to reproduce

1. Clone the repo at tag v0.41.0 (commit d722304).
2. Create cmd/repro-redis-cimd/main.go with the harness below (self-contained; uses miniredis, a test-only dependency already present in go.mod — no go.mod/go.sum changes needed).
3. Run go run ./cmd/repro-redis-cimd. The run is deterministic; two consecutive runs produced identical results (modulo the ephemeral loopback port), exit code 0.

The harness is modeled directly on the repo's own test construction patterns (newTestRedisStorage/newRedisTestRequester in pkg/authserver/storage/redis_test.go, serveCIMDDoc/newEnabledDecorator in pkg/authserver/storage/cimd_decorator_test.go). It runs two variants through the identical CIMDStorageDecorator + RedisStorage stack: (1) a CIMD client resolved via the decorator, which fails at the token-exchange read-back, and (2) a contrast variant with a regular client persisted via RegisterClient (the same method the DCR handler uses), which succeeds.

cmd/repro-redis-cimd/main.go (click to expand)

// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
// SPDX-License-Identifier: Apache-2.0

// Command repro-redis-cimd is a minimal, throwaway reproduction of a defect in
// ToolHive's embedded OAuth 2.0 authorization server: RedisStorage rehydrates a
// persisted authorization-code session by calling GetClient on the BARE
// *RedisStorage (see unmarshalRequester in pkg/authserver/storage/redis.go),
// bypassing the CIMDStorageDecorator that the server wraps RedisStorage in
// (pkg/authserver/storage/cimd_decorator.go). A CIMD client (Client ID Metadata
// Document, resolved on the fly from a URL-shaped client_id) is never persisted
// to the client store, so the inner lookup fails and the token endpoint later
// reports invalid_grant for any client that authenticated via CIMD.
//
// This file is a disposable diagnostic harness. It is not part of the ToolHive
// build and is deleted after use.
package main

import (
	"context"
	"encoding/json"
	"errors"
	"fmt"
	"net/http"
	"net/http/httptest"
	"net/url"
	"os"
	"time"

	"github.com/alicebob/miniredis/v2"
	"github.com/ory/fosite"
	"github.com/redis/go-redis/v9"

	"github.com/stacklok/toolhive/pkg/authserver/server/session"
	"github.com/stacklok/toolhive/pkg/authserver/storage"
	"github.com/stacklok/toolhive/pkg/oauthproto/cimd"
)

func main() {
	fmt.Println("=== Repro: RedisStorage rehydration bypasses CIMDStorageDecorator ===")
	fmt.Println()

	if err := runCIMDVariant(); err != nil {
		fmt.Printf("[CIMD variant] harness error: %v\n", err)
		os.Exit(1)
	}

	fmt.Println()

	if err := runRegularClientVariant(); err != nil {
		fmt.Printf("[Contrast variant] harness error: %v\n", err)
		os.Exit(1)
	}
}

// newDecoratedRedisStorage starts miniredis, builds a *storage.RedisStorage
// exactly as pkg/authserver/storage/redis_test.go does (see newTestRedisStorage),
// then wraps it in a CIMDStorageDecorator exactly as the server does
// (see the NewCIMDStorageDecorator wiring in pkg/authserver/server_impl.go).
func newDecoratedRedisStorage() (storage.Storage, *storage.RedisStorage, func(), error) {
	mr := miniredis.NewMiniRedis()
	if err := mr.Start(); err != nil {
		return nil, nil, nil, fmt.Errorf("failed to start miniredis: %w", err)
	}

	client := redis.NewClient(&redis.Options{Addr: mr.Addr()})
	base := storage.NewRedisStorageWithClient(client, "repro:auth:")

	cleanup := func() {
		_ = base.Close()
		mr.Close()
	}

	decorated, err := storage.NewCIMDStorageDecorator(base, storage.CIMDDecoratorConfig{
		Enabled:              true,
		CacheMaxSize:         10,
		FallbackTTL:          time.Minute,
		ScopesSupported:      nil, // unconstrained AS: same shape as the decorator's own unit tests
		BaselineClientScopes: nil,
	})
	if err != nil {
		cleanup()
		return nil, nil, nil, fmt.Errorf("failed to build CIMDStorageDecorator: %w", err)
	}

	return decorated, base, cleanup, nil
}

// serveCIMDDoc starts an httptest.Server serving a valid CIMD document at path,
// copying the construction pattern from
// pkg/authserver/storage/cimd_decorator_test.go's serveCIMDDoc helper. httptest
// servers bind to 127.0.0.1, which oauthproto.IsClientIDMetadataDocumentURL and
// cimd.FetchClientMetadataDocument both accept for loopback HTTP as a
// test-only affordance (see pkg/oauthproto/cimd.go).
func serveCIMDDoc(path string) *httptest.Server {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.URL.Path != path {
			http.NotFound(w, r)
			return
		}
		clientID := "http://" + r.Host + r.URL.Path
		doc := cimd.ClientMetadataDocument{
			ClientID:     clientID,
			RedirectURIs: []string{"https://example.com/callback"},
		}
		w.Header().Set("Content-Type", "application/json")
		_ = json.NewEncoder(w).Encode(doc)
	}))
	return srv
}

// newRequestForClient builds a fosite.Request the same way
// pkg/authserver/storage/redis_test.go's newRedisTestRequester does, using the
// real session.Session type so JSON round-tripping through Redis works exactly
// as it does in the running server.
func newRequestForClient(id string, client fosite.Client) fosite.Requester {
	return &fosite.Request{
		ID:                id,
		RequestedAt:       time.Now(),
		Client:            client,
		RequestedScope:    fosite.Arguments{"openid", "profile"},
		GrantedScope:      fosite.Arguments{"openid"},
		RequestedAudience: fosite.Arguments{},
		GrantedAudience:   fosite.Arguments{},
		Form:              make(url.Values),
		Session:           session.New("test-subject", "", "", session.UserClaims{}),
	}
}

// runCIMDVariant is the defect-reproducing path: a CIMD client is resolved
// through the decorator (succeeds), an authorization-code session referencing
// that client is created through the decorator (as the /oauth/authorize
// handler would), and then read back through the decorator (as the token
// handler would at the /oauth/token exchange). The read-back is expected to
// fail because unmarshalRequester resolves the client via the bare
// *RedisStorage, which never saw this client persisted.
func runCIMDVariant() error {
	fmt.Println("--- Variant 1: CIMD client (e.g. ChatGPT connector) ---")

	decorated, _, cleanup, err := newDecoratedRedisStorage()
	if err != nil {
		return err
	}
	defer cleanup()

	srv := serveCIMDDoc("/client.json")
	defer srv.Close()

	documentURL := srv.URL + "/client.json"
	ctx := context.Background()

	// Step 1: resolve the CIMD client via the decorator, exactly as the
	// /oauth/authorize handler does when a client presents a URL-shaped
	// client_id.
	cimdClient, err := decorated.GetClient(ctx, documentURL)
	if err != nil {
		return fmt.Errorf("unexpected failure resolving CIMD client via decorator: %w", err)
	}
	fmt.Printf("Step 1 OK: decorator.GetClient resolved CIMD client, id=%s\n", cimdClient.GetID())

	// Step 2: create an authorization code session through the decorator,
	// exactly as fosite's authorize endpoint does after the resource owner
	// approves the request. CreateAuthorizeCodeSession is not overridden by
	// CIMDStorageDecorator, so this delegates straight to RedisStorage.
	code := "cimd-auth-code-123"
	request := newRequestForClient("cimd-req-1", cimdClient)
	if err := decorated.CreateAuthorizeCodeSession(ctx, code, request); err != nil {
		return fmt.Errorf("unexpected failure creating authorize code session: %w", err)
	}
	fmt.Println("Step 2 OK: decorator.CreateAuthorizeCodeSession stored the session")

	// Step 3: read the session back through the decorator, exactly as the
	// /oauth/token handler does during authorization_code token exchange.
	_, err = decorated.GetAuthorizeCodeSession(ctx, code, nil)
	fmt.Println("Step 3 (token exchange read-back):")
	if err == nil {
		fmt.Println("  UNEXPECTED: no error — decorator.GetAuthorizeCodeSession succeeded")
		return errors.New("expected the CIMD variant to fail with a client-lookup error, but it succeeded")
	}
	fmt.Printf("  VERBATIM ERROR: %v\n", err)
	fmt.Println("  (fosite's token endpoint surfaces this as invalid_grant to the client)")
	return nil
}

// runRegularClientVariant is the contrast path: a regular (DCR-style) client
// IS persisted in the client store via RegisterClient, the same store method
// the DCR handler uses. The same create/read-back round trip through the
// decorator succeeds because unmarshalRequester's bare-RedisStorage GetClient
// call finds the persisted client row.
func runRegularClientVariant() error {
	fmt.Println("--- Variant 2 (contrast): regular DCR-registered client ---")

	decorated, base, cleanup, err := newDecoratedRedisStorage()
	if err != nil {
		return err
	}
	defer cleanup()

	ctx := context.Background()

	regularClient := &fosite.DefaultClient{
		ID:            "dcr-client-abc",
		RedirectURIs:  []string{"https://example.com/callback"},
		GrantTypes:    []string{"authorization_code", "refresh_token"},
		ResponseTypes: []string{"code"},
		Scopes:        []string{"openid", "profile"},
		Public:        true,
	}

	// Persist the client the way the DCR registration handler does: directly
	// on the underlying storage via RegisterClient (storage.ClientRegistry).
	if err := base.RegisterClient(ctx, regularClient); err != nil {
		return fmt.Errorf("unexpected failure registering regular client: %w", err)
	}
	fmt.Printf("Step 1 OK: base.RegisterClient persisted client, id=%s\n", regularClient.ID)

	resolved, err := decorated.GetClient(ctx, regularClient.ID)
	if err != nil {
		return fmt.Errorf("unexpected failure resolving regular client via decorator: %w", err)
	}
	fmt.Printf("Step 2 OK: decorator.GetClient resolved regular client, id=%s (delegated to base, opaque ID is not CIMD-shaped)\n", resolved.GetID())

	code := "regular-auth-code-456"
	request := newRequestForClient("regular-req-1", resolved)
	if err := decorated.CreateAuthorizeCodeSession(ctx, code, request); err != nil {
		return fmt.Errorf("unexpected failure creating authorize code session: %w", err)
	}
	fmt.Println("Step 3 OK: decorator.CreateAuthorizeCodeSession stored the session")

	retrieved, err := decorated.GetAuthorizeCodeSession(ctx, code, nil)
	fmt.Println("Step 4 (token exchange read-back):")
	if err != nil {
		fmt.Printf("  UNEXPECTED ERROR: %v\n", err)
		return fmt.Errorf("expected the regular-client variant to succeed, got: %w", err)
	}
	fmt.Printf("  OK: decorator.GetAuthorizeCodeSession succeeded, retrieved request id=%s, client id=%s\n",
		retrieved.GetID(), retrieved.GetClient().GetID())
	return nil
}

Real-world reproduction (how we actually hit it): run the embedded auth server with the Redis storage backend, add the server as a ChatGPT connector (registers via CIMD, user agent openai-mcp/1.0.0, MCP protocol 2025-11-25), complete the sign-in. Authorization succeeds and the browser is redirected back with a code; the connector's token exchange then fails, and retries fail identically.

Expected behavior

A CIMD client that just completed authorization can redeem its authorization code: session rehydration resolves the session's client through the same decorated storage the OAuth2 provider was configured with (or through some path that can see decorator-resolved CIMD clients), so both harness variants succeed.

Actual behavior

Variant 1 (CIMD client) fails at the token-exchange read-back; Variant 2 (persisted DCR-style client) succeeds through the identical stack, confirming the defect is specific to the CIMD × Redis combination and not a general storage bug. Verbatim harness output:

=== Repro: RedisStorage rehydration bypasses CIMDStorageDecorator ===

--- Variant 1: CIMD client (e.g. ChatGPT connector) ---
Step 1 OK: decorator.GetClient resolved CIMD client, id=http://127.0.0.1:61093/client.json
Step 2 OK: decorator.CreateAuthorizeCodeSession stored the session
Step 3 (token exchange read-back):
  VERBATIM ERROR: failed to unmarshal request: failed to get client for session: storage: item not found: not_found
  (fosite's token endpoint surfaces this as invalid_grant to the client)

--- Variant 2 (contrast): regular DCR-registered client ---
Step 1 OK: base.RegisterClient persisted client, id=dcr-client-abc
Step 2 OK: decorator.GetClient resolved regular client, id=dcr-client-abc (delegated to base, opaque ID is not CIMD-shaped)
Step 3 OK: decorator.CreateAuthorizeCodeSession stored the session
Step 4 (token exchange read-back):
  OK: decorator.GetAuthorizeCodeSession succeeded, retrieved request id=regular-req-1, client id=dcr-client-abc

A second run was identical except for the ephemeral loopback port (127.0.0.1:53045), exit code 0 both times.

In the live deployment the operator-visible symptom is a single log line per failed exchange, with no client id and no underlying cause:

failed to create access request error=invalid_grant

Environment (if relevant)

• ToolHive v0.41.0 (behavior verified at tag v0.41.0, commit d722304, and the cited code paths are unchanged on current main as of 2026-08-04).
• Live deployment context: Kubernetes operator with the proxyrunner image, embedded auth server with the Redis storage backend (multi-replica HA is the reason Redis is in use). The repro above is deployment-independent (miniredis, go run, no network beyond loopback).
• Client that hit this in production: the ChatGPT connector (CIMD registration, client_id = https://chatgpt.com/oauth/<id>/client.json).

Additional context

Why the existing test suite cannot catch this: MemoryStorage keeps the live fosite.Requester (client pointer included) and never re-resolves the client on read-back (pkg/authserver/storage/memory.go:435-451), so any test running the CIMD flow against memory storage passes. redis_test.go round-trips sessions for clients that were persisted with RegisterClient, which is exactly the case that works. The gap is a round-trip test of a decorator-resolved CIMD client through the decorated Redis storage — the harness above is essentially that test, and it fails today.

Proposed direction (either would fix it):

1. Resolve the session's client through the same decorated storage the provider was built with — for example, have unmarshalRequester receive the outer fosite storage (a fosite.ClientManager) instead of the bare *RedisStorage, so the CIMDStorageDecorator.GetClient interception applies on rehydration too.
2. Alternatively, have the decorator persist resolved CIMD clients into the wrapped storage (with a TTL matching its cache/fallback TTL) so the inner lookup finds them.

Diagnosability request, independent of the fix: log the wrapped underlying error (and the client id) at the token endpoint. Today pkg/authserver/server/handlers/token.go:30 logs only the sanitized fosite error, so the operator sees failed to create access request error=invalid_grant while the actionable cause (failed to get client for session: storage: item not found) is discarded — this made the failure a black-box hunt.

Related (checked, not duplicates):

Ref	Relation
#6082	Sibling architectural weakness in the same client-persistence/storage layer (no provisioning path for confidential clients for the RFC 8693 token-exchange grant, broken discovery metadata). It mentions Redis and CIMD only in passing and does not describe session rehydration bypassing the CIMD decorator's GetClient — distinct root cause, same family.
#3335	Historical/closed precursor about DCR (RFC 7591) clients not persisting across thv run restarts, predating both the Redis backend and the CIMD decorator. CIMD clients are resolved on the fly by design and were never meant to be persisted, so it is not the same defect — cited for lineage only.
#5343	Implementation history: introduced CIMDStorageDecorator.GetClient itself (merged); establishes where the decorator lives, but does not touch redis.go's unmarshalRequester client-lookup path.
#5348	Implementation history: wired the CIMD decorator into the embedded auth server config chain (merged); no interaction with Redis session rehydration.
#3639	Implementation history: the original Redis-backed storage.Storage implementation (merged), which predates CIMD entirely — the origin of unmarshalRequester, not a report of this bypass.

[alex-feel]

Sibling reports from the same investigation: #6186 (CIMD omitted-scope rejection, hit first), #6188 (revision classifier rejects the connector's tools/call shape, hit after this one was worked around).

[jerm-dro]

Thanks for the report, @alex-feel. The Redis-versus-memory contrast in your harness makes the root cause unambiguous — session rehydration re-resolving the client through the bare RedisStorage instead of the decorated storage.

We have read this and noted it. It is not something we are prioritizing right now, but we are leaving it open so it stays visible. Both proposed directions look reasonable, and the diagnosability request (logging the wrapped error and client id at the token endpoint) is well taken.

cc @jhrozek, who knows this area well and may have thoughts on the direction.

Thanks again for the thorough write-up.

[alex-feel]

#6186's fix is up as #6283; this one is next, so here's the direction I plan to take before opening the PR. Of the two directions proposed in the report, I'd go with persisting resolved CIMD clients rather than routing rehydration through the decorated storage:

• The hot path stays untouched. Once the decorator write-through-persists a successfully resolved client, unmarshalRequester's existing GetClient row lookup finds it — no new wiring between RedisStorage and the decorator, and no decorator reference threading through the four rehydration call sites.
• Sessions survive pod restarts and document rotation. The decorator's LRU is in-memory and per-instance, so under the re-resolve direction a restart (or a second replica) turns token exchanges into live re-fetches — and a document that changed since authorize time then invalidates sessions that were validly established (CIMD rejects clients that advertise a supported fallback auth method #6278 is a live example of a vendor rotating token_endpoint_auth_method mid-flight). A persisted snapshot binds the session to the client as validated when the session began, which is also the semantically right binding for an in-flight authorization.
• The TTL machinery already exists. Public-client registration rows already carry a TTL that token issuance renews best-effort, so persisted CIMD clients inherit sensible lifecycle behavior instead of needing a new expiry mechanism.

Concretely: on a successful CIMD document fetch, the decorator best-effort persists the resolved public client via the underlying storage's RegisterClient (write-through on each fresh resolution keeps the snapshot current; a persistence failure logs a WARN and does not fail the authorization). Plus the diagnosability part of the report: when rehydration's client lookup fails, unmarshalRequester wraps and WARN-logs the error with the stored client id, so invalid_grant at the token endpoint stops being silent.

Tests (miniredis, from the repro in the report): CIMD authorize → fresh decorator instance with an empty LRU (simulating a restart) → token exchange succeeds via the persisted row; the report's Redis-vs-memory contrast case; TTL renewal applying to the persisted row; and a rotation case — the document changes after authorize, the established session still completes its exchange.

@jhrozek if you'd rather see the re-resolve-through-the-decorator direction (or both), happy to switch — flagging before I write the code.