From bf7d156bb0993afa155c73e93dd45fb966eca4e5 Mon Sep 17 00:00:00 2001 From: pashpashpash Date: Sat, 25 Apr 2026 14:08:56 -0700 Subject: [PATCH] Bound native hook permission fingerprints (#71758) * fix: bound native hook permission fingerprints * fix: address native hook fingerprint review * test: isolate native jiti runtime assertions --- src/agents/harness/native-hook-relay.test.ts | 124 +++++++++++++++++++ src/agents/harness/native-hook-relay.ts | 63 ++++++++-- src/plugin-sdk/facade-loader.test.ts | 20 +++ src/plugins/setup-registry.test.ts | 20 +++ 4 files changed, 218 insertions(+), 9 deletions(-) diff --git a/src/agents/harness/native-hook-relay.test.ts b/src/agents/harness/native-hook-relay.test.ts index 9c0a57a3032..aeb6445225f 100644 --- a/src/agents/harness/native-hook-relay.test.ts +++ b/src/agents/harness/native-hook-relay.test.ts @@ -937,6 +937,130 @@ describe("native hook relay registry", () => { expect(responses.at(-1)).toEqual({ stdout: "", stderr: "", exitCode: 0 }); }); + it("deduplicates pending PermissionRequest approvals before consuming approval budget", async () => { + const relay = registerNativeHookRelay({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + }); + const resolvers: Array<(decision: "allow") => void> = []; + const approvalRequester = vi.fn( + () => + new Promise<"allow">((resolve) => { + resolvers.push(resolve); + }), + ); + __testing.setNativeHookRelayPermissionApprovalRequesterForTests(approvalRequester); + + const duplicatePayload = { + hook_event_name: "PermissionRequest", + tool_name: "Bash", + tool_use_id: "native-call-1", + tool_input: { command: "git push" }, + }; + const duplicateRequests = Array.from({ length: 12 }, () => + invokeNativeHookRelay({ + provider: "codex", + relayId: relay.relayId, + event: "permission_request", + rawPayload: duplicatePayload, + }), + ); + await Promise.resolve(); + expect(approvalRequester).toHaveBeenCalledTimes(1); + + const newRequest = invokeNativeHookRelay({ + provider: "codex", + relayId: relay.relayId, + event: "permission_request", + rawPayload: { + ...duplicatePayload, + tool_use_id: "native-call-2", + tool_input: { command: "curl https://example.com" }, + }, + }); + await Promise.resolve(); + expect(approvalRequester).toHaveBeenCalledTimes(2); + + for (const resolve of resolvers) { + resolve("allow"); + } + await expect(Promise.all([...duplicateRequests, newRequest])).resolves.toHaveLength(13); + }); + + it("uses canonical PermissionRequest content fingerprints for ordinary objects", () => { + const first = __testing.permissionRequestContentFingerprintForTests({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + toolName: "exec", + toolInput: { a: 1, b: { x: 2, y: 3 } }, + }); + const second = __testing.permissionRequestContentFingerprintForTests({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + toolName: "exec", + toolInput: { b: { y: 3, x: 2 }, a: 1 }, + }); + + expect(second).toBe(first); + }); + + it("keeps broad PermissionRequest content fingerprints sensitive to tail changes", () => { + const firstToolInput = Object.fromEntries( + Array.from({ length: 205 }, (_, index) => [`key-${index}`, `value-${index}`]), + ); + const secondToolInput = { + ...firstToolInput, + "key-204": "changed", + }; + + expect( + __testing.permissionRequestContentFingerprintForTests({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + toolName: "exec", + toolInput: firstToolInput, + }), + ).not.toBe( + __testing.permissionRequestContentFingerprintForTests({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + toolName: "exec", + toolInput: secondToolInput, + }), + ); + }); + + it("fingerprints broad PermissionRequest inputs without Object.keys enumeration", () => { + const toolInput = Object.fromEntries( + Array.from({ length: 300 }, (_, index) => [`key-${index}`, `value-${index}`]), + ); + const objectKeys = vi.spyOn(Object, "keys").mockImplementation(() => { + throw new Error("Object.keys should not be used for permission fingerprints"); + }); + + try { + expect(__testing.permissionRequestToolInputKeyFingerprintForTests(toolInput)).toContain( + "key-", + ); + expect( + __testing.permissionRequestContentFingerprintForTests({ + provider: "codex", + sessionId: "session-1", + runId: "run-1", + toolName: "exec", + toolInput, + }), + ).toMatch(/^[a-f0-9]{64}$/); + } finally { + objectKeys.mockRestore(); + } + }); + it("sanitizes PermissionRequest approval previews and reports omitted keys", () => { expect( __testing.formatPermissionApprovalDescriptionForTests({ diff --git a/src/agents/harness/native-hook-relay.ts b/src/agents/harness/native-hook-relay.ts index 0dd7f6161f0..fb15169c37f 100644 --- a/src/agents/harness/native-hook-relay.ts +++ b/src/agents/harness/native-hook-relay.ts @@ -122,6 +122,7 @@ const MAX_NATIVE_HOOK_RELAY_HISTORY_ARRAY_ITEMS = 50; const MAX_NATIVE_HOOK_RELAY_HISTORY_OBJECT_KEYS = 50; const MAX_PERMISSION_FALLBACK_KEYS = 200; const MAX_PERMISSION_FALLBACK_KEY_CHARS = 240; +const MAX_PERMISSION_FINGERPRINT_SORT_KEYS = 200; const MAX_APPROVAL_TITLE_LENGTH = 80; const MAX_APPROVAL_DESCRIPTION_LENGTH = 700; const MAX_PERMISSION_APPROVALS_PER_WINDOW = 12; @@ -442,7 +443,7 @@ async function runNativeHookRelayPermissionRequest(params: { const pendingApproval = pendingPermissionApprovals.get(approvalKey); try { const decision = await (pendingApproval ?? - requestNativeHookRelayPermissionApprovalWithBudget({ + startNativeHookRelayPermissionApprovalWithBudget({ registration: params.registration, approvalKey, request, @@ -463,7 +464,7 @@ async function runNativeHookRelayPermissionRequest(params: { return params.adapter.renderNoopResponse(params.invocation.event); } -async function requestNativeHookRelayPermissionApprovalWithBudget(params: { +async function startNativeHookRelayPermissionApprovalWithBudget(params: { registration: NativeHookRelayRegistration; approvalKey: string; request: NativeHookRelayPermissionApprovalRequest; @@ -505,18 +506,18 @@ function permissionRequestFallbackKey(request: NativeHookRelayPermissionApproval function permissionRequestToolInputKeyFingerprint(toolInput: Record): string { let fingerprint = ""; - let processed = 0; - for (const key of Object.keys(toolInput).toSorted()) { - if (processed >= MAX_PERMISSION_FALLBACK_KEYS) { - break; - } + const { keys, truncated } = readBoundedOwnKeys(toolInput, MAX_PERMISSION_FALLBACK_KEYS); + for (const key of keys) { const separator = fingerprint ? "," : ""; const remaining = MAX_PERMISSION_FALLBACK_KEY_CHARS - fingerprint.length - separator.length; if (remaining <= 0) { break; } fingerprint += `${separator}${key.slice(0, remaining)}`; - processed += 1; + } + if (truncated && fingerprint.length < MAX_PERMISSION_FALLBACK_KEY_CHARS) { + const marker = `${fingerprint ? "," : ""}...`; + fingerprint += marker.slice(0, MAX_PERMISSION_FALLBACK_KEY_CHARS - fingerprint.length); } return fingerprint || "none"; } @@ -559,15 +560,51 @@ function updateJsonHash(hash: ReturnType, value: JsonValue): return; } hash.update("{"); - for (const key of Object.keys(value).toSorted()) { + const { keys, truncated } = readBoundedOwnKeys(value, MAX_PERMISSION_FINGERPRINT_SORT_KEYS); + for (const key of keys) { hash.update(JSON.stringify(key)); hash.update(":"); updateJsonHash(hash, value[key]); hash.update(","); } + if (truncated) { + // Keep ordinary objects order-independent without sorting a broad native + // hook payload. The tail remains content-sensitive in traversal order. + const sortedKeySet = new Set(keys); + hash.update("#object-tail:"); + for (const key in value) { + if (!Object.prototype.hasOwnProperty.call(value, key) || sortedKeySet.has(key)) { + continue; + } + hash.update(JSON.stringify(key)); + hash.update(":"); + updateJsonHash(hash, value[key]); + hash.update(","); + } + } hash.update("}"); } +function readBoundedOwnKeys( + value: Record, + maxKeys: number, +): { keys: string[]; truncated: boolean } { + const keys: string[] = []; + let truncated = false; + for (const key in value) { + if (!Object.prototype.hasOwnProperty.call(value, key)) { + continue; + } + if (keys.length >= maxKeys) { + truncated = true; + break; + } + keys.push(key); + } + keys.sort(); + return { keys, truncated }; +} + function consumeNativeHookRelayPermissionBudget(relayId: string, now = Date.now()): boolean { const windowStart = now - PERMISSION_APPROVAL_WINDOW_MS; const timestamps = (permissionApprovalWindows.get(relayId) ?? []).filter( @@ -1032,6 +1069,14 @@ export const __testing = { ): string { return formatPermissionApprovalDescription(request); }, + permissionRequestContentFingerprintForTests( + request: NativeHookRelayPermissionApprovalRequest, + ): string { + return permissionRequestContentFingerprint(request); + }, + permissionRequestToolInputKeyFingerprintForTests(toolInput: Record): string { + return permissionRequestToolInputKeyFingerprint(toolInput); + }, setNativeHookRelayPermissionApprovalRequesterForTests( requester: NativeHookRelayPermissionApprovalRequester, ): void { diff --git a/src/plugin-sdk/facade-loader.test.ts b/src/plugin-sdk/facade-loader.test.ts index cba63c0a96d..98e8de84680 100644 --- a/src/plugin-sdk/facade-loader.test.ts +++ b/src/plugin-sdk/facade-loader.test.ts @@ -19,6 +19,24 @@ const originalBundledPluginsDir = process.env.OPENCLAW_BUNDLED_PLUGINS_DIR; const FACADE_LOADER_GLOBAL = "__openclawTestLoadBundledPluginPublicSurfaceModuleSync"; type FacadeLoaderJitiFactory = NonNullable[0]>; +function forceNodeRuntimeVersionsForTest(): () => void { + const originalVersions = process.versions; + const nodeVersions = { ...originalVersions } as NodeJS.ProcessVersions & { + bun?: string | undefined; + }; + delete nodeVersions.bun; + Object.defineProperty(process, "versions", { + configurable: true, + value: nodeVersions, + }); + return () => { + Object.defineProperty(process, "versions", { + configurable: true, + value: originalVersions, + }); + }; +} + function createBundledPluginDir(prefix: string, marker: string): string { return createBundledPluginPublicSurfaceFixture({ createTempDirSync, marker, prefix }); } @@ -127,6 +145,7 @@ describe("plugin-sdk facade loader", () => { })) as unknown as ReturnType; }) as FacadeLoaderJitiFactory); const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + const restoreVersions = forceNodeRuntimeVersionsForTest(); try { expect( @@ -143,6 +162,7 @@ describe("plugin-sdk facade loader", () => { }), ); } finally { + restoreVersions(); platformSpy.mockRestore(); } }); diff --git a/src/plugins/setup-registry.test.ts b/src/plugins/setup-registry.test.ts index a953e7a9b18..e2326258aaf 100644 --- a/src/plugins/setup-registry.test.ts +++ b/src/plugins/setup-registry.test.ts @@ -17,6 +17,24 @@ let resolvePluginSetupProvider: typeof import("./setup-registry.js").resolvePlug let resolvePluginSetupCliBackend: typeof import("./setup-registry.js").resolvePluginSetupCliBackend; let runPluginSetupConfigMigrations: typeof import("./setup-registry.js").runPluginSetupConfigMigrations; +function forceNodeRuntimeVersionsForTest(): () => void { + const originalVersions = process.versions; + const nodeVersions = { ...originalVersions } as NodeJS.ProcessVersions & { + bun?: string | undefined; + }; + delete nodeVersions.bun; + Object.defineProperty(process, "versions", { + configurable: true, + value: nodeVersions, + }); + return () => { + Object.defineProperty(process, "versions", { + configurable: true, + value: originalVersions, + }); + }; +} + function makeTempDir(): string { return makeTrackedTempDir("openclaw-setup-registry", tempDirs); } @@ -166,6 +184,7 @@ describe("setup-registry getJiti", () => { diagnostics: [], }); const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + const restoreVersions = forceNodeRuntimeVersionsForTest(); try { resolvePluginSetupRegistry({ @@ -173,6 +192,7 @@ describe("setup-registry getJiti", () => { env: {}, }); } finally { + restoreVersions(); platformSpy.mockRestore(); }