Emit a packetParseFailure diagnostic event for malformed packets

docs/API.md

@@ -284,6 +284,22 @@ Emitted after the player enters the PLAY protocol state and can send and recieve
 
 Called when an error occurs within the client. Takes an Error as parameter.
 
+### `packetParseFailure` event
+
+Diagnostic event emitted when an incoming packet could not be parsed, or was not fully read. Nothing is computed
+unless there is a listener for this event. It does not replace the `error` event: a `ReadError` is still followed by an
+`error` event as before, while `PartialReadError` and `SizeMismatch` are otherwise silent. Takes one object parameter:
+* type -- `'ReadError'` (the deserializer threw), `'PartialReadError'` (the packet ended before its fields were fully read)
+  or `'SizeMismatch'` (the packet parsed but did not consume the whole buffer)
+* state -- the protocol state the packet was parsed in
+* direction -- `'toClient'` on a client, `'toServer'` on a server-side client
+* buffer -- the raw (decompressed) packet buffer, including the packet id
+* packetId -- the leading varint of the buffer, or null
+* packetName -- the packet name resolved from this version's protocol data, or null
+* error -- the Error, for `ReadError` and `PartialReadError`, else null
+* bytesRead -- how many bytes were parsed, for `SizeMismatch`, else null
+* partialData -- the parsed data, for `SizeMismatch`, else null
+
 ### `playerChat` event
 
 Called when a chat message from another player arrives. The emitted object contains:

src/client.js

@@ -13,6 +13,19 @@ const createDecipher = require('./transforms/encryption').createDecipher
 
 const closeTimeout = 30 * 1000
 
+// Read the leading varint of a packet buffer (the packet id), or null if it can't be read
+function readLeadingVarint (buffer) {
+  let result = 0
+  let shift = 0
+  for (let i = 0; i < buffer.length && i < 5; i++) {
+    const byte = buffer[i]
+    result |= (byte & 0x7f) << shift
+    if (!(byte & 0x80)) return result >>> 0
+    shift += 7
+  }
+  return null
+}
+
 class Client extends EventEmitter {
   constructor (isServer, version, customPackets, hideErrors = false) {
     super()
@@ -67,18 +80,47 @@ class Client extends EventEmitter {
       this.emit('error', e)
     })
 
+    const deserializerDirection = this.isServer ? 'toServer' : 'toClient'
+    // Diagnostic for packets the deserializer could not (fully) parse. Only built when
+    // someone listens for it, and never allowed to throw into the stream pipeline.
+    const emitParseFailure = (type, buffer, extra) => {
+      if (this.listenerCount('packetParseFailure') === 0) return
+      try {
+        const packetId = Buffer.isBuffer(buffer) ? readLeadingVarint(buffer) : null
+        this.emit('packetParseFailure', {
+          type,
+          state,
+          direction: deserializerDirection,
+          buffer,
+          packetId,
+          packetName: this._resolvePacketName(state, deserializerDirection, packetId),
+          error: null,
+          bytesRead: null,
+          partialData: null,
+          ...extra
+        })
+      } catch (err) {
+        debug('packetParseFailure diagnostic failed', err)
+      }
+    }
+
     this.deserializer.on('error', (e) => {
+      emitParseFailure('ReadError', e.buffer, { error: e })
       let parts = []
       if (e.field) {
         parts = e.field.split('.')
         parts.shift()
       }
-      const deserializerDirection = this.isServer ? 'toServer' : 'toClient'
       e.field = [this.protocolState, deserializerDirection].concat(parts).join('.')
       e.message = e.buffer ? `Parse error for ${e.field} (${e.buffer?.length} bytes, ${e.buffer?.toString('hex').slice(0, 6)}...) : ${e.message}` : `Parse error for ${e.field}: ${e.message}`
       if (!this.compressor) { this.splitter.pipe(this.deserializer) } else { this.decompressor.pipe(this.deserializer) }
       this.emit('error', e)
     })
+    // protodef's FullPacketParser swallows partial reads (it only logs them unless
+    // noErrorLogging is set) and emits this event (protodef >= 1.20) with the chunk it could not read
+    this.deserializer.on('partialReadError', (e) => {
+      emitParseFailure('PartialReadError', e.buffer, { error: e })
+    })
     this._mcBundle = []
     const emitPacket = (parsed) => {
       this.emit('packet', parsed.data, parsed.metadata, parsed.buffer, parsed.fullBuffer)
@@ -87,6 +129,9 @@ class Client extends EventEmitter {
       this.emit('raw', parsed.buffer, parsed.metadata)
     }
     this.deserializer.on('data', (parsed) => {
+      if (parsed.fullBuffer && parsed.metadata.size !== parsed.fullBuffer.length) {
+        emitParseFailure('SizeMismatch', parsed.fullBuffer, { bytesRead: parsed.metadata.size, partialData: parsed.data })
+      }
       parsed.metadata.name = parsed.data.name
       parsed.data = parsed.data.params
       parsed.metadata.state = state
@@ -116,6 +161,17 @@ class Client extends EventEmitter {
     })
   }
 
+  // Resolve a packet id to its name from this version's protocol data, or null
+  _resolvePacketName (state, direction, packetId) {
+    if (packetId === null || packetId === undefined) return null
+    try {
+      const mappings = require('minecraft-data')(this.version).protocol[state][direction].types.packet[1][0].type[1].mappings
+      return mappings['0x' + packetId.toString(16).padStart(2, '0')] ?? null
+    } catch (err) {
+      return null
+    }
+  }
+
   set state (newProperty) {
     const oldProperty = this.protocolState
     this.protocolState = newProperty

src/index.d.ts

@@ -48,6 +48,7 @@ declare module 'minecraft-protocol' {
 		on(event: 'state', handler: (newState: States, oldState: States) => PromiseLike): this
 		on(event: 'end', handler: (reason: string) => PromiseLike): this
 		on(event: 'connect', handler: () => PromiseLike): this
+		on(event: 'packetParseFailure', handler: (failure: PacketParseFailure) => PromiseLike): this
 		on(event: string, handler: (data: any, packetMeta: PacketMeta) => PromiseLike): this
 		on(event: `raw.${string}`, handler: (buffer: Buffer, packetMeta: PacketMeta) => PromiseLike): this
 		on(event: 'playerChat', handler: (data: {
@@ -74,6 +75,7 @@ declare module 'minecraft-protocol' {
 		once(event: 'state', handler: (newState: States, oldState: States) => PromiseLike): this
 		once(event: 'end', handler: (reasonce: string) => PromiseLike): this
 		once(event: 'connect', handler: () => PromiseLike): this
+		once(event: 'packetParseFailure', handler: (failure: PacketParseFailure) => PromiseLike): this
 		once(event: string, handler: (data: any, packetMeta: PacketMeta) => PromiseLike): this
 		once(event: `raw.${string}`, handler: (buffer: Buffer, packetMeta: PacketMeta) => PromiseLike): this
 	}
@@ -246,6 +248,18 @@ declare module 'minecraft-protocol' {
 		state: States
 	}
 
+	export interface PacketParseFailure {
+		type: 'ReadError' | 'PartialReadError' | 'SizeMismatch'
+		state: States
+		direction: 'toServer' | 'toClient'
+		buffer: Buffer
+		packetId: number | null
+		packetName: string | null
+		error: Error | null
+		bytesRead: number | null
+		partialData: any
+	}
+
 	export interface PingOptions {
 		host?: string
 		majorVersion?: string

test/packetParseFailureTest.js

@@ -0,0 +1,119 @@
+/* eslint-env mocha */
+
+const mc = require('../')
+const assert = require('power-assert')
+const { once } = require('events')
+const { getPort } = require('./common/util')
+
+// FullPacketParser only emits 'partialReadError' since protodef 1.20.0 ; before that, partial reads are swallowed
+const protodefVersion = require('protodef/package.json').version.split('.').map(Number)
+const protodefEmitsPartialReadError = protodefVersion[0] > 1 || (protodefVersion[0] === 1 && protodefVersion[1] >= 20)
+
+function writeVarint (value) {
+  const bytes = []
+  while (value >= 0x80) {
+    bytes.push((value & 0x7f) | 0x80)
+    value >>>= 7
+  }
+  bytes.push(value)
+  return Buffer.from(bytes)
+}
+
+for (const supportedVersion of mc.supportedVersions) {
+  const mcData = require('minecraft-data')(supportedVersion)
+  const version = mcData.version
+
+  const packetMappings = mcData.protocol.play.toClient.types.packet[1][0].type[1].mappings
+  const keepAliveId = parseInt(Object.keys(packetMappings).find(id => packetMappings[id] === 'keep_alive'))
+  const keepAliveIdType = mcData.protocol.play.toClient.types.packet_keep_alive[1][0].type
+  const validKeepAlive = mc.createSerializer({ state: mc.states.PLAY, isServer: true, version: version.minecraftVersion })
+    .createPacketBuffer({ name: 'keep_alive', params: { keepAliveId: keepAliveIdType === 'i64' ? 42n : 42 } })
+
+  describe('packetParseFailure ' + supportedVersion + 'v', function () {
+    this.timeout(10000)
+
+    // Starts a server and connects a client to it ; resolves once the server-side client is in play state
+    async function connect () {
+      const port = await getPort()
+      const server = mc.createServer({
+        'online-mode': false,
+        version: version.minecraftVersion,
+        port
+      })
+      const serverErrors = []
+      server.on('error', err => serverErrors.push(err))
+      await once(server, 'listening')
+      const client = mc.createClient({
+        username: 'parseFailure',
+        host: '127.0.0.1',
+        version: version.minecraftVersion,
+        port
+      })
+      const [serverClient] = await once(server, 'playerJoin')
+      // The client only enters play after the server does on older versions
+      if (client.state !== mc.states.PLAY) await once(client, 'state')
+      assert.strictEqual(client.state, mc.states.PLAY)
+      const close = async () => {
+        client.end()
+        server.close()
+        await once(server, 'close')
+        assert.deepStrictEqual(serverErrors, [])
+      }
+      return { client, serverClient, close }
+    }
+
+    it('emits PartialReadError for a packet whose body is missing', async function () {
+      if (!protodefEmitsPartialReadError) return this.skip()
+      const sent = writeVarint(keepAliveId)
+      const { client, serverClient, close } = await connect()
+      const failurePromise = once(client, 'packetParseFailure')
+      serverClient.writeRaw(sent)
+      const [failure] = await failurePromise
+      assert.strictEqual(failure.type, 'PartialReadError')
+      assert.strictEqual(failure.state, 'play')
+      assert.strictEqual(failure.direction, 'toClient')
+      assert.strictEqual(failure.packetId, keepAliveId)
+      assert.strictEqual(failure.packetName, 'keep_alive')
+      assert.ok(failure.buffer.equals(sent), `buffer ${failure.buffer.toString('hex')} !== ${sent.toString('hex')}`)
+      assert.ok(failure.error instanceof Error)
+      assert.ok(failure.error.partialReadError)
+      assert.strictEqual(failure.bytesRead, null)
+      assert.strictEqual(failure.partialData, null)
+      await close()
+    })
+
+    it('emits SizeMismatch for a packet with trailing bytes, and still emits the packet', async function () {
+      const sent = Buffer.concat([validKeepAlive, Buffer.from([0xde, 0xad, 0xbe, 0xef])])
+      const { client, serverClient, close } = await connect()
+      const failurePromise = once(client, 'packetParseFailure')
+      const packetPromise = once(client, 'keep_alive')
+      serverClient.writeRaw(sent)
+      const [failure] = await failurePromise
+      assert.strictEqual(failure.type, 'SizeMismatch')
+      assert.strictEqual(failure.state, 'play')
+      assert.strictEqual(failure.direction, 'toClient')
+      assert.strictEqual(failure.packetId, keepAliveId)
+      assert.strictEqual(failure.packetName, 'keep_alive')
+      assert.ok(failure.buffer.equals(sent), `buffer ${failure.buffer.toString('hex')} !== ${sent.toString('hex')}`)
+      assert.strictEqual(failure.bytesRead, validKeepAlive.length)
+      assert.ok(failure.bytesRead < failure.buffer.length)
+      assert.strictEqual(failure.error, null)
+      assert.strictEqual(failure.partialData.name, 'keep_alive')
+      assert.strictEqual(Number(failure.partialData.params.keepAliveId), 42)
+      const [keepAlive] = await packetPromise
+      assert.strictEqual(Number(keepAlive.keepAliveId), 42)
+      await close()
+    })
+
+    it('does not emit anything for a well-formed packet', async function () {
+      const { client, serverClient, close } = await connect()
+      let failures = 0
+      client.on('packetParseFailure', () => failures++)
+      const packetPromise = once(client, 'keep_alive')
+      serverClient.writeRaw(validKeepAlive)
+      await packetPromise
+      assert.strictEqual(failures, 0)
+      await close()
+    })
+  })
+}