diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f6f7f8cb4a..eb229d53383 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ Docs: https://docs.openclaw.ai - Zalouser/Pairing auth tests: add account-scoped DM pairing-store regression coverage (`monitor.account-scope.test.ts`) to prevent cross-account allowlist bleed in multi-account setups. (#26672) Thanks @bmendonca3. - Security/Web tools SSRF guard: keep DNS pinning for untrusted `web_fetch` and citation-redirect URL checks when proxy env vars are set, and require explicit dangerous opt-in before env-proxy routing can bypass pinned dispatch for trusted/operator-controlled endpoints. Thanks @tdjackey for reporting. - Gateway/Security canonicalization hardening: decode plugin route path variants to canonical fixpoint (with bounded depth), fail closed on canonicalization anomalies, and enforce gateway auth for deeply encoded `/api/channels/*` variants to prevent alternate-path auth bypass through plugin handlers. Thanks @tdjackey for reporting. +- Gateway/Plugin HTTP hardening: require explicit `auth` for plugin route registration, add route ownership guards for duplicate `path+match` registrations, centralize plugin path matching/auth logic into dedicated modules, and share webhook target-route lifecycle wiring across channel monitors to avoid stale or conflicting registrations. Thanks @tdjackey for reporting. - Agents/Sessions list transcript paths: handle missing/non-string/relative `sessions.list.path` values and per-agent `{agentId}` templates when deriving `transcriptPath`, so cross-agent session listings resolve to concrete agent session files instead of workspace-relative paths. (#24775) Thanks @martinfrancois. - Agents/Subagents `sessions_spawn`: reject malformed `agentId` inputs before normalization (for example error-message/path-like strings) to prevent unintended synthetic agent IDs and ghost workspace/session paths; includes strict validation regression coverage. (#31381) Thanks @openperf. - macOS/PeekabooBridge: add compatibility socket symlinks for legacy `clawdbot`, `clawdis`, and `moltbot` Application Support socket paths so pre-rename clients can still connect. (#6033) Thanks @lumpinif and @vincentkoc. diff --git a/extensions/bluebubbles/src/monitor.ts b/extensions/bluebubbles/src/monitor.ts index d1c296275dd..48646fb7975 100644 --- a/extensions/bluebubbles/src/monitor.ts +++ b/extensions/bluebubbles/src/monitor.ts @@ -4,8 +4,7 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk"; import { isRequestBodyLimitError, readRequestBodyWithLimit, - registerPluginHttpRoute, - registerWebhookTarget, + registerWebhookTargetWithPluginRoute, rejectNonPostWebhookRequest, requestBodyErrorToText, resolveSingleWebhookTarget, @@ -236,23 +235,25 @@ function removeDebouncer(target: WebhookTarget): void { } export function registerBlueBubblesWebhookTarget(target: WebhookTarget): () => void { - const registered = registerWebhookTarget(webhookTargets, target, { - onFirstPathTarget: ({ path }) => - registerPluginHttpRoute({ - path, - pluginId: "bluebubbles", - source: "bluebubbles-webhook", - accountId: target.account.accountId, - log: target.runtime.log, - handler: async (req, res) => { - const handled = await handleBlueBubblesWebhookRequest(req, res); - if (!handled && !res.headersSent) { - res.statusCode = 404; - res.setHeader("Content-Type", "text/plain; charset=utf-8"); - res.end("Not Found"); - } - }, - }), + const registered = registerWebhookTargetWithPluginRoute({ + targetsByPath: webhookTargets, + target, + route: { + auth: "plugin", + match: "exact", + pluginId: "bluebubbles", + source: "bluebubbles-webhook", + accountId: target.account.accountId, + log: target.runtime.log, + handler: async (req, res) => { + const handled = await handleBlueBubblesWebhookRequest(req, res); + if (!handled && !res.headersSent) { + res.statusCode = 404; + res.setHeader("Content-Type", "text/plain; charset=utf-8"); + res.end("Not Found"); + } + }, + }, }); return () => { registered.unregister(); @@ -530,20 +531,10 @@ export async function monitorBlueBubblesProvider( path, statusSink, }); - const unregisterRoute = registerPluginHttpRoute({ - path, - auth: "plugin", - match: "exact", - pluginId: "bluebubbles", - accountId: account.accountId, - log: (message) => logVerbose(core, runtime, message), - handler: handleBlueBubblesWebhookRequest, - }); return await new Promise((resolve) => { const stop = () => { unregister(); - unregisterRoute(); resolve(); }; diff --git a/extensions/googlechat/src/monitor.ts b/extensions/googlechat/src/monitor.ts index b43dba6dace..49ef6ce3263 100644 --- a/extensions/googlechat/src/monitor.ts +++ b/extensions/googlechat/src/monitor.ts @@ -5,9 +5,7 @@ import { createScopedPairingAccess, createReplyPrefixOptions, readJsonBodyWithLimit, - registerPluginHttpRoute, - registerWebhookTarget, - registerPluginHttpRoute, + registerWebhookTargetWithPluginRoute, rejectNonPostWebhookRequest, isDangerousNameMatchingEnabled, resolveAllowlistProviderRuntimeGroupPolicy, @@ -102,23 +100,25 @@ function warnDeprecatedUsersEmailEntries( } export function registerGoogleChatWebhookTarget(target: WebhookTarget): () => void { - return registerWebhookTarget(webhookTargets, target, { - onFirstPathTarget: ({ path }) => - registerPluginHttpRoute({ - path, - pluginId: "googlechat", - source: "googlechat-webhook", - accountId: target.account.accountId, - log: target.runtime.log, - handler: async (req, res) => { - const handled = await handleGoogleChatWebhookRequest(req, res); - if (!handled && !res.headersSent) { - res.statusCode = 404; - res.setHeader("Content-Type", "text/plain; charset=utf-8"); - res.end("Not Found"); - } - }, - }), + return registerWebhookTargetWithPluginRoute({ + targetsByPath: webhookTargets, + target, + route: { + auth: "plugin", + match: "exact", + pluginId: "googlechat", + source: "googlechat-webhook", + accountId: target.account.accountId, + log: target.runtime.log, + handler: async (req, res) => { + const handled = await handleGoogleChatWebhookRequest(req, res); + if (!handled && !res.headersSent) { + res.statusCode = 404; + res.setHeader("Content-Type", "text/plain; charset=utf-8"); + res.end("Not Found"); + } + }, + }, }).unregister; } @@ -981,19 +981,9 @@ export function monitorGoogleChatProvider(options: GoogleChatMonitorOptions): () statusSink: options.statusSink, mediaMaxMb, }); - const unregisterRoute = registerPluginHttpRoute({ - path: webhookPath, - auth: "plugin", - match: "exact", - pluginId: "googlechat", - accountId: options.account.accountId, - log: (message) => logVerbose(core, options.runtime, message), - handler: handleGoogleChatWebhookRequest, - }); return () => { unregisterTarget(); - unregisterRoute(); }; } diff --git a/extensions/phone-control/index.test.ts b/extensions/phone-control/index.test.ts index 480d1390452..4711400c700 100644 --- a/extensions/phone-control/index.test.ts +++ b/extensions/phone-control/index.test.ts @@ -33,7 +33,6 @@ function createApi(params: { logger: { info() {}, warn() {}, error() {} }, registerTool() {}, registerHook() {}, - registerHttpHandler() {}, registerHttpRoute() {}, registerChannel() {}, registerGatewayMethod() {}, diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts index 431dfd2cbd2..ca7a3e31b45 100644 --- a/extensions/synology-chat/src/channel.ts +++ b/extensions/synology-chat/src/channel.ts @@ -295,6 +295,8 @@ export function createSynologyChatPlugin() { const unregister = registerPluginHttpRoute({ path: account.webhookPath, + auth: "plugin", + replaceExisting: true, pluginId: CHANNEL_ID, accountId: account.accountId, log: (msg: string) => log?.info?.(msg), diff --git a/extensions/zalo/src/monitor.ts b/extensions/zalo/src/monitor.ts index 72e6ffdba6f..e3087e6ad00 100644 --- a/extensions/zalo/src/monitor.ts +++ b/extensions/zalo/src/monitor.ts @@ -3,7 +3,6 @@ import type { MarkdownTableMode, OpenClawConfig, OutboundReplyPayload } from "op import { createScopedPairingAccess, createReplyPrefixOptions, - registerPluginHttpRoute, resolveDirectDmAuthorizationOutcome, resolveSenderCommandAuthorizationWithRuntime, resolveOutboundMediaUrls, @@ -77,22 +76,22 @@ function logVerbose(core: ZaloCoreRuntime, runtime: ZaloRuntimeEnv, message: str export function registerZaloWebhookTarget(target: ZaloWebhookTarget): () => void { return registerZaloWebhookTargetInternal(target, { - onFirstPathTarget: ({ path }) => - registerPluginHttpRoute({ - path, - pluginId: "zalo", - source: "zalo-webhook", - accountId: target.account.accountId, - log: target.runtime.log, - handler: async (req, res) => { - const handled = await handleZaloWebhookRequest(req, res); - if (!handled && !res.headersSent) { - res.statusCode = 404; - res.setHeader("Content-Type", "text/plain; charset=utf-8"); - res.end("Not Found"); - } - }, - }), + route: { + auth: "plugin", + match: "exact", + pluginId: "zalo", + source: "zalo-webhook", + accountId: target.account.accountId, + log: target.runtime.log, + handler: async (req, res) => { + const handled = await handleZaloWebhookRequest(req, res); + if (!handled && !res.headersSent) { + res.statusCode = 404; + res.setHeader("Content-Type", "text/plain; charset=utf-8"); + res.end("Not Found"); + } + }, + }, }); } @@ -653,17 +652,7 @@ export async function monitorZaloProvider(options: ZaloMonitorOptions): Promise< mediaMaxMb: effectiveMediaMaxMb, fetcher, }); - const unregisterRoute = registerPluginHttpRoute({ - path, - auth: "plugin", - match: "exact", - pluginId: "zalo", - accountId: account.accountId, - log: (message) => logVerbose(core, runtime, message), - handler: handleZaloWebhookRequest, - }); stopHandlers.push(unregister); - stopHandlers.push(unregisterRoute); abortSignal.addEventListener( "abort", () => { diff --git a/extensions/zalo/src/monitor.webhook.ts b/extensions/zalo/src/monitor.webhook.ts index 269a97ee4e6..b699d986de4 100644 --- a/extensions/zalo/src/monitor.webhook.ts +++ b/extensions/zalo/src/monitor.webhook.ts @@ -7,7 +7,9 @@ import { createWebhookAnomalyTracker, readJsonWebhookBodyOrReject, applyBasicWebhookRequestGuards, + registerWebhookTargetWithPluginRoute, type RegisterWebhookTargetOptions, + type RegisterWebhookPluginRouteOptions, registerWebhookTarget, resolveSingleWebhookTarget, resolveWebhookTargets, @@ -109,11 +111,21 @@ function recordWebhookStatus( export function registerZaloWebhookTarget( target: ZaloWebhookTarget, - opts?: Pick< + opts?: { + route?: RegisterWebhookPluginRouteOptions; + } & Pick< RegisterWebhookTargetOptions, "onFirstPathTarget" | "onLastPathTargetRemoved" >, ): () => void { + if (opts?.route) { + return registerWebhookTargetWithPluginRoute({ + targetsByPath: webhookTargets, + target, + route: opts.route, + onLastPathTargetRemoved: opts.onLastPathTargetRemoved, + }).unregister; + } return registerWebhookTarget(webhookTargets, target, opts).unregister; } diff --git a/src/gateway/server-http.test-harness.ts b/src/gateway/server-http.test-harness.ts index 09c7510b8d1..bf963487038 100644 --- a/src/gateway/server-http.test-harness.ts +++ b/src/gateway/server-http.test-harness.ts @@ -128,7 +128,7 @@ export async function sendRequest( authorization?: string; method?: string; }, -) { +): Promise> { const response = createResponse(); await dispatchRequest(server, createRequest(params), response.res); return response; diff --git a/src/gateway/server-http.ts b/src/gateway/server-http.ts index 447a66ede84..5e493544f27 100644 --- a/src/gateway/server-http.ts +++ b/src/gateway/server-http.ts @@ -170,6 +170,59 @@ async function runGatewayHttpRequestStages( return false; } +function buildPluginRequestStages(params: { + req: IncomingMessage; + res: ServerResponse; + requestPath: string; + pluginPathContext: PluginRoutePathContext | null; + handlePluginRequest?: PluginHttpRequestHandler; + shouldEnforcePluginGatewayAuth?: (pathContext: PluginRoutePathContext) => boolean; + resolvedAuth: ResolvedGatewayAuth; + trustedProxies: string[]; + allowRealIpFallback: boolean; + rateLimiter?: AuthRateLimiter; +}): GatewayHttpRequestStage[] { + if (!params.handlePluginRequest) { + return []; + } + return [ + { + name: "plugin-auth", + run: async () => { + const pathContext = + params.pluginPathContext ?? resolvePluginRoutePathContext(params.requestPath); + if ( + !(params.shouldEnforcePluginGatewayAuth ?? shouldEnforceDefaultPluginGatewayAuth)( + pathContext, + ) + ) { + return false; + } + const pluginAuthOk = await enforcePluginRouteGatewayAuth({ + req: params.req, + res: params.res, + auth: params.resolvedAuth, + trustedProxies: params.trustedProxies, + allowRealIpFallback: params.allowRealIpFallback, + rateLimiter: params.rateLimiter, + }); + if (!pluginAuthOk) { + return true; + } + return false; + }, + }, + { + name: "plugin-http", + run: () => { + const pathContext = + params.pluginPathContext ?? resolvePluginRoutePathContext(params.requestPath); + return params.handlePluginRequest?.(params.req, params.res, pathContext) ?? false; + }, + }, + ]; +} + export function createHooksRequestHandler( opts: { getHooksConfig: () => HooksConfigResolved | null; @@ -555,40 +608,20 @@ export function createGatewayHttpServer(opts: { } // Plugins run after built-in gateway routes so core surfaces keep // precedence on overlapping paths. - if (handlePluginRequest) { - requestStages.push({ - name: "plugin-auth", - run: async () => { - const pathContext = pluginPathContext ?? resolvePluginRoutePathContext(requestPath); - if ( - !(shouldEnforcePluginGatewayAuth ?? shouldEnforceDefaultPluginGatewayAuth)( - pathContext, - ) - ) { - return false; - } - const pluginAuthOk = await enforcePluginRouteGatewayAuth({ - req, - res, - auth: resolvedAuth, - trustedProxies, - allowRealIpFallback, - rateLimiter, - }); - if (!pluginAuthOk) { - return true; - } - return false; - }, - }); - requestStages.push({ - name: "plugin-http", - run: () => { - const pathContext = pluginPathContext ?? resolvePluginRoutePathContext(requestPath); - return handlePluginRequest(req, res, pathContext); - }, - }); - } + requestStages.push( + ...buildPluginRequestStages({ + req, + res, + requestPath, + pluginPathContext, + handlePluginRequest, + shouldEnforcePluginGatewayAuth, + resolvedAuth, + trustedProxies, + allowRealIpFallback, + rateLimiter, + }), + ); requestStages.push({ name: "gateway-probes", diff --git a/src/gateway/server.plugin-http-auth.test.ts b/src/gateway/server.plugin-http-auth.test.ts index 13474787fc6..fdaabc9b7bb 100644 --- a/src/gateway/server.plugin-http-auth.test.ts +++ b/src/gateway/server.plugin-http-auth.test.ts @@ -418,11 +418,36 @@ describe("gateway plugin HTTP auth boundary", () => { run: async (server) => { for (const variant of buildChannelPathFuzzCorpus()) { const response = await sendRequest(server, { path: variant.path }); - expect(response.res.statusCode, variant.label).not.toBe(200); - expect(response.getBody(), variant.label).not.toContain( - '"route":"channel-canonicalized"', - ); + expect(response.res.statusCode, variant.label).toBe(401); + expect(response.getBody(), variant.label).toContain("Unauthorized"); } + expect(handlePluginRequest).not.toHaveBeenCalled(); + }, + }); + }); + + test("enforces auth before plugin handlers on encoded protected-path variants", async () => { + const encodedVariants = buildChannelPathFuzzCorpus().filter((variant) => + variant.path.includes("%"), + ); + const handlePluginRequest = vi.fn(async (_req: IncomingMessage, res: ServerResponse) => { + res.statusCode = 200; + res.setHeader("Content-Type", "application/json; charset=utf-8"); + res.end(JSON.stringify({ ok: true, route: "should-not-run" })); + return true; + }); + + await withGatewayServer({ + prefix: "openclaw-plugin-http-auth-encoded-order-test-", + resolvedAuth: AUTH_TOKEN, + overrides: { handlePluginRequest }, + run: async (server) => { + for (const variant of encodedVariants) { + const response = await sendRequest(server, { path: variant.path }); + expect(response.res.statusCode, variant.label).toBe(401); + expect(response.getBody(), variant.label).toContain("Unauthorized"); + } + expect(handlePluginRequest).not.toHaveBeenCalled(); }, }); }); diff --git a/src/gateway/server/plugins-http.ts b/src/gateway/server/plugins-http.ts index c5f1f132f3b..2fd0554bf10 100644 --- a/src/gateway/server/plugins-http.ts +++ b/src/gateway/server/plugins-http.ts @@ -2,144 +2,30 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { createSubsystemLogger } from "../../logging/subsystem.js"; import type { PluginRegistry } from "../../plugins/registry.js"; import { - PROTECTED_PLUGIN_ROUTE_PREFIXES, - canonicalizePathForSecurity, - canonicalizePathVariant, -} from "../security-path.js"; + resolvePluginRoutePathContext, + type PluginRoutePathContext, +} from "./plugins-http/path-context.js"; +import { findMatchingPluginHttpRoutes } from "./plugins-http/route-match.js"; + +export { + isProtectedPluginRoutePathFromContext, + resolvePluginRoutePathContext, + type PluginRoutePathContext, +} from "./plugins-http/path-context.js"; +export { + findRegisteredPluginHttpRoute, + isRegisteredPluginHttpRoutePath, +} from "./plugins-http/route-match.js"; +export { shouldEnforceGatewayAuthForPluginPath } from "./plugins-http/route-auth.js"; type SubsystemLogger = ReturnType; -export type PluginRoutePathContext = { - pathname: string; - canonicalPath: string; - candidates: string[]; - malformedEncoding: boolean; - decodePassLimitReached: boolean; - rawNormalizedPath: string; -}; - export type PluginHttpRequestHandler = ( req: IncomingMessage, res: ServerResponse, pathContext?: PluginRoutePathContext, ) => Promise; -type PluginHttpRouteEntry = NonNullable[number]; - -function normalizeProtectedPrefix(prefix: string): string { - const collapsed = prefix.toLowerCase().replace(/\/{2,}/g, "/"); - if (collapsed.length <= 1) { - return collapsed || "/"; - } - return collapsed.replace(/\/+$/, ""); -} - -function prefixMatch(pathname: string, prefix: string): boolean { - return ( - pathname === prefix || pathname.startsWith(`${prefix}/`) || pathname.startsWith(`${prefix}%`) - ); -} - -const NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES = - PROTECTED_PLUGIN_ROUTE_PREFIXES.map(normalizeProtectedPrefix); - -export function isProtectedPluginRoutePathFromContext(context: PluginRoutePathContext): boolean { - if ( - context.candidates.some((candidate) => - NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES.some((prefix) => prefixMatch(candidate, prefix)), - ) - ) { - return true; - } - if (!context.malformedEncoding) { - return false; - } - return NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES.some((prefix) => - prefixMatch(context.rawNormalizedPath, prefix), - ); -} - -export function resolvePluginRoutePathContext(pathname: string): PluginRoutePathContext { - const canonical = canonicalizePathForSecurity(pathname); - return { - pathname, - canonicalPath: canonical.canonicalPath, - candidates: canonical.candidates, - malformedEncoding: canonical.malformedEncoding, - decodePassLimitReached: canonical.decodePassLimitReached, - rawNormalizedPath: canonical.rawNormalizedPath, - }; -} - -function doesRouteMatchPath(route: PluginHttpRouteEntry, context: PluginRoutePathContext): boolean { - const routeCanonicalPath = canonicalizePathVariant(route.path); - if (route.match === "prefix") { - return context.candidates.some((candidate) => prefixMatch(candidate, routeCanonicalPath)); - } - return context.candidates.some((candidate) => candidate === routeCanonicalPath); -} - -function findMatchingPluginHttpRoutes( - registry: PluginRegistry, - context: PluginRoutePathContext, -): PluginHttpRouteEntry[] { - const routes = registry.httpRoutes ?? []; - if (routes.length === 0) { - return []; - } - const exactMatches: PluginHttpRouteEntry[] = []; - const prefixMatches: PluginHttpRouteEntry[] = []; - for (const route of routes) { - if (!doesRouteMatchPath(route, context)) { - continue; - } - if (route.match === "prefix") { - prefixMatches.push(route); - } else { - exactMatches.push(route); - } - } - exactMatches.sort((a, b) => b.path.length - a.path.length); - prefixMatches.sort((a, b) => b.path.length - a.path.length); - return [...exactMatches, ...prefixMatches]; -} - -export function findRegisteredPluginHttpRoute( - registry: PluginRegistry, - pathname: string, -): PluginHttpRouteEntry | undefined { - const pathContext = resolvePluginRoutePathContext(pathname); - return findMatchingPluginHttpRoutes(registry, pathContext)[0]; -} - -export function isRegisteredPluginHttpRoutePath( - registry: PluginRegistry, - pathname: string, -): boolean { - return findRegisteredPluginHttpRoute(registry, pathname) !== undefined; -} - -export function shouldEnforceGatewayAuthForPluginPath( - registry: PluginRegistry, - pathnameOrContext: string | PluginRoutePathContext, -): boolean { - const pathContext = - typeof pathnameOrContext === "string" - ? resolvePluginRoutePathContext(pathnameOrContext) - : pathnameOrContext; - if (pathContext.malformedEncoding || pathContext.decodePassLimitReached) { - return true; - } - if (isProtectedPluginRoutePathFromContext(pathContext)) { - return true; - } - const route = findMatchingPluginHttpRoutes(registry, pathContext)[0]; - if (!route) { - return false; - } - return route.auth === "gateway"; -} - export function createGatewayPluginRequestHandler(params: { registry: PluginRegistry; log: SubsystemLogger; diff --git a/src/gateway/server/plugins-http/path-context.ts b/src/gateway/server/plugins-http/path-context.ts new file mode 100644 index 00000000000..605a96b6b15 --- /dev/null +++ b/src/gateway/server/plugins-http/path-context.ts @@ -0,0 +1,60 @@ +import { + PROTECTED_PLUGIN_ROUTE_PREFIXES, + canonicalizePathForSecurity, +} from "../../security-path.js"; + +export type PluginRoutePathContext = { + pathname: string; + canonicalPath: string; + candidates: string[]; + malformedEncoding: boolean; + decodePassLimitReached: boolean; + rawNormalizedPath: string; +}; + +function normalizeProtectedPrefix(prefix: string): string { + const collapsed = prefix.toLowerCase().replace(/\/{2,}/g, "/"); + if (collapsed.length <= 1) { + return collapsed || "/"; + } + return collapsed.replace(/\/+$/, ""); +} + +export function prefixMatchPath(pathname: string, prefix: string): boolean { + return ( + pathname === prefix || pathname.startsWith(`${prefix}/`) || pathname.startsWith(`${prefix}%`) + ); +} + +const NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES = + PROTECTED_PLUGIN_ROUTE_PREFIXES.map(normalizeProtectedPrefix); + +export function isProtectedPluginRoutePathFromContext(context: PluginRoutePathContext): boolean { + if ( + context.candidates.some((candidate) => + NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES.some((prefix) => + prefixMatchPath(candidate, prefix), + ), + ) + ) { + return true; + } + if (!context.malformedEncoding) { + return false; + } + return NORMALIZED_PROTECTED_PLUGIN_ROUTE_PREFIXES.some((prefix) => + prefixMatchPath(context.rawNormalizedPath, prefix), + ); +} + +export function resolvePluginRoutePathContext(pathname: string): PluginRoutePathContext { + const canonical = canonicalizePathForSecurity(pathname); + return { + pathname, + canonicalPath: canonical.canonicalPath, + candidates: canonical.candidates, + malformedEncoding: canonical.malformedEncoding, + decodePassLimitReached: canonical.decodePassLimitReached, + rawNormalizedPath: canonical.rawNormalizedPath, + }; +} diff --git a/src/gateway/server/plugins-http/route-auth.ts b/src/gateway/server/plugins-http/route-auth.ts new file mode 100644 index 00000000000..7549bde34b3 --- /dev/null +++ b/src/gateway/server/plugins-http/route-auth.ts @@ -0,0 +1,28 @@ +import type { PluginRegistry } from "../../../plugins/registry.js"; +import { + isProtectedPluginRoutePathFromContext, + resolvePluginRoutePathContext, + type PluginRoutePathContext, +} from "./path-context.js"; +import { findMatchingPluginHttpRoutes } from "./route-match.js"; + +export function shouldEnforceGatewayAuthForPluginPath( + registry: PluginRegistry, + pathnameOrContext: string | PluginRoutePathContext, +): boolean { + const pathContext = + typeof pathnameOrContext === "string" + ? resolvePluginRoutePathContext(pathnameOrContext) + : pathnameOrContext; + if (pathContext.malformedEncoding || pathContext.decodePassLimitReached) { + return true; + } + if (isProtectedPluginRoutePathFromContext(pathContext)) { + return true; + } + const route = findMatchingPluginHttpRoutes(registry, pathContext)[0]; + if (!route) { + return false; + } + return route.auth === "gateway"; +} diff --git a/src/gateway/server/plugins-http/route-match.ts b/src/gateway/server/plugins-http/route-match.ts new file mode 100644 index 00000000000..bab082c813e --- /dev/null +++ b/src/gateway/server/plugins-http/route-match.ts @@ -0,0 +1,60 @@ +import type { PluginRegistry } from "../../../plugins/registry.js"; +import { canonicalizePathVariant } from "../../security-path.js"; +import { + prefixMatchPath, + resolvePluginRoutePathContext, + type PluginRoutePathContext, +} from "./path-context.js"; + +type PluginHttpRouteEntry = NonNullable[number]; + +export function doesPluginRouteMatchPath( + route: PluginHttpRouteEntry, + context: PluginRoutePathContext, +): boolean { + const routeCanonicalPath = canonicalizePathVariant(route.path); + if (route.match === "prefix") { + return context.candidates.some((candidate) => prefixMatchPath(candidate, routeCanonicalPath)); + } + return context.candidates.some((candidate) => candidate === routeCanonicalPath); +} + +export function findMatchingPluginHttpRoutes( + registry: PluginRegistry, + context: PluginRoutePathContext, +): PluginHttpRouteEntry[] { + const routes = registry.httpRoutes ?? []; + if (routes.length === 0) { + return []; + } + const exactMatches: PluginHttpRouteEntry[] = []; + const prefixMatches: PluginHttpRouteEntry[] = []; + for (const route of routes) { + if (!doesPluginRouteMatchPath(route, context)) { + continue; + } + if (route.match === "prefix") { + prefixMatches.push(route); + } else { + exactMatches.push(route); + } + } + exactMatches.sort((a, b) => b.path.length - a.path.length); + prefixMatches.sort((a, b) => b.path.length - a.path.length); + return [...exactMatches, ...prefixMatches]; +} + +export function findRegisteredPluginHttpRoute( + registry: PluginRegistry, + pathname: string, +): PluginHttpRouteEntry | undefined { + const pathContext = resolvePluginRoutePathContext(pathname); + return findMatchingPluginHttpRoutes(registry, pathContext)[0]; +} + +export function isRegisteredPluginHttpRoutePath( + registry: PluginRegistry, + pathname: string, +): boolean { + return findRegisteredPluginHttpRoute(registry, pathname) !== undefined; +} diff --git a/src/line/monitor.ts b/src/line/monitor.ts index 84790e27e6a..f10d1ac7117 100644 --- a/src/line/monitor.ts +++ b/src/line/monitor.ts @@ -289,6 +289,7 @@ export async function monitorLineProvider( const unregisterHttp = registerPluginHttpRoute({ path: normalizedPath, auth: "plugin", + replaceExisting: true, pluginId: "line", accountId: resolvedAccountId, log: (msg) => logVerbose(msg), diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index 840f697c292..da4f01c5c10 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -123,12 +123,17 @@ export { acquireFileLock, withFileLock } from "./file-lock.js"; export { normalizeWebhookPath, resolveWebhookPath } from "./webhook-path.js"; export { registerWebhookTarget, + registerWebhookTargetWithPluginRoute, rejectNonPostWebhookRequest, resolveSingleWebhookTarget, resolveSingleWebhookTargetAsync, resolveWebhookTargets, } from "./webhook-targets.js"; -export type { RegisterWebhookTargetOptions, WebhookTargetMatchResult } from "./webhook-targets.js"; +export type { + RegisterWebhookPluginRouteOptions, + RegisterWebhookTargetOptions, + WebhookTargetMatchResult, +} from "./webhook-targets.js"; export { applyBasicWebhookRequestGuards, isJsonContentType, diff --git a/src/plugin-sdk/webhook-targets.test.ts b/src/plugin-sdk/webhook-targets.test.ts index ab56d3fccfc..d18cb6b22e6 100644 --- a/src/plugin-sdk/webhook-targets.test.ts +++ b/src/plugin-sdk/webhook-targets.test.ts @@ -1,8 +1,11 @@ import { EventEmitter } from "node:events"; import type { IncomingMessage, ServerResponse } from "node:http"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createEmptyPluginRegistry } from "../plugins/registry.js"; +import { setActivePluginRegistry } from "../plugins/runtime.js"; import { registerWebhookTarget, + registerWebhookTargetWithPluginRoute, rejectNonPostWebhookRequest, resolveSingleWebhookTarget, resolveSingleWebhookTargetAsync, @@ -17,6 +20,10 @@ function createRequest(method: string, url: string): IncomingMessage { return req; } +afterEach(() => { + setActivePluginRegistry(createEmptyPluginRegistry()); +}); + describe("registerWebhookTarget", () => { it("normalizes the path and unregisters cleanly", () => { const targets = new Map>(); @@ -86,6 +93,49 @@ describe("registerWebhookTarget", () => { }); }); +describe("registerWebhookTargetWithPluginRoute", () => { + it("registers plugin route on first target and removes it on last target", () => { + const registry = createEmptyPluginRegistry(); + setActivePluginRegistry(registry); + const targets = new Map>(); + + const registeredA = registerWebhookTargetWithPluginRoute({ + targetsByPath: targets, + target: { path: "/hook", id: "A" }, + route: { + auth: "plugin", + pluginId: "demo", + source: "demo-webhook", + handler: () => {}, + }, + }); + const registeredB = registerWebhookTargetWithPluginRoute({ + targetsByPath: targets, + target: { path: "/hook", id: "B" }, + route: { + auth: "plugin", + pluginId: "demo", + source: "demo-webhook", + handler: () => {}, + }, + }); + + expect(registry.httpRoutes).toHaveLength(1); + expect(registry.httpRoutes[0]).toEqual( + expect.objectContaining({ + pluginId: "demo", + path: "/hook", + source: "demo-webhook", + }), + ); + + registeredA.unregister(); + expect(registry.httpRoutes).toHaveLength(1); + registeredB.unregister(); + expect(registry.httpRoutes).toHaveLength(0); + }); +}); + describe("resolveWebhookTargets", () => { it("resolves normalized path targets", () => { const targets = new Map>(); diff --git a/src/plugin-sdk/webhook-targets.ts b/src/plugin-sdk/webhook-targets.ts index 0f15cca1517..5d5f4200d23 100644 --- a/src/plugin-sdk/webhook-targets.ts +++ b/src/plugin-sdk/webhook-targets.ts @@ -1,4 +1,5 @@ import type { IncomingMessage, ServerResponse } from "node:http"; +import { registerPluginHttpRoute } from "../plugins/http-registry.js"; import { normalizeWebhookPath } from "./webhook-path.js"; export type RegisteredWebhookTarget = { @@ -11,6 +12,30 @@ export type RegisterWebhookTargetOptions = { onLastPathTargetRemoved?: (params: { path: string }) => void; }; +type RegisterPluginHttpRouteParams = Parameters[0]; + +export type RegisterWebhookPluginRouteOptions = Omit< + RegisterPluginHttpRouteParams, + "path" | "fallbackPath" +>; + +export function registerWebhookTargetWithPluginRoute(params: { + targetsByPath: Map; + target: T; + route: RegisterWebhookPluginRouteOptions; + onLastPathTargetRemoved?: RegisterWebhookTargetOptions["onLastPathTargetRemoved"]; +}): RegisteredWebhookTarget { + return registerWebhookTarget(params.targetsByPath, params.target, { + onFirstPathTarget: ({ path }) => + registerPluginHttpRoute({ + ...params.route, + path, + replaceExisting: params.route.replaceExisting ?? true, + }), + onLastPathTargetRemoved: params.onLastPathTargetRemoved, + }); +} + const pathTeardownByTargetMap = new WeakMap, Map void>>(); function getPathTeardownMap(targetsByPath: Map): Map void> { diff --git a/src/plugins/http-registry.test.ts b/src/plugins/http-registry.test.ts index 12f8277d0d4..73174d6385d 100644 --- a/src/plugins/http-registry.test.ts +++ b/src/plugins/http-registry.test.ts @@ -9,6 +9,7 @@ describe("registerPluginHttpRoute", () => { const unregister = registerPluginHttpRoute({ path: "/plugins/demo", + auth: "plugin", handler, registry, }); @@ -16,7 +17,7 @@ describe("registerPluginHttpRoute", () => { expect(registry.httpRoutes).toHaveLength(1); expect(registry.httpRoutes[0]?.path).toBe("/plugins/demo"); expect(registry.httpRoutes[0]?.handler).toBe(handler); - expect(registry.httpRoutes[0]?.auth).toBe("gateway"); + expect(registry.httpRoutes[0]?.auth).toBe("plugin"); expect(registry.httpRoutes[0]?.match).toBe("exact"); unregister(); @@ -28,6 +29,7 @@ describe("registerPluginHttpRoute", () => { const logs: string[] = []; const unregister = registerPluginHttpRoute({ path: "", + auth: "plugin", handler: vi.fn(), registry, accountId: "default", @@ -39,7 +41,7 @@ describe("registerPluginHttpRoute", () => { expect(() => unregister()).not.toThrow(); }); - it("replaces stale route on same path and keeps latest registration", () => { + it("replaces stale route on same path when replaceExisting=true", () => { const registry = createEmptyPluginRegistry(); const logs: string[] = []; const firstHandler = vi.fn(); @@ -47,6 +49,7 @@ describe("registerPluginHttpRoute", () => { const unregisterFirst = registerPluginHttpRoute({ path: "/plugins/synology", + auth: "plugin", handler: firstHandler, registry, accountId: "default", @@ -56,6 +59,8 @@ describe("registerPluginHttpRoute", () => { const unregisterSecond = registerPluginHttpRoute({ path: "/plugins/synology", + auth: "plugin", + replaceExisting: true, handler: secondHandler, registry, accountId: "default", @@ -77,4 +82,67 @@ describe("registerPluginHttpRoute", () => { unregisterSecond(); expect(registry.httpRoutes).toHaveLength(0); }); + + it("rejects conflicting route registrations without replaceExisting", () => { + const registry = createEmptyPluginRegistry(); + const logs: string[] = []; + + registerPluginHttpRoute({ + path: "/plugins/demo", + auth: "plugin", + handler: vi.fn(), + registry, + pluginId: "demo-a", + source: "demo-a-src", + log: (msg) => logs.push(msg), + }); + + const unregister = registerPluginHttpRoute({ + path: "/plugins/demo", + auth: "plugin", + handler: vi.fn(), + registry, + pluginId: "demo-b", + source: "demo-b-src", + log: (msg) => logs.push(msg), + }); + + expect(registry.httpRoutes).toHaveLength(1); + expect(logs.at(-1)).toContain("route conflict"); + + unregister(); + expect(registry.httpRoutes).toHaveLength(1); + }); + + it("rejects route replacement when a different plugin owns the route", () => { + const registry = createEmptyPluginRegistry(); + const logs: string[] = []; + + registerPluginHttpRoute({ + path: "/plugins/demo", + auth: "plugin", + handler: vi.fn(), + registry, + pluginId: "demo-a", + source: "demo-a-src", + log: (msg) => logs.push(msg), + }); + + const unregister = registerPluginHttpRoute({ + path: "/plugins/demo", + auth: "plugin", + replaceExisting: true, + handler: vi.fn(), + registry, + pluginId: "demo-b", + source: "demo-b-src", + log: (msg) => logs.push(msg), + }); + + expect(registry.httpRoutes).toHaveLength(1); + expect(logs.at(-1)).toContain("route replacement denied"); + + unregister(); + expect(registry.httpRoutes).toHaveLength(1); + }); }); diff --git a/src/plugins/http-registry.ts b/src/plugins/http-registry.ts index 9e7303424d9..a1af2cf9fc4 100644 --- a/src/plugins/http-registry.ts +++ b/src/plugins/http-registry.ts @@ -12,8 +12,9 @@ export function registerPluginHttpRoute(params: { path?: string | null; fallbackPath?: string | null; handler: PluginHttpRouteHandler; - auth?: PluginHttpRouteRegistration["auth"]; + auth: PluginHttpRouteRegistration["auth"]; match?: PluginHttpRouteRegistration["match"]; + replaceExisting?: boolean; pluginId?: string; source?: string; accountId?: string; @@ -36,6 +37,22 @@ export function registerPluginHttpRoute(params: { (entry) => entry.path === normalizedPath && entry.match === routeMatch, ); if (existingIndex >= 0) { + const existing = routes[existingIndex]; + if (!existing) { + return () => {}; + } + if (!params.replaceExisting) { + params.log?.( + `plugin: route conflict at ${normalizedPath} (${routeMatch})${suffix}; owned by ${existing.pluginId ?? "unknown-plugin"} (${existing.source ?? "unknown-source"})`, + ); + return () => {}; + } + if (existing.pluginId && params.pluginId && existing.pluginId !== params.pluginId) { + params.log?.( + `plugin: route replacement denied for ${normalizedPath} (${routeMatch})${suffix}; owned by ${existing.pluginId}`, + ); + return () => {}; + } const pluginHint = params.pluginId ? ` (${params.pluginId})` : ""; params.log?.( `plugin: replacing stale webhook path ${normalizedPath} (${routeMatch})${suffix}${pluginHint}`, @@ -46,7 +63,7 @@ export function registerPluginHttpRoute(params: { const entry: PluginHttpRouteRegistration = { path: normalizedPath, handler: params.handler, - auth: params.auth ?? "gateway", + auth: params.auth, match: routeMatch, pluginId: params.pluginId, source: params.source, diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 31d5d1c147d..48c51a0e137 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -548,7 +548,7 @@ describe("loadOpenClawPlugins", () => { id: "http-route-demo", filename: "http-route-demo.cjs", body: `module.exports = { id: "http-route-demo", register(api) { - api.registerHttpRoute({ path: "/demo", handler: async (_req, res) => { res.statusCode = 200; res.end("ok"); } }); + api.registerHttpRoute({ path: "/demo", auth: "gateway", handler: async (_req, res) => { res.statusCode = 200; res.end("ok"); } }); } };`, }); @@ -568,6 +568,95 @@ describe("loadOpenClawPlugins", () => { expect(httpPlugin?.httpRoutes).toBe(1); }); + it("rejects plugin http routes missing explicit auth", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "http-route-missing-auth", + filename: "http-route-missing-auth.cjs", + body: `module.exports = { id: "http-route-missing-auth", register(api) { + api.registerHttpRoute({ path: "/demo", handler: async () => true }); +} };`, + }); + + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["http-route-missing-auth"], + }, + }); + + expect(registry.httpRoutes.find((entry) => entry.pluginId === "http-route-missing-auth")).toBe( + undefined, + ); + expect( + registry.diagnostics.some((diag) => + String(diag.message).includes("http route registration missing or invalid auth"), + ), + ).toBe(true); + }); + + it("allows explicit replaceExisting for same-plugin http route overrides", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "http-route-replace-self", + filename: "http-route-replace-self.cjs", + body: `module.exports = { id: "http-route-replace-self", register(api) { + api.registerHttpRoute({ path: "/demo", auth: "plugin", handler: async () => false }); + api.registerHttpRoute({ path: "/demo", auth: "plugin", replaceExisting: true, handler: async () => true }); +} };`, + }); + + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["http-route-replace-self"], + }, + }); + + const routes = registry.httpRoutes.filter( + (entry) => entry.pluginId === "http-route-replace-self", + ); + expect(routes).toHaveLength(1); + expect(routes[0]?.path).toBe("/demo"); + expect(registry.diagnostics).toEqual([]); + }); + + it("rejects http route replacement when another plugin owns the route", () => { + useNoBundledPlugins(); + const first = writePlugin({ + id: "http-route-owner-a", + filename: "http-route-owner-a.cjs", + body: `module.exports = { id: "http-route-owner-a", register(api) { + api.registerHttpRoute({ path: "/demo", auth: "plugin", handler: async () => false }); +} };`, + }); + const second = writePlugin({ + id: "http-route-owner-b", + filename: "http-route-owner-b.cjs", + body: `module.exports = { id: "http-route-owner-b", register(api) { + api.registerHttpRoute({ path: "/demo", auth: "plugin", replaceExisting: true, handler: async () => true }); +} };`, + }); + + const registry = loadOpenClawPlugins({ + cache: false, + config: { + plugins: { + load: { paths: [first.file, second.file] }, + allow: ["http-route-owner-a", "http-route-owner-b"], + }, + }, + }); + + const route = registry.httpRoutes.find((entry) => entry.path === "/demo"); + expect(route?.pluginId).toBe("http-route-owner-a"); + expect( + registry.diagnostics.some((diag) => + String(diag.message).includes("http route replacement rejected"), + ), + ).toBe(true); + }); + it("respects explicit disable in config", () => { process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; const plugin = writePlugin({ diff --git a/src/plugins/registry.ts b/src/plugins/registry.ts index bc94a47de73..0b8d8144780 100644 --- a/src/plugins/registry.ts +++ b/src/plugins/registry.ts @@ -284,6 +284,12 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { record.gatewayMethods.push(trimmed); }; + const describeHttpRouteOwner = (entry: PluginHttpRouteRegistration): string => { + const plugin = entry.pluginId?.trim() || "unknown-plugin"; + const source = entry.source?.trim() || "unknown-source"; + return `${plugin} (${source})`; + }; + const registerHttpRoute = (record: PluginRecord, params: OpenClawPluginHttpRouteParams) => { const normalizedPath = normalizePluginHttpPath(params.path); if (!normalizedPath) { @@ -295,24 +301,58 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { }); return; } - const match = params.match ?? "exact"; - if ( - registry.httpRoutes.some((entry) => entry.path === normalizedPath && entry.match === match) - ) { + if (params.auth !== "gateway" && params.auth !== "plugin") { pushDiagnostic({ level: "error", pluginId: record.id, source: record.source, - message: `http route already registered: ${normalizedPath} (${match})`, + message: `http route registration missing or invalid auth: ${normalizedPath}`, }); return; } + const match = params.match ?? "exact"; + const existingIndex = registry.httpRoutes.findIndex( + (entry) => entry.path === normalizedPath && entry.match === match, + ); + if (existingIndex >= 0) { + const existing = registry.httpRoutes[existingIndex]; + if (!existing) { + return; + } + if (!params.replaceExisting) { + pushDiagnostic({ + level: "error", + pluginId: record.id, + source: record.source, + message: `http route already registered: ${normalizedPath} (${match}) by ${describeHttpRouteOwner(existing)}`, + }); + return; + } + if (existing.pluginId && existing.pluginId !== record.id) { + pushDiagnostic({ + level: "error", + pluginId: record.id, + source: record.source, + message: `http route replacement rejected: ${normalizedPath} (${match}) owned by ${describeHttpRouteOwner(existing)}`, + }); + return; + } + registry.httpRoutes[existingIndex] = { + pluginId: record.id, + path: normalizedPath, + handler: params.handler, + auth: params.auth, + match, + source: record.source, + }; + return; + } record.httpRoutes += 1; registry.httpRoutes.push({ pluginId: record.id, path: normalizedPath, handler: params.handler, - auth: params.auth ?? "gateway", + auth: params.auth, match, source: record.source, }); diff --git a/src/plugins/types.ts b/src/plugins/types.ts index 1c120acf314..e664327a373 100644 --- a/src/plugins/types.ts +++ b/src/plugins/types.ts @@ -205,8 +205,9 @@ export type OpenClawPluginHttpRouteHandler = ( export type OpenClawPluginHttpRouteParams = { path: string; handler: OpenClawPluginHttpRouteHandler; - auth?: OpenClawPluginHttpRouteAuth; + auth: OpenClawPluginHttpRouteAuth; match?: OpenClawPluginHttpRouteMatch; + replaceExisting?: boolean; }; export type OpenClawPluginCliContext = {