From f8e8ea35ff08ce5071acbccab188df808708b71c Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:14:15 +0000 Subject: [PATCH] fix(bridge): harden bounded Hopper boundaries --- bridge/hopper_bridge.py | 22 ++++++++-- src/application/EnhancedTools.ts | 2 + src/contracts/toolOutputSchemas.ts | 12 +++--- src/domain/errors.ts | 13 +++++- src/domain/hopperValues.ts | 41 +++++++++++++++--- src/hopper/protocol.ts | 21 ++++++++- tests/bridgeTruthfulness.test.ts | 11 +++++ tests/enhancedTools.test.ts | 69 ++++++++++++++++++++++++++++++ tests/fixtures/fakeHopper.mjs | 68 ++++++++++++++++++++++++++++- tests/hopperClient.test.ts | 11 +++++ tests/hopperValues.test.ts | 24 +++++++++++ 11 files changed, 275 insertions(+), 19 deletions(-) diff --git a/bridge/hopper_bridge.py b/bridge/hopper_bridge.py index de9d3297b..46f8515c3 100644 --- a/bridge/hopper_bridge.py +++ b/bridge/hopper_bridge.py @@ -274,9 +274,15 @@ def _search_page(document, kind, params): raise ValueError("Invalid regex pattern") from error matches = lambda value: expression.search(value) is not None - matching = [item for item in _search_inventory(document, kind) if matches(item[1])] - selected = matching[offset:offset + limit] - total = len(matching) + selected = [] + total = 0 + page_end = offset + limit + for item in _search_inventory(document, kind): + if not matches(item[1]): + continue + if offset <= total < page_end: + selected.append(item) + total += 1 next_offset = offset + len(selected) has_more = next_offset < total return { @@ -654,7 +660,7 @@ def _serve_connection(connection): should_stop = request["method"] == "shutdown" response = {"id": request_id, "result": _json_safe(result)} except Exception as error: - response = {"id": request_id if isinstance(request_id, int) else 0, "error": {"code": -32000, "message": str(error)[:512]}} + response = {"id": request_id if isinstance(request_id, int) else 0, "error": {"code": -32000, "message": str(error)[:512], "type": _diagnostic_type(error)}} file.write((json.dumps(response, separators=(",", ":")) + "\n").encode("utf-8")) file.flush() if should_stop: @@ -663,6 +669,14 @@ def _serve_connection(connection): connection.close() +def _diagnostic_type(error): + if isinstance(error, PermissionError): + return "authorization" + if isinstance(error, (ValueError, TypeError, KeyError)): + return "invalid_request" + return "bridge_exception" + + def _run(): """Own a permission-restricted, single-client Unix socket for this bridge.""" if os.path.exists(REA_SOCKET): diff --git a/src/application/EnhancedTools.ts b/src/application/EnhancedTools.ts index aa9cdf949..9f65c3a99 100644 --- a/src/application/EnhancedTools.ts +++ b/src/application/EnhancedTools.ts @@ -6,6 +6,7 @@ import type { EnhancedToolName } from "../contracts/enhancedInputs.js"; import { enhancedInputSchemas } from "../contracts/enhancedInputs.js"; import { AnalysisInputError, + AnalysisCancelledError, AnalysisOutputError, type AnalysisError, } from "../domain/errors.js"; @@ -472,6 +473,7 @@ export class EnhancedTools { arguments_: Readonly>, signal?: AbortSignal, ): Promise> { + if (signal?.aborted === true) return err(new AnalysisCancelledError(name)); const execution = await this.analysis.execute( name, arguments_, diff --git a/src/contracts/toolOutputSchemas.ts b/src/contracts/toolOutputSchemas.ts index ac323bd71..a1988ef65 100644 --- a/src/contracts/toolOutputSchemas.ts +++ b/src/contracts/toolOutputSchemas.ts @@ -87,12 +87,14 @@ const providerCapability = z.object({ }), limitations: z.array(z.string()), }); +const providerIdentity = z.object({ + id: z.string(), + name: z.string(), + version: z.string().nullable(), +}); const sessionProvider = z.object({ - provider: z.object({ - id: z.string(), - name: z.string(), - version: z.string().nullable(), - }), + provider: providerIdentity, + providers: z.array(providerIdentity), capabilities: z.array(providerCapability), }); const nullableText = z.string().nullable(); diff --git a/src/domain/errors.ts b/src/domain/errors.ts index d53376e62..ed67d576f 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -216,6 +216,12 @@ export class HopperProtocolError extends HopperError { readonly _tag = "HopperProtocolError"; } +export type HopperDiagnosticType = + | "remote" + | "authorization" + | "invalid_request" + | "bridge_exception"; + /** Hopper's JSON-RPC endpoint returned an expected remote error response. */ export class HopperRemoteError extends HopperError { readonly _tag = "HopperRemoteError"; @@ -223,6 +229,7 @@ export class HopperRemoteError extends HopperError { constructor( readonly code: number, readonly safeMessage: string, + readonly diagnosticType: HopperDiagnosticType = "remote", ) { super(`Hopper request failed (${String(code)}): ${safeMessage}`); } @@ -322,7 +329,11 @@ const safeDetails = ( if (error instanceof HopperTimeoutError) return { timeoutMs: error.timeoutMs }; if (error instanceof HopperRemoteError) - return { code: error.code, safeMessage: error.safeMessage }; + return { + code: error.code, + safeMessage: error.safeMessage, + diagnosticType: error.diagnosticType, + }; if (error instanceof HopperProcessError) return { exitCode: error.exitCode }; return {}; }; diff --git a/src/domain/hopperValues.ts b/src/domain/hopperValues.ts index 79f551120..a1e1377c6 100644 --- a/src/domain/hopperValues.ts +++ b/src/domain/hopperValues.ts @@ -37,11 +37,31 @@ const segmentSchema = z.object({ writable: z.boolean().nullable().default(null), executable: z.boolean().nullable().default(null), }); -const addressedPageSchema = z.object({ - items: z.array(z.object({ address: z.string(), value: z.string() })), - next_offset: z.number().int().min(0).nullable(), - has_more: z.boolean(), -}); +const addressedPageSchema = z + .object({ + items: z.array(z.object({ address: z.string(), value: z.string() })), + offset: z.number().int().min(0), + limit: z.number().int().min(1), + total: z.number().int().min(0), + next_offset: z.number().int().min(0).nullable(), + has_more: z.boolean(), + }) + .superRefine((value, context) => { + if (value.has_more && value.next_offset === null) { + context.addIssue({ + code: "custom", + message: "a page with more results must provide next_offset", + path: ["next_offset"], + }); + } + if (!value.has_more && value.next_offset !== null) { + context.addIssue({ + code: "custom", + message: "a complete page must not provide next_offset", + path: ["next_offset"], + }); + } + }); const unavailableSchema = z .object({ available: z.literal(false), reason: z.string() }) .strict(); @@ -86,6 +106,17 @@ const boundedSchema = (item: T) => path: ["next_offset"], }); } + if ( + !value.truncated && + value.total !== null && + value.total !== value.returned + ) { + context.addIssue({ + code: "custom", + message: "a complete collection total must equal returned", + path: ["total"], + }); + } }); const referenceEdgeSchema = z .object({ diff --git a/src/hopper/protocol.ts b/src/hopper/protocol.ts index 3aabcfc7b..92571a42e 100644 --- a/src/hopper/protocol.ts +++ b/src/hopper/protocol.ts @@ -11,7 +11,18 @@ const responseSchema = z.union([ }), z.object({ id: z.number().int().nonnegative(), - error: z.object({ code: z.number().int(), message: z.string() }), + error: z.object({ + code: z.number().int(), + message: z.string(), + type: z + .enum([ + "remote", + "authorization", + "invalid_request", + "bridge_exception", + ]) + .default("remote"), + }), }), ]); @@ -46,5 +57,11 @@ export const responseResult = ( response: HopperResponse, ): Result => "error" in response - ? err(new HopperRemoteError(response.error.code, response.error.message)) + ? err( + new HopperRemoteError( + response.error.code, + response.error.message, + response.error.type, + ), + ) : ok(response.result); diff --git a/tests/bridgeTruthfulness.test.ts b/tests/bridgeTruthfulness.test.ts index e84318053..2a46717aa 100644 --- a/tests/bridgeTruthfulness.test.ts +++ b/tests/bridgeTruthfulness.test.ts @@ -73,6 +73,11 @@ describe("Hopper bridge truthfulness", () => { expect(bridgeSource).toContain("MAX_SEARCH_PATTERN_LENGTH = 256"); expect(bridgeSource).toContain("MAX_SEARCH_VALUE_LENGTH = 4096"); expect(bridgeSource).toContain('"value_truncated"'); + expect(bridgeSource).toContain("page_end = offset + limit"); + expect(bridgeSource).toContain("if offset <= total < page_end:"); + expect(bridgeSource).not.toContain( + "matching = [item for item in _search_inventory(document, kind)", + ); }); it("caches sorted search inventories and invalidates them after renames", () => { @@ -80,4 +85,10 @@ describe("Hopper bridge truthfulness", () => { expect(bridgeSource).toContain("tuple(sorted(values.items()"); expect(bridgeSource).toContain("_invalidate_search_inventory(document)"); }); + + it("classifies bridge exceptions without forwarding tracebacks", () => { + expect(bridgeSource).toContain("def _diagnostic_type(error):"); + expect(bridgeSource).toContain('"type": _diagnostic_type(error)'); + expect(bridgeSource).not.toContain("traceback.format_exc"); + }); }); diff --git a/tests/enhancedTools.test.ts b/tests/enhancedTools.test.ts index cf7b26103..0b441c578 100644 --- a/tests/enhancedTools.test.ts +++ b/tests/enhancedTools.test.ts @@ -3,6 +3,7 @@ import type { CallToolResult } from "@modelcontextprotocol/server"; import { afterEach, describe, expect, it } from "vitest"; import type { AnalysisOperationPort } from "../src/application/AnalysisProvider.js"; +import { EnhancedTools } from "../src/application/EnhancedTools.js"; import { ENHANCED_TOOL_CONTRACTS } from "../src/contracts/toolContracts.js"; import { observed as ok } from "./fixtures/analysisExecution.js"; import { jsonValueSchema, type JsonValue } from "../src/domain/jsonValue.js"; @@ -304,6 +305,38 @@ describe("enhanced MCP tools", () => { expect(result).toMatchObject({ count: 2 }); }); + it("does not start another page after cancellation", async () => { + const controller = new AbortController(); + let calls = 0; + const analysis: AnalysisOperationPort = { + execute: () => { + calls += 1; + controller.abort(); + return Promise.resolve( + ok({ + items: [{ address: "0x1", value: "_TtC5First" }], + offset: 0, + limit: 500, + total: 2, + next_offset: 500, + has_more: true, + }), + ); + }, + }; + + const result = await new EnhancedTools(analysis).execute( + "swift_classes", + {}, + controller.signal, + ); + + expect(calls).toBe(1); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected cancellation"); + expect(result.error._tag).toBe("AnalysisCancelledError"); + }); + it("follows name pagination for exhaustive Objective-C discovery", async () => { const offsets: number[] = []; const client = await connect({ @@ -534,4 +567,40 @@ describe("enhanced MCP tools", () => { "AnalysisOutputError", ); }); + + it("rejects complete dossier collections with inflated totals", async () => { + const malformedPort = fixturePort(); + const client = await connect({ + execute: async (name, arguments_, options) => { + const result = await malformedPort.execute(name, arguments_, options); + if (!result.ok || name !== "analyze_function") return result; + const dossier = result.value.result; + if ( + typeof dossier !== "object" || + dossier === null || + Array.isArray(dossier) + ) + return result; + return ok({ + ...dossier, + callers: { + items: [], + total: 1, + returned: 0, + truncated: false, + next_offset: null, + }, + }); + }, + }); + const result = await client.callTool({ + name: "analyze_function", + arguments: { procedure: "0x1" }, + }); + expect(result.isError).toBe(true); + const text = result.content.find((item) => item.type === "text"); + expect(text?.type === "text" ? text.text : "").toContain( + "AnalysisOutputError", + ); + }); }); diff --git a/tests/fixtures/fakeHopper.mjs b/tests/fixtures/fakeHopper.mjs index 1d39effd1..e5262d665 100644 --- a/tests/fixtures/fakeHopper.mjs +++ b/tests/fixtures/fakeHopper.mjs @@ -26,9 +26,69 @@ const enhancedFixtureResult = (method) => { has_more: false, }; case "list_strings": - return ["fixture"]; + return { + items: [{ address: "0x1000", value: "fixture" }], + offset: 0, + limit: 500, + total: 1, + next_offset: null, + has_more: false, + }; case "procedure_pseudo_code": return "return 0;"; + case "analyze_function": { + const empty = () => ({ + items: [], + total: 0, + returned: 0, + truncated: false, + next_offset: null, + }); + return { + procedure: { + address: "0x1000", + name: "fixture", + signature: null, + locals: [], + }, + pseudocode: { + text: "return 0;", + total_chars: 9, + returned_chars: 9, + truncated: false, + next_offset: null, + }, + assembly: { ...empty(), items: ["ret"], total: 1, returned: 1 }, + comments: empty(), + callers: empty(), + callees: empty(), + incoming_references: empty(), + outgoing_references: empty(), + referenced_strings: empty(), + referenced_names: empty(), + basic_blocks: { + ...empty(), + items: [{ start: "0x1000", end: "0x1001", successors: [] }], + total: 1, + returned: 1, + }, + instruction_scan: { scanned: 1, truncated: false }, + }; + } + case "search_strings": + case "search_procedures": + return { + items: [ + { address: "0x1000", value: "fixture", value_truncated: false }, + ], + offset: 0, + limit: 100, + total: 1, + next_offset: null, + has_more: false, + }; + case "xrefs": + return ["0x1000"]; default: return undefined; } @@ -79,7 +139,11 @@ const server = createServer((socket) => { } else if (request.method === "remote_error") { send({ id: request.id, - error: { code: -32001, message: "safe fake failure" }, + error: { + code: -32001, + message: "safe fake failure", + type: "bridge_exception", + }, }); } else if (request.method === "current_document") { send({ id: request.id, result: "fixture" }); diff --git a/tests/hopperClient.test.ts b/tests/hopperClient.test.ts index 4f9f1833b..5be1949d2 100644 --- a/tests/hopperClient.test.ts +++ b/tests/hopperClient.test.ts @@ -131,6 +131,17 @@ describe("HopperClient", () => { if (!result.ok) expect(result.error._tag).toBe(expectedTag); }); + it("preserves a sanitized bridge exception diagnostic", async () => { + const client = await startClient(); + const result = await client.callTool("remote_error"); + expect(result.ok).toBe(false); + if (!result.ok) + expect(result.error).toMatchObject({ + _tag: "HopperRemoteError", + diagnosticType: "bridge_exception", + }); + }); + it("cancels and ignores late responses without corrupting the session", async () => { const client = await startClient(); const controller = new AbortController(); diff --git a/tests/hopperValues.test.ts b/tests/hopperValues.test.ts index 14a6c2262..4c319d7fd 100644 --- a/tests/hopperValues.test.ts +++ b/tests/hopperValues.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { parseDocuments, + parseAddressedPage, parseListCount, parseNames, parseProcedures, @@ -62,6 +63,29 @@ describe("Hopper boundary values", () => { }); }); + it.each([ + { + items: [], + offset: 0, + limit: 100, + total: 1, + next_offset: null, + has_more: true, + }, + { + items: [], + offset: 0, + limit: 100, + total: 0, + next_offset: 100, + has_more: false, + }, + ])("rejects contradictory page continuation metadata", (page) => { + const result = parseAddressedPage(page); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error._tag).toBe("HopperProtocolError"); + }); + it.each([ ["procedures", () => parseProcedures(["not-a-map"])], ["names", () => parseNames([{ address: 1, name: "bad" }])],