Wang1rrr opened a new pull request, #4800: URL: https://github.com/apache/rocketmq-dashboard/pull/4800
### Which Issue(s) This PR Fixes No issue - hardening item from a review of where untrusted strings reach a subprocess. ### Brief Description `CliBinaryProbe.isAvailable(String)` interpolates its argument straight into `sh -c "command -v <name>"`, and the method is public. Every call site today passes a compile-time constant (`claude`, `qodercli`, `rmqctl`), so nothing reaches that concatenation with a variable right now and this is **not** a live vulnerability. But the invariant is a convention spread across four classes (`ClaudeCodeAgentProvider`, `QoderAgentProvider`, `RmqctlWorkspace`, `AgentCapabilityProbe`), 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`. The name is now validated against a bare POSIX-style identifier and the probe fails closed, which matches the posture the class 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. ### How Did You Test This Change? Two new cases in `CliAgentProviderTest`, next to the existing probe tests: - `probeRefusesANameTheShellCouldReadAsSyntaxTest` - `rmqctl; id`, `rmqctl && id`, `rmqctl | id`, `claude$(id)`, a backtick substitution, an embedded newline, `/bin/sh`, `-x`, a blank and a space, plus `null`, are all refused; the injected `ProcessStarter` throws if it is ever called, proving nothing reached the shell. - `probeStillAcceptsTheBareNamesItsCallersPassTest` - `rmqctl`, `claude`, `qodercli`, `definitely-not-on-path-9f3a` and `rmqctl.beta_2+x` are still accepted and still build exactly `sh -c command -v <name>`. ``` cd server mvn -B -Dtest=CliAgentProviderTest#probeRefusesANameTheShellCouldReadAsSyntaxTest+probeStillAcceptsTheBareNamesItsCallersPassTest test [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` The whole class, for context - the six `sh`-dependent cases fail on a Windows dev box that has no `sh` on PATH, identically before and after this change: ``` mvn -B -Dtest=CliAgentProviderTest test before: Tests run: 10, Failures: 3, Errors: 3 after: Tests run: 12, Failures: 3, Errors: 3 (the two new cases pass) ``` Checkstyle: `You have 0 Checkstyle violations.` Backend only - no UI text, no new source files, no API change. ### Checklist - [x] One coherent change; unrelated modifications are not bundled in - [x] Commit subject follows Conventional Commits (`feat:` / `fix:` / `refactor:` / `chore:` / `docs:` / `perf:`) - [x] Tests added or updated for non-trivial changes, test methods named `...Test` - [ ] New UI text has both Chinese and English entries under `web/src/i18n/` - N/A, no UI text - [ ] Architecture constraints stay green (`mvn test` runs the ArchUnit checks) - no new types or packages introduced - [x] New source files carry the ASF license header - N/A, no new files - [ ] Documentation touched where behaviour changed - the probe's own javadoc states the accepted shape -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
