Skip to content

Commit

Permalink
Moves websocket error handling to the first possible place to avoid r…
Browse files Browse the repository at this point in the history
…ace condition (#425)
  • Loading branch information
janthurau authored Oct 19, 2022
1 parent 839044b commit cc5cfe2
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 12 deletions.
12 changes: 0 additions & 12 deletions packages/server/src/Connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export class Connection {
this.webSocket.on('close', this.close.bind(this))
this.webSocket.on('message', this.handleMessage.bind(this))
this.webSocket.on('pong', () => { this.pongReceived = true })
this.webSocket.on('error', this.handleError.bind(this))

this.sendCurrentAwareness()
}
Expand Down Expand Up @@ -197,17 +196,6 @@ export class Connection {
})
}

/**
* Handle a ws instance error, which is required to prevent
* the server from crashing when one happens
* See https://github.com/websockets/ws/issues/1777#issuecomment-660803472
* @private
*/
private handleError(error: any): void {
this.logger.log('Error emitted from webSocket instance:')
this.logger.log(error)
}

/**
* Get the underlying connection instance
* @deprecated
Expand Down
12 changes: 12 additions & 0 deletions packages/server/src/Hocuspocus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,18 @@ export class Hocuspocus {
const webSocketServer = new WebSocketServer({ noServer: true })

webSocketServer.on('connection', async (incoming: WebSocket, request: IncomingMessage) => {

incoming.on('error', error => {
/**
* Handle a ws instance error, which is required to prevent
* the server from crashing when one happens
* See https://github.com/websockets/ws/issues/1777#issuecomment-660803472
* @private
*/
this.debugger.log('Error emitted from webSocket instance:')
this.debugger.log(error)
})

this.handleConnection(incoming, request, await this.getDocumentNameFromRequest(request))
})

Expand Down
31 changes: 31 additions & 0 deletions tests/server/websocketError.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import test from 'ava'
import { onAuthenticatePayload } from '@hocuspocus/server'
import { newHocuspocus, newHocuspocusProvider } from '../utils'

test('does not crash when invalid opcode is sent', async t => {
Expand All @@ -23,3 +24,33 @@ test('does not crash when invalid opcode is sent', async t => {
})
})
})

test('does not crash when invalid utf-8 sequence is sent', async t => {
await new Promise(resolve => {
const server = newHocuspocus({
async onAuthenticate(data: onAuthenticatePayload) {
return new Promise(resolve => {
setTimeout(resolve, 2000)
})
},
})

const provider = newHocuspocusProvider(server, {
token: 'test123',
onClose({ event }) {
t.is(event.code, 1002)
provider.destroy()
},
onDestroy() {
t.pass()
resolve(true)
},
})

setInterval(() => {
// @ts-ignore
provider.webSocket?._socket.write(Buffer.from([0x81, 0x04, 0xce, 0xba, 0xe1, 0xbd])) // eslint-disable-line
}, 500)

})
})

0 comments on commit cc5cfe2

Please sign in to comment.