From 5d71770931b1db8bd5eb452b0f0d904258208cbe Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Wed, 5 Jul 2023 11:38:21 +0100 Subject: [PATCH] Several command permission fixes - Attach permission checks to the first argument (so the literal command name) rather than the last argument. This fixes commands showing up when they shouldn't. - HelpingArgumentBuilder now inherits permissions of its leaf nodes. This only really impacts the "track" subcommand. - Don't autocomplete the computer selector for the "queue" subcommand. As everyone has permission for this command, it's possible to find all computer ids and labels in the world. I'm in mixed minds about this, but don't think this is an exploit - computer ids/labels are sent to in-range players so shouldn't be considered secret - but worth patching none-the-less. --- .../shared/command/CommandComputerCraft.java | 8 ++++- .../shared/command/UserLevel.java | 23 ++++++++++++++ .../command/builder/CommandBuilder.java | 26 ++++++++-------- .../builder/HelpingArgumentBuilder.java | 30 +++++++++++++++++-- 4 files changed, 70 insertions(+), 17 deletions(-) diff --git a/projects/common/src/main/java/dan200/computercraft/shared/command/CommandComputerCraft.java b/projects/common/src/main/java/dan200/computercraft/shared/command/CommandComputerCraft.java index 7c67aab144..93b96d0698 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/command/CommandComputerCraft.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/command/CommandComputerCraft.java @@ -6,9 +6,12 @@ import com.mojang.brigadier.CommandDispatcher; import com.mojang.brigadier.arguments.StringArgumentType; +import com.mojang.brigadier.builder.RequiredArgumentBuilder; import com.mojang.brigadier.exceptions.CommandSyntaxException; +import com.mojang.brigadier.suggestion.Suggestions; import dan200.computercraft.core.computer.ComputerSide; import dan200.computercraft.core.metrics.Metrics; +import dan200.computercraft.shared.command.arguments.ComputersArgumentType; import dan200.computercraft.shared.command.text.TableBuilder; import dan200.computercraft.shared.computer.core.ComputerFamily; import dan200.computercraft.shared.computer.core.ServerComputer; @@ -169,7 +172,10 @@ public static void register(CommandDispatcher dispatcher) { .then(command("queue") .requires(UserLevel.ANYONE) - .arg("computer", manyComputers()) + .arg( + RequiredArgumentBuilder.argument("computer", manyComputers()) + .suggests((context, builder) -> Suggestions.empty()) + ) .argManyValue("args", StringArgumentType.string(), Collections.emptyList()) .executes((ctx, args) -> { var computers = getComputersArgument(ctx, "computer"); diff --git a/projects/common/src/main/java/dan200/computercraft/shared/command/UserLevel.java b/projects/common/src/main/java/dan200/computercraft/shared/command/UserLevel.java index 2d9438121d..c8bdbb0f2a 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/command/UserLevel.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/command/UserLevel.java @@ -49,6 +49,29 @@ public boolean test(CommandSourceStack source) { return source.hasPermission(toLevel()); } + /** + * Take the union of two {@link UserLevel}s. + *

+ * This satisfies the property that for all sources {@code s}, {@code a.test(s) || b.test(s) == (a ∪ b).test(s)}. + * + * @param left The first user level to take the union of. + * @param right The second user level to take the union of. + * @return The union of two levels. + */ + public static UserLevel union(UserLevel left, UserLevel right) { + if (left == right) return left; + + // x ∪ ANYONE = ANYONE + if (left == ANYONE || right == ANYONE) return ANYONE; + + // x ∪ OWNER = OWNER + if (left == OWNER) return right; + if (right == OWNER) return left; + + // At this point, we have x != y and x, y ∈ { OP, OWNER_OP }. + return OWNER_OP; + } + private static boolean isOwner(CommandSourceStack source) { var server = source.getServer(); var sender = source.getEntity(); diff --git a/projects/common/src/main/java/dan200/computercraft/shared/command/builder/CommandBuilder.java b/projects/common/src/main/java/dan200/computercraft/shared/command/builder/CommandBuilder.java index 3fe77ff482..2a395ff7b6 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/command/builder/CommandBuilder.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/command/builder/CommandBuilder.java @@ -48,11 +48,15 @@ public CommandBuilder requires(Predicate predicate) { return this; } - public CommandBuilder arg(String name, ArgumentType type) { - args.add(RequiredArgumentBuilder.argument(name, type)); + public CommandBuilder arg(ArgumentBuilder arg) { + args.add(arg); return this; } + public CommandBuilder arg(String name, ArgumentType type) { + return arg(RequiredArgumentBuilder.argument(name, type)); + } + public CommandNodeBuilder>> argManyValue(String name, ArgumentType type, List empty) { return argMany(name, type, () -> empty); } @@ -74,7 +78,7 @@ private CommandNodeBuilder>> argMany(String name, R return command -> { // The node for no arguments - var tail = tail(ctx -> command.run(ctx, empty.get())); + var tail = setupTail(ctx -> command.run(ctx, empty.get())); // The node for one or more arguments ArgumentBuilder moreArg = RequiredArgumentBuilder @@ -83,7 +87,7 @@ private CommandNodeBuilder>> argMany(String name, R // Chain all of them together! tail.then(moreArg); - return link(tail); + return buildTail(tail); }; } @@ -94,20 +98,16 @@ private static List getList(CommandContext context, String name) { @Override public CommandNode executes(Command command) { - if (args.isEmpty()) throw new IllegalStateException("Cannot have empty arg chain builder"); - - return link(tail(command)); + return buildTail(setupTail(command)); } - private ArgumentBuilder tail(Command command) { - var defaultTail = args.get(args.size() - 1); - defaultTail.executes(command); - if (requires != null) defaultTail.requires(requires); - return defaultTail; + private ArgumentBuilder setupTail(Command command) { + return args.get(args.size() - 1).executes(command); } - private CommandNode link(ArgumentBuilder tail) { + private CommandNode buildTail(ArgumentBuilder tail) { for (var i = args.size() - 2; i >= 0; i--) tail = args.get(i).then(tail); + if (requires != null) tail.requires(requires); return tail.build(); } } diff --git a/projects/common/src/main/java/dan200/computercraft/shared/command/builder/HelpingArgumentBuilder.java b/projects/common/src/main/java/dan200/computercraft/shared/command/builder/HelpingArgumentBuilder.java index 20aaa2e907..dfe2fecc34 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/command/builder/HelpingArgumentBuilder.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/command/builder/HelpingArgumentBuilder.java @@ -10,6 +10,7 @@ import com.mojang.brigadier.context.CommandContext; import com.mojang.brigadier.tree.CommandNode; import com.mojang.brigadier.tree.LiteralCommandNode; +import dan200.computercraft.shared.command.UserLevel; import net.minecraft.ChatFormatting; import net.minecraft.commands.CommandSourceStack; import net.minecraft.network.chat.ClickEvent; @@ -18,6 +19,8 @@ import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Collection; +import java.util.function.Predicate; +import java.util.stream.Stream; import static dan200.computercraft.core.util.Nullability.assertNonNull; import static dan200.computercraft.shared.command.text.ChatHelpers.coloured; @@ -37,6 +40,29 @@ public static HelpingArgumentBuilder choice(String literal) { return new HelpingArgumentBuilder(literal); } + @Override + public LiteralArgumentBuilder requires(Predicate requirement) { + throw new IllegalStateException("Cannot use requires on a HelpingArgumentBuilder"); + } + + @Override + public Predicate getRequirement() { + // The requirement of this node is the union of all child's requirements. + var requirements = Stream.concat( + children.stream().map(ArgumentBuilder::getRequirement), + getArguments().stream().map(CommandNode::getRequirement) + ).toList(); + + // If all requirements are a UserLevel, take the union of those instead. + var userLevel = UserLevel.OWNER; + for (var requirement : requirements) { + if (!(requirement instanceof UserLevel level)) return x -> requirements.stream().anyMatch(y -> y.test(x)); + userLevel = UserLevel.union(userLevel, level); + } + + return userLevel; + } + @Override public LiteralArgumentBuilder executes(final Command command) { throw new IllegalStateException("Cannot use executes on a HelpingArgumentBuilder"); @@ -80,9 +106,7 @@ private LiteralCommandNode buildImpl(String id, String comma helpCommand.node = node; // Set up a /... help command - var helpNode = LiteralArgumentBuilder.literal("help") - .requires(x -> getArguments().stream().anyMatch(y -> y.getRequirement().test(x))) - .executes(helpCommand); + var helpNode = LiteralArgumentBuilder.literal("help").executes(helpCommand); // Add all normal command children to this and the help node for (var child : getArguments()) {