Skip to content

Fix multiple bugs: XSS vulnerabilities, operator precedence, event listener loss, race condition#2180

Open
hobostay wants to merge 1 commit intomicrosoft:mainfrom
hobostay:fix/multiple-bugs
Open

Fix multiple bugs: XSS vulnerabilities, operator precedence, event listener loss, race condition#2180
hobostay wants to merge 1 commit intomicrosoft:mainfrom
hobostay:fix/multiple-bugs

Conversation

@hobostay
Copy link
Copy Markdown

Summary

  • Fix XSS in browserViewManager.ts: Three locations directly interpolated URLs and error descriptions into document.body.innerHTML via executeJavaScript without escaping. Malicious URLs containing <script> tags or HTML entities could execute arbitrary JavaScript in the Electron renderer. Fixed by HTML-escaping all interpolated values before use.

  • Fix operator precedence bug in videoActionHandler.ts: The expression "text" + statusData.failure_reason ?? "" evaluates as ("text" + statusData.failure_reason) ?? "" because + has higher precedence than ??. This makes the nullish coalescing operator dead code — when failure_reason is undefined, the string "undefined" is displayed instead of an empty string. Added parentheses: (statusData.failure_reason ?? "").

  • Fix unhandled promise rejection in setContent.ts: The Promise.all(promises).then(...) for loading CSS into iframes has no .catch() handler. If any CSS fetch fails, the iframe never gets srcdoc content at all. Added a .catch() that renders the message content even without CSS.

  • Fix event listener loss in setContent.ts: contentElm.innerHTML += contentHtml serializes and re-parses the entire DOM, destroying all existing event listeners on child elements (including previously-attached link click handlers). Replaced with insertAdjacentHTML("beforeend") which preserves existing DOM nodes.

  • Fix race condition in webSocketAPI.ts: The onclose handler unconditionally calls createWebSocket() when autoReconnect is true. If the socket closes rapidly multiple times (e.g., network flapping), multiple concurrent reconnection attempts are spawned, potentially creating duplicate WebSocket connections. Added a reconnecting flag to serialize reconnection attempts.

Test plan

  • Verify browser tabs still show error pages correctly when URLs fail to load
  • Verify video agent displays failure reason correctly when failure_reason is undefined vs set
  • Verify iframe content renders even when CSS fetch fails
  • Verify link click handlers still work after multiple messages are appended in chat
  • Verify WebSocket reconnects properly after connection loss without creating duplicates

🤖 Generated with Claude Code

…e condition

- Fix XSS in browserViewManager.ts: HTML-escape URLs and error messages
  before interpolating into innerHTML via executeJavaScript (3 locations)
- Fix operator precedence bug in videoActionHandler.ts: `+` has higher
  precedence than `??`, making `failure_reason ?? ""` dead code. Added
  parentheses to correct the logic
- Fix unhandled promise rejection in setContent.ts: added .catch() handler
  for CSS loading in iframe so content is still shown on fetch failure
- Fix event listener loss in setContent.ts: replaced `innerHTML +=` with
  `insertAdjacentHTML("beforeend")` to avoid destroying existing DOM nodes
  and their event listeners
- Fix race condition in webSocketAPI.ts: added `reconnecting` flag to
  prevent multiple concurrent reconnection attempts on rapid socket closes

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
// only show the error if it's for the page the user was asking
// it's possible some other resource failed to load (image, script, etc.)
if (validatedURL === options.url) {
const safeUrl = options.url
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this is pattern is repeated for safeErr, can we make this a utility method somewhere and call it.

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.

2 participants