From 3b4488fbb96662d4b123cfc561e5ee810e1b4dd6 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Mon, 9 Mar 2026 14:17:21 +0100 Subject: [PATCH] Fix telemetry PII concerns: sanitize exceptions, project paths, and custom names - Wrap raw Exception in SanitizedException before passing to VS FaultEvent to strip file paths and usernames from messages and stack traces - Emit only the file name (not full directory path) for BuildTelemetry.ProjectPath - Hash custom build target names using SHA-256; preserve well-known targets - Sanitize BuildCheckTelemetry.ExceptionMessage using CrashTelemetry.TruncateMessage - Extend path-redaction regex to match UNC paths (\\\\server\\share\\...) - Apply general path redaction in SanitizeFilePathsInText for non-stack-frame lines - Add tests for UNC path redaction, non-stack-frame path sanitization, ProjectPath file-name-only emission, and custom target hashing --- .../BackEnd/KnownTelemetry_Tests.cs | 96 ++++++++++++++++++- .../CrashTelemetry_Tests.cs | 53 +++++++++- .../Telemetry/BuildCheckTelemetry.cs | 6 +- src/Framework/Telemetry/BuildTelemetry.cs | 67 ++++++++++++- src/Framework/Telemetry/CrashTelemetry.cs | 29 ++++-- .../Telemetry/CrashTelemetryRecorder.cs | 23 ++++- 6 files changed, 252 insertions(+), 22 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/KnownTelemetry_Tests.cs b/src/Build.UnitTests/BackEnd/KnownTelemetry_Tests.cs index 41afe33d841..adb04753648 100644 --- a/src/Build.UnitTests/BackEnd/KnownTelemetry_Tests.cs +++ b/src/Build.UnitTests/BackEnd/KnownTelemetry_Tests.cs @@ -9,6 +9,7 @@ using Xunit; using static Microsoft.Build.BackEnd.Logging.BuildErrorTelemetryTracker; using static Microsoft.Build.Framework.Telemetry.BuildInsights; +using static Microsoft.Build.Framework.Telemetry.TelemetryDataUtils; namespace Microsoft.Build.UnitTests.Telemetry; @@ -72,11 +73,11 @@ public void BuildTelemetryCreateProperProperties() buildTelemetry.BuildEngineHost = "Host description"; buildTelemetry.InitialMSBuildServerState = "hot"; buildTelemetry.InnerStartAt = innerStartAt; - buildTelemetry.ProjectPath = @"C:\\dev\\theProject"; + buildTelemetry.ProjectPath = "C:/dev/theProject"; buildTelemetry.ServerFallbackReason = "busy"; buildTelemetry.StartAt = startAt; buildTelemetry.BuildSuccess = true; - buildTelemetry.BuildTarget = "clean"; + buildTelemetry.BuildTarget = "Clean"; buildTelemetry.BuildEngineVersion = new Version(1, 2, 3, 4); buildTelemetry.BuildCheckEnabled = true; buildTelemetry.MultiThreadedModeEnabled = false; @@ -90,10 +91,10 @@ public void BuildTelemetryCreateProperProperties() properties["BuildEngineFrameworkName"].ShouldBe("new .NET"); properties["BuildEngineHost"].ShouldBe("Host description"); properties["InitialMSBuildServerState"].ShouldBe("hot"); - properties["ProjectPath"].ShouldBe(@"C:\\dev\\theProject"); + properties["ProjectPath"].ShouldBe("theProject"); properties["ServerFallbackReason"].ShouldBe("busy"); properties["BuildSuccess"].ShouldBe("True"); - properties["BuildTarget"].ShouldBe("clean"); + properties["BuildTarget"].ShouldBe("Clean"); properties["BuildEngineVersion"].ShouldBe("1.2.3.4"); properties["BuildCheckEnabled"].ShouldBe("True"); properties["MultiThreadedModeEnabled"].ShouldBe("False"); @@ -204,4 +205,91 @@ public void BuildTelemetryActivityPropertiesIncludesFailureData() errorCounts.ShouldNotBeNull(); errorCounts.Task.ShouldBe(10); } + + [Fact] + public void BuildTelemetryProjectPathEmitsOnlyFileName() + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.ProjectPath = "C:/Users/useralias/repos/MyProject/MyProject.csproj"; + buildTelemetry.StartAt = DateTime.UtcNow; + buildTelemetry.FinishedAt = DateTime.UtcNow; + + var properties = buildTelemetry.GetProperties(); + + // Should only contain the file name, not the directory path + properties["ProjectPath"].ShouldBe("MyProject.csproj"); + properties["ProjectPath"].ShouldNotContain("useralias"); + properties["ProjectPath"].ShouldNotContain("Users"); + } + + [Fact] + public void BuildTelemetryBuildTargetHashesCustomTargets() + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.BuildTarget = "MySecretCustomTarget"; + buildTelemetry.StartAt = DateTime.UtcNow; + buildTelemetry.FinishedAt = DateTime.UtcNow; + + var properties = buildTelemetry.GetProperties(); + + // Custom target name should be hashed + properties["BuildTarget"].ShouldNotBe("MySecretCustomTarget"); + properties["BuildTarget"].ShouldBe(GetHashed("MySecretCustomTarget")); + } + + [Fact] + public void BuildTelemetryBuildTargetPreservesKnownTargets() + { + string[] knownTargets = { "Build", "Clean", "Rebuild", "Restore", "Pack", "Publish", "Test" }; + + foreach (string target in knownTargets) + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.BuildTarget = target; + buildTelemetry.StartAt = DateTime.UtcNow; + buildTelemetry.FinishedAt = DateTime.UtcNow; + + var properties = buildTelemetry.GetProperties(); + properties["BuildTarget"].ShouldBe(target, $"Known target '{target}' should not be hashed"); + } + } + + [Fact] + public void BuildTelemetryActivityPropertiesHashCustomTarget() + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.BuildTarget = "InternalCustomTarget"; + + var activityProperties = buildTelemetry.GetActivityProperties(); + + activityProperties["BuildTarget"].ShouldBe(GetHashed("InternalCustomTarget")); + } + + [Fact] + public void BuildTelemetryBuildTargetHandlesCommaSeparatedTargets() + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.BuildTarget = "Build,Clean"; + buildTelemetry.StartAt = DateTime.UtcNow; + buildTelemetry.FinishedAt = DateTime.UtcNow; + + var properties = buildTelemetry.GetProperties(); + + // Both known targets should be preserved individually, not hashed as a whole string + properties["BuildTarget"].ShouldBe("Build,Clean"); + } + + [Fact] + public void BuildTelemetryBuildTargetHashesMixedTargets() + { + BuildTelemetry buildTelemetry = new BuildTelemetry(); + buildTelemetry.BuildTarget = "Build,MyCustomTarget,Restore"; + buildTelemetry.StartAt = DateTime.UtcNow; + buildTelemetry.FinishedAt = DateTime.UtcNow; + + var properties = buildTelemetry.GetProperties(); + + // Known targets preserved, custom target hashed + properties["BuildTarget"].ShouldBe($"Build,{GetHashed("MyCustomTarget")},Restore"); + } } diff --git a/src/Framework.UnitTests/CrashTelemetry_Tests.cs b/src/Framework.UnitTests/CrashTelemetry_Tests.cs index 3f003db1625..db24cf7bff1 100644 --- a/src/Framework.UnitTests/CrashTelemetry_Tests.cs +++ b/src/Framework.UnitTests/CrashTelemetry_Tests.cs @@ -497,11 +497,11 @@ public void TruncateMessage_TruncatesLongMessages() [Fact] public void TruncateMessage_RedactsWindowsPaths() { - string message = @"C:\Users\johndoe\src\project.csproj unexpectedly not a rooted path"; + string message = @"C:\Users\useralias\src\project.csproj unexpectedly not a rooted path"; string? result = CrashTelemetry.TruncateMessage(message); result.ShouldNotBeNull(); - result.ShouldNotContain("johndoe"); + result.ShouldNotContain("useralias"); result.ShouldNotContain(@"C:\Users"); result.ShouldContain(""); result.ShouldContain("unexpectedly not a rooted path"); @@ -510,11 +510,11 @@ public void TruncateMessage_RedactsWindowsPaths() [Fact] public void TruncateMessage_RedactsUnixPaths() { - string message = @"/home/johndoe/src/project.csproj unexpectedly not a rooted path"; + string message = @"/home/useralias/src/project.csproj unexpectedly not a rooted path"; string? result = CrashTelemetry.TruncateMessage(message); result.ShouldNotBeNull(); - result.ShouldNotContain("johndoe"); + result.ShouldNotContain("useralias"); result.ShouldContain(""); } @@ -762,6 +762,51 @@ private static Exception CreateExceptionWithStack(string fakeStack) return new ExceptionWithFakeStack(fakeStack); } + [Fact] + public void TruncateMessage_RedactsUncPaths() + { + string message = @"Could not access \\server\share\builds\project.sln during build"; + string? result = CrashTelemetry.TruncateMessage(message); + + result.ShouldNotBeNull(); + result.ShouldNotContain("server"); + result.ShouldNotContain("share"); + result.ShouldContain(""); + } + + [Fact] + public void TruncateMessage_RedactsPathsWithSpaces() + { + // Paths containing spaces are partially redacted — each non-space segment + // after a recognized root prefix is replaced. The PII-relevant parts + // (username, project name) are in non-space segments and will be redacted. + string message = @"Could not find C:\Users\useralias\my-project\bin\app.exe"; + string? result = CrashTelemetry.TruncateMessage(message); + + result.ShouldNotBeNull(); + result.ShouldNotContain("useralias"); + result.ShouldNotContain("my-project"); + result.ShouldContain(""); + } + + [Fact] + public void SanitizeFilePathsInText_RedactsPathsInNonStackFrameLines() + { + string input = "System.IO.FileNotFoundException: Could not find C:\\Users\\useralias\\project.csproj"; + string result = CrashTelemetry.SanitizeFilePathsInText(input); + result.ShouldNotContain("useralias"); + result.ShouldContain(""); + } + + [Fact] + public void SanitizeFilePathsInText_RedactsUncPathsInNonStackFrameLines() + { + string input = "Failed to load assembly from \\\\server\\share\\assembly.dll"; + string result = CrashTelemetry.SanitizeFilePathsInText(input); + result.ShouldNotContain("server"); + result.ShouldContain(""); + } + private sealed class ExceptionWithFakeStack : Exception { private readonly string _stack; diff --git a/src/Framework/Telemetry/BuildCheckTelemetry.cs b/src/Framework/Telemetry/BuildCheckTelemetry.cs index 8555a1b33e8..ced5b04316c 100644 --- a/src/Framework/Telemetry/BuildCheckTelemetry.cs +++ b/src/Framework/Telemetry/BuildCheckTelemetry.cs @@ -30,9 +30,11 @@ internal class BuildCheckTelemetry { properties["ExceptionType"] = exceptionType; } - if (exception.Message != null) + + string? sanitizedMessage = CrashTelemetry.TruncateMessage(exception.Message); + if (sanitizedMessage != null) { - properties["ExceptionMessage"] = exception.Message; + properties["ExceptionMessage"] = sanitizedMessage; } return (FailedAcquisitionEventName, properties); diff --git a/src/Framework/Telemetry/BuildTelemetry.cs b/src/Framework/Telemetry/BuildTelemetry.cs index f035265c18a..34a95a589d7 100644 --- a/src/Framework/Telemetry/BuildTelemetry.cs +++ b/src/Framework/Telemetry/BuildTelemetry.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Globalization; +using System.IO; using System.Runtime.CompilerServices; using static Microsoft.Build.Framework.Telemetry.BuildInsights; @@ -72,9 +73,41 @@ internal class BuildTelemetry : TelemetryBase, IActivityTelemetryDataHolder /// /// Path to project file. + /// Only the file name (no directory) is emitted in telemetry to avoid leaking PII + /// (usernames, directory structure) that are commonly embedded in full paths. /// public string? ProjectPath { get; set; } + /// + /// Well-known target names that are safe to emit in cleartext. + /// Custom target names could reveal project internals and are hashed. + /// + private static readonly HashSet KnownTargetNames = new(StringComparer.OrdinalIgnoreCase) + { + "Build", + "Clean", + "Rebuild", + "Restore", + "Pack", + "Publish", + "Test", + "VSTest", + "Run", + "GetTargetFrameworks", + "GetTargetFrameworksWithPlatformForSingleTargetFramework", + "GetReferenceNearestTargetFrameworkTask", + "GetTargetPath", + "GetNativeManifest", + "ResolveAssemblyReferences", + "ResolveProjectReferences", + "CoreCompile", + "Compile", + "PrepareForBuild", + "GenerateBuildDependencyFile", + "GenerateBindingRedirects", + "GenerateRuntimeConfigurationFiles", + }; + /// /// Host in which MSBuild build was executed. /// For example: "VS", "VSCode", "Azure DevOps", "GitHub Action", "CLI", ... @@ -137,7 +170,7 @@ public Dictionary GetActivityProperties() AddIfNotNull(BuildEngineHost); AddIfNotNull(BuildSuccess); - AddIfNotNull(BuildTarget); + AddIfNotNull(SanitizeBuildTarget(BuildTarget), nameof(BuildTarget)); AddIfNotNull(BuildEngineVersion); AddIfNotNull(BuildCheckEnabled); AddIfNotNull(MultiThreadedModeEnabled); @@ -165,9 +198,9 @@ public override IDictionary GetProperties() AddIfNotNull(BuildEngineFrameworkName); AddIfNotNull(BuildEngineHost); AddIfNotNull(InitialMSBuildServerState); - AddIfNotNull(ProjectPath); + AddIfNotNull(ProjectPath != null ? Path.GetFileName(ProjectPath) : null, nameof(ProjectPath)); AddIfNotNull(ServerFallbackReason); - AddIfNotNull(BuildTarget); + AddIfNotNull(SanitizeBuildTarget(BuildTarget), nameof(BuildTarget)); AddIfNotNull(BuildEngineVersion?.ToString(), nameof(BuildEngineVersion)); AddIfNotNull(BuildSuccess?.ToString(), nameof(BuildSuccess)); AddIfNotNull(BuildCheckEnabled?.ToString(), nameof(BuildCheckEnabled)); @@ -200,5 +233,33 @@ void AddIfNotNull(string? value, [CallerArgumentExpression(nameof(value))] strin } } } + + /// + /// Returns the build target name if it is a well-known target, otherwise returns a SHA-256 hash. + /// This prevents custom target names (which could reveal proprietary project details) from being + /// sent in cleartext telemetry. + /// BuildTarget may be a comma-separated list of target names (e.g., "Build,Clean"), + /// so each target is sanitized individually. + /// + internal static string? SanitizeBuildTarget(string? buildTarget) + { + if (buildTarget is null) + { + return null; + } + + // BuildTarget can be a comma-separated list (set via string.Join(",", targetNames)). + // Split, sanitize each target individually, and rejoin. + string[] targets = buildTarget.Split(','); + for (int i = 0; i < targets.Length; i++) + { + string target = targets[i].Trim(); + targets[i] = KnownTargetNames.Contains(target) + ? target + : TelemetryDataUtils.GetHashed(target); + } + + return string.Join(",", targets); + } } } diff --git a/src/Framework/Telemetry/CrashTelemetry.cs b/src/Framework/Telemetry/CrashTelemetry.cs index 32b8998ed7a..d01e6c4865f 100644 --- a/src/Framework/Telemetry/CrashTelemetry.cs +++ b/src/Framework/Telemetry/CrashTelemetry.cs @@ -6,6 +6,7 @@ using System.Runtime.CompilerServices; using System.Security.Cryptography; using System.Text; +using System.Text.RegularExpressions; namespace Microsoft.Build.Framework.Telemetry; @@ -159,6 +160,15 @@ internal class CrashTelemetry : TelemetryBase, IActivityTelemetryDataHolder /// internal const int MaxStackTraceLength = 4096; + /// + /// Compiled regex pattern for matching file/directory paths that may contain PII. + /// Matches Windows absolute paths (X:\...), UNC paths (\\server\share\...), and Unix paths (/...). + /// Shared between and . + /// + private static readonly Regex FilePathPattern = new( + @"(?:[A-Za-z]:\\|\\\\|/)(?:[^\s""'<>|*?\r\n]+)", + RegexOptions.Compiled); + /// /// A prefix of the exception message, truncated and sanitized to avoid PII. /// Particularly useful for InternalErrorException where the message text @@ -639,12 +649,8 @@ internal static CrashOriginKind ClassifyOrigin(string? originNamespace) message = message.Substring(internalErrorPrefix.Length); } - // Redact file/directory paths that may contain PII (e.g., C:\Users\johndoe\...). - // Matches Windows paths (X:\...) and Unix paths (/home/...). - message = System.Text.RegularExpressions.Regex.Replace( - message, - @"(?:[A-Za-z]:\\|/)(?:[^\s""'<>|*?]+)", - ""); + // Redact file/directory paths that may contain PII (e.g., C:\Users\useralias\...). + message = FilePathPattern.Replace(message, ""); const int maxLength = 256; return message.Length <= maxLength ? message : message.Substring(0, maxLength); @@ -789,8 +795,9 @@ private static string SanitizeStackFrame(string frame) } /// - /// Sanitizes file paths embedded in multi-line text (e.g., exception dumps) to remove PII. - /// Each line that looks like a stack frame gets its file path redacted. + /// Sanitizes file paths embedded in multi-line text (e.g., stack traces, exception dumps) to remove PII. + /// Handles both standard .NET stack frame patterns (" in path:line N") and + /// bare file paths (Windows absolute, UNC, Unix absolute) that may appear in exception messages. /// internal static string SanitizeFilePathsInText(string text) { @@ -819,6 +826,12 @@ internal static string SanitizeFilePathsInText(string text) lines[i] = line.Substring(0, inIndex + inToken.Length) + ""; } } + else + { + // For non-stack-frame lines (e.g., exception messages embedded in ToString()), + // apply general path redaction to catch paths that appear outside " in " patterns. + lines[i] = FilePathPattern.Replace(line, ""); + } } return string.Join("\n", lines); diff --git a/src/Framework/Telemetry/CrashTelemetryRecorder.cs b/src/Framework/Telemetry/CrashTelemetryRecorder.cs index dc1a16ba9f5..ac0cae36bf8 100644 --- a/src/Framework/Telemetry/CrashTelemetryRecorder.cs +++ b/src/Framework/Telemetry/CrashTelemetryRecorder.cs @@ -142,7 +142,8 @@ private static void PostFaultEvent(CrashTelemetry crashTelemetry) string eventName = $"{TelemetryConstants.EventPrefix}{TelemetryConstants.Crash}"; string description = $"{crashTelemetry.ExitType}: {crashTelemetry.ExceptionType}"; - var faultEvent = new FaultEvent(eventName, description, crashTelemetry.Exception); + var sanitizedException = new SanitizedException(crashTelemetry.Exception!); + var faultEvent = new FaultEvent(eventName, description, sanitizedException); faultEvent.Properties[$"{TelemetryConstants.PropertyPrefix}ExitType"] = crashTelemetry.ExitType.ToString(); faultEvent.Properties[$"{TelemetryConstants.PropertyPrefix}CrashOrigin"] = crashTelemetry.CrashOrigin.ToString(); @@ -166,6 +167,26 @@ private static void PostFaultEvent(CrashTelemetry crashTelemetry) #endif } + /// + /// Exception wrapper that sanitizes message and stack trace to remove PII + /// before being passed to VS Telemetry's FaultEvent. + /// + internal sealed class SanitizedException : Exception + { + private readonly string? _sanitizedStackTrace; + + public SanitizedException(Exception original) + : base(CrashTelemetry.TruncateMessage(original.Message) ?? original.GetType().FullName, + original.InnerException is not null ? new SanitizedException(original.InnerException) : null) + { + _sanitizedStackTrace = original.StackTrace is not null + ? CrashTelemetry.SanitizeFilePathsInText(original.StackTrace) + : null; + } + + public override string? StackTrace => _sanitizedStackTrace; + } + private static CrashTelemetry CreateCrashTelemetry( Exception exception, CrashExitType exitType,