fix(tradein-frontend): safeUrl validator, apiFetch dedup, ListingsCard a11y, error boundary #512

Merged

lekss361 merged 1 commit from feat/tradein-frontend-security-a11y into main

2026-05-24 11:47:39 +00:00

lekss361 commented

2026-05-24 11:40:07 +00:00

Owner

Summary

Four security/UX fixes from 2026-05-24 trade-in audit:

#3 lib/safeUrl.ts (new): validate external URLs via new URL(), reject non-http(s) schemes. Backend supplies lot.source_url verbatim from scraped sites — javascript: / data: / file: would have executed on click.
#8 lib/api.ts: refactor apiFetch to delegate to apiFetchWithStatus (which already had the correct read-body-once pattern). Drops the body-stream-twice anti-pattern; both functions retain their signatures.
#7 components/trade-in/ListingsCard.tsx: drop <tr onClick={...}> (not keyboard accessible), keep the inline <a> as the only interactive element, gate href through safeUrl().
#16 app/error.tsx (new): Next 15 App Router route-segment error boundary. Prevents white-screen on component crash; renders Russian error UI with reset button + error.digest for tracing.

Scope-cuts vs original audit

#17 (sessionId crypto.randomUUID) — already implemented in lib/sessionId.ts:11 (native + Math.random fallback for HTTP). No change needed.
DealsCard.tsx / IMVBenchmark.tsx — neither renders user-supplied URLs (DealsCard has no <a>, IMVBenchmark links to a hardcoded avito.ru domain). safeUrl() is reusable for future components.

safeUrl smoke (from worker)

safeUrl("javascript:alert(1)")           → null
safeUrl("https://avito.ru/foo")          → "https://avito.ru/foo"
safeUrl(null)                            → null
safeUrl("data:text/html,<h1>")           → null
safeUrl("http://cian.ru/rent/flat/123/") → "http://cian.ru/rent/flat/123/"

Test plan

Manually attempt lot.source_url = "javascript:alert(1)" via DevTools mutation of TanStack Query cache; verify <a> is not rendered (href would be null).
Trigger a render crash in HouseInfoCard (e.g. throw inside component); verify error.tsx renders with reset button instead of white screen.
Tab through ListingsCard rows; verify each <a> row-link receives focus, Enter opens the external listing in new tab.
Network failure (Caddy 502); verify apiFetch<T> throws HTTPError with parsed-or-fallback body (no "body stream already read" exception).

## Summary Four security/UX fixes from 2026-05-24 trade-in audit: - **#3** `lib/safeUrl.ts` (new): validate external URLs via `new URL()`, reject non-`http(s)` schemes. Backend supplies `lot.source_url` verbatim from scraped sites — `javascript:` / `data:` / `file:` would have executed on click. - **#8** `lib/api.ts`: refactor `apiFetch` to delegate to `apiFetchWithStatus` (which already had the correct read-body-once pattern). Drops the body-stream-twice anti-pattern; both functions retain their signatures. - **#7** `components/trade-in/ListingsCard.tsx`: drop `<tr onClick={...}>` (not keyboard accessible), keep the inline `<a>` as the only interactive element, gate `href` through `safeUrl()`. - **#16** `app/error.tsx` (new): Next 15 App Router route-segment error boundary. Prevents white-screen on component crash; renders Russian error UI with reset button + error.digest for tracing. ## Scope-cuts vs original audit - **#17 (sessionId crypto.randomUUID)** — already implemented in `lib/sessionId.ts:11` (native + Math.random fallback for HTTP). No change needed. - `DealsCard.tsx` / `IMVBenchmark.tsx` — neither renders user-supplied URLs (DealsCard has no `<a>`, IMVBenchmark links to a hardcoded `avito.ru` domain). `safeUrl()` is reusable for future components. ## safeUrl smoke (from worker) ``` safeUrl("javascript:alert(1)") → null safeUrl("https://avito.ru/foo") → "https://avito.ru/foo" safeUrl(null) → null safeUrl("data:text/html,<h1>") → null safeUrl("http://cian.ru/rent/flat/123/") → "http://cian.ru/rent/flat/123/" ``` ## Test plan - [ ] Manually attempt `lot.source_url = "javascript:alert(1)"` via DevTools mutation of TanStack Query cache; verify `<a>` is not rendered (href would be null). - [ ] Trigger a render crash in HouseInfoCard (e.g. throw inside component); verify `error.tsx` renders with reset button instead of white screen. - [ ] Tab through ListingsCard rows; verify each `<a>` row-link receives focus, Enter opens the external listing in new tab. - [ ] Network failure (Caddy 502); verify `apiFetch<T>` throws `HTTPError` with parsed-or-fallback body (no "body stream already read" exception).

lekss361 added 1 commit 2026-05-24 11:40:08 +00:00

fix(tradein-frontend): safeUrl validator, apiFetch dedup, ListingsCard a11y, error boundary 5bd9c2f66d

Four security/UX fixes from 2026-05-24 audit:

* lib/safeUrl.ts (new): validate external URLs, reject non-http(s) schemes
  (closes finding #3 — backend supplies lot.source_url verbatim from scraped
  sites; javascript:/data:/file: would have executed on click).
* lib/api.ts: refactor apiFetch to delegate to apiFetchWithStatus — single
  read-body-once implementation, drops the body-stream-twice anti-pattern
  (finding #8).
* components/trade-in/ListingsCard.tsx: drop tr.onClick (not keyboard
  accessible), keep the inline <a> as the only interactive element, gate it
  through safeUrl() (findings #7 + #3).
* app/error.tsx (new): Next 15 App Router route-segment error boundary —
  prevents white-screen on component crash (finding #16).

Scope-cut from original audit:
* #17 (sessionId crypto.randomUUID) — already implemented in sessionId.ts;
  no change needed.
* safeUrl wiring in DealsCard / IMVBenchmark — neither renders user-supplied
  URLs (DealsCard has no links, IMVBenchmark links a hardcoded domain).

lekss361 commented

2026-05-24 11:47:29 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 4 (P0:1 safeUrl.ts, P1:1 api.ts, P1:1 ListingsCard.tsx, P2:1 error.tsx)
Lines: +96 / -26 · PR: #512 · base sha 0767fb74 · head sha 5bd9c2f

Security 🔒

lib/safeUrl.ts — WHATWG URL parser + strict http:/https: allowlist. Correctly blocks javascript:/data:/vbscript:/file:/ftp:/custom schemes. Type guard typeof raw !== "string" defends against runtime non-string mutation (TanStack cache poisoning, backend type drift). u.toString() returns WHATWG-canonical form — prevents smuggling via backslash mixing.
ListingsCard.tsx — rel="noopener noreferrer" retained, <tr onClick> removed (was keyboard-inaccessible), inline <a> becomes sole interactive element. Tab+Enter UX restored.
error.tsx — error.digest interpolated via React JSX (auto-escaped). No DOM injection vector.

IDN homograph (intentional non-blocker)

new URL() accepts Cyrillic/IDN hostnames — by design. Blocking IDN at the validator would break legitimate scraped listings from РФ sites (entire product domain). XSS execution requires scheme, not hostname; IDN homograph is a phishing concern, not script execution. Correct scope cut for this PR.

Correctness 🎯

apiFetch now delegates to apiFetchWithStatus. Behavior change: throws HTTPError extends Error (carries .status + .body) instead of plain Error. All consumers in lib/trade-in-api.ts are TanStack Query hooks treating errors as opaque Error — fully backward-compatible.
PR description claims old apiFetch had "body-stream-twice" bug — actually the old code only read the body once per call path (.text() in error, .json() on success — never both in same call). The refactor is still net-positive: DRY, single error-handling path, richer thrown errors. Just want to flag the description nuance for the record.
error.tsx matches Next 15 App Router error.tsx contract ("use client", {error, reset} props, useEffect for logging).

Performance ⚡

No regression. new URL() per AnalogRow render is negligible.

Vault cross-check 📚

Aligned with decisions/Decision_TradeIn_DataQuality_8PR_Roadmap (findings #3/#7/#8/#16 from 2026-05-24 audit — explicitly enumerated in PR body).
Reuses safeUrl naming precedent from old/fixes/Bug_RenderMarkdown_JavascriptUrl_May14. Allowlist is narrower (no mailto:///#) — correct for scraped external listing URLs, not user markdown.

Tests 🧪

No automated tests added; tradein-mvp/frontend/ has no existing test infra. PR includes manual smoke results + 4-item test-plan checklist. Acceptable for scope per project conventions.

Positive

Surgical 4-file PR, no scope creep.
Defence-in-depth: safeUrl is reusable for future scraped-URL surfaces (DealsCard/IMVBenchmark called out in PR body as future consumers).
error.tsx uses inline styles + CSS vars — renders even if Tailwind fails to hydrate, which is what you want from an error boundary.

Complexity / blast radius

Risk: Low. Pure additive security + UX hardening.
Reversibility: Trivial (4 files, no DB, no API contract change).
Merge window: any time.

## Deep Code Review — verdict ### Summary - Status: APPROVE - Files: 4 (P0:1 safeUrl.ts, P1:1 api.ts, P1:1 ListingsCard.tsx, P2:1 error.tsx) - Lines: +96 / -26 · PR: #512 · base sha 0767fb74 · head sha 5bd9c2f ### Security 🔒 - `lib/safeUrl.ts` — WHATWG `URL` parser + strict `http:`/`https:` allowlist. Correctly blocks `javascript:`/`data:`/`vbscript:`/`file:`/`ftp:`/custom schemes. Type guard `typeof raw !== "string"` defends against runtime non-string mutation (TanStack cache poisoning, backend type drift). `u.toString()` returns WHATWG-canonical form — prevents smuggling via backslash mixing. - `ListingsCard.tsx` — `rel="noopener noreferrer"` retained, `<tr onClick>` removed (was keyboard-inaccessible), inline `<a>` becomes sole interactive element. Tab+Enter UX restored. - `error.tsx` — `error.digest` interpolated via React JSX (auto-escaped). No DOM injection vector. ### IDN homograph (intentional non-blocker) `new URL()` accepts Cyrillic/IDN hostnames — by design. Blocking IDN at the validator would break legitimate scraped listings from РФ sites (entire product domain). XSS execution requires scheme, not hostname; IDN homograph is a phishing concern, not script execution. Correct scope cut for this PR. ### Correctness 🎯 - `apiFetch` now delegates to `apiFetchWithStatus`. Behavior change: throws `HTTPError extends Error` (carries `.status` + `.body`) instead of plain `Error`. All consumers in `lib/trade-in-api.ts` are TanStack Query hooks treating errors as opaque `Error` — fully backward-compatible. - PR description claims old `apiFetch` had "body-stream-twice" bug — actually the old code only read the body once per call path (`.text()` in error, `.json()` on success — never both in same call). The refactor is still net-positive: DRY, single error-handling path, richer thrown errors. Just want to flag the description nuance for the record. - `error.tsx` matches Next 15 App Router `error.tsx` contract (`"use client"`, `{error, reset}` props, `useEffect` for logging). ### Performance ⚡ No regression. `new URL()` per `AnalogRow` render is negligible. ### Vault cross-check 📚 - Aligned with `decisions/Decision_TradeIn_DataQuality_8PR_Roadmap` (findings #3/#7/#8/#16 from 2026-05-24 audit — explicitly enumerated in PR body). - Reuses `safeUrl` naming precedent from `old/fixes/Bug_RenderMarkdown_JavascriptUrl_May14`. Allowlist is narrower (no `mailto:`/`/`/`#`) — correct for scraped external listing URLs, not user markdown. ### Tests 🧪 No automated tests added; `tradein-mvp/frontend/` has no existing test infra. PR includes manual smoke results + 4-item test-plan checklist. Acceptable for scope per project conventions. ### Positive - Surgical 4-file PR, no scope creep. - Defence-in-depth: `safeUrl` is reusable for future scraped-URL surfaces (DealsCard/IMVBenchmark called out in PR body as future consumers). - `error.tsx` uses inline styles + CSS vars — renders even if Tailwind fails to hydrate, which is what you want from an error boundary. ### Complexity / blast radius - Risk: Low. Pure additive security + UX hardening. - Reversibility: Trivial (4 files, no DB, no API contract change). - Merge window: any time.