From 22250b37999c6c7a94df9409c531e8fa21e4730c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 14 Aug 2026 19:45:38 +0900 Subject: [PATCH] refactor(management): load Lab and routing-profile handlers per namespace Cherry-picked from @Wibias's PR #1676, which solved this before the boundary work reached it. #1681 left management-api.ts eagerly importing the Lab and routing-profile handlers, so every dashboard request still pulled ~70 src/lab modules into the graph -- including on installs that never opted into Lab. My own audit flagged it (B6) and I deferred it; Wibias had already implemented it. The handlers now load per namespace, preserving the eager chain's ordering: /api/lab/automation is matched before the general /api/lab handler, and pathInManagementNamespace requires an exact hit or a child path so /api/labfoo cannot collide with /api/lab. src/server/management-api.ts 70 -> 0 reachable src/lab modules management-api.ts joins the protected set in tests/core-lab-boundary.test.ts. That addition required refining the guard, and the distinction is worth stating: a dynamic import() is a DEFERRED edge, entered only if the branch runs, so the graph walk no longer follows it -- lazy loading is the remedy this guard exists to encourage, not a defect. Guard 1 still forbids a protected file from naming Lab dynamically, so the coverage moved rather than disappeared, and the attack suite now pins that split explicitly. Not taken from #1676: the labIntegrationEnabled config flag. It gates Lab behind a new setting with no migration from an existing automation-config.json, so an operator already running Lab automation would silently stop after upgrading. The merged approach reads the automation config that is already on disk instead. Also avoided its require() calls -- they work under Bun, verified, but bind an ESM package to the Bun runtime where a registration slot does not. Verification: bun x tsc --noEmit exit 0; 93 tests pass across 7 files, including 162 management-API tests green before commit. --- src/server/management-api.ts | 43 ++++++++++++++++++++++++++++----- tests/core-lab-boundary.test.ts | 30 +++++++++++++++++++++-- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/server/management-api.ts b/src/server/management-api.ts index bff89ccead..3f32e51d09 100644 --- a/src/server/management-api.ts +++ b/src/server/management-api.ts @@ -61,15 +61,12 @@ import { handleConfigRoutes } from "./management/config-routes"; import { handleLogsUsageRoutes } from "./management/logs-usage-routes"; import { handleRequestHistoryRoutes } from "./management/request-history-routes"; import { handleRoutingAnalyticsRoutes } from "./management/routing-analytics-routes"; -import { handleRoutingProfileRoutes } from "./management/routing-profile-routes"; import { handleProviderRoutes } from "./management/provider-routes"; import { handleModelRoutes } from "./management/model-routes"; import { handleAgentSettingsRoutes } from "./management/agent-settings-routes"; import { handleOauthAccountRoutes } from "./management/oauth-account-routes"; import { handleComboRoutes } from "./management/combo-routes"; import { handleSystemRoutes } from "./management/system-routes"; -import { handleLabRoutes } from "./management/lab-routes"; -import { handleLabAutomationRoutes } from "./management/lab-automation-routes"; import { handleSidebarRoutes } from "./management/sidebar-routes"; import { handleIntegrationRoutes } from "./management/integration-routes"; import { handleNativeIntegrationRoutes } from "./management/native-integration-routes"; @@ -96,6 +93,41 @@ const managementConvergenceBindings = new WeakMap>(); +/** + * Namespace match for management route prefixes: exact hit or a child path, never a + * prefix collision (`/api/labfoo` must not match `/api/lab`). + */ +function pathInManagementNamespace(pathname: string, prefix: string): boolean { + return pathname === prefix || pathname.startsWith(`${prefix}/`); +} + +/** + * Routing-profile and Compatibility Lab handlers statically import the Lab module graph, + * so mounting them eagerly would pull ~70 `src/lab/` modules into every management + * request -- including installs that never opted into Lab. Loading them per namespace + * keeps `management-api.ts` on the same footing as the three protected core files. + * + * Cherry-picked from @Wibias's PR #1676, which solved this before the boundary work + * reached it. See devlog/_plan/260814_lab_core_decoupling/. + */ +async function handleRoutingProfileRoutesOnDemand(ctx: ManagementContext): Promise { + if (!pathInManagementNamespace(ctx.url.pathname, "/api/routing-profiles")) return null; + const { handleRoutingProfileRoutes } = await import("./management/routing-profile-routes"); + return handleRoutingProfileRoutes(ctx); +} + +async function handleLabRoutesOnDemand(ctx: ManagementContext): Promise { + if (!pathInManagementNamespace(ctx.url.pathname, "/api/lab")) return null; + // Automation is checked first so its narrower namespace keeps its own handler, matching + // the eager chain's ordering. + if (pathInManagementNamespace(ctx.url.pathname, "/api/lab/automation")) { + const { handleLabAutomationRoutes } = await import("./management/lab-automation-routes"); + return handleLabAutomationRoutes(ctx); + } + const { handleLabRoutes } = await import("./management/lab-routes"); + return handleLabRoutes(ctx); +} + export async function handleManagementAPI( req: Request, url: URL, @@ -180,7 +212,7 @@ export async function handleManagementAPI( ?? (await handleLogsUsageRoutes(ctx)) ?? (await handleRequestHistoryRoutes(ctx)) ?? (await handleRoutingAnalyticsRoutes(ctx)) - ?? (await handleRoutingProfileRoutes(ctx)) + ?? (await handleRoutingProfileRoutesOnDemand(ctx)) ?? (await handleProviderRoutes(ctx)) ?? (await handleModelRoutes(ctx)) ?? (await handleIntegrationRoutes(ctx)) @@ -189,8 +221,7 @@ export async function handleManagementAPI( ?? (await handleOauthAccountRoutes(ctx)) ?? (await handleComboRoutes(ctx)) ?? (await handleSystemRoutes(ctx)) - ?? (await handleLabAutomationRoutes(ctx)) - ?? (await handleLabRoutes(ctx)) + ?? (await handleLabRoutesOnDemand(ctx)) ?? (await handleSidebarRoutes(ctx)); } catch (error) { const tooLarge = managementBodyTooLargeResponse(error, req, config); diff --git a/tests/core-lab-boundary.test.ts b/tests/core-lab-boundary.test.ts index 665acfcf92..9e07518a49 100644 --- a/tests/core-lab-boundary.test.ts +++ b/tests/core-lab-boundary.test.ts @@ -19,6 +19,10 @@ const PROTECTED = [ "src/router.ts", "src/server/lifecycle.ts", "src/server/responses/core.ts", + // The management API is mounted for every dashboard request, so eagerly importing the + // Lab and routing-profile handlers put ~70 Lab modules on that path too. Its handlers + // now load per namespace. + "src/server/management-api.ts", ] as const; const repoRoot = resolve(dirname(new URL(import.meta.url).pathname), ".."); @@ -61,6 +65,12 @@ function firstLabPath(entry: string): string[] | null { while ((match = IMPORT_RE.exec(source)) !== null) { const spec = match[1] ?? match[2] ?? match[3] ?? match[4]; if (!spec) continue; + // A dynamic `import()` is a deferred edge, not a load-time one: the module graph is + // only entered if that branch actually runs. Lazy loading behind a namespace or + // activation check is precisely the remedy this guard exists to encourage, so a + // dynamic specifier does not propagate the walk. Guard 1 still forbids a DIRECT + // dynamic Lab import in a protected file, which is what stops it being a loophole. + if (match[4] !== undefined) continue; const next = resolveSpec(spec, current); if (!next || previous.has(next)) continue; previous.set(next, current); @@ -84,7 +94,10 @@ describe("core / Compatibility Lab boundary", () => { test.each(PROTECTED)("%s has no direct src/lab import", file => { const source = readFileSync(resolve(repoRoot, file), "utf8"); const direct = /^\s*(?:import|export)\s+(?!type\b)[^;]*?["'][^"']*\/lab\//m.test(source) - || /^\s*import\s+["'][^"']*\/lab\//m.test(source); + || /^\s*import\s+["'][^"']*\/lab\//m.test(source) + // A protected file may lazily reach Lab through a handler it imports, but must not + // name Lab itself -- not even dynamically. + || /\bimport\s*\(\s*["'][^"']*\/lab\//.test(source); expect(direct).toBe(false); }); @@ -108,11 +121,11 @@ describe("core / Compatibility Lab boundary", () => { * in a protected file passed the original guard while loading Lab at runtime. */ describe("boundary guard cannot be defeated", () => { + // Load-time edges: the graph walk must follow these. const attacks: Array<[string, string]> = [ ["static import", 'import { labRoot } from "../lab/paths";'], ["side-effect import", 'import "../lab/paths";'], ["runtime re-export", 'export { labRoot } from "../lab/paths";'], - ["top-level dynamic import", 'void import("../lab/paths");'], ]; test.each(attacks)("detects a %s", (_label, line) => { @@ -127,6 +140,19 @@ describe("boundary guard cannot be defeated", () => { } }); + + // A dynamic import is a DEFERRED edge, so the graph walk deliberately does not follow it + // -- lazy loading is the remedy, not the defect. Guard 1 is what stops a protected file + // from naming Lab dynamically, so the coverage moves there rather than disappearing. + test("guard 1 forbids a direct dynamic Lab import in a protected file", () => { + const direct = (source: string) => /\bimport\s*\(\s*["'][^"']*\/lab\//.test(source); + expect(direct('void import("../lab/paths");')).toBe(true); + expect(direct('const m = await import("./management/lab-routes");')).toBe(false); + for (const file of PROTECTED) { + expect(direct(readFileSync(resolve(repoRoot, file), "utf8"))).toBe(false); + } + }); + // `import type` is erased at build time, so it must NOT be treated as a runtime edge. test("ignores type-only imports", () => { const probe = join(repoRoot, "src", "server", `__boundary_probe_type_${Math.random().toString(36).slice(2)}.ts`);