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
This commit is contained in:
Mike Griese 2020-03-20 15:35:51 -05:00 committed by GitHub
parent 9f95b54f2c
commit 99fa9460fd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 248 additions and 106 deletions

View file

@ -34,6 +34,7 @@ namespace TerminalAppLocalTests
namespace TerminalAppUnitTests
{
class DynamicProfileTests;
class JsonTests;
};
namespace TerminalApp
@ -124,4 +125,5 @@ private:
friend class TerminalAppLocalTests::KeyBindingsTests;
friend class TerminalAppLocalTests::TabTests;
friend class TerminalAppUnitTests::DynamicProfileTests;
friend class TerminalAppUnitTests::JsonTests;
};

View file

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

View file

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

View file

@ -46,24 +46,99 @@ namespace TerminalApp::JsonUtils
// - target: the optional object to receive the value from json
// - conversion: a std::function<T(const Json::Value&)> 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:
// - <none>
template<typename T, typename F>
void GetOptionalValue(const Json::Value& json,
std::string_view key,
std::optional<T>& target,
F&& conversion)
F&& conversion,
const std::function<bool(const Json::Value&)>& 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<T(const Json::Value&)> 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:
// - <none>
template<typename T, typename F>
void GetValue(const Json::Value& json,
std::string_view key,
T& target,
F&& conversion,
const std::function<bool(const Json::Value&)>& 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);
};

View file

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

View file

@ -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::CharReader>(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<Json::CharReader> 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<double>
VERIFY_IS_FALSE(profile._backgroundImageOpacity.has_value());
VERIFY_ARE_EQUAL(defaults._useAcrylic, profile._useAcrylic);
}
}