From 439414cc2b0f74ddd7cd69973063380485b1bd63 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Fri, 11 Sep 2026 17:39:28 +0200 Subject: [PATCH] feat(cli): name the machine on the approval page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The approval page defaulted every token to "Mergify CLI", the client name every machine shares, on a page whose own helper text asks the user to name it after the machine. `auth login` now sends the hostname as the `device_name` field of the grant request, so the default reads "Mergify CLI on work-laptop" and a user with a laptop, a desktop and a devbox can tell three tokens apart on the CLI Tokens page. The bare hostname, not a composed label: the server composes ` on ` itself, so sending "Mergify CLI on host" would render "Mergify CLI on Mergify CLI on host". Nothing is trimmed, lower-cased, or cut to length on this side. The server sanitizes the value — printable ASCII, collapsed whitespace, 60 characters — and caps the label it composes, and a client that pre-trimmed would only disagree with what the page then shows. The field is optional and the machine may have no answer, so `machine::name()` returns an `Option` and the field is omitted rather than sent empty: a grant without it is what every client sent until it existed, and the server keeps its own default for one. The name comes from `hostname(1)`, which ships on all three platforms and stays right when the name changes under a running shell, falling back to `COMPUTERNAME` / `HOSTNAME` for a container that has no `hostname` on `PATH`. No new dependency for it. A hostname can carry a person's name, and this sends it to the API. It is shown in an editable field on the approval page before anything is stored, so the user sees the value and can change it in the moment they are already looking at that page. Fixes MRGFY-9262 Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I7e2ec96a405831003ab0eb03e2a9208f2e7db5d8 --- crates/mergify-auth/src/device.rs | 88 +++++++++++++++++-- crates/mergify-auth/src/lib.rs | 3 + crates/mergify-auth/src/login.rs | 48 +++++++++- crates/mergify-auth/src/machine.rs | 135 +++++++++++++++++++++++++++++ 4 files changed, 266 insertions(+), 8 deletions(-) create mode 100644 crates/mergify-auth/src/machine.rs diff --git a/crates/mergify-auth/src/device.rs b/crates/mergify-auth/src/device.rs index e3f62dc2..4bff2b9c 100644 --- a/crates/mergify-auth/src/device.rs +++ b/crates/mergify-auth/src/device.rs @@ -193,9 +193,34 @@ pub fn client(api_url: Url) -> Result { /// Open a grant: ask the server for the code pair the user is about /// to approve. -pub async fn authorize(client: &HttpClient) -> Result { +/// +/// `device_name` names the machine asking, so the approval page can +/// default the token's name to this one rather than to the client +/// name every machine shares. **Send the bare hostname**: the server +/// composes ` on ` itself, so a client +/// that sent "Mergify CLI on host" would produce "Mergify CLI on +/// Mergify CLI on host". It also sanitizes the value and caps the +/// composed label, which is why nothing is trimmed on this side. +/// +/// Optional on the wire: a grant without the field is what every +/// client sent until it existed, and the server keeps its own +/// default for one. It is additive against a deployment that +/// predates it, too — that endpoint reads its form fields by name +/// rather than as a strict model, so an unknown one is ignored and +/// an older on-prem still logs in. +pub async fn authorize( + client: &HttpClient, + device_name: Option<&str>, +) -> Result { + let mut form = vec![("client_id", CLIENT_ID)]; + // Omitted rather than sent empty when this machine cannot name + // itself: a grant without the field is the case every client had + // until now, and the server still has its own default for it. + if let Some(device_name) = device_name { + form.push(("device_name", device_name)); + } match client - .post_form::(DEVICE_CODE_PATH, &[("client_id", CLIENT_ID)]) + .post_form::(DEVICE_CODE_PATH, &form) .await? { ApiOutcome::Ok(authorization) => checked(authorization), @@ -423,7 +448,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!(authorization.device_code, "dev-secret"); assert_eq!(authorization.user_code, "BCDF-GHJK"); assert_eq!( @@ -435,6 +460,55 @@ mod tests { assert_eq!(schedule.expires_in, Duration::from_secs(600)); } + // The machine's own name, so the approval page can default the + // token to "Mergify CLI on work-laptop" instead of the client + // name every machine shares. + #[tokio::test] + async fn authorize_sends_the_device_name() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/oauth/device/code")) + .and(body_string_contains("client_id=mergify-cli")) + .and(body_string_contains("device_name=work-laptop")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "device_code": "dev-secret", + "user_code": "BCDF-GHJK", + "verification_uri": "https://dashboard.mergify.com/device", + }))) + .expect(1) + .mount(&server) + .await; + + authorize(&test_client(&server), Some("work-laptop")) + .await + .unwrap(); + } + + // A machine that cannot name itself sends no field at all. An + // empty one is not the same thing: the server would sanitize it + // back to nothing, and a deployment that predates the field has + // to see exactly the request it has always seen. + #[tokio::test] + async fn authorize_omits_the_device_name_when_there_is_none() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/oauth/device/code")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "device_code": "dev-secret", + "user_code": "BCDF-GHJK", + "verification_uri": "https://dashboard.mergify.com/device", + }))) + .expect(1) + .mount(&server) + .await; + + authorize(&test_client(&server), None).await.unwrap(); + + let requests = server.received_requests().await.unwrap(); + let body = String::from_utf8_lossy(&requests[0].body); + assert_eq!(body, "client_id=mergify-cli", "got {body:?}"); + } + // A deployment that omits the two optional numbers still has to // produce a usable schedule, at the RFC's defaults. #[tokio::test] @@ -451,7 +525,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!(authorization.verification_uri_complete, None); let schedule = PollSchedule::from_authorization(&authorization); assert_eq!(schedule.interval, DEFAULT_INTERVAL); @@ -532,7 +606,7 @@ mod tests { .mount(&server) .await; - let err = authorize(&test_client(&server)).await.unwrap_err(); + let err = authorize(&test_client(&server), None).await.unwrap_err(); assert!(err.to_string().contains("only http(s)"), "got {err}"); } @@ -556,7 +630,7 @@ mod tests { .mount(&server) .await; - let authorization = authorize(&test_client(&server)).await.unwrap(); + let authorization = authorize(&test_client(&server), None).await.unwrap(); assert_eq!( authorization.user_code, "BCDF[2Kgo to https://attacker.test", @@ -631,7 +705,7 @@ mod tests { .mount(&server) .await; - let err = authorize(&test_client(&server)).await.unwrap_err(); + let err = authorize(&test_client(&server), None).await.unwrap_err(); assert!(err.to_string().contains("Unknown client_id."), "got {err}"); assert_eq!(err.exit_code(), mergify_core::ExitCode::MergifyApiError); } diff --git a/crates/mergify-auth/src/lib.rs b/crates/mergify-auth/src/lib.rs index ec13d973..dc4e4eb7 100644 --- a/crates/mergify-auth/src/lib.rs +++ b/crates/mergify-auth/src/lib.rs @@ -9,6 +9,8 @@ //! - [`identity`] — `GET /v1/user`, the only way to turn a //! credential into an account name and the only way to tell a //! live one from a revoked one. +//! - [`machine`] — the hostname a login sends so the approval page +//! can name the machine asking. //! - [`login`] / [`logout`] / [`status`] — the three commands. //! //! The credential itself is stored by @@ -20,6 +22,7 @@ pub mod device; pub mod identity; pub mod login; pub mod logout; +pub mod machine; pub mod status; #[cfg(test)] diff --git a/crates/mergify-auth/src/login.rs b/crates/mergify-auth/src/login.rs index 36aa7ab6..781b68e5 100644 --- a/crates/mergify-auth/src/login.rs +++ b/crates/mergify-auth/src/login.rs @@ -16,6 +16,7 @@ use url::Url; use crate::browser::Browser; use crate::device; use crate::identity; +use crate::machine; pub struct LoginOptions<'a> { pub api_url: Option<&'a str>, @@ -54,7 +55,14 @@ pub async fn run(opts: LoginOptions<'_>, output: &mut dyn Output) -> Result<(), // store, so this costs nothing. let previous = opts.store.get(&api_url)?; - let authorization = device::authorize(&client).await?; + // The machine's own name, so the approval page's Token name + // field defaults to this machine instead of to the client name + // every machine shares — the page's own helper text already asks + // the user to name it after the machine. Optional: a machine + // that cannot name itself, or a deployment that predates the + // field, falls back to that shared default. + let device_name = machine::name().await; + let authorization = device::authorize(&client, device_name.as_deref()).await?; let opened = open_verification_page(opts.browser, verification_url(&authorization)); output.status(&instructions(&authorization, opened))?; @@ -808,6 +816,44 @@ mod tests { }); } + // The wiring, not the renderer: `device_name` has to leave this + // command on the grant request, or the approval page goes back + // to naming every machine "Mergify CLI". Asserted against what + // this machine actually calls itself, so the test says the same + // thing on a laptop and in a container with no `hostname`. + #[test] + fn login_tells_the_server_which_machine_asked() { + with_mergify_token(None, async { + let server = MockServer::start().await; + mount_flow(&server, true).await; + let (dir, store) = file_store(); + let mut captured = Captured::human(); + + run( + LoginOptions { + api_url: Some(&server.uri()), + store: &store, + browser: None, + }, + &mut captured.output, + ) + .await + .unwrap(); + + let requests = server.received_requests().await.unwrap(); + let grant = requests + .iter() + .find(|r| r.url.path() == "/v1/oauth/device/code") + .expect("the grant request"); + let body = String::from_utf8_lossy(&grant.body); + match crate::machine::name().await { + Some(_) => assert!(body.contains("device_name="), "got {body:?}"), + None => assert_eq!(body, "client_id=mergify-cli", "got {body:?}"), + } + drop(dir); + }); + } + // `--no-browser`: nothing is spawned, and the instructions go // back to telling the user to open the URL themselves. #[test] diff --git a/crates/mergify-auth/src/machine.rs b/crates/mergify-auth/src/machine.rs new file mode 100644 index 00000000..ec730038 --- /dev/null +++ b/crates/mergify-auth/src/machine.rs @@ -0,0 +1,135 @@ +//! The name this machine calls itself. +//! +//! One caller: the `device_name` `auth login` sends when it opens a +//! grant, so the approval page's Token name field defaults to +//! `Mergify CLI on ` rather than to the static client name +//! every machine shares. The field is optional on the wire — a +//! machine that cannot name itself sends nothing and the server +//! keeps its own default — so nothing here is worth failing a login +//! over, and every failure is a `None`. + +use std::process::Command; +use std::time::Duration; + +/// How long `hostname` gets. It reads a name the kernel already +/// holds, so this is not a budget — it is the ceiling on how long a +/// wedged shim on `PATH` can hold up a login. `login` runs this +/// before anything is on screen, and a blank terminal is a worse +/// failure than a token named "Mergify CLI". +const LOOKUP_TIMEOUT: Duration = Duration::from_secs(2); + +/// The machine's hostname, as the machine reports it. +/// +/// `hostname(1)` first: it ships on all three platforms and is the +/// only answer that is still right after the name changes under a +/// running shell. The variables are the fallback for a container +/// with no `hostname` on `PATH`. +/// +/// Verbatim, on purpose. Not lower-cased, not stripped of a +/// `.local` suffix, not cut to length. The server sanitizes the +/// value (printable ASCII, collapsed whitespace, 60 characters) and +/// composes the label itself, so a client that trimmed first would +/// only disagree with it. +pub async fn name() -> Option { + // Off the runtime thread and on a deadline. The value decorates + // the approval page and nothing waits on it, so a `hostname` + // that does not come back is dropped rather than waited for. + let from_command = + tokio::time::timeout(LOOKUP_TIMEOUT, tokio::task::spawn_blocking(from_command)) + .await + .ok() + .and_then(Result::ok) + .flatten(); + from_command.or_else(from_env) +} + +fn from_command() -> Option { + let output = Command::new("hostname").output().ok()?; + if !output.status.success() { + return None; + } + // Lossy because a name is worth more than the byte that did not + // decode: the server maps anything outside printable ASCII to a + // space anyway. + as_name(&String::from_utf8_lossy(&output.stdout)) +} + +fn from_env() -> Option { + // `COMPUTERNAME` is Windows' own; `HOSTNAME` is exported by some + // shells and images and is worth asking for once the command is + // gone. + mergify_core::env::var_non_empty("COMPUTERNAME") + .or_else(|| mergify_core::env::var_non_empty("HOSTNAME")) + .as_deref() + .and_then(as_name) +} + +/// What a `hostname` process printed, as a name. +/// +/// Dropping the line's own newline is reading the command's output, +/// not sanitizing the value — what is inside the line goes to the +/// server as it stands. A name that is only whitespace is no name. +fn as_name(raw: &str) -> Option { + let trimmed = raw.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + // Whatever this machine is called, what comes back has to be + // sendable: the newline `hostname` prints must not reach the + // form body. + #[tokio::test] + async fn the_name_of_the_machine_running_the_suite_is_sendable() { + if let Some(name) = name().await { + assert!(!name.is_empty()); + assert_eq!(name.trim(), name, "a name must carry no surrounding space"); + } + } + + #[test] + fn a_printed_name_loses_its_line_and_nothing_else() { + assert_eq!(as_name("work-laptop\n").as_deref(), Some("work-laptop")); + // The `.local` stays: the server composes and caps the + // label, and a client that trimmed here would disagree with + // what the approval page shows. + assert_eq!( + as_name("some-macbook.local\n").as_deref(), + Some("some-macbook.local"), + ); + } + + #[test] + fn a_machine_that_names_itself_nothing_has_no_name() { + assert_eq!(as_name(" \n"), None); + assert_eq!(as_name(""), None); + } + + // The container case: no `hostname` on PATH, and the image sets + // the variable instead. + #[test] + fn the_variables_are_the_fallback() { + let from_windows = temp_env::with_vars( + [ + ("COMPUTERNAME", Some("WIN-BOX")), + ("HOSTNAME", Some("ignored")), + ], + from_env, + ); + assert_eq!(from_windows.as_deref(), Some("WIN-BOX")); + + let from_shell = temp_env::with_vars( + [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))], + from_env, + ); + assert_eq!(from_shell.as_deref(), Some("build-42")); + + let from_nothing = temp_env::with_vars( + [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)], + from_env, + ); + assert_eq!(from_nothing, None); + } +}