maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Invalidate nested command with no valid subcommands (#9495)

Browse source 
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
This commit is contained in:
Don-Vito 2021-03-15 18:34:06 +02:00 committed by GitHub
parent b76087f561
commit 28da6c4ecb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 82 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

63
src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file

@ -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.

3
src/cascadia/TerminalApp/AppLogic.cpp
View file

@ -45,7 +45,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"),

5
src/cascadia/TerminalApp/Resources/en-US/Resources.resw
View file

@ -229,6 +229,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>
@ -575,4 +578,4 @@
<data name="WindowRestoreDownButtonToolTip" xml:space="preserve">
<value>Restore Down</value>
</data>
</root>
</root>

5
src/cascadia/TerminalApp/ShortcutActionDispatch.cpp
View file

@ -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 } :

6
src/cascadia/TerminalSettingsModel/Command.cpp
View file

@ -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)))

7
src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
View file

@ -337,6 +337,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);

1
src/cascadia/TerminalSettingsModel/TerminalWarnings.idl
View file

@ -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.
};