-
Notifications
You must be signed in to change notification settings - Fork 808
feat: warn when picking up .grype.yaml from CWD #3428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| // Package configwarn detects when grype is about to pick up a configuration | ||
| // file from the current working directory (e.g. ./.grype.yaml) without an | ||
| // explicit --config flag or GRYPE_CONFIG environment variable. Reading a | ||
| // hidden config file from the CWD silently can be a debugging hazard | ||
| // (see anchore/grype#3427), so we surface a WARN-level log when it happens. | ||
| package configwarn | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| ) | ||
|
|
||
| // supportedExts mirrors viper.SupportedExts. We hard-code the list rather | ||
| // than depend on viper here so this package can be used without pulling the | ||
| // configuration loader stack into tests. | ||
| var supportedExts = []string{ | ||
| "json", "toml", "yaml", "yml", "properties", "props", "prop", | ||
| "hcl", "tfvars", "dotenv", "env", "ini", | ||
| } | ||
|
|
||
| // fileExists is overridable for tests. | ||
| var fileExists = func(path string) bool { | ||
| info, err := os.Stat(path) | ||
| return err == nil && !info.IsDir() | ||
| } | ||
|
|
||
| // Detect returns the path to a CWD-resident grype config file if one would | ||
| // be picked up implicitly, otherwise it returns "". | ||
| // | ||
| // It returns "" (no warning) when: | ||
| // - the user passed an explicit -c / --config / --config=... flag | ||
| // - GRYPE_CONFIG (or the equivalent <appName>_CONFIG) is set | ||
| // - no recognised config file exists in the current working directory | ||
| // | ||
| // It looks for both finder layouts that fangs uses by default: | ||
| // - ./.<appName>.<ext> | ||
| // - ./.<appName>/config.<ext> | ||
| func Detect(appName string, args []string, env func(string) string) string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This just checks whether a file exists, not whether content is loaded from it. Do we want to log.Warn based on file names, or based on unexpectedly loading configs? |
||
| if explicitConfigFlag(args) { | ||
| return "" | ||
| } | ||
| envKey := strings.ToUpper(appName) + "_CONFIG" | ||
| if env != nil && env(envKey) != "" { | ||
| return "" | ||
| } | ||
|
|
||
| for _, ext := range supportedExts { | ||
| candidate := "." + appName + "." + ext | ||
| if fileExists(candidate) { | ||
| return candidate | ||
| } | ||
| } | ||
| subdir := "." + appName | ||
| for _, ext := range supportedExts { | ||
| candidate := filepath.Join(subdir, "config."+ext) | ||
| if fileExists(candidate) { | ||
| return candidate | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // explicitConfigFlag returns true if a -c / --config flag appears in args | ||
| // (in any of: "-c file", "--config file", "-c=file", "--config=file", or | ||
| // the bundled short form "-cfoo"). We intentionally accept false positives | ||
| // here over false negatives: if anything that looks like a config flag is | ||
| // present, suppress the warning. | ||
| func explicitConfigFlag(args []string) bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This duplicates flag parsing elsewhere. If we added |
||
| for i, a := range args { | ||
| switch { | ||
| case a == "-c", a == "--config": | ||
| if i+1 < len(args) { | ||
| return true | ||
| } | ||
| case strings.HasPrefix(a, "--config="): | ||
| return true | ||
| case strings.HasPrefix(a, "-c=") && len(a) > 3: | ||
| return true | ||
| case strings.HasPrefix(a, "-c") && len(a) > 2 && !strings.HasPrefix(a, "--"): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this case for? when does |
||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package configwarn | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestDetect(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| app string | ||
| args []string | ||
| env map[string]string | ||
| present []string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "no file present, no flag, no env", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: nil, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "cwd .grype.yaml present and no flag/env", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: ".grype.yaml", | ||
| }, | ||
| { | ||
| name: "cwd .grype.yml present and no flag/env", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: []string{".grype.yml"}, | ||
| want: ".grype.yml", | ||
| }, | ||
| { | ||
| name: "cwd .grype.json present and no flag/env", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: []string{".grype.json"}, | ||
| want: ".grype.json", | ||
| }, | ||
| { | ||
| name: "cwd .grype/config.yaml present and no flag/env", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: []string{filepath.Join(".grype", "config.yaml")}, | ||
| want: filepath.Join(".grype", "config.yaml"), | ||
| }, | ||
| { | ||
| name: "explicit --config suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "--config", "/etc/grype.yaml", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "explicit --config= suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "--config=/etc/grype.yaml", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "explicit -c suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "-c", "/etc/grype.yaml", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "explicit -c=path suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "-c=/etc/grype.yaml", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "explicit -cpath bundled short form suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "-c/etc/grype.yaml", "alpine:latest"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "GRYPE_CONFIG env suppresses warning", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| env: map[string]string{"GRYPE_CONFIG": "/etc/grype.yaml"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: "", | ||
| }, | ||
| { | ||
| name: "trailing -c with no value is not treated as explicit", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest", "-c"}, | ||
| present: []string{".grype.yaml"}, | ||
| want: ".grype.yaml", | ||
| }, | ||
| { | ||
| name: "subdir form is only checked after the flat form", | ||
| app: "grype", | ||
| args: []string{"grype", "alpine:latest"}, | ||
| present: []string{".grype.yaml", filepath.Join(".grype", "config.yaml")}, | ||
| want: ".grype.yaml", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| present := map[string]struct{}{} | ||
| for _, p := range tt.present { | ||
| present[p] = struct{}{} | ||
| } | ||
| origExists := fileExists | ||
| fileExists = func(path string) bool { | ||
| _, ok := present[path] | ||
| return ok | ||
| } | ||
| t.Cleanup(func() { fileExists = origExists }) | ||
|
|
||
| env := func(string) string { return "" } | ||
| if tt.env != nil { | ||
| env = func(k string) string { return tt.env[k] } | ||
| } | ||
|
|
||
| got := Detect(tt.app, tt.args, env) | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are formats in here grype doesn't currently support (
.inifor one), and we don't want to have to keep this in sync with the formats we support.