mirror of
https://github.com/azaion/ui.git
synced 2026-06-21 23:31:11 +00:00
[AZ-512] Cycle 4 Steps 12-15: test-spec sync + docs + sec + perf
ci/woodpecker/push/build-arm Pipeline failed
ci/woodpecker/push/build-arm Pipeline failed
Steps 12-15 closure for cycle 4 (AZ-512 admin class inline edit):
- Step 12 (Test-Spec Sync): traceability O9 -> Covered; new FT-P-62
+ FT-N-18 in blackbox-tests.md.
- Step 13 (Update Docs): AdminPage module doc gains the inline-edit
state slots, four new handlers, PATCH integrations row, expanded
i18n key list, tests section. architecture.md row 272 now lists
PATCH /api/admin/classes/{id} with AZ-513 deploy-gate caveat.
- Step 14 (Security Audit): cycle-4 delta report records one new
LOW finding (F-SAST-CY4-1 lost-update / mid-air-collision on
PATCH, by design per spec); verdict carries PASS_WITH_WARNINGS;
bun audit re-run clean.
- Step 15 (Performance Test): NFT-PERF-01 bundle = 291 332 B
(+757 B / +0.26% vs cycle 3; ~13.89% of 2 MB budget); PASS.
Tests 243 passed / 13 skipped / 0 failed (+12 AZ-512 cases).
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,188 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user