diff --git a/src/cli/init.ts b/src/cli/init.ts index 9e9dfb16..b7c2b451 100644 --- a/src/cli/init.ts +++ b/src/cli/init.ts @@ -727,7 +727,12 @@ export async function runInit(options: InitOptions = {}): Promise { autoHideSensitiveFiles, }); - await writeFile(outputPath, JSON.stringify(config, null, 2) + '\n', 'utf-8'); + // mode 0o600: config.json can hold Telegram/Discord bot tokens and inline + // MCP env secrets — restrict it to the owner. + await writeFile(outputPath, JSON.stringify(config, null, 2) + '\n', { + encoding: 'utf-8', + mode: 0o600, + }); write(` Config written to ${outputPath}\n`); // Step 12: Health check diff --git a/src/core/agent-runner.ts b/src/core/agent-runner.ts index 9aa0ea0a..9ca31484 100644 --- a/src/core/agent-runner.ts +++ b/src/core/agent-runner.ts @@ -511,10 +511,14 @@ export async function manifestToSpawnOptions( if (resolved) { allowedTools = resolved; } else { + // Fail closed: an unknown/misspelled profile must NOT result in an + // unrestricted worker (no --allowedTools flag = full tool access). Fall + // back to the most restrictive read-only profile instead. logger.warn( { profile: manifest.profile }, - 'Unknown profile name — no tools restriction applied', + 'Unknown profile name — falling back to read-only tool restriction', ); + allowedTools = [...TOOLS_READ_ONLY]; } } @@ -559,7 +563,12 @@ export async function manifestToSpawnOptions( }; } - await writeFile(tempFilePath, JSON.stringify({ mcpServers: mcpServersConfig }, null, 2), 'utf-8'); + // mode 0o600: this file contains raw MCP server tokens (Slack/Gmail/Canva) and + // lives in the world-readable shared tmp dir — restrict it to the owner only. + await writeFile(tempFilePath, JSON.stringify({ mcpServers: mcpServersConfig }, null, 2), { + encoding: 'utf-8', + mode: 0o600, + }); logger.debug( { tempFilePath, serverCount: manifest.mcpServers.length }, @@ -653,7 +662,14 @@ export function sanitizePrompt( ): string { // eslint-disable-next-line no-control-regex const cleaned = prompt.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, ''); - return truncatePrompt(cleaned, maxLength, context); + const truncated = truncatePrompt(cleaned, maxLength, context); + // Argument-injection guard: the prompt is handed to the CLI as a single + // positional argv element. If it starts with '-', the CLI's arg parser would + // treat it as a flag — e.g. a message of "--dangerously-skip-permissions" or + // "--mcp-config" could disable tool restrictions or repoint MCP servers. A + // single leading space neutralizes this for any parser and is cosmetically + // irrelevant to the model (which trims leading whitespace anyway). + return truncated.startsWith('-') ? ` ${truncated}` : truncated; } /** Options accepted by AgentRunner.spawn() */ diff --git a/src/core/auth.ts b/src/core/auth.ts index c9d2e652..0d6c80f8 100644 --- a/src/core/auth.ts +++ b/src/core/auth.ts @@ -61,6 +61,7 @@ const PAIRING_RATE_LIMIT_WINDOW_MS = 60 * 60 * 1000; // 1 hour export class AuthService { private whitelist: Set; + private allowAllSenders: boolean; private prefix: string; private allowPatterns: RegExp[]; private denyPatterns: RegExp[]; @@ -129,6 +130,7 @@ export class AuthService { constructor(config: AuthConfig) { const detailed = AuthService.buildWhitelistDetailed(config.whitelist); this.whitelist = detailed.whitelist; + this.allowAllSenders = config.allowAllSenders ?? false; this.prefix = config.prefix; this.defaultRole = config.defaultRole ?? 'owner'; this.channelRoles = config.channelRoles ?? {}; @@ -165,9 +167,15 @@ export class AuthService { } if (this.whitelist.size === 0) { - logger.warn( - 'Auth whitelist is empty — ALL senders are authorized. To restrict access, add phone numbers to auth.whitelist in config.json.', - ); + if (this.allowAllSenders) { + logger.warn( + 'Auth whitelist is empty and auth.allowAllSenders=true — ALL senders are authorized to execute code on this host. Use only for local development.', + ); + } else { + logger.error( + 'Auth whitelist is empty — ALL senders will be DENIED. Add phone numbers/IDs to auth.whitelist in config.json, or set auth.allowAllSenders=true to intentionally allow everyone (local dev only).', + ); + } } this.startExpiryTimer(); @@ -214,9 +222,6 @@ export class AuthService { /** Check if a sender is allowed to use the bridge */ isAuthorized(sender: string, channel?: string): boolean { - if (this.whitelist.size === 0) { - return true; // No whitelist = open access - } const normalizedSender = AuthService.normalizeId(sender); if (this.whitelist.has(normalizedSender)) { return true; // Whitelisted — always authorized @@ -228,7 +233,8 @@ export class AuthService { } } // Pairing is additive: a user approved via pairing has an active access_control entry. - // Check the DB when pairing is enabled so paired users coexist with the whitelist. + // Check the DB when pairing is enabled so paired users are authorized even when the + // whitelist is empty (pairing is the intended way to grant access without a whitelist). if (this.pairingEnabled && this.db && channel) { try { const entry = getAccess(this.db, sender, channel); @@ -242,6 +248,12 @@ export class AuthService { ); } } + // Fail closed: an empty whitelist denies every otherwise-unknown sender unless the + // operator has explicitly opted into open access via auth.allowAllSenders. Inbound + // messages can execute code on the host, so this must not silently open up. + if (this.whitelist.size === 0) { + return this.allowAllSenders; + } return false; } @@ -276,8 +288,16 @@ export class AuthService { try { entry = getAccess(this.db, userId, channel); } catch (err) { - logger.warn({ err, userId, channel }, 'AuthService: getAccess failed — defaulting to allow'); - return { allowed: true }; + // Fail closed: a DB error during an access-control check must not elevate + // the caller to unrestricted access. Deny and let them retry. + logger.error( + { err, userId, channel }, + 'AuthService: getAccess failed — denying (fail closed)', + ); + return { + allowed: false, + reason: 'Access control is temporarily unavailable. Please try again.', + }; } // No entry → default to owner (backward-compatible with pre-ACL whitelist behaviour). @@ -324,11 +344,16 @@ export class AuthService { // Determine the effective allowed-action set: // • If the entry has an explicit allowed_actions list, use that. // • Otherwise fall back to the role default. - // • null means "no restriction" (owner / admin). + // • A known role mapped to null means "no restriction" (owner / admin / custom). + // • An UNKNOWN/corrupt role string falls back to the most restrictive role + // (viewer) rather than null — fail closed instead of granting full access. + const roleDefault = Object.prototype.hasOwnProperty.call(ROLE_ALLOWED_ACTIONS, entry.role) + ? ROLE_ALLOWED_ACTIONS[entry.role] + : ROLE_ALLOWED_ACTIONS['viewer']; const effectiveAllowed = entry.allowed_actions && entry.allowed_actions.length > 0 ? entry.allowed_actions - : (ROLE_ALLOWED_ACTIONS[entry.role] ?? null); + : (roleDefault ?? null); if (effectiveAllowed !== null && !effectiveAllowed.includes(action)) { logger.warn( @@ -426,6 +451,7 @@ export class AuthService { updateConfig(config: AuthConfig): void { const detailed = AuthService.buildWhitelistDetailed(config.whitelist); this.whitelist = detailed.whitelist; + this.allowAllSenders = config.allowAllSenders ?? false; this.prefix = config.prefix; this.defaultRole = config.defaultRole ?? 'owner'; this.channelRoles = config.channelRoles ?? {}; diff --git a/src/core/command-handlers.ts b/src/core/command-handlers.ts index ee111f41..3fc308ef 100644 --- a/src/core/command-handlers.ts +++ b/src/core/command-handlers.ts @@ -1083,7 +1083,10 @@ export class CommandHandlers { if (memory) { try { const callerEntry = await memory.getAccess(message.sender, message.source); - const callerRole = callerEntry?.role ?? 'owner'; + // Fail closed: a caller with no access entry yet is treated as the most + // restrictive role, not 'owner'. Privileged commands (/role, /approve) + // must never default-grant owner powers to an un-provisioned sender. + const callerRole = callerEntry?.role ?? 'viewer'; if (callerRole !== 'owner' && callerRole !== 'admin') { await send( `Permission denied. The /role command requires owner or admin role. Your current role is *${callerRole}*.`, @@ -1176,7 +1179,10 @@ export class CommandHandlers { if (memory) { try { const callerEntry = await memory.getAccess(message.sender, message.source); - const callerRole = callerEntry?.role ?? 'owner'; + // Fail closed: a caller with no access entry yet is treated as the most + // restrictive role, not 'owner'. Privileged commands (/role, /approve) + // must never default-grant owner powers to an un-provisioned sender. + const callerRole = callerEntry?.role ?? 'viewer'; if (callerRole !== 'owner' && callerRole !== 'admin') { await send( `Permission denied. The /approve command requires owner or admin role. Your current role is *${callerRole}*.`, diff --git a/src/core/config.ts b/src/core/config.ts index d2487d66..5c47e7ff 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -115,7 +115,11 @@ export async function writeMcpConfig( const dotFolderPath = join(workspacePath, '.openbridge'); await mkdir(dotFolderPath, { recursive: true }); const outputPath = join(dotFolderPath, 'mcp-config.json'); - await writeFile(outputPath, JSON.stringify({ mcpServers: merged }, null, 2), 'utf-8'); + // mode 0o600: holds inline MCP server env tokens — restrict to the owner. + await writeFile(outputPath, JSON.stringify({ mcpServers: merged }, null, 2), { + encoding: 'utf-8', + mode: 0o600, + }); _mcpConfigPath = outputPath; logger.info({ path: outputPath, servers: Object.keys(merged) }, 'MCP config written'); @@ -171,6 +175,8 @@ export function convertV2ToInternal(v2Config: V2Config): AppConfig { defaultWorkspace: 'default', auth: { whitelist: v2Config.auth.whitelist, + // V2 requires a non-empty whitelist, so open-access never applies here. + allowAllSenders: false, prefix: v2Config.auth.prefix, rateLimit: v2Config.auth.rateLimit ?? { enabled: true, diff --git a/src/integrations/adapters/email-adapter.ts b/src/integrations/adapters/email-adapter.ts index 7f255ad1..0057641f 100644 --- a/src/integrations/adapters/email-adapter.ts +++ b/src/integrations/adapters/email-adapter.ts @@ -461,7 +461,10 @@ export class EmailAdapter implements BusinessIntegration { host: imapHost, port: imapPort, tls: imapPort === 993, - tlsOptions: { rejectUnauthorized: false }, + // Verify the server certificate by default (prevents MITM credential + // theft). Operators connecting to a self-signed IMAP server can opt out + // explicitly with options.allowInsecureTLS = true. + tlsOptions: { rejectUnauthorized: opts['allowInsecureTLS'] !== true }, }); const messages: EmailMessage[] = []; @@ -588,7 +591,9 @@ export class EmailAdapter implements BusinessIntegration { host: imapHost, port: imapPort, tls: imapPort === 993, - tlsOptions: { rejectUnauthorized: false }, + // Verify the server certificate by default; opt out explicitly with + // options.allowInsecureTLS = true for self-signed servers. + tlsOptions: { rejectUnauthorized: opts['allowInsecureTLS'] !== true }, }); const attachments: EmailAttachment[] = []; diff --git a/src/intelligence/processors/email-processor.ts b/src/intelligence/processors/email-processor.ts index 176b8eb6..ab5d7d89 100644 --- a/src/intelligence/processors/email-processor.ts +++ b/src/intelligence/processors/email-processor.ts @@ -8,7 +8,7 @@ import { readFile, writeFile, mkdtemp, rm } from 'fs/promises'; import { tmpdir } from 'os'; -import { join } from 'path'; +import { join, basename } from 'path'; import { createLogger } from '../../core/logger.js'; import type { ExtractedTable, ProcessorResult } from '../../types/intelligence.js'; @@ -122,7 +122,14 @@ async function processAttachments( for (const attachment of attachments) { const filename = attachment.filename ?? `attachment_${Date.now()}`; const tmpDir = await mkdtemp(join(tmpdir(), 'ob-email-')); - const tmpPath = join(tmpDir, filename); + // Path-traversal guard: the MIME filename is fully attacker-controlled + // (e.g. "../../../../Users/x/.zshrc"). Strip every path component with + // basename(); reject the residual "."/".." cases which basename leaves intact. + let safeName = basename(filename); + if (!safeName || safeName === '.' || safeName === '..') { + safeName = `attachment_${Date.now()}`; + } + const tmpPath = join(tmpDir, safeName); try { await writeFile(tmpPath, attachment.content); diff --git a/src/types/config.ts b/src/types/config.ts index 0b98725c..b865c60d 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -38,6 +38,13 @@ export const CommandFilterConfigSchema = z.object({ /** Schema for auth configuration */ export const AuthConfigSchema = z.object({ whitelist: z.array(z.string()).default([]), + /** + * Fail-open opt-in. When the whitelist is empty, access is DENIED by default + * (every inbound message can trigger code execution on the host, so an empty + * whitelist must not silently authorize everyone). Set this to `true` to + * explicitly allow all senders — intended only for local/console development. + */ + allowAllSenders: z.boolean().default(false), prefix: z.string().default('/ai'), rateLimit: RateLimitConfigSchema.default({}), commandFilter: CommandFilterConfigSchema.default({}), diff --git a/tests/connectors/telegram/telegram-integration.test.ts b/tests/connectors/telegram/telegram-integration.test.ts index 625dc2d4..0d67d4f3 100644 --- a/tests/connectors/telegram/telegram-integration.test.ts +++ b/tests/connectors/telegram/telegram-integration.test.ts @@ -236,11 +236,12 @@ describe('Telegram connector integration (OB-323)', () => { expect(latestBot().api.sendMessage).not.toHaveBeenCalled(); }); - it('allows all senders when the whitelist is empty', async () => { + it('allows all senders when allowAllSenders is enabled (empty whitelist)', async () => { bridge = new Bridge({ ...baseConfig(), auth: { whitelist: [], + allowAllSenders: true, prefix: '/ai', rateLimit: { enabled: false, windowMs: 60000, maxMessages: 5 }, }, diff --git a/tests/connectors/webchat/webchat-integration.test.ts b/tests/connectors/webchat/webchat-integration.test.ts index c1c5024f..a735c7d3 100644 --- a/tests/connectors/webchat/webchat-integration.test.ts +++ b/tests/connectors/webchat/webchat-integration.test.ts @@ -291,11 +291,12 @@ describe('WebChat connector integration (OB-323)', () => { expect(client.send).not.toHaveBeenCalled(); }); - it('allows all senders when the whitelist is empty', async () => { + it('allows all senders when allowAllSenders is enabled (empty whitelist)', async () => { bridge = new Bridge({ ...baseConfig(), auth: { whitelist: [], + allowAllSenders: true, prefix: '/ai', rateLimit: { enabled: false, windowMs: 60000, maxMessages: 5 }, }, diff --git a/tests/core/agent-runner.test.ts b/tests/core/agent-runner.test.ts index a1c146cf..0fe7d176 100644 --- a/tests/core/agent-runner.test.ts +++ b/tests/core/agent-runner.test.ts @@ -1707,12 +1707,14 @@ describe('manifestToSpawnOptions', () => { expect(opts.allowedTools).toBeUndefined(); }); - it('leaves allowedTools undefined for unrecognized profile', async () => { + it('falls back to read-only tools for an unrecognized profile (fail closed)', async () => { const { spawnOptions: opts } = await manifestToSpawnOptions({ ...baseManifest, profile: 'custom-unknown', }); - expect(opts.allowedTools).toBeUndefined(); + // An unknown profile must NOT leave the worker unrestricted (undefined = + // no --allowedTools = full access). It fails closed to read-only. + expect(opts.allowedTools).toEqual(['Read', 'Glob', 'Grep']); }); it('resolves custom profile when provided', async () => { diff --git a/tests/core/auth-access-control.test.ts b/tests/core/auth-access-control.test.ts index 6128abb7..3aa8a96d 100644 --- a/tests/core/auth-access-control.test.ts +++ b/tests/core/auth-access-control.test.ts @@ -15,10 +15,13 @@ import { AuthService } from '../../src/core/auth.js'; // Helpers // --------------------------------------------------------------------------- -function makeAuth(overrides: Partial<{ whitelist: string[]; prefix: string }> = {}): AuthService { +function makeAuth( + overrides: Partial<{ whitelist: string[]; prefix: string; allowAllSenders: boolean }> = {}, +): AuthService { return new AuthService({ whitelist: overrides.whitelist ?? ['+1234567890'], prefix: overrides.prefix ?? '/ai', + allowAllSenders: overrides.allowAllSenders ?? false, }); } @@ -415,8 +418,13 @@ describe('AuthService — access control', () => { expect(auth.isAuthorized('1234567890@c.us')).toBe(true); }); - it('returns true when whitelist is empty (open access)', () => { - const openAuth = makeAuth({ whitelist: [] }); + it('returns false when whitelist is empty (fail closed by default)', () => { + const closedAuth = makeAuth({ whitelist: [] }); + expect(closedAuth.isAuthorized('+9999999999')).toBe(false); + }); + + it('returns true when whitelist is empty and allowAllSenders is opted in', () => { + const openAuth = makeAuth({ whitelist: [], allowAllSenders: true }); expect(openAuth.isAuthorized('+9999999999')).toBe(true); }); }); diff --git a/tests/core/auth.test.ts b/tests/core/auth.test.ts index 683020a7..5399d1b7 100644 --- a/tests/core/auth.test.ts +++ b/tests/core/auth.test.ts @@ -23,8 +23,13 @@ describe('AuthService', () => { expect(auth.isAuthorized('+1111111111')).toBe(false); }); - it('should allow all senders when whitelist is empty', () => { + it('should deny all senders when whitelist is empty (fail closed)', () => { const auth = new AuthService({ whitelist: [], prefix: '/ai' }); + expect(auth.isAuthorized('+anyone')).toBe(false); + }); + + it('should allow all senders when whitelist is empty and allowAllSenders is true', () => { + const auth = new AuthService({ whitelist: [], prefix: '/ai', allowAllSenders: true }); expect(auth.isAuthorized('+anyone')).toBe(true); }); diff --git a/tests/e2e/whatsapp-bridge-e2e.test.ts b/tests/e2e/whatsapp-bridge-e2e.test.ts index 7ab2f371..53324c3f 100644 --- a/tests/e2e/whatsapp-bridge-e2e.test.ts +++ b/tests/e2e/whatsapp-bridge-e2e.test.ts @@ -174,11 +174,12 @@ describe('E2E: WhatsApp → Bridge → Provider → WhatsApp', () => { expect(waServer.sentMessages).toHaveLength(0); }); - it('allows all senders when whitelist is empty', async () => { + it('allows all senders when allowAllSenders is enabled (empty whitelist)', async () => { provider.setResponse({ content: 'open sesame' }); await startBridge({ auth: { whitelist: [], + allowAllSenders: true, prefix: '/ai', rateLimit: { enabled: false, windowMs: 60000, maxMessages: 100 }, }, diff --git a/tests/integration/message-flow.test.ts b/tests/integration/message-flow.test.ts index 2243ec18..1bd74335 100644 --- a/tests/integration/message-flow.test.ts +++ b/tests/integration/message-flow.test.ts @@ -150,10 +150,11 @@ describe('Full message flow: connector → bridge → provider → connector', ( expect(ctx.connector.sentMessages).toHaveLength(0); }); - it('allows all senders when whitelist is empty', async () => { + it('allows all senders when allowAllSenders is enabled (empty whitelist)', async () => { ctx = buildBridge({ auth: { whitelist: [], + allowAllSenders: true, prefix: '/ai', rateLimit: { enabled: false, windowMs: 60000, maxMessages: 5 }, },