Skip to content

fix(security): use providers for messaging credential injection#1081

Draft
ericksoa wants to merge 7 commits intomainfrom
feat/messaging-credential-providers
Draft

fix(security): use providers for messaging credential injection#1081
ericksoa wants to merge 7 commits intomainfrom
feat/messaging-credential-providers

Conversation

@ericksoa
Copy link
Copy Markdown
Contributor

@ericksoa ericksoa commented Mar 30, 2026

Summary

  • Create providers for Discord, Slack, and Telegram tokens during onboard via openshell provider create
  • Slack and Discord credentials flow through the provider/placeholder system — sandbox processes see placeholders, real tokens are injected at the proxy layer via Authorization header rewriting
  • Telegram provider is created for credential storage; host-side bridge continues to operate as before (URL-path auth pattern requires future proxy work)
  • Raw env var injection for Discord and Slack removed from sandbox creation
  • Expanded credential blocklist in sandboxEnv to cover all known secret env vars

Fixes #616
Supersedes #617

How it works

All three messaging SDKs authenticate via HTTP Authorization headers:

  • Discord (@buape/carbon): REST-only, Authorization: Bot {token}
  • Slack (@slack/bolt Socket Mode): REST call to apps.connections.open with Authorization: Bearer {appToken} to get a pre-authenticated WebSocket URL
  • Telegram (grammy): token in URL path (/bot{TOKEN}/) — not header-rewritable, host-side bridge unchanged

The proxy TLS-terminates outbound requests and rewrites placeholder values in Authorization headers with real secrets before forwarding upstream. Sandbox processes never see real token values.

Changes

bin/lib/onboard.js — In createSandbox(), create generic providers for each detected messaging token using the existing upsertProvider() helper. Attach them to the sandbox via --provider flags. Remove direct DISCORD_BOT_TOKEN and SLACK_BOT_TOKEN injection into sandbox env. Verify providers exist post-creation. Replace individual env var deletes with a comprehensive blocklist.

test/onboard.test.js — New test verifies provider create commands are issued with correct credential keys, sandbox create includes --provider flags for all three tokens, and real token values never appear in the sandbox create command.

Testing

  • All unit tests pass (35/35 in onboard.test.js)
  • No new dependencies
  • Negative path: onboard completes without messaging providers when no tokens are set
  • Draft: pending E2E validation with real bot tokens

Create OpenShell providers for Discord, Slack, and Telegram tokens
during onboard instead of passing them as raw environment variables
into the sandbox. The L7 proxy rewrites Authorization headers
(Bearer/Bot) with real secrets at egress, so sandbox processes never
see the actual token values.

- Discord and Slack tokens flow through provider/placeholder system
- Telegram provider is created for credential storage but host-side
  bridge still reads from host env (Telegram uses URL-path auth that
  the proxy can't rewrite yet)
- Raw env var injection removed for Discord and Slack
@ericksoa ericksoa added fix Getting Started Use this label to identify setup, installation, or onboarding issues. labels Mar 30, 2026
@ericksoa ericksoa self-assigned this Mar 30, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b50f21fe-699d-4b8f-bb82-41827934f5c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

createSandbox now upserts messaging providers (discord-bridge, slack-bridge, telegram-bridge when token present), passes provider flags to sandbox creation, filters/removes sensitive messaging and API env vars from the sandbox environment, adds verifyProviderExists(name), and logs actionable warnings if providers are missing.

Changes

Cohort / File(s) Summary
Sandbox onboarding & provider provisioning
bin/lib/onboard.js
Added internal verifyProviderExists(name) which runs openshell provider get. Replaced direct injection of messaging tokens with conditional upsertProvider(...) calls for discord-bridge, slack-bridge, and telegram-bridge (when TELEGRAM_BOT_TOKEN present). Appends --provider <name> flags to sandbox create args. Filters process.env to block selected credential and API env vars (Discord/Slack/Telegram tokens, API keys, etc.) before passing env into the sandbox; no longer passes Telegram token as sandbox env var. After DNS proxy setup, verifies attached providers and logs warnings with “To fix” provider creation commands for any missing providers.
Tests — onboarding
test/onboard.test.js
Added Vitest integration test that stubs runner/registry/preflight/credentials, asserts upsertProvider/provider creation commands for discord-bridge, slack-bridge, telegram-bridge include correct credential flag keys, verifies --provider flags appear in sandbox create invocation, and asserts raw token values are not leaked into the sandbox create argv/env.

Sequence Diagram

sequenceDiagram
    participant Creator as createSandbox()
    participant CLI as OpenShell CLI
    participant Providers as Providers Store
    participant Sandbox as Sandbox Creator
    participant Proxy as DNS Proxy/Gateway

    Creator->>CLI: check provider with `openshell provider get <name>` (verifyProviderExists)
    alt provider missing
        CLI-->>Creator: non-zero / missing
        Creator->>Creator: log warning with "To fix: openshell provider upsert ..."
    else provider present
        Creator->>CLI: `openshell provider upsert <name> --credential ...`
        CLI-->>Providers: create/update provider
        Providers-->>Creator: success
    end

    Creator->>Creator: append `--provider <name>` to createArgs
    Creator->>Sandbox: create sandbox with createArgs and filtered env
    Sandbox-->>Creator: sandbox created

    Creator->>Proxy: configure DNS proxy / attach providers
    Proxy->>Providers: request credential for provider
    Providers-->>Proxy: return secret
    Proxy->>Sandbox: inject secret on egress
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped through code and stitched the seams,
Providers planted like bright little beams.
Tokens tucked away where prying noses stop,
Telegram joined the hop-hop squad on top.
Whisker-twitch—sandboxed secrets keep their crop.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main security improvement: migrating messaging credentials from direct environment variable injection to a provider-based system for better credential isolation.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/messaging-credential-providers

Comment @coderabbitai help to get the list of available commands and usage tips.

@ericksoa ericksoa changed the title feat(onboard): use providers for messaging credential injection fix(onboard): use providers for messaging credential injection Mar 30, 2026
@ericksoa ericksoa changed the title fix(onboard): use providers for messaging credential injection fix(security): use providers for messaging credential injection Mar 30, 2026
@ericksoa ericksoa added the security Something isn't secure label Mar 30, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
bin/lib/onboard.js (1)

1816-1819: Consider adding a defensive test for credential deletion.

Per the test at test/credential-exposure.test.js:76-84, the current tests verify that credentials aren't pushed to envArgs, but don't assert that the delete statements exist. This means if the deletion logic were accidentally removed, the test would still pass.

Consider adding a test that explicitly verifies the delete statements are present, or that sandboxEnv passed to streamSandboxCreate excludes these keys.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 1816 - 1819, Add a defensive test that
ensures the credential keys are removed before creating the sandbox: call or
simulate the same code path that builds sandboxEnv (the code that copies
process.env into sandboxEnv and runs delete for NVIDIA_API_KEY,
DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) and assert that sandboxEnv no longer has
those properties (or mock/spy the call to streamSandboxCreate and assert the
passed env object excludes those keys). Locate the logic around sandboxEnv and
the call to streamSandboxCreate and add a unit test that explicitly checks
sandboxEnv[NVIDIA_API_KEY], sandboxEnv[DISCORD_BOT_TOKEN], and
sandboxEnv[SLACK_BOT_TOKEN] are undefined (or that streamSandboxCreate received
an env object without those keys).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1817-1819: The code deletes Discord and Slack tokens from the
sandboxEnv but misses removing TELEGRAM_BOT_TOKEN, so add a deletion for
sandboxEnv.TELEGRAM_BOT_TOKEN in the same block (the same place where delete
sandboxEnv.NVIDIA_API_KEY, delete sandboxEnv.DISCORD_BOT_TOKEN, delete
sandboxEnv.SLACK_BOT_TOKEN are called) before calling streamSandboxCreate;
ensure sandboxEnv no longer contains TELEGRAM_BOT_TOKEN when passed into
streamSandboxCreate to prevent leaking the token into the sandbox environment.

---

Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1816-1819: Add a defensive test that ensures the credential keys
are removed before creating the sandbox: call or simulate the same code path
that builds sandboxEnv (the code that copies process.env into sandboxEnv and
runs delete for NVIDIA_API_KEY, DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN) and assert
that sandboxEnv no longer has those properties (or mock/spy the call to
streamSandboxCreate and assert the passed env object excludes those keys).
Locate the logic around sandboxEnv and the call to streamSandboxCreate and add a
unit test that explicitly checks sandboxEnv[NVIDIA_API_KEY],
sandboxEnv[DISCORD_BOT_TOKEN], and sandboxEnv[SLACK_BOT_TOKEN] are undefined (or
that streamSandboxCreate received an env object without those keys).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 56f004ea-2e71-4082-85da-58cba9169f7b

📥 Commits

Reviewing files that changed from the base of the PR and between a146385 and 6aa13c8.

📒 Files selected for processing (1)
  • bin/lib/onboard.js

Add verifyProviderExists() check post-sandbox-creation to confirm
messaging credential providers are actually in the gateway. Warns
with remediation steps if a provider is missing.
…aging tokens

Verify that messaging credentials flow through the provider system:
- Provider create commands issued with correct credential key names
- Sandbox create includes --provider flags for all detected tokens
- Real token values never appear in sandbox create command or env
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
bin/lib/onboard.js (1)

1815-1824: ⚠️ Potential issue | 🔴 Critical

sandboxEnv is still a secret leak path.

createSandbox() runs after setupInference(), and setupInference() hydrates the selected inference credential back into process.env. Starting from { ...process.env } and deleting only three names still forwards raw OPENAI_API_KEY, ANTHROPIC_API_KEY, GEMINI_API_KEY, COMPATIBLE_API_KEY, COMPATIBLE_ANTHROPIC_API_KEY, and TELEGRAM_BOT_TOKEN when present, which contradicts the “non-sensitive env vars only” comment.

Proposed fix
-  const sandboxEnv = { ...process.env };
-  delete sandboxEnv.NVIDIA_API_KEY;
-  delete sandboxEnv.DISCORD_BOT_TOKEN;
-  delete sandboxEnv.SLACK_BOT_TOKEN;
+  const blockedSandboxEnvNames = new Set([
+    "NVIDIA_API_KEY",
+    "OPENAI_API_KEY",
+    "ANTHROPIC_API_KEY",
+    "GEMINI_API_KEY",
+    "COMPATIBLE_API_KEY",
+    "COMPATIBLE_ANTHROPIC_API_KEY",
+    "DISCORD_BOT_TOKEN",
+    "SLACK_BOT_TOKEN",
+    "TELEGRAM_BOT_TOKEN",
+  ]);
+  const sandboxEnv = Object.fromEntries(
+    Object.entries(process.env).filter(([name]) => !blockedSandboxEnvNames.has(name))
+  );

An explicit allowlist would be safer long-term, but the current blocklist is incomplete even for credentials this file already hydrates itself.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 1815 - 1824, The sandboxEnv assignment leaks
secret keys because it clones process.env then deletes a few tokens; change it
to build sandboxEnv from an explicit allowlist of non-sensitive variables
instead of spreading process.env. In the block that currently defines envArgs
and sandboxEnv (symbols: formatEnvAssignment, envArgs, sandboxEnv), replace the
copy-and-delete approach with a whitelist array of safe env names and populate
sandboxEnv only from those names (ensuring formatEnvAssignment still sets
CHAT_UI_URL), and remove reliance on setupInference() hydrating secrets into
process.env when constructing sandboxEnv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1789-1791: The Telegram branch creates the provider by calling
upsertProvider() but doesn't populate the host env from stored credentials, so
legacy bridge won't see TELEGRAM_BOT_TOKEN; call hydrateCredentialEnv() (the
helper that writes stored credentials into process.env) before creating the
Telegram provider and before any early return that creates/upserts the provider
(i.e., add hydrateCredentialEnv() calls in the Telegram handling branches around
where upsertProvider() is invoked) so stored tokens are applied to process.env
prior to provider creation.
- Around line 480-483: The current verifyProviderExists function only checks
output text for "not found" which can hide command failures or an unbound
sandbox; change verifyProviderExists (and the duplicate call site around the
other provider-check code) to rely on the command exit status from
runCaptureOpenshell (or its underlying spawn API) instead of string matching,
and additionally validate the provider is attached to the target sandbox by
running a sandbox-scoped query (e.g., list/inspect the sandbox's providers) and
confirming the provider name appears in that result; update logic in
verifyProviderExists to return false on non-zero exit or when the
sandbox-attached provider list does not include the given name.

---

Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 1815-1824: The sandboxEnv assignment leaks secret keys because it
clones process.env then deletes a few tokens; change it to build sandboxEnv from
an explicit allowlist of non-sensitive variables instead of spreading
process.env. In the block that currently defines envArgs and sandboxEnv
(symbols: formatEnvAssignment, envArgs, sandboxEnv), replace the copy-and-delete
approach with a whitelist array of safe env names and populate sandboxEnv only
from those names (ensuring formatEnvAssignment still sets CHAT_UI_URL), and
remove reliance on setupInference() hydrating secrets into process.env when
constructing sandboxEnv.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aa829268-dd6c-401d-87b5-6321a6d7a4e4

📥 Commits

Reviewing files that changed from the base of the PR and between 6aa13c8 and d898544.

📒 Files selected for processing (1)
  • bin/lib/onboard.js

Comment on lines +480 to +483
function verifyProviderExists(name) {
const output = runCaptureOpenshell(["provider", "get", name], { ignoreError: true });
return Boolean(output && !output.includes("not found"));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

This verification can pass even when the sandbox binding is broken.

verifyProviderExists() only does a global provider get text 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 --provider attachment 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
Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 480 - 483, The current verifyProviderExists
function only checks output text for "not found" which can hide command failures
or an unbound sandbox; change verifyProviderExists (and the duplicate call site
around the other provider-check code) to rely on the command exit status from
runCaptureOpenshell (or its underlying spawn API) instead of string matching,
and additionally validate the provider is attached to the target sandbox by
running a sandbox-scoped query (e.g., list/inspect the sandbox's providers) and
confirming the provider name appears in that result; update logic in
verifyProviderExists to return false on non-zero exit or when the
sandbox-attached provider list does not include the given name.

- Use exit code instead of string matching in verifyProviderExists()
- Use hydrateCredentialEnv() for Telegram so host-side bridge can
  find the token from stored credentials
- Replace individual delete statements with a blocklist that covers
  all credential env vars (NVIDIA, OpenAI, Anthropic, Gemini,
  compatible endpoints, Discord, Slack, Telegram)
@ericksoa ericksoa marked this pull request as draft March 30, 2026 03:07
@ericksoa
Copy link
Copy Markdown
Contributor Author

⚠️ Draft — WebSocket blocker identified

The provider/placeholder flow works for REST API calls (Authorization header rewriting at the proxy layer). However, Discord and Slack bots require WebSocket connections for receiving events:

  • Discord: gateway.discord.gg — bot sends IDENTIFY with token in JSON body, not a header. Endpoint uses access: full (CONNECT tunnel, no L7 inspection).
  • Slack: wss-primary.slack.com — Socket Mode token in WebSocket handshake/payload, not a header. Also access: full.

The proxy can only rewrite HTTP header values. WebSocket payloads and request bodies are passed through verbatim. This means the placeholder string would be sent as the auth token and rejected by both services.

Needs E2E validation with real bot tokens before landing. Possible paths forward:

  1. Keep raw env var injection alongside providers (dual path — providers for storage, env vars for runtime)
  2. Request proxy body rewriting or WebSocket credential injection support
  3. Scope this to Telegram provider only (no behavioral change for Discord/Slack yet)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
bin/lib/onboard.js (1)

480-483: ⚠️ Potential issue | 🟠 Major

This still gives false confidence about provider attachment.

verifyProviderExists() only runs openshell provider get <name>, so it proves the provider exists globally, not that the --provider flag actually attached it to this sandbox. The comment on Line 1920 says the opposite. Because this goes through runOpenshell(), the raw provider get output/errors will also hit onboarding logs before your custom warning. Please make this probe silent and inspect sandbox-attached providers instead; if the CLI cannot do that yet, at least rename this to an existence check so it does not imply attachment validation.

Also applies to: 1920-1927

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/onboard.test.js`:
- Around line 1032-1144: The test must also assert that sensitive env vars are
not forwarded in the sandbox env: after locating payload.commands and the
createCommand entry (as you already do), add assertions that createCommand.env
is either null/undefined or does not contain the keys/values for
DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN, TELEGRAM_BOT_TOKEN (and the new API-key
blocklist key if applicable); reference the existing payload.commands array and
the createCommand variable and assert createCommand.env does not include the
secret names or the literal token values used earlier in the harness.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 37c32b17-312a-4a82-85ed-8918760e22b7

📥 Commits

Reviewing files that changed from the base of the PR and between d898544 and 9d4d5e1.

📒 Files selected for processing (2)
  • bin/lib/onboard.js
  • test/onboard.test.js

Comment on lines +1032 to +1144
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/);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 createCommand.command. If sandboxEnv starts forwarding DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN, TELEGRAM_BOT_TOKEN, or the new API-key blocklist again, this test still passes. Please seed those vars in the harness and assert they are absent from createCommand.env.

🧪 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
Verify each finding against the current code and only fix it if needed.

In `@test/onboard.test.js` around lines 1032 - 1144, The test must also assert
that sensitive env vars are not forwarded in the sandbox env: after locating
payload.commands and the createCommand entry (as you already do), add assertions
that createCommand.env is either null/undefined or does not contain the
keys/values for DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN, TELEGRAM_BOT_TOKEN (and the
new API-key blocklist key if applicable); reference the existing
payload.commands array and the createCommand variable and assert
createCommand.env does not include the secret names or the literal token values
used earlier in the harness.

@ericksoa
Copy link
Copy Markdown
Contributor Author

Update: WebSocket blocker was a false alarm

After tracing the actual SDK code paths:

  • Discord: OpenClaw uses @buape/carbon (REST-only). All API calls use Authorization: Bot {token} header. No WebSocket gateway connection. Provider flow works.
  • Slack: @slack/bolt Socket Mode calls apps.connections.open via REST with Authorization: Bearer {appToken} header to get a temporary, pre-authenticated WebSocket URL. The WebSocket connection itself carries no token. Provider flow works.
  • Telegram: grammy uses getUpdates long polling with token in URL path. Still needs host-side bridge.

All three messaging SDKs authenticate via HTTP Authorization headers that the proxy can rewrite. Keeping draft status until we validate with real tokens in E2E.

The test pattern-matched on the old `{ ...process.env }` spread.
Update to verify the blocklist approach that strips all credential
env vars from sandboxEnv.
@WuKongAI-CMU
Copy link
Copy Markdown
Contributor

Reviewed the diff — this looks solid. A few observations from the OpenClaw side:

  1. Telegram URL-path auth caveat is well-handled — the comment about /bot{TOKEN}/ needing future proxy work is correct. The L7 proxy's header rewriting can't cover URL-path credentials today, so the host-side bridge approach is the right interim solution.

  2. blockedSandboxEnvNames is a great pattern — switching from delete of individual keys to a blocklist Set is more robust and easier to audit. Consider whether COMPATIBLE_ANTHROPIC_API_KEY and similar future credential env vars should be auto-discovered from the provider list rather than hardcoded, to prevent drift.

  3. Provider verification post-createverifyProviderExists only checks the provider exists, not that it's actually attached to the sandbox. If --provider silently fails during sandbox create, the provider might exist in the gateway but not be wired to the sandbox. Might be worth a openshell sandbox get <name> --json check to verify the provider attachment.

Happy to help test this against real Discord/Slack/Telegram bridges if useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Getting Started Use this label to identify setup, installation, or onboarding issues. security Something isn't secure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: unify credential passing pattern across all integrations

2 participants