From 032ceaf5d11d841b369e94d9f8a2d820f29903b1 Mon Sep 17 00:00:00 2001 From: Sergey Gorbunov Date: Tue, 4 Feb 2025 19:26:36 +0300 Subject: [PATCH] fix: Issue #9364 JSON config validation (#10679) * Fix JSON config validation * updated ReflectToMap --------- Co-authored-by: galargh Co-authored-by: Guillaume Michel Co-authored-by: guillaumemichel --- config/config.go | 101 +++++++++++++++++++++ config/config_test.go | 132 ++++++++++++++++++++++++++++ docs/changelogs/v0.34.md | 8 +- repo/fsrepo/fsrepo.go | 6 ++ test/sharness/t0002-docker-image.sh | 8 +- test/sharness/t0021-config.sh | 83 ++++++++--------- test/sharness/t0070-user-config.sh | 8 +- 7 files changed, 289 insertions(+), 57 deletions(-) diff --git a/config/config.go b/config/config.go index 91fb219a2a1..3db7573d06c 100644 --- a/config/config.go +++ b/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 +} diff --git a/config/config_test.go b/config/config_test.go index dead06f8a23..d4f38f08662 100644 --- a/config/config_test.go +++ b/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") + } +} diff --git a/docs/changelogs/v0.34.md b/docs/changelogs/v0.34.md index 67c8d4c6ced..aa64b942784 100644 --- a/docs/changelogs/v0.34.md +++ b/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 - diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index b21b555cf7d..e7577f9dc19 100644 --- a/repo/fsrepo/fsrepo.go +++ b/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 { diff --git a/test/sharness/t0002-docker-image.sh b/test/sharness/t0002-docker-image.sh index 2ff827806ba..8812c277a74 100755 --- a/test/sharness/t0002-docker-image.sh +++ b/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 ' diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index 95a8a7d8746..3e688634863 100755 --- a/test/sharness/t0021-config.sh +++ b/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 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 succeeds" " + ipfs config $cfg_flags \"$cfg_key\" \"$cfg_val\" + " + + 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" ' diff --git a/test/sharness/t0070-user-config.sh b/test/sharness/t0070-user-config.sh index 63c26ea3afb..1dc4c0369a0 100755 --- a/test/sharness/t0070-user-config.sh +++ b/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 '