Skip to content

[vMCP] Extend identity-refresh integration test to wire TokenValidator.Middleware + fake UpstreamTokenRefresher #5334

Open

Description

@tgrunnagle

Context

The fix for #5323 added a higher-level integration test (pkg/vmcp/session/internal/backend/mcp_session_identity_refresh_test.go) that exercises the full transport chain — headerForwardRoundTripper → identityRoundTripper → authRoundTripper (UpstreamInjectStrategy) → http.DefaultTransport — and verifies that a fresh identity placed on req.Context() drives the upstream Authorization header.

That test attaches freshIdentity to the request context directly via auth.WithIdentity(...), which is the same call TokenValidator.Middleware makes at pkg/auth/token.go:1245. The iteration 2 review of #5323 explicitly allowed this higher-level alternative, but noted that it does not verify the link from TokenValidator.Middleware → loadUpstreamTokens → UpstreamTokenRefresher → fresh map on identity.UpstreamTokens.

Gap

The user-visible "forced re-auth every ~24h" symptom can recur via multiple distinct failure modes:

  1. The transport overriding fresh identity (the bug fixed in [vMCP] identityPropagatingRoundTripper captures stale identity, bypassing per-request upstream token refresh #5323, now covered).
  2. loadUpstreamTokens returning a stale map because UpstreamTokenRefresher is broken.
  3. TokenValidator.Middleware failing to call WithIdentity on the refresh path.

The new test only catches (1). Modes (2) and (3) would still produce the same user-visible symptom.

Proposal

Extend mcp_session_identity_refresh_test.go (or create a sibling test in a higher-level package such as pkg/vmcp/server/...) with an end-to-end case that:

  1. Builds a minimal TokenValidator wired to a fake UpstreamTokenRefresher whose returned tokens differ between the first and second call (simulating transparent refresh).
  2. Constructs a signed JWT carrying a tsid claim that the validator can resolve via the fake UpstreamTokenRefresher.
  3. Issues two requests through the validator's middleware into the backend session.
  4. Asserts the upstream observes the rotated token on the second call.

This is the AC5 acceptance criterion as literally written on #5323. Closing this gap is non-trivial test setup (signed JWT, fake refresher, real middleware wiring), which is why the iteration 2 review allowed deferring it.

Acceptance criteria

Notes

Raised as a MEDIUM finding on the iteration 2 review of #5323. The reviewer noted that either filing this issue or extending the existing test is acceptable; this issue makes the gap visible rather than implicit.

Related: #5323, #3877, #3869

Activity

  1. added
    goPull requests that update go code
    vmcpVirtual MCP Server related issues
    on May 19, 2026
  2. 0xbrainkid commented on Jul 3, 2026

    @0xbrainkid

    This is a good place to keep the invariant narrow: the backend should only trust the identity object that came through the validator/refresher path for this request, not a fallback session identity that happens to be available lower in the transport stack.

    I would make the regression assert three different identities explicitly:

    jwt/tsid identity -> resolved by TokenValidator.Middleware
    refreshed upstream token map -> returned by UpstreamTokenRefresher
    fallback session identity -> present but stale
    

    Then the second request should prove both parts of the chain:

    TokenValidator.Middleware attaches refreshed identity to req.Context()
    identityRoundTripper prefers req.Context() identity over fallbackIdentity
    authRoundTripper injects Authorization from the refreshed UpstreamTokens map
    upstream sees token_B, not token_A
    

    That catches the dangerous failure mode where the transport test still passes because auth.WithIdentity(...) was manually attached, while the real middleware path quietly keeps serving a stale token map.

    For longer-lived agent sessions, I would keep external identity evidence out of this core authorization path. AgentFolio/SATP-style proofs can be useful later as optional evidence that “this is the same long-lived agent profile,” but ToolHive should still mint/refresh/enforce its own local session authority and upstream tokens. SATP should be treated as RC-S6 / building-in-public evidence, not as a finished protocol or a substitute for local policy.

    A nice test shape might be table-driven around the precedence rule:

    fresh context identity + stale fallback -> inject fresh token
    missing context identity + fallback identity -> inject fallback token only where fallback is valid
    context identity with empty upstream map + fallback with token -> fail closed or document fallback behavior
    refresher rotates token_A to token_B -> second upstream call observes token_B
    

    That makes the security property easy to read: fallback identity is continuity metadata for the session connector, while the validator/refresher path is the authority for request-time upstream credentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    authenticationgoPull requests that update go codevmcpVirtual MCP Server related issues

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions