oauth/cimd: post-merge review nits (F1, F2, F6, F7)

cmd/altinity-mcp/oauth_server.go

@@ -113,7 +113,20 @@ type oauthIssuedCode struct {
 // whoever holds the verifier — i.e. the legitimate client. Trading strict
 // RFC 6749 §4.1.2 single-use for zero shared state across replicas.
 
+// writeOAuthTokenError writes an RFC 6749 §5.2 JSON error response. When
+// status is 401 it also sets a Bearer-scheme WWW-Authenticate per RFC 7235
+// §3.1 (which mandates the header on 401) and RFC 6750 §3 (Bearer challenge
+// shape). The header value carries the OAuth error code so a client can
+// parse it from either the header or the body.
+//
+// Used uniformly across /oauth/{authorize,callback,token} so every broker-
+// mode error response shares one shape — JSON body, optional WWW-Authenticate.
+// Resource-server 401s use writeOAuthError instead because they need the
+// RFC 9728 `resource_metadata=` hint.
 func writeOAuthTokenError(w http.ResponseWriter, status int, code, description string) {
+	if status == http.StatusUnauthorized {
+		w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer error=%q, error_description=%q`, code, description))
+	}
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(status)
 	_ = json.NewEncoder(w).Encode(map[string]string{
@@ -1023,14 +1036,14 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques
 		return
 	}
 	if r.Method != http.MethodGet {
-		http.Error(w, "Method not allowed", http.StatusMethodNotAllowed)
+		writeOAuthTokenError(w, http.StatusMethodNotAllowed, "invalid_request", "method not allowed")
 		return
 	}
 	q := r.URL.Query()
 	clientID := q.Get("client_id")
 	redirectURI := q.Get("redirect_uri")
 	if clientID == "" || redirectURI == "" || q.Get("response_type") != "code" {
-		http.Error(w, "Invalid authorization request", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "missing client_id, redirect_uri, or response_type=code")
 		return
 	}
 	// CIMD inbound (#115): client_id is the HTTPS URL of the MCP client's
@@ -1041,15 +1054,15 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques
 	client, err := a.resolveCIMDClient(r.Context(), clientID)
 	if err != nil {
 		log.Debug().Err(err).Str("client_id", truncateForLog(clientID, 80)).Msg("OAuth /authorize rejected: CIMD resolution failed")
-		http.Error(w, "Unknown OAuth client", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_client", "unknown OAuth client")
 		return
 	}
 	if !slices.Contains(client.RedirectURIs, redirectURI) {
-		http.Error(w, "Unknown OAuth client", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "redirect_uri not registered for this client")
 		return
 	}
 	if q.Get("code_challenge") == "" || q.Get("code_challenge_method") != "S256" {
-		http.Error(w, "PKCE S256 is required", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "PKCE S256 is required")
 		return
 	}
 	// RFC 8707 §2 / MCP authorization spec: clients SHOULD include `resource`.
@@ -1063,7 +1076,7 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques
 		got := strings.TrimRight(resource, "/")
 		if got != want {
 			log.Debug().Str("got", resource).Str("want", a.resourceBaseURL(r)).Msg("OAuth /authorize rejected: resource indicator mismatch")
-			http.Error(w, "Invalid resource indicator", http.StatusBadRequest)
+			writeOAuthTokenError(w, http.StatusBadRequest, "invalid_target", "resource indicator does not identify this MCP server")
 			return
 		}
 	}
@@ -1075,7 +1088,7 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques
 	// the /token exchange in handleOAuthCallback.
 	upstreamVerifier, err := newPKCEVerifier()
 	if err != nil {
-		http.Error(w, "Failed to generate PKCE verifier", http.StatusInternalServerError)
+		writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to generate PKCE verifier")
 		return
 	}
 
@@ -1092,14 +1105,14 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques
 	})
 	if err != nil {
 		log.Error().Err(err).Msg("Failed to encode pending-auth JWE")
-		http.Error(w, "Failed to initialize OAuth state", http.StatusInternalServerError)
+		writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to initialize OAuth state")
 		return
 	}
 
 	cfg := a.GetCurrentConfig()
 	authURL, err := a.resolveUpstreamAuthURL()
 	if err != nil {
-		http.Error(w, "Failed to resolve upstream authorization endpoint", http.StatusBadGateway)
+		writeOAuthTokenError(w, http.StatusBadGateway, "server_error", "failed to resolve upstream authorization endpoint")
 		return
 	}
 	callbackURL := joinURLPath(a.oauthAuthorizationServerBaseURL(r), a.oauthCallbackPath())
@@ -1129,13 +1142,13 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request
 	requestID := r.URL.Query().Get("state")
 	code := r.URL.Query().Get("code")
 	if requestID == "" || code == "" {
-		http.Error(w, "Missing callback parameters", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "missing state or code on callback")
 		return
 	}
 
 	pending, ok := a.decodePendingAuth(requestID)
 	if !ok {
-		http.Error(w, "Unknown OAuth request", http.StatusBadRequest)
+		writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "unknown or expired authorization request")
 		return
 	}
 
@@ -1159,7 +1172,7 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request
 	authCode, err := a.encodeAuthCode(issuedCode)
 	if err != nil {
 		log.Error().Err(err).Msg("Failed to encode auth-code JWE")
-		http.Error(w, "Failed to issue authorization code", http.StatusInternalServerError)
+		writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to issue authorization code")
 		return
 	}
 
@@ -1170,7 +1183,7 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request
 
 	redirect, err := url.Parse(pending.RedirectURI)
 	if err != nil {
-		http.Error(w, "Invalid redirect URI", http.StatusBadGateway)
+		writeOAuthTokenError(w, http.StatusBadGateway, "server_error", "pending-auth carried an unparseable redirect_uri")
 		return
 	}
 	params := redirect.Query()

cmd/altinity-mcp/oauth_server_test.go

@@ -597,13 +597,90 @@ func TestTtlSeconds(t *testing.T) {
 
 func TestWriteOAuthTokenError(t *testing.T) {
 	t.Parallel()
-	rr := httptest.NewRecorder()
-	writeOAuthTokenError(rr, http.StatusBadRequest, "invalid_request", "bad thing happened")
-	require.Equal(t, http.StatusBadRequest, rr.Code)
-	require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
-	var body map[string]string
-	require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
-	require.Equal(t, "invalid_request", body["error"])
-	require.Equal(t, "bad thing happened", body["error_description"])
+	t.Run("400 has no WWW-Authenticate", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		writeOAuthTokenError(rr, http.StatusBadRequest, "invalid_request", "bad thing happened")
+		require.Equal(t, http.StatusBadRequest, rr.Code)
+		require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+		require.Empty(t, rr.Header().Get("WWW-Authenticate"), "non-401 responses must not advertise an auth challenge")
+		var body map[string]string
+		require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
+		require.Equal(t, "invalid_request", body["error"])
+		require.Equal(t, "bad thing happened", body["error_description"])
+	})
+	t.Run("401 carries Bearer challenge per RFC 7235 §3.1", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		writeOAuthTokenError(rr, http.StatusUnauthorized, "invalid_client", "unknown OAuth client")
+		require.Equal(t, http.StatusUnauthorized, rr.Code)
+		challenge := rr.Header().Get("WWW-Authenticate")
+		require.NotEmpty(t, challenge, "401 responses MUST carry WWW-Authenticate")
+		require.Contains(t, challenge, "Bearer ")
+		require.Contains(t, challenge, `error="invalid_client"`)
+		require.Contains(t, challenge, `error_description="unknown OAuth client"`)
+	})
+}
+
+// TestOAuthAuthorizeErrorsAreJSON pins F1 from the post-merge review: every
+// 4xx/5xx error response from /oauth/authorize and /oauth/callback returns
+// the RFC 6749 §5.2 JSON shape (application/json + error/error_description),
+// never the bare text/plain Go default that http.Error produces. Regression
+// guard for the high-severity finding.
+func TestOAuthAuthorizeErrorsAreJSON(t *testing.T) {
+	t.Parallel()
+	app := &application{
+		config: config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{
+			Enabled:             true,
+			Mode:                "forward",
+			Issuer:              "https://idp.example.com",
+			PublicAuthServerURL: "https://mcp.example.com",
+			SigningSecret:       "regression-f1-jsonerr-32bytes!!!!",
+		}}},
+	}
+
+	t.Run("/authorize missing params → JSON invalid_request", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		app.handleOAuthAuthorize(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/authorize", nil))
+		require.Equal(t, http.StatusBadRequest, rr.Code)
+		require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+		var body map[string]string
+		require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
+		require.Equal(t, "invalid_request", body["error"])
+	})
+
+	t.Run("/authorize wrong method → JSON invalid_request", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		app.handleOAuthAuthorize(rr, httptest.NewRequest(http.MethodPost, "https://mcp.example.com/oauth/authorize", nil))
+		require.Equal(t, http.StatusMethodNotAllowed, rr.Code)
+		require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+		var body map[string]string
+		require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
+		require.Equal(t, "invalid_request", body["error"])
+	})
+
+	t.Run("/callback missing state+code → JSON invalid_request", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		app.handleOAuthCallback(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/callback", nil))
+		require.Equal(t, http.StatusBadRequest, rr.Code)
+		require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+		var body map[string]string
+		require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
+		require.Equal(t, "invalid_request", body["error"])
+	})
+
+	t.Run("/callback bogus state → JSON invalid_request", func(t *testing.T) {
+		t.Parallel()
+		rr := httptest.NewRecorder()
+		app.handleOAuthCallback(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/callback?state=bogus&code=x", nil))
+		require.Equal(t, http.StatusBadRequest, rr.Code)
+		require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+		var body map[string]string
+		require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body))
+		require.Equal(t, "invalid_request", body["error"])
+	})
 }
 

docs/oauth_authorization.md

@@ -64,7 +64,7 @@ config:
       # signing_secret injected via MCP_OAUTH_SIGNING_SECRET env var
 ```
 
-Fields that **must not be present** under gating (startup refuses with a clear error naming the field): `client_id`, `client_secret` / `MCP_OAUTH_CLIENT_SECRET`, `token_url`, `auth_url`, `userinfo_url`, `public_auth_server_url`, `refresh_revokes_tracking`, `callback_path`.
+Fields that **must not be present** under gating (startup refuses with a clear error naming the field): `client_id`, `client_secret` / `MCP_OAUTH_CLIENT_SECRET`, `token_url`, `auth_url`, `userinfo_url`, `public_auth_server_url`, `callback_path`.
 
 **Forward mode** (live example: `$MCP_DEPLOY_DIR/antalya/mcp-values.yaml`):
 
@@ -112,7 +112,6 @@ An operator moving from the old gating (MCP-as-AS) to new gating (pure resource
 - `auth_url`
 - `userinfo_url`
 - `public_auth_server_url`
-- `refresh_revokes_tracking`
 - `callback_path`
 
 The canonical diff pattern is the `otel` values change committed on `feature/dcr-via-auth0` (`$MCP_DEPLOY_DIR/otel/mcp-values.yaml`).
@@ -294,7 +293,7 @@ config:
         - "https://mcp.example.com"
       # signing_secret via MCP_OAUTH_SIGNING_SECRET env var
       # DO NOT set: client_id, client_secret, token_url, auth_url,
-      #             userinfo_url, public_auth_server_url, refresh_revokes_tracking
+      #             userinfo_url, public_auth_server_url
 ```
 
 ### Cluster-secret authentication (optional)
@@ -478,7 +477,7 @@ server:
     # - "gating": pure resource server — validate AS-issued JWTs (JWKS + RFC 8707 aud + scopes).
     #             Upstream IdP handles DCR/authorize/token. Requires issuer + audience.
     #             Forbidden fields: client_id, client_secret, token_url, auth_url,
-    #             userinfo_url, public_auth_server_url, refresh_revokes_tracking.
+    #             userinfo_url, public_auth_server_url.
     # - "forward": MCP proxies DCR + authorize + token to upstream; relays upstream tokens.
     #              Requires client_id, client_secret, auth_url, token_url.
     mode: "gating"

pkg/config/config.go

@@ -102,7 +102,7 @@ type JWEConfig struct {
 //
 // Every flag-tagged field is settable via CLI flag (`flag:` tag) or env var
 // (`env:` tag). The env-var convention here is `MCP_OAUTH_<UPPER_SNAKE>` so
-// secrets like GatingSecretKey can be injected from a Kubernetes Secret via
+// secrets like SigningSecret can be injected from a Kubernetes Secret via
 // the Helm chart's env: array using valueFrom.secretKeyRef.
 type OAuthConfig struct {
 	// Mode controls whether altinity-mcp forwards external OAuth bearers or gates them into local MCP tokens.
@@ -218,12 +218,13 @@ type OAuthConfig struct {
 	// RefreshTokenTTLSeconds controls how long minted refresh tokens remain valid.
 	RefreshTokenTTLSeconds int `json:"refresh_token_ttl_seconds" yaml:"refresh_token_ttl_seconds" flag:"oauth-refresh-token-ttl-seconds" env:"MCP_OAUTH_REFRESH_TOKEN_TTL_SECONDS" desc:"Refresh token lifetime in seconds"`
 
-	// SigningSecret is the server-side symmetric secret used to HMAC-sign every
-	// stateless OAuth artifact this server mints: self-issued JWT access tokens
-	// (HS256), authorization codes, refresh tokens, and RFC 7591 dynamic-client-
-	// registration `client_secret`s. Required whenever OAuth is enabled, in both
-	// forward and gating modes.
-	SigningSecret string `json:"signing_secret" yaml:"signing_secret" flag:"oauth-signing-secret" env:"MCP_OAUTH_SIGNING_SECRET" desc:"Server-side HMAC secret for all stateless OAuth artifacts (JWTs, auth codes, refresh tokens, DCR client_secrets)"`
+	// SigningSecret is the server-side symmetric secret used to HKDF-derive
+	// keys for every stateless OAuth JWE this server mints: pending-auth
+	// state (the upstream `state` parameter) and the downstream auth-code
+	// returned from /oauth/callback. Required whenever OAuth broker mode is
+	// active (forward, or gating + broker_upstream). Per #115 v1 issues no
+	// downstream refresh tokens and no DCR client_secrets.
+	SigningSecret string `json:"signing_secret" yaml:"signing_secret" flag:"oauth-signing-secret" env:"MCP_OAUTH_SIGNING_SECRET" desc:"Server-side HKDF master secret for OAuth JWE artifacts (pending-auth state, downstream auth codes). Required whenever broker mode is active."`
 }
 
 func (cfg OAuthConfig) NormalizedMode() string {