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

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

### 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):
```js
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_