fix: Issue #9364 JSON config validation (#10679)

* Fix JSON config validation
* updated ReflectToMap

---------

Co-authored-by: galargh <piotr.galar@gmail.com>
Co-authored-by: Guillaume Michel <guillaumemichel@users.noreply.github.com>
Co-authored-by: guillaumemichel <guillaume@michel.id>

config/config.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 
 	"github.com/ipfs/kubo/misc/fsutil"
@@ -137,6 +138,71 @@ func ToMap(conf *Config) (map[string]interface{}, error) {
 	return m, nil
 }
 
+// Convert config to a map, without using encoding/json, since
+// zero/empty/'omitempty' fields are excluded by encoding/json during
+// marshaling.
+func ReflectToMap(conf interface{}) interface{} {
+	v := reflect.ValueOf(conf)
+	if !v.IsValid() {
+		return nil
+	}
+
+	// Handle pointer type
+	if v.Kind() == reflect.Ptr {
+		if v.IsNil() {
+			// Create a zero value of the pointer's element type
+			elemType := v.Type().Elem()
+			zero := reflect.Zero(elemType)
+			return ReflectToMap(zero.Interface())
+		}
+		v = v.Elem()
+	}
+
+	switch v.Kind() {
+	case reflect.Struct:
+		result := make(map[string]interface{})
+		t := v.Type()
+		for i := 0; i < v.NumField(); i++ {
+			field := v.Field(i)
+			// Only include exported fields
+			if field.CanInterface() {
+				result[t.Field(i).Name] = ReflectToMap(field.Interface())
+			}
+		}
+		return result
+
+	case reflect.Map:
+		result := make(map[string]interface{})
+		iter := v.MapRange()
+		for iter.Next() {
+			key := iter.Key()
+			// Convert map keys to strings for consistency
+			keyStr := fmt.Sprint(ReflectToMap(key.Interface()))
+			result[keyStr] = ReflectToMap(iter.Value().Interface())
+		}
+		// Add a sample to differentiate between a map and a struct on validation.
+		sample := reflect.Zero(v.Type().Elem())
+		if sample.CanInterface() {
+			result["*"] = ReflectToMap(sample.Interface())
+		}
+		return result
+
+	case reflect.Slice, reflect.Array:
+		result := make([]interface{}, v.Len())
+		for i := 0; i < v.Len(); i++ {
+			result[i] = ReflectToMap(v.Index(i).Interface())
+		}
+		return result
+
+	default:
+		// For basic types (int, string, etc.), just return the value
+		if v.CanInterface() {
+			return v.Interface()
+		}
+		return nil
+	}
+}
+
 // Clone copies the config. Use when updating.
 func (c *Config) Clone() (*Config, error) {
 	var newConfig Config
@@ -152,3 +218,38 @@ func (c *Config) Clone() (*Config, error) {
 
 	return &newConfig, nil
 }
+
+// Check if the provided key is present in the structure.
+func CheckKey(key string) error {
+	conf := Config{}
+
+	// Convert an empty config to a map without JSON.
+	cursor := ReflectToMap(&conf)
+
+	// Parse the key and verify it's presence in the map.
+	var ok bool
+	var mapCursor map[string]interface{}
+
+	parts := strings.Split(key, ".")
+	for i, part := range parts {
+		mapCursor, ok = cursor.(map[string]interface{})
+		if !ok {
+			if cursor == nil {
+				return nil
+			}
+			path := strings.Join(parts[:i], ".")
+			return fmt.Errorf("%s key is not a map", path)
+		}
+
+		cursor, ok = mapCursor[part]
+		if !ok {
+			// If the config sections is a map, validate against the default entry.
+			if cursor, ok = mapCursor["*"]; ok {
+				continue
+			}
+			path := strings.Join(parts[:i+1], ".")
+			return fmt.Errorf("%s not found", path)
+		}
+	}
+	return nil
+}

config/config_test.go

@@ -27,3 +27,135 @@ func TestClone(t *testing.T) {
 		t.Fatal("HTTP headers not preserved")
 	}
 }
+
+func TestReflectToMap(t *testing.T) {
+	// Helper function to create a test config with various field types
+	reflectedConfig := ReflectToMap(new(Config))
+
+	mapConfig, ok := reflectedConfig.(map[string]interface{})
+	if !ok {
+		t.Fatal("Config didn't convert to map")
+	}
+
+	reflectedIdentity, ok := mapConfig["Identity"]
+	if !ok {
+		t.Fatal("Identity field not found")
+	}
+
+	mapIdentity, ok := reflectedIdentity.(map[string]interface{})
+	if !ok {
+		t.Fatal("Identity field didn't convert to map")
+	}
+
+	// Test string field reflection
+	reflectedPeerID, ok := mapIdentity["PeerID"]
+	if !ok {
+		t.Fatal("PeerID field not found in Identity")
+	}
+	if _, ok := reflectedPeerID.(string); !ok {
+		t.Fatal("PeerID field didn't convert to string")
+	}
+
+	// Test omitempty json string field
+	reflectedPrivKey, ok := mapIdentity["PrivKey"]
+	if !ok {
+		t.Fatal("PrivKey omitempty field not found in Identity")
+	}
+	if _, ok := reflectedPrivKey.(string); !ok {
+		t.Fatal("PrivKey omitempty field didn't convert to string")
+	}
+
+	// Test slices field
+	reflectedBootstrap, ok := mapConfig["Bootstrap"]
+	if !ok {
+		t.Fatal("Bootstrap field not found in config")
+	}
+	bootstrap, ok := reflectedBootstrap.([]interface{})
+	if !ok {
+		t.Fatal("Bootstrap field didn't convert to []string")
+	}
+	if len(bootstrap) != 0 {
+		t.Fatal("Bootstrap len is incorrect")
+	}
+
+	reflectedDatastore, ok := mapConfig["Datastore"]
+	if !ok {
+		t.Fatal("Datastore field not found in config")
+	}
+	datastore, ok := reflectedDatastore.(map[string]interface{})
+	if !ok {
+		t.Fatal("Datastore field didn't convert to map")
+	}
+	storageGCWatermark, ok := datastore["StorageGCWatermark"]
+	if !ok {
+		t.Fatal("StorageGCWatermark field not found in Datastore")
+	}
+	// Test int field
+	if _, ok := storageGCWatermark.(int64); !ok {
+		t.Fatal("StorageGCWatermark field didn't convert to int64")
+	}
+	noSync, ok := datastore["NoSync"]
+	if !ok {
+		t.Fatal("NoSync field not found in Datastore")
+	}
+	// Test bool field
+	if _, ok := noSync.(bool); !ok {
+		t.Fatal("NoSync field didn't convert to bool")
+	}
+
+	reflectedDNS, ok := mapConfig["DNS"]
+	if !ok {
+		t.Fatal("DNS field not found in config")
+	}
+	DNS, ok := reflectedDNS.(map[string]interface{})
+	if !ok {
+		t.Fatal("DNS field didn't convert to map")
+	}
+	reflectedResolvers, ok := DNS["Resolvers"]
+	if !ok {
+		t.Fatal("Resolvers field not found in DNS")
+	}
+	// Test map field
+	if _, ok := reflectedResolvers.(map[string]interface{}); !ok {
+		t.Fatal("Resolvers field didn't convert to map")
+	}
+
+	// Test pointer field
+	if _, ok := DNS["MaxCacheTTL"].(map[string]interface{}); !ok {
+		// Since OptionalDuration only field is private, we cannot test it
+		t.Fatal("MaxCacheTTL field didn't convert to map")
+	}
+}
+
+// Test validation of options set through "ipfs config"
+func TestCheckKey(t *testing.T) {
+	err := CheckKey("Foo.Bar")
+	if err == nil {
+		t.Fatal("Foo.Bar isn't a valid key in the config")
+	}
+
+	err = CheckKey("Provider.Strategy")
+	if err != nil {
+		t.Fatalf("%s: %s", err, "Provider.Strategy is a valid key in the config")
+	}
+
+	err = CheckKey("Provider.Foo")
+	if err == nil {
+		t.Fatal("Provider.Foo isn't a valid key in the config")
+	}
+
+	err = CheckKey("Gateway.PublicGateways.Foo.Paths")
+	if err != nil {
+		t.Fatalf("%s: %s", err, "Gateway.PublicGateways.Foo.Paths is a valid key in the config")
+	}
+
+	err = CheckKey("Gateway.PublicGateways.Foo.Bar")
+	if err == nil {
+		t.Fatal("Gateway.PublicGateways.Foo.Bar isn't a valid key in the config")
+	}
+
+	err = CheckKey("Plugins.Plugins.peerlog.Config.Enabled")
+	if err != nil {
+		t.Fatalf("%s: %s", err, "Plugins.Plugins.peerlog.Config.Enabled is a valid key in the config")
+	}
+}

docs/changelogs/v0.34.md

@@ -1,11 +1,12 @@
 # Kubo changelog v0.34
 
-- [v0.34.0](#v0310)
+- [v0.34.0](#v0340)
 
 ## v0.34.0
 
 - [Overview](#overview)
 - [🔦 Highlights](#-highlights)
+  - [JSON config validation](#json-config-validation)
   - [Reprovide command moved to routing](#reprovide-command-moved-to-routing)
   - [Additional stats for Accelerated DHT Reprovides](#additional-stats-for-accelerated-dht-reprovides)
 - [📝 Changelog](#-changelog)
@@ -15,6 +16,10 @@
 
 ### 🔦 Highlights
 
+#### JSON config validation
+
+`ipfs config` is now validating json fields ([#10679](https://github.com/ipfs/kubo/pull/10679)).
+
 #### Reprovide command moved to routing
 
 Moved the `bitswap reprovide` command to `routing reprovide`. ([#10677](https://github.com/ipfs/kubo/pull/10677))
@@ -26,4 +31,3 @@ The `stats reprovide` command now shows additional stats for the DHT Accelerated
 ### 📝 Changelog
 
 ### 👨‍👩‍👧‍👦 Contributors
-

repo/fsrepo/fsrepo.go

@@ -676,6 +676,12 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error {
 		return errors.New("repo is closed")
 	}
 
+	// Validate the key's presence in the config structure.
+	err := config.CheckKey(key)
+	if err != nil {
+		return err
+	}
+
 	// Load into a map so we don't end up writing any additional defaults to the config file.
 	var mapconf map[string]interface{}
 	if err := serialize.ReadConfigFile(r.configFilePath, &mapconf); err != nil {

test/sharness/t0002-docker-image.sh

@@ -36,8 +36,8 @@ test_expect_success "docker image build succeeds" '
 '
 
 test_expect_success "write init scripts" '
-  echo "ipfs config Foo Bar" > 001.sh &&
-  echo "ipfs config Baz Qux" > 002.sh &&
+  echo "ipfs config Provider.Strategy Bar" > 001.sh &&
+  echo "ipfs config Pubsub.Router Qux" > 002.sh &&
   chmod +x 002.sh
 '
 
@@ -65,10 +65,10 @@ test_expect_success "check that init scripts were run correctly and in the corre
 
 test_expect_success "check that init script configs were applied" '
   echo Bar > expected &&
-  docker exec "$DOC_ID" ipfs config Foo > actual &&
+  docker exec "$DOC_ID" ipfs config Provider.Strategy > actual &&
   test_cmp actual expected &&
   echo Qux > expected &&
-  docker exec "$DOC_ID" ipfs config Baz > actual &&
+  docker exec "$DOC_ID" ipfs config Pubsub.Router > actual &&
   test_cmp actual expected
 '
 

test/sharness/t0021-config.sh

@@ -13,41 +13,23 @@ test_config_cmd_set() {
 
   cfg_key=$1
   cfg_val=$2
-  test_expect_success "ipfs config succeeds" '
-    ipfs config $cfg_flags "$cfg_key" "$cfg_val"
-  '
+  test_expect_success "ipfs config succeeds" "
+    ipfs config $cfg_flags \"$cfg_key\" \"$cfg_val\"
+  "
 
-  test_expect_success "ipfs config output looks good" '
-    echo "$cfg_val" >expected &&
-    ipfs config "$cfg_key" >actual &&
-    test_cmp expected actual
-  '
-
-  # also test our lib function. it should work too.
-  cfg_key="Lib.$cfg_key"
-  test_expect_success "test_config_set succeeds" '
-    test_config_set $cfg_flags "$cfg_key" "$cfg_val"
-  '
-
-  test_expect_success "test_config_set value looks good" '
-    echo "$cfg_val" >expected &&
-    ipfs config "$cfg_key" >actual &&
-    test_cmp expected actual
-  '
+  test_expect_success "ipfs config output looks good" "
+    echo \"$cfg_val\" >expected &&
+    if [$cfg_flags != \"--json\"]; then
+      ipfs config \"$cfg_key\" >actual &&
+      test_cmp expected actual
+    else 
+      ipfs config \"$cfg_key\" | tr -d \"\\n\\t \" >actual &&
+      echo >>actual &&
+      test_cmp expected actual
+    fi
+  "
 }
 
-# this is a bit brittle. the problem is we need to test
-# with something that will be forced to unmarshal as a struct.
-# (i.e. just setting 'ipfs config --json foo "[1, 2, 3]"') may
-# set it as astring instead of proper json. We leverage the
-# unmarshalling that has to happen.
-CONFIG_SET_JSON_TEST='{
-  "MDNS": {
-    "Enabled": true,
-    "Interval": 10
-  }
-}'
-
 test_profile_apply_revert() {
   profile=$1
   inverse_profile=$2
@@ -87,27 +69,32 @@ test_profile_apply_dry_run_not_alter() {
 }
 
 test_config_cmd() {
-  test_config_cmd_set "beep" "boop"
-  test_config_cmd_set "beep1" "boop2"
-  test_config_cmd_set "beep1" "boop2"
-  test_config_cmd_set "--bool" "beep2" "true"
-  test_config_cmd_set "--bool" "beep2" "false"
-  test_config_cmd_set "--json" "beep3" "true"
-  test_config_cmd_set "--json" "beep3" "false"
-  test_config_cmd_set "--json" "Discovery" "$CONFIG_SET_JSON_TEST"
-  test_config_cmd_set "--json" "deep-not-defined.prop" "true"
-  test_config_cmd_set "--json" "deep-null" "null"
-  test_config_cmd_set "--json" "deep-null.prop" "true"
+  test_config_cmd_set "Addresses.API" "foo"
+  test_config_cmd_set "Addresses.Gateway" "bar"
+  test_config_cmd_set "Datastore.GCPeriod" "baz"
+  test_config_cmd_set "AutoNAT.ServiceMode" "enabled"
+  test_config_cmd_set "--bool" "Discovery.MDNS.Enabled" "true"
+  test_config_cmd_set "--bool" "Discovery.MDNS.Enabled" "false"
+  test_config_cmd_set "--json" "Datastore.HashOnRead" "true"
+  test_config_cmd_set "--json" "Datastore.HashOnRead" "false"
+  test_config_cmd_set "--json" "Experimental.FilestoreEnabled" "true"
+  test_config_cmd_set "--json" "Import.BatchMaxSize" "null"
+  test_config_cmd_set "--json" "Import.UnixFSRawLeaves" "true"
+  test_config_cmd_set "--json" "Routing.Routers.Test" "{\\\"Parameters\\\":\\\"Test\\\",\\\"Type\\\":\\\"Test\\\"}"
+  test_config_cmd_set "--json" "Experimental.OptimisticProvideJobsPoolSize" "1337"
+  test_config_cmd_set "--json" "Addresses.Swarm" "[\\\"test\\\",\\\"test\\\",\\\"test\\\"]"
+  test_config_cmd_set "--json" "Gateway.PublicGateways.Foo" "{\\\"DeserializedResponses\\\":true,\\\"InlineDNSLink\\\":false,\\\"NoDNSLink\\\":false,\\\"Paths\\\":[\\\"Bar\\\",\\\"Baz\\\"],\\\"UseSubdomains\\\":true}"
+  test_config_cmd_set "--bool" "Gateway.PublicGateways.Foo.UseSubdomains" "false"
 
   test_expect_success "'ipfs config show' works" '
     ipfs config show >actual
   '
 
   test_expect_success "'ipfs config show' output looks good" '
-    grep "\"beep\": \"boop\"," actual &&
-    grep "\"beep1\": \"boop2\"," actual &&
-    grep "\"beep2\": false," actual &&
-    grep "\"beep3\": false," actual
+    grep "\"API\": \"foo\"," actual &&
+    grep "\"Gateway\": \"bar\"" actual &&
+    grep "\"Enabled\": false" actual &&
+    grep "\"HashOnRead\": false" actual
   '
 
   test_expect_success "'ipfs config show --config-file' works" '

test/sharness/t0070-user-config.sh

@@ -11,10 +11,12 @@ test_description="Test user-provided config values"
 test_init_ipfs
 
 test_expect_success "bootstrap doesn't overwrite user-provided config keys (top-level)" '
-  ipfs config Foo.Bar baz &&
+  ipfs config Provider.Strategy >previous &&
+  ipfs config Provider.Strategy foo &&
   ipfs bootstrap rm --all &&
-  echo "baz" >expected &&
-  ipfs config Foo.Bar >actual &&
+  echo "foo" >expected &&
+  ipfs config Provider.Strategy >actual &&
+  ipfs config Provider.Strategy $(cat previous) &&
   test_cmp expected actual
 '