mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-07 17:09:13 -10:00
fix(workspace): finish the home-workspace state exemption
Carries the state-path decision to the call sites the first three commits missed, and closes the two gaps a review of them found. - fleet store: a workspace Fleet is now checked through the same `workspace_state_path`, and `ensure_fleets_dir` hands its resolved root to the writer. Checking one path and writing another left workspace-scope save and select failing at a home workspace with "Fleet artifact path must stay within the workspace", after `create_dir_all` had already run through the junction. - hooks: `/hooks edit` takes the same decision. The editor is given the literal `~/.codewhale/hooks.toml` rather than a resolved root, because `canonicalize` returns a verbatim `\\?\` path on Windows and several terminal editors will not open one. - mcp: `reject_linked_workspace_state_path` asked `home == root` literally while every other site canonicalizes. It now uses `workspace_is_user_home`, so one rule decides what "the user's own directory" means. - doctor: the session-recovery scan rejected any state root whose `file_type().is_symlink()` was true. A junction reports as a symlink in this Rust version, so `doctor` could not run the diagnostic on the layout it reports about. It now resolves the root and inspects the target, while still ignoring symlink entries during the scan. Two unix tests move from "rejects a linked root" to "follows a linked root". - `workspace_state_path` asserts its `relative` argument in release builds, not only in debug ones: it is the one place a security boundary is decided. Verified on Windows with junctions on both sides (`reject_linked_path` still refuses `<repo>/.codewhale`, passes `<home>/.codewhale`), `cargo check -p codewhale-engine -p codewhale-tui` passing, `cargo fmt --all -- --check` passing, and `cargo test -p codewhale-engine --lib -- fleet::store` reporting 16 passed. The base commit already fails Lint, both Test jobs and the Safety gate for reasons unrelated to these files; that is tracked upstream in #7204.
This commit is contained in:
1 parent
5ad3d821c7
commit
8f82d02e78
5 files changed
+117
-80
No files matched your search
@@ -607,31 +607,43 @@ fn workspace_fleets_dir(workspace: &Path) -> PathBuf {
|
||||
}
|
||||
|
||||
/// The fleet directory for a scope, creating it if needed.
|
||||
fn ensure_fleets_dir(scope: FleetScope, workspace: &Path) -> Result<PathBuf, FleetStoreError> {
|
||||
let dir = match scope {
|
||||
FleetScope::Personal => personal_fleets_dir()?,
|
||||
FleetScope::Workspace => workspace_fleets_dir(workspace),
|
||||
///
|
||||
/// Returns the confinement root and the directory. They are one decision: a
|
||||
/// home workspace keeps its state behind a junction the user made, so the
|
||||
/// writer must open through the resolved root, or it would refuse the very
|
||||
/// directory this check just accepted.
|
||||
fn ensure_fleets_dir(
|
||||
scope: FleetScope,
|
||||
workspace: &Path,
|
||||
) -> Result<(PathBuf, PathBuf), FleetStoreError> {
|
||||
let (root, dir) = match scope {
|
||||
FleetScope::Personal => {
|
||||
let dir = personal_fleets_dir()?;
|
||||
(dir.clone(), dir)
|
||||
}
|
||||
FleetScope::Workspace => {
|
||||
codewhale_util::files::workspace_state_path(workspace, Path::new(FLEET_DIR))
|
||||
}
|
||||
};
|
||||
reject_linked_workspace_dir(scope, workspace, &dir)?;
|
||||
reject_linked_workspace_dir(scope, workspace)?;
|
||||
fs::create_dir_all(&dir).map_err(|e| FleetStoreError::Io {
|
||||
path: dir.display().to_string(),
|
||||
message: e.to_string(),
|
||||
})?;
|
||||
Ok(dir)
|
||||
Ok((root, dir))
|
||||
}
|
||||
|
||||
/// A workspace's `.codewhale/fleets` is repository content: refuse it when it,
|
||||
/// or `.codewhale`, is a link, so saving, selecting or deleting a Fleet cannot
|
||||
/// reach outside the workspace. The personal directory is the user's own.
|
||||
fn reject_linked_workspace_dir(
|
||||
scope: FleetScope,
|
||||
workspace: &Path,
|
||||
dir: &Path,
|
||||
) -> Result<(), FleetStoreError> {
|
||||
/// reach outside the workspace. The personal directory is the user's own, and
|
||||
/// so is a home workspace's relocated state directory: the root walk starts at
|
||||
/// the real directory there, so the user's own junction is not refused.
|
||||
fn reject_linked_workspace_dir(scope: FleetScope, workspace: &Path) -> Result<(), FleetStoreError> {
|
||||
if scope != FleetScope::Workspace {
|
||||
return Ok(());
|
||||
}
|
||||
codewhale_util::files::reject_linked_path(workspace, dir).map_err(|e| FleetStoreError::Io {
|
||||
let (root, dir) = codewhale_util::files::workspace_state_path(workspace, Path::new(FLEET_DIR));
|
||||
codewhale_util::files::reject_linked_path(&root, &dir).map_err(|e| FleetStoreError::Io {
|
||||
path: dir.display().to_string(),
|
||||
message: e.to_string(),
|
||||
})
|
||||
@@ -840,7 +852,7 @@ pub fn save_fleet(
|
||||
workspace: &Path,
|
||||
) -> Result<PathBuf, FleetStoreError> {
|
||||
fleet.validate()?;
|
||||
let dir = ensure_fleets_dir(scope, workspace)?;
|
||||
let (root, dir) = ensure_fleets_dir(scope, workspace)?;
|
||||
let path = dir.join(format!("{}.toml", fleet.file_slug()));
|
||||
if path.is_file() {
|
||||
// Only a readable v2 Fleet of this same name may be replaced. Any
|
||||
@@ -879,7 +891,7 @@ pub fn save_fleet(
|
||||
}
|
||||
}
|
||||
let rendered = fleet.render_toml()?;
|
||||
write_scoped(scope, workspace, &path, rendered.as_bytes())?;
|
||||
write_scoped(scope, &root, &path, rendered.as_bytes())?;
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
@@ -893,7 +905,7 @@ pub fn delete_fleet(
|
||||
FleetScope::Personal => personal_fleets_dir()?,
|
||||
FleetScope::Workspace => workspace_fleets_dir(workspace),
|
||||
};
|
||||
reject_linked_workspace_dir(scope, workspace, &dir)?;
|
||||
reject_linked_workspace_dir(scope, workspace)?;
|
||||
let path = dir.join(format!("{}.toml", slugify(name)));
|
||||
if !path.is_file() {
|
||||
return Err(FleetStoreError::NotFound(name.to_string()));
|
||||
@@ -1005,7 +1017,7 @@ pub fn set_selected(
|
||||
scope: FleetScope,
|
||||
workspace: &Path,
|
||||
) -> Result<PathBuf, FleetStoreError> {
|
||||
let dir = ensure_fleets_dir(scope, workspace)?;
|
||||
let (root, dir) = ensure_fleets_dir(scope, workspace)?;
|
||||
let name = name.trim();
|
||||
let exists_in_scope = |target: FleetScope| {
|
||||
let target_dir = match target {
|
||||
@@ -1025,7 +1037,7 @@ pub fn set_selected(
|
||||
)));
|
||||
}
|
||||
let selected = dir.join(SELECTED_FILE);
|
||||
write_scoped(scope, workspace, &selected, name.as_bytes())?;
|
||||
write_scoped(scope, &root, &selected, name.as_bytes())?;
|
||||
Ok(selected)
|
||||
}
|
||||
|
||||
@@ -1035,7 +1047,7 @@ fn clear_selection_if_matching(scope: FleetScope, workspace: &Path, name: &str)
|
||||
FleetScope::Workspace => Some(workspace_fleets_dir(workspace)),
|
||||
};
|
||||
let Some(dir) = dir else { return };
|
||||
if reject_linked_workspace_dir(scope, workspace, &dir).is_err() {
|
||||
if reject_linked_workspace_dir(scope, workspace).is_err() {
|
||||
return;
|
||||
}
|
||||
let selected = dir.join(SELECTED_FILE);
|
||||
@@ -1048,9 +1060,13 @@ fn clear_selection_if_matching(scope: FleetScope, workspace: &Path, name: &str)
|
||||
/// the pinned no-follow writer (parents created without following a link, the
|
||||
/// final name replaced, never written through); the personal scope is the
|
||||
/// user's own directory and keeps the plain atomic write.
|
||||
///
|
||||
/// `root` is the confinement root [`ensure_fleets_dir`] decided, not the raw
|
||||
/// workspace: for a home workspace it is the resolved state directory, so the
|
||||
/// writer opens the same path the link walk approved.
|
||||
fn write_scoped(
|
||||
scope: FleetScope,
|
||||
workspace: &Path,
|
||||
root: &Path,
|
||||
path: &Path,
|
||||
bytes: &[u8],
|
||||
) -> Result<(), FleetStoreError> {
|
||||
@@ -1061,13 +1077,11 @@ fn write_scoped(
|
||||
path: path.display().to_string(),
|
||||
message: e.to_string(),
|
||||
};
|
||||
let relative = path
|
||||
.strip_prefix(workspace)
|
||||
.map_err(|_| FleetStoreError::Io {
|
||||
path: path.display().to_string(),
|
||||
message: "a workspace Fleet file must stay within the workspace".to_string(),
|
||||
})?;
|
||||
codewhale_util::files::WorkspaceFile::open_shared(workspace, relative, true)
|
||||
let relative = path.strip_prefix(root).map_err(|_| FleetStoreError::Io {
|
||||
path: path.display().to_string(),
|
||||
message: "a workspace Fleet file must stay within the workspace".to_string(),
|
||||
})?;
|
||||
codewhale_util::files::WorkspaceFile::open_shared(root, relative, true)
|
||||
.and_then(|file| file.replace(bytes))
|
||||
.map_err(io_error)
|
||||
}
|
||||
|
||||
@@ -116,7 +116,7 @@ fn reject_linked_workspace_state_path(path: &Path) -> Result<()> {
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
if codewhale_paths::user_home().is_some_and(|home| home == root) {
|
||||
if codewhale_util::files::workspace_is_user_home(root) {
|
||||
return Ok(());
|
||||
}
|
||||
codewhale_util::files::reject_linked_path(root, path)
|
||||
|
||||
+52
-41
@@ -6741,12 +6741,14 @@ fn doctor_legacy_state_report(
|
||||
/// This is deliberately separate from `SessionManager::default_location()`:
|
||||
/// constructing the manager can trigger the additive legacy migration, while
|
||||
/// doctor must remain a read-only diagnostic. Session history is stored as
|
||||
/// top-level JSON files. Directories (including `checkpoints`) and symlinks
|
||||
/// observed during the scan are ignored, so the diagnostic does not
|
||||
/// intentionally traverse checkpoint internals or link targets. These checks
|
||||
/// are best-effort observations, not a race-free no-follow guarantee.
|
||||
/// A matching filename is only a regular-file counterpart check: doctor does
|
||||
/// not parse or compare session descriptors.
|
||||
/// top-level JSON files. The state and sessions roots may be links the user
|
||||
/// made to relocate state (a junction onto another volume is the supported way
|
||||
/// to keep `~/.codewhale` off the system drive on Windows), so a linked root is
|
||||
/// resolved and read through; symlink *entries* observed during the scan are
|
||||
/// still ignored, so the diagnostic does not traverse checkpoint internals or
|
||||
/// per-file link targets. These checks are best-effort observations, not a
|
||||
/// race-free no-follow guarantee. A matching filename is only a regular-file
|
||||
/// counterpart check: doctor does not parse or compare session descriptors.
|
||||
fn doctor_session_recovery_report(
|
||||
primary_root: &Path,
|
||||
legacy_root: &Path,
|
||||
@@ -6910,11 +6912,15 @@ fn doctor_session_recovery_report(
|
||||
|
||||
/// Validate a session-state directory from observed metadata.
|
||||
///
|
||||
/// `doctor` only compares top-level filenames. It rejects a state-root or
|
||||
/// sessions-root symlink observed during inspection rather than using it for a
|
||||
/// recovery suggestion. This is a best-effort observation, not a race-free
|
||||
/// no-follow guarantee. Missing paths are normal on a fresh install and are
|
||||
/// reported as `false`.
|
||||
/// `doctor` only compares top-level filenames. A directory the user relocated
|
||||
/// behind a link is resolved and inspected as its target: a junction is the
|
||||
/// supported way to keep `~/.codewhale` off the system drive on Windows, and
|
||||
/// refusing it left the migration diagnostic unable to run on exactly the
|
||||
/// layout it reports about. Only this directory check follows the link; the
|
||||
/// enumeration below still walks the path it was handed.
|
||||
///
|
||||
/// This is a best-effort observation, not a race-free no-follow guarantee.
|
||||
/// Missing paths are normal on a fresh install and are reported as `false`.
|
||||
fn doctor_session_directory_is_safe(path: &Path, label: &str) -> std::result::Result<bool, String> {
|
||||
let metadata = match std::fs::symlink_metadata(path) {
|
||||
Ok(metadata) => metadata,
|
||||
@@ -6926,13 +6932,20 @@ fn doctor_session_directory_is_safe(path: &Path, label: &str) -> std::result::Re
|
||||
));
|
||||
}
|
||||
};
|
||||
if metadata.file_type().is_symlink() {
|
||||
return Err(format!(
|
||||
"could not inspect {label} at {}: path is a symlink",
|
||||
codewhale_util::utils::display_path(path)
|
||||
));
|
||||
}
|
||||
if !metadata.file_type().is_dir() {
|
||||
let metadata = if codewhale_util::path_identity::metadata_is_link_or_reparse(&metadata) {
|
||||
match std::fs::metadata(path) {
|
||||
Ok(target) => target,
|
||||
Err(error) => {
|
||||
return Err(format!(
|
||||
"could not resolve {label} at {}: {error}",
|
||||
codewhale_util::utils::display_path(path)
|
||||
));
|
||||
}
|
||||
}
|
||||
} else {
|
||||
metadata
|
||||
};
|
||||
if !metadata.is_dir() {
|
||||
return Err(format!(
|
||||
"could not inspect {label} at {}: path is not a directory",
|
||||
codewhale_util::utils::display_path(path)
|
||||
@@ -15179,7 +15192,7 @@ mod doctor_legacy_state_tests {
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn doctor_session_recovery_rejects_a_symlinked_legacy_sessions_root() {
|
||||
fn doctor_session_recovery_follows_a_linked_legacy_sessions_root() {
|
||||
use std::os::unix::fs::symlink;
|
||||
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
@@ -15187,8 +15200,8 @@ mod doctor_legacy_state_tests {
|
||||
let external_sessions = tmp.path().join("external-sessions");
|
||||
fs::create_dir_all(&external_sessions).expect("external sessions");
|
||||
fs::write(
|
||||
external_sessions.join("must-not-be-enumerated.json"),
|
||||
b"session contents must stay unread",
|
||||
external_sessions.join("relocated.json"),
|
||||
b"a relocated session",
|
||||
)
|
||||
.expect("external session fixture");
|
||||
fs::create_dir_all(&legacy_root).expect("legacy root");
|
||||
@@ -15197,22 +15210,17 @@ mod doctor_legacy_state_tests {
|
||||
|
||||
let report = doctor_session_recovery_report(&primary_root, &legacy_root, false);
|
||||
|
||||
assert_eq!(report.status, DoctorSessionRecoveryStatus::ScanFailed);
|
||||
assert!(report.needs_attention());
|
||||
assert_eq!(report.legacy_session_file_count, 0);
|
||||
assert!(report.recoverable.is_empty());
|
||||
assert!(
|
||||
report
|
||||
.error
|
||||
.as_deref()
|
||||
.is_some_and(|error| error.contains("legacy sessions root")
|
||||
&& error.contains("path is a symlink"))
|
||||
);
|
||||
// A relocated state directory is the user's own; the diagnostic reads it
|
||||
// instead of giving up, which is what a junction layout needs to migrate.
|
||||
assert!(report.error.is_none(), "{:?}", report.error);
|
||||
assert_eq!(report.legacy_session_file_count, 1);
|
||||
assert_eq!(report.recoverable_file_count, 1);
|
||||
assert_eq!(report.status, DoctorSessionRecoveryStatus::MigrationPending);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn doctor_session_recovery_rejects_symlinked_primary_root_and_sessions_root() {
|
||||
fn doctor_session_recovery_follows_linked_primary_root_and_sessions_root() {
|
||||
use std::os::unix::fs::symlink;
|
||||
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
@@ -15223,10 +15231,11 @@ mod doctor_legacy_state_tests {
|
||||
symlink(&external_primary, &primary_root).expect("symlinked primary root");
|
||||
|
||||
let root_report = doctor_session_recovery_report(&primary_root, &legacy_root, false);
|
||||
assert_eq!(root_report.status, DoctorSessionRecoveryStatus::ScanFailed);
|
||||
assert!(root_report.error.as_deref().is_some_and(|error| {
|
||||
error.contains("primary state root") && error.contains("path is a symlink")
|
||||
}));
|
||||
assert!(root_report.error.is_none(), "{:?}", root_report.error);
|
||||
assert_eq!(
|
||||
root_report.status,
|
||||
DoctorSessionRecoveryStatus::NoLegacySessions
|
||||
);
|
||||
|
||||
fs::remove_file(&primary_root).expect("remove primary root symlink");
|
||||
fs::create_dir_all(&primary_root).expect("primary root");
|
||||
@@ -15234,13 +15243,15 @@ mod doctor_legacy_state_tests {
|
||||
.expect("symlinked primary sessions root");
|
||||
|
||||
let sessions_report = doctor_session_recovery_report(&primary_root, &legacy_root, false);
|
||||
assert!(
|
||||
sessions_report.error.is_none(),
|
||||
"{:?}",
|
||||
sessions_report.error
|
||||
);
|
||||
assert_eq!(
|
||||
sessions_report.status,
|
||||
DoctorSessionRecoveryStatus::ScanFailed
|
||||
DoctorSessionRecoveryStatus::NoLegacySessions
|
||||
);
|
||||
assert!(sessions_report.error.as_deref().is_some_and(|error| {
|
||||
error.contains("primary sessions root") && error.contains("path is a symlink")
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -3404,10 +3404,22 @@ fn apply_validated_profile_config(
|
||||
/// time, and reloads the hook set on return so the screen reflects the edit
|
||||
/// immediately.
|
||||
fn edit_project_hooks_from_tui(terminal: &mut AppTerminal, app: &mut App, config: &Config) {
|
||||
let path = app.workspace.join(".codewhale").join("hooks.toml");
|
||||
// A home workspace may keep `.codewhale` on another volume behind a junction
|
||||
// the user made; the walk starts at the real directory there, so the user's
|
||||
// own relocation is not refused, while a link below it still is. Every other
|
||||
// workspace keeps itself as the root with `.codewhale` in the walked path.
|
||||
let (state_root, path) = codewhale_util::files::workspace_state_path(
|
||||
&app.workspace,
|
||||
std::path::Path::new("hooks.toml"),
|
||||
);
|
||||
// The editor takes the spelling the user would type. A resolved root is a
|
||||
// verbatim path on Windows, which several terminal editors will not open,
|
||||
// and the file is the same one either way.
|
||||
let editor_path = app.workspace.join(".codewhale").join("hooks.toml");
|
||||
// A link in `.codewhale` or at hooks.toml is never created through, and the
|
||||
// editor is not pointed at one: the file must really live in the workspace.
|
||||
if let Err(error) = codewhale_util::files::reject_linked_path(&app.workspace, &path) {
|
||||
// editor is not pointed at one: the file must really live in the state
|
||||
// directory.
|
||||
if let Err(error) = codewhale_util::files::reject_linked_path(&state_root, &path) {
|
||||
app.push_status_toast(
|
||||
format!("Could not use {}: {error}", path.display()),
|
||||
StatusToastLevel::Warning,
|
||||
@@ -3417,7 +3429,7 @@ fn edit_project_hooks_from_tui(terminal: &mut AppTerminal, app: &mut App, config
|
||||
}
|
||||
if !path.exists()
|
||||
&& let Err(error) = codewhale_util::fs_confined::write(
|
||||
&app.workspace,
|
||||
&state_root,
|
||||
&path,
|
||||
codewhale_engine::hooks::PROJECT_HOOKS_TEMPLATE.as_bytes(),
|
||||
)
|
||||
@@ -3435,7 +3447,7 @@ fn edit_project_hooks_from_tui(terminal: &mut AppTerminal, app: &mut App, config
|
||||
app.use_alt_screen(),
|
||||
app.use_mouse_capture,
|
||||
app.use_bracketed_paste,
|
||||
&path,
|
||||
&editor_path,
|
||||
// Open the file, not a position in it: this edits hooks.toml whole.
|
||||
None,
|
||||
);
|
||||
@@ -3455,7 +3467,7 @@ fn edit_project_hooks_from_tui(terminal: &mut AppTerminal, app: &mut App, config
|
||||
let reloaded = app.hooks.config();
|
||||
let mut content = format!(
|
||||
"Reloaded hooks from {} — {} configured.",
|
||||
path.display(),
|
||||
editor_path.display(),
|
||||
reloaded.hooks.len()
|
||||
);
|
||||
// Project hooks are executable repository configuration; an
|
||||
|
||||
@@ -74,16 +74,16 @@ fn resolved_home_state(workspace: &Path) -> Option<std::path::PathBuf> {
|
||||
/// inside the path, so a repository-supplied link is refused exactly as it
|
||||
/// was before this helper existed.
|
||||
///
|
||||
/// `relative` is joined as given. Every caller passes literal path components
|
||||
/// it wrote itself; a `relative` carrying `..` or an absolute path would step
|
||||
/// outside the state directory, so this is asserted for those callers rather
|
||||
/// than defended against a caller that does not exist.
|
||||
/// `relative` is joined as given and must be a relative path of literal
|
||||
/// components. Every caller passes components it wrote itself; anything else
|
||||
/// could step outside the state directory this root exists to bound, so the
|
||||
/// contract is enforced in release builds too rather than only in debug ones.
|
||||
#[must_use]
|
||||
pub fn workspace_state_path(
|
||||
workspace: &Path,
|
||||
relative: &Path,
|
||||
) -> (std::path::PathBuf, std::path::PathBuf) {
|
||||
debug_assert!(
|
||||
assert!(
|
||||
path_is_confined(relative),
|
||||
"workspace state path must be a relative path of literal components: {}",
|
||||
relative.display()
|
||||
|
||||
Reference in new issue
Block a user