From 7ee174aead34ebef26d9e96fab2f9dc169828c31 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 28 Aug 2026 15:52:59 -0700 Subject: [PATCH] Do not trace raw credential output TryGetCredential and TryGetCertificatePassword added the raw stdout of "git credential fill" to the activity metadata when the parse of the output failed. That output is a list of "key=value" lines and includes the plaintext secret, so a failed parse wrote the secret to the log and to telemetry. Trace only the key names that the credential helper returned. The key names keep the diagnostic value of the trace, and the values never leave the process. Add unit tests that assert a sentinel secret does not appear in the activity metadata for either method. MockTracer now records the metadata that is passed to Stop so tests can inspect it. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitProcess.cs | 32 +++++++- .../Common/Git/GitProcessCredentialTests.cs | 79 +++++++++++++++++++ GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs | 10 +++ 3 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 03cc27b417..b2f3612679 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -306,7 +306,8 @@ public virtual bool TryGetCertificatePassword( if (!success) { - metadata.Add("Output", gitCredentialOutput.Output); + // Never trace the raw output: it can contain the secret itself. + metadata.Add("OutputKeys", GetCredentialOutputKeys(gitCredentialOutput.Output)); } activity.Stop(metadata); @@ -370,7 +371,8 @@ public virtual bool TryGetCredential( metadata.Add("Success", success); if (!success) { - metadata.Add("Output", gitCredentialOutput.Output); + // Never trace the raw output: it can contain the secret itself. + metadata.Add("OutputKeys", GetCredentialOutputKeys(gitCredentialOutput.Output)); } activity.Stop(metadata); @@ -1075,6 +1077,32 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + /// + /// Summarizes the output of "git credential fill" for diagnostics. + /// The output is a list of "key=value" lines that can include the + /// plaintext secret, so only the key names are returned. The values + /// must never reach telemetry or the log. + /// + private static string GetCredentialOutputKeys(string credentialOutput) + { + if (string.IsNullOrEmpty(credentialOutput)) + { + return string.Empty; + } + + IEnumerable keys = credentialOutput + .Split('\n') + .Select(line => line.Trim('\r')) + .Where(line => !string.IsNullOrWhiteSpace(line)) + .Select(line => + { + int separatorIndex = line.IndexOf('='); + return separatorIndex > 0 ? line.Substring(0, separatorIndex) : ""; + }); + + return string.Join(",", keys); + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; diff --git a/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs new file mode 100644 index 0000000000..550bc6e14d --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs @@ -0,0 +1,79 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; +using NUnit.Framework; +using System.Linq; + +namespace GVFS.UnitTests.Common.Git +{ + [TestFixture] + public class GitProcessCredentialTests + { + private const string SecretValue = "S3cretSentinelValueThatMustNotBeTraced"; + private const string AzureDevOpsUseHttpPathString = "-c credential.\"https://dev.azure.com\".useHttpPath=true"; + + [TestCase] + public void TryGetCredentialDoesNotTraceSecretWhenParseFails() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + + // The secret is on the last line and has no terminating newline, so the parse fails. + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result( + "protocol=https\nhost=example.com\nusername=someone\npassword=" + SecretValue, + string.Empty, + GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCredential(tracer, "mock://repoUrl", out _, out _, out _) + .ShouldBeFalse("Parse of the credential output must fail for this test"); + + EventMetadata metadata = GetActivityMetadata(tracer); + AssertNoSecret(metadata); + metadata["OutputKeys"].ShouldEqual("protocol,host,username,password"); + } + + [TestCase] + public void TryGetCertificatePasswordDoesNotTraceSecretWhenParseFails() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + + // The secret is on the last line and has no terminating newline, so the parse fails. + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result( + "protocol=cert\npath=mock://certificate\npassword=" + SecretValue, + string.Empty, + GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCertificatePassword(tracer, "mock://certificate", out _, out _) + .ShouldBeFalse("Parse of the credential output must fail for this test"); + + EventMetadata metadata = GetActivityMetadata(tracer); + AssertNoSecret(metadata); + metadata["OutputKeys"].ShouldEqual("protocol,path,password"); + } + + private static EventMetadata GetActivityMetadata(MockTracer tracer) + { + MockTracer activityTracer = tracer.StartActivityTracer; + activityTracer.ShouldNotBeNull("The credential call must start an activity"); + activityTracer.StoppedActivityMetadata.Count.ShouldEqual(1); + + return activityTracer.StoppedActivityMetadata.Single(); + } + + private static void AssertNoSecret(EventMetadata metadata) + { + foreach (object value in metadata.Values) + { + string text = value?.ToString() ?? string.Empty; + text.Contains(SecretValue).ShouldBeFalse("Credential output must not be traced: " + text); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index d933584e94..dbe4c947a8 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -17,6 +17,7 @@ public MockTracer() this.RelatedWarningEvents = new List(); this.RelatedErrorEvents = new List(); this.RelatedEventNames = new List(); + this.StoppedActivityMetadata = new List(); } public MockTracer StartActivityTracer { get; private set; } @@ -30,6 +31,10 @@ public MockTracer() // do not otherwise get recorded). Lets tests assert a specific diagnostic event fired. public List RelatedEventNames { get; } + // Metadata passed to Stop when an activity ends. Lets tests assert on + // what an activity reports, including that a secret is not present. + public List StoppedActivityMetadata { get; } + public void WaitForRelatedEvent() { this.waitEvent.WaitOne(); @@ -136,6 +141,11 @@ public ITracer StartActivity(string activityName, EventLevel level, Keywords sta public TimeSpan Stop(EventMetadata metadata) { + if (metadata != null) + { + this.StoppedActivityMetadata.Add(metadata); + } + return TimeSpan.Zero; }