Repository: microsoft/terminal (language: cpp)

Issue:
Run as Admin shortcut + elevate:true -> crash loop
### Windows Terminal version

1.14.2281.0 or preview 1.15.2282.0

### Windows build number

10.0.19044.0

### Other Software

_No response_

### Steps to reproduce

1. Install terminal regardless preview version or  not.
2. Turn on the "Run this profile as Administrator" in Profile->default
3. Restart the terminal.

### Expected Behavior

_No response_

### Actual Behavior

https://user-images.githubusercontent.com/53345636/188541035-ff4005a1-dbfb-4ed7-8c34-3e9f8367c068.mp4

I also tried run terminal as administrator,but it seemed didn't work.


Discussion / hints:
Just xlinking a few issues:

* #12369
* #13675
* #13908


Something weird is going on, for sure
> Just xlinking a few issues:
> 
> * [The new 'Run as Administrator' feature returns a 'cannot find WindowsTerminal.exe' error #12369](https://github.com/microsoft/terminal/issues/12369)
> * [Run as administrator profiles do not run until terminal is ran as administrator. #13675](https://github.com/microsoft/terminal/issues/13675)
> * [Open a new Administrator instance by pressing the '+' in the tab bar while pressing the Ctrl key is not working #13908](https://github.com/microsoft/terminal/issues/13908)
> 
> Something weird is going on, for sure

I read them before I raised the issue.I logged in as my admin user,the only user on my windows,and tried to install Terminal from the MicrosoftStore,yet it still didn't work.
Oh man, you know what, I think another thread might have a better repro. Copying deets here.

> If a shortcut properties are set to run wt.exe as Admin
> AND
> Run as Admin is also set in wt app settings, new tab or the app will not start, and DesktopWindowXamlSource continuously flashes in taskbar.
>
> Terminal ver = 1.15.2874.0
> Windows ver = 10.0.19044
Repro'd a version of the crash loop with the default `Administrator` account. The issue stems from the way IsElevated() is checking for elevation.

edit: these instructions reproduce the bug; I'm not aware of a fix.

1) Enable the default `Administrator` account on your machine (this account is special)
2) Log into the account (by default no password was set); I used the "Switch User" feature (note: you probably want to set a password and/or disable the account later)
3) Launch Terminal and change the settings to include a default elevate=true for all profiles
4) Exit Terminal
5) Launch Terminal (now w/ all profiles default to elevate); observe an infinite loop while it repeatedly tries to launch an elevated instance from the current instance.


<img width="646" alt="image" src="https://user-images.githubusercontent.com/3619564/211223601-40fd21b1-4e4a-4c09-9702-8b4c5982b0fa.png">

Change the default profile settings
```json
    "profiles": 
    {
        "defaults": { "elevate": true },
    ...
```

## Root Cause

I built a custom dev build with some extra logging in the utils.cpp IsElevated check. The built-in Administrator account meets the criteria that it has a `TokenElevationTypeDefault` token and `TokenIsElevated` is 1. This in turn causes `IsElevated()` to return `false`.

This interacts with `TerminalPage::ShouldImmediatelyHandoffToElevated` which knows it wants to launch an elevated profile, but  (mistakenly) thinks it is not currently elevated. This causes an infinite loop of `WindowsTerminal.exe` -> `elevate-shim.exe` while it tries to get an elevated process.

In short, the check for `TokenElevationTypeDefault ` short circuits the elevation check to false and creates an infinite loop.

https://github.com/microsoft/terminal/blob/main/src/types/utils.cpp#L664-L673

```cpp
            if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated)
            {
                // In this case, the user has UAC entirely disabled. This is sort of
                // weird, we treat this like the user isn't an admin at all. There's no
                // separation of powers, so the things we normally want to gate on
                // "having special powers" doesn't apply.
                //
                // See GH#7754, GH#11096
                return false;
            }
```

Related Docs:

**TokenElevationTypeDefault**: _Type 1 is a full token with no privileges removed or groups disabled. A full token is only used if User Account Control is disabled or if the user is the built-in Administrator account (for which UAC disabled by default), service account or local system account._

procmon trace (screenshot) of the loop on latest 1.16.10261.0. This is running under the default Administrator account on 10.0.22621.1105 with elevate=true in the defaults.

The sequence doesn't look right given some lost events, but the PID parent/child relationships are as expected.

WindowsTerminal.exe doesn't think it's elevated (per comment above); it's launching elevate-shim which is using shell:Appsfolder with the runas verb to launch WindowsTerminal elevated and the cycle repeats.

<img width="1431" alt="image" src="https://user-images.githubusercontent.com/3619564/215428635-8640b46c-76ae-4813-96a2-854013ea459f.png">

> ```cpp
>                 return false;
> ```

Ah, heck. This came up in an earlier code review that we didn't end up merging, too.

We need separate checks for `IsUserAnAdminInAWayThatBreaksSomeOfOurFeatures` and `IsUserAnAdminAtAll`. Names TBD. @zadjii-msft, do you remember when that came up? Was it for elevated-state?
![image](https://user-images.githubusercontent.com/18356694/222755794-ae991f6d-6b8e-49de-ac94-71efdb219a76.png)



Fix patch:
diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp
index c8677eb7f82..9cb4e9de1b5 100644
--- a/src/cascadia/TerminalApp/AppLogic.cpp
+++ b/src/cascadia/TerminalApp/AppLogic.cpp
@@ -132,7 +132,12 @@ namespace winrt::TerminalApp::implementation
         // cause you to chase down the rabbit hole of "why is App not
         // registered?" when it definitely is.
 
-        _isElevated = ::Microsoft::Console::Utils::IsElevated();
+        // The TerminalPage has to be constructed during our construction, to
+        // make sure that there's a terminal page for callers of
+        // SetTitleBarContent
+
+        _isElevated = ::Microsoft::Console::Utils::IsRunningElevated();
+        _canDragDrop = ::Microsoft::Console::Utils::CanUwpDragDrop();
 
         _reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() {
             if (auto self{ weakSelf.get() })
@@ -163,10 +168,14 @@ namespace winrt::TerminalApp::implementation
     // - <none> - reports internal state
     // Return Value:
     // - True if elevated, false otherwise.
-    bool AppLogic::IsElevated() const noexcept
+    bool AppLogic::IsRunningElevated() const noexcept
     {
         return _isElevated;
     }
+    bool AppLogic::CanDragDrop() const noexcept
+    {
+        return _canDragDrop;
+    }
 
     // Method Description:
     // - Called by UWP context invoker to let us know that we may have to change some of our behaviors
diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h
index 821ec0f5f10..c7eb2eaee96 100644
--- a/src/cascadia/TerminalApp/AppLogic.h
+++ b/src/cascadia/TerminalApp/AppLogic.h
@@ -49,7 +49,8 @@ namespace winrt::TerminalApp::implementation
         void Create();
         bool IsUwp() const noexcept;
         void RunAsUwp();
-        bool IsElevated() const noexcept;
+        bool IsRunningElevated() const noexcept;
+        bool CanDragDrop() const noexcept;
         void ReloadSettings();
 
         bool HasSettingsStartupActions() const noexcept;
@@ -72,6 +73,7 @@ namespace winrt::TerminalApp::implementation
     private:
         bool _isUwp{ false };
         bool _isElevated{ false };
+        bool _canDragDrop{ false };
 
         Microsoft::Terminal::Settings::Model::CascadiaSettings _settings{ nullptr };
 
diff --git a/src/cascadia/TerminalApp/AppLogic.idl b/src/cascadia/TerminalApp/AppLogic.idl
index a5db001de67..3091847cf8b 100644
--- a/src/cascadia/TerminalApp/AppLogic.idl
+++ b/src/cascadia/TerminalApp/AppLogic.idl
@@ -25,7 +25,8 @@ namespace TerminalApp
 
         Boolean IsUwp();
         void RunAsUwp();
-        Boolean IsElevated();
+        Boolean IsRunningElevated();
+        Boolean CanDragDrop();
 
         Boolean HasSettingsStartupActions();
 
diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
index 6cfbc4bdf0b..b63a3750ef3 100644
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -153,27 +153,26 @@ namespace winrt::TerminalApp::implementation
         _systemRowsToScroll = _ReadSystemRowsToScroll();
     }
 
-    bool TerminalPage::IsElevated() const noexcept
+    bool TerminalPage::IsRunningElevated() const noexcept
     {
-        // use C++11 magic statics to make sure we only do this once.
-        // This won't change over the lifetime of the application
-
-        static const auto isElevated = []() {
-            // *** THIS IS A SINGLETON ***
-            auto result = false;
-
-            // GH#2455 - Make sure to try/catch calls to Application::Current,
-            // because that _won't_ be an instance of TerminalApp::App in the
-            // LocalTests
-            try
-            {
-                result = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsElevated();
-            }
-            CATCH_LOG();
-            return result;
-        }();
-
-        return isElevated;
+        // GH#2455 - Make sure to try/catch calls to Application::Current,
+        // because that _won't_ be an instance of TerminalApp::App in the
+        // LocalTests
+        try
+        {
+            return Application::Current().as<TerminalApp::App>().Logic().IsRunningElevated();
+        }
+        CATCH_LOG();
+        return false;
+    }
+    bool TerminalPage::CanDragDrop() const noexcept
+    {
+        try
+        {
+            return Application::Current().as<TerminalApp::App>().Logic().CanDragDrop();
+        }
+        CATCH_LOG();
+        return true;
     }
 
     void TerminalPage::Create()
@@ -186,11 +185,11 @@ namespace winrt::TerminalApp::implementation
         _tabView = _tabRow.TabView();
         _rearranging = false;
 
-        const auto isElevated = IsElevated();
+        const auto canDragDrop = CanDragDrop();
 
         _tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });
-        _tabView.CanReorderTabs(!isElevated);
-        _tabView.CanDragTabs(!isElevated);
+        _tabView.CanReorderTabs(canDragDrop);
+        _tabView.CanDragTabs(canDragDrop);
         _tabView.TabDragStarting({ get_weak(), &TerminalPage::_TabDragStarted });
         _tabView.TabDragCompleted({ get_weak(), &TerminalPage::_TabDragCompleted });
 
@@ -312,7 +311,7 @@ namespace winrt::TerminalApp::implementation
         // Setup mouse vanish attributes
         SystemParametersInfoW(SPI_GETMOUSEVANISH, 0, &_shouldMouseVanish, false);
 
-        _tabRow.ShowElevationShield(IsElevated() && _settings.GlobalSettings().ShowAdminShield());
+        _tabRow.ShowElevationShield(IsRunningElevated() && _settings.GlobalSettings().ShowAdminShield());
 
         // Store cursor, so we can restore it, e.g., after mouse vanishing
         // (we'll need to adapt this logic once we make cursor context aware)
@@ -339,7 +338,7 @@ namespace winrt::TerminalApp::implementation
         // GH#12267: Don't forget about defterm handoff here. If we're being
         // created for embedding, then _yea_, we don't need to handoff to an
         // elevated window.
-        if (!_startupActions || IsElevated() || _shouldStartInboundListener || _startupActions.Size() == 0)
+        if (!_startupActions || IsRunningElevated() || _shouldStartInboundListener || _startupActions.Size() == 0)
         {
             // there aren't startup actions, or we're elevated. In that case, go for it.
             return false;
@@ -1128,7 +1127,7 @@ namespace winrt::TerminalApp::implementation
                         WI_IsFlagSet(lAltState, CoreVirtualKeyStates::Down) &&
                         WI_IsFlagSet(rAltState, CoreVirtualKeyStates::Down);
 
-        const auto dispatchToElevatedWindow = ctrlPressed && !IsElevated();
+        const auto dispatchToElevatedWindow = ctrlPressed && !IsRunningElevated();
 
         if ((shiftPressed || dispatchToElevatedWindow) && !debugTap)
         {
@@ -2876,7 +2875,7 @@ namespace winrt::TerminalApp::implementation
         // want to create an animation.
         WUX::Media::Animation::Timeline::AllowDependentAnimations(!_settings.GlobalSettings().DisableAnimations());
 
-        _tabRow.ShowElevationShield(IsElevated() && _settings.GlobalSettings().ShowAdminShield());
+        _tabRow.ShowElevationShield(IsRunningElevated() && _settings.GlobalSettings().ShowAdminShield());
 
         Media::SolidColorBrush transparent{ Windows::UI::Colors::Transparent() };
         _tabView.Background(transparent);
@@ -4016,7 +4015,7 @@ namespace winrt::TerminalApp::implementation
     {
         // Try to handle auto-elevation
         const auto requestedElevation = controlSettings.DefaultSettings().Elevate();
-        const auto currentlyElevated = IsElevated();
+        const auto currentlyElevated = IsRunningElevated();
 
         // We aren't elevated, but we want to be.
         if (requestedElevation && !currentlyElevated)
diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h
index a99854374ca..10af6dff877 100644
--- a/src/cascadia/TerminalApp/TerminalPage.h
+++ b/src/cascadia/TerminalApp/TerminalPage.h
@@ -126,7 +126,8 @@ namespace winrt::TerminalApp::implementation
 
         TerminalApp::WindowProperties WindowProperties() const noexcept { return _WindowProperties; };
 
-        bool IsElevated() const noexcept;
+        bool CanDragDrop() const noexcept;
+        bool IsRunningElevated() const noexcept;
 
         void OpenSettingsUI();
         void WindowActivated(const bool activated);
diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp
index eb0ef7c78b9..1041f176300 100644
--- a/src/cascadia/TerminalApp/TerminalWindow.cpp
+++ b/src/cascadia/TerminalApp/TerminalWindow.cpp
@@ -201,35 +201,6 @@ namespace winrt::TerminalApp::implementation
         return isUwp;
     }
 
-    // Method Description:
-    // - Called around the codebase to discover if Terminal is running elevated
-    // Arguments:
-    // - <none> - reports internal state
-    // Return Value:
-    // - True if elevated, false otherwise.
-    bool TerminalWindow::IsElevated() const noexcept
-    {
-        // use C++11 magic statics to make sure we only do this once.
-        // This won't change over the lifetime of the application
-
-        static const auto isElevated = []() {
-            // *** THIS IS A SINGLETON ***
-            auto result = false;
-
-            // GH#2455 - Make sure to try/catch calls to Application::Current,
-            // because that _won't_ be an instance of TerminalApp::App in the
-            // LocalTests
-            try
-            {
-                result = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsElevated();
-            }
-            CATCH_LOG();
-            return result;
-        }();
-
-        return isElevated;
-    }
-
     // Method Description:
     // - Build the UI for the terminal app. Before this method is called, it
     //   should not be assumed that the TerminalApp is usable. The Settings
diff --git a/src/cascadia/TerminalApp/TerminalWindow.h b/src/cascadia/TerminalApp/TerminalWindow.h
index 1a446010761..0f9b086591c 100644
--- a/src/cascadia/TerminalApp/TerminalWindow.h
+++ b/src/cascadia/TerminalApp/TerminalWindow.h
@@ -65,7 +65,6 @@ namespace winrt::TerminalApp::implementation
         void Create();
 
         bool IsUwp() const noexcept;
-        bool IsElevated() const noexcept;
 
         void Quit();
 
diff --git a/src/cascadia/TerminalApp/TerminalWindow.idl b/src/cascadia/TerminalApp/TerminalWindow.idl
index a945f88e007..0dc6550e7b8 100644
--- a/src/cascadia/TerminalApp/TerminalWindow.idl
+++ b/src/cascadia/TerminalApp/TerminalWindow.idl
@@ -51,8 +51,6 @@ namespace TerminalApp
         // registered?" when it definitely is.
         void Create();
 
-        Boolean IsElevated();
-
         Boolean HasCommandlineArguments();
 
         Int32 SetStartupCommandline(String[] commands);
diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp
index 59ebb0ccd08..6f684f91d66 100644
--- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp
+++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp
@@ -177,7 +177,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             //   from state.json. We'll then load the Local props from
             //   `elevated-state.json`
             // - If we're unelevated, then load _everything_ from state.json.
-            if (::Microsoft::Console::Utils::IsElevated())
+            if (::Microsoft::Console::Utils::IsRunningElevated())
             {
                 // Only load shared properties if we're elevated
                 FromJson(root, FileSource::Shared);
@@ -225,7 +225,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         //
         // After that's done, we'll write our Local properties into
         // elevated-state.json.
-        if (::Microsoft::Console::Utils::IsElevated())
+        if (::Microsoft::Console::Utils::IsRunningElevated())
         {
             std::string errs;
             std::unique_ptr<Json::CharReader> reader{ Json::CharReaderBuilder{}.newCharReader() };
@@ -353,7 +353,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     //   permissions, so we don't potentially read malicious data.
     std::optional<std::string> ApplicationState::_readLocalContents() const
     {
-        return ::Microsoft::Console::Utils::IsElevated() ?
+        return ::Microsoft::Console::Utils::IsRunningElevated() ?
                    ReadUTF8FileIfExists(_elevatedPath, true) :
                    ReadUTF8FileIfExists(_sharedPath, false);
     }
@@ -374,7 +374,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     //   will atomically write to `user-state.json`
     void ApplicationState::_writeLocalContents(const std::string_view content) const
     {
-        if (::Microsoft::Console::Utils::IsElevated())
+        if (::Microsoft::Console::Utils::IsRunningElevated())
         {
             // DON'T use WriteUTF8FileAtomic, which will write to a temporary file
             // then rename that file to the final filename. That actually lets us
diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp
index 3dbe7928c9a..45e16a049de 100644
--- a/src/types/inc/utils.hpp
+++ b/src/types/inc/utils.hpp
@@ -96,7 +96,8 @@ namespace Microsoft::Console::Utils
 
     GUID CreateV5Uuid(const GUID& namespaceGuid, const std::span<const std::byte> name);
 
-    bool IsElevated();
+    bool CanUwpDragDrop();
+    bool IsRunningElevated();
 
     // This function is only ever used by the ConPTY connection in
     // TerminalConnection. However, that library does not have a good system of
diff --git a/src/types/utils.cpp b/src/types/utils.cpp
index dac6212c6e0..94813bfc967 100644
--- a/src/types/utils.cpp
+++ b/src/types/utils.cpp
@@ -653,9 +653,18 @@ GUID Utils::CreateV5Uuid(const GUID& namespaceGuid, const std::span<const std::b
     return EndianSwap(newGuid);
 }
 
-bool Utils::IsElevated()
+// * Elevated users cannot use the modern drag drop experience. This is
+//   specifically normal users running the Terminal as admin
+// * The Default Administrator, who does not have a split token, CAN drag drop
+//   perfectly fine. So in that case, we want to return false.
+// * This has to be kept separate from IsRunningElevated, which is exclusively
+//   used for "is this instance running as admin".
+bool Utils::CanUwpDragDrop()
 {
-    static auto isElevated = []() {
+    // There's a lot of wacky double negatives here so that the logic is
+    // basically the same as IsRunningElevated, but the end result semantically
+    // makes sense as "CanDragDrop".
+    static auto isDragDropBroken = []() {
         try
         {
             wil::unique_handle processToken{ GetCurrentProcessToken() };
@@ -670,8 +679,30 @@ bool Utils::IsElevated()
                 //
                 // See GH#7754, GH#11096
                 return false;
+                // drag drop is _not_ broken -> they _can_ drag drop
             }
 
+            // If they are running admin, they cannot drag drop.
+            return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS);
+        }
+        catch (...)
+        {
+            LOG_CAUGHT_EXCEPTION();
+            // This failed? That's very peculiar indeed. Let's err on the side
+            // of "drag drop is broken", just in case.
+            return true;
+        }
+    }();
+
+    return !isDragDropBroken;
+}
+
+// See CanUwpDragDrop, GH#13928 for why this is different.
+bool Utils::IsRunningElevated()
+{
+    static auto isElevated = []() {
+        try
+        {
             return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS);
         }
         catch (...)


Test patch: