fix(tradein-frontend): MapPicker focus trap + Leaflet SRI, PhotoUpload size check #514

Merged

lekss361 merged 1 commit from feat/tradein-frontend-isolated-ux into main

2026-05-24 12:01:17 +00:00

lekss361 commented

2026-05-24 11:53:32 +00:00

Owner

Summary

Two isolated UX/security fixes from 2026-05-24 trade-in audit.

`MapPicker.tsx` (#12)

A11y: add role=dialog, aria-modal=true, aria-labelledby="map-picker-title" on the inner dialog div.
Focus trap: hand-rolled Tab/Shift+Tab cycling within the dialog (no new deps).
Escape closes: window-level keydown listener on the modal lifecycle.
Focus restore: save document.activeElement on mount, restore on unmount.
Initial focus: close button (safe default — user can immediately press Esc or Tab into map controls).
Leaflet 1.9.4 SRI: pin CSS + JS with sha256- integrity hashes and crossOrigin="anonymous". Defends against unpkg CDN hijack. Hashes computed locally via curl -sL ... | openssl dgst -sha256 -binary | openssl base64 -A and verified to match the values committed:
- leaflet.js → sha256-20nQCchB9co0qIjJZRGuk2/Z9VM+kNiyxNV1lvTlZBo=
- leaflet.css → sha256-p4NxAoJBhIIN+hmNHrzRCf9tD/miZyoHS5obTRR9BMY=

`PhotoUpload.tsx` (#13)

Client-side size check: MAX_FILE_BYTES = 10 * 1024 * 1024 mirrors backend _MAX_PHOTO_BYTES. Rejects oversized files before upload (no wasted bandwidth, no waiting for 413).
Per-file errors: replace single error: string | null with errors: string[], render as <ul>. Each failed file gets its own line with name + reason + (for size errors) actual MB.
Clear stale errors: setErrors([]) on every new file pick — old failures don't linger after a successful subsequent upload.
Empty-file guard: client also rejects file.size === 0 before upload (mirrors backend 400 "empty file").

Test plan

Open MapPicker via address-input map button; press Tab repeatedly — focus must cycle through close button → map controls → "Подставить адрес" → back to close. Shift+Tab cycles backwards.
Press Escape inside MapPicker — modal closes, focus returns to the element that opened it.
Inspect <link> and <script> injected by loadLeaflet(); both must have integrity and crossorigin="anonymous".
Manually corrupt the integrity hash in DevTools <script> and reload — browser must block the resource (SRI failure).
Try uploading a 50 MB phone photo via PhotoUpload — must reject client-side with <filename>: файл больше 10 МБ (50.0 МБ) instead of waiting for backend 413.
Upload 3 files where 1 is 20 MB and 2 are valid — should see 1 error line + 2 photos appear in the grid.

## Summary Two isolated UX/security fixes from 2026-05-24 trade-in audit. ### `MapPicker.tsx` (#12) - **A11y**: add `role=dialog`, `aria-modal=true`, `aria-labelledby="map-picker-title"` on the inner dialog div. - **Focus trap**: hand-rolled Tab/Shift+Tab cycling within the dialog (no new deps). - **Escape closes**: window-level keydown listener on the modal lifecycle. - **Focus restore**: save `document.activeElement` on mount, restore on unmount. - **Initial focus**: close button (safe default — user can immediately press Esc or Tab into map controls). - **Leaflet 1.9.4 SRI**: pin CSS + JS with `sha256-` integrity hashes and `crossOrigin="anonymous"`. Defends against unpkg CDN hijack. Hashes computed locally via `curl -sL ... | openssl dgst -sha256 -binary | openssl base64 -A` and verified to match the values committed: - `leaflet.js` → `sha256-20nQCchB9co0qIjJZRGuk2/Z9VM+kNiyxNV1lvTlZBo=` - `leaflet.css` → `sha256-p4NxAoJBhIIN+hmNHrzRCf9tD/miZyoHS5obTRR9BMY=` ### `PhotoUpload.tsx` (#13) - **Client-side size check**: `MAX_FILE_BYTES = 10 * 1024 * 1024` mirrors backend `_MAX_PHOTO_BYTES`. Rejects oversized files before upload (no wasted bandwidth, no waiting for 413). - **Per-file errors**: replace single `error: string | null` with `errors: string[]`, render as `<ul>`. Each failed file gets its own line with name + reason + (for size errors) actual MB. - **Clear stale errors**: `setErrors([])` on every new file pick — old failures don't linger after a successful subsequent upload. - **Empty-file guard**: client also rejects `file.size === 0` before upload (mirrors backend 400 "empty file"). ## Test plan - [ ] Open MapPicker via address-input map button; press Tab repeatedly — focus must cycle through close button → map controls → "Подставить адрес" → back to close. Shift+Tab cycles backwards. - [ ] Press Escape inside MapPicker — modal closes, focus returns to the element that opened it. - [ ] Inspect `<link>` and `<script>` injected by `loadLeaflet()`; both must have `integrity` and `crossorigin="anonymous"`. - [ ] Manually corrupt the integrity hash in DevTools `<script>` and reload — browser must block the resource (SRI failure). - [ ] Try uploading a 50 MB phone photo via PhotoUpload — must reject client-side with `<filename>: файл больше 10 МБ (50.0 МБ)` instead of waiting for backend 413. - [ ] Upload 3 files where 1 is 20 MB and 2 are valid — should see 1 error line + 2 photos appear in the grid.

lekss361 added 1 commit 2026-05-24 11:53:33 +00:00

fix(tradein-frontend): MapPicker focus trap + Leaflet SRI, PhotoUpload size check c5506aab37

Two isolated UX/security fixes from 2026-05-24 audit:

* MapPicker.tsx (#12): add role=dialog, aria-modal, aria-labelledby; trap
  Tab/Shift+Tab focus inside modal; Escape closes; restore focus on close.
  Pin Leaflet 1.9.4 CSS+JS with sha256 SRI hashes + crossOrigin=anonymous
  (defends against unpkg CDN hijack).
* PhotoUpload.tsx (#13): client-side file.size check (10 MB mirror of
  backend _MAX_PHOTO_BYTES) prevents wasted bandwidth on oversized uploads.
  Replace single error state with per-file errors[] list. Clear stale
  errors on every new file pick.

lekss361 commented

2026-05-24 12:01:06 +00:00

Author

Owner

Deep Code Review — verdict: APPROVE

Summary: Status ✅ APPROVE · Files: 2 (P1: MapPicker.tsx · P2: PhotoUpload.tsx) · +84 / -7 · scope-disciplined a11y + SRI + client size guard.

Verifications performed

SRI hashes — recomputed locally against unpkg.com/leaflet@1.9.4:

leaflet.js → sha256-20nQCchB9co0qIjJZRGuk2/Z9VM+kNiyxNV1lvTlZBo= ✓ matches commit
leaflet.css → sha256-p4NxAoJBhIIN+hmNHrzRCf9tD/miZyoHS5obTRR9BMY= ✓ matches commit

Backend constant alignment (PhotoUpload.tsx:11 ↔ tradein-mvp/backend/app/api/v1/trade_in.py:442):

MAX_FILE_BYTES = 10 * 1024 * 1024 == _MAX_PHOTO_BYTES = 10 * 1024 * 1024 ✓
file.size === 0 rejection mirrors backend if not content: 400 "empty file" ✓

WAI-ARIA dialog pattern (MapPicker):

role="dialog", aria-modal="true", aria-labelledby="map-picker-title" on inner panel ✓
Initial focus → close button (sensible default; user can Esc immediately) ✓
previouslyFocused.current = document.activeElement saved on mount, restored in cleanup ✓
window keydown listener for Escape — properly removed in cleanup, no leak ✓
Tab/Shift+Tab cycling via onKeyDown on dialog (React synthetic; bubble OK since Escape listener is window-level keydown — different event semantics, no conflict) ✓
Focusables selector covers a[href], button:not([disabled]), input/select/textarea:not([disabled]), [tabindex]:not([tabindex="-1"]) — standard pattern ✓

SSR safety (Next 15 app router):

"use client" directive on line 1 ✓
All window/document access is inside useEffect or loadLeaflet() (called from effect) — no SSR hazard ✓
Already conditionally mounted in EstimateForm.tsx:439 as {mapOpen && <MapPicker .../>} — focus effect runs only when modal opens ✓

Effect lifecycle: focus-management useEffect has [onClose] dep. If parent passes a fresh function each render, cleanup re-runs (removeEventListener + refocus previouslyFocused) and then re-attaches. Behaviorally harmless because previouslyFocused.current is set inside the effect body, so a re-run between user keystrokes would re-record document.activeElement (now likely the close button itself) — losing the original opener. Mitigation: EstimateForm.tsx:442 passes () => setMapOpen(false) inline, which technically changes identity per render of EstimateForm. In practice EstimateForm only re-renders when its own state changes (mostly tied to form fields), and during an open MapPicker the dialog backdrop covers the form so user interactions don't propagate field-changes — risk is theoretical for this codebase. Non-blocking for this PR; flag for follow-up: either wrap onClose in useCallback at the call site or read onClose via ref inside the effect.

Error array overwrite (PhotoUpload.tsx:60): setErrors(collected) replaces accumulated errors at end of batch. The PR description claims "old failures don't linger after a successful subsequent upload" — verified: pre-loop setErrors([]) clears stale, post-loop setErrors(collected) shows current-batch errors only. Correct.

Key prop on error <li key={i}>: index-as-key is acceptable here because the list rebuilds atomically per batch and is never re-ordered/inserted-in-the-middle. Non-issue.

Cross-file impact

MapPicker consumer: EstimateForm.tsx:439-444 (single call site) — props unchanged, no breakage.
PhotoUpload consumer: search shows it's mounted on the trade-in estimate detail page; state shape change (error: string | null → errors: string[]) is internal, no prop API change.
Leaflet CDN load: existing <script data-leaflet> selector check still works; new SRI attrs are applied only on first inject (when no element exists). Re-opens reuse existing tags — fine, integrity already enforced on first load.

Vault cross-check

audits/TradeIn_MVP_CodeReview_May23.md (40 findings) does not previously call out MapPicker a11y or Leaflet SRI — these are net-new improvements aligned with the audit's general direction (XSS allowlist hardening, C-6 pattern).
decisions/Decision_TradeIn_DataQuality_8PR_Roadmap.md does not conflict.

Non-blocking follow-ups (do NOT block this PR)

L-1: unpkg.com is third-party CDN with no SLA — long-term consider self-hosting Leaflet in frontend/public/vendor/. Out of scope for this fix.
L-2: focus effect dep [onClose] — see Effect lifecycle note above; consider useCallback at call site or ref-in-effect pattern.
L-3: no tests added — trade-in subproject has no existing component test harness; introducing one is its own PR.

Positive

Clean, surgical diff: two isolated concerns kept independent.
Backend ↔ frontend constants synced and commented (// match backend _MAX_PHOTO_BYTES).
SRI hashes computed transparently with documented reproduction command in PR body.
Hand-rolled focus trap avoids adding a runtime dep (focus-trap-react would be overkill for one modal).
Per-file error UX is a clear regression-resistance improvement (old single-string setError overwrote previous failures inside the loop — visible bug pre-PR).

Complexity / blast radius

Risk: low — pure UX/security improvements, no DB/API changes.
Reversibility: trivial revert.
Merge window: any.

Squash-merge approved.

## Deep Code Review — verdict: APPROVE **Summary**: Status ✅ APPROVE · Files: 2 (P1: MapPicker.tsx · P2: PhotoUpload.tsx) · +84 / -7 · scope-disciplined a11y + SRI + client size guard. ### Verifications performed **SRI hashes** — recomputed locally against `unpkg.com/leaflet@1.9.4`: - `leaflet.js` → `sha256-20nQCchB9co0qIjJZRGuk2/Z9VM+kNiyxNV1lvTlZBo=` ✓ matches commit - `leaflet.css` → `sha256-p4NxAoJBhIIN+hmNHrzRCf9tD/miZyoHS5obTRR9BMY=` ✓ matches commit **Backend constant alignment** (`PhotoUpload.tsx:11` ↔ `tradein-mvp/backend/app/api/v1/trade_in.py:442`): - `MAX_FILE_BYTES = 10 * 1024 * 1024` == `_MAX_PHOTO_BYTES = 10 * 1024 * 1024` ✓ - `file.size === 0` rejection mirrors backend `if not content: 400 "empty file"` ✓ **WAI-ARIA dialog pattern** (MapPicker): - `role="dialog"`, `aria-modal="true"`, `aria-labelledby="map-picker-title"` on inner panel ✓ - Initial focus → close button (sensible default; user can Esc immediately) ✓ - `previouslyFocused.current = document.activeElement` saved on mount, restored in cleanup ✓ - `window` keydown listener for Escape — properly removed in cleanup, no leak ✓ - Tab/Shift+Tab cycling via `onKeyDown` on dialog (React synthetic; bubble OK since Escape listener is window-level keydown — different event semantics, no conflict) ✓ - Focusables selector covers `a[href], button:not([disabled]), input/select/textarea:not([disabled]), [tabindex]:not([tabindex="-1"])` — standard pattern ✓ **SSR safety** (Next 15 app router): - `"use client"` directive on line 1 ✓ - All `window`/`document` access is inside `useEffect` or `loadLeaflet()` (called from effect) — no SSR hazard ✓ - Already conditionally mounted in `EstimateForm.tsx:439` as `{mapOpen && <MapPicker .../>}` — focus effect runs only when modal opens ✓ **Effect lifecycle**: focus-management `useEffect` has `[onClose]` dep. If parent passes a fresh function each render, cleanup re-runs (removeEventListener + refocus previouslyFocused) and then re-attaches. Behaviorally harmless because `previouslyFocused.current` is set inside the effect *body*, so a re-run between user keystrokes would re-record `document.activeElement` (now likely the close button itself) — losing the original opener. **Mitigation**: `EstimateForm.tsx:442` passes `() => setMapOpen(false)` inline, which technically changes identity per render of EstimateForm. In practice EstimateForm only re-renders when its own state changes (mostly tied to form fields), and during an open MapPicker the dialog backdrop covers the form so user interactions don't propagate field-changes — risk is theoretical for this codebase. **Non-blocking** for this PR; flag for follow-up: either wrap `onClose` in `useCallback` at the call site or read `onClose` via ref inside the effect. **Error array overwrite** (`PhotoUpload.tsx:60`): `setErrors(collected)` replaces accumulated errors at end of batch. The PR description claims "old failures don't linger after a successful subsequent upload" — verified: pre-loop `setErrors([])` clears stale, post-loop `setErrors(collected)` shows current-batch errors only. Correct. **Key prop** on error `<li key={i}>`: index-as-key is acceptable here because the list rebuilds atomically per batch and is never re-ordered/inserted-in-the-middle. Non-issue. ### Cross-file impact - `MapPicker` consumer: `EstimateForm.tsx:439-444` (single call site) — props unchanged, no breakage. - `PhotoUpload` consumer: search shows it's mounted on the trade-in estimate detail page; state shape change (`error: string | null` → `errors: string[]`) is internal, no prop API change. - Leaflet CDN load: existing `<script data-leaflet>` selector check still works; new SRI attrs are applied only on first inject (when no element exists). Re-opens reuse existing tags — fine, integrity already enforced on first load. ### Vault cross-check - `audits/TradeIn_MVP_CodeReview_May23.md` (40 findings) does **not** previously call out MapPicker a11y or Leaflet SRI — these are net-new improvements aligned with the audit's general direction (XSS allowlist hardening, C-6 pattern). - `decisions/Decision_TradeIn_DataQuality_8PR_Roadmap.md` does not conflict. ### Non-blocking follow-ups (do NOT block this PR) - L-1: `unpkg.com` is third-party CDN with no SLA — long-term consider self-hosting Leaflet in `frontend/public/vendor/`. Out of scope for this fix. - L-2: focus effect dep `[onClose]` — see Effect lifecycle note above; consider `useCallback` at call site or ref-in-effect pattern. - L-3: no tests added — trade-in subproject has no existing component test harness; introducing one is its own PR. ### Positive - Clean, surgical diff: two isolated concerns kept independent. - Backend ↔ frontend constants synced and commented (`// match backend _MAX_PHOTO_BYTES`). - SRI hashes computed transparently with documented reproduction command in PR body. - Hand-rolled focus trap avoids adding a runtime dep (`focus-trap-react` would be overkill for one modal). - Per-file error UX is a clear regression-resistance improvement (old single-string `setError` overwrote previous failures inside the loop — visible bug pre-PR). ### Complexity / blast radius - Risk: **low** — pure UX/security improvements, no DB/API changes. - Reversibility: trivial revert. - Merge window: any. **Squash-merge approved.**