Skip to content

Harden browser response boundaries - #467

Open
parsakhaz wants to merge 4 commits into
mainfrom
local-403-react-doctor-4i
Open

Harden browser response boundaries#467
parsakhaz wants to merge 4 commits into
mainfrom
local-403-react-doctor-4i

Conversation

@parsakhaz

@parsakhaz parsakhaz commented Aug 18, 2026

Copy link
Copy Markdown
Member

Summary

  • isolate the web fallback Discord window from its opener
  • branch on remote-daemon HTTP status before decoding success or error payloads
  • separate notebook JSON text formatting from the component sanitized HTML render paths

Part of #403.

React Doctor

  • current-main baseline: 0 errors, 317 warnings, 317 total
  • after: 0 errors, 314 warnings, 314 total
  • removes window-open-without-noopener, no-fetch-response-used-without-status-check, and the false-positive unsafe-json-in-html finding without replacements

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm --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 --json

Automated QA

  • Status: pass at 4c4017cb
  • Remote invoke transport: 42/42 focused tests passed across browser, main client, and HTTP server, including non-JSON 401 and malformed 200 responses.
  • Notebook JSON boundary: server-render proof confirms script-like JSON is emitted as escaped text, with no raw script element.
  • Full gates: 267/267 frontend tests, root typecheck, root lint, and React Doctor all passed. React Doctor reported 0 errors and 315 warnings on current main; none are the three boundary findings addressed by this PR.
  • Screenshots: omitted because the UI changes have no intended visual delta.
  • Remaining human check: optionally open the Discord fallback in web mode and inspect that the new tab has no opener.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 4c4017c.

@parsakhaz parsakhaz left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@parsakhaz
parsakhaz force-pushed the local-403-react-doctor-4i branch from de82aac to 4c4017c Compare August 23, 2026 19:43
@parsakhaz

Copy link
Copy Markdown
Member Author

Deep refactor Follow-ups

  • Consolidate browser and main-process retry/status policy only after adding a shared cross-runtime failure matrix for 408, 401/403, 429, 5xx, malformed JSON, and aborted requests.
  • Validate daemon command results as JSON-safe values before HTTP serialization. The server registry currently returns unknown, and tightening that boundary needs broader command coverage.
  • Add focused notebook sanitizer tests for hostile HTML and SVG payloads. This pass proves the JSON text path, while the existing DOMPurify paths remain unchanged.

@parsakhaz

Copy link
Copy Markdown
Member Author

Three-pass review complete

Review

  • Rebased the PR onto current main.
  • Found one Must-Fix: auth and non-retryable status handling still depended on parsing a JSON body.
  • Fixed it in ff8e8253 and added coverage proving an HTML 401 stops after one request and returns connection-code guidance.
  • The formal review is COMMENT because GitHub does not allow an author to request changes on their own PR.

Simplify

  • f2b79748 removes the redundant RequestInit spread and success else branch.
  • MIME string and line-array decoding now uses the existing union schema once.
  • DiscordPopup and the regression fixture were reviewed but left unchanged because further abstraction would add indirection.

Refactor

  • 4c4017cb moves the invoke success/error envelope and BoundarySchema into the shared remote-daemon contract.
  • The HTTP server, main client, and browser client now consume one response definition.
  • The browser now runtime-decodes successful envelopes instead of trusting a TypeScript assertion.
  • Added tests for malformed 200 envelopes and escaped notebook JSON text.

Validation

  • pnpm typecheck
  • pnpm lint, including zero advisory anti-slop findings and passing boundary conformance
  • pnpm --filter frontend test: 267/267
  • focused remote transport suites: 42/42
  • React Doctor 0.9.2: 0 errors, 315 warnings on current main; none are the three boundary findings addressed by this PR
  • No screenshots because there is no intended visual change

Left for parsa

  • Wait for the newly triggered GitHub checks, which are currently in progress.
  • Optionally verify the Discord web fallback manually and inspect that the new tab has no opener.
  • Review the separate Follow-ups comment, then merge when satisfied. I did not merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant