Fix profile matching for paths containing unquoted whitespace (#12348)

The previous code had two bugs for:
* paths with more than 1 whitespace
  The code joins the argv array by replacing null-word terminators with
  whitespace. Unfortunately it always referred to the separator between
  `argv[0]` and `argv[1]` for this instead of continuing to join
  those between 1 and 2, etc.
* paths sharing a common prefix with another directory
  `SearchPathW` returns paths that aren't necessarily paths to files.
  A call to `GetFileAttributesW` was added, ensuring we only resolve file paths.

## PR Checklist
* [x] Closes #12345
* [x] I work here
* [ ] Tests added/passed

## Validation Steps Performed
* Paths with more than 1 whitespace resolve correctly ✅
* Paths with neighboring directories sharing a common prefix resolve correctly ✅
* Tests added ✅

(cherry picked from commit 3171a89)

.github/actions/spelling/allow/apis.txt

@@ -10,16 +10,16 @@ bitfields
 BUILDBRANCH
 BUILDMSG
 BUILDNUMBER
-BYPOSITION
 BYCOMMAND
+BYPOSITION
 charconv
 CLASSNOTAVAILABLE
 cmdletbinding
 COLORPROPERTY
 colspan
 COMDLG
-comparand
 commandlinetoargv
+comparand
 cstdint
 CXICON
 CYICON
@@ -52,8 +52,8 @@ hotkeys
 href
 hrgn
 HTCLOSE
-HWINSTA
 hwinsta
+HWINSTA
 IActivation
 IApp
 IAppearance
@@ -76,8 +76,8 @@ IObject
 iosfwd
 IPackage
 IPeasant
-isspace
 ISetup
+isspace
 IStorage
 istream
 IStringable
@@ -94,14 +94,14 @@ lround
 Lsa
 lsass
 LSHIFT
+memicmp
 MENUCOMMAND
 MENUDATA
 MENUINFO
 MENUITEMINFOW
-memicmp
-mptt
 MOUSELEAVE
 mov
+mptt
 msappx
 MULTIPLEUSE
 NCHITTEST
@@ -125,6 +125,7 @@ oaidl
 ocidl
 ODR
 offsetof
+ofstream
 onefuzz
 osver
 OSVERSIONINFOEXW
@@ -176,6 +177,7 @@ THEMECHANGED
 tlg
 TME
 tmp
+tmpdir
 tolower
 toupper
 TRACKMOUSEEVENT
@@ -188,14 +190,14 @@ UOI
 UPDATEINIFILE
 userenv
 USEROBJECTFLAGS
-WSF
 wcsstr
 wcstoui
 winmain
 winsta
 winstamin
 wmemcmp
 wpc
+WSF
 wsregex
 wwinmain
 xchg

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

@@ -38,6 +38,7 @@ namespace SettingsModelLocalTests
         TEST_METHOD(TryCreateWinRTType);
         TEST_METHOD(TestTerminalArgsForBinding);
         TEST_METHOD(CommandLineToArgvW);
+        TEST_METHOD(NormalizeCommandLine);
         TEST_METHOD(GetProfileForArgsWithCommandline);
         TEST_METHOD(MakeSettingsForProfile);
         TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);
@@ -120,6 +121,59 @@ namespace SettingsModelLocalTests
         VERIFY_ARE_EQUAL(0, memcmp(beg, expectedArgv.data(), expectedArgv.size()));
     }
 
+    // This unit test covers GH#12345.
+    // * paths with more than 1 whitespace
+    // * paths sharing a common prefix with another directory
+    void TerminalSettingsTests::NormalizeCommandLine()
+    {
+        using namespace std::string_literals;
+
+        static constexpr auto touch = [](const auto& path) {
+            std::ofstream file{ path };
+        };
+
+        std::wstring guid;
+        {
+            GUID g{};
+            THROW_IF_FAILED(CoCreateGuid(&g));
+            guid = fmt::format(
+                L"{:08x}-{:04x}-{:04x}-{:02x}{:02x}-{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}",
+                g.Data1,
+                g.Data2,
+                g.Data3,
+                g.Data4[0],
+                g.Data4[1],
+                g.Data4[2],
+                g.Data4[3],
+                g.Data4[4],
+                g.Data4[5],
+                g.Data4[6],
+                g.Data4[7]);
+        }
+
+        const auto tmpdir = std::filesystem::temp_directory_path();
+        const auto dir1 = tmpdir / guid;
+        const auto dir2 = tmpdir / (guid + L" two");
+        const auto file1 = dir1 / L"file 1.exe";
+        const auto file2 = dir2 / L"file 2.exe";
+
+        const auto cleanup = wil::scope_exit([&]() {
+            std::error_code ec;
+            remove_all(dir1, ec);
+            remove_all(dir2, ec);
+        });
+
+        create_directory(dir1);
+        create_directory(dir2);
+        touch(file1);
+        touch(file2);
+
+        const auto commandLine = file2.native() + LR"( -foo "bar1 bar2" -baz)"s;
+        const auto expected = file2.native() + L"\0-foo\0bar1 bar2\0-baz"s;
+        const auto actual = implementation::CascadiaSettings::NormalizeCommandLine(commandLine.c_str());
+        VERIFY_ARE_EQUAL(expected, actual);
+    }
+
     void TerminalSettingsTests::GetProfileForArgsWithCommandline()
     {
         // I'm exclusively using cmd.exe as I know exactly where it resides at.

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -593,7 +593,7 @@ Model::Profile CascadiaSettings::GetProfileForArgs(const Model::NewTerminalArgs&
 Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring& commandLine) const
 {
     // We're going to cache all the command lines we got, as
-    // _normalizeCommandLine is a relatively heavy operation.
+    // NormalizeCommandLine is a relatively heavy operation.
     std::call_once(_commandLinesCacheOnce, [this]() {
         _commandLinesCache.reserve(_allProfiles.Size());
 
@@ -612,7 +612,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring&
 
             try
             {
-                _commandLinesCache.emplace_back(_normalizeCommandLine(cmd.c_str()), profile);
+                _commandLinesCache.emplace_back(NormalizeCommandLine(cmd.c_str()), profile);
             }
             CATCH_LOG()
         }
@@ -631,7 +631,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring&
 
     try
     {
-        const auto needle = _normalizeCommandLine(commandLine.c_str());
+        const auto needle = NormalizeCommandLine(commandLine.c_str());
 
         // til::starts_with(string, prefix) will always return false if prefix.size() > string.size().
         // --> Using binary search we can safely skip all items in _commandLinesCache where .first.size() > needle.size().
@@ -678,7 +678,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring&
 // is considered a compatible profile with
 //   "C:\Program Files\PowerShell\7\pwsh.exe -WorkingDirectory ~"
 // as it shares the same (normalized) prefix.
-std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine)
+std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
 {
     // Turn "%SystemRoot%\System32\cmd.exe" into "C:\WINDOWS\System32\cmd.exe".
     // We do this early, as environment variables might occur anywhere in the commandLine.
@@ -692,6 +692,15 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine)
     wil::unique_hlocal_ptr<PWSTR[]> argv{ CommandLineToArgvW(normalized.c_str(), &argc) };
     THROW_LAST_ERROR_IF(!argc);
 
+    // The index of the first argument in argv for our executable in argv[0].
+    // Given {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"} this will be 1.
+    int startOfArguments = 1;
+    // Returns true if the executable in argv[0] to argv[startOfArguments - 1]
+    // has any further arguments in argv[startOfArguments] to argv[argc - 1].
+    const auto hasTrailingArguments = [&]() noexcept {
+        return (argc - startOfArguments) > 1;
+    };
+
     // The given commandLine should start with an executable name or path.
     // For instance given the following argv arrays:
     // * {"C:\WINDOWS\System32\cmd.exe"}
@@ -713,59 +722,59 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine)
         // CreateProcessW uses RtlGetExePath to get the lpPath for SearchPathW.
         // The difference between the behavior of SearchPathW if lpPath is nullptr and what RtlGetExePath returns
         // seems to be mostly whether SafeProcessSearchMode is respected and the support for relative paths.
-        // Windows Terminal makes the use relative paths rather impractical which is why we simply dropped the call to RtlGetExePath.
+        // Windows Terminal makes the use of relative paths rather impractical which is why we simply dropped the call to RtlGetExePath.
         const auto status = wil::SearchPathW(nullptr, argv[0], L".exe", normalized);
 
         if (status == S_OK)
         {
-            std::filesystem::path path{ std::move(normalized) };
+            const auto attributes = GetFileAttributesW(normalized.c_str());
 
-            // ExpandEnvironmentStringsW() might have returned a string that's not in the canonical capitalization.
-            // For instance %SystemRoot% is set to C:\WINDOWS on my system (ugh), even though the path is actually C:\Windows.
-            // We need to fix this as case-sensitive path comparisons will fail otherwise (Windows supports case-sensitive file systems).
-            // If we fail to resolve the path for whatever reason (pretty unlikely given that SearchPathW found it)
-            // we fall back to leaving the path as is. Better than throwing a random exception and making this unusable.
+            if (attributes != INVALID_FILE_ATTRIBUTES && WI_IsFlagClear(attributes, FILE_ATTRIBUTE_DIRECTORY))
             {
-                std::error_code ec;
-                auto canonicalPath = std::filesystem::canonical(path, ec);
-                if (!ec)
+                std::filesystem::path path{ std::move(normalized) };
+
+                // canonical() will resolve symlinks, etc. for us.
                 {
-                    path = std::move(canonicalPath);
+                    std::error_code ec;
+                    auto canonicalPath = std::filesystem::canonical(path, ec);
+                    if (!ec)
+                    {
+                        path = std::move(canonicalPath);
+                    }
                 }
-            }
 
-            // std::filesystem::path has no way to extract the internal path.
-            // So about that.... I own you, computer. Give me that path.
-            normalized = std::move(const_cast<std::wstring&>(path.native()));
-            break;
+                // std::filesystem::path has no way to extract the internal path.
+                // So about that.... I own you, computer. Give me that path.
+                normalized = std::move(const_cast<std::wstring&>(path.native()));
+                break;
+            }
         }
-
         // If the file path couldn't be found by SearchPathW this could be the result of us being given a commandLine
         // like "C:\foo bar\baz.exe -arg" which is resolved to the argv array {"C:\foo", "bar\baz.exe", "-arg"}.
         // Just like CreateProcessW() we thus try to concatenate arguments until we successfully resolve a valid path.
         // Of course we can only do that if we have at least 2 remaining arguments in argv.
         // All other error types aren't handled at the moment.
-        if (argc < 2 || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
+        else if (!hasTrailingArguments() || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
         {
             break;
         }
 
         // As described in the comment right above, we concatenate arguments in an attempt to resolve a valid path.
         // The code below turns argv from {"C:\foo", "bar\baz.exe", "-arg"} into {"C:\foo bar\baz.exe", "-arg"}.
         // The code abuses the fact that CommandLineToArgvW allocates all arguments back-to-back on the heap separated by '\0'.
-        argv[1][-1] = L' ';
-        --argc;
+        argv[startOfArguments][-1] = L' ';
+        ++startOfArguments;
     }
 
     // We've (hopefully) finished resolving the path to the executable.
     // We're now going to append all remaining arguments to the resulting string.
     // If argv is {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"},
     // then we'll get "C:\Program Files\PowerShell\7\pwsh.exe\0-WorkingDirectory\0~"
-    if (argc > 1)
+    if (hasTrailingArguments())
     {
         // normalized contains a canonical form of argv[0] at this point.
         // -1 allows us to include the \0 between argv[0] and argv[1] in the call to append().
-        const auto beg = argv[1] - 1;
+        const auto beg = argv[startOfArguments] - 1;
         const auto lastArg = argv[argc - 1];
         const auto end = lastArg + wcslen(lastArg);
         normalized.append(beg, end);

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

@@ -134,6 +134,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         winrt::hstring GetSerializationErrorMessage() const;
 
         // defterm
+        static std::wstring NormalizeCommandLine(LPCWSTR commandLine);
         static bool IsDefaultTerminalAvailable() noexcept;
         static bool IsDefaultTerminalSet() noexcept;
         winrt::Windows::Foundation::Collections::IObservableVector<Model::DefaultTerminal> DefaultTerminals() noexcept;
@@ -142,7 +143,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
     private:
         static const std::filesystem::path& _settingsPath();
-        static std::wstring _normalizeCommandLine(LPCWSTR commandLine);
 
         winrt::com_ptr<implementation::Profile> _createNewProfile(const std::wstring_view& name) const;
         Model::Profile _getProfileForCommandLine(const winrt::hstring& commandLine) const;