Skip to content

Hostname/Plugin Messages Length Limit Increase/Removal.#885

Open
THEMPGUYAlt wants to merge 3 commits into
GemstoneGG:libdeflatefrom
THEMPGUYAlt:libdeflate
Open

Hostname/Plugin Messages Length Limit Increase/Removal.#885
THEMPGUYAlt wants to merge 3 commits into
GemstoneGG:libdeflatefrom
THEMPGUYAlt:libdeflate

Conversation

@THEMPGUYAlt
Copy link
Copy Markdown

@THEMPGUYAlt THEMPGUYAlt commented May 11, 2026

Clone of https://github.com/SendableMetatype/Velocity

Support for https://github.com/SendableMetatype/EduGeyser
-- NOTE: I AM NEW TO GITHUB PRs in general, so don't expect me to be perfect at this.
This PR fixes handshake hostname length limit (especially for EduGeyser and my server in general, as the domain can be very long, so now the EDU connection is dropped. Some plugins can make forwarding data very long and cause it to be dropped.
This PR also fixes the long plugin messages bug that causes the data and the connection in general to be silently dropped for being too long.

Copy link
Copy Markdown
Member

@R00tB33rMan R00tB33rMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was LegacyForgeConstants stripped altogether? I'd assume a hybrid approach can be implemented to ensure that the edge case of the FML client handshake (state COMPLETE) is still retained, unless I'm misinterpreting something?

@THEMPGUYAlt THEMPGUYAlt changed the title EduGeyser Support EduGeyser Support & Plugin Messaging Fixes May 11, 2026
@THEMPGUYAlt
Copy link
Copy Markdown
Author

Why was LegacyForgeConstants stripped altogether? I'd assume a hybrid approach can be implemented to ensure that the edge case of the FML client handshake (state COMPLETE) is still retained, unless I'm misinterpreting something?

I will see what i can do about that

@SendableMetatype
Copy link
Copy Markdown

For reference, my original PR is at PaperMC/Velocity#1783. These are general Velocity bugfixes, not EduGeyser support. Once upstream merges it, this fork would pull it in on the next upstream sync.

@SendableMetatype
Copy link
Copy Markdown

SendableMetatype commented May 11, 2026

Why was LegacyForgeConstants stripped altogether? I'd assume a hybrid approach can be implemented to ensure that the edge case of the FML client handshake (state COMPLETE) is still retained, unless I'm misinterpreting something?

The Forge handshake edge case isn't lost. The old code only allowed getConnectionInFlight() as a fallback for the Forge channel specifically. The new code allows it for all channels. Forge packets still take the same path, they just aren't the only ones allowed through during the null window anymore.

@THEMPGUYAlt THEMPGUYAlt changed the title EduGeyser Support & Plugin Messaging Fixes Hostname/Plugin Messages Length Limit Increase. May 11, 2026
@THEMPGUYAlt THEMPGUYAlt changed the title Hostname/Plugin Messages Length Limit Increase. Hostname/Plugin Messages Length Limit Increase/Removal. May 11, 2026
@THEMPGUYAlt
Copy link
Copy Markdown
Author

  • Fixed the title of the PR to prevent misleading people.

@THEMPGUYAlt
Copy link
Copy Markdown
Author

This is also a PR here until Velocity adds these changes themselves, assuming it might get released here first to fix these bugs faster, as this is basically an "enhanced fork".

@WouterGritter
Copy link
Copy Markdown
Collaborator

WouterGritter commented May 11, 2026

Sounds good, though there are a couple valid concerns being raised in the upstream PR. Modifying the handle(PluginMessagePacket packet) method seems to remove a check that would introduce a bug that it's trying to prevent, which isn't good. And as per my comment we might want to consider sticking to vanilla defaults, allowing to increase the limit through a system property.

But let's try to keep the discussion in one place from now (upstream's PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants