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

docs(16): add code review report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-13 08:35:28 -04:00
parent 9a108a3618
commit 5dd84a2861
1 changed files with 250 additions and 0 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
+250
View File
@@ -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 `<structural_findings>` 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_