Skip to content

Commit e2df8bf

Browse files
committed
Warn instead of silently storing plaintext API keys; fix runtime-manager polling storm; add ManagedPythonWorker timeouts
- secrets-store: expose isEncryptionAvailable() and log a warning when falling back to unencrypted storage (no OS credential store available). Settings now shows a prominent plaintext-storage warning in that case instead of the normal "encrypted at rest" note, and the README/docs no longer claim unconditional encryption. - RuntimeManager: stop re-running full Python environment inspection (which can spawn nvidia-smi/rocminfo/wsl.exe and a Python interpreter per family) on the same 2.5s interval as lightweight process-status polling. Environment status now refreshes once on page entry and on manual refresh instead. - ManagedPythonWorker: add a per-request timeout so a hung-but-alive worker can't leave a request pending forever, an output buffer cap to bound memory from a runaway/non-responsive worker, and safe handling of malformed response lines instead of an unguarded JSON.parse.
1 parent cc493c1 commit e2df8bf

10 files changed

Lines changed: 123 additions & 15 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ The `app` suite covers the store layer (atomic writes, corrupted-file recovery),
209209

210210
- **Process isolation**: `contextIsolation: true`, `nodeIntegration: false` — the renderer only ever talks to the main process through an explicit, typed preload bridge.
211211
- **Content Security Policy** restricting plugins, frames, and form submissions; external links open in your default browser instead of an unmanaged Electron window.
212-
- **API keys** are encrypted at rest via the OS credential store (`safeStorage`) and never leave the device.
212+
- **API keys** are encrypted at rest via the OS credential store (`safeStorage`) and never leave the device. On the rare system with no OS credential store available (e.g. some keyring-less Linux setups), keys fall back to being stored in plain text on disk rather than being silently dropped — Settings shows a prominent warning in that case rather than the normal "encrypted" note.
213213
- **Agent mode** tool calls are workspace-sandboxed (path-traversal rejected) and require explicit per-call approval — see [Agent mode](#agent-mode) above and [docs/AGENT_MODE.md](docs/AGENT_MODE.md) for the full detail.
214214
- No telemetry, no analytics, no data sent anywhere except directly to whichever provider (Ollama, OpenAI, Anthropic) you've configured.
215215

app/src/main.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,7 @@ function registerIpcHandlers(): void {
738738
ipcMain.handle("secrets:set", (_event: IpcMainInvokeEvent, { key, value }: { key: string; value: string }) =>
739739
secretsStore.setSecret(requireString(key, "secret key"), value ?? "")
740740
);
741+
ipcMain.handle("secrets:isEncryptionAvailable", () => secretsStore.isEncryptionAvailable());
741742

742743
ipcMain.handle("accounts:status", (_event: IpcMainInvokeEvent, provider: accounts.AccountProvider) =>
743744
accounts.getLinkedAccount(provider)

app/src/preload.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ contextBridge.exposeInMainWorld("api", {
177177
secrets: {
178178
has: (key: string) => ipcRenderer.invoke("secrets:has", key),
179179
set: (key: string, value: string) => ipcRenderer.invoke("secrets:set", { key, value }),
180+
isEncryptionAvailable: () => ipcRenderer.invoke("secrets:isEncryptionAvailable") as Promise<boolean>,
180181
},
181182

182183
accounts: {

app/src/python-runtime-manager.ts

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,20 @@ export async function getPythonEnvironmentStatuses(): Promise<PythonEnvironmentS
137137
}
138138

139139
interface WorkerResponse { protocol: number; id: string; ok: boolean; result?: unknown; error?: { code: string; message: string } }
140+
// A request has no inherent deadline otherwise: if the worker process stays
141+
// alive but stops responding (stuck in a long computation, deadlocked, or
142+
// just never writes a line back), the promise in `pending` would never
143+
// settle and would leak forever.
144+
const REQUEST_TIMEOUT_MS = 20_000;
145+
// Bounds how much unterminated stdout `consume` will buffer waiting for a
146+
// newline. A worker that never emits one (buggy print, corrupted output)
147+
// would otherwise grow this string without limit for as long as the process
148+
// lives — this caps the damage and fails loudly instead of leaking memory.
149+
const MAX_BUFFER_BYTES = 8 * 1024 * 1024;
140150
export class ManagedPythonWorker {
141-
private child: ChildProcess | null = null; private pending = new Map<string, { resolve(value: unknown): void; reject(error: Error): void }>(); private buffer = "";
151+
private child: ChildProcess | null = null;
152+
private pending = new Map<string, { resolve(value: unknown): void; reject(error: Error): void; timer: NodeJS.Timeout }>();
153+
private buffer = "";
142154
constructor(private readonly family: PythonRuntimeFamily) {}
143155
start(): void {
144156
if (this.child) return;
@@ -153,10 +165,54 @@ export class ManagedPythonWorker {
153165
// event with no listener, which Node treats as an uncaught exception
154166
// and crashes the whole main process instead of just failing this
155167
// one request.
156-
this.child.once("error", (error: Error) => { for (const request of this.pending.values()) request.reject(error); this.pending.clear(); this.child = null; });
157-
this.child.once("exit", () => { for (const request of this.pending.values()) request.reject(new Error("Python worker exited")); this.pending.clear(); this.child = null; });
168+
this.child.once("error", (error: Error) => { this.failAllPending(error); this.child = null; });
169+
this.child.once("exit", () => { this.failAllPending(new Error("Python worker exited")); this.child = null; });
170+
}
171+
private failAllPending(error: Error): void {
172+
for (const request of this.pending.values()) { clearTimeout(request.timer); request.reject(error); }
173+
this.pending.clear();
174+
this.buffer = "";
175+
}
176+
private consume(chunk: string): void {
177+
this.buffer += chunk;
178+
if (this.buffer.length > MAX_BUFFER_BYTES) {
179+
// Something is badly wrong (runaway output, no newlines) — treat
180+
// it as fatal rather than let the process keep growing memory.
181+
const error = new Error(`Python worker "${this.family}" exceeded the ${MAX_BUFFER_BYTES}-byte output buffer without a complete line`);
182+
const child = this.child;
183+
this.failAllPending(error);
184+
if (child?.pid) killProcessTree(child.pid, "SIGKILL");
185+
this.child = null;
186+
return;
187+
}
188+
const lines = this.buffer.split(/\r?\n/); this.buffer = lines.pop() ?? "";
189+
for (const line of lines) {
190+
if (!line) continue;
191+
let response: WorkerResponse;
192+
try {
193+
response = JSON.parse(line) as WorkerResponse;
194+
} catch {
195+
console.error(`[python-worker:${this.family}] Ignoring malformed response line: ${line.slice(0, 200)}`);
196+
continue;
197+
}
198+
const pending = this.pending.get(response.id);
199+
if (!pending) continue;
200+
clearTimeout(pending.timer);
201+
this.pending.delete(response.id);
202+
response.ok ? pending.resolve(response.result) : pending.reject(new Error(response.error?.message ?? "Worker request failed"));
203+
}
204+
}
205+
request(method: "health" | "metrics" | "recommend", params: Record<string, unknown> = {}): Promise<unknown> {
206+
this.start();
207+
const id = crypto.randomUUID();
208+
return new Promise((resolve, reject) => {
209+
const timer = setTimeout(() => {
210+
this.pending.delete(id);
211+
reject(new Error(`Python worker "${this.family}" request "${method}" timed out after ${REQUEST_TIMEOUT_MS}ms`));
212+
}, REQUEST_TIMEOUT_MS);
213+
this.pending.set(id, { resolve, reject, timer });
214+
this.child?.stdin?.write(JSON.stringify({ protocol: PYTHON_WORKER_PROTOCOL_VERSION, id, method, params }) + "\n");
215+
});
158216
}
159-
private consume(chunk: string): void { this.buffer += chunk; const lines = this.buffer.split(/\r?\n/); this.buffer = lines.pop() ?? ""; for (const line of lines) { if (!line) continue; const response = JSON.parse(line) as WorkerResponse; const pending = this.pending.get(response.id); if (!pending) continue; this.pending.delete(response.id); response.ok ? pending.resolve(response.result) : pending.reject(new Error(response.error?.message ?? "Worker request failed")); } }
160-
request(method: "health" | "metrics" | "recommend", params: Record<string, unknown> = {}): Promise<unknown> { this.start(); const id = crypto.randomUUID(); return new Promise((resolve, reject) => { this.pending.set(id, { resolve, reject }); this.child?.stdin?.write(JSON.stringify({ protocol: PYTHON_WORKER_PROTOCOL_VERSION, id, method, params }) + "\n"); }); }
161217
async shutdown(): Promise<void> { const child = this.child; if (!child) return; const pid = child.pid; try { const id = crypto.randomUUID(); child.stdin?.write(JSON.stringify({ protocol: PYTHON_WORKER_PROTOCOL_VERSION, id, method: "shutdown", params: {} }) + "\n"); } catch { /* pipe closed */ } await new Promise((resolve) => setTimeout(resolve, 500)); if (pid && this.child) killProcessTree(pid, "SIGTERM"); this.child = null; }
162218
}

app/src/secrets-store.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as path from "node:path";
22
import { app, safeStorage } from "electron";
33
import { readJson, writeJson } from "./json-store";
4+
import { logger } from "./logger";
45

56
function filePath(): string {
67
return path.join(app.getPath("userData"), "secrets.json");
@@ -14,15 +15,27 @@ function writeAll(data: Record<string, string>): void {
1415
writeJson(filePath(), data);
1516
}
1617

18+
// Exposed so callers (and Settings UI) can warn the user before a key ends
19+
// up unencrypted, rather than only finding out after the fact.
20+
export function isEncryptionAvailable(): boolean {
21+
return safeStorage.isEncryptionAvailable();
22+
}
23+
24+
// Environments without an OS credential store (e.g. some Linux setups with no
25+
// keyring) can't use safeStorage at all. We still allow storing the key there
26+
// rather than silently dropping it or hard-blocking the feature, but this is
27+
// no longer a silent fallback: it's logged at warn level (surfaced in Settings
28+
// -> Diagnostics -> Copy diagnostic info) and the Settings UI checks
29+
// isEncryptionAvailable() up front to show a plaintext-storage warning next to
30+
// every API key field instead of the normal "encrypted at rest" note.
1731
export function setSecret(key: string, value: string): void {
1832
const all = readAll();
1933
if (!value) {
2034
delete all[key];
2135
} else if (safeStorage.isEncryptionAvailable()) {
2236
all[key] = safeStorage.encryptString(value).toString("base64");
2337
} else {
24-
// Fallback for environments without an OS credential store (e.g. some
25-
// Linux setups with no keyring). Better to work than to silently drop the key.
38+
logger.warn(`No OS credential store available; storing secret "${key}" unencrypted in secrets.json`);
2639
all[key] = value;
2740
}
2841
writeAll(all);

docs/ARCHITECTURE.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,12 @@ plain JSON files under Electron's `userData` directory, going through the shared
102102

103103
Secrets (provider API keys) go through `secrets-store.ts` instead, which encrypts values at rest
104104
via Electron's `safeStorage` (backed by the OS credential store — Keychain, DPAPI, or
105-
libsecret/kwallet) rather than writing them out as plain JSON.
105+
libsecret/kwallet) rather than writing them out as plain JSON. On a system with no OS credential
106+
store available (`safeStorage.isEncryptionAvailable()` returns `false` — some keyring-less Linux
107+
setups), the store falls back to writing the value unencrypted rather than silently dropping the
108+
key; this fallback is logged at warn level, and `secrets:isEncryptionAvailable` lets the renderer
109+
check the same condition to show a plaintext-storage warning in Settings instead of the normal
110+
"encrypted at rest" note.
106111

107112
## Native addon (`lib/`)
108113

frontend/src/lib/translations.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ export interface Dictionary {
5050
yourSystem: string;
5151
cloudProviders: string;
5252
keysEncryptedNote: string;
53+
keysNotEncryptedWarning: string;
5354
ollamaModelsSection: string;
5455
otherInstalledModels: string;
5556
chatDefaults: string;
@@ -450,6 +451,7 @@ export const en: Dictionary = {
450451
yourSystem: "Your system",
451452
cloudProviders: "Cloud providers",
452453
keysEncryptedNote: "Keys are encrypted at rest using your OS credential store and never leave this device.",
454+
keysNotEncryptedWarning: "No OS credential store was found on this system, so keys are being saved in plain text on disk instead of encrypted. They still never leave this device, but anyone with file access to your user profile can read them.",
453455
ollamaModelsSection: "Ollama models",
454456
otherInstalledModels: "Other installed models",
455457
chatDefaults: "Chat defaults",
@@ -860,6 +862,8 @@ export const tr: Dictionary = {
860862
cloudProviders: "Bulut sağlayıcılar",
861863
keysEncryptedNote:
862864
"Anahtarlar, işletim sistemi kimlik bilgisi deposu kullanılarak şifrelenir ve bu cihazdan çıkmaz.",
865+
keysNotEncryptedWarning:
866+
"Bu sistemde bir işletim sistemi kimlik bilgisi deposu bulunamadığı için anahtarlar şifrelenmeden düz metin olarak diske kaydediliyor. Yine de bu cihazdan çıkmazlar, ancak kullanıcı profilinize dosya erişimi olan herkes onları okuyabilir.",
863867
ollamaModelsSection: "Ollama modelleri",
864868
otherInstalledModels: "Diğer yüklü modeller",
865869
chatDefaults: "Sohbet varsayılanları",

0 commit comments

Comments
 (0)