From c95507978f0044a7e0b6d18ee2c997a3698e3c86 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Fri, 17 Apr 2026 09:56:54 +0530 Subject: [PATCH] fix(plugins): tighten register rollback --- extensions/webhooks/src/http.test.ts | 67 +++++++++++++++++++++++++++- extensions/webhooks/src/http.ts | 24 +++++++--- src/plugins/loader.test.ts | 15 +++++++ src/plugins/loader.ts | 2 +- 4 files changed, 99 insertions(+), 9 deletions(-) diff --git a/extensions/webhooks/src/http.test.ts b/extensions/webhooks/src/http.test.ts index c7e1c6d8f94..3dad0138bdd 100644 --- a/extensions/webhooks/src/http.test.ts +++ b/extensions/webhooks/src/http.test.ts @@ -10,10 +10,12 @@ const hoisted = vi.hoisted(() => { const sendMessageMock = vi.fn(); const cancelSessionMock = vi.fn(); const killSubagentRunAdminMock = vi.fn(); + const resolveConfiguredSecretInputStringMock = vi.fn(); return { sendMessageMock, cancelSessionMock, killSubagentRunAdminMock, + resolveConfiguredSecretInputStringMock, }; }); @@ -31,6 +33,17 @@ vi.mock("../../../src/agents/subagent-control.js", () => ({ killSubagentRunAdmin: (params: unknown) => hoisted.killSubagentRunAdminMock(params), })); +vi.mock("../runtime-api.js", async (importOriginal) => { + const actual = await importOriginal(); + hoisted.resolveConfiguredSecretInputStringMock.mockImplementation( + actual.resolveConfiguredSecretInputString, + ); + return { + ...actual, + resolveConfiguredSecretInputString: hoisted.resolveConfiguredSecretInputStringMock, + }; +}); + type MockIncomingMessage = IncomingMessage & { destroyed?: boolean; destroy: () => MockIncomingMessage; @@ -58,7 +71,7 @@ function createJsonRequest(params: { return req; }) as MockIncomingMessage["destroy"]; - void Promise.resolve().then(() => { + setImmediate(() => { req.emit("data", Buffer.from(JSON.stringify(params.body), "utf8")); req.emit("end"); }); @@ -95,6 +108,17 @@ function createHandler(): { }; } +function createHandlerWithTarget( + target: TaskFlowWebhookTarget, + cfg: OpenClawConfig = {} as OpenClawConfig, +): ReturnType { + const targetsByPath = new Map([[target.path, [target]]]); + return createTaskFlowWebhookRequestHandler({ + cfg, + targetsByPath, + }); +} + async function dispatchJsonRequest(params: { handler: ReturnType; path: string; @@ -136,6 +160,47 @@ describe("createTaskFlowWebhookRequestHandler", () => { expect(target.taskFlow.list()).toEqual([]); }); + it("caches SecretRef resolution across requests for the same route", async () => { + const runtime = createRuntimeTaskFlow(); + const target: TaskFlowWebhookTarget = { + routeId: "cached", + path: "/plugins/webhooks/cached", + secretInput: { + source: "env", + provider: "default", + id: "OPENCLAW_WEBHOOK_SECRET", + }, + secretConfigPath: "plugins.entries.webhooks.routes.cached.secret", + defaultControllerId: "webhooks/cached", + taskFlow: runtime.bindSession({ + sessionKey: "agent:main:webhook-cached", + }), + }; + hoisted.resolveConfiguredSecretInputStringMock.mockResolvedValue({ value: "shared-secret" }); + const handler = createHandlerWithTarget(target); + + const first = await dispatchJsonRequest({ + handler, + path: target.path, + secret: "shared-secret", + body: { + action: "list_flows", + }, + }); + const second = await dispatchJsonRequest({ + handler, + path: target.path, + secret: "shared-secret", + body: { + action: "list_flows", + }, + }); + + expect(first.statusCode).toBe(200); + expect(second.statusCode).toBe(200); + expect(hoisted.resolveConfiguredSecretInputStringMock).toHaveBeenCalledTimes(1); + }); + it("creates flows through the bound session and scrubs owner metadata from responses", async () => { const { handler, target, secret } = createHandler(); const res = await dispatchJsonRequest({ diff --git a/extensions/webhooks/src/http.ts b/extensions/webhooks/src/http.ts index 3448c698ceb..f01d4839e08 100644 --- a/extensions/webhooks/src/http.ts +++ b/extensions/webhooks/src/http.ts @@ -667,6 +667,7 @@ export function createTaskFlowWebhookRequestHandler(params: { targetsByPath: Map; inFlightLimiter?: WebhookInFlightLimiter; }): (req: IncomingMessage, res: ServerResponse) => Promise { + const secretByTarget = new WeakMap>(); const rateLimiter = createFixedWindowRateLimiter({ windowMs: WEBHOOK_RATE_LIMIT_DEFAULTS.windowMs, maxRequests: WEBHOOK_RATE_LIMIT_DEFAULTS.maxRequests, @@ -678,6 +679,20 @@ export function createTaskFlowWebhookRequestHandler(params: { maxInFlightPerKey: WEBHOOK_IN_FLIGHT_DEFAULTS.maxInFlightPerKey, maxTrackedKeys: WEBHOOK_IN_FLIGHT_DEFAULTS.maxTrackedKeys, }); + const resolveTargetSecret = (target: TaskFlowWebhookTarget): Promise => { + const cached = secretByTarget.get(target); + if (cached) { + return cached; + } + const pending = resolveConfiguredSecretInputString({ + config: params.cfg, + env: process.env, + value: target.secretInput, + path: target.secretConfigPath, + }).then((resolved) => resolved.value); + secretByTarget.set(target, pending); + return pending; + }; return async (req: IncomingMessage, res: ServerResponse): Promise => { return await withResolvedWebhookRequestPipeline({ @@ -708,14 +723,9 @@ export function createTaskFlowWebhookRequestHandler(params: { if (presentedSecret.length === 0) { return false; } - const resolvedSecret = await resolveConfiguredSecretInputString({ - config: params.cfg, - env: process.env, - value: candidate.secretInput, - path: candidate.secretConfigPath, - }); + const resolvedSecret = await resolveTargetSecret(candidate); return Boolean( - resolvedSecret.value && timingSafeEquals(resolvedSecret.value, presentedSecret), + resolvedSecret && timingSafeEquals(resolvedSecret, presentedSecret), ); }, }); diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 684aeb01735..d46e6f590b8 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -1795,6 +1795,18 @@ module.exports = { id: "throws-after-import", register() {} };`, description: "Fail me", handler: async () => ({ text: "nope" }), }); + api.registerReload({ + onConfigReload: async () => {}, + }); + api.registerNodeHostCommand({ + command: "failme", + description: "failme", + run: async () => ({ ok: true }), + }); + api.registerSecurityAuditCollector({ + id: "failme", + collect: async () => [], + }); api.registerInteractiveHandler({ channel: "slack", namespace: "failme", @@ -1831,6 +1843,9 @@ module.exports = { id: "throws-after-import", register() {} };`, ); expect(getRegisteredEventKeys()).toEqual([]); expect(getPluginCommandSpecs()).toEqual([]); + expect(registry.reloads).toEqual([]); + expect(registry.nodeHostCommands).toEqual([]); + expect(registry.securityAuditCollectors).toEqual([]); expect(resolvePluginInteractiveNamespaceMatch("slack", "failme:payload")).toBeNull(); expect(getContextEngineFactory("failme-context")).toBeUndefined(); expect(listContextEngineIds()).not.toContain("failme-context"); diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 581ae019dac..91b0e09e08c 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -2209,6 +2209,7 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi hookPolicy: entry?.hooks, registrationMode, }); + const registrySnapshot = snapshotPluginRegistry(registry); const previousAgentHarnesses = listRegisteredAgentHarnesses(); const previousCompactionProviders = listRegisteredCompactionProviders(); const previousMemoryEmbeddingProviders = listRegisteredMemoryEmbeddingProviders(); @@ -2217,7 +2218,6 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi const previousMemoryCorpusSupplements = listMemoryCorpusSupplements(); const previousMemoryPromptSupplements = listMemoryPromptSupplements(); const previousMemoryRuntime = getMemoryRuntime(); - const registrySnapshot = snapshotPluginRegistry(registry); try { runPluginRegisterSync(register, api);