config: refactor macro substitution with string-level env macros#470
config: refactor macro substitution with string-level env macros#470mostlygeek merged 2 commits intomainfrom
Conversation
Simplify macro processing by substituting ${env.VAR} macros at string
level before YAML parsing.
- Remove substituteEnvMacrosInValue recursive function
- Move env macro substitution to string level (before unmarshal)
- Simplify LoadConfigFromReader flow
- Update tests for new error message format
- Net reduction of ~141 lines of code
WalkthroughMove ${env.VAR} substitution to a pre-unmarshal string pass, sanitize env values for YAML, and centralize per-model macro merging and LIFO expansion. PORT handling, unknown-macro checks, API key and peers validation were adjusted to operate after consolidated macro expansion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proxy/config/config.go (2)
289-303:Filters.SetParamsis not substituted for models, unlike peers.Model macro substitution processes
Filters.StripParams(line 293) but skipsFilters.SetParams. However, peer processing (lines 428-435) does substitute macros inSetParams. This inconsistency means users can use macros insetParamsfor peers but not for models.🔧 Suggested fix to add SetParams substitution for models
modelConfig.Filters.StripParams = strings.ReplaceAll(modelConfig.Filters.StripParams, macroSlug, macroStr) + // Substitute in setParams (type-preserving) + if len(modelConfig.Filters.SetParams) > 0 { + result, err := substituteMacroInValue(modelConfig.Filters.SetParams, entry.Name, entry.Value) + if err != nil { + return Config{}, fmt.Errorf("model %s filters.setParams: %s", modelId, err.Error()) + } + modelConfig.Filters.SetParams = result.(map[string]any) + } + // Substitute in metadata (type-preserving) if len(modelConfig.Metadata) > 0 {
331-358: Missing unknown macro validation forFilters.SetParamsin models.The validation for unknown macros (lines 340-352) covers string fields and then validates
Metadata(lines 354-358), butFilters.SetParamsfor models is not validated. IfSetParamscan contain macros (per the peer handling pattern), it should also be checked.🔧 Suggested fix to add SetParams validation
if len(modelConfig.Metadata) > 0 { if err := validateNestedForUnknownMacros(modelConfig.Metadata, fmt.Sprintf("model %s metadata", modelId)); err != nil { return Config{}, err } } + + if len(modelConfig.Filters.SetParams) > 0 { + if err := validateNestedForUnknownMacros(modelConfig.Filters.SetParams, fmt.Sprintf("model %s filters.setParams", modelId)); err != nil { + return Config{}, err + } + }
🧹 Nitpick comments (2)
proxy/config/config.go (2)
187-206: Consider YAML injection risk from environment variable values.String-level substitution before YAML parsing is cleaner but introduces a subtle risk: if an environment variable contains YAML metacharacters (
:,#,[,],{,}, newlines, etc.), it could corrupt parsing or inject unintended structure. For example, an env var containingvalue\nmalicious_key: badcould add unexpected keys.This may be acceptable if environment variables are considered trusted (like config files per the learnings), but it's worth noting. Consider documenting this assumption or, for sensitive fields like
apiKey, validating the substituted value doesn't contain YAML-breaking characters.
406-415: Redundant assignment can be removed.Line 414 assigns
apikeyback toconfig.RequiredAPIKeys[i], butapikeyis already the value at that index (from the range loop). This appears to be leftover from when env macro substitution was applied here.♻️ Suggested cleanup
// Validate API keys (env macros already substituted at string level) for i, apikey := range config.RequiredAPIKeys { if apikey == "" { return Config{}, fmt.Errorf("empty api key found in apiKeys") } if strings.Contains(apikey, " ") { return Config{}, fmt.Errorf("api key cannot contain spaces: `%s`", apikey) } - config.RequiredAPIKeys[i] = apikey }
Add sanitizeEnvValueForYAML to prevent YAML injection from env vars: - Reject values containing newlines or null bytes - Escape backslashes and quotes for double-quoted YAML contexts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@proxy/config/config.go`:
- Around line 695-711: sanitizeEnvValueForYAML currently only rejects
newlines/nulls and escapes for double-quoted YAML, but values containing YAML
structural characters will break parsing when substituted into unquoted
contexts; update sanitizeEnvValueForYAML to also reject values containing the
problematic unquoted-substitution characters/sequences (colon-space ": ", space
followed by "#" " #", and any of the structural characters "[" , "]" , "{" ,
"}") and return a clear error mentioning varName so callers fail fast instead of
producing invalid YAML (keep the existing newline/null checks and escaping
behavior for double-quoted usages).
🧹 Nitpick comments (1)
proxy/config/config.go (1)
406-415: Consider removing redundant assignment.Line 414 assigns
apikeyback to the slice, butapikeyis never modified in the loop body. This appears to be leftover from earlier code that may have trimmed or transformed values.♻️ Suggested fix
for i, apikey := range config.RequiredAPIKeys { if apikey == "" { return Config{}, fmt.Errorf("empty api key found in apiKeys") } if strings.Contains(apikey, " ") { return Config{}, fmt.Errorf("api key cannot contain spaces: `%s`", apikey) } - config.RequiredAPIKeys[i] = apikey }
| // sanitizeEnvValueForYAML ensures an environment variable value is safe for YAML substitution. | ||
| // It rejects values with characters that break YAML structure and escapes quotes/backslashes | ||
| // for compatibility with double-quoted YAML strings. | ||
| func sanitizeEnvValueForYAML(value, varName string) (string, error) { | ||
| // Reject values that would break YAML structure regardless of quoting context | ||
| if strings.ContainsAny(value, "\n\r\x00") { | ||
| return "", fmt.Errorf("environment variable '%s' contains newlines or null bytes which are not allowed in YAML substitution", varName) | ||
| } | ||
|
|
||
| case []any: | ||
| newSlice := make([]any, len(v)) | ||
| for i, val := range v { | ||
| newVal, err := substituteEnvMacrosInValue(val) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| newSlice[i] = newVal | ||
| } | ||
| return newSlice, nil | ||
| // Escape backslashes and double quotes for safe use in double-quoted YAML strings. | ||
| // In unquoted contexts, these escapes appear literally (harmless for most use cases). | ||
| // In double-quoted contexts, they are interpreted correctly. | ||
| value = strings.ReplaceAll(value, `\`, `\\`) | ||
| value = strings.ReplaceAll(value, `"`, `\"`) | ||
|
|
||
| default: | ||
| return value, nil | ||
| } | ||
| return value, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the code around lines 670-711
cd /tmp && git ls-files proxy/config/config.go
wc -l proxy/config/config.goRepository: mostlygeek/llama-swap
Length of output: 187
🏁 Script executed:
# Read the relevant section with context
sed -n '660,720p' proxy/config/config.goRepository: mostlygeek/llama-swap
Length of output: 1867
🏁 Script executed:
# Search for usages of sanitizeEnvValueForYAML to understand how substituted values are used
rg -n "sanitizeEnvValueForYAML|substituteEnvMacros" proxy/config/config.go -B 2 -A 5Repository: mostlygeek/llama-swap
Length of output: 1675
🏁 Script executed:
# Search for how the YAML is generated/used - look for yaml formatting or generation
rg -n "yaml\|YAML" proxy/config/config.go | head -20Repository: mostlygeek/llama-swap
Length of output: 47
🌐 Web query:
YAML parsing rules for unquoted values special characters colon hash bracket
💡 Result:
Summary of YAML rules for unquoted (plain) scalars — colon, hash, brackets (YAML 1.2):
- Plain scalars have no escaping and are context‑sensitive; they must not be empty or contain leading/trailing whitespace. [1]
- Colon (:)
- A colon followed by a space (": ") is the mapping value indicator and therefore forbidden inside a plain scalar (it would be ambiguous). A colon may appear inside a plain scalar if it is NOT followed by a space (e.g., "a:1" is a single plain scalar, while "a: 1" is a mapping). [1][3]
- A plain scalar must also obey first-character indicator rules: it generally must not start with mapping/sequence/other indicator characters unless the grammar’s lookahead permits (e.g., ":" may start a plain scalar only when followed by a non-space “safe” character). [1]
- Hash / comment (#)
- The sequence " # " (a space then # that begins a comment) is not allowed inside a plain scalar. A "#" may appear in a plain scalar when it is not interpreted as a comment (e.g., when not preceded by a space) but behavior can be implementation-dependent; quoting is safest. [1][3]
- Brackets and flow-collection punctuation ([ ] { } ,)
- Inside flow collections ([...] or {...}) plain scalars must not contain flow-collection characters: "[" "]" "{" "}" "," because those would be parsed as collection syntax. Outside flow collections these characters are allowed in plain scalars provided they don’t create other ambiguities. [1][3]
- Other common practical rules / first-character restrictions
- Many characters are reserved as indicators for other constructs and therefore should not be the first character of an unquoted scalar (examples: [, ], {, }, ,, #, &, *, !, |, >, ', ", %, @, `). The characters "?", ":", "-" may start a plain scalar only if the next character is a non-space safe character. Implementations vary, so quoting is recommended for ambiguous cases. [1][5]
Quick examples:
- key: a:1 # value is plain scalar "a:1". [allowed]
- key: a: 1 # mapping key "a" with value 1. [not plain scalar]
- key: value#1 # "#" here is part of scalar (no space before "#") — may be parsed as scalar by many parsers but quoting is safer. [implementation nuance]
- key: [a,b] # inside flow sequence, unquoted "a,b" would conflict — use quotes or separate items. [disallowed inside flow]
References:
- YAML 1.2 specification (plain style, flow collections, indicators). [1]
- Practical discussion / examples (Stack Overflow). [3]
- Ansible YAML guidance (common first-character restrictions). [5]
Document or reject YAML structural characters in unquoted substitution contexts.
The sanitization correctly rejects newlines/nulls and escapes for double-quoted YAML strings. However, when environment values are substituted into unquoted YAML contexts (e.g., key: ${env.VAR}), certain characters break parsing regardless of escaping:
:(colon-space) → causes mapping ambiguity#(space-hash) → starts a comment[,],{,}→ structural indicators
For example, MY_VAR="value: extra" in key: ${env.MY_VAR} becomes invalid YAML key: value: extra.
The code's reliance on double-quoted escaping only works if users quote their values: key: "${env.VAR}". Consider either documenting this requirement explicitly or rejecting values containing these characters to fail fast rather than produce invalid YAML.
🤖 Prompt for AI Agents
In `@proxy/config/config.go` around lines 695 - 711, sanitizeEnvValueForYAML
currently only rejects newlines/nulls and escapes for double-quoted YAML, but
values containing YAML structural characters will break parsing when substituted
into unquoted contexts; update sanitizeEnvValueForYAML to also reject values
containing the problematic unquoted-substitution characters/sequences
(colon-space ": ", space followed by "#" " #", and any of the structural
characters "[" , "]" , "{" , "}") and return a clear error mentioning varName so
callers fail fast instead of producing invalid YAML (keep the existing
newline/null checks and escaping behavior for double-quoted usages).
1 participant
Summary
${env.VAR}macro substitution to string level before YAML parsingsubstituteEnvMacrosInValuerecursive functionLoadConfigFromReaderflowSummary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.