Invalidate nested command with no valid subcommands (#9495)

Currently, when loading command with sub-commands that fail to parse,
we result with command that:
* Is not considered nested (has no sub-commands)
* Has no action of its own

The commit contains a few changes:
1. Protection in the dispatch that will prevent NPE
2. Change in the command parsing that will no load
a command if all its sub-commands failed to parse
3. We will add a warning in this case (the solution is somewhat
hacky, due to the hack that was there previously)

When such command is passed to a dispatch we crash with NPE.

Closes #9448

(cherry picked from commit 28da6c4)

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

@@ -79,6 +79,7 @@ namespace SettingsModelLocalTests
         TEST_METHOD(TestCommandsAndKeybindings);
 
         TEST_METHOD(TestNestedCommandWithoutName);
+        TEST_METHOD(TestNestedCommandWithBadSubCommands);
         TEST_METHOD(TestUnbindNestedCommand);
         TEST_METHOD(TestRebindNestedCommand);
 
@@ -1376,7 +1377,7 @@ namespace SettingsModelLocalTests
                 },
                 {
                     "name": "parent",
-                    "commands": [                        
+                    "commands": [
                         { "command": { "action": "setColorScheme", "colorScheme": "invalidScheme" } }
                     ]
                 }
@@ -1403,11 +1404,11 @@ namespace SettingsModelLocalTests
                 },
                 {
                     "name": "grandparent",
-                    "commands": [                        
+                    "commands": [
                         {
                             "name": "parent",
                             "commands": [
-                                { 
+                                {
                                     "command": { "action": "setColorScheme", "colorScheme": "invalidScheme" }
                                 }
                             ]
@@ -1909,7 +1910,8 @@ namespace SettingsModelLocalTests
             "keybindings": [
                 { "command": { "action": "splitPane", "split":"auto" }, "keys": [ "ctrl+alt+t", "ctrl+a" ] },
                 { "command": { "action": "moveFocus" }, "keys": [ "ctrl+a" ] },
-                { "command": { "action": "resizePane" }, "keys": [ "ctrl+b" ] }
+                { "command": { "action": "resizePane" }, "keys": [ "ctrl+b" ] },
+                { "name": "invalid nested", "commands":[ { "name" : "hello" }, { "name" : "world" } ] }
             ]
         })" };
 
@@ -1918,18 +1920,20 @@ namespace SettingsModelLocalTests
 
         VERIFY_ARE_EQUAL(0u, settings->_globals->_keymap->_keyShortcuts.size());
 
-        VERIFY_ARE_EQUAL(3u, settings->_globals->_keybindingsWarnings.size());
+        VERIFY_ARE_EQUAL(4u, settings->_globals->_keybindingsWarnings.size());
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::TooManyKeysForChord, settings->_globals->_keybindingsWarnings.at(0));
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::MissingRequiredParameter, settings->_globals->_keybindingsWarnings.at(1));
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::MissingRequiredParameter, settings->_globals->_keybindingsWarnings.at(2));
+        VERIFY_ARE_EQUAL(SettingsLoadWarnings::FailedToParseSubCommands, settings->_globals->_keybindingsWarnings.at(3));
 
         settings->_ValidateKeybindings();
 
-        VERIFY_ARE_EQUAL(4u, settings->_warnings.Size());
+        VERIFY_ARE_EQUAL(5u, settings->_warnings.Size());
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::AtLeastOneKeybindingWarning, settings->_warnings.GetAt(0));
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::TooManyKeysForChord, settings->_warnings.GetAt(1));
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.GetAt(2));
         VERIFY_ARE_EQUAL(SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.GetAt(3));
+        VERIFY_ARE_EQUAL(SettingsLoadWarnings::FailedToParseSubCommands, settings->_warnings.GetAt(4));
     }
 
     void DeserializationTests::ValidateExecuteCommandlineWarning()
@@ -2316,6 +2320,53 @@ namespace SettingsModelLocalTests
         VERIFY_ARE_EQUAL(0u, commands.Size());
     }
 
+    void DeserializationTests::TestNestedCommandWithBadSubCommands()
+    {
+        // This test tests a nested command without a name specified. This type
+        // of command should just be ignored, since we can't auto-generate names
+        // for nested commands, they _must_ have names specified.
+
+        const std::string settingsJson{ R"(
+        {
+            "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1,
+                    "commandline": "cmd.exe"
+                }
+            ],
+            "actions": [
+                {
+                    "name": "nested command",
+                    "commands": [
+                        {
+                            "name": "child1"
+                        },
+                        {
+                            "name": "child2"
+                        }
+                    ]
+                },
+            ],
+            "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors.
+        })" };
+
+        VerifyParseSucceeded(settingsJson);
+
+        auto settings = winrt::make_self<implementation::CascadiaSettings>();
+        settings->_ParseJsonString(settingsJson, false);
+        settings->LayerJson(settings->_userSettings);
+        auto commands = settings->_globals->Commands();
+        settings->_ValidateSettings();
+
+        VERIFY_ARE_EQUAL(2u, settings->_warnings.Size());
+        VERIFY_ARE_EQUAL(SettingsLoadWarnings::AtLeastOneKeybindingWarning, settings->_warnings.GetAt(0));
+        VERIFY_ARE_EQUAL(SettingsLoadWarnings::FailedToParseSubCommands, settings->_warnings.GetAt(1));
+        VERIFY_ARE_EQUAL(0u, commands.Size());
+    }
+
     void DeserializationTests::TestUnbindNestedCommand()
     {
         // Test that layering a command with `"commands": null` set will unbind a command that already exists.

src/cascadia/TerminalApp/AppLogic.cpp

@@ -44,7 +44,8 @@ static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWar
     USES_RESOURCE(L"FailedToWriteToSettings"),
     USES_RESOURCE(L"InvalidColorSchemeInCmd"),
     USES_RESOURCE(L"InvalidSplitSize"),
-    USES_RESOURCE(L"FailedToParseStartupActions")
+    USES_RESOURCE(L"FailedToParseStartupActions"),
+    USES_RESOURCE(L"FailedToParseSubCommands"),
 };
 static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
     USES_RESOURCE(L"NoProfilesText"),

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

@@ -226,6 +226,9 @@
     <value>&#x2022; Found a keybinding with too many strings for the "keys" array. There should only be one string value in the "keys" array.</value>
     <comment>{Locked="\"keys\"","&#x2022;"} This glyph is a bullet, used in a bulleted list.</comment>
   </data>
+  <data name="FailedToParseSubCommands" xml:space="preserve">
+    <value>&#x2022; Failed to parse all subcommands of nested command.</value>
+  </data>
   <data name="MissingRequiredParameter" xml:space="preserve">
     <value>&#x2022; Found a keybinding that was missing a required parameter value. This keybinding will be ignored.</value>
     <comment>{Locked="&#x2022;"} This glyph is a bullet, used in a bulleted list.</comment>

src/cascadia/TerminalApp/ShortcutActionDispatch.cpp

@@ -22,6 +22,11 @@ namespace winrt::TerminalApp::implementation
     // - true if we handled the event was handled, else false.
     bool ShortcutActionDispatch::DoAction(const ActionAndArgs& actionAndArgs)
     {
+        if (!actionAndArgs)
+        {
+            return false;
+        }
+
         const auto& action = actionAndArgs.Action();
         const auto& args = actionAndArgs.Args();
         auto eventArgs = args ? ActionEventArgs{ args } :

src/cascadia/TerminalSettingsModel/Command.cpp

@@ -162,6 +162,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             // It's possible that the nested commands have some warnings
             warnings.insert(warnings.end(), nestedWarnings.begin(), nestedWarnings.end());
 
+            if (result->_subcommands.Size() == 0)
+            {
+                warnings.push_back(SettingsLoadWarnings::FailedToParseSubCommands);
+                return nullptr;
+            }
+
             nested = true;
         }
         else if (json.isMember(JsonKey(CommandsKey)))

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

@@ -321,6 +321,13 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)
 
             // Now parse the array again, but this time as a list of commands.
             warnings = implementation::Command::LayerJson(_commands, bindings);
+
+            // We cannot add all warnings, as some of them were already populated while parsing key mapping.
+            // Hence let's cherry-pick the ones relevant for command parsing.
+            if (std::count(warnings.begin(), warnings.end(), SettingsLoadWarnings::FailedToParseSubCommands))
+            {
+                _keybindingsWarnings.push_back(SettingsLoadWarnings::FailedToParseSubCommands);
+            }
         }
     };
     parseBindings(LegacyKeybindingsKey);

src/cascadia/TerminalSettingsModel/TerminalWarnings.idl

@@ -21,6 +21,7 @@ namespace Microsoft.Terminal.Settings.Model
         InvalidColorSchemeInCmd = 11,
         InvalidSplitSize = 12,
         FailedToParseStartupActions = 13,
+        FailedToParseSubCommands = 14,
         WARNINGS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder.
     };