test(broker): direct cross-user isolation test for CredentialResolver (MCP-2578)

[Dumbris]

Summary

Backlog follow-up to MCP-1039 / #688 (merged). #688 verified per-user isolation only structurally — every lookup is store.Get(userID, serverKey)-keyed with no shared fallback — but there was no explicit test seeding user B's credential and asserting user A's Resolve never returns it.

This PR adds TestResolve_CrossUserIsolation_NeverReturnsAnotherUsersCredential in internal/serveredition/broker (build tag server):

• Seeds a valid, non-expiring credential for userB + serverKey.
• Calls Resolve as userA for the same serverKey.
• Asserts userA gets the fail-closed path and never userB's token:
  • token_exchange mode → propagated exchange error, got == nil, and userA goes through its own acquisition (1 Exchange call, proving no cache short-circuit to userB).
  • oauth_connect mode → *NotConnectedError with userA's connect URL, no refresh against userB's cached credential, error never contains userB's token.

Why it's a real regression guard

The seeded userB credential is valid and not near expiry, so a regression dropping per-user keying (a shared/static fallback) would make acquire() return it directly from cache. Verified by temporarily simulating a shared fallback — both sub-tests fail and userA literally receives userB-secret-token-MUST-NOT-LEAK.

Verification

• go test -tags server ./internal/serveredition/broker/... -race → green
• golangci-lint (CI v2 config, --build-tags server) → 0 issues
• Test-only change; no production code touched.

Related #688

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

📦 Build Artifacts

Workflow Run: View Run
Branch: test/mcp-2578-cred-resolver-cross-user-isolation

Available Artifacts

• archive-darwin-amd64 (28 MB)
• archive-darwin-arm64 (25 MB)
• archive-linux-amd64 (16 MB)
• archive-linux-arm64 (14 MB)
• archive-windows-amd64 (28 MB)
• archive-windows-arm64 (25 MB)
• frontend-dist-pr (0 MB)
• installer-dmg-darwin-amd64 (21 MB)
• installer-dmg-darwin-arm64 (19 MB)

How to Download

Option 1: GitHub Web UI (easiest)

1. Go to the workflow run page linked above
2. Scroll to the bottom "Artifacts" section
3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 27558046516 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

[Dumbris]

Code Review — CEO Agent (MCP-2582)

Test-only PR. Verified locally: go test -tags server ./internal/serveredition/broker/... -run TestResolve_CrossUserIsolation -race → ok

Assessment: ACCEPT

Test design is correct and the regression guard is well-constructed:

• Valid long-lived userB seed ensures a shared-cache regression would leak the credential (not a vacuous pass)
• token_exchange branch: 1 Exchange call proves userA traversed its own acquisition path, not userB's cache
• oauth_connect branch: 0 refresh calls + error text check closes the leak surface
• Seed-intact assertion rules out false-positives from universal cache misses

No production code touched. Ready to merge.

[cloudflare-workers-and-pages]

Deploying mcpproxy-docs with    Cloudflare Pages

Latest commit:	7846879
Status:	✅  Deploy successful!
Preview URL:	https://32f4f0d4.mcpproxy-docs.pages.dev
Branch Preview URL:	https://test-mcp-2578-cred-resolver.mcpproxy-docs.pages.dev

View logs

[mcpproxy-gatekeeper]

Claude review ACCEPT (codex out). Direct cross-user isolation test for CredentialResolver: seeds userB cred (valid + refresh-token variants), resolves as userA, asserts userA gets fail-closed (nil + NotConnectedError/err) and the tight isolation checks ex.calls==1 / refreshCalls==0 / error never contains userBToken — would genuinely fail if keying leaked. -tags server -race passes. Closes the #688 follow-up gap (MCP-2578).

[Dumbris]

CEO Code Review — LGTM (CodexReviewer exhausted credits; reviewed as MCP-2587 backstop)

Two subtests, both PASS (-tags server):

• token_exchange mode falls closed, not to user B's cache ✅
• oauth_connect mode falls closed to NotConnectedError, not user B's cache ✅

Review notes:

• Adversarially structured: user B's credential is valid + long-lived, so a shared-cache regression would return it directly and skip Exchange entirely — the ex.calls == 1 assertion catches exactly that regression.
• Both acquisition modes covered (token_exchange + oauth_connect).
• User B's credential verified intact post-run — proves the miss is per-user isolation, not data absence.
• No production code changes; 94-line pure test addition.

Ready to merge.