fix(server): normalise process identity on policy update for custom-image sandboxes

crates/navigator-cli/src/run.rs

@@ -1517,18 +1517,7 @@ pub async fn sandbox_create(
     )
     .await?;
 
-    let mut policy = load_sandbox_policy(policy)?;
-
-    // When a custom image is specified, clear the default run_as_user/group
-    // to prevent failures on images that lack the "sandbox" user/group.
-    // If no explicit policy was provided, start from the restrictive default
-    // so the server stores a policy with cleared identity — otherwise the
-    // sandbox falls back to disk discovery which may re-introduce
-    // run_as_user: sandbox for images that don't have that user.
-    if image.is_some() {
-        let p = policy.get_or_insert_with(navigator_policy::restrictive_default_policy);
-        navigator_policy::clear_process_identity(p);
-    }
+    let policy = load_sandbox_policy(policy)?;
 
     let template = image.map(|img| SandboxTemplate {
         image: img,

crates/navigator-policy/src/lib.rs

@@ -363,14 +363,20 @@ pub fn restrictive_default_policy() -> SandboxPolicy {
     }
 }
 
-/// Clear `run_as_user` / `run_as_group` from the policy's process section.
+/// Ensure the policy has `run_as_user: sandbox` and `run_as_group: sandbox`.
 ///
-/// Call this when a custom image is specified, since the image may lack the
-/// default "sandbox" user/group.
-pub fn clear_process_identity(policy: &mut SandboxPolicy) {
-    if let Some(ref mut process) = policy.process {
-        process.run_as_user = String::new();
-        process.run_as_group = String::new();
+/// If the process section is missing, or either field is empty, this fills in
+/// the required `"sandbox"` value. Call this before validation so that
+/// policies without an explicit process section get the correct default.
+pub fn ensure_sandbox_process_identity(policy: &mut SandboxPolicy) {
+    let process = policy
+        .process
+        .get_or_insert_with(|| ProcessPolicy::default());
+    if process.run_as_user.is_empty() {
+        process.run_as_user = "sandbox".into();
+    }
+    if process.run_as_group.is_empty() {
+        process.run_as_group = "sandbox".into();
     }
 }
 
@@ -387,8 +393,8 @@ const MAX_PATH_LENGTH: usize = 4096;
 /// A safety violation found in a sandbox policy.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum PolicyViolation {
-    /// `run_as_user` or `run_as_group` is "root" or "0".
-    RootProcessIdentity { field: &'static str, value: String },
+    /// `run_as_user` or `run_as_group` is not "sandbox".
+    InvalidProcessIdentity { field: &'static str, value: String },
     /// A filesystem path contains `..` components.
     PathTraversal { path: String },
     /// A filesystem path is not absolute (does not start with `/`).
@@ -404,8 +410,8 @@ pub enum PolicyViolation {
 impl fmt::Display for PolicyViolation {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Self::RootProcessIdentity { field, value } => {
-                write!(f, "{field} cannot be '{value}' (root is not allowed)")
+            Self::InvalidProcessIdentity { field, value } => {
+                write!(f, "{field} must be 'sandbox', got '{value}'")
             }
             Self::PathTraversal { path } => {
                 write!(f, "path contains '..' traversal component: {path}")
@@ -439,7 +445,7 @@ impl fmt::Display for PolicyViolation {
 /// error vs. logged warning).
 ///
 /// Checks performed:
-/// - `run_as_user` / `run_as_group` must not be "root" or "0"
+/// - `run_as_user` / `run_as_group` must be "sandbox"
 /// - Filesystem paths must be absolute (start with `/`)
 /// - Filesystem paths must not contain `..` components
 /// - Read-write paths must not be overly broad (just `/`)
@@ -450,16 +456,18 @@ pub fn validate_sandbox_policy(
 ) -> std::result::Result<(), Vec<PolicyViolation>> {
     let mut violations = Vec::new();
 
-    // Check process identity
+    // Check process identity — must be "sandbox".
+    // `ensure_sandbox_process_identity` should be called before this to
+    // fill in defaults; anything other than "sandbox" is rejected.
     if let Some(ref process) = policy.process {
-        if is_root_identity(&process.run_as_user) {
-            violations.push(PolicyViolation::RootProcessIdentity {
+        if process.run_as_user != "sandbox" {
+            violations.push(PolicyViolation::InvalidProcessIdentity {
                 field: "run_as_user",
                 value: process.run_as_user.clone(),
             });
         }
-        if is_root_identity(&process.run_as_group) {
-            violations.push(PolicyViolation::RootProcessIdentity {
+        if process.run_as_group != "sandbox" {
+            violations.push(PolicyViolation::InvalidProcessIdentity {
                 field: "run_as_group",
                 value: process.run_as_group.clone(),
             });
@@ -519,15 +527,6 @@ pub fn validate_sandbox_policy(
     }
 }
 
-/// Check if a user/group identity string refers to root.
-fn is_root_identity(value: &str) -> bool {
-    if value.is_empty() {
-        return false;
-    }
-    let trimmed = value.trim();
-    trimmed == "root" || trimmed == "0"
-}
-
 /// Truncate a string for safe inclusion in error messages.
 fn truncate_for_display(s: &str) -> String {
     if s.len() <= 80 {
@@ -724,13 +723,35 @@ network_policies:
     }
 
     #[test]
-    fn clear_process_identity_clears_fields() {
+    fn ensure_sandbox_process_identity_fills_defaults() {
         let mut policy = restrictive_default_policy();
-        assert_eq!(policy.process.as_ref().unwrap().run_as_user, "sandbox");
-        clear_process_identity(&mut policy);
+        policy.process = None;
+        ensure_sandbox_process_identity(&mut policy);
+        let proc = policy.process.unwrap();
+        assert_eq!(proc.run_as_user, "sandbox");
+        assert_eq!(proc.run_as_group, "sandbox");
+    }
+
+    #[test]
+    fn ensure_sandbox_process_identity_fills_empty_strings() {
+        let mut policy = restrictive_default_policy();
+        policy.process = Some(ProcessPolicy {
+            run_as_user: String::new(),
+            run_as_group: String::new(),
+        });
+        ensure_sandbox_process_identity(&mut policy);
         let proc = policy.process.unwrap();
-        assert!(proc.run_as_user.is_empty());
-        assert!(proc.run_as_group.is_empty());
+        assert_eq!(proc.run_as_user, "sandbox");
+        assert_eq!(proc.run_as_group, "sandbox");
+    }
+
+    #[test]
+    fn ensure_sandbox_process_identity_preserves_sandbox() {
+        let mut policy = restrictive_default_policy();
+        ensure_sandbox_process_identity(&mut policy);
+        let proc = policy.process.unwrap();
+        assert_eq!(proc.run_as_user, "sandbox");
+        assert_eq!(proc.run_as_group, "sandbox");
     }
 
     #[test]
@@ -750,7 +771,7 @@ network_policies:
         let violations = validate_sandbox_policy(&policy).unwrap_err();
         assert!(violations.iter().any(|v| matches!(
             v,
-            PolicyViolation::RootProcessIdentity {
+            PolicyViolation::InvalidProcessIdentity {
                 field: "run_as_user",
                 ..
             }
@@ -768,6 +789,28 @@ network_policies:
         assert_eq!(violations.len(), 2);
     }
 
+    #[test]
+    fn validate_rejects_non_sandbox_user() {
+        let mut policy = restrictive_default_policy();
+        policy.process = Some(ProcessPolicy {
+            run_as_user: "nobody".into(),
+            run_as_group: "nogroup".into(),
+        });
+        let violations = validate_sandbox_policy(&policy).unwrap_err();
+        assert_eq!(violations.len(), 2);
+        assert!(
+            violations
+                .iter()
+                .all(|v| matches!(v, PolicyViolation::InvalidProcessIdentity { .. }))
+        );
+    }
+
+    #[test]
+    fn validate_accepts_sandbox_identity() {
+        let policy = restrictive_default_policy();
+        assert!(validate_sandbox_policy(&policy).is_ok());
+    }
+
     #[test]
     fn validate_rejects_path_traversal() {
         let mut policy = restrictive_default_policy();
@@ -835,13 +878,14 @@ network_policies:
     }
 
     #[test]
-    fn validate_accepts_empty_run_as_user() {
+    fn validate_rejects_empty_run_as_user() {
         let mut policy = restrictive_default_policy();
         policy.process = Some(ProcessPolicy {
             run_as_user: String::new(),
             run_as_group: String::new(),
         });
-        assert!(validate_sandbox_policy(&policy).is_ok());
+        let violations = validate_sandbox_policy(&policy).unwrap_err();
+        assert_eq!(violations.len(), 2);
     }
 
     #[test]
@@ -893,12 +937,13 @@ network_policies:
 
     #[test]
     fn policy_violation_display() {
-        let v = PolicyViolation::RootProcessIdentity {
+        let v = PolicyViolation::InvalidProcessIdentity {
             field: "run_as_user",
             value: "root".into(),
         };
         let s = format!("{v}");
         assert!(s.contains("root"));
         assert!(s.contains("run_as_user"));
+        assert!(s.contains("sandbox"));
     }
 }

crates/navigator-sandbox/src/lib.rs

@@ -163,7 +163,7 @@ pub async fn run_sandbox(
 
     // Load policy and initialize OPA engine
     let navigator_endpoint_for_proxy = navigator_endpoint.clone();
-    let (mut policy, opa_engine) = load_policy(
+    let (policy, opa_engine) = load_policy(
         sandbox_id.clone(),
         sandbox,
         navigator_endpoint.clone(),
@@ -172,13 +172,10 @@ pub async fn run_sandbox(
     )
     .await?;
 
-    // When no run_as_user/group is configured (e.g. custom/community images
-    // where the CLI cleared identity) and the supervisor is running as root
-    // (forced via runAsUser: 0 in the pod spec), auto-detect the image's
-    // intended sandbox user. This prevents child processes from running as
-    // root unnecessarily.
+    // Validate that the required "sandbox" user exists in this image.
+    // All sandbox images must include this user for privilege dropping.
     #[cfg(unix)]
-    maybe_infer_sandbox_user(&mut policy);
+    validate_sandbox_user(&policy)?;
 
     // Fetch provider environment variables from the server.
     // This is done after loading the policy so the sandbox can still start
@@ -1026,61 +1023,36 @@ fn discover_policy_from_path(path: &std::path::Path) -> navigator_core::proto::S
     }
 }
 
-/// When the policy has no `run_as_user`/`run_as_group` and the supervisor is
-/// running as root, try to detect the image's intended non-root user for
-/// child process privilege dropping.
+/// Validate that the `sandbox` user exists in this image.
 ///
-/// This handles the BYOC / community-image case where the CLI clears
-/// `run_as_user` (to avoid failures if the default "sandbox" user doesn't
-/// exist), but the K8s pod spec forces `runAsUser: 0` so the supervisor can
-/// perform privileged setup. Without this, child processes would inherit
-/// root — undermining the image author's security intent.
-///
-/// Detection strategy: look for a `sandbox` user in `/etc/passwd`. This is
-/// the conventional non-root user in OpenShell sandbox images.
+/// All sandbox images must include a `sandbox` user for privilege dropping.
+/// This check runs at supervisor startup (inside the container) where we can
+/// inspect `/etc/passwd`. If the user is missing, the sandbox fails fast
+/// with a clear error instead of silently running child processes as root.
 #[cfg(unix)]
-fn maybe_infer_sandbox_user(policy: &mut SandboxPolicy) {
-    use nix::unistd::{Uid, User};
+fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> {
+    use nix::unistd::User;
 
-    // Only infer when both fields are empty and we're running as root.
-    let has_user = policy
-        .process
-        .run_as_user
-        .as_ref()
-        .is_some_and(|u| !u.is_empty());
-    let has_group = policy
-        .process
-        .run_as_group
-        .as_ref()
-        .is_some_and(|g| !g.is_empty());
-    if has_user || has_group || !Uid::effective().is_root() {
-        return;
-    }
+    let user_name = policy.process.run_as_user.as_deref().unwrap_or("sandbox");
 
-    // Check if a "sandbox" user exists in the image.
-    match User::from_name("sandbox") {
-        Ok(Some(_)) => {
-            info!(
-                "Policy has no run_as_user but image has 'sandbox' user; \
-                 inferring run_as_user=sandbox, run_as_group=sandbox"
-            );
-            policy.process.run_as_user = Some("sandbox".to_string());
-            policy.process.run_as_group = Some("sandbox".to_string());
-        }
-        Ok(None) => {
-            warn!(
-                "Policy has no run_as_user and image has no 'sandbox' user; \
-                 child processes will run as root. Set process.run_as_user in \
-                 your sandbox policy to specify a non-root user."
-            );
-        }
-        Err(e) => {
-            warn!(
-                error = %e,
-                "Failed to look up 'sandbox' user; child processes will run as root"
-            );
+    if user_name.is_empty() || user_name == "sandbox" {
+        match User::from_name("sandbox") {
+            Ok(Some(_)) => {
+                info!("Validated 'sandbox' user exists in image");
+            }
+            Ok(None) => {
+                return Err(miette::miette!(
+                    "sandbox user 'sandbox' not found in image; \
+                     all sandbox images must include a 'sandbox' user and group"
+                ));
+            }
+            Err(e) => {
+                return Err(miette::miette!("failed to look up 'sandbox' user: {e}"));
+            }
         }
     }
+
+    Ok(())
 }
 
 /// Prepare filesystem for the sandboxed process.