feat: validate remote sessions live from the consent page - #6175
Conversation
🦋 Changeset detectedLatest commit: cf84526 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Running ultrareview automatically — Introduces live credential validation that presents stored upstream tokens in a real handshake, refactors shared session logic used by runtime dispatch, and adds new DB columns/clear paths — a mis-validation could mislead users or leak credentials, and the refactor risks regressions in existing.... I'll post findings when complete. |
5c292e7 to
a5c7d1a
Compare
There was a problem hiding this comment.
Ultrareview completed in 20m 37s
All reported issues were addressed across 25 files
Linked issue analysis
Linked issue: AIM-204: Live session validation via MCP initialize: validate consent action, cached status, keepalive re-check
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add a consent-page validate/Re-check action that probes the connected remote-session credential. | The consent action handles validate and the card exposes Re-check for connected, probeable sessions. |
| ✅ | Present the routed credential through an MCP initialize, notifications/initialized, and session DELETE handshake. | The shared upstream handshake performs initialize, acknowledgment, and best-effort close, with end-to-end wire-shape tests. |
| ✅ | Route validation through the same upstream path used by runtime dispatch for gateway members and standalone endpoints. | The implementation resolves gateway members through dialMetaMember and standalone backends through shared remote/tunnel proxy builders. |
| ✅ | Classify probe results as valid for 2xx, rejected_by_member for 401/403, and unknown otherwise, without quoting upstream response bodies. | Classification is implemented and tested for success, authentication rejection, non-auth statuses, transport errors, and timeouts; reasons are Gram-authored. |
| ✅ | Persist validation timestamp, status, and safe reason on the matching remote-session row, while preserving a stored valid result across unknown probes. | Scoped writer methods persist the verdict without changing updated_at, and validationSupersedes prevents unknown from downgrading valid. |
| ✅ | Show cached validation status on the consent card without probing on page load, including verified, rejected/reconnect, unknown, and not-yet-verified states. | Card construction reads stored fields and templates render all specified states; the only probe trigger is the explicit action. |
| ✅ | Clear validation fields when a remote session is deleted or replaced by a new grant. | Soft-delete queries clear all validation columns and the existing upsert reset is retained; deletion and re-grant behavior is tested. |
| ✅ | Emit live-validation metrics by outcome and issuer host, and add the remote-session ID logging attribute at refresh-failure sites. | The new counter records outcome and issuer dimensions, and refresh failure attributes include the session ID. |
| ✅ | Keep keepalive validation out of this phase-1 PR. | The issue explicitly defers keepalive re-check to phase 2, and the PR describes and implements only the consent-page validation flow. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
4b7bbc1 to
5e2ed6e
Compare
|
Testing summary: Reviews. Three independent passes (security, runtime regression risk of the handshake refactor, data and test coverage). No blockers; every should-fix applied: verdict write is a compare-and-swap on Unit. Status classification (2xx, 401, 403, other, transport, timeout), never-downgrade rule, time-ago rendering including future timestamps, template rendering for verified, rejected, unknown, never-validated, and rate-limited states with escaping of the reason and member name, ambiguous-credential error typing. End to end against a scripted httptest member and the fake tunnel gateway: 200 records valid and shows "Verified"; hang records unknown and leaves a stored valid untouched; 401 and 403 record rejected with the Reconnect control; one initialize with the Bearer token per probe and a DELETE after each minted session; stateless member (no session id) records valid with no ack or DELETE; 200 then 401 on notifications/initialized records rejected after a DELETE; a failing DELETE keeps valid; standalone remote and tunneled backends; org-level client; expired grant refused; toolset-backed endpoint offers no Re-check; a reconnect landing mid-probe leaves the new grant unvalidated; the seventh Re-check in a minute is refused with no upstream request and no write; a refresh through the refresh harness clears the three columns. Runtime parity. The reviewer traced the refactored Suites. mcp and remotesessions: 1,417 tests. Server lint clean. Full server suite result to follow. Real provider (2026-09-09, head after the dispatch fix). Against mcp.linear.app from a local stack, with the Linear issuer created over RPC, the client registered with Linear through Automatic verification (db605e8, reworked in a2f651d after a three-way review: the callback returns the grant instead of calling a hook, the endpoint is resolved from the challenge reference, the auto probe is exempt from the per-challenge limiter, panics are recovered). Unit: a committed grant is probed and renders Verified on first view; a rejecting member lands Rejected with Reconnect; unknown challenge, unknown slug, or unbound client is skipped with no probe; CompleteRemoteLogin returns the committed grant keyed to its consent challenge. Live against Linear from the local stack: Disconnect → Connect → Linear authorize → the consent page rendered "Verified just now" with no click, row 2026-07-28 probe (a9e03e4). New fixture modes for a member that speaks only the 2026-07-28 shape: a versioned Probe review follow-up (c48b488). The stateless ping now mirrors its revision and client in Stateless dispatch and control ping (71394e6, 406e6f0, 1a8e976). A member that answers pings unauthenticated no longer reads Verified: the probe repeats the ping without the bearer and falls back to the handshake when that also succeeds (test with an open-ping member). Dispatch: a member that refuses |
5e2ed6e to
f2eb240
Compare
a5c7d1a to
c170945
Compare
f2eb240 to
4761aa4
Compare
This comment has been minimized.
This comment has been minimized.
c170945 to
a62f997
Compare
4761aa4 to
3f47ddb
Compare
a62f997 to
a13bad2
Compare
3f47ddb to
979f543
Compare
This comment has been minimized.
This comment has been minimized.
e6f8bef to
ccf44cc
Compare
38db55c to
3c35700
Compare
4534ba3 to
f83bbd7
Compare
f459ffe to
6a328a5
Compare
f83bbd7 to
8e3bc0b
Compare
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/mcp/serve_meta_proxy.go">
<violation number="1" location="server/internal/mcp/serve_meta_proxy.go:347">
P2: When dispatch reaches this branch for a session-bearing initialize error, `ackBody` sends literal backslashes from a raw string, so the member cannot parse `notifications/initialized`. Use an unescaped JSON raw literal.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
bflad
left a comment
There was a problem hiding this comment.
Requesting changes, on packaging rather than on the idea. The motivation is real and the core design choices are the right ones: probe through the same routing the runtime dispatches through, strip session and census interceptors so a probe is never counted as traffic, enforce "unknown never downgrades valid" in the UPDATE rather than in Go, and never quote upstream bodies back to the page. My concern is that this is 4,005 lines across 42 files, and that a large piece of it is outside AIM-204's scope.
Please split the mechanical work out first
Three chunks are independently reviewable and mergeable right now. Landing them separately front-loads the boring half of the diff and leaves a much smaller feature PR behind.
The remoteBackendProxyBuilder extraction in serveendpoint.go, around 40 lines. Pure refactor, no behavior change, independent of everything else.
The upstream exchange plumbing in serve_meta_proxy.go, around 150 lines once the protocol revision work is set aside. This is the openMemberSession/memberExchange split into handshakeUpstream/upstreamExchange/closeUpstreamSession, plus the dialMetaMember/routeMetaMember split that moves metrics into the counting wrapper. Mostly mechanical, but it carries two real behavior changes that deserve their own review rather than being read as part of a much larger diff: capturing the session id via UpstreamResponseInterceptor so a session minted before a body failure still gets closed, and the new errAmbiguousMemberCredential sentinel.
The storage layer, around 300 lines plus generated sqlc. remotesessions/validation.go, SetRemoteSessionValidation, the verdict clearing across the soft delete and refresh queries, the RemoteSessionState fields, and the UpstreamToken CAS pair. Fully self-contained, and the columns already exist on main from #6104, so a writer can land ahead of any caller. One request while it is still unreferenced: ValidationOutcome is documented as "the closed set remote_sessions.validation_status stores", which makes it a persistence concern, but it currently lives in remotesessionmetrics. The result is that both the storage layer and the HTML renderer import a metrics package to talk about domain state. Moving it into remotesessions alongside RemoteSessionValidation is much cheaper now than after three packages depend on it.
What remains after those three is the probe and the manual Verify action, which is the actual AIM-204 deliverable, plus automatic verification on connect (CompleteRemoteLogin, authnchallenge_consent_autoverify.go, AutoVerifyWait, the shouldAutoCloseFirstParty change). That last piece is not in the ticket, and it changes when a first-party connect tab closes, so I would like it to stand on its own where that decision is easy to see.
On the handshake work: please use the official SDK
github.com/modelcontextprotocol/go-sdk is already a direct dependency at v1.7.0, and it solves most of the handshake problems this PR solves by hand.
Its latestProtocolVersion is 2026-07-28 (mcp/shared.go:50), it negotiates across all five revisions, and negotiatedVersion already encodes the detail that initialize is deprecated in that revision. So statelessProbeUpstream, statelessPing, withRevisionMeta, handshakeRefused, openStatelessMemberSession, and the credential-free control ping are reimplementing behavior we already ship. client.Close() also issues the session DELETE (mcp/streamable.go:2730), which retires the cleanupBudget and recordBudget arithmetic in probeUpstreamSession.
We already have this probe shape in tree. unproxiedmcp/impl.go:325 runs a one-shot bounded initialize plus tools/list with DisableRetries: true and a response cap, and externalmcp.AuthRejectedError carries the 401/403 status from both the connect leg and the list leg, which maps directly onto rejected_by_member.
I would also like the verdict to come from tools/list rather than from the initialize result. It makes the probe a literal dry run of dispatch instead of a parallel handshake implementation, so the answer is definitionally "what would a real tool call have seen", and it exercises more of the path than the handshake alone. tools/list is a proven-safe payload here, since the meta gateway already issues it against members on the normal serving path, well inside the 4 MiB MaxMemberResponseBytes cap.
It also sidesteps a live bug. withRevisionMeta (serve_meta_proxy.go:432) stamps _meta by round-tripping params through json.Marshal into map[string]any and back, but toolsCallParams.Arguments is a json.RawMessage carrying caller-supplied arguments verbatim, so every nested number decodes as float64. Concretely, {"id":1234567890123456789} reaches the member as {"id":1234567890123456800} on the stateless dispatch path. tools/list takes no caller-supplied arguments, so the lossy hop has nowhere to happen.
The one gap is routing. externalmcp.NewClient dials a URL directly, while member routing (tunnel affinity, configured headers, RBAC, visibility) lives in proxy.Proxy. But mcp.StreamableClientTransport accepts an HTTPClient, and this PR already performs its exchanges as serveProxyBackend(rec, req, p) into a recorder, so wrapping that as a RoundTripper looks like a small adapter over code that already exists rather than new machinery. I have not prototyped it, so please sanity check that before committing to the approach.
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Split as requested: #6291 (proxy builder) → #6292 (upstream exchange; 2026-07-28 dispatch dropped, separate ticket) → #6293 (verdict storage, ValidationOutcome in remotesessions) → this PR (probe + Verify, base #6293, head needs a force push) → #6295 (auto-verify on connect, pending card, shutdown drain). |
1742f91 to
887d5b4
Compare
bflad
left a comment
There was a problem hiding this comment.
This is the right shape now. The probe runs on the official SDK client, serve_meta_proxy.go is untouched so the dispatch path carries no risk from this PR, the hand-rolled revision handling is gone from the whole stack, and with it the _meta round trip that was corrupting large JSON numbers in tool arguments. probeProxyBuilder still strips the census interceptors, so a probe is still not counted as traffic. Splitting automatic verification out to #6295 also makes both halves reviewable.
Approving on trust rather than holding the stack, but there is one thing I would like fixed before this merges. It is new code rather than anything carried over.
rt.rejected is sticky across a leg dispatch never performs. memberRoundTripper.RoundTrip records rejected on any 401 or 403 from any request in the session, and classifyProbe checks rt.rejection() before every other case. The SDK's Connect opens with a SEP-2575 server/discover request, falls back to the legacy initialize handshake on any error from it, and that fallback is invisible to the verdict logic. So a member whose gateway or WAF answers 403 to an unrecognised method, followed by any later ListTools failure, is reported as rejected_by_member. The card then tells the person to reconnect a credential that is working. Scoping rejected to the legs dispatch also performs, or at minimum ignoring a rejection observed on server/discover, fixes this without touching the design.
Three smaller notes.
The "dry run of dispatch" claim is not true yet, and the verdict rests on it. The SDK client opens with server/discover declaring 2026-07-28 and falls back to initialize at 2025-11-25, while metaMemberUpstreamProtocolVersion pins dispatch at 2025-06-18 and always sends initialize. Under 2026-07-28 the initialize handshake, notifications/initialized, Mcp-Session-Id, and ping are all removed (SEP-2575 and SEP-2567), so a member that serves only that revision will answer server/discover and list its tools, read as valid, and then fail every tool call dispatch sends it. That is the feature asserting the opposite of the truth in precisely the case it exists to catch. It is near-hypothetical today given how new the revision is, and the SDK gives no way to pin the client version (protocolVersion on the connect options is unexported and marked for testing), so I am not asking you to solve it here. But the header comment should describe what the probe actually does rather than claim parity it does not have. I am filing the follow-up that makes the claim true by moving member dispatch onto the same client.
guardianPolicy.Client() is decorative here and the comment oversells it. Called with no options it returns &http.Client{Transport: roundTripper} and nothing else, so httpClient.Transport = rt discards the guarded dialer, TLS roots, otel transport and pooled connections it just built. The substance is fine, since proxymanager.go:317 puts the policy on the inner proxy and that is where the dial happens, but "The policy's client, with the member proxy as its transport" invites a future reader to believe guardian applies at that layer. &http.Client{Transport: rt} is equivalent, cheaper, and does not mislead.
Two items from the first review did not move. The verify limiter is still keyed on challengeState.ID, which idp_callback rotates, so the six per minute budget resets on every flow and the natural key is the client or session id. And ValidationTimeout is still hardcoded to 0 in start.go with no flag, while the sibling MemberCallTimeout has one, which matters because this handler blocks its POST for up to fifteen seconds.
2558772 to
e4f17da
Compare
|
@bflad Fixed the sticky discovery rejection in 2a2615f: server/discover 401/403 responses no longer affect auth classification, while initialize, initialized, and tools/list rejections remain sticky. Added real SDK/proxy regressions for discovery 403 followed by success, tools/list 500, or timeout, plus tools/list 403 coverage. Focused validation tests passed: 28/28. Left your review thread unresolved. |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2a2615f to
cf84526
Compare

AIM-204, part 4 of 5, stacked on #6293. The head branch is reset onto part 3 locally (
887d5b4ef2) and needs a force push before this diff reads correctly.routeMetaMember, standalone remote and tunneled endpoints via the proxy builders), session and census interceptors stripped.mcp.NewClient+StreamableClientTransportwhose HTTP client's RoundTripper runs each request throughserveProxyBackendon the member's proxy builder;DisableStandaloneSSE, no retries, 4 MiB response cap; a session minted before a body failure is closed from the RoundTripper.tools/list: 401/403 on connect or list →rejected_by_member; a tool list →valid; anything else →unknown(fixed phrases, nothing upstream quoted). Stored through part 3,unknownnever downgradesvalid.ValidationTimeout(15s) less a reserve (min 2s, a quarter) that floors the SDK's DELETE and the verdict write; six Verifys per minute per consent challenge.