Add OOBE configuration validation

Bug: 854101
Change-Id: Ideef55c0e73fcf02096cd50f86a6f320f48651f1
Reviewed-on: https://chromium-review.googlesource.com/1190242
Commit-Queue: Denis Kuznetsov <antrim@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588366}

chrome/browser/chromeos/login/configuration_keys.cc

@@ -44,5 +44,67 @@ const char kUpdateSkipUpdate[] = "updateSkip";
 // enrollment at appropriate moment.
 const char kWizardAutoEnroll[] = "wizardAutoEnroll";
 
+using ValueType = base::Value::Type;
+
+constexpr struct {
+  const char* key;
+  ValueType type;
+  ConfigurationHandlerSide side;
+} kAllConfigurationKeys[] = {
+    {kWelcomeNext, ValueType::BOOLEAN, ConfigurationHandlerSide::HANDLER_JS},
+    {kNetworkSelectGUID, ValueType::STRING,
+     ConfigurationHandlerSide::HANDLER_JS},
+    {kEULASendUsageStatistics, ValueType::BOOLEAN,
+     ConfigurationHandlerSide::HANDLER_JS},
+    {kEULAAutoAccept, ValueType::BOOLEAN, ConfigurationHandlerSide::HANDLER_JS},
+    {kUpdateSkipUpdate, ValueType::BOOLEAN,
+     ConfigurationHandlerSide::HANDLER_CPP},
+    {kWizardAutoEnroll, ValueType::BOOLEAN,
+     ConfigurationHandlerSide::HANDLER_CPP},
+    {"desc", ValueType::STRING, ConfigurationHandlerSide::HANDLER_DOC},
+    {"testValue", ValueType::STRING, ConfigurationHandlerSide::HANDLER_BOTH},
+};
+
+bool ValidateConfiguration(const base::Value& configuration) {
+  if (configuration.type() != ValueType::DICTIONARY) {
+    LOG(ERROR) << "Configuration should be a dictionary";
+    return false;
+  }
+  base::Value clone = configuration.Clone();
+  bool valid = true;
+  for (const auto& key : kAllConfigurationKeys) {
+    auto* value = clone.FindKey(key.key);
+    if (value) {
+      if (value->type() != key.type) {
+        valid = false;
+        LOG(ERROR) << "Invalid configuration: key " << key.key
+                   << " type is invalid";
+      }
+      clone.RemoveKey(key.key);
+    }
+  }
+  valid = valid && clone.DictEmpty();
+  for (const auto& item : clone.DictItems()) {
+    LOG(ERROR) << "Unknown configuration key " << item.first;
+  }
+  return valid;
+}
+
+void FilterConfiguration(const base::Value& configuration,
+                         ConfigurationHandlerSide side,
+                         base::Value& filtered_result) {
+  DCHECK(side == ConfigurationHandlerSide::HANDLER_CPP ||
+         side == ConfigurationHandlerSide::HANDLER_JS);
+  for (const auto& key : kAllConfigurationKeys) {
+    if (key.side == side ||
+        key.side == ConfigurationHandlerSide::HANDLER_BOTH) {
+      auto* value = configuration.FindKey(key.key);
+      if (value) {
+        filtered_result.SetKey(key.key, value->Clone());
+      }
+    }
+  }
+}
+
 }  // namespace configuration
 }  // namespace chromeos

chrome/browser/chromeos/login/configuration_keys.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_LOGIN_CONFIGURATION_KEYS_H_
 #define CHROME_BROWSER_CHROMEOS_LOGIN_CONFIGURATION_KEYS_H_
 
+#include "base/values.h"
+
 namespace chromeos {
 namespace configuration {
 // Configuration keys that are used to automate OOBE screens go here.
@@ -22,6 +24,23 @@ extern const char kUpdateSkipUpdate[];
 
 extern const char kWizardAutoEnroll[];
 
+enum class ConfigurationHandlerSide : unsigned int {
+  HANDLER_JS,    // Handled by JS code
+  HANDLER_CPP,   // Handled by C++ code
+  HANDLER_BOTH,  // Used in both JS and C++ code
+  HANDLER_DOC    // Not used by code, serves for documentation purposes only.
+};
+
+// Checks if configuration is valid (all fields have correct types, no extra
+// fields).
+bool ValidateConfiguration(const base::Value& configuration);
+
+// Copies only fields handled by particular |side| from |configuration| to
+// |filtered_result|.
+void FilterConfiguration(const base::Value& configuration,
+                         ConfigurationHandlerSide side,
+                         base::Value& filtered_result);
+
 }  // namespace configuration
 }  // namespace chromeos
 

chrome/browser/chromeos/login/oobe_configuration.cc

@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
+#include "chrome/browser/chromeos/login/configuration_keys.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/oobe_configuration_client.h"
 
@@ -73,11 +74,16 @@ void OobeConfiguration::OnConfigurationCheck(bool has_configuration,
   auto value = base::JSONReader::ReadAndReturnError(
       configuration, base::JSONParserOptions::JSON_ALLOW_TRAILING_COMMAS,
       &error_code, &error_message, &row, &col);
-  if (!value || !value->is_dict()) {
+  if (!value) {
     LOG(ERROR) << "Error parsing OOBE configuration: " << error_message;
     return;
   }
 
+  if (!chromeos::configuration::ValidateConfiguration(*value)) {
+    LOG(ERROR) << "Invalid OOBE configuration";
+    return;
+  }
+
   configuration_ = std::move(value);
   for (auto& observer : observer_list_)
     observer.OnOobeConfigurationChanged();

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1423,7 +1423,11 @@ void WizardController::OnHIDScreenNecessityCheck(bool screen_needed) {
 }
 
 void WizardController::OnOobeConfigurationChanged() {
-  oobe_configuration_ = OobeConfiguration::Get()->GetConfiguration().Clone();
+  oobe_configuration_ = base::Value(base::Value::Type::DICTIONARY);
+  chromeos::configuration::FilterConfiguration(
+      OobeConfiguration::Get()->GetConfiguration(),
+      chromeos::configuration::ConfigurationHandlerSide::HANDLER_CPP,
+      oobe_configuration_);
   if (current_screen_) {
     current_screen_->SetConfiguration(&oobe_configuration_, true /*notify */);
   }

chrome/browser/resources/chromeos/login/oobe_types.js

@@ -71,8 +71,6 @@ OobeTypes.TimezoneDsc;
  * @typedef {{
  *   welcomeNext: boolean|undefined,
  *   networkSelectGuid: string|undefined,
- *   eulaAutoAccept: boolean|undefined,
- *   eulaSendStatistics: boolean|undefined,
  * }}
  */
 OobeTypes.OobeConfiguration;

chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
 #include "chrome/browser/chromeos/accessibility/magnification_manager.h"
+#include "chrome/browser/chromeos/login/configuration_keys.h"
 #include "chrome/browser/chromeos/login/demo_mode/demo_setup_controller.h"
 #include "chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h"
 #include "chrome/browser/chromeos/login/helper.h"
@@ -607,8 +608,12 @@ void CoreOobeHandler::OnOobeConfigurationChanged() {
 
 void CoreOobeHandler::UpdateOobeConfiguration() {
   if (OobeConfiguration::Get()) {
-    CallJSOrDefer("updateOobeConfiguration",
-                  OobeConfiguration::Get()->GetConfiguration());
+    base::Value configuration(base::Value::Type::DICTIONARY);
+    chromeos::configuration::FilterConfiguration(
+        OobeConfiguration::Get()->GetConfiguration(),
+        chromeos::configuration::ConfigurationHandlerSide::HANDLER_JS,
+        configuration);
+    CallJSOrDefer("updateOobeConfiguration", configuration);
   }
 }
 

chromeos/test/data/oobe_configuration/non_empty_configuration.json

 {
-  "desc" : "Simple non-empty configuration file to test that configuration is loaded correctly"
+  "desc" : "Simple non-empty configuration file to test that configuration is loaded correctly",
+  "testValue" : "something"
 }
\ No newline at end of file