From 2c7ce484a54ddaa0b9f0e42c0aedbfa2b32531d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Garci=CC=81a?=
 <dani-garcia@users.noreply.github.com>
Date: Thu, 30 Nov 2023 12:47:18 +0100
Subject: [PATCH] Improve ClientSettings parsing

---
 crates/bitwarden-json/src/client.rs | 45 ++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/crates/bitwarden-json/src/client.rs b/crates/bitwarden-json/src/client.rs
index 416067b66..590cf45e6 100644
--- a/crates/bitwarden-json/src/client.rs
+++ b/crates/bitwarden-json/src/client.rs
@@ -24,16 +24,10 @@ impl Client {
             }
         };
 
-        if let Some(cmd_value_map) = cmd_value.as_object_mut() {
-            cmd_value_map.retain(|_, v| !v.is_null());
-
-            for &subcommand in SUBCOMMANDS_TO_CLEAN {
-                if let Some(cmd_value_secrets) = cmd_value_map
-                    .get_mut(subcommand)
-                    .and_then(|v| v.as_object_mut())
-                {
-                    cmd_value_secrets.retain(|_, v| !v.is_null());
-                }
+        clean_null_fields(&mut cmd_value);
+        for &subcommand in SUBCOMMANDS_TO_CLEAN {
+            if let Some(v) = cmd_value.get_mut(subcommand) {
+                clean_null_fields(v)
             }
         }
 
@@ -85,7 +79,17 @@ impl Client {
 
     fn parse_settings(settings_input: Option<String>) -> Option<ClientSettings> {
         if let Some(input) = settings_input.as_ref() {
-            match serde_json::from_str(input) {
+            let mut value: serde_json::Value = match serde_json::from_str(input) {
+                Ok(value) => value,
+                Err(e) => {
+                    log::error!("Failed to parse settings: {}", e);
+                    return None;
+                }
+            };
+
+            clean_null_fields(&mut value);
+
+            match serde_json::from_value(value) {
                 Ok(settings) => return Some(settings),
                 Err(e) => {
                     log::error!("Failed to parse settings: {}", e);
@@ -95,3 +99,22 @@ impl Client {
         None
     }
 }
+
+/// Removes null fields from a json object value, note that this isn't recursive.
+/// This is needed because some of the language bindings will send null values when a value is not set,
+/// and this causes problems with some deserializations, for example:
+///
+/// ClientSettings is using #[serde(default)] and non-optional fields, which means any missing
+/// field will be deserialized as the value from ClientSettings::default(). This does not work if the value
+/// is explicitly defined as null, which will produce a deserialization error.
+///
+/// The Command enum is serialized/deserialized as '{ "variant_name": {/* Variant contents */} }', serde
+/// is expecting only a single variant field in the root object, but some of the bindings are sending the
+/// rest of fields with null values, which will produce a deserialization error.
+/// '{ "other_variant": null, "variant_name": {/* Variant contents */}, "another_variant": null }'
+///
+fn clean_null_fields(value: &mut serde_json::Value) {
+    if let Some(object) = value.as_object_mut() {
+        object.retain(|_, v| !v.is_null());
+    }
+}