diff --git a/Cargo.lock b/Cargo.lock index ff190e07..ad7170a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1335,6 +1335,7 @@ dependencies = [ "hm-exec", "hm-plugin-protocol", "hm-render", + "hm-util", "percent-encoding", "reqwest", "secrecy", diff --git a/crates/hm-core/workspace.rs b/crates/hm-core/workspace.rs index 17c57e86..23603b90 100644 --- a/crates/hm-core/workspace.rs +++ b/crates/hm-core/workspace.rs @@ -1,28 +1,42 @@ //! Global utility for managing the workspace. +use std::io; use std::path::{Path, PathBuf}; use hm_config::Config; use hm_util::path::{AbsPath, AbsPathBuf}; use thiserror::Error; -/// Failure loading a directory as a [`Workspace`]. +/// Failure resolving or loading a directory as a [`Workspace`]. #[derive(Debug, Error)] pub enum LoadError { #[error("{} does not appear to be a valid path", .0.display())] InvalidPath(PathBuf), #[error("{} is not a harmont workspace. harmont workspaces must have a `.hm/` directory", .0.display())] InvalidWorkspace(PathBuf), + + /// The process working directory could not be read while resolving a root. + #[error("cannot determine current directory")] + CurrentDir(#[source] io::Error), + + /// Walked to the filesystem root without finding a `.hm/` directory. + #[error( + "no harmont workspace found\n → run from a directory that contains `.hm/`, or initialize one with `hm init`" + )] + NotFound, + #[error(transparent)] Config(#[from] hm_config::LoadError), } /// Utility for accessing well-known paths inside a workspace. /// -/// Construct with [`Workspace::load`] on a **specific** project root (the -/// directory that contains `.hm/`). Callers that need parent-directory -/// discovery should walk first (e.g. [`hm_util::dirs::find_project_root`]) -/// and only then call [`Workspace::load`]. +/// Two constructors: +/// +/// - [`Workspace::resolve`] — the one CLI verbs want. Honors a `--dir`-style +/// override, else walks up from the cwd. +/// - [`Workspace::load`] — validates one **specific** project root (the +/// directory that contains `.hm/`), with no discovery. #[derive(Debug)] pub struct Workspace { /// Absolute path to the `.hm` directory. @@ -37,6 +51,36 @@ pub struct Workspace { } impl Workspace { + /// Resolve the workspace a command should operate on. + /// + /// The single root-resolution rule for every verb that takes a `--dir`: + /// + /// - **`dir` given** — that directory is the project root, verbatim. No + /// walk-up: the backend runs discovery against cloned repo roots, and + /// walking up out of a clone could bind to an unrelated `.hm/` above it. + /// A relative path is resolved against the cwd, since a workspace root + /// must be absolute. + /// - **`dir` absent** — walk up from the cwd, so a verb works from any + /// subdirectory of the project. + /// + /// # Errors + /// + /// Returns [`LoadError`] if the cwd cannot be determined, no project root + /// is found while walking up, or the resolved root fails [`Self::load`]. + pub fn resolve(dir: Option<&Path>) -> Result { + let root = match dir { + Some(d) if d.is_absolute() => d.to_path_buf(), + Some(d) => std::env::current_dir() + .map_err(LoadError::CurrentDir)? + .join(d), + None => { + let start = std::env::current_dir().map_err(LoadError::CurrentDir)?; + hm_util::dirs::find_project_root(&start).ok_or(LoadError::NotFound)? + } + }; + Self::load(&root) + } + /// Attempt to load the given directory as a workspace, if it appears to be one. /// /// We label a workspace any directory which has a `.hm` directory within it. diff --git a/crates/hm-plugin-cloud/Cargo.toml b/crates/hm-plugin-cloud/Cargo.toml index 65e35fa7..9c696ceb 100644 --- a/crates/hm-plugin-cloud/Cargo.toml +++ b/crates/hm-plugin-cloud/Cargo.toml @@ -19,6 +19,7 @@ hm-exec = { workspace = true } hm-render = { workspace = true } hm-config = { workspace = true } hm-core = { workspace = true } +hm-util = { workspace = true } hm-plugin-protocol = { workspace = true } serde_json = { workspace = true } chrono = { workspace = true } diff --git a/crates/hm-plugin-cloud/src/settings.rs b/crates/hm-plugin-cloud/src/settings.rs index 598a79da..8018f383 100644 --- a/crates/hm-plugin-cloud/src/settings.rs +++ b/crates/hm-plugin-cloud/src/settings.rs @@ -15,6 +15,24 @@ use anyhow::{Context, Result}; use harmont_cloud::HarmontClient; use secrecy::{ExposeSecret, SecretString}; +/// The layered config for the current directory. +/// +/// Includes the project `.hm/config.toml` layer whenever the cwd is inside a +/// harmont project — [`ResolvedCtx::org`]'s own error text tells users to set +/// `[cloud] org` there, so it has to be read. Outside a project there simply is +/// no project layer, and the cloud verbs still work from anywhere. +/// +/// **Read-only callers only.** `hm cloud org switch` deliberately loads the user +/// layer alone: it saves back to the user file, and merging the project layer in +/// first would persist project-scoped values into `~/.config/hm/config.toml`. +fn config() -> Result { + let project = std::env::current_dir() + .ok() + .and_then(|cwd| hm_util::dirs::find_project_root(&cwd)) + .map(|root| hm_config::Config::project_config_path(&root)); + hm_config::Config::load(project.as_deref()).context("loading config") +} + /// Resolve the bearer token, or the shared "not logged in" error. /// /// Every authenticated path routes through here so the not-logged-in text @@ -56,7 +74,7 @@ impl ResolvedCtx { /// /// Returns an error if config can't be loaded or no token is available. pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { - let cfg = hm_config::Config::load(None).context("loading config")?; // user + env layering + let cfg = config()?; // user + project + env layering let api = cfg.cloud.api_url.clone(); let client = HarmontClient::with_base_url(token()?.expose_secret(), &api); Ok(( @@ -81,7 +99,7 @@ pub fn client() -> Result<(HarmontClient, ResolvedCtx)> { /// Returns an error if config can't be loaded, no token is available, or no /// organization is configured. pub fn raw_org_ctx() -> Result<(String, SecretString, String)> { - let cfg = hm_config::Config::load(None).context("loading config")?; + let cfg = config()?; let api = cfg.cloud.api_url.clone(); let token = token()?; let org = ResolvedCtx { @@ -98,7 +116,7 @@ pub fn raw_org_ctx() -> Result<(String, SecretString, String)> { /// /// Returns an error if config can't be loaded. pub fn anon_client() -> Result<(HarmontClient, String)> { - let cfg = hm_config::Config::load(None).context("loading config")?; + let cfg = config()?; let api = cfg.cloud.api_url.clone(); Ok((HarmontClient::anonymous(&api), api)) } diff --git a/crates/hm-plugin-cloud/src/verbs/org.rs b/crates/hm-plugin-cloud/src/verbs/org.rs index 6d9f76fe..cdf3708d 100644 --- a/crates/hm-plugin-cloud/src/verbs/org.rs +++ b/crates/hm-plugin-cloud/src/verbs/org.rs @@ -27,6 +27,9 @@ async fn switch(client: &harmont_cloud::HarmontClient, slug: &str) -> Result<()> .iter() .find(|o| o.slug == slug) .ok_or_else(|| anyhow::anyhow!("no organization with slug '{slug}'"))?; + // Load-mutate-save on the *user* config, so this deliberately does NOT use + // `settings::config()`: merging the project `.hm/config.toml` layer in first + // would persist project-scoped values into ~/.config/hm/config.toml. let mut cfg = hm_config::Config::load(None)?; cfg.cloud.org = Some(found.slug.clone()); cfg.save_user().context("saving config")?; diff --git a/crates/hm-plugin-cloud/src/verbs/secret.rs b/crates/hm-plugin-cloud/src/verbs/secret.rs index 9a388d14..5af4cd3f 100644 --- a/crates/hm-plugin-cloud/src/verbs/secret.rs +++ b/crates/hm-plugin-cloud/src/verbs/secret.rs @@ -17,11 +17,27 @@ use std::io::Read; use std::path::Path; use anyhow::{Context, Result, bail}; -use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; +use percent_encoding::{AsciiSet, NON_ALPHANUMERIC, utf8_percent_encode}; use secrecy::{ExposeSecret, SecretString}; use crate::cli::SecretCommand; +/// Characters to percent-encode in a single URL path segment. +/// +/// RFC 3986 leaves the *unreserved* set (`A-Z a-z 0-9 - . _ ~`) safe in a path +/// segment; everything else is escaped. `NON_ALPHANUMERIC` escapes all four of +/// `-._~` too, so they are removed here — secret names are conventionally +/// `[A-Za-z0-9_]`, and encoding `_` as `%5F` requests a different resource than +/// the one the user named. +/// +/// What must NOT be removed: `/` and space, which have to survive as `%2F` / +/// `%20` so a name like `a/b c` cannot escape its scope. +const SEGMENT: &AsciiSet = &NON_ALPHANUMERIC + .remove(b'-') + .remove(b'.') + .remove(b'_') + .remove(b'~'); + /// Where the secret value comes from on `set`. /// /// Exactly one source is allowed; the resolver rejects none and ambiguity. @@ -47,7 +63,7 @@ fn secrets_path(org: &str, pipeline: Option<&str>) -> String { /// Append a URL-path-escaped secret name to a collection path. fn secret_item_path(collection: &str, name: &str) -> String { - format!("{collection}/{}", utf8_percent_encode(name, NON_ALPHANUMERIC)) + format!("{collection}/{}", utf8_percent_encode(name, SEGMENT)) } /// Resolve the secret value from exactly one of: positional `VALUE`, @@ -318,6 +334,29 @@ mod tests { ); } + #[test] + fn unreserved_name_is_not_encoded() { + // RFC 3986 unreserved chars (incl. `-` `_` `.` `~`) must pass through. + // Escaping any of them requests a different resource than the user + // named. Pinned per-character: encoding these has regressed once + // already, via a blanket `NON_ALPHANUMERIC`. + let coll = secrets_path("acme", None); + for name in [ + "DEPLOY-TOKEN_v2.0~rc", + "DEPLOY_TOKEN", + "a-b", + "a.b", + "a~b", + "A-Za-z0-9-._~", + ] { + assert_eq!( + secret_item_path(&coll, name), + format!("/api/v0/organizations/acme/secrets/{name}"), + "unreserved char was escaped in {name:?}" + ); + } + } + #[test] fn item_path_escapes_funny_names() { let coll = secrets_path("acme", Some("ci")); @@ -432,11 +471,13 @@ mod tests { } #[test] - fn special_chars_are_encoded() { + fn reserved_chars_are_encoded() { + // The other half of the contract: anything outside the unreserved set + // must escape, so a name cannot break out of its path segment. let coll = secrets_path("acme", None); assert_eq!( - secret_item_path(&coll, "DEPLOY-TOKEN_v2.0~rc"), - "/api/v0/organizations/acme/secrets/DEPLOY%2DTOKEN%5Fv2%2E0%7Erc" + secret_item_path(&coll, "a/b c?d#e%f"), + "/api/v0/organizations/acme/secrets/a%2Fb%20c%3Fd%23e%25f" ); } diff --git a/crates/hm/src/cli/mod.rs b/crates/hm/src/cli/mod.rs index b1cd2875..a13efa62 100644 --- a/crates/hm/src/cli/mod.rs +++ b/crates/hm/src/cli/mod.rs @@ -107,7 +107,7 @@ pub async fn dispatch(command: Command, cli: &Cli) -> Result { match command { Command::Init(args) => crate::commands::init::handle(args).await.map(|()| 0), Command::Run(args) => { - let ctx = RunContext::from_cli(cli)?; + let ctx = RunContext::from_cli(cli, args.dir.as_deref())?; crate::commands::run::handle(args, ctx).await } Command::Pipelines(args) => crate::cli::pipelines::run(args).await.map(|()| 0), diff --git a/crates/hm/src/cli/pipelines.rs b/crates/hm/src/cli/pipelines.rs index 5ee250ee..223ace30 100644 --- a/crates/hm/src/cli/pipelines.rs +++ b/crates/hm/src/cli/pipelines.rs @@ -28,10 +28,19 @@ const EMPTY_ENVELOPE: &str = r#"{"schema_version":"1","pipelines":[]}"#; /// Returns an error if the engine can't start or the DSL runtime fails to /// evaluate the pipelines. pub async fn run(args: PipelinesArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, + use hm_core::WorkspaceLoadError as WsErr; + let workspace = match hm_core::Workspace::resolve(args.dir.as_deref()) { + Ok(w) => w, + // "Not a harmont project" is not an error here — it means this repo + // declares no pipelines. A malformed `.hm/config.toml` still fails + // loudly rather than masquerading as an empty repo. + Err(WsErr::NotFound | WsErr::InvalidPath(_) | WsErr::InvalidWorkspace(_)) => { + print!("{EMPTY_ENVELOPE}"); + return Ok(()); + } + Err(e) => return Err(e.into()), }; + let repo_root = workspace.path().as_path().to_path_buf(); if !detect::has_pipeline_files(&repo_root) { print!("{EMPTY_ENVELOPE}"); diff --git a/crates/hm/src/cli/render.rs b/crates/hm/src/cli/render.rs index cc10ca99..6b45f15b 100644 --- a/crates/hm/src/cli/render.rs +++ b/crates/hm/src/cli/render.rs @@ -26,10 +26,8 @@ pub struct RenderArgs { /// or the slug is unknown / fails to render (the available slugs are written to /// stderr by the DSL runtime). pub async fn run(args: RenderArgs) -> Result<()> { - let repo_root = match args.dir { - Some(d) => d, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + let workspace = hm_core::Workspace::resolve(args.dir.as_deref())?; + let repo_root = workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).context("detecting pipeline language")?; let engine = python_engine().context("initializing DSL engine")?; diff --git a/crates/hm/src/commands/run/mod.rs b/crates/hm/src/commands/run/mod.rs index d89427dc..a83265a1 100644 --- a/crates/hm/src/commands/run/mod.rs +++ b/crates/hm/src/commands/run/mod.rs @@ -344,12 +344,12 @@ fn git_remote_repo_name(root: &std::path::Path) -> Option { /// the DSL detection / pipeline-render step fails. async fn render_pipeline( args: &RunArgs, - _ctx: &RunContext, + ctx: &RunContext, ) -> Result<(std::path::PathBuf, String, String)> { - let repo_root = match args.dir.clone() { - Some(p) => p, - None => std::env::current_dir().context("cannot determine current directory")?, - }; + // The root the run executes against is the workspace's, not a second + // resolution from `--dir`/cwd — otherwise config could come from one tree + // and code from another. `RunContext` already resolved `--dir`. + let repo_root = ctx.workspace.path().as_path().to_path_buf(); detect::check_python(&repo_root).map_err(|e| HmError::DslEngine(format!("{e:#}")))?; let engine = diff --git a/crates/hm/src/context.rs b/crates/hm/src/context.rs index 95fc6a47..71c6bcb4 100644 --- a/crates/hm/src/context.rs +++ b/crates/hm/src/context.rs @@ -1,27 +1,9 @@ use std::io::IsTerminal; +use std::path::Path; use crate::cli::Cli; -use hm_core::Workspace; +use hm_core::{Workspace, WorkspaceLoadError}; use hm_render::OutputMode; -use thiserror::Error; - -/// Failure building a [`RunContext`]. -#[derive(Debug, Error)] -pub enum Error { - /// Process current directory could not be resolved. - #[error("cannot determine current directory")] - CurrentDir(#[source] std::io::Error), - - /// No directory containing `.hm/` was found above the process cwd. - #[error( - "no harmont workspace found\n → run from a directory that contains `.hm/`, or initialize one with `hm init`" - )] - NotFound, - - /// A discovered project root failed [`Workspace::load`]. - #[error(transparent)] - Workspace(#[from] hm_core::WorkspaceLoadError), -} /// Runtime context for commands that operate on a harmont project workspace. /// @@ -40,23 +22,20 @@ pub struct RunContext { } impl RunContext { - /// Build a [`RunContext`] from parsed CLI args. + /// Build a [`RunContext`] from parsed CLI args and the verb's `--dir`. /// - /// Discovers the project root by walking up from the current directory - /// ([`hm_util::dirs::find_project_root`]), then loads a [`Workspace`] for - /// that exact path. Walk-up is intentionally separate from - /// [`Workspace::load`]. + /// Root resolution is [`Workspace::resolve`]'s, shared with every other + /// `--dir` verb. Passing `dir` through matters: the workspace this loads is + /// the one the run executes against, so resolving it from the cwd while the + /// run used `--dir` would let config come from one tree and code from + /// another. /// /// # Errors /// - /// Returns [`Error`] if the current directory cannot be determined, no - /// project root is found, or workspace load fails. - pub fn from_cli(cli: &Cli) -> Result { - let start_dir = std::env::current_dir().map_err(Error::CurrentDir)?; - // Walk-up is separate from Workspace::load (which only validates the - // exact path). - let root = hm_util::dirs::find_project_root(&start_dir).ok_or(Error::NotFound)?; - let workspace = Workspace::load(&root)?; + /// Returns [`WorkspaceLoadError`] if the current directory cannot be + /// determined, no project root is found, or workspace load fails. + pub fn from_cli(cli: &Cli, dir: Option<&Path>) -> Result { + let workspace = Workspace::resolve(dir)?; let output = OutputMode::Human { // Single source of truth for the color/TTY rule (still honors --no-color).