From 9472cb3c4cfa8b05c05d0c3359dc4fd941c996f8 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 17:02:03 -0700 Subject: [PATCH] Resolve host paths before mounting them, and stop reading stored data as choice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes that share a shape: a value already on disk, or one spelled around a check, being taken at face value. `/..` bind-mounted the entire host filesystem read-write. `is_filesystem_root` was purely lexical — trim trailing separators, refuse what was left only if it was empty or a bare `C:` — and nothing in the file ever called `canonicalize`, so `/..`, `/./`, `/home/..`, `/etc/../` and `C:\..` all passed. The daemon resolves them: `docker run -v /..:/mnt/probe` mounts the host root, and the app mounts read-*write* into a container whose agent has passwordless sudo. It is the escalation `check_mount_name_stays_under_workspace` exists to close, reached through the host-path half of the mount instead of the mount-name half. `classify_mount_source` replaces it and asks the OS: `canonicalize` applies `..`, follows symlinks, and resolves 8.3 aliases and UNC spellings on Windows. A path that cannot be resolved — `projects.json` synced from another machine, a folder not created yet — falls back to a lexical collapse rather than being refused, because refusing would make such a project unsavable; the gap is bounded, since what resolution adds is a property of paths that exist. A path that names no location at all (`C:x`, a relative path) is refused rather than guessed at. Same check now guards `ssh_key_path` and `ca_cert_path`, whose read-only mounts were whole-host disclosure at /tmp/.host-ssh. Custom env var names had no charset check anywhere, so `BASH_FUNC_stat%%` — bash's wire format for an exported shell function, body in the value — reached the container environment verbatim. Latent today because the image's /bin/sh is dash, but the pre-commit scrub runs `/bin/sh -c` as root and nothing pins that. Keys are now shell identifiers, on the project and the global list both, with the same grandfathering the folder rows get: a stored key is admitted, a new or edited one is not. The blank workspace row was persisted. The comment said it was dropped on save; the code computed the filtered list and then saved the unfiltered one, so "+ Add folder" plus a blur stored `{"Target": "/workspace/", "Source": ""}` and the project could never be started or recreated again. Every save in the section now goes through one filter, and a blur that changed nothing saves nothing. Widening the five `ClaudeCodeSettings` booleans to `Option` reinterpreted every stored record. They were plain `bool`s that always serialised, so every project ever saved carries an explicit `"env_scrub": false` that nobody chose — and under the new merge that `Some(false)` beats a global `Some(true)`, where the old rule let the global win. Upgrading silently turned five settings off, "strip credentials from subprocess environments" among them. Deserialisation now goes through a shim that dates the record by the presence of the pre-widening `enable_session_recap` key and reads its `false`s as unset. The fields skip serialising when unset, so an older binary can still parse `projects.json` after a downgrade — a `null` would fail to parse and take the whole list down, since `ProjectsStore` parses all-or-nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- .../src/commands/project_commands.rs | 426 ++++++++++++++++-- .../src/commands/settings_commands.rs | 9 + app/src-tauri/src/models/project.rs | 361 ++++++++++++++- .../ClaudeCodeSettingsEditor.test.tsx | 23 + .../projects/ClaudeCodeSettingsEditor.tsx | 23 +- .../home/config/WorkspaceSection.test.tsx | 177 ++++++++ .../projects/home/config/WorkspaceSection.tsx | 41 +- app/src/lib/types.ts | 21 +- 8 files changed, 1027 insertions(+), 54 deletions(-) create mode 100644 app/src/components/projects/home/config/WorkspaceSection.test.tsx diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 3ff42ae..23a9265 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -195,7 +195,9 @@ pub(crate) fn load_secrets_for_project(project: &mut Project) { /// entire host filesystem read-write into a container whose agent has /// passwordless sudo. Anything short of a filesystem root is the user choosing /// a folder — the Browse button and the free-text field lead to the same place -/// — so only the roots themselves are refused. +/// — so only the roots themselves are refused, and *root* is answered by +/// resolving the path rather than by reading it: `/..` is spelled like a folder +/// and is the root. See [`classify_mount_source`]. /// /// This is the **whole** rule set, and it belongs to `add_project`, where every /// row is new by definition. `update_project` runs @@ -228,17 +230,31 @@ fn validate_one_path(p: &ProjectPath) -> Result<(), String> { return Err(format!("Mount name '{}' contains invalid characters. Use alphanumeric, dash, underscore, or dot.", p.mount_name)); } check_mount_name_stays_under_workspace(&p.mount_name)?; - if p.host_path.is_empty() { + // Trimmed: a host path of spaces is not a folder, and `classify_mount_source` + // is deliberately silent about a path with nothing in it — this is the + // message that names the mount it belongs to. + if p.host_path.trim().is_empty() { return Err(format!( "Folder mounted at '/workspace/{}' has no host path.", p.mount_name )); } - if is_filesystem_root(&p.host_path) { - return Err(format!( - "'{}' is a filesystem root. Choose the project folder itself — mounting the whole drive gives the container everything on it.", - p.host_path - )); + match classify_mount_source(&p.host_path) { + None => {} + Some(UnmountableHostPath::FilesystemRoot { resolved }) => { + return Err(filesystem_root_message( + &p.host_path, + &resolved, + "using it as a project folder", + )); + } + Some(UnmountableHostPath::NotAbsolute) => { + return Err(format!( + "'{}' is not a full path to a folder — where it lands is decided by wherever \ + Triple-C is running from rather than by you. Give the whole path.", + p.host_path + )); + } } Ok(()) } @@ -264,6 +280,23 @@ fn check_mount_name_stays_under_workspace(mount_name: &str) -> Result<(), String mount_name )); } + // **An empty name is not refused here, and that is a live residual.** It + // makes the target `/workspace/`, which the daemon normalises to + // `/workspace` — so the host folder shadows the directory the other mounts + // land in, and Docker then creates their mount points *inside it*, on the + // host. It is not refused because it cannot be: an empty name is what a + // half-filled row holds, `legacy_rows` shows those are already in + // `projects.json`, and this function runs on grandfathered rows too, so + // refusing it would make every such project unsavable — the exact + // regression [`validate_project_paths_update`] exists to prevent. + // + // Introducing one is closed at both ends: `validate_one_path` refuses an + // empty name on any new or edited row, and `WorkspaceSection` no longer + // sends half-filled or blank ones. What remains is the *stored* row, and it + // belongs where the mount is built — `docker::create_container` should skip + // a row with no mount name or no host path, which is the same filter that + // stops a stored blank row failing the create with + // `field Source must not be empty`. if !mount_name.is_empty() && mount_name.chars().all(|c| c == '.') { return Err(format!( "Mount name '{}' is not a folder name — it names the directory the mount would sit in.", @@ -363,7 +396,8 @@ fn validate_project_paths_update( /// `/tmp/.host-ssh` and `/tmp/.host-ca` — and neither had any check at all, so /// `/` handed the whole host filesystem to the agent to read. Read-only, so /// this is disclosure rather than the read-write hole a `/` project folder is, -/// but the fix is the same one line. +/// but it is the same check: [`classify_mount_source`], resolved rather than +/// spelled, so `/..` and `/home/..` are refused here too. /// /// Same grandfathering as the folder list, for the same reason: a value already /// stored is already mounted on every start, and refusing an unrelated save @@ -379,29 +413,239 @@ fn validate_mounted_host_path( if stored.map(str::trim) == Some(value) { return Ok(()); } - if is_filesystem_root(value) { - return Err(format!( - "'{}' is a filesystem root, so setting it as {} would mount the whole drive into the \ - container. Choose the folder itself.", - value, label - )); + match classify_mount_source(value) { + None => {} + Some(UnmountableHostPath::FilesystemRoot { resolved }) => { + return Err(filesystem_root_message( + value, + &resolved, + &format!("setting it as {}", label), + )); + } + Some(UnmountableHostPath::NotAbsolute) => { + return Err(format!( + "'{}' is not a full path, so it cannot be used as {}. Give the whole path.", + value, label + )); + } } Ok(()) } -/// Whether a host path is the root of a filesystem, in any spelling the three -/// desktop platforms produce: `/`, a Windows drive root, or a bare UNC/share -/// prefix. Trailing separators are ignored, so `C:\\` and `C:/` are the same -/// answer. -fn is_filesystem_root(host_path: &str) -> bool { - let trimmed = host_path.trim_end_matches(['/', '\\']); - if trimmed.is_empty() { - // Nothing but separators: `/`, `\\`, `//`. - return true; +/// Why a host path may not be used as the source of a bind mount. +/// +/// Two answers rather than a `bool` because they need different sentences, and +/// because "is a root" is no longer a question about how the path is *spelled* +/// — the refusal has to be able to say where the path actually landed. +#[derive(Debug, PartialEq)] +enum UnmountableHostPath { + /// The path is, or resolves to, the root of a filesystem. `resolved` is + /// what it lands on, which is the same string only when a root was typed + /// outright. + FilesystemRoot { resolved: String }, + /// The path does not name a location at all. Where it lands is decided by + /// whatever directory Triple-C happens to be running from, so it can be a + /// root tomorrow and a folder today, and nothing here can judge it. + NotAbsolute, +} + +/// Length of a `C:` drive prefix at the head of `path`, or 0. +/// +/// Duplicated from `commands::file_commands::drive_prefix_len`, together with +/// [`is_windows_style_path`] and [`normalize_host_path`] below. Those are +/// private to that module and it is not this branch's file to change; if the +/// two copies are ever merged, that one is the original and carries the wider +/// test coverage. +fn drive_prefix_len(path: &str) -> usize { + let b = path.as_bytes(); + if b.len() >= 2 && b[0].is_ascii_alphabetic() && b[1] == b':' { + 2 + } else { + 0 } - // `C:` — a drive with no path on it. - let bytes = trimmed.as_bytes(); - bytes.len() == 2 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' +} + +/// Whether `path` is written in Windows form, and so whether `\` separates its +/// components. On Linux a backslash is an ordinary filename character, which is +/// why this is a question rather than an unconditional substitution. +/// +/// Copy of `file_commands::is_windows_style_path` — see [`drive_prefix_len`]. +fn is_windows_style_path(path: &str) -> bool { + cfg!(windows) || path.starts_with("\\\\") || drive_prefix_len(path) > 0 +} + +/// `path` with its separators unified and any Win32 verbatim/device prefix +/// removed — the form every rule below is expressed against. +/// +/// `\\?\C:\Windows` and `\\?\UNC\server\share` name the *same locations* as +/// `C:\Windows` and `\\server\share`; the prefix only turns off Win32 path +/// parsing. Stripping it is what stops four characters being a bypass — and it +/// has to run on our own output as well, because `std::fs::canonicalize` hands +/// back exactly that spelling on Windows. +/// +/// Copy of `file_commands::normalize_host_path` — see [`drive_prefix_len`]. +fn normalize_host_path(path: &str) -> String { + let mut s = if is_windows_style_path(path) { + path.replace('\\', "/") + } else { + path.to_string() + }; + // Slicing by byte index is safe here only because a prefix matched + // case-insensitively as ASCII is ASCII, so its end is a char boundary. + for prefix in ["//?/unc/", "//./unc/"] { + if s.len() >= prefix.len() + && s.as_bytes()[..prefix.len()].eq_ignore_ascii_case(prefix.as_bytes()) + { + return format!("//{}", &s[prefix.len()..]); + } + } + for prefix in ["//?/", "//./"] { + if s.len() >= prefix.len() + && s.as_bytes()[..prefix.len()].eq_ignore_ascii_case(prefix.as_bytes()) + { + s = s[prefix.len()..].to_string(); + break; + } + } + s +} + +/// A normalised absolute path split into the root it hangs off and the part +/// below it, or `None` when it names no location at all. +/// +/// The three roots the desktop platforms have: `/`, a drive (`C:/`), and a UNC +/// share (`//server/share` — the share *is* the root; `//server` alone names a +/// machine and nothing on it). +fn split_host_root(norm: &str) -> Option<(&str, &str)> { + if let Some(rest) = norm.strip_prefix("//") { + let mut parts = rest.splitn(3, '/'); + let server = parts.next().unwrap_or(""); + let share = parts.next().unwrap_or(""); + if server.is_empty() || share.is_empty() { + // `//server`, `//server/`: no share, so nothing under it is named. + return Some((norm, "")); + } + let root_len = 2 + server.len() + 1 + share.len(); + return Some((&norm[..root_len], &norm[root_len..])); + } + let drive = drive_prefix_len(norm); + if drive > 0 { + // `C:x` is drive-*relative* — it means "x under the current directory + // on C:", which is a location only the process's own state decides. + return match norm[drive..].strip_prefix('/') { + Some(tail) => Some((&norm[..drive + 1], tail)), + None if norm.len() == drive => Some((norm, "")), + None => None, + }; + } + norm.strip_prefix('/').map(|tail| (&norm[..1], tail)) +} + +/// How many named components deep `tail` ends up, with `.` dropped and `..` +/// applied — clamped at the root, because `/..` is `/` and not an error. +fn depth_below_root(tail: &str) -> usize { + let mut depth = 0usize; + for segment in tail.split('/') { + match segment { + "" | "." => {} + ".." => depth = depth.saturating_sub(1), + _ => depth += 1, + } + } + depth +} + +/// Whether a host path can be handed to Docker as a bind-mount source, and if +/// not, why. +/// +/// ## Resolved, not spelled +/// +/// This used to be `is_filesystem_root`, and it was purely lexical: trim the +/// trailing separators, say yes to what was left over only if it was empty or a +/// bare `C:`. Nothing in this file called `canonicalize`, so `/..`, `/./`, +/// `/home/..`, `/etc/../` and `C:\..` all sailed through and were passed +/// verbatim to `docker::create_container`, which builds a +/// `Mount { source, read_only: Some(false) }` out of them. Verified against the +/// daemon: `-v /..:/mnt/probe` mounts the host root. That is the whole host +/// filesystem, read-write, in a container whose agent has passwordless sudo — +/// the same escalation `check_mount_name_stays_under_workspace` exists to +/// close, reached through the host-path half of the mount instead of the +/// mount-name half. +/// +/// So the answer comes from the OS where the OS can give one: `canonicalize` +/// applies `..`, follows every symlink in the path, and on Windows returns the +/// long name for an 8.3 alias and the verbatim spelling of a UNC share — all +/// things a string comparison cannot see. +/// +/// ## When the path cannot be resolved +/// +/// `canonicalize` fails on a path that does not exist *here*, which is an +/// ordinary state rather than an attack: `projects.json` syncs between machines +/// and names `C:\Users\jo\code` on a box that has never had a `C:`, and a +/// folder can be created after the project is. Refusing outright would make +/// every such project unsavable, which is the exact failure +/// [`validate_project_paths_update`] exists to avoid — so an unresolvable path +/// falls back to the lexical answer, with `.` and `..` collapsed by +/// [`depth_below_root`] rather than ignored. +/// +/// That is a weaker guarantee, not a wrong one, and the gap is bounded: what +/// resolution adds over the lexical rule is symlinks and 8.3 aliases, and both +/// of those are properties of a path that *exists* — precisely the case where +/// `canonicalize` answers. What is left is a path that does not exist at save +/// time and is a symlink to the root by the time the container starts, i.e. the +/// user doing it to themselves after being asked. +/// +/// Blocking on the filesystem here is deliberate: this runs on a save, once per +/// row, and is a single `realpath` walk. +fn classify_mount_source(host_path: &str) -> Option { + let raw = host_path.trim(); + if raw.is_empty() { + // Emptiness is somebody else's error message — see + // `validate_one_path`, which names the mount it belongs to. + return None; + } + + // Nothing but separators, in either spelling: `/`, `//`, `\`, `\\`. Taken + // first because a lone `\` is a *relative* name on Linux, and reporting a + // Windows root as "not a full path" would be answering a question the user + // did not ask. + if raw.chars().all(|c| c == '/' || c == '\\') { + return Some(UnmountableHostPath::FilesystemRoot { + resolved: raw.to_string(), + }); + } + + let canonical = std::fs::canonicalize(raw) + .ok() + .map(|p| p.to_string_lossy().into_owned()); + let judged = canonical.as_deref().unwrap_or(raw); + let norm = normalize_host_path(judged); + + let Some((root, tail)) = split_host_root(&norm) else { + return Some(UnmountableHostPath::NotAbsolute); + }; + if depth_below_root(tail) == 0 { + return Some(UnmountableHostPath::FilesystemRoot { + resolved: root.to_string(), + }); + } + None +} + +/// The refusal for a host path that lands on a filesystem root, naming the +/// resolved location as well as what was typed when those differ. `/..` reads +/// as a folder; `/..` *is* `/`, and a message that only quoted it back would +/// leave the user with nothing to act on. +fn filesystem_root_message(typed: &str, resolved: &str, use_for: &str) -> String { + let where_it_lands = if typed.trim() == resolved { + format!("'{}' is a filesystem root", typed) + } else { + format!("'{}' resolves to '{}', which is a filesystem root", typed, resolved) + }; + format!( + "{}, so {} would mount the whole drive into the container. Choose the folder itself.", + where_it_lands, use_for + ) } #[tauri::command] @@ -551,6 +795,12 @@ pub async fn update_project( project.ca_cert_path.as_deref(), )?; + // Custom env var names had no charset check anywhere, so a key like + // `BASH_FUNC_stat%%` reached the container environment verbatim. Same + // grandfathering as the folder list, for the same reason — see + // [`crate::models::validate_env_vars_update`]. + crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?; + project.container_id = stored.container_id; project.status = stored.status; project.created_at = stored.created_at; @@ -1159,6 +1409,130 @@ mod tests { assert!(validate_project_paths(&[path("C:\\Users\\u\\project", "project")]).is_ok()); } + /// Every spelling of a root that is not *spelled* like one. + /// + /// The predicate this replaces trimmed trailing separators and compared + /// what was left, so `/..` — which the daemon mounts as the host root, + /// verified with `docker run -v /..:/mnt/probe` — was indistinguishable + /// from a project folder called `..`. No test in the repo contained a `.` + /// or a `..` in a host path, which is why it shipped. + #[test] + fn a_host_path_that_resolves_to_a_root_is_refused() { + let escapes = [ + "/..", + "/../", + "/./", + "/.", + "/home/..", + "/etc/../", + "/tmp/../..", + // Deliberately not present on any machine, so this is the + // unresolvable path taking the lexical route. + "/no-such-dir-here/../..", + "C:\\..", + "C:\\Users\\..", + "c:/foo/..", + // Win32 verbatim spelling of a drive root. + "\\\\?\\C:\\", + "\\\\?\\C:\\..", + // A UNC share root is the root of everything on that share, which + // the old predicate accepted despite its doc comment claiming + // otherwise. + "\\\\server\\share", + "//server/share/", + ]; + for escape in escapes { + assert!( + validate_project_paths(&[path(escape, "everything")]).is_err(), + "host path '{}' was accepted as a project folder, which bind-mounts a whole \ + filesystem read-write into a container with passwordless sudo", + escape + ); + // The same value must not be reachable through the editor either. + assert!( + validate_project_paths_update(&[], &[path(escape, "everything")]).is_err(), + "host path '{}' was accepted through update_project", + escape + ); + // And the two read-only mounts are the same check. + assert!( + validate_mounted_host_path("the SSH key folder", None, Some(escape)).is_err(), + "'{}' was accepted as an SSH key path, which read-only bind-mounts a whole \ + filesystem at /tmp/.host-ssh", + escape + ); + } + } + + /// A drive-relative path (`C:x`, no separator) means "x under whatever the + /// current directory on C: happens to be" — a location decided by the + /// process rather than by the user, so it may be the drive root. + #[test] + fn a_path_that_names_no_location_is_refused_rather_than_guessed_at() { + for relative in ["C:x", "C:Users\\jo", "relative/path", "./project"] { + assert!( + validate_project_paths(&[path(relative, "project")]).is_err(), + "'{}' was accepted, though where it lands depends on Triple-C's own \ + working directory", + relative + ); + } + } + + /// The dots that are *not* an escape have to keep working — a folder can + /// legitimately be reached through `.` or a `..` that goes back down again, + /// and the Browse button produces paths on machines this list is not + /// running on. + #[test] + fn an_ordinary_folder_is_still_accepted_however_it_is_spelled() { + for ok in [ + "/home/u/./project", + "/home/u/x/../project", + "/home/u/..project", + "/home/u/project/..hidden", + "C:\\Users\\u\\x\\..\\project", + "\\\\server\\share\\project", + "\\\\?\\C:\\Users\\u\\project", + ] { + assert!( + validate_project_paths(&[path(ok, "project")]).is_ok(), + "host path '{}' should be usable", + ok + ); + } + } + + /// The half of this that only resolution can answer. + /// + /// A lexical check sees a two-component path under `/tmp` and stops. The + /// container is what plants the link — `/proc/self/mountinfo` inside a + /// Triple-C container spells the host's project paths out verbatim — so the + /// symlink is reachable, and the mount that follows it is read-write. + #[cfg(unix)] + #[test] + fn a_symlink_to_the_root_is_refused_because_resolution_is_what_answers() { + let dir = std::env::temp_dir().join(format!( + "triple-c-root-link-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + let link = dir.join("innocent"); + std::os::unix::fs::symlink("/", &link).unwrap(); + + let verdict = validate_project_paths(&[path(&link.to_string_lossy(), "project")]); + std::fs::remove_file(&link).ok(); + std::fs::remove_dir(&dir).ok(); + + assert!( + verdict.is_err(), + "a symlink to / was accepted as a project folder; only canonicalisation can see it" + ); + } + #[test] fn duplicate_and_half_filled_rows_are_refused_but_the_blank_row_is_not() { assert!(validate_project_paths(&[ diff --git a/app/src-tauri/src/commands/settings_commands.rs b/app/src-tauri/src/commands/settings_commands.rs index 51d949a..883a052 100644 --- a/app/src-tauri/src/commands/settings_commands.rs +++ b/app/src-tauri/src/commands/settings_commands.rs @@ -16,6 +16,15 @@ pub async fn update_settings( state: State<'_, AppState>, ) -> Result { let before = state.settings_store.get(); + + // The global half of the same rule the project half gets in + // `update_project`: a global custom env var is merged into every project's + // container environment, so an unchecked name here reaches all of them. + crate::models::validate_env_vars_update( + &before.global_custom_env_vars, + &settings.global_custom_env_vars, + )?; + let saved = state.settings_store.update(settings)?; // Persisting a setting is not the same as applying it. The gateway is the diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index 45022be..dfa8f38 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -8,6 +8,100 @@ pub struct EnvVar { pub value: String, } +/// Whether `key` is a name a shell will read back as an ordinary variable: +/// `[A-Za-z_][A-Za-z0-9_]*`. +/// +/// ## Why a charset rule, and not just the reserved-name list +/// +/// `docker::container::is_reserved_env_key` answers a different question — "is +/// this one of the names Triple-C manages itself" — and nothing anywhere asked +/// what the *characters* were. A key is joined into `KEY=VALUE` and handed to +/// the daemon, which puts it in the container's environment verbatim, so a name +/// that is not an identifier travels through unchallenged. +/// +/// The one that matters is `BASH_FUNC_name%%`, bash's wire format for an +/// exported shell function: bash imports those at startup and the *body* is the +/// value. Today that is latent rather than live — the image's `/bin/sh` is +/// dash, which does not import them, and an auditor confirmed the vector fires +/// under `bash -c` and not under `sh -c` in the shipped image. But the +/// pre-commit scrub runs `/bin/sh -c` **as root**, `/bin/sh` is whatever +/// `ubuntu:24.04` points it at, and nothing pins that. One base-image change, +/// or one call site spelled `bash`, turns a stored project setting into root +/// code execution inside the container at commit time. +/// +/// So the rule is the shape of the thing rather than a list of the names that +/// are known to be dangerous: `IFS`, `LD_PRELOAD` and `PATH` are all perfectly +/// good identifiers and are the user's business, while nothing legitimate needs +/// a `%`, a `(` or a space in an environment variable name. +/// +/// The key is judged **trimmed**, because that is what `create_container` sends +/// — ` FOO ` already reaches the container as `FOO`, and refusing it here would +/// break a setting that works. +pub fn is_valid_env_key(key: &str) -> bool { + let mut chars = key.trim().chars(); + match chars.next() { + Some(c) if c.is_ascii_alphabetic() || c == '_' => {} + _ => return false, + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + +/// Validate a custom environment variable list that is about to be stored, +/// admitting the entries it is already stored with. +/// +/// Same shape, and the same reasoning, as +/// `commands::project_commands::validate_project_paths_update`: nothing ever +/// checked these keys, so `projects.json` and `settings.json` in the field can +/// hold whatever was typed. Holding every save to the new rule would make such +/// a project unsavable *entirely* — `update_project` is the single command +/// behind the whole Config tab — and would buy nothing, because the stored key +/// is already being handed to every container that starts. An entry carried +/// over verbatim is admitted; a new or edited one is held to the rule, which is +/// what keeps the escalation closed, since escalation means *introducing* a bad +/// key through this command. +/// +/// Counted rather than set-tested, for the same reason as the folder rows: a +/// second copy of an existing entry is a new entry. +/// +/// The blank entry is not a violation. "+ Add variable" appends +/// `{key: "", value: ""}` and saves the list immediately, so refusing it would +/// turn the button itself into an error toast; `create_container` skips an +/// empty key, so it reaches nothing. +pub fn validate_env_vars_update(stored: &[EnvVar], incoming: &[EnvVar]) -> Result<(), String> { + // An entry with no key is the placeholder, whatever is in its value: + // `create_container` skips it, so it reaches nothing and there is nothing + // to refuse. The editor saves on every blur, and typing the value before + // the name is an ordinary way to fill a row in. + let is_blank = |v: &EnvVar| v.key.trim().is_empty(); + + let mut carried: std::collections::HashMap<(&str, &str), usize> = + std::collections::HashMap::new(); + for v in stored.iter().filter(|v| !is_blank(v)) { + *carried + .entry((v.key.as_str(), v.value.as_str())) + .or_insert(0) += 1; + } + + for v in incoming.iter().filter(|v| !is_blank(v)) { + match carried.get_mut(&(v.key.as_str(), v.value.as_str())) { + Some(remaining) if *remaining > 0 => { + *remaining -= 1; + } + _ => { + if !is_valid_env_key(&v.key) { + return Err(format!( + "'{}' is not a usable environment variable name. Use a letter or \ + underscore followed by letters, digits or underscores.", + v.key + )); + } + } + } + } + + Ok(()) +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct ProjectPath { pub host_path: String, @@ -85,6 +179,7 @@ impl PermissionMode { /// Settings for Claude Code CLI behavior inside the container. /// These map to Claude Code env vars and ~/.claude/settings.json entries. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Default)] +#[serde(from = "StoredClaudeCodeSettings")] /// Every field is three-state, and the third state is load-bearing. /// /// `None` means "not set at this level". For a *project* that is "inherit @@ -98,24 +193,24 @@ pub struct ClaudeCodeSettings { /// what lets Claude Code pick the renderer itself; `Some("default")` pins /// the classic main-screen renderer and `Some("fullscreen")` the alt-screen /// one. All three are distinct — "let it choose" is not "classic". - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub tui_mode: Option, /// Saved `/effort` level: `None` = unset, otherwise one of /// `"low" | "medium" | "high" | "xhigh"`. Written to settings.json as /// `effortLevel` (**not** `effort`, which Claude Code has never read). - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub effort: Option, /// Disable auto-scroll in fullscreen TUI mode. Held in the *disabled* sense /// because Claude Code's `autoScrollEnabled` defaults to `true`, so the /// zero value of this field has to mean "leave it on". - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub auto_scroll_disabled: Option, /// Collapse tool output to one-line summaries. Written to settings.json as /// `viewMode: "focus"`; there is no `focusMode` key in Claude Code. - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub focus_mode: Option, /// Show thinking summaries in responses - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub show_thinking_summaries: Option, /// Turn the session recap **off**. /// @@ -128,16 +223,99 @@ pub struct ClaudeCodeSettings { /// never touched the control holds — as "the user turned the recap off" and /// silently disabled it for all of them. A new name lets the old key be /// ignored, which lands every existing project on the correct default. - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub session_recap_disabled: Option, /// Strip credentials from subprocess environments - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub env_scrub: Option, /// Enable 1-hour prompt cache TTL (vs default 5-minute) - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub prompt_caching_1h: Option, } +/// `ClaudeCodeSettings` in every shape `projects.json` and `settings.json` can +/// be holding, which is what [`ClaudeCodeSettings`] is actually deserialised +/// through. +/// +/// ## The upgrade this exists to survive +/// +/// Before the widening, the five booleans were plain `bool`s with +/// `#[serde(default)]` and no `skip_serializing_if`, so **every** settings +/// object ever written carries an explicit `"env_scrub": false` — not because +/// anyone chose it, but because that is what a `bool` serialises to. Under the +/// old merge (`if p.x { true } else { g.x }`) that `false` carried no +/// information at all: it was the only value an unset switch could produce, and +/// the global always won. +/// +/// Read as `Some(false)` by the new code it becomes a *deliberate off* that +/// beats a global `Some(true)` — so upgrading silently turned five settings off +/// for every project that had ever opened this editor, `env_scrub` ("strip +/// credentials from subprocess environments") among them. There is no store +/// migration anywhere: `projects_store` parses these structs directly. +/// +/// ## How an old record is told apart from a new one +/// +/// By `enable_session_recap`. It was in the struct from the day it existed and +/// was a plain `bool`, so its key is present in every pre-widening record and +/// in no other — the field was *renamed* to `session_recap_disabled` precisely +/// so the old key could be ignored (see the doc on that field), and the new +/// code has never written it. Its presence is therefore an exact statement that +/// these bytes were written by a binary in which `false` meant "unset", and the +/// booleans are read back that way: `true` is a real choice and survives, +/// `false` becomes `None` and inherits again. +/// +/// Nothing marks a *new* record, and nothing needs to: absent is `None` (the +/// fields skip serialising when unset) and a present `false` is the deliberate +/// off the widening was for. That is also what keeps a downgrade survivable — +/// an older binary reads an absent key as `false` through its own +/// `#[serde(default)]`, where a `null` would fail to parse and take the whole +/// of `projects.json` down with it, since `ProjectsStore` parses all-or-nothing +/// and starts empty on an error. +#[derive(Deserialize)] +struct StoredClaudeCodeSettings { + #[serde(default)] + tui_mode: Option, + #[serde(default)] + effort: Option, + #[serde(default)] + auto_scroll_disabled: Option, + #[serde(default)] + focus_mode: Option, + #[serde(default)] + show_thinking_summaries: Option, + #[serde(default)] + session_recap_disabled: Option, + #[serde(default)] + env_scrub: Option, + #[serde(default)] + prompt_caching_1h: Option, + /// The pre-widening spelling of `session_recap_disabled`, and the *only* + /// use of its value: presence dates the record. Its meaning was inverted + /// and it never worked, so it is read for the marker and discarded. + #[serde(default)] + enable_session_recap: Option, +} + +impl From for ClaudeCodeSettings { + fn from(stored: StoredClaudeCodeSettings) -> Self { + let pre_widening = stored.enable_session_recap.is_some(); + // On a pre-widening record `false` is what an untouched switch wrote, + // so it means "not set at this level" and must inherit. A `true` was a + // real choice either way. + let read = |v: Option| if pre_widening { v.filter(|on| *on) } else { v }; + ClaudeCodeSettings { + tui_mode: stored.tui_mode, + effort: stored.effort, + auto_scroll_disabled: read(stored.auto_scroll_disabled), + focus_mode: read(stored.focus_mode), + show_thinking_summaries: read(stored.show_thinking_summaries), + session_recap_disabled: read(stored.session_recap_disabled), + env_scrub: read(stored.env_scrub), + prompt_caching_1h: read(stored.prompt_caching_1h), + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Project { pub id: String, @@ -467,3 +645,170 @@ impl Project { val } } + +#[cfg(test)] +mod tests { + use super::*; + + // ── Custom environment variable names ───────────────────────────────── + + #[test] + fn an_env_var_name_has_to_be_a_shell_identifier() { + for ok in ["PATH", "_", "_x", "MY_VAR2", "a", " SPACED_BY_THE_EDITOR "] { + assert!(is_valid_env_key(ok), "'{}' should be a usable name", ok); + } + for bad in [ + // bash's wire format for an exported shell function: the value is + // the body, and a `bash` that imports it runs it. The scrub exec is + // `/bin/sh -c` as root, and nothing pins `/bin/sh` to dash. + "BASH_FUNC_stat%%", + "BASH_FUNC_ls()", + "MY VAR", + "2FAST", + "WITH-DASH", + "WITH.DOT", + "", + " ", + "$(id)", + "A=B", + ] { + assert!(!is_valid_env_key(bad), "'{}' should be refused", bad); + } + } + + fn env(key: &str, value: &str) -> EnvVar { + EnvVar { key: key.to_string(), value: value.to_string() } + } + + #[test] + fn a_bad_env_var_name_cannot_be_introduced_but_a_stored_one_does_not_brick_the_editor() { + let bad = [env("BASH_FUNC_stat%%", "() { id; }")]; + // Introducing it through the Config tab is the escalation. + assert!(validate_env_vars_update(&[], &bad).is_err()); + // Already stored: it is handed to every container that starts whether + // or not an unrelated save is allowed through, and refusing the save + // would make every toggle on the Config tab fail. + assert!(validate_env_vars_update(&bad, &bad).is_ok()); + // Editing its value is a new entry, and refused again. + assert!( + validate_env_vars_update(&bad, &[env("BASH_FUNC_stat%%", "() { rm -rf /; }")]).is_err() + ); + // Fixing the name is what the message asks for, and it saves. + assert!(validate_env_vars_update(&bad, &[env("STAT", "() { id; }")]).is_ok()); + // Dropping it entirely is always fine. + assert!(validate_env_vars_update(&bad, &[]).is_ok()); + } + + #[test] + fn the_blank_row_the_add_button_saves_is_not_an_error() { + // "+ Add variable" appends an empty entry and saves the list at once, + // so this is the button, not an attempt at anything. + assert!(validate_env_vars_update(&[], &[env("", "")]).is_ok()); + // Typing the value before the name is an ordinary way to fill it in, + // and an entry with no name reaches no container either way. + assert!(validate_env_vars_update(&[], &[env("", "value-first")]).is_ok()); + assert!(validate_env_vars_update(&[], &[env("GOOD", "v"), env("", "")]).is_ok()); + } + + #[test] + fn a_stored_entry_may_be_kept_but_not_multiplied() { + let stored = [env("BAD NAME", "v")]; + assert!(validate_env_vars_update(&stored, &stored).is_ok()); + // A second copy is a new entry, and held to the rule. + assert!( + validate_env_vars_update(&stored, &[env("BAD NAME", "v"), env("BAD NAME", "v")]) + .is_err() + ); + } + + // ── Claude Code settings written before the fields were widened ─────── + + /// `projects.json` exactly as the shipped `main` binary wrote it: the five + /// booleans were plain `bool`s that always serialised, so every project + /// that ever opened the editor carries `false` for the ones it never + /// touched. + const MAIN_SHAPE_PROJECT: &str = r#"{ + "id": "p1", + "name": "demo", + "paths": [{ "host_path": "/home/u/demo", "mount_name": "demo" }], + "container_id": null, + "status": "stopped", + "backend": "anthropic", + "bedrock_config": null, + "ollama_config": null, + "openai_compatible_config": null, + "allow_docker_access": false, + "ssh_key_path": null, + "git_user_name": null, + "git_user_email": null, + "claude_code_settings": { + "tui_mode": "fullscreen", + "effort": null, + "auto_scroll_disabled": false, + "focus_mode": false, + "show_thinking_summaries": false, + "enable_session_recap": false, + "env_scrub": false, + "prompt_caching_1h": false + }, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#; + + #[test] + fn a_setting_stored_as_false_by_the_old_binary_still_inherits_the_global() { + let project: Project = serde_json::from_str(MAIN_SHAPE_PROJECT).unwrap(); + let stored = project.claude_code_settings.expect("settings should parse"); + + // Read verbatim these would be `Some(false)`, which under + // `docker::container::merge_claude_code_settings` beats the global. + assert_eq!(stored.env_scrub, None); + assert_eq!(stored.auto_scroll_disabled, None); + assert_eq!(stored.focus_mode, None); + assert_eq!(stored.show_thinking_summaries, None); + assert_eq!(stored.prompt_caching_1h, None); + assert_eq!(stored.session_recap_disabled, None); + // A value the user did choose is untouched. + assert_eq!(stored.tui_mode.as_deref(), Some("fullscreen")); + + // The merge rule itself, spelled the way + // `merge_claude_code_settings` spells it. `main` resolved this with + // `if p.env_scrub { true } else { g.env_scrub }`, i.e. the global won — + // and it has to go on winning, because the user never turned this off. + let global = ClaudeCodeSettings { env_scrub: Some(true), ..Default::default() }; + assert_eq!( + stored.env_scrub.or(global.env_scrub), + Some(true), + "upgrading silently turned off 'strip credentials from subprocess environments'" + ); + } + + #[test] + fn an_off_chosen_in_the_new_editor_still_beats_a_global_on() { + // Same record without the pre-widening key: this `false` is the + // deliberate off the widening exists to make expressible. + let json = r#"{ "env_scrub": false }"#; + let chosen: ClaudeCodeSettings = serde_json::from_str(json).unwrap(); + assert_eq!(chosen.env_scrub, Some(false)); + let global = ClaudeCodeSettings { env_scrub: Some(true), ..Default::default() }; + assert_eq!(chosen.env_scrub.or(global.env_scrub), Some(false)); + } + + #[test] + fn an_unset_setting_is_written_as_absent_rather_than_null() { + // A downgrade parses these fields as plain `bool` with + // `#[serde(default)]`: an absent key is `false`, a `null` is a parse + // error — and `ProjectsStore` parses all-or-nothing, so one project + // with one null empties the whole list and the next save persists that. + let json = serde_json::to_string(&ClaudeCodeSettings::default()).unwrap(); + assert_eq!(json, "{}"); + assert!(!json.contains("null")); + + let partial = ClaudeCodeSettings { env_scrub: Some(false), ..Default::default() }; + let json = serde_json::to_string(&partial).unwrap(); + assert_eq!(json, r#"{"env_scrub":false}"#); + // And it reads back as what it is. + let round_tripped: ClaudeCodeSettings = serde_json::from_str(&json).unwrap(); + assert_eq!(round_tripped, partial); + } +} diff --git a/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx b/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx index 4993a32..85f5ff3 100644 --- a/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx +++ b/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx @@ -193,4 +193,27 @@ describe("ClaudeCodeSettingsEditor", () => { expect(screen.getByRole("switch", { name: label })).toBeChecked(); }); }); + + /** + * A settings object with nothing set at this level arrives as `{}`: the Rust + * struct skips serialising a field it has no value for, which is what keeps + * an older binary able to parse `projects.json` after a downgrade. It is also + * the exact shape a project stored before the fields were widened is read + * back as — every one of its `false`s meant "unset" — so reading absent as + * "off" would show a switch the user never touched as a deliberate choice. + */ + it("reads an absent field as Global rather than as Off", () => { + renderEditor({} as ClaudeCodeSettings, "project"); + expect((screen.getByLabelText("Env scrub") as HTMLSelectElement).value).toBe("global"); + expect((screen.getByLabelText("Session recap") as HTMLSelectElement).value).toBe("global"); + }); + + it("still collapses to null when an absent-field object is edited back", () => { + const onSave = renderEditor({} as ClaudeCodeSettings, "global"); + // Off and straight back on: the round trip has to land on `null`, or an + // untouched global stops being indistinguishable from one never opened. + fireEvent.click(screen.getByRole("switch", { name: "Session recap" })); + fireEvent.click(screen.getByRole("switch", { name: "Session recap" })); + expect(onSave).toHaveBeenLastCalledWith(null); + }); }); diff --git a/app/src/components/projects/ClaudeCodeSettingsEditor.tsx b/app/src/components/projects/ClaudeCodeSettingsEditor.tsx index ff9f3d4..e3b5bb3 100644 --- a/app/src/components/projects/ClaudeCodeSettingsEditor.tsx +++ b/app/src/components/projects/ClaudeCodeSettingsEditor.tsx @@ -37,15 +37,20 @@ export const CLAUDE_CODE_DEFAULTS: ClaudeCodeSettings = { * overrides a global on, so a settings object holding one has to be persisted. */ function isAllDefaults(s: ClaudeCodeSettings): boolean { + // `== null`, not `===`: an unset field is *absent* on the wire, not null. + // The Rust struct skips serialising one it has no value for, so a project + // whose stored settings were all "unset" arrives here as `{}` — and reading + // that as "off" is exactly the mistake the three-state control exists to + // avoid. See the note on `ClaudeCodeSettings` in `lib/types.ts`. return ( - s.tui_mode === null && - s.effort === null && - s.auto_scroll_disabled === null && - s.focus_mode === null && - s.show_thinking_summaries === null && - s.session_recap_disabled === null && - s.env_scrub === null && - s.prompt_caching_1h === null + s.tui_mode == null && + s.effort == null && + s.auto_scroll_disabled == null && + s.focus_mode == null && + s.show_thinking_summaries == null && + s.session_recap_disabled == null && + s.env_scrub == null && + s.prompt_caching_1h == null ); } @@ -204,7 +209,7 @@ export default function ClaudeCodeSettingsEditor({ // `stored` holds the deviation from Claude Code's default, so an // inverted field reads back the other way round — see BOOLEAN_FIELDS. const selected = - stored === null ? "global" : (invert ? !stored : stored) ? "on" : "off"; + stored == null ? "global" : (invert ? !stored : stored) ? "on" : "off"; return ( ({ + open: (...args: unknown[]) => open(...args), +})); + +const baseProject: Project = { + id: "p1", + name: "api-server", + paths: [{ host_path: "/src/api", mount_name: "api" }], + container_id: null, + status: "stopped", + backend: "anthropic", + bedrock_config: null, + ollama_config: null, + llamacpp_config: null, + openai_compatible_config: null, + allow_docker_access: false, + sandbox_mode_enabled: true, + mission_control_enabled: false, + auth_bridge_enabled: false, + browser_view_enabled: false, + vpn_support_enabled: false, + use_shared_auth_token: true, + full_permissions: false, + permission_mode: null, + ssh_key_path: null, + ca_cert_path: null, + git_token: null, + git_user_name: null, + git_user_email: null, + custom_env_vars: [], + port_mappings: [], + claude_instructions: null, + claude_code_settings: null, + renamed_session_names: {}, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", +}; + +const save = vi.fn().mockResolvedValue(true); + +function renderSection(over: Partial = {}, disabled = false) { + return render( + , + ); +} + +/** Every folder list this component has sent to `update_project`. */ +function savedLists() { + return save.mock.calls + .filter(([patch]) => "paths" in patch) + .map(([patch]) => patch.paths); +} + +describe("WorkspaceSection — the blank row is never stored", () => { + beforeEach(() => vi.clearAllMocks()); + + /** + * The bug this file exists for. `create_container` mounts every stored row + * unfiltered, so a persisted `{host_path: "", mount_name: ""}` becomes + * `{"Target": "/workspace/", "Source": ""}` and the daemon refuses the whole + * container with `field Source must not be empty` — the project can never be + * started or recreated again. Click "+ Add folder", blur a field, and it is + * bricked. + */ + it("drops the placeholder row when a real edit is saved", () => { + renderSection(); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + + const hostPath = screen.getByLabelText("Folder 1 host path"); + fireEvent.change(hostPath, { target: { value: "/src/api-v2" } }); + fireEvent.blur(hostPath); + + expect(save).toHaveBeenCalledTimes(1); + expect(savedLists()[0]).toEqual([{ host_path: "/src/api-v2", mount_name: "api" }]); + }); + + it("drops it when Browse fills a different row in", async () => { + open.mockResolvedValueOnce("/src/api-v2"); + renderSection(); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + + // The picker is awaited inside the handler, so the state update that + // follows it lands outside the click. + await act(async () => { + fireEvent.click(screen.getAllByRole("button", { name: "Browse" })[0]); + }); + + expect(savedLists()[0]).toEqual([{ host_path: "/src/api-v2", mount_name: "api" }]); + }); + + it("drops it when a row is removed", () => { + renderSection({ + paths: [ + { host_path: "/src/api", mount_name: "api" }, + { host_path: "/src/web", mount_name: "web" }, + ], + }); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + fireEvent.click(screen.getByRole("button", { name: "Remove folder 2" })); + + expect(savedLists()[0]).toEqual([{ host_path: "/src/api", mount_name: "api" }]); + }); + + it("never sends a row with an empty host path, whatever the route", () => { + renderSection(); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + const hostPath = screen.getByLabelText("Folder 1 host path"); + fireEvent.change(hostPath, { target: { value: "/src/api-v2" } }); + fireEvent.blur(hostPath); + + for (const list of savedLists()) { + for (const row of list) { + expect(row.host_path).not.toBe(""); + expect(row.mount_name).not.toBe(""); + } + } + }); +}); + +describe("WorkspaceSection — what a blur is allowed to save", () => { + beforeEach(() => vi.clearAllMocks()); + + /** + * Both inputs save on blur, so tabbing from the host path to the mount name + * fires a save with the name still empty — which `update_project` refuses, + * turning an ordinary keystroke into an error toast. + */ + it("holds a half-filled row back until it is complete", () => { + renderSection(); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + + const newHostPath = screen.getByLabelText("Folder 2 host path"); + fireEvent.change(newHostPath, { target: { value: "/src/web" } }); + fireEvent.blur(newHostPath); + expect(save).not.toHaveBeenCalled(); + + const newMountName = screen.getByLabelText("Folder 2 mount name"); + fireEvent.change(newMountName, { target: { value: "web" } }); + fireEvent.blur(newMountName); + expect(savedLists()[0]).toEqual([ + { host_path: "/src/api", mount_name: "api" }, + { host_path: "/src/web", mount_name: "web" }, + ]); + }); + + /** + * Blurring out of an untouched field is not an edit. Saving anyway would + * round-trip the filtered list through `project` and take the empty row away + * while the user was still filling it in. + */ + it("saves nothing when the blur changed nothing", () => { + renderSection(); + fireEvent.click(screen.getByRole("button", { name: "+ Add folder" })); + fireEvent.blur(screen.getByLabelText("Folder 1 mount name")); + expect(save).not.toHaveBeenCalled(); + expect(screen.getByLabelText("Folder 2 host path")).toBeTruthy(); + }); + + it("still saves a rename, which does not go through the folder list", () => { + renderSection(); + const name = screen.getByDisplayValue("api-server"); + fireEvent.change(name, { target: { value: "api-v2" } }); + fireEvent.blur(name); + expect(save).toHaveBeenCalledWith({ name: "api-v2" }); + }); +}); diff --git a/app/src/components/projects/home/config/WorkspaceSection.tsx b/app/src/components/projects/home/config/WorkspaceSection.tsx index 18019ea..cc6790c 100644 --- a/app/src/components/projects/home/config/WorkspaceSection.tsx +++ b/app/src/components/projects/home/config/WorkspaceSection.tsx @@ -10,6 +10,14 @@ interface Props { disabled: boolean; } +/** Whether two folder lists are the same rows in the same order. */ +function sameRows(a: ProjectPath[], b: ProjectPath[]): boolean { + return ( + a.length === b.length && + a.every((row, i) => row.host_path === b[i].host_path && row.mount_name === b[i].mount_name) + ); +} + export default function WorkspaceSection({ project, save, disabled }: Props) { const [name, setName] = useState(project.name); const [paths, setPaths] = useState(project.paths ?? []); @@ -19,6 +27,27 @@ export default function WorkspaceSection({ project, save, disabled }: Props) { setPaths(project.paths ?? []); }, [project]); + /** + * Persist a folder list, minus the rows that are only in it because the UI + * put them there. + * + * **The blank row must never reach the store.** "+ Add folder" inserts + * `{host_path: "", mount_name: ""}` deliberately, and `create_container` + * mounts every stored row unfiltered — a stored blank one becomes + * `{"Target": "/workspace/", "Source": ""}`, which the daemon rejects with + * `field Source must not be empty`. The project then cannot be started or + * recreated at all, from a click and a blur. `AddProjectDialog` has always + * filtered this; this section computed the filtered list and then saved the + * unfiltered one. + * + * Every save goes through here for that reason — Browse and Remove write the + * list too, and either can be holding a blank row from an earlier click. + */ + const persist = (rows: ProjectPath[]) => { + const filled = rows.filter((p) => p.host_path.trim() || p.mount_name.trim()); + return save({ paths: filled }); + }; + /** * Save only when every row is fully filled in. * @@ -27,12 +56,18 @@ export default function WorkspaceSection({ project, save, disabled }: Props) { * a half-filled row is refused — so the unconditional save turned an ordinary * keystroke into an error toast. A blank row is *not* incomplete: the * "+ Add folder" button adds one deliberately, and it is dropped on save. + * + * A blur that changed nothing saves nothing, which is what keeps the blank + * row on screen while it is being filled in: persisting the filtered list + * would round-trip through `project` and take the empty row away under the + * cursor. */ const saveIfComplete = () => { const filled = paths.filter((p) => p.host_path.trim() || p.mount_name.trim()); const halfFilled = filled.some((p) => !p.host_path.trim() || !p.mount_name.trim()); if (halfFilled) return; - return save({ paths }); + if (sameRows(filled, project.paths ?? [])) return; + return persist(paths); }; return ( @@ -106,7 +141,7 @@ export default function WorkspaceSection({ project, save, disabled }: Props) { mount_name: updated[i].mount_name || basename, }; setPaths(updated); - save({ paths: updated }); + persist(updated); } }} > @@ -137,7 +172,7 @@ export default function WorkspaceSection({ project, save, disabled }: Props) { onClick={() => { const updated = paths.filter((_, j) => j !== i); setPaths(updated); - save({ paths: updated }); + persist(updated); }} > Remove diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index 8e05b28..641820f 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -162,23 +162,28 @@ export interface OpenAiCompatibleConfig { * project that is "inherit the global value", and on the global settings it is * "leave Claude Code's own default alone". `false` is a deliberate off, which * is what lets a project turn a globally-enabled setting back off. + * + * Every field is optional as well as nullable: the Rust struct skips + * serialising a field it has no value for, so an object with nothing set at + * this level arrives as `{}`. Absent and `null` mean the same thing, which is + * why every read of one of these has to use `== null` rather than `=== null`. */ export interface ClaudeCodeSettings { /** `null` = let Claude Code choose the renderer; `"default"` = classic, `"fullscreen"` = alt-screen. */ - tui_mode: string | null; + tui_mode?: string | null; /** `null` = unset, else `"low" | "medium" | "high" | "xhigh"`. Written as `effortLevel`. */ - effort: string | null; - auto_scroll_disabled: boolean | null; + effort?: string | null; + auto_scroll_disabled?: boolean | null; /** Written as `viewMode: "focus"`. */ - focus_mode: boolean | null; - show_thinking_summaries: boolean | null; + focus_mode?: boolean | null; + show_thinking_summaries?: boolean | null; /** * Turns the session recap **off**. Held in the disabled sense because Claude * Code's recap is on by default — see the Rust doc on `ClaudeCodeSettings`. */ - session_recap_disabled: boolean | null; - env_scrub: boolean | null; - prompt_caching_1h: boolean | null; + session_recap_disabled?: boolean | null; + env_scrub?: boolean | null; + prompt_caching_1h?: boolean | null; } export interface ContainerInfo {