Return 401+WWW-Authenticate when vMCP upstream token is unrefreshable

[tgrunnagle]

Summary

When a vMCP backend's upstream provider token expires and cannot be refreshed, the
failure was surfacing as a generic backend error with no HTTP 401 + `WWW-Authenticate`
challenge, leaving MCP clients with no standard signal to re-authenticate. This is
inconsistent with the single-server runner path (`upstreamswap` middleware), which
already returns a proper RFC 6750 challenge in the same situation.

• Extends `TokenReader.GetAllValidTokens` to return the names of providers whose
  token refresh failed alongside the successful tokens map, and surfaces that signal
  in `TokenValidator.Middleware`: when any provider refresh fails the middleware writes
  HTTP 401 + `WWW-Authenticate` immediately — before any inner handler runs — so the
  MCP client sees a proper re-auth signal instead of a generic JSON-RPC error.
• Adds `Identity.FailedUpstreamProviders []string` to carry the failed provider names
  through the enrichment path for observability.
• Adds an explicit `errors.Is(err, authtypes.ErrUpstreamTokenNotFound)` branch in
  `wrapBackendError` as defense-in-depth, replacing a fragile substring match.

Closes #5507

Type of change

• Bug fix

Test plan

• Unit tests (`task test`)
• Linting (`task lint-fix`)

Changes

File	Change
`pkg/auth/upstreamtoken/types.go`	`TokenReader.GetAllValidTokens` gains a `failed []string` return for providers whose refresh failed
`pkg/auth/upstreamtoken/service.go`	`InProcessService.GetAllValidTokens` collects failed provider names instead of silently dropping them
`pkg/auth/upstreamtoken/mocks/`	Regenerated mock to match updated interface
`pkg/auth/identity.go`	Adds `FailedUpstreamProviders []string` to `Identity`
`pkg/auth/token.go`	`loadUpstreamTokens` threads the failed list through; `Middleware` returns HTTP 401 + `WWW-Authenticate` when non-empty
`pkg/auth/token_test.go`	Updates mock `Return` calls; adds 401-path test for failed provider refresh
`pkg/authserver/integration_test.go`	Updates `GetAllValidTokens` call sites to 3-return form
`pkg/vmcp/server/server.go`	Removes `upstreamTokenCheckMiddleware` wiring; updates middleware-chain comment
`pkg/vmcp/server/upstream_token_check.go`	Deleted — logic now lives in `TokenValidator.Middleware`
`pkg/vmcp/server/upstream_token_check_test.go`	Deleted
`pkg/vmcp/client/client.go`	Explicit `ErrUpstreamTokenNotFound` → `ErrAuthenticationFailed` mapping in `wrapBackendError`
`pkg/vmcp/client/client_test.go`	Two new table cases for the sentinel mapping

Does this introduce a user-facing change?

Yes. Previously, a tool call made when the user's upstream provider credential had
expired (and was not refreshable) returned a generic JSON-RPC error with no
re-authentication hint. After this change, vMCP returns HTTP 401 with:

```
WWW-Authenticate: Bearer error="invalid_token", error_description="upstream token is no longer valid; re-authentication required"
```

MCP clients that respect RFC 6750 can now detect the condition and prompt the user
to re-authenticate with the upstream provider.

Special notes for reviewers

Where the 401 fires: `TokenValidator.Middleware` in `pkg/auth/token.go` — the
generic OIDC auth middleware, not a vMCP-specific layer. This means any path that uses
`WithUpstreamTokenReader` gets the behaviour for free. The 401 fires before any inner
handler, so the mcp-go SDK never sees the request and cannot commit to HTTP 200.

No registry dependency: The previous approach read the backend registry on every
request to determine which providers were "required". The new approach detects the
failure at the source — inside `GetAllValidTokens` — so no registry lookup is needed
and the signal is exact: 401 fires only when a refresh actually failed, not merely when
a token key happens to be absent.

`ErrUpstreamTokenNotFound` in `wrapBackendError`: Kept as defense-in-depth for
any code path that calls the backend auth strategy directly without going through the
middleware (e.g., background refresh or test harnesses). The explicit sentinel mapping
takes priority over the string-based fallback.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.35%. Comparing base (5310b0c) to head (c5df449).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/token.go	94.73%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5651   +/-   ##
=======================================
  Coverage   70.34%   70.35%           
=======================================
  Files         649      649           
  Lines       66101    66185   +84     
=======================================
+ Hits        46500    46562   +62     
- Misses      16253    16269   +16     
- Partials     3348     3354    +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: Security, Error Handling, Test Coverage, Architecture, General Quality (Codex: skipped — CLI not installed)

Consensus Summary

#	Finding	Score	Severity	Action
F1	firstMissingProvider return value unused — name misleads	8/10	LOW	Fix
F2	No log line when 401 short-circuit fires	9/10	LOW	Fix
F3	Empty SubjectProviderName creates undocumented silent pass-through assumption	8/10	MEDIUM	Fix
F4	Nil-registry guard skips middleware silently — no doc or log	7/10	LOW	Fix

Overall

This PR correctly addresses the stated issue: a missing upstream provider token no longer silently degrades to a JSON-RPC error — it now returns HTTP 401 with a proper RFC 6750 WWW-Authenticate challenge at the HTTP boundary, consistent with the single-server upstreamswap middleware. The pre-check approach (verify all backends' required tokens before dispatch) is the right call for vMCP's fan-out model; the alternative of aggregating per-backend 401s after mcp-go commits to HTTP 200 would require significantly more complexity.

The four consensus findings are polish items, not correctness issues. The most important is F3 (MEDIUM): upstreamProviderName returns "" for token_exchange and aws_sts backends when SubjectProviderName is empty, causing the middleware to silently skip those backends. If SubjectProviderName can be empty at request time (the field is described as optionally auto-populated), those backends would still produce a JSON-RPC error on missing identity.Token — the original problem. The middleware doc comment should document this assumption. F1 and F2 are minor: firstMissingProvider should either use its return value (in a log line or the WWW-Authenticate description) or be renamed to reflect that only the boolean result matters; and a slog.Debug call when the 401 fires would make incident diagnosis faster in vMCP's multi-backend environment. F4 is a one-liner comment on the guard condition.

The wrapBackendError change (explicit errors.Is for ErrUpstreamTokenNotFound) is a clean, well-motivated hardening of the existing fragile substring match. The test suite is thorough: it covers all four strategy types, nil configs, the fan-out partial-failure case, and errors.Is chain traversal through wrapped errors.

---

Generated with Claude Code

[jhrozek]

Three related issues in the new upstream-refresh-failure 401 block.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: security-auth, error-handling, test-coverage, architecture, general-quality

Consensus Summary

#	Finding	Consensus	Severity	Action
F1	401 path bypasses buildWWWAuthenticate (no realm/resource_metadata)	7/10	MEDIUM	Fix
F3	Generic middleware embeds vMCP-specific fan-out 401 policy	8/10	MEDIUM	Document / Discuss
F4	FailedUpstreamProviders set before 401 short-circuit but never reaches context	8/10	LOW	Fix
F5	Missing test: multiple providers all failing in one GetAllValidTokens call	7/10	MEDIUM	Fix
F6	Missing test: partial failure in Middleware (some providers succeed, one fails)	8/10	MEDIUM	Fix

Overall

This PR correctly addresses the stated bug: when an upstream provider token expires and cannot be refreshed, the failure now surfaces as an HTTP 401 + WWW-Authenticate challenge rather than a generic backend error, giving MCP clients the RFC 6750 signal they need to re-authenticate. The approach — extending GetAllValidTokens to return named failed providers and short-circuiting in TokenValidator.Middleware — cleanly separates infrastructure errors (storage unavailable → non-nil error → 503) from application-level refresh failures (failed []string → 401). Removing upstreamTokenCheckMiddleware in favor of the generic middleware path is a clear simplification, and the explicit ErrUpstreamTokenNotFound sentinel in wrapBackendError closes the fragile string-match fallback.

Two items need attention before merge. F1: the new 401 branch hard-codes a bare Bearer challenge string instead of calling v.buildWWWAuthenticate() — the helper that every other 401 path in this function uses. This omits the realm and resource_metadata fields required by RFC 9728 discovery, which is the mechanism that lets MCP clients find the right auth server for re-authentication. It also uses http.Error (plain text) instead of writeOAuthError (JSON), inconsistent with all other error branches. F3: the "any failed provider = full 401" policy is baked into the generic pkg/auth middleware without acknowledging the fan-out limitation the issue thread called out — a vMCP session with backends needing different providers will reject the whole request even when only one provider fails. The behavior is safe but the trade-off should be documented in a comment.

F5 and F6 are missing test cases that would verify the all-or-nothing rejection policy holds under multi-provider failure and partial-failure scenarios respectively. Both are straightforward to add. F4 is a cleanup: identity.FailedUpstreamProviders is assigned on a transient struct that is discarded when the 401 fires, making the field effectively unreachable from the served context and its doc comment confusing.

Documentation

The TODO(auth) comment removed from pkg/auth/upstreamtoken/types.go tracked a richer per-provider metadata approach (distinguishing "never had a token" from "had a token that expired and couldn't refresh"). This distinction is still unresolved — issue #5507's description explicitly flags it. Consider opening a follow-up issue or restoring an updated TODO before closing #5507.

---

Generated with Claude Code