From f11da3dd96c05cf8214e4a305e54dfc4d9101c0f Mon Sep 17 00:00:00 2001 From: Courier Date: Sat, 3 Oct 2026 17:48:09 +0000 Subject: [PATCH] fix(ci): scope the npm audit gate to production dependencies (#1166) .npmrc include=dev (added #428 for reproducible installs) overrides --omit in npm's config, so `npm audit --omit=dev` silently audited the dev tree. The dev-only braces advisory GHSA-vfj7-8cjw-p6xm (all versions <=3.0.3, no patched release; reached via eslint-config-next -> fast-glob -> micromatch) turned the Security Audit gate red on main. - audit script: explicit --include=prod replaces the .npmrc value so --omit=dev takes effect; production and optional deps stay audited (verified: a prod- or optional-installed vulnerable package is still reported) - security-audit.yaml: non-blocking dev-inclusive audit step keeps dev advisories visible without turning main red - package-audit.test.ts: regression guards for the flag pair, rationale note, and workflow wiring - SECURITY-ACCEPTED-RISKS.md: accept the dev-only braces advisory, correct the shipping-surface claims (main server image excludes it; -mcp image carries it as inert tooling), record the resolution --- .github/workflows/security-audit.yaml | 9 +++ SECURITY-ACCEPTED-RISKS.md | 18 +++++- package-audit.test.ts | 82 +++++++++++++++++++++++++++ package.json | 5 +- 4 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 package-audit.test.ts diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index a7d4ef90..026a74db 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -14,4 +14,13 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 - uses: ./.github/actions/setup-node - run: npm ci + # Blocking gate: production + optional dependencies only (#1166). The + # .npmrc include=dev would otherwise cancel --omit=dev, so the script + # passes an explicit --include=prod; see the "//".audit note in package.json. - run: npm run audit + # Non-blocking visibility: dev-inclusive audit so dev-only advisories + # (e.g. GHSA-vfj7-8cjw-p6xm in the lint chain) stay reported without + # turning main red again. + - name: npm audit (dev dependencies, informational) + continue-on-error: true + run: npm audit --include=dev --audit-level=high --fetch-retries=5 --fetch-timeout=120000 --fetch-retry-mintimeout=20000 --fetch-retry-maxtimeout=120000 diff --git a/SECURITY-ACCEPTED-RISKS.md b/SECURITY-ACCEPTED-RISKS.md index d198710c..fed83a9d 100644 --- a/SECURITY-ACCEPTED-RISKS.md +++ b/SECURITY-ACCEPTED-RISKS.md @@ -1,10 +1,10 @@ # Accepted Security Risks -**Last updated: 2026-08-15** +**Last updated: 2026-10-03** There are currently no accepted npm runtime advisories. -`npm audit --omit=dev` reports **0 vulnerabilities** across 17 production dependencies. +`npm audit --omit=dev --include=prod` reports **0 vulnerabilities** across the production dependency tree (17 direct production dependencies plus optional/native deps such as the Next.js platform binaries). ## Non-NPM Risks @@ -29,7 +29,7 @@ The following risks are tracked beyond npm advisories: - The project uses 17 production dependencies with transitive chains managed by npm. - Key deep-chain dependencies: `next` (framework), `@modelcontextprotocol/sdk` (MCP protocol), `prisma` / `@prisma/client` (ORM). -- **Mitigation:** Renovate keeps dependencies updated; `npm audit --omit=dev --audit-level=high` runs on every push to `main` and every pull request via `.github/workflows/security-audit.yaml` (separate from the main CI workflow) and fails the build on high/critical vulnerabilities. +- **Mitigation:** Renovate keeps dependencies updated; `npm audit --omit=dev --include=prod --audit-level=high` runs on every push to `main` and every pull request via `.github/workflows/security-audit.yaml` (separate from the main CI workflow) and fails the build on high/critical vulnerabilities. ### Groomer Autonomous Issue Rewrites (accepted risk) @@ -45,6 +45,17 @@ The following risks are tracked beyond npm advisories: - Rate limits on mutating endpoints (`src/lib/rate-limit.ts`) use module-level in-memory state; limits reset on restart and are not shared across replicas. - **Mitigation:** acceptable for the current single-node deployment; move to a shared store if the app is ever scaled horizontally. +## Dev-Only Advisories + +### braces stack-exhaustion DoS (GHSA-vfj7-8cjw-p6xm) + +- **Severity:** high. +- **Affected:** all published `braces` versions (`<=3.0.3`; no patched release as of 2026-10-03). +- **Reachability:** dev lint chain only — `eslint-config-next -> @next/eslint-plugin-next -> fast-glob -> micromatch -> braces`. +- **Shipping surface:** the main server runtime image installs production dependencies only (`Dockerfile` `prod-deps` stage: `npm ci --omit=dev`), so the chain is absent from the server image. Caveat: the separately published `-mcp` image runs from the `deps` stage tree (`npm ci`, devDependencies included, entrypoint `tsx`), so the lint chain is present there as inert tooling — the MCP server never invokes micromatch/braces. +- **Exploit path:** requires feeding attacker-controlled deeply-nested glob patterns to micromatch during lint tooling; no request path in the server or MCP image reaches it. +- **Decision:** accepted for the dev toolchain; the production audit gate is scoped with `--omit=dev --include=prod` (#1166), and dev advisories stay visible via the non-blocking dev-inclusive audit step in `.github/workflows/security-audit.yaml`. Revisit if an upstream patched release lands (then remove nothing — the scoped gate stays; optionally test whether the advisory clears). + ## Retired Risks The following previously accepted risks have been retired: @@ -60,3 +71,4 @@ The following previously accepted risks have been retired: |---|---|---| | Trivy action pinned to SHA | ✅ Resolved | `aquasecurity/trivy-action@ed142fd` (v0.36.0). The SHA pin is intentional: trivy is the release gate, so a floating tag must not reach a release build. Renovate's `github-tags` datasource cannot resolve a bare SHA pin (it only produced a `no-result` lookup failure on the dashboard), so the action is excluded from Renovate in `renovate.json` (`matchPackageNames: ["aquasecurity/trivy-action"]`, `enabled: false`) and is bumped manually, with the version comment, after reviewing an upstream release. | | `.npmrc` invalid omit config | ✅ Resolved | Fixed `omit=` → `omit=dev` | +| `npm audit` gate silently auditing the dev tree | ✅ Resolved (#1166) | `.npmrc` `include=dev` (added #428) overrides `--omit=dev` in npm's config, so `npm audit --omit=dev` audited devDependencies too. The dev-only `braces` advisory GHSA-vfj7-8cjw-p6xm (all versions, no fix) exposed it on main. Fixed by adding an explicit `--include=prod` to the `audit` script so `--omit=dev` takes effect (prod + optional still audited); regression-guarded by `package-audit.test.ts`. | diff --git a/package-audit.test.ts b/package-audit.test.ts new file mode 100644 index 00000000..b2ea29bf --- /dev/null +++ b/package-audit.test.ts @@ -0,0 +1,82 @@ +// @vitest-environment node +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +// #1166: `.npmrc` sets `include=dev` (added in #428) so `npm ci` installs +// devDependencies for lint/test. In npm's config the `include` set wins over +// the `omit` set, so `npm audit --omit=dev` silently audited the dev tree — +// the dev-only braces advisory GHSA-vfj7-8cjw-p6xm (all versions, no patched +// release) turned the gate red on main. The fix is an explicit `--include=prod` +// in the audit script, which cancels the .npmrc value and lets `--omit=dev` +// take effect; production and optional dependencies stay fully in scope. + +// The audit gate is now robust to .npmrc at any config level, so this test +// deliberately does NOT pin `include=dev` in .npmrc — CI installs pass +// `--include=dev` explicitly via .github/actions/setup-node/action.yml. + +// Vitest runs tests from the project root, so `process.cwd()` is the +// repository root regardless of where the test file lives. +const packageJsonPath = join(process.cwd(), "package.json"); + +function readJson(path: string) { + return JSON.parse(readFileSync(path, "utf8")) as Record; +} + +type PackageJson = { + scripts?: Record; + "//"?: Record; +}; + +describe("npm audit gate scoped to prod dependencies (#1166)", () => { + it("audit script contains --omit=dev", () => { + const pkg = readJson(packageJsonPath) as PackageJson; + const audit = pkg.scripts?.audit; + expect(audit, "package.json must declare an audit script").toBeDefined(); + expect(audit).toContain("--omit=dev"); + }); + + it("audit script contains --include=prod", () => { + const pkg = readJson(packageJsonPath) as PackageJson; + const audit = pkg.scripts?.audit; + expect(audit, "package.json must declare an audit script").toBeDefined(); + expect(audit).toContain("--include=prod"); + }); + + it("audit script contains --audit-level=high", () => { + const pkg = readJson(packageJsonPath) as PackageJson; + const audit = pkg.scripts?.audit; + expect(audit, "package.json must declare an audit script").toBeDefined(); + expect(audit).toContain("--audit-level=high"); + }); + + it("audit script carries the empirically validated adjacent flag pair `--omit=dev --include=prod`", () => { + const pkg = readJson(packageJsonPath) as PackageJson; + const audit = pkg.scripts?.audit; + expect(audit, "package.json must declare an audit script").toBeDefined(); + // The exact adjacent pair is what was validated on this lockfile: + // `--omit=dev` alone is silently overridden by .npmrc's `include=dev`. + expect(audit).toContain("--omit=dev --include=prod"); + }); + + it('package.json "//" block documents the audit scoping rationale', () => { + const pkg = readJson(packageJsonPath) as PackageJson; + const auditComment = pkg["//"]?.audit ?? ""; + expect( + auditComment.length, + '"//".audit rationale comment must be present and non-empty', + ).toBeGreaterThan(0); + expect(auditComment).toMatch(/#1166/); + expect(auditComment).toMatch(/GHSA/i); + }); + + it("Security Audit workflow still runs the blocking `npm run audit` gate", () => { + const workflow = readFileSync( + join(process.cwd(), ".github", "workflows", "security-audit.yaml"), + "utf8", + ); + expect(workflow).toContain("npm run audit"); + // The dev-inclusive pass must stay non-blocking (visibility, not a gate). + expect(workflow).toMatch(/continue-on-error:\s*true[\s\S]*npm audit --include=dev/); + }); +}); diff --git a/package.json b/package.json index 248b0ef4..4ee07a11 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "dev": "next dev", "build": "NODE_ENV=production next build", "start": "next start", - "audit": "npm audit --omit=dev --audit-level=high --fetch-retries=5 --fetch-timeout=120000 --fetch-retry-mintimeout=20000 --fetch-retry-maxtimeout=120000", + "audit": "npm audit --omit=dev --include=prod --audit-level=high --fetch-retries=5 --fetch-timeout=120000 --fetch-retry-mintimeout=20000 --fetch-retry-maxtimeout=120000", "lint": "NODE_ENV=development eslint .", "test": "NODE_ENV=development vitest run", "test:watch": "NODE_ENV=development vitest", @@ -63,7 +63,8 @@ "vitest": "^5.0.0" }, "//": { - "overrides": "These pins exist to remediate npm advisories (originally added in #350). DO NOT remove without verifying the originating transitive deps have shipped patched versions: postcss ^8.5.10 (XSS-class advisory), sharp ^0.35.0 (libvips CVE-2026-33327/33328/35590/35591, #675 — already satisfied transitively by next@16.3.0's optional dep `sharp: ^0.35.3`, but the override survives any future `next` downgrade), deepmerge-ts ^8.0.0 (GHSA-ggr8-5vv4-36mx stack-exhaustion — pulled in by @prisma/config <=6.13.0-dev.1 and the transitive prisma <=7.10.0-integration-fix-prisma-publish-token.1, #761), mysql2 ^3.22.0 (GHSA-3f6p-5ww8-9rcr plaintext-credential leak via mysql_clear_password auth-plugin downgrade — pulled in by the transitive prisma >=6.20.0-dev.1 / <=7.10.0, #897). npm validates every entry of `overrides` strictly, so this rationale lives at the top level rather than inside the `overrides` block." + "overrides": "These pins exist to remediate npm advisories (originally added in #350). DO NOT remove without verifying the originating transitive deps have shipped patched versions: postcss ^8.5.10 (XSS-class advisory), sharp ^0.35.0 (libvips CVE-2026-33327/33328/35590/35591, #675 — already satisfied transitively by next@16.3.0's optional dep `sharp: ^0.35.3`, but the override survives any future `next` downgrade), deepmerge-ts ^8.0.0 (GHSA-ggr8-5vv4-36mx stack-exhaustion — pulled in by @prisma/config <=6.13.0-dev.1 and the transitive prisma <=7.10.0-integration-fix-prisma-publish-token.1, #761), mysql2 ^3.22.0 (GHSA-3f6p-5ww8-9rcr plaintext-credential leak via mysql_clear_password auth-plugin downgrade — pulled in by the transitive prisma >=6.20.0-dev.1 / <=7.10.0, #897). npm validates every entry of `overrides` strictly, so this rationale lives at the top level rather than inside the `overrides` block.", + "audit": "The audit gate scopes to production dependencies. .npmrc sets include=dev so npm ci installs devDependencies for lint/test, and npm's include set overrides --omit, which silently pulled the dev tree into `npm audit --omit=dev` — the dev-only braces advisory GHSA-vfj7-8cjw-p6xm (all versions, no patched release, reached via eslint-config-next -> fast-glob -> micromatch) turned that latent bug red on main (#1166). The explicit CLI --include=prod replaces the .npmrc include value so --omit=dev takes effect; production and optional dependencies remain fully in scope (verified: a prod-installed vulnerable package is still reported). Guarded by package-audit.test.ts." }, "overrides": { "postcss": "^8.5.10",