From 371fb9daa785b052d7a4df646420eb858f4bea9a Mon Sep 17 00:00:00 2001 From: Timofey Gelazoniya Date: Wed, 25 Jun 2025 01:46:53 +0300 Subject: [PATCH] feat: skip SRV lookup for IP addresses and localhost Performing DNS SRV lookups for hosts that are already direct IP addresses or special-case hostnames like 'localhost' is unnecessary. In addition, several related test suite improvements have been included: - A bug in the Bedrock mock packet creation was fixed where the magic GUID was being written to an incorrect buffer offset. - The DNS mocking strategy in the Java tests has been refactored for better accuracy by mocking the `dns.promises.Resolver` class. - An error test for the Bedrock pinger was corrected to properly handle a promise rejection instead of a synchronous throw. --- lib/java.js | 75 ++++++++++++++++++++++++++++---------------- test/bedrock.test.js | 8 +++-- test/java.test.js | 21 ++++++++++--- 3 files changed, 69 insertions(+), 35 deletions(-) diff --git a/lib/java.js b/lib/java.js index e2c4602..ecf6c20 100644 --- a/lib/java.js +++ b/lib/java.js @@ -5,7 +5,7 @@ "use strict"; -import net from "node:net"; +import { createConnection, isIP } from "node:net"; import { Resolver } from "node:dns/promises"; import createDebug from "debug"; import * as varint from "./varint.js"; @@ -147,39 +147,60 @@ export async function pingJava(host, options = {}) { let targetHost = host; let targetPort = fallbackPort; - try { + // A list of hostnames that should never have an SRV lookup. + const nonSrvLookableHostnames = ["localhost"]; + + // Check if the host is a valid IP address (v4 or v6). + // net.isIP() returns 0 for invalid IPs, 4 for IPv4, and 6 for IPv6. + const isDirectIp = isIP(host) !== 0; + const isNonLookableHostname = nonSrvLookableHostnames.includes( + host.toLowerCase() + ); + + if (isDirectIp || isNonLookableHostname) { debug( - "attempting SRV lookup for _minecraft._tcp.%s with %dms timeout", - host, - timeout + "host '%s' is a direct IP or a non-lookable hostname, skipping SRV lookup.", + host ); - const resolver = new Resolver({ timeout, tries: 3 }); - const srvRecords = await resolver.resolveSrv(`_minecraft._tcp.${host}`); - if (srvRecords.length > 0) { - targetHost = srvRecords[0].name; - targetPort = srvRecords[0].port; - debug("SRV lookup successful, new target: %s:%d", targetHost, targetPort); - } - } catch (err) { - // Common errors like ENODATA, ENOTFOUND, or a DNS timeout (ETIMEOUT) are expected - // when a server does not have an SRV record, so we ignore them and proceed. - const nonFatalDnsCodes = ["ENODATA", "ENOTFOUND", "ETIMEOUT"]; - if ( - err instanceof Error && - "code" in err && - nonFatalDnsCodes.includes(err.code) - ) { - debug("SRV lookup for %s failed (%s), using fallback.", host, err.code); - } else { - // Re-throw anything else to fail the operation - debug("SRV lookup for %s failed unexpectedly, re-throwing.", host, err); - throw err; + } else { + try { + debug( + "attempting SRV lookup for _minecraft._tcp.%s with %dms timeout", + host, + timeout + ); + const resolver = new Resolver({ timeout, tries: 3 }); + const srvRecords = await resolver.resolveSrv(`_minecraft._tcp.${host}`); + if (srvRecords.length > 0) { + targetHost = srvRecords[0].name; + targetPort = srvRecords[0].port; + debug( + "SRV lookup successful, new target: %s:%d", + targetHost, + targetPort + ); + } + } catch (err) { + // Common errors like ENODATA, ENOTFOUND, or a DNS timeout (ETIMEOUT) are expected + // when a server does not have an SRV record, so we ignore them and proceed. + const nonFatalDnsCodes = ["ENODATA", "ENOTFOUND", "ETIMEOUT"]; + if ( + err instanceof Error && + "code" in err && + nonFatalDnsCodes.includes(err.code) + ) { + debug("SRV lookup for %s failed (%s), using fallback.", host, err.code); + } else { + // Re-throw anything else to fail the operation + debug("SRV lookup for %s failed unexpectedly, re-throwing.", host, err); + throw err; + } } } return new Promise((resolve, reject) => { debug("creating TCP connection to %s:%d", targetHost, targetPort); - const socket = net.createConnection({ host: targetHost, port: targetPort }); + const socket = createConnection({ host: targetHost, port: targetPort }); // Prevent cleanup tasks from running more than once // in case of multiple error callbacks diff --git a/test/bedrock.test.js b/test/bedrock.test.js index 8365e48..0bcf33e 100644 --- a/test/bedrock.test.js +++ b/test/bedrock.test.js @@ -125,8 +125,10 @@ describe("bedrock.js", () => { }); describe("errors", () => { - it("should throw an error if host is not provided", () => { - expect(() => pingBedrock(null)).toThrow("Host argument is required"); + it("should throw an error if host is not provided", async () => { + await expect(pingBedrock(null)).rejects.toThrow( + "Host argument is required" + ); }); it("should reject on socket timeout", async () => { @@ -167,7 +169,7 @@ function createMockPongPacket(motd) { const packet = Buffer.alloc(35 + motdBuffer.length); packet.writeUInt8(0x1c, 0); packet.writeBigInt64LE(BigInt(Date.now()), 1); - Buffer.from("00ffff00fefefefefdfdfdfd12345678", "hex").copy(packet, 17); + Buffer.from("00ffff00fefefefefdfdfdfd12345678", "hex").copy(packet, 9); packet.writeUInt16BE(motdBuffer.length, 33); motdBuffer.copy(packet, 35); return packet; diff --git a/test/java.test.js b/test/java.test.js index 6b46b1c..a1eca70 100644 --- a/test/java.test.js +++ b/test/java.test.js @@ -1,18 +1,25 @@ import net from "node:net"; -import dns from "node:dns/promises"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { pingJava } from "../lib/java.js"; import * as varint from "../lib/varint.js"; +const mockResolveSrv = vi.fn(); + vi.mock("node:net"); -vi.mock("node:dns/promises"); +vi.mock("node:dns/promises", () => ({ + Resolver: vi.fn().mockImplementation(() => ({ + resolveSrv: mockResolveSrv, + })), +})); describe("pingJava", () => { let mockSocket; beforeEach(() => { - // Simulate no SRV record found. - dns.resolveSrv.mockResolvedValue([]); + // Reset mocks before each test. + mockResolveSrv.mockClear(); + // Simulate no SRV record found by default. + mockResolveSrv.mockResolvedValue([]); const mockHandlers = {}; mockSocket = { @@ -44,7 +51,11 @@ describe("pingJava", () => { // Allow the async SRV lookup to complete await vi.runAllTicks(); - expect(dns.resolveSrv).toHaveBeenCalledWith(`_minecraft._tcp.${host}`); + expect(net.createConnection).toHaveBeenCalledWith({ + host: host, + port: options.port, + }); + expect(net.createConnection).toHaveBeenCalledWith({ host, port: options.port,