Repository: microsoft/terminal (language: cpp)

Issue:
Profile matching doesn't work with paths containing unquoted whitespace
## Windows Terminal version

1.12.2922.0

## Steps to reproduce

### Issue 1 - paths with more than 1 whitespace

This code:
https://github.com/microsoft/terminal/blob/e064c1567548f098233b7a2d1101cb5201dec049/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp#L756-L757

When the loop is executed a second time (meaning: paths having more than one whitespace), `argv[2][-1]` should be set to whitespace, since `argv[1][-1]` still refers to the old whitespace between the first two arguments.

Due to this, the path
```
C:\foo - bar\baz.exe 60 120
```

is resolved to
```cpp
"C:\\foo - bar\\baz.exe 60 120"
```

when it should be
```cpp
"C:\\foo - bar\\baz.exe\060\0120"
```

Since `SearchPathW` will never have been given a valid path, path resolution will fail entirely and the input path be returned unmodified.

### Issue 2 - path resolution picks the first matching directory

This code:
https://github.com/microsoft/terminal/blob/e064c1567548f098233b7a2d1101cb5201dec049/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp#L717-L719

`SearchPathW` resolves to any path including directories and not just to file paths.

If you have
* a directory at `C:\foo`
* an executable at `C:\foo - bar\baz.exe`

then the path
```
C:\foo - bar\baz.exe 60 120
```

will be resolved to
```cpp
"C:\\foo\0-\0bar\\baz.exe\060\0120"
        ^^ ^^
```

when it should be
```cpp
"C:\\foo - bar\\baz.exe\060\0120"
```

Profile matching will still succeed since the path of the handed off process is normalized the exact same way. However matching will be buggy, since it's ambigious with any path that actually has arguments in these places.


Discussion / hints:

Fix patch:
diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt
index 31b249c76ec..13d32233b58 100644
--- a/.github/actions/spelling/allow/apis.txt
+++ b/.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,7 +190,6 @@ UOI
 UPDATEINIFILE
 userenv
 USEROBJECTFLAGS
-WSF
 wcsstr
 wcstoui
 winmain
@@ -196,6 +197,7 @@ winsta
 winstamin
 wmemcmp
 wpc
+WSF
 wsregex
 wwinmain
 xchg
diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
index 100634874c8..841c43a8827 100644
--- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
+++ b/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.
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index e3916eff51b..fba878395e9 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/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,39 +722,39 @@ 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;
         }
@@ -753,19 +762,19 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine)
         // 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);
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
index 716ccf64d87..772f7625beb 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h
+++ b/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;


Test patch: