Skip to content
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

Fix missing handling of text components in 1.11.1>1.12 chat item rewriter #3662

Closed
wants to merge 1 commit into from

Conversation

LeonTG
Copy link

@LeonTG LeonTG commented Jan 21, 2024

I never noticed that this was not handled by ViaVersion until upgrading to 1.20.3 and above

1.20.3 changed it so a client will be kicked if an invalid chat json text is sent to the client.

ViaVersion did not check for hover events using a JsonObject with a text element inside, and then made the hover event an empty array instead, which on 1.20.3 and above, would kick the client for the reason seen here: image

I have tested this and it seems to have fixed the issue, now sending a item or entity hoverEvent on a 1.20.3+ client on a 1.8-1.11 server no longer kicks me

Hopefully I described the issue well and submitted an adequate fix for it! Let me know if anything is unclear or if there is changes you'd want me to do to the fix!

Copy link
Member

@FlorianMichael FlorianMichael left a comment

Choose a reason for hiding this comment

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

Didn't had the time to look at it in the last weeks, sorry about the delay!

While this seems to be valid, we should take a closer look at all ChatItemRewriter when properly migrating to MCStructs / adding all missing conversions.

Thanks!

@FlorianMichael
Copy link
Member

Also could you please provide the nbt / command you used to fix/debug this? I might found some issues in the existing code as well

@LeonTG
Copy link
Author

LeonTG commented Mar 4, 2024

Also could you please provide the nbt / command you used to fix/debug this? I might found some issues in the existing code as well

The json that crashed me in the image above is:
{"extra":[{"bold":true,"color":"dark_red","text":"UHC"},{"color":"dark_gray","text":" » "},{"color":"gray","extra":[{"color":"white","hoverEvent":{"action":"show_item","value":[{"text":"{id:\"minecraft:diamond_sword\",Count:1b,Damage:0s}"}]},"extra":[{"translate":"item.swordDiamond.name"}],"text":"1x "},{"text":"\u0027 to \u0027"},{"color":"white","text":"MarcusWest"},{"text":"\u0027."}],"text":"Gave \u0027"}],"text":""}

When viewing a more clean version of that json, I've highlighted here the area that gets removed without my PR:
image

@FlorianMichael
Copy link
Member

Are you able to reproduce this kick using /tellraw and your json? I can't reproduce this using latest ViaProxy, 1.20.4 client and 1.8 server, same for ViaVersion being installed on the spigot server.

@LeonTG
Copy link
Author

LeonTG commented Mar 5, 2024

Are you able to reproduce this kick using /tellraw and your json? I can't reproduce this using latest ViaProxy, 1.20.4 client and 1.8 server, same for ViaVersion being installed on the spigot server.

I believe /tellraw would change the formatting of the hover event back to a basic string primitive, which ViaVerison already handles

The way this json gets sent in my case is using the Spigot components API

@FlorianMichael
Copy link
Member

Thanks for the PR, I decided to use MCStructs for the conversion to fix even more issues the current code has (For example you can split the nbt using translate and extra or you can insert multiple arrays into each other), see my PR: #3740

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.

None yet

2 participants