fix(helm): escape all user-supplied TOML string values with toJson#1
fix(helm): escape all user-supplied TOML string values with toJson#1
Conversation
There was a problem hiding this comment.
Summary
This PR correctly fixes a TOML injection vulnerability in the env field by using toJson for proper escaping. The fix prevents malformed TOML when environment variable values contain quotes or backslashes.
Critical Security Issues Found
While reviewing this security fix, I identified identical TOML injection vulnerabilities in other user-supplied string fields throughout the same template:
command(line 81)allow_bot_messagesfor Discord (line 32) and Slack (line 67)allow_user_messagesfor Slack (line 76)model(line 103)base_url(line 104)
These fields use the same unsafe pattern "{{ $value }}" that this PR fixes for env. They are vulnerable to the same attack: user-supplied values containing quotes or backslashes will produce malformed TOML and enable injection attacks.
Recommendation
The current fix for env is correct and should be merged, but a follow-up PR should apply the same toJson escaping to all other user-supplied string fields to comprehensively address this vulnerability class across the entire template.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Replace raw string interpolation "{{ $v }}" with {{ $v | toJson }}
across all user-supplied string fields in configmap.yaml:
- env values (injection risk with quotes/backslashes)
- command, working_dir
- discord/slack allow_bot_messages, slack allow_user_messages
- stt model, base_url
toJson produces a valid JSON string (with quotes and escaping), which
is compatible with TOML basic string syntax. Existing values without
special characters render identically.
Fields NOT changed (no user input):
- bot_token, app_token, api_key: use env var placeholders (${...})
- allowed_channels, allowed_users, trusted_bot_ids: already use toJson
- args: already uses toJson
a4e6583 to
e0627b0
Compare
|
/q review |
There was a problem hiding this comment.
This PR correctly addresses a TOML injection security vulnerability by replacing raw string interpolation with the toJson filter across all affected fields. The implementation properly escapes special characters (quotes, backslashes) in user-supplied values, preventing malformed TOML and potential injection attacks. The changes are consistent, well-tested, and backward compatible. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
What problem does this solve?
The configmap template renders user-supplied string values with raw interpolation:
If a value contains double quotes or backslashes, the rendered TOML is malformed. For example,
MY_VAR: 'say "hello"'produces:This breaks TOML parsing at runtime. A crafted value could also inject arbitrary TOML keys.
The same unsafe pattern exists in 8 fields across the template:
envvalues,command,working_dir,allow_bot_messages(Discord/Slack),allow_user_messages(Slack), and STTmodel/base_url.Related: openabdev#425 (Helm chart analysis report)
Credit
This issue was originally identified and fixed by @thekkagent in openabdev/openab#380 as part of a larger Helm chart extensibility PR. This PR extracts the TOML escaping fix as a standalone security patch, and extends it to cover all remaining string fields in the template.
Proposed Solution
Replace all
"{{ $v }}"patterns with{{ $v | toJson }}.toJsonoutputs a valid JSON string (with surrounding quotes and proper escaping). TOML basic string syntax is compatible with JSON strings, so the output is valid TOML.hello"hello""hello"say "hi""say "hi""❌"say \"hi\""✅path\to\file"path\to\file"❌"path\\to\\file"✅Fields changed
envvaluescommandworking_dirallow_bot_messages(Discord)allow_bot_messages(Slack)allow_user_messages(Slack)model(STT)base_url(STT)Fields NOT changed (no user input)
bot_token,app_token,api_key${...}), not user valuesallowed_channels,allowed_users,trusted_bot_idstoJsonargstoJsonWhy this approach?
toJsonis already used elsewhere in this template (forargs,allowed_channels, etc.), so this is consistent with the existing escaping strategy. It produces output identical to the original for values without special characters, making it fully backward compatible.Alternatives Considered
1. Fix only
envvalues — rejectedThe
envfield has the highest risk since it accepts arbitrary user input. However, applying the same fix to all string fields is trivial (same one-line pattern) and eliminates the entire vulnerability class from the template. Leaving some fields unfixed would be inconsistent.2. Use Helm
quotefunction — rejectedquotewraps a value in double quotes but does not escape internal quotes or backslashes. It would not fix the injection.3. Use TOML literal strings (
'...') — rejectedTOML literal strings don't process escape sequences, but they also can't contain single quotes. This would trade one injection vector for another.
Validation
helm lint charts/openab— passed (0 failures)main(no behavioral change)Output:
All fields render correctly with proper quoting.
Scope
One file changed, 8 lines modified. No new features, no new values fields, no template logic changes. Pure security fix.