From a6c33d23a9e7577937da80f87230b872c70fd78f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 1 Oct 2026 10:59:42 -0700 Subject: [PATCH 1/2] Show credential prompts above other windows for background mounts Background GVFS processes such as GVFS.Mount.exe run with a hidden console window. When git launches the Git Credential Manager from such a process, the credential manager parents its interactive sign-in prompt to the hidden console. The prompt then has no foreground right and sinks behind other windows, so the user never sees it. Launch git detached from the console (CreateNoWindow=true) for commands that may require authentication when the current process has no visible console window. With no console, git starts the credential manager detached, so the credential manager parents its prompt to a topmost stub window instead of the hidden console. A process with a visible console window (e.g. gvfs.exe run from a classic console) keeps the current behavior, so the credential manager parents the prompt to that window. Detection keys on the console window, not an arbitrary GUI window. A process whose console is hidden, or a non-displayed pseudo-console such as a foreground gvfs.exe run from Windows Terminal, is treated as having no visible window and also detaches git. For the Git Credential Manager this is equivalent or better, because the detached prompt is topmost instead of owned by a hidden host window. Add GVFSPlatform.CurrentProcessHasVisibleConsoleWindow. The Windows implementation checks GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) and IsWindowVisible. Add a mayRequireAuth option to GitProcess.GetGitProcess, which detaches git from a hidden console only when set. Only credential fill can trigger an interactive prompt, so only the url and certificate fills set mayRequireAuth. Credential approve and reject store or erase an already-obtained credential and never prompt, so they keep the default. Every other git command GVFS runs is local and keeps the default, so the per-child conhost cost stays avoided for every non-authenticating call. Log hasVisibleConsoleWindow from TryGetCredential so a credential fill that does not surface a prompt stays diagnosable from telemetry. Guard the native console probe so a binding failure keeps the previous behavior instead of aborting the fill. Add a certificate-fill test for the mayRequireAuth wiring. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSPlatform.cs | 14 +++ GVFS/GVFS.Common/Git/GitProcess.cs | 57 +++++++-- GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 30 +++++ .../Git/GitAuthenticationTests.cs | 110 ++++++++++++++++++ .../Mock/Common/MockPlatform.cs | 17 ++- .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 11 +- 6 files changed, 223 insertions(+), 16 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSPlatform.cs b/GVFS/GVFS.Common/GVFSPlatform.cs index 5b232628aa..964b8d6acd 100644 --- a/GVFS/GVFS.Common/GVFSPlatform.cs +++ b/GVFS/GVFS.Common/GVFSPlatform.cs @@ -65,6 +65,20 @@ public static void Register(GVFSPlatform platform) /// public abstract void PrepareProcessToRunInBackground(); + /// + /// Returns true if the current process owns a visible console window that a child process + /// (e.g. git launching the credential manager) can use as the owner for an interactive prompt. + /// + /// + /// This inspects the current process's console window and its root owner, not arbitrary GUI + /// windows. Background processes such as GVFS.Mount run with a hidden console, so this returns + /// false for them; foreground processes launched from a visible console return true. Credential + /// helpers launched by a process with no visible console window should be started detached from + /// the console, so the credential manager parents its prompt to a topmost stub window instead + /// of the hidden console. + /// + public abstract bool CurrentProcessHasVisibleConsoleWindow(); + public abstract bool IsProcessActive(int processId); /// diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 43a40ced02..9d0e80ea88 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -273,13 +273,14 @@ public virtual bool TryGetCertificatePassword( using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) { - // See GetFromConfig for why pre-command hook is disabled - // for bootstrap-time git operations. + // See GetFromConfig for why the pre-command hook is disabled for + // bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), parseStdOutLine: null, - usePreCommandHook: false); + usePreCommandHook: false, + mayRequireAuth: true); if (gitCredentialOutput.ExitCodeIsFailure) { @@ -328,14 +329,15 @@ public virtual bool TryGetCredential( using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational)) { - // See GetFromConfig for why pre-command hook is disabled - // for bootstrap-time git operations. + // See GetFromConfig for why the pre-command hook is disabled for + // bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( GenerateCredentialVerbCommand("fill"), stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + mayRequireAuth: true); if (gitCredentialOutput.ExitCodeIsFailure) { @@ -361,6 +363,14 @@ public virtual bool TryGetCredential( return false; } + // Record whether the current process had a visible console window. GetGitProcess uses + // this to decide whether to detach git from a hidden console so the credential prompt is + // not left behind other windows; logging it keeps that decision diagnosable from telemetry + // when a prompt fails to appear. + EventMetadata consoleMetadata = new EventMetadata(); + consoleMetadata.Add("hasVisibleConsoleWindow", CurrentProcessHasVisibleConsoleWindowSafe()); + activity.RelatedInfo(consoleMetadata, "Credential fill console window visibility"); + username = ParseValue(gitCredentialOutput.Output, "username="); password = ParseValue(gitCredentialOutput.Output, "password="); @@ -902,7 +912,7 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook) + public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; @@ -919,7 +929,13 @@ public Process GetGitProcess(string command, string workingDirectory, string dot // instead), CreateNoWindow must be set to true for that case — otherwise // the non-redirected stream inherits the parent's console handle, which // may be absent when running as a service, causing lost output. - processInfo.CreateNoWindow = false; + // + // When a command may require authentication, git can launch the credential + // manager. Detaching from a hidden console (CreateNoWindow=true) makes that + // prompt appear on top instead of behind other windows. See + // GVFSPlatform.CurrentProcessHasVisibleConsoleWindow for the full rationale. + bool hasVisibleConsoleWindow = !mayRequireAuth || CurrentProcessHasVisibleConsoleWindowSafe(); + processInfo.CreateNoWindow = mayRequireAuth && !hasVisibleConsoleWindow; processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -986,7 +1002,8 @@ protected virtual Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + bool mayRequireAuth = false) { if (failedToSetEncoding && writeStdIn != null) { @@ -998,7 +1015,7 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) + using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth)) { // Bound how much stdout/stderr we buffer so a pathologically noisy git command // cannot grow these buffers without limit until GVFS.Mount hits an @@ -1086,6 +1103,20 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + private static bool CurrentProcessHasVisibleConsoleWindowSafe() + { + try + { + return GVFSPlatform.Instance.CurrentProcessHasVisibleConsoleWindow(); + } + catch (Exception ex) when (ex is DllNotFoundException || ex is EntryPointNotFoundException) + { + // If the native console probe cannot run, assume a visible console so callers keep + // the previous behavior (CreateNoWindow=false) rather than aborting the command. + return true; + } + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; @@ -1166,7 +1197,8 @@ private Result InvokeGitAgainstDotGitFolder( Action parseStdOutLine, bool usePreCommandHook = true, string gitObjectsDirectory = null, - int timeoutMs = -1) + int timeoutMs = -1, + bool mayRequireAuth = false) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1180,7 +1212,8 @@ private Result InvokeGitAgainstDotGitFolder( parseStdOutLine: parseStdOutLine, timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + mayRequireAuth: mayRequireAuth); } public class Result diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index 91da42f767..a27c980834 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -11,6 +11,7 @@ using System.IO; using System.IO.Pipes; using System.Linq; +using System.Runtime.InteropServices; using System.Security.AccessControl; using System.Security.Principal; using System.ServiceProcess; @@ -28,6 +29,9 @@ public partial class WindowsPlatform : GVFSPlatform private const string ProfileImagePathRegistryValue = "ProfileImagePath"; private const string AdminProtectionProfilePrefix = "ADMIN_"; + // GetAncestor flag GA_ROOTOWNER (3): walk the owner chain to the root owned window. + private const uint GA_ROOTOWNER = 3; + public WindowsPlatform() : base(underConstruction: new UnderConstructionFlags()) { } @@ -224,6 +228,22 @@ public override void PrepareProcessToRunInBackground() // No additional work required } + public override bool CurrentProcessHasVisibleConsoleWindow() + { + IntPtr consoleWindow = GetConsoleWindow(); + if (consoleWindow == IntPtr.Zero) + { + // The process has no console at all (e.g. a GUI process), so there is no console + // window to own an interactive prompt. + return false; + } + + // GVFS.Mount runs with a hidden console, so its console window exists but its root + // owner is not visible. Only treat the window as usable when its root owner is visible. + IntPtr rootOwner = GetAncestor(consoleWindow, GA_ROOTOWNER); + return rootOwner != IntPtr.Zero && IsWindowVisible(rootOwner); + } + public override NamedPipeServerStream CreatePipeByName(string pipeName) { PipeSecurity security = new PipeSecurity(); @@ -458,6 +478,16 @@ private static object GetValueFromRegistry(RegistryHive registryHive, string key return value; } + [DllImport("kernel32.dll")] + private static extern IntPtr GetConsoleWindow(); + + [DllImport("user32.dll")] + private static extern IntPtr GetAncestor(IntPtr hwnd, uint gaFlags); + + [DllImport("user32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool IsWindowVisible(IntPtr hWnd); + public class WindowsPlatformConstants : GVFSPlatformConstants { public override string ExecutableExtension diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 25aa675b7e..83affa8a36 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,7 +1,9 @@ using System; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; +using GVFS.Common; using GVFS.Common.Git; using GVFS.Tests; using GVFS.Tests.Should; @@ -342,6 +344,114 @@ public void TryGetCredentialsWaitsForBackgroundInitializationThenSucceeds() authString.ShouldNotBeNull("A credential string should be returned"); } + [TestCase] + public void CredentialFillIsFlaggedAsMayRequireAuth() + { + // Only credential fill can trigger an interactive sign-in from the credential manager, so + // only it is flagged mayRequireAuth=true. GetGitProcess uses that flag to detach git from a + // hidden console so the prompt is not left behind other windows. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out _, out string error).ShouldEqual(true, "Failed to get credential: " + error); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential fill"] + .ShouldEqual(true, "Credential fill should be flagged as possibly requiring auth"); + + // Non-credential git commands never prompt, so they must not be flagged. + gitProcess.MayRequireAuthByCommand["config --get-urlmatch http mock://repoUrl"] + .ShouldEqual(false, "Non-credential git commands should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialApproveIsNotFlaggedAsMayRequireAuth() + { + // Credential approve stores an already-obtained credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.ApproveCredentials(tracer, authString); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential approve"] + .ShouldEqual(false, "Credential approve should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialRejectIsNotFlaggedAsMayRequireAuth() + { + // Credential reject erases a stored credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.RejectCredentials(tracer, authString); + + gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential reject"] + .ShouldEqual(false, "Credential reject should not be flagged as requiring auth"); + } + + [TestCase] + public void CertificateCredentialFillIsFlaggedAsMayRequireAuth() + { + // The certificate credential fill can also trigger an interactive sign-in, so it is flagged + // mayRequireAuth=true like the URL fill. It is covered directly here because the standard + // credential flow does not exercise the certificate path. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result("password=certpassword\r\n", string.Empty, GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(true, "Failed to get certificate password: " + error); + + gitProcess.MayRequireAuthByCommand["credential fill"] + .ShouldEqual(true, "Certificate credential fill should be flagged as possibly requiring auth"); + } + + [TestCase] + public void GetGitProcessDetachesFromConsoleOnlyWhenAuthMayBeRequiredAndNoVisibleWindow() + { + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + GitProcess gitProcess = new GitProcess("git.exe", workingDirectoryRoot: null); + + // No visible console + command may require auth => detach so the prompt is topmost. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "Should detach when auth may be required and there is no visible console window"); + } + + // Visible console + command may require auth => keep the console so the prompt parents to it. + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should keep the console when it is visible"); + } + + // Command cannot require auth => never detach, regardless of console visibility. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("rev-parse HEAD", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: true, mayRequireAuth: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should never detach when auth cannot be required"); + } + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index 3ee7425de0..c3f38b885b 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -21,6 +21,12 @@ public class MockPlatform : GVFSPlatform public string MockCurrentUser { get; set; } + /// + /// Controls the return value of . Defaults to true + /// so tests keep the foreground behavior unless they opt into the background-process case. + /// + public bool HasVisibleWindow { get; set; } = true; + public override IKernelDriver KernelDriver => throw new NotSupportedException(); public override IGitInstallation GitInstallation { get; } = new MockGitInstallation(); @@ -195,9 +201,14 @@ public override Process StartBackgroundVFS4GProcess(ITracer tracer, string progr throw new NotSupportedException(); } - public override void PrepareProcessToRunInBackground() - { - throw new NotSupportedException(); + public override void PrepareProcessToRunInBackground() + { + throw new NotSupportedException(); + } + + public override bool CurrentProcessHasVisibleConsoleWindow() + { + return this.HasVisibleWindow; } public override bool IsGitStatusCacheSupported() diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index c9095cc2c8..f37cebba8e 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,12 +18,19 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); + this.MayRequireAuthByCommand = new Dictionary(StringComparer.Ordinal); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); } public List CommandsRun { get; } + + /// + /// Records the mayRequireAuth value passed to for each command, so + /// tests can assert that only credential-fill commands are flagged as possibly requiring auth. + /// + public Dictionary MayRequireAuthByCommand { get; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } @@ -84,9 +91,11 @@ protected override Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePrecommandHook = true) + bool usePrecommandHook = true, + bool mayRequireAuth = false) { this.CommandsRun.Add(command); + this.MayRequireAuthByCommand[command] = mayRequireAuth; if (this.ShouldFail) { From 58edd7c8b277f3b289a84bbd0a167fed285d03a0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 7 Oct 2026 11:12:40 -0700 Subject: [PATCH 2/2] Make credential-fill console-visibility telemetry authoritative and log it on failure Address review feedback on the credential-prompt console-detach change. - Probe CurrentProcessHasVisibleConsoleWindow once per credential-fill call and thread the value through InvokeGitAgainstDotGitFolder, InvokeGitImpl, and GetGitProcess so the logged value and the CreateNoWindow decision are the same probe. GetGitProcess treats a supplied value as authoritative and only probes when none is passed. - Log hasVisibleConsoleWindow on the credential-fill failure path (including the timeout a hidden, unanswered prompt produces), not only on success. The success-path metadata and the certificate-fill success and failure paths log the same value. - Remove the misnamed intermediate local in GetGitProcess; compute CreateNoWindow directly so the && short-circuits the probe when auth cannot be required. - Restore the original pre-command-hook comment wording. - MockPlatform gains ThrowOnConsoleProbe to simulate a native probe that cannot run; add tests for the safe fallback, the caller-supplied authoritative value, and console-visibility telemetry on the URL and certificate credential-fill failure paths. The telemetry tests assert the logged value, not just the key. - MockGitProcess records each git invocation (command, mayRequireAuth, and hasVisibleConsoleWindow) instead of keeping only the last value per command, so the wiring assertions cannot be fooled if a command runs twice with different flags. The failure-telemetry tests also assert the probed console-visibility value threads down to the git invocation. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitProcess.cs | 53 +++++++--- .../Git/GitAuthenticationTests.cs | 97 ++++++++++++++++++- .../Mock/Common/MockPlatform.cs | 17 +++- .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 46 +++++++-- 4 files changed, 184 insertions(+), 29 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 9d0e80ea88..df54900b7c 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -273,19 +273,27 @@ public virtual bool TryGetCertificatePassword( using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) { - // See GetFromConfig for why the pre-command hook is disabled for - // bootstrap-time git operations. + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths when a credential prompt fails to appear. + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + + // See GetFromConfig for why pre-command hook is disabled + // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), parseStdOutLine: null, usePreCommandHook: false, - mayRequireAuth: true); + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); errorData.Add("CertificatePath", certificatePath); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); tracer.RelatedWarning( errorData, "Git could not get credentials: " + gitCredentialOutput.Errors, @@ -302,7 +310,8 @@ public virtual bool TryGetCertificatePassword( EventMetadata metadata = new EventMetadata { { "Success", success }, - { "CertificatePath", certificatePath } + { "CertificatePath", certificatePath }, + { "hasVisibleConsoleWindow", hasVisibleConsoleWindow } }; if (!success) @@ -329,19 +338,27 @@ public virtual bool TryGetCredential( using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational)) { - // See GetFromConfig for why the pre-command hook is disabled for - // bootstrap-time git operations. + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths (including the timeout that a hidden, unanswered prompt produces). + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + + // See GetFromConfig for why pre-command hook is disabled + // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( GenerateCredentialVerbCommand("fill"), stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, timeoutMs: timeoutMs, - mayRequireAuth: true); + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); if (gitCredentialOutput.Errors.StartsWith("Operation timed out")) { @@ -368,7 +385,7 @@ public virtual bool TryGetCredential( // not left behind other windows; logging it keeps that decision diagnosable from telemetry // when a prompt fails to appear. EventMetadata consoleMetadata = new EventMetadata(); - consoleMetadata.Add("hasVisibleConsoleWindow", CurrentProcessHasVisibleConsoleWindowSafe()); + consoleMetadata.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); activity.RelatedInfo(consoleMetadata, "Credential fill console window visibility"); username = ParseValue(gitCredentialOutput.Output, "username="); @@ -912,7 +929,7 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false) + public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false, bool? hasVisibleConsoleWindow = null) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; @@ -934,8 +951,11 @@ public Process GetGitProcess(string command, string workingDirectory, string dot // manager. Detaching from a hidden console (CreateNoWindow=true) makes that // prompt appear on top instead of behind other windows. See // GVFSPlatform.CurrentProcessHasVisibleConsoleWindow for the full rationale. - bool hasVisibleConsoleWindow = !mayRequireAuth || CurrentProcessHasVisibleConsoleWindowSafe(); - processInfo.CreateNoWindow = mayRequireAuth && !hasVisibleConsoleWindow; + // A caller that already probed the console visibility passes the result in + // hasVisibleConsoleWindow so the logged and acted-on value stay identical; + // otherwise probe here. The && short-circuits the probe when auth cannot + // be required. + processInfo.CreateNoWindow = mayRequireAuth && !(hasVisibleConsoleWindow ?? CurrentProcessHasVisibleConsoleWindowSafe()); processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -1003,7 +1023,8 @@ protected virtual Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePreCommandHook = true, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { if (failedToSetEncoding && writeStdIn != null) { @@ -1015,7 +1036,7 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth)) + using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth, hasVisibleConsoleWindow: hasVisibleConsoleWindow)) { // Bound how much stdout/stderr we buffer so a pathologically noisy git command // cannot grow these buffers without limit until GVFS.Mount hits an @@ -1198,7 +1219,8 @@ private Result InvokeGitAgainstDotGitFolder( bool usePreCommandHook = true, string gitObjectsDirectory = null, int timeoutMs = -1, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1213,7 +1235,8 @@ private Result InvokeGitAgainstDotGitFolder( timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, - mayRequireAuth: mayRequireAuth); + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } public class Result diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 83affa8a36..682a80537f 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -357,11 +357,11 @@ public void CredentialFillIsFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out _, out string error).ShouldEqual(true, "Failed to get credential: " + error); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential fill"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential fill") .ShouldEqual(true, "Credential fill should be flagged as possibly requiring auth"); // Non-credential git commands never prompt, so they must not be flagged. - gitProcess.MayRequireAuthByCommand["config --get-urlmatch http mock://repoUrl"] + gitProcess.MayRequireAuthFor("config --get-urlmatch http mock://repoUrl") .ShouldEqual(false, "Non-credential git commands should not be flagged as requiring auth"); } @@ -377,7 +377,7 @@ public void CredentialApproveIsNotFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); dut.ApproveCredentials(tracer, authString); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential approve"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential approve") .ShouldEqual(false, "Credential approve should not be flagged as requiring auth"); } @@ -393,7 +393,7 @@ public void CredentialRejectIsNotFlaggedAsMayRequireAuth() dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); dut.RejectCredentials(tracer, authString); - gitProcess.MayRequireAuthByCommand[$"{AzureDevOpsUseHttpPathString} credential reject"] + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential reject") .ShouldEqual(false, "Credential reject should not be flagged as requiring auth"); } @@ -412,7 +412,7 @@ public void CertificateCredentialFillIsFlaggedAsMayRequireAuth() gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) .ShouldEqual(true, "Failed to get certificate password: " + error); - gitProcess.MayRequireAuthByCommand["credential fill"] + gitProcess.MayRequireAuthFor("credential fill") .ShouldEqual(true, "Certificate credential fill should be flagged as possibly requiring auth"); } @@ -445,6 +445,93 @@ public void GetGitProcessDetachesFromConsoleOnlyWhenAuthMayBeRequiredAndNoVisibl { process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should never detach when auth cannot be required"); } + + // Native console probe cannot run => safe fallback assumes a visible console, so do not detach. + platform.HasVisibleWindow = false; + platform.ThrowOnConsoleProbe = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should not detach when the console probe cannot run"); + } + + // A caller-supplied console-visibility value is authoritative and bypasses the platform probe + // (the probe would throw if consulted here). + platform.ThrowOnConsoleProbe = true; + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true, hasVisibleConsoleWindow: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "A caller-supplied false should force detach without probing the platform"); + } + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + platform.ThrowOnConsoleProbe = false; + } + } + + [TestCase] + public void TryGetCredentialLogsConsoleVisibilityWhenCredentialFillFails() + { + // The console-visibility telemetry must be recorded on the failure path too, because the + // hidden-prompt timeout that this diagnostic exists to explain is itself a failure result. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCredential(tracer, "mock://repoUrl", out string username, out string password, out string error, timeoutMs: 5000) + .ShouldEqual(false, "Credential fill should report failure when git times out"); + + // Assert the logged value, not just the key: the diagnostic is only useful if it records + // the actual console-visibility state (false here) that explains the hidden-prompt timeout. + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == $"{AzureDevOpsUseHttpPathString} credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + + [TestCase] + public void TryGetCertificatePasswordLogsConsoleVisibilityWhenCredentialFillFails() + { + // The certificate credential fill stamps the same console-visibility telemetry as the URL fill. + // It is covered separately because the standard credential flow does not exercise the certificate + // path, and the value must be present on the failure path where a hidden prompt times out. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(false, "Certificate password fill should report failure when git times out"); + + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == "credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); } finally { diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index c3f38b885b..a5c92b0c9f 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -27,6 +27,12 @@ public class MockPlatform : GVFSPlatform /// public bool HasVisibleWindow { get; set; } = true; + /// + /// When true, throws to simulate a native + /// console probe that cannot run (missing entry point), so tests can cover the safe fallback. + /// + public bool ThrowOnConsoleProbe { get; set; } + public override IKernelDriver KernelDriver => throw new NotSupportedException(); public override IGitInstallation GitInstallation { get; } = new MockGitInstallation(); @@ -201,13 +207,18 @@ public override Process StartBackgroundVFS4GProcess(ITracer tracer, string progr throw new NotSupportedException(); } - public override void PrepareProcessToRunInBackground() - { - throw new NotSupportedException(); + public override void PrepareProcessToRunInBackground() + { + throw new NotSupportedException(); } public override bool CurrentProcessHasVisibleConsoleWindow() { + if (this.ThrowOnConsoleProbe) + { + throw new EntryPointNotFoundException(); + } + return this.HasVisibleWindow; } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index f37cebba8e..cf786a9835 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,7 +18,7 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); - this.MayRequireAuthByCommand = new Dictionary(StringComparer.Ordinal); + this.InvocationsRun = new List(); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); @@ -27,15 +27,34 @@ public MockGitProcess() public List CommandsRun { get; } /// - /// Records the mayRequireAuth value passed to for each command, so - /// tests can assert that only credential-fill commands are flagged as possibly requiring auth. + /// Records every call into in order, with the mayRequireAuth and + /// hasVisibleConsoleWindow values passed to it. Recording each invocation (rather than keeping only + /// the last value per command) keeps the wiring assertions honest if a command is ever run more than + /// once with different flags. /// - public Dictionary MayRequireAuthByCommand { get; } + public List InvocationsRun { get; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } public Dictionary> CredentialRejections { get; } + /// + /// Returns the mayRequireAuth value the given command was invoked with, asserting the command ran and + /// that every invocation of it agreed on the value. + /// + public bool MayRequireAuthFor(string command) + { + List matches = this.InvocationsRun + .Where(invocation => string.Equals(invocation.Command, command, StringComparison.Ordinal)) + .ToList(); + + matches.Count.ShouldBeAtLeast(1, "Command was never run: " + command); + matches.Select(invocation => invocation.MayRequireAuth).Distinct().Count() + .ShouldEqual(1, "Command ran with conflicting mayRequireAuth values: " + command); + + return matches[0].MayRequireAuth; + } + public void SetExpectedCommandResult(string command, Func result, bool matchPrefix = false) { CommandInfo commandInfo = new CommandInfo(command, result, matchPrefix); @@ -92,10 +111,11 @@ protected override Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePrecommandHook = true, - bool mayRequireAuth = false) + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { this.CommandsRun.Add(command); - this.MayRequireAuthByCommand[command] = mayRequireAuth; + this.InvocationsRun.Add(new GitInvocation(command, mayRequireAuth, hasVisibleConsoleWindow)); if (this.ShouldFail) { @@ -134,6 +154,20 @@ protected override Result InvokeGitImpl( return result; } + public class GitInvocation + { + public GitInvocation(string command, bool mayRequireAuth, bool? hasVisibleConsoleWindow) + { + this.Command = command; + this.MayRequireAuth = mayRequireAuth; + this.HasVisibleConsoleWindow = hasVisibleConsoleWindow; + } + + public string Command { get; } + public bool MayRequireAuth { get; } + public bool? HasVisibleConsoleWindow { get; } + } + public class Credential { public Credential(string username, string password)