From c96c41450db568cc70ba708f9bcbac9c349e49a3 Mon Sep 17 00:00:00 2001 From: James Kominick Date: Fri, 17 Jul 2026 21:10:06 -0400 Subject: [PATCH] feat: add `check_install_path_writable` preflight and `Error::InstallPathNotWritable`, name the install path in install errors - new opt-in builder setter `check_install_path_writable(bool)` (default off, all backends): probe `bin_install_path` writability before the download and fail with the new `Error::InstallPathNotWritable { path }` on a definite permission refusal, before any bytes are fetched. Indeterminate probe results proceed. Sync and async flows. (#112) - install-step IO failures now always name the install path: `PermissionDenied` from self_replace / move-to-dest becomes `InstallPathNotWritable`; any other IO error is rewrapped with "installing to {path}" context, preserving the `ErrorKind`. - the crate never escalates privileges (documented non-goal); a new Permissions section in the crate docs shows detecting the error and relaunching elevated via the `restart` module as the application's choice. - hermetic integration tests in tests/permissions.rs prove a refused preflight downloads nothing (recording client, zero requests) with async parity. --- CHANGELOG.md | 9 + README.md | 41 ++++ specs/ref-errors.md | 18 +- specs/ref-update-pipeline.md | 22 +- src/backends/common.rs | 8 + src/errors.rs | 61 ++++++ src/lib.rs | 41 ++++ src/macros.rs | 19 ++ src/update.rs | 199 ++++++++++++++++++- tests/permissions.rs | 375 +++++++++++++++++++++++++++++++++++ 10 files changed, 785 insertions(+), 8 deletions(-) create mode 100644 tests/permissions.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f509fb..7352d7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,11 @@ client-less build (e.g. `default-features = false, features = ["rustls"]`) produces, and shows the fix (add a client, e.g. `features = ["ureq", "rustls", "github"]`). ([#168](https://github.com/jaemk/self_update/issues/168)) +- `check_install_path_writable(bool)` builder setter (default `false`) and + `Error::InstallPathNotWritable { path: PathBuf }`: opt-in preflight that probes `bin_install_path` + writability before the download; only a definite `PermissionDenied` refusal errors, indeterminate + results proceed. The install step also raises this error on a permission failure, always naming + the path. ([#112](https://github.com/jaemk/self_update/issues/112)) ### Changed - A recognized-but-unsupported compression extension now fails loudly instead of silently @@ -64,6 +69,10 @@ the `compression-tar-xz` feature returns `Error::CompressionNotEnabled("xz")` (matching the existing `.gz` handling), rather than writing the compressed archive to the install path. ([#143](https://github.com/jaemk/self_update/issues/143)) +- Install-step IO failures now name the install path: `PermissionDenied` becomes + `Error::InstallPathNotWritable { path }` and any other IO error becomes `Error::Io` with the + install path embedded in the message (the `ErrorKind` is preserved). + ([#112](https://github.com/jaemk/self_update/issues/112)) ### Removed diff --git a/README.md b/README.md index 8410e49..5b42f39 100644 --- a/README.md +++ b/README.md @@ -283,6 +283,47 @@ with a fresh argument list (e.g. to drop an `--upgrade` flag so the new process again). On unix the process image is replaced with `exec` (the PID is preserved); on windows the new binary is spawned and the current process exits. See the module docs for the platform details. +### Permissions + +The crate never escalates privileges. There is no sudo re-exec, no polkit interaction, and no UAC +prompt. Privilege escalation is always the caller's choice. + +An install into an unwritable location fails with +[`Error::InstallPathNotWritable`](crate::errors::Error::InstallPathNotWritable) naming the path +(the configured `bin_install_path`). Any other IO failure at the install step surfaces as +[`Error::Io`](crate::errors::Error::Io) with a message naming the install path, so the path is +visible in the error regardless of the kind. + +Setting `check_install_path_writable(true)` on the builder opts into a preflight probe that runs +immediately before the download. Only a definite `PermissionDenied` refusal errors early; +indeterminate results (a missing parent directory, an unusual filesystem) are treated as "proceed" +and let the real install step surface the outcome. The default is `false`. + +```rust +fn update() -> Result<(), Box> { + match self_update::backends::github::Update::configure() + .repo_owner("owner") + .repo_name("repo") + .bin_name("app") + .current_version(self_update::cargo_crate_version!()) + .check_install_path_writable(true) + .build()? + .update() + { + Ok(status) => println!("updated: {}", status.version()), + Err(self_update::Error::InstallPathNotWritable { .. }) => { + // The install path is not writable by this process. Elevation is the + // application's choice: re-run under sudo, spawn a UAC-elevated child, etc. + // Use the `restart` module for the exec/spawn mechanics when relaunching + // with a modified argument list. + eprintln!("install path not writable; re-run with elevated privileges"); + } + Err(e) => return Err(e.into()), + } + Ok(()) +} +``` + ### Periodic update checks Every `update()` / `is_update_available()` call makes a network request. To avoid checking on every diff --git a/specs/ref-errors.md b/specs/ref-errors.md index 2653a1f..22cf3f3 100644 --- a/specs/ref-errors.md +++ b/specs/ref-errors.md @@ -35,6 +35,7 @@ code builds them via the public constructors (`http_status_error(404, ..)`, | `MissingAssetField { field: String }` | A release/asset payload was missing a required field (`url`/`name`/`tag_name`/`created_at`/`assets`/`browser_download_url`/`assets.links`) in each backend's DTO conversion (`github.rs`, `gitlab.rs`, `gitea.rs`). `String` so a custom source can report a dynamic field path (e.g. `assets[2].url`). `#[non_exhaustive]`. | none | no (struct fields) | | `InvalidResponse { source: Box }` | A backend response could not be parsed: a malformed (non-array) JSON release-listing body (`github.rs`, `gitlab.rs`, `gitea.rs`), the S3 listing regex build failure, and the S3 XML parse failure (`s3.rs`). The underlying error is carried as `source`. `#[non_exhaustive]`. | none | yes (boxed source) | | `MissingField { field: &'static str }` | A required builder/configuration field was not set: `current_version`/`bin_name`/`bin_path_in_archive` (`common.rs`), `version` (`update.rs`), `source` (`custom.rs`), `repo_owner`/`repo_name` (`github.rs`, `gitlab.rs`, `gitea.rs`), `host` (`gitea.rs`), `bucket_name`/`region` (`s3.rs`). `#[non_exhaustive]`. | none | no (struct fields) | +| `InstallPathNotWritable { path: PathBuf }` | The opt-in preflight probe (`check_install_path_writable(true)`, `probe_install_path_writable` at `update.rs:1606`) when the path is definitely not writable, or the install step (`map_install_io_error` at `update.rs:1582`) when the replace/move fails with `PermissionDenied`. `path` is the configured `bin_install_path`. `#[non_exhaustive]`. | none | no (struct fields) | | `InvalidHeader { source: Box }` | A request header (`request_header` on the builders or on `Download`) was not a valid HTTP header. The setters are infallible; the error is deferred and surfaced from `build()` (via `common.rs`) or from `Download::download_to` / `download_to_async` (`lib.rs`). The source is a crate-internal `MessageError` carrying the validation message. `#[non_exhaustive]`. | none | yes (boxed source) | | `InvalidAuthToken { source: Box }` | An auth token could not be encoded as an HTTP `Authorization` header value (`github.rs`, `gitlab.rs`, `gitea.rs`, `update.rs`). The underlying header-value parse error is carried as `source`. `#[non_exhaustive]`. | none | yes (boxed source) | | `InvalidCertificate { source: Box }` | A custom TLS root certificate could not be parsed, or the HTTP client that would trust it could not be built. Produced by `RequestConfig::check()` (`common.rs`, surfaced from `build()`) and by `Download::download_to` / `download_to_async` (`lib.rs`) when `add_root_certificate` certs are supplied. Exception: on a ureq-only build a malformed **DER** certificate is not caught at `build()` (ureq's `from_der` is infallible) and surfaces as `Transport` at connection time; PEM is validated at `build()` on both clients. `#[non_exhaustive]`. | none | yes (boxed source) | @@ -126,6 +127,7 @@ Each variant renders with a specific Display string: - `MissingAssetField { field }` -> `"ReleaseError: release/asset payload missing \`{field}\`"` - `InvalidResponse { source }` -> `"ReleaseError: invalid response: {source}"` - `MissingField { field }` -> `"ConfigError: \`{field}\` required"` +- `InstallPathNotWritable { path }` -> `"InstallPathNotWritableError: cannot write to install path {path}: run with elevated privileges or choose a user-writable bin_install_path"` - `InvalidHeader { source }` -> `"ConfigError: invalid HTTP header: {source}"` - `InvalidAuthToken { source }` -> `"ConfigError: failed to parse auth token: {source}"` - `InvalidCertificate { source }` -> `"ConfigError: invalid root certificate: {source}"` @@ -157,8 +159,9 @@ boxed-source variants `InvalidResponse`, `InvalidHeader`, `InvalidAuthToken`, `Internal` when its `source` is `Some` -- each via deref of the box. The `Internal { source: None }` form and all field-only variants (`VerificationRejected`, `ChecksumMismatch`, `Aborted`, `NotFound`, `Unauthorized`, `HttpStatus`, -`NoReleaseFound`, `MissingAssetField`, `MissingField`, `ArchiveNotEnabled`, -`CompressionNotEnabled`, `InvalidAssetName`, `NoSignatures`, `SignatureNonUTF8`) return `None`. The concrete inner error of +`NoReleaseFound`, `MissingAssetField`, `MissingField`, `InstallPathNotWritable`, +`ArchiveNotEnabled`, `CompressionNotEnabled`, `InvalidAssetName`, `NoSignatures`, +`SignatureNonUTF8`) return `None`. The concrete inner error of a boxed variant is reachable at runtime through `source()` and `downcast_ref::()` (e.g. `err.source().and_then(|s| s.downcast_ref::())`). @@ -267,8 +270,9 @@ type directly, since `std::io::Error` is stable std.) - A user-declined confirmation prompt produces `Error::Aborted`. - Every struct-form variant carries `#[non_exhaustive]` on the variant (`Unauthorized`, `HttpStatus`, `Internal`, `VerificationRejected`, `NoReleaseFound`, `MissingAssetField`, - `InvalidResponse`, `MissingField`, `InvalidHeader`, `InvalidAuthToken`, `InvalidCertificate`, - `InvalidProgressStyle`, `InvalidAssetName`, `NotFound`, `ChecksumMismatch`). + `InvalidResponse`, `MissingField`, `InstallPathNotWritable`, `InvalidHeader`, `InvalidAuthToken`, + `InvalidCertificate`, `InvalidProgressStyle`, `InvalidAssetName`, `NotFound`, + `ChecksumMismatch`). - `Error::Internal` is reserved for genuine internal/invariant failures: extractor invariants, archive-path failures, and tokio blocking-task join failures (which carry the `JoinError` as `source`). @@ -276,6 +280,12 @@ type directly, since `std::io::Error` is stable std.) - The sites that previously stringified-and-discarded a source now chain it via `source()`: the S3 XML/regex parse (`InvalidResponse`), the auth-token header-value parse (`InvalidAuthToken`), and the tokio `JoinError` sites (`Internal`). +- `Error::InstallPathNotWritable { path }` names the `bin_install_path` that could not be + written. It carries no source and exposes no `http_status()`/`url()`. Display prefix is + `"InstallPathNotWritableError: "`. Raised by the opt-in preflight probe + (`check_install_path_writable(true)`) on a definite `PermissionDenied`, and always by the + install step on a permission failure. Other install-step IO errors map to `Error::Io` with the + path embedded in the message, `ErrorKind` preserved. - `Error::Config(String)` no longer exists. Its former producers route to structured variants: the `s3-auth` SigV4 host-extraction site (`s3.rs`) -> `S3Auth`; the root-certificate/client-build failures in `RequestConfig::check()` (`common.rs`) and `Download::download_to` / diff --git a/specs/ref-update-pipeline.md b/specs/ref-update-pipeline.md index 6a89261..21930cb 100644 --- a/specs/ref-update-pipeline.md +++ b/specs/ref-update-pipeline.md @@ -59,8 +59,12 @@ and `update_extended_async`'s future stays `Send` (the `PageRequest::parse` pars ### Download `resolve_and_confirm` prints the release-status block and (unless `no_confirm`) prompts -(see below). Then a `tempfile::TempDir` is created and the asset is downloaded to -`/` (`update.rs:608-613`). `build_download` (`update.rs:741`) builds the +(see below). If `check_install_path_writable()` is `true`, `probe_install_path_writable` +(`update.rs:1606`) runs immediately after the confirmation and before any download +(`update.rs:1005-1009` sync, `update.rs:1509-1510` async): only a definite `PermissionDenied` +errors as `Error::InstallPathNotWritable { path }`; any other result (missing parent directory, +unusual filesystem, `Ok`) proceeds. Default is `false` (off). Then a `tempfile::TempDir` is +created and the asset is downloaded to `/` (`update.rs:608-613`). `build_download` (`update.rs:741`) builds the `Download` from the asset URL, applies auth/`api_headers`, sets `ACCEPT: application/octet-stream`, merges the user's `request_headers()` *after* (so a same-named user header overrides), forwards the injected HTTP client, per-request timeout, progress @@ -126,6 +130,13 @@ and renames it back if the source->dest rename fails (rollback). `rename` cannot filesystems, so source, dest, and temp must share one. The high-level flow does not call `replace_using_temp`. +Both the `self_replace` call and the `Move::to_dest` call have their IO errors wrapped by +`map_install_io_error` (`update.rs:1582`): a `PermissionDenied` becomes +`Error::InstallPathNotWritable { path }` naming the install path; any other `io::Error` kind is +rewrapped as `Error::Io` with the message `"installing to {path}: {orig}"`, preserving the +original `ErrorKind` for inspection. This annotation is always on, independent of the opt-in +preflight probe (`check_install_path_writable`). + ### Multi-file install `MoveAll` (`lib.rs:988`) is the transactional multi-file primitive, not used by the @@ -216,6 +227,13 @@ under feature `async`; the free `update::update_extended_async` they route to is `!no_confirm`. Suppressing one does not suppress the other. - The retry budget covers the download's request-establishment phase (before bytes stream); mid-stream failures are not retried. User `request_headers` override the crate's ACCEPT/auth headers on the download. +- When `check_install_path_writable` is `true`, the preflight probe (`probe_install_path_writable`, + `update.rs:1606`) runs after confirmation and before any download; only a definite + `PermissionDenied` errors, indeterminate results proceed. Default is `false`. +- The install step always annotates IO failures with the install path: `PermissionDenied` becomes + `Error::InstallPathNotWritable { path }` and other kinds become `Error::Io` with the path in the + message, `ErrorKind` preserved (`map_install_io_error`, `update.rs:1582`). Independent of the + preflight probe. - `update()` reports `VersionStatus` (version only); `update_extended()` reports `ReleaseStatus` (`UpToDate` or `Updated(Release)`). - The async path never blocks the executor on the finish tail: `finish_update_owned` runs inside diff --git a/src/backends/common.rs b/src/backends/common.rs index 4452116..1a4849e 100644 --- a/src/backends/common.rs +++ b/src/backends/common.rs @@ -440,6 +440,10 @@ pub(crate) struct CommonBuilderConfig { pub asset_identifier: Option, pub bin_name: Option, pub bin_install_path: Option, + /// Opt-in preflight: probe `bin_install_path` writability before any download, failing early + /// with [`Error::InstallPathNotWritable`] on a definite permission refusal. Default `false`; + /// set via `check_install_path_writable(true)`. + pub check_install_path_writable: bool, pub bin_path_in_archive: Option, /// `true` when `bin_path_in_archive` was auto-derived from `bin_name` (not set explicitly by /// the user). Used by `bin_name` to re-derive when called again, while leaving an explicitly @@ -485,6 +489,7 @@ impl Default for CommonBuilderConfig { asset_identifier: None, bin_name: None, bin_install_path: None, + check_install_path_writable: false, bin_path_in_archive: None, bin_path_in_archive_auto: false, show_download_progress: false, @@ -550,6 +555,7 @@ impl CommonBuilderConfig { Some(p) => p.clone(), None => std::env::current_exe()?, }, + check_install_path_writable: self.check_install_path_writable, bin_path_in_archive: self .bin_path_in_archive .clone() @@ -589,6 +595,8 @@ pub(crate) struct CommonConfig { pub tag_prefix: Option, pub bin_name: String, pub bin_install_path: PathBuf, + /// Opt-in preflight writability probe of `bin_install_path` (default `false`). + pub check_install_path_writable: bool, pub bin_path_in_archive: String, pub show_download_progress: bool, pub show_output: bool, diff --git a/src/errors.rs b/src/errors.rs index 8432f44..3f6fb5c 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -125,6 +125,18 @@ pub enum Error { /// The name of the missing required field. field: &'static str, }, + /// The binary's install path (or its parent directory) is not writable by this process. + /// + /// Returned either by the opt-in preflight writability check + /// (`check_install_path_writable(true)`), which probes before any download, or by the install + /// step itself when the replace/move fails with a permission error. `path` is the configured + /// `bin_install_path`. Re-run with elevated privileges, or choose a user-writable + /// `bin_install_path`; the crate never escalates privileges itself. + #[non_exhaustive] + InstallPathNotWritable { + /// The install path (`bin_install_path`) that could not be written. + path: std::path::PathBuf, + }, /// A bare release listing ([`ReleaseList::fetch`](crate::backends)) carries no current version, /// so [`Releases::is_update_available`](crate::update::Releases::is_update_available) has nothing /// to compare its releases against. @@ -407,6 +419,12 @@ impl std::fmt::Display for Error { } InvalidResponse { source } => write!(f, "ReleaseError: invalid response: {}", source), MissingField { field } => write!(f, "ConfigError: `{}` required", field), + InstallPathNotWritable { path } => write!( + f, + "InstallPathNotWritableError: cannot write to install path {}: run with elevated \ + privileges or choose a user-writable bin_install_path", + path.display() + ), NoCurrentVersion => write!( f, "ReleaseError: this Releases has no current_version to compare against; use \ @@ -1235,6 +1253,49 @@ mod tests { assert_eq!(err.url(), None); } + // `InstallPathNotWritable` Display names the path and suggests elevated privileges or a + // user-writable bin_install_path. It carries no source and exposes no http_status()/url(). + #[test] + fn install_path_not_writable_display_and_no_source() { + let err = Error::InstallPathNotWritable { + path: std::path::PathBuf::from("/usr/local/bin/app"), + }; + let shown = err.to_string(); + assert!( + shown.starts_with("InstallPathNotWritableError: "), + "InstallPathNotWritable Display must keep the greppable prefix, got: {shown}" + ); + assert!( + shown.contains("/usr/local/bin/app"), + "InstallPathNotWritable Display must name the path, got: {shown}" + ); + assert!( + shown.contains("elevated privileges") && shown.contains("bin_install_path"), + "InstallPathNotWritable Display must suggest elevated privileges or a user-writable \ + bin_install_path, got: {shown}" + ); + assert!( + err.source().is_none(), + "InstallPathNotWritable carries no source, got {:?}", + err.source() + ); + assert_eq!(err.http_status(), None); + assert_eq!(err.url(), None); + } + + // `InstallPathNotWritable` is `#[non_exhaustive]`; a `..`-destructure that reads `path` must + // compile (adding a field stays non-breaking for downstream matchers). + #[test] + fn install_path_not_writable_is_non_exhaustive_struct_variant() { + let err = Error::InstallPathNotWritable { + path: std::path::PathBuf::from("/opt/app"), + }; + let Error::InstallPathNotWritable { path, .. } = err else { + panic!("expected InstallPathNotWritable"); + }; + assert_eq!(path, std::path::PathBuf::from("/opt/app")); + } + // `NoCurrentVersion` is a distinct, self-describing variant (not `MissingField`): its Display // names the missing current_version and points at `Update::is_update_available`, carries no // source, and exposes no http_status()/url(). Pins that the bare-listing precheck error is not diff --git a/src/lib.rs b/src/lib.rs index c07b46b..3e42ec6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -288,6 +288,47 @@ with a fresh argument list (e.g. to drop an `--upgrade` flag so the new process again). On unix the process image is replaced with `exec` (the PID is preserved); on windows the new binary is spawned and the current process exits. See the module docs for the platform details. +### Permissions + +The crate never escalates privileges. There is no sudo re-exec, no polkit interaction, and no UAC +prompt. Privilege escalation is always the caller's choice. + +An install into an unwritable location fails with +[`Error::InstallPathNotWritable`](crate::errors::Error::InstallPathNotWritable) naming the path +(the configured `bin_install_path`). Any other IO failure at the install step surfaces as +[`Error::Io`](crate::errors::Error::Io) with a message naming the install path, so the path is +visible in the error regardless of the kind. + +Setting `check_install_path_writable(true)` on the builder opts into a preflight probe that runs +immediately before the download. Only a definite `PermissionDenied` refusal errors early; +indeterminate results (a missing parent directory, an unusual filesystem) are treated as "proceed" +and let the real install step surface the outcome. The default is `false`. + +```rust,no_run +fn update() -> Result<(), Box> { + match self_update::backends::github::Update::configure() + .repo_owner("owner") + .repo_name("repo") + .bin_name("app") + .current_version(self_update::cargo_crate_version!()) + .check_install_path_writable(true) + .build()? + .update() + { + Ok(status) => println!("updated: {}", status.version()), + Err(self_update::Error::InstallPathNotWritable { .. }) => { + // The install path is not writable by this process. Elevation is the + // application's choice: re-run under sudo, spawn a UAC-elevated child, etc. + // Use the `restart` module for the exec/spawn mechanics when relaunching + // with a modified argument list. + eprintln!("install path not writable; re-run with elevated privileges"); + } + Err(e) => return Err(e.into()), + } + Ok(()) +} +``` + ### Periodic update checks Every `update()` / `is_update_available()` call makes a network request. To avoid checking on every diff --git a/src/macros.rs b/src/macros.rs index 8e560e1..274e675 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -288,6 +288,9 @@ macro_rules! impl_update_config_accessors { fn bin_install_path(&self) -> &std::path::Path { &self.common.bin_install_path } + fn check_install_path_writable(&self) -> bool { + self.common.check_install_path_writable + } fn bin_path_in_archive(&self) -> &str { &self.common.bin_path_in_archive } @@ -559,6 +562,22 @@ macro_rules! impl_common_builder_setters { self } + /// Opt-in preflight: probe whether `bin_install_path` is writable *before* anything is + /// downloaded, failing fast with + /// [`Error::InstallPathNotWritable`](crate::errors::Error::InstallPathNotWritable) when it + /// is definitely not (so a long download is not wasted only to hit a permission error at + /// the final replace step). Defaults to `false` (off). + /// + /// The probe is conservative: only a definite permission refusal errors. Indeterminate + /// results (a missing parent directory, an unusual filesystem, any non-permission IO error) + /// are treated as "proceed" and let the real install step surface the outcome. It never + /// escalates privileges. Regardless of this setting, the install step always annotates a + /// permission failure as `InstallPathNotWritable` naming the path. + pub fn check_install_path_writable(&mut self, check: bool) -> &mut Self { + self.common.check_install_path_writable = check; + self + } + /// Set the path of the exe inside the release tarball. This is the location of the /// executable relative to the base of the tar'd directory and is the path that will /// be copied to the `bin_install_path`. If not specified, this will default to the diff --git a/src/update.rs b/src/update.rs index 6960b4e..551e319 100644 --- a/src/update.rs +++ b/src/update.rs @@ -798,6 +798,12 @@ pub trait UpdateConfig: sealed::Sealed { /// Installation path for the binary being updated fn bin_install_path(&self) -> &std::path::Path; + /// Whether to run the opt-in preflight writability probe of `bin_install_path` before + /// downloading (set via `check_install_path_writable`). Defaults to `false`. + fn check_install_path_writable(&self) -> bool { + false + } + /// Path of the binary to be extracted from release package fn bin_path_in_archive(&self) -> &str; @@ -995,6 +1001,11 @@ pub trait ReleaseUpdate: UpdateConfig + UpdateInternals { let target_asset = resolve_and_confirm(self, &release)?; + // Opt-in preflight: bail before downloading if the install path is definitely not writable. + if self.check_install_path_writable() { + probe_install_path_writable(self.bin_install_path())?; + } + let tmp_archive_dir = tempfile::TempDir::new()?; let tmp_archive_path = tmp_archive_dir.path().join(target_asset.name()); let mut tmp_archive = fs::File::create(&tmp_archive_path)?; @@ -1493,6 +1504,12 @@ where let target_asset = resolve_and_confirm(u, &release)?; + // Opt-in preflight: bail before downloading if the install path is definitely not writable. + // Shares the sync probe for exact parity with `update_extended`. + if u.check_install_path_writable() { + probe_install_path_writable(u.bin_install_path())?; + } + let tmp_archive_dir = tempfile::TempDir::new()?; let tmp_archive_path = tmp_archive_dir.path().join(target_asset.name()); let mut tmp_archive = fs::File::create(&tmp_archive_path)?; @@ -1538,14 +1555,83 @@ fn install_binary( })?; } let current_exe = std::env::current_exe()?; + // Only the two install-step writes are wrapped with path context (not `current_exe()` or the + // verify hook above): a permission failure here becomes `InstallPathNotWritable` naming the + // path, and any other IO error is rewrapped so the path shows up while the `ErrorKind` stays + // inspectable. This annotation is always on, independent of the opt-in preflight probe. if same_file(bin_install_path, ¤t_exe) { - self_replace::self_replace(new_exe)?; + self_replace::self_replace(new_exe) + .map_err(|e| map_install_io_error(e, bin_install_path))?; } else { - Move::from_source(new_exe).to_dest(bin_install_path)?; + Move::from_source(new_exe) + .to_dest(bin_install_path) + .map_err(|e| match e { + Error::Io(io) => map_install_io_error(io, bin_install_path), + other => other, + })?; } Ok(()) } +/// Map an IO error from an install-step write into the crate error, always naming the install path. +/// +/// A `PermissionDenied` becomes [`Error::InstallPathNotWritable`] carrying the path; any other kind +/// is rewrapped as [`Error::Io`] whose message names the path (`"installing to {path}: {orig}"`) +/// while preserving the original [`std::io::ErrorKind`] so callers can still inspect it. Non-permission +/// failures (e.g. a full disk) are deliberately NOT reclassified as `InstallPathNotWritable`. +fn map_install_io_error(e: std::io::Error, bin_install_path: &std::path::Path) -> Error { + if e.kind() == std::io::ErrorKind::PermissionDenied { + Error::InstallPathNotWritable { + path: bin_install_path.to_path_buf(), + } + } else { + Error::Io(std::io::Error::new( + e.kind(), + format!("installing to {}: {}", bin_install_path.display(), e), + )) + } +} + +/// Best-effort preflight probe of whether `bin_install_path` can be written, run before any +/// download when `check_install_path_writable` is set. +/// +/// Filesystem-only and conservative. If the path already exists, probe the file by opening it for +/// append; otherwise probe its parent directory by creating and immediately dropping a temporary +/// sibling (a `.self_update_writecheck` prefix). ONLY a definite +/// [`PermissionDenied`](std::io::ErrorKind::PermissionDenied) maps to +/// [`Error::InstallPathNotWritable`]; success or ANY other error kind (a missing parent, an unusual +/// filesystem, etc.) returns `Ok(())` so the update proceeds and the real install step surfaces the +/// outcome. The create/delete probe is the honest test under Windows ACLs too, so no `cfg` split is +/// needed. +pub(crate) fn probe_install_path_writable(bin_install_path: &std::path::Path) -> Result<()> { + let probe: std::io::Result<()> = if bin_install_path.exists() { + std::fs::OpenOptions::new() + .append(true) + .open(bin_install_path) + .map(|_| ()) + } else { + // `parent()` is `Some("")` for a bare file name; treat that (and `None`) as the CWD. + let parent = match bin_install_path.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + _ => std::path::Path::new("."), + }; + tempfile::Builder::new() + .prefix(".self_update_writecheck") + .tempfile_in(parent) + .map(|_| ()) + }; + match probe { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + Err(Error::InstallPathNotWritable { + path: bin_install_path.to_path_buf(), + }) + } + // Indeterminate (missing parent, weird fs, etc.): proceed and let the real op surface it. + Err(_) => Ok(()), + } +} + /// Whether two paths refer to the same file. Compares canonicalized paths (resolving symlinks and /// `..`), falling back to a raw comparison when canonicalization fails (for example when a path does /// not yet exist). `current_exe()` is symlink-resolved on some platforms while a user-supplied @@ -1652,6 +1738,7 @@ mod tests { use super::{ReleaseAsset, Releases, UpdateStrategy, choose_latest_release, install_binary}; use crate::Download; use crate::DynVerifyFn; + use crate::errors::Error; use crate::errors::Result; use crate::update::Release; @@ -2732,6 +2819,114 @@ mod tests { assert_eq!(std::fs::read(&dest).unwrap(), b"new binary"); } + // Installing into a read-only (0555) directory fails with a permission error at the move step; + // `install_binary` must annotate it as `InstallPathNotWritable` naming the install path, not + // leak a bare `Io(Permission denied)`. Pins the always-on install-error path context. + #[cfg(unix)] + #[test] + fn install_binary_maps_permission_denied_to_install_path_not_writable() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let new_exe = dir.path().join("new"); + std::fs::write(&new_exe, b"new binary").unwrap(); + + let ro_dir = dir.path().join("ro"); + std::fs::create_dir(&ro_dir).unwrap(); + std::fs::set_permissions(&ro_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + let dest = ro_dir.join("installed"); + + let res = install_binary(&new_exe, &dest, None); + // Restore write perms so the tempdir can be cleaned up regardless of the assertion outcome. + std::fs::set_permissions(&ro_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let err = res.expect_err("installing into a 0555 dir must fail"); + match err { + Error::InstallPathNotWritable { path } => { + assert_eq!( + path, dest, + "InstallPathNotWritable must carry the exact install path" + ); + } + other => panic!("expected Error::InstallPathNotWritable, got {:?}", other), + } + } + + // A non-permission install failure (here: the destination's parent directory does not exist, + // so the move fails `NotFound`) must NOT be reclassified as `InstallPathNotWritable`; it stays + // an `Error::Io` whose message names the install path and whose `ErrorKind` is preserved. + #[test] + fn install_binary_wraps_non_permission_io_error_with_path_context() { + let dir = tempfile::tempdir().unwrap(); + let new_exe = dir.path().join("new"); + std::fs::write(&new_exe, b"new binary").unwrap(); + // Parent directory intentionally absent -> rename fails with NotFound, not PermissionDenied. + let dest = dir.path().join("no-such-dir").join("installed"); + + let err = install_binary(&new_exe, &dest, None) + .expect_err("moving into a missing parent dir must fail"); + match err { + Error::Io(io) => { + assert_ne!( + io.kind(), + std::io::ErrorKind::PermissionDenied, + "a NotFound failure must not be reported as permission denied" + ); + let msg = io.to_string(); + assert!( + msg.contains(&dest.display().to_string()), + "the rewrapped Io error must name the install path, got: {msg}" + ); + } + other => panic!( + "a non-permission failure must stay Error::Io, got {:?}", + other + ), + } + } + + // The preflight probe returns `Ok(())` for a writable directory (a not-yet-existing target + // whose parent is writable: the temp-sibling create/delete succeeds). + #[test] + fn probe_install_path_writable_ok_for_writable_dir() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("app"); + super::probe_install_path_writable(&target).expect("a writable parent dir must probe Ok"); + } + + // The preflight probe returns `InstallPathNotWritable` when the parent dir is read-only (0555): + // the temp-sibling create fails with PermissionDenied. + #[cfg(unix)] + #[test] + fn probe_install_path_writable_errors_for_readonly_dir() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let ro_dir = dir.path().join("ro"); + std::fs::create_dir(&ro_dir).unwrap(); + std::fs::set_permissions(&ro_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + let target = ro_dir.join("app"); + + let res = super::probe_install_path_writable(&target); + std::fs::set_permissions(&ro_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + match res { + Err(Error::InstallPathNotWritable { path }) => { + assert_eq!(path, target, "the probe must name the probed install path"); + } + other => panic!("expected InstallPathNotWritable, got {:?}", other), + } + } + + // The preflight probe is conservative: a missing parent directory is indeterminate (the temp + // create fails with NotFound, not PermissionDenied), so the probe returns `Ok(())` and lets the + // real install step surface any error. + #[test] + fn probe_install_path_writable_ok_for_missing_parent() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("no-such-dir").join("app"); + super::probe_install_path_writable(&target) + .expect("a missing parent dir is indeterminate and must probe Ok"); + } + // Build a custom-backend `Update` carrying `checksum`, to drive `finish_update` directly. #[cfg(feature = "checksums")] fn update_with_checksum(checksum: crate::Checksum) -> crate::backends::custom::Update { diff --git a/tests/permissions.rs b/tests/permissions.rs new file mode 100644 index 0000000..3fb3d37 --- /dev/null +++ b/tests/permissions.rs @@ -0,0 +1,375 @@ +//! End-to-end coverage for the permissions-UX preflight (#112), driven entirely through the public +//! API of the `custom` backend. +//! +//! The `custom` backend performs its release *listing* in-process via a [`ReleaseSource`] (no HTTP), +//! and routes only the crate-controlled *download* through the injected +//! [`HttpClient`](self_update::http_client::HttpClient). That split makes the central claim of the +//! feature cleanly observable: when the opt-in preflight +//! ([`check_install_path_writable(true)`](self_update::backends)) refuses because the install path +//! is definitely not writable, the recording download client must see **zero** requests -- nothing +//! is downloaded. When the preflight is off (the default) or indeterminate (a missing parent dir), +//! the flow must proceed *past* the probe and reach the download, so the recording client *does* get +//! a request. +//! +//! The unix-permission scenarios (a read-only `0555` install dir) are gated `#[cfg(unix)]`; the +//! indeterminate-parent scenario is cross-platform. No unix-only imports live at the top level, so +//! the file compiles on Windows. + +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use self_update::Error; +use self_update::backends::custom; +use self_update::http_client::{HeaderMap, HttpClient, HttpResponse}; +use self_update::update::{Release, ReleaseAsset, ReleaseSource}; + +/// A download transport that records every URL it is asked to GET and then fails the transfer. It is +/// never expected to produce a real body -- the tests assert on *whether* it was hit, and the +/// deliberate transport error stops the update right after the request so nothing touches the real +/// filesystem beyond the temp archive dir. +struct RecordingClient { + hits: Arc>>, +} + +impl HttpClient for RecordingClient { + fn get( + &self, + url: &str, + _headers: &HeaderMap, + _timeout: Option, + ) -> self_update::Result> { + self.hits.lock().unwrap().push(url.to_string()); + Err(Error::transport("recording client: no body served")) + } +} + +/// A canned in-process release source: one release, newer than the driving current version, carrying +/// a single asset. The listing therefore never hits the network, so the only requests a driving +/// updater can make are downloads through the injected client. +struct CannedSource { + releases: Vec, +} + +impl CannedSource { + /// One release `9.9.9` (strictly newer than the `0.0.1` the tests configure) with a single, + /// traversal-safe asset pointing at an unroutable loopback URL. The asset is never actually + /// fetched in the refusal tests; in the proceed tests the recording client intercepts it. + fn one_newer() -> Self { + let asset = ReleaseAsset::new("app-9.9.9.tar.gz", "http://127.0.0.1:9/app-9.9.9.tar.gz"); + let release = Release::builder() + .version("9.9.9") + .asset(asset) + .build() + .unwrap(); + Self { + releases: vec![release], + } + } +} + +impl ReleaseSource for CannedSource { + fn get_releases(&self) -> self_update::Result> { + Ok(self.releases.clone()) + } +} + +/// Whether `dir` genuinely rejects writes for this process. A `0555` directory is *not* write +/// protected when the tests run as root (the DAC check is bypassed), so the permission-refusal +/// assertions guard on this to avoid a spurious failure in a root CI container: if the dir is +/// writable anyway, there is nothing for the preflight to refuse and the test is skipped. +#[cfg(unix)] +fn is_write_protected(dir: &std::path::Path) -> bool { + match tempfile::Builder::new() + .prefix(".permcheck") + .tempfile_in(dir) + { + Ok(_) => false, + Err(e) => e.kind() == std::io::ErrorKind::PermissionDenied, + } +} + +/// Create a `0555` (read + execute, no write) child directory under `parent` and return its path. +#[cfg(unix)] +fn make_readonly_dir(parent: &std::path::Path) -> std::path::PathBuf { + use std::os::unix::fs::PermissionsExt; + let ro = parent.join("ro"); + std::fs::create_dir(&ro).unwrap(); + std::fs::set_permissions(&ro, std::fs::Permissions::from_mode(0o555)).unwrap(); + ro +} + +/// Restore write permission so the enclosing `TempDir` can be cleaned up. +#[cfg(unix)] +fn restore_writable(dir: &std::path::Path) { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o755)); +} + +// (a) Preflight ON + a definitely-unwritable (0555) install dir: `update()` must fail with +// `InstallPathNotWritable` naming the configured install path, and the injected download client must +// have recorded ZERO requests. This is the load-bearing guarantee: on a definite refusal, NOTHING is +// downloaded. +#[cfg(unix)] +#[test] +fn preflight_on_unwritable_dir_refuses_before_any_download() { + let tmp = tempfile::tempdir().unwrap(); + let ro_dir = make_readonly_dir(tmp.path()); + + if !is_write_protected(&ro_dir) { + // Running as root: a 0555 dir is still writable, so there is nothing to refuse. Skip. + restore_writable(&ro_dir); + eprintln!("skipping: install dir is writable (running as root?)"); + return; + } + + let install_path = ro_dir.join("app"); + let hits = Arc::new(Mutex::new(Vec::new())); + + let updater = custom::Update::configure() + .source(CannedSource::one_newer()) + .bin_name("app") + .current_version("0.0.1") + .bin_install_path(&install_path) + .check_install_path_writable(true) + .asset_matcher(|assets| assets.first().cloned()) + .no_confirm(true) + .show_output(false) + .http_client(Arc::new(RecordingClient { hits: hits.clone() })) + .build() + .unwrap(); + + let result = updater.update(); + + // Restore before asserting so a failed assertion still leaves the tempdir cleanable. + restore_writable(&ro_dir); + + match result { + Err(Error::InstallPathNotWritable { path, .. }) => { + assert_eq!( + path, install_path, + "the preflight error must name the configured bin_install_path" + ); + } + other => panic!("expected Err(InstallPathNotWritable), got {other:?}"), + } + + assert!( + hits.lock().unwrap().is_empty(), + "a refused preflight must download NOTHING, but the client saw requests: {:?}", + hits.lock().unwrap() + ); +} + +// (c) Preflight OFF (the default) + the same unwritable (0555) dir: the flow must NOT short-circuit +// on writability before the network. It proceeds past the (absent) probe and reaches the download, +// so the recording client gets a request and the surfaced error is the download's transport failure, +// never `InstallPathNotWritable` raised ahead of any request. +#[cfg(unix)] +#[test] +fn preflight_off_unwritable_dir_proceeds_to_download() { + let tmp = tempfile::tempdir().unwrap(); + let ro_dir = make_readonly_dir(tmp.path()); + let install_path = ro_dir.join("app"); + let hits = Arc::new(Mutex::new(Vec::new())); + + let updater = custom::Update::configure() + .source(CannedSource::one_newer()) + .bin_name("app") + .current_version("0.0.1") + .bin_install_path(&install_path) + // check_install_path_writable left at its default (false). + .asset_matcher(|assets| assets.first().cloned()) + .no_confirm(true) + .show_output(false) + .http_client(Arc::new(RecordingClient { hits: hits.clone() })) + .build() + .unwrap(); + + let result = updater.update(); + + restore_writable(&ro_dir); + + assert!( + !matches!(result, Err(Error::InstallPathNotWritable { .. })), + "with the preflight off, writability must not be raised before a download is attempted, \ + got {result:?}" + ); + assert!( + !hits.lock().unwrap().is_empty(), + "with the preflight off, the update must proceed to the download and hit the client" + ); +} + +// (d) Preflight ON but the case is indeterminate: the install path's parent directory does not +// exist. The probe cannot get a definite PermissionDenied (the temp-sibling create fails with +// NotFound), so it must return Ok and let the flow proceed to the download -- the recording client +// gets a request, and no `InstallPathNotWritable` is raised at the probe. Cross-platform (no unix +// permissions involved). +#[test] +fn preflight_on_indeterminate_missing_parent_proceeds_to_download() { + let tmp = tempfile::tempdir().unwrap(); + // `no-such-dir` is never created, so the parent of the install path is missing. + let install_path = tmp.path().join("no-such-dir").join("app"); + let hits = Arc::new(Mutex::new(Vec::new())); + + let updater = custom::Update::configure() + .source(CannedSource::one_newer()) + .bin_name("app") + .current_version("0.0.1") + .bin_install_path(&install_path) + .check_install_path_writable(true) + .asset_matcher(|assets| assets.first().cloned()) + .no_confirm(true) + .show_output(false) + .http_client(Arc::new(RecordingClient { hits: hits.clone() })) + .build() + .unwrap(); + + let result = updater.update(); + + assert!( + !matches!(result, Err(Error::InstallPathNotWritable { .. })), + "an indeterminate (missing parent) preflight must proceed, not refuse, got {result:?}" + ); + assert!( + !hits.lock().unwrap().is_empty(), + "an indeterminate preflight must proceed to the download and hit the client" + ); +} + +// --- async parity -------------------------------------------------------------------------------- + +#[cfg(feature = "async")] +mod async_parity { + use super::*; + use self_update::futures_util::future::BoxFuture; + use self_update::http_client::{AsyncHttpClient, AsyncHttpResponse}; + use self_update::update::AsyncReleaseSource; + + /// Async sibling of [`RecordingClient`]: records the download URL then fails the transfer. + struct RecordingAsyncClient { + hits: Arc>>, + } + + impl AsyncHttpClient for RecordingAsyncClient { + fn get<'a>( + &'a self, + url: &'a str, + _headers: &'a HeaderMap, + _timeout: Option, + ) -> BoxFuture<'a, self_update::Result>> { + let hits = self.hits.clone(); + let url = url.to_string(); + Box::pin(async move { + hits.lock().unwrap().push(url); + Err(Error::transport("recording async client: no body served")) + }) + } + } + + /// Async sibling of [`CannedSource`]. + struct CannedAsyncSource { + releases: Vec, + } + + impl CannedAsyncSource { + fn one_newer() -> Self { + Self { + releases: CannedSource::one_newer().releases, + } + } + } + + impl AsyncReleaseSource for CannedAsyncSource { + fn get_releases( + &self, + ) -> impl std::future::Future>> + Send + '_ + { + let releases = self.releases.clone(); + async move { Ok(releases) } + } + } + + // (b) Async parity for (a): a refused preflight through `build_async()`/`update_async` downloads + // NOTHING -- `InstallPathNotWritable` naming the path, zero requests to the async client. + #[cfg(unix)] + #[tokio::test] + async fn preflight_on_unwritable_dir_refuses_before_any_download_async() { + let tmp = tempfile::tempdir().unwrap(); + let ro_dir = make_readonly_dir(tmp.path()); + + if !is_write_protected(&ro_dir) { + restore_writable(&ro_dir); + eprintln!("skipping: install dir is writable (running as root?)"); + return; + } + + let install_path = ro_dir.join("app"); + let hits = Arc::new(Mutex::new(Vec::new())); + + let updater = custom::AsyncUpdate::::configure() + .source(CannedAsyncSource::one_newer()) + .bin_name("app") + .current_version("0.0.1") + .bin_install_path(&install_path) + .check_install_path_writable(true) + .asset_matcher(|assets| assets.first().cloned()) + .no_confirm(true) + .show_output(false) + .http_client_async(Arc::new(RecordingAsyncClient { hits: hits.clone() })) + .build_async() + .unwrap(); + + let result = updater.update_async().await; + + restore_writable(&ro_dir); + + match result { + Err(Error::InstallPathNotWritable { path, .. }) => { + assert_eq!( + path, install_path, + "the async preflight error must name the configured bin_install_path" + ); + } + other => panic!("expected Err(InstallPathNotWritable), got {other:?}"), + } + + assert!( + hits.lock().unwrap().is_empty(), + "a refused async preflight must download NOTHING, but the client saw: {:?}", + hits.lock().unwrap() + ); + } + + // Async parity for the indeterminate case: a missing parent dir proceeds to the download. + #[tokio::test] + async fn preflight_on_indeterminate_missing_parent_proceeds_to_download_async() { + let tmp = tempfile::tempdir().unwrap(); + let install_path = tmp.path().join("no-such-dir").join("app"); + let hits = Arc::new(Mutex::new(Vec::new())); + + let updater = custom::AsyncUpdate::::configure() + .source(CannedAsyncSource::one_newer()) + .bin_name("app") + .current_version("0.0.1") + .bin_install_path(&install_path) + .check_install_path_writable(true) + .asset_matcher(|assets| assets.first().cloned()) + .no_confirm(true) + .show_output(false) + .http_client_async(Arc::new(RecordingAsyncClient { hits: hits.clone() })) + .build_async() + .unwrap(); + + let result = updater.update_async().await; + + assert!( + !matches!(result, Err(Error::InstallPathNotWritable { .. })), + "an indeterminate async preflight must proceed, not refuse, got {result:?}" + ); + assert!( + !hits.lock().unwrap().is_empty(), + "an indeterminate async preflight must proceed to the download and hit the client" + ); + } +}