# Security Audit — Cycle 4 Delta Report

**Date**: 2026-05-13
**Mode**: Resume / incremental — cycle-2 base (`security_report.md` + companion artifacts) plus cycle-3 delta (`security_report_cycle3_delta.md`) are kept verbatim; this report records ONLY the deltas introduced by cycle 4.
**Cycle**: Phase B / Cycle 4 (AZ-512 only — `admin/` AZ-513 prerequisite still un-shipped; UI implemented under user-authorized Option B against MSW stubs)
**Scope of delta**: cycle-4 commits only — `ef56d9c` (AZ-512 reactivation chore), `ecacfa8` (AZ-512 implementation batch 16). No infrastructure / CI / nginx / Dockerfile changes; no new dependencies; no new external surface; no new secrets.
**Verdict (post-cycle-4)**: **PASS_WITH_WARNINGS** — unchanged from cycle 3. One new LOW finding documented (F-SAST-CY4-1 — lost-update / mid-air-collision admission on PATCH). All cycle-3 carries remain unchanged.

---

## Verdict change

| Verdict component | Cycle 3 (2026-05-13 — pre-cycle-4) | Cycle 4 (2026-05-13 — post AZ-512) | Driver |
|-------------------|------------------------------------|------------------------------------|--------|
| Overall           | PASS_WITH_WARNINGS                 | PASS_WITH_WARNINGS                 | No change in severity ceiling |
| Critical          | 0                                  | 0                                  | — |
| High              | 1 carried (F-SAST-1 — Google Geocode key in `mission-planner/` git history; production-bundle exposure NONE) | 1 carried (unchanged) | User-action gate: key revocation still pending |
| Medium            | 7 carried                          | 7 carried (unchanged)              | No cycle-4 changes to CI / nginx / Dockerfile |
| Low               | 3 carried                          | 4 (new: F-SAST-CY4-1)              | New lost-update admission on `PATCH /api/admin/classes/{id}` |

---

## Cycle 4 scope — exactly what changed and what each change can / cannot affect

| File | Domain | Security-relevant? | Why / why not |
|------|--------|--------------------|----------------|
| `src/features/admin/AdminPage.tsx` | Production source | Yes — adds a new wire call to `PATCH /api/admin/classes/{id}` and a new client-side validation path. | See finding F-SAST-CY4-1 below + carry analysis. No new credentials, no new external surface, no string interpolation in URL (`endpoints.admin.class(id)` builder is unchanged from cycle 2). |
| `src/i18n/en.json`, `src/i18n/ua.json` | Production source | No | New translation keys are static strings rendered through React (auto-escaped). No interpolation of untrusted input. |
| `tests/admin_class_edit.test.tsx` | Test-only | No | Vitest fixture; never shipped. |
| `tests/msw/handlers/admin.ts` | Test-only | No | MSW worker; never shipped. `Dockerfile` final stage is `nginx:alpine` serving `dist/`. |
| `tests/destructive_ux.test.tsx` | Test-only | No | Selector-target fix; logic unchanged. |
| `_docs/02_document/**/*.md`, `_docs/03_implementation/**/*.md` | Documentation | No | Documentation only. |

> **No new package added, no version bumped.** `bun audit` re-run 2026-05-13 against `ui/` reports **"No vulnerabilities found"** (bun 1.3.11). The cycle-3 OWASP A06 PASS verdict carries forward.

---

## Resolved findings (cycle 3 → cycle 4)

**None.** Cycle 4 did not close any prior finding.

| Pending user-action items (carried for visibility) |
|---------------------------------------------------|
| F-SAST-1 — Google Geocode API key in `mission-planner/` git history → user-action: revoke at GCP credentials console + externalize via `VITE_GOOGLE_GEOCODE_KEY` (AZ-499 pattern). |
| OpenWeatherMap key revocation — recorded in cycle-2 retrospective; the **AZ-449** code-side fix shipped but the **revocation of the previously committed key** is still a pending user action. |

These two pending revocations are visible in `_docs/06_metrics/retro_2026-05-12.md` and the `_docs/_process_leftovers/` set; they were not in scope for AZ-512 and remain open.

---

## New cycle-4 findings

### F-SAST-CY4-1 — Lost-update / mid-air-collision on PATCH `/api/admin/classes/{id}` — LOW

| Field | Value |
|-------|-------|
| Severity | LOW |
| Category | Insecure Design (OWASP A04) / Software & Data Integrity (OWASP A08) |
| Location | `src/features/admin/AdminPage.tsx:57-75` (`handleUpdateClass`) |
| Introduced by | AZ-512 (commit `ecacfa8`) |
| Production exposure | The `/admin` route is gated by Header's `ADM` permission AND backend authZ on every `/api/admin/*` call. Surface is restricted to authenticated admins. |

**Description**

`handleUpdateClass` performs an inline edit with this sequence:

1. Client-side validation (`name.trim()` non-empty, `maxSizeM > 0`).
2. `await api.patch(endpoints.admin.class(editingId), editForm)` — sends the **complete** edit-form body (the documented "Risk 2 mitigation" so partial-merge vs full-replace PATCH semantics are equivalent for the UI).
3. `await api.get(endpoints.annotations.classes())` — refetch list, replace `classes`, clear `editingId`.

The intentional full-body PATCH guarantees the UI's view of the row replaces whatever is on the server. There is **no concurrency guard** (`If-Match` / `ETag` / `version`). If admin A and admin B open the same row simultaneously, the last `PATCH` wins silently and overwrites the other admin's edit without notification.

This is a deliberate trade-off: the task spec (`AZ-512_admin_edit_detection_class.md`) explicitly scopes optimistic-locking out, and AZ-513's backend spec mirrors that (no ETag header). The risk class is documented in the task spec's "Risks" section. The audit records it for completeness so future hardening can re-open it.

**Impact**

- Two admins editing the same detection class in the same window → second save silently overwrites the first.
- Audit trail (if any — owned by `admin/` service) would show both PATCHes, so attribution survives.
- Detection-class editing is a low-frequency administrative operation with typically a single active admin, so practical exposure is low.

**Production-bundle exposure**

Limited to authenticated `ADM` users, in a low-multi-admin operation domain, with no user-data leak. **No exploitable path to data exfiltration or escalation.** This is a correctness / data-integrity weakness, not an authN/authZ break.

**Remediation (future / out-of-cycle)**

1. When AZ-513 lands the backend, decide whether `admin/` will emit an `ETag` on `GET /api/admin/classes/{id}` and accept `If-Match` on `PATCH`. If yes, the UI side becomes:
   - Capture `etag` from the row on edit-start.
   - Send `If-Match: <etag>` header on `PATCH`.
   - On `412 Precondition Failed`, render a "this class was changed by someone else — reload?" inline alert (analogous to today's `editError = 'updateFailed'`).
2. Cheaper short-term alternative: append a generated `version: number` to `DetectionClass` and have the UI assert it on PATCH; backend returns 409 on mismatch.

**Track as**: open in `_docs/05_security/`; not blocking. To be promoted to a UI ticket only when AZ-513 lands and the backend's chosen concurrency model is known.

---

## Cross-cutting cycle-4 verification

### Static analysis — AZ-512 deltas

- **URL construction**: `endpoints.admin.class(editingId)` is the same builder used by `handleDeleteClass` (cycle-2 audited path). `editingId: number | null` is constrained at the type level and is only set from a server-returned `DetectionClass.id`. No tainted-input → URL path.
- **JSON body**: `editForm` is a plain `{ name, shortName, color, maxSizeM }` object. React form-controlled inputs feed it; no `dangerouslySetInnerHTML`, no `innerHTML`, no template injection surface. Backend must still validate length / charset (UI relies on backend per AZ-513 ACs).
- **Error path**: the `catch` block sets a discriminated-union error kind, not the raw thrown message. No information leak from server error responses into the rendered UI.
- **Optimistic refetch**: same shape as cycle-2-audited `handleAddClass` refetch. No new surface.
- **Test-only MSW handler in `tests/msw/handlers/admin.ts`**: not bundled. Vite's `bundle-introspect.test.ts` (cycle-2 evidence) already enforces `tests/` is excluded from `dist/`.

**Verdict**: PASS — no new injection, no new secret, no new auth-surface.

### Authentication & authorization — AZ-512 deltas

- **Route gating**: AZ-512 does not change `/admin` route gating. Header's `hasPermission('ADM')` continues to filter the visible nav entry. As cycle-2 noted (F2 / AC-22 carry), a user who deep-links to `/admin` without `ADM` still renders the page but every fetch 401/403s. AZ-512 inherits that posture exactly.
- **Per-action authZ**: each PATCH/DELETE/POST/GET is authZ'd server-side by `admin/`. The UI does not perform pre-flight permission checks for the edit affordance specifically. This matches the existing add / delete posture (cycle-2 audited).

**Verdict**: PASS — no degradation; carries F2 / AC-22 unchanged.

### Cryptographic failures, secrets, data exposure — AZ-512 deltas

- **No new secrets** introduced. `bun audit` clean. No new env vars touched.
- **No PII** in the PATCH body (detection-class metadata only).
- **No new log output**: `client.ts` has no new logging path; `AdminPage.tsx` adds no `console.*`.
- **Error message localization**: errors are mapped to i18n keys (`admin.classes.updateFailed`) — no server-message echo into the UI string.

**Verdict**: PASS.

### OWASP Top 10 — categories whose status would change

None. All ten categories carry forward from the cycle-3 delta verdict unchanged. The new LOW finding F-SAST-CY4-1 maps to A04 (Insecure Design) but the category's status was already PASS (cycle 2) and stays PASS because LOW findings do not flip the category.

| # | Category | Cycle-3 status | Cycle-4 status |
|---|----------|----------------|----------------|
| A01 | Broken Access Control | PASS_WITH_KNOWN (F2/AC-22 carry) | **unchanged** |
| A02 | Cryptographic Failures | PASS_WITH_KNOWN (ADR-008 carry) | **unchanged** |
| A03 | Injection | PASS | **unchanged** |
| A04 | Insecure Design | PASS | **unchanged** (new LOW F-SAST-CY4-1 is informational only) |
| A05 | Security Misconfiguration | FAIL (F-INF-2 carry) | **unchanged** |
| A06 | Vulnerable & Outdated Components | PASS | **unchanged** (`bun audit` re-run clean 2026-05-13) |
| A07 | Identification & Authentication Failures | PASS | **unchanged** |
| A08 | Software & Data Integrity Failures | FAIL (F-INF-1, F-INF-3, F-INF-4 carry) | **unchanged** |
| A09 | Logging & Monitoring | N/A | **unchanged** |
| A10 | SSRF | N/A | **unchanged** |

### Infrastructure / CI / Container — AZ-512 deltas

**None.** Cycle 4 did not touch `Dockerfile`, `nginx.conf`, `.woodpecker/build-arm.yml`, `.env.example`, or any container/CI artifact. Carries F-INF-1..5 verbatim.

---

## Cross-workspace dependency note

AZ-512 ships against MSW stubs in tests. The live `PATCH /api/admin/classes/{id}` endpoint does not exist in production until **AZ-513** is implemented and deployed by the `admin/` workspace team. Until then:

- A real admin clicking ✎ + Save in the deployed dev/stage/prod UI will hit a backend `404` (or 405 depending on how `admin/` rejects unknown methods).
- The UI surfaces a generic `editError = 'updateFailed'` ⇒ "Update failed" inline alert. No information leak.
- **Deploy gate**: Step 16 of cycle 4 must NOT promote this build past the boundary where AZ-513 has not yet landed. The `_docs/_process_leftovers/2026-05-13_az-512-admin-classes-prereq.md` leftover entry remains open until AZ-513 ships + deploys.

This is a process control concern, not a security finding — captured here so the audit history records why a deploy-gate exists for an otherwise-clean cycle.

---

## Updated counts (carries from cycle 3 + cycle-4 net)

| Severity | Cycle 3 | Cycle 4 | Net change |
|----------|---------|---------|------------|
| Critical | 0       | 0       | — |
| High     | 1 (F-SAST-1) | 1 (F-SAST-1) | — |
| Medium   | 7       | 7       | — |
| Low      | 3 (F-SAST-4, F-INF-5, F-SAST-CY3-1) | 4 (+F-SAST-CY4-1) | +1 |

## Self-verification (Phase 5 of `security/SKILL.md`)

- [x] All cycle-4 changed files reviewed (6 source/test files + doc files; surface enumerated above).
- [x] No duplicate findings (F-SAST-CY4-1 is new, not a restatement of F-INF-1..5 or F-SAST-CY3-1).
- [x] Every finding has remediation guidance (see F-SAST-CY4-1 § Remediation).
- [x] Verdict matches severity logic (PASS_WITH_WARNINGS = only Medium/Low new findings + carried High is pre-existing).
- [x] `bun audit` re-run is clean.
- [x] No new credentials / secrets in cycle-4 commits (`ef56d9c`, `ecacfa8`).
- [x] Cross-workspace dependency (AZ-513) is recorded as a process / deploy-gate concern, not a security finding.

## Recommendations

### Immediate (Critical/High)
- None new from cycle 4. Cycle-2 / cycle-3 carries unchanged: revoke Google Geocode key (F-SAST-1); revoke OpenWeatherMap key (carried).

### Short-term (Medium)
- None new from cycle 4. Cycle-2 carries unchanged: nginx security headers (F-INF-2); `bun audit` in CI (F-INF-1); Trivy/Grype in CI (F-INF-3); SBOM + image signing (F-INF-4).

### Long-term (Low / Hardening)
- **F-SAST-CY4-1 follow-up**: when AZ-513 lands, decide on the concurrency model with `admin/`. If `ETag` / `If-Match`: open a UI ticket to thread the header through `client.ts` and surface 412 as a "reload" alert. If `version` field: open a UI ticket to assert version on PATCH and surface 409 the same way. Cheap fix once the backend picks a model — until then, it stays LOW.