-
Notifications
You must be signed in to change notification settings - Fork 113
Description
Summary
- Context: The
Commandmodule is responsible for translating Amber commands (e.g.,$echo "hi"$) into Bash code, including handling modifiers likesudo,silent, andsuppress. - Bug: The translation logic joins the
sudoprefix, the command body, and the redirection modifiers without ensuring there are spaces between them. - Actual vs. expected: For a command like
sudo $echo "hi"$, it generates${__sudo}echo "hi"instead of${__sudo} echo "hi". Forsilent $echo 1$, it generatesecho 1>/dev/null 2>&1instead ofecho 1 >/dev/null 2>&1. - Impact: The
sudomodifier is completely broken because it tries to execute a concatenated command name (e.g.,sudoecho) which doesn't exist. Thesilentmodifier can cause incorrect behavior when the command ends with a digit, as Bash interprets it as file descriptor redirection (e.g.,1>/dev/nullredirects stdout).
Code with bug
// src/modules/command/cmd.rs
00097| let translation = fragments!(sudo_prefix, translation, suppress, silent); // <-- BUG 🔴 Missing spaces between componentsEvidence
1. Reproduction Test
I created a test case that specifically checks for the presence of spaces in the generated Bash code.
// src/tests/repro_sudo.rs
#[test]
fn test_translate_with_sudo_space() {
let code = r#"
main {
sudo $echo "test"$?
}
"#;
let result = translate_amber_code(code).expect("Couldn't translate Amber code");
println!("Result: {}", result);
assert!(
result.contains("${__sudo} echo"),
"Output should contain sudo followed by a space and echo, but got: {}",
result
);
}Test Output:
---- tests::repro_sudo::tests::test_translate_with_sudo_space stdout ----
Result: ${__sudo}echo "test"
...
thread 'tests::repro_sudo::tests::test_translate_with_sudo_space' panicked at src/tests/repro_sudo.rs:29:9:
Output should contain sudo followed by a space and echo, but got: ${__sudo}echo "test"
2. Concatenation logic in fragments!
The fragments! macro in src/translate/fragments/mod.rs uses ListFragment::new() which defaults to an empty string separator.
// src/translate/fragments/list.rs
00018| pub fn new(value: Vec<FragmentKind>) -> Self {
00019| ListFragment {
00020| values: value,
00021| separator: ListFragmentSeparator::Empty, // <-- Separator is empty by default
00022| }
00023| }3. Digit redirection bug with silent
If a command ends in a digit (like echo 1), the silent modifier (which expands to >/dev/null 2>&1 without a leading space) will cause Bash to interpret the digit as a file descriptor.
Reproduction Test:
#[test]
fn test_translate_with_silent_space() {
let code = r#"
main {
silent $echo 1$?
}
"#;
let result = translate_amber_code(code).expect("Couldn't translate Amber code");
assert!(result.contains("echo 1 >/dev/null"), "Output should contain a space before >/dev/null, but got: {}", result);
}Result: echo 1>/dev/null 2>&1 (Fails because Bash interprets 1> as redirecting stdout, but if the command was echo 2, echo 2>/dev/null would redirect stderr and print nothing to stdout, which is incorrect for echo 2).
Why has this bug gone undetected?
Existing tests for sudo (like test_translate_with_sudo in src/tests/compiling.rs) only check if the output contains "sudo", but not if it's correctly separated from the command. Most commands used in tests don't end in digits, so the silent redirection bug is also easily missed.
Recommended fix
Use ListFragment::new().with_spaces() to ensure all components are separated by spaces, or explicitly add spaces in the fragments! call.
// src/modules/command/cmd.rs
let translation = ListFragment::new(vec![sudo_prefix, translation, suppress, silent])
.with_spaces() // <-- FIX 🟢
.to_frag();History
This bug was introduced in commit 82e51f1 (@callframe, 2026-02-17, PR #1017). During a refactor to support TextPart and fix string interpolation issues, the Command::translate method was updated to use the fragments! macro instead of ListFragment::new(...).with_spaces(), which caused command components to be concatenated without necessary separating spaces.