-
Notifications
You must be signed in to change notification settings - Fork 17
Disallow invalid identifiers as environment keys
#53
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
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 |
|---|---|---|
|
|
@@ -3,11 +3,14 @@ package ejson2env | |
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "regexp" | ||
| ) | ||
|
|
||
| var errNoEnv = errors.New("environment is not set in ejson") | ||
| var errEnvNotMap = errors.New("environment is not a map[string]interface{}") | ||
|
|
||
| var validIdentifierPattern = regexp.MustCompile(`\A[a-zA-Z_][a-zA-Z0-9_]*\z`) | ||
|
|
||
| // ExtractEnv extracts the environment values from the map[string]interface{} | ||
| // containing all secrets, and returns a map[string]string containing the | ||
| // key value pairs. If there's an issue (the environment key doesn't exist, for | ||
|
|
@@ -26,6 +29,12 @@ func ExtractEnv(secrets map[string]interface{}) (map[string]string, error) { | |
| envSecrets := make(map[string]string, len(envMap)) | ||
|
|
||
| for key, rawValue := range envMap { | ||
| // Reject keys that would be invalid environment variable identifiers | ||
| if !validIdentifierPattern.MatchString(key) { | ||
| err := fmt.Errorf("invalid identifier as key in environment: %q", key) | ||
|
|
||
| return nil, err | ||
| } | ||
|
Comment on lines
+32
to
+37
Contributor
Author
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. According to the comment below, we seem to ignore when the values don't convert to strings properly, rather than return an error. IMO we should be loud about these failures (I think non-strings should also be errors, or at least print something to stderr), however if we really think it's better to silently continue, then we could change this to "only export keys that are valid identifiers" (and simply ignore invalid ones). Separately, I may open a PR to add said stderr message (since making it an error would be a breaking change that could break production deployments). |
||
|
|
||
| // Only export values that convert to strings properly. | ||
| if value, ok := rawValue.(string); ok { | ||
|
|
||
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.
This would also work, and would be shorter
but after thinking about it, I think the long form is clearer, as it explicitly shows how the first character can't be a digit, but that otherwise it's all the same.