fix(intune): stop writing an app inventory to TEMP on every analysis run

src-tauri/src/commands/intune.rs

@@ -56,6 +56,112 @@ struct IntuneAnalysisProgressPayload {
     total_files: Option<usize>,
 }
 
+/// 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<String>);
+
+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<std::ffi::OsString>,
+        _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<T>(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};