fix: avoid false hardcoded API key warning for env interpolation#1
fix: avoid false hardcoded API key warning for env interpolation#1100yenadmin wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a false-positive security warning in the Cortex OpenClaw plugin by distinguishing ${ENV_VAR} interpolation from truly hardcoded API keys before environment resolution.
Changes:
- Detect
${VAR}interpolation using the raw plugin config value (pre-resolveEnv) to suppress the warning for env-backed keys. - Keep the warning for truly hardcoded API keys (with a new “warn once per process” guard).
- Add a regression test covering both interpolated and hardcoded API key cases.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Adds isEnvInterpolation() and updates hardcoded-key warning logic to consult raw config before env resolution. |
tests/api-key-warning.test.mjs |
Adds regression tests for interpolated vs hardcoded apiKey warning behavior. |
dist/index.js |
Compiled output reflecting the updated warning logic. |
dist/index.js.map |
Updated sourcemap for the compiled JS. |
dist/index.d.ts.map |
Updated declaration sourcemap for the compiled types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { pathToFileURL } from 'node:url'; | ||
|
|
||
| async function loadPlugin() { | ||
| const url = pathToFileURL(new URL('../dist/index.js', import.meta.url).pathname); |
There was a problem hiding this comment.
loadPlugin() builds a file URL via pathToFileURL(new URL(...).pathname). Using .pathname can break on Windows (leading /C:/...) and loses URL encoding info. Prefer using the new URL('../dist/index.js', import.meta.url) result directly (or pathToFileURL(fileURLToPath(...)) if you need a path) so the test is portable across platforms.
| import { pathToFileURL } from 'node:url'; | |
| async function loadPlugin() { | |
| const url = pathToFileURL(new URL('../dist/index.js', import.meta.url).pathname); | |
| async function loadPlugin() { | |
| const url = new URL('../dist/index.js', import.meta.url); |
| process.env.CORTEX_API_KEY = 'secret-from-env'; | ||
| const warnings = []; | ||
| const plugin = await loadPlugin(); | ||
| plugin.register(makeApi( | ||
| { apiKey: '${CORTEX_API_KEY}', cortexUrl: 'http://localhost:8000' }, | ||
| { warn: (msg) => warnings.push(msg), info() {} }, | ||
| )); | ||
|
|
||
| assert.equal(warnings.some((msg) => msg.includes('hardcoded in config')), false); |
There was a problem hiding this comment.
This test sets process.env.CORTEX_API_KEY but never restores it. That can leak state into other tests when the suite grows or when tests are executed in a shared process. Save the previous value and restore it in a finally block or via test(..., (t) => { t.after(() => ...) })/afterEach.
| process.env.CORTEX_API_KEY = 'secret-from-env'; | |
| const warnings = []; | |
| const plugin = await loadPlugin(); | |
| plugin.register(makeApi( | |
| { apiKey: '${CORTEX_API_KEY}', cortexUrl: 'http://localhost:8000' }, | |
| { warn: (msg) => warnings.push(msg), info() {} }, | |
| )); | |
| assert.equal(warnings.some((msg) => msg.includes('hardcoded in config')), false); | |
| const previousApiKey = process.env.CORTEX_API_KEY; | |
| process.env.CORTEX_API_KEY = 'secret-from-env'; | |
| try { | |
| const warnings = []; | |
| const plugin = await loadPlugin(); | |
| plugin.register(makeApi( | |
| { apiKey: '${CORTEX_API_KEY}', cortexUrl: 'http://localhost:8000' }, | |
| { warn: (msg) => warnings.push(msg), info() {} }, | |
| )); | |
| assert.equal(warnings.some((msg) => msg.includes('hardcoded in config')), false); | |
| } finally { | |
| if (previousApiKey === undefined) { | |
| delete process.env.CORTEX_API_KEY; | |
| } else { | |
| process.env.CORTEX_API_KEY = previousApiKey; | |
| } | |
| } |
| // Module-level flag: emit hardcoded-key warning only once per process lifetime | ||
| let _apiKeyWarningEmitted = false; | ||
|
|
There was a problem hiding this comment.
The new module-level _apiKeyWarningEmitted changes behavior to only warn once per process lifetime. This is not mentioned in the PR description and can suppress warnings if the plugin is re-registered later with a hardcoded key (or in multi-tenant scenarios). If this is intentional, consider calling it out in the PR description and/or scoping the flag to a plugin instance/config so each distinct registration can still emit the warning when appropriate.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import { dirname, join } from "node:path"; | ||
|
|
||
| // Module-level flag: emit hardcoded-key warning only once per process lifetime | ||
| let _apiKeyWarningEmitted = false; |
There was a problem hiding this comment.
📝 Info: Module-level _apiKeyWarningEmitted suppresses warnings across independent registrations
The _apiKeyWarningEmitted flag at src/index.ts:32 is module-scoped, meaning once set by the first register() call with a hardcoded key, ALL subsequent registrations — even for different agents or owners with different hardcoded keys — will silently skip the warning. This is intentional per the commit message (suppress subagent noise), but it means a genuinely misconfigured second registration in the same process would not be warned about. In practice this is unlikely since multi-registration typically uses the same config, but worth noting as a behavioral trade-off.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async function loadPlugin() { | ||
| const url = pathToFileURL(new URL('../dist/index.js', import.meta.url).pathname); | ||
| url.searchParams.set('t', `${Date.now()}-${Math.random()}`); | ||
| const mod = await import(url.href); | ||
| return mod.default?.default ?? mod.default ?? mod; | ||
| } |
There was a problem hiding this comment.
📝 Info: Test cache busting may not isolate CJS module state, but tests pass regardless
The loadPlugin() function at tests/api-key-warning.test.mjs:6-9 appends a random query parameter to bust the ESM import cache. However, for CJS modules loaded via import(), Node.js may still resolve to the same require cache entry, sharing the module-level _apiKeyWarningEmitted state across test invocations. This doesn't cause test failures because: Test 1 (env-var config) never sets the flag (the isEnvInterpolation check short-circuits), so Test 2 (hardcoded config) still sees _apiKeyWarningEmitted === false and correctly fires the warning. The tests are order-independent for the same reason. However, if someone adds a test with a hardcoded key before the 'does not warn' test, and the cache busting doesn't work, the shared flag state could cause unexpected interactions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function isEnvInterpolation(value: unknown): value is string { | ||
| return typeof value === "string" && /^\$\{[^}]+\}$/.test(value.trim()); | ||
| } |
There was a problem hiding this comment.
📝 Info: isEnvInterpolation only matches full-string interpolation, not partial
The regex /^\$\{[^}]+\}$/ at src/index.ts:94 requires the entire trimmed string to be a single ${...} token. Partial interpolation like "prefix-${KEY}" or "${KEY1}-${KEY2}" would NOT match, causing the hardcoded-key warning to fire. This is reasonable behavior — partial interpolation of API keys is atypical and arguably a misconfiguration worth flagging — but it's worth noting that resolveEnv() at src/index.ts:89-91 supports partial/multiple interpolations via its global .replace() regex.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
${VAR}API key config from the raw plugin config before env resolutionVerification
Context
Fixes the false warning reported in 100yenadmin/electric-sheep#1701 when users follow the recommended
~/.openclaw/.env+${CORTEX_API_KEY}setup.