From 057c586642c8ea0a8ecee0f98384149f726ccfc2 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Sun, 26 Mar 2023 08:33:02 -0400 Subject: [PATCH] loader: fix overly aggressive project name validation There were cases where the loader would infer a perfectly valid project name by normalizing a directory, but would then re-validate the un-normalized project directory and throw an error. This moves all the validation into a single spot during the load, which is the only point that it has all the context. Some of this logic needs to be pushed out of here: it should NOT be reading the config files, for example. For now, this resolves the main issues. Signed-off-by: Milas Bowman --- cli/options.go | 12 ++-- cli/options_test.go | 14 ++++- cli/testdata/UNNORMALIZED PATH/compose.yaml | 3 + loader/loader.go | 70 +++++++++++---------- 4 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 cli/testdata/UNNORMALIZED PATH/compose.yaml diff --git a/cli/options.go b/cli/options.go index d9b5bce0..ecae4d2e 100644 --- a/cli/options.go +++ b/cli/options.go @@ -103,11 +103,8 @@ func WithName(name string) ProjectOptionsFn { // however, on the options object, the name is optional: if unset, // a name will be inferred by the loader, so it's legal to set the // name to an empty string here - if name != "" { - normalized := loader.NormalizeProjectName(name) - if err := loader.CheckOriginalProjectNameIsNormalized(name, normalized); err != nil { - return err - } + if name != loader.NormalizeProjectName(name) { + return loader.InvalidProjectNameErr(name) } o.Name = name return nil @@ -439,7 +436,10 @@ func withNamePrecedenceLoad(absWorkingDir string, options *ProjectOptions) func( } else if nameFromEnv, ok := options.Environment[consts.ComposeProjectName]; ok && nameFromEnv != "" { opts.SetProjectName(nameFromEnv, true) } else { - opts.SetProjectName(filepath.Base(absWorkingDir), false) + opts.SetProjectName( + loader.NormalizeProjectName(filepath.Base(absWorkingDir)), + false, + ) } } } diff --git a/cli/options_test.go b/cli/options_test.go index a5945818..1b322953 100644 --- a/cli/options_test.go +++ b/cli/options_test.go @@ -82,9 +82,9 @@ func TestProjectName(t *testing.T) { assert.NilError(t, err) p, err := ProjectFromOptions(opts) - // On macOS and Linux, the message will start with "/". On Windows, it will - // start with "\\\\". So we leave that part of the error off here. - assert.ErrorContains(t, err, `is not a valid project name`) + // root directory will resolve to an empty project name since there + // IS no directory name! + assert.ErrorContains(t, err, `project name must not be empty`) assert.Assert(t, p == nil) }) @@ -156,6 +156,14 @@ func TestProjectName(t *testing.T) { assert.Equal(t, p.Name, "simple") }) + t.Run("by compose file parent dir special", func(t *testing.T) { + opts, err := NewProjectOptions([]string{"testdata/UNNORMALIZED PATH/compose.yaml"}) + assert.NilError(t, err) + p, err := ProjectFromOptions(opts) + assert.NilError(t, err) + assert.Equal(t, p.Name, "unnormalizedpath") + }) + t.Run("by COMPOSE_PROJECT_NAME", func(t *testing.T) { os.Setenv("COMPOSE_PROJECT_NAME", "my_project_from_env") //nolint:errcheck defer os.Unsetenv("COMPOSE_PROJECT_NAME") //nolint:errcheck diff --git a/cli/testdata/UNNORMALIZED PATH/compose.yaml b/cli/testdata/UNNORMALIZED PATH/compose.yaml new file mode 100644 index 00000000..c895b614 --- /dev/null +++ b/cli/testdata/UNNORMALIZED PATH/compose.yaml @@ -0,0 +1,3 @@ +services: + nginx: + image: nginx diff --git a/loader/loader.go b/loader/loader.go index 82c0abdf..59f0ed9a 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -64,16 +64,13 @@ type Options struct { projectName string // Indicates when the projectName was imperatively set or guessed from path projectNameImperativelySet bool - // The value of projectName before normalization - projectNameBeforeNormalization string // Profiles set profiles to enable Profiles []string } func (o *Options) SetProjectName(name string, imperativelySet bool) { - o.projectName = NormalizeProjectName(name) + o.projectName = name o.projectNameImperativelySet = imperativelySet - o.projectNameBeforeNormalization = name } func (o Options) GetProjectName() (string, bool) { @@ -267,49 +264,54 @@ func Load(configDetails types.ConfigDetails, options ...func(*Options)) (*types. return project, err } -func CheckOriginalProjectNameIsNormalized(original, normalized string) error { - if original != normalized { - return fmt.Errorf("%q is not a valid project name: it must contain only "+ - "characters from [a-z0-9_-] and start with [a-z0-9]", original) - } else if normalized == "" { - return fmt.Errorf("project name must not be empty") - } - return nil +func InvalidProjectNameErr(v string) error { + return fmt.Errorf( + "%q is not a valid project name: it must contain only "+ + "characters from [a-z0-9_-] and start with [a-z0-9]", v, + ) } func projectName(details types.ConfigDetails, opts *Options) (string, error) { projectName, projectNameImperativelySet := opts.GetProjectName() - projectNameBeforeNormalization := opts.projectNameBeforeNormalization - var pjNameFromConfigFile string - - for _, configFile := range details.ConfigFiles { - yml, err := ParseYAML(configFile.Content) - if err != nil { - return "", nil + // if user did NOT provide a name explicitly, then see if one is defined + // in any of the config files + if !projectNameImperativelySet { + var pjNameFromConfigFile string + for _, configFile := range details.ConfigFiles { + yml, err := ParseYAML(configFile.Content) + if err != nil { + return "", nil + } + if val, ok := yml["name"]; ok && val != "" { + pjNameFromConfigFile = yml["name"].(string) + } } - if val, ok := yml["name"]; ok && val != "" { - pjNameFromConfigFile = yml["name"].(string) + if !opts.SkipInterpolation { + interpolated, err := interp.Interpolate( + map[string]interface{}{"name": pjNameFromConfigFile}, + *opts.Interpolate, + ) + if err != nil { + return "", err + } + pjNameFromConfigFile = interpolated["name"].(string) } - } - if !opts.SkipInterpolation { - interpolated, err := interp.Interpolate(map[string]interface{}{"name": pjNameFromConfigFile}, *opts.Interpolate) - if err != nil { - return "", err + pjNameFromConfigFile = NormalizeProjectName(pjNameFromConfigFile) + if pjNameFromConfigFile != "" { + projectName = pjNameFromConfigFile } - pjNameFromConfigFile = interpolated["name"].(string) } - pjNameFromConfigFileNormalized := NormalizeProjectName(pjNameFromConfigFile) - if !projectNameImperativelySet && pjNameFromConfigFileNormalized != "" { - projectName = pjNameFromConfigFileNormalized - projectNameBeforeNormalization = pjNameFromConfigFile + + if projectName == "" { + return "", errors.New("project name must not be empty") } - if err := CheckOriginalProjectNameIsNormalized( - projectNameBeforeNormalization, projectName); err != nil { - return "", err + if NormalizeProjectName(projectName) != projectName { + return "", InvalidProjectNameErr(projectName) } + // TODO(milas): this should probably ALWAYS set (overriding any existing) if _, ok := details.Environment[consts.ComposeProjectName]; !ok && projectName != "" { details.Environment[consts.ComposeProjectName] = projectName }