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.22k stars 239 forks source link

Java client throwing DecoderException on client.end #1087

Open CyanCode opened 1 year ago

CyanCode commented 1 year ago

[✅ ] The FAQ doesn't contain a resolution to my issue

Versions

minecraft-protocol: 1.41.0 server: vanilla node: 18.4.0

Detailed description of a problem

When calling client.end(...) on a connecting 1.19.2 client the connection is lost with the following message: Internal Exception: io.netty.handler.codec.DecoderException: aa: Non [a-z0-9_.-] character in namespace of location: {"text":"no server with subdomain localhost found"}

javaw_9kPvO8A7om

"no server with subdomain localhost found" is the message I intended to respond to the connecting client.

Current code

     const address = await getLocalAddress();
      const mcServer = mc.createServer({
        'online-mode': false,
        host: address,
        port: MC_SERVER_PORT,
        keepAlive: true,
      });

      mcServer.on('error', (e) => {
        this.logger.error('Error occurred while listening for logins', e);
      });
      mcServer.on(
        'login',
        async (client: ServerClient) => {
            // lines removed for brevity
            client.end("no server with subdomain localhost found");
        }
      );
      mcServer.on('listening', () => {
        this.logger.info(
          `Listening for login attempts on ${address}:${MC_SERVER_PORT}`,
        );
        resolve();
      }); 

Expected behavior

String passed into client.end(...) is what appears to the connecting Java client instead of the exception

Additional context

frej4189 commented 1 year ago

Send the client a disconnect packet and then close the connection after instead

CyanCode commented 1 year ago

Send the client a disconnect packet and then close the connection after instead

How do I send a disconnect packet (assuming this is a suggested change on my end and not one to the library). Will this apply to all previous versions as well or just 1.19+? Previously the string sent to client.end would simply appear on the MC client without any additional changes

frej4189 commented 1 year ago

client.write('kick_disconnect', { reason: 'Reason for kicking the client' })

CyanCode commented 1 year ago

Thanks I'll give that a try. Will this apply to all previous versions as well? i.e. I don't have to have separate paths for older versions using client.end and 1.19+ using client.write('kick_disconnect', ...), all versions will accept the disconnect packet.

frej4189 commented 1 year ago

The disconnect packet is present on all versions supported by nmp to my knowledge

CyanCode commented 1 year ago

Great, thanks. Do we want to keep this issue open for the purpose of keeping client.end's implementation consistent across versions (e.g. used for relaying the passed reason to the client)? If not then the docs should probably be updated to include client.write('kick_disconnect', { reason: 'Reason for kicking the client' }) as an example method of disconnecting with a reason.

frej4189 commented 1 year ago

In my mind this is not something NMP should handle. Docs say otherwise, though. Gonna need an opinion from @rom1504

extremeheat commented 1 year ago

Instead of updating client.end, a client.disconnect function can be added instead if it doesn't already exist. This is handled internally already in cases where the client needs to be disconnected (eg invalid auth), so should just be a matter of exposing it. Bedrock-protocol has a similar API function to do this.

CyanCode commented 1 year ago

After sending the kick_disconnect packet the Java client is still running into the same exception (albeit with a slightly different message)

Function I'm calling

async function disconnect(reason: string) {
    await client.write('kick_disconnect', { reason });
}

image

extremeheat commented 1 year ago

There are 2 different disconnect packets depending on the state of the client. You might be sending the wrong one, try disconnect, and make sure to run close afterwards.

That said, the error here seems completely unrelated. It's triggered by the client handling. You are likely sending invalid data to the client before calling .end. Don't send anything else (not even login) and check that disconnect works. If it works, you need to fix the packets you're sending previously.

CyanCode commented 1 year ago

When you say disconnect and close afterwards are you suggesting something like this?

async function disconnect(reason: string) {
    await client.write('disconnect', { reason });
    await client.end();
}

Because doing so leads to a different error message image

I also tried calling .end() after sending the kick_disconnect packet with the same error as before:

async function disconnect(reason: string) {
    await client.write('kick_disconnect', { reason });
    await client.end();
}

I did discover something though-- looking at the Client object in the debugger I'm seeing the version read 1.19.3, but the MC client I'm using is 1.19.2, is this expected behavior? If not I can try reinstalling Minecraft and see if something odd is happening with my local installation.

image

You are likely sending invalid data to the client before calling .end. Don't send anything else (not even login) and check that disconnect works. If it works, you need to fix the packets you're sending previously.

Also, I'm assuming login packets are being sent before the login event is called? i.e. connect event is called, NMP performs the login, and login event is called. Unless waiting for the login event is sending invalid data, I'm not sending any custom data (I'm really only using NMP to grab the connecting MC client's server host). Unfortunately I specifically need the login event to have occurred because I use the serverHost property which isn't available in the connect event's client object.

extremeheat commented 1 year ago

That's not what I mean. This is the server example, and notice it sends a login packet. https://github.com/PrismarineJS/node-minecraft-protocol/blob/3974c503e4e08b271f028a87a91b1d9013990a35/examples/server/server.js#L25

I mean remove that and all the code below, and disconnect the client to make sure it works. Also, 1.19.3 and 1.19.2 are not compatible. Some things are broken in the current release, so you can downgrade node-minecraft-protocol to see if it fixes anything, npm install minecraft-protocol@1.40

CyanCode commented 1 year ago

I see-- while I am referencing the server example, I'm not sending any packets at all, basically all I'm doing is:

  1. Intercepting the login event
  2. Grabbing the host from client.serverHost and doing something unrelated to NMP with it
  3. Disconnecting with client.end(...) (up until this point where I've been trying client.write('kick_disconnect', { reason });)

The "Current code" section in the issue contains the only NMP code I'm running, the only interaction I'm having with the client is calling .end as soon as they login.

I think you're probably right that its just some things being broken in the current release. This very well may be fixed once those pending PRs are merged in.

CyanCode commented 1 year ago

Seems like this is still an issue in the latest release-- is anyone else experiencing this when logging in with v1.19.2+ ?

ImAhmadAbdullah commented 8 months ago

I'm also getting a decoder exception, but a different error at the end: https://github.com/PrismarineJS/node-minecraft-protocol/issues/1279