ayan4m1 / minestat-es

Minecraft server status library for Node.js
MIT License
8 stars 0 forks source link

Not parsing all server query responses correctly #3

Closed StelStelox closed 2 weeks ago

StelStelox commented 3 weeks ago

One of the projects uses your library. But some servers in minecraft, which proxy the server through cloudflare, do not see the server status https://github.com/AuroraTeam/Launcher/blob/44564e9c7864a996376dfb2c8d55b2a866158e09/src/main/scenes/ServersList.ts#L35C28-L35C62 image image

StelStelox commented 3 weeks ago

image

ayan4m1 commented 3 weeks ago

Sorry, I'm having a very hard time understanding your issue. The first screenshot of JSON - where is that coming from? What is the context for this JSON? Also, what is the context of the second screenshot? It doesn't really tell me anything, I need to see the actual result from the fetchServerInfo promise.

To be clear, what I need are specifics for this scenario:

Also, why is there a warning exclamation triangle next to the SRV record in your DNS screenshot? What does it tell you if you hover over / click on that?

StelStelox commented 2 weeks ago

The launcher uses JSON to specify the path to the client libraries, as well as to specify the address of the game server, more details here https://docs.aurora-launcher.ru/basic/clients.html#%D0%BD%D0%B0%D1%81%D1%82%D1%80%D0%BE%D0%B8%D0%BA%D0%B0-%D0%BF%D1%80%D0%BE%D1%84%D0%B8%D0%BB%D1%8F . So the client part of the launcher uses your library in version 1.1.7, the problem is that the developer of the launcher himself cannot give a specific answer why, when proxy through the server, Cloudflare does not want to display the status not through ip:port and through A and SRV records (I checked on server monitoring, they see the server status). About the exclamation mark in DNS records Cloudfalre complains that SRV reveals the IP of the VDS server. About fetchServerInfo it is no longer used anywhere and pings servers independently

JoCat commented 2 weeks ago

It looks like the problem may be on the server side or a flaw in the script. image

By the way, another issue I wanted to mention but didn't have time to, is the requirement to specify the “_minecraft._tcp” subdomain to allow SRV writes. This is not mentioned in the Readme, and it is not handled in any way in the code. You need to pick one of the options and fix it.

ayan4m1 commented 2 weeks ago

@JoCat Yes, that could certainly be an issue with the server. It could be caused by CloudFlare too, but I don't plan on supporting CloudFlare explicitly if they do some weird munging of the packets for Minecraft traffic.

The error you screenshotted is thrown when we receive less than 6 bytes back in response to the 0xfe, 0x01 preamble. That indicates that we got some invalid/incomprehensible reply from the server. Look into this code for more debugging help - log out that info variable to see what the server is actually returning.

By the way, another issue I wanted to mention but didn't have time to, is the requirement to specify the “_minecraft._tcp” subdomain to allow SRV writes. This is not mentioned in the Readme, and it is not handled in any way in the code. You need to pick one of the options and fix it.

Not sure what this is saying. Are you asking for the library to prepend _minecraft._tcp to the hostname parameter for SRV lookups? The tone of this message comes across as very disrespectful and arrogant to me, can you please clarify what you are trying to say here?

JoCat commented 2 weeks ago

Are you asking for the library to prepend _minecraft._tcp to the hostname parameter for SRV lookups?

Yeah, that's an option.

The tone of this message comes across as very disrespectful and arrogant to me, can you please clarify what you are trying to say here?

Disrespect or arrogance is out of the question. I just wanted to mention that SRV records have a certain format, but your code or documentation does not take this into account in any way.

ayan4m1 commented 2 weeks ago

@JoCat Sorry, I had assumed it would be obvious that it would pass the string directly to the SRV resolver, and that the user would be prefixing _minecraft._tcp. in a real world scenario. I'll update the documentation and prepend the string if it's not already the start of the hostname string and release a v1.2.0 in the near future.

As to the actual issue reported here, please let me know when you've inspected the info variable and can see the bytes that are coming back from the server.

JoCat commented 2 weeks ago

The first log is the output of the raw variable Second - variable info

image

JoCat commented 2 weeks ago

I think I've realized what the error is, your delimiters are counted by 3 zeros, although the correct way is 2 (I took the information from wiki.vg) and if you make this change - everything will work. image

JoCat commented 2 weeks ago

Implemented a more correct algorithm for reading a package based on information from wiki.vg

if (raw[0] !== 0xff) {
  return resolveOffline(
    new Error('Got invalid packet from Minecraft server!')
  );
}

// get usefull data from buffer and swap bytes (convert big endian to little endian for reading)
const infoBuffer = Buffer.alloc(raw.readInt16BE(1) * 2, 0, 'binary');
// Copy data into buffer
raw.copy(infoBuffer, 0, 3);
// swap bytes (convert big endian to little endian for reading)
infoBuffer.swap16();

// read buffer to string
const info = infoBuffer.toString('utf16le').split('\x00');

if (info.length < 6) {
  return resolveOffline(
    new Error('Got invalid reply from Minecraft server!')
  );
}

// attempt to parse data from server
const version = info[2];
const motd = info[3];
const players = parseInt(info[4], 10);
const maxPlayers = parseInt(info[5], 10);

Example of work:

image

JoCat commented 2 weeks ago

I made a fork to test my concept. Everything seems to work, the only thing is that due to a small change in the parsing logic, the tests that catch the parsing error will need to be updated. If there are any changes or requirements from your side, please let me know. I will try to make them as soon as possible. Or you can use the sample code I posted above and refine it yourself.

ayan4m1 commented 2 weeks ago

I'm working with the code you've provided, but my "valid parse" test case is broken at the moment and I don't have any more time this weekend for personal projects. I'll try to take a look this week and get something merged in.

The byte you had to change in the tests, that was captured from a real server so it should be correct.

ayan4m1 commented 2 weeks ago

@JoCat Please update to 2.1.0 and let me know if you continue to have issues. Thanks for your contribution.

JoCat commented 2 weeks ago

The byte you had to change in the tests, that was captured from a real server so it should be correct

strange, it seems that the packet is either longer (several bytes are missing, it seems to be in MOTD), or there is a wrong packet length specified in principle