fix(intune): stop writing an app inventory to TEMP on every analysis run - #563
Merged
Conversation
Every Intune analysis wrote %TEMP%/cmtrace-guid-diag.log, unconditionally. No operator asked for it, nothing cleaned it up, and it was never redacted. What it carried is an organisation's app inventory: every GUID registry entry with its application name, every event name enriched or missed with its GUID, and every download name with its content id and size. That is the same class of identifier the Intune lanes mask everywhere else, written in cleartext to a directory other users on the machine can read. This is developer instrumentation for diagnosing GUID enrichment. The summary an operator needs already goes to the application log; only the verbose per-entry trace went to the file. It is now collected only when CMTRACE_INTUNE_GUID_DIAG is set to a non-empty value, and nothing reaches disk otherwise. Implemented as a sink that implements fmt::Write and discards when off, so the eleven writeln! call sites are unchanged and stay interleaved with the enrichment logic they describe. An enabled-but-empty run writes no file either, rather than leaving an empty one in TEMP. Mutation-checked: making the sink always collect fails two of the four tests. This is the one item in #549's "related, same root cause" list that needs no boundary ruling from ADR-004. It is not an export that should have been projected; it is a write nobody requested. The export-boundary items in #549 and #556 still wait on #550. Refs #549. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
bug
📝 WalkthroughWalkthroughThe Intune command adds opt-in GUID diagnostic tracing through ChangesIntune GUID diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents Intune analysis from writing a verbose GUID-enrichment diagnostic log to a predictable %TEMP%/cmtrace-guid-diag.log file on every run by default, reducing unintended disclosure of sensitive app inventory data. The diagnostic trace is now collected and written only when explicitly enabled via CMTRACE_INTUNE_GUID_DIAG, while keeping the existing writeln! instrumentation call sites intact.
Changes:
- Added a
fmt::Writesink (GuidDiagLog) that conditionally collects the verbose trace only whenCMTRACE_INTUNE_GUID_DIAGis set to a non-empty value. - Gated the temp-file write behind
GuidDiagLog::contents()so enabled-but-empty runs do not leave an empty file behind. - Added unit tests covering env-var gating behavior for the trace collection.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/commands/intune.rs`:
- Around line 390-395: Update the pipeline summary logic around GuidDiagLog
contents so the temporary diagnostic file is created and written only when prior
GUID diagnostic content exists; keep the pipeline summary append inside that
same content check, and leave log::info! outside the conditional.
- Around line 1450-1462: Update with_var so environment restoration is handled
by an RAII guard whose Drop implementation restores the saved
CMTRACE_INTUNE_GUID_DIAG value, including when body() unwinds; create the guard
while ENV_LOCK remains held so Drop restores the variable before the mutex is
released, and remove the manual post-body restoration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1142ad7c-9510-4c2b-a85d-4a724b505940
📒 Files selected for processing (1)
src-tauri/src/commands/intune.rs
…bbered Review findings on the gate itself. The trace went to a fixed name in the system temp directory opened with File::create, which truncates whatever is already there. That directory is world-writable, so a guessable name is the classic clobber and symlink-follow target, and this trace is opt-in precisely because its contents are sensitive. The name now carries the process id and a nanosecond stamp, and the file is opened with create_new, so two analyses cannot overwrite each other and the call fails rather than writing through something an unprivileged user planted. A failure is logged rather than silently swallowed. The test helper restored the environment variable after the body ran, which is skipped while unwinding. One failing assertion would have left the trace enabled for every test after it, turning a single real failure into a cascade of unrelated ones. It is an RAII guard now, so Drop restores during unwinding too. Also renamed a test that claimed more than it checked. It asserts the sink holds nothing, which is true, but its name implied an enabled run writes no file, and in the real analysis the pipeline summary always writes once the trace is on. A test passing for a reason its name denies is worse than no test. Mutation-checked: putting the fixed name and File::create back fails one test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/commands/intune.rs`:
- Around line 1571-1576: Update the test around GuidDiagLog::create_file to
pre-create the target path, invoke GuidDiagLog::create_file for that existing
path, and assert the helper returns an error. Remove the direct fs::OpenOptions
assertion so the test verifies the helper’s exclusive-open behavior.
- Around line 414-419: Update the Ok branch of diag_buffer.create_file in the
GUID diagnostic-writing flow to handle file.write_all(contents.as_bytes())
explicitly. Log guid_diag_written only when the write succeeds, and log
guid_diag_write_failed with the write error when it fails; preserve the existing
create_file error handling.
- Around line 89-108: Update the OpenOptions construction in create_file to set
Unix trace-file permissions to 0o600 using std::os::unix::fs::OpenOptionsExt
behind #[cfg(unix)]. Preserve create_new(true) and the existing behavior on
non-Unix platforms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b48fdc5d-f4f8-44f0-bdb1-fcb6eaa19e99
📒 Files selected for processing (1)
src-tauri/src/commands/intune.rs
Owner
Author
|
@coderabbitai review |
|
…r not the stdlib Three more review findings, and the first is the one that mattered. The trace file was created with the default umask, which leaves it world-readable, in a shared temp directory. That is the app inventory this whole change exists to stop leaking, sitting readable by every local user on the one path that does write it. It is opened 0o600 on Unix now. Windows inherits the per-user ACL of the profile temp directory. The write result was discarded, so a failed or partial write still logged guid_diag_written. Success is reported only once the bytes are down, which is the same "looks fine, is not" shape the rest of this change is about. The exclusive-open test asserted on fs::OpenOptions directly, so it proved the standard library works and would still have passed if the helper stopped using create_new. Path generation is split from opening so the helper itself can be handed a path that already exists, which is the case that matters. Mutation-checked separately: dropping the mode fails one test, dropping create_new fails another. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/commands/intune.rs`:
- Around line 100-110: Update trace_path to guarantee unique paths even when
SystemTime repeats or moves backward, preferably by adding a process-local
atomic sequence suffix; alternatively, make the caller retry create_new(true)
with a newly generated path after AlreadyExists. Ensure the diagnostic trace
still proceeds after a collision and add a test covering collision recovery.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69bb404a-4240-4106-b20b-59e605088fa2
📒 Files selected for processing (1)
src-tauri/src/commands/intune.rs
Owner
Author
|
@coderabbitai review |
|
…t matters Two analyses in one process can land inside a single clock tick, and a clock can move backwards. Either repeats the timestamp, and because the file is opened exclusively a repeat does not overwrite anything but does lose the trace the operator asked for. A process-local atomic sequence makes the path distinct by construction rather than by luck. The first version of the test called trace_path in a loop and passed with the sequence removed, because successive clock reads on this machine differ anyway. It proved nothing about the thing it was named for. Naming is now split from the clock so the stamp can be held fixed, which is the only way to exercise the collision case; with the sequence removed it fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/src/commands/intune.rs (2)
1619-1620: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest owner-only permissions, not an exact mode.
A restrictive umask can change
0o600to0o400. Assert that group and other permission bits are clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/commands/intune.rs` around lines 1619 - 1620, Update the permission assertion near the mode calculation to verify that group and other permission bits are unset, rather than requiring an exact 0o600 mode. Preserve the owner-only requirement while allowing restrictive umasks to produce modes such as 0o400.Source: Coding guidelines
96-152: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove diagnostic file I/O to Tokio.
analyze_intune_logsusesspawn_blocking, so this write does not block a Tokio worker. However,create_file_atandwrite_allstill use synchronousstd::fsandstd::ioAPIs, which violates the repository requirement for Tokio file I/O.Return the diagnostic contents from the blocking analysis. Write them after the task completes with
tokio::fs::OpenOptionsandtokio::io::AsyncWriteExt. Preservecreate_new(true)and Unix mode0o600. The configured Tokiofsandio-utilfeatures support these APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/commands/intune.rs` around lines 96 - 152, Update the diagnostic flow around analyze_intune_logs to return the generated contents from spawn_blocking instead of performing file I/O there. Replace the synchronous create_file/create_file_at and write_all path with post-task Tokio file operations using tokio::fs::OpenOptions and tokio::io::AsyncWriteExt, while preserving create_new(true), the Unix 0o600 mode, and the diagnostic filename generation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src-tauri/src/commands/intune.rs`:
- Around line 1619-1620: Update the permission assertion near the mode
calculation to verify that group and other permission bits are unset, rather
than requiring an exact 0o600 mode. Preserve the owner-only requirement while
allowing restrictive umasks to produce modes such as 0o400.
- Around line 96-152: Update the diagnostic flow around analyze_intune_logs to
return the generated contents from spawn_blocking instead of performing file I/O
there. Replace the synchronous create_file/create_file_at and write_all path
with post-task Tokio file operations using tokio::fs::OpenOptions and
tokio::io::AsyncWriteExt, while preserving create_new(true), the Unix 0o600
mode, and the diagnostic filename generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da6c60f4-8987-4f92-ac68-dd7d04a7334d
📒 Files selected for processing (1)
src-tauri/src/commands/intune.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #549 — the one item in its "related, same root cause" list that needs no ADR-004 ruling.
The leak
Every Intune analysis wrote
%TEMP%/cmtrace-guid-diag.log. Unconditionally, with no operator action, never cleaned up, never redacted.What it carried:
That is an organisation's app inventory — the same class of identifier the Intune lanes mask everywhere else — in cleartext, in a directory other users on the machine can read.
Why this one does not wait on the ADR
#549 and #556 are export-boundary questions: a value that should be projected reaches a user-chosen file. Ruling 1 in #550 decides where that projection binds, so fixing them now would front-run the decision.
This is not that. It is a write nobody requested. No ruling in the draft would sanction it, and the fix is not "project it" but "do not write it".
The fix
Developer instrumentation for diagnosing GUID enrichment. The summary an operator actually needs already goes to the application log via
log::info!; only the verbose per-entry trace went to the file. It is now collected only whenCMTRACE_INTUNE_GUID_DIAGis set to a non-empty value.Implemented as a sink implementing
fmt::Writethat discards when off, so the elevenwriteln!call sites are untouched and stay interleaved with the enrichment logic they describe — the writes are woven throughevent.name = enrichedand the counters, so deleting blocks was not an option.An empty env value does not enable it (a stray
CMTRACE_INTUNE_GUID_DIAG=in a shell profile is not a request), and an enabled run that logged nothing writes no file rather than leaving an empty one in TEMP.Verification
Mutation-checked rather than assumed: making the sink always collect fails two of the four tests.
src-taurisuite and clippy-D warningsclean. One file, no formatting churn.Summary by CodeRabbit
New Features
CMTRACE_INTUNE_GUID_DIAGsetting.Bug Fixes
Tests