From e68e548a3d4548236688c1f3cd4db64e056c91cd Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:50:59 +0800 Subject: [PATCH] fix(cli): validate plist and dylib-root options with their contract fields (#10557) * fix(cli): validate plist and dylib-root options with their contract fields inspect-plist --relative-path and trace-dylib-resolution --root were declared as bare strings on the CLI. --schema under-advertised them, and an empty or NUL value reached the operation contract, whose refusal named the contract field (path, roots) instead of the CLI option. Declare both options with the contract's own field schemas (localPathStringSchema; dylibResolutionInputSchema.shape.roots), so the CLI refuses them first and names --relative-path or --root. Fixes #9764 Fixes #9823 * fix(mcp): observe SDK initiated cleanup failures --- src/android/JadxSession.ts | 7 ++- src/cli/artifactCommands.ts | 10 ++-- src/cli/utilityCommands.ts | 3 +- src/mcpDoctor.ts | 3 +- src/process/OwnedMcpClient.ts | 12 +++++ tests/boundary/cli/dylibResolutionCli.test.ts | 29 ++++++++++- .../cli/emptyLocalTargetAdmission.test.ts | 48 +++++++++++++++++++ 7 files changed, 100 insertions(+), 12 deletions(-) create mode 100644 src/process/OwnedMcpClient.ts diff --git a/src/android/JadxSession.ts b/src/android/JadxSession.ts index 08c1e94ac1..dbaf9c5cea 100644 --- a/src/android/JadxSession.ts +++ b/src/android/JadxSession.ts @@ -1,5 +1,5 @@ import { throwIfJadxInterrupted } from "./JadxInterruption.js"; -import { Client } from "@modelcontextprotocol/client"; +import { OwnedMcpClient } from "../process/OwnedMcpClient.js"; import { createAnalysisExecution } from "../application/AnalysisProvider.js"; import type { AndroidAnalysisExecution } from "../application/android/AndroidAnalysisPort.js"; import type { AndroidRequest } from "../domain/android/androidAnalysis.js"; @@ -64,7 +64,10 @@ type JadxCallObservation = { /** One owned engine connection; its wire protocol stays inside the Android adapter. */ export class JadxSession { readonly transport: JadxMcpTransport; - readonly #client = new Client({ name: "rea-android-adapter", version: "1" }); + readonly #client = new OwnedMcpClient({ + name: "rea-android-adapter", + version: "1", + }); #loaded: ReturnType | undefined; #connected = false; #partialContext: diff --git a/src/cli/artifactCommands.ts b/src/cli/artifactCommands.ts index ec98f5004d..e84ef66c96 100644 --- a/src/cli/artifactCommands.ts +++ b/src/cli/artifactCommands.ts @@ -13,6 +13,7 @@ import { withCommandCancellation } from "./commandCancellation.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { ARTIFACT_GRAPH_PROVIDER } from "../domain/investigationProviders.js"; import { keyedArchiveInputSchema } from "../domain/apple/keyedArchive.js"; +import { dylibResolutionInputSchema } from "../domain/apple/dylibResolution.js"; import { localPathStringSchema } from "../domain/localPath.js"; import { z } from "incur"; import { cliDecimalField } from "./options.js"; @@ -109,12 +110,9 @@ const registerDylibResolutionCommand = ( ), }), options: z.object({ - root: z - .array(z.string()) - .optional() - .describe( - "Process root relative to the bundle or the Mach-O's directory (repeatable)", - ), + root: dylibResolutionInputSchema.shape.roots.describe( + "Process root relative to the bundle or the Mach-O's directory (repeatable)", + ), architecture: z .enum(["arm64", "arm64e", "x86_64"]) .optional() diff --git a/src/cli/utilityCommands.ts b/src/cli/utilityCommands.ts index 244ce3c768..0b9e3ec257 100644 --- a/src/cli/utilityCommands.ts +++ b/src/cli/utilityCommands.ts @@ -229,8 +229,7 @@ const registerNativeCommands = ( path: localPathStringSchema.describe("App or Mach-O path"), }), options: z.object({ - relativePath: z - .string() + relativePath: localPathStringSchema .optional() .describe( "Plist path relative to the app root, or an absolute path (default: the bundle's Info.plist)", diff --git a/src/mcpDoctor.ts b/src/mcpDoctor.ts index 4113e4577d..5a7397723b 100644 --- a/src/mcpDoctor.ts +++ b/src/mcpDoctor.ts @@ -1,4 +1,5 @@ import { Client, SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; +import { OwnedMcpClient } from "./process/OwnedMcpClient.js"; import { Help } from "incur"; import { performance } from "node:perf_hooks"; import { resolve } from "node:path"; @@ -135,7 +136,7 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { }, stderr.append, ); - const client = new Client({ name: "rea-mcp-doctor", version: "1" }); + const client = new OwnedMcpClient({ name: "rea-mcp-doctor", version: "1" }); let result: ProductionMcpDoctorResult; let cleanupFailure: { readonly cause: unknown } | undefined; try { diff --git a/src/process/OwnedMcpClient.ts b/src/process/OwnedMcpClient.ts new file mode 100644 index 0000000000..7df1ea23d8 --- /dev/null +++ b/src/process/OwnedMcpClient.ts @@ -0,0 +1,12 @@ +import { Client } from "@modelcontextprotocol/client"; + +/** Observe SDK-initiated close failures while keeping them available to callers. */ +export class OwnedMcpClient extends Client { + override close(): Promise { + const closing = super.close(); + // The SDK starts close without awaiting it after a legacy handshake fails. + // The transport owner still receives the original failure when it joins cleanup. + void closing.catch(() => undefined); + return closing; + } +} diff --git a/tests/boundary/cli/dylibResolutionCli.test.ts b/tests/boundary/cli/dylibResolutionCli.test.ts index a06e5231e3..47ad439874 100644 --- a/tests/boundary/cli/dylibResolutionCli.test.ts +++ b/tests/boundary/cli/dylibResolutionCli.test.ts @@ -66,6 +66,33 @@ const expectRootInputError = async ( }); }; +/** The CLI option shares the contract's root format, so it refuses an escaping root itself and names the option. */ +const expectEscapingRootRefusedByCli = async ( + cli: TestCli, + app: string, +): Promise => { + const escaping = await cli.run({ + arguments: [ + "trace-dylib-resolution", + app, + "--root", + "../outside", + "--json", + ], + environment: ENVIRONMENT, + }); + expect(escaping.exitCode).toBe(1); + expect(escaping.json).toMatchObject({ + code: "VALIDATION_ERROR", + fieldErrors: [ + expect.objectContaining({ + path: expect.stringMatching(/^root/u), + message: expect.stringContaining("Use a normalized path"), + }), + ], + }); +}; + const fixtureApp = async (): Promise => { const app = join( await createTestTempDirectory("rea-dylib-cli-"), @@ -534,8 +561,8 @@ describe("trace-dylib-resolution CLI", () => { const outside = await createTestTempDirectory("rea-dylib-outside-"); await writeFiles(outside, { Tool: machoImage({}) }); await symlink(outside, join(app, "Contents/Linked"), "dir"); + await expectEscapingRootRefusedByCli(cli, app); for (const [root, reason, message] of [ - ["../outside", "invalid_format", "Use a normalized path"], ["Contents/Info.plist", "invalid_value", "is not a Mach-O image"], [ "Contents/Linked/Tool", diff --git a/tests/boundary/cli/emptyLocalTargetAdmission.test.ts b/tests/boundary/cli/emptyLocalTargetAdmission.test.ts index f2ff4e58d1..84dfc79c17 100644 --- a/tests/boundary/cli/emptyLocalTargetAdmission.test.ts +++ b/tests/boundary/cli/emptyLocalTargetAdmission.test.ts @@ -1,4 +1,5 @@ import { Cli } from "incur"; +import { Ajv2020 } from "ajv/dist/2020.js"; import { expect, it, vi } from "vitest"; import { registerArtifactCommands } from "../../../src/cli/artifactCommands.js"; import { registerCoreBinaryCommands } from "../../../src/cli/coreBinaryCommands.js"; @@ -132,6 +133,32 @@ it.each([ }, ); +it.each([ + { + command: "inspect-plist", + field: "relativePath", + valid: "Contents/Info.plist", + invalid: ["", "Contents/a\x00b"], + }, + { + command: "trace-dylib-resolution", + field: "root", + valid: ["Contents/MacOS/app"], + invalid: [[""], ["Contents/a\x00b"], ["../outside"]], + }, +])( + "advertises the $field option constraints", + async ({ command, field, valid, invalid }) => { + const { serve } = registry(); + const schema = (await serve([command, "--schema"])) as { options: object }; + const validate = new Ajv2020({ strict: false }).compile(schema.options); + expect(validate({})).toBe(true); + expect(validate({ [field]: valid })).toBe(true); + for (const value of invalid) + expect(validate({ [field]: value })).toBe(false); + }, +); + it("rejects an empty or NUL --snapshot option before analysis", async () => { const { serve, runs } = registry(); expect(await serve(["inspect", ".", "--snapshot", ""])).toMatchObject({ @@ -150,3 +177,24 @@ it("rejects an empty or NUL --snapshot option before analysis", async () => { }); for (const run of runs) expect(run).not.toHaveBeenCalled(); }); + +it.each([ + { argv: ["inspect-plist", ".", "--relative-path"], field: "relativePath" }, + { argv: ["trace-dylib-resolution", ".", "--root"], field: "root" }, +])( + "rejects an empty or NUL $field option on the CLI and names that option", + async ({ argv, field }) => { + const { serve, runs } = registry(); + for (const value of ["", "Contents/a\x00b"]) { + const result = (await serve([...argv, value])) as { + code: string; + fieldErrors: readonly { path: string }[]; + }; + expect(result.code).toBe("VALIDATION_ERROR"); + expect( + new Set(result.fieldErrors.map(({ path }) => path.split(".")[0])), + ).toEqual(new Set([field])); + } + for (const run of runs) expect(run).not.toHaveBeenCalled(); + }, +);