luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(16): re-review clean + code review fix report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-13 08:52:12 -04:00
parent bb1e97556d
commit e0ec4a40a5
2 changed files with 244 additions and 213 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/16-ci-dependency-audit-and-security-checks/16-REVIEW.md
+84 -213
View File
@@ -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 `<structural_findings>` 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;
```
---