Config Error Messages Replace "-" With "_"

[rgajason]

Environment

• ggshield version: 1.14.3
• Operating system (Linux, macOS, Windows): macOS
• Operating system version: Ventura 13.1
• Python version: 3.10.10

Describe the bug

The "Unrecognized key in config" error message displays an underscore instead of hyphen when an invalid configuration is present.

Steps to reproduce:

Create an invalid configuration file such as (note ignored-matches misspelled):

.gitguardian.yml

version: 2
secret:
  ignore-matches:
    - name: some-secret-i-want-to-ignore
      match: 4f2f11652a0<snip>5ead42463

Execute a git commit that includes the secret you wish to ignore (with GGShield configured as a pre-commit hook).

Actual result:

Error message:

Unrecognized key in config: ignore_matches

Note the underscore when the configuration has a hyphen.

Expected result:

Error message:

Unrecognized key in config: ignore-matches

(hyphen instead of underscore)

[pierrelalanne]

Thank you for raising this issue @rgajason, we will look into this.

[karo-fox]

Is it ok if I try to fix this one, if it's not taken yet?

[agateau-gg]

Sure, you're welcome to fix this! Do you have any idea in mind? I am not sure what would be the best way to fix it 🤔

[karo-fox]

I'm thinking of modyfing filter_fields method in FilteredConfig (here's its code) so it will display better warning. Maybe we can change the message to something like: Unrecognized key in config: {invalid key}, expected {valid key}

[agateau-gg]

I think that's not what this issue is about 😕.

For historical reasons, ggshield accepts config keys written using both - or _, so ignored-matches and ignored_matches are the same. Internally this works by replacing all - with _ (this is done by core.config.load_yaml_dict()).

The problem of this feature is: if a user adds a does-not-exist (dash-separated) key to their config, then ggshield is going to report that does_not_exist (underscore-separated) is not a valid key, which is confusing.

This problem is potentially complicated to fix because at the point we find a non-existing key, we do not have the original key name anymore, so we can't report it.

[karo-fox]

I came up with possible solution:

We can pass optionally self to load_yaml_dict() (when we call it from UserConfig, for example), extract field names of dataclass and check for uknown names before we replace - with _ and report them immediately

This will possibly separate checking and alerting from filtering uknown names, as it is now in FilteredConfig

[agateau-gg]

I would prefer if load_yaml_dict() remained simple. Actually, I think we could fix this by making it simpler:

• modify load_yaml_dict() so that it no longer calls replace_in_keys()
• make FilteredConfig perform the replacement: this way it has access to the original key name and can print it

For consistency, it would be good to remove the call to replace_in_keys() from save_yaml_dict() and instead explicitly call it in UserConfig.save().

We do not need to worry about the impact of the changes on other callers of load_yaml_dict() and save_yaml_dict(): the other callers do not depend on this feature.

Does this sound like a good plan to you @karo-fox?

(Looking at your initial answer, maybe this is what you had in mind and I did not get it 😅)

[karo-fox]

It sounds great. I tried implementing something similar at first on my forked repository. I encountered some failed functional tests with github actions and changed the solution I had in mind. Then I read that those fails can be related to GitHub actions not having access to gitguardian api key.

Anyway, your idea sounds great. I can try implementing it if you don't mind. I am not really experienced with open source contributions and would like to resolve this issue. I also have a question: if functional tests pass on my local machine, is it ok to open pull request, in case those tests fail with github actions on my fork?

[agateau-gg]

Sure, you are more than welcome to work on it 👍🏻.

The problem with the API key not being available for forks is annoying indeed but it's not a blocker (There is an issue for it: #374, we need to look into it on our side). Until that is fixed, we can run the functional tests manually with a valid key and merge your work if all tests pass.