Skip to main content

max / makenotwork

server: parse build command into escaped argv instead of raw shell interpolation The operator build_command was validated by a shell-metacharacter denylist and then interpolated raw into the remote sh -c script — a denylist wearing allowlist clothing, one added character away from reopening injection. Replace it with a RemoteCommand parser: the command is tokenized into leading NAME=VALUE env assignments plus a program and args, then rendered with every token individually shell-escaped and env applied via `env`. No operator- or record-derived byte reaches the shell unescaped, so injection is structurally impossible rather than denylist-gated. validate_build_command now delegates to the same parser, so a config that validates at write time can never fail to render safely. Also extract the duplicated SSH/SCP host-key options into ssh_host_key_args, which logs a warning when known_hosts is absent (the trust-on-first-use fallback is now visible in the logs rather than silent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-02 22:04 UTC
Commit: a8bd6eb2be8c7e89c87eb7ce7e20318b36873083
Parent: 376071e
1 file changed, +199 insertions, -49 deletions
M server/src/build_runner.rs +199 -49
@@ -447,16 +447,17 @@ async fn execute_target(
447 447 .replace("{target}", rust_triple)
448 448 .replace("{version}", &build.version);
449 449
450 - // Validate build_command and artifact_path before interpolation into shell
451 - validate_build_command(&build_cmd)
450 + // Parse build_command into a structured, fully-escaped remote command and
451 + // validate artifact_path before interpolation into the shell script.
452 + let remote_cmd = RemoteCommand::parse(&build_cmd)
452 453 .map_err(|e| format!("invalid build command: {e}"))?;
453 454 validate_artifact_path(&artifact_path)
454 455 .map_err(|e| format!("invalid artifact path: {e}"))?;
455 456
456 - // Build the SSH command sequence
457 - // Note: build_cmd is validated (no shell metacharacters beyond safe set) but
458 - // intentionally NOT shell-escaped since it must execute as a shell command.
459 - // artifact_path is validated AND shell-escaped since it's used as a file path.
457 + // Build the SSH command sequence. Every interpolated value is shell-escaped:
458 + // the git/cd/test scaffolding args via `shell_escape`, and the operator build
459 + // command via `RemoteCommand::render` (each token escaped, env applied via
460 + // `env`). No operator- or record-derived byte reaches the shell unescaped.
460 461 let remote_script = format!(
461 462 "set -e && \
462 463 git clone --depth 1 --branch {tag} {clone_path} {build_dir} && \
@@ -466,7 +467,7 @@ async fn execute_target(
466 467 tag = shell_escape(&build.tag),
467 468 clone_path = shell_escape(&clone_path),
468 469 build_dir = shell_escape(&build_dir),
469 - build_cmd = build_cmd,
470 + build_cmd = remote_cmd.render(),
470 471 artifact_path = shell_escape(&artifact_path),
471 472 );
472 473
@@ -588,19 +589,41 @@ async fn execute_target(
588 589 /// When absent, StrictHostKeyChecking=accept-new (trust on first use).
589 590 const BUILD_SSH_KNOWN_HOSTS: &str = "/etc/mnw/known_hosts";
590 591
591 - /// Run a command on a remote host via SSH.
592 - async fn run_ssh_command(host: &str, command: &str) -> std::result::Result<String, String> {
593 - let mut args = vec!["-o", "ConnectTimeout=10", "-o", "BatchMode=yes"];
594 - let known_hosts_arg;
592 + /// SSH/SCP host-key verification options. Pins host keys
593 + /// (`StrictHostKeyChecking=yes`) when the known_hosts file is present; otherwise
594 + /// falls back to trust-on-first-use AND logs a warning, so an unprovisioned
595 + /// known_hosts is visible in the logs rather than silently accepting any host
596 + /// key. Returned owned so callers can push them straight into an argv vector.
597 + /// Shared by [`run_ssh_command`] and [`run_scp_download`] so the two can't drift.
598 + fn ssh_host_key_args() -> Vec<String> {
595 599 if std::path::Path::new(BUILD_SSH_KNOWN_HOSTS).exists() {
596 - args.extend(["-o", "StrictHostKeyChecking=yes", "-o"]);
597 - known_hosts_arg = format!("UserKnownHostsFile={BUILD_SSH_KNOWN_HOSTS}");
598 - args.push(&known_hosts_arg);
600 + vec![
601 + "-o".into(),
602 + "StrictHostKeyChecking=yes".into(),
603 + "-o".into(),
604 + format!("UserKnownHostsFile={BUILD_SSH_KNOWN_HOSTS}"),
605 + ]
599 606 } else {
600 - args.extend(["-o", "StrictHostKeyChecking=accept-new"]);
607 + tracing::warn!(
608 + known_hosts = BUILD_SSH_KNOWN_HOSTS,
609 + "build SSH known_hosts file absent; falling back to trust-on-first-use (accept-new). \
610 + Provision {BUILD_SSH_KNOWN_HOSTS} to pin build-host keys",
611 + );
612 + vec!["-o".into(), "StrictHostKeyChecking=accept-new".into()]
601 613 }
602 - args.push(host);
603 - args.push(command);
614 + }
615 +
616 + /// Run a command on a remote host via SSH.
617 + async fn run_ssh_command(host: &str, command: &str) -> std::result::Result<String, String> {
618 + let mut args: Vec<String> = vec![
619 + "-o".into(),
620 + "ConnectTimeout=10".into(),
621 + "-o".into(),
622 + "BatchMode=yes".into(),
623 + ];
624 + args.extend(ssh_host_key_args());
625 + args.push(host.to_string());
626 + args.push(command.to_string());
604 627 let mut child = tokio::process::Command::new("ssh")
605 628 .args(&args)
606 629 .stdin(std::process::Stdio::null())
@@ -671,17 +694,15 @@ async fn run_scp_download(
671 694 local_path: &str,
672 695 ) -> std::result::Result<(), String> {
673 696 let remote = format!("{host}:{remote_path}");
674 - let mut args: Vec<&str> = vec!["-o", "ConnectTimeout=10", "-o", "BatchMode=yes"];
675 - let known_hosts_arg;
676 - if std::path::Path::new(BUILD_SSH_KNOWN_HOSTS).exists() {
677 - args.extend(["-o", "StrictHostKeyChecking=yes", "-o"]);
678 - known_hosts_arg = format!("UserKnownHostsFile={BUILD_SSH_KNOWN_HOSTS}");
679 - args.push(&known_hosts_arg);
680 - } else {
681 - args.extend(["-o", "StrictHostKeyChecking=accept-new"]);
682 - }
683 - args.push(&remote);
684 - args.push(local_path);
697 + let mut args: Vec<String> = vec![
698 + "-o".into(),
699 + "ConnectTimeout=10".into(),
700 + "-o".into(),
701 + "BatchMode=yes".into(),
702 + ];
703 + args.extend(ssh_host_key_args());
704 + args.push(remote);
705 + args.push(local_path.to_string());
685 706 let scp = tokio::process::Command::new("scp")
686 707 .args(&args)
687 708 // Kill scp if this future is dropped (build timeout, or the transfer
@@ -761,35 +782,108 @@ fn strip_ansi_escapes(s: &str) -> String {
761 782 result
762 783 }
763 784
764 - /// Validate a build command for shell safety.
785 + /// A build command parsed into a shell-injection-proof structured form.
765 786 ///
766 - /// Rejects shell metacharacters that enable command chaining or redirection.
767 - /// Allowed: alphanumeric, spaces, hyphens, underscores, dots, slashes, equals,
768 - /// braces (for template vars), colons, commas, plus signs.
769 - pub fn validate_build_command(cmd: &str) -> std::result::Result<(), String> {
770 - if cmd.is_empty() {
771 - return Err("build command is empty".to_string());
787 + /// The operator-configured `build_command` is a single string (e.g.
788 + /// `RUSTFLAGS=--cfg cargo build --release`). Rather than interpolate it raw into
789 + /// the remote `sh -c` script — where its safety rested entirely on a
790 + /// metacharacter denylist, one added allowed character away from reopening
791 + /// injection — it is tokenised into leading `NAME=VALUE` environment
792 + /// assignments followed by a program and its arguments. `render` emits every
793 + /// element individually shell-escaped, applying assignments via `env`, so no
794 + /// operator byte can break out of its shell word. Shell injection is
795 + /// structurally impossible here, not denylist-gated; the per-token charset
796 + /// check below is defense-in-depth, no longer the sole guard.
797 + struct RemoteCommand {
798 + /// Leading `NAME=VALUE` assignments, applied via `env` before the program.
799 + assignments: Vec<String>,
800 + /// The program to execute.
801 + program: String,
802 + /// The program's arguments.
803 + args: Vec<String>,
804 + }
805 +
806 + impl RemoteCommand {
807 + /// Parse a (template-substituted) build command string into its structured
808 + /// form. Tokenised on ASCII whitespace; leading `NAME=VALUE` tokens become
809 + /// env assignments, the first remaining token is the program, the rest are
810 + /// arguments. Each token is charset-validated as defense-in-depth.
811 + fn parse(cmd: &str) -> std::result::Result<Self, String> {
812 + if cmd.len() > 1024 {
813 + return Err("build command too long (max 1024 chars)".to_string());
814 + }
815 + let tokens: Vec<&str> = cmd.split_whitespace().collect();
816 + if tokens.is_empty() {
817 + return Err("build command is empty".to_string());
818 + }
819 + for tok in &tokens {
820 + validate_command_token(tok)?;
821 + }
822 +
823 + let mut assignments = Vec::new();
824 + let mut rest = tokens.as_slice();
825 + while let Some((first, tail)) = rest.split_first() {
826 + if is_env_assignment(first) {
827 + assignments.push((*first).to_string());
828 + rest = tail;
829 + } else {
830 + break;
831 + }
832 + }
833 +
834 + let (program, args) = rest
835 + .split_first()
836 + .ok_or_else(|| "build command has environment assignments but no program".to_string())?;
837 +
838 + Ok(Self {
839 + assignments,
840 + program: (*program).to_string(),
841 + args: args.iter().map(|s| (*s).to_string()).collect(),
842 + })
843 + }
844 +
845 + /// Render as a single shell command line with every element escaped. Safe to
846 + /// interpolate into a larger `sh -c` script: no element can inject.
847 + fn render(&self) -> String {
848 + let mut parts = Vec::with_capacity(self.assignments.len() + self.args.len() + 2);
849 + if !self.assignments.is_empty() {
850 + parts.push("env".to_string());
851 + parts.extend(self.assignments.iter().map(|a| shell_escape(a)));
852 + }
853 + parts.push(shell_escape(&self.program));
854 + parts.extend(self.args.iter().map(|a| shell_escape(a)));
855 + parts.join(" ")
772 856 }
773 - if cmd.len() > 1024 {
774 - return Err("build command too long (max 1024 chars)".to_string());
857 + }
858 +
859 + /// True if a token is a `NAME=VALUE` environment assignment (a valid shell
860 + /// identifier before the first `=`).
861 + fn is_env_assignment(tok: &str) -> bool {
862 + match tok.split_once('=') {
863 + Some((name, _)) => {
864 + !name.is_empty()
865 + && name.chars().enumerate().all(|(i, c)| {
866 + c == '_' || c.is_ascii_alphabetic() || (i > 0 && c.is_ascii_digit())
867 + })
868 + }
869 + None => false,
775 870 }
776 - for (i, c) in cmd.chars().enumerate() {
871 + }
872 +
873 + /// Validate a single build-command token's charset. Rejects shell
874 + /// metacharacters and control characters as defense-in-depth; the rendered
875 + /// command escapes every token regardless, so this is not the sole guard.
876 + fn validate_command_token(tok: &str) -> std::result::Result<(), String> {
877 + for (i, c) in tok.chars().enumerate() {
777 878 match c {
778 879 'a'..='z' | 'A'..='Z' | '0'..='9' => {}
779 - ' ' | '-' | '_' | '.' | '/' | '=' | ':' | ',' | '+' | '{' | '}' | '@' => {}
780 - ';' | '&' | '|' | '$' | '`' | '(' | ')' | '<' | '>' | '!' | '\\' | '\'' | '"'
781 - | '\n' | '\r' | '\0' => {
782 - return Err(format!(
783 - "shell metacharacter '{}' at position {} is not allowed",
784 - c.escape_default(),
785 - i
786 - ));
787 - }
880 + '-' | '_' | '.' | '/' | '=' | ':' | ',' | '+' | '{' | '}' | '@' => {}
788 881 _ => {
789 882 return Err(format!(
790 - "unexpected character '{}' at position {} is not allowed",
883 + "character '{}' at position {} in token '{}' is not allowed",
791 884 c.escape_default(),
792 - i
885 + i,
886 + tok.escape_default(),
793 887 ));
794 888 }
795 889 }
@@ -797,6 +891,13 @@ pub fn validate_build_command(cmd: &str) -> std::result::Result<(), String> {
797 891 Ok(())
798 892 }
799 893
894 + /// Validate a build command for shell safety at config-write time. Validation is
895 + /// exactly "parses into a [`RemoteCommand`]" — the same parser the executor uses
896 + /// — so a stored command that validates here can never fail to render safely.
897 + pub fn validate_build_command(cmd: &str) -> std::result::Result<(), String> {
898 + RemoteCommand::parse(cmd).map(|_| ())
899 + }
900 +
800 901 /// Validate an artifact path for shell and path safety.
801 902 ///
802 903 /// Must be a relative path with no shell metacharacters or path traversal.
@@ -927,6 +1028,55 @@ mod tests {
927 1028 assert!(validate_build_command("`whoami`").is_err());
928 1029 assert!(validate_build_command("cargo build > /dev/null").is_err());
929 1030 assert!(validate_build_command("").is_err());
1031 + assert!(validate_build_command(" ").is_err(), "whitespace-only has no program");
1032 + assert!(validate_build_command("FOO=bar").is_err(), "assignment with no program");
1033 + }
1034 +
1035 + #[test]
1036 + fn remote_command_parse_separates_env_program_args() {
1037 + let c = RemoteCommand::parse("cargo build --release").unwrap();
1038 + assert!(c.assignments.is_empty());
1039 + assert_eq!(c.program, "cargo");
1040 + assert_eq!(c.args, vec!["build", "--release"]);
1041 +
1042 + let c = RemoteCommand::parse("RUSTFLAGS=--cfg CARGO_INCREMENTAL=0 cargo build").unwrap();
1043 + assert_eq!(c.assignments, vec!["RUSTFLAGS=--cfg", "CARGO_INCREMENTAL=0"]);
1044 + assert_eq!(c.program, "cargo");
1045 + assert_eq!(c.args, vec!["build"]);
1046 + }
1047 +
1048 + #[test]
1049 + fn remote_command_render_escapes_every_token() {
1050 + // Plain command: each token individually single-quoted.
1051 + let c = RemoteCommand::parse("cargo build --release").unwrap();
1052 + assert_eq!(c.render(), "'cargo' 'build' '--release'");
1053 +
1054 + // Env prefix: applied via `env`, each element escaped.
1055 + let c = RemoteCommand::parse("RUSTFLAGS=--cfg cargo build").unwrap();
1056 + assert_eq!(c.render(), "env 'RUSTFLAGS=--cfg' 'cargo' 'build'");
1057 + }
1058 +
1059 + #[test]
1060 + fn is_env_assignment_recognizes_valid_identifiers_only() {
1061 + assert!(is_env_assignment("FOO=bar"));
1062 + assert!(is_env_assignment("_X1=y"));
1063 + assert!(is_env_assignment("A=")); // empty value is a valid assignment
1064 + assert!(!is_env_assignment("1FOO=bar"), "identifier can't start with a digit");
1065 + assert!(!is_env_assignment("cargo"), "no '='");
1066 + assert!(!is_env_assignment("--target=x"), "not a shell identifier");
1067 + }
1068 +
1069 + #[test]
1070 + fn render_defuses_would_be_injection_even_if_charset_bypassed() {
1071 + // Construct a RemoteCommand directly with a hostile arg (bypassing the
1072 + // token charset check) to prove render() is the real guard: the shell
1073 + // sees a single quoted word, not a command separator.
1074 + let c = RemoteCommand {
1075 + assignments: vec![],
1076 + program: "cargo".to_string(),
1077 + args: vec!["build; rm -rf /".to_string()],
1078 + };
1079 + assert_eq!(c.render(), "'cargo' 'build; rm -rf /'");
930 1080 }
931 1081
932 1082 #[test]