familysync/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
16-ci-dependency-audit-and-security-checks	2026-06-13T00:00:00Z	standard	5	.gitea/workflows/ci.yml .gitea/workflows/publish.yml scripts/check-audit.mjs scripts/check-outdated.mjs scripts/__tests__/check-audit.test.mjs	critical warning info total 0 0 1 1	clean

Phase 16: Code Review Report

Reviewed: 2026-06-13 Depth: standard Files Reviewed: 5 Status: clean

Summary

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.

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 <structural_findings> block, so there is no fallow substrate section.

Prior-finding verification

• 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_names, 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.

Regression check on the fixes

• 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: Unparseable expires in the audit allowlist waives indefinitely

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):

if (w.expires) {
  const t = Date.parse(w.expires);
  if (Number.isNaN(t) || t <= Date.now()) return false;
}
return true;

---

Reviewed: 2026-06-13 Reviewer: Claude (gsd-code-reviewer) Depth: standard