From 99fa9460fd8546f0a7dc89016e46c04cf68fe921 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 20 Mar 2020 15:35:51 -0500 Subject: [PATCH] Gracefully handle json data with the wrong value types (#4961) ## Summary of the Pull Request Currently, if the Terminal attempts to parse a setting that _should_ be a `bool` and the user provided a string, then we'll throw an exception while parsing the settings, and display an error message that's pretty unrelated to the actual problem. The same goes for `bool`s as `int`s, `float`s as `int`s, etc. This PR instead updates our settings parsing to ensure that we check the type of a json value before actually trying to get its parsed value. ## References ## PR Checklist * [x] Closes #4299 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments I made a bunch of `JsonUtils` helpers for this in the same vein as the `GetOptionalValue` ones. Notably, any other value type can safely be treated as a string value. ## Validation Steps Performed * added tests * ran the Terminal and verified we can parse settings with the wrong types --- src/cascadia/TerminalApp/CascadiaSettings.h | 2 + .../TerminalApp/GlobalAppSettings.cpp | 51 ++++------- src/cascadia/TerminalApp/JsonUtils.cpp | 68 ++++++++++++++- src/cascadia/TerminalApp/JsonUtils.h | 79 ++++++++++++++++- src/cascadia/TerminalApp/Profile.cpp | 85 ++++++------------- src/cascadia/ut_app/JsonTests.cpp | 69 +++++++++++++-- 6 files changed, 248 insertions(+), 106 deletions(-) diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 063a8ba8000..18ab5324db8 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -34,6 +34,7 @@ namespace TerminalAppLocalTests namespace TerminalAppUnitTests { class DynamicProfileTests; + class JsonTests; }; namespace TerminalApp @@ -124,4 +125,5 @@ class TerminalApp::CascadiaSettings final friend class TerminalAppLocalTests::KeyBindingsTests; friend class TerminalAppLocalTests::TabTests; friend class TerminalAppUnitTests::DynamicProfileTests; + friend class TerminalAppUnitTests::JsonTests; }; diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp index 8b18e32f30b..832061aa4ad 100644 --- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp @@ -262,22 +262,14 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) _defaultProfile = guid; } - if (auto alwaysShowTabs{ json[JsonKey(AlwaysShowTabsKey)] }) - { - _alwaysShowTabs = alwaysShowTabs.asBool(); - } - if (auto confirmCloseAllTabs{ json[JsonKey(ConfirmCloseAllKey)] }) - { - _confirmCloseAllTabs = confirmCloseAllTabs.asBool(); - } - if (auto initialRows{ json[JsonKey(InitialRowsKey)] }) - { - _initialRows = initialRows.asInt(); - } - if (auto initialCols{ json[JsonKey(InitialColsKey)] }) - { - _initialCols = initialCols.asInt(); - } + JsonUtils::GetBool(json, AlwaysShowTabsKey, _alwaysShowTabs); + + JsonUtils::GetBool(json, ConfirmCloseAllKey, _confirmCloseAllTabs); + + JsonUtils::GetInt(json, InitialRowsKey, _initialRows); + + JsonUtils::GetInt(json, InitialColsKey, _initialCols); + if (auto rowsToScroll{ json[JsonKey(RowsToScrollKey)] }) { //if it's not an int we fall back to setting it to 0, which implies using the system setting. This will be the case if it's set to "system" @@ -290,29 +282,19 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) _rowsToScroll = 0; } } + if (auto initialPosition{ json[JsonKey(InitialPositionKey)] }) { _ParseInitialPosition(GetWstringFromJson(initialPosition), _initialX, _initialY); } - if (auto showTitleInTitlebar{ json[JsonKey(ShowTitleInTitlebarKey)] }) - { - _showTitleInTitlebar = showTitleInTitlebar.asBool(); - } - if (auto showTabsInTitlebar{ json[JsonKey(ShowTabsInTitlebarKey)] }) - { - _showTabsInTitlebar = showTabsInTitlebar.asBool(); - } + JsonUtils::GetBool(json, ShowTitleInTitlebarKey, _showTitleInTitlebar); - if (auto wordDelimiters{ json[JsonKey(WordDelimitersKey)] }) - { - _wordDelimiters = GetWstringFromJson(wordDelimiters); - } + JsonUtils::GetBool(json, ShowTabsInTitlebarKey, _showTabsInTitlebar); - if (auto copyOnSelect{ json[JsonKey(CopyOnSelectKey)] }) - { - _copyOnSelect = copyOnSelect.asBool(); - } + JsonUtils::GetWstring(json, WordDelimitersKey, _wordDelimiters); + + JsonUtils::GetBool(json, CopyOnSelectKey, _copyOnSelect); if (auto launchMode{ json[JsonKey(LaunchModeKey)] }) { @@ -341,10 +323,7 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) _keybindingsWarnings.insert(_keybindingsWarnings.end(), warnings.begin(), warnings.end()); } - if (auto snapToGridOnResize{ json[JsonKey(SnapToGridOnResizeKey)] }) - { - _SnapToGridOnResize = snapToGridOnResize.asBool(); - } + JsonUtils::GetBool(json, SnapToGridOnResizeKey, _SnapToGridOnResize); } // Method Description: diff --git a/src/cascadia/TerminalApp/JsonUtils.cpp b/src/cascadia/TerminalApp/JsonUtils.cpp index 09363d0ad98..8331612a8bc 100644 --- a/src/cascadia/TerminalApp/JsonUtils.cpp +++ b/src/cascadia/TerminalApp/JsonUtils.cpp @@ -52,8 +52,74 @@ void TerminalApp::JsonUtils::GetOptionalDouble(const Json::Value& json, const auto conversionFn = [](const Json::Value& value) -> double { return value.asFloat(); }; + const auto validationFn = [](const Json::Value& value) -> bool { + return value.isNumeric(); + }; GetOptionalValue(json, key, target, - conversionFn); + conversionFn, + validationFn); +} + +void TerminalApp::JsonUtils::GetInt(const Json::Value& json, + std::string_view key, + int& target) +{ + const auto conversionFn = [](const Json::Value& value) -> int { + return value.asInt(); + }; + const auto validationFn = [](const Json::Value& value) -> bool { + return value.isInt(); + }; + GetValue(json, key, target, conversionFn, validationFn); +} + +void TerminalApp::JsonUtils::GetUInt(const Json::Value& json, + std::string_view key, + uint32_t& target) +{ + const auto conversionFn = [](const Json::Value& value) -> uint32_t { + return value.asUInt(); + }; + const auto validationFn = [](const Json::Value& value) -> bool { + return value.isUInt(); + }; + GetValue(json, key, target, conversionFn, validationFn); +} + +void TerminalApp::JsonUtils::GetDouble(const Json::Value& json, + std::string_view key, + double& target) +{ + const auto conversionFn = [](const Json::Value& value) -> double { + return value.asFloat(); + }; + const auto validationFn = [](const Json::Value& value) -> bool { + return value.isNumeric(); + }; + GetValue(json, key, target, conversionFn, validationFn); +} + +void TerminalApp::JsonUtils::GetBool(const Json::Value& json, + std::string_view key, + bool& target) +{ + const auto conversionFn = [](const Json::Value& value) -> bool { + return value.asBool(); + }; + const auto validationFn = [](const Json::Value& value) -> bool { + return value.isBool(); + }; + GetValue(json, key, target, conversionFn, validationFn); +} + +void TerminalApp::JsonUtils::GetWstring(const Json::Value& json, + std::string_view key, + std::wstring& target) +{ + const auto conversionFn = [](const Json::Value& value) -> std::wstring { + return GetWstringFromJson(value); + }; + GetValue(json, key, target, conversionFn, nullptr); } diff --git a/src/cascadia/TerminalApp/JsonUtils.h b/src/cascadia/TerminalApp/JsonUtils.h index 1186b48fc92..423f43c274a 100644 --- a/src/cascadia/TerminalApp/JsonUtils.h +++ b/src/cascadia/TerminalApp/JsonUtils.h @@ -46,24 +46,99 @@ namespace TerminalApp::JsonUtils // - target: the optional object to receive the value from json // - conversion: a std::function which can be used to // convert the Json::Value to the appropriate type. + // - validation: optional, if provided, will be called first to ensure that + // the json::value is of the correct type before attempting to call + // `conversion`. // Return Value: // - template void GetOptionalValue(const Json::Value& json, std::string_view key, std::optional& target, - F&& conversion) + F&& conversion, + const std::function& validation = nullptr) { if (json.isMember(JsonKey(key))) { if (auto jsonVal{ json[JsonKey(key)] }) { - target = conversion(jsonVal); + if (validation == nullptr || validation(jsonVal)) + { + target = conversion(jsonVal); + } } else { + // This branch is hit when the json object contained the key, + // but the key was set to `null`. In this case, explicitly clear + // the target. target = std::nullopt; } } } + + // Method Description: + // - Helper that can be used for retrieving a value from a json + // object, and parsing it's value to set on a given target object. + // - If the key we're looking for _doesn't_ exist in the json object, + // we'll leave the target object unmodified. + // - If the key exists in the json object, we'll use the provided + // `validation` function to ensure that the json value is of the + // correct type. + // - If we successfully validate the json value type (or no validation + // function was provided), then we'll use `conversion` to parse the + // value and place the result into `target` + // - Each caller should provide a conversion function that takes a + // Json::Value and returns an object of the same type as target. + // - Unlike GetOptionalValue, if the key exists but is set to `null`, we'll + // just ignore it. + // Arguments: + // - json: The json object to search for the given key + // - key: The key to look for in the json object + // - target: the optional object to receive the value from json + // - conversion: a std::function which can be used to + // convert the Json::Value to the appropriate type. + // - validation: optional, if provided, will be called first to ensure that + // the json::value is of the correct type before attempting to call + // `conversion`. + // Return Value: + // - + template + void GetValue(const Json::Value& json, + std::string_view key, + T& target, + F&& conversion, + const std::function& validation = nullptr) + { + if (json.isMember(JsonKey(key))) + { + if (auto jsonVal{ json[JsonKey(key)] }) + { + if (validation == nullptr || validation(jsonVal)) + { + target = conversion(jsonVal); + } + } + } + } + + void GetInt(const Json::Value& json, + std::string_view key, + int& target); + + void GetUInt(const Json::Value& json, + std::string_view key, + uint32_t& target); + + void GetDouble(const Json::Value& json, + std::string_view key, + double& target); + + void GetBool(const Json::Value& json, + std::string_view key, + bool& target); + + void GetWstring(const Json::Value& json, + std::string_view key, + std::wstring& target); }; diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index aaf98d1076d..da620ae8859 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -628,19 +628,11 @@ bool Profile::_ConvertJsonToBool(const Json::Value& json) void Profile::LayerJson(const Json::Value& json) { // Profile-specific Settings - if (json.isMember(JsonKey(NameKey))) - { - auto name{ json[JsonKey(NameKey)] }; - _name = GetWstringFromJson(name); - } + JsonUtils::GetWstring(json, NameKey, _name); JsonUtils::GetOptionalGuid(json, GuidKey, _guid); - if (json.isMember(JsonKey(HiddenKey))) - { - auto hidden{ json[JsonKey(HiddenKey)] }; - _hidden = hidden.asBool(); - } + JsonUtils::GetBool(json, HiddenKey, _hidden); // Core Settings JsonUtils::GetOptionalColor(json, ForegroundKey, _defaultForeground); @@ -672,22 +664,14 @@ void Profile::LayerJson(const Json::Value& json) i++; } } - if (json.isMember(JsonKey(HistorySizeKey))) - { - auto historySize{ json[JsonKey(HistorySizeKey)] }; - // TODO:MSFT:20642297 - Use a sentinel value (-1) for "Infinite scrollback" - _historySize = historySize.asInt(); - } - if (json.isMember(JsonKey(SnapOnInputKey))) - { - auto snapOnInput{ json[JsonKey(SnapOnInputKey)] }; - _snapOnInput = snapOnInput.asBool(); - } - if (json.isMember(JsonKey(CursorHeightKey))) - { - auto cursorHeight{ json[JsonKey(CursorHeightKey)] }; - _cursorHeight = cursorHeight.asUInt(); - } + + // TODO:MSFT:20642297 - Use a sentinel value (-1) for "Infinite scrollback" + JsonUtils::GetInt(json, HistorySizeKey, _historySize); + + JsonUtils::GetBool(json, SnapOnInputKey, _snapOnInput); + + JsonUtils::GetUInt(json, CursorHeightKey, _cursorHeight); + if (json.isMember(JsonKey(CursorShapeKey))) { auto cursorShape{ json[JsonKey(CursorShapeKey)] }; @@ -698,46 +682,25 @@ void Profile::LayerJson(const Json::Value& json) // Control Settings JsonUtils::GetOptionalGuid(json, ConnectionTypeKey, _connectionType); - if (json.isMember(JsonKey(CommandlineKey))) - { - auto commandline{ json[JsonKey(CommandlineKey)] }; - _commandline = GetWstringFromJson(commandline); - } - if (json.isMember(JsonKey(FontFaceKey))) - { - auto fontFace{ json[JsonKey(FontFaceKey)] }; - _fontFace = GetWstringFromJson(fontFace); - } - if (json.isMember(JsonKey(FontSizeKey))) - { - auto fontSize{ json[JsonKey(FontSizeKey)] }; - _fontSize = fontSize.asInt(); - } - if (json.isMember(JsonKey(AcrylicTransparencyKey))) - { - auto acrylicTransparency{ json[JsonKey(AcrylicTransparencyKey)] }; - _acrylicTransparency = acrylicTransparency.asFloat(); - } - if (json.isMember(JsonKey(UseAcrylicKey))) - { - auto useAcrylic{ json[JsonKey(UseAcrylicKey)] }; - _useAcrylic = useAcrylic.asBool(); - } - if (json.isMember(JsonKey(SuppressApplicationTitleKey))) - { - auto suppressApplicationTitle{ json[JsonKey(SuppressApplicationTitleKey)] }; - _suppressApplicationTitle = suppressApplicationTitle.asBool(); - } + JsonUtils::GetWstring(json, CommandlineKey, _commandline); + + JsonUtils::GetWstring(json, FontFaceKey, _fontFace); + + JsonUtils::GetInt(json, FontSizeKey, _fontSize); + + JsonUtils::GetDouble(json, AcrylicTransparencyKey, _acrylicTransparency); + + JsonUtils::GetBool(json, UseAcrylicKey, _useAcrylic); + + JsonUtils::GetBool(json, SuppressApplicationTitleKey, _suppressApplicationTitle); + if (json.isMember(JsonKey(CloseOnExitKey))) { auto closeOnExit{ json[JsonKey(CloseOnExitKey)] }; _closeOnExitMode = ParseCloseOnExitMode(closeOnExit); } - if (json.isMember(JsonKey(PaddingKey))) - { - auto padding{ json[JsonKey(PaddingKey)] }; - _padding = GetWstringFromJson(padding); - } + + JsonUtils::GetWstring(json, PaddingKey, _padding); JsonUtils::GetOptionalString(json, ScrollbarStateKey, _scrollbarState); diff --git a/src/cascadia/ut_app/JsonTests.cpp b/src/cascadia/ut_app/JsonTests.cpp index f5afb8c248d..ca1d5687a81 100644 --- a/src/cascadia/ut_app/JsonTests.cpp +++ b/src/cascadia/ut_app/JsonTests.cpp @@ -5,16 +5,20 @@ #include "../TerminalApp/ColorScheme.h" #include "../TerminalApp/Profile.h" +#include "../TerminalApp/CascadiaSettings.h" +#include "../LocalTests_TerminalApp/JsonTestClass.h" using namespace Microsoft::Console; using namespace TerminalApp; using namespace WEX::Logging; using namespace WEX::TestExecution; using namespace WEX::Common; +using namespace winrt::TerminalApp; +using namespace winrt::Microsoft::Terminal::Settings; namespace TerminalAppUnitTests { - class JsonTests + class JsonTests : public JsonTestClass { BEGIN_TEST_CLASS(JsonTests) TEST_CLASS_PROPERTY(L"ActivationContext", L"TerminalApp.Unit.Tests.manifest") @@ -26,10 +30,11 @@ namespace TerminalAppUnitTests TEST_METHOD(DiffProfile); TEST_METHOD(DiffProfileWithNull); + TEST_METHOD(TestWrongValueType); + TEST_CLASS_SETUP(ClassSetup) { - reader = std::unique_ptr(Json::CharReaderBuilder::CharReaderBuilder().newCharReader()); - + InitializeJsonReader(); // Use 4 spaces to indent instead of \t _builder.settings_["indentation"] = " "; return true; @@ -39,7 +44,6 @@ namespace TerminalAppUnitTests void VerifyParseFailed(std::string content); private: - std::unique_ptr reader; Json::StreamWriterBuilder _builder; }; @@ -47,7 +51,7 @@ namespace TerminalAppUnitTests { Json::Value root; std::string errs; - const bool parseResult = reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); + const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); VERIFY_IS_TRUE(parseResult, winrt::to_hstring(errs).c_str()); return root; } @@ -55,7 +59,7 @@ namespace TerminalAppUnitTests { Json::Value root; std::string errs; - const bool parseResult = reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); + const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); VERIFY_IS_FALSE(parseResult); } @@ -221,4 +225,57 @@ namespace TerminalAppUnitTests VERIFY_IS_TRUE("bar" == diff["icon"].asString()); } + void JsonTests::TestWrongValueType() + { + // This json blob has a whole bunch of settings with the wrong value + // types - strings for int values, ints for strings, floats for ints, + // etc. When we encounter data that's the wrong data type, we should + // gracefully ignore it, as opposed to throwing an exception, causing us + // to fail to load the settings at all. + + const std::string settings0String{ R"( + { + "defaultProfile" : "{00000000-1111-0000-0000-000000000000}", + "profiles": [ + { + "guid" : "{00000000-1111-0000-0000-000000000000}", + "acrylicOpacity" : "0.5", + "closeOnExit" : "true", + "fontSize" : "10", + "historySize" : 1234.5678, + "padding" : 20, + "snapOnInput" : "false", + "icon" : 4, + "backgroundImageOpacity": false, + "useAcrylic" : 14 + } + ] + })" }; + + const auto settings0Json = VerifyParseSucceeded(settings0String); + + CascadiaSettings settings; + + settings._ParseJsonString(settings0String, false); + // We should not throw an exception trying to parse the settings here. + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(1u, settings._profiles.size()); + auto& profile = settings._profiles.at(0); + Profile defaults{}; + + VERIFY_ARE_EQUAL(defaults._acrylicTransparency, profile._acrylicTransparency); + VERIFY_ARE_EQUAL(defaults._closeOnExitMode, profile._closeOnExitMode); + VERIFY_ARE_EQUAL(defaults._fontSize, profile._fontSize); + VERIFY_ARE_EQUAL(defaults._historySize, profile._historySize); + // A 20 as an int can still be treated as a json string + VERIFY_ARE_EQUAL(L"20", profile._padding); + VERIFY_ARE_EQUAL(defaults._snapOnInput, profile._snapOnInput); + // 4 is a valid string value + VERIFY_ARE_EQUAL(L"4", profile._icon); + // false is not a valid optional + VERIFY_IS_FALSE(profile._backgroundImageOpacity.has_value()); + VERIFY_ARE_EQUAL(defaults._useAcrylic, profile._useAcrylic); + } + }