From 4c0cfb6bffde3d38c9517d4d3f11b8a4a152f3d6 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 21 Nov 2022 14:25:18 -0800 Subject: [PATCH 1/5] authbase: gracefully handle no-default UI helper case If there is no hard-coded UI helper, we should gracefully handle the lookup by returning false, rather than crashing due to a argument exception when passing `null` or an empty string to a Path API. --- src/shared/Core/Authentication/AuthenticationBase.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/shared/Core/Authentication/AuthenticationBase.cs b/src/shared/Core/Authentication/AuthenticationBase.cs index 6b2bd9511..65d38e002 100644 --- a/src/shared/Core/Authentication/AuthenticationBase.cs +++ b/src/shared/Core/Authentication/AuthenticationBase.cs @@ -140,6 +140,11 @@ protected bool TryFindHelperCommand(string envar, string configName, string defa Context.Trace.WriteLine($"UI helper override specified: '{helperName}'."); } + else if (string.IsNullOrWhiteSpace(defaultValue)) + { + Context.Trace.WriteLine("No default UI supplied."); + return false; + } else { Context.Trace.WriteLine($"Using default UI helper: '{defaultValue}'."); From 4851182f8fb686ae35635bba665d81d2508a2ae1 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 21 Nov 2022 13:58:24 -0800 Subject: [PATCH 2/5] env: ensure we don't return empty PATH vars on Windows Ensure that we don't return empty `PATH` values on Windows when splitting. It's common that on Windows the `PATH` can be inadvertently modified such that an empty value is included, and this can lead to weird resolution of executables using the current directory. As it stands, no existing codepaths would trigger this today when invoked by Git for Windows, but let's make sure this never happens in the future by mistake! --- src/shared/Core/Interop/Windows/WindowsEnvironment.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/shared/Core/Interop/Windows/WindowsEnvironment.cs b/src/shared/Core/Interop/Windows/WindowsEnvironment.cs index c438582c9..b85979d66 100644 --- a/src/shared/Core/Interop/Windows/WindowsEnvironment.cs +++ b/src/shared/Core/Interop/Windows/WindowsEnvironment.cs @@ -22,7 +22,10 @@ internal WindowsEnvironment(IFileSystem fileSystem, IReadOnlyDictionary Date: Mon, 21 Nov 2022 12:54:45 -0800 Subject: [PATCH 3/5] app: fix installation dir calculation On Windows, when invoked as a partial name (e.g., `manager`) our command line argv[0] is just a file name, not a full file path! We incorrectly assumed that `Environment.GetCommandLineArgs()[0]` was always absolute! We should instead use `AppContext.BaseDirectory` that, in all circumstances tested, all on TFMs and OSs, all publish options, and as a .NET tool... returns the expected full, absolute path to the installation directory. --- src/shared/Core/ApplicationBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/Core/ApplicationBase.cs b/src/shared/Core/ApplicationBase.cs index fd78596a4..c2e1c05b6 100644 --- a/src/shared/Core/ApplicationBase.cs +++ b/src/shared/Core/ApplicationBase.cs @@ -91,7 +91,7 @@ public static string GetEntryApplicationPath() public static string GetInstallationDirectory() { - return Path.GetDirectoryName(Environment.GetCommandLineArgs()[0]); + return AppContext.BaseDirectory; } /// From f2d6cebabd93a7f4294c40162950676712f632cd Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 21 Nov 2022 13:39:19 -0800 Subject: [PATCH 4/5] app: fix bug in oldname warning on Windows Fix a bug in the old-name warning detection that only presents on Windows when bundled with Git for Windows, and when configured using the shorthand config `credential.helper=manager-core`. In this scenario, our argv[0] is missing the ".exe" extension (that's actually present in the file name of course) because ... mingw reasons. Instead of matching "git-credential-manager-core.exe" on Windows, just strip ".exe" if present on Windows, and always match "git-credential-manager-core" on all platforms. --- src/shared/Git-Credential-Manager/Program.cs | 24 +++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/shared/Git-Credential-Manager/Program.cs b/src/shared/Git-Credential-Manager/Program.cs index 5e463b504..14f20d13a 100644 --- a/src/shared/Git-Credential-Manager/Program.cs +++ b/src/shared/Git-Credential-Manager/Program.cs @@ -43,16 +43,24 @@ public static void Main(string[] args) // // On UNIX systems we do the same check, except instead of a copy we use a symlink. // - string oldName = PlatformUtils.IsWindows() - ? "git-credential-manager-core.exe" - : "git-credential-manager-core"; - if (appPath?.EndsWith(oldName, StringComparison.OrdinalIgnoreCase) ?? false) + if (!string.IsNullOrWhiteSpace(appPath)) { - context.Streams.Error.WriteLine( - "warning: git-credential-manager-core was renamed to git-credential-manager"); - context.Streams.Error.WriteLine( - $"warning: see {Constants.HelpUrls.GcmExecRename} for more information"); + // Trim any (.exe) file extension if we're on Windows + // Note that in some circumstances (like being called by Git when config is set + // to just `helper = manager-core`) we don't always have ".exe" at the end. + if (PlatformUtils.IsWindows() && appPath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) + { + appPath = appPath.Substring(0, appPath.Length - 4); + } + + if (appPath.EndsWith("git-credential-manager-core", StringComparison.OrdinalIgnoreCase)) + { + context.Streams.Error.WriteLine( + "warning: git-credential-manager-core was renamed to git-credential-manager"); + context.Streams.Error.WriteLine( + $"warning: see {Constants.HelpUrls.GcmExecRename} for more information"); + } } // Register all supported host providers at the normal priority. From b848716411992008c8545fb2e5a0d5d3c6b793ed Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 21 Nov 2022 14:21:54 -0800 Subject: [PATCH 5/5] app: fix Windows app-path resolution Fix another bug in how we compute the Application Executable path on Windows. It some cases we can still get a relative/non-absolute path to the entry executable. If we get a filename/non-path from the native API calls then return null and fallback to using process/module image resolution instead. For .NET Framework on Windows, this is OK. We will want to revisit this on .NET (Core) on Windows, if and when we get around to that... --- src/shared/Core/PlatformUtils.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/shared/Core/PlatformUtils.cs b/src/shared/Core/PlatformUtils.cs index 614677e6b..7a267c743 100644 --- a/src/shared/Core/PlatformUtils.cs +++ b/src/shared/Core/PlatformUtils.cs @@ -289,7 +289,11 @@ private static string GetWindowsEntryPath() IntPtr argv0Ptr = Marshal.ReadIntPtr(argvPtr); string argv0 = Marshal.PtrToStringAuto(argv0Ptr); Interop.Windows.Native.Kernel32.LocalFree(argvPtr); - return argv0; + + // If this isn't absolute then we should return null to prevent any + // caller that expect only an absolute path from mis-using this result. + // They will have to fall-back to other mechanisms for getting the entry path. + return Path.IsPathRooted(argv0) ? argv0 : null; } #endregion