-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add environment subsition for auth blocks #5439
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
Conversation
1d19b9e to
f92c6f1
Compare
|
@danielcooper Can you run a quick |
Allows auth to be defined like:
```
extensions:
- name: example
enabled: true
type: streamable_http
uri: https://example.com/mcp
env_keys:
- EXAMPLE_COM_SERVICE_TOKEN
headers:
Authorization: Bearer ${ EXAMPLE_COM_SERVICE_TOKEN }
```
Which is very useful if you’re sharing recipes with a team
Signed-off-by: Daniel Cooper <daniel@14lines.com>
fbf82bd to
10c7b5e
Compare
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.
Pull Request Overview
This PR adds environment variable substitution support for HTTP headers in StreamableHttp extension configurations. Header values can now reference environment variables using ${VAR} or $VAR syntax, with the variables being resolved from both direct envs config and keychain-stored env_keys.
- Implements a
substitute_env_varshelper function supporting${VAR}and$VARsyntax with optional whitespace - Merges environment variables from multiple sources before substitution
- Adds comprehensive test coverage for the substitution functionality
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}") | ||
| .expect("valid regex"); |
Copilot
AI
Nov 5, 2025
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.
Compiling regex patterns on every function call is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock (already used elsewhere in this codebase) and stored as static variables to avoid recompilation overhead.
| let re_simple = | ||
| regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); |
Copilot
AI
Nov 5, 2025
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.
Compiling regex patterns on every function call is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock (already used elsewhere in this codebase) and stored as static variables to avoid recompilation overhead.
| // Then handle $VAR syntax (simple variable without braces) | ||
| let re_simple = | ||
| regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); | ||
| for cap in re_simple.captures_iter(&result.clone()) { |
Copilot
AI
Nov 5, 2025
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.
Unnecessary clone of result string. Since captures_iter only needs a reference and doesn't consume the string, you can pass &result directly without cloning.
| for cap in re_simple.captures_iter(&result.clone()) { | |
| for cap in re_simple.captures_iter(&result) { |
| let mut result = value.to_string(); | ||
|
|
||
| // First handle ${VAR} syntax (with optional whitespace) | ||
| let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}") | ||
| .expect("valid regex"); | ||
| for cap in re_braces.captures_iter(value) { | ||
| if let Some(var_name) = cap.get(1) { | ||
| if let Some(env_value) = env_map.get(var_name.as_str()) { | ||
| result = result.replace(&cap[0], env_value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Then handle $VAR syntax (simple variable without braces) | ||
| let re_simple = | ||
| regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); | ||
| for cap in re_simple.captures_iter(&result.clone()) { | ||
| if let Some(var_name) = cap.get(1) { | ||
| // Only substitute if it wasn't already part of ${VAR} syntax | ||
| if !value.contains(&format!("${{{}}}", var_name.as_str())) { | ||
| if let Some(env_value) = env_map.get(var_name.as_str()) { | ||
| result = result.replace(&cap[0], env_value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result |
Copilot
AI
Nov 5, 2025
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.
Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., ${TOKEN} and ${TOKEN}), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.
| let mut result = value.to_string(); | |
| // First handle ${VAR} syntax (with optional whitespace) | |
| let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}") | |
| .expect("valid regex"); | |
| for cap in re_braces.captures_iter(value) { | |
| if let Some(var_name) = cap.get(1) { | |
| if let Some(env_value) = env_map.get(var_name.as_str()) { | |
| result = result.replace(&cap[0], env_value); | |
| } | |
| } | |
| } | |
| // Then handle $VAR syntax (simple variable without braces) | |
| let re_simple = | |
| regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); | |
| for cap in re_simple.captures_iter(&result.clone()) { | |
| if let Some(var_name) = cap.get(1) { | |
| // Only substitute if it wasn't already part of ${VAR} syntax | |
| if !value.contains(&format!("${{{}}}", var_name.as_str())) { | |
| if let Some(env_value) = env_map.get(var_name.as_str()) { | |
| result = result.replace(&cap[0], env_value); | |
| } | |
| } | |
| } | |
| } | |
| result | |
| // First handle ${VAR} syntax (with optional whitespace) | |
| let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}") | |
| .expect("valid regex"); | |
| let result = re_braces.replace_all(value, |caps: ®ex::Captures| { | |
| let var_name = &caps[1]; | |
| env_map.get(var_name).map(|v| v.as_str()).unwrap_or(caps.get(0).unwrap().as_str()) | |
| }); | |
| // Then handle $VAR syntax (simple variable without braces) | |
| let re_simple = regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); | |
| // Only substitute $VAR if it wasn't already part of ${VAR} syntax in the original value | |
| let result = re_simple.replace_all(&result, |caps: ®ex::Captures| { | |
| let var_name = &caps[1]; | |
| // Only substitute if not present as ${VAR} in the original value | |
| if !value.contains(&format!("${{{}}}", var_name)) { | |
| env_map.get(var_name).map(|v| v.as_str()).unwrap_or(caps.get(0).unwrap().as_str()) | |
| } else { | |
| caps.get(0).unwrap().as_str() | |
| } | |
| }); | |
| result.into_owned() |
| // Only substitute if it wasn't already part of ${VAR} syntax | ||
| if !value.contains(&format!("${{{}}}", var_name.as_str())) { | ||
| if let Some(env_value) = env_map.get(var_name.as_str()) { | ||
| result = result.replace(&cap[0], env_value); |
Copilot
AI
Nov 5, 2025
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.
Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., $TOKEN and $TOKEN), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.
| result = result.replace(&cap[0], env_value); | |
| result = result.replacen(&cap[0], env_value, 1); |
| } | ||
|
|
||
| // Then handle $VAR syntax (simple variable without braces) | ||
| let re_simple = regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); |
Copilot
AI
Nov 5, 2025
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.
Compiling regex patterns on every test invocation is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock and stored as static variables to avoid recompilation overhead.
|
|
||
| // Then handle $VAR syntax (simple variable without braces) | ||
| let re_simple = regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex"); | ||
| for cap in re_simple.captures_iter(&result.clone()) { |
Copilot
AI
Nov 5, 2025
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.
Unnecessary clone of result string. Since captures_iter only needs a reference and doesn't consume the string, you can pass &result directly without cloning.
| for cap in re_simple.captures_iter(&result.clone()) { | |
| for cap in re_simple.captures_iter(&result) { |
| for cap in re_braces.captures_iter(value) { | ||
| if let Some(var_name) = cap.get(1) { | ||
| if let Some(env_value) = env_map.get(var_name.as_str()) { | ||
| result = result.replace(&cap[0], env_value); |
Copilot
AI
Nov 5, 2025
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.
Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., ${TOKEN} and ${TOKEN}), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.
| result = result.replace(&cap[0], env_value); | |
| result = result.replacen(&cap[0], env_value, 1); |
| // Only substitute if it wasn't already part of ${VAR} syntax | ||
| if !value.contains(&format!("${{{}}}", var_name.as_str())) { | ||
| if let Some(env_value) = env_map.get(var_name.as_str()) { | ||
| result = result.replace(&cap[0], env_value); |
Copilot
AI
Nov 5, 2025
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.
Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., $TOKEN and $TOKEN), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.
|
|
||
| // Test the substitute_env_vars helper function (which is defined inside add_extension) | ||
| // We'll recreate it here for testing purposes | ||
| fn substitute_env_vars(value: &str, env_map: &HashMap<String, String>) -> String { |
Copilot
AI
Nov 5, 2025
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.
The substitute_env_vars function is duplicated between the production code (line 362) and the test code (line 1569). Consider extracting this function to module-level scope or creating a shared helper module to eliminate code duplication and ensure consistent behavior.
* main: (41 commits) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) ...
* main: fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409)
* origin/main: (75 commits) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) ...
* main: (60 commits) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) ...
* main: (31 commits) Standardize CLI argument flags and update documentation (#5516) Release 1.13.0 fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) ...
Signed-off-by: Daniel Cooper <daniel@14lines.com> Signed-off-by: fbalicchia <fbalicchia@gmail.com>
Signed-off-by: Daniel Cooper <daniel@14lines.com>
Signed-off-by: Daniel Cooper <daniel@14lines.com>
Signed-off-by: Daniel Cooper <daniel@14lines.com> Signed-off-by: Blair Allan <Blairallan@icloud.com>
Summary
Allows auth to be defined like:
Which is very useful if you’re sharing recipes with a team. We were previously relying on: #5282 and not defining extentions in recipes we shared.
Note: this is somewhat vibey, I don't know rust.
Type of Change
Testing
Unit test, tested manually with our workflow.
Related Issues
Relates to #ISSUE_ID
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After:
Email: