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.2k stars 241 forks source link

Add support for non-full packets #1278

Closed Pandapip1 closed 6 months ago

Pandapip1 commented 6 months ago

CC #811, #1094

Currently, this project seems extraordinarily dead, in that it isn't being updated.

Admittedly, this PR is a band-aid and not a fix. But I'd rather have the option for hacky non-0% support than forced 0% support, when it comes to any recent version of the game.

extremeheat commented 6 months ago

What does this actually do?

Pandapip1 commented 6 months ago

What does this actually do?

It adds a client / server option to have createDeserializer return a Parser object instead of a FullPacketParser:

https://github.com/PrismarineJS/node-minecraft-protocol/pull/1278/files#diff-7e2074fe137467401d88e0861ae91fede6ef856efbdea692c4656c68ae243d8fR49-R52

extremeheat commented 6 months ago

What does that do?

Pandapip1 commented 6 months ago

What does that do?

FullPacketParser performs packet validity checks. New minecraft versions extend the existing protocol in ways that break those validitity checks, but where existing code can read 90% of the useful information. node-minecraft-protocol does need to be updated to read that remaining 10% that gets cut off, but that doesn't have to break compatibility if those features aren't yet needed.

extremeheat commented 6 months ago

extend the existing protocol in ways that break those validitity checks

Where in the code are you seeing this and for what version? Do you have a link to code somewhere?

Pandapip1 commented 6 months ago

This conversation is already well-documented at #811, except oddly I can't find MC version numbers. IIRC, >1.19.0

https://github.com/ProtoDef-io/node-protodef/blob/e008e688d52478d82926888a59d85d79704ff462/src/serializer.js#L76

extremeheat commented 6 months ago

There is no such thing AFAIK of a partial packet. #811 is unrelated, that's caused by error in minecraft-data protocol, or invalid server data.

Pandapip1 commented 6 months ago

There is no such thing AFAIK of a partial packet. https://github.com/PrismarineJS/node-minecraft-protocol/issues/811 is unrelated, that's caused by error in minecraft-data protocol, or invalid server data.

Well then, this allows clients to handle invalid server data, and vice versa.

I'll fully admit: I'm not particularly experienced with regards to the MC protocol or protobuf in general. But what I do know is that there was a server that vanilla MC clients were able to connect to and that node-minecraft-protocol (and by extension, mineflayer) clients were not able to connect to, and it was because of that particular check. When decoding the extra data, it didn't look like invalid data. In fact, it looked like extra data that belonged in the packet.

At the very least, this config option allows us users to connect to such servers and submit more upstream PRs to node-minecraft-protocol to fix issues associated with these unusual configurations. The point of node-minecraft-protocol is to have a programmable client / server that can connect to the same servers / clients that vanilla MC can, right?

extremeheat commented 6 months ago

I think you misunderstood my last comment. This PR implies there is a thing as a partial packet, like one that has variable length. Or something to do with packet batching. As far as I am aware, this does not exist. Actually I think this seems like a dupe PR of https://github.com/PrismarineJS/node-minecraft-protocol/pull/1094, what's changed here ?

As for #811, you are looking at two different errors. The deserializer (in node-minecraft-protocol) is failing to read something (not fatal) and the serializer is incorrectly encoding something that is being sent to a vanilla client, causing it to crash (which is fatal). These are two independent errors, and like I mention, caused by legitimate error in data from outside or a wrong definition in minecraft-data

Pandapip1 commented 6 months ago

I think you misunderstood my last comment. This PR implies there is a thing as a partial packet, like one that has variable length. Or something to do with packet batching. As far as I am aware, this does not exist. Actually I think this seems like a dupe PR of https://github.com/PrismarineJS/node-minecraft-protocol/pull/1094, what's changed here ?

1094 and this PR share a git branch, which is why their files are synced. #1094 was closed when https://github.com/PrismarineJS/node-minecraft-protocol/pull/1278/commits/7da26b49285179e7277ca7911706da3e40d05b84 was the latest commit. It fully disabled the FullPacketParser checks all the time.

This PR adds an option to disable them instead of having it disabled 100% of the time.

I'll look at #811 again. It's been half a year since I last touched this stuff.

Pandapip1 commented 6 months ago

Never mind. I can't find the code that reproduces the error. Closing unless I can repro again.