-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(security): use providers for messaging credential injection #1081
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
base: main
Are you sure you want to change the base?
Changes from all commits
6aa13c8
451dc57
d898544
499053d
9d4d5e1
752fc94
3fa1cf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1029,6 +1029,120 @@ const { createSandbox } = require(${onboardPath}); | |
| assert.doesNotMatch(createCommand.command, /SLACK_BOT_TOKEN=/); | ||
| }); | ||
|
|
||
| it("creates providers for messaging tokens and attaches them to the sandbox", async () => { | ||
| const repoRoot = path.join(import.meta.dirname, ".."); | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-messaging-providers-")); | ||
| const fakeBin = path.join(tmpDir, "bin"); | ||
| const scriptPath = path.join(tmpDir, "messaging-provider-check.js"); | ||
| const onboardPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "onboard.js")); | ||
| const runnerPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "runner.js")); | ||
| const registryPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "registry.js")); | ||
| const preflightPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "preflight.js")); | ||
| const credentialsPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "credentials.js")); | ||
|
|
||
| fs.mkdirSync(fakeBin, { recursive: true }); | ||
| fs.writeFileSync(path.join(fakeBin, "openshell"), "#!/usr/bin/env bash\nexit 0\n", { mode: 0o755 }); | ||
|
|
||
| const script = String.raw` | ||
| const runner = require(${runnerPath}); | ||
| const registry = require(${registryPath}); | ||
| const preflight = require(${preflightPath}); | ||
| const credentials = require(${credentialsPath}); | ||
| const childProcess = require("node:child_process"); | ||
| const { EventEmitter } = require("node:events"); | ||
|
|
||
| const commands = []; | ||
| runner.run = (command, opts = {}) => { | ||
| commands.push({ command, env: opts.env || null }); | ||
| return { status: 0 }; | ||
| }; | ||
| runner.runCapture = (command) => { | ||
| if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; | ||
| if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; | ||
| if (command.includes("'provider' 'get'")) return "Provider: discord-bridge"; | ||
| return ""; | ||
| }; | ||
| registry.registerSandbox = () => true; | ||
| registry.removeSandbox = () => true; | ||
| preflight.checkPortAvailable = async () => ({ ok: true }); | ||
| credentials.prompt = async () => ""; | ||
|
|
||
| childProcess.spawn = (...args) => { | ||
| const child = new EventEmitter(); | ||
| child.stdout = new EventEmitter(); | ||
| child.stderr = new EventEmitter(); | ||
| commands.push({ command: args[1][1], env: args[2]?.env || null }); | ||
| process.nextTick(() => { | ||
| child.stdout.emit("data", Buffer.from("Created sandbox: my-assistant\n")); | ||
| child.emit("close", 0); | ||
| }); | ||
| return child; | ||
| }; | ||
|
|
||
| const { createSandbox } = require(${onboardPath}); | ||
|
|
||
| (async () => { | ||
| process.env.OPENSHELL_GATEWAY = "nemoclaw"; | ||
| process.env.DISCORD_BOT_TOKEN = "test-discord-token-value"; | ||
| process.env.SLACK_BOT_TOKEN = "xoxb-test-slack-token-value"; | ||
| process.env.TELEGRAM_BOT_TOKEN = "123456:ABC-test-telegram-token"; | ||
| const sandboxName = await createSandbox(null, "gpt-5.4"); | ||
| console.log(JSON.stringify({ sandboxName, commands })); | ||
| })().catch((error) => { | ||
| console.error(error); | ||
| process.exit(1); | ||
| }); | ||
| `; | ||
| fs.writeFileSync(scriptPath, script); | ||
|
|
||
| const result = spawnSync(process.execPath, [scriptPath], { | ||
| cwd: repoRoot, | ||
| encoding: "utf-8", | ||
| env: { | ||
| ...process.env, | ||
| HOME: tmpDir, | ||
| PATH: `${fakeBin}:${process.env.PATH || ""}`, | ||
| NEMOCLAW_NON_INTERACTIVE: "1", | ||
| }, | ||
| }); | ||
|
|
||
| assert.equal(result.status, 0, result.stderr); | ||
| const payloadLine = result.stdout | ||
| .trim() | ||
| .split("\n") | ||
| .slice() | ||
| .reverse() | ||
| .find((line) => line.startsWith("{") && line.endsWith("}")); | ||
| assert.ok(payloadLine, `expected JSON payload in stdout:\n${result.stdout}`); | ||
| const payload = JSON.parse(payloadLine); | ||
|
|
||
| // Verify providers were created with the right credential keys | ||
| const providerCommands = payload.commands.filter((e) => e.command.includes("'provider' 'create'")); | ||
| const discordProvider = providerCommands.find((e) => e.command.includes("discord-bridge")); | ||
| assert.ok(discordProvider, "expected discord-bridge provider create command"); | ||
| assert.match(discordProvider.command, /'--credential' 'DISCORD_BOT_TOKEN'/); | ||
|
|
||
| const slackProvider = providerCommands.find((e) => e.command.includes("slack-bridge")); | ||
| assert.ok(slackProvider, "expected slack-bridge provider create command"); | ||
| assert.match(slackProvider.command, /'--credential' 'SLACK_BOT_TOKEN'/); | ||
|
|
||
| const telegramProvider = providerCommands.find((e) => e.command.includes("telegram-bridge")); | ||
| assert.ok(telegramProvider, "expected telegram-bridge provider create command"); | ||
| assert.match(telegramProvider.command, /'--credential' 'TELEGRAM_BOT_TOKEN'/); | ||
|
|
||
| // Verify sandbox create includes --provider flags for all three | ||
| const createCommand = payload.commands.find((e) => e.command.includes("'sandbox' 'create'")); | ||
| assert.ok(createCommand, "expected sandbox create command"); | ||
| assert.match(createCommand.command, /'--provider' 'discord-bridge'/); | ||
| assert.match(createCommand.command, /'--provider' 'slack-bridge'/); | ||
| assert.match(createCommand.command, /'--provider' 'telegram-bridge'/); | ||
|
|
||
| // Verify real token values are NOT in the sandbox create command or env | ||
| assert.doesNotMatch(createCommand.command, /test-discord-token-value/); | ||
| assert.doesNotMatch(createCommand.command, /xoxb-test-slack-token-value/); | ||
| assert.doesNotMatch(createCommand.command, /123456:ABC-test-telegram-token/); | ||
| }); | ||
|
Comment on lines
+1032
to
+1144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major The secret-scrubbing path is still untested. Line 1140 says “command or env”, but the assertions only inspect 🧪 Suggested test hardening (async () => {
process.env.OPENSHELL_GATEWAY = "nemoclaw";
- process.env.DISCORD_BOT_TOKEN = "test-discord-token-value";
- process.env.SLACK_BOT_TOKEN = "xoxb-test-slack-token-value";
- process.env.TELEGRAM_BOT_TOKEN = "123456:ABC-test-telegram-token";
+ Object.assign(process.env, {
+ DISCORD_BOT_TOKEN: "test-discord-token-value",
+ SLACK_BOT_TOKEN: "xoxb-test-slack-token-value",
+ TELEGRAM_BOT_TOKEN: "123456:ABC-test-telegram-token",
+ OPENAI_API_KEY: "sk-openai-test",
+ ANTHROPIC_API_KEY: "sk-ant-test",
+ GEMINI_API_KEY: "gemini-test",
+ COMPATIBLE_API_KEY: "compatible-test",
+ COMPATIBLE_ANTHROPIC_API_KEY: "compatible-ant-test",
+ });
const sandboxName = await createSandbox(null, "gpt-5.4");
console.log(JSON.stringify({ sandboxName, commands }));
})().catch((error) => {
@@
assert.doesNotMatch(createCommand.command, /test-discord-token-value/);
assert.doesNotMatch(createCommand.command, /xoxb-test-slack-token-value/);
assert.doesNotMatch(createCommand.command, /123456:ABC-test-telegram-token/);
+ for (const name of [
+ "DISCORD_BOT_TOKEN",
+ "SLACK_BOT_TOKEN",
+ "TELEGRAM_BOT_TOKEN",
+ "OPENAI_API_KEY",
+ "ANTHROPIC_API_KEY",
+ "GEMINI_API_KEY",
+ "COMPATIBLE_API_KEY",
+ "COMPATIBLE_ANTHROPIC_API_KEY",
+ ]) {
+ assert.equal(createCommand.env?.[name], undefined, `${name} leaked into sandbox env`);
+ }
});🤖 Prompt for AI Agents |
||
|
|
||
| it("continues once the sandbox is Ready even if the create stream never closes", async () => { | ||
| const repoRoot = path.join(import.meta.dirname, ".."); | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-create-ready-")); | ||
|
|
||
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.
This verification can pass even when the sandbox binding is broken.
verifyProviderExists()only does a globalprovider gettext check. It treats any non-empty output that does not literally contain"not found"as success, and it never inspects whether the just-created sandbox is actually bound to that provider. A CLI/gateway error or a dropped--providerattachment will still pass here, so this warning gives false confidence.At minimum, base the existence check on command status; ideally, verify the sandbox’s attached providers rather than only global provider existence.
Also applies to: 1910-1916
🤖 Prompt for AI Agents