Do not trace raw credential output

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}";
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        private static string GetCredentialOutputKeys(string credentialOutput)
+        {
+            if (string.IsNullOrEmpty(credentialOutput))
+            {
+                return string.Empty;
+            }
+
+            IEnumerable<string> 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) : "<malformed>";
+                });
+
+            return string.Join(",", keys);
+        }
+
         private static string ParseValue(string contents, string prefix)
         {
             int startIndex = contents.IndexOf(prefix) + prefix.Length;

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

GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs

@@ -17,6 +17,7 @@ public MockTracer()
             this.RelatedWarningEvents = new List<string>();
             this.RelatedErrorEvents = new List<string>();
             this.RelatedEventNames = new List<string>();
+            this.StoppedActivityMetadata = new List<EventMetadata>();
         }
 
         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<string> 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<EventMetadata> 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;
         }