Harden browser response boundaries - #467
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
parsakhaz
left a comment
There was a problem hiding this comment.
Verdict: Request changes - HTTP failure classification still depends on decoding the response body. GitHub does not permit the PR author to submit a REQUEST_CHANGES state, so this formal review is posted as COMMENT with a blocking Must-Fix.
Counts: Must Fix: 1 (security: 0) · Should Fix: 0 · pass 1/3
Must Fix
- MF-1 - Classify failure status before requiring a JSON error payload · frontend/src/remote/runtime/remoteDaemonBrowserClient.ts:126 · derive auth and retry behavior from response.status first, then parse an optional server message on a best-effort basis · violates the PR intent to branch on remote-daemon HTTP status before decoding payloads
- Evidence: lines 126-132 await response.json() before the 401/403 branch. A non-JSON response throws into the generic catch at line 147, so status classification is skipped.
- Failure scenario: a reverse proxy or incompatible remote host returns 401 with an HTML or empty body; invoke retries four times and finally exposes a JSON parse error instead of stopping immediately with the invalid connection-code guidance.
Praise
- frontend/src/components/DiscordPopup.tsx:81 isolates the fallback window with noopener,noreferrer.
- frontend/src/components/panels/editor/NotebookPreview.tsx:115 keeps notebook JSON formatting in React text content while DOMPurify remains limited to HTML and SVG render paths.
Checks run: pnpm typecheck; pnpm lint; pnpm --filter frontend test (266 passed); main/src/daemon/remotePwaBrowserRuntime.test.ts passed during the main Vitest run.
de82aac to
4c4017c
Compare
Deep refactor Follow-ups
|
Three-pass review completeReview
Simplify
Refactor
Validation
Left for parsa
|
Summary
Part of #403.
React Doctor
window-open-without-noopener,no-fetch-response-used-without-status-check, and the false-positiveunsafe-json-in-htmlfinding without replacementsValidation
pnpm lintpnpm typecheckpnpm --filter frontend test(245 tests)pnpm build(including signed universal DMG/ZIP packaging)pnpm dlx react-doctor@0.9.2 frontend --no-score --no-supply-chain --no-dead-code --jsonAutomated QA
4c4017cb