Config: Add support for environment variable expansion.#111
Config: Add support for environment variable expansion.#111
Conversation
📝 WalkthroughWalkthroughThe changes add environment variable expansion support to the configuration loading system. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Claude finished @ggreer's task in 1m 22s —— View job PR Review: Config environment variable expansion
SummaryThis PR adds Issues1. YAML injection via env var values — The expansion happens on raw YAML text before parsing. If an env var value contains YAML-significant characters ( For example, if Consider either:
2. Regex is recompiled on every call —
3. Missing tests There are no tests for 4. Strict failure on missing env vars — The function returns an error if any
5. Author's own TODO: remove env var expansion from connection string The PR description notes: "I think it needs the env var expansion removed from the connection string before merging." This hasn't been done yet. There may be existing env var expansion in the DSN/connection string handling that's now redundant with this general approach. Positive aspects
RecommendationThis needs the YAML injection issue (item 1) addressed before merging — it's a correctness bug that will surface in real configs with passwords or DSNs containing special characters. Adding tests and addressing the author's noted TODO would also be good before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/bsql/config.go`:
- Around line 531-533: Update the error messages in pkg/bsql/config.go that
validate/expand environment variables to include the connector prefix and proper
wrapping: change the "missing environment variables" return to prefix with
"baton-sql: " (e.g., "baton-sql: missing environment variables: ...") and modify
the error returned when expansion fails to include the "baton-sql: " prefix and
use %w to wrap the original error (e.g., "baton-sql: failed to expand
environment variables: %w"). Locate the two error returns used after env
validation/expansion (the missingVars join and the expansion failure) and update
their fmt.Errorf calls accordingly.
- Around line 512-529: The current envVarRegex.ReplaceAllStringFunc run on
dataStr mutates the raw file text (including comments, keys, and already-quoted
scalars); instead parse the YAML into a yaml.Node and perform the ${...}
expansion only on node.Value for string scalar nodes. Replace the
ReplaceAllStringFunc usage by: unmarshal into a root *yaml.Node, recursively
walk nodes and for nodes where Kind==yaml.ScalarNode (and/or Tag=="!!str") run
the envVarRegex replacement using lookup and append to missingVars as before;
when a replacement contains newlines or needs escaping, set the node.Style to
yaml.DoubleQuotedStyle and escape backslashes, quotes and newline characters in
node.Value so the resulting node remains valid YAML; finally marshal the node
back to bytes (replacing dataStr) and preserve the existing missingVars
collection and lookup semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dbcc10b-a173-40b5-8be9-a04cc7af7fb7
📒 Files selected for processing (2)
pkg/bsql/config.gopkg/connector/connector.go
| dataStr := envVarRegex.ReplaceAllStringFunc(string(data), func(match string) string { | ||
| envVar := match[2 : len(match)-1] | ||
| if envVal, exists := lookup(envVar); exists { | ||
| if strings.ContainsAny(envVal, "\n\r") { | ||
| // Quote multi-line values so they remain a single YAML scalar. | ||
| escaped := strings.ReplaceAll(envVal, `\`, `\\`) | ||
| escaped = strings.ReplaceAll(escaped, `"`, `\"`) | ||
| escaped = strings.ReplaceAll(escaped, "\n", `\n`) | ||
| escaped = strings.ReplaceAll(escaped, "\r", `\r`) | ||
| return `"` + escaped + `"` | ||
| } | ||
| return envVal | ||
| } | ||
| if !slices.Contains(missingVars, envVar) { | ||
| missingVars = append(missingVars, envVar) | ||
| } | ||
| return match | ||
| }) |
There was a problem hiding this comment.
Expand parsed YAML scalars, not raw file text.
This replacement runs before YAML parsing, so it also rewrites ${...} inside comments, keys, and already-quoted values. That breaks valid configs in common cases—for example, a commented example like # ${DB_PASSWORD} becomes a hard startup error, and a quoted placeholder with a multiline secret produces invalid YAML. Please expand only yaml.Node string scalar values after parsing.
Possible direction
- dataStr := envVarRegex.ReplaceAllStringFunc(string(data), func(match string) string {
- ...
- })
- return []byte(dataStr), nil
+ var root yaml.Node
+ if err := yaml.Unmarshal(data, &root); err != nil {
+ return nil, err
+ }
+
+ // Walk the parsed tree and expand only scalar string nodes.
+ // Then marshal the modified node tree back to bytes.
+ walkStringScalars(&root, func(value string) (string, error) {
+ return expandString(value, lookup)
+ })
+
+ out, err := yaml.Marshal(&root)
+ if err != nil {
+ return nil, err
+ }
+ return out, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/bsql/config.go` around lines 512 - 529, The current
envVarRegex.ReplaceAllStringFunc run on dataStr mutates the raw file text
(including comments, keys, and already-quoted scalars); instead parse the YAML
into a yaml.Node and perform the ${...} expansion only on node.Value for string
scalar nodes. Replace the ReplaceAllStringFunc usage by: unmarshal into a root
*yaml.Node, recursively walk nodes and for nodes where Kind==yaml.ScalarNode
(and/or Tag=="!!str") run the envVarRegex replacement using lookup and append to
missingVars as before; when a replacement contains newlines or needs escaping,
set the node.Style to yaml.DoubleQuotedStyle and escape backslashes, quotes and
newline characters in node.Value so the resulting node remains valid YAML;
finally marshal the node back to bytes (replacing dataStr) and preserve the
existing missingVars collection and lookup semantics.
| if len(missingVars) > 0 { | ||
| return nil, fmt.Errorf("missing environment variables: %s", strings.Join(missingVars, ", ")) | ||
| } |
There was a problem hiding this comment.
Prefix the new errors with baton-sql.
Both new error messages omit the required connector prefix. Please make these baton-sql: missing environment variables: ... and baton-sql: failed to expand environment variables: %w.
Suggested diff
- return nil, fmt.Errorf("missing environment variables: %s", strings.Join(missingVars, ", "))
+ return nil, fmt.Errorf("baton-sql: missing environment variables: %s", strings.Join(missingVars, ", "))
- return nil, fmt.Errorf("failed to expand environment variables: %w", err)
+ return nil, fmt.Errorf("baton-sql: failed to expand environment variables: %w", err)As per coding guidelines, **/*.go: Always wrap errors with %w format verb and include connector prefix (baton-*) in error messages.
Also applies to: 547-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/bsql/config.go` around lines 531 - 533, Update the error messages in
pkg/bsql/config.go that validate/expand environment variables to include the
connector prefix and proper wrapping: change the "missing environment variables"
return to prefix with "baton-sql: " (e.g., "baton-sql: missing environment
variables: ...") and modify the error returned when expansion fails to include
the "baton-sql: " prefix and use %w to wrap the original error (e.g.,
"baton-sql: failed to expand environment variables: %w"). Locate the two error
returns used after env validation/expansion (the missingVars join and the
expansion failure) and update their fmt.Errorf calls accordingly.
1 participant
Description
This is a more general solution to #101.
I think it needs the env var expansion removed from the connection string before merging.
Summary by CodeRabbit
Release Notes