fix(server): guard JSON.parse in protocol message handlers#40756
Conversation
PlaywrightConnection and PipeTransport call JSON.parse on incoming messages without try-catch. In PlaywrightConnection, the parse runs inside an async event handler whose rejected Promise is unhandled, crashing the server process. In PipeTransport, the parse runs inside a setImmediate callback, causing an uncaughtException. Wrap both in try-catch to match the existing pattern in WebSocketTransport (added in microsoft#23689). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
| this._waitForNextTask(() => { | ||
| let parsedMessage; | ||
| try { | ||
| parsedMessage = JSON.parse(message); |
There was a problem hiding this comment.
(meaning keep crashing on invalid pipe data)
There was a problem hiding this comment.
Agreed, reverted the pipeTransport changes. The guard now only covers PlaywrightConnection (untrusted remote clients).
PipeTransport talks to a local browser process over stdio. If the browser sends malformed JSON, the situation is unrecoverable; crashing is the correct behavior. Keep the guard only in PlaywrightConnection, which handles untrusted remote WebSocket clients. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Test results for "MCP"7 failed 7050 passed, 1068 skipped Merge workflow run. |
Test results for "tests 1"2 flaky41718 passed, 850 skipped Merge workflow run. |
2 participants
Summary
PlaywrightConnectionandPipeTransportcallJSON.parseon incoming messages without try-catch, unlikeWebSocketTransportwhich already guards against this (chore: log when websockets are proactively closed #23689).PlaywrightConnection, the parse runs inside an async event handler. The rejected Promise is unhandled (no globalunhandledRejectionhandler in the remote server), crashing the process. A single malformed WebSocket frame takes down all connected clients.PipeTransport, the parse runs inside asetImmediatecallback, causinguncaughtExceptionon corrupted browser output.WebSocketTransportpattern.PlaywrightConnectioncloses the offending connection with WebSocket code 1007;PipeTransportlogs and skips the malformed message.The
PlaywrightConnectionfix is the same approach from #40121 (approved by @pavelfeldman but not merged). This PR also coversPipeTransport.The unguarded
JSON.parseinPlaywrightConnectionwas introduced in #28141 (2023-11-16). InPipeTransport, it has been unguarded since #1567 (2020-03-26).