Skip to content

Resolve environment variable placeholders in configuration #47

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.

Sign up for GitHub

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

Open
iHildy wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Resolve environment variable placeholders in configuration #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a23f3a
fix: resolve {env:VAR} placeholders in config at runtime
google-labs-jules[bot] Mar 14, 2026
8b2e53d
fix: resolve {env:VAR} placeholders in config at runtime
google-labs-jules[bot] Mar 14, 2026
0599e0a
fix: resolve {env:VAR} placeholders in config at runtime
google-labs-jules[bot] Mar 14, 2026
96f3988
fix: resolve {env:VAR} placeholders in config at runtime
google-labs-jules[bot] Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/sync/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,34 @@ export function applyOverridesToRuntimeConfig(
overrides: Record<string, unknown>
): void {
const merged = deepMerge(config, overrides) as Record<string, unknown>;
const resolved = resolveEnvPlaceholders(merged);
for (const key of Object.keys(config)) {
delete config[key];
}
Object.assign(config, merged);
Object.assign(config, resolved);
}

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;
}
Comment on lines +195 to 216
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This function has a critical bug and a potential robustness issue.

  1. Critical Bug: The regular expression /{env:([^}]+)}/g is invalid because the { character is not escaped. This will cause a SyntaxError at runtime. It must be escaped as \{.
  2. 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;
}


export function deepMerge<T>(base: T, override: unknown): T {
Expand Down
61 changes: 61 additions & 0 deletions src/sync/resolve.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { describe, expect, it } from 'vitest';
import { applyOverridesToRuntimeConfig } from './config.js';

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;
});
});
Comment on lines +4 to +61
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  });
});

Loading