This is an automated email from the ASF dual-hosted git repository.
lizhimins pushed a commit to branch rocketmq-studio
in repository https://gitbox.apache.org/repos/asf/rocketmq-dashboard.git
The following commit(s) were added to refs/heads/rocketmq-studio by this push:
new bffcc9e06 fix(ai): refuse a probed binary name the shell could parse
as syntax (#4800)
bffcc9e06 is described below
commit bffcc9e06e8e865733d0bd675d3bfb445cfff47a
Author: Wang1rrr <[email protected]>
AuthorDate: Sat Sep 26 18:10:34 2026 +0800
fix(ai): refuse a probed binary name the shell could parse as syntax (#4800)
CliBinaryProbe interpolates its argument straight into sh -c "command -v
<name>",
and isAvailable(String) is public. Every call site today passes a
compile-time
constant (claude, qodercli, rmqctl), so nothing reaches that concatenation
with a
variable right now. But the invariant is a convention spread over four
classes, and
the first caller that forwards a configured or request-derived name turns
it into
command injection: "rmqctl; id" runs id, "/bin/sh" runs the shell, "-x" is
read as
an option to command.
Validate against a bare POSIX-style identifier and fail closed, matching
the posture
the probe already documents - a timeout, an I/O failure and an interrupt
all mean
"not available", because availability gates spawning a CLI and guessing yes
is worse
than guessing no. Refusal happens before a ProcessBuilder exists, so a
rejected name
never reaches the shell, and the log line does not echo the value: this is
the branch
a hostile name would arrive on.
No behaviour change for the constants in use. Two new cases in
CliAgentProviderTest
cover it: the injection shapes are refused without a process ever starting,
and the
names the callers pass still build the same "command -v <name>" lookup.
---
.../ops/ai/conversation/agent/CliBinaryProbe.java | 20 ++++++++++-
.../studio/ops/ai/CliAgentProviderTest.java | 39 ++++++++++++++++++++++
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git
a/server/src/main/java/org/apache/rocketmq/studio/ops/ai/conversation/agent/CliBinaryProbe.java
b/server/src/main/java/org/apache/rocketmq/studio/ops/ai/conversation/agent/CliBinaryProbe.java
index 02e23107e..e19360168 100644
---
a/server/src/main/java/org/apache/rocketmq/studio/ops/ai/conversation/agent/CliBinaryProbe.java
+++
b/server/src/main/java/org/apache/rocketmq/studio/ops/ai/conversation/agent/CliBinaryProbe.java
@@ -22,6 +22,7 @@ import
org.apache.rocketmq.studio.ops.ai.CliProcessEnvironment;
import java.io.IOException;
import java.util.Map;
import java.util.concurrent.TimeUnit;
+import java.util.regex.Pattern;
/**
* Answers "is this CLI on PATH in the runtime image?" by running {@code sh -c
"command -v <binary>"}
@@ -49,6 +50,16 @@ public final class CliBinaryProbe {
* not be able to stall the caller. */
private static final long PROBE_TIMEOUT_SECONDS = 5;
+ /**
+ * A bare POSIX-style binary name. The probe interpolates this value into
{@code sh -c}, so
+ * anything a shell could read as syntax — a space, a command separator, a
substitution, a
+ * glob, a path separator — has to be refused rather than escaped: no
quoting survives being
+ * handed to a command line somebody else assembled. Every caller today
passes a compile-time
+ * constant ({@code claude}, {@code qodercli}, {@code rmqctl}); this is
what keeps that true by
+ * construction instead of by convention.
+ */
+ private static final Pattern BINARY_NAME =
Pattern.compile("[A-Za-z0-9][A-Za-z0-9._+-]*");
+
private final EnvironmentApplier environmentApplier;
private final ProcessStarter processStarter;
@@ -69,9 +80,16 @@ public final class CliBinaryProbe {
/**
* @return true when {@code command -v <binaryName>} exits 0 within the
probe budget. A timeout,
* an I/O failure and an interrupt all mean "not available":
availability is a precondition
- * for spawning a CLI, and guessing yes is worse than guessing no.
+ * for spawning a CLI, and guessing yes is worse than guessing no. A
name that is not a bare
+ * identifier (see {@link #BINARY_NAME}) is refused the same way,
before a process is built.
*/
public boolean isAvailable(String binaryName) {
+ if (binaryName == null || !BINARY_NAME.matcher(binaryName).matches()) {
+ // Fail closed, and log without echoing the value: this is the
branch a hostile name
+ // would reach, and a log line is not the place to hand it back
unescaped.
+ log.warn("refusing to probe a binary name that is not a bare
identifier");
+ return false;
+ }
Process process = null;
try {
ProcessBuilder builder = new ProcessBuilder("sh", "-c", "command
-v " + binaryName);
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/ops/ai/CliAgentProviderTest.java
b/server/src/test/java/org/apache/rocketmq/studio/ops/ai/CliAgentProviderTest.java
index 932141790..6f2579ebe 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/ops/ai/CliAgentProviderTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/ops/ai/CliAgentProviderTest.java
@@ -233,4 +233,43 @@ class CliAgentProviderTest {
assertThat(probe.isAvailable("sh")).isTrue();
assertThat(applied).containsExactly(Map.of());
}
+
+ @Test
+ void probeRefusesANameTheShellCouldReadAsSyntaxTest() {
+ CliBinaryProbe probe = new CliBinaryProbe(
+ (builder, providerEnvironment) -> { },
+ builder -> {
+ throw new AssertionError("a refused name must never reach
the shell");
+ });
+
+ // Every case is a bare name plus something "sh -c" would parse, which
is exactly what
+ // interpolating into "command -v " turns into a second command, an
option or a path.
+ for (String name : List.of("rmqctl; id", "rmqctl && id", "rmqctl |
id", "claude$(id)",
+ "claude`id`", "rmqctl\nid", "/bin/sh", "-x", " ", "")) {
+ assertThat(probe.isAvailable(name)).as("name=[%s]",
name).isFalse();
+ }
+ assertThat(probe.isAvailable(null)).isFalse();
+ }
+
+ @Test
+ void probeStillAcceptsTheBareNamesItsCallersPassTest() throws
InterruptedException {
+ AtomicReference<List<String>> probed = new AtomicReference<>();
+ Process process = mock(Process.class);
+ when(process.waitFor(anyLong(),
eq(java.util.concurrent.TimeUnit.SECONDS))).thenReturn(true);
+ when(process.exitValue()).thenReturn(0);
+ CliBinaryProbe probe = new CliBinaryProbe(
+ (builder, providerEnvironment) -> { },
+ builder -> {
+ probed.set(List.copyOf(builder.command()));
+ return process;
+ });
+
+ // The constants the callers pass today, plus the shapes the
identifier pattern allows.
+ assertThat(probe.isAvailable("rmqctl")).isTrue();
+ assertThat(probed.get()).containsExactly("sh", "-c", "command -v
rmqctl");
+ assertThat(probe.isAvailable("claude")).isTrue();
+ assertThat(probe.isAvailable("qodercli")).isTrue();
+ assertThat(probe.isAvailable("definitely-not-on-path-9f3a")).isTrue();
+ assertThat(probe.isAvailable("rmqctl.beta_2+x")).isTrue();
+ }
}
