From 5bc1c0916e10f7cdd399b03f132149f96d0fbbbb Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:07:49 +0100 Subject: [PATCH 1/6] msal: ensure we only parent to visible console windows Ensure that we only consider a console window (or hosting terminal in case of ConPTY) a valid parent for MSAL windows if it is visible. This will prevent MSAL windows from being hidden too. Note that we do **not** apply the visibility check to the optional explicitly provided parent window (via `GCM_MODEL_PARENTHWND)` - the caller should be the one to decide if their handle is good to use or not. Finally, note that we do **not** check for a minimised parent window (via `IsIconic(HWND)`) because we should probably also remain minimised if our parent was. Signed-off-by: Matthew John Cheetham --- src/Core/Authentication/Entra/MsalAdaptors.cs | 11 ++++++++++- src/Core/Interop/Windows/Native/User32.cs | 3 +++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Core/Authentication/Entra/MsalAdaptors.cs b/src/Core/Authentication/Entra/MsalAdaptors.cs index 33997fc5e0..cce41a5f29 100644 --- a/src/Core/Authentication/Entra/MsalAdaptors.cs +++ b/src/Core/Authentication/Entra/MsalAdaptors.cs @@ -59,9 +59,18 @@ public object GetWindow() if (PlatformUtils.IsWindows()) { IntPtr consoleHandle = Kernel32.GetConsoleWindow(); + + // When the parent is using ConPTY (pseudo-terminals) the console + // window may be a fake/stub (as is the case with Windows Terminal). + // This means we need to walk up the ancestor chain to get the actual + // root owner (typically the terminal emulator's window). IntPtr parentHandle = User32.GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); - if (parentHandle != IntPtr.Zero) + // Only use the parent handle if it is valid and visible! + // Note that minimised windows are still 'visibile', and to filter + // those out we'd need to call IsIconic(HWND), but we're happy to + // parent to a minimised parent window and be minimised ourself. + if (parentHandle != IntPtr.Zero && User32.IsWindowVisible(parentHandle)) { return parentHandle; } diff --git a/src/Core/Interop/Windows/Native/User32.cs b/src/Core/Interop/Windows/Native/User32.cs index 8cf05a8d88..7745a10db9 100644 --- a/src/Core/Interop/Windows/Native/User32.cs +++ b/src/Core/Interop/Windows/Native/User32.cs @@ -36,6 +36,9 @@ public static IntPtr SetWindowLongPtr(IntPtr hWnd, int nIndex, IntPtr value) [DllImport(LibraryName, SetLastError = true)] public static extern bool GetClientRect(IntPtr hwnd, out RECT lpRect); + [DllImport(LibraryName)] + public static extern bool IsWindowVisible(IntPtr hwnd); + /// /// Retrieves the handle to the ancestor of the specified window. /// From a7a2d47f5102eb710594693106b7b3bcb13ad115 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:14:09 +0100 Subject: [PATCH 2/6] msal: extract console parent to a method Extract the console parent lookup logic to a static method in preparation for allowing tests to mock it. Signed-off-by: Matthew John Cheetham --- src/Core/Authentication/Entra/MsalAdaptors.cs | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/Core/Authentication/Entra/MsalAdaptors.cs b/src/Core/Authentication/Entra/MsalAdaptors.cs index cce41a5f29..fc917bc4ad 100644 --- a/src/Core/Authentication/Entra/MsalAdaptors.cs +++ b/src/Core/Authentication/Entra/MsalAdaptors.cs @@ -55,30 +55,34 @@ public object GetWindow() return ProgressWindow.ShowAndGetHandle(_cts.Token); } - // On Windows we can try and get the console window parent handle if that exists - if (PlatformUtils.IsWindows()) + // See if we can use the console window as a parent. + // We only consider the window if it is valid and visible. + IntPtr consoleParent = GetConsoleParentWindow(); + if (consoleParent != IntPtr.Zero && User32.IsWindowVisible(consoleParent)) { - IntPtr consoleHandle = Kernel32.GetConsoleWindow(); - - // When the parent is using ConPTY (pseudo-terminals) the console - // window may be a fake/stub (as is the case with Windows Terminal). - // This means we need to walk up the ancestor chain to get the actual - // root owner (typically the terminal emulator's window). - IntPtr parentHandle = User32.GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); - - // Only use the parent handle if it is valid and visible! - // Note that minimised windows are still 'visibile', and to filter - // those out we'd need to call IsIconic(HWND), but we're happy to - // parent to a minimised parent window and be minimised ourself. - if (parentHandle != IntPtr.Zero && User32.IsWindowVisible(parentHandle)) - { - return parentHandle; - } + return consoleParent; } return null; } + private static IntPtr GetConsoleParentWindow() + { + // On Windows we can try and get the console window parent handle if that exists + if (!PlatformUtils.IsWindows()) + { + return IntPtr.Zero; + } + + IntPtr consoleHandle = Kernel32.GetConsoleWindow(); + + // When the parent is using ConPTY (pseudo-terminals) the console + // window may be a fake/stub (as is the case with Windows Terminal). + // This means we need to walk up the ancestor chain to get the actual + // root owner (typically the terminal emulator's window). + return User32.GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); + } + public void Dispose() { // Close and clean up any stub window we may have created From d997195c06de8bcadaeff1030957df0c4447b380 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:20:05 +0100 Subject: [PATCH 3/6] msal: allow testing of MsalParentWindowAdatper Allow testing of the parent window adapter by allowing mocks of the core platform-specific APIs: * creation of stub window * window visibilty checking * get console window Signed-off-by: Matthew John Cheetham --- src/Core/Authentication/Entra/MsalAdaptors.cs | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/Core/Authentication/Entra/MsalAdaptors.cs b/src/Core/Authentication/Entra/MsalAdaptors.cs index fc917bc4ad..48393353b7 100644 --- a/src/Core/Authentication/Entra/MsalAdaptors.cs +++ b/src/Core/Authentication/Entra/MsalAdaptors.cs @@ -31,15 +31,30 @@ internal class MsalParentWindowAdapter : IDisposable private readonly bool _createIfMissing; private readonly CancellationTokenSource _cts = new(); + private readonly Func _getConsoleParentWindow; + private readonly Func _isWindowVisible; + private readonly Func _createWindow; + public static MsalParentWindowAdapter Create(object parentWindow, bool createIfMissing = false) { - return new MsalParentWindowAdapter(parentWindow, createIfMissing); + return new MsalParentWindowAdapter(parentWindow, createIfMissing, + GetConsoleParentWindow, User32.IsWindowVisible, ProgressWindow.ShowAndGetHandle); } - private MsalParentWindowAdapter(object parentWindow, bool createIfMissing = false) + internal MsalParentWindowAdapter(object parentWindow, bool createIfMissing, + Func getConsoleParentWindow, Func isWindowVisible, + Func createWindow) { + EnsureArgument.NotNull(getConsoleParentWindow, nameof(getConsoleParentWindow)); + EnsureArgument.NotNull(isWindowVisible, nameof(isWindowVisible)); + EnsureArgument.NotNull(createWindow, nameof(createWindow)); + _parentWindow = parentWindow; _createIfMissing = createIfMissing; + + _getConsoleParentWindow = getConsoleParentWindow; + _isWindowVisible = isWindowVisible; + _createWindow = createWindow; } public object GetWindow() @@ -52,13 +67,13 @@ public object GetWindow() // Create a stub window to use as a parent if (_createIfMissing) { - return ProgressWindow.ShowAndGetHandle(_cts.Token); + return _createWindow(_cts.Token); } // See if we can use the console window as a parent. // We only consider the window if it is valid and visible. - IntPtr consoleParent = GetConsoleParentWindow(); - if (consoleParent != IntPtr.Zero && User32.IsWindowVisible(consoleParent)) + IntPtr consoleParent = _getConsoleParentWindow(); + if (consoleParent != IntPtr.Zero && _isWindowVisible(consoleParent)) { return consoleParent; } From 261bdd39b9fa29188d674a514009def869129e8c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:21:55 +0100 Subject: [PATCH 4/6] msal: prefer the console window over progress stub In previous versions of GCM (2.x) we preferred to parent MSAL windows to the console window, if possible, over creating our own stub 'progress' window. In GCM 3.0.0 we accidentally reversed that preference, which means in practice we always used the stub window and never the console window. Let's reverse that back to the desired order: 1. Explicit parent (via `GCM_MODAL_PARENTHWND`) 2. Console window parent (if present and visible) 3. Stub progress window (if required by the caller) 4. None Signed-off-by: Matthew John Cheetham --- src/Core/Authentication/Entra/MsalAdaptors.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Core/Authentication/Entra/MsalAdaptors.cs b/src/Core/Authentication/Entra/MsalAdaptors.cs index 48393353b7..e169ee8387 100644 --- a/src/Core/Authentication/Entra/MsalAdaptors.cs +++ b/src/Core/Authentication/Entra/MsalAdaptors.cs @@ -64,12 +64,6 @@ public object GetWindow() return _parentWindow; } - // Create a stub window to use as a parent - if (_createIfMissing) - { - return _createWindow(_cts.Token); - } - // See if we can use the console window as a parent. // We only consider the window if it is valid and visible. IntPtr consoleParent = _getConsoleParentWindow(); @@ -78,6 +72,12 @@ public object GetWindow() return consoleParent; } + // Create a stub window to use as a parent + if (_createIfMissing) + { + return _createWindow(_cts.Token); + } + return null; } From 6f4fedcdbd3d663e131318fe6a745330cc12372c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:24:17 +0100 Subject: [PATCH 5/6] msal: add tests of the parent window adapter Add comprehensive tests of the MSAL parent window adapter, and the precedence logic for selecting (and creating) an appropriate parent window. Signed-off-by: Matthew John Cheetham --- .../Entra/MsalParentWindowAdapterTests.cs | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs diff --git a/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs b/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs new file mode 100644 index 0000000000..831ae489f8 --- /dev/null +++ b/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs @@ -0,0 +1,138 @@ +using System; +using System.Threading; +using GitCredentialManager.Authentication.Entra; +using Xunit; + +namespace GitCredentialManager.Tests.Authentication.Entra; + +public class MsalParentWindowAdapterTests +{ + [Theory] + [InlineData(42, false)] + [InlineData(42, true)] + [InlineData(-1, false)] + [InlineData(-1, true)] + public void GetWindow_ExplicitParent_TakesPrecedence(int parentValue, bool createIfMissing) + { + object parentWindow = new IntPtr(parentValue); + using var adapter = new MsalParentWindowAdapter(parentWindow, createIfMissing, + () => throw new InvalidOperationException("Console lookup should not be called."), + _ => throw new InvalidOperationException("Visibility should not be checked."), + _ => throw new InvalidOperationException("A progress window should not be created.")); + + object result = adapter.GetWindow(); + Assert.Same(parentWindow, result); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void GetWindow_VisibleConsoleRoot_PrecedesProgressWindow(bool createIfMissing) + { + var consoleRoot = new IntPtr(42); + bool visibilityChecked = false; + using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, createIfMissing, + () => consoleRoot, + hwnd => + { + Assert.Equal(consoleRoot, hwnd); + visibilityChecked = true; + return true; + }, + _ => throw new InvalidOperationException("A progress window should not be created.")); + + IntPtr result = Assert.IsType(adapter.GetWindow()); + + Assert.Equal(consoleRoot, result); + Assert.True(visibilityChecked); + } + + [Theory] + [InlineData(0)] + [InlineData(42)] + public void GetWindow_NoVisibleConsoleRoot_CreatesProgressWhenRequired(int consoleRootValue) + { + var consoleRoot = new IntPtr(consoleRootValue); + var progressWindow = new IntPtr(84); + int visibilityChecks = 0; + var windowToken = CancellationToken.None; + using (var adapter = new MsalParentWindowAdapter(IntPtr.Zero, true, + () => consoleRoot, + hwnd => + { + Assert.Equal(consoleRoot, hwnd); + visibilityChecks++; + return false; + }, + ct => + { + windowToken = ct; + return progressWindow; + })) + { + IntPtr result = Assert.IsType(adapter.GetWindow()); + + Assert.Equal(progressWindow, result); + Assert.Equal(consoleRootValue == 0 ? 0 : 1, visibilityChecks); + Assert.True(windowToken.CanBeCanceled); + Assert.False(windowToken.IsCancellationRequested); + } + + Assert.True(windowToken.IsCancellationRequested); + } + + [Theory] + [InlineData(0)] + [InlineData(42)] + public void GetWindow_NoVisibleConsoleRoot_ReturnsNullWhenParentNotRequired(int consoleRootValue) + { + var consoleRoot = new IntPtr(consoleRootValue); + int visibilityChecks = 0; + using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, false, + () => consoleRoot, + hwnd => + { + Assert.Equal(consoleRoot, hwnd); + visibilityChecks++; + return false; + }, + _ => throw new InvalidOperationException("A progress window should not be created.")); + + object result = adapter.GetWindow(); + + Assert.Null(result); + Assert.Equal(consoleRootValue == 0 ? 0 : 1, visibilityChecks); + } + + [Fact] + public void GetWindow_VisibilityCheckThrows_PropagatesException() + { + var exception = new InvalidOperationException("Visibility check failed."); + using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, true, + () => new IntPtr(42), + _ => throw exception, + _ => throw new InvalidOperationException("A progress window should not be created.")); + + var thrown = Assert.Throws(adapter.GetWindow); + Assert.Same(exception, thrown); + } + + [Fact] + public void Create_ExplicitParent_IsReturned() + { + object parentWindow = new IntPtr(42); + using var adapter = MsalParentWindowAdapter.Create(parentWindow, true); + + object result = adapter.GetWindow(); + Assert.Same(parentWindow, result); + } + + [PosixFact] + public void Create_NoExplicitParent_Posix_ReturnsNull() + { + using var adapter = MsalParentWindowAdapter.Create(IntPtr.Zero); + + object result = adapter.GetWindow(); + Assert.Null(result); + } +} From 8413a8f3ef274753905d8a970e3fd88303f94321 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 5 Oct 2026 14:40:25 +0100 Subject: [PATCH 6/6] docs: document the GCM_MODAL_PARENTHWND variable Signed-off-by: Matthew John Cheetham --- docs/environment.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/environment.md b/docs/environment.md index 424867a689..888e13adbb 100644 --- a/docs/environment.md +++ b/docs/environment.md @@ -173,6 +173,40 @@ Defaults to enabled. --- +### GCM_MODAL_PARENTHWND + +Specify the parent window handle (`HWND`) for modal dialogs on Windows. + +> [!NOTE] +> This is intended for use by application developers who are calling Git and/or +> GCM from their application and wish to correctly parent authentication dialogs +> to an existing window. +> +> Do **not** set this manually. + +#### Example + +##### Windows + +```csharp +public static int Example(IntPtr myWindowHandle) +{ + using var process = new Process(); + process.StartInfo = new ProcessStartInfo("git.exe", "pull") + { + Environment = + { + ["GCM_MODAL_PARENTHWND"] = myWindowHandle.ToString("d") + } + }; + process.Start(); + process.WaitForExit(); + return process.ExitCode; +} +``` + +--- + ### GCM_PROVIDER Define the host provider to use when authenticating.