# Review, master (private attendance photos, spec 0004), 2026-10-27

**Reviewed by**: deepseek v4.1 Flash (author on composer 2.5)
**Scope**: 18 reviewable files (34 paths on disk: 2 generated min stylesheets, 15 audio clips, 1 lock file), uncommitted on master
**Verdict**: Approve with nits

## Summary
This slice adds the Photos page, its engine and AJAX dispatcher, the browser liveness module, spoken prompt clips, the third nav tile, and the scoped CSS, plus the shared access helper and the App settings slice that are still uncommitted. The security shape is strong for a biometric feature: the bucket stays private, portraits go straight from the browser to B2 through a 120 second signed PUT, viewing uses a 300 second signed GET, object keys are built on the server with a per school allowlist, bytes never pass through PHP, and no secret or signed URL is logged. The code follows the spec and AGENTS.md closely, and the seven Minor findings below are polish and diagnostic quality, not correctness or exposure. Two of them (the retry loop and the camera starting on page load) are the ones I would fix first.

## Minor

### 🟡 PUT retry fires again with no delay, `app/admin/scripts/staff_attendance_photos.js:742`
**Problem**: `putUntilExpiry` calls `once()` again on any status that is not 200, with no delay and no attempt cap, until the 120 second deadline. On a CORS misconfiguration, an expired signature, or a drop in connectivity this becomes a tight loop of requests, and the page sits on "Sending portrait" for the full 2 minutes.
**Why it matters**: B2 receives hundreds of wasted writes that cannot succeed, the phone spends that time retrying, and the admin gets no feedback until the deadline. AC-17 makes the CORS step a manual console task, so the 403 case is a realistic first run.
**Suggested fix**: keep the retry until `expires_in` but add a short growing pause between tries (for example 400 ms, then 1 s, then 2 s) and stop after a fixed number of tries. The observable contract ("retry, then The photo did not upload.") does not change.

### 🟡 Camera opens on page load, `app/admin/scripts/staff_attendance_photos.js:876`
**Problem**: `startCamera()` runs during init, so `getUserMedia` and the MediaPipe model load the moment the Photos page opens, before the admin picks a staff member or taps Start.
**Why it matters**: The camera indicator turns on and a permission prompt appears on a page the admin may have opened only to browse the list. On iOS a denied prompt needs a reload to recover, and `Start` stays disabled with no obvious recovery path.
**Suggested fix**: request the camera inside the Start handler (or when a staff member is selected), and keep `cameraReady` gating on the button. The liveness flow already starts on that tap.

### 🟡 The camera stream is never stopped, and bfcache restore leaves a dead preview, `app/admin/scripts/staff_attendance_photos.js:885`
**Problem**: The `pagehide` handler cancels the checker but never stops the `MediaStream` tracks. `cameraReady` also stays true when the page comes back from the back and forward cache.
**Why it matters**: A restored page can show Start as enabled over a frozen or dead video, so the next run fails with "Verification failed" for no visible reason. In a WebView that keeps the page alive, the camera can stay on.
**Suggested fix**: keep a reference to the stream, call `track.stop()` for each track on `pagehide`, and set `cameraReady = false`. On `pageshow` with `event.persisted`, call `startCamera()` again.

### 🟡 A failure after a successful upload still says the photo did not upload, `app/admin/scripts/staff_attendance_photos.js:735`
**Problem**: One `catch` covers mint, the B2 PUT, and confirm. A network error on `mint` or on `confirm` lands on `The photo did not upload.`, even when the object is already in B2.
**Why it matters**: The admin may retry a portrait that was stored, creating a second capture, and no one can tell an upload failure from a confirm failure when debugging.
**Suggested fix**: give each stage its own message. Keep the PUT wording for `putUntilExpiry`, show the server message for a failed confirm, and use a neutral "Could not reach the photo service." for a failed mint fetch.

### 🟡 Confirm pages through the whole school prefix on every upload, `admin/includes/staff_attendance_photos_engine.php:754`
**Problem**: `sap_purge_other_keys` lists `{school_id}/` page by page (200 keys each) on every successful confirm, then deletes the matches. The AJAX call at `admin/includes/staff_attendance_photos_ajax.php:237` waits for all of it.
**Why it matters**: Cost grows with the number of portraits in the school. On a large school or a slow B2 response, the confirm request gets long, and a PHP time limit would return the retry screen after the object was already stored.
**Suggested fix**: cap the number of list pages walked in one confirm (the common case has zero stale keys) and keep the "purge failure must not fail the upload" rule as it is. Record the capped case quietly, the next capture can retry.

### 🟡 Every storage failure is reported as "did not respond", `admin/includes/staff_attendance_photos_engine.php:537`
**Problem**: Any cURL error, a TLS failure, a wrong bucket, and a bad key all become `b2_timeout` with "The photo store did not respond." The HTTP status from B2 is kept on success only, and nothing is logged.
**Why it matters**: A wrong credential or a wrong bucket name looks identical to a network outage, which makes a first deployment hard to diagnose. The spec asks that details are not shown to the admin, but it does not ask for silence in the log.
**Suggested fix**: keep the same user facing message, and `error_log` the cURL errno plus the HTTP status with no URL and no key material.

### 🟡 Signed URLs are returned without a no store header, `admin/includes/staff_attendance_photos_ajax.php:20`
**Problem**: `sap_json` sets the content type and the status only. `put_url` and `get_url` are presigned and land in any cache layer that chooses to keep the POST response, and in the WebView back and forward store.
**Why it matters**: A signed PUT URL is a write capability for 120 seconds; a signed GET is a read for 300 seconds. The spec treats both as short lived secrets, so dropping them into a cache undercuts that choice.
**Suggested fix**: add `Cache-Control: no-store` (and `Pragma: no-cache`) to every JSON response from this dispatcher.

### 🟡 A shared security helper was widened for this slice, `admin/includes/security.php:231`
**Problem**: `request_is_same_domain` now falls back to `HTTP_ORIGIN`. It also compares the host only, so `http://host` and `https://host` both pass, and `HTTP_HOST` keeps its port while `parse_url` drops it, so any non default port fails. Roughly 30 existing endpoints call this method.
**Why it matters**: No exposure is added (Origin is set by the browser, and the CSRF token is still required), but a single helper used across the desktop and the app changed in a way that no longer compares scheme. The app runs inside a React Native WebView, where both headers can be absent; the earlier review already flagged that a 403 would break saving and uploading with only "forbidden" on screen.
**Suggested fix**: normalise both sides to `scheme://host[:port]` in one place and compare that. Run `/check verify` through the real WebView for one save and one upload, not only in the desktop browser.

### 🟡 A failed restore turns a saved branch into a reported failure, `admin/includes/staff_attendance_settings_engine.php:231`
**Problem**: `sas_with_school_db` throws from its `finally` block when the restore fails. That exception replaces the branch result, so `sas_copy_extras` records "Could not copy settings." for a branch that was already updated.
**Why it matters**: The admin is told the branch was not saved when it was, and the flash pushes them to save again. It is a rare path, but the message is wrong in the one case it appears.
**Suggested fix**: capture the restore failure separately and keep the branch result. Stop the loop as the code already does when the connection cannot be returned to the session database.

### 🟡 The staff flyout is rebuilt from every activated row, `admin/includes/staff_attendance_photos_engine.php:264`
**Problem**: `sap_staff_options` selects every activated staff row with no limit, and the list action sends all of them on each page request.
**Why it matters**: A large school pays that payload on every paging click and every reload after an upload. A native select also becomes hard to use past a few hundred entries.
**Suggested fix**: cap the query (for example 500 rows) or add a search action later. The mint path already re reads the one row it needs, so a capped picker does not weaken validation.

## Nits
- ⚪ `app/admin/styles/zapapp.css:85881`, five `.sap-sheet__*` rules sit outside any `.staff-attendance-photos-page` or `.staff-attendance-photos-app` scope, while the rest of the block is scoped. The prefix is unique, so the risk is low.
- ⚪ `admin/includes/staff_attendance_photos_ajax.php:102`, `global $info, $table_name, $mysqli, $security;` at file scope does nothing. Same nit as the earlier settings review.
- ⚪ `admin/includes/staff_attendance_photos_engine.php:463`, add `LIBXML_NONET` to `simplexml_load_string`. The host is configured and trusted, so this is only cheap hardening.
- ⚪ `admin/includes/staff_attendance_photos_engine.php:482`, the canonical `host` header is lowercased but the `Host:` header sent to cURL keeps the configured case. A mixed case endpoint would break the signature, so normalise once.
- ⚪ `app/admin/scripts/staff-attendance-liveness.js:285`, `result.faceBlendshapes[0].categories` throws if blendshapes are missing while landmarks exist. The generic "Verification failed, try again." covers it, but a guard is one line.
- ⚪ `app/admin/staff_attendance_photos.php:154`, the view `<dialog>` is printed even when the photo store is not configured. Harmless, the page never offers a View button in that state.

## Strengths
- The security boundary is the B2 prefix, and it is enforced on the server in `sap_object_key_allowed`: regex shape, the school prefix with its trailing slash, and a `..` reject. Keys are always built from the session school and the staff row, never from the client, and every signed action is CSRF gated, session gated, and role gated before any B2 call.
- Portraits go browser to B2 directly. PHP never holds portrait bytes, `put_url` and `get_url` are never logged, and the application key stays out of every response and out of the page HTML.
- The engine and the AJAX dispatcher follow AGENTS.md: mysqli plus `$ctx` in, arrays out, no echo, no `$_POST` or `$_GET`, prepared statements, JSON hex flags, and a fresh CSRF token in every body.
- The tests that do exist are runnable and meaningful: `php admin/includes/staff_attendance_photos_engine_test.php` exits 0, and it checks the allowlist against school `13`, `120`, traversal, a non enrollments folder, and a non jpg, not just the happy path. All 15 prompt clips exist and are valid MP3 files.
- The minified stylesheet is in sync with the source (50 booth selectors and 10 sheet selectors in both), and the verify record is unusually complete, including the CORS run, the 200 key paging, the name edit purge, and the incomplete config case.

## Test coverage
`TESTS = none-by-design`, so the gate is `/check verify`, and the verify record covers AC-1 to AC-18 against the live app. The engine test covers staff labels, the prefix allowlist, traversal, slug rules, cursor rules, and the challenge pick. It does not cover `sap_object_key` itself, the SigV4 canonical strings, or `sap_purge_other_keys` matching. Those are the parts the browser walk cannot see, and verify exercised them by hand, so this is a note about where a future regression would hide rather than a request for a suite.