fix(core): handle JSON.parse errors in page callback and DevTools handlers#398
fix(core): handle JSON.parse errors in page callback and DevTools handlers#398lawrence3699 wants to merge 1 commit intoulixee:mainfrom
Conversation
…dlers Wrap unprotected JSON.parse calls in onPageCallback and Connection.onMessage with try-catch to prevent malformed payloads from crashing the session.
There was a problem hiding this comment.
Pull request overview
This PR hardens the session against malformed external JSON inputs by preventing unhandled exceptions from JSON.parse() in two event-handling paths (page callback payloads and DevTools transport messages).
Changes:
- Wrap
JSON.parse(payload)inTab.onPageCallbackwith try/catch and skip invalid payloads with a warning log. - Wrap
JSON.parse(message)inConnection.onMessagewith try/catch and skip invalid DevTools messages with a warning log (message truncated to 500 chars).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/lib/Tab.ts | Adds parse error handling and warning logging for injected page callback payloads. |
| agent/main/lib/Connection.ts | Adds parse error handling and warning logging for incoming DevTools transport messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.warn('Tab.onPageCallback:InvalidPayload', { | ||
| sessionId: this.sessionId, | ||
| frameId, | ||
| payload, | ||
| error, | ||
| }); |
There was a problem hiding this comment.
payload for page recorder events can be very large (and may include DOM content). Logging the full raw payload on parse failure risks log bloat and potential data exposure. Consider truncating the payload (and optionally include payloadLength) similar to the 500-char cap used for DevTools messages.
| error, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Catching JSON.parse errors prevents SyntaxErrors, but parseable non-conforming JSON (e.g., null, a number, or an unexpected object) will still be passed into onPageRecorderEvents and can throw (it assumes an array-of-arrays). Add a lightweight shape/type check (eg, Array.isArray(parsedPayload) and expected length) and warn+return when invalid.
| } | |
| } | |
| // Validate payload shape: expect an array-of-arrays. | |
| const isValidArrayOfArrays = | |
| Array.isArray(parsedPayload) && parsedPayload.every(item => Array.isArray(item)); | |
| if (!isValidArrayOfArrays) { | |
| log.warn('Tab.onPageCallback:InvalidPayloadShape', { | |
| sessionId: this.sessionId, | |
| frameId, | |
| payload, | |
| }); | |
| return; | |
| } |
| error, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
JSON.parse can succeed but return a non-object (eg, null, a number, an array). In that case object.timestamp = ... will throw and still tear down the connection. After parsing, add a guard that object is a non-null plain object before mutating/reading fields; otherwise log+return.
| } | |
| } | |
| if ( | |
| typeof object !== 'object' || | |
| object === null || | |
| Array.isArray(object) | |
| ) { | |
| log.warn('Connection.InvalidMessageType', { | |
| sessionId: null, | |
| message: message.slice(0, 500), | |
| parsedType: typeof object, | |
| }); | |
| return; | |
| } |
Summary
Noticed two places where
JSON.parse()is called on external data without try-catch, which can crash the session if the payload is malformed:Tab.onPageCallback()incore/lib/Tab.ts—JSON.parse(payload)is called directly on the page callback event payload from injected scripts. If a page sends a malformed callback (e.g., during a navigation race or from a misbehaving script), the unhandledSyntaxErrorcrashes the DOM recorder event handling for that tab. Added try-catch with a warning log that includes the sessionId, frameId, and payload for debugging, matching the existing pattern used ingetElementHtml()and other methods in the same file.Connection.onMessage()inagent/main/lib/Connection.ts—JSON.parse(message)on incoming DevTools WebSocket messages has no error handling. While the protocol typically sends valid JSON, transport issues (partial messages, connection resets mid-frame) can produce malformed data. An unhandled exception here tears down the entire browser connection. Added try-catch with a truncated message log (capped at 500 chars to avoid log bloat), consistent with the existinglog.warnpattern used for unknown sessions in the same method.Test plan