fix(gateway): harden plugin HTTP route auth
This commit is contained in:
@@ -131,4 +131,37 @@ describe("registerPluginHttpRoute", () => {
|
||||
expectedLogFragment: "route replacement denied",
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects mixed-auth overlapping routes", () => {
|
||||
const registry = createEmptyPluginRegistry();
|
||||
const logs: string[] = [];
|
||||
|
||||
registerPluginHttpRoute({
|
||||
path: "/plugin/secure",
|
||||
auth: "gateway",
|
||||
match: "prefix",
|
||||
handler: vi.fn(),
|
||||
registry,
|
||||
pluginId: "demo-gateway",
|
||||
source: "demo-gateway-src",
|
||||
log: (msg) => logs.push(msg),
|
||||
});
|
||||
|
||||
const unregister = registerPluginHttpRoute({
|
||||
path: "/plugin/secure/report",
|
||||
auth: "plugin",
|
||||
match: "exact",
|
||||
handler: vi.fn(),
|
||||
registry,
|
||||
pluginId: "demo-plugin",
|
||||
source: "demo-plugin-src",
|
||||
log: (msg) => logs.push(msg),
|
||||
});
|
||||
|
||||
expect(registry.httpRoutes).toHaveLength(1);
|
||||
expect(logs.at(-1)).toContain("route overlap denied");
|
||||
|
||||
unregister();
|
||||
expect(registry.httpRoutes).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import type { IncomingMessage, ServerResponse } from "node:http";
|
||||
import { normalizePluginHttpPath } from "./http-path.js";
|
||||
import { findOverlappingPluginHttpRoute } from "./http-route-overlap.js";
|
||||
import type { PluginHttpRouteRegistration, PluginRegistry } from "./registry.js";
|
||||
import { requireActivePluginRegistry } from "./runtime.js";
|
||||
|
||||
@@ -33,6 +34,18 @@ export function registerPluginHttpRoute(params: {
|
||||
}
|
||||
|
||||
const routeMatch = params.match ?? "exact";
|
||||
const overlappingRoute = findOverlappingPluginHttpRoute(routes, {
|
||||
path: normalizedPath,
|
||||
match: routeMatch,
|
||||
});
|
||||
if (overlappingRoute && overlappingRoute.auth !== params.auth) {
|
||||
params.log?.(
|
||||
`plugin: route overlap denied at ${normalizedPath} (${routeMatch}, ${params.auth})${suffix}; ` +
|
||||
`overlaps ${overlappingRoute.path} (${overlappingRoute.match}, ${overlappingRoute.auth}) ` +
|
||||
`owned by ${overlappingRoute.pluginId ?? "unknown-plugin"} (${overlappingRoute.source ?? "unknown-source"})`,
|
||||
);
|
||||
return () => {};
|
||||
}
|
||||
const existingIndex = routes.findIndex(
|
||||
(entry) => entry.path === normalizedPath && entry.match === routeMatch,
|
||||
);
|
||||
|
||||
44
src/plugins/http-route-overlap.ts
Normal file
44
src/plugins/http-route-overlap.ts
Normal file
@@ -0,0 +1,44 @@
|
||||
import { canonicalizePathVariant } from "../gateway/security-path.js";
|
||||
import type { OpenClawPluginHttpRouteMatch } from "./types.js";
|
||||
|
||||
type PluginHttpRouteLike = {
|
||||
path: string;
|
||||
match: OpenClawPluginHttpRouteMatch;
|
||||
};
|
||||
|
||||
function prefixMatchPath(pathname: string, prefix: string): boolean {
|
||||
return (
|
||||
pathname === prefix || pathname.startsWith(`${prefix}/`) || pathname.startsWith(`${prefix}%`)
|
||||
);
|
||||
}
|
||||
|
||||
export function doPluginHttpRoutesOverlap(
|
||||
a: Pick<PluginHttpRouteLike, "path" | "match">,
|
||||
b: Pick<PluginHttpRouteLike, "path" | "match">,
|
||||
): boolean {
|
||||
const aPath = canonicalizePathVariant(a.path);
|
||||
const bPath = canonicalizePathVariant(b.path);
|
||||
|
||||
if (a.match === "exact" && b.match === "exact") {
|
||||
return aPath === bPath;
|
||||
}
|
||||
if (a.match === "prefix" && b.match === "prefix") {
|
||||
return prefixMatchPath(aPath, bPath) || prefixMatchPath(bPath, aPath);
|
||||
}
|
||||
|
||||
const prefixRoute = a.match === "prefix" ? a : b;
|
||||
const exactRoute = a.match === "exact" ? a : b;
|
||||
return prefixMatchPath(
|
||||
canonicalizePathVariant(exactRoute.path),
|
||||
canonicalizePathVariant(prefixRoute.path),
|
||||
);
|
||||
}
|
||||
|
||||
export function findOverlappingPluginHttpRoute<
|
||||
T extends {
|
||||
path: string;
|
||||
match: OpenClawPluginHttpRouteMatch;
|
||||
},
|
||||
>(routes: readonly T[], candidate: PluginHttpRouteLike): T | undefined {
|
||||
return routes.find((route) => doPluginHttpRoutesOverlap(route, candidate));
|
||||
}
|
||||
@@ -731,6 +731,59 @@ describe("loadOpenClawPlugins", () => {
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects mixed-auth overlapping http routes", () => {
|
||||
useNoBundledPlugins();
|
||||
const plugin = writePlugin({
|
||||
id: "http-route-overlap",
|
||||
filename: "http-route-overlap.cjs",
|
||||
body: `module.exports = { id: "http-route-overlap", register(api) {
|
||||
api.registerHttpRoute({ path: "/plugin/secure", auth: "gateway", match: "prefix", handler: async () => true });
|
||||
api.registerHttpRoute({ path: "/plugin/secure/report", auth: "plugin", match: "exact", handler: async () => true });
|
||||
} };`,
|
||||
});
|
||||
|
||||
const registry = loadRegistryFromSinglePlugin({
|
||||
plugin,
|
||||
pluginConfig: {
|
||||
allow: ["http-route-overlap"],
|
||||
},
|
||||
});
|
||||
|
||||
const routes = registry.httpRoutes.filter((entry) => entry.pluginId === "http-route-overlap");
|
||||
expect(routes).toHaveLength(1);
|
||||
expect(routes[0]?.path).toBe("/plugin/secure");
|
||||
expect(
|
||||
registry.diagnostics.some((diag) =>
|
||||
String(diag.message).includes("http route overlap rejected"),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("allows same-auth overlapping http routes", () => {
|
||||
useNoBundledPlugins();
|
||||
const plugin = writePlugin({
|
||||
id: "http-route-overlap-same-auth",
|
||||
filename: "http-route-overlap-same-auth.cjs",
|
||||
body: `module.exports = { id: "http-route-overlap-same-auth", register(api) {
|
||||
api.registerHttpRoute({ path: "/plugin/public", auth: "plugin", match: "prefix", handler: async () => true });
|
||||
api.registerHttpRoute({ path: "/plugin/public/report", auth: "plugin", match: "exact", handler: async () => true });
|
||||
} };`,
|
||||
});
|
||||
|
||||
const registry = loadRegistryFromSinglePlugin({
|
||||
plugin,
|
||||
pluginConfig: {
|
||||
allow: ["http-route-overlap-same-auth"],
|
||||
},
|
||||
});
|
||||
|
||||
const routes = registry.httpRoutes.filter(
|
||||
(entry) => entry.pluginId === "http-route-overlap-same-auth",
|
||||
);
|
||||
expect(routes).toHaveLength(2);
|
||||
expect(registry.diagnostics).toEqual([]);
|
||||
});
|
||||
|
||||
it("respects explicit disable in config", () => {
|
||||
process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins";
|
||||
const plugin = writePlugin({
|
||||
|
||||
@@ -12,6 +12,7 @@ import type { HookEntry } from "../hooks/types.js";
|
||||
import { resolveUserPath } from "../utils.js";
|
||||
import { registerPluginCommand } from "./commands.js";
|
||||
import { normalizePluginHttpPath } from "./http-path.js";
|
||||
import { findOverlappingPluginHttpRoute } from "./http-route-overlap.js";
|
||||
import type { PluginRuntime } from "./runtime/types.js";
|
||||
import {
|
||||
isPluginHookName,
|
||||
@@ -335,6 +336,22 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) {
|
||||
return;
|
||||
}
|
||||
const match = params.match ?? "exact";
|
||||
const overlappingRoute = findOverlappingPluginHttpRoute(registry.httpRoutes, {
|
||||
path: normalizedPath,
|
||||
match,
|
||||
});
|
||||
if (overlappingRoute && overlappingRoute.auth !== params.auth) {
|
||||
pushDiagnostic({
|
||||
level: "error",
|
||||
pluginId: record.id,
|
||||
source: record.source,
|
||||
message:
|
||||
`http route overlap rejected: ${normalizedPath} (${match}, ${params.auth}) ` +
|
||||
`overlaps ${overlappingRoute.path} (${overlappingRoute.match}, ${overlappingRoute.auth}) ` +
|
||||
`owned by ${describeHttpRouteOwner(overlappingRoute)}`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
const existingIndex = registry.httpRoutes.findIndex(
|
||||
(entry) => entry.path === normalizedPath && entry.match === match,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user