diff --git a/CHANGELOG.md b/CHANGELOG.md index f0f7caf..99770c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,9 +15,12 @@ Unreleased compatibility update. - Validate canonical nested campaign solver parameters, including numeric candidate matrices, using non-recursive tool schemas. - Keep GPU integration explicitly opt-in with VARIAQ_TEST_GPU=1, independently of CUDA-Q CPU tests. - Run hosted legacy and candidate integration against immutable VariaQ commit pins. -- Document the six development-toolchain audit findings separately from runtime dependencies. +- Document the development-toolchain audit findings separately from runtime dependencies. - Fix duplicated plot-format requests in `variaq_report_campaign` when both `formats: ["plots"]` and `plots: true` are supplied; `formats` is now canonical and `plots: true` only ensures "plots" appears once. +- Convert invalid `--compare` values on `bb variaq report-campaign`, `analyze-runs`, and `analyze-campaign` from an uncaught `ZodError` into structured `PluginError` CLI errors with valid-value hints, preserving valid-comparison behavior and output contracts. +- Correct the `SECURITY.md` dependency review to the current audit state: the `source-map-js` row is remediated by the 1.2.2 override, five development-only packages remain (three high, two moderate), and advisory lists per package are now complete with production dependencies still clean. +- Extend regression coverage: operation-level schema violations (e.g. exceeding the group-by limit) surface as structured CLI errors, and the fake VariaQ harness echoes the received `--compare` value into report envelopes/artifacts so tests prove the comparison selector is actually forwarded to the subprocess. - Update GitHub Actions to Node 24-native action versions (`actions/checkout@v6`, `actions/setup-node@v7`, `actions/setup-python@v7`) to resolve Node 20 deprecation warnings while preserving existing workflow behavior. - Apply a compatible `overrides` remediation for npm audit findings in the `bb-app` development dependency chain; runtime dependencies remain clean. diff --git a/SECURITY.md b/SECURITY.md index 7fa018f..310367f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -39,26 +39,31 @@ code is introduced by candidate compatibility. ## Candidate dependency review -The candidate lockfile audit reports six affected development packages (four +The candidate lockfile audit reports five affected development packages (three high, two moderate); `npm audit --omit=dev` reports zero affected production -packages. These are real upstream advisories, not a claim that development use -is harmless. The plugin runtime imports Zod and Node built-ins, invokes local -Python with argument arrays, and does not call the affected libraries directly. +packages. The previously reported `source-map-js` 1.2.1 exposure +(GHSA-68fv-2mgg-jv7q) is remediated by the lockfile override to 1.2.2 and no +longer appears in the audit. These are real upstream advisories, not a claim +that development use is harmless. The plugin runtime imports Zod and Node +built-ins, invokes local Python with argument arrays, and does not call the +affected libraries directly. All five packages are development-only transitive +dependencies of the npm tooling bundled inside the `bb-app` development +dependency; none is reachable from the shipped plugin runtime. | Locked package | Development dependency path | Advisory exposure | | --- | --- | --- | -| brace-expansion 5.0.9 | bb-app → npm → minimatch | [Stack/CPU exhaustion from hostile brace patterns](https://github.com/advisories/GHSA-qhr7-859c-m2p7) | -| http-cache-semantics 4.2.0 | bb-app → npm → make-fetch-happen | [Cross-user cached response disclosure with max-stale](https://github.com/advisories/GHSA-ch52-4w7c-c8xp) | -| ip-address 10.5.0 | bb-app → npm → proxy/SOCKS tooling | [Incorrect address classifiers/subnet checks](https://github.com/advisories/GHSA-j6r3-76f7-8jcv), plus unbounded diagnostic input | -| postcss-selector-parser 7.1.4 | bb-app → npm → dependency query tooling | [CPU exhaustion from hostile selectors](https://github.com/advisories/GHSA-rj75-hqrm-r3gf) | -| source-map-js 1.2.1 | vitest → vite → postcss | [Event-loop exhaustion from hostile indexed source maps](https://github.com/advisories/GHSA-68fv-2mgg-jv7q) | -| undici 6.28.0 | bb-app → npm → download/build tooling | [WebSocket denial of service](https://github.com/advisories/GHSA-rfgv-xxqx-mfg5), plus retry-interceptor response splitting | +| brace-expansion 5.0.9 | bb-app → npm → minimatch | [Quadratic-time `{a},b}` expansion CPU exhaustion](https://github.com/advisories/GHSA-q2hr-2g5m-vwhr) (moderate); [nested brace group recursion stack exhaustion](https://github.com/advisories/GHSA-qhr7-859c-m2p7) and [parseCommaParts recursion stack exhaustion](https://github.com/advisories/GHSA-6j4f-fj2g-mc7p) (high) | +| http-cache-semantics 4.2.0 | bb-app → npm → make-fetch-happen | [Cross-user cached response disclosure with max-stale](https://github.com/advisories/GHSA-ch52-4w7c-c8xp) (high) | +| ip-address 10.5.0 | bb-app → npm → proxy/SOCKS tooling | [isLinkLocal recognizes fe80::/64 rather than fe80::/10, allowing SSRF](https://github.com/advisories/GHSA-rpw4-54j3-4h4q); [no NAT64 64:ff9b:1::/48 classifier, allowing SSRF](https://github.com/advisories/GHSA-2vr4-cq9g-pvrc); [cross-family isInSubnet comparison can admit out-of-range addresses](https://github.com/advisories/GHSA-j6r3-76f7-8jcv); [unbounded parse diagnostic can stall the process](https://github.com/advisories/GHSA-h3mg-xc3c-68pw) (all moderate) | +| postcss-selector-parser 7.1.4 | bb-app → npm → dependency query tooling | [Quadratic flat selector parsing CPU exhaustion](https://github.com/advisories/GHSA-rj75-hqrm-r3gf) (moderate) | +| undici 6.28.0 | bb-app → npm → node-gyp build tooling | [DoS via unrequested WebSocket subprotocol](https://github.com/advisories/GHSA-rfgv-xxqx-mfg5) (high); [DoS via unhandled error in WebSocket permessage-deflate decompression](https://github.com/advisories/GHSA-3wwx-pv8p-q78v) (moderate); [downstream response splitting via retry interceptor](https://github.com/advisories/GHSA-r53p-7pc4-xj5r) (low) | -The reviewed plugin surfaces expose no user-controlled glob, HTTP cache/proxy, -CSS/source-map parsing or WebSocket path into these packages. Exploitability in -npm/build/test tooling depends on its inputs and configuration; hostile package -metadata, network peers and development assets remain relevant risks. This review -does not audit BB host internals or establish that these advisories are unreachable -in every development invocation. Use isolated CI/cache environments and review -narrow compatible dependency fixes before a broad public release. No forced audit -fix, broad upgrade or suppression was applied. +The reviewed plugin surfaces expose no user-controlled glob expansion, +HTTP cache/proxy, CSS selector, or WebSocket path into these packages. +Exploitability in npm/build/test tooling depends on its inputs and +configuration; hostile package metadata, network peers and development assets +remain relevant risks. This review does not audit BB host internals or +establish that these advisories are unreachable in every development +invocation. Use isolated CI/cache environments and review narrow compatible +dependency fixes before a broad public release. No forced audit fix, broad +upgrade or suppression was applied. diff --git a/server.ts b/server.ts index c704dc0..5c8defc 100644 --- a/server.ts +++ b/server.ts @@ -549,14 +549,20 @@ export default async function plugin(bb: BbPluginApi) { async function opAnalyzeRuns(query: import("./lib/schema.js").AnalyzeQuery) { const c = await cfg(); - const validated = analyzeQuerySchema.parse(query); - if (validated.compare !== undefined) { + const validated = analyzeQuerySchema.safeParse(query); + if (!validated.success) { + throw new PluginError( + `Invalid analyze-runs input: ${minimizeZodIssue(validated.error)}`, + "Run `bb variaq analyze-runs --help` for usage.", + ); + } + if (validated.data.compare !== undefined) { throw new PluginError( "VariaQ analyze runs does not accept comparison selectors.", "Use variaq_analyze_campaign or bb variaq analyze-campaign for comparisons, including paired_solvers on VariaQ 0.8.x.", ); } - const argv = buildAnalyzeArgv("runs", validated); + const argv = buildAnalyzeArgv("runs", validated.data); const result = await runVariaqJson(c, argv, { timeoutMs: 60_000 }); const envelope = requireSuccess(result, ["analyze", "runs"]); const { data, warnings } = successData(envelope, "analyze runs"); @@ -565,7 +571,14 @@ export default async function plugin(bb: BbPluginApi) { async function opAnalyzeCampaign(campaignId: string, query: Omit) { const c = await cfg(); - const fullQuery = analyzeQuerySchema.parse({ ...query, campaign_id: campaignId }); + const validated = analyzeQuerySchema.safeParse({ ...query, campaign_id: campaignId }); + if (!validated.success) { + throw new PluginError( + `Invalid analyze-campaign input: ${minimizeZodIssue(validated.error)}`, + "Run `bb variaq analyze-campaign --help` for usage.", + ); + } + const fullQuery = validated.data; await requireComparisonSupport(c, fullQuery.compare); const argv = buildAnalyzeArgv("campaign", fullQuery); const result = await runVariaqJson(c, argv, { timeoutMs: 60_000 }); @@ -605,8 +618,12 @@ export default async function plugin(bb: BbPluginApi) { }, ) { const c = await cfg(); - if (options.compare !== undefined) comparisonTypeSchema.parse(options.compare); - await requireComparisonSupport(c, options.compare); + // Validate the comparison selector before any VariaQ invocation: an + // invalid value must surface as a structured PluginError because the CLI + // wrapper only converts PluginError instances into command results (a raw + // ZodError would escape as an unhandled exception). + const compare = parseComparisonOption(options.compare); + await requireComparisonSupport(c, compare); const root = resolveReportOutputDir(c); const containedDir = await resolveContainedReportDir(root, options.outputDir); @@ -617,7 +634,7 @@ export default async function plugin(bb: BbPluginApi) { for (const key of options.groupBy) argv.push("--group-by", key); } if (options.scalingX !== undefined) argv.push("--scaling-x", options.scalingX); - if (options.compare !== undefined) argv.push("--compare", options.compare); + if (compare !== undefined) argv.push("--compare", compare); // `--plots` is conveyed only through the canonical formats list. if (options.overwrite) argv.push("--overwrite"); @@ -846,7 +863,7 @@ export default async function plugin(bb: BbPluginApi) { scaling_x: opts.get("scaling-x"), include_failed: args.includes("--include-failed"), include_unavailable: args.includes("--include-unavailable"), - compare: opts.get("compare") as import("./lib/schema.js").AnalyzeQuery["compare"], + compare: parseComparisonOption(opts.get("compare")), }); } case "analyze-campaign": { @@ -858,7 +875,7 @@ export default async function plugin(bb: BbPluginApi) { scaling_x: opts.get("scaling-x"), include_failed: args.includes("--include-failed"), include_unavailable: args.includes("--include-unavailable"), - compare: opts.get("compare") as import("./lib/schema.js").AnalyzeQuery["compare"], + compare: parseComparisonOption(opts.get("compare")), }); } case "report-campaign": { @@ -1313,6 +1330,37 @@ export default async function plugin(bb: BbPluginApi) { // ------------------------------------------------------------ CLI helpers -- +/** + * Validate a user-supplied comparison selector from the CLI boundary. + * + * Invalid values must produce a structured, actionable PluginError: the CLI + * wrapper only converts PluginError instances into command results (exit code + * + "message / Hint:" stderr), and rethrows everything else — a raw ZodError + * from schema.parse() would escape that handler as an unhandled exception. + */ +function parseComparisonOption(value: string | undefined): ComparisonType | undefined { + if (value === undefined) return undefined; + const parsed = comparisonTypeSchema.safeParse(value); + if (!parsed.success) { + throw new PluginError( + `Invalid comparison '${value}'.`, + `Valid comparisons: ${comparisonTypeSchema.options.join(", ")}.`, + ); + } + return parsed.data; +} + +/** + * Reduce a ZodError to its first issue as a short "path: message" string for + * embedding in a PluginError message. + */ +function minimizeZodIssue(error: z.ZodError): string { + const issue = error.issues[0]; + if (issue === undefined) return error.message; + const path = issue.path.length > 0 ? `${issue.path.join(".")}: ` : ""; + return `${path}${issue.message}`; +} + function readOptions(args: string[]): Map { const map = new Map(); for (let i = 0; i < args.length; i++) { diff --git a/tests/fake.test.ts b/tests/fake.test.ts index 8b21e01..16afd1d 100644 --- a/tests/fake.test.ts +++ b/tests/fake.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createFakePluginHost } from "@get-bb/plugin-sdk/testing"; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -669,6 +669,96 @@ describe("report campaign", () => { expect(cliResult.stdout).toContain("Report: report-"); expect(cliResult.stdout).toContain("generated files: 4"); }); + + it("keeps valid, invalid, and omitted --compare values on the structured CLI error path", async () => { + const h = await setup(); + const run = JSON.parse(String(await tool(h, "variaq_campaign_run", { + campaign_format_version: "1", + name: "compare-guard", + family: "maxcut", + problem_sizes: [4], + problem_seeds: [1], + solvers: ["exact"], + repeats: 1, + base_seed: 42, + generator_parameters: {}, + solver_config: {}, + maxRuns: 10, + }))); + + // Omitted: still valid, exit 0 with the human-readable summary. + const omitted = await cli(h, [ + "report-campaign", run.campaignId, "--output-dir", "compare-omitted", "--formats", "json", + ]); + expect(omitted.exitCode, omitted.stderr).toBe(0); + expect(String(omitted.stdout)).toContain("Report: report-"); + + // Valid comparison: accepted and actually forwarded to the VariaQ + // subprocess. The fake records the received value in the generated + // markdown artifact ("comparison: "); the plugin's typed result + // intentionally drops extra envelope fields, so read the artifact. + const valid = JSON.parse(String(await tool(h, "variaq_report_campaign", { + campaignId: run.campaignId, + outputDir: "compare-valid-tool", + formats: ["json", "markdown"], + compare: "classical_vs_quantum", + }))); + expect(valid.reportId).toMatch(/^report-/); + expect(readFileSync(valid.paths.markdown, "utf8")).toContain("comparison: classical_vs_quantum"); + + // Invalid comparison: structured CLI error, never a leaked ZodError. + const invalid = await cli(h, [ + "report-campaign", run.campaignId, "--output-dir", "compare-invalid", + "--formats", "json", "--compare", "totally-bogus", "--overwrite", "--json", + ]); + expect(invalid.exitCode, invalid.stderr).toBe(1); + expect(String(invalid.stderr)).toContain("Invalid comparison 'totally-bogus'"); + expect(String(invalid.stderr)).toContain("Valid comparisons: classical_vs_quantum, qiskit_vs_cudaq, gpu_vs_cpu, paired_solvers"); + expect(String(invalid.stderr)).not.toMatch(/ZodError/i); + }); + + it("rejects an invalid --compare on analyze commands as a structured CLI error", async () => { + const h = await setup(); + // Validation precedes any VariaQ invocation, so a non-existent campaign id + // must fail with the plugin's structured comparison error, not VariaQ's + // exit-2 lookup failure and not a leaked ZodError. + const badRuns = await cli(h, ["analyze-runs", "--run-id", "run-1", "--compare", "nope", "--json"]); + expect(badRuns.exitCode, badRuns.stderr).toBe(1); + expect(String(badRuns.stderr)).toContain("Invalid comparison 'nope'"); + expect(String(badRuns.stderr)).toContain("Valid comparisons:"); + expect(String(badRuns.stderr)).not.toMatch(/ZodError/i); + + const badCampaign = await cli(h, ["analyze-campaign", "campaign-missing", "--compare", "nope", "--json"]); + expect(badCampaign.exitCode, badCampaign.stderr).toBe(1); + expect(String(badCampaign.stderr)).toContain("Invalid comparison 'nope'"); + }); + + it("surfaces operation-level schema errors over the CLI as structured errors", async () => { + const h = await setup(); + // Nine --group-by values pass the comparison parser but violate + // analyzeQuerySchema's group_by max(8) inside the operation, exercising + // the safeParse -> structured PluginError conversion there. + const groupBy = ["a", "b", "c", "d", "e", "f", "g", "h", "i"]; + const badRuns = await cli(h, [ + "analyze-runs", "--run-id", "run-1", + ...groupBy.flatMap((key) => ["--group-by", key]), + "--json", + ]); + expect(badRuns.exitCode, badRuns.stderr).toBe(1); + expect(String(badRuns.stderr)).toContain("Invalid analyze-runs input: group_by: "); + expect(String(badRuns.stderr)).toContain("Hint:"); + expect(String(badRuns.stderr)).not.toMatch(/ZodError/i); + + const badCampaign = await cli(h, [ + "analyze-campaign", "campaign-schema-guard", + ...groupBy.flatMap((key) => ["--group-by", key]), + "--json", + ]); + expect(badCampaign.exitCode, badCampaign.stderr).toBe(1); + expect(String(badCampaign.stderr)).toContain("Invalid analyze-campaign input: group_by: "); + expect(String(badCampaign.stderr)).toContain("Hint:"); + expect(String(badCampaign.stderr)).not.toMatch(/ZodError/i); + }); }); describe("benchmark", () => { diff --git a/tests/fake_variaq.py b/tests/fake_variaq.py index 8c6e2d7..c8506d2 100755 --- a/tests/fake_variaq.py +++ b/tests/fake_variaq.py @@ -350,6 +350,7 @@ def main(argv): formats = ["json"] group_by = [] scaling_x = None + compare = None plots = False overwrite = False for i, a in enumerate(args): @@ -361,6 +362,8 @@ def main(argv): group_by.append(args[i + 1]) if a == "--scaling-x" and i + 1 < len(args): scaling_x = args[i + 1] + if a == "--compare" and i + 1 < len(args): + compare = args[i + 1] if a == "--plots": plots = True if a == "--overwrite": @@ -410,14 +413,21 @@ def main(argv): if "markdown" in formats: md_path = os.path.join(output_dir, f"{report_id}.md") with open(md_path, "w") as f: - f.write(f"# Report {report_id}\n") + # "comparison:" echoes the received --compare argv value + # (None when the flag was absent) so tests can assert that the + # plugin actually forwarded the comparison selector. + f.write(f"# Report {report_id}\ncomparison: {compare}\n") paths["markdown"] = md_path if "plots" in formats: plot_path = os.path.join(output_dir, f"{report_id}_plot.png") with open(plot_path, "wb") as f: f.write(b"PNG") paths["plots"] = {"summary": plot_path} - print(json.dumps(envelope("report campaign", "success", {"report_id": report_id, "paths": paths}))) + print(json.dumps(envelope("report campaign", "success", { + "report_id": report_id, + "paths": paths, + "compare": compare, + }))) return 0 if cmd == "capabilities": diff --git a/tests/server.test.ts b/tests/server.test.ts index cf2c512..370c70b 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -569,6 +569,27 @@ describeReal("failure paths", () => { }); }); +describeReal("invalid --compare values stay structured CLI errors", () => { + it("report-campaign and analyze-campaign reject invalid --compare before invoking VariaQ", async () => { + const h = await setupHost(); + // Validation precedes any VariaQ invocation, so a well-formed but + // non-existent campaign id must never reach VariaQ: exit 1 + structured + // message, not VariaQ's exit-2 lookup error and not a leaked ZodError. + const badReport = await cli(h, [ + "report-campaign", "campaign-compare-guard", "--output-dir", "compare-guard", + "--formats", "json", "--compare", "totally-bogus", "--overwrite", "--json", + ]); + expect(badReport.exitCode, badReport.stderr).toBe(1); + expect(String(badReport.stderr)).toContain("Invalid comparison 'totally-bogus'"); + expect(String(badReport.stderr)).toContain("Valid comparisons: classical_vs_quantum, qiskit_vs_cudaq, gpu_vs_cpu, paired_solvers"); + expect(String(badReport.stderr)).not.toMatch(/ZodError/i); + + const badAnalyze = await cli(h, ["analyze-campaign", "campaign-compare-guard", "--compare", "nope", "--json"]); + expect(badAnalyze.exitCode, badAnalyze.stderr).toBe(1); + expect(String(badAnalyze.stderr)).toContain("Invalid comparison 'nope'"); + }); +}); + describe.skipIf(!HAS_REAL_VARIAQ || !RUN_CUDAQ)("optional CUDA-Q CPU integration", () => { it("runs a bounded qpp-cpu solve when CUDA-Q is installed", async () => { const h = await setupHost();