fix(mcp): preserve ':-' inside ${VAR:-default} default values - #1933
Conversation
expandEnvVarsInString split the ${VAR:-default} syntax with
varContent.split(':-', 2). The code comment says the limit is there to
"preserve :- in defaults", but JavaScript's String.split(sep, limit) caps
the array length and discards the remainder — it is not a maxsplit that
glues the tail back on. So a default that itself contains ':-' is
truncated at the first occurrence: ${VAR:-a:-b} expands to "a" instead of
bash's "a:-b".
Slice at the first ':-' with indexOf so any later ':-' stays in the
default. This runs over user .mcp.json command/args/env/url/headers values.
Add coverage for the ':-'-in-default case plus set/unset/empty-default and
missing-var paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
src/{skills,utils/plugins,services/mcp}/**⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.44.1)src/services/mcp/envExpansion.test.ts[error] 8-8: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) [error] 13-16: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughChangesEnvironment expansion
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
…ne#1933) expandEnvVarsInString split the ${VAR:-default} syntax with varContent.split(':-', 2). The code comment says the limit is there to "preserve :- in defaults", but JavaScript's String.split(sep, limit) caps the array length and discards the remainder — it is not a maxsplit that glues the tail back on. So a default that itself contains ':-' is truncated at the first occurrence: ${VAR:-a:-b} expands to "a" instead of bash's "a:-b". Slice at the first ':-' with indexOf so any later ':-' stays in the default. This runs over user .mcp.json command/args/env/url/headers values. Add coverage for the ':-'-in-default case plus set/unset/empty-default and missing-var paths.
3 participants
Problem
expandEnvVarsInStringsplit the${VAR:-default}syntax with:The comment says the limit preserves
:-inside the default, but JavaScript'sString.split(sep, limit)caps the array length and discards the remainder — it is not a Python-stylemaxsplitthat glues the tail back on. So a default containing:-is truncated at the first occurrence:${VAR:-a:-b}->a:-baReachability
expandEnvVars(src/services/mcp/config.ts) runs this over user.mcp.jsonserver fields —command, eachargs[], everyenvvalue,url, and everyheadersvalue (also via the plugin MCP/LSP integrations). Any${VAR:-default}whose default contains:-(unset var) gets truncated.Fix
Slice at the first
:-withindexOfso any later:-stays in the default, matching bash.Test
New suite:
:--in-default preserved, set var wins, unset uses default, empty default${VAR:-}->'', missing var reported. The first case fails before the fix (avsa:-b).Summary by CodeRabbit
Bug Fixes
:-are preserved.Tests