From d08c258ff1a88c1ba3bedccf267de1fe064aa14a Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 10 Sep 2026 14:52:26 -0400 Subject: [PATCH] fix(mobile): fail closed on escaped RPC forwarders Reject partial caller families when sender bindings escape or alias calls are not enumerated. Inventory unsubscribe frame constructions and sender calls as a separate kind, and pin the failure modes with mutation-checked regressions. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- mobile/rpc-foundation/access-inventory.json | 29 +++++-- mobile/scripts/rpc-access-inventory.mts | 24 ++++-- mobile/scripts/rpc-access-resolution.mts | 82 +++++++++++++++--- mobile/scripts/rpc-artifact-io.mts | 6 +- .../transport/rpc-access-resolution.test.ts | 85 ++++++++++++++++++- 5 files changed, 196 insertions(+), 30 deletions(-) diff --git a/mobile/rpc-foundation/access-inventory.json b/mobile/rpc-foundation/access-inventory.json index 8dbdb5c56cf..6f50bd5f710 100644 --- a/mobile/rpc-foundation/access-inventory.json +++ b/mobile/rpc-foundation/access-inventory.json @@ -2,13 +2,13 @@ "schemaVersion": 2, "scope": ["mobile/src", "mobile/app"], "summary": { - "callSites": 431, - "literalMethod": 387, - "resolvedFamily": 26, - "unresolvedDynamic": 18, - "referenceOccurrences": 1183, + "callSites": 442, + "literalMethod": 395, + "resolvedFamily": 23, + "unresolvedDynamic": 24, + "referenceOccurrences": 1184, "referenceFileCount": 167, - "note": "calls are `file:line:column kind method`, where method is one literal, a `|`-joined family the checker resolved, or `dynamic:` when it resolved nothing. References are bare occurrences of the `sendRequest`/`subscribe` token with no call attached; only their count and files are recorded." + "note": "calls are `file:line:column kind method`, where method is one literal, a `|`-joined family the checker resolved, or `dynamic:` when completeness cannot be proven. Kinds: request = sendRequest calls; subscribe = subscribe calls (including local listeners); unsubscribe = sendUnsubscribe calls and literal *.unsubscribe method fields in frame constructions, including test fixtures, without a reachability claim. References are bare occurrences of the `sendRequest`/`subscribe`/`sendUnsubscribe` token with no call attached; only their count and files are recorded." }, "calls": [ "mobile/app/connection-log.tsx:89:20 subscribe dynamic:connectionLogStore.subscribe", @@ -31,7 +31,7 @@ "mobile/src/browser/use-mobile-browser-commands.ts:137:17 request browser.mouseMove", "mobile/src/browser/use-mobile-browser-commands.ts:141:17 request browser.mouseDown", "mobile/src/browser/use-mobile-browser-commands.ts:145:17 request browser.mouseUp", - "mobile/src/browser/use-mobile-browser-request.ts:41:32 request browser.back|browser.forward|browser.goto|browser.reload", + "mobile/src/browser/use-mobile-browser-request.ts:41:32 request dynamic:client.sendRequest", "mobile/src/browser/use-mobile-browser-stream.ts:229:25 subscribe browser.screencast", "mobile/src/components/codex-reset-credit-capability.ts:14:28 request status.get", "mobile/src/components/codex-reset-credit.ts:228:26 request accounts.consumeCodexResetCredit", @@ -118,7 +118,7 @@ "mobile/src/session/mobile-structured-agent-session-launch.ts:85:31 request agentSession.createSupport", "mobile/src/session/mobile-structured-agent-session-launch.ts:123:22 request agentSession.create", "mobile/src/session/mobile-structured-agent-session-launch.ts:130:24 request agentSession.create", - "mobile/src/session/mobile-structured-agent-session-rpc.ts:41:26 request agentSession.cancel|agentSession.conversationCommand|agentSession.history|agentSession.hold|agentSession.options|agentSession.release|agentSession.respondToApproval|agentSession.respondToQuestion|agentSession.send|agentSession.setOption", + "mobile/src/session/mobile-structured-agent-session-rpc.ts:41:26 request dynamic:client.sendRequest", "mobile/src/session/mobile-terminal-stream-subscribe.ts:10:12 subscribe terminal.subscribe", "mobile/src/session/mobile-worker-takeover-send-sites.test.ts:178:25 request terminal.send", "mobile/src/session/pr-ai-triage-launch.ts:18:25 request session.tabs.createTerminal", @@ -192,7 +192,7 @@ "mobile/src/source-control/mobile-pr-link.ts:121:28 request worktree.show", "mobile/src/source-control/MobileGitHistoryList.tsx:108:10 request git.commitCompare", "mobile/src/source-control/reveal-mobile-source-control-session-diff.ts:64:28 request session.tabs.list", - "mobile/src/source-control/use-mobile-git-requests.ts:25:30 request git.commit|git.fetch|git.pull|git.push|git.status|git.upstreamStatus", + "mobile/src/source-control/use-mobile-git-requests.ts:25:30 request dynamic:client.sendRequest", "mobile/src/source-control/use-mobile-source-control-loaders.ts:120:32 request git.branchCompare", "mobile/src/source-control/use-mobile-source-control-loaders.ts:198:36 request git.status", "mobile/src/source-control/use-mobile-source-control-openers.ts:114:30 request files.openDiff", @@ -346,10 +346,12 @@ "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:145:20 subscribe dynamic:streams.subscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:165:22 subscribe session.tabs.subscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:170:7 subscribe session.tabs.subscribe", + "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:182:9 unsubscribe session.tabs.unsubscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:202:27 subscribe dynamic:streams.subscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:203:27 subscribe dynamic:streams.subscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:229:25 subscribe nativeChat.subscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:232:5 subscribe nativeChat.subscribe", + "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:237:7 unsubscribe nativeChat.unsubscribe", "mobile/src/transport/mobile-relay-rpc-stream-cancellation.test.ts:246:22 subscribe runtime.clientEvents.subscribe", "mobile/src/transport/mobile-relay-rpc-streams.test.ts:23:20 subscribe session.tabs.subscribe", "mobile/src/transport/mobile-relay-rpc-streams.test.ts:50:20 subscribe session.tabs.subscribe", @@ -357,6 +359,10 @@ "mobile/src/transport/mobile-relay-rpc-streams.test.ts:103:20 subscribe session.tabs.subscribe", "mobile/src/transport/mobile-relay-rpc-streams.test.ts:126:5 subscribe session.tabs.subscribe", "mobile/src/transport/mobile-relay-rpc-streams.test.ts:145:5 subscribe session.tabs.subscribe", + "mobile/src/transport/mobile-relay-rpc-streams.ts:41:34 unsubscribe terminal.unsubscribe", + "mobile/src/transport/mobile-relay-rpc-streams.ts:196:11 unsubscribe dynamic:this.sendUnsubscribe", + "mobile/src/transport/mobile-relay-rpc-streams.ts:206:11 unsubscribe dynamic:this.sendUnsubscribe", + "mobile/src/transport/mobile-relay-rpc-streams.ts:214:11 unsubscribe dynamic:this.sendUnsubscribe", "mobile/src/transport/mobile-runtime-capability-negotiation.ts:21:22 request runtime.clientCapabilities.update", "mobile/src/transport/pairing-candidate-race.ts:19:12 request status.get", "mobile/src/transport/pairing-relay-candidate.test.ts:88:18 request status.get", @@ -397,10 +403,15 @@ "mobile/src/transport/rpc-client-request-deadline.test.ts:161:21 request terminal.send", "mobile/src/transport/rpc-client-runtime-events.test.ts:112:25 subscribe runtime.clientEvents.subscribe", "mobile/src/transport/rpc-client-runtime-events.test.ts:127:25 subscribe runtime.clientEvents.subscribe", + "mobile/src/transport/rpc-client-server-subscription.ts:6:14 unsubscribe browser.screencast.unsubscribe", + "mobile/src/transport/rpc-client-server-subscription.ts:9:14 unsubscribe runtime.clientEvents.unsubscribe", "mobile/src/transport/rpc-client-stream-registry.test.ts:57:5 subscribe terminal.subscribe", "mobile/src/transport/rpc-client-stream-registry.test.ts:84:21 subscribe browser.screencast", + "mobile/src/transport/rpc-client-stream-registry.test.ts:97:7 unsubscribe browser.screencast.unsubscribe", "mobile/src/transport/rpc-client-terminal-reconnect.test.ts:133:5 subscribe terminal.subscribe", "mobile/src/transport/rpc-client-terminal-reconnect.test.ts:139:21 request status.get", + "mobile/src/transport/rpc-client-terminal-subscription.ts:51:11 unsubscribe session.tabs.unsubscribe", + "mobile/src/transport/rpc-client-terminal-subscription.ts:59:16 unsubscribe nativeChat.unsubscribe", "mobile/src/transport/rpc-client.test.ts:194:25 subscribe session.tabs.subscribe", "mobile/src/transport/rpc-client.test.ts:212:9 subscribe notifications.subscribe", "mobile/src/transport/rpc-client.test.ts:235:25 subscribe browser.screencast", diff --git a/mobile/scripts/rpc-access-inventory.mts b/mobile/scripts/rpc-access-inventory.mts index fe1acf3c415..f3a15ce9ae2 100644 --- a/mobile/scripts/rpc-access-inventory.mts +++ b/mobile/scripts/rpc-access-inventory.mts @@ -1,10 +1,13 @@ import { join } from 'node:path' import ts from 'typescript' -import { createRpcAccessResolver } from './rpc-access-resolution.mts' +import { + createRpcAccessResolver, + unsubscribeFrameMethod, + type RpcAccessKind +} from './rpc-access-resolution.mts' import { emit, files, root } from './rpc-artifact-io.mts' -type AccessKind = 'request' | 'subscribe' -type CallSite = { file: string; line: number; column: number; kind: AccessKind; method: string } +type CallSite = { file: string; line: number; column: number; kind: RpcAccessKind; method: string } const paths = [...files(join(root, 'mobile/src')), ...files(join(root, 'mobile/app'))].filter( (file) => /\.[jt]sx?$/.test(file) ) @@ -24,7 +27,7 @@ const { entryKind, resolveKind, methods } = createRpcAccessResolver( for (const file of paths) { const sf = program.getSourceFile(join(root, file))! const positions = new Set() - function add(node: ts.Node, kind: AccessKind, call?: ts.CallExpression): void { + function add(node: ts.Node, kind: RpcAccessKind, call?: ts.CallExpression): void { const start = node.getStart(sf) if (positions.has(start)) { return @@ -50,6 +53,17 @@ for (const file of paths) { }) } function visit(node: ts.Node): void { + const unsubscribe = unsubscribeFrameMethod(node) + if (unsubscribe) { + const location = sf.getLineAndCharacterOfPosition(node.getStart(sf)) + calls.push({ + file, + line: location.line + 1, + column: location.character + 1, + kind: 'unsubscribe', + method: unsubscribe + }) + } if (ts.isCallExpression(node)) { const kind = resolveKind(node.expression) if (kind) { @@ -88,7 +102,7 @@ await emit('access-inventory', { unresolvedDynamic: calls.filter((call) => call.method.startsWith('dynamic:')).length, referenceOccurrences: referenceCount, referenceFileCount: referenceFiles.size, - note: 'calls are `file:line:column kind method`, where method is one literal, a `|`-joined family the checker resolved, or `dynamic:` when it resolved nothing. References are bare occurrences of the `sendRequest`/`subscribe` token with no call attached; only their count and files are recorded.' + note: 'calls are `file:line:column kind method`, where method is one literal, a `|`-joined family the checker resolved, or `dynamic:` when completeness cannot be proven. Kinds: request = sendRequest calls; subscribe = subscribe calls (including local listeners); unsubscribe = sendUnsubscribe calls and literal *.unsubscribe method fields in frame constructions, including test fixtures, without a reachability claim. References are bare occurrences of the `sendRequest`/`subscribe`/`sendUnsubscribe` token with no call attached; only their count and files are recorded.' }, calls: rows, referenceFiles: [...referenceFiles].sort() diff --git a/mobile/scripts/rpc-access-resolution.mts b/mobile/scripts/rpc-access-resolution.mts index f870f7a9e7e..c4cf3cf7ccf 100644 --- a/mobile/scripts/rpc-access-resolution.mts +++ b/mobile/scripts/rpc-access-resolution.mts @@ -1,9 +1,25 @@ import ts from 'typescript' -export type RpcAccessKind = 'request' | 'subscribe' +export type RpcAccessKind = 'request' | 'subscribe' | 'unsubscribe' +export function unsubscribeFrameMethod(node: ts.Node): string | undefined { + if ( + ts.isPropertyAssignment(node) && + node.name.getText().replace(/['"]/g, '') === 'method' && + ts.isStringLiteralLike(node.initializer) && + node.initializer.text.endsWith('.unsubscribe') + ) { + return node.initializer.text + } +} export function createRpcAccessResolver(program: ts.Program, paths: string[]) { const checker = program.getTypeChecker() function entryKind(name: string | undefined): RpcAccessKind | undefined { - return name === 'sendRequest' ? 'request' : name === 'subscribe' ? 'subscribe' : undefined + return name === 'sendRequest' + ? 'request' + : name === 'subscribe' + ? 'subscribe' + : name === 'sendUnsubscribe' + ? 'unsubscribe' + : undefined } function resolveKind(node: ts.Node, seen = new Set()): RpcAccessKind | undefined { if (seen.has(node)) { @@ -55,6 +71,7 @@ export function createRpcAccessResolver(program: ts.Program, paths: string[]) { } } const calls: ts.CallExpression[] = [] + const references = new Map() /** Symbol of the binding a function value is stored in, mapped to every function-type alias it * is contextually assigned to. A call through such an alias resolves to the alias signature, * not to the function, so without this the caller set is silently partial. */ @@ -73,6 +90,19 @@ export function createRpcAccessResolver(program: ts.Program, paths: string[]) { if (ts.isCallExpression(node)) { calls.push(node) } + if (ts.isIdentifier(node)) { + let symbol = ts.isShorthandPropertyAssignment(node.parent) + ? checker.getShorthandAssignmentValueSymbol(node.parent) + : checker.getSymbolAtLocation(node) + if (symbol && symbol.flags & ts.SymbolFlags.Alias) { + symbol = checker.getAliasedSymbol(symbol) + } + if (symbol) { + const uses = references.get(symbol) ?? [] + uses.push(node) + references.set(symbol, uses) + } + } if (ts.isShorthandPropertyAssignment(node)) { // The symbol at a shorthand name is the object's property, not the value it carries. recordAliasCarrier( @@ -105,18 +135,50 @@ export function createRpcAccessResolver(program: ts.Program, paths: string[]) { ? checker.getSymbolAtLocation(owner.name) : undefined } - function callers(owner: ts.SignatureDeclaration): ts.CallExpression[] { - const direct = calls.filter((call) => checker.getResolvedSignature(call)?.declaration === owner) - const binding = ownerBinding(owner) - const aliases = binding && aliasCarriers.get(binding) - if (!aliases?.size) { - return direct + function escapes( + binding: ts.Symbol, + enumerated: Set, + seen = new Set() + ): boolean { + if (seen.has(binding)) { + return false } + seen.add(binding) + return (references.get(binding) ?? []).some((use) => { + const parent = use.parent + if ( + (ts.isVariableDeclaration(parent) || ts.isFunctionDeclaration(parent)) && + parent.name === use + ) { + return false + } + if (ts.isImportSpecifier(parent)) { + return false + } + if (ts.isCallExpression(parent) && parent.expression === use) { + return !enumerated.has(parent) + } + if (ts.isVariableDeclaration(parent) && parent.initializer === use) { + const alias = checker.getSymbolAtLocation(parent.name) + return !alias || escapes(alias, enumerated, seen) + } + // Only direct calls and fully enumerated local aliases prove a complete caller set. + return true + }) + } + function callers(owner: ts.SignatureDeclaration): ts.CallExpression[] { + const binding = ownerBinding(owner) + if (!binding) { + return [] + } + const direct = calls.filter((call) => checker.getResolvedSignature(call)?.declaration === owner) + const aliases = binding && aliasCarriers.get(binding) const aliased = calls.filter((call) => { const declaration = checker.getResolvedSignature(call)?.declaration - return Boolean(declaration?.parent && aliases.has(declaration.parent)) + return Boolean(declaration?.parent && aliases?.has(declaration.parent)) }) - return [...direct, ...aliased.filter((call) => !direct.includes(call))] + const enumerated = new Set([...direct, ...aliased]) + return escapes(binding, enumerated) ? [] : [...enumerated] } function methods(node: ts.Expression | undefined, seen = new Set()): string[] { if (!node) { diff --git a/mobile/scripts/rpc-artifact-io.mts b/mobile/scripts/rpc-artifact-io.mts index 5a4f5c841f1..742fec7617b 100644 --- a/mobile/scripts/rpc-artifact-io.mts +++ b/mobile/scripts/rpc-artifact-io.mts @@ -39,9 +39,9 @@ export async function emit(name: string, value: unknown): Promise { writeFileSync(path, `${JSON.stringify(value, null, 2)}\n`) // Committed artifacts are checked by `format:check`; format here so regeneration cannot leave CI red. const formatted = await runProcess({ - program: resolve(root, 'node_modules/.bin/oxfmt'), - args: ['--write', path], - cwd: root + program: 'pnpm', + args: ['exec', 'oxfmt', '--write', path], + cwd: resolve(root, 'mobile') }) if (formatted.code !== 0) { throw new Error(`oxfmt --write ${path}: ${formatted.stderr}`) diff --git a/mobile/src/transport/rpc-access-resolution.test.ts b/mobile/src/transport/rpc-access-resolution.test.ts index 66f6fc31220..d1461cd15a3 100644 --- a/mobile/src/transport/rpc-access-resolution.test.ts +++ b/mobile/src/transport/rpc-access-resolution.test.ts @@ -1,6 +1,9 @@ import ts from 'typescript' import { describe, expect, it } from 'vitest' -import { createRpcAccessResolver } from '../../scripts/rpc-access-resolution.mts' +import { + createRpcAccessResolver, + unsubscribeFrameMethod +} from '../../scripts/rpc-access-resolution.mts' function fixture(source: string) { const file = 'rpc-fixture.ts' @@ -64,7 +67,7 @@ describe('RPC access symbol resolution', () => { const raw = resolver.calls.find((call) => resolver.resolveKind(call.expression) === 'request')! expect(resolver.methods(raw.arguments[0])).toEqual(['hidden.viaTypeAlias', 'resolved.direct']) }) - it('carries the alias through a shorthand property handed to another function', () => { + it('fails closed for a property handed to another function', () => { const resolver = fixture(` declare const client: { sendRequest(method: string): void } type RunHook = (method: string) => void @@ -76,7 +79,7 @@ describe('RPC access symbol resolution', () => { args.run('hidden.viaShorthand') `) const raw = resolver.calls.find((call) => resolver.resolveKind(call.expression) === 'request')! - expect(resolver.methods(raw.arguments[0])).toEqual(['hidden.viaShorthand', 'resolved.direct']) + expect(resolver.methods(raw.arguments[0])).toEqual([]) }) it('does not hide an unresolved caller behind an object binding', () => { const resolver = fixture(` @@ -88,4 +91,80 @@ describe('RPC access symbol resolution', () => { const raw = resolver.calls.find((call) => resolver.resolveKind(call.expression) === 'request')! expect(resolver.methods(raw.arguments[0])).toEqual([]) }) + it.each([ + 'return { send }', + 'consume(send); return { send }', + 'target.send = send; return target', + 'const alias = send; return { send: alias }' + ])('fails closed across a destructured hook result: %s', (escape) => { + const resolver = fixture(` + declare const client: { sendRequest(method: string): void } + declare function consume(send: (method: string) => void): void + declare const target: { send: (method: string) => void } + function useFoo() { + const send = (method: string) => { client.sendRequest(method) } + send('git.status') + ${escape} + } + const { send: x } = useFoo() + x('git.checkout') + `) + const raw = resolver.calls.find((call) => resolver.resolveKind(call.expression) === 'request')! + expect(resolver.methods(raw.arguments[0])).toEqual([]) + }) + it('fails closed when an alias call is absent from the enumerated signatures', () => { + const resolver = fixture(` + declare const client: { sendRequest(method: string): void } + type RunHook = (method: string) => void + const send = (method: string) => { client.sendRequest(method) } + send('git.status') + const alias = send + const run: RunHook = alias + run('git.checkout') + `) + const raw = resolver.calls.find((call) => resolver.resolveKind(call.expression) === 'request')! + expect(resolver.methods(raw.arguments[0])).toEqual([]) + }) + it('records unsubscribe frame methods separately from requests and subscriptions', () => { + const source = ts.createSourceFile( + 'frames.ts', + ` + const frames = [ + { method: 'terminal.unsubscribe' }, + { method: 'session.tabs.unsubscribe' }, + { method: 'nativeChat.unsubscribe' }, + { method: 'browser.screencast.unsubscribe' }, + { method: 'runtime.clientEvents.unsubscribe' }, + { method: 'terminal.subscribe' }, + { label: 'ignored.unsubscribe' } + ] + const dead = new Set(['github.prComments', 'ignored.unsubscribe']) + `, + ts.ScriptTarget.Latest, + true + ) + const methods: string[] = [] + function visit(node: ts.Node): void { + const method = unsubscribeFrameMethod(node) + if (method) { + methods.push(method) + } + ts.forEachChild(node, visit) + } + visit(source) + expect(methods).toEqual([ + 'terminal.unsubscribe', + 'session.tabs.unsubscribe', + 'nativeChat.unsubscribe', + 'browser.screencast.unsubscribe', + 'runtime.clientEvents.unsubscribe' + ]) + const resolver = fixture(` + declare const streams: { sendUnsubscribe(frame: unknown): void } + streams.sendUnsubscribe({ method: unknownMethod }) + `) + expect(resolver.calls.map((call) => resolver.resolveKind(call.expression))).toEqual([ + 'unsubscribe' + ]) + }) })