Skip to content

fix: claude-lane/task_1780281408931_wl61hi99e-20260602145932 #31

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

Closed
nehraa wants to merge 1 commit into from
Closed

fix: claude-lane/task_1780281408931_wl61hi99e-20260602145932 #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 22 additions & 2 deletions src/lib/opencode/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
* OpenCode configuration helpers
*/

import { writeFileSync, mkdtempSync, chmodSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
Comment on lines +5 to +7

@gemini-code-assist gemini-code-assist Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To prevent leaking temporary directories and sensitive API key files on disk indefinitely, we should track the created temporary directories and clean them up when the process exits.

We can import rmSync and register a synchronous process.on('exit') handler to delete these directories.

import { writeFileSync, mkdtempSync, chmodSync, rmSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";

const createdTempDirs: string[] = [];

if (typeof process !== "undefined") {
  process.on("exit", () => {
    for (const dir of createdTempDirs) {
      try {
        rmSync(dir, { recursive: true, force: true });
      } catch {}
    }
  });
}

Comment on lines +5 to +7
import type { OpencodeConfig, OpencodeProvider } from "./types";
import { PROVIDER_CONFIGS } from "./types";

Expand Down Expand Up @@ -49,14 +52,31 @@ export function buildOpencodeConfig(
}

/**
* Convert OpencodeConfig to environment variables for OpenCode CLI
* Convert OpencodeConfig to environment variables for OpenCode CLI.
*
* SECURITY: The API key is NEVER placed directly in the environment.
* Environment variables are readable via `/proc/<pid>/environ` on Linux
* by any local user, which would expose the key to local privilege
* escalation. Instead, the key is written to a temp file with mode
* 0600 (readable/writable only by the owner) and the file path is
* passed as the env var value. The child process is expected to read
* the key from the file.
Comment on lines +55 to +63

@qodo-code-review qodo-code-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Api key still in env 🐞 Bug ⛨ Security

Despite the new claim that the API key is never placed in environment variables,
OpencodeServer.start() still sets OPENCODE_CONFIG_CONTENT to JSON.stringify(config), which includes
config.apiKey and therefore exposes the key in the child process environment.
Agent Prompt 
### Issue description
`OPENCODE_CONFIG_CONTENT` is set to `JSON.stringify(config)` and `config` includes `apiKey`, so the spawned `opencode` process still receives the API key via environment variables, contradicting the new security guarantee.

### Issue Context
The PR moved provider API key env vars to a temp file, but `OPENCODE_CONFIG_CONTENT` continues to serialize and export the entire config object.

### Fix Focus Areas
- src/lib/opencode/server.ts[40-45]
- src/lib/opencode/types.ts[19-28]

### Suggested fix
- Build a sanitized config for `OPENCODE_CONFIG_CONTENT` that omits `apiKey` (or replaces it with a non-secret reference if truly required).
  - Example: `const { apiKey, ...safeConfig } = config; OPENCODE_CONFIG_CONTENT: JSON.stringify(safeConfig)`
- Ensure any other env var or logging path does not include the raw key.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +55 to +63
*/
export function configToEnv(config: OpencodeConfig): Record<string, string> {
const env: Record<string, string> = {};
const providerConfig = PROVIDER_CONFIGS[config.provider];

if (config.apiKey && providerConfig.apiKeyEnvVar) {
env[providerConfig.apiKeyEnvVar] = config.apiKey;
// Write the API key to a unique temp file with restrictive permissions.
// Using mkdtempSync guarantees an unguessable, exclusive directory name,
// avoiding symlink attacks in the shared /tmp directory.
const dir = mkdtempSync(join(tmpdir(), "codeflow-opencode-"));
const keyFile = join(dir, "api_key");
Comment on lines +70 to +74
writeFileSync(keyFile, config.apiKey, { mode: 0o600 });
// Belt-and-suspenders: explicitly chmod in case the umask interfered
// with the mode option (writeFileSync's mode is masked by process.umask).
chmodSync(keyFile, 0o600);
env[providerConfig.apiKeyEnvVar] = keyFile;
Comment on lines +73 to +79

@gemini-code-assist gemini-code-assist Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

There are two important security/robustness improvements here:

  1. Directory Permissions: While the API key file itself is written with 0o600 permissions, the parent directory created by mkdtempSync is subject to the process's umask (often resulting in 0o755 or 0o700). To ensure other local users cannot traverse or list the directory, we should explicitly set the directory permissions to 0o700 using chmodSync(dir, 0o700).
  2. Cleanup Tracking: Push the created directory to createdTempDirs so it can be cleaned up on process exit.

⚠️ Critical Security Warning

While this function goes to great lengths to avoid placing the API key directly in the environment, this protection is completely bypassed in src/lib/opencode/server.ts.

In server.ts (line 44), the entire config object is stringified and placed in the environment:

OPENCODE_CONFIG_CONTENT: JSON.stringify(config),

Since config contains the raw apiKey, the API key is still exposed in the environment variables of the spawned child process. To fix this, you should sanitize the config object in server.ts before stringifying it (e.g., by deleting or replacing the apiKey property).

    const dir = mkdtempSync(join(tmpdir(), "codeflow-opencode-"));
    chmodSync(dir, 0o700);
    createdTempDirs.push(dir);
    const keyFile = join(dir, "api_key");
    writeFileSync(keyFile, config.apiKey, { mode: 0o600 });
    // Belt-and-suspenders: explicitly chmod in case the umask interfered
    // with the mode option (writeFileSync's mode is masked by process.umask).
    chmodSync(keyFile, 0o600);
    env[providerConfig.apiKeyEnvVar] = keyFile;

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restore API key values for OpenCode env vars

When starting OpenCode with a hosted provider, this now sets ANTHROPIC_API_KEY/OPENAI_API_KEY/etc. to a temp-file path instead of the API key itself. OpenCode’s config docs distinguish {env:ANTHROPIC_API_KEY} as substituting the environment variable value from {file:...} as reading file contents, so a provider launched through src/lib/opencode/server.ts will receive /tmp/codeflow-opencode-.../api_key as the credential and authentication will fail rather than reading the file.

Useful? React with 👍 / 👎.

Comment on lines +73 to +79

@qodo-code-review qodo-code-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Temp key file not cleaned 🐞 Bug ☼ Reliability

configToEnv() writes the API key into a newly created temp directory but provides no cleanup path,
so API keys persist on disk and repeated starts/restarts will accumulate secret-bearing files under
the system temp directory.
Agent Prompt 
### Issue description
A temp directory and key file are created for every `configToEnv()` call, but nothing deletes them. This leaves API keys at rest on disk indefinitely and can also create unbounded growth in the temp directory across restarts.

### Issue Context
`configToEnv()` is used by `OpencodeServer.start()` to construct the environment for the spawned `opencode` process. `OpencodeServer.stop()` kills the process but does not remove any temp files.

### Fix Focus Areas
- src/lib/opencode/config.ts[65-90]
- src/lib/opencode/server.ts[29-86]
- src/lib/opencode/server.ts[88-118]

### Suggested fix
- Change `configToEnv()` (or its caller) to return both:
  1) the env object, and
  2) a cleanup function (or list of created paths) that removes the temp directory.
- In `OpencodeServer.start()`, register cleanup on:
  - normal stop (`stop()`), and
  - child process `exit`/`error` handlers.
- Use best-effort removal: `rmSync(dir, { recursive: true, force: true })` (or async equivalent) and ignore errors.
- (Optional hardening) explicitly ensure the temp directory permissions are owner-only before writing the key file.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}

if (config.baseUrl) {
Expand Down