refactor(gateway): trim attach grant implementation

obviyus · obviyus · commit 1241885db6ad · 2026-06-30T18:22:05.000-07:00
diff --git a/src/gateway/mcp-grant-store.test.ts b/src/gateway/mcp-grant-store.test.ts
@@ -9,7 +9,7 @@ import {
   sweepExpiredAttachGrants,
 } from "./mcp-grant-store.js";
 
-const T0 = 1_000_000_000_000; // fixed epoch for deterministic TTL tests
+const T0 = 1_000_000_000_000;
 
 describe("mcp-grant-store", () => {
   beforeEach(() => resetAttachGrantsForTest());
@@ -30,7 +30,6 @@ describe("mcp-grant-store", () => {
     const g = mintAttachGrant({ sessionKey: "agent:main:x", ttlMs: 1_000, nowMs: T0 });
     expect(resolveAttachGrant(g.token, T0)?.sessionKey).toBe("agent:main:x");
     expect(resolveAttachGrant(g.token, T0 + 999)?.sessionKey).toBe("agent:main:x");
-    // at/after expiry → undefined, and the lookup self-sweeps it
     expect(resolveAttachGrant(g.token, T0 + 1_000)).toBeUndefined();
     expect(resolveAttachGrant(g.token, T0 + 1_001)).toBeUndefined();
     expect(attachGrantStoreSize()).toBe(0);
@@ -43,7 +42,6 @@ describe("mcp-grant-store", () => {
   it("binds the sessionKey to the grant (token carries scope identity, not the caller)", () => {
     const a = mintAttachGrant({ sessionKey: "agent:main:telegram:1", nowMs: T0 });
     const b = mintAttachGrant({ sessionKey: "agent:main:telegram:2", nowMs: T0 });
-    // each token resolves only to its own bound session — the basis for header-spoof resistance
     expect(resolveAttachGrant(a.token, T0)?.sessionKey).toBe("agent:main:telegram:1");
     expect(resolveAttachGrant(b.token, T0)?.sessionKey).toBe("agent:main:telegram:2");
     expect(a.token).not.toBe(b.token);
@@ -83,8 +81,6 @@ describe("mcp-grant-store", () => {
   it("evicts expired grants on mint, bounding the store (no accumulation)", () => {
     mintAttachGrant({ sessionKey: "s", ttlMs: 1_000, nowMs: T0 });
     expect(attachGrantStoreSize()).toBe(1);
-    // a later mint past the first's expiry sweeps the stale entry before inserting — without
-    // sweep-on-mint the never-looked-up grant would linger and the size would be 2.
     mintAttachGrant({ sessionKey: "s", ttlMs: 1_000, nowMs: T0 + 5_000 });
     expect(attachGrantStoreSize()).toBe(1);
   });
diff --git a/src/gateway/mcp-grant-store.ts b/src/gateway/mcp-grant-store.ts
@@ -1,23 +1,3 @@
-/**
- * Per-session MCP loopback **attach grants**.
- *
- * The loopback MCP server (`mcp-http.ts`) authenticates the gateway-spawned cli-backend with two
- * process-global bearer tokens (owner / non-owner) and scopes tools from client-supplied headers —
- * adequate because that client is cooperative and gateway-launched. An **attach** grant is the
- * primitive for a *less-trusted* external/interactive harness (Claude Code, OpenCode, or a harness
- * reached via a node/companion app over its existing gateway connection): a short-lived, revocable
- * bearer whose **scope is bound to the grant**, not to the request headers.
- *
- * Security properties:
- * - The bound `sessionKey` comes from the grant, so an attach caller cannot scope-shop by setting
- *   `x-session-key` (the request layer ignores the header when a grant matches).
- * - Grants are always treated as **non-owner** (`senderIsOwner=false`) — the loopback surface
- *   already excludes the destructive native tools (`read/write/edit/apply_patch/exec/process`).
- * - TTL + explicit revoke bound the blast radius of a leaked token.
- *
- * Transport-independent: the same grant is presented whether the harness reaches the loopback over
- * 127.0.0.1 (gateway host) or tunnelled in over a node/app's existing authenticated channel.
- */
 import crypto from "node:crypto";
 
 export interface McpAttachGrant {
@@ -32,7 +12,7 @@ export interface McpAttachGrant {
 }
 
 const DEFAULT_TTL_MS = 60 * 60 * 1000; // 1h
-const MAX_TTL_MS = 12 * 60 * 60 * 1000; // hard ceiling so a caller can't request a forever-grant
+const MAX_TTL_MS = 12 * 60 * 60 * 1000;
 
 const grantsByToken = new Map<string, McpAttachGrant>();
 
@@ -43,7 +23,6 @@ function clampTtlMs(ttlMs: number | undefined): number {
   return Math.min(ttlMs as number, MAX_TTL_MS);
 }
 
-/** Mint a grant bound to `sessionKey`. Returns the grant (the caller hands `token` to the harness). */
 export function mintAttachGrant(params: {
   sessionKey: string;
   ttlMs?: number;
@@ -54,8 +33,7 @@ export function mintAttachGrant(params: {
     throw new Error("mintAttachGrant: sessionKey is required");
   }
   const nowMs = params.nowMs ?? Date.now();
-  // Sweep on mint so grants that are minted but never looked up again (harness never connects)
-  // don't accumulate — lookup self-sweeps only the token it touches, so this bounds the map.
+  // Mint sweeps stale entries so abandoned grants do not accumulate.
   sweepExpiredAttachGrants(nowMs);
   const grant: McpAttachGrant = {
     token: crypto.randomBytes(32).toString("hex"),
@@ -67,11 +45,6 @@ export function mintAttachGrant(params: {
   return grant;
 }
 
-/**
- * Resolve a bearer token to a live grant, or `undefined` if unknown/expired. An expired grant is
- * dropped on lookup (lazy sweep). Lookup is by full-token map key: a caller must already hold the
- * complete 256-bit token to get a hit, so there is no partial-match timing oracle to defend.
- */
 export function resolveAttachGrant(
   token: string,
   nowMs: number = Date.now(),
@@ -87,12 +60,10 @@ export function resolveAttachGrant(
   return grant;
 }
 
-/** Revoke a grant by token. Returns true if a grant was removed. */
 export function revokeAttachGrant(token: string): boolean {
   return grantsByToken.delete(token);
 }
 
-/** Revoke every live grant for a session (e.g. on session teardown). Returns the count removed. */
 export function revokeAttachGrantsForSession(sessionKey: string): number {
   const key = sessionKey.trim();
   let removed = 0;
@@ -105,7 +76,6 @@ export function revokeAttachGrantsForSession(sessionKey: string): number {
   return removed;
 }
 
-/** Drop expired grants. Returns the count swept. Call opportunistically; lookup also self-sweeps. */
 export function sweepExpiredAttachGrants(nowMs: number = Date.now()): number {
   let removed = 0;
   for (const [token, grant] of grantsByToken) {
@@ -117,12 +87,10 @@ export function sweepExpiredAttachGrants(nowMs: number = Date.now()): number {
   return removed;
 }
 
-/** Number of entries currently held (test/diagnostics). Does not sweep; reflects raw store size. */
 export function attachGrantStoreSize(): number {
   return grantsByToken.size;
 }
 
-/** Clear all grants (test isolation only). */
 export function resetAttachGrantsForTest(): void {
   grantsByToken.clear();
 }
diff --git a/src/gateway/mcp-http.request.ts b/src/gateway/mcp-http.request.ts
@@ -120,9 +120,6 @@ function resolveMcpSender(params: {
   if (ownerTokenMatched || nonOwnerTokenMatched) {
     return { senderIsOwner: ownerTokenMatched };
   }
-  // Attach grant: an external/interactive harness presents a per-session grant token (mcp-grant-store).
-  // Always non-owner, and its scope is bound to the grant's sessionKey so a grant holder cannot widen
-  // scope via the x-session-key header — resolveMcpRequestContext honors boundSessionKey instead.
   const grantToken = authHeader.startsWith("Bearer ") ? authHeader.slice("Bearer ".length) : "";
   const grant = grantToken ? resolveAttachGrant(grantToken) : undefined;
   if (grant) {
@@ -368,12 +365,8 @@ export function resolveMcpRequestContext(
   cfg: OpenClawConfig,
   auth: { senderIsOwner: boolean; boundSessionKey?: string },
 ): McpRequestContext {
-  // An attach grant is a lower-trust boundary: bind the session server-side AND ignore every
-  // caller-supplied delivery/action context header (message channel, account, current channel/
-  // thread/message, inbound-audio, event-kind, source-reply mode, explicit-target). Those headers
-  // feed scoped tools and the message tool, so a grant holder must not be able to spoof them —
-  // only the grant's pinned sessionKey is trusted. Owner/non-owner (the cooperative, gateway-
-  // launched cli-backend) keep header-driven context.
+  // Grant-authenticated callers get only their server-bound session; spoofable
+  // delivery/action headers stay reserved for the gateway-launched loopback client.
   if (auth.boundSessionKey) {
     return {
       sessionKey: auth.boundSessionKey,
diff --git a/src/gateway/mcp-http.test.ts b/src/gateway/mcp-http.test.ts
@@ -738,7 +738,6 @@ describe("mcp loopback server", () => {
       port: serverPort,
       token: grant.token,
       headers: jsonHeaders({
-        // spoof the full delivery/action context, not just the session key
         "x-session-key": "agent:main:SPOOFED-other-session",
         "x-openclaw-message-channel": "telegram",
         "x-openclaw-account-id": "victim-account",
@@ -752,12 +751,9 @@ describe("mcp loopback server", () => {
 
     expect(response.status).toBe(200);
     const call = getScopedToolsCall(0);
-    // session is grant-bound, NOT the spoofed header
     expect(call.sessionKey).toBe("agent:main:attach-host");
     expect(call.senderIsOwner).toBe(false);
     expect(call.surface).toBe("loopback");
-    // a grant is a lower-trust boundary: every other caller-supplied context header is ignored
-    // (fail-closed), so a grant holder cannot spoof delivery/action context into scoped tools.
     expect(call.messageProvider).toBeUndefined();
     expect(call.accountId).toBeUndefined();
     expect(call.currentChannelId).toBeUndefined();
diff --git a/src/gateway/methods/core-descriptors.ts b/src/gateway/methods/core-descriptors.ts
@@ -218,9 +218,6 @@ export const CORE_GATEWAY_METHOD_SPECS: readonly CoreGatewayMethodSpec[] = [
   { name: "poll", scope: "operator.write", advertise: false },
   { name: "sessions.steer", scope: "operator.write", advertise: false },
   { name: "push.test", scope: "operator.write", advertise: false },
-  // Appended at the end of the advertised methods to preserve the legacy advertised order
-  // (server-methods-list.test.ts locks the prefix/middle). grant mints process-global state so it
-  // is control-plane rate-limited; revoke is intentionally not (cheap, idempotent, frees memory).
   { name: "attach.grant", scope: "operator.admin", controlPlaneWrite: true },
   { name: "attach.revoke", scope: "operator.admin" },
   { name: "push.web.vapidPublicKey", scope: "operator.write", advertise: false },
diff --git a/src/gateway/server-methods/attach.test.ts b/src/gateway/server-methods/attach.test.ts
@@ -37,7 +37,6 @@ describe("attach gateway methods", () => {
     expect(body.mcpConfig).toBeTruthy();
     expect(body.env.OPENCLAW_MCP_TOKEN).toBe(body.token);
     expect(body.env.OPENCLAW_MCP_SESSION_KEY).toBe("agent:main:attach-method");
-    // the minted token resolves to the bound session in the shared store
     expect(resolveAttachGrant(body.token)?.sessionKey).toBe("agent:main:attach-method");
   });
 
@@ -78,12 +77,12 @@ describe("attach gateway methods", () => {
 
     const r2 = vi.fn();
     await attachHandlers["attach.grant"]({
-      params: { sessionKey: "agent:main:ttl2", ttlMs: -5 }, // non-positive → default ttl
+      params: { sessionKey: "agent:main:ttl2", ttlMs: -5 },
       respond: r2,
       context: { getRuntimeConfig: () => ({}) },
     } as unknown as GatewayRequestHandlerOptions);
     const b2 = r2.mock.calls[0][1] as { expiresAtMs: number };
-    expect(b2.expiresAtMs).toBeGreaterThan(Date.now() + 50 * 60_000); // ~1h default window
+    expect(b2.expiresAtMs).toBeGreaterThan(Date.now() + 50 * 60_000);
   });
 
   it("attach.revoke treats non-object params as a missing token (INVALID_REQUEST)", async () => {
diff --git a/src/gateway/server-methods/attach.ts b/src/gateway/server-methods/attach.ts
@@ -1,5 +1,3 @@
-// Gateway RPC handlers for attach grants: mint a per-session, scoped, revocable MCP loopback grant
-// so an external/interactive harness can reach the gateway's scoped tools, and revoke it on detach.
 import { ErrorCodes, errorShape } from "../../../packages/gateway-protocol/src/index.js";
 import { resolveMainSessionKey } from "../../config/sessions.js";
 import { mintAttachGrant, revokeAttachGrant } from "../mcp-grant-store.js";
@@ -14,20 +12,19 @@ function paramRecord(params: unknown): Record<string, unknown> {
   return params && typeof params === "object" ? (params as Record<string, unknown>) : {};
 }
 
-function readString(params: unknown, key: string): string | undefined {
-  const value = paramRecord(params)[key];
+function readString(params: Record<string, unknown>, key: string): string | undefined {
+  const value = params[key];
   return typeof value === "string" && value.trim() ? value.trim() : undefined;
 }
 
-function readPositiveNumber(params: unknown, key: string): number | undefined {
-  const value = paramRecord(params)[key];
+function readPositiveNumber(params: Record<string, unknown>, key: string): number | undefined {
+  const value = params[key];
   return typeof value === "number" && Number.isFinite(value) && value > 0 ? value : undefined;
 }
 
 export const attachHandlers: GatewayRequestHandlers = {
-  // Mint a grant bound to a session, returning the loopback MCP config + the token env the harness
-  // needs. ensureMcpLoopbackServer lazily brings the singleton up if no cli-backend turn started it.
   "attach.grant": async ({ params, respond, context }) => {
+    const grantParams = paramRecord(params);
     await ensureMcpLoopbackServer();
     const runtime = getActiveMcpLoopbackRuntime();
     if (!runtime) {
@@ -39,24 +36,21 @@ export const attachHandlers: GatewayRequestHandlers = {
       return;
     }
     const sessionKey =
-      readString(params, "sessionKey") ?? resolveMainSessionKey(context.getRuntimeConfig());
-    const grant = mintAttachGrant({ sessionKey, ttlMs: readPositiveNumber(params, "ttlMs") });
+      readString(grantParams, "sessionKey") ?? resolveMainSessionKey(context.getRuntimeConfig());
+    const grant = mintAttachGrant({ sessionKey, ttlMs: readPositiveNumber(grantParams, "ttlMs") });
     respond(true, {
       sessionKey: grant.sessionKey,
       token: grant.token,
       expiresAtMs: grant.expiresAtMs,
-      // The harness writes mcpConfig to its MCP client config and sets env so the ${...} placeholders
-      // resolve. Loopback today; node/app conduits reuse the same client config over their channel.
       mcpConfig: createMcpLoopbackServerConfig(runtime.port),
       env: {
         OPENCLAW_MCP_TOKEN: grant.token,
         OPENCLAW_MCP_SESSION_KEY: grant.sessionKey,
       },
     });
   },
-  // Revoke a previously minted grant. Idempotent: an unknown/already-expired token reports revoked=false.
   "attach.revoke": async ({ params, respond }) => {
-    const token = readString(params, "token");
+    const token = readString(paramRecord(params), "token");
     if (!token) {
       respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "token is required"));
       return;
