diff --git a/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW-FIX.md b/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW-FIX.md new file mode 100644 index 0000000..758914d --- /dev/null +++ b/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW-FIX.md @@ -0,0 +1,160 @@ +--- +phase: 16-ci-dependency-audit-and-security-checks +fixed_at: 2026-06-13T00:00:00Z +review_path: .planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md +iteration: 1 +findings_in_scope: 9 +fixed: 6 +skipped: 3 +status: partial +--- + +# Phase 16: Code Review Fix Report + +**Fixed at:** 2026-06-13 +**Source review:** .planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 9 (CR-01; WR-01..WR-05; IN-01..IN-04 — IN-04 carries no fix) +- Fixed: 6 (CR-01, WR-01, WR-02, WR-03, WR-04, WR-05, plus IN-01) +- Skipped: 3 (IN-02, IN-03, IN-04 — all optional/no-action) + +IN-01 was applied inside the CR-01 commit (same file, `scripts/check-audit.mjs`), +so it does not get a standalone commit line below but is counted as fixed. + +## Validation + +All validation run inside the isolated review-fix worktree: + +- `pnpm lint` — PASS (apps/api + apps/pwa, `--max-warnings 0`). Note: the project + lint scope does not include `scripts/*.mjs`, so the edited `.mjs` scripts are + syntax-checked via `node -c` instead (all pass). +- `pnpm typecheck` — PASS (apps/api + apps/pwa `tsc --noEmit`, PWA e2e tsconfig). +- `node --test scripts/__tests__/check-audit.test.mjs` — 8 pass / 0 fail + (includes 3 new expired-waiver / isWaived assertions for CR-01). +- `python3 -c 'yaml.safe_load(...)'` for `.gitea/workflows/ci.yml` and + `.gitea/workflows/publish.yml` — both parse OK after edits. +- Working tree clean after all commits (no stray/uncommitted changes). + +The API DB-integration tests (`tests/routes/lists.test.ts`, +`tests/lib/listAccess.test.ts`, etc.) fail with `ER_ACCESS_DENIED` in this +environment because no dev MariaDB is reachable. That is pre-existing/environmental +and unrelated to these fixes; no new test failures were introduced. + +## Fixed Issues + +### CR-01: Audit-waiver `expires` field is decorative — expired waivers never re-block + +**Files modified:** `scripts/check-audit.mjs`, `scripts/__tests__/check-audit.test.mjs` +**Commit:** 4bb205f +**Applied fix:** Added an exported `isWaived(adv, allowlist)` predicate. An allowlist +entry with no `expires`, or a future `expires`, waives the advisory; an entry whose +`expires` is in the past (`<= Date.now()`) is treated as absent so the High/Critical +advisory re-blocks. Applied `isWaived` in BOTH `selectBlocking` and +`partitionAdvisories` (replacing the bare `!allowlist[...]` checks). Added three +unit tests: expired waiver re-blocks via `selectBlocking`, expired waiver re-blocks +via `partitionAdvisories`, and a direct `isWaived` truth-table test (future expiry → +waived, past expiry → not waived, no-expiry → waived, missing → not waived). All 8 +tests pass. + +### WR-01: `github.base_ref` interpolated into a shell command (script-injection vector) + +**Files modified:** `.gitea/workflows/ci.yml` +**Commit:** 26a6b2e +**Applied fix:** Bound `github.event.pull_request.base.sha`, `head.sha`, and +`github.base_ref` through an `env:` block (`PR_BASE_SHA`, `PR_HEAD_SHA`, +`PR_BASE_REF`) on the "Probe PR base/head SHA" step. The `run:` body now references +only the already-quoted shell variables — no `${{ ... }}` context interpolation +inside the script. The merge-base fallback uses `git rev-parse "origin/$PR_BASE_REF"`. + +### WR-03: `HEAD_SHA` has no fallback while `BASE_SHA` does — asymmetric defense + +**Files modified:** `.gitea/workflows/ci.yml` +**Commit:** 26a6b2e (committed together with WR-01 — same step in the same file) +**Applied fix:** Added a symmetric head fallback (`if [ -z "$HEAD_SHA" ]; then +HEAD_SHA=$(git rev-parse HEAD); fi`) and an `echo "Secret-scan range: +${BASE_SHA}..${HEAD_SHA}"` line before the gitleaks invocation so the scanned range +is logged rather than relying on git's `A..` → `A..HEAD` default. + +### WR-02: Boot-smoke false-PASS if a regressed image emits ≥20 lines before binding + +**Files modified:** `.gitea/workflows/publish.yml` +**Commit:** 3daa351 +**Applied fix:** Captured `docker run` output into `OUT=$(...)` and read `EXIT=$?` +from the docker command directly (no `| head -20` in the exit-bearing command), so a +chatty-but-booting image can no longer SIGPIPE docker to exit 141 and false-PASS. +`head -20` is now applied only to the printed `echo "$OUT"`. Treat both `0` and `124` +as FAIL ("did not refuse boot"). Added a positive belt-and-suspenders assertion: +the output must contain `DEV_AUTH_BYPASS=true is set in a production environment` +(the exact D-08 guard marker from `bootGuards.ts`), so a refusal for an unrelated +reason cannot masquerade as the guard working. + +> Logic note: this change alters the PASS/FAIL decision logic of a security smoke +> test. The shell logic was reviewed against the guard marker string, but the actual +> container behavior under the forbidden env is not exercisable in this environment +> (no docker daemon / built image). Recommend a human confirm the smoke step on a +> real publish run. + +### WR-05: Static `.dockerignore` assertions use unquoted-regex `grep` (false-positive prone) + +**Files modified:** `.gitea/workflows/publish.yml` +**Commit:** 3daa351 (committed together with WR-02 — same file) +**Applied fix:** Switched the per-pattern assertion to comment-stripped, fixed-string +matching: `grep -v '^[[:space:]]*#' .dockerignore | grep -qF "$pattern"`. Patterns +are no longer treated as regexes (`.env` can't match `denv`) and a commented-out +rule (`# .env was here`) no longer satisfies the check. Failure message updated to +"missing active rule". + +### WR-04: AUDIT-ADVISORY tier in the outdated report is effectively dead code + +**Files modified:** `scripts/check-outdated.mjs` +**Commit:** 3e609b2 +**Applied fix:** Took the SAFE relabel option (no risky full-tree rewrite). Renamed +the tier from `AUDIT-ADVISORY` to `OUTDATED-WITH-ADVISORY` and documented, in the +file header, the inline classification comment, and the printed header text, that it +only matches outdated *direct* deps against advisory `module_name`s (most advisories +are on transitive deps, so it rarely fires) and that the authoritative advisory gate +is `check-audit.mjs`. The report still always exits 0 (advisory-only). The +audit-parse-failure warning message was updated to the new tier name. + +## Skipped Issues + +### IN-02: `pnpm audit --json` is run twice per CI security job + +**File:** `scripts/check-audit.mjs:84`, `scripts/check-outdated.mjs:65` +**Reason:** Skipped — explicitly optional and out of v1 performance scope per REVIEW.md +("Out of v1 performance scope and harmless"). The suggested fix (pipe one audit pass +to both scripts via stdin/arg, or merge the two scripts) is a structural change to +script interfaces and CI invocation with no correctness benefit; applying it here +would be speculative scope creep. +**Original issue:** Both scripts independently spawn `pnpm audit --json`, doubling +the audit work in the security job. + +### IN-03: `outdated-pins.json` reasons are not cross-checked against the audit allowlist + +**File:** `scripts/outdated-pins.json` / `scripts/audit-allowlist.json` +**Reason:** Skipped — explicitly optional ("Documentation-level coupling only"). +Both JSON files are flat maps that the consuming scripts iterate directly: +`check-outdated.mjs` reads `pins[pkgName]` as a pin reason, and `check-audit.mjs` +reads `allowlist[github_advisory_id]`. Injecting a meta `__note`/cross-reference key +risks the consumers misreading it as real data (a `__note` pin would be treated as a +pin reason if a package were ever named `__note`). The safer choice is to leave the +data files as pure data rather than add inert-but-fragile meta keys. The suggested +lint-step variant is net-new tooling, out of scope for a review fix. +**Original issue:** Two independent suppression lists with no linkage between a +pinned package and an audit waiver for the same package. + +### IN-04: `expand.test.ts` is in scope but unrelated to this CI/security phase + +**File:** `apps/api/tests/broker/expand.test.ts` +**Reason:** Skipped — no action required. REVIEW.md states "**Fix:** None." The +reviewer found no defects; the file appears in the review set only because it was +touched/moved and is orthogonal to this phase. +**Original issue:** Well-constructed test noted for completeness; no defect. + +--- + +_Fixed: 2026-06-13_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_ diff --git a/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md b/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md index b272967..297a417 100644 --- a/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md +++ b/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md @@ -2,246 +2,117 @@ phase: 16-ci-dependency-audit-and-security-checks reviewed: 2026-06-13T00:00:00Z depth: standard -files_reviewed: 17 +files_reviewed: 5 files_reviewed_list: - - .dockerignore - .gitea/workflows/ci.yml - .gitea/workflows/publish.yml - - .gitleaks.toml - - apps/api/Dockerfile - - apps/api/src/index.ts - - apps/api/src/lib/bootGuards.ts - - apps/api/tests/broker/expand.test.ts - - apps/api/tests/lib/bootGuards.test.ts - - eslint.config.js - - package.json - - scripts/__tests__/check-audit.test.mjs - - scripts/audit-allowlist.json - scripts/check-audit.mjs - scripts/check-outdated.mjs - - scripts/gitleaks-baseline.json - - scripts/outdated-pins.json + - scripts/__tests__/check-audit.test.mjs findings: - critical: 1 - warning: 5 - info: 4 - total: 10 -status: issues_found + critical: 0 + warning: 0 + info: 1 + total: 1 +status: clean --- # Phase 16: Code Review Report **Reviewed:** 2026-06-13 **Depth:** standard -**Files Reviewed:** 17 -**Status:** issues_found +**Files Reviewed:** 5 +**Status:** clean ## Summary -CI / dependency-audit / security-hardening phase. The boot-guard logic -(`bootGuards.ts`) is correct and correctly ordered as the first statement inside -`isMainModule()` in `index.ts`. The audit wrapper (`check-audit.mjs`) parses the -real `pnpm audit --json` shape correctly, keys the allowlist on -`github_advisory_id` (verified against live audit output: the High advisory -`GHSA-gv7w-rqvm-qjhr` is present and is waived, so the gate currently passes), -and fails closed (exit 2) on parse errors. The publish boot-smoke `set +e` + -`pipefail` interaction was verified to propagate the container exit code through -the `| head -20` pipe correctly. +Iteration-2 re-review of the fixer's changes to the 5 in-scope files. Every prior +finding that targeted these files (CR-01, WR-01, WR-02, WR-03, WR-04, WR-05, +IN-01) is genuinely resolved, and the fixes introduced no Critical/Warning +regression. The previously-accepted skipped INFO items (IN-02, IN-03, IN-04) are +not re-litigated. -Findings concentrate in three areas: (1) the audit allowlist's `expires` field -is **never enforced** — an expired security waiver silently keeps suppressing -High/Critical advisories forever (BLOCKER); (2) Gitea Actions context expressions -(notably `github.base_ref`) are interpolated directly into shell script bodies, -the canonical script-injection anti-pattern that `publish.yml` itself avoids for -secrets but `ci.yml` does not for context values; (3) several robustness gaps in -the boot-smoke and the outdated cross-check tier. +Verification performed: +- `node --test scripts/__tests__/check-audit.test.mjs` → 8 pass / 0 fail + (includes the 3 new expired-waiver / `isWaived` truth-table assertions). +- Both workflow files parse under `yaml.safe_load`. +- `node -c` syntax-clean on both `.mjs` scripts. +- Guard marker string asserted by the boot-smoke + (`DEV_AUTH_BYPASS=true is set in a production environment`) confirmed to match + the literal in `apps/api/src/lib/bootGuards.ts:29`. I was not given a `` block, so there is no fallow substrate section. -## Critical Issues +### Prior-finding verification -### CR-01: Audit-waiver `expires` field is decorative — expired waivers never re-block +- **CR-01 (resolved).** `isWaived(adv, allowlist)` (`scripts/check-audit.mjs:37-43`) + returns false for a missing entry and for a past/equal `expires` + (`Date.parse(w.expires) <= Date.now()`), true for future/no `expires`. Wired into + BOTH `selectBlocking` (line 55) and `partitionAdvisories` (line 72). Tests + exercise expired-waiver re-block via both functions plus a direct `isWaived` + truth-table. The time-boxed waiver is now actually time-boxed. +- **IN-01 (resolved).** `isMainModule()` (`scripts/check-audit.mjs:86-93`) now + compares `realpathSync(process.argv[1])` to the resolved `__filename`, mirroring + `index.ts`. A symlinked/non-canonical entrypoint no longer silently skips the gate. +- **WR-01 (resolved).** `github.event.pull_request.base.sha`, `head.sha`, and + `github.base_ref` are bound through `env:` (`ci.yml:365-368`) and referenced only + as already-quoted shell variables (`$PR_BASE_SHA`, `$PR_HEAD_SHA`, `$PR_BASE_REF`). + No `${{ ... }}` context value is interpolated into the rendered `run:` body. The + merge-base fallback uses `git rev-parse "origin/$PR_BASE_REF"` (quoted). The + script-injection vector is closed. +- **WR-03 (resolved).** Symmetric head fallback added (`ci.yml:382-386`): + `if [ -z "$HEAD_SHA" ]; then HEAD_SHA=$(git rev-parse HEAD); fi`, plus an explicit + `echo "Secret-scan range: ${BASE_SHA}..${HEAD_SHA}"` (line 387). The gitleaks step + consumes both via `$GITHUB_ENV` under `set -u`, so a missing range fails loudly + rather than scanning a silently-wrong range. +- **WR-02 (resolved).** Boot-smoke now captures `OUT=$(timeout 15 docker run ...)` + and `EXIT=$?` directly (`publish.yml:132-137`), with `head -20` applied only to the + display `echo` (line 139). A chatty-but-booting regressed image can no longer + SIGPIPE docker to exit 141 and false-PASS. 0/124 → FAIL, and a positive + belt-and-suspenders grep requires the exact D-08 guard marker (line 147). +- **WR-04 (resolved, relabel option).** The tier is relabeled `OUTDATED-WITH-ADVISORY` + with an honest sub-line and header doc (`check-outdated.mjs:4-11, 144-145`) + stating it only cross-checks outdated *direct* deps against advisory + `module_name`s, rarely fires, and that `check-audit.mjs` is the authoritative gate. + The misleading implied check is gone. +- **WR-05 (resolved).** Per-pattern `.dockerignore` assertion now strips comment + lines and fixed-string matches (`publish.yml:103-109`): + `grep -v '^[[:space:]]*#' .dockerignore | grep -qF "$pattern"`. `.env` no longer + regex-matches `denv`, and a commented-out rule no longer satisfies the check. -**File:** `scripts/check-audit.mjs:32-38, 48-61` (and `scripts/audit-allowlist.json:5`) -**Issue:** `selectBlocking` and `partitionAdvisories` waive an advisory using only -`!allowlist[adv.github_advisory_id]`. The `expires` date stored in the allowlist -(`"expires": "2026-09-01"`) is never read. After the expiry date the High/Critical -advisory `GHSA-gv7w-rqvm-qjhr` (esbuild) will continue to be suppressed indefinitely, -silently defeating the entire point of a time-boxed security waiver. The gate's -security guarantee degrades to "any GHSA ever added to the allowlist is permanently -ignored." The unit tests (`check-audit.test.mjs`) reinforce the gap — none of them -exercise an expired waiver. -**Fix:** Treat an expired waiver as absent. Evaluate expiry inside the predicate: -```js -function isWaived(adv, allowlist) { - const w = allowlist[adv.github_advisory_id]; - if (!w) return false; - // No expiry or future expiry → waived; past expiry → NOT waived (re-blocks). - if (w.expires && Date.parse(w.expires) <= Date.now()) return false; - return true; -} +### Regression check on the fixes -export function selectBlocking(advisories, allowlist) { - return Object.values(advisories).filter( - (adv) => BLOCKING_SEVERITIES.has(adv.severity) && !isWaived(adv, allowlist), - ); -} -``` -Apply the same `isWaived` check in `partitionAdvisories`, and add a unit test for -an expired-waiver fixture asserting it is blocking. - -## Warnings - -### WR-01: `github.base_ref` interpolated into a shell command (script-injection vector) - -**File:** `.gitea/workflows/ci.yml:369` (also `base.sha`/`head.sha` at lines 363-366) -**Issue:** `BASE_SHA=$(git merge-base "$(git rev-parse origin/${{ github.base_ref }})" HEAD)` -splices the attacker-influenceable PR target branch name directly into the rendered -shell body. This is the exact script-injection anti-pattern `publish.yml:64-75` -deliberately avoids for `REGISTRY_PAT` ("Bind the secret through env: so it is never -substituted into the rendered script body"). `base.sha`/`head.sha` are Git-validated -SHAs (low risk), but `base_ref` is a branch name and Gitea permits a broad charset. -A target branch name containing shell metacharacters would execute in the runner. -**Fix:** Bind context values through `env:` and reference them as already-quoted -shell variables, never inline `${{ ... }}` in `run:`: -```yaml - - name: Probe PR base/head SHA - env: - PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - PR_BASE_REF: ${{ github.base_ref }} - run: | - set -euo pipefail - BASE_SHA="$PR_BASE_SHA" - HEAD_SHA="$PR_HEAD_SHA" - if [ -z "$BASE_SHA" ]; then - BASE_SHA=$(git merge-base "$(git rev-parse "origin/$PR_BASE_REF")" HEAD) - fi - { echo "BASE_SHA=$BASE_SHA"; echo "HEAD_SHA=$HEAD_SHA"; } >> "$GITHUB_ENV" -``` - -### WR-02: Boot-smoke false-PASS if a regressed image emits ≥20 lines before binding - -**File:** `.gitea/workflows/publish.yml:125-140` -**Issue:** `timeout 15 docker run ... 2>&1 | head -20` followed by `EXIT=$?`. The exit -semantics handle 0 (FAIL), 124 (FAIL) and "other non-zero" (PASS). But if the guard -regresses and the image boots **and is chatty** (web-push VAPID-unset warning, -worker startup logs, "FamilySync API running…"), it can emit 20 lines fast; `head` -then closes the pipe and SIGPIPEs `docker`, making the pipeline exit `141` -(128+SIGPIPE). `141` is neither 0 nor 124, so the broken-guard image is reported as -PASS. The smoke test's stated contract is "a container that BOOTS under the forbidden -env must FAIL" — this hole lets a chatty boot slip through. -**Fix:** Capture docker's exit code directly rather than the pipeline's, and treat -"started" as anything that did not exit before the timeout. For example, drop the -`| head` from the exit-bearing command and tee logs separately, or invert the test -to a positive assertion: -```bash - set +e - OUT=$(timeout 15 docker run --rm \ - --env NODE_ENV=production --env DEV_AUTH_BYPASS=true "$IMAGE" 2>&1) - EXIT=$? - set -e - echo "$OUT" | head -20 - # 124 (timeout) or 0 (clean start) both mean the guard did NOT refuse boot. - if [ "$EXIT" -eq 0 ] || [ "$EXIT" -eq 124 ]; then - echo "FAIL: image did not refuse dev-bypass in production (exit $EXIT)"; exit 1 - fi - # Belt-and-suspenders: require the FATAL marker in the output. - echo "$OUT" | grep -q "DEV_AUTH_BYPASS=true is set in a production environment" \ - || { echo "FAIL: refused boot but not via the expected guard (exit $EXIT)"; exit 1; } -``` - -### WR-03: `HEAD_SHA` has no fallback while `BASE_SHA` does — asymmetric defense - -**File:** `.gitea/workflows/ci.yml:365-373` -**Issue:** The step adds a `merge-base` fallback when `base.sha` is empty (Gitea version -parity), but applies no equivalent guard when `head.sha` is empty. If `head.sha` is -empty on a given Gitea version, the gitleaks range `--no-merges ${BASE_SHA}..` is -emitted (line 390). `A..` happens to default to `A..HEAD` in git, so the scan still -runs — but only by luck of git's range parsing, not by design, and the failure is -silent (no log of which range was scanned). Given this job is the secret-scan gate, -a silently-wrong range is a real risk. -**Fix:** Mirror the base fallback: `if [ -z "$HEAD_SHA" ]; then HEAD_SHA=$(git rev-parse HEAD); fi`, -and echo the final `${BASE_SHA}..${HEAD_SHA}` range before invoking gitleaks. - -### WR-04: AUDIT-ADVISORY tier in the outdated report is effectively dead code - -**File:** `scripts/check-outdated.mjs:62-76, 105, 116-117` -**Issue:** `vulnerableModules` is built from `pnpm audit` `module_name` values (which -are mostly **transitive** packages — verified live: the only advisories are on -`esbuild`, a transitive dep). `outdatedData` keys come from `pnpm outdated`, which -lists only **direct/top-level** dependencies (verified live: `hono`, `@types/react`, -`eslint`, …). The two sets almost never intersect, so the `hasAdvisory` branch — the -report's highest-priority tier — will essentially never fire. The report claims to -surface "packages with active advisories on the pinned version" but cannot, because -the advisory subject (esbuild) never appears in the outdated list. This is advisory- -only (never gates), hence WARNING not BLOCKER, but the tier is misleading. -**Fix:** Either cross-check against the full installed dependency tree (e.g. walk -`pnpm list -r --json` and match transitive advisory `module_name`s), or relabel/remove -the tier so the report does not imply a check it does not perform. - -### WR-05: Static `.dockerignore` assertions use unquoted-regex `grep` (false-positive prone) - -**File:** `.gitea/workflows/publish.yml:100-105` -**Issue:** `grep -q "$pattern"` treats each pattern as a regex, so `.env` matches -`denv`, `.git` matches `xgit`, `.planning` matches `Xplanning`, etc. More importantly -the assertion only checks that the *substring* appears **anywhere** in `.dockerignore`, -not that it is an effective ignore rule. A commented-out line (`# .env was here`) would -satisfy the check while ignoring nothing — the hygiene gate would pass on a regressed -ignore file. The gate's purpose is to prevent secrets/tests leaking into the image; -a string-presence check is weaker than that promise. -**Fix:** Use fixed-string, anchored matching and ignore comment lines: -```bash - for pattern in ".env" "node_modules" "apps/api/scripts" ".git" \ - ".planning" "apps/api/tests" "apps/pwa/e2e"; do - if ! grep -v '^[[:space:]]*#' .dockerignore | grep -qF "$pattern"; then - echo "FAIL: .dockerignore missing active rule: $pattern"; exit 1 - fi - done -``` +- `ci.yml` env-binding: with `set -u`, an empty `PR_BASE_REF` + empty `base.sha` + makes `git rev-parse "origin/"` fail and the step fails closed — acceptable. +- `publish.yml` boot-smoke: the only residual edge is that under `pipefail` a very + large `OUT` could SIGPIPE the display `echo "$OUT" | head -20` and fail the step. + That direction is fail-safe (blocks a good image, never false-PASSes a bad one) + and strictly more conservative than the original bug — not a defect. +- `check-audit.mjs` `isWaived`: logic-traced for missing / past / equal / future / + absent-expires cases — all correct. One latent gap noted as IN-01 below. +- `check-outdated.mjs` relabel: classification logic unchanged; only strings moved. ## Info -### IN-01: `check-audit.mjs` `isMain` uses raw string compare vs `index.ts`'s `realpathSync` +### IN-01: Unparseable `expires` in the audit allowlist waives indefinitely -**File:** `scripts/check-audit.mjs:64-65` -**Issue:** `process.argv[1] === __filename` is a plain string comparison. -`index.ts:101-109` deliberately uses `realpathSync(process.argv[1])` for symlink/relative -robustness and documents why (WR-05 in that file). Invoking the script via a symlink or -a non-canonical path (`./scripts/check-audit.mjs`) would silently skip the main body and -exit 0 — a security gate that no-ops without error. CI invokes `node scripts/check-audit.mjs` -from repo root, which works today, so this is latent. -**Fix:** Mirror the `index.ts` pattern: compare `realpathSync(process.argv[1])` to -`__filename`, or use `import.meta.url === pathToFileURL(process.argv[1]).href` with realpath. - -### IN-02: `pnpm audit --json` is run twice per CI security job - -**File:** `scripts/check-audit.mjs:84` and `scripts/check-outdated.mjs:65` -**Issue:** Both scripts independently spawn `pnpm audit --json`. The security job runs -them back-to-back, doubling the audit work. Out of v1 performance scope and harmless, -but a shared cache or a single audit pass piped to both would be cleaner. -**Fix:** Optional — have `check-outdated.mjs` accept the audit JSON via stdin/arg, or -merge the two into one script with two report sections. - -### IN-03: `outdated-pins.json` reasons are not cross-checked against the audit allowlist - -**File:** `scripts/outdated-pins.json` / `scripts/audit-allowlist.json` -**Issue:** Two independent suppression lists (pin reasons keyed by package name; audit -waivers keyed by GHSA). Nothing keeps them consistent, and neither references the other. -A pinned package (e.g. `eslint`) that later acquires a High advisory would be waived in -one place and pinned in another with no linkage. Documentation-level coupling only. -**Fix:** Optional — add a note in each file referencing the other, or a lint step that -flags a pinned package carrying an unwaived blocking advisory. - -### IN-04: `expand.test.ts` is in scope but unrelated to this CI/security phase - -**File:** `apps/api/tests/broker/expand.test.ts` -**Issue:** This is a substantive, well-constructed test (DST wall-clock, EXDATE, DURATION, -COUNT, Temporal round-trip). No defects found. It appears in the review set only because -it was touched/moved; it is orthogonal to the CI/dependency/security changes. Noted for -completeness — no action required. -**Fix:** None. +**File:** `scripts/check-audit.mjs:41` +**Issue:** `isWaived` guards expiry with `if (w.expires && Date.parse(w.expires) <= Date.now())`. +If `expires` is a non-empty but unparseable string (e.g. `"soon"`, `"2026-13-40"`), +`Date.parse` returns `NaN`, `NaN <= Date.now()` is `false`, and the entry waives the +advisory indefinitely — the same failure mode CR-01 fixed, reachable via a typo in +committed allowlist data. Low severity: the allowlist is reviewed, committed, +non-attacker data, and CR-01's primary case (a real past date) is handled. Flagged +for completeness only; not a regression introduced by the fix. +**Fix:** Treat an unparseable `expires` as expired (fail-closed): +```js +if (w.expires) { + const t = Date.parse(w.expires); + if (Number.isNaN(t) || t <= Date.now()) return false; +} +return true; +``` ---