fix(security): fix all four web-ui advisories and raise the deploy audit gate to high (#1131)

[frankbria]

Closes #1131.

Both halves, in the order the issue requires

The issue is explicit that these must move together: raising the gate first blocks every deploy, fixing only the advisories leaves the gate blind next time.

1. All four advisories — fixed, not accepted

Every one had fixAvailable: true with an empty effects array, so npm audit fix resolved them inside the existing major versions. There is therefore no allowlist entry and no review date: an allowlist would be a record of a risk nobody is taking.

Package	Sev	Before → After
brace-expansion	high	1.1.13 → 1.1.18, 2.1.1 → 2.1.4, 5.0.6 → 5.0.9
js-yaml	high	3.14.2 → 3.15.1, 4.3.0 → 4.3.1
nanoid	high	3.3.16 → 3.3.18
@babel/core	low	→ 7.29.7

$ npm audit
found 0 vulnerabilities
$ npm test    → 104 suites, 1280 tests passed
$ npm run build → succeeded

Only package-lock.json changed — no package.json edit, no major bumps.

2. The gate rises to high

Both deploy jobs: npm audit --audit-level=critical → --audit-level=high, with the reason inline. The tradeoff is stated rather than discovered later: audit results change without a code change, so a newly published high can block a deploy. That is the gate doing its job — six highs accumulated unnoticed under critical.

The decision is recorded where it survives

tests/test_deploy_audit_gate_1131.py carries it — the four advisories, the fix-don't-accept reasoning, and why there is no allowlist — because the issue asks for something durable and a PR comment is not. It also asserts:

• both deploy jobs audit (a gate on one path is not a gate)
• the level never weakens below high, expressed as a floor so tightening stays allowed
• the audit runs before anything is restarted
• no lockfile copy drops below the versions this PR shipped

One thing I got wrong, and how I caught it

That last table was green theatre on the first pass. I set the floors from the advisories' "patched in" versions — which the pre-fix lockfile already satisfied, so every assertion passed against the broken state. The floors now come from the shipped lockfile, and I verified in the failing direction:

$ git show main:web-ui/package-lock.json > web-ui/package-lock.json
$ uv run pytest tests/test_deploy_audit_gate_1131.py
4 failed, 4 passed   # brace-expansion, js-yaml, nanoid, @babel/core

AC4 can only be checked after merge — deliberately

"Verify a deploy still succeeds with the raised threshold." The deploy does git reset --hard origin/main on the box and then npm ci, so a deploy from this branch would audit main's lockfile, which still carries the three highs — it would fail, and would tell us nothing about this change.

So the sequence is: merge, then the automatic push-to-main deploy is the verification. I will report that run on this PR and fix forward if it fails. Staging has no users.

Note taken from the issue

an open Dependabot PR is not evidence that a class of advisory is being handled

None of these four had one. That is the reason they were still open.

[github-actions]

Claude finished @frankbria's task in 7m 48s —— View job

---

GLM precision bug-review — PR #1149

✅ GLM review: no defects found.

Scoped to concrete defects only (logic errors, null/error paths, security, data loss, races, breaking changes, resource leaks). Style, naming, architecture, and test-coverage are covered by the separate general review.

Area	Verdict
deploy.yml — critical → high (×2 jobs)	✅ Correct. Exactly two npm audit --audit-level=high lines (staging L248, production L673); both run before their job's pm2 start (L288, L711); no critical audit-level remains anywhere.
test_deploy_audit_gate_1131.py — level floor	✅ Logic correct. LEVELS.index(level) <= LEVELS.index("high") correctly forbids only critical (the loosest gate); info/low/moderate/high all pass, so tightening stays allowed and loosening is the pinned regression.
test_… — ordering assertion	✅ Passes against the real file. "pm2 start" in after holds for both audit matches.
test_… — FIXED_FLOORS	✅ Floors match the shipped lockfile exactly (verified independently via jq): brace-expansion 1.1.18 / 2.1.4 / 5.0.9, js-yaml 3.15.1 / 4.3.1, nanoid 3.3.18, @babel/core 7.29.7. Every major line present in the lockfile (1,2,5 / 3,4 / 3 / 7) has a floor, so the "major line #1131 never saw" branch can't misfire.
package-lock.json — full 562-line diff	✅ Every version change is an upgrade (new ≥ old); no downgrades that could reintroduce a patched advisory. The 4 target packages land at their fixed versions; the rest are transitive bumps (browserslist, caniuse-lite, electron-to-chromium, node-releases, baseline-browser-mapping, @babel/* family). Two @emnapi/* optional packages were dropped (tree re-resolution), no orphaned references.

No inline findings to post.

[claude]

Review

Solid, well-scoped fix — does exactly what #1131 asked (fix the advisories and raise the gate together, in that order) and backs the decision with a durable regression test rather than just a PR description.

What's good:

• All four fixes are patch/minor bumps within the existing major line (confirmed: web-ui/package.json is untouched, only package-lock.json changed — no surprise breaking-change risk from this PR).
• tests/test_deploy_audit_gate_1131.py checks the things that actually matter: both deploy jobs audit (not just one), the level never regresses below high, the audit runs before pm2 start restarts the process, and the shipped lockfile versions are floored so a lockfile revert would be caught. I independently re-derived the "floor" versions from the current lockfile (@babel/core 7.29.7, js-yaml 3.15.1/4.3.1, nanoid 3.3.18, brace-expansion 1.1.18/2.1.4/5.0.9) and they match FIXED_FLOORS exactly.
• Good self-correction called out in the PR description (catching that the first pass floors were derived from the pre-fix lockfile and would have passed against a revert) — and it's verifiable: exactly 2 audit occurrences / 2 deploy jobs, matching test_both_deploy_jobs_run_an_audit's assumption.
• Raising critical → high is the right tradeoff given the explicit accepted downside (a newly-published high can now block a deploy) documented inline at the point of change, not just in the PR body.

Minor, non-blocking:

1. Advisory count nit: both the deploy.yml comment and the test module docstring say "six high-severity findings," but the table in the PR description and docstring lists 7 distinct GHSA IDs at high (3 for brace-expansion, 3 for js-yaml, 1 for nanoid). Since this comment is explicitly meant to be the durable record of why, worth reconciling the number (maybe npm audit's summary counts differently than raw GHSA IDs, but as written it reads inconsistent with the table right above it).
2. test_the_audit_runs_before_anything_is_restarted's docstring says "within the same job, the restart must come later," but the implementation only checks that some pm2 start string appears anywhere later in the whole file, not scoped to that job's block. It happens to hold today because of file ordering (staging audit → staging pm2 start → production pm2 start; production audit → production pm2 start), but it wouldn't catch, say, a production audit accidentally reordered to after production's own restart while staging's pm2 start (which comes later in the file) still exists. Not a real bug today, just a test that's slightly weaker than its docstring claims — could split on job boundaries for a tighter guarantee if this file grows more jobs.
3. The 14-line audit-gate comment block is duplicated verbatim across both jobs. Fine for a static YAML workflow with only two call sites, just flagging rather than asking for a change.

No security, performance, or test-coverage concerns beyond the above — this is exactly the kind of gate-plus-fix pairing the linked issue asked for.

[github-actions]

Claude finished @frankbria's task in 4m 18s —— View job

---

Precision bug review — PR #1149 (synchronize)

Scoped to concrete defects only (logic errors, null/error paths, security, data loss, races, breaking changes, resource leaks). Style/naming/architecture/test-coverage are covered by the separate general review.

• Gather PR context (gh pr diff, changed files)
• Check existing comments to avoid repeats
• Read deploy.yml audit-gate hunks + the SSH heredoc that wraps them
• Read tests/test_deploy_audit_gate_1131.py against the real workflow file
• Verify lockfile version floors (incl. the new regen commit 8301e8e)
• Post summary

✅ GLM review: no defects found.

This run was triggered by the synchronize that added 8301e8e (lockfile regen), so I re-checked the current HEAD rather than the previously-reviewed tree. What I verified against the checked-out code:

Area	Verdict
deploy.yml critical→high (L248, L673)	✅ Both audit lines are the only npm audit occurrences; no critical level remains.
Gate is actually enforced	✅ Both run inside ssh … "bash -s" << ENDSSH blocks that open with set -e (L216, L628), so a non-zero npm audit aborts the remote script before pm2 start (L288, L711). The audit is a standalone simple command — not piped or `
Ordering (audit before restart)	✅ File order: staging audit (248) → staging pm2 (288) → production audit (673) → production pm2 (711). Each audit has a later pm2 start in-file.
Level floor test	✅ LEVELS.index(level) <= LEVELS.index("high") forbids only critical (loosest), allows info/low/moderate/high. Correct regression direction.
FIXED_FLOORS vs shipped lockfile	✅ Independently confirmed via jq: brace-expansion 1.1.18 / 2.1.4 / 5.0.9 (×3 copies), js-yaml 3.15.1 / 4.3.1, nanoid 3.3.18, @babel/core 7.29.7 — every value in FIXED_FLOORS matches a real packages entry.
_versions extraction	✅ path.rsplit("node_modules/", 1)[-1] correctly resolves top-level, nested (…/node_modules/X), and @scoped paths — all 9 entries above matched their package.
Lockfile regen (6135a3a→8301e8e)	✅ +22 lines, only adds @emnapi/core 1.8.1 and @emnapi/runtime 1.11.3 (optional/dev, referenced by sharp's native bindings) to make npm ci self-consistent. Touches none of the four fixed packages; no downgrade, no new advisory-bearing dep.

No inline findings to post.

[claude]

Follow-up review (second commit)

My prior review here covered the first commit (advisory fixes + gate raise + the regression test). This commit fixes a real gap it exposed: npm audit fix (no --package-lock-only) reconciled the lockfile against the local node_modules tree and dropped @emnapi/core/@emnapi/runtime, which CI's npm ci correctly rejected as a lockfile/tree mismatch — good catch, and the fix is the right one (--package-lock-only edits the lockfile without touching the installed tree).

Verified independently:

• @emnapi/core@1.8.1 and @emnapi/runtime@1.11.3 are back, and every existing constraint on them (^1.4.3, ^1.7.1, ^1.11.1, ^1.11.3) is satisfied by the restored versions — no stale references.
• @emnapi/wasi-threads stays at 1.1.0, matching @emnapi/core's own dependency on it.
• The four target packages are unaffected and still land exactly where FIXED_FLOORS in tests/test_deploy_audit_gate_1131.py expects: brace-expansion 1.1.18/2.1.4/5.0.9, js-yaml 3.15.1/4.3.1, nanoid 3.3.18, @babel/core 7.29.7.
• npm audit is still 0 vulnerabilities per the PR description, and this diff only adds back entries — no version regressed below what the first commit shipped.

The rest of my earlier review stands unchanged (the two minor/non-blocking notes — the "six" vs. 7-GHSA-ID count in the deploy.yml/test docstring, and the pm2 start ordering assertion being file-wide rather than job-scoped — are cosmetic and not reintroduced or worsened by this commit).

No new concerns. This looks ready to merge, with AC4 (verifying a real deploy succeeds at the raised threshold) deliberately left as a post-merge check per the PR description, which is a reasonable call given deploy.yml's git reset --hard origin/main would otherwise audit main's lockfile, not this branch's.