fix(proxy): re-assert Connect's login decision after other plugins - #77
Merged
robinbraemer merged 5 commits intoJul 28, 2026
Merged
Conversation
Connect authenticates tunneled players at its edge and forces offline mode at pre-login. A login plugin that unconditionally forces online mode later in the same event reverts that, and the player then waits forever for an encryption request nothing can answer - the LibreLogin + Connect login hang, and the same for AuthMe, nLogin, or anything else with that behaviour. Connect now registers a second pre-login handler that runs after every other plugin's and restores its own decision. It is a floor, not a veto: it only acts on connections Connect itself authenticated, never overrides another plugin's kick, and does nothing when the decision is still what Connect set. It keys purely off Connect's own result on the event object, so it never reads, links against, or version-checks any third-party plugin - which is what makes it plugin-agnostic and impossible to drift out from under. Ordering, both levers layered: - Velocity registers at short priority Short.MIN_VALUE, one slot below the Short.MIN_VALUE + 1 that PostOrder.LAST maps to, so it wins regardless of load order. That overload only exists on Velocity builds from 2024-09-16 on, so it is feature-detected with one reflective lookup on the public interface; older builds fall back to PostOrder.LAST plus the optional librelogin dependency now declared in velocity-plugin.json, which Velocity turns into load order. velocity-api is deliberately not bumped: reading a PostOrder constant an older runtime lacks throws while Velocity collects the listener's methods and would kill all of Connect's handlers. - BungeeCord uses @eventhandler(priority = Byte.MAX_VALUE); equal priorities there are identity-keyed, so registration order is no lever at all. Two operator controls in proxy config.yml: - login-reassert.enabled (default true) turns the whole thing off, for operators who deliberately want another plugin to override Connect. - login-reassert.restore-full-profile (default false) additionally restores the Mojang UUID and username. Off by default because login plugins key their storage on the proxy UUID; the LibreLogin-side new-uuid-creator: MOJANG prerequisite is documented next to the option. The default restores the skin properties only, which fixes the hang and the missing skin while keeping such plugins internally consistent. VelocityGameProfiles.fromConnectPlayer is now idempotent so the late re-assert cannot append a second copy of Connect's own properties. Guarded by a new :velocity:eventOrderTest source set that runs the real VelocityEventManager and PluginDependencyUtils against a Velocity proxy jar pinned by sha256, plus unit coverage for every fail-safe path.
![no-mistakes[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix the LibreLogin+Connect login hang - and the whole class of 'a login plugin forces online mode after Connect' (AuthMe, nLogin, etc.) - entirely from Connect's side, with no dependency on the unmerged upstream LibreLogin PR.
Background: Connect authenticates tunneled players at the Minekube edge and forces offline mode at pre-login. A login plugin that unconditionally forces online mode later in the same event reverts that decision, and the proxy then sends an EncryptionRequest nothing can answer - the player hangs at 'Logging in...' forever. The current official workaround is 'disable premium autologin', which also costs the skin and the UUID and forces every premium user to type /login.
The design was fully worked out and empirically proven by a prior scout report that executed the REAL Velocity dispatcher across proxy versions 3.2.0/3.4.0/3.5.1/4.0.0 and the real BungeeCord event bus. I followed it. Deliberate decisions a reviewer reading only the diff would not know:
The safety property is STRUCTURAL, not maintained. Connect adds a SECOND, late pre-login handler that re-asserts ITS OWN decision. It reacts only to Connect's own result having been changed on the event object. It deliberately never reads, imports, links against, reflects into, or version-checks LibreLogin or any other plugin - that is what makes it plugin-agnostic and impossible for a third-party internal to drift out from under. Do not 'improve' it by adding a plugin-specific check.
Two ordering levers are layered ON PURPOSE, neither is redundant. Velocity registers at short priority Short.MIN_VALUE (one slot below the Short.MIN_VALUE+1 that PostOrder.LAST maps to), feature-detected with ONE reflective lookup on the PUBLIC EventManager interface method, because that overload only exists on Velocity builds from 2024-09-16 onward. Older builds fall back to PostOrder.LAST plus a new optional 'librelogin' dependency in velocity-plugin.json, which Velocity turns into load order via a topological sort; an optional dep on an absent plugin is a proven silent no-op. BungeeCord uses @eventhandler(priority = Byte.MAX_VALUE) - EventPriority is byte constants not an enum, the bus dispatches the whole byte range, and Bungee's same-priority tie-break is identity-keyed HashMap order, so no softDepends could ever be a lever there.
velocity-api is deliberately NOT bumped past 3.2.0-SNAPSHOT, even though PostOrder.CUSTOM would be the 'obvious' way to express this. Velocity reads the @subscribe annotation while collecting a listener's methods, so an enum constant an older runtime lacks throws EnumConstantNotPresentException there and kills ALL of Connect's handlers on pre-2024-09-16 proxies - unguardably, since Velocity does the read, not Connect. The reflective lookup buys identical ordering power with a catchable NoSuchMethodException. This rejection is intentional and is documented in AGENTS.md.
Two additions were explicitly required and approved by the captain, and are non-negotiable:
(a) An operator off-switch, login-reassert.enabled, DEFAULT ON, documented and disableable. Making Connect the last writer of its own pre-login decision is a posture change toward every plugin, so an operator who deliberately wants another plugin to modify Connect's decision must be able to turn it off; off restores the exact pre-fix behaviour.
(b) Full-profile (UUID+username) restoration kept available as a documented OPT-IN, login-reassert.restore-full-profile, NOT default and NOT absent. It is opt-in because forcing Connect's UUID breaks login plugins that key their own database on the proxy UUID: LibreLogin's default new-uuid-creator CRACKED stores an offline UUID derived from the name, so a full re-assert makes its getByUUID lookups miss and NPEs its PostLogin/chooseServer handlers on every join. The 'new-uuid-creator: MOJANG' prerequisite is therefore documented directly next to the option in proxy-config.yml and in the docs, and a test pins that text. An opt-in with a documented prerequisite is fine; a default with a hidden one is a footgun. The default (properties-only, i.e. skin) fixes the hang and the missing skin while keeping such plugins internally consistent.
Every path must degrade to today's behaviour, never worse: login plugin absent -> no-op (result already offline); Velocity too old for short-register -> caught, falls back to LAST + optional dep; any plugin legitimately denies -> deny preserved, a kick is NEVER converted into a join; registration throws -> ConnectPlatform.enable() catches Throwable; channel/marker unreadable -> null -> no-op.
Scope was intentionally held to the Velocity and Bungee listener modules plus config and docs. The only core change is the ProxyConnectConfig/proxy-config.yml addition, which the captain's off-switch requirement necessarily implies. Connect's existing EARLY (Velocity) and LOWEST (Bungee) handlers are left exactly as they are - this only ADDS a floor.
Two things I did beyond the brief, on my own judgment:
On the test bar the captain set explicitly: the regression guard runs the REAL VelocityEventManager and PluginDependencyUtils, not a re-implementation of their ordering rules. PaperMC publishes velocity-api to Maven but NOT the Velocity proxy, so the proxy jar is pinned by sha256 through a content-addressed ivy repo added to settings.gradle.kts, and the test lives in its own :velocity:eventOrderTest source set because that shaded jar carries its own velocity-api 3.4.0 and real com.velocitypowered.proxy classes, which must not shadow the 3.2.0 API the plugin compiles against nor the proxy test stubs in velocity/src/test. In that test the re-assert is registered BEFORE the PostOrder.LAST handler on purpose, so a mere PostOrder.LAST fallback would win the tie and pass vacuously - only the numeric priority passes. I mutation-checked it: setting AFTER_LAST to LAST's value makes it fail. This does add a new external build dependency (a 17.8 MB download from fill-data.papermc.io) to 'check', and therefore to the release path; I judged that acceptable since the build already resolves from eight external repos including PaperMC's own, and noted the failure mode in AGENTS.md.
Not done, and known: no live end-to-end proxy join with a real LibreLogin (the scout report flagged that same gap). Everything else is source-established or executed against real dispatchers.
What Changed
login-reassert.enabledswitch and opt-inlogin-reassert.restore-full-profilebehavior, with configuration and integration documentation covering thenew-uuid-creator: MOJANGprerequisite.Risk Assessment
✅ Low: The requested single per-registrar feature lookup is now implemented, and the reviewed late-registration fallback and rollback paths satisfy the stated invariants without additional material issues.
Testing
Fresh targeted Gradle selectors passed, including the real VelocityEventManager/PluginDependencyUtils dispatcher path, config behavior, Bungee ordering, safety fallbacks, and idempotent profile restoration. Evidence was copied to the required temporary directory; no broad suite, linter, or formatter was run. A live proxy/client join with a real login plugin remains unverified.
Evidence: Real Velocity dispatcher evidence
Real Velocity dispatcher report includes reassert-after-LAST, deny-preservation, dependency-order, and absent-optional-dependency cases with zero failures/errors.Evidence: Velocity reassert behavior
Evidence: Bungee priority evidence
Evidence: Config defaults and documentation
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java:43- The new late-listener registration can escape startup failure containment:ListenerRegisteris eager, so this runs during child-injector creation afterConnectPlatform.enable()'scatch (Throwable)ends. A registration failure can therefore abort proxy initialization, contradicting the required criterion: “registration throws -> ConnectPlatform.enable() catches Throwable.”🔧 Fix: Contained late listener registration failures locally
1 error still open:
velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java:95- The new catch does not restore the promised degraded state when registration is only partially successful.VelocityLateReassertListener.register(...)registers the late pre-login handler before the game-profile handler; if the second registration throws, the catch at this line logs that Connect will behave as before, but the pre-login floor remains installed. That leaves a partial late floor active and violates the required fail-safe: “no late re-assert floor is registered.” Make the two-handler registration atomic/rollback on failure, or otherwise ensure neither late handler remains when either registration fails.🔧 Fix: Made late handler registration atomic with rollback
1 warning still open:
velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java:95- The two handler registrations each callregisterAfterLast(...), which performsEventManager.class.getMethod(...)again. This executes two reflective lookups during startup, contrary to the required/documented single feature-detection lookup. Resolve or cache the method once per registration and reuse it for both handlers.🔧 Fix: Cached Velocity feature detection per registrar
✅ Re-checked - no issues remain.
./gradlew --no-daemon :core:test --tests 'com.minekube.connect.config.ConfigLoaderTest' :bungee:test --tests 'com.minekube.connect.listener.BungeeLateReassertListenerTest' :velocity:test --tests 'com.minekube.connect.listener.VelocityLateReassertListenerTest' --tests 'com.minekube.connect.listener.VelocityGameProfilesTest' --tests 'com.minekube.connect.listener.VelocityLateEventRegistrarTest' --tests 'com.minekube.connect.listener.VelocityListenerRegistrationTest' :velocity:eventOrderTest./gradlew --no-daemon --console=plain :velocity:eventOrderTest --tests 'com.minekube.connect.listener.VelocityLateEventOrderTest' --infoInspected JUnit XML evidence and verifiedgit status --short --branchremained clean after removing generated build outputs.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.