Skip to content
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

Revert environment regexp in opts #16608

Merged
merged 1 commit into from Sep 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 17 additions & 11 deletions opts/envfile.go
Expand Up @@ -4,18 +4,22 @@ import (
"bufio"
"fmt"
"os"
"regexp"
"strings"
)

var (
// EnvironmentVariableRegexp is a regexp to validate correct environment variables
// Environment variables set by the user must have a name consisting solely of
// alphabetics, numerics, and underscores - the first of which must not be numeric.
EnvironmentVariableRegexp = regexp.MustCompile("^[[:alpha:]_][[:alpha:][:digit:]_]*$")
)

// ParseEnvFile reads a file with environment variables enumerated by lines
//
// ``Environment variable names used by the utilities in the Shell and
// Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase
// letters, digits, and the '_' (underscore) from the characters defined in
// Portable Character Set and do not begin with a digit. *But*, other
// characters may be permitted by an implementation; applications shall
// tolerate the presence of such names.''
// -- http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html
//
// As of #16585, it's up to application inside docker to validate or not
// environment variables, that's why we just strip leading whitespace and
// nothing more.
func ParseEnvFile(filename string) ([]string, error) {
fh, err := os.Open(filename)
if err != nil {
Expand All @@ -31,11 +35,13 @@ func ParseEnvFile(filename string) ([]string, error) {
// line is not empty, and not starting with '#'
if len(line) > 0 && !strings.HasPrefix(line, "#") {
data := strings.SplitN(line, "=", 2)
variable := data[0]

if !EnvironmentVariableRegexp.MatchString(variable) {
return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' is not a valid environment variable", variable)}
// trim the front of a variable, but nothing else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you should add some of the discussion in this PR (and the linked issue) in a comment (and in the commit message) to explain why we're not validating that the characters are valid for environment vars. (also to prevent someone to re-introduce the regex in future)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah true 😝

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and done :)

variable := strings.TrimLeft(data[0], whiteSpaces)
if strings.ContainsAny(variable, whiteSpaces) {
return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' has white spaces", variable)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why trim whitespace? It's technically allowed to have an env var with leading whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phemmer it's how it was done before the regexp so I reverted to it. But yeah.. and in ValidateEnv we do not trim leading space so.. there is something to do.. (probably removing this trim)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's leave this here. I'd rather have the previous behavior, that seemed to work as expected, than a new different behavior.

}

if len(data) > 1 {

// pass the value through, no trimming
Expand Down
8 changes: 6 additions & 2 deletions opts/envfile_test.go
Expand Up @@ -28,6 +28,8 @@ func TestParseEnvFileGoodFile(t *testing.T) {
# comment

_foobar=foobaz
with.dots=working
and_underscore=working too
`
// Adding a newline + a line with pure whitespace.
// This is being done like this instead of the block above
Expand All @@ -47,6 +49,8 @@ _foobar=foobaz
"foo=bar",
"baz=quux",
"_foobar=foobaz",
"with.dots=working",
"and_underscore=working too",
}

if !reflect.DeepEqual(lines, expectedLines) {
Expand Down Expand Up @@ -96,7 +100,7 @@ func TestParseEnvFileBadlyFormattedFile(t *testing.T) {
if _, ok := err.(ErrBadEnvVariable); !ok {
t.Fatalf("Expected a ErrBadEnvVariable, got [%v]", err)
}
expectedMessage := "poorly formatted environment: variable 'f ' is not a valid environment variable"
expectedMessage := "poorly formatted environment: variable 'f ' has white spaces"
if err.Error() != expectedMessage {
t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error())
}
Expand Down Expand Up @@ -131,7 +135,7 @@ another invalid line`
if _, ok := err.(ErrBadEnvVariable); !ok {
t.Fatalf("Expected a ErrBadEnvvariable, got [%v]", err)
}
expectedMessage := "poorly formatted environment: variable 'first line' is not a valid environment variable"
expectedMessage := "poorly formatted environment: variable 'first line' has white spaces"
if err.Error() != expectedMessage {
t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error())
}
Expand Down
8 changes: 4 additions & 4 deletions opts/opts.go
Expand Up @@ -256,16 +256,16 @@ func validatePath(val string, validator func(string) bool) (string, error) {
}

// ValidateEnv validates an environment variable and returns it.
// It uses EnvironmentVariableRegexp to ensure the name of the environment variable is valid.
// If no value is specified, it returns the current value using os.Getenv.
//
// As on ParseEnvFile and related to #16585, environment variable names
// are not validate what so ever, it's up to application inside docker
// to validate them or not.
func ValidateEnv(val string) (string, error) {
arr := strings.Split(val, "=")
if len(arr) > 1 {
return val, nil
}
if !EnvironmentVariableRegexp.MatchString(arr[0]) {
return val, ErrBadEnvVariable{fmt.Sprintf("variable '%s' is not a valid environment variable", val)}
}
if !doesEnvExist(val) {
return val, nil
}
Expand Down
44 changes: 16 additions & 28 deletions opts/opts_test.go
Expand Up @@ -377,35 +377,23 @@ func TestValidateDevice(t *testing.T) {
}

func TestValidateEnv(t *testing.T) {
invalids := map[string]string{
"some spaces": "poorly formatted environment: variable 'some spaces' is not a valid environment variable",
"asd!qwe": "poorly formatted environment: variable 'asd!qwe' is not a valid environment variable",
"1asd": "poorly formatted environment: variable '1asd' is not a valid environment variable",
"123": "poorly formatted environment: variable '123' is not a valid environment variable",
}
valids := map[string]string{
"a": "a",
"something": "something",
"_=a": "_=a",
"env1=value1": "env1=value1",
"_env1=value1": "_env1=value1",
"env2=value2=value3": "env2=value2=value3",
"env3=abc!qwe": "env3=abc!qwe",
"env_4=value 4": "env_4=value 4",
"PATH": fmt.Sprintf("PATH=%v", os.Getenv("PATH")),
"PATH=something": "PATH=something",
}
for value, expectedError := range invalids {
_, err := ValidateEnv(value)
if err == nil {
t.Fatalf("Expected ErrBadEnvVariable, got nothing")
}
if _, ok := err.(ErrBadEnvVariable); !ok {
t.Fatalf("Expected ErrBadEnvVariable, got [%s]", err)
}
if err.Error() != expectedError {
t.Fatalf("Expected ErrBadEnvVariable with message [%s], got [%s]", expectedError, err.Error())
}
"a": "a",
"something": "something",
"_=a": "_=a",
"env1=value1": "env1=value1",
"_env1=value1": "_env1=value1",
"env2=value2=value3": "env2=value2=value3",
"env3=abc!qwe": "env3=abc!qwe",
"env_4=value 4": "env_4=value 4",
"PATH": fmt.Sprintf("PATH=%v", os.Getenv("PATH")),
"PATH=something": "PATH=something",
"asd!qwe": "asd!qwe",
"1asd": "1asd",
"123": "123",
"some space": "some space",
" some space before": " some space before",
"some space after ": "some space after ",
}
for value, expected := range valids {
actual, err := ValidateEnv(value)
Expand Down