Fix handshake hostname length limit and plugin message drop during join

[SendableMetatype]

• Remove restrictive handshake hostname length limit: The 261-character MAXIMUM_HOSTNAME_LENGTH in HandshakePacket.decode() silently disconnects clients when enough plugins inject forwarding data into the hostname field that the combined length exceeds the limit. readString(buf) now uses the default 65536 limit, matching every other string field and aligning with Paper/Spigot's equivalent fix (readUtf(255) to readUtf(Short.MAX_VALUE) in ClientIntentionPacket).

• Fix plugin messages silently dropped during player join: Fixes Plugin messages silently dropped during player join when getConnectedServer() is null #1766

[WouterGritter]

I think returning -1 in HandshakePacket#decodeExpectedMaxLength isn't desirable. No authentication is required to send this packet, anyone can open TCP connections & spam this packet possibly opening up DOS attacks again that were mitigated by some recent commits.

[SendableMetatype]

I think returning -1 in HandshakePacket#decodeExpectedMaxLength isn't desirable. No authentication is required to send this packet, anyone can open TCP connections & spam this packet possibly opening up DOS attacks again that were mitigated by some recent commits.

Good point. I was assuming the VarInt limit handled it, but 2MB is obviously still an exploitable size. The hostname is now capped at Short.MAX_VALUE characters, which matches Paper's readUtf(Short.MAX_VALUE) in ClientIntentionPacket.

[WouterGritter]

See GemstoneGG#885

Is is true that this PR adds support for SendableMetatype/EduGeyser? Is there any other reason why this PR is needed?

[SendableMetatype]

These fixes have nothing strictly to do with EduGeyser support. They fix general Velocity bugs that any plugin setup can trigger under the right conditions. I just happened to run into them and bother diagnosing them.

[electronicboy]

I mean, this looks like two distinct set of changes

1. The plugin message thing, this would be a bug

2. the hostname length limit, this isn't a bug fix, at least not as we consider it given that we cater to the vanilla protocol, increasing it beyond the vanilla length limit on the incoming side is pretty breaky behaviorally

[THEMPGUYAlt]

See GemstoneGG#885

Is is true that this PR adds support for SendableMetatype/EduGeyser? Is there any other reason why this PR is needed?

Just cloning into velocity ctd before velocity, i fixed the title too to fix the misleading stuff

[WouterGritter]

increasing it beyond the vanilla length limit on the incoming side is pretty breaky behaviorally

I'd consider putting it behind a system property, with vanilla defaults. That could at least give the user the option to have support for things like EduGeyser, just like we have with many other limits.

[SendableMetatype]

I mean, this looks like two distinct set of changes

1. The plugin message thing, this would be a bug
2. the hostname length limit, this isn't a bug fix, at least not as we consider it given that we cater to the vanilla protocol, increasing it beyond the vanilla length limit on the incoming side is pretty breaky behaviorally

Regarding the hostname limit: Paper/Spigot made the equivalent change years ago (readUtf(255) to readUtf(Short.MAX_VALUE) in ClientIntentionPacket) for the same reason - plugins inject forwarding data into the hostname field, which is standard practice in the BungeeCord forwarding model. Velocity's own encode() already writes hostnames with no length cap, so the decode side is just being made consistent. The limit is now Short.MAX_VALUE to match Paper, and decodeExpectedMaxLength is kept in place for DoS protection on the unauthenticated packet.

[electronicboy]

Spigot made that change year ago purely for their forwarding mechanism and nothing else, it breaks compatability with the vanilla protocol in a way which prevents the proxy from being able to connect to vanilla servers entirely. The decode/encode inconsistency here is just a sad reality of how that mechanism is intended to work to keep compat with legacy forwarding.

That data inside of that packet is somewhat trusted due to the vanilla protocol limit on the decode side usually getting in the way of doing anything fancy there, effectively removing that limit stands out as a potential security risk at the very least; it's definetly not something I feel should just be changed overnight

[SendableMetatype]

The incoming decode limit doesn't affect vanilla backend compatibility. Velocity controls what it sends to backends based on forwarding mode, and the encode side already has no length cap, so anything that would break from a longer incoming hostname is already breakable through Velocity's own outgoing path. The decode/encode inconsistency doesn't protect anything if the encode side is already uncapped.

The security concern is fair, which is why the limit is Short.MAX_VALUE with decodeExpectedMaxLength kept in place, not removed entirely.

[WouterGritter]

In my opinion it does not need to matter if upping the limit breaks vanilla backend compatibility if we put this limit behind a system property.

It's already possible to completely break vanilla (or any) connections with system properties by lowering packet size limits. Adding another system property that's a NOP by default, but allows a server admin to break vanilla connections (and fix support for EduGeyser) by increasing this limit isn't something new in that regard.

[SendableMetatype]

A system property that defaults to the broken behavior means every affected server admin has to independently diagnose a silent disconnect with no error output and somehow discover the flag. That's the same problem as the current bug, just with an extra step. The default should handle legitimate use cases without manual intervention. And to be clear, this has nothing to do with EduGeyser support. EduGeyser typically works fine without this change. These are general Velocity bugs that any plugin setup can hit under the right conditions.