NeoForge icon indicating copy to clipboard operation
NeoForge copied to clipboard

Allow running client commands from chat components

Open SquidDev opened this issue 1 year ago • 9 comments

Currently client commands are only handled when the user sends the chat message directly. "Unsigned commands" (i.e. those sent via ClickEvent.Action.RUN_COMMAND) are not handled by the client command system, instead being sent directly to the server.

This patch changes that behaviour, so ClickEvents can also run client commands.

SquidDev avatar Jan 22 '24 10:01 SquidDev

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jan 22 '24 10:01 CLAassistant

  • [x] Publish PR to GitHub Packages

Last commit published: 55d84c29d6950bc7e990f079987c19e13ed13884.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #554' // https://github.com/neoforged/NeoForge/pull/554
        url 'https://prmaven.neoforged.net/NeoForge/pr554'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr554.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr554
cd NeoForge-pr554
curl -L https://prmaven.neoforged.net/NeoForge/pr554/net/neoforged/neoforge/21.3.4-beta-pr-554-client-commands/mdk-pr554.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

What's the use case for this? I'm a bit reluctant on this, partly because I'm not sure if there are potential unintended consequences for allowing clickable chat events with client commands.

sciwhiz12 avatar Feb 03 '24 14:02 sciwhiz12

What's the use case for this?

In my particular case, I've some commands which print profiling and other debugging information to the console (a bit like /neoforge track). The output of these commands also includes some buttons, such as "teleport to this block". One of these, in a single player world, runs a client-side command.

I'm not sure if there are potential unintended consequences for allowing clickable chat events with client commands.

Yeah, this is something I've definitely ummed and ahhed about. If one is a playing on a malicious server, it would be quite easy to social engineer someone into clicking in chat and running a client-side command. Obviously then you'd need an "exploitable" client-side command, and I'm sure there's a mod out there which just allows you to eval arbitrary Javascript or something.

That all said, I'm not really sure how likely that is to happen in practice.

For what it's worth, Forge used to support this until 1.19.x, and Fabric still does. Which obviously isn't a "oh, it's perfectly safe", but might alleviate some of your concerns.

SquidDev avatar Feb 03 '24 15:02 SquidDev

About security concern, would writing command to be executed on chat be suitable? so users can see full command on chat and then need to press enter to actually run it.

tmvkrpxl0 avatar Feb 04 '24 12:02 tmvkrpxl0

After thinking about this briefly today, I think there's merit in unifying handling of commands such that a RUN_COMMAND action event works equivalently with server and client commands, as if the player was asked to put the command in chat and run it manually.

@SquidDev, do you still intend on updating this PR to 1.21? If you do so, please also add a comment at that patched-in site explaining that this enables client commands to run from chat components, so we remember the difference compared to the hook in sendCommand. (If you feel up to it, add a comment as well in the hook for sendCommand.)

sciwhiz12 avatar Jul 04 '24 21:07 sciwhiz12

Yep, happy to rebase+retarget this PR. Will have a look after #1231 is merged.

SquidDev avatar Jul 05 '24 11:07 SquidDev

@SquidDev, this pull request has conflicts, please resolve them for this PR to move forward.

@SquidDev #1231 is stalled for the time being, so if you intend to update this PR, you shouldn't await completion of that change.

Shadows-of-Fire avatar Sep 17 '24 18:09 Shadows-of-Fire