Fixes to protocol Holder implementation (#1355)

* Continued updates

* bail tests early on failure

* improve deserialization errors to include packet headers

* Update play.js

* Update server.js

* Update ci.yml

* add debug logging

* update chat player_info handling

* Fix 1.7 player_info and chat type field

* cleanup

* Update compiler-minecraft.js - important fix

* Update compiler-minecraft.js fix holder type

* Update ci.yml

---------

Co-authored-by: Romain Beaumont <[email protected]>

Author: extremeheat

docs/README.md

@@ -174,7 +174,7 @@ server.on('playerJoin', function(client) {
       plainMessage: message,
       signedChatContent: '',
       unsignedChatContent: chatText(message),
-      type: 0,
+      type: mcData.supportFeature('chatTypeIsHolder') ? { chatType: 1 } : 0,
       senderUuid: 'd3527a0b-bc03-45d5-a878-2aafdd8c8a43', // random
       senderName: JSON.stringify({ text: 'me' }),
       senderTeam: undefined,

examples/server/server.js

@@ -75,7 +75,7 @@ function sendBroadcastMessage (server, clients, message, sender) {
       plainMessage: message,
       signedChatContent: '',
       unsignedChatContent: chatText(message),
-      type: 0,
+      type: mcData.supportFeature('chatTypeIsHolder') ? { chatType: 1 } : 0,
       senderUuid: 'd3527a0b-bc03-45d5-a878-2aafdd8c8a43', // random
       senderName: JSON.stringify({ text: sender }),
       senderTeam: undefined,

examples/server_helloworld/server_helloworld.js

@@ -68,7 +68,7 @@ server.on('playerJoin', function (client) {
       plainMessage: message,
       signedChatContent: '',
       unsignedChatContent: chatText(message),
-      type: 0,
+      type: mcData.supportFeature('chatTypeIsHolder') ? { chatType: 1 } : 0,
       senderUuid: 'd3527a0b-bc03-45d5-a878-2aafdd8c8a43', // random
       senderName: JSON.stringify({ text: 'me' }),
       senderTeam: undefined,

src/client.js

@@ -67,14 +67,14 @@ class Client extends EventEmitter {
     })
 
     this.deserializer.on('error', (e) => {
-      let parts
+      let parts = []
       if (e.field) {
         parts = e.field.split('.')
         parts.shift()
-      } else { parts = [] }
+      }
       const deserializerDirection = this.isServer ? 'toServer' : 'toClient'
       e.field = [this.protocolState, deserializerDirection].concat(parts).join('.')
-      e.message = `Deserialization error for ${e.field} : ${e.message}`
+      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)
     })

src/client/chat.js

@@ -106,38 +106,29 @@ module.exports = function (client, options) {
   })
 
   client.on('player_info', (packet) => {
-    if (mcData.supportFeature('playerInfoActionIsBitfield')) { // 1.19.3+
-      if (packet.action & 2) { // chat session
-        for (const player of packet.data) {
-          if (!player.chatSession) continue
-          client._players[player.UUID] = {
-            publicKey: crypto.createPublicKey({ key: player.chatSession.publicKey.keyBytes, format: 'der', type: 'spki' }),
-            publicKeyDER: player.chatSession.publicKey.keyBytes,
-            sessionUuid: player.chatSession.uuid
-          }
-          client._players[player.UUID].sessionIndex = true
-          client._players[player.UUID].hasChainIntegrity = true
+    for (const player of packet.data) {
+      if (player.chatSession) {
+        client._players[player.uuid] = {
+          publicKey: crypto.createPublicKey({ key: player.chatSession.publicKey.keyBytes, format: 'der', type: 'spki' }),
+          publicKeyDER: player.chatSession.publicKey.keyBytes,
+          sessionUuid: player.chatSession.uuid
         }
+        client._players[player.uuid].sessionIndex = true
+        client._players[player.uuid].hasChainIntegrity = true
       }
 
-      return
-    }
-
-    if (packet.action === 0) { // add player
-      for (const player of packet.data) {
-        if (player.crypto) {
-          client._players[player.UUID] = {
-            publicKey: crypto.createPublicKey({ key: player.crypto.publicKey, format: 'der', type: 'spki' }),
-            publicKeyDER: player.crypto.publicKey,
-            signature: player.crypto.signature,
-            displayName: player.displayName || player.name
-          }
-          client._players[player.UUID].hasChainIntegrity = true
+      if (player.crypto) {
+        client._players[player.uuid] = {
+          publicKey: crypto.createPublicKey({ key: player.crypto.publicKey, format: 'der', type: 'spki' }),
+          publicKeyDER: player.crypto.publicKey,
+          signature: player.crypto.signature,
+          displayName: player.displayName || player.name
         }
+        client._players[player.uuid].hasChainIntegrity = true
       }
-    } else if (packet.action === 4) { // remove player
-      for (const player of packet.data) {
-        delete client._players[player.UUID]
+
+      if (packet.action === 'remove_player') { // Only 1.8-1.9
+        delete client._players[player.uuid]
       }
     }
   })

src/client/play.js

@@ -66,7 +66,6 @@ module.exports = function (client, options) {
     }
 
     function onReady () {
-      client.emit('playerJoin')
       if (mcData.supportFeature('signedChat')) {
         if (options.disableChatSigning && client.serverFeatures.enforcesSecureChat) {
           throw new Error('"disableChatSigning" was enabled in client options, but server is enforcing secure chat')
@@ -86,8 +85,8 @@ module.exports = function (client, options) {
       function unsignedChat (message) {
         client.write('chat', { message })
       }
-
       client.chat = client._signedChat || unsignedChat
+      client.emit('playerJoin')
     }
   }
 }

src/datatypes/compiler-minecraft.js

@@ -42,56 +42,14 @@ module.exports = {
       code += '}'
       return compiler.wrapCode(code)
     }],
-    arrayWithLengthOffset: ['parametrizable', (compiler, array) => { // TODO: remove
-      let code = ''
-      if (array.countType) {
-        code += 'const { value: count, size: countSize } = ' + compiler.callType(array.countType) + '\n'
-      } else if (array.count) {
-        code += 'const count = ' + array.count + '\n'
-        code += 'const countSize = 0\n'
-      } else {
-        throw new Error('Array must contain either count or countType')
-      }
-      code += 'if (count > 0xffffff) throw new Error("array size is abnormally large, not reading: " + count)\n'
-      code += 'const data = []\n'
-      code += 'let size = countSize\n'
-      code += `for (let i = 0; i < count + ${array.lengthOffset}; i++) {\n`
-      code += '  const elem = ' + compiler.callType(array.type, 'offset + size') + '\n'
-      code += '  data.push(elem.value)\n'
-      code += '  size += elem.size\n'
-      code += '}\n'
-      code += 'return { value: data, size }'
-      return compiler.wrapCode(code)
-    }],
-    bitflags: ['parametrizable', (compiler, { type, flags, shift, big }) => {
-      let fstr = JSON.stringify(flags)
-      if (Array.isArray(flags)) {
-        fstr = '{'
-        for (const [k, v] of Object.entries(flags)) fstr += `"${v}": ${big ? (1n << BigInt(k)) : (1 << k)}` + (big ? 'n,' : ',')
-        fstr += '}'
-      } else if (shift) {
-        fstr = '{'
-        for (const key in flags) fstr += `"${key}": ${1 << flags[key]},`
-        fstr += '}'
-      }
-      return compiler.wrapCode(`
-        const { value: _value, size } = ${compiler.callType(type, 'offset')}
-        const value = { _value }
-        const flags = ${fstr}
-        for (const key in flags) {
-          value[key] = (_value & flags[key]) == flags[key]
-        }
-        return { value, size }
-      `.trim())
-    }],
     registryEntryHolder: ['parametrizable', (compiler, opts) => {
       return compiler.wrapCode(`
 const { value: n, size: nSize } = ${compiler.callType('varint')}
 if (n !== 0) {
   return { value: { ${opts.baseName}: n - 1 }, size: nSize }
 } else {
-  const holder = ${compiler.callType(opts.otherwise.type)}
-  return { value: { ${opts.otherwise.name}: holder.data }, size: nSize + holder.size }
+  const holder = ${compiler.callType(opts.otherwise.type, 'offset + nSize')}
+  return { value: { ${opts.otherwise.name}: holder.value }, size: nSize + holder.size }
 }
       `.trim())
     }],
@@ -145,46 +103,14 @@ if (n !== 0) {
       code += 'return offset'
       return compiler.wrapCode(code)
     }],
-    arrayWithLengthOffset: ['parametrizable', (compiler, array) => {
-      let code = ''
-      if (array.countType) {
-        code += 'offset = ' + compiler.callType('value.length', array.countType) + '\n'
-      } else if (array.count === null) {
-        throw new Error('Array must contain either count or countType')
-      }
-      code += 'for (let i = 0; i < value.length; i++) {\n'
-      code += '  offset = ' + compiler.callType('value[i]', array.type) + '\n'
-      code += '}\n'
-      code += 'return offset'
-      return compiler.wrapCode(code)
-    }],
-    bitflags: ['parametrizable', (compiler, { type, flags, shift, big }) => {
-      let fstr = JSON.stringify(flags)
-      if (Array.isArray(flags)) {
-        fstr = '{'
-        for (const [k, v] of Object.entries(flags)) fstr += `"${v}": ${big ? (1n << BigInt(k)) : (1 << k)}` + (big ? 'n,' : ',')
-        fstr += '}'
-      } else if (shift) {
-        fstr = '{'
-        for (const key in flags) fstr += `"${key}": ${1 << flags[key]},`
-        fstr += '}'
-      }
-      return compiler.wrapCode(`
-        const flags = ${fstr}
-        let val = value._value ${big ? '|| 0n' : ''}
-        for (const key in flags) {
-          if (value[key]) val |= flags[key]
-        }
-        return (ctx.${type})(val, buffer, offset)
-      `.trim())
-    }],
     registryEntryHolder: ['parametrizable', (compiler, opts) => {
       const baseName = `value.${opts.baseName}`
       const otherwiseName = `value.${opts.otherwise.name}`
       return compiler.wrapCode(`
-if (${baseName}) {
+if (${baseName} != null) {
   offset = ${compiler.callType(`${baseName} + 1`, 'varint')}
 } else if (${otherwiseName}) {
+  offset += 1
   offset = ${compiler.callType(`${otherwiseName}`, opts.otherwise.type)}
 } else {
   throw new Error('registryEntryHolder type requires "${baseName}" or "${otherwiseName}" fields to be set')
@@ -196,7 +122,7 @@ return offset
       const baseName = `value.${opts.base.name}`
       const otherwiseName = `value.${opts.otherwise.name}`
       return compiler.wrapCode(`
-if (${baseName}) {
+if (${baseName} != null) {
   offset = ${compiler.callType(0, 'varint')}
   offset = ${compiler.callType(`${baseName}`, opts.base.type)}
 } else if (${otherwiseName}) {
@@ -234,53 +160,15 @@ return offset
       code += 'return size'
       return compiler.wrapCode(code)
     }],
-    arrayWithLengthOffset: ['parametrizable', (compiler, array) => {
-      let code = ''
-      if (array.countType) {
-        code += 'let size = ' + compiler.callType('value.length', array.countType) + '\n'
-      } else if (array.count) {
-        code += 'let size = 0\n'
-      } else {
-        throw new Error('Array must contain either count or countType')
-      }
-      if (!isNaN(compiler.callType('value[i]', array.type))) {
-        code += 'size += value.length * ' + compiler.callType('value[i]', array.type) + '\n'
-      } else {
-        code += 'for (let i = 0; i < value.length; i++) {\n'
-        code += '  size += ' + compiler.callType('value[i]', array.type) + '\n'
-        code += '}\n'
-      }
-      code += 'return size'
-      return compiler.wrapCode(code)
-    }],
-    bitflags: ['parametrizable', (compiler, { type, flags, shift, big }) => {
-      let fstr = JSON.stringify(flags)
-      if (Array.isArray(flags)) {
-        fstr = '{'
-        for (const [k, v] of Object.entries(flags)) fstr += `"${v}": ${big ? (1n << BigInt(k)) : (1 << k)}` + (big ? 'n,' : ',')
-        fstr += '}'
-      } else if (shift) {
-        fstr = '{'
-        for (const key in flags) fstr += `"${key}": ${1 << flags[key]},`
-        fstr += '}'
-      }
-      return compiler.wrapCode(`
-        const flags = ${fstr}
-        let val = value._value ${big ? '|| 0n' : ''}
-        for (const key in flags) {
-          if (value[key]) val |= flags[key]
-        }
-        return (ctx.${type})(val)
-      `.trim())
-    }],
     registryEntryHolder: ['parametrizable', (compiler, opts) => {
       const baseName = `value.${opts.baseName}`
       const otherwiseName = `value.${opts.otherwise.name}`
       return compiler.wrapCode(`
 let size = 0
-if (${baseName}) {
+if (${baseName} != null) {
   size += ${compiler.callType(`${baseName} + 1`, 'varint')}
 } else if (${otherwiseName}) {
+  size += 1
   size += ${compiler.callType(`${otherwiseName}`, opts.otherwise.type)}
 } else {
   throw new Error('registryEntryHolder type requires "${baseName}" or "${otherwiseName}" fields to be set')
@@ -293,7 +181,7 @@ return size
       const otherwiseName = `value.${opts.otherwise.name}`
       return compiler.wrapCode(`
 let size = 0
-if (${baseName}) {
+if (${baseName} != null) {
   size += ${compiler.callType(0, 'varint')}
   size += ${compiler.callType(`${baseName}`, opts.base.type)}
 } else if (${otherwiseName}) {

src/datatypes/minecraft.js

@@ -11,8 +11,7 @@ module.exports = {
   compressedNbt: [readCompressedNbt, writeCompressedNbt, sizeOfCompressedNbt],
   restBuffer: [readRestBuffer, writeRestBuffer, sizeOfRestBuffer],
   entityMetadataLoop: [readEntityMetadata, writeEntityMetadata, sizeOfEntityMetadata],
-  topBitSetTerminatedArray: [readTopBitSetTerminatedArray, writeTopBitSetTerminatedArray, sizeOfTopBitSetTerminatedArray],
-  arrayWithLengthOffset: [readArrayWithLengthOffset, writeArrayWithLengthOffset, sizeOfArrayWithLengthOffset]
+  topBitSetTerminatedArray: [readTopBitSetTerminatedArray, writeTopBitSetTerminatedArray, sizeOfTopBitSetTerminatedArray]
 }
 const PartialReadError = require('protodef').utils.PartialReadError
 
@@ -181,36 +180,3 @@ function sizeOfTopBitSetTerminatedArray (value, { type }) {
   }
   return size
 }
-
-//
-const { getCount, sendCount, calcCount, tryDoc } = require('protodef/src/utils')
-
-function readArrayWithLengthOffset (buffer, offset, typeArgs, rootNode) {
-  const results = {
-    value: [],
-    size: 0
-  }
-  let value
-  let { count, size } = getCount.call(this, buffer, offset, typeArgs, rootNode)
-  offset += size
-  results.size += size
-  for (let i = 0; i < count + typeArgs.lengthOffset; i++) {
-    ({ size, value } = tryDoc(() => this.read(buffer, offset, typeArgs.type, rootNode), i))
-    results.size += size
-    offset += size
-    results.value.push(value)
-  }
-  return results
-}
-
-// no changes
-function writeArrayWithLengthOffset (value, buffer, offset, typeArgs, rootNode) {
-  offset = sendCount.call(this, value.length, buffer, offset, typeArgs, rootNode)
-  return value.reduce((offset, v, index) => tryDoc(() => this.write(v, buffer, offset, typeArgs.type, rootNode), index), offset)
-}
-
-function sizeOfArrayWithLengthOffset (value, typeArgs, rootNode) {
-  let size = calcCount.call(this, value.length, typeArgs, rootNode)
-  size = value.reduce((size, v, index) => tryDoc(() => size + this.sizeOf(v, typeArgs.type, rootNode), index), size)
-  return size + typeArgs
-}

src/server.js

@@ -34,7 +34,8 @@ class Server extends EventEmitter {
             : JSON.stringify({ text: endReason })
           client.write('kick_disconnect', { reason: fullReason })
         } else if (client.state === states.LOGIN) {
-          client.write('disconnect', { reason: fullReason || endReason })
+          fullReason ||= JSON.stringify({ text: endReason })
+          client.write('disconnect', { reason: fullReason })
         }
         client._end(endReason)
       }