From 55ce6fc957a3cf15a4ea0e4d256f42020cce5e3d Mon Sep 17 00:00:00 2001 From: Fangyuan_Zhang Date: Wed, 12 Aug 2026 18:06:20 -0700 Subject: [PATCH] fix(security): baseline the pre-existing fs-lint backlog + scope the gate to changed lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Security workflow's ESLint job lints whole changed FILES, so touching a large legacy file surfaces its entire pre-existing detect-non-literal-fs-filename backlog. That failed the v0.6.0 release push — 128 errors in src/commands/test.ts, none introduced by the release, on lines the release never touched. Two layers, with no rule severity lowered and no part of the tree exempted: - (C) Commit eslint-suppressions.json — ESLint's native bulk suppressions, auto-read from the repo root — recording today's backlog, so pre-existing findings are not re-reported. A genuinely new fs call still surfaces because the per-file count exceeds the baseline. Retire entries with `eslint --prune-suppressions` in a maintenance pass; never regenerate it to hide new findings. - (A) .github/scripts/filter-changed-line-findings.mjs then keeps only error findings on lines this diff actually added or modified, so touching a legacy file never fails on its untouched lines. The lint step now emits --format json and runs with --pass-on-unpruned-suppressions so fixing a baselined finding in a changed file doesn't fail the job over a now-stale suppression entry. --- .../scripts/filter-changed-line-findings.mjs | 93 +++++++++++ .github/workflows/security.yml | 63 +++++++- .prettierignore | 4 + eslint-suppressions.json | 147 ++++++++++++++++++ 4 files changed, 301 insertions(+), 6 deletions(-) create mode 100644 .github/scripts/filter-changed-line-findings.mjs create mode 100644 eslint-suppressions.json diff --git a/.github/scripts/filter-changed-line-findings.mjs b/.github/scripts/filter-changed-line-findings.mjs new file mode 100644 index 00000000..0c9cfd3e --- /dev/null +++ b/.github/scripts/filter-changed-line-findings.mjs @@ -0,0 +1,93 @@ +/** + * Fail the security lint only on ERROR-severity findings that land on lines + * THIS change actually added or modified. + * + * This is the "changed-line" half of the gate. The other half — the + * committed `eslint-suppressions.json` baseline — has already removed the + * pre-existing backlog from the ESLint report before it reaches this script, + * so what remains is genuinely new/excess findings; this step additionally + * narrows them to the diff's own lines so a large legacy file (e.g. + * `src/commands/test.ts`) does not fail a release just for being touched. + * + * Input: argv[2] = path to an ESLint JSON report (already baseline-filtered) + * env RESOLVED_BASE = base commit to diff against ('' = no base) + * env HEAD_SHA = head commit (default 'HEAD') + * Exit: 1 if any error finding falls on an added/changed line, else 0. + */ +import { readFileSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; + +const reportPath = process.argv[2]; +if (!reportPath) { + console.error('usage: filter-changed-line-findings.mjs '); + process.exit(2); +} + +const base = (process.env.RESOLVED_BASE || '').trim(); +const head = (process.env.HEAD_SHA || 'HEAD').trim(); +const root = process.cwd(); +const report = JSON.parse(readFileSync(reportPath, 'utf8')); + +/** + * The set of line numbers this diff added/modified in `relPath`, parsed from + * `git diff --unified=0` hunk headers (`@@ -a,b +c,d @@` → lines c..c+d-1). + * Returns null to mean "keep every finding" — used when there is no base to + * diff against (a genuine first commit / full-tree fallback) or the diff + * cannot be computed, so nothing new is ever silently hidden. + */ +function addedLines(relPath) { + if (!base) return null; + let out; + try { + out = execFileSync( + 'git', + ['diff', '--unified=0', '--diff-filter=ACMR', base, head, '--', relPath], + { encoding: 'utf8' }, + ); + } catch { + return null; + } + const lines = new Set(); + for (const line of out.split('\n')) { + const m = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!m) continue; + const start = Number(m[1]); + const count = m[2] === undefined ? 1 : Number(m[2]); + // count === 0 is a pure deletion at that position — no added line. + for (let i = 0; i < count; i++) lines.add(start + i); + } + return lines; +} + +const offenders = []; +for (const result of report) { + const errors = (result.messages || []).filter(m => m.severity === 2); + if (errors.length === 0) continue; + const rel = result.filePath.startsWith(`${root}/`) + ? result.filePath.slice(root.length + 1) + : result.filePath; + const scope = addedLines(rel); + for (const m of errors) { + if (scope === null || scope.has(m.line)) { + offenders.push(`${rel}:${m.line}:${m.column} ${m.ruleId} ${m.message}`); + } + } +} + +if (offenders.length > 0) { + console.error( + `Security lint: ${offenders.length} finding(s) on lines this change added/modified ` + + `(and not in eslint-suppressions.json):\n`, + ); + for (const o of offenders) console.error(` ${o}`); + console.error( + '\nFix them, or if the call is genuinely safe, disable the specific rule at that line ' + + 'with a justification comment. Do NOT regenerate the baseline to hide new findings.', + ); + process.exit(1); +} + +console.log( + 'Security lint: no new findings on changed lines. ' + + '(The pre-existing backlog is baselined in eslint-suppressions.json.)', +); diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 55b36f6a..d980e5a7 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -20,10 +20,26 @@ # ~380 pre-existing findings (almost entirely # `security/detect-non-literal-fs-filename` on ordinary local-path fs # calls a config-file-reading CLI makes routinely — not a real -# vulnerability signal for this codebase shape). Blocking on that -# backlog on day one would make the job noise from the first run; this -# keeps every rule at its designed severity and holds new/changed code to -# it without silently exempting the existing tree from the rule. +# vulnerability signal for this codebase shape). +# +# ESLint lints whole FILES, though, so "changed files only" still dumps a +# legacy file's entire backlog the moment one line of it is touched — which +# is exactly what happened on the v0.6.0 release push (touching the ~11k-line +# `src/commands/test.ts` surfaced 128 pre-existing `detect-non-literal-fs- +# filename` errors and failed the job). Two layers fix that WITHOUT lowering +# any rule's severity or exempting the tree: +# (C) A committed `eslint-suppressions.json` baseline (ESLint's native bulk +# suppressions, auto-read from repo root) records today's backlog, so +# pre-existing findings are not re-reported; a genuinely NEW fs call +# pushes the per-file count past the baseline and surfaces. +# (A) `.github/scripts/filter-changed-line-findings.mjs` then keeps only the +# remaining error findings that fall on lines THIS diff added/modified, +# so touching a legacy file never fails on its untouched lines. +# Net: every rule stays at its designed severity, new/changed code is held to +# it, and the existing tree is neither re-linted from scratch nor exempted. +# To retire baseline entries as they get fixed, run ESLint with +# `--prune-suppressions` in a maintenance PR (never regenerate it to hide new +# findings). # - Secret scanning: this repo's `ci.yml` already runs a gitleaks # WORKING-TREE scan on every PR/push (added by atlas #274, 2026-07). This # file adds the complementary FULL-HISTORY scan from #220 as its own job, @@ -165,6 +181,11 @@ jobs: echo "Base used: ${BASE_DESC}" + # Persist the resolved base for the lint step's changed-line filter + # (empty string on the full-tree fallback — the filter treats that as + # "keep every finding", so nothing new is hidden). + printf '%s' "${RESOLVED_BASE}" > "$RUNNER_TEMP/resolved-base.txt" + if [ -n "$RESOLVED_BASE" ]; then git diff --name-only --diff-filter=ACMR -z "$RESOLVED_BASE" "$HEAD_SHA" -- ':(glob)src/**/*.ts' \ > "$RUNNER_TEMP/changed-ts-files.txt" @@ -207,8 +228,38 @@ jobs: fi FILE_COUNT=$(tr -dc '\0' < "$RUNNER_TEMP/changed-ts-files.txt" | wc -c) echo "Linting ${FILE_COUNT} file(s)." - xargs -a "$RUNNER_TEMP/changed-ts-files.txt" -0 \ - npx eslint --config eslint.security.config.mjs --format stylish -- + + # Read the NUL-separated list into an array (NUL-safe, single ESLint + # invocation so `--format json` emits ONE array) and lint with: + # - the committed eslint-suppressions.json baseline (auto-read from + # repo root) so the pre-existing backlog is not re-reported; and + # - --pass-on-unpruned-suppressions so FIXING a baselined finding in + # a changed file (its count drops) doesn't fail the job over a now- + # stale suppression entry (prune it in a separate maintenance pass). + # The trailing `--` stops ESLint's option parser at the file boundary. + mapfile -d '' -t FILES < "$RUNNER_TEMP/changed-ts-files.txt" + + # ESLint exits 1 when it reports findings and 2 on a fatal (config/crash) + # error; only 2 is a real failure here, because the changed-line filter + # below — not ESLint's own exit — decides pass/fail. Capture the report + # even on exit 1, but surface a genuine crash. + set +e + npx eslint --config eslint.security.config.mjs \ + --pass-on-unpruned-suppressions --format json -- "${FILES[@]}" \ + > "$RUNNER_TEMP/eslint-report.json" + ESLINT_EXIT=$? + set -e + if [ "$ESLINT_EXIT" -gt 1 ]; then + echo "::error title=Security lint crashed::ESLint exited ${ESLINT_EXIT} (fatal error, not a lint finding)." + cat "$RUNNER_TEMP/eslint-report.json" || true + exit "$ESLINT_EXIT" + fi + + # Fail only on findings that land on lines this change added/modified + # (see the header note and .github/scripts/filter-changed-line-findings.mjs). + RESOLVED_BASE="$(cat "$RUNNER_TEMP/resolved-base.txt")" \ + HEAD_SHA="${{ github.sha }}" \ + node .github/scripts/filter-changed-line-findings.mjs "$RUNNER_TEMP/eslint-report.json" # ── 4. Full-history secret scan (push only) ──────────────────────────────── # Complements ci.yml's working-tree gitleaks job (every PR/push) with the diff --git a/.prettierignore b/.prettierignore index 0350ddf0..95a57bd1 100644 --- a/.prettierignore +++ b/.prettierignore @@ -3,6 +3,10 @@ coverage/ node_modules/ *.tgz package-lock.json +# ESLint-generated (bulk suppressions baseline) — written/rewritten by +# `eslint --suppress-all` / `--prune-suppressions`, so leave it in ESLint's own +# format rather than fighting it with Prettier on every regeneration. +eslint-suppressions.json .claude/worktrees/ # Internal design docs (dropped wholesale from the public snapshot) — not Prettier-governed diff --git a/eslint-suppressions.json b/eslint-suppressions.json new file mode 100644 index 00000000..99761e38 --- /dev/null +++ b/eslint-suppressions.json @@ -0,0 +1,147 @@ +{ + "src/commands/agent.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 7 + } + }, + "src/commands/agent.ts": { + "security/detect-non-literal-fs-filename": { + "count": 4 + } + }, + "src/commands/project.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 3 + } + }, + "src/commands/project.ts": { + "security/detect-non-literal-fs-filename": { + "count": 6 + } + }, + "src/commands/test.artifact.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 12 + } + }, + "src/commands/test.cancel.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/commands/test.create-batch-run.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 3 + } + }, + "src/commands/test.flaky.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/commands/test.quickwins.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 20 + } + }, + "src/commands/test.rerun.closure-fanout.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/commands/test.rerun.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 3 + } + }, + "src/commands/test.result.history.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/commands/test.run.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 4 + } + }, + "src/commands/test.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 92 + } + }, + "src/commands/test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 29 + } + }, + "src/commands/test.wait.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/lib/agent-targets.ts": { + "security/detect-non-literal-fs-filename": { + "count": 1 + } + }, + "src/lib/bundle.commit.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 19 + } + }, + "src/lib/bundle.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 18 + } + }, + "src/lib/bundle.ts": { + "security/detect-non-literal-fs-filename": { + "count": 23 + } + }, + "src/lib/credentials.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 17 + } + }, + "src/lib/credentials.ts": { + "security/detect-non-literal-fs-filename": { + "count": 14 + } + }, + "src/lib/junit-report.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 4 + } + }, + "src/lib/junit-report.ts": { + "security/detect-non-literal-fs-filename": { + "count": 6 + } + }, + "src/lib/plan-schema.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 1 + } + }, + "src/lib/skill-nudge.ts": { + "security/detect-non-literal-fs-filename": { + "count": 1 + } + }, + "src/lib/telemetry.spec.ts": { + "security/detect-non-literal-fs-filename": { + "count": 2 + } + }, + "src/lib/update-check.test.ts": { + "security/detect-non-literal-fs-filename": { + "count": 1 + } + }, + "src/lib/update-check.ts": { + "security/detect-non-literal-fs-filename": { + "count": 3 + } + } +} \ No newline at end of file