Resolve environment variable placeholders in configuration#47
Resolve environment variable placeholders in configuration#47
Conversation
MCP secrets are often scrubbed and replaced with {env:VAR} placeholders
by the opencode-synced plugin. However, these placeholders were not
being resolved by opencode at runtime, causing authentication failures.
This change implements a recursive environment variable resolution
mechanism in `applyOverridesToRuntimeConfig` to ensure that these
placeholders are correctly replaced with values from `process.env`.
Added tests to verify resolution of nested placeholders and handling
of missing environment variables.
Co-authored-by: iHildy <25069719+iHildy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's configuration system by enabling the dynamic resolution of environment variable placeholders. This change allows sensitive information, such as secrets, to be injected into the configuration at runtime via environment variables, improving security practices and making the application more adaptable across different deployment environments without hardcoding values. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
MCP secrets are often scrubbed and replaced with {env:VAR} placeholders
by the opencode-synced plugin. However, these placeholders were not
being resolved by opencode at runtime, causing authentication failures.
This change implements a recursive environment variable resolution
mechanism in `applyOverridesToRuntimeConfig` to ensure that these
placeholders are correctly replaced with values from `process.env`.
Added tests to verify resolution of nested placeholders and handling
of missing environment variables.
Co-authored-by: iHildy <25069719+iHildy@users.noreply.github.com>
MCP secrets are often scrubbed and replaced with {env:VAR} placeholders
by the opencode-synced plugin. However, these placeholders were not
being resolved by opencode at runtime, causing authentication failures.
This change implements a recursive environment variable resolution
mechanism in `applyOverridesToRuntimeConfig` to ensure that these
placeholders are correctly replaced with values from `process.env`.
Added tests to verify resolution of nested placeholders and handling
of missing environment variables.
Co-authored-by: iHildy <25069719+iHildy@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to resolve environment variable placeholders in the configuration. The implementation adds a recursive resolveEnvPlaceholders function. My review found a critical issue in the regular expression used for placeholder matching which would cause a runtime error. I've also suggested an improvement to make the recursive function robust against circular references. Additionally, I've provided feedback on the new tests to improve their isolation and reliability by using Vitest's environment stubbing features instead of modifying process.env directly.
| export function resolveEnvPlaceholders(config: unknown): unknown { | ||
| if (typeof config === 'string') { | ||
| return config.replace(/\{env:([^}]+)\}/g, (match, envVar) => { | ||
| const value = process.env[envVar]; | ||
| return value !== undefined ? value : match; | ||
| }); | ||
| } | ||
|
|
||
| if (Array.isArray(config)) { | ||
| return config.map((item) => resolveEnvPlaceholders(item)); | ||
| } | ||
|
|
||
| if (isPlainObject(config)) { | ||
| const result: Record<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(config)) { | ||
| result[key] = resolveEnvPlaceholders(value); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| return config; | ||
| } |
There was a problem hiding this comment.
This function has a critical bug and a potential robustness issue.
- Critical Bug: The regular expression
/{env:([^}]+)}/gis invalid because the{character is not escaped. This will cause aSyntaxErrorat runtime. It must be escaped as\{. - Robustness: The function is recursive and does not handle circular references in the input object, which could lead to a stack overflow.
I've provided a suggestion that fixes the regex and adds cycle detection using a WeakSet.
export function resolveEnvPlaceholders(config: unknown, seen = new WeakSet<object>()): unknown {
if (typeof config === 'string') {
return config.replace(/\{env:([^}]+)\}/g, (match, envVar) => {
const value = process.env[envVar];
return value !== undefined ? value : match;
});
}
if (config === null || typeof config !== 'object') {
return config;
}
if (seen.has(config)) {
return config;
}
seen.add(config);
if (Array.isArray(config)) {
return config.map((item) => resolveEnvPlaceholders(item, seen));
}
if (isPlainObject(config)) {
const result: Record<string, unknown> = {};
for (const [key, value] of Object.entries(config)) {
result[key] = resolveEnvPlaceholders(value, seen);
}
return result;
}
return config;
}| describe('applyOverridesToRuntimeConfig environment resolution', () => { | ||
| it('resolves {env:VAR} placeholders from process.env', () => { | ||
| process.env.TEST_VAR = 'secret-value'; | ||
| process.env.OTHER_VAR = 'other-value'; | ||
|
|
||
| const config: Record<string, unknown> = { | ||
| mcp: { | ||
| github: { | ||
| headers: { | ||
| Authorization: 'Bearer {env:TEST_VAR}', | ||
| }, | ||
| other: '{env:OTHER_VAR}', | ||
| }, | ||
| }, | ||
| unrelated: 'keep-me', | ||
| }; | ||
|
|
||
| const overrides = { | ||
| mcp: { | ||
| github: { | ||
| headers: { | ||
| Authorization: 'Bearer {env:TEST_VAR}', | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| // We apply overrides (which might already contain placeholders) | ||
| applyOverridesToRuntimeConfig(config, overrides); | ||
|
|
||
| const mcp = config.mcp as Record<string, Record<string, Record<string, string>>>; | ||
| expect(mcp.github.headers.Authorization).toBe('Bearer secret-value'); | ||
| expect(mcp.github.other).toBe('other-value'); | ||
| expect(config.unrelated).toBe('keep-me'); | ||
|
|
||
| delete process.env.TEST_VAR; | ||
| delete process.env.OTHER_VAR; | ||
| }); | ||
|
|
||
| it('handles missing environment variables by leaving placeholder intact', () => { | ||
| process.env.PRESENT_VAR = 'present'; | ||
| delete process.env.ABSENT_VAR; | ||
|
|
||
| const config: Record<string, unknown> = { | ||
| val: '{env:PRESENT_VAR}', | ||
| missing: '{env:ABSENT_VAR}', | ||
| }; | ||
|
|
||
| applyOverridesToRuntimeConfig(config, {}); | ||
|
|
||
| expect(config.val).toBe('present'); | ||
| // If it's missing, we keep it as is to avoid breaking things silently or passing empty strings | ||
| // that might be harder to debug. | ||
| expect(config.missing).toBe('{env:ABSENT_VAR}'); | ||
|
|
||
| delete process.env.PRESENT_VAR; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Modifying the global process.env object directly in tests can lead to flakiness, especially when tests are run in parallel. It's best practice to isolate tests from each other. Vitest provides vi.stubEnv() for this purpose, which is automatically restored after each test if you use vi.unstubAllEnvs() in an afterEach hook. This makes tests more robust and reliable.
Here's how you could refactor the tests:
import { describe, expect, it, vi, afterEach } from 'vitest';
// ...
describe('applyOverridesToRuntimeConfig environment resolution', () => {
afterEach(() => {
vi.unstubAllEnvs();
});
it('resolves {env:VAR} placeholders from process.env', () => {
vi.stubEnv('TEST_VAR', 'secret-value');
vi.stubEnv('OTHER_VAR', 'other-value');
// ... rest of test logic, no manual cleanup needed
});
it('handles missing environment variables by leaving placeholder intact', () => {
vi.stubEnv('PRESENT_VAR', 'present');
// ABSENT_VAR is not stubbed, so it will be undefined
// ... rest of test logic, no manual cleanup needed
});
});MCP secrets are often scrubbed and replaced with {env:VAR} placeholders
by the opencode-synced plugin. However, these placeholders were not
being resolved by opencode at runtime, causing authentication failures.
This change implements a recursive environment variable resolution
mechanism in `applyOverridesToRuntimeConfig` to ensure that these
placeholders are correctly replaced with values from `process.env`.
Added tests to verify resolution of nested placeholders and handling
of missing environment variables.
Co-authored-by: iHildy <25069719+iHildy@users.noreply.github.com>
1 participant
Implemented environment variable placeholder resolution in the configuration system. This ensures that
{env:VAR_NAME}placeholders (typically used for MCP secrets) are replaced with actual environment variable values at runtime. Added a recursiveresolveEnvPlaceholdersfunction and integrated it intoapplyOverridesToRuntimeConfig. Included comprehensive tests for various resolution scenarios.Fixes #44
PR created automatically by Jules for task 17967297301129502178 started by @iHildy