diff --git a/crates/hk-core/src/sanitize.rs b/crates/hk-core/src/sanitize.rs index 9e619b2..7e04eca 100644 --- a/crates/hk-core/src/sanitize.rs +++ b/crates/hk-core/src/sanitize.rs @@ -1,3 +1,4 @@ +use crate::HkError; use anyhow::{Result, bail}; use std::path::{Path, PathBuf}; @@ -106,6 +107,60 @@ pub fn is_windows_abs_path(s: &str) -> bool { && (bytes[2] == b'\\' || bytes[2] == b'/') } +/// Resolve `~` and validate a user-supplied custom config path. +/// +/// Custom config paths are user-typed pointers to files/directories the user +/// wants HarnessKit to track. They are not subject to a home/project gate — +/// the OS already gates whether HK can read those paths via filesystem +/// permissions, and forcing HPC users (or anyone working outside `$HOME`) to +/// register a project just to track a single config file is unnecessary +/// friction. +/// +/// Used by both web (`hk-web::handlers::agents`) and desktop +/// (`hk-desktop::commands::settings`) so the two surfaces stay in lockstep. +/// +/// Returns `HkError` with kind-specific failure modes: +/// - `PathNotAllowed`: path contains `..` (literal traversal attempt) +/// - `Validation`: path does not exist, or equals home dir itself +pub fn resolve_and_validate_config_path(path: &str) -> Result { + let resolved = if path == "~" { + dirs::home_dir() + .map(|h| h.to_string_lossy().to_string()) + .unwrap_or_else(|| path.to_string()) + } else if let Some(rest) = path.strip_prefix("~/") { + dirs::home_dir() + .map(|h| h.join(rest).to_string_lossy().to_string()) + .unwrap_or_else(|| path.to_string()) + } else { + path.to_string() + }; + if resolved.contains("..") { + return Err(HkError::PathNotAllowed( + "Config paths cannot contain '..' components".into(), + )); + } + let resolved_path = Path::new(&resolved); + if !resolved_path.exists() { + return Err(HkError::Validation( + "Path does not exist on disk. Create the file or directory before adding it.".into(), + )); + } + // Compare canonical paths so trailing slashes, symlinks, and other + // surface variants (e.g. `/Users/zoe/` vs `/Users/zoe`, or a symlink + // pointing to home) all reject as "home itself". + let home = dirs::home_dir().unwrap_or_default(); + if let (Ok(rp), Ok(hp)) = ( + std::fs::canonicalize(resolved_path), + std::fs::canonicalize(&home), + ) && rp == hp + { + return Err(HkError::Validation( + "Cannot use home directory itself as a config path".into(), + )); + } + Ok(resolved) +} + #[cfg(test)] mod tests { use super::*; @@ -221,4 +276,67 @@ mod tests { assert!(!is_windows_abs_path("~/foo")); assert!(!is_windows_abs_path("C:")); // too short } + + /// D-spirit regression: a custom config path under a directory that is + /// neither home nor a registered project should be accepted. The OS + /// gates whether HK can later read the file; HK does not impose its + /// own home/project boundary on user-typed config paths. + #[test] + fn config_path_accepts_path_outside_home() { + let tmp = tempfile::tempdir().unwrap(); + let outside = tmp.path().join("custom_config.json"); + std::fs::write(&outside, "{}").unwrap(); + + let result = resolve_and_validate_config_path(&outside.to_string_lossy()); + assert!(result.is_ok(), "expected ok, got: {result:?}"); + } + + #[test] + fn config_path_rejects_dotdot() { + let tmp = tempfile::tempdir().unwrap(); + let path = format!("{}/foo/../bar.json", tmp.path().display()); + let result = resolve_and_validate_config_path(&path); + assert!(matches!(result, Err(HkError::PathNotAllowed(_)))); + } + + #[test] + fn config_path_rejects_nonexistent() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("does-not-exist.json"); + let result = resolve_and_validate_config_path(&path.to_string_lossy()); + assert!(matches!(result, Err(HkError::Validation(_)))); + } + + /// Bare "~" should expand to home and be rejected as "home itself" — not + /// treated as a literal "~" path which then fails the existence check. + #[test] + fn config_path_bare_tilde_expands_to_home_then_rejected() { + let result = resolve_and_validate_config_path("~"); + match result { + Err(HkError::Validation(msg)) => assert!( + msg.contains("home directory itself"), + "expected home-itself error, got: {msg}" + ), + other => panic!("expected Validation('home directory itself'), got: {other:?}"), + } + } + + /// Home with trailing slash (or any surface variant pointing at home) + /// should be caught by the canonicalize-based comparison. + #[test] + fn config_path_rejects_home_with_trailing_slash() { + let home = match dirs::home_dir() { + Some(h) => h, + None => return, // skip on environments without a home dir + }; + let with_trailing = format!("{}/", home.to_string_lossy()); + let result = resolve_and_validate_config_path(&with_trailing); + match result { + Err(HkError::Validation(msg)) => assert!( + msg.contains("home directory itself"), + "expected home-itself error, got: {msg}" + ), + other => panic!("expected Validation('home directory itself'), got: {other:?}"), + } + } } diff --git a/crates/hk-desktop/src/commands/extensions.rs b/crates/hk-desktop/src/commands/extensions.rs index bf81251..56befd6 100644 --- a/crates/hk-desktop/src/commands/extensions.rs +++ b/crates/hk-desktop/src/commands/extensions.rs @@ -1,5 +1,5 @@ use super::AppState; -use super::helpers::{FileEntry, is_path_within_allowed_dirs, list_dir_entries}; +use super::helpers::{FileEntry, list_dir_entries}; use hk_core::service::ExtensionContent; use hk_core::{HkError, manager, models::*, scanner, service}; use tauri::{Emitter, State}; @@ -51,31 +51,24 @@ pub fn uninstall_cli_binary(binary_path: String) -> Result<(), HkError> { /// List files in a skill directory as a shallow tree (2 levels deep). #[tauri::command] -pub fn list_skill_files(state: State, path: String) -> Result, HkError> { +pub fn list_skill_files( + _state: State, + path: String, +) -> Result, HkError> { let root = std::path::Path::new(&path); if !root.is_dir() { return Err(HkError::Validation("Path is not a directory".into())); } - if !is_path_within_allowed_dirs(root, &state)? { - return Err(HkError::PathNotAllowed( - "Path is not within a known agent or project directory".into(), - )); - } list_dir_entries(root, 0) } /// Open a file or directory in the system's default application. #[tauri::command] -pub fn open_in_system(state: State, path: String) -> Result<(), HkError> { +pub fn open_in_system(_state: State, path: String) -> Result<(), HkError> { let file_path = std::path::Path::new(&path); if !file_path.exists() { return Err(HkError::NotFound("Path does not exist".into())); } - if !is_path_within_allowed_dirs(file_path, &state)? { - return Err(HkError::PathNotAllowed( - "Path is not within a known agent or project directory".into(), - )); - } if file_path.is_file() { let allowed_extensions = [ "md", "txt", "json", "toml", "yaml", "yml", "xml", "js", "ts", "py", "rs", "go", "css", @@ -115,16 +108,11 @@ pub fn open_in_system(state: State, path: String) -> Result<(), HkErro /// Reveal a file or directory in the system file manager (Finder / Explorer). #[tauri::command] -pub fn reveal_in_file_manager(state: State, path: String) -> Result<(), HkError> { +pub fn reveal_in_file_manager(_state: State, path: String) -> Result<(), HkError> { let file_path = std::path::Path::new(&path); if !file_path.exists() { return Err(HkError::NotFound("Path does not exist".into())); } - if !is_path_within_allowed_dirs(file_path, &state)? { - return Err(HkError::PathNotAllowed( - "Path is not within a known agent or project directory".into(), - )); - } #[cfg(target_os = "macos")] { std::process::Command::new("open") diff --git a/crates/hk-desktop/src/commands/helpers.rs b/crates/hk-desktop/src/commands/helpers.rs index 7b7f744..71b175f 100644 --- a/crates/hk-desktop/src/commands/helpers.rs +++ b/crates/hk-desktop/src/commands/helpers.rs @@ -1,47 +1,5 @@ use hk_core::HkError; -/// Validate that a path is within a known agent directory, registered project, or the app data dir. -pub(super) fn is_path_within_allowed_dirs( - path: &std::path::Path, - state: &super::AppState, -) -> Result { - let canonical = path.canonicalize()?; - let adapters = &*state.adapters; - let store = state.store.lock(); - let projects = store.list_projects().unwrap_or_default(); - let custom_paths = store.list_all_custom_config_paths().unwrap_or_default(); - - let allowed = adapters.iter().any(|a| { - a.base_dir() - .canonicalize() - .is_ok_and(|d| canonical.starts_with(&d)) - || a.skill_dirs() - .iter() - .any(|sd| sd.canonicalize().is_ok_and(|d| canonical.starts_with(&d))) - || a.plugin_dirs() - .iter() - .any(|pd| pd.canonicalize().is_ok_and(|d| canonical.starts_with(&d))) - || a.mcp_config_path() - .canonicalize() - .is_ok_and(|d| canonical == d) - || a.global_settings_files() - .iter() - .any(|f| f.canonicalize().is_ok_and(|d| canonical == d)) - }) || projects.iter().any(|p| { - std::path::Path::new(&p.path) - .canonicalize() - .is_ok_and(|d| canonical.starts_with(&d)) - }) || custom_paths.iter().any(|p| { - std::path::Path::new(p) - .canonicalize() - .is_ok_and(|d| canonical.starts_with(&d)) - }) || dirs::home_dir() - .map(|h| h.join(".harnesskit")) - .and_then(|d| d.canonicalize().ok()) - .is_some_and(|d| canonical.starts_with(&d)); - Ok(allowed) -} - #[derive(serde::Serialize)] pub struct FileEntry { pub name: String, diff --git a/crates/hk-desktop/src/commands/settings.rs b/crates/hk-desktop/src/commands/settings.rs index 984ad99..618384e 100644 --- a/crates/hk-desktop/src/commands/settings.rs +++ b/crates/hk-desktop/src/commands/settings.rs @@ -1,5 +1,4 @@ use super::AppState; -use super::helpers::is_path_within_allowed_dirs; use hk_core::{HkError, models::*}; use tauri::State; @@ -104,7 +103,7 @@ pub fn toggle_by_pack( #[tauri::command] pub fn read_config_file_preview( - state: State, + _state: State, path: String, max_lines: Option, ) -> Result { @@ -113,12 +112,8 @@ pub fn read_config_file_preview( return Err(HkError::NotFound("File not found".into())); } - if !is_path_within_allowed_dirs(file_path, &state)? { - return Err(HkError::PathNotAllowed( - "Path is not within a known agent or project directory".into(), - )); - } - + // OS gates whether the user can read this file; HK does not impose its + // own home/project boundary on read-only previews of user-typed paths. if file_path.is_dir() { return Ok(render_dir_tree(file_path)); } @@ -226,33 +221,7 @@ pub fn add_custom_config_path( category: String, target_scope: ConfigScope, ) -> Result { - // Resolve ~ to home directory - let resolved = if path.starts_with("~/") { - dirs::home_dir() - .map(|h| h.join(&path[2..]).to_string_lossy().to_string()) - .unwrap_or(path.clone()) - } else { - path - }; - // Reject paths with ".." to prevent traversal bypass (e.g., ~/../../etc/passwd) - if resolved.contains("..") { - return Err(HkError::PathNotAllowed( - "Config paths cannot contain '..' components".into(), - )); - } - let resolved_path = std::path::Path::new(&resolved); - let home = dirs::home_dir() - .ok_or_else(|| HkError::Internal("Cannot determine home directory".into()))?; - if !resolved_path.starts_with(&home) { - return Err(HkError::PathNotAllowed( - "Custom config paths must be within your home directory".into(), - )); - } - if resolved_path == home { - return Err(HkError::Validation( - "Cannot use home directory itself as a config path".into(), - )); - } + let resolved = hk_core::sanitize::resolve_and_validate_config_path(&path)?; let scope_json = serde_json::to_string(&target_scope).ok(); let store = state.store.lock(); store.add_custom_config_path(&agent, &resolved, &label, &category, scope_json.as_deref()) @@ -266,31 +235,7 @@ pub fn update_custom_config_path( label: String, category: String, ) -> Result<(), HkError> { - let resolved = if path.starts_with("~/") { - dirs::home_dir() - .map(|h| h.join(&path[2..]).to_string_lossy().to_string()) - .unwrap_or(path.clone()) - } else { - path - }; - if resolved.contains("..") { - return Err(HkError::PathNotAllowed( - "Config paths cannot contain '..' components".into(), - )); - } - let resolved_path = std::path::Path::new(&resolved); - let home = dirs::home_dir() - .ok_or_else(|| HkError::Internal("Cannot determine home directory".into()))?; - if !resolved_path.starts_with(&home) { - return Err(HkError::PathNotAllowed( - "Custom config paths must be within your home directory".into(), - )); - } - if resolved_path == home { - return Err(HkError::Validation( - "Cannot use home directory itself as a config path".into(), - )); - } + let resolved = hk_core::sanitize::resolve_and_validate_config_path(&path)?; let store = state.store.lock(); store.update_custom_config_path(id, &resolved, &label, &category) } @@ -303,41 +248,7 @@ pub fn remove_custom_config_path(state: State, id: i64) -> Result<(), #[cfg(test)] mod tests { - use super::super::AppState; use super::*; - use hk_core::store::Store; - use parking_lot::Mutex; - use std::collections::HashMap; - use std::sync::Arc; - use tempfile::TempDir; - - fn test_state() -> (AppState, TempDir) { - let dir = tempfile::tempdir().unwrap(); - let store = Store::open(&dir.path().join("test.db")).unwrap(); - ( - AppState { - store: Arc::new(Mutex::new(store)), - adapters: Arc::new(hk_core::adapter::all_adapters()), - pending_clones: Arc::new(Mutex::new(HashMap::new())), - }, - dir, - ) - } - - #[test] - fn test_custom_paths_are_allowed_for_preview_and_open() { - let (state, dir) = test_state(); - let custom_dir = dir.path().join("custom"); - std::fs::create_dir_all(&custom_dir).unwrap(); - - state - .store - .lock() - .add_custom_config_path("claude", &custom_dir.to_string_lossy(), "", "settings", None) - .unwrap(); - - assert!(is_path_within_allowed_dirs(&custom_dir, &state).unwrap()); - } #[test] fn test_render_dir_tree_truncates_large_directories() { diff --git a/crates/hk-web/src/handlers/agents.rs b/crates/hk-web/src/handlers/agents.rs index 1523f2c..b26dc24 100644 --- a/crates/hk-web/src/handlers/agents.rs +++ b/crates/hk-web/src/handlers/agents.rs @@ -9,38 +9,6 @@ use crate::state::WebState; type Result = std::result::Result, ApiError>; -/// Resolve `~` and validate custom config paths (mirrors desktop logic). -fn resolve_and_validate_config_path( - path: &str, - store: &hk_core::store::Store, -) -> std::result::Result { - let resolved = if path.starts_with("~/") { - dirs::home_dir() - .map(|h| h.join(&path[2..]).to_string_lossy().to_string()) - .unwrap_or_else(|| path.to_string()) - } else { - path.to_string() - }; - if resolved.contains("..") { - return Err(hk_core::HkError::PathNotAllowed( - "Config paths cannot contain '..' components".into(), - )); - } - let resolved_path = std::path::Path::new(&resolved); - if !super::is_path_allowed(resolved_path, store) { - return Err(hk_core::HkError::PathNotAllowed( - "Custom config paths must be within your home directory or a registered project".into(), - )); - } - let home = dirs::home_dir().unwrap_or_default(); - if resolved_path == home { - return Err(hk_core::HkError::Validation( - "Cannot use home directory itself as a config path".into(), - )); - } - Ok(resolved) -} - pub async fn list_agents( State(state): State, ) -> Result> { @@ -227,8 +195,8 @@ pub async fn add_custom_config_path( Json(params): Json, ) -> Result { blocking(move || { + let resolved = hk_core::sanitize::resolve_and_validate_config_path(¶ms.path)?; let store = state.store.lock(); - let resolved = resolve_and_validate_config_path(¶ms.path, &store)?; let scope_json = serde_json::to_string(¶ms.target_scope).ok(); store.add_custom_config_path( ¶ms.agent, @@ -253,8 +221,8 @@ pub async fn update_custom_config_path( Json(params): Json, ) -> Result<()> { blocking(move || { + let resolved = hk_core::sanitize::resolve_and_validate_config_path(¶ms.path)?; let store = state.store.lock(); - let resolved = resolve_and_validate_config_path(¶ms.path, &store)?; store.update_custom_config_path(params.id, &resolved, ¶ms.label, ¶ms.category)?; Ok(()) }).await @@ -275,3 +243,4 @@ pub async fn remove_custom_config_path( Ok(()) }).await } + diff --git a/crates/hk-web/src/handlers/mod.rs b/crates/hk-web/src/handlers/mod.rs index f1be781..d5789c6 100644 --- a/crates/hk-web/src/handlers/mod.rs +++ b/crates/hk-web/src/handlers/mod.rs @@ -28,20 +28,114 @@ fn normalize(p: &Path) -> std::path::PathBuf { } /// Check if a path is within the home directory or any registered project path. +/// +/// Both the input path and each candidate root are canonicalized before +/// comparison so that environments where the home directory is a symlink +/// (e.g. Lumi's `/users/` → `/pfs/lustrep*/users/`) match +/// correctly. Non-existent paths fail canonicalization and are rejected, +/// matching the desktop's `is_path_within_allowed_dirs` behavior. pub(crate) fn is_path_allowed(path: &Path, store: &Store) -> bool { - let normalized = normalize(path); + let Ok(canonical) = std::fs::canonicalize(path) else { + return false; + }; + let canonical = normalize(&canonical); + + let under = |root: &Path| -> bool { + std::fs::canonicalize(root) + .map(|r| canonical.starts_with(normalize(&r))) + .unwrap_or(false) + }; + if let Some(home) = dirs::home_dir() - && normalized.starts_with(&home) + && under(&home) { return true; } if let Ok(projects) = store.list_projects() { for p in &projects { - let proj_path = normalize(Path::new(&p.path)); - if normalized.starts_with(&proj_path) { + if under(Path::new(&p.path)) { return true; } } } false } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use chrono::Utc; + use hk_core::models::Project; + use std::fs; + use std::os::unix::fs::symlink; + + fn test_store(db_dir: &Path) -> Store { + Store::open(&db_dir.join("test.db")).unwrap() + } + + fn register_project(store: &Store, path: &Path) { + store + .insert_project(&Project { + id: "proj-test".into(), + name: "test".into(), + path: path.to_string_lossy().to_string(), + created_at: Utc::now(), + exists: true, + }) + .unwrap(); + } + + /// Symlinked project root: a file path under the symlink should be allowed + /// even though canonicalize resolves it to a different real path. This is + /// the Lumi case (`/users/` → `/pfs/lustrep*/users/`). + #[test] + fn allows_path_under_symlinked_root() { + let tmp = tempfile::tempdir().unwrap(); + let real = tmp.path().join("real"); + let link = tmp.path().join("link"); + fs::create_dir_all(&real).unwrap(); + symlink(&real, &link).unwrap(); + let file = real.join("config.json"); + fs::write(&file, "{}").unwrap(); + + let store = test_store(tmp.path()); + register_project(&store, &link); + + // Caller passes a path through the symlink; canonical resolves to real. + let via_link = link.join("config.json"); + assert!(is_path_allowed(&via_link, &store)); + // Caller passes the already-canonical real path. + assert!(is_path_allowed(&file, &store)); + } + + /// Non-existent path: canonicalize fails, function rejects. Matches the + /// desktop's `is_path_within_allowed_dirs` behavior. + #[test] + fn rejects_nonexistent_path() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let store = test_store(tmp.path()); + register_project(&store, &project); + + let ghost = project.join("does-not-exist.json"); + assert!(!is_path_allowed(&ghost, &store)); + } + + /// Path outside any registered root: rejected. + #[test] + fn rejects_path_outside_roots() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("project"); + let outside = tmp.path().join("outside"); + fs::create_dir_all(&project).unwrap(); + fs::create_dir_all(&outside).unwrap(); + let stray = outside.join("file.json"); + fs::write(&stray, "{}").unwrap(); + + let store = test_store(tmp.path()); + register_project(&store, &project); + + assert!(!is_path_allowed(&stray, &store)); + } +} diff --git a/crates/hk-web/src/handlers/settings.rs b/crates/hk-web/src/handlers/settings.rs index 29265f0..6454f9c 100644 --- a/crates/hk-web/src/handlers/settings.rs +++ b/crates/hk-web/src/handlers/settings.rs @@ -150,7 +150,7 @@ pub struct ReadConfigPreviewParams { } pub async fn read_config_file_preview( - State(state): State, + State(_state): State, Json(params): Json, ) -> Result { blocking(move || { @@ -160,17 +160,15 @@ pub async fn read_config_file_preview( } let canonical = std::fs::canonicalize(file_path) .map_err(|_| hk_core::HkError::NotFound("File not found".into()))?; - let store = state.store.lock(); - if !super::is_path_allowed(&canonical, &store) { - return Err(hk_core::HkError::PermissionDenied("Path not allowed".into())); - } if file_path.is_dir() { return Ok(render_dir_tree(file_path)); } - let content = std::fs::read_to_string(&canonical) - .map_err(|_| hk_core::HkError::NotFound("Cannot read file".into()))?; + // OS gates whether the user can read this file; HK does not impose + // its own home/project boundary on read-only previews of user-typed + // config paths. + let content = std::fs::read_to_string(&canonical).map_err(hk_core::HkError::from)?; let max = params.max_lines.unwrap_or(30); let total_lines = content.lines().count(); let mut preview: String = content.lines().take(max).collect::>().join("\n"); diff --git a/src/components/agents/agent-detail.tsx b/src/components/agents/agent-detail.tsx index bc265b0..a74469b 100644 --- a/src/components/agents/agent-detail.tsx +++ b/src/components/agents/agent-detail.tsx @@ -146,7 +146,12 @@ export function AgentDetail() { value={customPath} onChange={(e) => setCustomPath(e.target.value)} onKeyDown={(e) => { - if (e.key === "Enter" && customPath.trim()) { + if ( + e.key === "Enter" && + !e.nativeEvent.isComposing && + e.keyCode !== 229 && + customPath.trim() + ) { // Add Custom Path lands in the current scope; All-scopes // mode falls back to Global since "all" is not a real // install target. diff --git a/src/components/extensions/install-dialog.tsx b/src/components/extensions/install-dialog.tsx index f9263f4..071b83f 100644 --- a/src/components/extensions/install-dialog.tsx +++ b/src/components/extensions/install-dialog.tsx @@ -6,8 +6,8 @@ import { useFocusTrap } from "@/hooks/use-focus-trap"; import { useScope } from "@/hooks/use-scope"; import { openDirectoryPicker } from "@/lib/dialog"; import { humanizeError } from "@/lib/errors"; -import { isDesktop } from "@/lib/transport"; import { api } from "@/lib/invoke"; +import { isDesktop } from "@/lib/transport"; import type { ConfigScope, DiscoveredSkill } from "@/lib/types"; import { agentDisplayName, sortAgents } from "@/lib/types"; import { useAgentStore } from "@/stores/agent-store"; @@ -258,7 +258,11 @@ export function InstallDialog({ open, mode, onClose }: InstallDialogProps) { value={source} onChange={(e) => setSource(e.target.value)} onKeyDown={(e) => - e.key === "Enter" && !loading && scanBtnRef.current?.click() + e.key === "Enter" && + !e.nativeEvent.isComposing && + e.keyCode !== 229 && + !loading && + scanBtnRef.current?.click() } placeholder={placeholder} aria-label={ diff --git a/src/lib/__tests__/errors.test.ts b/src/lib/__tests__/errors.test.ts index 0d65596..3e1cd32 100644 --- a/src/lib/__tests__/errors.test.ts +++ b/src/lib/__tests__/errors.test.ts @@ -19,13 +19,38 @@ describe("humanizeError", () => { expect(humanizeError("fatal: repository gone")).toContain("repository"); }); - it("detects permission/auth errors", () => { - expect(humanizeError("permission denied")).toContain("Access denied"); + it("detects HTTP auth errors", () => { expect(humanizeError("HTTP 403 Forbidden")).toContain("Access denied"); expect(humanizeError("401 Unauthorized")).toContain("Access denied"); expect(humanizeError("authentication required")).toContain("Access denied"); }); + it("passes plain filesystem 'permission denied' through (not git-flavored)", () => { + // No git-flavored "repository may be private" wording — that misled + // users when the real cause was filesystem perms or a stale custom path. + expect(humanizeError("permission denied")).toBe("permission denied"); + }); + + it("preserves PathNotAllowed message instead of collapsing to a generic sentence", () => { + // Different backend call sites encode different reasons — e.g. ".." + // rejection vs. policy violation. The humanizer used to flatten them all + // to "This path is not within an allowed directory." which hid the cause. + expect( + humanizeError({ + kind: "PathNotAllowed", + message: "Config paths cannot contain '..' components", + }), + ).toBe("Config paths cannot contain '..' components"); + }); + + it("accepts Tauri-shaped HkError objects (not just strings)", () => { + // Tauri IPC rejects with a reified object, not a JSON string. Earlier + // call sites wrapped errors in `String()` and produced "[object Object]". + expect(humanizeError({ kind: "Validation", message: "bad input" })).toBe( + "bad input", + ); + }); + it("detects not-found errors", () => { expect(humanizeError("not found")).toContain("Not found"); expect(humanizeError("404")).toContain("Not found"); diff --git a/src/lib/errors.ts b/src/lib/errors.ts index d70a87b..b132cd0 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -3,13 +3,14 @@ import { parseError } from "./error-types"; /** * Converts a raw backend error into a user-friendly message. - * Accepts either a raw string (legacy) or an HkError object. - * When an HkError with a `kind` is available, uses kind-based routing - * for consistent messaging; falls back to string heuristics for legacy errors. + * + * Accepts any error shape — strings (HTTP transport), HkError objects (Tauri + * IPC reject value), Error instances, or anything else. Routes everything + * through `parseError` so callers don't have to wrap errors in `String()` + * (which produces `"[object Object]"` for Tauri's reified HkError objects). */ -export function humanizeError(raw: string | HkError): string { - // If given a string, try parsing it as an HkError first - const err: HkError = typeof raw === "string" ? parseError(raw) : raw; +export function humanizeError(raw: unknown): string { + const err: HkError = parseError(raw); // Kind-based routing for typed errors const kindMessage = humanizeByKind(err.kind, err.message); @@ -26,9 +27,15 @@ function humanizeByKind(kind: HkErrorKind, message: string): string | null { case "NotFound": return `Not found: ${truncate(message, 100)}`; case "PermissionDenied": - return "Access denied. The repository may be private or require authentication."; + // Pass through verbatim. PermissionDenied covers both filesystem perms + // (io::Error) and HK web token auth — the previous git-flavored + // "repository may be private" message was misleading in both cases. + return truncate(message, 120); case "PathNotAllowed": - return "This path is not within an allowed directory."; + // Preserve the specific reason (e.g. "Config paths cannot contain '..' + // components") rather than collapsing every PathNotAllowed to a generic + // sentence — different backend call sites encode different reasons. + return truncate(message, 120); case "ConfigCorrupted": return "A configuration file appears to be corrupted. Try resetting it."; case "Database": @@ -67,8 +74,10 @@ function humanizeByMessage(raw: string): string { return "Could not access the repository. Check the URL and make sure it's publicly accessible."; } + // HTTP-style auth markers — typically come from git over https. Plain + // "permission denied" intentionally falls through to the default truncate + // since it's usually a filesystem-perm error, not a credentials issue. if ( - lower.includes("permission denied") || lower.includes("403") || lower.includes("401") || lower.includes("authentication") diff --git a/src/pages/settings.tsx b/src/pages/settings.tsx index 5dc7826..d4cf761 100644 --- a/src/pages/settings.tsx +++ b/src/pages/settings.tsx @@ -335,7 +335,12 @@ export default function SettingsPage() { aria-label={`${agent} config path`} onChange={(e) => setEditingPath(e.target.value)} onKeyDown={(e) => { - if (e.key === "Enter" && editingPath.trim()) { + if ( + e.key === "Enter" && + !e.nativeEvent.isComposing && + e.keyCode !== 229 && + editingPath.trim() + ) { updatePath(agent, editingPath.trim()); setEditingAgent(null); } @@ -434,7 +439,12 @@ export default function SettingsPage() { value={projectPathInput} onChange={(e) => setProjectPathInput(e.target.value)} onKeyDown={(e) => { - if (e.key === "Enter" && projectPathInput.trim()) + if ( + e.key === "Enter" && + !e.nativeEvent.isComposing && + e.keyCode !== 229 && + projectPathInput.trim() + ) handleAddPath(projectPathInput.trim()); }} className="flex-1 rounded-md border border-border bg-card px-3 py-1.5 text-sm text-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-ring" diff --git a/src/stores/agent-config-store.ts b/src/stores/agent-config-store.ts index aa0e03d..aafefa8 100644 --- a/src/stores/agent-config-store.ts +++ b/src/stores/agent-config-store.ts @@ -135,7 +135,7 @@ export const useAgentConfigStore = create((set, get) => ({ return content; } catch (error) { const nextErrors = new Map(get().previewErrors); - nextErrors.set(path, humanizeError(String(error))); + nextErrors.set(path, humanizeError(error)); set({ previewErrors: nextErrors }); return ""; } finally { @@ -193,8 +193,8 @@ export const useAgentConfigStore = create((set, get) => ({ await api.addCustomConfigPath(agent, path, label, category, targetScope); toast.success("Custom path added"); get().fetch(); - } catch { - toast.error("Failed to add custom path"); + } catch (error) { + toast.error(humanizeError(error)); } }, @@ -203,8 +203,8 @@ export const useAgentConfigStore = create((set, get) => ({ await api.updateCustomConfigPath(id, path, label, category); toast.success("Custom path updated"); get().fetch(); - } catch { - toast.error("Failed to update custom path"); + } catch (error) { + toast.error(humanizeError(error)); } }, @@ -213,8 +213,8 @@ export const useAgentConfigStore = create((set, get) => ({ await api.removeCustomConfigPath(id); toast.success("Custom path removed"); get().fetch(); - } catch { - toast.error("Failed to remove custom path"); + } catch (error) { + toast.error(humanizeError(error)); } }, }));