[sandbox audit] Make the process id protocol unambiguous

README.md

@@ -45,10 +45,11 @@ ABI-6 abstract-socket scoping).
 GET  /health                              → {"status","version","uptime_ms"}   (no auth)
 POST /v1/exec        {cmd, shell?, env?, cwd?, timeout?, stdin?, background?, tag?}
                      foreground → NDJSON stream: start / stdout / stderr / ping / exit
-                     background → {"pid", "tag"}
+                     background → {"id", "pid", "tag"}
 POST /v1/processes   {cmd, shell?, env?, cwd?, tag?}   → {"id", "pid", "cmd", "tag"}  (background)
 GET  /v1/processes                        → [{"id","pid","cmd","tag","running","exit_code",...}]
-DELETE /v1/processes/{id}                 → {"id","ok"}   (terminate + forget; idempotent)
+DELETE /v1/processes/{id}                 → {"id","killed"}  (terminate + forget; idempotent.
+                                             `id` is the opaque id, never the OS pid)
 GET  /v1/files/read?path=&offset=&length= → raw bytes
 PUT  /v1/files/write?path=&mode=&offset=  → raw body to file (parents created)
 GET  /v1/files/list?path=  /stat?path=
@@ -131,8 +132,6 @@ sandbox, a real VM) for mutually distrusting code.
 - **A hijacked proxy connection is authenticated and routed only once**, then bytes are
   spliced until EOF. A second HTTP request written on that connection reaches the first
   backend without new routing, depending on the upstream proxy's behaviour.
-- **`DELETE /v1/processes/{id}` answers 200 for an unknown id**, so addressing a process by
-  OS pid (as the current client does) silently does nothing.
 - **Uids are never recycled**, so a host that has created ~45k sandboxes over its lifetime can
   no longer create more, even when empty.
 - **A `setsid` descendant outlives a per-process `kill`** (it leaves the signalled process

src/exec.rs

@@ -603,7 +603,13 @@ pub fn handle_exec(
 
     if spec.background {
         return match start_background(state, &spec) {
-            Ok(proc) => resp.json(200, &serde_json::json!({"pid": proc.pid, "tag": proc.tag})),
+            // `id` is the only identifier `DELETE /processes/{id}` accepts, so
+            // omitting it made a process started this way unstoppable through
+            // the documented protocol.
+            Ok(proc) => resp.json(
+                200,
+                &serde_json::json!({"id": proc.id, "pid": proc.pid, "tag": proc.tag}),
+            ),
             Err(e) => resp.error(400, &e),
         };
     }
@@ -674,23 +680,49 @@ pub fn handle_process_start(
     }
 }
 
+/// Whether `id` has the shape of a server-assigned process id (`p-<n>`).
+///
+/// A bare number is almost certainly an OS pid, which this route has never
+/// accepted. Saying so beats treating it as an unknown id, because the two
+/// answers used to be indistinguishable: a client sending a pid got `200` and
+/// concluded the process was stopped.
+fn is_opaque_process_id(id: &str) -> bool {
+    id.strip_prefix("p-").is_some_and(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit()))
+}
+
 /// `DELETE /processes/{id}` — terminate a background process by its opaque id and
-/// forget it. Idempotent: an unknown id (or one owned by another sandbox) is a no-op.
+/// forget it.
+///
+/// Still idempotent, but no longer indiscriminate: the reply distinguishes "I
+/// killed it" from "there was nothing by that id", and a malformed id is an
+/// error rather than a cheerful no-op.
 pub fn handle_process_delete(
     state: &Arc<State>,
     id: &str,
     sandbox_id: Option<&str>,
     resp: &mut ResponseWriter,
 ) -> std::io::Result<()> {
-    if let Some(proc) = state.procs.remove_by_id(id, sandbox_id) {
-        // Only signal while the process is still ours to signal: once it has
-        // exited, its PID (and therefore its pgid) may belong to something else.
-        let exited = proc.state.lock().unwrap().exit.is_some();
-        if !exited {
-            kill_group(proc.pid, libc::SIGKILL);
-        }
+    if !is_opaque_process_id(id) {
+        return resp.error(
+            400,
+            &format!(
+                "'{id}' is not a process id. Use the opaque `id` from the create response \
+                 (e.g. 'p-3'), not the OS pid."
+            ),
+        );
     }
-    resp.json(200, &serde_json::json!({"id": id, "ok": true}))
+    let Some(proc) = state.procs.remove_by_id(id, sandbox_id) else {
+        // Already reaped, already deleted, or never existed. Idempotent, but say
+        // which so a caller can tell "stopped it" from "it was already gone".
+        return resp.json(200, &serde_json::json!({"id": id, "killed": false}));
+    };
+    // Only signal while the process is still ours to signal: once it has
+    // exited, its PID (and therefore its pgid) may belong to something else.
+    let exited = proc.state.lock().unwrap().exit.is_some();
+    if !exited {
+        kill_group(proc.pid, libc::SIGKILL);
+    }
+    resp.json(200, &serde_json::json!({"id": id, "killed": !exited}))
 }
 
 fn wait_detached(
@@ -701,3 +733,55 @@ fn wait_detached(
 ) {
     std::thread::spawn(move || wait_and_report(command, started_at, timeout_secs, tx));
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// The client sent an OS pid where the server expects its own opaque id, and
+    /// the server answered 200 either way -- so a `kill()` that did nothing
+    /// looked like a success. Rejecting the wrong shape is what makes that
+    /// class of mistake loud instead of silent.
+    #[test]
+    fn only_server_assigned_ids_are_accepted() {
+        for id in ["p-0", "p-1", "p-42", "p-9007199254740993"] {
+            assert!(is_opaque_process_id(id), "{id} should be accepted");
+        }
+        for id in ["", "p-", "p", "0", "42", "9000", "-1", "p-1a", "p--1", "P-1", " p-1", "p-1 "] {
+            assert!(!is_opaque_process_id(id), "{id:?} should be rejected");
+        }
+    }
+
+    #[test]
+    fn allocated_ids_have_the_accepted_shape() {
+        let registry = ProcRegistry::default();
+        for _ in 0..3 {
+            let id = registry.alloc_id();
+            assert!(is_opaque_process_id(&id), "allocated {id} but the route would reject it");
+        }
+    }
+
+    #[test]
+    fn ids_are_unique_and_scoped_lookup_does_not_cross_sandboxes() {
+        let registry = ProcRegistry::default();
+        let first = registry.alloc_id();
+        let second = registry.alloc_id();
+        assert_ne!(first, second);
+
+        let proc = Arc::new(Proc {
+            id: first.clone(),
+            pid: 1,
+            tag: None,
+            cmd_json: serde_json::Value::Null,
+            started_at_ms: 0,
+            sandbox_id: Some("mine".to_string()),
+            state: Mutex::new(ProcState { exit: None }),
+        });
+        registry.insert(Arc::clone(&proc));
+
+        assert!(registry.remove_by_id(&first, Some("theirs")).is_none(), "a sibling reached it");
+        assert!(registry.remove_by_id(&second, Some("mine")).is_none(), "an unknown id matched");
+        assert!(registry.remove_by_id(&first, Some("mine")).is_some());
+        assert!(registry.remove_by_id(&first, Some("mine")).is_none(), "removal is not idempotent");
+    }
+}