Redo chat parsing and validation by danthedaniel · Pull Request #24 · Tildes-MC/minecraft-web-chat

danthedaniel · 2024-12-19T14:34:54Z

Fixes #20

Components are rendered to DOM elements (eliminates hand-rolled escaping)
Structures are validated up-front instead of throughout the code (including depth checks)
I also fixed some incorrect types from chat.mjs where I thought we stored parsed messages

danthedaniel · 2024-12-19T14:35:44Z

@creesch the structure from prismarine-chat is out-of-date. The value field is deprecated. Perhaps only used by old Minecraft versions?

danthedaniel · 2024-12-19T14:36:51Z

src/client/resources/web/js/message_parsing.mjs

        // that automatically pauses when tab is inactive
        if (timestamp - lastUpdate >= updateInterval) {
-            const elements = document.getElementsByClassName('minecraft-obfuscated');
+            const elements = document.getElementsByClassName('mc-obfuscated');


I threw in this random change because the different class name format was upsetting me.

danthedaniel · 2024-12-19T14:55:26Z

src/client/resources/web/js/message_parsing.mjs

- * }} HoverEventContents
+ * @typedef {Object} ShowEntityHoverEvent
+ * @property {'show_entity'} action - Displays entity information
+ * @property {{ type: string, id: unknown, name?: string }} contents - The entity data to show


I left id as unknown. It seems it should be a UUID string, but it's coming through as an array of 4 numbers (which could make sense if the 4 numbers are 1:1 with a UUID at 32 bits each). So perhaps we're doing a different serialization than the Wiki expects.

Oh btw

So perhaps we're doing a different serialization than the Wiki expects.

We are using native serialization, its also possible the wiki is incorrect. There is also differences between bedrock and java. Which wiki have you been looking at?

creesch · 2024-12-19T16:05:20Z

src/client/resources/web/js/chat.mjs

-    div.innerHTML = parseMinecraftText(json);
+
+    /** @type {unknown} */
+    const json = JSON.parse(rawJson);


I feel like this would fail. While the variable is called rawJSON it is already parsed and an object when received through websockets.

Trying to parse a js object as JSON again will throw a syntax error.

creesch · 2024-12-19T16:07:30Z

@creesch the structure from prismarine-chat is out-of-date. The value field is deprecated. Perhaps only used by old Minecraft versions?

That is entirely possible as they support multiple version of minecraft, if we are sure it is no longer used by newer versions of minecraft we can leave that field out I am not planning on figuring out support for older versions of minecraft.

I'll have a proper look a bit later, on the face of it it seems fairly solid. Some things that do jump out:

Are we sure that for titles it is enough to rely on the browser to sanitize what we put in there? I feel like I have seen some stuff about attribute injection in the past.
One potential bug in initial json handling.
Error handling feels like it might be too strict in some cases but I want to test it first before I comment on that.

creesch · 2024-12-19T16:25:33Z

Little bit of work todo

{"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"test"]}
chat.mjs:79 Invalid component: Component is not an object at .with.1
displayMessage @ chat.mjs:79
chat.mjs:68 {"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"cool"]}
chat.mjs:79 Invalid component: Component is not an object at .with.1
displayMessage @ chat.mjs:79
chat.mjs:68 {"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"works"]}
chat.mjs:79 Invalid component: Component is not an object at .with.1
displayMessage @ chat.mjs:79
chat.mjs:68 {"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"like it should"]}
chat.mjs:79 Invalid component: Component is not an object at .with.1
displayMessage @ chat.mjs:79
chat.mjs:68 {"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"awesome! "]}
chat.mjs:79 Invalid component: Component is not an object at .with.1
displayMessage @ chat.mjs:79
chat.mjs:107 Connected to server
chat.mjs:68 {"translate":"chat.type.text","with":[{"text":"creesch","hoverEvent":{"contents":{"type":"minecraft:player","id":[2097578677,-1517796232,-2140994661,-1953866359],"name":"creesch"},"action":"show_entity"},"insertion":"creesch","clickEvent":{"action":"suggest_command","value":"/tell creesch "}},"does this come through?"]}
chat.mjs:79 Invalid component: Component is not an object at .with.1

It looks like value is still in use even in 1.21.1.

creesch · 2024-12-19T16:28:25Z

Also going to be a bit real, I like code to be actually tested before a PR is made. Or have the PR marked as a draft if you haven't been able to test it.

danthedaniel · 2024-12-19T16:39:51Z

That's fair. I did test the PR. I did not see any errors when I tested. I do not have a good understanding of what cases to run through.

creesch · 2024-12-19T17:17:17Z

That's fair. I did test the PR. I did not see any errors when I tested

Interesting, I just realized that this only happens in a game I am running locally. I did open it up to LAN so it acts as a server and the same still happens there.

I do not have a good understanding of what cases to run through.

At the very least the messages from above ;)

I have also been using https://mcstacker.net/1.21.php to create /tellraw commands to test various formatting.

creesch · 2024-12-19T17:33:08Z

So it looks like that in Minecraft's chat format, the elements in the with array can actually be a string. Same as you handle extra

src/client/resources/web/js/message_parsing.mjs

danthedaniel · 2024-12-19T18:30:36Z

Are we sure that for titles it is enough to rely on the browser to sanitize what we put in there? I feel like I have seen some stuff about attribute injection in the past.

I think that's an issue if:

We are doing string interpolation for building HTML elements - at which point an attacker could terminate the attribute's string and inject a <script> tag.
We put raw HTML into the title attribute and read it back later. We might do this at some point for more rich hover information, but that's currently not the case.

creesch · 2024-12-19T20:28:53Z

Alright, did some testing and seems to work as expected now.

danthedaniel commented Dec 19, 2024

View reviewed changes

danthedaniel force-pushed the parsing-validating branch from 41c9c2b to 29639fa Compare December 19, 2024 14:52

danthedaniel commented Dec 19, 2024

View reviewed changes

Redo chat parsing and validation

3c3f552

danthedaniel force-pushed the parsing-validating branch from 29639fa to 3c3f552 Compare December 19, 2024 15:52

creesch reviewed Dec 19, 2024

View reviewed changes

src/client/resources/web/js/message_parsing.mjs Outdated Show resolved Hide resolved

danthedaniel added 3 commits December 19, 2024 13:34

Fix handling of string elements in with array

0d284a2

Add partial support for legacy hoverEvent format

ccdcded

Use null-coalesce operator where appropriate

6d659aa

creesch merged commit 0da96ce into Tildes-MC:main Dec 19, 2024

Conversation

danthedaniel commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danthedaniel commented Dec 19, 2024

Uh oh!

danthedaniel Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

danthedaniel Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

creesch Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

creesch Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

creesch commented Dec 19, 2024

Uh oh!

creesch commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

creesch commented Dec 19, 2024

Uh oh!

danthedaniel commented Dec 19, 2024

Uh oh!

creesch commented Dec 19, 2024

Uh oh!

creesch commented Dec 19, 2024

Uh oh!

Uh oh!

danthedaniel commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

creesch commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danthedaniel commented Dec 19, 2024 •

edited

Loading

creesch Dec 19, 2024 •

edited

Loading

creesch commented Dec 19, 2024 •

edited

Loading

danthedaniel commented Dec 19, 2024 •

edited

Loading