From 3fc290db5906eb3c9b5892d27d163819aa6c5613 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 30 Sep 2026 08:43:49 +0000 Subject: [PATCH] feat(diagnose): offer a report the user sends themselves A report reaches AMD because a person read it and sent it. This builds the prefilled mail, decides whether a mail client may be started, and offers it behind `--send`. It holds nothing that could send anything: no SMTP client, no credential, nothing to authenticate with. That is the design, not an omission, so the absence is worth keeping visible. The destination is a mailbox fixed in code. An address a caller can choose is an address an attacker can choose, and the user would be reading a report they believe goes to AMD while it goes somewhere else. The address is pinned by a test as a literal, because a typo there sends every report somewhere nobody is watching and nothing else would notice. A mailbox has no labels, so the classification an issue would carry in metadata moves to the subject line, which is the only place a mail rule and a person scanning an inbox can both read. It is ordered most stable first, so a sorted inbox groups by cause and then by machine. A subject is as public as the body and travels further, since it shows in an inbox list, so a field not approved for the body is not approved for the subject either, and a test plants markers to prove none reaches it. `--send` requires `--report`, which makes "the content is shown first" structural rather than a promise: the flag that sends a report cannot be given without the flag that prints it. It refuses `--json`, because that combination is for scripts and a script is not a person who can read a mail before sending it. Two conditions gate a mail client, and both are checked. The user has to have asked, and the machine has to look like a desktop they are at. Every unknown answers no, a session reached over SSH answers no whatever DISPLAY says, and an empty variable is not evidence. The printed form names the address on its own rather than only inside the link, because a server or a container usually has no mail client and the person has to send it by hand. The link body is compared against the report as JSON rather than as a `Report`. Deserializing discards fields the struct does not know, so a body carrying a hostname beside the approved fields round-trips to an identical `Report` and passes. A planted leak survived the first version of that test. The opener moves behind a trait and out of the ComfyUI path, which held the only implementation. Callers take the trait, so a test can observe that a client was deliberately not started, which is otherwise indistinguishable from one that failed. There is now exactly one opener in the tree. One cost is worth stating where it is incurred rather than in a footnote: a mail carries the sender's address, which the report itself deliberately does not. The transport identifies a person the payload was designed not to. Signed-off-by: Eugene Volen --- README.md | 12 + apps/rocm/src/comfyui.rs | 34 +- apps/rocm/src/main.rs | 177 ++++++++- crates/rocm-core/src/browser.rs | 67 ++++ crates/rocm-core/src/lib.rs | 2 + crates/rocm-core/src/report_delivery.rs | 491 ++++++++++++++++++++++++ docs/testing.md | 30 ++ 7 files changed, 778 insertions(+), 35 deletions(-) create mode 100644 crates/rocm-core/src/browser.rs create mode 100644 crates/rocm-core/src/report_delivery.rs diff --git a/README.md b/README.md index e4265e908..33f7a6354 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,18 @@ fix` takes the id, not the position. GPU on WSL yet, so it cannot confirm the hardware is on the compatibility matrix and says that rather than claiming the architecture could not be read. +- `--send`, which requires `--report`, additionally offers a prefilled mail + carrying that report. It still sends nothing: the mail opens already filled + in with the content `--report` just printed, addressed to `ROCmCLI@amd.com`, + and it leaves the machine only when you send it yourself. Requiring + `--report` is what guarantees the content is shown before the mail is + offered. A mail client opens only when you asked and the machine looks like + a desktop you are at; over SSH, with no display, or with `ROCM_NO_BROWSER` + set, the address and the link are printed instead, which is also what + happens on a machine with no mail client. It is not combinable with + `--json`, which exists for scripts, and a script is not a person who can + read a mail before sending it. Note that a mail carries your address, which + the report itself does not. `fix` applies a known fix by the `id:` that `diagnose` reported — not the ranking position noted above, which isn't a stable name. Run it with no id diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 24ab029ee..14ac0f625 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -6,6 +6,7 @@ use crate::cli_progress::AnimatedSpinner; use crate::{format_structured_tool_call, runtime_usability_status, therock}; use anyhow::{Context, Result, bail}; use flate2::read::GzDecoder; +use rocm_core::browser::{Opener, SystemOpener}; use rocm_core::{ AppPaths, RocmCliConfig, download_file_to_path_with_progress, ensure_uv_binary, format_http_base_url, runtime_is_linux, runtime_is_windows, runtime_path_for_windows_child, @@ -474,7 +475,7 @@ pub(crate) fn start(paths: &AppPaths, options: ComfyUiStartOptions) -> Result "opened".to_owned(), Err(error) => format!("not opened ({error})"), } @@ -1938,37 +1939,6 @@ fn child_path_string(path: &Path) -> String { } } -fn open_browser(url: &str) -> Result<()> { - let status = if runtime_is_windows() { - Command::new("cmd") - .args(["/C", "start", "", url]) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } else if cfg!(target_os = "macos") && !runtime_is_linux() { - Command::new("open") - .arg(url) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } else { - Command::new("xdg-open") - .arg(url) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } - .context("failed to open browser")?; - if status.success() { - Ok(()) - } else { - bail!("browser opener exited with status {status}") - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 7e4cf2cc1..a086c576b 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -28,6 +28,7 @@ use crate::uninstall::uninstall; use anyhow::{Context, Result, bail}; use clap::{CommandFactory, FromArgMatches, Parser, Subcommand, ValueEnum}; +use rocm_core::browser::Opener; use rocm_core::{ AppPaths, AuditEventRecord, AutomationEventRecord, AutomationProposalRecord, AutomationRuntimeState, CodexBridgeEngine, CodexBridgeGpuSnapshot, CodexBridgeSnapshot, @@ -165,6 +166,18 @@ enum Command { /// architecture check. #[arg(long, conflicts_with = "distro")] report: bool, + /// Also offer the prefilled issue form, so the report can be filed. + /// + /// Still sends nothing. This opens the form with the same content + /// `--report` printed, already filled in; it reaches the tracker only + /// when you submit it yourself. On a machine with no desktop, or one + /// reached over SSH, the link is printed instead of opened. + /// + /// Requires `--report`, so the content is always shown before the + /// form is offered. Not combinable with `--json`, which is for + /// scripts, and a script is not a person who can read a form. + #[arg(long, requires = "report", conflicts_with = "json")] + send: bool, }, /// Apply a known fix by id (see `rocm diagnose`); run with no id to list fixes. /// @@ -2079,7 +2092,8 @@ fn dispatch(cli: Cli) -> Result<()> { json, distro, report, - }) => diagnose(symptom, top, json, distro, report), + send, + }) => diagnose(symptom, top, json, distro, report, send), // Keep this error chained rather than discarding it into a fresh // `anyhow!(...)` (e.g. via a `.map_err` that restringifies it) -- see // `FixExitCode`'s doc comment for why that would silently break its @@ -2756,6 +2770,7 @@ fn diagnose( json: bool, distro: Option, report_requested: bool, + send: bool, ) -> Result<()> { // `rocm diagnose` is a query: it exits 0 whether it matched, found nothing, // or is out of scope. Callers read `has_match` / `out_of_scope` / @@ -2790,7 +2805,7 @@ fn diagnose( .is_some_and(|wsl| !wsl.locally_probed); let report = rocm_core::run_diagnose(&examination, &symptom.unwrap_or_default()); if report_requested { - return show_prepared_report(&examination, &report, json); + return show_prepared_report(&examination, &report, json, send); } if json { println!("{}", serde_json::to_string_pretty(&report)?); @@ -2829,11 +2844,50 @@ fn established_entry(report: &rocm_core::DiagnoseReport) -> (Option<&str>, bool) }) } +/// Act on a delivery decision, and say what happened. +/// +/// Takes the decision rather than making it, and takes the opener rather than +/// being one. Both for the same reason: the decision is tested in `rocm-core` +/// against every environment, and this half has to be tested against an opener +/// that does not exist, on a machine with no browser. A function that decided +/// and opened could be verified on neither. +fn perform_delivery( + delivery: &rocm_core::report_delivery::Delivery, + opener: &dyn Opener, +) -> String { + use rocm_core::report_delivery::{DESTINATION, Delivery}; + match delivery { + // No mail client is started here on purpose, and the reason is worth + // the line: this is the branch for a machine held over SSH, or a + // server with no mail client at all, where starting one would open on + // somebody else's desktop or fail silently. The address is named as + // well as the link, because a machine in this state often cannot act + // on a `mailto:` at all and the user has to send the mail by hand. + Delivery::Show(url) => format!( + "Nothing has been sent. To send this yourself, mail the report above to \ + {DESTINATION}, or open:\n {url}" + ), + Delivery::Open(url) => match opener.open(url) { + Ok(()) => format!( + "Nothing has been sent yet. A prefilled mail to {DESTINATION} was opened, and \ + it is sent only when you send it:\n {url}" + ), + // A failed open is not a failed command. The user still has the + // address and the link, which is the whole of what this offers. + Err(error) => format!( + "Nothing has been sent. A mail client could not be started ({error}). To send \ + this yourself, mail the report above to {DESTINATION}, or open:\n {url}" + ), + }, + } +} + /// Print the report this machine would contribute, and send nothing. fn show_prepared_report( examination: &rocm_core::Examination, report: &rocm_core::DiagnoseReport, json: bool, + send: bool, ) -> Result<()> { let (entry, fix_offered) = established_entry(report); // Exit 0 either way. A refusal is this command working, not failing: it @@ -2850,7 +2904,17 @@ fn show_prepared_report( println!(); println!("{}", serde_json::to_string_pretty(&prepared)?); println!(); - println!("Nothing has been sent. Sending is not implemented yet."); + // The content is printed above before this decides anything, + // so a report is always read before its form is offered. That + // ordering is the promise `--send` makes, and `--send` + // requires `--report` so it cannot be skipped. + let delivery = rocm_core::report_delivery::deliver(&prepared, send, &|key| { + std::env::var(key).ok() + }); + println!( + "{}", + perform_delivery(&delivery, &rocm_core::browser::SystemOpener) + ); } Ok(()) } @@ -22166,6 +22230,113 @@ fn treat_as_natural_language(args: &[String]) -> bool { #[cfg(test)] mod tests { + use std::cell::RefCell; + + use rocm_core::browser::Opener; + use rocm_core::report_delivery::Delivery; + + use super::perform_delivery; + + /// An opener that records rather than opens, and can be told to fail. + /// + /// The whole reason the opener is a trait: the real one spawns a browser + /// against whatever desktop exists, so neither "it was opened" nor "it was + /// deliberately not opened" can be observed in CI without this. + struct RecordingOpener { + opened: RefCell>, + fails: bool, + } + + impl RecordingOpener { + fn working() -> Self { + Self { + opened: RefCell::new(Vec::new()), + fails: false, + } + } + fn broken() -> Self { + Self { + opened: RefCell::new(Vec::new()), + fails: true, + } + } + fn opened(&self) -> Vec { + self.opened.borrow().clone() + } + } + + impl Opener for RecordingOpener { + fn open(&self, url: &str) -> anyhow::Result<()> { + self.opened.borrow_mut().push(url.to_owned()); + if self.fails { + anyhow::bail!("no browser here"); + } + Ok(()) + } + } + + /// Nothing is opened unless the decision was to open. + /// + /// The assertion that matters is on the opener, not on the wording. A + /// message saying no browser was started is satisfied by any string; an + /// opener that recorded nothing is the actual claim. + #[test] + fn a_delivery_that_is_not_an_open_never_reaches_the_browser() { + let delivery = Delivery::Show("mailto:nobody@example.invalid".to_owned()); + let opener = RecordingOpener::working(); + let said = perform_delivery(&delivery, &opener); + + assert!( + opener.opened().is_empty(), + "a mail client was started for {delivery:?}, which is the one thing this path must \ + not do on a machine the user is holding over SSH" + ); + assert!( + said.contains("Nothing has been sent"), + "the user has to be told nothing left the machine: {said}" + ); + // A machine in this state often cannot act on a `mailto:` at all, so + // the address has to be readable on its own, not only inside the link. + assert!( + said.contains(rocm_core::report_delivery::DESTINATION), + "a user who has to send the mail by hand needs the address: {said}" + ); + } + + /// Opening is what an open decision does, and the user is told it is not + /// filed yet. + #[test] + fn an_open_decision_reaches_the_browser_and_is_still_not_a_send() { + let url = "https://example.invalid/new?body=x"; + let opener = RecordingOpener::working(); + let said = perform_delivery(&Delivery::Open(url.to_owned()), &opener); + + assert_eq!( + opener.opened(), + vec![url.to_owned()], + "premise failed: an open decision must reach the opener, otherwise the cases above \ + are satisfied by never opening anything" + ); + assert!( + said.contains("only when you send it"), + "opening a prefilled mail is not sending it, and the user has to know which one \ + happened: {said}" + ); + } + + /// A browser that will not start still leaves the user the link. + #[test] + fn a_browser_that_fails_to_start_still_hands_the_user_the_link() { + let url = "https://example.invalid/new?body=x"; + let said = perform_delivery(&Delivery::Open(url.to_owned()), &RecordingOpener::broken()); + + assert!( + said.contains(url), + "the link is the whole of what this offers, so a failed browser must not lose it: \ + {said}" + ); + assert!(said.contains("Nothing has been sent")); + } /// A diagnosis report holding exactly one finding. /// diff --git a/crates/rocm-core/src/browser.rs b/crates/rocm-core/src/browser.rs new file mode 100644 index 000000000..6be0a5549 --- /dev/null +++ b/crates/rocm-core/src/browser.rs @@ -0,0 +1,67 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Handing a URL to the user's browser. +//! +//! Behind a trait because starting a browser is the one thing in this area +//! that cannot run in CI: the real implementation spawns a process against +//! whatever desktop the machine has, so any code path that opens a URL is +//! untestable unless the opening itself can be replaced. Callers take +//! `&dyn Opener`, tests pass a fake, and the decision to open stays separate +//! from the opening. +//! +//! One implementation, deliberately. A second opener somewhere else is how the +//! seam stops being a seam. + +use std::process::{Command, Stdio}; + +use anyhow::{Context, Result, bail}; + +use crate::{runtime_is_linux, runtime_is_windows}; + +/// Something that can show the user a URL. +pub trait Opener { + /// Hand `url` to the user's browser. + /// + /// # Errors + /// When no browser could be started, or the opener reported failure. + fn open(&self, url: &str) -> Result<()>; +} + +/// The real one: whatever this platform uses to open a link. +#[derive(Debug, Clone, Copy, Default)] +pub struct SystemOpener; + +impl Opener for SystemOpener { + fn open(&self, url: &str) -> Result<()> { + let status = if runtime_is_windows() { + Command::new("cmd") + .args(["/C", "start", "", url]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } else if cfg!(target_os = "macos") && !runtime_is_linux() { + Command::new("open") + .arg(url) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } else { + Command::new("xdg-open") + .arg(url) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } + .context("failed to open browser")?; + if status.success() { + Ok(()) + } else { + bail!("browser opener exited with status {status}") + } + } +} diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 80ccf01ec..dcfc1aab9 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -27,6 +27,7 @@ use windows_sys::Win32::System::Threading::{ WaitForSingleObject, }; +pub mod browser; pub mod diagnose; pub mod disk_space; pub mod examine; @@ -34,6 +35,7 @@ pub mod fix; pub mod openmpi; pub mod proc_lifecycle; pub mod report; +pub mod report_delivery; pub mod runtime; #[cfg(test)] mod test_env; diff --git a/crates/rocm-core/src/report_delivery.rs b/crates/rocm-core/src/report_delivery.rs new file mode 100644 index 000000000..6f620e93f --- /dev/null +++ b/crates/rocm-core/src/report_delivery.rs @@ -0,0 +1,491 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! How a report leaves the machine, which is only ever by the user's own act. +//! +//! This module builds a link and decides whether a browser may be started. It +//! never sends anything, and there is deliberately no code here that could: +//! no HTTP client, no token, nothing to authenticate with. A report reaches a +//! tracker because a person read it and pressed a button. +//! +//! Kept apart from [`crate::report`], which decides what a report *contains*. +//! That module is pure by design and says so; this one is where the outside +//! world starts, so the boundary is worth keeping visible. + +use crate::report::{Report, UNRECOGNISED}; + +/// Where a report is sent. +/// +/// Fixed in code rather than configurable on purpose: an address a caller can +/// choose is an address an attacker can choose, and the user would be reading a +/// report they believe is going to AMD while it goes somewhere else. +/// +/// A mailbox rather than an issue tracker. That choice costs the report its +/// anonymity, because a mail envelope carries the sender's address whatever the +/// body says, and it costs the ability to count reports, because a mailbox has +/// no query. Both are recorded where the decision was made rather than here. +pub const DESTINATION: &str = "ROCmCLI@amd.com"; + +/// The first word of every subject line, so a mail rule can route the whole set. +pub const SUBJECT_TAG: &str = "[rocm-doctor]"; + +/// What a subject says when the catalog recognised nothing. +pub const UNRECOGNISED_SUBJECT: &str = "unrecognised"; + +/// What should happen next, decided here and performed by the caller. +/// +/// A value rather than an action, so the decision can be tested without a +/// browser, a network, or a display. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Delivery { + /// Hand this to the user's mail client, then tell them what was opened. + Open(String), + /// Show this and start nothing. + /// + /// The headless case, and the honest default whenever there is doubt. A + /// link on screen costs a user one paste, while a mail client started on a + /// machine they are holding over SSH is a process they did not ask for on + /// a display that is not theirs. + /// + /// This case matters more for mail than it did for a web link. Servers, + /// lab machines and containers usually have no mail client at all, so the + /// link would fail silently rather than open anything. + Show(String), +} + +/// The subject line: a routing tag, the cause, and the coarse description. +/// +/// A mailbox has no labels, so the classification an issue would carry in +/// metadata has to live somewhere a mail rule and a human scanning an inbox can +/// both read. The subject is the only such place. +/// +/// Ordered most stable first, so a sorted inbox groups by cause and then by +/// machine. Nothing is here that the body does not already carry: a subject is +/// as public as the body, and a field that is not approved for one is not +/// approved for the other. +#[must_use] +pub fn subject_for(report: &Report) -> String { + let cause = if report.entry == UNRECOGNISED { + UNRECOGNISED_SUBJECT + } else { + report.entry.as_str() + }; + format!( + "{SUBJECT_TAG} {cause} on {} / {}-{}", + report.architecture, report.distro, report.os_major + ) +} + +/// The prefilled mail link for a report. +/// +/// The body is the report as the user was shown it. Nothing is added here: a +/// field that is not in [`Report`] has not been through the approved-field +/// check, and this is exactly the seam where "just one more useful detail" +/// would bypass it. +#[must_use] +pub fn report_url(report: &Report) -> String { + mail_to(DESTINATION, report) +} + +/// The link builder, separated from the destination so it can be exercised +/// against an address that is not the real mailbox. A test that sends to the +/// real one would be indistinguishable from a bug that does. +fn mail_to(destination: &str, report: &Report) -> String { + // `expect` rather than a fallible return: every field of `Report` is a + // `String`, a `u32` or a `bool`, so this has no failing case to handle, + // and inventing one would add a branch no test could ever reach. + let body = serde_json::to_string_pretty(report).expect("a report has no unserializable field"); + // The address is not escaped. RFC 6068 allows `@` and `.` unescaped in the + // address part, and a percent-escaped `@` there is handled poorly by some + // mail clients. This is safe because the address is a compile-time + // constant this crate owns, never a value read from a machine. The query + // values that follow are escaped, because they carry machine-derived text. + format!( + "mailto:{destination}?subject={}&body={}", + percent_encode(&subject_for(report)), + percent_encode(&body), + ) +} + +/// Whether a browser may be started for this user. +/// +/// Reads the environment rather than probing anything, and every unknown +/// answers no. Starting a browser is the one irreversible thing this module +/// can do, so it happens only where there is positive evidence of a desktop +/// the user is sitting at. +#[must_use] +pub fn may_open_browser(env: &dyn Fn(&str) -> Option) -> bool { + let set = |key: &str| env(key).is_some_and(|value| !value.trim().is_empty()); + + // A session reached over SSH belongs to a display somewhere else. Opening + // a browser here either fails or opens it on a machine the user is not + // looking at. + if set("SSH_CONNECTION") || set("SSH_CLIENT") || set("SSH_TTY") { + return false; + } + // An explicit opt-out is honoured before any positive evidence: a user who + // said no has said no. + if set("ROCM_NO_BROWSER") { + return false; + } + if cfg!(target_os = "windows") || cfg!(target_os = "macos") { + return true; + } + // On Linux a desktop is not implied by anything except a display. + set("DISPLAY") || set("WAYLAND_DISPLAY") +} + +/// Decide what to do with a report. +/// +/// `sending` is whether the user asked to be taken to a prefilled mail, rather +/// than only to read what it would say. Both conditions have to hold before a +/// mail client starts: the user asked, and this looks like a desktop they are +/// sitting at. Either one alone is not enough, and the user's is checked +/// first, because a machine that could open a mail client is not a reason to. +#[must_use] +pub fn deliver(report: &Report, sending: bool, env: &dyn Fn(&str) -> Option) -> Delivery { + choose(report_url(report), sending, env) +} + +/// The choice itself, taking the link rather than building it, so the two +/// branches can be tested without reaching the real mailbox. +fn choose(url: String, sending: bool, env: &dyn Fn(&str) -> Option) -> Delivery { + if sending && may_open_browser(env) { + Delivery::Open(url) + } else { + Delivery::Show(url) + } +} + +/// Percent-encode for a query-string value. +/// +/// Written out rather than taken from a crate: this is the only encoding this +/// binary needs, and a signed artifact is not worth a dependency for fifteen +/// lines. Everything outside the unreserved set of RFC 3986 is escaped, which +/// is stricter than necessary and wrong in no case. +fn percent_encode(value: &str) -> String { + const HEX: &[u8; 16] = b"0123456789ABCDEF"; + let mut out = String::with_capacity(value.len()); + for byte in value.bytes() { + if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'.' | b'_' | b'~') { + out.push(byte as char); + } else { + out.push('%'); + out.push(HEX[(byte >> 4) as usize] as char); + out.push(HEX[(byte & 0x0f) as usize] as char); + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::examine::{Examination, Gpu}; + use crate::report::prepare_report; + + /// A machine that produces a report, so the link under test is built from + /// what the product actually emits rather than a hand-written `Report`. + fn reportable_machine() -> Examination { + Examination { + os_family: "linux".to_owned(), + distro_id: "ubuntu".to_owned(), + distro_version: "22.04".to_owned(), + os_version: "#1 SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC 2026".to_owned(), + has_amd_gpu: true, + // Markers rather than plausible values, so a leak into the subject + // or the body is visible by eye in a failure message instead of + // reading like a real machine. + user_name: "SENTINEL-USER".to_owned(), + rocm_path: "/SENTINEL-PATH/rocm".to_owned(), + cpu_model: "SENTINEL-CPU".to_owned(), + gpus: vec![Gpu { + name: "SENTINEL-MARKETING-NAME".to_owned(), + gfx_target: "gfx1100".to_owned(), + pci_id: "SENTINEL-PCI".to_owned(), + is_apu: Some(false), + is_amd: true, + }], + ..Examination::default() + } + } + + fn report_of(entry: Option<&str>) -> Report { + prepare_report(&reportable_machine(), entry, false) + .expect("a released machine must produce a report") + } + + /// No environment at all, which is the headless shape. + /// + /// Linux-only, like its two call sites: on Windows and macOS + /// [`may_open_browser`] answers yes without consulting the environment, so + /// there is no "no display" case to construct there, and an unguarded + /// helper would be dead code on those targets -- which is exactly what + /// failed the Windows lane here before this was guarded. + #[cfg(target_os = "linux")] + fn no_env() -> impl Fn(&str) -> Option { + |_| None + } + + fn env_of(pairs: &'static [(&'static str, &'static str)]) -> impl Fn(&str) -> Option { + move |key| { + pairs + .iter() + .find(|(k, _)| *k == key) + .map(|(_, v)| (*v).to_owned()) + } + } + + /// The link carries the report and nothing else. + /// + /// Compared as JSON rather than as a `Report`, which is the whole point. + /// Deserializing into `Report` discards fields the struct does not know, + /// so a body carrying an extra `"hostname"` beside the approved fields + /// round-trips to an identical `Report` and passes. Found by mutation: a + /// planted leak survived the first version of this test. Comparing the + /// parsed values keeps every key, including ones nothing agreed to. + #[test] + fn the_link_body_is_the_report_the_user_was_shown_and_nothing_more() { + let report = report_of(None); + let url = mail_to("nobody@example.invalid", &report); + + let body = query_value(&url, "body").expect("the link must carry a body"); + let carried: serde_json::Value = + serde_json::from_str(body.trim()).expect("the body must be the report as JSON"); + let approved = serde_json::to_value(&report).expect("a report must serialize"); + + assert_eq!( + carried, approved, + "the link body is not exactly the report the user approved. An added field has not \ + been through the approved-field check, and this seam is where one would be added" + ); + } + + /// The link addresses the mailbox and carries the subject. + #[test] + fn the_link_addresses_the_destination_and_carries_the_subject() { + let report = report_of(Some("fix-6-path")); + let url = report_url(&report); + + assert!( + url.starts_with(&format!("mailto:{DESTINATION}?")), + "a report has to address the mailbox it is meant for: {url}" + ); + assert_eq!( + query_value(&url, "subject").as_deref(), + Some(subject_for(&report).as_str()), + "the subject is the only classification a mailbox can route on, so it has to \ + survive the link: {url}" + ); + } + + /// The subject names the cause, because a mailbox has no labels. + /// + /// Paired, so "always says unrecognised" cannot satisfy the first half. + #[test] + fn the_subject_names_the_cause_so_a_mailbox_can_be_sorted_by_it() { + let unrecognised = subject_for(&report_of(None)); + assert!(unrecognised.starts_with(SUBJECT_TAG)); + assert!( + unrecognised.contains(UNRECOGNISED_SUBJECT), + "a report with no matched cause has to say so in the subject: {unrecognised}" + ); + + let recognised = subject_for(&report_of(Some("fix-6-path"))); + assert!(recognised.starts_with(SUBJECT_TAG)); + assert!( + recognised.contains("fix-6-path"), + "premise failed: a matched entry has to reach the subject, or the case above is \ + satisfied by never naming a cause: {recognised}" + ); + assert!( + !recognised.contains(UNRECOGNISED_SUBJECT), + "a matched entry must not also be called unrecognised: {recognised}" + ); + } + + /// The subject carries nothing the body does not. + /// + /// A subject line is as public as the body and travels further, since it + /// shows in an inbox list. Anything here that is not an approved field has + /// bypassed the field check by a side door. + #[test] + fn the_subject_carries_no_field_the_report_does_not() { + let machine = reportable_machine(); + let report = report_of(None); + let subject = subject_for(&report); + + for planted in [ + machine.user_name.as_str(), + machine.rocm_path.as_str(), + machine.cpu_model.as_str(), + machine.gpus[0].pci_id.as_str(), + machine.gpus[0].name.as_str(), + ] { + if planted.is_empty() { + continue; + } + assert!( + !subject.contains(planted), + "'{planted}' reached the subject line: {subject}" + ); + } + } + + /// A session reached over SSH is never given a browser, on any platform. + /// + /// Both refusals here are checked before the platform is consulted, so + /// they hold everywhere and are asserted unconditionally. The rules that + /// depend on a display live in the Linux-only test below, because Windows + /// and macOS have no `DISPLAY` to reason about and + /// [`may_open_browser`] treats them as a desktop outright. + #[test] + fn a_session_over_ssh_is_shown_the_link_rather_than_having_a_browser_started() { + assert!( + !may_open_browser(&env_of(&[ + ("DISPLAY", ":0"), + ("SSH_CONNECTION", "10.0.0.1 22") + ])), + "a display variable does not make an SSH session local" + ); + assert!( + !may_open_browser(&env_of(&[("DISPLAY", ":0"), ("ROCM_NO_BROWSER", "1")])), + "an explicit opt-out is not overridden by a display" + ); + + // The premise. Without it both assertions above are satisfied by a + // function that refuses everything, which would take the feature with + // it. Linux-only because it is the platform that needs evidence: see + // the test below. + #[cfg(target_os = "linux")] + assert!( + may_open_browser(&env_of(&[("DISPLAY", ":0")])), + "premise failed: a plain local display must be allowed" + ); + } + + /// On Linux, a desktop has to be evidenced, and an empty variable is not + /// evidence. + /// + /// Linux-only, and that is the point rather than a convenience. Windows + /// and macOS have no `DISPLAY`, so [`may_open_browser`] answers yes there + /// without looking at the environment at all, and asserting the Linux rule + /// on them tests nothing about either platform. The first version of this + /// was not guarded and failed the Windows lane, having passed locally. + #[cfg(target_os = "linux")] + #[test] + fn an_empty_display_variable_does_not_count_as_a_desktop_on_linux() { + assert!(!may_open_browser(&env_of(&[("DISPLAY", "")]))); + assert!(!may_open_browser(&env_of(&[("DISPLAY", " ")]))); + assert!( + !may_open_browser(&no_env()), + "no display at all is not a desktop either" + ); + + // Paired, so the three refusals above cannot be satisfied by refusing + // everything. + assert!( + may_open_browser(&env_of(&[("WAYLAND_DISPLAY", "wayland-0")])), + "premise failed: a Wayland display is evidence of a desktop" + ); + } + + /// The destination is the agreed mailbox and nothing else. + /// + /// Pinned as a literal because it is the one value in this module that + /// decides where a user's machine description goes. A typo here sends + /// every report somewhere nobody is watching, or somewhere nobody should + /// be watching, and no other test would notice. + #[test] + fn reports_address_the_agreed_mailbox() { + assert_eq!(DESTINATION, "ROCmCLI@amd.com"); + assert!( + report_url(&report_of(None)).contains(DESTINATION), + "the destination has to survive into the link the user is handed" + ); + } + + /// Reading a report is not asking to file one. + /// + /// Two conditions gate a browser, and this covers the one the machine + /// cannot tell you: the user has to have asked. A desktop is permission + /// from the environment, never from the person. Without this, adding a + /// display to a machine would change what `--report` does. + #[test] + fn a_desktop_is_not_permission_to_open_anything_the_user_did_not_ask_for() { + let url = "https://example.invalid/new".to_owned(); + let desktop = env_of(&[("DISPLAY", ":0")]); + + assert_eq!( + choose(url.clone(), false, &desktop), + Delivery::Show(url.clone()), + "a report the user only asked to read must not open a browser, whatever the \ + machine looks like" + ); + + // The premise. Without this the assertion above is satisfied by never + // opening anything, which would take the feature with it. Every + // platform reaches this: a `DISPLAY` is evidence on Linux, and + // Windows and macOS are a desktop regardless. + assert_eq!( + choose(url.clone(), true, &desktop), + Delivery::Open(url.clone()), + "premise failed: asking, on a desktop, has to open" + ); + + // The machine's half of the gate, which only Linux can express. On + // Windows and macOS there is no environment that means "not a + // desktop", so asserting this there would test nothing. + #[cfg(target_os = "linux")] + assert_eq!( + choose(url.clone(), true, &no_env()), + Delivery::Show(url.clone()), + "asking does not override a machine with no desktop" + ); + + // The user's half, which every platform can express, because an + // explicit opt-out is honoured before the platform is consulted. + assert_eq!( + choose( + url.clone(), + true, + &env_of(&[("DISPLAY", ":0"), ("ROCM_NO_BROWSER", "1")]) + ), + Delivery::Show(url), + "an opt-out has to hold on every platform, not only where a display is read" + ); + } + + /// Everything outside the unreserved set is escaped. + #[test] + fn a_value_is_escaped_so_it_cannot_end_the_query_or_start_a_new_field() { + assert_eq!(percent_encode("a b"), "a%20b"); + assert_eq!(percent_encode("a&labels=x"), "a%26labels%3Dx"); + assert_eq!(percent_encode("a#b"), "a%23b"); + assert_eq!(percent_encode("-._~"), "-._~"); + assert_eq!(percent_encode("é"), "%C3%A9"); + } + + /// Read one query-string value back out of a link, decoded. + fn query_value(url: &str, key: &str) -> Option { + let query = url.split_once('?')?.1; + let raw = query + .split('&') + .find_map(|pair| pair.strip_prefix(&format!("{key}=")))?; + let bytes = raw.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'%' && i + 2 < bytes.len() { + let hex = std::str::from_utf8(&bytes[i + 1..i + 3]).ok()?; + out.push(u8::from_str_radix(hex, 16).ok()?); + i += 3; + } else { + out.push(bytes[i]); + i += 1; + } + } + String::from_utf8(out).ok() + } +} diff --git a/docs/testing.md b/docs/testing.md index e27d573c6..9b2b90877 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1194,6 +1194,36 @@ envelope also carries `architecture_matrix`, the same compatibility-matrix snapshot stamp a genuine report carries, so a refusal is just as traceable to a matrix revision as a report is. +Offer a prefilled mail carrying that report, which still sends nothing: + +```bash +rocm diagnose --report --send +``` + +Two argument rules are worth checking by hand, because both are the kind that +only break when somebody reorders a declaration. `--send` without `--report` +must be refused, since showing the content first is the guarantee `--send` +makes. `--send` with `--json` must also be refused: that combination is for +scripts, and starting a browser from a scripted invocation is not wanted. + +Whether `--send` opens a mail client or prints the address and link depends on +the machine, and the printed line says which happened. It prints rather than +opens over SSH, with no `DISPLAY` or `WAYLAND_DISPLAY` on Linux, or with +`ROCM_NO_BROWSER` set to a non-empty value. A machine with no mail client +configured reaches the same printed form, which is why the address appears on +its own and not only inside the `mailto:` link. That is the common case on +servers and in containers. The opt-out is the easiest to check on a desktop: + +```bash +ROCM_NO_BROWSER=1 rocm diagnose --report --send +``` + +The destination is `ROCmCLI@amd.com`, fixed in code. A report also carries its +classification in the mail subject, because a mailbox has no labels: the +subject names the matched catalog entry, or `unrecognised`, then the +architecture and the distribution. Check that the subject carries no field the +report body does not. + On a host with an approved architecture (see `APPROVED_ARCHITECTURES` in `crates/rocm-core/src/report.rs`), the command prints the full `Report`: `schema`, `architecture`, `architecture_matrix`, `entry`, `os_family`,