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 new file mode 100644 index 0000000..b272967 --- /dev/null +++ b/.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md @@ -0,0 +1,250 @@ +--- +phase: 16-ci-dependency-audit-and-security-checks +reviewed: 2026-06-13T00:00:00Z +depth: standard +files_reviewed: 17 +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 +findings: + critical: 1 + warning: 5 + info: 4 + total: 10 +status: issues_found +--- + +# Phase 16: Code Review Report + +**Reviewed:** 2026-06-13 +**Depth:** standard +**Files Reviewed:** 17 +**Status:** issues_found + +## 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. + +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. + +I was not given a `` block, so there is no fallow substrate +section. + +## Critical Issues + +### CR-01: Audit-waiver `expires` field is decorative — expired waivers never re-block + +**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; +} + +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 +``` + +## Info + +### IN-01: `check-audit.mjs` `isMain` uses raw string compare vs `index.ts`'s `realpathSync` + +**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. + +--- + +_Reviewed: 2026-06-13_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_