stacklok / stacklok/toolhive

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

Open
#5,334 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

authentication go vmcp
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

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.MiddlewareloadUpstreamTokensUpstreamTokenRefresher → 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 #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

  • A test exists that exercises TokenValidator.Middleware → loadUpstreamTokens → UpstreamTokenRefresher end-to-end against a vMCP backend session.
  • The fake UpstreamTokenRefresher rotates tokens between the two calls.
  • The upstream httptest.Server observes the rotated token on the second call.
  • The test fails if any link in the chain (middleware, refresher, or transport) is broken in a way that would surface the original #5323 symptom.

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with pkg/vmcp/session/internal/backend/mcp_session_identity_refresh_test.go and pkg/auth/token.go:1245, then trace the TokenValidator and UpstreamTokenRefresher wiring used by the existing vMCP session path. Run the current identity-refresh integration test first. Done means two middleware requests reach the httptest upstream and the second observes the fake refresher's rotated token, covering the full chain.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
authentication, backend, testing
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.