search

PrismarineJS / node-minecraft-protocol

Parse and serialize minecraft packets, plus authentication and encryption.
https://prismarinejs.github.io/node-minecraft-protocol/
BSD 3-Clause "New" or "Revised" License
1.23k stars 239 forks source link

Handling unknown packets, and/or registering custom packets #367

Closed deathcap closed 8 years ago

deathcap commented 8 years ago

When connecting to Forge servers, some mods send their own custom packets (instead of using the custom_payload plugin channel packet meant for this, for some reason), causing node-minecraft-protocol to throw an error since it is unrecognized:

Error: Read error for name : -26 is not in the mappings value
    at ProtoDef.readMapper (/Users/admin/games/voxeljs/ProtoDef/dist/datatypes/utils.js:38:41)
    at ProtoDef.read (/Users/admin/games/voxeljs/ProtoDef/dist/protodef.js:103:31)
    at /Users/admin/games/voxeljs/ProtoDef/dist/datatypes/structures.js:114:32
    at tryCatch (/Users/admin/games/voxeljs/ProtoDef/dist/utils.js:31:12)
    at tryDoc (/Users/admin/games/voxeljs/ProtoDef/dist/utils.js:38:10)
    at /Users/admin/games/voxeljs/ProtoDef/dist/datatypes/structures.js:113:5
    at Array.forEach (native)
    at ProtoDef.readContainer (/Users/admin/games/voxeljs/ProtoDef/dist/datatypes/structures.js:108:12)
    at ProtoDef.read (/Users/admin/games/voxeljs/ProtoDef/dist/protodef.js:46:25)
    at ProtoDef.read (/Users/admin/games/voxeljs/ProtoDef/dist/protodef.js:103:31)

any ideas how to best handle this. The list of custom packets cannot be known beforehand (at least for a simple bot, more sophisticated clients could in theory know what packets to accept from modded servers), and although there is an option in node-minecraft-protocol to specify what packets to parse, it should still be possible to pass these unknown packets through proxies and to clients/servers as opaque blobs since in 1.7+ all the packets are length-prefixed.

Maybe an unknown_packet event? Or unknownpacket+(id)?

rom1504 commented 8 years ago

Hmm, I guess we could customize the error yeah. But it would still be an error I think and then people could listen on the error and parse the packet themselves or something ?

(by customize I mean something like https://github.com/roblabla/ProtoDef/blob/master/src/utils.js#L39)

(the problem of not considering this to be an error is that most often it means we have some error in protocol.json and it is an actual error)

rom1504 commented 8 years ago

if you listen to the error event, it doesn't manage to ignore the packet ?

deathcap commented 8 years ago

If I listen on error then I do get the deserialization error, but the program quits after it receives it (testing with client_forge from nmp-f). Would be better not to have it error =)

Is it possible for plugins to register custom packets? Trying to hook into https://github.com/PrismarineJS/node-minecraft-protocol/blob/master/src/transforms/serializer.js but it looks like the packet data is passed to the deserializer and then the plugin code runs too late to add its own custom packets. But if it is possible, I'm thinking it would be sufficient to just register packet id -26 (aka packet id 230), I found out what it is used for and its structure.

deathcap commented 8 years ago

Packet id -26 is the only custom packet I've found yet (most mods use plugin channels instead), parsing it specifically in https://github.com/PrismarineJS/minecraft-data/pull/121, works well so handling unknown packets is less of a priority now for me at least. Though it may still be interesting to allow plugins to register custom packets, augmenting or modifying the minecraft-data protocol before it is passed to the serializer, then new packets could be added to https://github.com/PrismarineJS/node-minecraft-protocol-forge (or elsewhere) as they are discovered if needed

rom1504 commented 8 years ago

https://github.com/roblabla/ProtoDef/commit/913b590991306e3a35d2c7d45cc5486fcd7dacb1 allows not to crash on parsing error.

Now (independently of that fix), I'm going to try to add a functionnality to register a packet here.

rom1504 commented 8 years ago

Hmm https://github.com/roblabla/ProtoDef/commit/913b590991306e3a35d2c7d45cc5486fcd7dacb1 improve a bit the issue but it seems https://github.com/roblabla/ProtoDef/blob/master/src/serializer.js#L53 eventually make that stream close anyway.

I'm not sure how to best handle this. Would emitting an error event not make the stream crash ?

(if the stream doesn't close then it will be possible to handle that error in nmp to emit a unknown_packet)

rom1504 commented 8 years ago

If the target stream emits an error, the source stream will disconnect from it (ie, .unpipe() itself from the target stream).

rom1504 commented 8 years ago

https://github.com/nodejs/node/issues/3045#issuecomment-142955116

rom1504 commented 8 years ago

So now you can listen on "error" and get the buffer on err.buffer.

Is this enough or should we try to figure out automatically whether it's an "unknown packet" error and emit a better error ?

deathcap commented 8 years ago

I think this is probably good enough, now that I handle packet -26 I expect not to see this error much more.

rom1504 commented 8 years ago

It's possible to pass the customPackets option now to createClient and createServer. I think this is enough. If you think it's not, feel free to reopen.