From 5dbc5150488c0e7545410cfffcc9df43b18cfbe1 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Wed, 15 Apr 2026 20:04:10 -0400 Subject: [PATCH] fix: harden QA runtime triage findings --- .../qa-lab/src/bundled-plugin-staging.ts | 15 +++- extensions/qa-lab/src/gateway-child.test.ts | 24 ++++++ .../qa-lab/src/qa-transport-registry.test.ts | 11 +++ .../qa-lab/src/qa-transport-registry.ts | 6 +- scripts/run-node.mjs | 5 +- src/infra/run-node.test.ts | 75 +++++++++++++------ src/plugin-sdk/root-alias.cjs | 4 +- .../contracts/plugin-sdk-root-alias.test.ts | 42 +++++++++-- 8 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 extensions/qa-lab/src/qa-transport-registry.test.ts diff --git a/extensions/qa-lab/src/bundled-plugin-staging.ts b/extensions/qa-lab/src/bundled-plugin-staging.ts index cb1d62e1057..47ecffe02f4 100644 --- a/extensions/qa-lab/src/bundled-plugin-staging.ts +++ b/extensions/qa-lab/src/bundled-plugin-staging.ts @@ -9,6 +9,13 @@ const QA_ALWAYS_STAGE_RUNTIME_PLUGIN_IDS = Object.freeze([ "speech-core", ]); const QA_OPENAI_PLUGIN_ID = "openai"; +const QA_BUNDLED_PLUGIN_ID_PATTERN = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; + +function assertSafeQaBundledPluginId(pluginId: string) { + if (!QA_BUNDLED_PLUGIN_ID_PATTERN.test(pluginId)) { + throw new Error(`invalid QA bundled plugin id: ${pluginId}`); + } +} function parseStableSemverFloor(value: string | undefined) { if (!value) { @@ -56,6 +63,7 @@ function isQaOpenAiResponsesProviderConfig(config: ModelProviderConfig) { } export function resolveQaBundledPluginSourceDir(params: { repoRoot: string; pluginId: string }) { + assertSafeQaBundledPluginId(params.pluginId); const candidates = [ path.join(params.repoRoot, "dist", "extensions", params.pluginId), path.join(params.repoRoot, "dist-runtime", "extensions", params.pluginId), @@ -141,7 +149,12 @@ function collectQaBundledPluginIds(params: { repoRoot: string; allowedPluginIds: readonly string[]; }) { - const pluginIds = new Set(params.allowedPluginIds); + const pluginIds = new Set( + params.allowedPluginIds.map((pluginId) => { + assertSafeQaBundledPluginId(pluginId); + return pluginId; + }), + ); for (const pluginId of QA_ALWAYS_STAGE_RUNTIME_PLUGIN_IDS) { if ( resolveQaBundledPluginSourceDir({ diff --git a/extensions/qa-lab/src/gateway-child.test.ts b/extensions/qa-lab/src/gateway-child.test.ts index ac4fed0c53d..50bc9a23be6 100644 --- a/extensions/qa-lab/src/gateway-child.test.ts +++ b/extensions/qa-lab/src/gateway-child.test.ts @@ -856,6 +856,30 @@ describe("qa bundled plugin dir", () => { ).resolves.toBeTruthy(); }); + it("rejects invalid bundled plugin ids before staging paths are built", async () => { + const repoRoot = await mkdtemp(path.join(os.tmpdir(), "qa-bundled-invalid-id-")); + cleanups.push(async () => { + await rm(repoRoot, { recursive: true, force: true }); + }); + await writeFile( + path.join(repoRoot, "package.json"), + JSON.stringify({ name: "openclaw", type: "module" }, null, 2), + "utf8", + ); + const tempRoot = await mkdtemp(path.join(os.tmpdir(), "qa-bundled-invalid-target-")); + cleanups.push(async () => { + await rm(tempRoot, { recursive: true, force: true }); + }); + + await expect( + __testing.createQaBundledPluginsDir({ + repoRoot, + tempRoot, + allowedPluginIds: ["../escape"], + }), + ).rejects.toThrow("invalid QA bundled plugin id: ../escape"); + }); + it("stages source-only bundled plugins into a repo-like runtime root with node_modules", async () => { const repoRoot = await mkdtemp(path.join(os.tmpdir(), "qa-bundled-source-stage-")); cleanups.push(async () => { diff --git a/extensions/qa-lab/src/qa-transport-registry.test.ts b/extensions/qa-lab/src/qa-transport-registry.test.ts new file mode 100644 index 00000000000..df6ce160efc --- /dev/null +++ b/extensions/qa-lab/src/qa-transport-registry.test.ts @@ -0,0 +1,11 @@ +import { describe, expect, it } from "vitest"; +import { normalizeQaTransportId } from "./qa-transport-registry.js"; + +describe("qa transport registry", () => { + it("rejects inherited prototype keys as unsupported transport ids", () => { + expect(() => normalizeQaTransportId("toString")).toThrow("unsupported QA transport: toString"); + expect(() => normalizeQaTransportId("__proto__")).toThrow( + "unsupported QA transport: __proto__", + ); + }); +}); diff --git a/extensions/qa-lab/src/qa-transport-registry.ts b/extensions/qa-lab/src/qa-transport-registry.ts index 0ff1e5d7299..d6433764618 100644 --- a/extensions/qa-lab/src/qa-transport-registry.ts +++ b/extensions/qa-lab/src/qa-transport-registry.ts @@ -23,9 +23,9 @@ const QA_TRANSPORT_REGISTRY = { >; export function normalizeQaTransportId(input?: string | null): QaTransportId { - const transportId = (input?.trim() || DEFAULT_QA_TRANSPORT_ID) as QaTransportId; - if (transportId in QA_TRANSPORT_REGISTRY) { - return transportId; + const transportId = input?.trim() || DEFAULT_QA_TRANSPORT_ID; + if (Object.hasOwn(QA_TRANSPORT_REGISTRY, transportId)) { + return transportId as QaTransportId; } throw new Error(`unsupported QA transport: ${transportId}`); } diff --git a/scripts/run-node.mjs b/scripts/run-node.mjs index 4dbcc159061..4c9a41b69cc 100644 --- a/scripts/run-node.mjs +++ b/scripts/run-node.mjs @@ -210,8 +210,8 @@ export const resolveBuildRequirement = (deps) => { } if ( deps.env.OPENCLAW_BUILD_PRIVATE_QA === "1" && - deps.privateQaDistEntry && - statMtime(deps.privateQaDistEntry, deps.fs) == null + ((deps.privateQaDistEntry && statMtime(deps.privateQaDistEntry, deps.fs) == null) || + (deps.privateQaBundledCliEntry && statMtime(deps.privateQaBundledCliEntry, deps.fs) == null)) ) { return { shouldBuild: true, reason: "missing_private_qa_dist" }; } @@ -398,6 +398,7 @@ export async function runNodeMain(params = {}) { })); deps.configFiles = runNodeConfigFiles.map((filePath) => path.join(deps.cwd, filePath)); deps.privateQaDistEntry = path.join(deps.distRoot, "plugin-sdk", "qa-lab.js"); + deps.privateQaBundledCliEntry = path.join(deps.distRoot, "extensions", "qa-lab", "cli.js"); if (deps.args[0] === "qa") { deps.env.OPENCLAW_BUILD_PRIVATE_QA = "1"; deps.env.OPENCLAW_ENABLE_PRIVATE_QA_CLI = "1"; diff --git a/src/infra/run-node.test.ts b/src/infra/run-node.test.ts index dd7cecca694..abe84094171 100644 --- a/src/infra/run-node.test.ts +++ b/src/infra/run-node.test.ts @@ -20,6 +20,7 @@ const GENERATED_A2UI_BUNDLE_HASH = "src/canvas-host/a2ui/.bundle.hash"; const DIST_ENTRY = "dist/entry.js"; const BUILD_STAMP = "dist/.buildstamp"; const QA_LAB_PLUGIN_SDK_ENTRY = "dist/plugin-sdk/qa-lab.js"; +const QA_LAB_BUNDLED_CLI_ENTRY = "dist/extensions/qa-lab/cli.js"; const EXTENSION_SRC = bundledPluginFile("demo", "src/index.ts"); const EXTENSION_MANIFEST = bundledPluginFile("demo", "openclaw.plugin.json"); const EXTENSION_PACKAGE = bundledPluginFile("demo", "package.json"); @@ -343,6 +344,46 @@ describe("run-node script", () => { }); it("skips rebuilding for private QA commands when the QA CLI facade is present", async () => { + await withTempDir({ prefix: "openclaw-run-node-" }, async (tmp) => { + await setupTrackedProject(tmp, { + files: { + [ROOT_SRC]: "export const value = 1;\n", + [QA_LAB_PLUGIN_SDK_ENTRY]: "export const qaLab = true;\n", + [QA_LAB_BUNDLED_CLI_ENTRY]: "export const registerQaLabCli = () => {};\n", + }, + oldPaths: [ + ROOT_SRC, + ROOT_TSCONFIG, + ROOT_PACKAGE, + QA_LAB_PLUGIN_SDK_ENTRY, + QA_LAB_BUNDLED_CLI_ENTRY, + ], + buildPaths: [DIST_ENTRY, BUILD_STAMP], + }); + + const { spawnCalls, spawn, spawnSync } = createSpawnRecorder({ + gitHead: "abc123\n", + gitStatus: "", + }); + const exitCode = await runQaCommand({ tmp, spawn, spawnSync }); + + expect(exitCode).toBe(0); + expect(spawnCalls).toEqual([ + [ + process.execPath, + "openclaw.mjs", + "qa", + "suite", + "--transport", + "qa-channel", + "--provider-mode", + "mock-openai", + ], + ]); + }); + }); + + it("rebuilds private QA commands when the QA bundled CLI surface is missing", async () => { await withTempDir({ prefix: "openclaw-run-node-" }, async (tmp) => { await setupTrackedProject(tmp, { files: { @@ -359,33 +400,19 @@ describe("run-node script", () => { }); const exitCode = await runQaCommand({ tmp, spawn, spawnSync }); - expect(exitCode).toBe(0); - expect(spawnCalls).toEqual([ - [process.execPath, "openclaw.mjs", "qa", "suite", "--transport", "qa-channel", "--provider-mode", "mock-openai"], - ]); - }); - }); - - it("rebuilds private QA commands when the QA CLI facade is missing", async () => { - await withTempDir({ prefix: "openclaw-run-node-" }, async (tmp) => { - await setupTrackedProject(tmp, { - files: { - [ROOT_SRC]: "export const value = 1;\n", - }, - oldPaths: [ROOT_SRC, ROOT_TSCONFIG, ROOT_PACKAGE], - buildPaths: [DIST_ENTRY, BUILD_STAMP], - }); - - const { spawnCalls, spawn, spawnSync } = createSpawnRecorder({ - gitHead: "abc123\n", - gitStatus: "", - }); - const exitCode = await runQaCommand({ tmp, spawn, spawnSync }); - expect(exitCode).toBe(0); expect(spawnCalls).toEqual([ expectedBuildSpawn(), - [process.execPath, "openclaw.mjs", "qa", "suite", "--transport", "qa-channel", "--provider-mode", "mock-openai"], + [ + process.execPath, + "openclaw.mjs", + "qa", + "suite", + "--transport", + "qa-channel", + "--provider-mode", + "mock-openai", + ], ]); }); }); diff --git a/src/plugin-sdk/root-alias.cjs b/src/plugin-sdk/root-alias.cjs index 9c2a5c30794..088395a64f3 100644 --- a/src/plugin-sdk/root-alias.cjs +++ b/src/plugin-sdk/root-alias.cjs @@ -141,7 +141,9 @@ function listPrivateLocalOnlyPluginSdkSubpaths() { if (!Array.isArray(parsed)) { return []; } - return parsed.filter((subpath) => typeof subpath === "string"); + return parsed.filter( + (subpath) => typeof subpath === "string" && /^[A-Za-z0-9][A-Za-z0-9_-]*$/.test(subpath), + ); } catch { return []; } diff --git a/src/plugins/contracts/plugin-sdk-root-alias.test.ts b/src/plugins/contracts/plugin-sdk-root-alias.test.ts index 599c373edf7..3e9647a9fb7 100644 --- a/src/plugins/contracts/plugin-sdk-root-alias.test.ts +++ b/src/plugins/contracts/plugin-sdk-root-alias.test.ts @@ -29,6 +29,7 @@ function loadRootAliasWithStubs(options?: { packageExports?: Record; platform?: string; existingPaths?: string[]; + privateLocalOnlySubpaths?: unknown; }) { let createJitiCalls = 0; let jitiLoadCalls = 0; @@ -61,13 +62,21 @@ function loadRootAliasWithStubs(options?: { } if (id === "node:fs") { return { - readFileSync: () => - JSON.stringify({ + readFileSync: (targetPath: string) => { + if ( + targetPath.endsWith( + path.join("scripts", "lib", "plugin-sdk-private-local-only-subpaths.json"), + ) + ) { + return JSON.stringify(options?.privateLocalOnlySubpaths ?? []); + } + return JSON.stringify({ exports: { "./plugin-sdk/group-access": { default: "./dist/plugin-sdk/group-access.js" }, ...options?.packageExports, }, - }), + }); + }, existsSync: (targetPath: string) => { if (targetPath.endsWith(path.join("dist", "infra", "diagnostic-events.js"))) { return options?.distExists ?? false; @@ -298,17 +307,40 @@ describe("plugin-sdk root alias", () => { (lazyModule.createJitiOptions.at(-1)?.alias ?? {}) as Record, ); expect(aliasKeys).toEqual([ - "openclaw/plugin-sdk", - "@openclaw/plugin-sdk", "openclaw/plugin-sdk/alpha", "@openclaw/plugin-sdk/alpha", "openclaw/plugin-sdk/group-access", "@openclaw/plugin-sdk/group-access", "openclaw/plugin-sdk/zeta", "@openclaw/plugin-sdk/zeta", + "openclaw/plugin-sdk", + "@openclaw/plugin-sdk", ]); }); + it("ignores unsafe private local-only plugin-sdk subpaths in the CJS root alias", () => { + const packageRoot = path.dirname(path.dirname(path.dirname(rootAliasPath))); + const lazyModule = loadRootAliasWithStubs({ + env: { OPENCLAW_ENABLE_PRIVATE_QA_CLI: "1" }, + privateLocalOnlySubpaths: ["qa-lab", "../escape", "nested/path"], + existingPaths: [path.join(packageRoot, "src", "plugin-sdk", "qa-lab.ts")], + monolithicExports: { + slowHelper: (): string => "loaded", + }, + }); + + expect((lazyModule.moduleExports.slowHelper as () => string)()).toBe("loaded"); + const aliasMap = (lazyModule.createJitiOptions.at(-1)?.alias ?? {}) as Record; + expect(aliasMap["openclaw/plugin-sdk/qa-lab"]).toBe( + path.join(packageRoot, "src", "plugin-sdk", "qa-lab.ts"), + ); + expect(aliasMap["@openclaw/plugin-sdk/qa-lab"]).toBe( + path.join(packageRoot, "src", "plugin-sdk", "qa-lab.ts"), + ); + expect(aliasMap).not.toHaveProperty("openclaw/plugin-sdk/../escape"); + expect(aliasMap).not.toHaveProperty("openclaw/plugin-sdk/nested/path"); + }); + it("builds source plugin-sdk subpath aliases through the wider source extension family", () => { const packageRoot = path.dirname(path.dirname(path.dirname(rootAliasPath))); const lazyModule = loadRootAliasWithStubs({