From 9599589f181520279c28ccaadcf0be257ff728d4 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 4 Oct 2023 16:57:30 +0200 Subject: [PATCH 1/4] Revert "fix(env): parsing --env incorrect in cli" This reverts commit 7ce654fea39195fae9f7f8d2cb1112b731e67fc3. see https://github.com/containers/podman/issues/19565 Signed-off-by: Paul Holzinger --- pkg/env/env.go | 62 +++++--------------------- pkg/env/env_test.go | 99 +++++++++++++++++++----------------------- pkg/env/env_unix.go | 2 +- pkg/env/env_windows.go | 2 +- 4 files changed, 58 insertions(+), 107 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index ad6b331c6d..80b13ed684 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -102,7 +102,7 @@ func ParseFile(path string) (_ map[string]string, err error) { // replace all \r\n and \r with \n text := strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(string(content)) - if err := parseEnv(env, text); err != nil { + if err := parseEnv(env, text, false); err != nil { return nil, err } @@ -110,14 +110,20 @@ func ParseFile(path string) (_ map[string]string, err error) { } // parseEnv parse the given content into env format +// @param enforceMatch bool "it throws an error if there is no match" // -// @example: parseEnv(env, "#comment") => nil -// @example: parseEnv(env, "") => nil -// @example: parseEnv(env, "KEY=FOO") => nil -// @example: parseEnv(env, "KEY") => nil -func parseEnv(env map[string]string, content string) error { +// @example: parseEnv(env, "#comment", true) => error("invalid variable: #comment") +// @example: parseEnv(env, "#comment", false) => nil +// @example: parseEnv(env, "", false) => nil +// @example: parseEnv(env, "KEY=FOO", true) => nil +// @example: parseEnv(env, "KEY", true) => nil +func parseEnv(env map[string]string, content string, enforceMatch bool) error { m := envMatch(content) + if len(m) == 0 && enforceMatch { + return fmt.Errorf("invalid variable: %q", content) + } + for _, match := range m { key := match[1] separator := strings.Trim(match[2], whiteSpaces) @@ -189,47 +195,3 @@ func envMatch(content string) [][]string { return m } - -// parseEnvWithSlice parsing a set of Env variables from a slice of strings -// because the majority of shell interpreters discard double quotes and single quotes, -// for example: podman run -e K='V', when passed into a program, it will become: K=V. -// This can lead to unexpected issues, as discussed in this link: https://github.com/containers/podman/pull/19096#issuecomment-1670164724. -// -// parseEnv method will discard all comments (#) that are not wrapped in quotation marks, -// so it cannot be used to parse env variables obtained from the command line. -// -// @example: parseEnvWithSlice(env, "KEY=FOO") => KEY: FOO -// @example: parseEnvWithSlice(env, "KEY") => KEY: "" -// @example: parseEnvWithSlice(env, "KEY=") => KEY: "" -// @example: parseEnvWithSlice(env, "KEY=FOO=BAR") => KEY: FOO=BAR -// @example: parseEnvWithSlice(env, "KEY=FOO#BAR") => KEY: FOO#BAR -func parseEnvWithSlice(env map[string]string, content string) error { - data := strings.SplitN(content, "=", 2) - - // catch invalid variables such as "=" or "=A" - if data[0] == "" { - return fmt.Errorf("invalid variable: %q", content) - } - // trim the front of a variable, but nothing else - name := strings.TrimLeft(data[0], whiteSpaces) - if len(data) > 1 { - env[name] = data[1] - } else { - if strings.HasSuffix(name, "*") { - name = strings.TrimSuffix(name, "*") - for _, e := range os.Environ() { - part := strings.SplitN(e, "=", 2) - if len(part) < 2 { - continue - } - if strings.HasPrefix(part[0], name) { - env[part[0]] = part[1] - } - } - } else if val, ok := os.LookupEnv(name); ok { - // if only a pass-through variable is given, clean it up. - env[name] = val - } - } - return nil -} diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 701663c486..1dc2b10eef 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -323,90 +323,79 @@ func Test_ParseFile(t *testing.T) { } } -func Test_parseEnvWithSlice(t *testing.T) { - type result struct { - key string - value string - hasErr bool - } +func Test_parseEnv(t *testing.T) { + good := make(map[string]string) - tests := []struct { - name string + type args struct { + env map[string]string line string - want result + } + tests := []struct { + name string + args args + wantErr bool }{ { name: "Good", - line: "apple=red", - want: result{ - key: "apple", - value: "red", - }, - }, - { - name: "NoValue", - line: "google=", - want: result{ - key: "google", - value: "", + args: args{ + env: good, + line: "apple=red", }, + wantErr: false, }, { - name: "OnlyKey", - line: "redhat", - want: result{ - key: "redhat", - value: "", + name: "GoodNoValue", + args: args{ + env: good, + line: "apple=", }, + wantErr: false, }, { - name: "NoKey", - line: "=foobar", - want: result{ - hasErr: true, + name: "GoodNoKeyNoValue", + args: args{ + env: good, + line: "=", }, + wantErr: true, }, { - name: "OnlyDelim", - line: "=", - want: result{ - hasErr: true, + name: "GoodOnlyKey", + args: args{ + env: good, + line: "apple", }, + wantErr: false, }, { - name: "Has#", - line: "facebook=red#blue", - want: result{ - key: "facebook", - value: "red#blue", + name: "BadNoKey", + args: args{ + env: good, + line: "=foobar", }, + wantErr: true, }, { - name: "Has\\n", - // twitter="foo - // bar" - line: "twitter=\"foo\nbar\"", - want: result{ - key: "twitter", - value: `"foo` + "\n" + `bar"`, + name: "BadOnlyDelim", + args: args{ + env: good, + line: "=", }, + wantErr: true, }, { name: "MultilineWithBackticksQuotes", - line: "github=`First line\nlast line`", - want: result{ - key: "github", - value: "`First line\nlast line`", + args: args{ + env: good, + line: "apple=`foo\nbar`", }, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - envs := make(map[string]string) - if err := parseEnvWithSlice(envs, tt.line); (err != nil) != tt.want.hasErr { - t.Errorf("parseEnv() error = %v, want has Err %v", err, tt.want.hasErr) - } else if envs[tt.want.key] != tt.want.value { - t.Errorf("parseEnv() result = map[%v:%v], but got %v", tt.want.key, tt.want.value, envs) + if err := parseEnv(tt.args.env, tt.args.line, true); (err != nil) != tt.wantErr { + t.Errorf("parseEnv() error = %v, wantErr %v", err, tt.wantErr) } }) } diff --git a/pkg/env/env_unix.go b/pkg/env/env_unix.go index b514917186..7fa3d59c91 100644 --- a/pkg/env/env_unix.go +++ b/pkg/env/env_unix.go @@ -8,7 +8,7 @@ package env func ParseSlice(s []string) (map[string]string, error) { env := make(map[string]string, len(s)) for _, e := range s { - if err := parseEnvWithSlice(env, e); err != nil { + if err := parseEnv(env, e, true); err != nil { return nil, err } } diff --git a/pkg/env/env_windows.go b/pkg/env/env_windows.go index f3eb4afce7..b9e4f4c589 100644 --- a/pkg/env/env_windows.go +++ b/pkg/env/env_windows.go @@ -17,7 +17,7 @@ func ParseSlice(s []string) (map[string]string, error) { continue } - if err := parseEnvWithSlice(env, e); err != nil { + if err := parseEnv(env, e, true); err != nil { return nil, err } } From 5f340487ee57a3ac307cf245bddb3d1c0e773b6d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 4 Oct 2023 16:58:57 +0200 Subject: [PATCH 2/4] Revert "docs(env-file): improve document description" This reverts commit c67ef7c1a12bb46e846c1b3dbda6acda1c6a5d30. see https://github.com/containers/podman/issues/19565 Signed-off-by: Paul Holzinger --- docs/source/markdown/options/env-file.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/source/markdown/options/env-file.md b/docs/source/markdown/options/env-file.md index 467aa4a91d..428cb78582 100644 --- a/docs/source/markdown/options/env-file.md +++ b/docs/source/markdown/options/env-file.md @@ -4,8 +4,4 @@ ####> are applicable to all of those. #### **--env-file**=*file* -Read the environment variables from the file, supporting prefix matching: `KEY*`, as well as multiline values in double quotes and single quotes, but not multiline values in backticks. -The env-file will ignore comments and empty lines. And spaces or tabs before and after the KEY. -If an invalid value is encountered, such as only an `=` sign, it will be skipped. If it is a prefix match (`KEY*`), all environment variables starting with KEY on the host machine will be loaded. -If it is only KEY (`KEY`), the KEY environment variable on the host machine will be loaded. -Compatible with the `export` syntax in **dotenv**, such as: `export KEY=bar`. +Read in a line-delimited file of environment variables. From 617af9bea974f9f17d92995c60dc3a97ade0f98d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 4 Oct 2023 17:01:18 +0200 Subject: [PATCH 3/4] Revert "feat(env): support multiline in env-file" This reverts commit 170a78631b4b0a0e5963e860cc3c3b297b4a7d09. This was a breaking change and users are hitting it, see https://github.com/containers/podman/issues/19565 Fixes #19565 Signed-off-by: Paul Holzinger --- pkg/env/env.go | 134 ++++++----------------- pkg/env/env_test.go | 242 +---------------------------------------- pkg/env/env_unix.go | 2 +- pkg/env/env_windows.go | 2 +- 4 files changed, 34 insertions(+), 346 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index 80b13ed684..8e87834f85 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -1,29 +1,13 @@ +// Package for processing environment variables. package env +// TODO: we need to add tests for this package. + import ( + "bufio" "fmt" - "io" "os" "strings" - - "github.com/containers/storage/pkg/regexp" -) - -var ( - // Form: https://github.com/motdotla/dotenv/blob/aa03dcad1002027390dac1e8d96ac236274de354/lib/main.js#L9C76-L9C76 - // (?:export\s+)?([\w.-]+) match key - // ([\w.%-]+)(\s*[=|*]\s*?|:\s+?) match separator - // Remaining match value - // e.g. KEY=VALUE => KEY, =, VALUE - // - // KEY= => KEY, =, "" - // KEY* => KEY, *, "" - // KEY*=1 => KEY, *, =1 - lineRegexp = regexp.Delayed( - `(?m)(?:^|^)\s*(?:export\s+)?([\w.%-]+)(\s*[=|*]\s*?|:\s+?)(\s*'(?:\\'|[^'])*'|\s*"(?:\\"|[^"])*"|\s*` + - "`(?:\\`|[^`])*`" + `|[^#\r\n]+)?\s*(?:#.*)?(?:$|$)`, - ) - onlyKeyRegexp = regexp.Delayed(`^[\w.-]+$`) ) const whiteSpaces = " \t" @@ -95,103 +79,47 @@ func ParseFile(path string) (_ map[string]string, err error) { } defer fh.Close() - content, err := io.ReadAll(fh) - if err != nil { - return nil, err - } - - // replace all \r\n and \r with \n - text := strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(string(content)) - if err := parseEnv(env, text, false); err != nil { - return nil, err + scanner := bufio.NewScanner(fh) + for scanner.Scan() { + // trim the line from all leading whitespace first + line := strings.TrimLeft(scanner.Text(), whiteSpaces) + // line is not empty, and not starting with '#' + if len(line) > 0 && !strings.HasPrefix(line, "#") { + if err := parseEnv(env, line); err != nil { + return nil, err + } + } } - - return env, nil + return env, scanner.Err() } -// parseEnv parse the given content into env format -// @param enforceMatch bool "it throws an error if there is no match" -// -// @example: parseEnv(env, "#comment", true) => error("invalid variable: #comment") -// @example: parseEnv(env, "#comment", false) => nil -// @example: parseEnv(env, "", false) => nil -// @example: parseEnv(env, "KEY=FOO", true) => nil -// @example: parseEnv(env, "KEY", true) => nil -func parseEnv(env map[string]string, content string, enforceMatch bool) error { - m := envMatch(content) +func parseEnv(env map[string]string, line string) error { + data := strings.SplitN(line, "=", 2) - if len(m) == 0 && enforceMatch { - return fmt.Errorf("invalid variable: %q", content) + // catch invalid variables such as "=" or "=A" + if data[0] == "" { + return fmt.Errorf("invalid variable: %q", line) } - - for _, match := range m { - key := match[1] - separator := strings.Trim(match[2], whiteSpaces) - value := match[3] - - if strings.Contains(value, "\n") { - if strings.HasPrefix(value, "`") { - return fmt.Errorf("only support multi-line environment variables surrounded by "+ - "double quotation marks or single quotation marks. invalid variable: %q", match[0]) - } - - // In the case of multi-line values, we need to remove the surrounding " ' - value = strings.Trim(value, "\"'") - } - - // KEY*=1 => KEY, *, =1 => KEY*, =, 1 - if separator == "*" && strings.HasPrefix(value, "=") { - key += "*" - separator = "=" - value = strings.TrimPrefix(value, "=") - } - - switch separator { - case "=": - // KEY= - if value == "" { - if val, ok := os.LookupEnv(key); ok { - env[key] = val - } - } else { - env[key] = value - } - case "*": + // trim the front of a variable, but nothing else + name := strings.TrimLeft(data[0], whiteSpaces) + if len(data) > 1 { + env[name] = data[1] + } else { + if strings.HasSuffix(name, "*") { + name = strings.TrimSuffix(name, "*") for _, e := range os.Environ() { part := strings.SplitN(e, "=", 2) if len(part) < 2 { continue } - if strings.HasPrefix(part[0], key) { + if strings.HasPrefix(part[0], name) { env[part[0]] = part[1] } } + } else if val, ok := os.LookupEnv(name); ok { + // if only a pass-through variable is given, clean it up. + env[name] = val } } return nil } - -func envMatch(content string) [][]string { - m := lineRegexp.FindAllStringSubmatch(content, -1) - - // KEY => KEY, =, "" - // Due to the above regex pattern, it will skip cases where only KEY is present (e.g., foo). - // However, in our requirement, this situation is equivalent to foo=(i.e., "foo" == "foo="). - // Therefore, we need to perform additional processing. - // The reason for needing to support this scenario is that we need to consider: `podman run -e CI -e USERNAME`. - { - noMatched := lineRegexp.ReplaceAllString(content, "") - nl := strings.Split(noMatched, "\n") - for _, key := range nl { - key := strings.Trim(key, whiteSpaces) - if key == "" { - continue - } - if onlyKeyRegexp.MatchString(key) { - m = append(m, []string{key, key, "=", ""}) - } - } - } - - return m -} diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 1dc2b10eef..c77061ecf2 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -1,8 +1,6 @@ package env import ( - "fmt" - "os" "testing" "github.com/stretchr/testify/assert" @@ -101,228 +99,6 @@ func TestJoin(t *testing.T) { } } -func createTmpFile(content string) (string, error) { - tmpfile, err := os.CreateTemp(os.TempDir(), "podman-test-parse-env-") - if err != nil { - return "", err - } - - if _, err := tmpfile.WriteString(content); err != nil { - return "", err - } - if err := tmpfile.Close(); err != nil { - return "", err - } - return tmpfile.Name(), nil -} - -func Test_ParseFile(t *testing.T) { - tests := []struct { - name string - key string - separator string // = or * - value string - - // environment variable - envKey string - envValue string - - expectedKey string - expectedValue string - success bool - }{ - { - name: "Good", - key: "Key", - separator: "=", - value: "Value1", - success: true, - }, - { - name: "HasDoubleQuotesWithSingleLine", - key: "Key2", - separator: "=", - value: `"Value2"`, - success: true, - }, - { - name: "HasSingleQuotesWithSingleLine", - key: "Key3", - separator: "=", - value: `'Value3'`, - success: true, - }, - { - name: "KeepValueSpace", - key: "Key4", - separator: "=", - value: " Value4 ", - success: true, - }, - { - name: "RemoveKeySpace", - key: " Key5 ", - separator: "=", - expectedKey: "Key5", - value: "Value5", - success: true, - }, - { - name: "NoValue", - key: "Key6", - separator: "=", - value: "", - envValue: "Value6", - expectedValue: "Value6", - success: true, - }, - { - name: "FromEnv", - key: "Key7", - separator: "=", - value: "", - envValue: "Value7", - expectedValue: "Value7", - success: true, - }, - { - name: "OnlyKey", - key: "Key8", - separator: "", - value: "", - envValue: "Value8", - expectedValue: "Value8", - success: true, - }, - { - name: "GlobKey", - key: "Key9", - separator: "*", - value: "", - envKey: "Key9999", - envValue: "Value9", - expectedKey: "Key9999", - expectedValue: "Value9", - success: true, - }, - { - name: "InvalidGlobKey", - key: "Key10*", - separator: "=", - value: "1", - envKey: "Key1010", - envValue: "Value10", - expectedKey: "Key10*", - expectedValue: "1", - success: true, - }, - { - name: "MultilineWithDoubleQuotes", - key: "Key11", - separator: "=", - value: "\"First line1\nlast line1\"", - expectedValue: "First line1\nlast line1", - success: true, - }, - { - name: "MultilineWithSingleQuotes", - key: "Key12", - separator: "=", - value: "'First line2\nlast line2'", - expectedValue: "First line2\nlast line2", - success: true, - }, - { - name: "Has%", - key: "BASH_FUNC__fmt_ctx%%", - separator: "=", - value: "() { echo 1; }", - success: true, - }, - { - name: "Export syntax", - key: "export Key13", - separator: "=", - value: "Value13", - expectedKey: "Key13", - expectedValue: "Value13", - success: true, - }, - { - name: "NoValueAndNoEnv", - key: "Key14", - separator: "=", - value: "", - success: false, - }, - { - name: "OnlyValue", - key: "", - separator: "=", - value: "Value", - success: false, - }, - { - name: "OnlyDelim", - key: "", - separator: "=", - value: "", - success: false, - }, - { - name: "Comment", - key: "#aaaa", - separator: "", - value: "", - success: false, - }, - } - - content := "" - for _, tt := range tests { - content += fmt.Sprintf("%s%s%s\n", tt.key, tt.separator, tt.value) - - if tt.envValue != "" { - key := tt.key - if tt.envKey != "" { - key = tt.envKey - } - t.Setenv(key, tt.envValue) - } - } - tFile, err := createTmpFile(content) - defer os.Remove(tFile) - assert.NoError(t, err) - - env, err := ParseFile(tFile) - assert.NoError(t, err) - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - key := tt.key - if tt.expectedKey != "" { - key = tt.expectedKey - } - val, ok := env[key] - if ok && !tt.success { - t.Errorf("not should set key:%s ", tt.key) - return - } else if !ok && tt.success { - t.Errorf("should set key:%s ", tt.key) - return - } - - if tt.success { - value := tt.value - if tt.expectedValue != "" { - value = tt.expectedValue - } - assert.Equal(t, value, val, "value should be equal") - } - }) - } -} - func Test_parseEnv(t *testing.T) { good := make(map[string]string) @@ -359,14 +135,6 @@ func Test_parseEnv(t *testing.T) { }, wantErr: true, }, - { - name: "GoodOnlyKey", - args: args{ - env: good, - line: "apple", - }, - wantErr: false, - }, { name: "BadNoKey", args: args{ @@ -383,18 +151,10 @@ func Test_parseEnv(t *testing.T) { }, wantErr: true, }, - { - name: "MultilineWithBackticksQuotes", - args: args{ - env: good, - line: "apple=`foo\nbar`", - }, - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := parseEnv(tt.args.env, tt.args.line, true); (err != nil) != tt.wantErr { + if err := parseEnv(tt.args.env, tt.args.line); (err != nil) != tt.wantErr { t.Errorf("parseEnv() error = %v, wantErr %v", err, tt.wantErr) } }) diff --git a/pkg/env/env_unix.go b/pkg/env/env_unix.go index 7fa3d59c91..690078f33c 100644 --- a/pkg/env/env_unix.go +++ b/pkg/env/env_unix.go @@ -8,7 +8,7 @@ package env func ParseSlice(s []string) (map[string]string, error) { env := make(map[string]string, len(s)) for _, e := range s { - if err := parseEnv(env, e, true); err != nil { + if err := parseEnv(env, e); err != nil { return nil, err } } diff --git a/pkg/env/env_windows.go b/pkg/env/env_windows.go index b9e4f4c589..1496dbfebf 100644 --- a/pkg/env/env_windows.go +++ b/pkg/env/env_windows.go @@ -17,7 +17,7 @@ func ParseSlice(s []string) (map[string]string, error) { continue } - if err := parseEnv(env, e, true); err != nil { + if err := parseEnv(env, e); err != nil { return nil, err } } From 1b3cedbf31c8d9557b55bcb2bb67878baa3696d8 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 4 Oct 2023 18:18:18 +0200 Subject: [PATCH 4/4] test/system: --env-file test fixes Now that the newline env file change is reverted we have to adapt the tests. Signed-off-by: Paul Holzinger --- test/system/300-cli-parsing.bats | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/test/system/300-cli-parsing.bats b/test/system/300-cli-parsing.bats index d45431f1cc..12ab4dcb6a 100644 --- a/test/system/300-cli-parsing.bats +++ b/test/system/300-cli-parsing.bats @@ -154,18 +154,8 @@ EOF --env-file $envfile2 \ $IMAGE sh -c 'env -0 >/envresults' - # FIXME FIXME FIXME #19565, exceptions - # - # FIXME FIXME FIXME #19565, octothorpe not handled in envariable values - # FIXME FIXME FIXME: this should be fixed, and the line below removed - expect[special]="bcd" - - # FIXME FIXME FIXME #19565, what should multi-line strings be? - # FIXME FIXME FIXME: For docker compat, this should be >>>"line1<<< - expect[withnl]=$'line1\nline2' - - # FIXME FIXME FIXME uncomment this once octothorpe parsing is fixed - #expect[weird*na#me!]=$weirdname + expect[withnl]=$'"line1' + expect[weird*na#me!]=$weirdname _check_env $resultsfile }