Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion src/cli/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,12 @@ export async function runInit(options: InitOptions = {}): Promise<void> {
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
Expand Down
22 changes: 19 additions & 3 deletions src/core/agent-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}

Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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() */
Expand Down
48 changes: 37 additions & 11 deletions src/core/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const PAIRING_RATE_LIMIT_WINDOW_MS = 60 * 60 * 1000; // 1 hour

export class AuthService {
private whitelist: Set<string>;
private allowAllSenders: boolean;
private prefix: string;
private allowPatterns: RegExp[];
private denyPatterns: RegExp[];
Expand Down Expand Up @@ -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 ?? {};
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 ?? {};
Expand Down
10 changes: 8 additions & 2 deletions src/core/command-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}*.`,
Expand Down Expand Up @@ -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}*.`,
Expand Down
8 changes: 7 additions & 1 deletion src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 7 additions & 2 deletions src/integrations/adapters/email-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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[] = [];
Expand Down
11 changes: 9 additions & 2 deletions src/intelligence/processors/email-processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions src/types/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({}),
Expand Down
3 changes: 2 additions & 1 deletion tests/connectors/telegram/telegram-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
},
Expand Down
3 changes: 2 additions & 1 deletion tests/connectors/webchat/webchat-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
},
Expand Down
6 changes: 4 additions & 2 deletions tests/core/agent-runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
14 changes: 11 additions & 3 deletions tests/core/auth-access-control.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}

Expand Down Expand Up @@ -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);
});
});
Expand Down
7 changes: 6 additions & 1 deletion tests/core/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
3 changes: 2 additions & 1 deletion tests/e2e/whatsapp-bridge-e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
},
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/message-flow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
},
Expand Down
Loading