-
-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to 1.19.1 #772
Update to 1.19.1 #772
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
I'll try it as soon as I fix my jdk.
Build failed... |
That is not a bug, what kind of server are you testing this with? It works perfectly fine with a supported server version |
I should note that ViaVersion hasn't implemented 1.19.1-auth support yet |
I also started on the checkstyle update, see the developers notes for the rest I haven't gotten around to fixing yet.
config.set("force-key-authentication", config.getOrElse("force-key-authentication", true)); | ||
config.setComment("force-key-authentication", | ||
"Should the proxy enforce the new public key security standard? By default, this is on."); | ||
config.set("config-version", configVersion == 2.0 ? "2.5" : "1.5"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to move to a .5
version without first having gone through .1
, .2
, .3
and .4
versions
Also, I think it is unnecessary to continue maintaining an old configuration version 1.x
since a single migration can be done by updating the old values and adding new configuration options to the configuration directly #788
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think that just generally, I wanna rewrite the way the config is handled as there's many oddities we've hit for god knows how long, many people seem to get burned by the fact that we're not writing the config out properly, etc pelican-eggs/eggs#1741 - idk what exactly they're doing but there is defo some oddities induced here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My egg should be pulling a perfectly valid config. I am not sure why it's doing what it's doing.
I will work on it soon. I have been moving my servers around the last few days.
This is still experimental, use at your own risk.You can find a build to try at |
I wasn't able to get this working while using a 1.19.1 client. 1.19 clients connected without issue though. This was the error 1.19.1 clients were triggering:
|
@blake-tisbury any logs from backend? Your backend server has to support 1.19.1 natively for this to work correctly |
@Xernium I was using the latest build of Via Version (was updated today for 1.19.1) with 1.19 Purpur, the latest build. Let me see if Purpur has any 1.19.1 dev builds and I can test with that. |
Do you mean that all servers connected to the proxy also need to run 1.19.1 for players to be able to join using the dev build? |
Found a bug, ServerData should also shave |
Experimental build 2:https://fivepb.me/ci/builds/oneshot/velocity-proxy-3.1.2-SNAPSHOT-c57fb48.jar |
c57fb48 seems to work flawlessly, for me with |
Using the same configuration except 1.19.1 players are getting instantly disconnected when typing in anything at all. Notes: |
I'm having the same issue as @blake-tisbury on the older Build 162 (latest from papermc.io) with EDIT: c57fb48 fixed the problem. No issue with NOT verifying keys for chat.
Server info: |
Hello! There are some errors that appear with users of modifications No Chat Reports.
I think a lot of players will put this modification, is it worth checking on the side Velocity this or turn to the creator of the mod? Related to: Velocity/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java Line 560 in c57fb48
Version: |
Oh, I think I got a LITTLE confused, but thanks for such a quick callback! <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping my own approval on this, I generally see no issues rather than maybe a cleanup or so, plus needed API changes for chat but that can probably come over the next week or so
@@ -94,6 +107,15 @@ public void encode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersi | |||
} else { | |||
buf.writeBoolean(false); | |||
} | |||
|
|||
if (version.compareTo(ProtocolVersion.MINECRAFT_1_19_1) >= 0) { | |||
if (playerKey != null && playerKey.getSignatureHolder() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a null check here just because it made sense in regards to letting people do whatever they want, but, that signature holder should probably just actually use the field in the packet, as right now you can't just blindly pass this packet through, I guess in part we'll probably never have a case where we have the key and not the UUID, but, some level of "bleh" here
Tried out both 1e8f9ea and c57fb48, I get the same two issues: First, I get an error when sending a message on a 1.19 client (but the message does send anyways):
|
Can get newest dev version to 1.19.1? |
Sure you can (link above). But make it work, I don't know. |
Running Latest Paper on backend, and the lastest commit of this PR on the proxy. No Chat Reports mod causes a disconnect when switching servers. Stacktrace below.
No Chat Reports mod also causes a disconnect when sending a chat message.
|
"No Chat Reports" flatly refuses to comply with even the letter of the Minecraft protocol - it is essentially providing signed messages without the player's public key. |
Note that if we ever choose to enforce chat signatures, then the mod will just break again... not our fault if we do that, you get what you pay for.
Is this something that should be reported as a bug to them? |
If it is behaviour that the Vanilla server allows, there is no reason why Velocity should not allow it as well. |
* 1.19.1-rc1 * More signature changes * Further 1.19.1 changes I also started on the checkstyle update, see the developers notes for the rest I haven't gotten around to fixing yet. * Fix checkstyle * Checkstyle imports * Fix logic error * Changes 1.19.1-pre2 * 1.19-pre3 * Progress, some parts still WIP * Overlooked changes * Fix ServerData * Fix ServerLogin send check * Workaround the broken behavior of "No Chat Reports" Note that if we ever choose to enforce chat signatures, then the mod will just break again... not our fault if we do that, you get what you pay for. * more Co-authored-by: Shane Freeder <theboyetronic@gmail.com> Co-authored-by: Andrew Steinborn <git@steinborn.me>
* 1.19.1-rc1 * More signature changes * Further 1.19.1 changes I also started on the checkstyle update, see the developers notes for the rest I haven't gotten around to fixing yet. * Fix checkstyle * Checkstyle imports * Fix logic error * Changes 1.19.1-pre2 * 1.19-pre3 * Progress, some parts still WIP * Overlooked changes * Fix ServerData * Fix ServerLogin send check * Workaround the broken behavior of "No Chat Reports" Note that if we ever choose to enforce chat signatures, then the mod will just break again... not our fault if we do that, you get what you pay for. * more Co-authored-by: Shane Freeder <theboyetronic@gmail.com> Co-authored-by: Andrew Steinborn <git@steinborn.me>
Pull-request to track 1.19.1 changes