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. diff --git a/pkg/env/env.go b/pkg/env/env.go index ad6b331c6d..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,120 +79,26 @@ 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); err != nil { - return nil, err - } - - return env, nil -} - -// parseEnv parse the given content into env format -// -// @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 { - m := envMatch(content) - - 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 "*": - for _, e := range os.Environ() { - part := strings.SplitN(e, "=", 2) - if len(part) < 2 { - continue - } - if strings.HasPrefix(part[0], key) { - env[part[0]] = part[1] - } + 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 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 + return env, scanner.Err() } -// 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) +func parseEnv(env map[string]string, line string) error { + data := strings.SplitN(line, "=", 2) // catch invalid variables such as "=" or "=A" if data[0] == "" { - return fmt.Errorf("invalid variable: %q", content) + return fmt.Errorf("invalid variable: %q", line) } // trim the front of a variable, but nothing else name := strings.TrimLeft(data[0], whiteSpaces) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 701663c486..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,312 +99,63 @@ 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) +func Test_parseEnv(t *testing.T) { + good := make(map[string]string) - 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_parseEnvWithSlice(t *testing.T) { - type result struct { - key string - value string - hasErr bool + type args struct { + env map[string]string + line string } - tests := []struct { - name string - line string - want result + 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: "", - }, - }, - { - name: "OnlyKey", - line: "redhat", - want: result{ - key: "redhat", - value: "", - }, - }, - { - name: "NoKey", - line: "=foobar", - want: result{ - hasErr: true, + args: args{ + env: good, + line: "apple=red", }, + wantErr: false, }, { - name: "OnlyDelim", - line: "=", - want: result{ - hasErr: true, + name: "GoodNoValue", + args: args{ + env: good, + line: "apple=", }, + wantErr: false, }, { - name: "Has#", - line: "facebook=red#blue", - want: result{ - key: "facebook", - value: "red#blue", + name: "GoodNoKeyNoValue", + args: args{ + env: good, + line: "=", }, + wantErr: true, }, { - name: "Has\\n", - // twitter="foo - // bar" - line: "twitter=\"foo\nbar\"", - want: result{ - key: "twitter", - value: `"foo` + "\n" + `bar"`, + name: "BadNoKey", + args: args{ + env: good, + line: "=foobar", }, + wantErr: true, }, { - name: "MultilineWithBackticksQuotes", - line: "github=`First line\nlast line`", - want: result{ - key: "github", - value: "`First line\nlast line`", + name: "BadOnlyDelim", + args: args{ + env: good, + line: "=", }, + 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); (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..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 := parseEnvWithSlice(env, e); 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 f3eb4afce7..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 := parseEnvWithSlice(env, e); err != nil { + if err := parseEnv(env, e); err != nil { return nil, err } } 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 }