fix(electrobun): complete all 16 Codex #3 findings
CRITICAL:
- Message persistence race: snapshot batchEnd before async save
- Double-start guard: startingProjects Set prevents concurrent launches
- Symlink path traversal: fs.realpathSync() in path-guard.ts
- Relay false success: connect() returns { ok, machineId, error }
HIGH:
- Session restore skips if active session exists
- Remote remove: new RPC, cleans backend map
- Task board poll token: stale responses discarded after drag-drop
- Health concurrent tools: toolsInFlight counter (was boolean)
- bttask transactions: delete wraps comments+task, addComment validates
- PTY buffer cleared on reconnect
- PTY large paste: chunked String.fromCharCode (8KB chunks)
- Sidecar max line: 10MB limit prevents unbounded memory
- btmsg authorization: group validation, channel membership checks
MEDIUM:
- Session retention: max 5 per project, purgeSession/untrackProject
- Relay IPv6: URL parser replaces string split
- PTY schema: fixed misleading base64 comment
This commit is contained in:
Hibryda
parent
c145e37316
commit
0f75cb8e32
12 changed files with 190 additions and 42 deletions
|
|
@ -200,6 +200,9 @@ export class BtmsgDb {
|
|||
|
||||
// ── Direct messages ──────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Fix #13 (Codex audit): Validate sender and recipient are in the same group.
|
||||
*/
|
||||
sendMessage(fromAgent: string, toAgent: string, content: string): string {
|
||||
// Get sender's group_id
|
||||
const sender = this.db.query<{ group_id: string }, [string]>(
|
||||
|
|
@ -207,6 +210,15 @@ export class BtmsgDb {
|
|||
).get(fromAgent);
|
||||
if (!sender) throw new Error(`Sender agent '${fromAgent}' not found`);
|
||||
|
||||
// Validate recipient exists and is in the same group
|
||||
const recipient = this.db.query<{ group_id: string }, [string]>(
|
||||
"SELECT group_id FROM agents WHERE id = ?"
|
||||
).get(toAgent);
|
||||
if (!recipient) throw new Error(`Recipient agent '${toAgent}' not found`);
|
||||
if (sender.group_id !== recipient.group_id) {
|
||||
throw new Error(`Cross-group messaging denied: '${fromAgent}' (${sender.group_id}) -> '${toAgent}' (${recipient.group_id})`);
|
||||
}
|
||||
|
||||
const id = randomUUID();
|
||||
this.db.query(
|
||||
`INSERT INTO messages (id, from_agent, to_agent, content, group_id, sender_group_id)
|
||||
|
|
@ -249,12 +261,23 @@ export class BtmsgDb {
|
|||
|
||||
// ── Channels ─────────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Fix #13 (Codex audit): Auto-add creator to channel membership on create.
|
||||
*/
|
||||
createChannel(name: string, groupId: string, createdBy: string): string {
|
||||
const id = randomUUID();
|
||||
this.db.query(
|
||||
`INSERT INTO channels (id, name, group_id, created_by)
|
||||
VALUES (?1, ?2, ?3, ?4)`
|
||||
).run(id, name, groupId, createdBy);
|
||||
const tx = this.db.transaction(() => {
|
||||
this.db.query(
|
||||
`INSERT INTO channels (id, name, group_id, created_by)
|
||||
VALUES (?1, ?2, ?3, ?4)`
|
||||
).run(id, name, groupId, createdBy);
|
||||
// Auto-add creator as channel member
|
||||
this.db.query(
|
||||
`INSERT OR IGNORE INTO channel_members (channel_id, agent_id)
|
||||
VALUES (?1, ?2)`
|
||||
).run(id, createdBy);
|
||||
});
|
||||
tx();
|
||||
return id;
|
||||
}
|
||||
|
||||
|
|
@ -292,7 +315,17 @@ export class BtmsgDb {
|
|||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Fix #13 (Codex audit): Validate sender is a member of the channel.
|
||||
*/
|
||||
sendChannelMessage(channelId: string, fromAgent: string, content: string): string {
|
||||
const member = this.db.query<{ agent_id: string }, [string, string]>(
|
||||
"SELECT agent_id FROM channel_members WHERE channel_id = ? AND agent_id = ?"
|
||||
).get(channelId, fromAgent);
|
||||
if (!member) {
|
||||
throw new Error(`Agent '${fromAgent}' is not a member of channel '${channelId}'`);
|
||||
}
|
||||
|
||||
const id = randomUUID();
|
||||
this.db.query(
|
||||
`INSERT INTO channel_messages (id, channel_id, from_agent, content)
|
||||
|
|
|
|||
|
|
@ -137,14 +137,26 @@ export class BttaskDb {
|
|||
return expectedVersion + 1;
|
||||
}
|
||||
|
||||
/** Fix #9 (Codex audit): Wrap delete in transaction for atomicity. */
|
||||
deleteTask(taskId: string): void {
|
||||
this.db.query("DELETE FROM task_comments WHERE task_id = ?").run(taskId);
|
||||
this.db.query("DELETE FROM tasks WHERE id = ?").run(taskId);
|
||||
const tx = this.db.transaction(() => {
|
||||
this.db.query("DELETE FROM task_comments WHERE task_id = ?").run(taskId);
|
||||
this.db.query("DELETE FROM tasks WHERE id = ?").run(taskId);
|
||||
});
|
||||
tx();
|
||||
}
|
||||
|
||||
// ── Comments ─────────────────────────────────────────────────────────────
|
||||
|
||||
/** Fix #9 (Codex audit): Validate task exists before adding comment. */
|
||||
addComment(taskId: string, agentId: string, content: string): string {
|
||||
const task = this.db.query<{ id: string }, [string]>(
|
||||
"SELECT id FROM tasks WHERE id = ?"
|
||||
).get(taskId);
|
||||
if (!task) {
|
||||
throw new Error(`Task '${taskId}' not found`);
|
||||
}
|
||||
|
||||
const id = randomUUID();
|
||||
this.db.query(
|
||||
`INSERT INTO task_comments (id, task_id, agent_id, content)
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@
|
|||
*/
|
||||
|
||||
import path from "path";
|
||||
import fs from "fs";
|
||||
import { settingsDb } from "../settings-db.ts";
|
||||
import { homedir } from "os";
|
||||
import { join } from "path";
|
||||
|
|
@ -29,14 +30,36 @@ function getAllowedRoots(): string[] {
|
|||
|
||||
/**
|
||||
* Validate that a file path is within an allowed boundary.
|
||||
* Returns the resolved path if valid, or null if outside boundaries.
|
||||
* Fix #3 (Codex audit): Uses realpathSync to resolve symlinks, preventing
|
||||
* symlink-based traversal attacks (CWE-59).
|
||||
* Returns the resolved real path if valid, or null if outside boundaries.
|
||||
*/
|
||||
export function validatePath(filePath: string): string | null {
|
||||
const resolved = path.resolve(filePath);
|
||||
let resolved: string;
|
||||
try {
|
||||
// Resolve symlinks to their actual target to prevent symlink traversal
|
||||
resolved = fs.realpathSync(path.resolve(filePath));
|
||||
} catch {
|
||||
// If the file doesn't exist yet, resolve without symlink resolution
|
||||
// but only allow if the parent directory resolves within boundaries
|
||||
const parent = path.dirname(path.resolve(filePath));
|
||||
try {
|
||||
const realParent = fs.realpathSync(parent);
|
||||
resolved = path.join(realParent, path.basename(filePath));
|
||||
} catch {
|
||||
return null; // Parent doesn't exist either — reject
|
||||
}
|
||||
}
|
||||
|
||||
const roots = getAllowedRoots();
|
||||
|
||||
for (const root of roots) {
|
||||
const resolvedRoot = path.resolve(root);
|
||||
let resolvedRoot: string;
|
||||
try {
|
||||
resolvedRoot = fs.realpathSync(path.resolve(root));
|
||||
} catch {
|
||||
resolvedRoot = path.resolve(root);
|
||||
}
|
||||
if (resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep)) {
|
||||
return resolved;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,10 +6,11 @@ import type { RelayClient } from "../relay-client.ts";
|
|||
|
||||
export function createRemoteHandlers(relayClient: RelayClient) {
|
||||
return {
|
||||
// Fix #4 (Codex audit): relay-client.connect() now returns { ok, machineId, error }
|
||||
"remote.connect": async ({ url, token, label }: { url: string; token: string; label?: string }) => {
|
||||
try {
|
||||
const machineId = await relayClient.connect(url, token, label);
|
||||
return { ok: true, machineId };
|
||||
const result = await relayClient.connect(url, token, label);
|
||||
return result;
|
||||
} catch (err) {
|
||||
const error = err instanceof Error ? err.message : String(err);
|
||||
console.error("[remote.connect]", err);
|
||||
|
|
@ -28,6 +29,18 @@ export function createRemoteHandlers(relayClient: RelayClient) {
|
|||
}
|
||||
},
|
||||
|
||||
// Fix #6 (Codex audit): Add remote.remove RPC that disconnects AND deletes
|
||||
"remote.remove": ({ machineId }: { machineId: string }) => {
|
||||
try {
|
||||
relayClient.removeMachine(machineId);
|
||||
return { ok: true };
|
||||
} catch (err) {
|
||||
const error = err instanceof Error ? err.message : String(err);
|
||||
console.error("[remote.remove]", err);
|
||||
return { ok: false, error };
|
||||
}
|
||||
},
|
||||
|
||||
"remote.list": () => {
|
||||
try {
|
||||
return { machines: relayClient.listMachines() };
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ export interface SessionInfo {
|
|||
export type DaemonEvent =
|
||||
| { type: "auth_result"; ok: boolean }
|
||||
| { type: "session_created"; session_id: string; pid: number }
|
||||
/** data is base64-encoded bytes from the PTY. */
|
||||
/** data is base64-encoded raw bytes from the PTY daemon. Decoded to Uint8Array by the consumer. */
|
||||
| { type: "session_output"; session_id: string; data: string }
|
||||
| { type: "session_closed"; session_id: string; exit_code: number | null }
|
||||
| { type: "session_list"; sessions: SessionInfo[] }
|
||||
|
|
@ -60,6 +60,9 @@ export class PtyClient extends EventEmitter {
|
|||
|
||||
/** Connect to daemon and authenticate. Fix #10: 5-second timeout. */
|
||||
async connect(): Promise<void> {
|
||||
// Fix #10 (Codex audit): Clear stale buffer from any previous connection
|
||||
this.buffer = "";
|
||||
|
||||
return new Promise((resolve, reject) => {
|
||||
let token: string;
|
||||
try {
|
||||
|
|
@ -157,14 +160,24 @@ export class PtyClient extends EventEmitter {
|
|||
});
|
||||
}
|
||||
|
||||
/** Write input to a session's PTY. data is encoded as base64 for transport. */
|
||||
/**
|
||||
* Write input to a session's PTY. data is encoded as base64 for transport.
|
||||
* Fix #11 (Codex audit): Uses chunked approach instead of String.fromCharCode(...spread)
|
||||
* which can throw RangeError on large pastes (>~65K bytes).
|
||||
*/
|
||||
writeInput(sessionId: string, data: string | Uint8Array): void {
|
||||
const bytes =
|
||||
typeof data === "string"
|
||||
? new TextEncoder().encode(data)
|
||||
: data;
|
||||
// Daemon expects base64 string per protocol.rs WriteInput { data: String }
|
||||
const b64 = btoa(String.fromCharCode(...bytes));
|
||||
const CHUNK_SIZE = 8192;
|
||||
let binary = "";
|
||||
for (let i = 0; i < bytes.length; i += CHUNK_SIZE) {
|
||||
const chunk = bytes.subarray(i, Math.min(i + CHUNK_SIZE, bytes.length));
|
||||
binary += String.fromCharCode(...chunk);
|
||||
}
|
||||
const b64 = btoa(binary);
|
||||
this.send({ type: "write_input", session_id: sessionId, data: b64 });
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -63,8 +63,12 @@ export class RelayClient {
|
|||
this.statusListeners.push(cb);
|
||||
}
|
||||
|
||||
/** Connect to an agor-relay instance. Returns a machine ID. */
|
||||
async connect(url: string, token: string, label?: string): Promise<string> {
|
||||
/**
|
||||
* Connect to an agor-relay instance.
|
||||
* Fix #4 (Codex audit): Returns { ok, machineId, error } instead of always
|
||||
* returning machineId even on failure.
|
||||
*/
|
||||
async connect(url: string, token: string, label?: string): Promise<{ ok: boolean; machineId?: string; error?: string }> {
|
||||
const machineId = randomUUID();
|
||||
const machine: MachineConnection = {
|
||||
machineId,
|
||||
|
|
@ -84,14 +88,14 @@ export class RelayClient {
|
|||
|
||||
try {
|
||||
await this.openWebSocket(machine);
|
||||
return { ok: true, machineId };
|
||||
} catch (err) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
machine.status = "error";
|
||||
this.emitStatus(machineId, "error", msg);
|
||||
this.scheduleReconnect(machine);
|
||||
return { ok: false, machineId, error: msg };
|
||||
}
|
||||
|
||||
return machineId;
|
||||
}
|
||||
|
||||
/** Disconnect from a relay and stop reconnection attempts. */
|
||||
|
|
@ -299,16 +303,24 @@ export class RelayClient {
|
|||
machine.reconnectTimer = setTimeout(attempt, delay);
|
||||
}
|
||||
|
||||
/** TCP-only probe to check if the relay host is reachable. */
|
||||
private tcpProbe(url: string): Promise<boolean> {
|
||||
/**
|
||||
* TCP-only probe to check if the relay host is reachable.
|
||||
* Fix #15 (Codex audit): Uses URL() to correctly parse IPv6, ports, etc.
|
||||
*/
|
||||
private tcpProbe(wsUrl: string): Promise<boolean> {
|
||||
return new Promise((resolve) => {
|
||||
const host = this.extractHost(url);
|
||||
if (!host) { resolve(false); return; }
|
||||
|
||||
const [hostname, portStr] = host.includes(":")
|
||||
? [host.split(":")[0], host.split(":")[1]]
|
||||
: [host, "9750"];
|
||||
const port = parseInt(portStr, 10);
|
||||
let hostname: string;
|
||||
let port: number;
|
||||
try {
|
||||
// Convert ws/wss to http/https so URL() can parse it
|
||||
const httpUrl = wsUrl.replace(/^ws(s)?:\/\//, "http$1://");
|
||||
const parsed = new URL(httpUrl);
|
||||
hostname = parsed.hostname; // strips IPv6 brackets automatically
|
||||
port = parsed.port ? parseInt(parsed.port, 10) : 9750;
|
||||
} catch {
|
||||
resolve(false);
|
||||
return;
|
||||
}
|
||||
|
||||
const socket = new Socket();
|
||||
const timer = setTimeout(() => { socket.destroy(); resolve(false); }, 5_000);
|
||||
|
|
@ -327,10 +339,6 @@ export class RelayClient {
|
|||
});
|
||||
}
|
||||
|
||||
private extractHost(url: string): string | null {
|
||||
return url.replace("wss://", "").replace("ws://", "").split("/")[0] ?? null;
|
||||
}
|
||||
|
||||
private emitStatus(machineId: string, status: ConnectionStatus, error?: string): void {
|
||||
for (const cb of this.statusListeners) {
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -134,6 +134,8 @@ function findNodeRuntime(): string {
|
|||
// ── Cleanup grace period ─────────────────────────────────────────────────────
|
||||
|
||||
const CLEANUP_GRACE_MS = 60_000; // 60s after done/error before removing session
|
||||
// Fix #12 (Codex audit): Max NDJSON line size — prevent OOM on malformed output
|
||||
const MAX_LINE_SIZE = 10 * 1024 * 1024; // 10 MB
|
||||
|
||||
// ── SidecarManager ───────────────────────────────────────────────────────────
|
||||
|
||||
|
|
@ -369,6 +371,13 @@ export class SidecarManager {
|
|||
for await (const chunk of reader) {
|
||||
buffer += decoder.decode(chunk, { stream: true });
|
||||
|
||||
// Fix #12 (Codex audit): Guard against unbounded buffer growth
|
||||
if (buffer.length > MAX_LINE_SIZE && !buffer.includes("\n")) {
|
||||
console.error(`[sidecar] Buffer exceeded ${MAX_LINE_SIZE} bytes without newline for ${sessionId}, truncating`);
|
||||
buffer = "";
|
||||
continue;
|
||||
}
|
||||
|
||||
let newlineIdx: number;
|
||||
while ((newlineIdx = buffer.indexOf("\n")) !== -1) {
|
||||
const line = buffer.slice(0, newlineIdx).trim();
|
||||
|
|
@ -379,7 +388,7 @@ export class SidecarManager {
|
|||
}
|
||||
}
|
||||
|
||||
// Fix #12: Parse any residual data left in the buffer after stream ends
|
||||
// Parse any residual data left in the buffer after stream ends
|
||||
const residual = buffer.trim();
|
||||
if (residual) {
|
||||
this.handleNdjsonLine(sessionId, session, residual);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue