ClientLoginNetworkAddon does not handle unsuccessful query responses

[masmc05]

fabric/fabric-networking-api-v1/src/client/java/net/fabricmc/fabric/impl/networking/client/ClientLoginNetworkAddon.java

Lines 87 to 89 in 1fbc78f

	CompletableFuture<@Nullable PacketByteBuf> future = handler.receive(this.client, this.handler, buf, futureListeners::add);
	future.thenAccept(result -> {
	LoginQueryResponseC2SPacket packet = new LoginQueryResponseC2SPacket(queryId, new PacketByteBufLoginQueryResponse(result));

As we can see here, even though the listener correctly states that the buffer can be null, it doesn't handle that, resulting in NPE on serialization

Maybe it should have been like that?

ServerboundCustomQueryAnswerPacket packet = new ServerboundCustomQueryAnswerPacket(queryId, result == null ? null : new PacketByteBufLoginQueryResponse(result));

[modmuss50]

Just to confirm are you using Minecraft 1.20.2?

[masmc05]

yes

[modmuss50]

Out of intrest whats your usecase for using the login networking APIs? In 1.20.2 in most cases the configuration phase should be a tottal replacement for the login phase.

[masmc05]

For several reasons from the past we add an additional security level for top admins, we'll feel safer that way

[modmuss50]

I would highly reccomend looking into doing this during the configuration phase, its the perfect place to send addional data such as this.

[apple502j]

No, this logic should be kept in the login phase.

[liach]

I would highly reccomend looking into doing this during the configuration phase, its the perfect place to send addional data such as this.

The login query request is guaranteed to receive a response from a vanilla client. In comparison, detecting whether a packet is understood with ping and pong to and attaching pending authentication info onto server-side client objects are more error-prone and is more invasive. And the login network handler has a hold of the vanilla authentication information that modders wish to access.

[modmuss50]

detecting whether a packet is understood with ping and pong to and attaching pending authentication info onto server-side client objects are more error-prone and is more invasive.

This is handled by Fabric API, if its error-prone I need to know about it ASAP as reg sync and other critical parts of fabric API wont be working as expected.

[apple502j]

The problem here is not that configuration networking is doomed (it isn't), it is that for this specific use (security check) you don't want to even proceed to configuration in the first place.