fix: Issue #9364 JSON config validation #10679

gsergey418alt · 2025-01-28T10:01:27Z

Fixes #9364. Disallow unknown fields when updating the config with ipfs config
Before:

$ ipfs config --json -- Provider  '{"Foo":""}' &&  ipfs config --json -- Provider && echo ok
{
  "Foo": ""
}
ok
$

After:

$ ipfs config --json -- Provider  '{"Foo":""}' &&  ipfs config --json -- Provider && echo ok
Error: failed to set config value: failure to decode config: json: unknown field "Foo" (maybe use --json?)
$

guillaumemichel · 2025-01-28T15:36:17Z

Triage notes:

Changelog should be moved to 0.34
Strict config file parsing should be optional (CLI flag e.g ipfs daemon --allow-unknown-config-fields~~--disallow-unknown-config-fields~~, true by default) to avoid breaking existing users

gsergey418alt · 2025-01-28T21:18:10Z

Yeah, you're right, that would break existing users, will fix that tomorrow.

…ne more test.

…o fix/9364-validation

guillaumemichel

Thanks @gsergey418alt for your contribution.

Could you add a flag to enable/disable strict config JSON parsing? e.g ipfs daemon --allow-unknown-config-fields, true by default.

This would allow existing users to pass that flag to continue using the same config, and would facilitate development when comparing different versions with the same config.

test/sharness/t0021-config.sh

config/config.go

gsergey418alt · 2025-01-30T10:53:39Z

Docker commands still failing in CI, maybe some issue on your side? I don't see anything related to ipfs config.

gsergey418alt · 2025-01-30T12:08:16Z

@guillaumemichel The existing users can keep using the same config, the validation is only done on the ipfs config command.

guillaumemichel · 2025-01-30T13:48:32Z

config/config.go

+// Convert config to a map, without using encoding/json.
+func ReflectToMap(conf interface{}) map[string]interface{} {


what is the reason to define a new function, and not use ToMap defined above?

There's no way to get encoding/json package (which is used in the ToMap function) to ignore the struct tags like json:",omitempty" that hide the map fields. That means that the user wouldn't be able to set these fields, since this map is used to validate user's input from ipfs config. Had to use the reflect package to make that happen.

gsergey418alt · 2025-01-30T14:13:08Z

Same stuff with the docker commands

guillaumemichel · 2025-01-30T14:21:03Z

I just checked locally, and only this PR is failing the docker sharness tests. master doesn't have this issue.

this CID probably needs to be adjusted. Not sure what it corresponds to, but maybe one of the files defined above?
this test seems directly linked with the config

gsergey418alt · 2025-01-30T14:23:52Z

@guillaumemichel Yep, seen that test. I fixed the values to be valid, but it's still failing because the docker container isn't starting up.

guillaumemichel · 2025-01-30T15:42:05Z

Turns out Datastore.Path isn't a valid config anymore: old style datatstore config detected, which is why the docker tests were failing.

Just opened #10686 to address the outdated ipfs config --help.

gsergey418alt · 2025-01-30T16:17:49Z

Awesome, good catch.

guillaumemichel · 2025-01-31T14:12:52Z

I updated the ReflectToMap since it was missing some cases, and added a test for it.

@gsergey418alt could you please adapt CheckKey accordingly, and add extensive sharness tests e.g in t0021-config.sh covering all possible key types (e.g bool, int, string, list, nested struct, etc.) both in json and "." (Key1.Key2.Key3) format where possible.

gsergey418alt · 2025-01-31T14:58:53Z

Good work, but you would need a way to distinguish between in-config maps and structure. In my implementation, this was done via the "map" value of a field, so when the validator encounters this field, it knows that the user is trying to set nested value inside a map and it discards the rest of key. I think I should've named the function something like GetValidationMap or something

guillaumemichel · 2025-01-31T15:22:52Z

you would need a way to distinguish between in-config maps and structure

@gsergey418alt could you expand on that? I am not sure to understand the difference in practice.

If your implementation works better, we can use it, I just want to cover all corner cases, and to test everything.

gsergey418alt · 2025-01-31T15:24:57Z

@guillaumemichel If we run ipfs config --json Routing.Routers.Test "{\"Parameters\":\"Test\",\"Type\":\"Test\"}", we need a way to know that .Test is a user-defined entry in the Routers map and isn't supposed to be validated.

guillaumemichel · 2025-01-31T15:27:49Z

@guillaumemichel If we run ipfs config --json Routing.Routers.Test "{"Parameters":"Test","Type":"Test"}", we need a way to know that .Test is a user-defined entry in the Routers map and isn't supposed to be validated.

Ok I see. Would that work for maps that have nested types?

gsergey418alt · 2025-01-31T15:31:31Z

@guillaumemichel Hmm, haven't thought about that actually. Maybe we could create a "ref" element inside that map that could be validated against if you want that covered.

gsergey418alt · 2025-01-31T15:45:14Z

@guillaumemichel I've added sharness test for other data types, changed function name to GetValidationMap. I've also changed the behavior of the function for maps, as it was in my previous implementation, without validation for map-nested structures. I won't be active until Monday, so have a good weekend!

guillaumemichel · 2025-01-31T15:48:17Z

Could you add a test for Gateway.PublicGateways and to set all PublicGateways parameters? if this works as expected, we should be good and there is no need to modify the reflect code further.

Have a nice weekend!

This reverts commit 9086971.

gsergey418alt · 2025-02-03T11:47:31Z

@guillaumemichel I've solved the problem with validating structs inside maps, and it now works as expected. Added test for PublicGateways.

config/config.go

guillaumemichel · 2025-02-04T09:58:26Z

@guillaumemichel I've solved the problem with validating structs inside maps, and it now works as expected. Added test for PublicGateways.

Looks good thanks!

A sharness test is still failing, probably due to the following config command:

ipfs config --json Plugins.Plugins.peerlog.Config.Enabled true

Additionally, could you also add a test TestCheckKey in config_test.go testing a few possible keys?

gsergey418alt · 2025-02-04T12:41:32Z

Added handling for interface{} in the config, like Plugins.Plugins.peerlog.Config.Enabled

guillaumemichel · 2025-02-04T14:09:39Z

config/config.go

+			if v, ok := cursor.(string); ok && v == "any" {
+				return nil
+			}


@gsergey418alt would that work? Using "any" as key seems strange.

Suggested change

if v, ok := cursor.(string); ok && v == "any" {

return nil

}

if cursor == nil {

return nil

}

I think I'm using it as a value here, try printing json.Marshal(confmap)

what do you mean?

I just think using the string "any" is strange. Would it work to test for a nil pointer or empty string instead? And adapting ReflectToMap accordingly.

There are other empty strings in the output of ReflectToMap. Nil pointer could work, but "any" is perhaps more expressive e. g. this key contain anything, stop validating. I can change it to nil ptr if you want to.

NVM, empty strings and nil values would also work

NVM, It wouldn't work with empty strings, but it would still fail at a later stage.

Changed "any" to nil

Fix JSON config validation

c5649c2

gsergey418alt requested a review from a team as a code owner January 28, 2025 10:01

gsergey418alt mentioned this pull request Jan 28, 2025

Validate configuration #9364

Open

3 tasks

Add changelog entry.

b69676b

gsergey418alt changed the title ~~fix: Issue #9364 JSON config validation [skip changelog]~~ fix: Issue #9364 JSON config validation Jan 28, 2025

Fix test

42692c6

gsergey418alt added 4 commits January 28, 2025 18:59

Fix more tests, 53 still failing

f589555

Remove JSON test due to fragilty, fix some more tests, Lib tests left.

c35ee66

Remove lib tests as they don't seem to be relevant anymore.

dd9627e

Move changelog into v0.34

f7b44d0

gsergey418alt marked this pull request as draft January 28, 2025 21:18

Fix config validation.

11f7f06

gsergey418alt marked this pull request as ready for review January 29, 2025 06:33

gsergey418alt and others added 7 commits January 29, 2025 09:35

Delete test file

ef3ff89

Use reflection to ignore encoding/json tags.

7b88c81

Add exceptions for fields that were already maps in the config, fix o…

2765ec0

…ne more test.

Fix last (hopefully) test

354256a

test: fix the socat tests after the ubuntu 24.04 upgrade

12a0185

Merge branch 'socat-ubuntu-noble' of https://github.com/ipfs/kubo int…

c546382

…o fix/9364-validation

Merge branch 'master' into fix/9364-validation

a091fbe

guillaumemichel requested changes Jan 30, 2025

View reviewed changes

test/sharness/t0021-config.sh Show resolved Hide resolved

config/config.go Show resolved Hide resolved

Merge branch 'master' into fix/9364-validation

ab2657e

guillaumemichel reviewed Jan 30, 2025

View reviewed changes

updated ReflectToMap

7a3732f

Fix nested maps and write a test for it.

9086971

Add tests for other data types

564d9dd

gsergey418alt added 6 commits February 3, 2025 14:00

Revert "Fix nested maps and write a test for it."

c34c068

This reverts commit 9086971.

Validate structs nested in maps.

44fbd88

Add comments.

8a669cd

Remove debug

0a25331

Add test for PublicGateways

eae4382

Add test for sturct in map

7e8d3ca

Merge branch 'master' into fix/9364-validation

db3cd49

guillaumemichel reviewed Feb 4, 2025

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

guillaumemichel and others added 2 commits February 4, 2025 10:21

comment typo

bed120f

slight refactor

69235e2

gsergey418alt added 2 commits February 4, 2025 14:23

Add CheckKey tests.

8e72a65

Add handling for interface{} in the config

87e42d8

guillaumemichel reviewed Feb 4, 2025

View reviewed changes

"any" -> nil

7d7633e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Issue #9364 JSON config validation #10679

fix: Issue #9364 JSON config validation #10679

gsergey418alt commented Jan 28, 2025

guillaumemichel commented Jan 28, 2025 •

edited

Loading

gsergey418alt commented Jan 28, 2025

guillaumemichel left a comment

gsergey418alt commented Jan 30, 2025

gsergey418alt commented Jan 30, 2025

guillaumemichel Jan 30, 2025

gsergey418alt Jan 30, 2025 •

edited

Loading

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 30, 2025

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 30, 2025 •

edited

Loading

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

guillaumemichel commented Jan 31, 2025 •

edited

Loading

gsergey418alt commented Jan 31, 2025 •

edited

Loading

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Feb 3, 2025

guillaumemichel commented Feb 4, 2025

gsergey418alt commented Feb 4, 2025

guillaumemichel Feb 4, 2025

gsergey418alt Feb 4, 2025

guillaumemichel Feb 4, 2025 •

edited

Loading

gsergey418alt Feb 4, 2025

gsergey418alt Feb 4, 2025

gsergey418alt Feb 4, 2025

gsergey418alt Feb 4, 2025

		// Convert config to a map, without using encoding/json.
		func ReflectToMap(conf interface{}) map[string]interface{} {

fix: Issue #9364 JSON config validation #10679

Are you sure you want to change the base?

fix: Issue #9364 JSON config validation #10679

Conversation

gsergey418alt commented Jan 28, 2025

guillaumemichel commented Jan 28, 2025 • edited Loading

gsergey418alt commented Jan 28, 2025

guillaumemichel left a comment

Choose a reason for hiding this comment

gsergey418alt commented Jan 30, 2025

gsergey418alt commented Jan 30, 2025

guillaumemichel Jan 30, 2025

Choose a reason for hiding this comment

gsergey418alt Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 30, 2025

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 30, 2025 • edited Loading

gsergey418alt commented Jan 30, 2025

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

guillaumemichel commented Jan 31, 2025 • edited Loading

gsergey418alt commented Jan 31, 2025 • edited Loading

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

gsergey418alt commented Jan 31, 2025

guillaumemichel commented Jan 31, 2025

gsergey418alt commented Feb 3, 2025

guillaumemichel commented Feb 4, 2025

gsergey418alt commented Feb 4, 2025

guillaumemichel Feb 4, 2025

Choose a reason for hiding this comment

gsergey418alt Feb 4, 2025

Choose a reason for hiding this comment

guillaumemichel Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

gsergey418alt Feb 4, 2025

Choose a reason for hiding this comment

gsergey418alt Feb 4, 2025

Choose a reason for hiding this comment

gsergey418alt Feb 4, 2025

Choose a reason for hiding this comment

gsergey418alt Feb 4, 2025

Choose a reason for hiding this comment

guillaumemichel commented Jan 28, 2025 •

edited

Loading

gsergey418alt Jan 30, 2025 •

edited

Loading

guillaumemichel commented Jan 30, 2025 •

edited

Loading

guillaumemichel commented Jan 31, 2025 •

edited

Loading

gsergey418alt commented Jan 31, 2025 •

edited

Loading

guillaumemichel Feb 4, 2025 •

edited

Loading