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;
}