Merge pull request #193 from httpsphl/fix/crew-isolation

fix(security): keep each crew apart from the others and from Botloft's data

Author: httpsphl

crates/botloftd/src/browser/launch.rs

@@ -226,6 +226,20 @@ fn log_text(line: &str) -> String {
     text.trim().to_owned()
 }
 
+/// The DevTools port written in each profile under `profiles`.
+pub(super) fn ports_in(profiles: &Path) -> Vec<u16> {
+    let Ok(profiles) = std::fs::read_dir(profiles) else {
+        return Vec::new();
+    };
+    profiles
+        .flatten()
+        .filter_map(|profile| {
+            let text = std::fs::read_to_string(profile.path().join(PORT_FILE)).ok()?;
+            text.lines().next()?.trim().parse().ok()
+        })
+        .collect()
+}
+
 /// `DevToolsActivePort` holds the port and the browser's WebSocket path, one
 /// per line. The file may be read half-written.
 fn devtools_url(text: &str) -> Option<String> {
@@ -240,6 +254,23 @@ fn devtools_url(text: &str) -> Option<String> {
 mod tests {
     use super::*;
 
+    #[test]
+    fn every_profile_gives_its_port() {
+        let dir = tempfile::tempdir().expect("tempdir");
+        for (bot, text) in [
+            ("bot_a", "9222\n/devtools/browser/a\n"),
+            ("bot_b", "61000\n"),
+        ] {
+            std::fs::create_dir(dir.path().join(bot)).expect("profile");
+            std::fs::write(dir.path().join(bot).join(PORT_FILE), text).expect("port");
+        }
+        std::fs::create_dir(dir.path().join("bot_c")).expect("no port yet");
+        let mut ports = ports_in(dir.path());
+        ports.sort_unstable();
+        assert_eq!(ports, [9222, 61000]);
+        assert!(ports_in(&dir.path().join("missing")).is_empty());
+    }
+
     #[test]
     fn reads_the_port_file() {
         assert_eq!(

crates/botloftd/src/browser/mod.rs

@@ -161,6 +161,12 @@ impl Browsers {
         }
     }
 
+    /// The DevTools ports of the bots' browsers, read from every profile:
+    /// no page may open them (spec 21.5). A stale file only adds a port.
+    pub fn devtools_ports(&self) -> Vec<u16> {
+        launch::ports_in(&self.profiles)
+    }
+
     fn slot<'a>(&self, slots: &'a mut HashMap<BotId, Slot>, bot: &BotId) -> &'a mut Slot {
         slots.entry(bot.clone()).or_insert_with(|| Slot {
             calls: Arc::default(),

crates/botloftd/src/browser/sites.rs

@@ -107,6 +107,44 @@ pub fn site_of(url: &str) -> Option<String> {
     (!host.is_empty()).then(|| host.to_owned())
 }
 
+/// The port of a loopback address (`localhost`, `127.x.x.x`, `[::1]` or
+/// `0.0.0.0`), or the scheme's own when none is written. `None` for any
+/// other address.
+pub fn loopback_port(url: &str) -> Option<u16> {
+    let lower = url.to_ascii_lowercase();
+    let (rest, default) = match lower.strip_prefix("https://") {
+        Some(rest) => (rest, 443),
+        None => (lower.strip_prefix("http://")?, 80),
+    };
+    let authority = rest.split(['/', '?', '#']).next()?;
+    let host_port = authority.rsplit('@').next()?;
+    let (host, port) = match host_port.strip_prefix('[') {
+        Some(v6) => {
+            let (host, after) = v6.split_once(']')?;
+            (host, after.strip_prefix(':'))
+        }
+        None => match host_port.split_once(':') {
+            Some((host, port)) => (host, Some(port)),
+            None => (host_port, None),
+        },
+    };
+    let host = host.trim_end_matches('.');
+    let loopback = host == "localhost"
+        || host.ends_with(".localhost")
+        || host == "::1"
+        || host == "0.0.0.0"
+        || host
+            .parse::<std::net::Ipv4Addr>()
+            .is_ok_and(|ip| ip.is_loopback());
+    if !loopback {
+        return None;
+    }
+    match port {
+        None | Some("") => Some(default),
+        Some(port) => port.parse().ok(),
+    }
+}
+
 /// Whether allowing `allowed` also allows `site`.
 pub fn covers(allowed: &str, site: &str) -> bool {
     site == allowed

crates/botloftd/src/browser/sites_tests.rs

@@ -140,3 +140,17 @@ fn what_the_owner_types_opens_only_as_a_web_address() {
     assert_eq!(open("https://"), None);
     assert_eq!(open(""), None);
 }
+
+#[test]
+fn loopback_addresses_give_their_port() {
+    assert_eq!(loopback_port("http://127.0.0.1:9222/json/list"), Some(9222));
+    assert_eq!(loopback_port("http://LOCALHOST:3000"), Some(3000));
+    assert_eq!(loopback_port("http://a@127.1.2.3:4000/"), Some(4000));
+    assert_eq!(loopback_port("http://[::1]:9222"), Some(9222));
+    assert_eq!(loopback_port("http://0.0.0.0:80"), Some(80));
+    assert_eq!(loopback_port("https://app.localhost/x"), Some(443));
+    assert_eq!(loopback_port("http://localhost"), Some(80));
+    assert_eq!(loopback_port("http://example.com:9222"), None);
+    assert_eq!(loopback_port("http://127.0.0.1.example.com:9222"), None);
+    assert_eq!(loopback_port("file:///C:/x.html"), None);
+}

crates/botloftd/src/service/bots.rs

@@ -114,7 +114,9 @@ pub(crate) fn insert(
         created_at: now_ms(),
         archived_at: None,
     };
-    workspace::prepare_bot(daemon.workspace_env(), crew, &record).map_err(ApiError::Workspace)?;
+    let crews = store.crews(true)?;
+    workspace::prepare_bot(daemon.workspace_env(), crew, &crews, &record)
+        .map_err(ApiError::Workspace)?;
     store.insert_bot(&record)?;
     Ok(record)
 }

crates/botloftd/src/service/crews.rs

@@ -32,7 +32,7 @@ pub fn create(daemon: &Daemon, params: CrewsCreateParams) -> ApiResult<Crew> {
     let chosen = params
         .work_folder
         .as_deref()
-        .map(|input| choose_folder(daemon, input))
+        .map(|input| choose_folder(daemon, &store, &slug, None, input))
         .transpose()?;
 
     let lead = params
@@ -85,13 +85,13 @@ pub fn rename(daemon: &Daemon, params: CrewsRenameParams) -> ApiResult<Crew> {
 /// Moves the crew to another work folder, or back to `shared`. Each bot
 /// restarts to reach it once nothing is in progress (spec 7.4).
 pub fn set_work_folder(daemon: &Daemon, params: CrewsSetWorkFolderParams) -> ApiResult<Crew> {
+    let store = daemon.store();
+    let mut crew = active(&store, &params.crew_id)?;
     let chosen = params
         .work_folder
         .as_deref()
-        .map(|input| choose_folder(daemon, input))
+        .map(|input| choose_folder(daemon, &store, &crew.slug, Some(&crew.id), input))
         .transpose()?;
-    let store = daemon.store();
-    let mut crew = active(&store, &params.crew_id)?;
     if crew.work_folder_chosen == chosen.is_some()
         && chosen
             .as_deref()
@@ -176,8 +176,22 @@ pub(crate) fn changed(daemon: &Daemon, crew: Crew) -> Crew {
     crew
 }
 
-fn choose_folder(daemon: &Daemon, input: &str) -> ApiResult<String> {
-    let path = workspace::folder::choose(&daemon.paths, input).map_err(ApiError::validation)?;
+/// A work folder of the crew `slug` (`id` once it exists), apart from every
+/// other crew's folders.
+fn choose_folder(
+    daemon: &Daemon,
+    store: &Store,
+    slug: &str,
+    id: Option<&CrewId>,
+    input: &str,
+) -> ApiResult<String> {
+    let others: Vec<Crew> = store
+        .crews(true)?
+        .into_iter()
+        .filter(|crew| Some(&crew.id) != id)
+        .collect();
+    let path = workspace::folder::choose(&daemon.paths, slug, &others, input)
+        .map_err(ApiError::validation)?;
     Ok(path.display().to_string())
 }
 

crates/botloftd/src/service/files/mod.rs

@@ -27,23 +27,38 @@ const PREVIEW_MAX_BYTES: u64 = 20 * 1024 * 1024;
 /// Generated for the bot by the daemon: not something it made for the owner.
 const WORKSPACE_SKIP: [&str; 2] = ["attachments", "CLAUDE.md"];
 
-/// Where a bot's files live, and what it wrote by name.
+/// Where a bot's files live, what it wrote by name, and where its writes
+/// never count: Botloft's data and the other crews' folders (spec 7.5).
 struct Places {
     work: PathBuf,
     workspace: PathBuf,
     written: Vec<String>,
+    fenced: Vec<PathBuf>,
+}
+
+impl Places {
+    /// A file the bot wrote, unless it is behind a fence.
+    fn wrote(&self, path: &Path) -> bool {
+        !self
+            .fenced
+            .iter()
+            .filter_map(|folder| folder.canonicalize().ok())
+            .any(|folder| path.starts_with(folder))
+    }
 }
 
 fn places(daemon: &Daemon, bot_id: &BotId) -> ApiResult<(BotRecord, Places)> {
     let store = daemon.store();
     let bot = bots::find(&store, bot_id)?;
     let crew = crews::find(&store, &bot.crew_id)?;
     let written = store.written_files(&bot.id, WRITTEN_MAX)?;
+    let crews = store.crews(true)?;
     drop(store);
     let places = Places {
         work: daemon.paths.work_folder(&crew),
         workspace: daemon.paths.bot_workspace(&crew.slug, &bot.slug),
         written,
+        fenced: crate::workspace::fences(&daemon.paths, &crew, &crews),
     };
     Ok((bot, places))
 }
@@ -74,6 +89,7 @@ pub fn list(daemon: &Daemon, params: FilesListParams) -> ApiResult<Vec<BotFile>>
         let path = PathBuf::from(path);
         if let Ok(meta) = std::fs::metadata(&path)
             && meta.is_file()
+            && path.canonicalize().is_ok_and(|real| places.wrote(&real))
         {
             found.entry(key(&path)).or_insert_with(|| Found {
                 size: meta.len(),
@@ -141,7 +157,8 @@ fn reach(places: &Places, asked: &Path) -> Result<(PathBuf, std::fs::Metadata),
         .written
         .iter()
         .filter_map(|file| Path::new(file).canonicalize().ok())
-        .any(|file| file == path);
+        .any(|file| file == path)
+        && places.wrote(&path);
     if !asked.is_absolute() || !(inside || written) {
         return Err(Refused::Outside);
     }

crates/botloftd/src/supervisor/spawn.rs

@@ -107,7 +107,8 @@ pub(super) fn launch_spec(
     session: Option<&str>,
 ) -> io::Result<(SpawnSpec, Launch)> {
     // settings.json and the rules are rewritten on every start.
-    let workspace = workspace::prepare_bot(daemon.workspace_env(), crew, bot)?;
+    let crews = daemon.store().crews(true).map_err(io::Error::other)?;
+    let workspace = workspace::prepare_bot(daemon.workspace_env(), crew, &crews, bot)?;
     let token = secrets::random_token()?;
     let (session, resumed) = match session {
         Some(session) => (session.to_owned(), true),

crates/botloftd/src/tools/browser_sites.rs

@@ -13,6 +13,15 @@ use crate::approvals::{self, Answer};
 use crate::browser::{Session, sites};
 use crate::state::Daemon;
 
+const CONTROL_PORT: &str = "That address is the control port of a browser Botloft runs, which no \
+    page may open, in any mode.";
+
+/// Whether `url` is the DevTools port of a bot's browser, its own or
+/// another crew's (spec 21.5).
+fn control_port(daemon: &Daemon, url: &str) -> bool {
+    sites::loopback_port(url).is_some_and(|port| daemon.browsers.devtools_ports().contains(&port))
+}
+
 /// Whether the bot may use `site`, asking the owner the first time when
 /// its mode asks first (spec 21.5).
 pub(super) async fn allowed(
@@ -23,6 +32,9 @@ pub(super) async fn allowed(
     site: &str,
     url: &str,
 ) -> Result<(), String> {
+    if control_port(daemon, url) {
+        return Err(CONTROL_PORT.to_owned());
+    }
     if matches!(
         bot.mode,
         PermissionMode::Auto | PermissionMode::BypassPermissions
@@ -71,6 +83,10 @@ pub(super) async fn page_allowed(
     if sites::is_file(url) {
         return outside_file(session, bot, url).await;
     }
+    if control_port(daemon, url) {
+        let _ = session.open("about:blank").await;
+        return Err(format!("{CONTROL_PORT} It was closed."));
+    }
     match sites::site_of(url) {
         Some(site) => allowed(daemon, id, generation, bot, &site, url).await,
         None => Ok(()),

crates/botloftd/src/workspace/files.rs

@@ -1,6 +1,6 @@
 //! Contents of the files the daemon generates in a bot's workspace.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::time::Duration;
 
 use botloft_core::protocol::Crew;
@@ -15,14 +15,19 @@ use crate::paths::permission_rule_path;
 const MCP_TIMEOUT_MARGIN: Duration = Duration::from_secs(120);
 
 /// `.claude/settings.json` (spec 7.5): keeps the bot out of the daemon's
-/// secrets, and the owner's own Claude Code memory out of the bot. Deny
-/// rules apply even in a folder nobody trusted.
-pub fn settings_json(botloft_home: &Path, crew_dir: &Path) -> Value {
+/// secrets, out of `fenced` (Botloft's data and the other crews' folders,
+/// see `fences`) and the owner's own Claude Code memory out of the bot.
+/// Deny rules apply even in a folder nobody trusted, and in every mode.
+pub fn settings_json(botloft_home: &Path, crew_dir: &Path, fenced: &[PathBuf]) -> Value {
     let secrets = permission_rule_path(&botloft_home.join("secrets"));
+    let mut deny = vec![format!("Read({secrets}/**)")];
+    for folder in fenced {
+        let folder = permission_rule_path(folder);
+        deny.push(format!("Read({folder}/**)"));
+        deny.push(format!("Edit({folder}/**)"));
+    }
     json!({
-        "permissions": {
-            "deny": [format!("Read({secrets}/**)")],
-        },
+        "permissions": { "deny": deny },
         // Instruction files above the crew's folder are the owner's.
         "claudeMdExcludes": memory::excludes_above(crew_dir),
         // The bot's memory is the `CLAUDE.md` in its folder. Claude Code's
@@ -215,18 +220,28 @@ mod tests {
     use super::*;
 
     #[test]
-    fn settings_deny_the_secrets_folder_and_nothing_else() {
+    fn settings_deny_the_secrets_and_the_fenced_folders() {
         let home = Path::new(r"C:\Users\ana\AppData\Local\Botloft");
-        let settings = settings_json(home, Path::new("/ws/site"));
+        let settings = settings_json(home, Path::new("/ws/site"), &[]);
         assert_eq!(
             settings["permissions"],
             json!({ "deny": ["Read(//c/Users/ana/AppData/Local/Botloft/secrets/**)"] })
         );
+        let blog = PathBuf::from("C:/Users/ana/Botloft/blog");
+        let settings = settings_json(home, Path::new("/ws/site"), &[blog]);
+        assert_eq!(
+            settings["permissions"]["deny"],
+            json!([
+                "Read(//c/Users/ana/AppData/Local/Botloft/secrets/**)",
+                "Read(//c/Users/ana/Botloft/blog/**)",
+                "Edit(//c/Users/ana/Botloft/blog/**)",
+            ])
+        );
     }
 
     #[test]
     fn settings_keep_the_owners_memory_out_of_the_bot() {
-        let settings = settings_json(Path::new("/data"), Path::new("/ws/site"));
+        let settings = settings_json(Path::new("/data"), Path::new("/ws/site"), &[]);
         assert_eq!(
             settings["claudeMdExcludes"],
             json!(memory::excludes_above(Path::new("/ws/site")))