PaperMC / Velocity

The modern, next-generation Minecraft server proxy.
https://papermc.io/software/velocity
GNU General Public License v3.0
1.75k stars 607 forks source link

A plugin tried to cancel a signed chat message. #804

Closed skerit closed 2 years ago

skerit commented 2 years ago

So I'm preparing to upgrade my servers to 1.19.1, I decided to start with my network-wide-chat plugin and I immediately run into this problem:

A plugin tried to cancel a signed chat message. This is no longer possible in 1.19.1 and newer. Disconnecting player

Added in this line: https://github.com/PaperMC/Velocity/blob/c57fb489f3cc664fe05cdbdbb24fdca1347ef446/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java#L200

Is there a technical reason why Velocity wants to turn this into an error? Because if I simply disable that line and rebuild, my original plugin works fine. Player chat messages are actually sent to all players across all servers as a simple system message.

electronicboy commented 2 years ago

Due to changes in 1.19.1, cancelling chat has wider implications, i.e. the thing cancelling chat still needs to ensure that everything else gets the relevant updates, etc, this is especially an issue in multi proxy environments in which API will need to be developed for

astei commented 2 years ago

I think it's very unlikely that we continue to support chat manipulation on the proxy side. It's an entire can of worms we don't really want to touch. In theory, there might be a path forward if the server simply doesn't verify messages, but we want Velocity to work on as many setups as possible.

A248 commented 2 years ago

If you choose to forsake chat handling, you'll have to accept that you have broken your API. Many plugins rely on this feature. It would be rather unfortunate for Velocity to stop handling chat entirely.

astei commented 2 years ago

For the most part, I am extrapolating. Player chat manipulation was broken by Mojang and we certainly haven't seen what Mojang's ultimate plans are with this.

We can support chat manipulation if Velocity rewrites all chat messages to be unsigned - this is probably the only path forward for allowing plugins to rewrite messages. Long term, however, Mojang may throw another wrench in the works (i.e. having Minecraft accounts that can only join servers that verify profiles and have signed chat messages) and that will soon force a decision upon us. All this really does is kick the can down the road.

electronicboy commented 2 years ago

Supporting chat on the proxy is doable, but it means deferring more behavior towards plugins, i.e. probably making them in control of updating the clients state, it creates many dozen issues because you need to keep the chain intact, otherwise you'll cause the client to DC whenever something uses a signed message, which is where the headaches come in

Me and Five has talked about this a bit, there's a few general ideas of stuff we can do but, this is largely a huge headache (and we generally recommend servers deal with chat on the actual servers and have for years, even before this BS generally showed up) - This will only be supported on servers implementing full V%whatever% forwarding support, and is in part going to rely on plugins to deal with certain aspects themselves, i.e. keeping clients happy so that they don't disconnect themselves, etc

davidmayr commented 2 years ago

Supporting chat on the proxy is doable, but it means deferring more behavior towards plugins, i.e. probably making them in control of updating the clients state, it creates many dozen issues because you need to keep the chain intact, otherwise you'll cause the client to DC whenever something uses a signed message, which is where the headaches come in

Me and Five has talked about this a bit, there's a few general ideas of stuff we can do but, this is largely a huge headache (and we generally recommend servers deal with chat on the actual servers and have for years, even before this BS generally showed up) - This will only be supported on servers implementing full V%whatever% forwarding support, and is in part going to rely on plugins to deal with certain aspects themselves, i.e. keeping clients happy so that they don't disconnect themselves, etc

I think that we should at least be able to cancel messages. This PR of the BungeeCord project would suggest that this is currently not a problem: https://github.com/SpigotMC/BungeeCord/pull/3319

electronicboy commented 2 years ago

What spigot is doing there is going to break the chat chain, maybe not a huge deal for them given that they don't even support that thing but pretty sure that's going to break stuff

On Thu, 4 Aug 2022, 19:59 David Mayr, @.***> wrote:

Supporting chat on the proxy is doable, but it means deferring more behavior towards plugins, i.e. probably making them in control of updating the clients state, it creates many dozen issues because you need to keep the chain intact, otherwise you'll cause the client to DC whenever something uses a signed message, which is where the headaches come in

Me and Five has talked about this a bit, there's a few general ideas of stuff we can do but, this is largely a huge headache (and we generally recommend servers deal with chat on the actual servers and have for years, even before this BS generally showed up) - This will only be supported on servers implementing full V%whatever% forwarding support, and is in part going to rely on plugins to deal with certain aspects themselves, i.e. keeping clients happy so that they don't disconnect themselves, etc

I think that we should at least be able to cancel messages. This PR of the BungeeCord project would suggest that this is currently not a problem: SpigotMC/BungeeCord#3319 https://github.com/SpigotMC/BungeeCord/pull/3319

— Reply to this email directly, view it on GitHub https://github.com/PaperMC/Velocity/issues/804#issuecomment-1205656788, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAZGDL7BVWEQ2XZKTX73VXQHI7ANCNFSM55AW6SBQ . You are receiving this because you commented.Message ID: @.***>

davidmayr commented 2 years ago

Well, MD-5 initially wrote that it would break the possibility of having authenticated chat. So it's not like they don't care entirely. But in the next comment, he said that the chat system would be only time based and that that wouldn't be an issue for now.

I would like to know who's in the right here. Is it an issue or not?

electronicboy commented 2 years ago

The server at the time only validated the time order of signed stuff, i.e. chat, commands, but that has however since changed

The server now has validation for the chain too, cancelling a packet on the proxy will bork the tracking of the chain, idk if it will induce a kick, looks like it will, but it will at least break the tracking of the chain itself on the client

davidmayr commented 2 years ago

The server at the time only validated the time order of signed stuff, i.e. chat, commands, but that has however since changed

The server now has validation for the chain too, cancelling a packet on the proxy will bork the tracking of the chain, idk if it will induce a kick, looks like it will, but it will at least break the tracking of the chain itself on the client

Ah, I get it now. Thanks for the clarification, I didn't look at the date of the pr. This stupid new system really makes my life hard. Now I have to move my mute stuff to a paper plugin and cache punishments there. Will be fun, yay.

astei commented 2 years ago

Yep, all of this is unfortunate. We'll have to think about a long-term future for chat given what Mojang has already done and what they might do in the future. We don't have a crystal ball.

SkyLicks commented 2 years ago

@astei are you sure that #827 is not a seperate issue? I can confirm that regular chat messages can still be cancelled (the unsigned ones). However no commands can be cancelled.

electronicboy commented 2 years ago

They're both the same issue, there is some carveout in the impl for unsigned chat messages as an exclusion, but cancelling this stuff in general needs a wider fix here which isn't strictly limited by the command def sent to the client

astei commented 2 years ago

@SkyLicks You provided no other information other than "I updated to support 1.19.1 and now cancelling commands doesn't work". I made the very reasonable assumption that you're using 1.19.1 and expected the functionality to work, but it doesn't.

SkyLicks commented 2 years ago

@SkyLicks You provided no other information other than "I updated to support 1.19.1 and now cancelling commands doesn't work". I made the very reasonable assumption that you're using 1.19.1 and expected the functionality to work, but it doesn't.

Apologies for the confusion. I've added additional details to #827 incase it needs to be reopened.

MegaTophat commented 2 years ago

I don't think preventing behavior is necessary at all as I really don't see an issue with allowing plugins to manipulate chat. If most server software breaks the chat chain anyways, it's inherently understood from a plugin developer perspective that spoofing chat messages or modifying sent ones will obviously invalidate the signature. A basic config option to let plugins do this but have no signatures, or have no cancelling but retain signatures up to the backend could potentially work, but there's probably a more elegant solution.

When it comes to Minecraft clients starting to force connecting to verify-only servers, that's currently not something Microsoft seems to be doing and I don't think it's reasonable to plan for something that isn't even really a rumor yet, let alone planned functionality. If something like this does become a reality, then most likely spoofing the behavior for the client to avoid it disconnecting but then not actually carrying it through is still an option. Remember, on Spigot servers we can cancel block breaks and to the client it shows the block break and then it comes back, just show the chat message being sent to the client sending but then don't actually forward it.

electronicboy commented 2 years ago

Because it's not really our desire to break this stuff and having config options which intentionally break stuff especially in code which mojang keeps screwing with, placing bets on anything here not causing wider breakages is practically a no-go.

hence #817 and other planned API to fix this properly; it's not a desire for a temporary solution here, as those are never temp; block breaks are an entirely irrelevant example given the nature of it being the only choice there and it not having wider state impacts which we don't have the means to resolve right now

MegaTophat commented 2 years ago

Look, the desire here is to give choice. Many many server owners aren't going to want the signed chat messages to begin with, and as soon as the idea of those already disliked features causing plugin breakage becomes apparent, they will extra not want them. I don't want a fork of Velocity having to be made just because of one stupid little thing that the developers themselves are saying they hate Microsoft for doing. If there's a solution coming in the works that will allow plugins to cancel signed chat messages without breaking stuff, perfect, I'll patiently wait and update all my plugins and make changes in the plugins I develop to accommodate them. In the meantime, I just have to deal with 1.19.1+ players having an awful chat experience and then I have to cobble together my own solution while I wait for an official one. And believe me, it's going to be a temporary one. The second I can update velocity itself, that little solution plugin I made is going in the trash.

Bottom line is this functionality is something I need to remain possible. I don't want to install chat filtering plugins on every single backend, nor do I want to install the anti-chat report plugin on every single backend. There must be a way.

Xernium commented 2 years ago

There is going to be a choice. But we aren’t going to instate a temporary fix for that reason. Such a fix will most definitely collide with the finished actual solution. So until this is properly implemented absolutely nothing about this here will change