From 2ef89800e5ec3aaebcdbe5b9d4eb9d303cdd3a48 Mon Sep 17 00:00:00 2001 From: Cristian Falcas Date: Tue, 1 Sep 2026 14:59:26 +0100 Subject: [PATCH 1/3] fix(auth): log in when the browser is not on this machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `aspect auth login` opened the browser by shelling out to `open`, then waited for the OAuth code on a loopback listener. Both halves fail away from a desktop: - `open` is macOS. On Linux it is at best Debian's xdg-utils symlink, so a login either recited `www-browser: not found` six times or died with "failed to spawn command open" and an AXL traceback (ENG-2098). - Worse, the redirect is a loopback address. Paste the URL into a browser on another machine and the code is delivered to *that* machine's port — ERR_CONNECTION_REFUSED for the user and a CLI that waits forever. No amount of browser-opening fixes it; over SSH the code has to come back by hand. Opening a browser moves into the runtime as `ctx.aspect.auth.open_browser`, which carries the platform table (`$BROWSER`, then xdg-open/gio/gnome-open/ kde-open/x-www-browser/wslview on Linux, `open` on macOS, rundll32 on Windows — it takes the URL as one argument, where `cmd /c start` would hand an `&`-separated authorize URL back to cmd.exe to re-parse). Launcher output is discarded; the task prints one message that says what to do instead. It reports "headless" without trying at all when this is an SSH session with no forwarded display. A display counts only if it names a host (`localhost:10.0`, what ssh -X sets) — a bare `:0` is the remote machine's own screen, which is what a Cloud Workstation image sets and where a browser would open in front of nobody. `AuthSession.redeem(pasted)` is then the login path for those sessions: it takes the browser's full address-bar URL, a bare query string, or the bare code. `wait()` and `redeem()` now share one `complete()`, so both reach an identical token exchange; `state` is still validated whenever it is present, and a stale paste from an earlier run is rejected. Off a TTY there is nothing to paste with, so the listener is still waited on — a forwarded port delivers. `--no-browser` forces the paste flow on both `login` and `configure`. The port is only worth forwarding for the Aspect account, which binds a fixed 19556; a self-hosted deployment takes a fresh port each run, so that caveat lives in the flag's help text rather than as a hint the user cannot act on. Co-Authored-By: Claude Opus 5 (1M context) --- .../aspect-cli/src/builtins/aspect/auth.axl | 83 ++- crates/axl-runtime/src/engine/aspect/auth.rs | 494 ++++++++++++++++-- 2 files changed, 525 insertions(+), 52 deletions(-) diff --git a/crates/aspect-cli/src/builtins/aspect/auth.axl b/crates/aspect-cli/src/builtins/aspect/auth.axl index db584d84b..7296ed38f 100644 --- a/crates/aspect-cli/src/builtins/aspect/auth.axl +++ b/crates/aspect-cli/src/builtins/aspect/auth.axl @@ -3,17 +3,68 @@ load("./private/lib/deployment_flags.axl", "REMOTE_DEFAULT_CAPS", "capability_la load("./private/lib/environment.axl", "error", "info", "warn") load("./private/lib/prompt.axl", "prompt_choice") +def _open_login_url(ctx: TaskContext, session) -> bool: + """Get the user in front of the authorize URL, returning whether a browser was + opened *here* — which is what decides whether the OAuth redirect can reach the + loopback listener on its own. + + `--no-browser` skips the attempt outright. Otherwise the runtime walks this + platform's openers ($BROWSER, then xdg-open/gio/... on Linux, `open` on macOS, + rundll32 on Windows) and reports "headless" without trying at all when this is + an SSH session with no forwarded display.""" + status = "headless" if ctx.args.no_browser else ctx.aspect.auth.open_browser(session.url) + if status == "opened": + print("Browser opened. Waiting for authentication...") + # The escape hatch for the case detection cannot see: a remote session that + # looks local, where the browser just opened on a screen nobody is watching. + print("(Nothing opened, or it opened elsewhere? Ctrl-C and re-run with --no-browser.)") + return True + print("Open this URL in a browser to log in:") + print("") + print(" " + session.url) + print("") + return False + +def _redeem_pasted_code(ctx: TaskContext, session): + """Take the authorization code back by hand and exchange it, or None if nothing + was entered. + + The path for a browser that is not on this machine — an SSH session, mostly. + The OAuth redirect resolves to a loopback address, which only *this* host + serves, so a browser on the user's laptop lands on a page that never loads and + the address bar is the only place the code can be read from. + + No port-forwarding hint here on purpose: a self-hosted deployment binds a fresh + port every run, so `ssh -L` is only durable advice for the Aspect account flow + (fixed port 19556). It lives in --no-browser's help text, with that caveat.""" + print("Authorizing sends the browser on to {}?code=...".format(session.callback_url)) + print("— an address only this machine serves. If the browser is on another machine") + print("it will report \"refused to connect\": that is expected, and the address bar") + print("still holds the code. Copy the whole address and paste it below.") + print("") + ctx.std.io.stdout.write("Paste the callback URL (or just the code): ") + ctx.std.io.stdout.flush() + pasted = str(ctx.std.io.stdin.read(8192)).strip() + if not pasted: + return None + return session.redeem(pasted) + def _run_browser_login(ctx: TaskContext, deployment: str): - """Open the deployment's authorize URL in a browser and wait for the callback, - returning the minted credentials. Prints the URL if a browser can't be opened.""" + """Authenticate the user in a browser and return the minted credentials, or None + when an interactive login was needed and the user entered nothing. + + Happy path: open a browser here and wait for the OAuth callback on the loopback + listener. When there is no browser on this machine — or it lives at the other + end of an SSH session, where that loopback redirect resolves to the wrong host — + fall back to taking the code by hand. Off a TTY there is nothing to paste with, + so keep waiting on the listener instead: a forwarded port still delivers.""" session = ctx.aspect.auth.login(deployment = deployment) - result = ctx.std.process.command("open").arg(session.url).spawn().wait() - if not result.success: - print("Open this URL to log in:") - print(" " + session.url) - else: - print("Browser opened. Waiting for authentication...") - return session.wait() + if _open_login_url(ctx, session): + return session.wait() + if not ctx.std.io.stdin.is_tty: + print("Waiting for the callback on localhost:{}...".format(session.callback_port)) + return session.wait() + return _redeem_pasted_code(ctx, session) # Width of the label column in a deployment's detail block so values align. Must # be wider than the longest label ("Results", 7) to leave a gap; a label >= this @@ -124,6 +175,9 @@ def _login_impl(ctx: TaskContext) -> int: creds = ctx.aspect.auth.login(api_token = inp, deployment = deployment) else: creds = _run_browser_login(ctx, deployment) + if creds == None: + error(ctx.std, "No authorization code entered; not logged in.") + return 1 # Persist under the deployment's own profile (so endpoint auth resolves it by # host) unless the user pinned an explicit --profile. @@ -149,6 +203,10 @@ login = task( "deployment": args.string( description = "Log in to this configured Workflows deployment (see `aspect auth configure`) instead of your Aspect account. Selects which account/deployment to authenticate.", ), + "no_browser": args.boolean( + default = False, + description = "Don't try to open a browser; print the URL and ask for the authorization code back. Use this when the browser is on another machine (e.g. over SSH), where the login callback would be redirected to the wrong host's localhost. Over SSH you can instead forward the callback port from your local machine — for your Aspect account that is the fixed `ssh -L 19556:localhost:19556 `; a self-hosted deployment binds a fresh port each run, so paste the code.", + ), "profile": args.string( description = "Advanced: the credential store key to save under (defaults to the deployment name, or \"default\" for the Aspect account). Unlike --deployment (which picks who to authenticate), --profile only changes where the credential is filed — use it to keep two identities for the same account/deployment side by side. Also settable via $ASPECT_AUTH_PROFILE.", ), @@ -222,6 +280,9 @@ def _configure_impl(ctx: TaskContext) -> int: print("(interactively, or with an API token via --with-api-token).") return 0 creds = _run_browser_login(ctx, info.name) + if creds == None: + error(ctx.std, "No authorization code entered; the deployment is configured but not logged in.") + return 1 ctx.aspect.auth.persist(creds, profile = info.name) print("") summary = ctx.aspect.auth.deployment_summary(info.name) @@ -253,6 +314,10 @@ configure = task( default = True, description = "Run the interactive login after configuring.", ), + "no_browser": args.boolean( + default = False, + description = "Don't try to open a browser; print the URL and ask for the authorization code back. Use this when the browser is on another machine (e.g. over SSH), where the login callback would be redirected to the wrong host's localhost. Over SSH you can instead forward the callback port from your local machine — for your Aspect account that is the fixed `ssh -L 19556:localhost:19556 `; a self-hosted deployment binds a fresh port each run, so paste the code.", + ), }, ) diff --git a/crates/axl-runtime/src/engine/aspect/auth.rs b/crates/axl-runtime/src/engine/aspect/auth.rs index ba5752c37..050efdee0 100644 --- a/crates/axl-runtime/src/engine/aspect/auth.rs +++ b/crates/axl-runtime/src/engine/aspect/auth.rs @@ -1580,6 +1580,139 @@ fn build_authorize_url( url } +/// Whether `DISPLAY` names a display reached over the network rather than the +/// machine's own console. SSH's X11 forwarding sets a display with a host in it +/// (`localhost:10.0` — display numbers start at 10); the local console is a bare +/// `:0`. The distinction is the whole point on a remote workstation whose image +/// sets `DISPLAY=:0` for a virtual screen nobody is watching. +fn is_forwarded_display(display: Option<&str>) -> bool { + display + .and_then(|display| display.split_once(':')) + .is_some_and(|(host, _)| !host.is_empty()) +} + +/// Whether this session has no browser of its own — an SSH login (including the +/// tunnelled kind, e.g. `gcloud workstations ssh`) with no forwarded display. +/// Launching a browser there either fails noisily or, worse, succeeds on the remote +/// machine's own screen where nobody is looking; either way the loopback the +/// browser is redirected to belongs to the wrong host. +/// +/// Forwarded X11 is the one exception: a browser launched over it draws on the +/// user's own screen *and* still reaches our loopback, so that session counts as +/// local. Wayland has no equivalent forwarding, so a `WAYLAND_DISPLAY` under SSH +/// is the remote compositor and earns no exception. +fn is_remote_session(get: impl Fn(&str) -> Option) -> bool { + let set = |key: &str| get(key).is_some_and(|value| !value.is_empty()); + if !(set("SSH_CONNECTION") || set("SSH_CLIENT") || set("SSH_TTY")) { + return false; + } + !is_forwarded_display(get("DISPLAY").as_deref()) +} + +/// Browser-launcher argv lists to try for this platform, most preferred first. +/// +/// `$BROWSER` — the Unix convention: a colon-separated list of command lines, each +/// with an optional `%s` where the URL goes — wins when set, since a user who set +/// it has already answered the question. Behind it is the platform table every CLI +/// that does this carries. `wslview` leads the Linux list unconditionally: it +/// exists only under WSL (where `xdg-open` is present but opens nothing the user +/// can see), and elsewhere it simply fails to spawn and costs a syscall. +/// +/// Windows gets `rundll32` alone. It takes the URL as a single argument, whereas +/// `cmd /c start` hands it back to `cmd.exe` to re-parse — and an OAuth authorize +/// URL is nothing but `&`-separated parameters. `$BROWSER` is not a Windows +/// convention and splitting a drive-lettered path on `:` mangles it, but the +/// mangled program merely fails to spawn and the table behind it still runs. +fn browser_launchers(browser_env: Option<&str>) -> Vec> { + let mut candidates: Vec> = Vec::new(); + for entry in browser_env.unwrap_or_default().split(':') { + let argv: Vec = entry.split_whitespace().map(str::to_string).collect(); + if !argv.is_empty() { + candidates.push(argv); + } + } + let table: &[&[&str]] = if cfg!(target_os = "macos") { + &[&["open"]] + } else if cfg!(target_os = "windows") { + &[&["rundll32.exe", "url.dll,FileProtocolHandler"]] + } else { + &[ + &["wslview"], + &["xdg-open"], + &["gio", "open"], + &["gnome-open"], + &["kde-open"], + &["x-www-browser"], + // Debian's xdg-utils installs this as a symlink to xdg-open; on macOS + // it is the real thing. Harmless last resort elsewhere. + &["open"], + ] + }; + candidates.extend( + table + .iter() + .map(|argv| argv.iter().map(|arg| arg.to_string()).collect()), + ); + candidates +} + +/// Run one launcher argv with `url` substituted for a `%s` placeholder (the +/// `$BROWSER` convention) or appended when there is none, reporting whether it +/// succeeded. +/// +/// Output is discarded on purpose: a launcher that cannot find a browser says so +/// on stderr, and `xdg-open`'s cascade of "www-browser: not found" lines is exactly +/// what the caller replaces with one message that says what to do instead. +fn spawn_launcher(argv: &[String], url: &str) -> bool { + let Some((program, rest)) = argv.split_first() else { + return false; + }; + let mut command = std::process::Command::new(program); + let mut substituted = false; + for arg in rest { + if arg.contains("%s") { + command.arg(arg.replace("%s", url)); + substituted = true; + } else { + command.arg(arg); + } + } + if !substituted { + command.arg(url); + } + command + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + // A missing launcher fails at spawn; one that ran but found no browser to + // launch (xdg-open on a machine with no desktop) reports it in the exit status. + matches!(command.status(), Ok(status) if status.success()) +} + +/// Try to open `url` in a browser here, reporting to the task which of the three +/// situations it is in: +/// +/// - `"opened"` — a browser was launched, so the OAuth redirect reaches this +/// machine's loopback listener on its own and the task can just wait. +/// - `"headless"` — not attempted, because this is an SSH session with no +/// forwarded display. +/// - `"no_browser"` — attempted, and no launcher on this platform worked. +/// +/// The latter two both mean the user opens the URL somewhere else, where the +/// loopback redirect cannot reach us and the code has to come back by hand. +fn open_url_in_browser(url: &str) -> String { + if is_remote_session(|key| std::env::var(key).ok()) { + return "headless".to_string(); + } + let browser_env = std::env::var("BROWSER").ok(); + for argv in browser_launchers(browser_env.as_deref()) { + if spawn_launcher(&argv, url) { + return "opened".to_string(); + } + } + "no_browser".to_string() +} + fn build_cloud_session(env: AuthEnv) -> anyhow::Result { let port: u16 = 19556; let listener = block_on(TcpListener::bind(format!("127.0.0.1:{}", port))).map_err(|e| { @@ -1603,6 +1736,10 @@ fn build_cloud_session(env: AuthEnv) -> anyhow::Result { ); Ok(AuthSession { url: authorize_url, + callback_port: port, + // The IdP redirects straight here, so the registered redirect is verbatim + // where the browser lands. + callback_url: redirect_uri.clone(), inner: Mutex::new(Some(AuthSessionInner { listener: Some(listener), code_verifier, @@ -1643,6 +1780,13 @@ fn build_endpoint_session(env: AuthEnv, host: &str) -> anyhow::Result, + require_state: bool, + ) -> anyhow::Result { + // The Aspect-cloud flow posts to the conventional token endpoint; the + // self-hosted flow resolves it via OIDC discovery and first validates + // the callback `state` (CSRF guard). + let token_url = match &self.kind { + SessionKind::Cloud => format!("{}/oauth/token", self.env.domain), + SessionKind::Endpoint { expected_state } => { + let matches = match state.as_deref() { + Some(state) => state == expected_state.as_str(), + None => !require_state, + }; + if !matches { + return Err(anyhow::anyhow!( + "authentication failed: callback state did not match" + )); + } + resolve_oidc_endpoints(&self.env.domain).await.token + } + }; + let token_resp = exchange_code( + &token_url, + &self.env.client_id, + &self.redirect_uri, + &code, + &self.code_verifier, + ) + .await?; + let refresh_token = token_resp.refresh_token.clone(); + // Self-hosted edges validate the id_token; the cloud flow the + // access_token. Record which so refresh keeps minting the same kind. + let prefer_id_token = matches!(self.kind, SessionKind::Endpoint { .. }); + CredentialsEntry::from_bearer( + token_resp.bearer(prefer_id_token)?, + refresh_token, + Some(self.env.domain.clone()), + Some(self.env.client_id.clone()), + prefer_id_token, + ) + } +} + +/// Take the pending login out of `session`. `wait()` and `redeem()` are two ways +/// to finish the same session, so either one consumes it. +fn take_pending(session: &AuthSession) -> anyhow::Result { + let mut guard = session + .inner + .lock() + .map_err(|_| anyhow::anyhow!("auth session already consumed or poisoned"))?; + guard + .take() + .ok_or_else(|| anyhow::anyhow!("auth session already consumed")) +} + +/// Extract the OAuth `code` (and `state`, when it came along) from whatever the +/// user pasted back: the browser's full address-bar URL, a bare query string, or +/// the code on its own. +/// +/// The `code=`/`error=` test — rather than merely looking for a `?` or `=` — keeps +/// a bare authorization code that happens to carry base64 padding from being read +/// as a query string and rejected for having no `code` parameter. +fn parse_pasted_callback(pasted: &str) -> anyhow::Result<(String, Option)> { + let pasted = pasted.trim(); + if pasted.is_empty() { + return Err(anyhow::anyhow!("no authorization code entered")); + } + let is_callback = pasted.starts_with("http://") + || pasted.starts_with("https://") + || pasted.contains("code=") + || pasted.contains("error="); + if !is_callback { + return Ok((pasted.to_string(), None)); + } + // `extract_query_param` splits on `?`; give a bare query string one to find. + let url = if pasted.contains('?') { + pasted.to_string() + } else { + format!("?{pasted}") + }; + if let Some(error) = extract_query_param(&url, "error") { + let desc = extract_query_param(&url, "error_description").unwrap_or_default(); + return Err(anyhow::anyhow!("authentication failed: {} {}", error, desc)); + } + let code = extract_query_param(&url, "code").ok_or_else(|| { + anyhow::anyhow!( + "no `code` parameter in what was pasted — after authorizing, copy the \ + browser's whole address, including everything after the `?`" + ) + })?; + Ok((code, extract_query_param(&url, "state"))) +} + #[derive(Display, ProvidesStaticType, NoSerialize, Allocative)] #[display("")] pub struct AuthSession { pub url: String, + /// The loopback port the OAuth callback ultimately lands on. Exposed so the + /// task can name it in the `ssh -L` hint. + pub callback_port: u16, + /// The loopback address the browser is left sitting on once the flow is done — + /// what the user reads the authorization code out of when the browser is on + /// another machine. Exposed so the task can quote it exactly rather than + /// approximately: the two flows do not agree on the spelling. + pub callback_url: String, #[allocative(skip)] inner: Mutex>, } @@ -1981,58 +2240,64 @@ fn auth_session_methods(registry: &mut MethodsBuilder) { attr_str!(this, AuthSession, url) } + /// The loopback port the OAuth redirect ends up on. Only this machine can + /// serve it, so a task that could not open a browser here names it in the + /// port-forwarding hint. + #[starlark(attribute)] + fn callback_port<'v>(this: values::Value<'v>) -> anyhow::Result { + Ok(this + .downcast_ref_err::() + .into_anyhow_result()? + .callback_port as i32) + } + + /// The loopback address the browser is left on when the flow finishes — where + /// the user reads the code out of the address bar. The cloud flow lands on + /// `http://localhost:/callback`; a self-hosted deployment's callback page + /// forwards to `http://127.0.0.1:/callback`. + #[starlark(attribute)] + fn callback_url<'v>(this: values::Value<'v>) -> anyhow::Result { + attr_str!(this, AuthSession, callback_url) + } + + /// Wait for the browser to deliver the authorization code to the loopback + /// listener, then exchange it. Requires the browser to be on this machine (or + /// the port to be forwarded here); see `redeem` for the other case. fn wait<'v>(this: values::Value<'v>) -> anyhow::Result { let session = this .downcast_ref_err::() .into_anyhow_result()?; - let mut guard = session - .inner - .lock() - .map_err(|_| anyhow::anyhow!("auth session already consumed or poisoned"))?; - let inner = guard.take().ok_or_else(|| { - anyhow::anyhow!("auth session already consumed (wait() called twice)") - })?; + let mut inner = take_pending(&session)?; + let listener = inner + .listener + .take() + .ok_or_else(|| anyhow::anyhow!("no listener in auth session"))?; let entry = block_on(async move { - let listener = inner - .listener - .ok_or_else(|| anyhow::anyhow!("no listener in auth session"))?; let (code, state) = accept_callback(listener).await?; - // The Aspect-cloud flow posts to the conventional token endpoint; the - // self-hosted flow resolves it via OIDC discovery and first validates - // the callback `state` (CSRF guard). - let token_url = match &inner.kind { - SessionKind::Cloud => format!("{}/oauth/token", inner.env.domain), - SessionKind::Endpoint { expected_state } => { - if state.as_deref() != Some(expected_state.as_str()) { - return Err(anyhow::anyhow!( - "authentication failed: callback state did not match" - )); - } - resolve_oidc_endpoints(&inner.env.domain).await.token - } - }; - let token_resp = exchange_code( - &token_url, - &inner.env.client_id, - &inner.redirect_uri, - &code, - &inner.code_verifier, - ) - .await?; - let refresh_token = token_resp.refresh_token.clone(); - // Self-hosted edges validate the id_token; the cloud flow the - // access_token. Record which so refresh keeps minting the same kind. - let prefer_id_token = matches!(inner.kind, SessionKind::Endpoint { .. }); - CredentialsEntry::from_bearer( - token_resp.bearer(prefer_id_token)?, - refresh_token, - Some(inner.env.domain.clone()), - Some(inner.env.client_id.clone()), - prefer_id_token, - ) + inner.complete(code, state, true).await })?; Ok(AuthCredentials::from_entry(&entry)) } + + /// Exchange an authorization code the user pasted back — the browser's full + /// callback URL, a bare query string, or the bare code. + /// + /// This is the login path for a browser that is not on this machine (an SSH + /// session, most often): the redirect resolves to *our* loopback, so the code + /// never reaches us and the browser's address bar is the only place left to + /// read it from. Drops the listener, since nothing will connect to it. + fn redeem<'v>( + this: values::Value<'v>, + #[starlark(require = pos)] pasted: &str, + ) -> anyhow::Result { + let session = this + .downcast_ref_err::() + .into_anyhow_result()?; + let inner = take_pending(&session)?; + let (code, state) = parse_pasted_callback(pasted)?; + let entry = block_on(inner.complete(code, state, false))?; + Ok(AuthCredentials::from_entry(&entry)) + } } /// One authorization server advertised by a deployment's discovery document, @@ -2468,6 +2733,17 @@ fn auth_methods(registry: &mut MethodsBuilder) { Ok(resolve_api_url()?) } + /// Try to open `url` in a browser on this machine. Returns `"opened"`, + /// `"headless"` (an SSH session with no forwarded display — not attempted), or + /// `"no_browser"` (no launcher on this platform worked). The last two tell the + /// caller the OAuth loopback redirect will land on the wrong machine. + fn open_browser<'v>( + #[allow(unused)] this: values::Value<'v>, + #[starlark(require = pos)] url: &str, + ) -> anyhow::Result { + Ok(open_url_in_browser(url)) + } + fn login<'v>( #[allow(unused)] this: values::Value<'v>, #[starlark(require = named)] token: Option<&str>, @@ -4148,4 +4424,136 @@ mod tests { Some("first".to_string()) ); } + + #[test] + fn parse_pasted_callback_reads_a_url_a_query_or_a_bare_code() { + // What the user actually copies out of the address bar. + assert_eq!( + parse_pasted_callback( + " http://localhost:19556/callback?code=abc123&state=nonce.19556 " + ) + .unwrap(), + ("abc123".to_string(), Some("nonce.19556".to_string())) + ); + // A bare query string is given a `?` so the same extractor reads it. + assert_eq!( + parse_pasted_callback("code=abc123&state=xyz").unwrap(), + ("abc123".to_string(), Some("xyz".to_string())) + ); + // Just the code, with no state to validate. + assert_eq!( + parse_pasted_callback("abc123").unwrap(), + ("abc123".to_string(), None) + ); + // Base64 padding in a bare code must not make it look like a query string — + // it would otherwise be rejected for having no `code` parameter. + assert_eq!( + parse_pasted_callback("YWJjMTIz==").unwrap(), + ("YWJjMTIz==".to_string(), None) + ); + // A url-encoded value survives the round trip. + assert_eq!( + parse_pasted_callback("https://x/cb?code=a%2Fb").unwrap().0, + "a/b".to_string() + ); + + // An authorize error the browser was redirected with is reported as one, + // rather than as a missing `code`. + let err = parse_pasted_callback( + "http://localhost:19556/callback?error=access_denied&error_description=nope", + ) + .unwrap_err() + .to_string(); + assert!( + err.contains("access_denied") && err.contains("nope"), + "{err}" + ); + + // A URL with no code at all, and an empty paste. + assert!( + parse_pasted_callback("http://localhost:19556/callback") + .unwrap_err() + .to_string() + .contains("no `code` parameter") + ); + assert!( + parse_pasted_callback(" ") + .unwrap_err() + .to_string() + .contains("no authorization code") + ); + } + + #[test] + fn is_remote_session_spots_ssh_without_a_forwarded_display() { + let env = |pairs: &'static [(&'static str, &'static str)]| { + move |key: &str| { + pairs + .iter() + .find(|(k, _)| *k == key) + .map(|(_, v)| v.to_string()) + } + }; + + // A plain SSH login: no browser here, and our loopback is the wrong host's. + assert!(is_remote_session(env(&[( + "SSH_CONNECTION", + "10.0.0.1 22 10.0.0.2 22" + )]))); + assert!(is_remote_session(env(&[("SSH_TTY", "/dev/pts/0")]))); + + // X11 forwarding: the browser draws on the user's screen and still reaches + // our loopback, so the session counts as local. + assert!(!is_remote_session(env(&[ + ("SSH_CONNECTION", "10.0.0.1 22 10.0.0.2 22"), + ("DISPLAY", "localhost:10.0"), + ]))); + + // The remote machine's *own* console is not an escape hatch — a browser on + // it opens in front of nobody. This is a Cloud Workstation over + // `gcloud workstations ssh`, whose image sets a virtual display. + assert!(is_remote_session(env(&[ + ("SSH_CONNECTION", "127.0.0.1 22 127.0.0.1 22"), + ("DISPLAY", ":0"), + ]))); + // Nor is Wayland, which ssh cannot forward at all. + assert!(is_remote_session(env(&[ + ("SSH_TTY", "/dev/pts/0"), + ("WAYLAND_DISPLAY", "wayland-0"), + ]))); + + // Not remote: a local terminal, and an empty variable is not a set one. + assert!(!is_remote_session(env(&[]))); + assert!(!is_remote_session(env(&[("SSH_CONNECTION", "")]))); + // A local desktop is not remote whatever its display looks like. + assert!(!is_remote_session(env(&[("DISPLAY", ":0")]))); + } + + #[test] + fn is_forwarded_display_needs_a_host_before_the_colon() { + assert!(is_forwarded_display(Some("localhost:10.0"))); // ssh -X + assert!(is_forwarded_display(Some("localhost:11"))); + assert!(!is_forwarded_display(Some(":0"))); // the local console + assert!(!is_forwarded_display(Some(":0.0"))); + assert!(!is_forwarded_display(None)); + assert!(!is_forwarded_display(Some(""))); // no colon at all + } + + #[test] + fn browser_launchers_prefers_the_user_s_browser_env() { + // $BROWSER is a colon-separated list of command lines; every entry is tried + // before the platform table, in the order the user wrote them. + let candidates = browser_launchers(Some("firefox %s:chromium")); + assert_eq!(candidates[0], vec!["firefox", "%s"]); + assert_eq!(candidates[1], vec!["chromium"]); + assert!(candidates.len() > 2, "the platform table follows"); + + // Empty entries (a trailing colon, or an unset-but-present variable) are + // dropped rather than spawning the empty program. + assert_eq!(browser_launchers(Some("")), browser_launchers(None)); + assert_eq!(browser_launchers(Some(":::")), browser_launchers(None)); + + // The platform table is never empty, so a login always has something to try. + assert!(!browser_launchers(None).is_empty()); + } } From 2ddcce616ddfcbe23b5aeb8336f4a95be756c4ac Mon Sep 17 00:00:00 2001 From: Cristian Falcas Date: Thu, 3 Sep 2026 20:00:44 +0100 Subject: [PATCH 2/3] fix(auth): harden remote browser login --- Cargo.lock | 1 + .../aspect-cli/src/builtins/aspect/auth.axl | 48 +--- crates/axl-runtime/BUILD.bazel | 1 + crates/axl-runtime/Cargo.toml | 1 + crates/axl-runtime/src/engine/aspect/auth.rs | 220 ++++++++---------- 5 files changed, 114 insertions(+), 157 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 86603ae89..c5be929bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -384,6 +384,7 @@ dependencies = [ "sha1", "sha2", "sha256", + "shell-words", "ssri", "starbuf-derive", "starlark", diff --git a/crates/aspect-cli/src/builtins/aspect/auth.axl b/crates/aspect-cli/src/builtins/aspect/auth.axl index 7296ed38f..8880605c3 100644 --- a/crates/aspect-cli/src/builtins/aspect/auth.axl +++ b/crates/aspect-cli/src/builtins/aspect/auth.axl @@ -3,20 +3,11 @@ load("./private/lib/deployment_flags.axl", "REMOTE_DEFAULT_CAPS", "capability_la load("./private/lib/environment.axl", "error", "info", "warn") load("./private/lib/prompt.axl", "prompt_choice") -def _open_login_url(ctx: TaskContext, session) -> bool: - """Get the user in front of the authorize URL, returning whether a browser was - opened *here* — which is what decides whether the OAuth redirect can reach the - loopback listener on its own. - - `--no-browser` skips the attempt outright. Otherwise the runtime walks this - platform's openers ($BROWSER, then xdg-open/gio/... on Linux, `open` on macOS, - rundll32 on Windows) and reports "headless" without trying at all when this is - an SSH session with no forwarded display.""" - status = "headless" if ctx.args.no_browser else ctx.aspect.auth.open_browser(session.url) - if status == "opened": +def _open_login_url(ctx: TaskContext, session: aspect.auth.AuthSession) -> bool: + """Open the authorization URL locally, returning whether launch succeeded.""" + opened = False if ctx.args.no_browser else ctx.aspect.auth.open_browser(session.url) + if opened: print("Browser opened. Waiting for authentication...") - # The escape hatch for the case detection cannot see: a remote session that - # looks local, where the browser just opened on a screen nobody is watching. print("(Nothing opened, or it opened elsewhere? Ctrl-C and re-run with --no-browser.)") return True print("Open this URL in a browser to log in:") @@ -25,18 +16,8 @@ def _open_login_url(ctx: TaskContext, session) -> bool: print("") return False -def _redeem_pasted_code(ctx: TaskContext, session): - """Take the authorization code back by hand and exchange it, or None if nothing - was entered. - - The path for a browser that is not on this machine — an SSH session, mostly. - The OAuth redirect resolves to a loopback address, which only *this* host - serves, so a browser on the user's laptop lands on a page that never loads and - the address bar is the only place the code can be read from. - - No port-forwarding hint here on purpose: a self-hosted deployment binds a fresh - port every run, so `ssh -L` is only durable advice for the Aspect account flow - (fixed port 19556). It lives in --no-browser's help text, with that caveat.""" +def _redeem_pasted_code(ctx: TaskContext, session: aspect.auth.AuthSession) -> aspect.auth.AuthCredentials | None: + """Exchange pasted callback input, returning None when it is empty.""" print("Authorizing sends the browser on to {}?code=...".format(session.callback_url)) print("— an address only this machine serves. If the browser is on another machine") print("it will report \"refused to connect\": that is expected, and the address bar") @@ -49,18 +30,13 @@ def _redeem_pasted_code(ctx: TaskContext, session): return None return session.redeem(pasted) -def _run_browser_login(ctx: TaskContext, deployment: str): - """Authenticate the user in a browser and return the minted credentials, or None - when an interactive login was needed and the user entered nothing. - - Happy path: open a browser here and wait for the OAuth callback on the loopback - listener. When there is no browser on this machine — or it lives at the other - end of an SSH session, where that loopback redirect resolves to the wrong host — - fall back to taking the code by hand. Off a TTY there is nothing to paste with, - so keep waiting on the listener instead: a forwarded port still delivers.""" +def _run_browser_login(ctx: TaskContext, deployment: str) -> aspect.auth.AuthCredentials | None: + """Authenticate through a local browser or pasted callback.""" session = ctx.aspect.auth.login(deployment = deployment) if _open_login_url(ctx, session): return session.wait() + if ctx.args.no_browser: + return _redeem_pasted_code(ctx, session) if not ctx.std.io.stdin.is_tty: print("Waiting for the callback on localhost:{}...".format(session.callback_port)) return session.wait() @@ -205,7 +181,7 @@ login = task( ), "no_browser": args.boolean( default = False, - description = "Don't try to open a browser; print the URL and ask for the authorization code back. Use this when the browser is on another machine (e.g. over SSH), where the login callback would be redirected to the wrong host's localhost. Over SSH you can instead forward the callback port from your local machine — for your Aspect account that is the fixed `ssh -L 19556:localhost:19556 `; a self-hosted deployment binds a fresh port each run, so paste the code.", + description = "Don't open a browser locally; print the URL and ask for the callback URL or authorization code. Use this when the browser is on another machine, such as over SSH.", ), "profile": args.string( description = "Advanced: the credential store key to save under (defaults to the deployment name, or \"default\" for the Aspect account). Unlike --deployment (which picks who to authenticate), --profile only changes where the credential is filed — use it to keep two identities for the same account/deployment side by side. Also settable via $ASPECT_AUTH_PROFILE.", @@ -316,7 +292,7 @@ configure = task( ), "no_browser": args.boolean( default = False, - description = "Don't try to open a browser; print the URL and ask for the authorization code back. Use this when the browser is on another machine (e.g. over SSH), where the login callback would be redirected to the wrong host's localhost. Over SSH you can instead forward the callback port from your local machine — for your Aspect account that is the fixed `ssh -L 19556:localhost:19556 `; a self-hosted deployment binds a fresh port each run, so paste the code.", + description = "Don't open a browser locally; print the URL and ask for the callback URL or authorization code. Use this when the browser is on another machine, such as over SSH.", ), }, ) diff --git a/crates/axl-runtime/BUILD.bazel b/crates/axl-runtime/BUILD.bazel index 9b17bbe56..895e02d0c 100644 --- a/crates/axl-runtime/BUILD.bazel +++ b/crates/axl-runtime/BUILD.bazel @@ -59,6 +59,7 @@ rust_library( "@crates//:sha1", "@crates//:sha2", "@crates//:sha256", + "@crates//:shell-words", "@crates//:ssri", "@crates//:starlark", "@crates//:starlark_map", diff --git a/crates/axl-runtime/Cargo.toml b/crates/axl-runtime/Cargo.toml index 24fba5144..038652554 100644 --- a/crates/axl-runtime/Cargo.toml +++ b/crates/axl-runtime/Cargo.toml @@ -43,6 +43,7 @@ wasmi_wasi = "0.51.0" clap = "4.5.47" getargs = "0.5.0" serde_json = { version = "1.0.145", features = ["preserve_order"] } +shell-words = "1.1.0" sha2 = "0.10" md5 = { package = "md-5", version = "0.10" } sha1 = "0.10" diff --git a/crates/axl-runtime/src/engine/aspect/auth.rs b/crates/axl-runtime/src/engine/aspect/auth.rs index 050efdee0..f2ceeacf0 100644 --- a/crates/axl-runtime/src/engine/aspect/auth.rs +++ b/crates/axl-runtime/src/engine/aspect/auth.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeMap, HashMap}; use std::fs; use std::path::PathBuf; use std::sync::{Mutex, OnceLock}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use allocative::Allocative; use derive_more::Display; @@ -1580,27 +1580,14 @@ fn build_authorize_url( url } -/// Whether `DISPLAY` names a display reached over the network rather than the -/// machine's own console. SSH's X11 forwarding sets a display with a host in it -/// (`localhost:10.0` — display numbers start at 10); the local console is a bare -/// `:0`. The distinction is the whole point on a remote workstation whose image -/// sets `DISPLAY=:0` for a virtual screen nobody is watching. +/// Whether `DISPLAY` names an X11 display through a host. fn is_forwarded_display(display: Option<&str>) -> bool { display .and_then(|display| display.split_once(':')) .is_some_and(|(host, _)| !host.is_empty()) } -/// Whether this session has no browser of its own — an SSH login (including the -/// tunnelled kind, e.g. `gcloud workstations ssh`) with no forwarded display. -/// Launching a browser there either fails noisily or, worse, succeeds on the remote -/// machine's own screen where nobody is looking; either way the loopback the -/// browser is redirected to belongs to the wrong host. -/// -/// Forwarded X11 is the one exception: a browser launched over it draws on the -/// user's own screen *and* still reaches our loopback, so that session counts as -/// local. Wayland has no equivalent forwarding, so a `WAYLAND_DISPLAY` under SSH -/// is the remote compositor and earns no exception. +/// Whether this is an SSH session without X11 forwarding. fn is_remote_session(get: impl Fn(&str) -> Option) -> bool { let set = |key: &str| get(key).is_some_and(|value| !value.is_empty()); if !(set("SSH_CONNECTION") || set("SSH_CLIENT") || set("SSH_TTY")) { @@ -1609,42 +1596,31 @@ fn is_remote_session(get: impl Fn(&str) -> Option) -> bool { !is_forwarded_display(get("DISPLAY").as_deref()) } -/// Browser-launcher argv lists to try for this platform, most preferred first. -/// -/// `$BROWSER` — the Unix convention: a colon-separated list of command lines, each -/// with an optional `%s` where the URL goes — wins when set, since a user who set -/// it has already answered the question. Behind it is the platform table every CLI -/// that does this carries. `wslview` leads the Linux list unconditionally: it -/// exists only under WSL (where `xdg-open` is present but opens nothing the user -/// can see), and elsewhere it simply fails to spawn and costs a syscall. -/// -/// Windows gets `rundll32` alone. It takes the URL as a single argument, whereas -/// `cmd /c start` hands it back to `cmd.exe` to re-parse — and an OAuth authorize -/// URL is nothing but `&`-separated parameters. `$BROWSER` is not a Windows -/// convention and splitting a drive-lettered path on `:` mangles it, but the -/// mangled program merely fails to spawn and the table behind it still runs. +/// Browser candidates, with `$BROWSER` entries before platform defaults. fn browser_launchers(browser_env: Option<&str>) -> Vec> { let mut candidates: Vec> = Vec::new(); for entry in browser_env.unwrap_or_default().split(':') { - let argv: Vec = entry.split_whitespace().map(str::to_string).collect(); - if !argv.is_empty() { + if let Ok(argv) = shell_words::split(entry) + && !argv.is_empty() + { candidates.push(argv); } } let table: &[&[&str]] = if cfg!(target_os = "macos") { &[&["open"]] } else if cfg!(target_os = "windows") { + // Avoid `cmd /c start`; it reparses `&` in OAuth URLs. &[&["rundll32.exe", "url.dll,FileProtocolHandler"]] } else { &[ + // WSL may provide `xdg-open` without a visible desktop. &["wslview"], &["xdg-open"], &["gio", "open"], &["gnome-open"], &["kde-open"], &["x-www-browser"], - // Debian's xdg-utils installs this as a symlink to xdg-open; on macOS - // it is the real thing. Harmless last resort elsewhere. + // Debian may install `open` as an `xdg-open` alias. &["open"], ] }; @@ -1656,13 +1632,28 @@ fn browser_launchers(browser_env: Option<&str>) -> Vec> { candidates } -/// Run one launcher argv with `url` substituted for a `%s` placeholder (the -/// `$BROWSER` convention) or appended when there is none, reporting whether it -/// succeeded. -/// -/// Output is discarded on purpose: a launcher that cannot find a browser says so -/// on stderr, and `xdg-open`'s cascade of "www-browser: not found" lines is exactly -/// what the caller replaces with one message that says what to do instead. +const LAUNCHER_PROBE_TIMEOUT: Duration = Duration::from_millis(500); + +fn launcher_started(mut child: std::process::Child, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + loop { + match child.try_wait() { + Ok(Some(status)) => return status.success(), + Ok(None) if Instant::now() >= deadline => { + let _ = std::thread::Builder::new() + .name("browser-launcher-reaper".to_string()) + .spawn(move || { + let _ = child.wait(); + }); + return true; + } + Ok(None) => std::thread::sleep(Duration::from_millis(10)), + Err(_) => return false, + } + } +} + +/// Run a launcher after substituting `%s`, or append the URL when absent. fn spawn_launcher(argv: &[String], url: &str) -> bool { let Some((program, rest)) = argv.split_first() else { return false; @@ -1684,33 +1675,24 @@ fn spawn_launcher(argv: &[String], url: &str) -> bool { .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()); - // A missing launcher fails at spawn; one that ran but found no browser to - // launch (xdg-open on a machine with no desktop) reports it in the exit status. - matches!(command.status(), Ok(status) if status.success()) + let Ok(child) = command.spawn() else { + return false; + }; + launcher_started(child, LAUNCHER_PROBE_TIMEOUT) } -/// Try to open `url` in a browser here, reporting to the task which of the three -/// situations it is in: -/// -/// - `"opened"` — a browser was launched, so the OAuth redirect reaches this -/// machine's loopback listener on its own and the task can just wait. -/// - `"headless"` — not attempted, because this is an SSH session with no -/// forwarded display. -/// - `"no_browser"` — attempted, and no launcher on this platform worked. -/// -/// The latter two both mean the user opens the URL somewhere else, where the -/// loopback redirect cannot reach us and the code has to come back by hand. -fn open_url_in_browser(url: &str) -> String { +/// Try to open `url` in a browser on this machine. +fn open_url_in_browser(url: &str) -> bool { if is_remote_session(|key| std::env::var(key).ok()) { - return "headless".to_string(); + return false; } let browser_env = std::env::var("BROWSER").ok(); for argv in browser_launchers(browser_env.as_deref()) { if spawn_launcher(&argv, url) { - return "opened".to_string(); + return true; } } - "no_browser".to_string() + false } fn build_cloud_session(env: AuthEnv) -> anyhow::Result { @@ -1737,8 +1719,6 @@ fn build_cloud_session(env: AuthEnv) -> anyhow::Result { Ok(AuthSession { url: authorize_url, callback_port: port, - // The IdP redirects straight here, so the registered redirect is verbatim - // where the browser lands. callback_url: redirect_uri.clone(), inner: Mutex::new(Some(AuthSessionInner { listener: Some(listener), @@ -1781,11 +1761,7 @@ fn build_endpoint_session(env: AuthEnv, host: &str) -> anyhow::Result, require_state: bool, ) -> anyhow::Result { - // The Aspect-cloud flow posts to the conventional token endpoint; the - // self-hosted flow resolves it via OIDC discovery and first validates - // the callback `state` (CSRF guard). let token_url = match &self.kind { SessionKind::Cloud => format!("{}/oauth/token", self.env.domain), SessionKind::Endpoint { expected_state } => { - let matches = match state.as_deref() { - Some(state) => state == expected_state.as_str(), - None => !require_state, - }; - if !matches { + if !callback_state_matches(expected_state, state.as_deref(), require_state) { return Err(anyhow::anyhow!( "authentication failed: callback state did not match" )); @@ -2149,8 +2111,11 @@ impl AuthSessionInner { } } -/// Take the pending login out of `session`. `wait()` and `redeem()` are two ways -/// to finish the same session, so either one consumes it. +fn callback_state_matches(expected: &str, actual: Option<&str>, require_state: bool) -> bool { + actual.map_or(!require_state, |actual| actual == expected) +} + +/// Consume a pending session so only one completion path can run. fn take_pending(session: &AuthSession) -> anyhow::Result { let mut guard = session .inner @@ -2161,18 +2126,13 @@ fn take_pending(session: &AuthSession) -> anyhow::Result { .ok_or_else(|| anyhow::anyhow!("auth session already consumed")) } -/// Extract the OAuth `code` (and `state`, when it came along) from whatever the -/// user pasted back: the browser's full address-bar URL, a bare query string, or -/// the code on its own. -/// -/// The `code=`/`error=` test — rather than merely looking for a `?` or `=` — keeps -/// a bare authorization code that happens to carry base64 padding from being read -/// as a query string and rejected for having no `code` parameter. +/// Parse a callback URL, query string, or bare authorization code. fn parse_pasted_callback(pasted: &str) -> anyhow::Result<(String, Option)> { let pasted = pasted.trim(); if pasted.is_empty() { return Err(anyhow::anyhow!("no authorization code entered")); } + // `=` alone does not imply a query string: bare codes may contain padding. let is_callback = pasted.starts_with("http://") || pasted.starts_with("https://") || pasted.contains("code=") @@ -2203,13 +2163,9 @@ fn parse_pasted_callback(pasted: &str) -> anyhow::Result<(String, Option #[display("")] pub struct AuthSession { pub url: String, - /// The loopback port the OAuth callback ultimately lands on. Exposed so the - /// task can name it in the `ssh -L` hint. + /// Loopback port reported while waiting for a forwarded callback. pub callback_port: u16, - /// The loopback address the browser is left sitting on once the flow is done — - /// what the user reads the authorization code out of when the browser is on - /// another machine. Exposed so the task can quote it exactly rather than - /// approximately: the two flows do not agree on the spelling. + /// Loopback URL displayed by the manual callback flow. pub callback_url: String, #[allocative(skip)] inner: Mutex>, @@ -2240,9 +2196,6 @@ fn auth_session_methods(registry: &mut MethodsBuilder) { attr_str!(this, AuthSession, url) } - /// The loopback port the OAuth redirect ends up on. Only this machine can - /// serve it, so a task that could not open a browser here names it in the - /// port-forwarding hint. #[starlark(attribute)] fn callback_port<'v>(this: values::Value<'v>) -> anyhow::Result { Ok(this @@ -2251,18 +2204,12 @@ fn auth_session_methods(registry: &mut MethodsBuilder) { .callback_port as i32) } - /// The loopback address the browser is left on when the flow finishes — where - /// the user reads the code out of the address bar. The cloud flow lands on - /// `http://localhost:/callback`; a self-hosted deployment's callback page - /// forwards to `http://127.0.0.1:/callback`. #[starlark(attribute)] fn callback_url<'v>(this: values::Value<'v>) -> anyhow::Result { attr_str!(this, AuthSession, callback_url) } - /// Wait for the browser to deliver the authorization code to the loopback - /// listener, then exchange it. Requires the browser to be on this machine (or - /// the port to be forwarded here); see `redeem` for the other case. + /// Exchange an authorization code received by the loopback listener. fn wait<'v>(this: values::Value<'v>) -> anyhow::Result { let session = this .downcast_ref_err::() @@ -2279,13 +2226,7 @@ fn auth_session_methods(registry: &mut MethodsBuilder) { Ok(AuthCredentials::from_entry(&entry)) } - /// Exchange an authorization code the user pasted back — the browser's full - /// callback URL, a bare query string, or the bare code. - /// - /// This is the login path for a browser that is not on this machine (an SSH - /// session, most often): the redirect resolves to *our* loopback, so the code - /// never reaches us and the browser's address bar is the only place left to - /// read it from. Drops the listener, since nothing will connect to it. + /// Exchange a pasted callback URL, query string, or bare code. fn redeem<'v>( this: values::Value<'v>, #[starlark(require = pos)] pasted: &str, @@ -2733,14 +2674,11 @@ fn auth_methods(registry: &mut MethodsBuilder) { Ok(resolve_api_url()?) } - /// Try to open `url` in a browser on this machine. Returns `"opened"`, - /// `"headless"` (an SSH session with no forwarded display — not attempted), or - /// `"no_browser"` (no launcher on this platform worked). The last two tell the - /// caller the OAuth loopback redirect will land on the wrong machine. + /// Try to open `url` in a browser on this machine. fn open_browser<'v>( #[allow(unused)] this: values::Value<'v>, #[starlark(require = pos)] url: &str, - ) -> anyhow::Result { + ) -> anyhow::Result { Ok(open_url_in_browser(url)) } @@ -4484,6 +4422,15 @@ mod tests { ); } + #[test] + fn callback_state_policy_requires_listener_state_and_checks_pasted_state() { + assert!(callback_state_matches("expected", Some("expected"), true)); + assert!(!callback_state_matches("expected", Some("wrong"), true)); + assert!(!callback_state_matches("expected", None, true)); + assert!(callback_state_matches("expected", None, false)); + assert!(!callback_state_matches("expected", Some("wrong"), false)); + } + #[test] fn is_remote_session_spots_ssh_without_a_forwarded_display() { let env = |pairs: &'static [(&'static str, &'static str)]| { @@ -4556,4 +4503,35 @@ mod tests { // The platform table is never empty, so a login always has something to try. assert!(!browser_launchers(None).is_empty()); } + + #[test] + fn browser_launchers_preserves_quoted_arguments() { + let candidates = + browser_launchers(Some("firefox --new-window \"%s\":sh -c 'open-browser %s'")); + assert_eq!(candidates[0], vec!["firefox", "--new-window", "%s"]); + assert_eq!(candidates[1], vec!["sh", "-c", "open-browser %s"]); + } + + #[test] + fn launcher_probe_does_not_wait_for_a_long_running_child() { + let child = std::process::Command::new(std::env::current_exe().unwrap()) + .arg("launcher_probe_test_child") + .env("ASPECT_LAUNCHER_TEST_CHILD", "sleep") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .unwrap(); + + let started = Instant::now(); + assert!(launcher_started(child, Duration::from_millis(50))); + assert!(started.elapsed() < Duration::from_millis(300)); + } + + #[test] + fn launcher_probe_test_child() { + if std::env::var("ASPECT_LAUNCHER_TEST_CHILD").as_deref() == Ok("sleep") { + std::thread::sleep(Duration::from_millis(500)); + } + } } From 7812c7ec411f9cf186b2286de88aab602792e97e Mon Sep 17 00:00:00 2001 From: Cristian Falcas Date: Thu, 3 Sep 2026 20:41:07 +0100 Subject: [PATCH 3/3] refactor(auth): simplify remote browser login --- Cargo.lock | 1 - .../aspect-cli/src/builtins/aspect/auth.axl | 37 +- crates/axl-runtime/BUILD.bazel | 1 - crates/axl-runtime/Cargo.toml | 1 - crates/axl-runtime/src/engine/aspect/auth.rs | 412 +++++++++--------- 5 files changed, 208 insertions(+), 244 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c5be929bf..86603ae89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -384,7 +384,6 @@ dependencies = [ "sha1", "sha2", "sha256", - "shell-words", "ssri", "starbuf-derive", "starlark", diff --git a/crates/aspect-cli/src/builtins/aspect/auth.axl b/crates/aspect-cli/src/builtins/aspect/auth.axl index 8880605c3..90421e762 100644 --- a/crates/aspect-cli/src/builtins/aspect/auth.axl +++ b/crates/aspect-cli/src/builtins/aspect/auth.axl @@ -3,44 +3,31 @@ load("./private/lib/deployment_flags.axl", "REMOTE_DEFAULT_CAPS", "capability_la load("./private/lib/environment.axl", "error", "info", "warn") load("./private/lib/prompt.axl", "prompt_choice") -def _open_login_url(ctx: TaskContext, session: aspect.auth.AuthSession) -> bool: - """Open the authorization URL locally, returning whether launch succeeded.""" - opened = False if ctx.args.no_browser else ctx.aspect.auth.open_browser(session.url) +def _run_browser_login(ctx: TaskContext, deployment: str) -> aspect.auth.AuthCredentials | None: + session = ctx.aspect.auth.login(deployment = deployment) + opened = not ctx.args.no_browser and session.open_browser() if opened: print("Browser opened. Waiting for authentication...") print("(Nothing opened, or it opened elsewhere? Ctrl-C and re-run with --no-browser.)") - return True + return session.finish() + print("Open this URL in a browser to log in:") print("") print(" " + session.url) print("") - return False - -def _redeem_pasted_code(ctx: TaskContext, session: aspect.auth.AuthSession) -> aspect.auth.AuthCredentials | None: - """Exchange pasted callback input, returning None when it is empty.""" - print("Authorizing sends the browser on to {}?code=...".format(session.callback_url)) - print("— an address only this machine serves. If the browser is on another machine") - print("it will report \"refused to connect\": that is expected, and the address bar") - print("still holds the code. Copy the whole address and paste it below.") + if not ctx.args.no_browser and not ctx.std.io.stdin.is_tty: + print("Waiting for the callback on localhost...") + return session.finish() + + print("If the browser is on another machine, its localhost callback will fail.") + print("Copy the callback URL from the address bar and paste it below.") print("") ctx.std.io.stdout.write("Paste the callback URL (or just the code): ") ctx.std.io.stdout.flush() pasted = str(ctx.std.io.stdin.read(8192)).strip() if not pasted: return None - return session.redeem(pasted) - -def _run_browser_login(ctx: TaskContext, deployment: str) -> aspect.auth.AuthCredentials | None: - """Authenticate through a local browser or pasted callback.""" - session = ctx.aspect.auth.login(deployment = deployment) - if _open_login_url(ctx, session): - return session.wait() - if ctx.args.no_browser: - return _redeem_pasted_code(ctx, session) - if not ctx.std.io.stdin.is_tty: - print("Waiting for the callback on localhost:{}...".format(session.callback_port)) - return session.wait() - return _redeem_pasted_code(ctx, session) + return session.finish(pasted) # Width of the label column in a deployment's detail block so values align. Must # be wider than the longest label ("Results", 7) to leave a gap; a label >= this diff --git a/crates/axl-runtime/BUILD.bazel b/crates/axl-runtime/BUILD.bazel index 895e02d0c..9b17bbe56 100644 --- a/crates/axl-runtime/BUILD.bazel +++ b/crates/axl-runtime/BUILD.bazel @@ -59,7 +59,6 @@ rust_library( "@crates//:sha1", "@crates//:sha2", "@crates//:sha256", - "@crates//:shell-words", "@crates//:ssri", "@crates//:starlark", "@crates//:starlark_map", diff --git a/crates/axl-runtime/Cargo.toml b/crates/axl-runtime/Cargo.toml index 038652554..24fba5144 100644 --- a/crates/axl-runtime/Cargo.toml +++ b/crates/axl-runtime/Cargo.toml @@ -43,7 +43,6 @@ wasmi_wasi = "0.51.0" clap = "4.5.47" getargs = "0.5.0" serde_json = { version = "1.0.145", features = ["preserve_order"] } -shell-words = "1.1.0" sha2 = "0.10" md5 = { package = "md-5", version = "0.10" } sha1 = "0.10" diff --git a/crates/axl-runtime/src/engine/aspect/auth.rs b/crates/axl-runtime/src/engine/aspect/auth.rs index f2ceeacf0..8625b5cfe 100644 --- a/crates/axl-runtime/src/engine/aspect/auth.rs +++ b/crates/axl-runtime/src/engine/aspect/auth.rs @@ -1147,6 +1147,20 @@ fn extract_query_param(path: &str, key: &str) -> Option { None } +fn parse_callback_response(path: &str) -> anyhow::Result<(String, Option)> { + if let Some(error) = extract_query_param(path, "error") { + let description = extract_query_param(path, "error_description").unwrap_or_default(); + return Err(anyhow::anyhow!( + "authentication failed: {} {}", + error, + description + )); + } + let code = extract_query_param(path, "code") + .ok_or_else(|| anyhow::anyhow!("OAuth callback has no `code` parameter"))?; + Ok((code, extract_query_param(path, "state"))) +} + pub(crate) fn block_on(fut: F) -> F::Output { Handle::current().block_on(fut) } @@ -1312,12 +1326,7 @@ async fn accept_callback(listener: TcpListener) -> anyhow::Result<(String, Optio .split_whitespace() .nth(1) .ok_or_else(|| anyhow::anyhow!("malformed HTTP request line"))?; - let code = extract_query_param(path, "code").ok_or_else(|| { - let error = extract_query_param(path, "error").unwrap_or_else(|| "unknown".to_string()); - let desc = extract_query_param(path, "error_description").unwrap_or_default(); - anyhow::anyhow!("authentication failed: {} {}", error, desc) - })?; - let state = extract_query_param(path, "state"); + let callback = parse_callback_response(path)?; let html = r##" @@ -1379,7 +1388,7 @@ async fn accept_callback(listener: TcpListener) -> anyhow::Result<(String, Optio .write_all(response.as_bytes()) .await .map_err(|e| anyhow::anyhow!("failed to write OAuth response: {}", e))?; - Ok((code, state)) + Ok(callback) } /// An OAuth token response. The bearer the CLI attaches to endpoints is the @@ -1580,33 +1589,29 @@ fn build_authorize_url( url } -/// Whether `DISPLAY` names an X11 display through a host. -fn is_forwarded_display(display: Option<&str>) -> bool { - display - .and_then(|display| display.split_once(':')) - .is_some_and(|(host, _)| !host.is_empty()) -} - -/// Whether this is an SSH session without X11 forwarding. fn is_remote_session(get: impl Fn(&str) -> Option) -> bool { let set = |key: &str| get(key).is_some_and(|value| !value.is_empty()); if !(set("SSH_CONNECTION") || set("SSH_CLIENT") || set("SSH_TTY")) { return false; } - !is_forwarded_display(get("DISPLAY").as_deref()) + let forwarded_display = get("DISPLAY").is_some_and(|display| { + let Some((host, display)) = display.rsplit_once(':') else { + return false; + }; + let Some(number) = display + .split('.') + .next() + .and_then(|number| number.parse::().ok()) + else { + return false; + }; + matches!(host, "localhost" | "127.0.0.1" | "[::1]") && number >= 10 + }); + !forwarded_display } -/// Browser candidates, with `$BROWSER` entries before platform defaults. -fn browser_launchers(browser_env: Option<&str>) -> Vec> { - let mut candidates: Vec> = Vec::new(); - for entry in browser_env.unwrap_or_default().split(':') { - if let Ok(argv) = shell_words::split(entry) - && !argv.is_empty() - { - candidates.push(argv); - } - } - let table: &[&[&str]] = if cfg!(target_os = "macos") { +fn browser_launchers() -> &'static [&'static [&'static str]] { + if cfg!(target_os = "macos") { &[&["open"]] } else if cfg!(target_os = "windows") { // Avoid `cmd /c start`; it reparses `&` in OAuth URLs. @@ -1623,13 +1628,7 @@ fn browser_launchers(browser_env: Option<&str>) -> Vec> { // Debian may install `open` as an `xdg-open` alias. &["open"], ] - }; - candidates.extend( - table - .iter() - .map(|argv| argv.iter().map(|arg| arg.to_string()).collect()), - ); - candidates + } } const LAUNCHER_PROBE_TIMEOUT: Duration = Duration::from_millis(500); @@ -1653,25 +1652,14 @@ fn launcher_started(mut child: std::process::Child, timeout: Duration) -> bool { } } -/// Run a launcher after substituting `%s`, or append the URL when absent. -fn spawn_launcher(argv: &[String], url: &str) -> bool { +fn spawn_launcher(argv: &[&str], url: &str) -> bool { let Some((program, rest)) = argv.split_first() else { return false; }; let mut command = std::process::Command::new(program); - let mut substituted = false; - for arg in rest { - if arg.contains("%s") { - command.arg(arg.replace("%s", url)); - substituted = true; - } else { - command.arg(arg); - } - } - if !substituted { - command.arg(url); - } command + .args(rest) + .arg(url) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()); @@ -1681,18 +1669,15 @@ fn spawn_launcher(argv: &[String], url: &str) -> bool { launcher_started(child, LAUNCHER_PROBE_TIMEOUT) } -/// Try to open `url` in a browser on this machine. +fn launch_first(candidates: &[&[&str]], url: &str) -> bool { + candidates.iter().any(|argv| spawn_launcher(argv, url)) +} + fn open_url_in_browser(url: &str) -> bool { if is_remote_session(|key| std::env::var(key).ok()) { return false; } - let browser_env = std::env::var("BROWSER").ok(); - for argv in browser_launchers(browser_env.as_deref()) { - if spawn_launcher(&argv, url) { - return true; - } - } - false + launch_first(browser_launchers(), url) } fn build_cloud_session(env: AuthEnv) -> anyhow::Result { @@ -1718,8 +1703,6 @@ fn build_cloud_session(env: AuthEnv) -> anyhow::Result { ); Ok(AuthSession { url: authorize_url, - callback_port: port, - callback_url: redirect_uri.clone(), inner: Mutex::new(Some(AuthSessionInner { listener: Some(listener), code_verifier, @@ -1760,9 +1743,6 @@ fn build_endpoint_session(env: AuthEnv, host: &str) -> anyhow::Result, require_state: b actual.map_or(!require_state, |actual| actual == expected) } -/// Consume a pending session so only one completion path can run. -fn take_pending(session: &AuthSession) -> anyhow::Result { - let mut guard = session - .inner - .lock() - .map_err(|_| anyhow::anyhow!("auth session already consumed or poisoned"))?; - guard - .take() - .ok_or_else(|| anyhow::anyhow!("auth session already consumed")) -} - -/// Parse a callback URL, query string, or bare authorization code. fn parse_pasted_callback(pasted: &str) -> anyhow::Result<(String, Option)> { let pasted = pasted.trim(); if pasted.is_empty() { return Err(anyhow::anyhow!("no authorization code entered")); } - // `=` alone does not imply a query string: bare codes may contain padding. - let is_callback = pasted.starts_with("http://") - || pasted.starts_with("https://") - || pasted.contains("code=") - || pasted.contains("error="); - if !is_callback { - return Ok((pasted.to_string(), None)); - } - // `extract_query_param` splits on `?`; give a bare query string one to find. - let url = if pasted.contains('?') { - pasted.to_string() + if pasted.starts_with("http://") || pasted.starts_with("https://") { + parse_callback_response(pasted) } else { - format!("?{pasted}") - }; - if let Some(error) = extract_query_param(&url, "error") { - let desc = extract_query_param(&url, "error_description").unwrap_or_default(); - return Err(anyhow::anyhow!("authentication failed: {} {}", error, desc)); + Ok((pasted.to_string(), None)) } - let code = extract_query_param(&url, "code").ok_or_else(|| { - anyhow::anyhow!( - "no `code` parameter in what was pasted — after authorizing, copy the \ - browser's whole address, including everything after the `?`" - ) - })?; - Ok((code, extract_query_param(&url, "state"))) +} + +fn finish_auth_session( + session: &AuthSession, + pasted: Option<&str>, +) -> anyhow::Result { + let mut guard = session + .inner + .lock() + .map_err(|_| anyhow::anyhow!("auth session already consumed or poisoned"))?; + let mut inner = guard + .take() + .ok_or_else(|| anyhow::anyhow!("auth session already consumed"))?; + drop(guard); + let entry = if let Some(pasted) = pasted { + let (code, state) = parse_pasted_callback(pasted)?; + block_on(inner.complete(code, state, false))? + } else { + let listener = inner + .listener + .take() + .ok_or_else(|| anyhow::anyhow!("no listener in auth session"))?; + block_on(async move { + let (code, state) = accept_callback(listener).await?; + inner.complete(code, state, true).await + })? + }; + Ok(AuthCredentials::from_entry(&entry)) } #[derive(Display, ProvidesStaticType, NoSerialize, Allocative)] #[display("")] pub struct AuthSession { pub url: String, - /// Loopback port reported while waiting for a forwarded callback. - pub callback_port: u16, - /// Loopback URL displayed by the manual callback flow. - pub callback_url: String, #[allocative(skip)] inner: Mutex>, } @@ -2196,48 +2167,22 @@ fn auth_session_methods(registry: &mut MethodsBuilder) { attr_str!(this, AuthSession, url) } - #[starlark(attribute)] - fn callback_port<'v>(this: values::Value<'v>) -> anyhow::Result { - Ok(this - .downcast_ref_err::() - .into_anyhow_result()? - .callback_port as i32) - } - - #[starlark(attribute)] - fn callback_url<'v>(this: values::Value<'v>) -> anyhow::Result { - attr_str!(this, AuthSession, callback_url) - } - - /// Exchange an authorization code received by the loopback listener. - fn wait<'v>(this: values::Value<'v>) -> anyhow::Result { + fn open_browser<'v>(this: values::Value<'v>) -> anyhow::Result { let session = this .downcast_ref_err::() .into_anyhow_result()?; - let mut inner = take_pending(&session)?; - let listener = inner - .listener - .take() - .ok_or_else(|| anyhow::anyhow!("no listener in auth session"))?; - let entry = block_on(async move { - let (code, state) = accept_callback(listener).await?; - inner.complete(code, state, true).await - })?; - Ok(AuthCredentials::from_entry(&entry)) + Ok(open_url_in_browser(&session.url)) } - /// Exchange a pasted callback URL, query string, or bare code. - fn redeem<'v>( + /// Complete from pasted input when supplied; otherwise wait on the listener. + fn finish<'v>( this: values::Value<'v>, - #[starlark(require = pos)] pasted: &str, + #[starlark(require = pos)] pasted: Option<&str>, ) -> anyhow::Result { let session = this .downcast_ref_err::() .into_anyhow_result()?; - let inner = take_pending(&session)?; - let (code, state) = parse_pasted_callback(pasted)?; - let entry = block_on(inner.complete(code, state, false))?; - Ok(AuthCredentials::from_entry(&entry)) + finish_auth_session(session, pasted) } } @@ -2674,14 +2619,6 @@ fn auth_methods(registry: &mut MethodsBuilder) { Ok(resolve_api_url()?) } - /// Try to open `url` in a browser on this machine. - fn open_browser<'v>( - #[allow(unused)] this: values::Value<'v>, - #[starlark(require = pos)] url: &str, - ) -> anyhow::Result { - Ok(open_url_in_browser(url)) - } - fn login<'v>( #[allow(unused)] this: values::Value<'v>, #[starlark(require = named)] token: Option<&str>, @@ -4364,35 +4301,26 @@ mod tests { } #[test] - fn parse_pasted_callback_reads_a_url_a_query_or_a_bare_code() { - // What the user actually copies out of the address bar. - assert_eq!( - parse_pasted_callback( - " http://localhost:19556/callback?code=abc123&state=nonce.19556 " - ) - .unwrap(), - ("abc123".to_string(), Some("nonce.19556".to_string())) - ); - // A bare query string is given a `?` so the same extractor reads it. - assert_eq!( - parse_pasted_callback("code=abc123&state=xyz").unwrap(), - ("abc123".to_string(), Some("xyz".to_string())) - ); - // Just the code, with no state to validate. - assert_eq!( - parse_pasted_callback("abc123").unwrap(), - ("abc123".to_string(), None) - ); - // Base64 padding in a bare code must not make it look like a query string — - // it would otherwise be rejected for having no `code` parameter. - assert_eq!( - parse_pasted_callback("YWJjMTIz==").unwrap(), - ("YWJjMTIz==".to_string(), None) - ); - // A url-encoded value survives the round trip. + fn parse_pasted_callback_reads_a_url_or_an_opaque_code() { + for (input, code, state) in [ + ( + " http://localhost:19556/callback?code=abc123&state=nonce.19556 ", + "abc123", + Some("nonce.19556"), + ), + ("abc123", "abc123", None), + ("YWJjMTIz==", "YWJjMTIz==", None), + ("opaque-code=value", "opaque-code=value", None), + ("error=opaque", "error=opaque", None), + ] { + assert_eq!( + parse_pasted_callback(input).unwrap(), + (code.to_string(), state.map(str::to_string)) + ); + } assert_eq!( parse_pasted_callback("https://x/cb?code=a%2Fb").unwrap().0, - "a/b".to_string() + "a/b" ); // An authorize error the browser was redirected with is reported as one, @@ -4407,7 +4335,6 @@ mod tests { "{err}" ); - // A URL with no code at all, and an empty paste. assert!( parse_pasted_callback("http://localhost:19556/callback") .unwrap_err() @@ -4431,6 +4358,72 @@ mod tests { assert!(!callback_state_matches("expected", Some("wrong"), false)); } + #[test] + fn finish_auth_session_exchanges_a_pasted_code_once() { + use std::io::{Read, Write}; + + let server = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let address = server.local_addr().unwrap(); + let token = jwt_with_exp(None); + let response_token = token.clone(); + let responder = std::thread::spawn(move || { + let (mut stream, _) = server.accept().unwrap(); + let mut request = Vec::new(); + loop { + let mut chunk = [0; 1024]; + let read = stream.read(&mut chunk).unwrap(); + request.extend_from_slice(&chunk[..read]); + if read == 0 || String::from_utf8_lossy(&request).contains("code_verifier=verifier") + { + break; + } + } + let request = String::from_utf8(request).unwrap(); + assert!(request.starts_with("POST /oauth/token HTTP/1.1")); + assert!(request.contains("code=auth-code")); + assert!(request.contains("redirect_uri=http%3A%2F%2Flocalhost%2Fcallback")); + let body = + format!(r#"{{"access_token":"{response_token}","refresh_token":"refresh"}}"#); + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ) + .unwrap(); + }); + let session = AuthSession { + url: "https://auth.example/authorize".to_string(), + inner: Mutex::new(Some(AuthSessionInner { + listener: None, + code_verifier: "verifier".to_string(), + redirect_uri: "http://localhost/callback".to_string(), + env: AuthEnv { + domain: format!("http://{address}"), + client_id: "client".to_string(), + scopes: Vec::new(), + authorize_params: BTreeMap::new(), + }, + kind: SessionKind::Cloud, + })), + }; + let runtime = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .unwrap(); + let _runtime = runtime.enter(); + + let credentials = finish_auth_session(&session, Some("auth-code")).unwrap(); + assert_eq!(credentials.access_token, token); + responder.join().unwrap(); + assert!( + finish_auth_session(&session, Some("auth-code")) + .unwrap_err() + .to_string() + .contains("already consumed") + ); + } + #[test] fn is_remote_session_spots_ssh_without_a_forwarded_display() { let env = |pairs: &'static [(&'static str, &'static str)]| { @@ -4442,80 +4435,61 @@ mod tests { } }; - // A plain SSH login: no browser here, and our loopback is the wrong host's. assert!(is_remote_session(env(&[( "SSH_CONNECTION", "10.0.0.1 22 10.0.0.2 22" )]))); assert!(is_remote_session(env(&[("SSH_TTY", "/dev/pts/0")]))); - // X11 forwarding: the browser draws on the user's screen and still reaches - // our loopback, so the session counts as local. assert!(!is_remote_session(env(&[ ("SSH_CONNECTION", "10.0.0.1 22 10.0.0.2 22"), ("DISPLAY", "localhost:10.0"), ]))); + assert!(!is_remote_session(env(&[ + ("SSH_CONNECTION", "10.0.0.1 22 10.0.0.2 22"), + ("DISPLAY", "127.0.0.1:11"), + ]))); - // The remote machine's *own* console is not an escape hatch — a browser on - // it opens in front of nobody. This is a Cloud Workstation over - // `gcloud workstations ssh`, whose image sets a virtual display. + // A display on the remote host is not SSH-forwarded, even with a hostname. assert!(is_remote_session(env(&[ ("SSH_CONNECTION", "127.0.0.1 22 127.0.0.1 22"), ("DISPLAY", ":0"), ]))); - // Nor is Wayland, which ssh cannot forward at all. + assert!(is_remote_session(env(&[ + ("SSH_CONNECTION", "127.0.0.1 22 127.0.0.1 22"), + ("DISPLAY", "unix:0"), + ]))); + assert!(is_remote_session(env(&[ + ("SSH_CONNECTION", "127.0.0.1 22 127.0.0.1 22"), + ("DISPLAY", "localhost:0"), + ]))); + assert!(is_remote_session(env(&[ + ("SSH_CONNECTION", "127.0.0.1 22 127.0.0.1 22"), + ("DISPLAY", "workstation:10.0"), + ]))); assert!(is_remote_session(env(&[ ("SSH_TTY", "/dev/pts/0"), ("WAYLAND_DISPLAY", "wayland-0"), ]))); - // Not remote: a local terminal, and an empty variable is not a set one. assert!(!is_remote_session(env(&[]))); assert!(!is_remote_session(env(&[("SSH_CONNECTION", "")]))); - // A local desktop is not remote whatever its display looks like. assert!(!is_remote_session(env(&[("DISPLAY", ":0")]))); } #[test] - fn is_forwarded_display_needs_a_host_before_the_colon() { - assert!(is_forwarded_display(Some("localhost:10.0"))); // ssh -X - assert!(is_forwarded_display(Some("localhost:11"))); - assert!(!is_forwarded_display(Some(":0"))); // the local console - assert!(!is_forwarded_display(Some(":0.0"))); - assert!(!is_forwarded_display(None)); - assert!(!is_forwarded_display(Some(""))); // no colon at all - } - - #[test] - fn browser_launchers_prefers_the_user_s_browser_env() { - // $BROWSER is a colon-separated list of command lines; every entry is tried - // before the platform table, in the order the user wrote them. - let candidates = browser_launchers(Some("firefox %s:chromium")); - assert_eq!(candidates[0], vec!["firefox", "%s"]); - assert_eq!(candidates[1], vec!["chromium"]); - assert!(candidates.len() > 2, "the platform table follows"); - - // Empty entries (a trailing colon, or an unset-but-present variable) are - // dropped rather than spawning the empty program. - assert_eq!(browser_launchers(Some("")), browser_launchers(None)); - assert_eq!(browser_launchers(Some(":::")), browser_launchers(None)); - - // The platform table is never empty, so a login always has something to try. - assert!(!browser_launchers(None).is_empty()); - } - - #[test] - fn browser_launchers_preserves_quoted_arguments() { - let candidates = - browser_launchers(Some("firefox --new-window \"%s\":sh -c 'open-browser %s'")); - assert_eq!(candidates[0], vec!["firefox", "--new-window", "%s"]); - assert_eq!(candidates[1], vec!["sh", "-c", "open-browser %s"]); + fn browser_launchers_has_a_platform_default() { + assert!(!browser_launchers().is_empty()); } #[test] fn launcher_probe_does_not_wait_for_a_long_running_child() { + if std::env::var("ASPECT_LAUNCHER_TEST_CHILD").as_deref() == Ok("sleep") { + std::thread::sleep(Duration::from_millis(500)); + return; + } let child = std::process::Command::new(std::env::current_exe().unwrap()) - .arg("launcher_probe_test_child") + .arg("launcher_probe_does_not_wait_for_a_long_running_child") .env("ASPECT_LAUNCHER_TEST_CHILD", "sleep") .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) @@ -4528,10 +4502,16 @@ mod tests { assert!(started.elapsed() < Duration::from_millis(300)); } + #[cfg(unix)] #[test] - fn launcher_probe_test_child() { - if std::env::var("ASPECT_LAUNCHER_TEST_CHILD").as_deref() == Ok("sleep") { - std::thread::sleep(Duration::from_millis(500)); - } + fn launcher_failures_fall_through() { + assert!(launch_first( + &[&["false"], &["true"]], + "https://example.com" + )); + assert!(!launch_first( + &[&["false"], &["aspect-no-such-browser-launcher"]], + "https://example.com" + )); } }