diff --git a/src-tauri/src/commands/intune.rs b/src-tauri/src/commands/intune.rs index ba61b2715..85fec197a 100644 --- a/src-tauri/src/commands/intune.rs +++ b/src-tauri/src/commands/intune.rs @@ -56,6 +56,112 @@ struct IntuneAnalysisProgressPayload { total_files: Option, } +/// Sink for the verbose GUID-enrichment trace, off unless explicitly asked for. +/// +/// The detail this carries is an organisation's app inventory: every registry +/// GUID with its application name, every enriched event name, and every download +/// name and content id. It was written to `%TEMP%/cmtrace-guid-diag.log` on every +/// analysis run, without the operator asking, and never cleaned up. +/// +/// Nobody chose that. It is developer instrumentation for diagnosing GUID +/// enrichment, and the summary an operator actually needs already goes to the +/// application log. Set `CMTRACE_INTUNE_GUID_DIAG` to collect the verbose trace +/// while debugging; leave it unset and nothing is written to disk. +/// +/// Implements `fmt::Write` so the existing `writeln!` call sites are unchanged +/// and discard when the trace is off. +struct GuidDiagLog(Option); + +impl GuidDiagLog { + fn from_env() -> Self { + Self( + std::env::var_os("CMTRACE_INTUNE_GUID_DIAG") + .filter(|value| !value.is_empty()) + .map(|_| String::new()), + ) + } + + /// The collected trace, or `None` when collection was off. + fn contents(&self) -> Option<&str> { + self.0.as_deref().filter(|trace| !trace.is_empty()) + } + + /// Creates the trace file, refusing to write through anything that already exists. + /// + /// The name carries the process id and a monotonic stamp, and the file is opened with + /// `create_new`, so two analyses cannot overwrite each other and the call fails rather than + /// following a symlink an unprivileged user planted at a guessable path. The system temp + /// directory is world-writable, so a fixed name there is the classic clobber target — and this + /// trace is opt-in precisely because its contents are sensitive. + fn create_file(&self) -> std::io::Result<(std::path::PathBuf, fs::File)> { + Self::create_file_at(Self::trace_path()) + } + + /// A path no other analysis will pick, in the system temp directory. + /// + /// The sequence number is what makes that true rather than likely. Two analyses in the same + /// process can land inside one clock tick, and a clock can move backwards, either of which + /// would repeat a timestamp; because the file is opened exclusively, a repeat does not + /// overwrite anything but does lose the trace the operator asked for. + fn trace_path() -> std::path::PathBuf { + let stamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or_default(); + Self::trace_path_for(stamp) + } + + /// The naming, with the clock supplied. + /// + /// Split so the sequence's contribution can be proved: a test that only calls `trace_path` + /// passes whether or not the sequence exists, because successive clock reads on a fast machine + /// happen to differ anyway. Holding the stamp fixed is the only way to show the collision case + /// is handled. + fn trace_path_for(stamp: u128) -> std::path::PathBuf { + static SEQUENCE: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + + let sequence = SEQUENCE.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + std::env::temp_dir().join(format!( + "cmtrace-guid-diag-{}-{stamp}-{sequence}.log", + std::process::id() + )) + } + + /// Creates `path` exclusively, readable only by the user who ran the analysis. + /// + /// Split from [`create_file`](Self::create_file) so the exclusive-open behaviour can be + /// exercised against a path that already exists, which is the case that matters and which a + /// test calling the composed function cannot reach. + fn create_file_at( + path: std::path::PathBuf, + ) -> std::io::Result<(std::path::PathBuf, fs::File)> { + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + + // The default umask leaves this world-readable, and the temp directory is shared. The + // trace holds the app inventory this whole change exists to stop leaking, so on the one + // path that does write it, only the owner may read it. Windows inherits the directory + // ACL, which is already per-user for the profile temp directory. + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + + let file = options.open(&path)?; + Ok((path, file)) + } +} + +impl FmtWrite for GuidDiagLog { + fn write_str(&mut self, text: &str) -> std::fmt::Result { + if let Some(trace) = &mut self.0 { + trace.push_str(text); + } + Ok(()) + } +} + /// Analyze Intune Management Extension logs and return structured results. /// /// Supports either: @@ -236,7 +342,7 @@ fn analyze_intune_logs_blocking( } // Enrich event and download names using the global GUID registry - let mut diag_buffer = String::new(); + let mut diag_buffer = GuidDiagLog::from_env(); let mut enriched_events = 0u32; let mut enriched_downloads = 0u32; let mut missed_events = 0u32; @@ -347,10 +453,22 @@ fn analyze_intune_logs_blocking( "event=guid_enrichment_summary registry={} enriched_events={} missed_events={} enriched_downloads={} missed_downloads={} total_downloads={}", guid_registry.len(), enriched_events, missed_events, enriched_downloads, missed_downloads, all_downloads.len() ); - let diag_path = std::env::temp_dir().join("cmtrace-guid-diag.log"); - if let Ok(mut f) = fs::File::create(&diag_path) { - let _ = f.write_all(diag_buffer.as_bytes()); - log::info!("event=guid_diag_written path=\"{}\"", diag_path.display()); + if let Some(contents) = diag_buffer.contents() { + match diag_buffer.create_file() { + // Reported only once the bytes are actually down. Discarding the write result + // logged success over a partial file, which is the same "looks fine, is not" + // shape this change is about. + Ok((diag_path, mut file)) => match file.write_all(contents.as_bytes()) { + Ok(()) => { + log::info!("event=guid_diag_written path=\"{}\"", diag_path.display()) + } + Err(error) => log::warn!( + "event=guid_diag_write_failed path=\"{}\" error=\"{error}\"", + diag_path.display() + ), + }, + Err(error) => log::warn!("event=guid_diag_write_failed error=\"{error}\""), + } } } @@ -1397,6 +1515,154 @@ fn download_signal_rank(state: DownloadSignalState) -> u8 { } } +#[cfg(test)] +mod guid_diag_tests { + use super::GuidDiagLog; + use std::fmt::Write as _; + use std::fs; + + /// Serializes the env-var mutation these tests share; cargo runs them on threads. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Restores the variable on drop, so a failing assertion cannot leak the mutation. + /// + /// A plain restore after `body()` is skipped while unwinding, which would leave the trace + /// enabled for every test that ran afterwards and turn one real failure into a cascade of + /// unrelated ones. + struct EnvRestore { + previous: Option, + _lock: std::sync::MutexGuard<'static, ()>, + } + + impl Drop for EnvRestore { + fn drop(&mut self) { + match self.previous.take() { + Some(previous) => std::env::set_var("CMTRACE_INTUNE_GUID_DIAG", previous), + None => std::env::remove_var("CMTRACE_INTUNE_GUID_DIAG"), + } + } + } + + fn with_var(value: Option<&str>, body: impl FnOnce() -> T) -> T { + let lock = ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let restore = EnvRestore { + previous: std::env::var_os("CMTRACE_INTUNE_GUID_DIAG"), + _lock: lock, + }; + match value { + Some(value) => std::env::set_var("CMTRACE_INTUNE_GUID_DIAG", value), + None => std::env::remove_var("CMTRACE_INTUNE_GUID_DIAG"), + } + let outcome = body(); + drop(restore); + outcome + } + + #[test] + fn collects_nothing_when_the_trace_was_not_asked_for() { + // The trace carries an organisation's app inventory. Unset means nothing is retained, so + // there is nothing for the caller to write to disk. + with_var(None, || { + let mut log = GuidDiagLog::from_env(); + let name = "Contoso Payroll"; + let _ = writeln!(log, "guid=abc name=\"{name}\""); + assert!(log.contents().is_none(), "no trace may be retained"); + }); + } + + #[test] + fn an_empty_value_does_not_enable_it() { + // CMTRACE_INTUNE_GUID_DIAG= in a shell profile is not a request for the trace. + with_var(Some(""), || { + let mut log = GuidDiagLog::from_env(); + let name = "Contoso Payroll"; + let _ = writeln!(log, "guid=abc name=\"{name}\""); + assert!(log.contents().is_none()); + }); + } + + #[test] + fn collects_the_trace_when_asked_for() { + with_var(Some("1"), || { + let mut log = GuidDiagLog::from_env(); + let name = "Contoso Payroll"; + let _ = writeln!(log, "guid=abc name=\"{name}\""); + let trace = log.contents().expect("the trace was requested"); + assert!(trace.contains("Contoso Payroll"), "got {trace:?}"); + }); + } + + #[test] + fn an_enabled_trace_that_collected_nothing_reports_no_contents() { + // Deliberately a statement about the sink, not about the file. In the real analysis the + // pipeline summary always writes once the trace is on, so an enabled run does produce a + // file; claiming otherwise here would be a test passing for a reason its name denies. + with_var(Some("1"), || { + let log = GuidDiagLog::from_env(); + assert!(log.contents().is_none()); + }); + } + + #[cfg(unix)] + #[test] + fn the_trace_file_is_readable_only_by_its_owner() { + // The temp directory is shared and the default umask leaves new files world-readable. + // This trace holds the app inventory the whole change exists to stop leaking, so on the + // one path that does write it, nobody else on the machine may read it. + use std::os::unix::fs::PermissionsExt; + + with_var(Some("1"), || { + let log = GuidDiagLog::from_env(); + let (path, _file) = log.create_file().expect("creates"); + let mode = fs::metadata(&path).expect("stat").permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "got {:o}", mode & 0o777); + let _ = fs::remove_file(&path); + }); + } + + #[test] + fn every_trace_path_is_distinct_even_within_one_clock_tick() { + // The stamp is held fixed, which is the whole point: two analyses in one process can land + // inside a single tick, and a clock can move backwards. Either repeats a timestamp, and + // because the file is opened exclusively a repeat does not overwrite anything but does + // lose the trace that was asked for. Calling trace_path in a loop would pass whether or + // not the sequence existed, because successive clock reads happen to differ. + let paths: std::collections::HashSet<_> = + (0..256).map(|_| GuidDiagLog::trace_path_for(42)).collect(); + assert_eq!(paths.len(), 256, "one clock tick must still give distinct paths"); + } + + #[test] + fn the_trace_file_refuses_to_write_through_something_that_exists() { + // The system temp directory is world-writable, so a fixed name there is the classic + // clobber and symlink-follow target, and this trace is opt-in precisely because its + // contents are sensitive. + with_var(Some("1"), || { + let log = GuidDiagLog::from_env(); + let (first_path, _first) = log.create_file().expect("creates"); + assert!(first_path.exists()); + + // A second call must not reuse the name, so no analysis can truncate another's trace. + let (second_path, _second) = log.create_file().expect("creates"); + assert_ne!(first_path, second_path); + + // The helper itself is asked to open a path that already exists. Asserting on + // OpenOptions directly would have proved only that the standard library works, and + // would still pass if the helper dropped create_new. + let refused = GuidDiagLog::create_file_at(first_path.clone()); + assert!( + refused.is_err(), + "create_file_at must refuse a path that already exists" + ); + + let _ = fs::remove_file(&first_path); + let _ = fs::remove_file(&second_path); + }); + } +} + #[cfg(test)] mod tests { use super::{build_summary, build_timestamp_bounds, finalize_coverage, CoverageAccumulator};