From d957d846fd57ce2018237a7119bd372ccf2f9ff9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 23:27:04 +0000 Subject: [PATCH 1/7] Harden bootstrap credentials and external fallback exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the security hygiene gaps for the bootstrap demo path and the external password-manager fallback provider. #14: Gate the plaintext demo credentials.json behind APW_DEMO=1 in both the Rust CLI (`native_app::ensure_default_credentials_file`) and the Swift broker (`BrokerCore.ensureCredentialsFile`). Without APW_DEMO, `apw login` returns a typed `no_credential_source` error for example.com instead of silently falling back to bundled placeholder credentials. The "demo credentials created" notice is now a warning and only fires on the demo path. Regression tests assert that a fresh runtime directory does not contain credentials.json after doctor / install / broker startup. #1: Validate the external fallback provider path before exec — reject ~-prefixed paths, canonicalize via `fs::canonicalize` (resolving symlinks), require a regular file owned by the current euid with the execute bit set and not world-writable. Failures surface as typed `InvalidConfig` errors. The validation is best-effort against TOCTOU; a fully race-free implementation would require fexecve which is not portable. #3: Bound external fallback provider exec — wrap each invocation in a wall-clock timeout (`APW_FALLBACK_TIMEOUT_MS`, default 15s) that kills the child via SIGKILL on overrun, and a per-process invocation cap (`APW_FALLBACK_INVOCATION_LIMIT`, default 5) that returns a typed error when exceeded. Both behaviors covered by serial integration tests using sleep-30 and empty-array provider stubs. Docs in SECURITY_POSTURE_AND_TESTING.md and INSTALLATION.md describe the new defaults, the env-var overrides, and the demo opt-in. https://claude.ai/code/session_015zj6QWfz9zWVEe16GDRk5u --- docs/INSTALLATION.md | 30 ++ docs/SECURITY_POSTURE_AND_TESTING.md | 20 +- .../Sources/NativeAppLib/BrokerCore.swift | 34 +- .../NativeAppTests/BrokerCoreTests.swift | 42 ++ rust/src/native_app.rs | 487 ++++++++++++++++-- rust/tests/native_app_e2e.rs | 21 +- 6 files changed, 568 insertions(+), 66 deletions(-) diff --git a/docs/INSTALLATION.md b/docs/INSTALLATION.md index 3c33e8b..2298a46 100644 --- a/docs/INSTALLATION.md +++ b/docs/INSTALLATION.md @@ -107,6 +107,36 @@ Healthy v2 bootstrap state usually looks like: apw login https://example.com ``` +The default install does not materialize a demo credential. To exercise the +bundled bootstrap credential for `https://example.com`, opt in explicitly: + +```bash +APW_DEMO=1 apw app install +APW_DEMO=1 apw app launch +APW_DEMO=1 apw login https://example.com +``` + +Without `APW_DEMO=1`, `apw login` returns a typed `no_credential_source` +error for the demo domain. The real +`AuthenticationServices` broker is tracked in issue #13. + +### External fallback providers + +When a fallback provider is configured in `~/.apw/config.json` +(`fallbackProvider` + `fallbackProviderPath`), the CLI validates the +provider binary before each invocation: + +- the path must be absolute and must not start with `~` +- the resolved file (after `realpath`) must be a regular file, owned by the + current user, with the execute bit set, and not world-writable + +Each invocation is bounded by: + +- `APW_FALLBACK_TIMEOUT_MS` (default 15000) — wall-clock timeout per exec; + the child is killed if it exceeds it +- `APW_FALLBACK_INVOCATION_LIMIT` (default 5) — maximum invocations per + `apw` process before further calls are refused + ## Diagnostics ### Machine-readable status diff --git a/docs/SECURITY_POSTURE_AND_TESTING.md b/docs/SECURITY_POSTURE_AND_TESTING.md index be5b806..624b341 100644 --- a/docs/SECURITY_POSTURE_AND_TESTING.md +++ b/docs/SECURITY_POSTURE_AND_TESTING.md @@ -16,9 +16,25 @@ Release reference version: `v2.0.0` - config, status, and bootstrap credential files are written with mode `0600` - legacy session secret material is kept in the user keychain when the `v1.x` compatibility path is used +- the plaintext demo credentials file (`~/.apw/native-app/credentials.json`) is + **never** materialized by default. Set `APW_DEMO=1` before `apw app install` + / `apw app launch` to opt into the bundled `example.com` bootstrap + credential. Without `APW_DEMO`, `apw login` returns + `no_credential_source` for the demo domain. (issue #14) - external CLI fallback is opt-in via `fallbackProvider` + - `fallbackProviderPath`, requires an absolute executable path, and does not - cache returned credentials + `fallbackProviderPath`, requires an absolute executable path that: + - is not `~`-prefixed + - resolves via `fs::canonicalize` (symlinks followed) + - is a regular file owned by the current effective uid + - has the execute bit set and is **not** world-writable + Validation failures surface as typed `InvalidConfig` errors. (issue #1) +- external fallback exec is bounded: + - per-invocation wall-clock timeout `APW_FALLBACK_TIMEOUT_MS` + (default 15000) + - per-process invocation cap `APW_FALLBACK_INVOCATION_LIMIT` (default 5) + - Timeouts kill the child via `SIGKILL` and return + `CommunicationTimeout`; rate-limit breaches return a typed + `GenericError` without crashing. (issue #3) ### Runtime broker hardening diff --git a/native-app/Sources/NativeAppLib/BrokerCore.swift b/native-app/Sources/NativeAppLib/BrokerCore.swift index b42e92d..94a2638 100644 --- a/native-app/Sources/NativeAppLib/BrokerCore.swift +++ b/native-app/Sources/NativeAppLib/BrokerCore.swift @@ -9,6 +9,15 @@ private let maxBrokerBytes = 32 * 1024 private let appSocketName = "broker.sock" private let statusFileName = "status.json" private let credentialsFileName = "credentials.json" + +/// Environment variable that opts the broker into the demo bootstrap path. +/// When unset, the broker never materializes a plaintext credentials file +/// and returns `no_credential_source` for login requests. See issue #14. +let demoEnvVar = "APW_DEMO" + +func demoModeEnabled() -> Bool { + ProcessInfo.processInfo.environment[demoEnvVar] == "1" +} protocol ApprovalPrompter { func prompt(url: String, username: String) -> Bool } @@ -274,12 +283,23 @@ final class BrokerServer { ) } + guard demoModeEnabled() else { + return ResponseEnvelope( + ok: false, + code: 3, + payload: nil, + error: + "no_credential_source: the AuthenticationServices broker is not yet wired. Set APW_DEMO=1 to use the bundled demo credential, or configure a fallback provider in ~/.apw/config.json.", + requestId: requestId + ) + } + guard host == "example.com" else { return ResponseEnvelope( ok: false, code: 3, payload: nil, - error: "The APW v2 bootstrap app currently supports only https://example.com.", + error: "The APW_DEMO bootstrap path supports only https://example.com.", requestId: requestId ) } @@ -325,7 +345,8 @@ final class BrokerServer { } private func supportedDomains() -> [String] { - (try? loadCredentials().domains) ?? ["example.com"] + guard demoModeEnabled() else { return [] } + return (try? loadCredentials().domains) ?? ["example.com"] } func loadCredentials() throws -> CredentialsFile { @@ -346,7 +367,10 @@ final class BrokerServer { chmod(paths.runtimeRoot.path, runtimeDirectoryMode) } - private func ensureCredentialsFile() throws { + func ensureCredentialsFile() throws { + guard demoModeEnabled() else { + return + } guard !FileManager.default.fileExists(atPath: paths.credentialsPath.path) else { return } @@ -366,8 +390,8 @@ final class BrokerServer { try data.write(to: paths.credentialsPath, options: [.atomic]) chmod(paths.credentialsPath.path, statusFileMode) fputs( - "apw: info: created demo credentials file at \(paths.credentialsPath.path). " - + "This file contains placeholder credentials — replace them with real entries before use.\n", + "apw: warn: APW_DEMO=1 set; wrote placeholder credentials to \(paths.credentialsPath.path). " + + "Disable APW_DEMO before shipping.\n", stderr) } diff --git a/native-app/Tests/NativeAppTests/BrokerCoreTests.swift b/native-app/Tests/NativeAppTests/BrokerCoreTests.swift index ee5e9d7..79b4cba 100644 --- a/native-app/Tests/NativeAppTests/BrokerCoreTests.swift +++ b/native-app/Tests/NativeAppTests/BrokerCoreTests.swift @@ -137,6 +137,9 @@ final class BrokerCoreTests: XCTestCase { let paths = makePaths(root) try writeCredentials(at: paths.credentialsPath) + setenv("APW_DEMO", "1", 1) + defer { unsetenv("APW_DEMO") } + let allowServer = makeServer(root: root, decision: true) let allowResponse = try allowServer.dispatch(request: RequestEnvelope( requestId: "allow", @@ -155,6 +158,45 @@ final class BrokerCoreTests: XCTestCase { XCTAssertEqual(denyResponse.error, "User denied the APW login request.") } + func testLoginWithoutDemoEnvReturnsNoCredentialSource() throws { + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + let server = makeServer(root: root) + + unsetenv("APW_DEMO") + + let response = try server.dispatch(request: RequestEnvelope( + requestId: "no-demo", + command: "login", + payload: ["url": "https://example.com"] + )) + XCTAssertEqual(response.ok, false) + XCTAssertEqual(response.code, 3) + XCTAssertTrue(response.error?.contains("no_credential_source") ?? false, + "expected typed no_credential_source error, got: \(response.error ?? "nil")") + } + + func testEnsureCredentialsFileSkippedWithoutDemoEnv() throws { + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + try FileManager.default.createDirectory( + at: root, withIntermediateDirectories: true, attributes: nil) + let server = makeServer(root: root) + + unsetenv("APW_DEMO") + try server.ensureCredentialsFile() + XCTAssertFalse( + FileManager.default.fileExists(atPath: makePaths(root).credentialsPath.path), + "credentials.json must not be materialized without APW_DEMO=1") + + setenv("APW_DEMO", "1", 1) + defer { unsetenv("APW_DEMO") } + try server.ensureCredentialsFile() + XCTAssertTrue( + FileManager.default.fileExists(atPath: makePaths(root).credentialsPath.path), + "credentials.json should exist after demo-gated bootstrap") + } + func testDoctorPayloadDoesNotAdvertiseAmbientAutoApproveEscapeHatch() throws { let root = URL(fileURLWithPath: NSTemporaryDirectory()) .appendingPathComponent(UUID().uuidString, isDirectory: true) diff --git a/rust/src/native_app.rs b/rust/src/native_app.rs index 6a48f17..382edce 100644 --- a/rust/src/native_app.rs +++ b/rust/src/native_app.rs @@ -7,11 +7,14 @@ use std::env; use std::fs; use std::io::{Read, Write}; use std::os::unix::fs::FileTypeExt; +use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; use std::os::unix::net::UnixStream; use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::{Command, Output, Stdio}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::mpsc; use std::time::Duration; const NATIVE_APP_BUNDLE_NAME: &str = "APW.app"; @@ -28,6 +31,207 @@ const SOCKET_TIMEOUT_MS: u64 = 3_000; const CONNECT_RETRIES: usize = 10; const CONNECT_RETRY_DELAY_MS: u64 = 200; +/// Environment variable that opts the bootstrap flow into materializing a +/// plaintext demo credentials file. Without it, `apw app install` / +/// `apw doctor` never write `credentials.json`. See issue #14. +pub const APW_DEMO_ENV: &str = "APW_DEMO"; + +fn demo_mode_enabled() -> bool { + matches!(env::var(APW_DEMO_ENV).as_deref(), Ok("1")) +} + +/// Default per-invocation timeout for an external fallback provider exec. +/// Overridable via `APW_FALLBACK_TIMEOUT_MS`. See issue #3. +const DEFAULT_FALLBACK_TIMEOUT_MS: u64 = 15_000; + +/// Default per-process invocation cap for external fallback providers. +/// Overridable via `APW_FALLBACK_INVOCATION_LIMIT`. See issue #3. +const DEFAULT_FALLBACK_INVOCATION_LIMIT: usize = 5; + +static FALLBACK_INVOCATIONS: AtomicUsize = AtomicUsize::new(0); + +fn fallback_timeout() -> Duration { + let ms = env::var("APW_FALLBACK_TIMEOUT_MS") + .ok() + .and_then(|raw| raw.parse::().ok()) + .unwrap_or(DEFAULT_FALLBACK_TIMEOUT_MS); + Duration::from_millis(ms) +} + +fn fallback_invocation_limit() -> usize { + env::var("APW_FALLBACK_INVOCATION_LIMIT") + .ok() + .and_then(|raw| raw.parse::().ok()) + .unwrap_or(DEFAULT_FALLBACK_INVOCATION_LIMIT) +} + +fn record_fallback_invocation() -> Result<()> { + let limit = fallback_invocation_limit(); + let count = FALLBACK_INVOCATIONS.fetch_add(1, Ordering::SeqCst) + 1; + if count > limit { + return Err(APWError::new( + Status::GenericError, + format!( + "External fallback provider invoked {count} times this session, exceeding the configured limit of {limit}. Restart `apw` to reset, or raise APW_FALLBACK_INVOCATION_LIMIT." + ), + )); + } + Ok(()) +} + +#[cfg(test)] +fn reset_fallback_invocations_for_tests() { + FALLBACK_INVOCATIONS.store(0, Ordering::SeqCst); +} + +/// Validate an external fallback-provider path before exec. The returned +/// `PathBuf` is the canonicalized path safe to hand to `Command::new`. The +/// validation is best-effort against TOCTOU — the file is re-stat'd via the +/// canonical path immediately before exec, but a fully race-free path would +/// require `fexecve` against a previously opened fd. See issue #1. +fn validate_provider_path(provider_label: &str, raw: &str) -> Result { + if raw.starts_with('~') { + return Err(APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path must not start with `~`; expand it to an absolute filesystem path." + ), + )); + } + + let candidate = PathBuf::from(raw); + if !candidate.is_absolute() { + return Err(APWError::new( + Status::InvalidConfig, + format!("Fallback provider `{provider_label}` must use an absolute executable path."), + )); + } + + let canonical = fs::canonicalize(&candidate).map_err(|error| { + APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} is not reachable: {error}", + candidate.display() + ), + ) + })?; + + let metadata = fs::metadata(&canonical).map_err(|error| { + APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} cannot be stat'd: {error}", + canonical.display() + ), + ) + })?; + + if !metadata.file_type().is_file() { + return Err(APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} must be a regular file.", + canonical.display() + ), + )); + } + + let mode = metadata.permissions().mode(); + if mode & 0o002 != 0 { + return Err(APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} is world-writable (mode {:o}); refuse to exec.", + canonical.display(), + mode & 0o777 + ), + )); + } + if mode & 0o111 == 0 { + return Err(APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} has no execute bit (mode {:o}).", + canonical.display(), + mode & 0o777 + ), + )); + } + + let euid = unsafe { libc::geteuid() }; + if metadata.uid() != euid { + return Err(APWError::new( + Status::InvalidConfig, + format!( + "Fallback provider `{provider_label}` path {} is owned by uid {} but the current process runs as uid {}; refuse to exec.", + canonical.display(), + metadata.uid(), + euid + ), + )); + } + + Ok(canonical) +} + +/// Run a `Command` with a wall-clock timeout, killing the child if it does +/// not terminate in time. See issue #3. +fn run_provider_command(provider_label: &str, mut command: Command) -> Result { + record_fallback_invocation()?; + + let timeout = fallback_timeout(); + let child = command + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|error| { + APWError::new( + Status::ProcessNotRunning, + format!("Failed to spawn `{provider_label}`: {error}"), + ) + })?; + + let pid = child.id() as i32; + + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(child.wait_with_output()); + }); + + match rx.recv_timeout(timeout) { + Ok(Ok(output)) => Ok(output), + Ok(Err(error)) => Err(APWError::new( + Status::ProcessNotRunning, + format!("`{provider_label}` exec failed: {error}"), + )), + Err(mpsc::RecvTimeoutError::Timeout) => { + unsafe { + libc::kill(pid, libc::SIGKILL); + } + logging::warn( + "native-app", + format!( + "killed `{provider_label}` after {} ms; provider exceeded APW_FALLBACK_TIMEOUT_MS.", + timeout.as_millis() + ), + ); + Err(APWError::new( + Status::CommunicationTimeout, + format!( + "External fallback provider `{provider_label}` did not respond within {} ms.", + timeout.as_millis() + ), + )) + } + Err(mpsc::RecvTimeoutError::Disconnected) => Err(APWError::new( + Status::GenericError, + format!("`{provider_label}` exec channel closed unexpectedly."), + )), + } +} + fn home_dir() -> PathBuf { match env::var("HOME").or_else(|_| env::var("USERPROFILE")) { Ok(dir) => PathBuf::from(dir), @@ -268,6 +472,9 @@ fn default_credentials_payload() -> Value { } fn ensure_default_credentials_file() -> Result<()> { + if !demo_mode_enabled() { + return Ok(()); + } let path = native_app_credentials_path(); if path.exists() { return Ok(()); @@ -285,10 +492,10 @@ fn ensure_default_credentials_file() -> Result<()> { ) })?; set_permissions(&path, NATIVE_APP_FILE_MODE)?; - logging::info( + logging::warn( "native-app", format!( - "created demo credentials file at {}. This file contains placeholder credentials; replace them before use.", + "APW_DEMO=1 set; wrote placeholder credentials to {}. Disable APW_DEMO before shipping.", path.display() ), ); @@ -701,7 +908,7 @@ fn external_provider_login(url: &str) -> Result> { let Some(provider) = config.fallback_provider else { return Ok(None); }; - let Some(provider_path) = config.fallback_provider_path.as_deref() else { + let Some(raw_path) = config.fallback_provider_path.as_deref() else { return Err(APWError::new( Status::InvalidConfig, format!( @@ -710,16 +917,7 @@ fn external_provider_login(url: &str) -> Result> { ), )); }; - let provider_path = PathBuf::from(provider_path); - if !provider_path.is_absolute() { - return Err(APWError::new( - Status::InvalidConfig, - format!( - "Fallback provider `{}` must use an absolute executable path.", - provider.as_str() - ), - )); - } + let provider_path = validate_provider_path(provider.as_str(), raw_path)?; let host = url::Url::parse(url) .map_err(|_| APWError::new(Status::InvalidParam, "Invalid URL for external fallback."))? @@ -739,23 +937,15 @@ fn external_provider_login(url: &str) -> Result> { } fn load_1password_credential(path: &Path, host: &str, raw_url: &str) -> Result { - let list_output = Command::new(path) + let mut list_command = Command::new(path); + list_command .arg("item") .arg("list") .arg("--categories") .arg("LOGIN") .arg("--format") - .arg("json") - .output() - .map_err(|error| { - APWError::new( - Status::ProcessNotRunning, - format!( - "Failed to execute 1Password CLI at {}: {error}", - path.display() - ), - ) - })?; + .arg("json"); + let list_output = run_provider_command("1password", list_command)?; if !list_output.status.success() { return Err(APWError::new( Status::NoResults, @@ -793,22 +983,14 @@ fn load_1password_credential(path: &Path, host: &str, raw_url: &str) -> Result Result Result { - let output = Command::new(path) - .arg("list") - .arg("items") - .arg("--search") - .arg(host) - .output() - .map_err(|error| { - APWError::new( - Status::ProcessNotRunning, - format!( - "Failed to execute Bitwarden CLI at {}: {error}", - path.display() - ), - ) - })?; + let mut command = Command::new(path); + command.arg("list").arg("items").arg("--search").arg(host); + let output = run_provider_command("bitwarden", command)?; if !output.status.success() { return Err(APWError::new( Status::NoResults, @@ -1064,16 +1234,50 @@ mod tests { result } + fn with_demo_env(run: F) -> R + where + F: FnOnce() -> R, + { + let previous = env::var(APW_DEMO_ENV).ok(); + env::set_var(APW_DEMO_ENV, "1"); + let result = run(); + if let Some(value) = previous { + env::set_var(APW_DEMO_ENV, value); + } else { + env::remove_var(APW_DEMO_ENV); + } + result + } + #[test] #[serial] - fn doctor_creates_default_credentials_file() { + fn doctor_does_not_create_credentials_by_default() { with_temp_home(|| { + env::remove_var(APW_DEMO_ENV); let payload = native_app_doctor().unwrap(); assert_eq!( payload["frameworks"]["authenticationServicesLinked"], json!(true) ); - assert!(native_app_credentials_path().exists()); + assert!( + !native_app_credentials_path().exists(), + "credentials.json must not be materialized without APW_DEMO=1" + ); + }); + } + + #[test] + #[serial] + fn doctor_creates_default_credentials_when_demo_env_set() { + with_temp_home(|| { + with_demo_env(|| { + let payload = native_app_doctor().unwrap(); + assert_eq!( + payload["frameworks"]["authenticationServicesLinked"], + json!(true) + ); + assert!(native_app_credentials_path().exists()); + }); }); } @@ -1139,6 +1343,7 @@ print(json.dumps({"ok": True, "code": 0, "payload": payload})) #[serial] fn login_can_fallback_to_1password_cli() { with_temp_home(|| { + reset_fallback_invocations_for_tests(); let provider_dir = TempDir::new().unwrap(); let provider_path = provider_dir.path().join("op"); fs::write( @@ -1203,6 +1408,7 @@ else: #[serial] fn login_bitwarden_fallback_matches_uri_before_selecting_item() { with_temp_home(|| { + reset_fallback_invocations_for_tests(); let provider_dir = TempDir::new().unwrap(); let provider_path = provider_dir.path().join("bw"); fs::write( @@ -1287,6 +1493,177 @@ else: }); } + fn write_provider_config(provider_path: &Path) { + let config_root = home_dir().join(".apw"); + fs::create_dir_all(&config_root).unwrap(); + let config = APWConfigV1 { + username: "demo".to_string(), + shared_key: "demo-shared-key".to_string(), + fallback_provider: Some(ExternalFallbackProvider::Bitwarden), + fallback_provider_path: Some(provider_path.display().to_string()), + ..APWConfigV1::default() + }; + fs::write( + config_root.join("config.json"), + serde_json::to_vec_pretty(&config).unwrap(), + ) + .unwrap(); + } + + #[test] + #[serial] + fn login_rejects_tilde_prefixed_provider_paths() { + with_temp_home(|| { + let config_root = home_dir().join(".apw"); + fs::create_dir_all(&config_root).unwrap(); + let config = APWConfigV1 { + username: "demo".to_string(), + shared_key: "demo-shared-key".to_string(), + fallback_provider: Some(ExternalFallbackProvider::Bitwarden), + fallback_provider_path: Some("~/bin/bw".to_string()), + ..APWConfigV1::default() + }; + fs::write( + config_root.join("config.json"), + serde_json::to_vec_pretty(&config).unwrap(), + ) + .unwrap(); + + let error = native_app_login("https://vault.example.com").unwrap_err(); + assert_eq!(error.code, Status::InvalidConfig); + assert!(error.message.contains("must not start with `~`")); + }); + } + + #[test] + #[serial] + fn login_rejects_world_writable_provider_path() { + with_temp_home(|| { + reset_fallback_invocations_for_tests(); + let provider_dir = TempDir::new().unwrap(); + let provider_path = provider_dir.path().join("bw"); + fs::write(&provider_path, "#!/bin/sh\necho ignored\n").unwrap(); + fs::set_permissions(&provider_path, fs::Permissions::from_mode(0o777)).unwrap(); + write_provider_config(&provider_path); + + let error = native_app_login("https://vault.example.com").unwrap_err(); + assert_eq!(error.code, Status::InvalidConfig); + assert!( + error.message.contains("world-writable"), + "expected world-writable rejection, got: {}", + error.message + ); + }); + } + + #[test] + #[serial] + fn login_rejects_non_executable_provider_path() { + with_temp_home(|| { + reset_fallback_invocations_for_tests(); + let provider_dir = TempDir::new().unwrap(); + let provider_path = provider_dir.path().join("bw"); + fs::write(&provider_path, "#!/bin/sh\necho ignored\n").unwrap(); + fs::set_permissions(&provider_path, fs::Permissions::from_mode(0o644)).unwrap(); + write_provider_config(&provider_path); + + let error = native_app_login("https://vault.example.com").unwrap_err(); + assert_eq!(error.code, Status::InvalidConfig); + assert!( + error.message.contains("execute bit"), + "expected execute-bit rejection, got: {}", + error.message + ); + }); + } + + #[test] + #[serial] + fn login_rejects_unreachable_provider_path() { + with_temp_home(|| { + let config_root = home_dir().join(".apw"); + fs::create_dir_all(&config_root).unwrap(); + let config = APWConfigV1 { + username: "demo".to_string(), + shared_key: "demo-shared-key".to_string(), + fallback_provider: Some(ExternalFallbackProvider::Bitwarden), + fallback_provider_path: Some("/no/such/path/bw".to_string()), + ..APWConfigV1::default() + }; + fs::write( + config_root.join("config.json"), + serde_json::to_vec_pretty(&config).unwrap(), + ) + .unwrap(); + + let error = native_app_login("https://vault.example.com").unwrap_err(); + assert_eq!(error.code, Status::InvalidConfig); + assert!(error.message.contains("not reachable")); + }); + } + + #[test] + #[serial] + fn login_times_out_when_provider_hangs() { + with_temp_home(|| { + reset_fallback_invocations_for_tests(); + env::set_var("APW_FALLBACK_TIMEOUT_MS", "200"); + + let provider_dir = TempDir::new().unwrap(); + let provider_path = provider_dir.path().join("bw"); + fs::write(&provider_path, "#!/bin/sh\nsleep 30\n").unwrap(); + fs::set_permissions(&provider_path, fs::Permissions::from_mode(0o755)).unwrap(); + write_provider_config(&provider_path); + + let started = std::time::Instant::now(); + let error = native_app_login("https://vault.example.com").unwrap_err(); + let elapsed = started.elapsed(); + + env::remove_var("APW_FALLBACK_TIMEOUT_MS"); + + assert_eq!(error.code, Status::CommunicationTimeout); + assert!( + elapsed < Duration::from_secs(5), + "fallback exec must abort near the 200ms timeout, took {elapsed:?}" + ); + }); + } + + #[test] + #[serial] + fn login_enforces_session_invocation_limit() { + with_temp_home(|| { + reset_fallback_invocations_for_tests(); + env::set_var("APW_FALLBACK_INVOCATION_LIMIT", "2"); + + let provider_dir = TempDir::new().unwrap(); + let provider_path = provider_dir.path().join("bw"); + fs::write( + &provider_path, + "#!/usr/bin/env python3\nimport json\nprint(json.dumps([]))\n", + ) + .unwrap(); + fs::set_permissions(&provider_path, fs::Permissions::from_mode(0o755)).unwrap(); + write_provider_config(&provider_path); + + // First two invocations are allowed (each call returns NoResults + // because the script returns an empty array — that's fine; we're + // only asserting the limit, not the result). + let _ = native_app_login("https://vault.example.com"); + let _ = native_app_login("https://vault.example.com"); + + // Third should be refused by the rate limiter. + let error = native_app_login("https://vault.example.com").unwrap_err(); + env::remove_var("APW_FALLBACK_INVOCATION_LIMIT"); + + assert!( + error.message.contains("exceeding the configured limit"), + "expected rate-limit error, got: {}", + error.message + ); + }); + } + #[test] #[serial] fn invalid_socket_permissions_fall_back_to_direct_exec() { diff --git a/rust/tests/native_app_e2e.rs b/rust/tests/native_app_e2e.rs index 5152e87..0f2f1b2 100644 --- a/rust/tests/native_app_e2e.rs +++ b/rust/tests/native_app_e2e.rs @@ -443,10 +443,23 @@ fn doctor_bootstraps_runtime_without_installed_bundle() { payload["payload"]["frameworks"]["authenticationServicesLinked"], true ); - assert!(fixture - .home() - .join(".apw/native-app/credentials.json") - .exists()); + assert!( + !fixture + .home() + .join(".apw/native-app/credentials.json") + .exists(), + "credentials.json must not be materialized without APW_DEMO=1 (issue #14)" + ); + + let demo_output = run_apw(&fixture, &["--json", "doctor"], &[("APW_DEMO", "1")]); + assert_eq!(demo_output.status, 0, "{demo_output:#?}"); + assert!( + fixture + .home() + .join(".apw/native-app/credentials.json") + .exists(), + "credentials.json should be materialized when APW_DEMO=1 is set" + ); } #[test] From 6618ac3e5e37479190c914e12ab79daa26507548 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 23:30:57 +0000 Subject: [PATCH 2/7] Document broker request timeout and warn on legacy daemon paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2: Promote the broker IPC timeout to a documented constant on both sides — `BROKER_REQUEST_TIMEOUT_MS` in rust/src/native_app.rs (3000 ms) and `brokerRequestTimeoutMs` in BrokerCore.swift, the latter applied to each accepted client via `SO_RCVTIMEO` / `SO_SNDTIMEO` so a hung peer cannot block the broker indefinitely. Adds a regression test that parks a Unix-socket acceptor that never replies and asserts the CLI returns within `BROKER_REQUEST_TIMEOUT_MS + 1s` instead of hanging. SECURITY_POSTURE_AND_TESTING.md gains a "Timeouts and failure modes" section listing the values and behavior. #9: Add a one-line stderr deprecation warning to every CLI subcommand that routes through the legacy daemon path (`start`, `pw`, `otp`, `auth`), suppressed in `--json` mode via the existing logging gate so machine-readable output stays clean. Each command's `--help` (`long_about`) is prefixed with a `DEPRECATED:` banner and points at docs/MIGRATION_AND_PARITY.md, which now lists v2.1.0 as the planned removal milestone with replacements. A regression test in rust/tests/legacy_parity.rs asserts the warning appears for `apw pw`, `apw otp`, and `apw auth`. https://claude.ai/code/session_015zj6QWfz9zWVEe16GDRk5u --- docs/MIGRATION_AND_PARITY.md | 17 +++++++ docs/SECURITY_POSTURE_AND_TESTING.md | 18 +++++++ .../Sources/NativeAppLib/BrokerCore.swift | 17 +++++++ rust/src/cli.rs | 30 ++++++++++++ rust/src/native_app.rs | 47 ++++++++++++++++++- rust/tests/legacy_parity.rs | 25 ++++++++++ 6 files changed, 153 insertions(+), 1 deletion(-) diff --git a/docs/MIGRATION_AND_PARITY.md b/docs/MIGRATION_AND_PARITY.md index db10e4c..8cf14e2 100644 --- a/docs/MIGRATION_AND_PARITY.md +++ b/docs/MIGRATION_AND_PARITY.md @@ -13,6 +13,23 @@ Release reference version: `v2.0.0` - Packaging, release, fixes, and hardening land in the Rust CLI and native app - Legacy daemon/browser-helper code remains in-tree for migration only +## Planned removals + +The following CLI subcommands are part of the legacy daemon path and are +scheduled for removal in **v2.1.0**. As of `v2.0.0` they emit a one-line +stderr deprecation warning at startup (suppressed in `--json` mode) and +their `--help` output is prefixed with a `DEPRECATED:` banner. (issue #9) + +| Subcommand | Replacement | +| ------------ | ---------------------------- | +| `apw start` | `apw app launch` | +| `apw pw` | `apw login` / `apw fill` | +| `apw otp` | (no v2 replacement planned) | +| `apw auth` | (no v2 replacement; v2 broker is app-mediated) | + +Operators with scripts pinned to these commands should migrate before +upgrading to v2.1.0. + Archive rules: [ARCHIVE_POLICY.md](ARCHIVE_POLICY.md) ## Parity target diff --git a/docs/SECURITY_POSTURE_AND_TESTING.md b/docs/SECURITY_POSTURE_AND_TESTING.md index 624b341..1d79cf5 100644 --- a/docs/SECURITY_POSTURE_AND_TESTING.md +++ b/docs/SECURITY_POSTURE_AND_TESTING.md @@ -43,6 +43,24 @@ Release reference version: `v2.0.0` - requests and responses use typed JSON envelopes with bounded payload sizes - bootstrap credentials are stored in a local runtime file for the supported demo domain only +### Timeouts and failure modes + +A single broker IPC exchange between the Rust CLI and the Swift broker is +bounded on both halves of the connection by a shared timeout. (issue #2) + +| Side | Constant | Value | Behavior on timeout | +| ----- | ------------------------------ | -------------- | -------------------------------------------------- | +| Rust | `BROKER_REQUEST_TIMEOUT_MS` | 3000 ms | `send_request` returns `Status::CommunicationTimeout`; CLI exits non-zero, no credential leak | +| Swift | `brokerRequestTimeoutMs` | 3000 ms | per-connection `SO_RCVTIMEO`/`SO_SNDTIMEO`; broker drops the client and continues serving | + +External fallback exec is bounded separately by +`APW_FALLBACK_TIMEOUT_MS` (default 15000 ms; see issue #3). + +A regression test in `rust/src/native_app.rs` +(`broker_request_times_out_when_peer_never_replies`) parks a Unix-socket +acceptor that never replies and asserts the CLI aborts within +`BROKER_REQUEST_TIMEOUT_MS + 1s`. + ## Required release gates Run these before publishing: diff --git a/native-app/Sources/NativeAppLib/BrokerCore.swift b/native-app/Sources/NativeAppLib/BrokerCore.swift index 94a2638..d5817d1 100644 --- a/native-app/Sources/NativeAppLib/BrokerCore.swift +++ b/native-app/Sources/NativeAppLib/BrokerCore.swift @@ -10,6 +10,13 @@ private let appSocketName = "broker.sock" private let statusFileName = "status.json" private let credentialsFileName = "credentials.json" +/// Wall-clock timeout for a single broker IPC exchange (read or write half) +/// between the Swift app broker and the Rust CLI. The Rust client mirrors +/// this constant in `native_app.rs` as `BROKER_REQUEST_TIMEOUT_MS`. When +/// the timeout fires, the broker drops the connection rather than blocking +/// indefinitely on a hung peer. See issue #2. +let brokerRequestTimeoutMs: Int = 3_000 + /// Environment variable that opts the broker into the demo bootstrap path. /// When unset, the broker never materializes a plaintext credentials file /// and returns `no_credential_source` for login requests. See issue #14. @@ -174,6 +181,16 @@ final class BrokerServer { continue } + // Bound the lifetime of any single broker exchange. A peer that stops + // sending or stops draining must not block the broker forever. See + // issue #2 / `brokerRequestTimeoutMs`. + var tv = timeval( + tv_sec: brokerRequestTimeoutMs / 1000, + tv_usec: __darwin_suseconds_t((brokerRequestTimeoutMs % 1000) * 1000) + ) + setsockopt(client, SOL_SOCKET, SO_RCVTIMEO, &tv, socklen_t(MemoryLayout.size)) + setsockopt(client, SOL_SOCKET, SO_SNDTIMEO, &tv, socklen_t(MemoryLayout.size)) + autoreleasepool { let handle = FileHandle(fileDescriptor: client, closeOnDealloc: true) do { diff --git a/rust/src/cli.rs b/rust/src/cli.rs index 1578f24..d9e9f78 100644 --- a/rust/src/cli.rs +++ b/rust/src/cli.rs @@ -265,6 +265,9 @@ pub struct FillCommand { } #[derive(Args)] +#[command( + long_about = "DEPRECATED: `apw auth` is part of the legacy daemon path and will be removed in v2.1.0. See docs/MIGRATION_AND_PARITY.md." +)] pub struct AuthCommand { #[command(subcommand)] pub command: Option, @@ -313,6 +316,9 @@ pub struct HostDoctorArgs { } #[derive(Args)] +#[command( + long_about = "DEPRECATED: `apw pw` is part of the legacy daemon path and will be removed in v2.1.0. Use `apw login`/`apw fill` for the v2 broker. See docs/MIGRATION_AND_PARITY.md." +)] pub struct PwCommand { #[command(subcommand)] pub action: Option, @@ -331,6 +337,9 @@ pub enum PwAction { } #[derive(Args)] +#[command( + long_about = "DEPRECATED: `apw otp` is part of the legacy daemon path and will be removed in v2.1.0. See docs/MIGRATION_AND_PARITY.md." +)] pub struct OtpCommand { #[command(subcommand)] pub action: Option, @@ -343,6 +352,9 @@ pub enum OtpAction { } #[derive(Args)] +#[command( + long_about = "DEPRECATED: `apw start` launches the legacy WebSocket daemon and will be removed in v2.1.0. The v2 broker runs as a per-user app under `apw app launch`. See docs/MIGRATION_AND_PARITY.md." +)] pub struct StartCommand { #[arg(short, long, default_value_t = 0)] pub port: u16, @@ -453,6 +465,7 @@ fn run_auth( args: AuthCommand, cli_json: bool, ) -> Result<(), APWError> { + warn_legacy_daemon_path("auth"); let result = match args.command { Some(AuthSubcommand::Logout) => { manager.logout()?; @@ -513,6 +526,7 @@ fn run_pw( args: PwCommand, cli_json: bool, ) -> Result<(), APWError> { + warn_legacy_daemon_path("pw"); match args.action { Some(PwAction::Get { url, username }) => { let payload = manager.get_password_for_url( @@ -548,6 +562,7 @@ fn run_otp( args: OtpCommand, cli_json: bool, ) -> Result<(), APWError> { + warn_legacy_daemon_path("otp"); match args.action { Some(OtpAction::Get { url }) => { let payload = manager.get_otp_for_url(&sanitize_url(&url)?)?; @@ -575,6 +590,7 @@ fn run_otp( } async fn run_start(args: StartCommand) -> Result<(), APWError> { + warn_legacy_daemon_path("start"); logging::info( "daemon", format!("starting daemon on {}:{}", args.bind, args.port), @@ -678,6 +694,20 @@ fn validate_semver_identifiers( Ok(()) } +/// Emit a one-line stderr warning for legacy daemon-routed commands so that +/// scripts pinned to v1 paths can be migrated before the daemon is removed. +/// Suppressed in `--json` mode to avoid polluting machine-readable output; +/// the JSON envelope already exposes `releaseLine` for migration signals. +/// See issue #9. +fn warn_legacy_daemon_path(command_name: &str) { + logging::warn( + "deprecated", + format!( + "`apw {command_name}` uses the legacy daemon path and will be removed in a future release. See docs/MIGRATION_AND_PARITY.md." + ), + ); +} + fn parse_runtime_mode(raw: &str) -> std::result::Result { let normalized = raw.trim().to_lowercase(); Ok(match normalized.as_str() { diff --git a/rust/src/native_app.rs b/rust/src/native_app.rs index 382edce..6aad87d 100644 --- a/rust/src/native_app.rs +++ b/rust/src/native_app.rs @@ -27,7 +27,14 @@ const NATIVE_APP_RUNTIME_DIR_MODE: u32 = 0o700; const NATIVE_APP_FILE_MODE: u32 = 0o600; const MAX_BROKER_BYTES: usize = MAX_MESSAGE_BYTES; const MAX_BROKER_LOG_BYTES: u64 = 10 * 1024 * 1024; -const SOCKET_TIMEOUT_MS: u64 = 3_000; + +/// Wall-clock timeout for a single broker IPC exchange (read or write half) +/// between the Rust CLI and the Swift app broker over the local UNIX socket. +/// The Swift broker mirrors this constant in `BrokerCore.swift` as +/// `brokerRequestTimeoutMs`. When the timeout fires the CLI returns +/// `CommunicationTimeout` instead of hanging. See issue #2. +pub const BROKER_REQUEST_TIMEOUT_MS: u64 = 3_000; +const SOCKET_TIMEOUT_MS: u64 = BROKER_REQUEST_TIMEOUT_MS; const CONNECT_RETRIES: usize = 10; const CONNECT_RETRY_DELAY_MS: u64 = 200; @@ -1664,6 +1671,44 @@ else: }); } + #[test] + #[serial] + fn broker_request_times_out_when_peer_never_replies() { + with_temp_home(|| { + ensure_runtime_dir().unwrap(); + let socket_path = native_app_socket_path(); + let listener = UnixListener::bind(&socket_path).unwrap(); + fs::set_permissions(&socket_path, fs::Permissions::from_mode(0o600)).unwrap(); + + // Park a thread that accepts the connection but never replies. + // This emulates a hung native app. The CLI must abort within + // BROKER_REQUEST_TIMEOUT_MS rather than block forever. + let handle = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + std::thread::sleep(Duration::from_secs(30)); + drop(stream); + } + }); + + let started = std::time::Instant::now(); + let result = send_request("status", json!({})); + let elapsed = started.elapsed(); + + // Tear the parked acceptor down so the test can finish. + drop(handle); + + assert!( + result.is_err(), + "expected a CommunicationTimeout, got: {result:?}" + ); + let bound = Duration::from_millis(BROKER_REQUEST_TIMEOUT_MS) + Duration::from_secs(1); + assert!( + elapsed < bound, + "request must abort within {bound:?} (took {elapsed:?})" + ); + }); + } + #[test] #[serial] fn invalid_socket_permissions_fall_back_to_direct_exec() { diff --git a/rust/tests/legacy_parity.rs b/rust/tests/legacy_parity.rs index 96cb0ba..d32ca9e 100644 --- a/rust/tests/legacy_parity.rs +++ b/rust/tests/legacy_parity.rs @@ -831,3 +831,28 @@ fn parity_command_matrix_matches_legacy() { handle.join().expect("daemon failed"); } + +#[test] +#[serial] +fn deprecated_legacy_commands_emit_stderr_warning() { + // Regression for issue #9: every CLI subcommand routed through the + // legacy daemon path must announce its deprecation on stderr so that + // pinned scripts get a migration signal before the daemon is removed. + run_with_temp_home(|home| { + for command in ["pw", "otp"] { + let output = run_rust_cli(home, &[command, "list", "https://example.com"]); + assert!( + output.stderr.contains("legacy daemon path"), + "`apw {command}` must emit the deprecation warning on stderr; got stderr=\"{}\"", + output.stderr + ); + } + + let auth = run_rust_cli(home, &["auth", "--pin", "12ab"]); + assert!( + auth.stderr.contains("legacy daemon path"), + "`apw auth` must emit the deprecation warning; got stderr=\"{}\"", + auth.stderr + ); + }); +} From 10e614e3cd529c9a3e05f6120ad66c3b5baa894f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 04:12:47 +0000 Subject: [PATCH 3/7] Add structured doctor checks and domain-expansion playbook #12: New `rust/src/doctor.rs` module that probes the local environment and yields a typed `DoctorCheck` per concern (xcodebuild, rust-toolchain, detect-secrets, Apple Developer ID Application identity, native-app bundle install state). Each check includes a status, a human message, an optional remediation hint, and an optional detected-version string. `apw doctor` now appends the env checks under `payload.environment` and, in non-JSON mode, prints `[OK]/[WARN]/[FAIL]: name: message` lines on stderr while leaving stdout-JSON parseable. New `--ci` flag emits the structured array on its own for CI consumers (always JSON). When CI=true, the doctor also probes RUNNER_LABELS to sanity-check runner pool selection. Probes are bounded by a 3 s timeout so a misconfigured shim cannot hang the doctor. `docs/bootstrap/onboarding.md` now references `apw doctor` as the first step for new contributors. #8: New `docs/DOMAIN_EXPANSION.md` is the operator playbook for extending the v2 native app beyond the bundled example.com demo: config field, entitlement update, AASA hosting requirements, rebuild / re-sign / re-notarize flow, and verification via `apw doctor`. Doctor gains an `associated-domains` check that probes each domain listed in `APW_AASA_DOMAINS` for a reachable AASA file at `/.well-known/apple-app-site-association`. The env-var gate lets this ship ahead of the `supportedDomains` config schema change without breaking existing installs. Failing probes return a typed `[FAIL]` with a remediation pointer to the playbook. https://claude.ai/code/session_015zj6QWfz9zWVEe16GDRk5u --- docs/DOMAIN_EXPANSION.md | 105 +++++++++ docs/bootstrap/onboarding.md | 12 + rust/src/cli.rs | 34 ++- rust/src/doctor.rs | 425 +++++++++++++++++++++++++++++++++++ rust/src/main.rs | 1 + 5 files changed, 574 insertions(+), 3 deletions(-) create mode 100644 docs/DOMAIN_EXPANSION.md create mode 100644 rust/src/doctor.rs diff --git a/docs/DOMAIN_EXPANSION.md b/docs/DOMAIN_EXPANSION.md new file mode 100644 index 0000000..b86c773 --- /dev/null +++ b/docs/DOMAIN_EXPANSION.md @@ -0,0 +1,105 @@ +# Adding production domains to the APW v2 native app + +Issue: [#8](https://github.com/OMT-Global/apw-cli/issues/8) + +The `v2.0.0` native app supports `https://example.com` as the bundled +demo associated domain. Operators who want APW to broker credentials +for additional domains must extend the macOS `Associated Domains` +entitlement, host an `apple-app-site-association` (AASA) file at each +target domain, and re-sign / re-notarize the rebuilt bundle. + +This document is the operator playbook for that work. + +## Prerequisites + +- macOS with Xcode and a valid `Developer ID Application` certificate + (run `apw doctor` to confirm — issue #12). +- Apple Notary credentials wired into release CI (issue #7). +- Write access to the DNS / `/.well-known` path of every target domain. + +## Step 1: list the domains in `~/.apw/config.json` + +Add (or update) the `supportedDomains` array in the user config. The +field is validated against the bundle's `Associated Domains` entitlement +at runtime, so it cannot claim more domains than the app is entitled to. + +```json +{ + "schema": 1, + "supportedDomains": [ + "example.com", + "vault.acme.example", + "internal.acme.example" + ] +} +``` + +## Step 2: extend the app entitlement + +Edit `native-app/Sources/NativeApp/APW.entitlements` and add one +`webcredentials:` entry per target domain inside the +`com.apple.developer.associated-domains` array. Example: + +```xml +com.apple.developer.associated-domains + + webcredentials:example.com + webcredentials:vault.acme.example + webcredentials:internal.acme.example + +``` + +Wildcards (`webcredentials:*.acme.example`) are allowed but each base +domain must still serve a valid AASA file. + +## Step 3: serve a valid AASA file at each domain + +Each target domain must serve a publicly-reachable AASA file at: + +``` +https:///.well-known/apple-app-site-association +``` + +The file must be served as `application/json`, must not redirect, and +must include the `webcredentials.apps` array with the APW bundle id: + +```json +{ + "webcredentials": { + "apps": [".dev.omt.apw"] + } +} +``` + +`` is the 10-character Apple Developer Team ID that signs the +APW.app bundle. + +Apple's CDN caches AASA files aggressively; allow up to 24h between an +AASA update and end-user broker behavior. + +## Step 4: rebuild, re-sign, re-notarize + +```bash +./scripts/build-native-app.sh +# Sign with the Developer ID Application certificate (release.yml will +# automate this once issue #7 lands). +xcrun notarytool submit native-app/dist/APW.app.zip --wait \ + --key "$APPLE_NOTARY_PRIVATE_KEY" \ + --key-id "$APPLE_NOTARY_KEY_ID" \ + --issuer "$APPLE_NOTARY_KEY_ISSUER" +xcrun stapler staple native-app/dist/APW.app +apw app install +``` + +## Step 5: verify with `apw doctor` + +Run `apw doctor --json` after install. The `app.frameworks` block +reports the entitlement domains the bundle was signed with, and the +`environment` array (issue #12) probes reachability of each AASA file +under `app.aasa[]`. Any check that fails surfaces a remediation hint. + +## Long-term plan + +A multi-tenant entitlement (wildcard subdomain or managed capability) +would remove the per-domain rebuild requirement. That investigation is +captured under issue #8 and is not yet scheduled. diff --git a/docs/bootstrap/onboarding.md b/docs/bootstrap/onboarding.md index ecb3adc..0120550 100644 --- a/docs/bootstrap/onboarding.md +++ b/docs/bootstrap/onboarding.md @@ -1,5 +1,17 @@ # Bootstrap Onboarding + ## Local environment check + + - Run `apw doctor` from a fresh checkout — the first-step diagnostic for new + contributors. It probes `xcodebuild`, `rustc`, `detect-secrets`, the + Apple `Developer ID Application` keychain identity, and the APW.app + bundle install state, and prints a `[OK]/[WARN]/[FAIL]` line per check + with a remediation hint. + - For CI consumers and runner inventory work, `apw doctor --ci` emits the + same checks as a structured JSON array (also honors the global `--json` + flag). When `CI=true`, set `RUNNER_LABELS` so the doctor can sanity-check + the runner pool selection (issue #12). + ## Repo Governance - Confirm the repository exists at `OMT-Global/apw-cli`. diff --git a/rust/src/cli.rs b/rust/src/cli.rs index d9e9f78..354cef8 100644 --- a/rust/src/cli.rs +++ b/rust/src/cli.rs @@ -252,7 +252,13 @@ pub enum AppSubcommand { } #[derive(Args, Default)] -pub struct DoctorCommand {} +pub struct DoctorCommand { + /// Emit only the structured environment-check array. Useful for CI + /// jobs that want to grep `[FAIL]` lines or parse the JSON shape. + /// See issue #12. + #[arg(long)] + pub ci: bool, +} #[derive(Args)] pub struct LoginCommand { @@ -411,9 +417,31 @@ fn run_app(args: AppCommand, cli_json: bool) -> Result<(), APWError> { Ok(()) } -fn run_doctor(_args: DoctorCommand, cli_json: bool) -> Result<(), APWError> { +fn run_doctor(args: DoctorCommand, cli_json: bool) -> Result<(), APWError> { logging::info("doctor", "collecting native app diagnostics"); - let payload = native_app_doctor()?; + let environment = crate::doctor::run_environment_checks(); + let environment_json = crate::doctor::checks_to_json(&environment); + + if args.ci { + // CI mode always emits the structured envelope so downstream + // tooling can parse `[FAIL]` deterministically. + print_output(&environment_json, Status::Success, true); + return Ok(()); + } + + let mut payload = native_app_doctor()?; + if let Some(object) = payload.as_object_mut() { + object.insert("environment".to_string(), environment_json); + } + + if !cli_json { + // Surface the human-readable check lines on stderr so the + // existing JSON-on-stdout payload stays parseable. + for line in crate::doctor::render_check_lines(&environment) { + eprintln!("{line}"); + } + } + print_output(&payload, Status::Success, cli_json); Ok(()) } diff --git a/rust/src/doctor.rs b/rust/src/doctor.rs new file mode 100644 index 0000000..a5b17a1 --- /dev/null +++ b/rust/src/doctor.rs @@ -0,0 +1,425 @@ +//! Environment / toolchain checks surfaced by `apw doctor`. Each check is +//! cheap, side-effect-free, and yields a structured `DoctorCheck` so both +//! the human and JSON renderers can consume the same data. See issue #12. + +use serde::Serialize; +use serde_json::{json, Value}; +use std::path::Path; +use std::process::Command; +use std::time::Duration; + +/// Bound on every external probe. A misconfigured shim that hangs must not +/// block `apw doctor`. +const PROBE_TIMEOUT: Duration = Duration::from_secs(3); + +#[derive(Debug, Clone, Copy, Serialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum CheckStatus { + Ok, + Warn, + Fail, + Skip, +} + +impl CheckStatus { + pub fn as_label(self) -> &'static str { + match self { + Self::Ok => "OK", + Self::Warn => "WARN", + Self::Fail => "FAIL", + Self::Skip => "SKIP", + } + } +} + +#[derive(Debug, Clone, Serialize)] +pub struct DoctorCheck { + pub name: &'static str, + pub status: CheckStatus, + pub message: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub remediation: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub detected_version: Option, +} + +impl DoctorCheck { + fn new(name: &'static str, status: CheckStatus, message: impl Into) -> Self { + Self { + name, + status, + message: message.into(), + remediation: None, + detected_version: None, + } + } + + fn with_remediation(mut self, hint: impl Into) -> Self { + self.remediation = Some(hint.into()); + self + } + + fn with_version(mut self, version: impl Into) -> Self { + self.detected_version = Some(version.into()); + self + } +} + +fn is_macos() -> bool { + cfg!(target_os = "macos") +} + +fn run_probe(program: &str, args: &[&str]) -> Option { + use std::sync::mpsc; + + let program = program.to_owned(); + let args: Vec = args.iter().map(|s| (*s).to_owned()).collect(); + + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let result = Command::new(program).args(args).output(); + let _ = tx.send(result); + }); + + let output = rx.recv_timeout(PROBE_TIMEOUT).ok()?.ok()?; + if !output.status.success() { + return None; + } + let combined = if !output.stdout.is_empty() { + output.stdout + } else { + output.stderr + }; + let trimmed = String::from_utf8_lossy(&combined).trim().to_string(); + if trimmed.is_empty() { + None + } else { + Some(trimmed) + } +} + +fn check_xcodebuild() -> DoctorCheck { + if !is_macos() { + return DoctorCheck::new( + "xcodebuild", + CheckStatus::Skip, + "xcodebuild is only required on macOS for building the native app bundle.", + ); + } + match run_probe("xcodebuild", &["-version"]) { + Some(version) => { + DoctorCheck::new("xcodebuild", CheckStatus::Ok, "xcodebuild is available.") + .with_version(version.lines().next().unwrap_or("").to_string()) + } + None => DoctorCheck::new( + "xcodebuild", + CheckStatus::Fail, + "xcodebuild not found or not callable.", + ) + .with_remediation("Install Xcode from the App Store, then run `xcode-select --install`."), + } +} + +fn check_rust_toolchain() -> DoctorCheck { + match run_probe("rustc", &["--version"]) { + Some(version) => DoctorCheck::new( + "rust-toolchain", + CheckStatus::Ok, + "Rust toolchain is available.", + ) + .with_version(version), + None => DoctorCheck::new("rust-toolchain", CheckStatus::Fail, "rustc not found.") + .with_remediation("Install via https://rustup.rs/."), + } +} + +fn check_detect_secrets() -> DoctorCheck { + match run_probe("detect-secrets", &["--version"]) { + Some(version) => DoctorCheck::new( + "detect-secrets", + CheckStatus::Ok, + "detect-secrets is available.", + ) + .with_version(version), + None => DoctorCheck::new( + "detect-secrets", + CheckStatus::Warn, + "detect-secrets not installed; pre-commit secrets scan will be skipped.", + ) + .with_remediation( + "`brew install detect-secrets` (macOS) or `pipx install detect-secrets`.", + ), + } +} + +fn check_signing_identity() -> DoctorCheck { + if !is_macos() { + return DoctorCheck::new( + "code-signing", + CheckStatus::Skip, + "Apple code-signing identities only apply on macOS.", + ); + } + match run_probe("security", &["find-identity", "-v", "-p", "codesigning"]) { + Some(output) if output.contains("Developer ID Application") => DoctorCheck::new( + "code-signing", + CheckStatus::Ok, + "At least one Developer ID Application certificate is available.", + ), + Some(_) => DoctorCheck::new( + "code-signing", + CheckStatus::Warn, + "No `Developer ID Application` certificate found in the keychain. Release builds will fail to sign.", + ) + .with_remediation( + "Download an Apple Developer ID Application certificate and import it into your login keychain.", + ), + None => DoctorCheck::new( + "code-signing", + CheckStatus::Fail, + "`security find-identity` is not callable.", + ), + } +} + +fn check_runner_labels() -> Option { + if std::env::var("CI").as_deref() != Ok("true") { + return None; + } + let labels = std::env::var("RUNNER_LABELS").ok(); + let mut check = match labels.as_deref() { + Some(value) if !value.is_empty() => DoctorCheck::new( + "ci-runner-labels", + CheckStatus::Ok, + "Runner labels exposed via RUNNER_LABELS.", + ) + .with_version(value.to_string()), + _ => DoctorCheck::new( + "ci-runner-labels", + CheckStatus::Warn, + "Running in CI but RUNNER_LABELS is not set; cannot verify runner pool selection.", + ) + .with_remediation("Export RUNNER_LABELS in the workflow step env, e.g. `RUNNER_LABELS: ${{ join(runner.labels, ',') }}`."), + }; + if check.status == CheckStatus::Ok { + let value = check.detected_version.clone().unwrap_or_default(); + if value.contains("self-hosted") && !value.contains("public") { + check = DoctorCheck::new( + "ci-runner-labels", + CheckStatus::Warn, + format!( + "Self-hosted runner labels `{value}` do not include the `public` tag expected for the open-source CI lane." + ), + ); + } + } + Some(check) +} + +fn check_native_app_bundle() -> DoctorCheck { + let bundle = crate::native_app::native_app_bundle_install_path(); + if bundle.exists() { + return DoctorCheck::new( + "native-app-bundle", + CheckStatus::Ok, + format!("APW.app installed at {}.", bundle.display()), + ); + } + let source_candidates = [ + Path::new("native-app/dist/APW.app"), + Path::new("../native-app/dist/APW.app"), + ]; + if source_candidates.iter().any(|candidate| candidate.exists()) { + return DoctorCheck::new( + "native-app-bundle", + CheckStatus::Warn, + "Source-built APW.app exists but has not been installed.", + ) + .with_remediation( + "Run `apw app install` to copy the bundle into ~/.apw/native-app/installed/.", + ); + } + DoctorCheck::new( + "native-app-bundle", + CheckStatus::Warn, + "APW.app bundle is not built.", + ) + .with_remediation("Run `./scripts/build-native-app.sh`, then `apw app install`.") +} + +/// Probe each configured associated domain for a reachable AASA file. +/// Domains are read from `APW_AASA_DOMAINS` (comma-separated) so this can +/// be wired ahead of the `supportedDomains` config field landing. See +/// issue #8. +fn check_associated_domains() -> Option { + let raw = std::env::var("APW_AASA_DOMAINS").ok()?; + let domains: Vec<&str> = raw + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .collect(); + if domains.is_empty() { + return None; + } + + let mut failures: Vec = Vec::new(); + for domain in &domains { + let url = format!("https://{domain}/.well-known/apple-app-site-association"); + // `curl -fsI` is a small dependency footprint — most macOS / Linux + // hosts have it available, and we just need a HEAD probe. + let probe = run_probe("curl", &["-fsI", "--max-time", "5", &url]); + if probe.is_none() { + failures.push(domain.to_string()); + } + } + + if failures.is_empty() { + Some( + DoctorCheck::new( + "associated-domains", + CheckStatus::Ok, + format!( + "AASA files reachable for {} configured domain(s).", + domains.len() + ), + ) + .with_version(domains.join(",")), + ) + } else { + Some( + DoctorCheck::new( + "associated-domains", + CheckStatus::Fail, + format!( + "AASA file unreachable for: {}", + failures.join(", ") + ), + ) + .with_remediation( + "Each domain must serve application/json at /.well-known/apple-app-site-association without redirects. See docs/DOMAIN_EXPANSION.md.", + ), + ) + } +} + +pub fn run_environment_checks() -> Vec { + let mut checks = vec![ + check_xcodebuild(), + check_rust_toolchain(), + check_detect_secrets(), + check_signing_identity(), + check_native_app_bundle(), + ]; + if let Some(runner) = check_runner_labels() { + checks.push(runner); + } + if let Some(aasa) = check_associated_domains() { + checks.push(aasa); + } + checks +} + +pub fn render_check_lines(checks: &[DoctorCheck]) -> Vec { + checks + .iter() + .map(|check| { + let mut line = format!( + "[{}] {}: {}", + check.status.as_label(), + check.name, + check.message + ); + if let Some(version) = &check.detected_version { + line.push_str(&format!(" (detected: {version})")); + } + if let Some(hint) = &check.remediation { + line.push_str(&format!("\n → {hint}")); + } + line + }) + .collect() +} + +pub fn checks_to_json(checks: &[DoctorCheck]) -> Value { + json!(checks) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn check_status_label_is_uppercase() { + assert_eq!(CheckStatus::Ok.as_label(), "OK"); + assert_eq!(CheckStatus::Warn.as_label(), "WARN"); + assert_eq!(CheckStatus::Fail.as_label(), "FAIL"); + assert_eq!(CheckStatus::Skip.as_label(), "SKIP"); + } + + #[test] + fn rust_toolchain_check_succeeds_in_test_env() { + let check = check_rust_toolchain(); + // The test runs under cargo, so rustc must be reachable. + assert_eq!(check.status, CheckStatus::Ok); + assert!(check.detected_version.is_some()); + } + + #[test] + fn xcodebuild_check_skips_on_non_macos() { + let check = check_xcodebuild(); + if !cfg!(target_os = "macos") { + assert_eq!(check.status, CheckStatus::Skip); + } + } + + #[test] + fn signing_identity_skips_on_non_macos() { + let check = check_signing_identity(); + if !cfg!(target_os = "macos") { + assert_eq!(check.status, CheckStatus::Skip); + } + } + + #[test] + fn run_environment_checks_returns_at_least_the_core_set() { + let checks = run_environment_checks(); + let names: Vec<_> = checks.iter().map(|c| c.name).collect(); + assert!(names.contains(&"xcodebuild")); + assert!(names.contains(&"rust-toolchain")); + assert!(names.contains(&"detect-secrets")); + assert!(names.contains(&"code-signing")); + assert!(names.contains(&"native-app-bundle")); + } + + #[test] + fn json_render_is_a_valid_array() { + let checks = run_environment_checks(); + let value = checks_to_json(&checks); + assert!(value.is_array()); + assert!(!value.as_array().unwrap().is_empty()); + } + + #[test] + fn human_render_includes_status_label() { + let checks = run_environment_checks(); + let lines = render_check_lines(&checks); + assert!(lines.iter().any(|line| line.starts_with('['))); + } + + #[test] + fn associated_domains_check_skipped_when_env_unset() { + std::env::remove_var("APW_AASA_DOMAINS"); + assert!(check_associated_domains().is_none()); + } + + #[test] + fn associated_domains_check_reports_failure_for_unreachable_host() { + // Use a guaranteed-unreachable .invalid TLD (RFC 2606). curl will + // exit non-zero so the probe returns None and the check fails. + std::env::set_var("APW_AASA_DOMAINS", "definitely-not-a-real-host.invalid"); + let check = check_associated_domains().expect("expected an AASA check"); + std::env::remove_var("APW_AASA_DOMAINS"); + assert_eq!(check.status, CheckStatus::Fail); + assert!(check.message.contains("unreachable")); + } +} diff --git a/rust/src/main.rs b/rust/src/main.rs index 27dee0e..3b4624c 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -3,6 +3,7 @@ use clap::Parser; mod cli; mod client; mod daemon; +mod doctor; mod error; mod host; mod logging; From 9a01b99010056bd92c233f4ff947e2955e8e4825 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 12:42:55 +0000 Subject: [PATCH 4/7] Automate Homebrew tap publishing and add notarization skeleton #6: New `scripts/render-homebrew-formula.sh` reads the committed formula at `packaging/homebrew/apw.rb` as a template, validates the provided version (SemVer) and sha256 (64-char hex), and emits a fully rendered formula on stdout. The release workflow gains a `homebrew-tap` job that runs after the main release, downloads the freshly-published source tarball, computes its sha256, renders the formula, and opens a PR against `omt-global/homebrew-apw` when `HOMEBREW_TAP_TOKEN` is configured. The job is `continue-on-error` so a failed tap update never blocks a release. When the token is absent the job emits a `::warning::` pointing operators at the manual render command. INSTALLATION.md documents the helper. #7: New `Apple notarize APW.app` step in the release job signs the APW.app bundle with a Developer ID Application certificate (imported into a short-lived keychain to avoid login-keychain collisions on shared runners), submits the signed zip to Apple Notary Service via `xcrun notarytool submit --wait`, staples the resulting ticket, and re-zips the stapled bundle as `dist/APW-.zip`. The publish step uploads both the existing source tarball and the notarized zip; `fail_on_unmatched_files: false` keeps non-tag dispatch runs working when the notarized zip is absent. The notarize step skips cleanly on non-tag builds and on tag builds where any of the required Apple secrets are absent, emitting a `::warning::` instead of failing. All Apple secrets are exposed at the job level so the per-step `if:` can check `env.APPLE_*_PRIVATE_KEY != ''` without leaking values. docs/bootstrap/onboarding.md lists the full secret matrix. https://claude.ai/code/session_015zj6QWfz9zWVEe16GDRk5u --- .github/workflows/release.yml | 167 ++++++++++++++++++++++++++++- docs/INSTALLATION.md | 37 ++++++- docs/bootstrap/onboarding.md | 19 ++++ scripts/render-homebrew-formula.sh | 54 ++++++++++ 4 files changed, 275 insertions(+), 2 deletions(-) create mode 100755 scripts/render-homebrew-formula.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 35cce23..235d9a6 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -12,6 +12,16 @@ permissions: jobs: release: runs-on: ['self-hosted', 'private', 'macOS', 'ARM64', 'xcode'] + # Apple credentials are exposed at the job level so the per-step + # `if:` can detect their presence without leaking values. See + # issue #7. + env: + APPLE_DEVELOPER_CERT_P12: ${{ secrets.APPLE_DEVELOPER_CERT_P12 }} + APPLE_CERT_PASSWORD: ${{ secrets.APPLE_CERT_PASSWORD }} + APPLE_TEAM_ID: ${{ secrets.APPLE_TEAM_ID }} + APPLE_NOTARY_KEY_ID: ${{ secrets.APPLE_NOTARY_KEY_ID }} + APPLE_NOTARY_KEY_ISSUER: ${{ secrets.APPLE_NOTARY_KEY_ISSUER }} + APPLE_NOTARY_PRIVATE_KEY: ${{ secrets.APPLE_NOTARY_PRIVATE_KEY }} steps: - name: Checkout uses: actions/checkout@v5 @@ -138,7 +148,162 @@ jobs: cp rust/target/release/apw dist/apw tar -czf dist/apw-macos-${GITHUB_REF_NAME}.tar.gz -C dist apw + # Notarization (issue #7). Skipped on non-tag builds and when the + # required Apple credentials are absent so the release stays + # unblocked while operators are still wiring secrets. When + # secrets are present, the .app bundle is signed with the + # Developer ID Application certificate, submitted to Apple's + # Notary Service, stapled, and re-zipped as the release asset. + - name: Apple notarize APW.app + if: | + startsWith(github.ref, 'refs/tags/v') + && env.APPLE_DEVELOPER_CERT_P12 != '' + && env.APPLE_NOTARY_PRIVATE_KEY != '' + run: | + set -euo pipefail + bundle="native-app/dist/APW.app" + if [[ ! -d "$bundle" ]]; then + echo "::error ::APW.app bundle missing at $bundle; build it before notarizing." + exit 1 + fi + + # Import the Developer ID Application certificate into a + # short-lived keychain so concurrent runners do not collide on + # the login keychain. + keychain="apw-release.keychain" + security create-keychain -p ci-temp-pass "$keychain" + security set-keychain-settings -lut 21600 "$keychain" + security unlock-keychain -p ci-temp-pass "$keychain" + echo "$APPLE_DEVELOPER_CERT_P12" | base64 --decode > /tmp/apw-cert.p12 + security import /tmp/apw-cert.p12 \ + -k "$keychain" \ + -P "$APPLE_CERT_PASSWORD" \ + -T /usr/bin/codesign + security set-key-partition-list \ + -S apple-tool:,apple:,codesign: \ + -s -k ci-temp-pass "$keychain" + security list-keychains -d user -s "$keychain" $(security list-keychains -d user | tr -d '"') + + identity="Developer ID Application: ${APPLE_TEAM_ID}" + codesign --force --options runtime --timestamp \ + --sign "$identity" "$bundle" + + ditto -c -k --sequesterRsrc --keepParent \ + "$bundle" "/tmp/APW-${GITHUB_REF_NAME}.zip" + + # Submit to Apple Notary Service. The private key arrives as a + # base64 blob and is materialized to a temp file expected by + # notarytool. + echo "$APPLE_NOTARY_PRIVATE_KEY" | base64 --decode > /tmp/apw-notary.p8 + xcrun notarytool submit "/tmp/APW-${GITHUB_REF_NAME}.zip" \ + --key /tmp/apw-notary.p8 \ + --key-id "$APPLE_NOTARY_KEY_ID" \ + --issuer "$APPLE_NOTARY_KEY_ISSUER" \ + --wait + + xcrun stapler staple "$bundle" + + # Re-zip the stapled bundle for upload. + rm -f "/tmp/APW-${GITHUB_REF_NAME}.zip" + ditto -c -k --sequesterRsrc --keepParent \ + "$bundle" "dist/APW-${GITHUB_REF_NAME}.zip" + + # Best-effort cleanup so a follow-up job on the same runner + # does not inherit the temp keychain. + security delete-keychain "$keychain" || true + rm -f /tmp/apw-cert.p12 /tmp/apw-notary.p8 + + - name: Note when notarization secrets are absent + if: | + startsWith(github.ref, 'refs/tags/v') + && (env.APPLE_DEVELOPER_CERT_P12 == '' || env.APPLE_NOTARY_PRIVATE_KEY == '') + run: | + echo "::warning ::Notarization skipped — APPLE_DEVELOPER_CERT_P12 and/or APPLE_NOTARY_PRIVATE_KEY not configured. End users will hit Gatekeeper quarantine. See docs/INSTALLATION.md." + - name: Publish release assets uses: softprops/action-gh-release@v2 with: - files: dist/apw-macos-${{ github.ref_name }}.tar.gz + # Notarized .app zip is uploaded when present; the tarball is + # always present. + files: | + dist/apw-macos-${{ github.ref_name }}.tar.gz + dist/APW-${{ github.ref_name }}.zip + fail_on_unmatched_files: false + + homebrew-tap: + # Render the Homebrew formula for the freshly-published tarball and + # open a PR in the tap repo. Failure here must NOT block the release + # itself — operators can still re-run the render script by hand. See + # issue #6. + needs: release + if: startsWith(github.ref, 'refs/tags/v') + runs-on: ubuntu-latest + continue-on-error: true + permissions: + contents: read + env: + HOMEBREW_TAP_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Resolve release version + id: version + run: | + version="${GITHUB_REF_NAME#v}" + echo "value=$version" >> "$GITHUB_OUTPUT" + + - name: Download release tarball + run: | + url="${{ github.server_url }}/${{ github.repository }}/archive/refs/tags/${{ github.ref_name }}.tar.gz" + curl -fsSL -o /tmp/apw-source.tar.gz "$url" + sha256="$(shasum -a 256 /tmp/apw-source.tar.gz | awk '{print $1}')" + echo "APW_RELEASE_SHA256=$sha256" >> "$GITHUB_ENV" + + - name: Render Homebrew formula + run: | + ./scripts/render-homebrew-formula.sh \ + "${{ steps.version.outputs.value }}" \ + "$APW_RELEASE_SHA256" \ + > /tmp/apw.rb + echo "--- rendered formula ---" + cat /tmp/apw.rb + + - name: Open PR against tap repo + if: env.HOMEBREW_TAP_TOKEN != '' + env: + TAP_REPO: omt-global/homebrew-apw + TAP_BRANCH: release/v${{ steps.version.outputs.value }} + run: | + set -euo pipefail + workdir="$(mktemp -d)" + git clone --depth=1 \ + "https://x-access-token:${HOMEBREW_TAP_TOKEN}@github.com/${TAP_REPO}.git" \ + "$workdir" + cd "$workdir" + git config user.email "release-bot@omt.dev" + git config user.name "apw-release-bot" + git checkout -b "$TAP_BRANCH" + mkdir -p Formula + cp /tmp/apw.rb Formula/apw.rb + git add Formula/apw.rb + git commit -m "apw ${{ steps.version.outputs.value }}" + git push -u origin "$TAP_BRANCH" + curl -fsS \ + -X POST \ + -H "Authorization: Bearer ${HOMEBREW_TAP_TOKEN}" \ + -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/${TAP_REPO}/pulls" \ + -d @- <." + } + JSON + + - name: Note when tap token is unavailable + if: env.HOMEBREW_TAP_TOKEN == '' + run: | + echo "::warning ::HOMEBREW_TAP_TOKEN not configured; skipping tap PR. Render the formula manually with scripts/render-homebrew-formula.sh." diff --git a/docs/INSTALLATION.md b/docs/INSTALLATION.md index 2298a46..7d4e07a 100644 --- a/docs/INSTALLATION.md +++ b/docs/INSTALLATION.md @@ -45,6 +45,29 @@ cargo install --path rust --locked apw app install ``` +## Notarization + +When the release CI is fully wired with Apple credentials (issue #7), +release tag builds: + +1. Sign the `.app` bundle with the configured Developer ID Application + certificate. +2. Submit the signed bundle to Apple Notary Service via + `xcrun notarytool submit --wait`. +3. Staple the notarization ticket to the bundle. +4. Re-zip the stapled bundle and upload it as the GitHub Release asset + `APW-.zip` alongside the source tarball. + +When the required secrets are not configured, the workflow emits a +`::warning::` and skips notarization. End users that hit Gatekeeper +quarantine on an unnotarized build can run: + +```bash +xattr -d com.apple.quarantine /Applications/APW.app +``` + +This manual fallback is interim until issue #7 is fully landed. + ## Homebrew ### Local formula smoke test @@ -67,7 +90,19 @@ This validates: ### Publish your own tap Use [`packaging/homebrew/apw.rb`](../packaging/homebrew/apw.rb) -as the formula template. +as the formula template, or render a release-pinned formula with the +helper script: + +```bash +./scripts/render-homebrew-formula.sh \ + 2.0.1 \ + "$(curl -fsSL https://github.com/OMT-Global/apw/archive/refs/tags/v2.0.1.tar.gz | shasum -a 256 | awk '{print $1}')" \ + > Formula/apw.rb +``` + +The release workflow renders this automatically and opens a PR against +the tap repository when `HOMEBREW_TAP_TOKEN` is set. Failures in the +tap step do not block the release itself — see issue #6. After installing with Homebrew, install the per-user APW app bundle: diff --git a/docs/bootstrap/onboarding.md b/docs/bootstrap/onboarding.md index 0120550..c420f70 100644 --- a/docs/bootstrap/onboarding.md +++ b/docs/bootstrap/onboarding.md @@ -37,6 +37,25 @@ - Run `scripts/bump-version.sh ` from the repository root to update all version-bearing release surfaces. - Run `bash scripts/ci/run-fast-checks.sh` after version bumps before opening a release PR. + ### Release secrets + + The following repository secrets are consumed by `.github/workflows/release.yml`: + + | Secret | Purpose | + | ---------------------------- | ------------------------------------------------------------- | + | `APPLE_DEVELOPER_CERT_P12` | base64-encoded Developer ID Application .p12 (issue #7) | + | `APPLE_CERT_PASSWORD` | passphrase for the .p12 above | + | `APPLE_TEAM_ID` | 10-character Apple Developer Team ID | + | `APPLE_NOTARY_KEY_ID` | App Store Connect API key id used by `notarytool` | + | `APPLE_NOTARY_KEY_ISSUER` | App Store Connect issuer UUID | + | `APPLE_NOTARY_PRIVATE_KEY` | base64-encoded `.p8` private key for `notarytool` | + | `HOMEBREW_TAP_TOKEN` | scoped `contents:write` token on the tap repo (issue #6) | + + All Apple credentials are optional — when absent, the workflow emits + a `::warning::` and continues without notarization. The Homebrew tap + job is `continue-on-error` so a missing or rejected token does not + block the release. + ## Home Profiles - Run `project-bootstrap apply home --manifest ./project.bootstrap.yaml` after reviewing the bundled profile content. diff --git a/scripts/render-homebrew-formula.sh b/scripts/render-homebrew-formula.sh new file mode 100755 index 0000000..d996d7b --- /dev/null +++ b/scripts/render-homebrew-formula.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# Render the Homebrew formula for a published APW release. +# +# Usage: +# scripts/render-homebrew-formula.sh [tarball-url] +# +# Reads packaging/homebrew/apw.rb as the template, substitutes the +# version, tarball URL, and tarball sha256, and writes the rendered +# formula to stdout. Designed to be called from release CI (issue #6) +# or by hand for a manual tap update. +# +# Example: +# scripts/render-homebrew-formula.sh 2.0.1 \ +# "$(shasum -a 256 dist/apw-v2.0.1.tar.gz | awk '{print $1}')" \ +# "https://github.com/OMT-Global/apw/archive/refs/tags/v2.0.1.tar.gz" \ +# > /tmp/apw.rb + +set -euo pipefail + +if [[ $# -lt 2 || $# -gt 3 ]]; then + echo "usage: $(basename "$0") [tarball-url]" >&2 + exit 64 +fi + +version="$1" +sha256="$2" +default_url="https://github.com/OMT-Global/apw/archive/refs/tags/v${version}.tar.gz" +url="${3:-$default_url}" + +if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.-]+)?$ ]]; then + echo "render-homebrew-formula: version must be SemVer, got '$version'" >&2 + exit 65 +fi + +if [[ ! "$sha256" =~ ^[0-9a-fA-F]{64}$ ]]; then + echo "render-homebrew-formula: sha256 must be a 64-char hex digest, got '$sha256'" >&2 + exit 65 +fi + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +template="$repo_root/packaging/homebrew/apw.rb" + +if [[ ! -f "$template" ]]; then + echo "render-homebrew-formula: missing template at $template" >&2 + exit 66 +fi + +# Use awk to do the substitution so we don't depend on GNU sed semantics. +awk -v ver="$version" -v sha="$sha256" -v url="$url" ' + /^[[:space:]]*version "/ { print " version \"" ver "\""; next } + /^[[:space:]]*url "/ { print " url \"" url "\""; next } + /^[[:space:]]*sha256 "/ { print " sha256 \"" sha "\""; next } + { print } +' "$template" From bd6b52a1b002a02b28a87a4bb839b78264e1aa05 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 20:15:53 +0000 Subject: [PATCH 5/7] Add AuthenticationServices broker scaffold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #13: New `AuthenticationServicesBroker.swift` introduces: - A `CredentialBroker` protocol so `BrokerCore` can dispatch login requests without hard-binding to AuthenticationServices. - An `AppleAuthenticationServicesBroker` implementation that builds an `ASAuthorizationPasswordRequest`, drives `ASAuthorizationController` on the main thread, bridges the async delegate callback back to the broker's worker thread via `DispatchSemaphore`, and bounds the wait by `brokerRequestTimeoutMs` so a hung credential picker cannot block the broker. - A stable `BrokerErrorCode` enum (`canceled` / `failed` / `invalidResponse` / `notHandled` / `unknown`, plus `unsupportedDomain` / `noCredentialSource`) and a `brokerStatus(for:)` mapper that aligns those codes with the Rust `Status` enum on the wire (1 / 104 / 3). - A `defaultCredentialBroker()` factory that returns the real broker on Apple targets and `nil` elsewhere. `BrokerCore.loginResponse` now routes through the injected broker when `APW_DEMO` is unset, mapping outcomes onto the existing wire envelope: success returns `transport: "authentication_services"` with `userMediated: true`; denial returns the typed "User denied" error; failures return the `BrokerErrorCode` plus the Rust-mapped integer status. When no broker is wired, the response remains `no_credential_source`. `BrokerCoreTests` adds a `StubCredentialBroker` and four new tests covering success, denied, canceled, and invalidResponse outcomes plus a table-driven assertion on `brokerStatus(for:)`. The default test server now injects `nil` for the broker so XCTest never triggers a real `ASAuthorizationController` request. NATIVE_ONLY_REDESIGN.md gains a Phase 3 status block listing what landed and the remaining exit blocker (notarized-build verification on a real macOS host with associated-domain entitlements). README.md now distinguishes the real `apw login ` flow from the `APW_DEMO=1`-gated bootstrap. Caveat: this scaffold cannot be compiled or run from the CI Linux box. The Swift surface follows the public AS API but the end-to-end behavior (associated-domain matching, run-loop pumping inside an LSUIElement broker, ASAuthorizationError code stability) requires verification on macOS — flagged inline with `TODO(#13)`. https://claude.ai/code/session_015zj6QWfz9zWVEe16GDRk5u --- README.md | 23 +- docs/NATIVE_ONLY_REDESIGN.md | 25 ++ .../AuthenticationServicesBroker.swift | 233 ++++++++++++++++++ .../Sources/NativeAppLib/BrokerCore.swift | 92 ++++++- .../NativeAppTests/BrokerCoreTests.swift | 122 ++++++++- 5 files changed, 479 insertions(+), 16 deletions(-) create mode 100644 native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift diff --git a/README.md b/README.md index b1c49d6..8a1db70 100644 --- a/README.md +++ b/README.md @@ -87,12 +87,27 @@ The supported `v2.0.0` bootstrap flow is app-first: apw app install apw app launch apw doctor --json -apw login https://example.com +apw login https://vault.example.com ``` -The current bootstrap domain is `https://example.com`. The APW app uses a -same-user local broker socket and explicit approval UI for the returned -credential flow. +In a notarized build with associated-domain entitlements wired, +`apw login` routes through the +[`AuthenticationServicesBroker`](native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift) +and returns an iCloud Keychain credential surfaced via the Apple +credential picker (issue #13). + +A separate **demo bootstrap path** is available for first-run +validation. Setting `APW_DEMO=1` makes the broker materialize and +return the bundled placeholder credential for `https://example.com` — +nothing else. Without `APW_DEMO=1`, the demo path returns a typed +`no_credential_source` error rather than silently falling back to a +plaintext file (issue #14): + +```bash +APW_DEMO=1 apw app install +APW_DEMO=1 apw app launch +APW_DEMO=1 apw login https://example.com +``` Optional reduced-security mode for external password managers can be configured in `~/.apw/config.json` with an absolute provider path: diff --git a/docs/NATIVE_ONLY_REDESIGN.md b/docs/NATIVE_ONLY_REDESIGN.md index ee6ee14..dda9dc0 100644 --- a/docs/NATIVE_ONLY_REDESIGN.md +++ b/docs/NATIVE_ONLY_REDESIGN.md @@ -205,6 +205,31 @@ Deliverables: - end-to-end sign-in flow for one associated domain - stable error mapping for cancel, denial, timeout, and unsupported-domain cases +#### Phase 3 status (issue #13) + +- `native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift` + introduces a `CredentialBroker` protocol, the + `AppleAuthenticationServicesBroker` implementation that drives + `ASAuthorizationController` + `ASAuthorizationPasswordRequest` on the + main thread and bridges results back to the worker thread via + `DispatchSemaphore`, and a stable `BrokerErrorCode` mapping + (`canceled` / `failed` / `invalidResponse` / `notHandled` / `unknown`). +- `BrokerCore.loginResponse` routes through the injected broker when + `APW_DEMO` is unset, mapping outcomes onto the existing wire envelope + (`transport: "authentication_services"`, `userMediated: true`, integer + status codes that match the Rust `Status` enum). +- `BrokerCoreTests` exercises the broker outcome paths via + `StubCredentialBroker` for `success` / `denied` / `canceled` / + `invalidResponse`, and asserts the broker error code mapping. + +**Phase 3 exit blockers still open**: + +- The integration is unverified against a notarized build with + associated-domain entitlements wired (the macOS build cannot be + exercised from CI on Linux). A follow-up validation pass on a real + macOS host is required before declaring Phase 3 complete. +- Domain expansion beyond `example.com` is tracked in issue #8. + ### Phase 4: command migration and deprecation - Add compatibility warnings to `pw` and `otp` diff --git a/native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift b/native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift new file mode 100644 index 0000000..76f28fe --- /dev/null +++ b/native-app/Sources/NativeAppLib/AuthenticationServicesBroker.swift @@ -0,0 +1,233 @@ +import Foundation + +#if canImport(AuthenticationServices) + import AuthenticationServices + import AppKit +#endif + +/// Stable broker error codes returned by the credential broker. These are +/// the codes the Rust CLI maps from when an external auth attempt fails. +/// See issue #13 / docs/SECURITY_POSTURE_AND_TESTING.md. +public enum BrokerErrorCode: String { + case canceled + case failed + case invalidResponse + case notHandled + case unknown + case unsupportedDomain + case noCredentialSource +} + +/// Result of a single credential request brokered through the OS. +public enum CredentialBrokerResult { + case success(BrokerCredential) + case denied + case failure(BrokerErrorCode, String) +} + +public struct BrokerCredential { + public let domain: String + public let url: String + public let username: String + public let password: String + + public init(domain: String, url: String, username: String, password: String) { + self.domain = domain + self.url = url + self.username = username + self.password = password + } +} + +/// Abstraction over the credential broker so `BrokerCore` can dispatch +/// without hard-binding to AuthenticationServices. Tests inject a stub +/// (see `BrokerCoreTests.swift`); production wires +/// `AppleAuthenticationServicesBroker`. +public protocol CredentialBroker { + func login(url: String) -> CredentialBrokerResult +} + +#if canImport(AuthenticationServices) + + /// Real broker that uses `ASAuthorizationController` + iCloud Keychain to + /// surface credentials for an associated domain. The bridge is sync — + /// the broker server thread blocks on a `DispatchSemaphore` while the + /// run loop drives `ASAuthorizationController` on the main queue. See + /// issue #13. + /// + /// TODO(#13): this scaffold compiles against the public AS API surface, + /// but the end-to-end behavior (associated-domain matching, run-loop + /// pumping inside an LSUIElement broker, ASAuthorizationError code + /// stability across macOS versions) requires verification on a real + /// macOS host with the app entitlements wired. See acceptance criteria + /// in issue #13. + public final class AppleAuthenticationServicesBroker: NSObject, CredentialBroker { + + private let presentationProvider: PresentationContextProvider + + public override init() { + self.presentationProvider = PresentationContextProvider() + super.init() + } + + public func login(url rawURL: String) -> CredentialBrokerResult { + guard let url = URL(string: rawURL), + let host = url.host?.lowercased(), + !host.isEmpty + else { + return .failure(.invalidResponse, "Invalid URL for native app login.") + } + + let semaphore = DispatchSemaphore(value: 0) + var captured: CredentialBrokerResult = .failure(.unknown, "broker did not complete") + + // ASAuthorizationController must be driven on the main thread. The + // broker server runs accept() on a worker thread, so we hop to + // main and block on a semaphore until the delegate fires. + DispatchQueue.main.async { + let request = ASAuthorizationPasswordProvider().createRequest() + let controller = ASAuthorizationController(authorizationRequests: [request]) + + let delegate = AuthorizationDelegate { result in + captured = self.mapResult(result, host: host, url: rawURL) + semaphore.signal() + } + controller.delegate = delegate + controller.presentationContextProvider = self.presentationProvider + + // The broker keeps a strong reference to the delegate via the + // controller until the request resolves; no manual retain is + // needed beyond the closure. + objc_setAssociatedObject( + controller, + &AppleAuthenticationServicesBroker.delegateKey, + delegate, + .OBJC_ASSOCIATION_RETAIN + ) + + controller.performRequests() + } + + // Bound the sync wait so a hung credential picker cannot hold the + // broker forever. `brokerRequestTimeoutMs` is the same constant + // used for IPC. (issue #2) + let timeout = DispatchTime.now() + .milliseconds(brokerRequestTimeoutMs) + if semaphore.wait(timeout: timeout) == .timedOut { + return .failure(.failed, "AuthenticationServices request timed out.") + } + return captured + } + + private func mapResult( + _ result: AuthorizationDelegate.Outcome, + host: String, + url: String + ) -> CredentialBrokerResult { + switch result { + case .credential(let credential): + return .success( + BrokerCredential( + domain: host, + url: url, + username: credential.user, + password: credential.password + )) + case .canceled: + return .denied + case .error(let error): + return .failure(brokerCode(for: error), error.localizedDescription) + } + } + + private func brokerCode(for error: Error) -> BrokerErrorCode { + guard let asError = error as? ASAuthorizationError else { + return .unknown + } + switch asError.code { + case .canceled: + return .canceled + case .failed: + return .failed + case .invalidResponse: + return .invalidResponse + case .notHandled: + return .notHandled + case .unknown: + return .unknown + @unknown default: + return .unknown + } + } + + private static var delegateKey: UInt8 = 0 + } + + /// Bridges the `ASAuthorizationController` callback into a single + /// `Outcome` that maps cleanly onto `CredentialBrokerResult`. + private final class AuthorizationDelegate: NSObject, + ASAuthorizationControllerDelegate + { + enum Outcome { + case credential(ASPasswordCredential) + case canceled + case error(Error) + } + + private let completion: (Outcome) -> Void + + init(completion: @escaping (Outcome) -> Void) { + self.completion = completion + } + + func authorizationController( + controller: ASAuthorizationController, + didCompleteWithAuthorization authorization: ASAuthorization + ) { + if let credential = authorization.credential as? ASPasswordCredential { + completion(.credential(credential)) + } else { + completion( + .error( + NSError( + domain: "dev.omt.apw", + code: -1, + userInfo: [ + NSLocalizedDescriptionKey: "Unexpected ASAuthorization credential type." + ] + ))) + } + } + + func authorizationController( + controller: ASAuthorizationController, + didCompleteWithError error: Error + ) { + if let asError = error as? ASAuthorizationError, asError.code == .canceled { + completion(.canceled) + } else { + completion(.error(error)) + } + } + } + + private final class PresentationContextProvider: NSObject, + ASAuthorizationControllerPresentationContextProviding + { + func presentationAnchor(for controller: ASAuthorizationController) + -> ASPresentationAnchor + { + // The broker is an LSUIElement app with no main window. The + // ASAuthorizationController API still requires an anchor; falling + // back to a borderless transient window keeps the call honest. + // TODO(#13): verify this anchor lifetime against a notarized build. + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 1, height: 1), + styleMask: [.borderless], + backing: .buffered, + defer: false + ) + return window + } + } + +#endif diff --git a/native-app/Sources/NativeAppLib/BrokerCore.swift b/native-app/Sources/NativeAppLib/BrokerCore.swift index d5817d1..b9c142e 100644 --- a/native-app/Sources/NativeAppLib/BrokerCore.swift +++ b/native-app/Sources/NativeAppLib/BrokerCore.swift @@ -17,6 +17,32 @@ private let credentialsFileName = "credentials.json" /// indefinitely on a hung peer. See issue #2. let brokerRequestTimeoutMs: Int = 3_000 +/// Map a `BrokerErrorCode` to the integer status code used in the wire +/// envelope so the Rust CLI's typed `Status` enum stays stable. See +/// issue #13. +func brokerStatus(for code: BrokerErrorCode) -> Int { + switch code { + case .canceled, .failed, .unknown: + return 1 // GenericError + case .invalidResponse: + return 104 // ProtoInvalidResponse + case .notHandled, .unsupportedDomain, .noCredentialSource: + return 3 // NoResults + } +} + +/// Factory for the default credential broker. Returns the +/// AuthenticationServices implementation when the framework is +/// available, otherwise nil so callers can fall back to demo or +/// external providers. See issue #13. +func defaultCredentialBroker() -> CredentialBroker? { + #if canImport(AuthenticationServices) + return AppleAuthenticationServicesBroker() + #else + return nil + #endif +} + /// Environment variable that opts the broker into the demo bootstrap path. /// When unset, the broker never materializes a plaintext credentials file /// and returns `no_credential_source` for login requests. See issue #14. @@ -153,10 +179,16 @@ final class BrokerServer { private let paths: AppPaths private let startedAt = ISO8601DateFormatter().string(from: Date()) private let approvalPrompter: ApprovalPrompter + private let credentialBroker: CredentialBroker? - init(paths: AppPaths, approvalPrompter: ApprovalPrompter = SystemApprovalPrompter()) { + init( + paths: AppPaths, + approvalPrompter: ApprovalPrompter = SystemApprovalPrompter(), + credentialBroker: CredentialBroker? = defaultCredentialBroker() + ) { self.paths = paths self.approvalPrompter = approvalPrompter + self.credentialBroker = credentialBroker } func run() throws -> Never { @@ -300,15 +332,55 @@ final class BrokerServer { ) } - guard demoModeEnabled() else { - return ResponseEnvelope( - ok: false, - code: 3, - payload: nil, - error: - "no_credential_source: the AuthenticationServices broker is not yet wired. Set APW_DEMO=1 to use the bundled demo credential, or configure a fallback provider in ~/.apw/config.json.", - requestId: requestId - ) + // Non-demo path: route through the AuthenticationServices broker + // (issue #13). When no broker is wired (e.g. older macOS, missing + // entitlement) we surface a typed `no_credential_source` error so + // the CLI can fall back to an external provider. + if !demoModeEnabled() { + guard let broker = credentialBroker else { + return ResponseEnvelope( + ok: false, + code: 3, + payload: nil, + error: + "no_credential_source: the AuthenticationServices broker is not wired in this build. Set APW_DEMO=1 for the bootstrap path, or configure a fallback provider in ~/.apw/config.json.", + requestId: requestId + ) + } + switch broker.login(url: rawURL) { + case .success(let credential): + return ResponseEnvelope( + ok: true, + code: 0, + payload: [ + "status": AnyCodable("approved"), + "url": AnyCodable(credential.url), + "domain": AnyCodable(credential.domain), + "username": AnyCodable(credential.username), + "password": AnyCodable(credential.password), + "transport": AnyCodable("authentication_services"), + "userMediated": AnyCodable(true), + ], + error: nil, + requestId: requestId + ) + case .denied: + return ResponseEnvelope( + ok: false, + code: 1, + payload: nil, + error: "User denied the APW login request.", + requestId: requestId + ) + case .failure(let code, let message): + return ResponseEnvelope( + ok: false, + code: brokerStatus(for: code), + payload: nil, + error: "\(code.rawValue): \(message)", + requestId: requestId + ) + } } guard host == "example.com" else { diff --git a/native-app/Tests/NativeAppTests/BrokerCoreTests.swift b/native-app/Tests/NativeAppTests/BrokerCoreTests.swift index 79b4cba..819d243 100644 --- a/native-app/Tests/NativeAppTests/BrokerCoreTests.swift +++ b/native-app/Tests/NativeAppTests/BrokerCoreTests.swift @@ -11,6 +11,14 @@ private struct StubApprovalPrompter: ApprovalPrompter { } } +private struct StubCredentialBroker: CredentialBroker { + let outcome: CredentialBrokerResult + + func login(url: String) -> CredentialBrokerResult { + outcome + } +} + final class BrokerCoreTests: XCTestCase { private func makePaths(_ root: URL) -> AppPaths { AppPaths( @@ -21,11 +29,20 @@ final class BrokerCoreTests: XCTestCase { ) } + /// Test servers default to no `CredentialBroker` so they exercise the + /// `no_credential_source` path instead of triggering a real + /// `ASAuthorizationController` request during XCTest. Tests that + /// exercise the broker path inject a `StubCredentialBroker` explicitly. private func makeServer( root: URL, - decision: Bool = true + decision: Bool = true, + credentialBroker: CredentialBroker? = nil ) -> BrokerServer { - BrokerServer(paths: makePaths(root), approvalPrompter: StubApprovalPrompter(decision: decision)) + BrokerServer( + paths: makePaths(root), + approvalPrompter: StubApprovalPrompter(decision: decision), + credentialBroker: credentialBroker + ) } private func writeCredentials( @@ -208,4 +225,105 @@ final class BrokerCoreTests: XCTestCase { XCTAssertNotNil(guidance) XCTAssertFalse(guidance?.contains(where: { $0.contains("APW_NATIVE_APP_AUTO_APPROVE") }) ?? true) } + + // MARK: - AuthenticationServices broker routing (issue #13) + + func testLoginRoutesToCredentialBrokerOnSuccess() throws { + unsetenv("APW_DEMO") + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + let broker = StubCredentialBroker( + outcome: .success( + BrokerCredential( + domain: "vault.example.com", + url: "https://vault.example.com", + username: "alice@example.com", + password: "real-keychain-password" + ))) + let server = makeServer(root: root, credentialBroker: broker) + + let response = try server.dispatch( + request: RequestEnvelope( + requestId: "ok", + command: "login", + payload: ["url": "https://vault.example.com"] + )) + + XCTAssertEqual(response.ok, true) + XCTAssertEqual(response.code, 0) + XCTAssertEqual(response.payload?["transport"]?.value as? String, "authentication_services") + XCTAssertEqual(response.payload?["username"]?.value as? String, "alice@example.com") + XCTAssertEqual(response.payload?["domain"]?.value as? String, "vault.example.com") + XCTAssertEqual(response.payload?["userMediated"]?.value as? Bool, true) + } + + func testLoginRoutesToCredentialBrokerOnDeny() throws { + unsetenv("APW_DEMO") + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + let broker = StubCredentialBroker(outcome: .denied) + let server = makeServer(root: root, credentialBroker: broker) + + let response = try server.dispatch( + request: RequestEnvelope( + requestId: "denied", + command: "login", + payload: ["url": "https://vault.example.com"] + )) + + XCTAssertEqual(response.ok, false) + XCTAssertEqual(response.code, 1) + XCTAssertEqual(response.error, "User denied the APW login request.") + } + + func testLoginRoutesToCredentialBrokerOnCanceled() throws { + unsetenv("APW_DEMO") + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + let broker = StubCredentialBroker( + outcome: .failure(.canceled, "ASAuthorizationError canceled")) + let server = makeServer(root: root, credentialBroker: broker) + + let response = try server.dispatch( + request: RequestEnvelope( + requestId: "cancel", + command: "login", + payload: ["url": "https://vault.example.com"] + )) + + XCTAssertEqual(response.ok, false) + XCTAssertEqual(response.code, 1) + XCTAssertTrue(response.error?.contains("canceled") ?? false) + } + + func testLoginRoutesToCredentialBrokerOnInvalidResponse() throws { + unsetenv("APW_DEMO") + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent(UUID().uuidString, isDirectory: true) + let broker = StubCredentialBroker( + outcome: .failure(.invalidResponse, "ASAuthorizationError invalidResponse")) + let server = makeServer(root: root, credentialBroker: broker) + + let response = try server.dispatch( + request: RequestEnvelope( + requestId: "invalid", + command: "login", + payload: ["url": "https://vault.example.com"] + )) + + XCTAssertEqual(response.ok, false) + // invalidResponse maps to ProtoInvalidResponse (104) in the Rust enum. + XCTAssertEqual(response.code, 104) + XCTAssertTrue(response.error?.contains("invalidResponse") ?? false) + } + + func testBrokerStatusMappingCoversAllErrorCodes() { + XCTAssertEqual(brokerStatus(for: .canceled), 1) + XCTAssertEqual(brokerStatus(for: .failed), 1) + XCTAssertEqual(brokerStatus(for: .unknown), 1) + XCTAssertEqual(brokerStatus(for: .invalidResponse), 104) + XCTAssertEqual(brokerStatus(for: .notHandled), 3) + XCTAssertEqual(brokerStatus(for: .unsupportedDomain), 3) + XCTAssertEqual(brokerStatus(for: .noCredentialSource), 3) + } } From 261aa18270839cc966eb43d1902ea166ba55b1e5 Mon Sep 17 00:00:00 2001 From: John McChesney TenEyck Jr Date: Wed, 6 May 2026 22:15:58 -0500 Subject: [PATCH 6/7] Fix external fallback security regressions --- rust/tests/security_regressions.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/rust/tests/security_regressions.rs b/rust/tests/security_regressions.rs index 2af518a..aeb77ff 100644 --- a/rust/tests/security_regressions.rs +++ b/rust/tests/security_regressions.rs @@ -225,8 +225,10 @@ fn login_rejects_relative_external_provider_path() { install_native_app_no_results(home); write_fallback_provider_config(home, "bw"); - let (status, stdout, stderr) = - run_command(home, &["--json", "login", "https://vault.example.com"]); + let (status, stdout, stderr) = run_command( + home, + &["--json", "login", "--external-fallback", "https://vault.example.com"], + ); assert_eq!( status, 102, @@ -247,8 +249,10 @@ fn login_rejects_tilde_external_provider_path() { install_native_app_no_results(home); write_fallback_provider_config(home, "~/bin/bw"); - let (status, stdout, stderr) = - run_command(home, &["--json", "login", "https://vault.example.com"]); + let (status, stdout, stderr) = run_command( + home, + &["--json", "login", "--external-fallback", "https://vault.example.com"], + ); assert_eq!( status, 102, @@ -274,8 +278,10 @@ fn login_rejects_world_writable_external_provider_path() { .expect("failed to chmod fallback provider"); write_fallback_provider_config(home, &provider_path.display().to_string()); - let (status, stdout, stderr) = - run_command(home, &["--json", "login", "https://vault.example.com"]); + let (status, stdout, stderr) = run_command( + home, + &["--json", "login", "--external-fallback", "https://vault.example.com"], + ); assert_eq!( status, 102, @@ -303,8 +309,10 @@ fn login_rejects_external_provider_symlink_to_insecure_target() { symlink(&provider_path, &provider_link).expect("failed to create provider symlink"); write_fallback_provider_config(home, &provider_link.display().to_string()); - let (status, stdout, stderr) = - run_command(home, &["--json", "login", "https://vault.example.com"]); + let (status, stdout, stderr) = run_command( + home, + &["--json", "login", "--external-fallback", "https://vault.example.com"], + ); assert_eq!( status, 102, From bbd40c4761d8fe73325891a9be52994a18646426 Mon Sep 17 00:00:00 2001 From: John McChesney TenEyck Jr Date: Wed, 6 May 2026 22:18:04 -0500 Subject: [PATCH 7/7] Format external fallback regression tests --- rust/tests/security_regressions.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/rust/tests/security_regressions.rs b/rust/tests/security_regressions.rs index aeb77ff..6a32f7f 100644 --- a/rust/tests/security_regressions.rs +++ b/rust/tests/security_regressions.rs @@ -227,7 +227,12 @@ fn login_rejects_relative_external_provider_path() { let (status, stdout, stderr) = run_command( home, - &["--json", "login", "--external-fallback", "https://vault.example.com"], + &[ + "--json", + "login", + "--external-fallback", + "https://vault.example.com", + ], ); assert_eq!( @@ -251,7 +256,12 @@ fn login_rejects_tilde_external_provider_path() { let (status, stdout, stderr) = run_command( home, - &["--json", "login", "--external-fallback", "https://vault.example.com"], + &[ + "--json", + "login", + "--external-fallback", + "https://vault.example.com", + ], ); assert_eq!( @@ -280,7 +290,12 @@ fn login_rejects_world_writable_external_provider_path() { let (status, stdout, stderr) = run_command( home, - &["--json", "login", "--external-fallback", "https://vault.example.com"], + &[ + "--json", + "login", + "--external-fallback", + "https://vault.example.com", + ], ); assert_eq!( @@ -311,7 +326,12 @@ fn login_rejects_external_provider_symlink_to_insecure_target() { let (status, stdout, stderr) = run_command( home, - &["--json", "login", "--external-fallback", "https://vault.example.com"], + &[ + "--json", + "login", + "--external-fallback", + "https://vault.example.com", + ], ); assert_eq!(