From 76865c55f21a295eb12513fd5ee27489d3b8d00d Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 16 May 2026 12:58:40 +0200 Subject: [PATCH 1/4] oauth/forward: refresh near-expired upstream id_token at /token (#121) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In forward mode the bearer the MCP client receives is the upstream id_token verbatim. Google's silent-SSO can return a cached id_token whose exp is set from the original mint time, sometimes leaving only minutes of remaining life. Pod log evidence from antalya-mcp 2026-05-15: 19:30:38 INF upstream code exchange succeeded expires_in=3598 19:38:39 ERR OAuth token expired exp=1778872374 i.e. an id_token with ~2 min of life forwarded as a 1h bearer. Result: ChatGPT / claude.ai reports "disconnected" minutes after connecting. Fix: * /authorize: when oauth.upstream_offline_access is true, add both Google's `access_type=offline` + `prompt=consent` and the RFC-strict `offline_access` scope. Either form unlocks a refresh_token from the upstream, depending on provider; the unrecognised form is ignored. * /token (forward mode + refresh_token returned + id_token remaining life < 55 min): immediately exchange the upstream refresh_token for a fresh id_token via grant_type=refresh_token, swap into the bearer before forwarding. Soft-fail on refresh errors — keep the original near-expired id_token rather than break /token entirely. * The upstream refresh_token is NOT exposed downstream. #115's no-downstream-refresh decision stands. This change only ensures the 1h window the AS metadata advertises is the window the client actually gets. Tests (oauth_forward_refresh_test.go): * TestOAuthAuthorize_OfflineAccessParams: with/without offline_access on cfg, /authorize sends access_type=offline+prompt=consent+offline_access scope (or none of them). * TestOAuthToken_RefreshesNearExpiredIDToken: 2-min id_token triggers one refresh_token call; expires_in in /token response > 50 min. * TestOAuthToken_SkipsRefreshWhenIDTokenFresh: 57-min id_token, zero refresh calls. * TestOAuthToken_RefreshFailureSoftFallsBack: upstream returns 500 on refresh; /token still 200, original id_token forwarded. * TestOAuthToken_NoRefreshWhenUpstreamReturnsNoRefreshToken: no upstream refresh_token → refresh path not attempted. Gating-mode deployments unaffected — the refresh branch is gated on oauthForwardMode(). Co-Authored-By: Claude Opus 4.7 --- .../oauth_forward_refresh_test.go | 403 ++++++++++++++++++ cmd/altinity-mcp/oauth_server.go | 108 ++++- 2 files changed, 509 insertions(+), 2 deletions(-) create mode 100644 cmd/altinity-mcp/oauth_forward_refresh_test.go diff --git a/cmd/altinity-mcp/oauth_forward_refresh_test.go b/cmd/altinity-mcp/oauth_forward_refresh_test.go new file mode 100644 index 0000000..e8d085f --- /dev/null +++ b/cmd/altinity-mcp/oauth_forward_refresh_test.go @@ -0,0 +1,403 @@ +package main + +import ( + "crypto/rand" + "crypto/rsa" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" + + altinitymcp "github.com/altinity/altinity-mcp/pkg/server" + "github.com/go-jose/go-jose/v4" + "github.com/stretchr/testify/require" + + "github.com/altinity/altinity-mcp/pkg/config" +) + +// Tests for issue #121: forward-mode id_token refresh. +// +// In forward mode the bearer the MCP client receives is the upstream +// id_token itself. Google's silent-SSO can return a cached id_token whose +// `exp` is set from the original mint time, leaving the MCP client with +// only minutes of session even though the access_token says 1h. The fix +// uses the upstream refresh_token at /token to mint a fresh id_token +// before forwarding. + +// --- /authorize: access_type=offline added when upstream_offline_access --- + +func TestOAuthAuthorize_OfflineAccessParams(t *testing.T) { + t.Parallel() + cases := []struct { + name string + offlineAccess bool + wantAccessTypeOff bool + }{ + {"offline_enabled", true, true}, + {"offline_disabled", false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, + Mode: "forward", + Issuer: "https://idp.example.com", + AuthURL: "https://idp.example.com/authorize", + TokenURL: "https://idp.example.com/token", + ClientID: "broker", + ClientSecret: "s", + PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-offline-32bytes!!!!!!", + UpstreamOfflineAccess: tc.offlineAccess, + Scopes: []string{"openid", "email"}, + }}} + cimdURL := "https://demo.example.com/cimd.json" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":["https://demo.example.com/cb"],"token_endpoint_auth_method":"none"}`, cimdURL) + })) + defer cimdServer.Close() + + app := &application{ + config: cfg, + mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), + cimdResolver: testResolver(t, cimdServer), + } + verifier, _ := newPKCEVerifier() + form := url.Values{} + form.Set("client_id", cimdURL) + form.Set("redirect_uri", "https://demo.example.com/cb") + form.Set("response_type", "code") + form.Set("code_challenge", pkceChallenge(verifier)) + form.Set("code_challenge_method", "S256") + req := httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/authorize?"+form.Encode(), nil) + rr := httptest.NewRecorder() + app.handleOAuthAuthorize(rr, req) + require.Equal(t, http.StatusFound, rr.Code, "body=%s", rr.Body.String()) + loc, err := url.Parse(rr.Header().Get("Location")) + require.NoError(t, err) + q := loc.Query() + if tc.wantAccessTypeOff { + require.Equal(t, "offline", q.Get("access_type"), "missing access_type=offline on upstream /authorize") + require.Equal(t, "consent", q.Get("prompt"), "missing prompt=consent on first-time offline auth") + require.Contains(t, q.Get("scope"), "offline_access", "Auth0-form offline_access scope also expected for cross-provider safety") + } else { + require.Empty(t, q.Get("access_type"), "access_type leaked when offline_access disabled") + require.Empty(t, q.Get("prompt"), "prompt=consent leaked when offline_access disabled") + require.NotContains(t, q.Get("scope"), "offline_access") + } + }) + } +} + +// --- /token: near-expired id_token triggers internal refresh --------------- + +type refreshProbeUpstream struct { + server *httptest.Server + priv *rsa.PrivateKey + keyID string + codeExchangeCt int32 // grant_type=authorization_code calls + refreshCt int32 // grant_type=refresh_token calls +} + +func newRefreshProbeUpstream(t *testing.T, initialIDTokenExp, refreshedIDTokenExp time.Time, subject string) *refreshProbeUpstream { + t.Helper() + priv, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + u := &refreshProbeUpstream{priv: priv, keyID: "refresh-probe-key"} + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + u.server = srv + + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{ + Key: &priv.PublicKey, KeyID: u.keyID, Use: "sig", Algorithm: string(jose.RS256), + }}}) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, r.ParseForm()) + now := time.Now().Unix() + w.Header().Set("Content-Type", "application/json") + switch r.Form.Get("grant_type") { + case "authorization_code": + atomic.AddInt32(&u.codeExchangeCt, 1) + idToken := u.issueIDToken(t, map[string]interface{}{ + "sub": subject, + "aud": "broker", + "iat": now, + "exp": initialIDTokenExp.Unix(), + "iss": srv.URL, + "email": subject, + "email_verified": true, + }) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id_token": idToken, + "access_token": "ax-1", + "refresh_token": "rf-1", // crucial: this enables the #121 refresh path + "token_type": "Bearer", + "expires_in": 3600, + "scope": "openid email", + }) + case "refresh_token": + atomic.AddInt32(&u.refreshCt, 1) + require.Equal(t, "rf-1", r.Form.Get("refresh_token")) + idToken := u.issueIDToken(t, map[string]interface{}{ + "sub": subject, + "aud": "broker", + "iat": now, + "exp": refreshedIDTokenExp.Unix(), + "iss": srv.URL, + "email": subject, + "email_verified": true, + }) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id_token": idToken, + "access_token": "ax-2", + "token_type": "Bearer", + "expires_in": 3600, + "scope": "openid email", + }) + default: + http.Error(w, "unsupported grant_type", http.StatusBadRequest) + } + }) + return u +} + +func (u *refreshProbeUpstream) issueIDToken(t *testing.T, claims map[string]interface{}) string { + t.Helper() + signer, err := jose.NewSigner(jose.SigningKey{ + Algorithm: jose.RS256, + Key: jose.JSONWebKey{ + Key: u.priv, + KeyID: u.keyID, + Use: "sig", + Algorithm: string(jose.RS256), + }, + }, (&jose.SignerOptions{}).WithType("JWT")) + require.NoError(t, err) + body, err := json.Marshal(claims) + require.NoError(t, err) + obj, err := signer.Sign(body) + require.NoError(t, err) + tok, err := obj.CompactSerialize() + require.NoError(t, err) + return tok +} + +func runTokenExchange(t *testing.T, app *application, cimdURL, redirectURI string) *httptest.ResponseRecorder { + t.Helper() + verifier, _ := newPKCEVerifier() + issued := oauthIssuedCode{ + ClientID: cimdURL, RedirectURI: redirectURI, Scope: "openid email", + CodeChallenge: pkceChallenge(verifier), CodeChallengeMethod: "S256", + UpstreamAuthCode: "uac", UpstreamPKCEVerifier: "uv", + ExpiresAt: time.Now().Add(60 * time.Second), + } + code, err := app.encodeAuthCode(issued) + require.NoError(t, err) + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("client_id", cimdURL) + form.Set("redirect_uri", redirectURI) + form.Set("code", code) + form.Set("code_verifier", verifier) + req := httptest.NewRequest(http.MethodPost, "https://mcp.example.com/oauth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + require.NoError(t, req.ParseForm()) + rr := httptest.NewRecorder() + app.handleOAuthTokenAuthCode(rr, req) + return rr +} + +func buildForwardModeApp(t *testing.T, upstream *refreshProbeUpstream, cimdURL, redirectURI string) *application { + t.Helper() + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":[%q],"token_endpoint_auth_method":"none"}`, cimdURL, redirectURI) + })) + t.Cleanup(cimdServer.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, + Mode: "forward", + Issuer: upstream.server.URL, + JWKSURL: upstream.server.URL + "/jwks", + AuthURL: upstream.server.URL + "/authorize", + TokenURL: upstream.server.URL + "/token", + ClientID: "broker", + ClientSecret: "s", + PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-refresh-32bytes!!!!!!", + UpstreamOfflineAccess: true, + }}} + return &application{ + config: cfg, + mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), + cimdResolver: testResolver(t, cimdServer), + } +} + +func TestOAuthToken_RefreshesNearExpiredIDToken(t *testing.T) { + t.Parallel() + // id_token exp 2 minutes from now — well below 55-min threshold. + nearExp := time.Now().Add(2 * time.Minute) + freshExp := time.Now().Add(60 * time.Minute) + upstream := newRefreshProbeUpstream(t, nearExp, freshExp, "alice@example.com") + app := buildForwardModeApp(t, upstream, "https://demo.example.com/cimd.json", "https://demo.example.com/cb") + + rr := runTokenExchange(t, app, "https://demo.example.com/cimd.json", "https://demo.example.com/cb") + require.Equal(t, http.StatusOK, rr.Code, "body=%s", rr.Body.String()) + + require.Equal(t, int32(1), atomic.LoadInt32(&upstream.codeExchangeCt)) + require.Equal(t, int32(1), atomic.LoadInt32(&upstream.refreshCt), "expected internal refresh_token call") + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + // expires_in should now reflect the refreshed id_token, not the near-exp original. + expiresIn, ok := body["expires_in"].(float64) + require.True(t, ok) + require.Greater(t, int64(expiresIn), int64(50*60), "expires_in must reflect refreshed id_token, got %v", expiresIn) +} + +func TestOAuthToken_SkipsRefreshWhenIDTokenFresh(t *testing.T) { + t.Parallel() + // id_token exp ~57 min — above 55-min threshold, no refresh. + nearExp := time.Now().Add(57 * time.Minute) + freshExp := time.Now().Add(60 * time.Minute) + upstream := newRefreshProbeUpstream(t, nearExp, freshExp, "bob@example.com") + app := buildForwardModeApp(t, upstream, "https://demo.example.com/cimd.json", "https://demo.example.com/cb") + + rr := runTokenExchange(t, app, "https://demo.example.com/cimd.json", "https://demo.example.com/cb") + require.Equal(t, http.StatusOK, rr.Code, "body=%s", rr.Body.String()) + + require.Equal(t, int32(1), atomic.LoadInt32(&upstream.codeExchangeCt)) + require.Equal(t, int32(0), atomic.LoadInt32(&upstream.refreshCt), "refresh_token must NOT be called when id_token is fresh") +} + +func TestOAuthToken_RefreshFailureSoftFallsBack(t *testing.T) { + t.Parallel() + // id_token exp near; upstream /token refresh_token responds with HTTP 500. + nearExp := time.Now().Add(3 * time.Minute) + priv, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + keyID := "soft-fallback-key" + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{ + Key: &priv.PublicKey, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256), + }}}) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, r.ParseForm()) + w.Header().Set("Content-Type", "application/json") + if r.Form.Get("grant_type") == "refresh_token" { + http.Error(w, "transient upstream outage", http.StatusInternalServerError) + return + } + // initial code exchange returns a near-expired id_token + refresh_token. + signer, _ := jose.NewSigner(jose.SigningKey{Algorithm: jose.RS256, Key: jose.JSONWebKey{Key: priv, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256)}}, (&jose.SignerOptions{}).WithType("JWT")) + payload, _ := json.Marshal(map[string]interface{}{ + "sub": "carol@example.com", "aud": "broker", "iat": time.Now().Unix(), + "exp": nearExp.Unix(), "iss": srv.URL, "email": "carol@example.com", "email_verified": true, + }) + obj, _ := signer.Sign(payload) + idTok, _ := obj.CompactSerialize() + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id_token": idTok, "access_token": "a", "refresh_token": "r", + "token_type": "Bearer", "expires_in": 3600, + }) + }) + cimdURL, redirectURI := "https://demo.example.com/cimd.json", "https://demo.example.com/cb" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":[%q],"token_endpoint_auth_method":"none"}`, cimdURL, redirectURI) + })) + t.Cleanup(cimdServer.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, Mode: "forward", Issuer: srv.URL, JWKSURL: srv.URL + "/jwks", + AuthURL: srv.URL + "/authorize", TokenURL: srv.URL + "/token", + ClientID: "broker", ClientSecret: "s", PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-softfail-32bytes!!!!!", UpstreamOfflineAccess: true, + }}} + app := &application{config: cfg, mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), cimdResolver: testResolver(t, cimdServer)} + + rr := runTokenExchange(t, app, cimdURL, redirectURI) + // MUST still return 200 — soft fallback to the original near-expired id_token. + require.Equal(t, http.StatusOK, rr.Code, "soft-fail must not break /token; body=%s", rr.Body.String()) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.NotEmpty(t, body["access_token"], "must still hand back the original id_token as bearer") +} + +func TestOAuthToken_NoRefreshWhenUpstreamReturnsNoRefreshToken(t *testing.T) { + t.Parallel() + // upstream returns near-expired id_token but no refresh_token. We must + // NOT attempt refresh (would 400 or call with empty token), just forward. + nearExp := time.Now().Add(3 * time.Minute) + priv, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + keyID := "no-rt-key" + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{ + Key: &priv.PublicKey, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256), + }}}) + }) + var refreshCalls int32 + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, r.ParseForm()) + w.Header().Set("Content-Type", "application/json") + if r.Form.Get("grant_type") == "refresh_token" { + atomic.AddInt32(&refreshCalls, 1) + http.Error(w, "should not be called", http.StatusBadRequest) + return + } + signer, _ := jose.NewSigner(jose.SigningKey{Algorithm: jose.RS256, Key: jose.JSONWebKey{Key: priv, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256)}}, (&jose.SignerOptions{}).WithType("JWT")) + payload, _ := json.Marshal(map[string]interface{}{ + "sub": "dave@example.com", "aud": "broker", "iat": time.Now().Unix(), + "exp": nearExp.Unix(), "iss": srv.URL, "email": "dave@example.com", "email_verified": true, + }) + obj, _ := signer.Sign(payload) + idTok, _ := obj.CompactSerialize() + // NO refresh_token in this response. + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id_token": idTok, "access_token": "a", + "token_type": "Bearer", "expires_in": 3600, + }) + }) + cimdURL, redirectURI := "https://demo.example.com/cimd.json", "https://demo.example.com/cb" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":[%q],"token_endpoint_auth_method":"none"}`, cimdURL, redirectURI) + })) + t.Cleanup(cimdServer.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, Mode: "forward", Issuer: srv.URL, JWKSURL: srv.URL + "/jwks", + AuthURL: srv.URL + "/authorize", TokenURL: srv.URL + "/token", + ClientID: "broker", ClientSecret: "s", PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-no-rt-32bytes!!!!!!!!", UpstreamOfflineAccess: true, + }}} + app := &application{config: cfg, mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), cimdResolver: testResolver(t, cimdServer)} + + rr := runTokenExchange(t, app, cimdURL, redirectURI) + require.Equal(t, http.StatusOK, rr.Code, "body=%s", rr.Body.String()) + require.Equal(t, int32(0), atomic.LoadInt32(&refreshCalls), "refresh attempted despite no upstream refresh_token") +} + +// keep "io" used for some test paths that may grow later +var _ = io.Discard diff --git a/cmd/altinity-mcp/oauth_server.go b/cmd/altinity-mcp/oauth_server.go index cb9a0ce..541eda8 100644 --- a/cmd/altinity-mcp/oauth_server.go +++ b/cmd/altinity-mcp/oauth_server.go @@ -51,6 +51,14 @@ const ( // codes "should be redeemed within seconds, never minutes." defaultAuthCodeTTLSeconds = 60 defaultAccessTokenTTLSeconds = 60 * 60 + // forwardModeIDTokenRefreshThresholdSeconds is the remaining-life floor + // below which we'll use the upstream refresh_token at /token to mint a + // fresh id_token before forwarding (#121). Set at 55 minutes so a + // freshly-minted Google id_token (exp = iat + 1h) is never re-fetched + // but anything Google reused from a warm session is. Only fires when + // upstream_offline_access is enabled AND the upstream actually returned + // a refresh_token. + forwardModeIDTokenRefreshThresholdSeconds = 55 * 60 ) // statelessRegisteredClient is the in-memory shape parseCIMDMetadata returns. @@ -928,6 +936,65 @@ func (a *application) resolveUpstreamTokenURL() (string, error) { return strings.TrimSpace(discovery.TokenEndpoint), nil } +// refreshUpstreamIDToken exchanges the upstream refresh_token for a fresh +// id_token via the upstream's RFC 6749 §6 refresh-token grant. Used in +// forward mode at /token when the just-redeemed id_token has a short +// remaining life (Google's silent-SSO can return a cached id_token whose +// `exp` is near). The returned refresh_token (if any) is discarded — #115 +// keeps downstream refresh out of scope. +// +// Returns the fresh id_token plus its parsed identity claims. On any +// failure the caller should fall back to the original (near-expired) +// id_token rather than fail the whole /token call. +func (a *application) refreshUpstreamIDToken(refreshToken string) (string, *altinitymcp.OAuthClaims, error) { + cfg := a.GetCurrentConfig() + tokenURL, err := a.resolveUpstreamTokenURL() + if err != nil { + return "", nil, fmt.Errorf("resolve token url: %w", err) + } + form := url.Values{} + form.Set("grant_type", "refresh_token") + form.Set("refresh_token", refreshToken) + form.Set("client_id", cfg.Server.OAuth.ClientID) + if cfg.Server.OAuth.ClientSecret != "" { + form.Set("client_secret", cfg.Server.OAuth.ClientSecret) + } + resp, err := (&http.Client{Timeout: oauthUpstreamHTTPTimeout}).PostForm(tokenURL, form) + if err != nil { + return "", nil, fmt.Errorf("post: %w", err) + } + defer func() { _ = resp.Body.Close() }() + body, err := io.ReadAll(io.LimitReader(resp.Body, maxOAuthResponseBytes)) + if err != nil { + return "", nil, fmt.Errorf("body: %w", err) + } + if resp.StatusCode >= 300 { + errCode, _ := safeUpstreamErrorFields(body) + return "", nil, fmt.Errorf("upstream %d: %s", resp.StatusCode, errCode) + } + var parsed struct { + IDToken string `json:"id_token"` + Error string `json:"error"` + } + if err := json.Unmarshal(body, &parsed); err != nil { + return "", nil, fmt.Errorf("decode: %w", err) + } + if parsed.Error != "" { + return "", nil, fmt.Errorf("upstream %s", parsed.Error) + } + if parsed.IDToken == "" { + // Some IdPs only return a new access_token on refresh, no id_token. + // We need id_token specifically (it's what we forward as the bearer + // in forward mode), so treat absence as failure and fall back. + return "", nil, fmt.Errorf("upstream returned no id_token") + } + claims, err := a.mcpServer.ValidateUpstreamIdentityToken(parsed.IDToken, cfg.Server.OAuth.ClientID) + if err != nil { + return "", nil, fmt.Errorf("validate: %w", err) + } + return parsed.IDToken, claims, nil +} + func (a *application) handleOAuthProtectedResource(w http.ResponseWriter, r *http.Request) { if !a.oauthEnabled() { http.NotFound(w, r) @@ -1127,8 +1194,19 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques if scope == "" { scope = "openid email" } - if a.oauthBrokerMode() && cfg.Server.OAuth.UpstreamOfflineAccess && !slices.Contains(strings.Fields(scope), "offline_access") { - scope = strings.TrimSpace(scope + " offline_access") + if a.oauthBrokerMode() && cfg.Server.OAuth.UpstreamOfflineAccess { + // Two refresh-token unlock mechanisms, one per provider family: + // - Auth0 + RFC-6749-strict providers: `offline_access` scope. + // - Google: NOT scope-gated; uses `access_type=offline` as a separate + // auth param (with `prompt=consent` on first authorization so the + // user grants offline access — Google silently refuses to mint a + // refresh_token if neither was set). We send both forms; the one + // the upstream doesn't recognise is silently ignored. + if !slices.Contains(strings.Fields(scope), "offline_access") { + scope = strings.TrimSpace(scope + " offline_access") + } + upstream.Set("access_type", "offline") + upstream.Set("prompt", "consent") } upstream.Set("scope", scope) upstream.Set("state", callbackState) @@ -1458,6 +1536,32 @@ func (a *application) handleOAuthTokenAuthCode(w http.ResponseWriter, r *http.Re return } } + // #121: in forward mode the bearer we return is the upstream id_token + // itself. Google's silent-SSO can return a cached id_token whose `exp` + // is set from the original mint time, not now — sometimes leaving only + // minutes of remaining life. If we have a refresh_token and the id_token + // is below the freshness threshold, exchange it for a fresh id_token. + // Soft-fail: if refresh fails, keep the original id_token rather than + // fail the whole /token call. The refresh_token itself is discarded — + // downstream refresh stays out of scope per #115. + if a.oauthForwardMode() && tokenResp.RefreshToken != "" && identityClaims != nil && identityClaims.ExpiresAt > 0 { + remaining := identityClaims.ExpiresAt - time.Now().Unix() + if remaining < int64(forwardModeIDTokenRefreshThresholdSeconds) { + freshIDToken, freshClaims, refreshErr := a.refreshUpstreamIDToken(tokenResp.RefreshToken) + if refreshErr != nil { + log.Warn().Err(refreshErr). + Int64("remaining_seconds", remaining). + Msg("OAuth /token: id_token refresh failed; forwarding original near-expired token") + } else { + tokenResp.IDToken = freshIDToken + identityClaims = freshClaims + log.Info(). + Int64("old_remaining_seconds", remaining). + Int64("new_remaining_seconds", freshClaims.ExpiresAt-time.Now().Unix()). + Msg("OAuth /token: refreshed near-expired id_token via upstream refresh_token grant") + } + } + } if tokenResp.Scope == "" { tokenResp.Scope = issued.Scope } From e0074e2170213ac87f5285381ed17661a8d0b3c4 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 16 May 2026 13:08:24 +0200 Subject: [PATCH 2/4] oauth/forward: provider-detect offline_access mechanism Sending offline_access scope to Google produces a HARD invalid_scope error at /authorize (Google does not silently ignore unknown scopes). Previous commit assumed it was a no-op; it isn't. Provider-detect by issuer: * Google issuer -> access_type=offline + prompt=consent, NO offline_access scope. * Anything else (Auth0, etc.) -> offline_access scope, no access_type. E2E failure that surfaced this: "Access blocked: Some requested scopes were invalid. invalid=[offline_access]" on first /authorize against antalya-mcp after #122 was deployed. Test updated to cover both providers x both offline_access settings. Co-Authored-By: Claude Opus 4.7 --- .../oauth_forward_refresh_test.go | 38 ++++++++++++------- cmd/altinity-mcp/oauth_server.go | 32 +++++++++++----- 2 files changed, 47 insertions(+), 23 deletions(-) diff --git a/cmd/altinity-mcp/oauth_forward_refresh_test.go b/cmd/altinity-mcp/oauth_forward_refresh_test.go index e8d085f..c05a7a3 100644 --- a/cmd/altinity-mcp/oauth_forward_refresh_test.go +++ b/cmd/altinity-mcp/oauth_forward_refresh_test.go @@ -34,13 +34,21 @@ import ( func TestOAuthAuthorize_OfflineAccessParams(t *testing.T) { t.Parallel() + // Provider-detect: Google MUST get access_type=offline+prompt=consent and + // MUST NOT receive offline_access scope (rejected as invalid_scope by + // Google's /authorize). Non-Google providers (Auth0 etc.) get the + // reverse — offline_access scope, no access_type. cases := []struct { - name string - offlineAccess bool - wantAccessTypeOff bool + name string + issuer string + offlineAccess bool + wantAccessType bool + wantOfflineScope bool }{ - {"offline_enabled", true, true}, - {"offline_disabled", false, false}, + {"google_enabled", "https://accounts.google.com", true, true, false}, + {"google_disabled", "https://accounts.google.com", false, false, false}, + {"auth0_enabled", "https://acme.auth0.com/", true, false, true}, + {"auth0_disabled", "https://acme.auth0.com/", false, false, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -48,7 +56,7 @@ func TestOAuthAuthorize_OfflineAccessParams(t *testing.T) { cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ Enabled: true, Mode: "forward", - Issuer: "https://idp.example.com", + Issuer: tc.issuer, AuthURL: "https://idp.example.com/authorize", TokenURL: "https://idp.example.com/token", ClientID: "broker", @@ -84,14 +92,18 @@ func TestOAuthAuthorize_OfflineAccessParams(t *testing.T) { loc, err := url.Parse(rr.Header().Get("Location")) require.NoError(t, err) q := loc.Query() - if tc.wantAccessTypeOff { - require.Equal(t, "offline", q.Get("access_type"), "missing access_type=offline on upstream /authorize") - require.Equal(t, "consent", q.Get("prompt"), "missing prompt=consent on first-time offline auth") - require.Contains(t, q.Get("scope"), "offline_access", "Auth0-form offline_access scope also expected for cross-provider safety") + if tc.wantAccessType { + require.Equal(t, "offline", q.Get("access_type")) + require.Equal(t, "consent", q.Get("prompt")) } else { - require.Empty(t, q.Get("access_type"), "access_type leaked when offline_access disabled") - require.Empty(t, q.Get("prompt"), "prompt=consent leaked when offline_access disabled") - require.NotContains(t, q.Get("scope"), "offline_access") + require.Empty(t, q.Get("access_type")) + require.Empty(t, q.Get("prompt")) + } + if tc.wantOfflineScope { + require.Contains(t, q.Get("scope"), "offline_access") + } else { + require.NotContains(t, q.Get("scope"), "offline_access", + "offline_access scope MUST NOT be sent to Google (rejected as invalid_scope)") } }) } diff --git a/cmd/altinity-mcp/oauth_server.go b/cmd/altinity-mcp/oauth_server.go index 541eda8..8af1ba0 100644 --- a/cmd/altinity-mcp/oauth_server.go +++ b/cmd/altinity-mcp/oauth_server.go @@ -921,6 +921,18 @@ func (a *application) resolveUpstreamAuthURL() (string, error) { return strings.TrimSpace(discovery.AuthorizationEndpoint), nil } +// isGoogleIssuer reports whether the configured issuer is Google's OIDC +// provider. Used to pick between `access_type=offline` (Google) and the +// `offline_access` scope (Auth0 and other RFC 6749 §6 strict providers). +// Both Google issuer URLs the OIDC spec lists are accepted. +func isGoogleIssuer(issuer string) bool { + host := strings.ToLower(strings.TrimSpace(issuer)) + host = strings.TrimPrefix(host, "https://") + host = strings.TrimPrefix(host, "http://") + host, _, _ = strings.Cut(host, "/") + return host == "accounts.google.com" || host == "www.google.com" +} + func (a *application) resolveUpstreamTokenURL() (string, error) { cfg := a.GetCurrentConfig().Server.OAuth if tokenURL := strings.TrimSpace(cfg.TokenURL); tokenURL != "" { @@ -1195,18 +1207,18 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques scope = "openid email" } if a.oauthBrokerMode() && cfg.Server.OAuth.UpstreamOfflineAccess { - // Two refresh-token unlock mechanisms, one per provider family: - // - Auth0 + RFC-6749-strict providers: `offline_access` scope. - // - Google: NOT scope-gated; uses `access_type=offline` as a separate - // auth param (with `prompt=consent` on first authorization so the - // user grants offline access — Google silently refuses to mint a - // refresh_token if neither was set). We send both forms; the one - // the upstream doesn't recognise is silently ignored. - if !slices.Contains(strings.Fields(scope), "offline_access") { + // Two refresh-token unlock mechanisms; mutually exclusive per + // provider — sending the wrong one is a HARD error for Google + // (`invalid_scope` if `offline_access` is requested) and a no-op + // for Auth0 (`access_type` is ignored). Provider-detect by issuer: + // - Google → access_type=offline + prompt=consent + // - Everything else (Auth0/etc.) → offline_access scope + if isGoogleIssuer(cfg.Server.OAuth.Issuer) { + upstream.Set("access_type", "offline") + upstream.Set("prompt", "consent") + } else if !slices.Contains(strings.Fields(scope), "offline_access") { scope = strings.TrimSpace(scope + " offline_access") } - upstream.Set("access_type", "offline") - upstream.Set("prompt", "consent") } upstream.Set("scope", scope) upstream.Set("state", callbackState) From aaf90528d6b6ea1f228fd56eae6e0455437a6932 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 16 May 2026 13:27:38 +0200 Subject: [PATCH 3/4] oauth/broker: broaden id_token refresh gate from forward to broker mode The bearer returned to MCP clients is the upstream id_token under ALL broker-mode deployments (`bearerToken := tokenResp.IDToken` runs unconditionally), not just forward mode. Initial #121 patch gated the refresh on `oauthForwardMode()` and silently skipped gating+broker_upstream deployments (github-mcp, otel-google-gating-mcp) which suffer the exact same Google-cached-id_token disconnect. Reproduced today on github-mcp: same overnight-disconnect symptom as antalya. Pulling `oauthForwardMode()` -> `oauthBrokerMode()` makes the refresh fire on every broker deployment. Constant renamed `forwardModeIDTokenRefreshThresholdSeconds` -> `brokerModeIDTokenRefreshThresholdSeconds` to match the new scope. Added test `TestOAuthToken_RefreshesNearExpiredIDToken_GatingBrokerUpstream` guarding the broader gate; existing forward-mode tests stay green. Co-Authored-By: Claude Opus 4.7 --- .../oauth_forward_refresh_test.go | 42 +++++++++++++++++++ cmd/altinity-mcp/oauth_server.go | 33 +++++++++------ 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/cmd/altinity-mcp/oauth_forward_refresh_test.go b/cmd/altinity-mcp/oauth_forward_refresh_test.go index c05a7a3..e119be4 100644 --- a/cmd/altinity-mcp/oauth_forward_refresh_test.go +++ b/cmd/altinity-mcp/oauth_forward_refresh_test.go @@ -353,6 +353,48 @@ func TestOAuthToken_RefreshFailureSoftFallsBack(t *testing.T) { require.NotEmpty(t, body["access_token"], "must still hand back the original id_token as bearer") } +// Gating-with-broker_upstream deployments (github-mcp, otel-google-gating-mcp) +// also return the upstream id_token as the bearer, so the same refresh logic +// must apply. Guards against regression to the original forward-mode-only +// gate which left gating+broker silently broken. +func TestOAuthToken_RefreshesNearExpiredIDToken_GatingBrokerUpstream(t *testing.T) { + t.Parallel() + nearExp := time.Now().Add(2 * time.Minute) + freshExp := time.Now().Add(60 * time.Minute) + upstream := newRefreshProbeUpstream(t, nearExp, freshExp, "alice@example.com") + cimdURL, redirectURI := "https://demo.example.com/cimd.json", "https://demo.example.com/cb" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":[%q],"token_endpoint_auth_method":"none"}`, cimdURL, redirectURI) + })) + t.Cleanup(cimdServer.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, + Mode: "gating", + BrokerUpstream: true, + Issuer: upstream.server.URL, + JWKSURL: upstream.server.URL + "/jwks", + AuthURL: upstream.server.URL + "/authorize", + TokenURL: upstream.server.URL + "/token", + ClientID: "broker", + ClientSecret: "s", + Audience: "broker", // matches client_id under broker_upstream + PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-refresh-gating-32b!!!!", + UpstreamOfflineAccess: true, + }}} + app := &application{ + config: cfg, + mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), + cimdResolver: testResolver(t, cimdServer), + } + rr := runTokenExchange(t, app, cimdURL, redirectURI) + require.Equal(t, http.StatusOK, rr.Code, "body=%s", rr.Body.String()) + require.Equal(t, int32(1), atomic.LoadInt32(&upstream.codeExchangeCt)) + require.Equal(t, int32(1), atomic.LoadInt32(&upstream.refreshCt), + "gating+broker_upstream MUST trigger refresh on near-expired id_token (#121)") +} + func TestOAuthToken_NoRefreshWhenUpstreamReturnsNoRefreshToken(t *testing.T) { t.Parallel() // upstream returns near-expired id_token but no refresh_token. We must diff --git a/cmd/altinity-mcp/oauth_server.go b/cmd/altinity-mcp/oauth_server.go index 8af1ba0..766e6a6 100644 --- a/cmd/altinity-mcp/oauth_server.go +++ b/cmd/altinity-mcp/oauth_server.go @@ -51,14 +51,16 @@ const ( // codes "should be redeemed within seconds, never minutes." defaultAuthCodeTTLSeconds = 60 defaultAccessTokenTTLSeconds = 60 * 60 - // forwardModeIDTokenRefreshThresholdSeconds is the remaining-life floor + // brokerModeIDTokenRefreshThresholdSeconds is the remaining-life floor // below which we'll use the upstream refresh_token at /token to mint a // fresh id_token before forwarding (#121). Set at 55 minutes so a // freshly-minted Google id_token (exp = iat + 1h) is never re-fetched // but anything Google reused from a warm session is. Only fires when // upstream_offline_access is enabled AND the upstream actually returned - // a refresh_token. - forwardModeIDTokenRefreshThresholdSeconds = 55 * 60 + // a refresh_token. Applies to all broker-mode deployments (forward and + // gating+broker_upstream alike) since the bearer is the id_token in + // both. + brokerModeIDTokenRefreshThresholdSeconds = 55 * 60 ) // statelessRegisteredClient is the in-memory shape parseCIMDMetadata returns. @@ -1548,17 +1550,22 @@ func (a *application) handleOAuthTokenAuthCode(w http.ResponseWriter, r *http.Re return } } - // #121: in forward mode the bearer we return is the upstream id_token - // itself. Google's silent-SSO can return a cached id_token whose `exp` - // is set from the original mint time, not now — sometimes leaving only - // minutes of remaining life. If we have a refresh_token and the id_token - // is below the freshness threshold, exchange it for a fresh id_token. - // Soft-fail: if refresh fails, keep the original id_token rather than - // fail the whole /token call. The refresh_token itself is discarded — - // downstream refresh stays out of scope per #115. - if a.oauthForwardMode() && tokenResp.RefreshToken != "" && identityClaims != nil && identityClaims.ExpiresAt > 0 { + // #121: whenever we're in broker mode the bearer we return is the + // upstream id_token itself (`bearerToken := tokenResp.IDToken` above). + // Google's silent-SSO can return a cached id_token whose `exp` is set + // from the original mint time, not now — sometimes leaving only minutes + // of remaining life. Forward mode forwards that bearer to ClickHouse; + // gating-with-broker_upstream returns it as the session bearer the MCP + // client uses for /mcp. Both modes are affected — gate on broker mode, + // not forward mode. + // + // If we have a refresh_token and the id_token is below the freshness + // threshold, exchange it for a fresh id_token. Soft-fail: keep the + // original on error. The refresh_token itself is discarded — downstream + // refresh stays out of scope per #115. + if a.oauthBrokerMode() && tokenResp.RefreshToken != "" && identityClaims != nil && identityClaims.ExpiresAt > 0 { remaining := identityClaims.ExpiresAt - time.Now().Unix() - if remaining < int64(forwardModeIDTokenRefreshThresholdSeconds) { + if remaining < int64(brokerModeIDTokenRefreshThresholdSeconds) { freshIDToken, freshClaims, refreshErr := a.refreshUpstreamIDToken(tokenResp.RefreshToken) if refreshErr != nil { log.Warn().Err(refreshErr). From 78c59407654a5144d791b499aef3ed6b38172882 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 16 May 2026 13:41:58 +0200 Subject: [PATCH 4/4] oauth/forward: PR #122 review fixes (1-5) 1. Drop default prompt=consent on Google /authorize. Google mints the refresh_token on first interactive consent and honors it on silent SSO thereafter; forcing consent every authorize shows the "access your account when not using" screen on every login and is UX-hostile. New config field oauth.upstream_force_consent (default false) restores the previous behaviour for operators who need to force re-enrollment (rotated upstream client, revoked grant). 2. Drop dead `var _ = io.Discard` placeholder from oauth_forward_refresh_test.go. 3. Surface error_description in refreshUpstreamIDToken's error wrapping. Google's refresh-token failures are diagnostically richer in error_description ("Token has been expired or revoked") than the bare error enum ("invalid_grant"). New refreshErrorFields helper + sanitizeErrorDesc (strips control chars, caps at 120 bytes) extract the description without leaking arbitrary IdP-side bytes. 4. Two new tests: - TestOAuthToken_RefreshFallsBackWhenUpstreamReturnsNoIDToken: refresh response with access_token only (no id_token) must soft-fail and forward the original near-expired id_token. - TestOAuthToken_RefreshErrorDescriptionSurfacedInLog: invalid_grant + "Token has been expired or revoked" round-trips through the wrapped error untouched. - TestOAuthAuthorize_ForceConsentFlag: prompt=consent only set when UpstreamForceConsent=true; default omitted. 5. billing-mcp values already carry upstream_offline_access: true (Auth0 issuer uses the offline_access scope path; no change to its pre-existing config needed). No file edit; verified in review. Existing TestOAuthAuthorize_OfflineAccessParams.google_enabled updated to assert prompt=consent is now absent by default. Co-Authored-By: Claude Opus 4.7 --- .../oauth_forward_refresh_test.go | 156 +++++++++++++++++- cmd/altinity-mcp/oauth_server.go | 67 +++++++- pkg/config/config.go | 10 ++ 3 files changed, 223 insertions(+), 10 deletions(-) diff --git a/cmd/altinity-mcp/oauth_forward_refresh_test.go b/cmd/altinity-mcp/oauth_forward_refresh_test.go index e119be4..c5c42ed 100644 --- a/cmd/altinity-mcp/oauth_forward_refresh_test.go +++ b/cmd/altinity-mcp/oauth_forward_refresh_test.go @@ -94,7 +94,11 @@ func TestOAuthAuthorize_OfflineAccessParams(t *testing.T) { q := loc.Query() if tc.wantAccessType { require.Equal(t, "offline", q.Get("access_type")) - require.Equal(t, "consent", q.Get("prompt")) + // prompt=consent is now opt-in via UpstreamForceConsent; + // the default Google path does NOT include it. Returning + // users with an existing offline-access grant get silent + // SSO. Covered in detail by TestOAuthAuthorize_ForceConsentFlag. + require.Empty(t, q.Get("prompt"), "prompt=consent must not be set by default (#121 review)") } else { require.Empty(t, q.Get("access_type")) require.Empty(t, q.Get("prompt")) @@ -453,5 +457,151 @@ func TestOAuthToken_NoRefreshWhenUpstreamReturnsNoRefreshToken(t *testing.T) { require.Equal(t, int32(0), atomic.LoadInt32(&refreshCalls), "refresh attempted despite no upstream refresh_token") } -// keep "io" used for some test paths that may grow later -var _ = io.Discard +// --- soft-fail: upstream refresh returns no id_token ---------------------- + +func TestOAuthToken_RefreshFallsBackWhenUpstreamReturnsNoIDToken(t *testing.T) { + t.Parallel() + // Initial code exchange returns near-expired id_token + refresh_token. + // Refresh-token grant returns 200 OK with only an access_token (no id_token). + // Must soft-fail: forward original near-expired id_token, /token returns 200. + nearExp := time.Now().Add(3 * time.Minute) + priv, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + keyID := "no-id-token-on-refresh-key" + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{ + Key: &priv.PublicKey, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256), + }}}) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, r.ParseForm()) + w.Header().Set("Content-Type", "application/json") + if r.Form.Get("grant_type") == "refresh_token" { + // No id_token field. Common for IdPs that only renew access_token. + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "access_token": "renewed-ax", "token_type": "Bearer", "expires_in": 3600, + }) + return + } + signer, _ := jose.NewSigner(jose.SigningKey{Algorithm: jose.RS256, Key: jose.JSONWebKey{Key: priv, KeyID: keyID, Use: "sig", Algorithm: string(jose.RS256)}}, (&jose.SignerOptions{}).WithType("JWT")) + payload, _ := json.Marshal(map[string]interface{}{ + "sub": "eve@example.com", "aud": "broker", "iat": time.Now().Unix(), + "exp": nearExp.Unix(), "iss": srv.URL, "email": "eve@example.com", "email_verified": true, + }) + obj, _ := signer.Sign(payload) + idTok, _ := obj.CompactSerialize() + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "id_token": idTok, "access_token": "a", "refresh_token": "r", + "token_type": "Bearer", "expires_in": 3600, + }) + }) + cimdURL, redirectURI := "https://demo.example.com/cimd.json", "https://demo.example.com/cb" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":[%q],"token_endpoint_auth_method":"none"}`, cimdURL, redirectURI) + })) + t.Cleanup(cimdServer.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, Mode: "forward", Issuer: srv.URL, JWKSURL: srv.URL + "/jwks", + AuthURL: srv.URL + "/authorize", TokenURL: srv.URL + "/token", + ClientID: "broker", ClientSecret: "s", PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-no-idtok-32bytes!!!!!", UpstreamOfflineAccess: true, + }}} + app := &application{config: cfg, mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), cimdResolver: testResolver(t, cimdServer)} + + rr := runTokenExchange(t, app, cimdURL, redirectURI) + require.Equal(t, http.StatusOK, rr.Code, "missing id_token on refresh must soft-fail; body=%s", rr.Body.String()) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.NotEmpty(t, body["access_token"], "must still forward original id_token") +} + +// --- soft-fail: upstream returns RFC 6749 error_description --------------- + +func TestOAuthToken_RefreshErrorDescriptionSurfacedInLog(t *testing.T) { + t.Parallel() + // Initial code exchange OK; refresh-token grant returns 400 invalid_grant + // with an error_description. The /token call must still return 200 (soft + // fail), but the helper's error chain must include the description so + // pod-log triage isn't blind. We exercise refreshUpstreamIDToken directly + // to assert the error string contains both code and description. + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, r.ParseForm()) + require.Equal(t, "refresh_token", r.Form.Get("grant_type")) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, `{"error":"invalid_grant","error_description":"Token has been expired or revoked."}`) + })) + t.Cleanup(upstream.Close) + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, Mode: "forward", Issuer: upstream.URL, JWKSURL: upstream.URL + "/jwks", + AuthURL: upstream.URL + "/authorize", TokenURL: upstream.URL + "/token", + ClientID: "broker", ClientSecret: "s", PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-errdesc-32bytes!!!!!!", UpstreamOfflineAccess: true, + }}} + app := &application{config: cfg, mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test")} + + _, _, refreshErr := app.refreshUpstreamIDToken("revoked-token") + require.Error(t, refreshErr) + require.Contains(t, refreshErr.Error(), "invalid_grant", "RFC 6749 error code missing from wrapped error") + require.Contains(t, refreshErr.Error(), "Token has been expired or revoked", "error_description not surfaced in wrapped error") +} + +// --- /authorize: force-consent flag honored ------------------------------- + +func TestOAuthAuthorize_ForceConsentFlag(t *testing.T) { + t.Parallel() + cases := []struct { + name string + forceConsent bool + wantPrompt string // "" = absent + }{ + {"default_no_prompt", false, ""}, + {"force_consent_on", true, "consent"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cfg := config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, + Mode: "forward", + Issuer: "https://accounts.google.com", + AuthURL: "https://accounts.google.com/o/oauth2/v2/auth", + TokenURL: "https://oauth2.googleapis.com/token", + ClientID: "broker", + ClientSecret: "s", + PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-fc-32bytes!!!!!!!!!!!", + UpstreamOfflineAccess: true, + UpstreamForceConsent: tc.forceConsent, + Scopes: []string{"openid", "email"}, + }}} + cimdURL := "https://demo.example.com/cimd.json" + cimdServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"client_id":%q,"client_name":"D","redirect_uris":["https://demo.example.com/cb"],"token_endpoint_auth_method":"none"}`, cimdURL) + })) + defer cimdServer.Close() + app := &application{config: cfg, mcpServer: altinitymcp.NewClickHouseMCPServer(cfg, "test"), cimdResolver: testResolver(t, cimdServer)} + verifier, _ := newPKCEVerifier() + form := url.Values{} + form.Set("client_id", cimdURL) + form.Set("redirect_uri", "https://demo.example.com/cb") + form.Set("response_type", "code") + form.Set("code_challenge", pkceChallenge(verifier)) + form.Set("code_challenge_method", "S256") + req := httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/authorize?"+form.Encode(), nil) + rr := httptest.NewRecorder() + app.handleOAuthAuthorize(rr, req) + require.Equal(t, http.StatusFound, rr.Code) + loc, err := url.Parse(rr.Header().Get("Location")) + require.NoError(t, err) + require.Equal(t, "offline", loc.Query().Get("access_type"), "access_type=offline always set when offline_access enabled on Google") + require.Equal(t, tc.wantPrompt, loc.Query().Get("prompt")) + }) + } +} diff --git a/cmd/altinity-mcp/oauth_server.go b/cmd/altinity-mcp/oauth_server.go index 766e6a6..1192985 100644 --- a/cmd/altinity-mcp/oauth_server.go +++ b/cmd/altinity-mcp/oauth_server.go @@ -613,6 +613,48 @@ func safeUpstreamErrorFields(body []byte) (errCode string, length int) { return parsed.Error, len(body) } +// refreshErrorFields is a narrow variant for the refresh-token grant: it +// surfaces error_description in addition to error. Used because Google's +// refresh-token failures are diagnostically richer in `error_description` +// ("Token has been expired or revoked") than the bare `error` enum +// ("invalid_grant"). Description is sanitised before return. +func refreshErrorFields(body []byte) (errCode, errDesc string) { + var parsed struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + } + _ = json.Unmarshal(body, &parsed) + return parsed.Error, sanitizeErrorDesc(parsed.ErrorDescription) +} + +// sanitizeErrorDesc bounds an OAuth error_description for inclusion in our +// own error messages and logs: strips newlines + control chars, caps at +// 120 bytes, returns a leading ": " separator if non-empty. IdP descriptions +// are short human strings in practice ("Token has been expired or revoked", +// "Bad Request", "User must reconsent") — bounded. +func sanitizeErrorDesc(s string) string { + s = strings.TrimSpace(s) + if s == "" { + return "" + } + if len(s) > 120 { + s = s[:120] + } + // Collapse any control chars to a single space. + out := make([]rune, 0, len(s)) + for _, r := range s { + if r == '\r' || r == '\n' || r == '\t' { + out = append(out, ' ') + continue + } + if r < 0x20 || r == 0x7f { + continue + } + out = append(out, r) + } + return ": " + string(out) +} + // challengeScope returns the scope string for the WWW-Authenticate header. // Prefers RequiredScopes (the operator-pinned minimum); falls back to the full // Scopes catalog so the client at least has something to request from. Both @@ -983,18 +1025,19 @@ func (a *application) refreshUpstreamIDToken(refreshToken string) (string, *alti return "", nil, fmt.Errorf("body: %w", err) } if resp.StatusCode >= 300 { - errCode, _ := safeUpstreamErrorFields(body) - return "", nil, fmt.Errorf("upstream %d: %s", resp.StatusCode, errCode) + errCode, errDesc := refreshErrorFields(body) + return "", nil, fmt.Errorf("upstream %d: %s%s", resp.StatusCode, errCode, errDesc) } var parsed struct { - IDToken string `json:"id_token"` - Error string `json:"error"` + IDToken string `json:"id_token"` + Error string `json:"error"` + ErrorDescription string `json:"error_description"` } if err := json.Unmarshal(body, &parsed); err != nil { return "", nil, fmt.Errorf("decode: %w", err) } if parsed.Error != "" { - return "", nil, fmt.Errorf("upstream %s", parsed.Error) + return "", nil, fmt.Errorf("upstream %s%s", parsed.Error, sanitizeErrorDesc(parsed.ErrorDescription)) } if parsed.IDToken == "" { // Some IdPs only return a new access_token on refresh, no id_token. @@ -1213,11 +1256,21 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques // provider — sending the wrong one is a HARD error for Google // (`invalid_scope` if `offline_access` is requested) and a no-op // for Auth0 (`access_type` is ignored). Provider-detect by issuer: - // - Google → access_type=offline + prompt=consent + // - Google → access_type=offline // - Everything else (Auth0/etc.) → offline_access scope + // + // `prompt=consent` is intentionally NOT sent by default. Google + // mints the refresh_token on the first interactive consent and + // honors it on subsequent silent SSO. Forcing consent on every + // authorize shows the "access your account when not using" screen + // to returning users, which is UX-hostile. Operators who need to + // re-enroll (rotated upstream client, revoked grant) set + // oauth.upstream_force_consent=true. See #121. if isGoogleIssuer(cfg.Server.OAuth.Issuer) { upstream.Set("access_type", "offline") - upstream.Set("prompt", "consent") + if cfg.Server.OAuth.UpstreamForceConsent { + upstream.Set("prompt", "consent") + } } else if !slices.Contains(strings.Fields(scope), "offline_access") { scope = strings.TrimSpace(scope + " offline_access") } diff --git a/pkg/config/config.go b/pkg/config/config.go index b3094db..2cfa4f7 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -160,6 +160,16 @@ type OAuthConfig struct { // any downstream refresh-token issuance. UpstreamOfflineAccess bool `json:"upstream_offline_access" yaml:"upstream_offline_access" flag:"oauth-upstream-offline-access" env:"MCP_OAUTH_UPSTREAM_OFFLINE_ACCESS" desc:"Append offline_access to the upstream scope so the IdP's consent screen offers long-lived sessions. v1 does NOT issue downstream refresh tokens regardless of this flag — clients re-authorize via /oauth/authorize."` + // UpstreamForceConsent forces `prompt=consent` on every upstream + // /authorize call (Google-family providers only). The first authorize + // for a user with `upstream_offline_access: true` always triggers the + // consent screen anyway — Google mints the refresh_token there and + // remembers it. Subsequent silent-SSO redemptions reuse the existing + // grant without re-prompting. Set this to true only when the operator + // needs to force re-enrollment (e.g. after rotating the upstream OAuth + // client). Default false avoids the surprise re-consent on every login. + UpstreamForceConsent bool `json:"upstream_force_consent" yaml:"upstream_force_consent" flag:"oauth-upstream-force-consent" env:"MCP_OAUTH_UPSTREAM_FORCE_CONSENT" desc:"Force prompt=consent on every upstream /authorize (Google providers only). Default false reuses Google's stored offline-access grant after the first consent."` + // BrokerUpstream opts gating mode into the DCR-via-MCP broker pattern that // forward mode uses by default. When true under gating mode, altinity-mcp: // - Acts as the OAuth AS to claude.ai/ChatGPT (hosts /oauth/{register,