From f2131436ef40f2638581c070743c390361247d61 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 21 Sep 2026 15:59:51 +0000 Subject: [PATCH 1/5] Add per-command network allow lists, and deny reasons from the ask callback registerCommandNetworkLists(commandId, { allowedDomains }) gives one invocation an allow list of its own for as long as it runs, in addition to the configured one; unregisterCommandNetworkLists(commandId) removes it and reset() removes all. Until now the only way to let one command reach one more host was updateConfig, which widens every command and the default injectHosts scope. For a connection the order is: deniedDomains denies, allowedDomains allows, strictAllowlist denies, the list registered for the id in the proxy username allows, then the ask callback, else deny. Entries get the same check as network.allowedDomains; the predicate moves to domain-pattern.ts so the manager does not load the schema module. An invalid entry throws, and so does a commandId shorter than 22 characters, because the id is the only thing that ties a connection to a list. SandboxAskCallback may now resolve to { allow: false, reason }, and the reason is reported in the violation line in place of "user denied". It is sanitized like the rest of the line and cut to 500 UTF-16 code units. Only the value true allows: a callback that resolved to some other truthy value used to allow and now denies. SandboxManager.askCallbackDenyReason is true so a caller can tell whether a reason object is understood before returning one. --- src/sandbox/domain-pattern.ts | 82 ++++++++++ src/sandbox/sandbox-config.ts | 83 ++-------- src/sandbox/sandbox-manager.ts | 215 ++++++++++++++++++++++++- src/sandbox/sandbox-schemas.ts | 38 ++++- src/sandbox/sandbox-utils.ts | 20 ++- src/sandbox/sandbox-violation-store.ts | 33 +++- 6 files changed, 377 insertions(+), 94 deletions(-) diff --git a/src/sandbox/domain-pattern.ts b/src/sandbox/domain-pattern.ts index 8019d6c79..9b9c8a96a 100644 --- a/src/sandbox/domain-pattern.ts +++ b/src/sandbox/domain-pattern.ts @@ -3,6 +3,10 @@ * (sandbox-manager) and config-time validation (sandbox-config). * Lives in its own module so the schema can import it without pulling * in sandbox-manager (which imports the schema — circular). + * + * The validity check for a list entry lives here too, for the converse + * reason: the manager applies it to lists registered at run time, and gets it + * from here without loading the schema module. */ import { isIP } from 'node:net' @@ -73,6 +77,84 @@ function parsePortSuffix(suffix: string): number | undefined { return port > 65535 ? undefined : port } +/** + * Host-only pattern check (e.g., "example.com", "*.npmjs.org"). Rejects + * protocols, paths, ports, and overly broad wildcards. + */ +export function isValidDomainPattern(val: string): boolean { + // A bare IPv6 literal as produced by splitDomainPatternPort for a + // bracketed entry (`[::1]`, `[2001:db8::1]:443` -> `::1`, `2001:db8::1`). + // Whether the *raw* entry was bracketed is enforced separately + // (hasValidIpv6Bracketing) before the split. + if (isIP(val) === 6) return true + + // Reject protocols, paths, ports, etc. + if (val.includes('://') || val.includes('/') || val.includes(':')) { + return false + } + + // Allow localhost + if (val === 'localhost') return true + + // Allow wildcard domains like *.example.com + if (val.startsWith('*.')) { + const domain = val.slice(2) + // After the *. there must be a valid domain with at least one more dot + // e.g., *.example.com is valid, *.com is not (too broad) + if ( + !domain.includes('.') || + domain.startsWith('.') || + domain.endsWith('.') + ) { + return false + } + // Count dots - must have at least 2 parts after the wildcard (e.g., example.com) + const parts = domain.split('.') + return parts.length >= 2 && parts.every(p => p.length > 0) + } + + // Reject any other use of wildcards (e.g., *, *., etc.) + if (val.includes('*')) { + return false + } + + // Regular domains must have at least one dot and only valid characters + return val.includes('.') && !val.startsWith('.') && !val.endsWith('.') +} + +export const DOMAIN_PATTERN_MESSAGE = + 'Invalid domain pattern. Must be a valid domain (e.g., "example.com"), a wildcard (e.g., "*.example.com"), or a bracketed IPv6 literal (e.g., "[::1]", "[2001:db8::1]:443"). Overly broad patterns like "*.com" or "*" are not allowed for security reasons.' + +/** + * Raw-entry rule applied before the port split: an entry with two or more + * colons is an IPv6 literal and must use RFC 3986 brackets (`[::1]`, + * `[::1]:443`). Unbracketed it is ambiguous (`2001:db8::1:443` is itself a + * valid 8-hextet address), so reject it and make the user say which they + * mean, rather than accept an entry that can silently match the wrong thing. + */ +export function hasValidIpv6Bracketing(val: string): boolean { + const first = val.indexOf(':') + const multiColon = first !== -1 && val.indexOf(':', first + 1) !== -1 + return !multiColon || val.startsWith('[') +} + +/** + * The check an allow-list entry gets: a domain pattern with an optional + * `:port` suffix. One function for both places such an entry arrives (the + * `network.allowedDomains` schema, and a per-command list registered with the + * manager at run time), so neither accepts what the other refuses. + */ +export function isValidAllowedDomainEntry(val: string): boolean { + return ( + hasValidIpv6Bracketing(val) && + isValidDomainPattern(splitDomainPatternPort(val).hostPattern) + ) +} + +export const ALLOWED_DOMAIN_ENTRY_MESSAGE = + DOMAIN_PATTERN_MESSAGE + + ' An optional ":port" suffix (1-65535) restricts the entry to that port.' + /** * The IP-literal entries of an allow/deny list as `{ range, port? }` rules * for the resolved-address check: an address the list names explicitly is diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index 6a2b32767..a36cf927b 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -3,13 +3,17 @@ * This is the main configuration interface that consumers pass to SandboxManager.initialize() */ -import { isIP } from 'node:net' import type { FilterRequestCallback } from './request-filter.js' import { isAbsolute, posix as posixPath, win32 as win32Path } from 'node:path' import { z } from 'zod' import { + ALLOWED_DOMAIN_ENTRY_MESSAGE, + DOMAIN_PATTERN_MESSAGE, + hasValidIpv6Bracketing, isInjectHostCoveredByAllowedDomains, + isValidAllowedDomainEntry, + isValidDomainPattern, splitDomainPatternPort, stripDomainPatternPort, } from './domain-pattern.js' @@ -17,54 +21,6 @@ import { parseAddressRange } from './address.js' import { containsGlobCharsForPlatform } from './sandbox-utils.js' import { getPlatform } from '../utils/platform.js' -/** - * Host-only pattern check (e.g., "example.com", "*.npmjs.org"). Rejects - * protocols, paths, ports, and overly broad wildcards. - */ -function isValidDomainPattern(val: string): boolean { - // A bare IPv6 literal as produced by splitDomainPatternPort for a - // bracketed entry (`[::1]`, `[2001:db8::1]:443` → `::1`, `2001:db8::1`). - // Whether the *raw* entry was bracketed is enforced separately - // (hasValidIpv6Bracketing) before the split. - if (isIP(val) === 6) return true - - // Reject protocols, paths, ports, etc. - if (val.includes('://') || val.includes('/') || val.includes(':')) { - return false - } - - // Allow localhost - if (val === 'localhost') return true - - // Allow wildcard domains like *.example.com - if (val.startsWith('*.')) { - const domain = val.slice(2) - // After the *. there must be a valid domain with at least one more dot - // e.g., *.example.com is valid, *.com is not (too broad) - if ( - !domain.includes('.') || - domain.startsWith('.') || - domain.endsWith('.') - ) { - return false - } - // Count dots - must have at least 2 parts after the wildcard (e.g., example.com) - const parts = domain.split('.') - return parts.length >= 2 && parts.every(p => p.length > 0) - } - - // Reject any other use of wildcards (e.g., *, *., etc.) - if (val.includes('*')) { - return false - } - - // Regular domains must have at least one dot and only valid characters - return val.includes('.') && !val.startsWith('.') && !val.endsWith('.') -} - -const DOMAIN_PATTERN_MESSAGE = - 'Invalid domain pattern. Must be a valid domain (e.g., "example.com"), a wildcard (e.g., "*.example.com"), or a bracketed IPv6 literal (e.g., "[::1]", "[2001:db8::1]:443"). Overly broad patterns like "*.com" or "*" are not allowed for security reasons.' - /** * Schema for domain patterns (e.g., "example.com", "*.npmjs.org") * Validates that domain patterns are safe and don't include overly broad wildcards @@ -77,32 +33,13 @@ const domainPatternSchema = z * Domain pattern with an optional `:port` suffix (e.g., "example.com:443", * "*.npmjs.org:8443"). Used for allowedDomains / deniedDomains, where the * proxy knows the destination port; an entry without a port matches any port. + * The check and its message live in domain-pattern.ts so the manager can hold + * an allow list registered at run time to the same rule without loading this + * module's schemas. */ -/** - * Raw-entry rule applied before the port split: an entry with two or more - * colons is an IPv6 literal and must use RFC 3986 brackets (`[::1]`, - * `[::1]:443`). Unbracketed it is ambiguous — `2001:db8::1:443` is itself a - * valid 8-hextet address — so reject it and make the user say which they - * mean, rather than accept an entry that can silently match the wrong thing. - */ -function hasValidIpv6Bracketing(val: string): boolean { - const first = val.indexOf(':') - const multiColon = first !== -1 && val.indexOf(':', first + 1) !== -1 - return !multiColon || val.startsWith('[') -} - const domainPortPatternSchema = z .string() - .refine( - val => - hasValidIpv6Bracketing(val) && - isValidDomainPattern(splitDomainPatternPort(val).hostPattern), - { - message: - DOMAIN_PATTERN_MESSAGE + - ' An optional ":port" suffix (1-65535) restricts the entry to that port.', - }, - ) + .refine(isValidAllowedDomainEntry, { message: ALLOWED_DOMAIN_ENTRY_MESSAGE }) /** * deniedDomains entry: a domainPortPattern, or a bare "*" / "*:port" @@ -738,7 +675,7 @@ export const NetworkConfigSchema = z.object({ .boolean() .optional() .describe( - 'If true, hosts not in allowedDomains are denied without consulting the ask callback. Set this when allowedDomains is policy enforcement, not a prompt-suppression hint.', + 'If true, hosts not in allowedDomains are denied without consulting the ask callback, and a per-command allow list (registerCommandNetworkLists) is ignored. Set this when allowedDomains is policy enforcement, not a prompt-suppression hint.', ), deniedResolvedAddresses: z .array(addressRangeSchema) diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 91daa448a..0b7d43c00 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -91,6 +91,7 @@ import { } from './sandbox-utils.js' import { SandboxViolationStore, + sanitizeDenialReason, sanitizeUnregisteredCommandKey, shouldIgnoreViolation, } from './sandbox-violation-store.js' @@ -103,6 +104,8 @@ import { resolveParentProxy, } from './parent-proxy.js' import { + ALLOWED_DOMAIN_ENTRY_MESSAGE, + isValidAllowedDomainEntry, matchesDomainPattern, matchesDomainPatternWithPort, stripDomainPatternPort, @@ -251,6 +254,77 @@ export function resolveCommandText(decodedKey: string): string { ) } +/** + * The shortest commandId a per-command allow list may be registered under: + * 16 random bytes (128 bits) written as unpadded base64url. A length floor + * cannot tell a random id from a predictable one of the same length: a + * formatted date or a time-based UUID is longer than this and passes. All it + * does is turn away what is too short to hold 128 bits however it was made, + * such as a small counter, an epoch timestamp or a short label. + */ +const MIN_NETWORK_LISTS_COMMAND_ID_LENGTH = 22 + +/** + * Attribution key to the network allow list registered for one invocation + * (registerCommandNetworkLists), keyed the way commandTextsByKey is because + * the same carrier, the proxy username, delivers the key. filterNetworkRequest + * consults the list for a connection whose username names the key. The + * embedder removes an entry when its command is gone and reset() clears them + * all. Not bounded the way commandTextsByKey is: dropping the oldest entry + * would silently take a list away from a command that is still running. + */ +const commandNetworkListsByKey = new Map() + +function registerCommandNetworkLists( + commandId: string, + lists: { allowedDomains?: string[] }, +): void { + // The id is not echoed in either message: it is what stands between a + // sandboxed process and this list, and error text ends up in logs. + if ( + typeof commandId !== 'string' || + commandId.length < MIN_NETWORK_LISTS_COMMAND_ID_LENGTH + ) { + throw new Error( + `registerCommandNetworkLists: commandId must be a string of at least ` + + `${MIN_NETWORK_LISTS_COMMAND_ID_LENGTH} characters. The id is the ` + + `only thing that binds a proxy connection to this allow list, and ` + + `the sandboxed process chooses which id it presents, so it must be ` + + `random (at least 128 bits, e.g. 16 random bytes as base64url) and ` + + `never derived from the command text, a counter or the time.`, + ) + } + const allowedDomains = lists?.allowedDomains ?? [] + if (!Array.isArray(allowedDomains)) { + throw new Error( + 'registerCommandNetworkLists: allowedDomains must be an array of strings.', + ) + } + for (const entry of allowedDomains) { + // The rule network.allowedDomains entries are held to, so a list that + // arrives at run time cannot carry a pattern the configuration refuses. + if (typeof entry !== 'string' || !isValidAllowedDomainEntry(entry)) { + throw new Error( + `registerCommandNetworkLists: invalid allowedDomains entry ` + + `${JSON.stringify(entry)}. ${ALLOWED_DOMAIN_ENTRY_MESSAGE}`, + ) + } + } + commandNetworkListsByKey.set( + decodeSandboxedCommand(encodeSandboxedCommand(commandId)), + // Copied, so a caller that goes on to change its array does not change + // what was validated. + { allowedDomains: [...allowedDomains] }, + ) +} + +function unregisterCommandNetworkLists(commandId: string): void { + if (typeof commandId !== 'string') return + commandNetworkListsByKey.delete( + decodeSandboxedCommand(encodeSandboxedCommand(commandId)), + ) +} + /** * Record a proxy-side denial in the violation store so the model sees a * structured block alongside the raw 403 / SOCKS @@ -329,6 +403,33 @@ function redactUrlForViolation(url: string): string { } } +/** Whether an ask-callback answer is one of the two documented denials. */ +function isDeclaredDenial(answer: unknown): boolean { + return ( + answer === false || + (typeof answer === 'object' && + answer !== null && + (answer as { allow?: unknown }).allow === false) + ) +} + +/** + * The reason an ask-callback answer gives for denying, or undefined when it + * gives none that can be shown. Only the documented `{ allow: false, reason }` + * supplies one. An object that carries a reason without saying allow: false + * (`{ allow: true, reason }`, say) contradicts itself: it denies like every + * answer that is not `true`, but its text was not written as the explanation + * of a denial, so the generic reason is reported instead. Sanitized and cut: + * it is caller-supplied text on its way into a violation line, which a model + * reads. + */ +function denialReasonOf(answer: unknown): string | undefined { + if (typeof answer !== 'object' || answer === null) return undefined + const { allow, reason } = answer as { allow?: unknown; reason?: unknown } + if (allow !== false || typeof reason !== 'string') return undefined + return sanitizeDenialReason(reason) || undefined +} + async function filterNetworkRequest( port: number, host: string, @@ -385,22 +486,60 @@ async function filterNetworkRequest( } } - // No matching rules - ask user or deny. strictAllowlist makes the - // allowlist deterministic enforcement: never fall through to the callback. - if (!sandboxAskCallback || config.network.strictAllowlist) { + // strictAllowlist makes the configured allowlist deterministic + // enforcement: nothing below this point, neither a per-command allow list + // nor the callback, can widen it. + if (config.network.strictAllowlist) { logForDebugging(`No matching config rule, denying: ${host}:${port}`) return denied('host is not on the allow list') } - logForDebugging(`No matching config rule, asking user: ${host}:${port}`) + // The allow list registered for the invocation this connection's proxy + // username names, if there is one. It comes after both configured lists, so + // it lifts the default deny for that one command and never a configured + // deniedDomains entry. The username is presented by the client inside the + // sandbox; see registerCommandNetworkLists for what that does and does not + // guarantee. + const perCommand = encodedCommand + ? commandNetworkListsByKey.get(decodeSandboxedCommand(encodedCommand)) + : undefined + for (const allowedDomain of perCommand?.allowedDomains ?? []) { + if (matchesDomainPatternWithPort(canonicalHost, port, allowedDomain)) { + logForDebugging(`Allowed by per-command rule: ${host}:${port}`) + return true + } + } + + // No matching rules - ask user or deny. + if (!sandboxAskCallback) { + logForDebugging(`No matching rule, denying: ${host}:${port}`) + return denied('host is not on the allow list') + } + + logForDebugging(`No matching rule, asking user: ${host}:${port}`) try { - const userAllowed = await sandboxAskCallback({ host, port }) - if (userAllowed) { + // Held as unknown: the declared type is what a TypeScript caller can + // return, not what a JavaScript one does. + const answer: unknown = await sandboxAskCallback({ host, port }) + // Only `true` allows. An object is how a callback denies with a reason, + // and an object is truthy, so a truthiness test here would read every + // such denial as an allow. + if (answer === true) { logForDebugging(`User allowed: ${host}:${port}`) return true } + if (!isDeclaredDenial(answer)) { + logForDebugging( + `Ask callback answered neither a boolean nor { allow: false, reason }; treating it as a denial: ${host}:${port}`, + { level: 'warn' }, + ) + } logForDebugging(`User denied: ${host}:${port}`) - return denied('user denied') + // The callback's own reason when it gave one, so whoever reads the + // violation line learns why and what to do instead; a bare `false`, and + // an answer that is neither of the documented denials, keep the generic + // text. + return denied(denialReasonOf(answer) ?? 'user denied') } catch (error) { logForDebugging(`Error in permission callback: ${error}`, { level: 'error', @@ -1620,6 +1759,10 @@ export type WrapWithSandboxOptions = { * command text: keys are compared on their first 100 characters, so two * long commands sharing a prefix would otherwise cross-attribute, and a * rerun of the same text would inherit the earlier run's events. + * + * An invocation that also registers a network allow list under this id + * (`registerCommandNetworkLists`) needs more than uniqueness: there the id + * must be unguessable, for the reasons given on that method. */ commandId?: string /** @@ -2325,6 +2468,7 @@ async function reset(): Promise { sentinelRegistry.clear() awsPairRegistry.clear() commandTextsByKey.clear() + commandNetworkListsByKey.clear() maskedFileStore.dispose() } @@ -2460,6 +2604,60 @@ export interface ISandboxManager { cwd?: string, options?: WrapWithSandboxOptions, ): Promise<{ argv: string[]; env: NodeJS.ProcessEnv }> + /** + * Register the network allow list of one invocation, under the `commandId` + * its wrap was given ({@link WrapWithSandboxOptions}). Entries use the + * grammar and the matcher of `network.allowedDomains` and get the same + * validation; an invalid entry throws. Registering an id again replaces + * its list. + * + * Order of evaluation for a connection: a configured `deniedDomains` match + * denies; a configured `allowedDomains` match allows; + * `network.strictAllowlist` denies; a match in the list registered for the + * id the connection presents allows; then the ask callback decides if + * there is one, and otherwise the connection is denied. So a per-command + * entry never overrides a configured `deniedDomains` entry and is ignored + * entirely under `strictAllowlist`. It also never enters `network.*`, so + * the default `injectHosts` scope of a masked credential, which is + * `network.allowedDomains`, does not grow. + * + * What binds a connection to a list: the proxy reads the id from the proxy + * username, and the username is presented by the client inside the + * sandbox. The id is therefore the ONLY thing binding a connection to an + * allow list. It MUST be unguessable: at least 128 bits of randomness (16 + * random bytes as base64url come to 22 characters), never a counter, a + * timestamp or anything derived from the command. An id shorter than 22 + * characters throws. A sandboxed process that presents another live + * invocation's id gets that invocation's allows, so this is attribution, + * not a boundary between concurrent commands of one session. + * + * Register just before spawning the wrapped command, and unregister when + * the child exits or never started: a registration that outlives its + * command widens the window in which its id is worth presenting. + * + * As with every attribution key, only the first 100 characters of an id + * take part, and an id whose encoded form does not fit in the proxy + * username (possible only with non-ASCII ids) is carried by no connection, + * so its list never applies. + */ + registerCommandNetworkLists( + commandId: string, + lists: { allowedDomains?: string[] }, + ): void + /** + * Remove the list registered for `commandId`. A no-op for an id that has + * none. `reset()` removes them all. + */ + unregisterCommandNetworkLists(commandId: string): void + /** + * True when the network filter understands an ask callback's + * `{ allow: false, reason }` answer (see {@link SandboxAskCallback}). Read + * it before returning that object: releases up to v0.0.77 allowed a + * connection on any truthy answer, and an object is truthy, so there the + * same answer would be read as an allow. Where this is absent, deny with a + * plain `false`. + */ + readonly askCallbackDenyReason: true getSandboxViolationStore(): SandboxViolationStore annotateStderrWithSandboxFailures(command: string, stderr: string): string getLinuxGlobPatternWarnings(): string[] @@ -2503,6 +2701,9 @@ export const SandboxManager: ISandboxManager = { waitForNetworkInitialization, wrapWithSandbox, wrapWithSandboxArgv, + registerCommandNetworkLists, + unregisterCommandNetworkLists, + askCallbackDenyReason: true, cleanupAfterCommand, reset, getMitmCA: () => mitmCA, diff --git a/src/sandbox/sandbox-schemas.ts b/src/sandbox/sandbox-schemas.ts index ce4243109..3ce30b647 100644 --- a/src/sandbox/sandbox-schemas.ts +++ b/src/sandbox/sandbox-schemas.ts @@ -91,11 +91,15 @@ export interface CredentialRestrictionConfig { * * Note: Empty `allowedHosts` means no host matches an allow rule (unlike * read's empty denyOnly). Whether an unmatched host is denied outright - * depends on the ask callback: deniedHosts are checked first and deny - * unconditionally; a host matching neither list falls through to the - * registered SandboxAskCallback when one exists, and is denied only when - * no callback is registered. Hosts needing a hard block-all regardless of - * callback behavior should use a `deniedHosts` wildcard. + * depends on what else may decide it: deniedHosts are checked first and deny + * unconditionally; under `network.strictAllowlist` a host matching neither + * list is denied there and then; otherwise it is allowed when the allow list + * registered for the invocation the connection presents + * (`SandboxManager.registerCommandNetworkLists`) matches it, and failing + * that falls through to the registered SandboxAskCallback when one exists, + * and is denied when no callback is registered. Hosts needing a hard + * block-all regardless of callback behavior should use a `deniedHosts` + * wildcard. * * Entries are the raw config patterns and may carry an optional `:port` * suffix (`api.example.com:443`, `*:22`) meaning "this rule applies only @@ -113,6 +117,28 @@ export type NetworkHostPattern = { port: number | undefined } +/** + * Asked about a host that no configured rule and no per-command allow list + * decided (never asked under `network.strictAllowlist`). Only the value + * `true` allows the connection. `false` denies it with the generic reason + * "user denied". An object `{ allow: false, reason }` denies it with that + * reason, which is what the violation line for the connection reports. Any + * other answer, truthy or not, denies with the generic reason: a `reason` on + * an object that does not say `allow: false` is not reported. + * + * The reason is sanitized before it is stored, the way the rest of a + * violation line is: each run of control characters (line breaks and tabs + * included) or of invisible ones (zero-width characters, the joiner among + * them, and bidi controls) becomes one space, `<` and `>` are removed, and + * the ends are trimmed. It is then cut to 500 UTF-16 code units, what + * `String.prototype.length` counts. A reason with nothing left after that + * falls back to "user denied". So write it as one line of plain text, and do + * not rely on angle brackets around a placeholder. + * + * Releases up to v0.0.77 allowed on any truthy answer, so an object answer + * returned to one of those would be read as an allow. Check + * `SandboxManager.askCallbackDenyReason` before returning one. + */ export type SandboxAskCallback = ( params: NetworkHostPattern, -) => Promise +) => Promise diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 23859d190..3aa84dc8c 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1055,12 +1055,20 @@ export function proxyUsernameFor(encodedCommand: string | undefined): string { /** * Inverse of {@link proxyUsernameFor}: extract the encodedCommand suffix * from `srt.`, or undefined for bare `srt` / anything else. - * The username is client-controlled inside the sandbox, so a forged suffix - * can only misattribute a denial in the violation report — it cannot - * authenticate (the token does that) or reach another command's data. A - * suffix past {@link MAX_ENCODED_COMMAND_BYTES} is longer than this process - * can mint, so it is dropped rather than stored: the denial is still - * recorded, unattributed. + * The username is presented by the client inside the sandbox. A forged + * suffix cannot authenticate (the token does that), but it decides two + * things: which invocation a denial is attributed to in the violation + * report, and which invocation's registered network allow list + * (`SandboxManager.registerCommandNetworkLists`) applies to the connection. + * The suffix is the ONLY thing binding a connection to such a list, so an id + * a list is registered under MUST be unguessable (at least 128 bits of + * randomness, never a counter, a timestamp or anything derived from the + * command), and even then a sandboxed process that presents another live + * invocation's id gets that invocation's allows: this is attribution, not a + * boundary between concurrent commands of one session. A suffix past + * {@link MAX_ENCODED_COMMAND_BYTES} is longer than this process can mint, so + * it is dropped rather than stored: the denial is still recorded, + * unattributed, and no registered list applies. */ export function encodedCommandFromProxyUser( username: string | undefined, diff --git a/src/sandbox/sandbox-violation-store.ts b/src/sandbox/sandbox-violation-store.ts index afc4ac297..5be42b880 100644 --- a/src/sandbox/sandbox-violation-store.ts +++ b/src/sandbox/sandbox-violation-store.ts @@ -143,10 +143,39 @@ function sanitizeViolationText(text: string): string { * that (an HTTP proxy header budget is kilobytes). */ export function sanitizeUnregisteredCommandKey(decodedKey: string): string { - const cut = sanitizeViolationText(decodedKey).slice( - 0, + return cutBetweenCharacters( + sanitizeViolationText(decodedKey), SANDBOXED_COMMAND_KEY_LENGTH, ) +} + +/** + * How much of a denial reason supplied by the ask callback a violation line + * carries, in UTF-16 code units (what `String.prototype.length` counts, so a + * character outside the Basic Multilingual Plane counts as two). The reason + * is prose for whoever reads the violation, a model included: a few sentences + * fit, a document does not. + */ +export const MAX_DENIAL_REASON_LENGTH = 500 + +/** + * A denial reason the ask callback supplied, as a violation line may carry + * it: sanitized like the rest of the line, and cut so that one answer cannot + * flood the text a model reads. The store sanitizes the whole line again at + * ingestion; doing it here first means the cut counts what is displayed. The + * cut can land just after a space, and the reason sits inside parentheses + * where the store's own trim of the line's ends does not reach, so the end is + * trimmed again here. + */ +export function sanitizeDenialReason(reason: string): string { + return cutBetweenCharacters( + sanitizeViolationText(reason), + MAX_DENIAL_REASON_LENGTH, + ).trimEnd() +} + +function cutBetweenCharacters(text: string, length: number): string { + const cut = text.slice(0, length) // A cut by UTF-16 code unit can land between a surrogate pair; the lone // high surrogate left behind renders as a replacement character. return /[\uD800-\uDBFF]$/.test(cut) ? cut.slice(0, -1) : cut From 91e515835b2c7f98b2ef31407df74e9c47d60ba6 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 21 Sep 2026 15:59:51 +0000 Subject: [PATCH 2/5] test: cover per-command allow lists and every ask-callback answer 64 cases through the live proxy against a loopback listener: the order of evaluation, entry validation, how the id is read from the proxy username, each shape the callback can answer with, what the reason is reduced to before it is stored, and the warning under SRT_DEBUG. --- test/sandbox/update-config.test.ts | 765 ++++++++++++++++++++++++++++- 1 file changed, 762 insertions(+), 3 deletions(-) diff --git a/test/sandbox/update-config.test.ts b/test/sandbox/update-config.test.ts index f10dfa1fc..8ac76c53d 100644 --- a/test/sandbox/update-config.test.ts +++ b/test/sandbox/update-config.test.ts @@ -1,7 +1,17 @@ -import { describe, it, expect, afterEach, beforeEach } from 'bun:test' -import { SandboxManager } from '../../src/index.js' -import { connect } from 'net' +import { describe, it, expect, afterEach, beforeEach, spyOn } from 'bun:test' +import { + SandboxManager, + SandboxRuntimeConfigSchema, + type SandboxAskCallback, + type SandboxRuntimeConfig, +} from '../../src/index.js' +import { connect, createServer, type AddressInfo, type Server } from 'net' +import { randomBytes } from 'node:crypto' import { getPlatform } from '../../src/utils/platform.js' +import { + encodeSandboxedCommand, + proxyUsernameFor, +} from '../../src/sandbox/sandbox-utils.js' import { spawnAsync } from '../helpers/spawn.js' import { isLinux, isMacOS } from '../helpers/platform.js' @@ -387,6 +397,755 @@ describe('proxy auth + network deny semantics', () => { }) }) +/** + * CONNECT `target` ("host:port") through the proxy under `username`, with the + * session token, and report the status code of the reply. + */ +function connectAs( + proxyPort: number, + target: string, + username: string, +): Promise { + return new Promise(resolve => { + const token = SandboxManager.getProxyAuthToken() + const auth = Buffer.from(`${username}:${token}`).toString('base64') + const socket = connect(proxyPort, '127.0.0.1', () => { + socket.write( + `CONNECT ${target} HTTP/1.1\r\nHost: ${target}\r\n` + + `Proxy-Authorization: Basic ${auth}\r\n\r\n`, + ) + }) + let data = '' + socket.on('data', chunk => { + data += chunk.toString() + if (data.includes('\r\n')) { + socket.destroy() + const status = data.match(/HTTP\/1\.\d (\d+)/) + resolve(status ? parseInt(status[1]) : undefined) + } + }) + socket.on('error', () => resolve(undefined)) + socket.setTimeout(2000, () => { + socket.destroy() + resolve(undefined) + }) + }) +} + +/** + * A listener on loopback for the proxy to dial, so an allow is observed as a + * 200 without leaving the machine. An IP-literal destination is dialled as + * it is, with no name to resolve and nothing for the resolved-address check + * to refuse. + */ +function listenOnLoopback(): Promise<{ server: Server; target: string }> { + return new Promise(resolve => { + const server = createServer(socket => socket.on('error', () => {})) + server.listen(0, '127.0.0.1', () => { + const { port } = server.address() as AddressInfo + resolve({ server, target: `127.0.0.1:${port}` }) + }) + }) +} + +/** An id of the kind the API asks for: 16 random bytes, 22 characters. */ +const randomCommandId = (): string => randomBytes(16).toString('base64url') + +/** The proxy username a wrap given this commandId puts in the proxy URLs. */ +const usernameFor = (commandId: string): string => + proxyUsernameFor(encodeSandboxedCommand(commandId)) + +const NO_FILESYSTEM_RULES = { denyRead: [], allowWrite: [], denyWrite: [] } + +const violationLines = (): string[] => + SandboxManager.getSandboxViolationStore() + .getViolations() + .map(v => v.line) + +describe('per-command network allow lists', () => { + let loopback: { server: Server; target: string } + let asked: string[] + const denyAndRecord: SandboxAskCallback = async ({ host, port }) => { + asked.push(`${host}:${port}`) + return false + } + + async function start( + network: Partial, + callback?: SandboxAskCallback, + ): Promise { + await SandboxManager.initialize( + { + network: { allowedDomains: [], deniedDomains: [], ...network }, + filesystem: NO_FILESYSTEM_RULES, + }, + callback, + ) + SandboxManager.getSandboxViolationStore().clear() + return SandboxManager.getProxyPort()! + } + + beforeEach(async () => { + await SandboxManager.reset() + loopback = await listenOnLoopback() + asked = [] + }) + + afterEach(async () => { + await SandboxManager.reset() + loopback.server.close() + }) + + it('lifts the default deny for the id the connection presents, and for no other', async () => { + const port = await start({}) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(200) + // An id nothing was registered under, and no id at all, see the + // configured lists alone. + expect( + await connectAs(port, loopback.target, usernameFor(randomCommandId())), + ).toBe(403) + expect(await connectAs(port, loopback.target, 'srt')).toBe(403) + expect(violationLines()).toEqual([ + `deny network-outbound ${loopback.target} (host is not on the allow list)`, + `deny network-outbound ${loopback.target} (host is not on the allow list)`, + ]) + }) + + it('is consulted before the ask callback, which still decides every host it does not list', async () => { + const port = await start({}, denyAndRecord) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(200) + expect(asked).toEqual([]) + + expect( + await connectAs(port, 'unlisted.test:443', usernameFor(registered)), + ).toBe(403) + expect(await connectAs(port, loopback.target, 'srt')).toBe(403) + expect(asked).toEqual(['unlisted.test:443', loopback.target]) + }) + + it('uses the grammar of network.allowedDomains: a port suffix narrows an entry, a wildcard widens it', async () => { + const port = await start({}, denyAndRecord) + const registered = randomCommandId() + const [host, listeningPort] = loopback.target.split(':') + // Any port but the listener's, and still a valid one: the listener can be + // handed 65535, the top of the range. + const anotherPort = + Number(listeningPort) === 65535 ? 65534 : Number(listeningPort) + 1 + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [`${host}:${anotherPort}`, '*.wild.test'], + }) + + // Listed for another port only, so the callback is asked and denies. + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + expect(asked).toEqual([loopback.target]) + // The wildcard matches a subdomain (the filter allows; the dial then + // fails, which is not a 403) and, as in the configuration, not the apex. + expect( + await connectAs(port, 'a.wild.test:443', usernameFor(registered)), + ).not.toBe(403) + expect( + await connectAs(port, 'wild.test:443', usernameFor(registered)), + ).toBe(403) + expect(asked).toEqual([loopback.target, 'wild.test:443']) + }) + + it('never overrides a configured deniedDomains entry', async () => { + const port = await start({ deniedDomains: ['127.0.0.1'] }, denyAndRecord) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + expect(violationLines()).toEqual([ + `deny network-outbound ${loopback.target} (host is on the deny list)`, + ]) + expect(asked).toEqual([]) + }) + + it('is ignored entirely under strictAllowlist', async () => { + const port = await start({ strictAllowlist: true }, denyAndRecord) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + expect(violationLines()).toEqual([ + `deny network-outbound ${loopback.target} (host is not on the allow list)`, + ]) + expect(asked).toEqual([]) + }) + + it('never enters network.*, so the configured allow list does not grow', async () => { + await start({ allowedDomains: ['example.com'] }) + SandboxManager.registerCommandNetworkLists(randomCommandId(), { + allowedDomains: [loopback.target, 'percommand.test'], + }) + + expect(SandboxManager.getConfig()?.network.allowedDomains).toEqual([ + 'example.com', + ]) + expect(SandboxManager.getNetworkRestrictionConfig().allowedHosts).toEqual([ + 'example.com', + ]) + }) + + it('stops applying once unregistered; unregistering an id that has no list changes nothing', async () => { + const port = await start({}) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + SandboxManager.unregisterCommandNetworkLists(randomCommandId()) + // Registering refuses what is not a string; unregistering has nothing to + // refuse, since no list can be registered under it, and lets it pass so + // that a cleanup path never throws. + for (const notAString of [null, undefined, 42]) { + expect(() => + SandboxManager.unregisterCommandNetworkLists( + notAString as unknown as string, + ), + ).not.toThrow() + } + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(200) + + SandboxManager.unregisterCommandNetworkLists(registered) + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + // Twice is as good as once. + SandboxManager.unregisterCommandNetworkLists(registered) + }) + + it('replaces the list when an id is registered again', async () => { + const port = await start({}) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: ['elsewhere.test'], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + }) + + it('keeps the list it validated when the caller changes its array afterwards', async () => { + const port = await start({}) + const registered = randomCommandId() + const entries = ['elsewhere.test'] + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: entries, + }) + entries.push(loopback.target) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + }) + + it('forgets every list on reset', async () => { + await start({}) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + await SandboxManager.reset() + const port = await start({}) + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + }) + + it('refuses an id shorter than 22 characters, and says what an id has to be', () => { + const lists = { allowedDomains: ['example.com'] } + for (const tooShort of ['', '1', 'inv-0001', 'a'.repeat(21)]) { + expect(() => + SandboxManager.registerCommandNetworkLists(tooShort, lists), + ).toThrow(/at least 22 characters/) + } + expect(() => + SandboxManager.registerCommandNetworkLists('a'.repeat(21), lists), + ).toThrow(/only thing that binds a proxy connection to this allow list/) + expect(() => + SandboxManager.registerCommandNetworkLists('a'.repeat(21), lists), + ).toThrow(/random .* never derived from the command text/) + // A JavaScript caller's null or number is no id either. + for (const notAString of [null, undefined, 1e21]) { + expect(() => + SandboxManager.registerCommandNetworkLists( + notAString as unknown as string, + lists, + ), + ).toThrow(/at least 22 characters/) + } + // The floor itself is accepted, and an empty or absent list is a list. + SandboxManager.registerCommandNetworkLists('a'.repeat(22), lists) + SandboxManager.registerCommandNetworkLists(randomCommandId(), {}) + SandboxManager.registerCommandNetworkLists(randomCommandId(), { + allowedDomains: [], + }) + // So is no second argument at all, which a JavaScript caller can pass. + expect(() => + SandboxManager.registerCommandNetworkLists( + randomCommandId(), + undefined as unknown as { allowedDomains?: string[] }, + ), + ).not.toThrow() + }) + + it('does not repeat the id in the error it throws', () => { + const almost = 'secret-id-0123456789x' + expect(almost).toHaveLength(21) + let message = '' + try { + SandboxManager.registerCommandNetworkLists(almost, {}) + } catch (error) { + message = (error as Error).message + } + expect(message).not.toBe('') + expect(message).not.toContain(almost) + }) + + // Accepted and refused by one predicate, so the two can only agree; this + // pins that a list arriving at run time cannot carry an entry the + // configuration would have refused, and the other way round. + it.each([ + ['example.com', true], + ['*.example.com', true], + ['api.example.com:443', true], + ['localhost', true], + ['127.0.0.1:3000', true], + ['[::1]', true], + ['[2001:db8::1]:443', true], + ['*', false], + ['*:443', false], + ['*.com', false], + ['https://example.com', false], + ['example.com/path', false], + ['example.com:0', false], + ['example.com:443.evil.test', false], + ['2001:db8::1', false], + ['nodots', false], + ['', false], + ])( + 'accepts the entry %p exactly when network.allowedDomains does (%p)', + (entry, valid) => { + expect( + SandboxRuntimeConfigSchema.safeParse({ + network: { allowedDomains: [entry], deniedDomains: [] }, + filesystem: NO_FILESYSTEM_RULES, + }).success, + ).toBe(valid) + const register = () => + SandboxManager.registerCommandNetworkLists(randomCommandId(), { + allowedDomains: [entry], + }) + if (valid) { + expect(register).not.toThrow() + } else { + expect(register).toThrow(/invalid allowedDomains entry/) + } + }, + ) + + it('refuses an entry that is not a string, and a list that is not an array', () => { + for (const entry of [null, undefined, 443, { host: 'example.com' }]) { + expect(() => + SandboxManager.registerCommandNetworkLists(randomCommandId(), { + allowedDomains: [entry as unknown as string], + }), + ).toThrow(/invalid allowedDomains entry/) + } + expect(() => + SandboxManager.registerCommandNetworkLists(randomCommandId(), { + allowedDomains: 'example.com' as unknown as string[], + }), + ).toThrow(/must be an array/) + }) + + it('registers nothing when any entry is invalid', async () => { + const port = await start({}) + const registered = randomCommandId() + expect(() => + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target, '*.com'], + }), + ).toThrow(/invalid allowedDomains entry "\*\.com"/) + + expect( + await connectAs(port, loopback.target, usernameFor(registered)), + ).toBe(403) + }) + + // An id is normalised before it keys the map, and what the proxy receives + // is decoded rather than compared as bytes. Each case here is an id or a + // username that differs from the registered one byte for byte and reaches + // its list all the same, or looks alike and does not. + describe('ids that differ byte for byte from the registered one', () => { + it('share a list when they agree on their first 100 characters', async () => { + const port = await start({}) + const prefix = randomBytes(75).toString('base64url') + expect(prefix).toHaveLength(100) + SandboxManager.registerCommandNetworkLists(`${prefix}-first`, { + allowedDomains: [loopback.target], + }) + + // A wrap given this other id mints the same username: the key is cut + // to 100 characters before it is encoded. + expect(usernameFor(`${prefix}-second`)).toBe( + usernameFor(`${prefix}-first`), + ) + expect( + await connectAs(port, loopback.target, usernameFor(`${prefix}-second`)), + ).toBe(200) + + // A username carrying all 105 characters was not minted here; it + // decodes to a longer key, which nothing is registered under. + const uncut = Buffer.from(`${prefix}-first`).toString('base64') + expect(await connectAs(port, loopback.target, `srt.${uncut}`)).toBe(403) + + // And unregistering either id removes the one list they share. + SandboxManager.unregisterCommandNetworkLists(`${prefix}-second`) + expect( + await connectAs(port, loopback.target, usernameFor(`${prefix}-first`)), + ).toBe(403) + }) + + it('reach the list through any base64 spelling of the same bytes', async () => { + const port = await start({}) + // 22 characters that encode with padding and with both of the + // characters the URL-safe alphabet spells differently. + const registered = 'id?>id?>id?>id?>id?>~~' + const minted = encodeSandboxedCommand(registered) + expect(minted).toMatch(/[+/]/) + expect(minted).toMatch(/=$/) + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + const unpadded = minted.replace(/=+$/, '') + const urlSafe = unpadded.replace(/\+/g, '-').replace(/\//g, '_') + for (const spelling of [minted, unpadded, urlSafe]) { + expect(await connectAs(port, loopback.target, `srt.${spelling}`)).toBe( + 200, + ) + } + }) + + it('share a list when they differ only in an unpaired surrogate', async () => { + const port = await start({}) + const stem = randomCommandId() + // Neither half of a surrogate pair can be encoded on its own; each + // becomes U+FFFD, so both ids normalise to one key. + SandboxManager.registerCommandNetworkLists(`${stem}\ud800`, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs(port, loopback.target, usernameFor(`${stem}\udfff`)), + ).toBe(200) + expect( + await connectAs(port, loopback.target, usernameFor(`${stem}\ufffd`)), + ).toBe(200) + // The stem alone is a different key. + expect(await connectAs(port, loopback.target, usernameFor(stem))).toBe( + 403, + ) + }) + + it('do not reach it by differing in case', async () => { + const port = await start({}) + const registered = `${randomCommandId()}abc` + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await connectAs( + port, + loopback.target, + usernameFor(registered.toUpperCase()), + ), + ).toBe(403) + }) + }) +}) + +describe('ask callback answers', () => { + let loopback: { server: Server; target: string } + + /** One CONNECT to the loopback listener, decided by `answer` alone. */ + async function decideWith( + answer: unknown, + ): Promise<{ status: number | undefined; lines: string[] }> { + await SandboxManager.initialize( + { + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: NO_FILESYSTEM_RULES, + }, + (async () => answer) as SandboxAskCallback, + ) + SandboxManager.getSandboxViolationStore().clear() + const status = await connectAs( + SandboxManager.getProxyPort()!, + loopback.target, + 'srt', + ) + return { status, lines: violationLines() } + } + + const deniedBecause = (reason: string): string[] => [ + `deny network-outbound ${loopback.target} (${reason})`, + ] + + beforeEach(async () => { + await SandboxManager.reset() + loopback = await listenOnLoopback() + }) + + afterEach(async () => { + await SandboxManager.reset() + loopback.server.close() + }) + + it('declares that it reads a denial with a reason', () => { + expect(SandboxManager.askCallbackDenyReason).toBe(true) + }) + + it('allows on true', async () => { + expect(await decideWith(true)).toEqual({ status: 200, lines: [] }) + }) + + it('denies on false, with the generic reason', async () => { + expect(await decideWith(false)).toEqual({ + status: 403, + lines: deniedBecause('user denied'), + }) + }) + + it('denies on { allow: false, reason }, and reports that reason', async () => { + const callback: SandboxAskCallback = async () => ({ + allow: false, + reason: 'not listed for this command; list the host and run it again', + }) + expect(await decideWith(await callback({ host: '', port: 0 }))).toEqual({ + status: 403, + lines: deniedBecause( + 'not listed for this command; list the host and run it again', + ), + }) + }) + + it.each([ + ['an empty reason', { allow: false, reason: '' }], + ['a reason of nothing but whitespace', { allow: false, reason: ' \n\t ' }], + ['no reason', { allow: false }], + ['a reason that is not a string', { allow: false, reason: 42 }], + ])('denies with the generic reason on %s', async (_name, answer) => { + expect(await decideWith(answer)).toEqual({ + status: 403, + lines: deniedBecause('user denied'), + }) + }) + + // Every one of these was an allow while the filter tested truthiness. + it.each([ + ['the number 1', 1], + ['a non-empty string', 'yes'], + ['an empty object', {}], + ['an empty array', []], + ['an object that says allow: true', { allow: true }], + ['the string "true"', 'true'], + // A reason is reported only for an answer that says allow: false. One + // that says otherwise was not written as the explanation of a denial. + [ + 'an object that says allow: true and gives a reason', + { allow: true, reason: 'approved for this session' }, + ], + [ + 'an object that gives a reason and no allow', + { reason: 'no verdict given' }, + ], + [ + 'an object whose allow is falsy but not false', + { allow: 0, reason: 'zero is not false' }, + ], + ])( + 'denies on a truthy answer that is not true: %s', + async (_name, answer) => { + expect(await decideWith(answer)).toEqual({ + status: 403, + lines: deniedBecause('user denied'), + }) + }, + ) + + it.each([ + ['undefined', undefined], + ['null', null], + ['zero', 0], + ])( + 'denies on a falsy answer that is not false: %s', + async (_name, answer) => { + expect(await decideWith(answer)).toEqual({ + status: 403, + lines: deniedBecause('user denied'), + }) + }, + ) + + it('cuts an over-long reason to 500 characters', async () => { + const { status, lines } = await decideWith({ + allow: false, + reason: 'x'.repeat(2000), + }) + expect(status).toBe(403) + expect(lines).toEqual(deniedBecause('x'.repeat(500))) + }) + + it('cuts between characters, never through a surrogate pair', async () => { + // The pair occupies code units 499 and 500, so a cut at 500 lands inside. + const { lines } = await decideWith({ + allow: false, + reason: `${'x'.repeat(499)}\u{1f600}tail`, + }) + expect(lines).toEqual(deniedBecause('x'.repeat(499))) + }) + + it('counts the 500 in UTF-16 code units, so a character outside the BMP counts twice', async () => { + const { lines } = await decideWith({ + allow: false, + reason: '\u{1f600}'.repeat(500), + }) + expect(lines).toEqual(deniedBecause('\u{1f600}'.repeat(250))) + }) + + it('leaves no space at the end when the cut lands just after one', async () => { + // Code unit 500 is the space. Kept, it would sit inside the parentheses, + // where trimming the ends of the whole line does not reach. + const { lines } = await decideWith({ + allow: false, + reason: `${'x'.repeat(499)} tail`, + }) + expect(lines).toEqual(deniedBecause('x'.repeat(499))) + }) + + it('strips control characters from the reason, and what would close the violations envelope', async () => { + const { lines } = await decideWith({ + allow: false, + reason: + 'first line\nsecond\x00line\x1b[31m red\u009b\u202eflipped\u200b', + }) + expect(lines).toHaveLength(1) + // eslint-disable-next-line no-control-regex + expect(lines[0]).not.toMatch(/[\x00-\x1f\x7f-\x9f\u202e\u200b<>]/) + expect(lines).toEqual( + deniedBecause( + 'first line second line [31m red flipped /sandbox_violations', + ), + ) + }) + + it('removes angle brackets outright, and turns a tab or a run of joiners into one space', async () => { + const { lines } = await decideWith({ + allow: false, + reason: 'a\tb: re-run with listed x\u200d\u200dy', + }) + expect(lines).toEqual(deniedBecause('a b: re-run with host listed x y')) + }) + + it('counts the cut in what is displayed, not in what was stripped', async () => { + // 600 control characters collapse to one space, so nothing is cut off + // the text that follows them. + const { lines } = await decideWith({ + allow: false, + reason: `before${'\x07'.repeat(600)}after`, + }) + expect(lines).toEqual(deniedBecause('before after')) + }) + + it('denies with "permission prompt failed" when the callback throws', async () => { + await SandboxManager.initialize( + { + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: NO_FILESYSTEM_RULES, + }, + async () => { + throw new Error('prompt went away') + }, + ) + SandboxManager.getSandboxViolationStore().clear() + expect( + await connectAs(SandboxManager.getProxyPort()!, loopback.target, 'srt'), + ).toBe(403) + expect(violationLines()).toEqual(deniedBecause('permission prompt failed')) + }) + + // The debug log is where an embedder caught by the change from "any truthy + // answer allows" finds out why its connections are now refused. + it('warns in the debug log about an answer that is neither a boolean nor { allow: false }, and about no other', async () => { + const before = process.env.SRT_DEBUG + process.env.SRT_DEBUG = '1' + const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}) + const errorSpy = spyOn(console, 'error').mockImplementation(() => {}) + const warningsAfter = async (answer: unknown): Promise => { + await SandboxManager.reset() + warnSpy.mockClear() + await decideWith(answer) + return warnSpy.mock.calls + .map(call => String(call[0])) + .filter(message => message.includes('Ask callback answered neither')) + } + try { + for (const undeclared of [1, 'yes', {}, { allow: true, reason: 'ok' }]) { + const warnings = await warningsAfter(undeclared) + expect(warnings).toHaveLength(1) + expect(warnings[0]).toContain('treating it as a denial') + expect(warnings[0]).toContain(loopback.target) + } + for (const declared of [true, false, { allow: false, reason: 'no' }]) { + expect(await warningsAfter(declared)).toEqual([]) + } + } finally { + warnSpy.mockRestore() + errorSpy.mockRestore() + if (before === undefined) { + delete process.env.SRT_DEBUG + } else { + process.env.SRT_DEBUG = before + } + } + }) +}) + describe('SandboxManager.updateConfig', () => { beforeEach(async () => { await SandboxManager.reset() From c519abcc037cbfd82d460773cee15068ffc22ae8 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 21 Sep 2026 15:59:51 +0000 Subject: [PATCH 3/5] docs: describe the ask callback's answers and registerCommandNetworkLists --- README.md | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/README.md b/README.md index 6116ecb15..f1cd04b50 100644 --- a/README.md +++ b/README.md @@ -280,6 +280,68 @@ const annotated = SandboxManager.annotateStderrWithSandboxFailures( ) ``` +**Asking about a host no rule decides (the ask callback).** The second argument to `SandboxManager.initialize` is an optional `SandboxAskCallback`. It is asked about a destination that neither `network.deniedDomains`, nor `network.allowedDomains`, nor a per-command allow list (next section) decided, and it is never asked under `network.strictAllowlist`. Without a callback such a destination is denied. The callback resolves to one of: + +- `true` - allow the connection. Only the value `true` allows. +- `false` - deny it. The violation line reports the generic reason `user denied`. +- `{ allow: false, reason: '...' }` - deny it, and report `reason` in the violation line in place of the generic text, so that whoever reads the `` block (a model included) learns why and what to do instead. + +Any other answer denies, whether it is truthy or not, and reports the generic reason. That includes an object that carries a `reason` without saying `allow: false`, such as `{ allow: true, reason: '...' }`: its reason is not reported. + +The reason is sanitized before it is stored, the way the rest of a violation line is. Each run of control characters (line breaks and tabs included) or of invisible ones (zero-width characters, the joiner among them, and bidi controls) becomes one space; `<` and `>` are removed, so `re-run with listed` is stored as `re-run with host listed`; the ends are trimmed. The result is cut to 500 characters, counted as UTF-16 code units the way `String.prototype.length` counts them. A reason with nothing left after that falls back to `user denied`. Write it as one line of plain text. + +**Behaviour change in the first release after v0.0.77:** the filter used to allow on any truthy answer, so a callback that resolved to a truthy value other than `true` (`1`, a string, any object) allowed the connection. It now denies. For the same reason, `{ allow: false, reason }` returned to an older release would be read there as an allow, because an object is truthy. `SandboxManager.askCallbackDenyReason` is `true` on a release that understands the object: check it before returning one, and deny with a plain `false` where it is absent. + +```typescript +const deny = (reason: string) => + SandboxManager.askCallbackDenyReason + ? { allow: false as const, reason } + : false + +await SandboxManager.initialize(config, async ({ host, port }) => { + if (await userApproves(host, port)) return true + return deny(`${host} was not approved for this session`) +}) +``` + +**Per-command network allow lists (`registerCommandNetworkLists`).** One invocation can carry an allow list of its own, in addition to the configured one, for as long as it runs: + +```typescript +import { randomBytes } from 'node:crypto' + +// 128 bits of randomness, 22 characters. See below for why nothing less will do. +const commandId = randomBytes(16).toString('base64url') +const wrapped = await SandboxManager.wrapWithSandbox( + command, + undefined, + undefined, + undefined, + { commandId }, +) + +SandboxManager.registerCommandNetworkLists(commandId, { + allowedDomains: ['registry.example.org', '*.cdn.example.org:443'], +}) +const child = spawn(wrapped, { shell: true }) +const unregister = () => SandboxManager.unregisterCommandNetworkLists(commandId) +child.once('exit', unregister) +child.once('error', unregister) // the child never started +``` + +Entries use the grammar and the matcher of `network.allowedDomains` and get the same validation. `registerCommandNetworkLists` throws on an invalid entry, and on a `commandId` shorter than 22 characters. Registering an id again replaces its list; `unregisterCommandNetworkLists` is a no-op for an id that has none; `reset()` removes every registration. + +The order of evaluation for a connection is: + +1. a `network.deniedDomains` match denies; +2. a `network.allowedDomains` match allows; +3. `network.strictAllowlist` denies; +4. a match in the list registered for the id the connection presents allows; +5. the ask callback decides if there is one, and otherwise the connection is denied. + +So a per-command entry never overrides a configured `deniedDomains` entry, and is ignored entirely under `strictAllowlist`. It never enters `network.*` either: `getConfig()` and `getNetworkRestrictionConfig()` do not show it, and the default `injectHosts` scope of a masked credential, which is `network.allowedDomains`, does not grow. A host allowed this way still goes through the resolved-address check when it is dialed, and an IP literal in a per-command list adds no exemption there. + +What the id has to be, and what this feature is not: the proxy learns which invocation a connection belongs to from the proxy username, and the username is presented by the client inside the sandbox. The id is therefore the **only** thing binding a connection to an allow list. It **must** be unguessable: at least 128 bits of randomness, never a counter, a timestamp or anything derived from the command. A sandboxed process that presents another live invocation's id gets that invocation's allows, so this is attribution, not a boundary between concurrent commands of one session. Register just before spawning the wrapped command, and unregister when the child exits or never started: a registration that outlives its command widens the window in which its id is worth presenting. As with every attribution key, only the first 100 characters of an id take part. + #### Available exports ```typescript From 214bd8d242b04126c27d9b52c359afc7d6edd55a Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 28 Sep 2026 14:01:39 +0000 Subject: [PATCH 4/5] Shorten the comments this change adds Comments only: the JavaScript emitted with comments removed is byte-identical for every file, and the test names are unchanged. Kept: every invariant and what could be done without it, the documentation of exported names, and the reasons for what is not done the obvious way. Cut: how the code came to be as it is, narration of the lines that follow, and explanations repeated at several sites. --- src/sandbox/domain-pattern.ts | 10 +-- src/sandbox/sandbox-config.ts | 4 +- src/sandbox/sandbox-manager.ts | 103 +++++++++---------------- src/sandbox/sandbox-schemas.ts | 42 ++++------ src/sandbox/sandbox-utils.ts | 20 ++--- src/sandbox/sandbox-violation-store.ts | 15 ++-- test/sandbox/update-config.test.ts | 42 ++++------ 7 files changed, 87 insertions(+), 149 deletions(-) diff --git a/src/sandbox/domain-pattern.ts b/src/sandbox/domain-pattern.ts index 9b9c8a96a..502c5f5ea 100644 --- a/src/sandbox/domain-pattern.ts +++ b/src/sandbox/domain-pattern.ts @@ -4,9 +4,8 @@ * Lives in its own module so the schema can import it without pulling * in sandbox-manager (which imports the schema — circular). * - * The validity check for a list entry lives here too, for the converse - * reason: the manager applies it to lists registered at run time, and gets it - * from here without loading the schema module. + * The validity check for a list entry lives here too, so the manager can + * apply it to lists registered at run time without loading the schema module. */ import { isIP } from 'node:net' @@ -140,9 +139,8 @@ export function hasValidIpv6Bracketing(val: string): boolean { /** * The check an allow-list entry gets: a domain pattern with an optional - * `:port` suffix. One function for both places such an entry arrives (the - * `network.allowedDomains` schema, and a per-command list registered with the - * manager at run time), so neither accepts what the other refuses. + * `:port` suffix. Shared by the `network.allowedDomains` schema and the lists + * registered at run time, so neither accepts what the other refuses. */ export function isValidAllowedDomainEntry(val: string): boolean { return ( diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index a36cf927b..bca9d5be3 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -33,9 +33,7 @@ const domainPatternSchema = z * Domain pattern with an optional `:port` suffix (e.g., "example.com:443", * "*.npmjs.org:8443"). Used for allowedDomains / deniedDomains, where the * proxy knows the destination port; an entry without a port matches any port. - * The check and its message live in domain-pattern.ts so the manager can hold - * an allow list registered at run time to the same rule without loading this - * module's schemas. + * The check and its message live in domain-pattern.ts, which says why. */ const domainPortPatternSchema = z .string() diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 0b7d43c00..0d4c25bed 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -256,22 +256,18 @@ export function resolveCommandText(decodedKey: string): string { /** * The shortest commandId a per-command allow list may be registered under: - * 16 random bytes (128 bits) written as unpadded base64url. A length floor - * cannot tell a random id from a predictable one of the same length: a - * formatted date or a time-based UUID is longer than this and passes. All it - * does is turn away what is too short to hold 128 bits however it was made, - * such as a small counter, an epoch timestamp or a short label. + * 16 random bytes (128 bits) as unpadded base64url. The floor only turns away + * what is too short to hold 128 bits: it cannot tell a random id from a + * predictable one of the same length. */ const MIN_NETWORK_LISTS_COMMAND_ID_LENGTH = 22 /** * Attribution key to the network allow list registered for one invocation * (registerCommandNetworkLists), keyed the way commandTextsByKey is because - * the same carrier, the proxy username, delivers the key. filterNetworkRequest - * consults the list for a connection whose username names the key. The - * embedder removes an entry when its command is gone and reset() clears them - * all. Not bounded the way commandTextsByKey is: dropping the oldest entry - * would silently take a list away from a command that is still running. + * the proxy username delivers both. The embedder removes an entry when its + * command is gone. Not bounded the way commandTextsByKey is: dropping the + * oldest entry would silently take a list away from a running command. */ const commandNetworkListsByKey = new Map() @@ -301,8 +297,6 @@ function registerCommandNetworkLists( ) } for (const entry of allowedDomains) { - // The rule network.allowedDomains entries are held to, so a list that - // arrives at run time cannot carry a pattern the configuration refuses. if (typeof entry !== 'string' || !isValidAllowedDomainEntry(entry)) { throw new Error( `registerCommandNetworkLists: invalid allowedDomains entry ` + @@ -312,8 +306,7 @@ function registerCommandNetworkLists( } commandNetworkListsByKey.set( decodeSandboxedCommand(encodeSandboxedCommand(commandId)), - // Copied, so a caller that goes on to change its array does not change - // what was validated. + // Copied, so the caller cannot change what was validated. { allowedDomains: [...allowedDomains] }, ) } @@ -414,14 +407,10 @@ function isDeclaredDenial(answer: unknown): boolean { } /** - * The reason an ask-callback answer gives for denying, or undefined when it - * gives none that can be shown. Only the documented `{ allow: false, reason }` - * supplies one. An object that carries a reason without saying allow: false - * (`{ allow: true, reason }`, say) contradicts itself: it denies like every - * answer that is not `true`, but its text was not written as the explanation - * of a denial, so the generic reason is reported instead. Sanitized and cut: - * it is caller-supplied text on its way into a violation line, which a model - * reads. + * The reason an ask-callback answer gives for denying, sanitized, or + * undefined when it gives none that can be shown. Only + * `{ allow: false, reason }` supplies one: a reason on any other object was + * not written as the explanation of a denial. */ function denialReasonOf(answer: unknown): string | undefined { if (typeof answer !== 'object' || answer === null) return undefined @@ -486,20 +475,17 @@ async function filterNetworkRequest( } } - // strictAllowlist makes the configured allowlist deterministic - // enforcement: nothing below this point, neither a per-command allow list - // nor the callback, can widen it. + // strictAllowlist makes the configured allowlist deterministic enforcement: + // neither a per-command allow list nor the callback below can widen it. if (config.network.strictAllowlist) { logForDebugging(`No matching config rule, denying: ${host}:${port}`) return denied('host is not on the allow list') } - // The allow list registered for the invocation this connection's proxy - // username names, if there is one. It comes after both configured lists, so - // it lifts the default deny for that one command and never a configured - // deniedDomains entry. The username is presented by the client inside the - // sandbox; see registerCommandNetworkLists for what that does and does not - // guarantee. + // The allow list registered for the invocation the proxy username names. + // Coming after both configured lists, it lifts the default deny for that + // one command and never a configured deniedDomains entry. The username is + // presented by the sandboxed client: see registerCommandNetworkLists. const perCommand = encodedCommand ? commandNetworkListsByKey.get(decodeSandboxedCommand(encodedCommand)) : undefined @@ -510,7 +496,6 @@ async function filterNetworkRequest( } } - // No matching rules - ask user or deny. if (!sandboxAskCallback) { logForDebugging(`No matching rule, denying: ${host}:${port}`) return denied('host is not on the allow list') @@ -518,12 +503,10 @@ async function filterNetworkRequest( logForDebugging(`No matching rule, asking user: ${host}:${port}`) try { - // Held as unknown: the declared type is what a TypeScript caller can - // return, not what a JavaScript one does. + // Held as unknown: a JavaScript caller is not bound by the declared type. const answer: unknown = await sandboxAskCallback({ host, port }) - // Only `true` allows. An object is how a callback denies with a reason, - // and an object is truthy, so a truthiness test here would read every - // such denial as an allow. + // Only `true` allows: an object is how a callback denies with a reason, + // and a truthiness test here would read every such denial as an allow. if (answer === true) { logForDebugging(`User allowed: ${host}:${port}`) return true @@ -535,10 +518,7 @@ async function filterNetworkRequest( ) } logForDebugging(`User denied: ${host}:${port}`) - // The callback's own reason when it gave one, so whoever reads the - // violation line learns why and what to do instead; a bare `false`, and - // an answer that is neither of the documented denials, keep the generic - // text. + // The callback's own reason, when it gave one, says what to do instead. return denied(denialReasonOf(answer) ?? 'user denied') } catch (error) { logForDebugging(`Error in permission callback: ${error}`, { @@ -1760,9 +1740,8 @@ export type WrapWithSandboxOptions = { * long commands sharing a prefix would otherwise cross-attribute, and a * rerun of the same text would inherit the earlier run's events. * - * An invocation that also registers a network allow list under this id - * (`registerCommandNetworkLists`) needs more than uniqueness: there the id - * must be unguessable, for the reasons given on that method. + * An id that a network allow list is registered under must also be + * unguessable: see `registerCommandNetworkLists`. */ commandId?: string /** @@ -2606,10 +2585,9 @@ export interface ISandboxManager { ): Promise<{ argv: string[]; env: NodeJS.ProcessEnv }> /** * Register the network allow list of one invocation, under the `commandId` - * its wrap was given ({@link WrapWithSandboxOptions}). Entries use the - * grammar and the matcher of `network.allowedDomains` and get the same - * validation; an invalid entry throws. Registering an id again replaces - * its list. + * its wrap was given ({@link WrapWithSandboxOptions}). Entries are matched + * and validated as `network.allowedDomains` entries are; an invalid one + * throws. Registering an id again replaces its list. * * Order of evaluation for a connection: a configured `deniedDomains` match * denies; a configured `allowedDomains` match allows; @@ -2618,27 +2596,23 @@ export interface ISandboxManager { * there is one, and otherwise the connection is denied. So a per-command * entry never overrides a configured `deniedDomains` entry and is ignored * entirely under `strictAllowlist`. It also never enters `network.*`, so - * the default `injectHosts` scope of a masked credential, which is - * `network.allowedDomains`, does not grow. + * the default `injectHosts` scope of a masked credential does not grow. * - * What binds a connection to a list: the proxy reads the id from the proxy - * username, and the username is presented by the client inside the - * sandbox. The id is therefore the ONLY thing binding a connection to an + * The proxy reads the id from the proxy username, which the sandboxed + * client presents, so the id is the ONLY thing binding a connection to an * allow list. It MUST be unguessable: at least 128 bits of randomness (16 - * random bytes as base64url come to 22 characters), never a counter, a - * timestamp or anything derived from the command. An id shorter than 22 - * characters throws. A sandboxed process that presents another live - * invocation's id gets that invocation's allows, so this is attribution, - * not a boundary between concurrent commands of one session. + * random bytes as base64url, 22 characters; a shorter id throws), never a + * counter, a timestamp or anything derived from the command. Presenting + * another live invocation's id gets that invocation's allows: this is + * attribution, not a boundary between concurrent commands of one session. * * Register just before spawning the wrapped command, and unregister when * the child exits or never started: a registration that outlives its * command widens the window in which its id is worth presenting. * * As with every attribution key, only the first 100 characters of an id - * take part, and an id whose encoded form does not fit in the proxy - * username (possible only with non-ASCII ids) is carried by no connection, - * so its list never applies. + * take part, and a list under an id whose encoded form does not fit in the + * proxy username (possible only with non-ASCII ids) never applies. */ registerCommandNetworkLists( commandId: string, @@ -2652,10 +2626,9 @@ export interface ISandboxManager { /** * True when the network filter understands an ask callback's * `{ allow: false, reason }` answer (see {@link SandboxAskCallback}). Read - * it before returning that object: releases up to v0.0.77 allowed a - * connection on any truthy answer, and an object is truthy, so there the - * same answer would be read as an allow. Where this is absent, deny with a - * plain `false`. + * it before returning that object: releases up to v0.0.77 allow on any + * truthy answer, so there the object would be read as an allow. Where this + * is absent, deny with a plain `false`. */ readonly askCallbackDenyReason: true getSandboxViolationStore(): SandboxViolationStore diff --git a/src/sandbox/sandbox-schemas.ts b/src/sandbox/sandbox-schemas.ts index 3ce30b647..f0b93e969 100644 --- a/src/sandbox/sandbox-schemas.ts +++ b/src/sandbox/sandbox-schemas.ts @@ -92,14 +92,12 @@ export interface CredentialRestrictionConfig { * Note: Empty `allowedHosts` means no host matches an allow rule (unlike * read's empty denyOnly). Whether an unmatched host is denied outright * depends on what else may decide it: deniedHosts are checked first and deny - * unconditionally; under `network.strictAllowlist` a host matching neither - * list is denied there and then; otherwise it is allowed when the allow list - * registered for the invocation the connection presents - * (`SandboxManager.registerCommandNetworkLists`) matches it, and failing - * that falls through to the registered SandboxAskCallback when one exists, - * and is denied when no callback is registered. Hosts needing a hard - * block-all regardless of callback behavior should use a `deniedHosts` - * wildcard. + * unconditionally; a host matching neither list is denied under + * `network.strictAllowlist`, else allowed by a matching per-command list + * (`SandboxManager.registerCommandNetworkLists`), else falls through to the + * registered SandboxAskCallback when one exists, and is denied when + * no callback is registered. Hosts needing a hard block-all regardless of + * callback behavior should use a `deniedHosts` wildcard. * * Entries are the raw config patterns and may carry an optional `:port` * suffix (`api.example.com:443`, `*:22`) meaning "this rule applies only @@ -119,25 +117,19 @@ export type NetworkHostPattern = { /** * Asked about a host that no configured rule and no per-command allow list - * decided (never asked under `network.strictAllowlist`). Only the value - * `true` allows the connection. `false` denies it with the generic reason - * "user denied". An object `{ allow: false, reason }` denies it with that - * reason, which is what the violation line for the connection reports. Any - * other answer, truthy or not, denies with the generic reason: a `reason` on - * an object that does not say `allow: false` is not reported. + * decided (never asked under `network.strictAllowlist`). Only `true` allows. + * `{ allow: false, reason }` denies with that reason, which the violation + * line reports. `false` and every other answer, truthy or not, deny with the + * generic reason "user denied": a `reason` on an object that does not say + * `allow: false` is not reported. * - * The reason is sanitized before it is stored, the way the rest of a - * violation line is: each run of control characters (line breaks and tabs - * included) or of invisible ones (zero-width characters, the joiner among - * them, and bidi controls) becomes one space, `<` and `>` are removed, and - * the ends are trimmed. It is then cut to 500 UTF-16 code units, what - * `String.prototype.length` counts. A reason with nothing left after that - * falls back to "user denied". So write it as one line of plain text, and do - * not rely on angle brackets around a placeholder. + * The reason is sanitized like the rest of a violation line (runs of control + * or invisible characters become one space, `<` and `>` are removed, the + * ends are trimmed) and cut to 500 UTF-16 code units, so write it as one + * line of plain text. With nothing left, "user denied" is reported. * - * Releases up to v0.0.77 allowed on any truthy answer, so an object answer - * returned to one of those would be read as an allow. Check - * `SandboxManager.askCallbackDenyReason` before returning one. + * Check `SandboxManager.askCallbackDenyReason` before returning an object: + * a release without it reads any truthy answer as an allow. */ export type SandboxAskCallback = ( params: NetworkHostPattern, diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 3aa84dc8c..9ca9b7b65 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1056,19 +1056,13 @@ export function proxyUsernameFor(encodedCommand: string | undefined): string { * Inverse of {@link proxyUsernameFor}: extract the encodedCommand suffix * from `srt.`, or undefined for bare `srt` / anything else. * The username is presented by the client inside the sandbox. A forged - * suffix cannot authenticate (the token does that), but it decides two - * things: which invocation a denial is attributed to in the violation - * report, and which invocation's registered network allow list - * (`SandboxManager.registerCommandNetworkLists`) applies to the connection. - * The suffix is the ONLY thing binding a connection to such a list, so an id - * a list is registered under MUST be unguessable (at least 128 bits of - * randomness, never a counter, a timestamp or anything derived from the - * command), and even then a sandboxed process that presents another live - * invocation's id gets that invocation's allows: this is attribution, not a - * boundary between concurrent commands of one session. A suffix past - * {@link MAX_ENCODED_COMMAND_BYTES} is longer than this process can mint, so - * it is dropped rather than stored: the denial is still recorded, - * unattributed, and no registered list applies. + * suffix cannot authenticate (the token does that), but it decides which + * invocation a denial is attributed to in the violation report and which + * registered network allow list applies to the connection: see + * `SandboxManager.registerCommandNetworkLists` for what that asks of an id. A + * suffix past {@link MAX_ENCODED_COMMAND_BYTES} is longer than this process + * can mint, so it is dropped rather than stored: the denial is still + * recorded, unattributed, and no registered list applies. */ export function encodedCommandFromProxyUser( username: string | undefined, diff --git a/src/sandbox/sandbox-violation-store.ts b/src/sandbox/sandbox-violation-store.ts index 5be42b880..eccda8e8f 100644 --- a/src/sandbox/sandbox-violation-store.ts +++ b/src/sandbox/sandbox-violation-store.ts @@ -151,21 +151,16 @@ export function sanitizeUnregisteredCommandKey(decodedKey: string): string { /** * How much of a denial reason supplied by the ask callback a violation line - * carries, in UTF-16 code units (what `String.prototype.length` counts, so a - * character outside the Basic Multilingual Plane counts as two). The reason - * is prose for whoever reads the violation, a model included: a few sentences - * fit, a document does not. + * carries, in UTF-16 code units: a few sentences fit, a document does not. */ export const MAX_DENIAL_REASON_LENGTH = 500 /** * A denial reason the ask callback supplied, as a violation line may carry - * it: sanitized like the rest of the line, and cut so that one answer cannot - * flood the text a model reads. The store sanitizes the whole line again at - * ingestion; doing it here first means the cut counts what is displayed. The - * cut can land just after a space, and the reason sits inside parentheses - * where the store's own trim of the line's ends does not reach, so the end is - * trimmed again here. + * it: sanitized first, so the cut counts what is displayed, then cut so that + * one answer cannot flood the text a model reads. The end is trimmed after + * the cut: the reason sits inside parentheses, where the store's own trim of + * the line's ends does not reach. */ export function sanitizeDenialReason(reason: string): string { return cutBetweenCharacters( diff --git a/test/sandbox/update-config.test.ts b/test/sandbox/update-config.test.ts index 8ac76c53d..c7c55ca12 100644 --- a/test/sandbox/update-config.test.ts +++ b/test/sandbox/update-config.test.ts @@ -434,9 +434,8 @@ function connectAs( /** * A listener on loopback for the proxy to dial, so an allow is observed as a - * 200 without leaving the machine. An IP-literal destination is dialled as - * it is, with no name to resolve and nothing for the resolved-address check - * to refuse. + * 200 without leaving the machine: an IP literal has no name to resolve and + * nothing for the resolved-address check to refuse. */ function listenOnLoopback(): Promise<{ server: Server; target: string }> { return new Promise(resolve => { @@ -506,8 +505,7 @@ describe('per-command network allow lists', () => { expect( await connectAs(port, loopback.target, usernameFor(registered)), ).toBe(200) - // An id nothing was registered under, and no id at all, see the - // configured lists alone. + // Any other id, and no id at all, see the configured lists alone. expect( await connectAs(port, loopback.target, usernameFor(randomCommandId())), ).toBe(403) @@ -541,8 +539,7 @@ describe('per-command network allow lists', () => { const port = await start({}, denyAndRecord) const registered = randomCommandId() const [host, listeningPort] = loopback.target.split(':') - // Any port but the listener's, and still a valid one: the listener can be - // handed 65535, the top of the range. + // Any valid port but the listener's, which can itself be 65535. const anotherPort = Number(listeningPort) === 65535 ? 65534 : Number(listeningPort) + 1 SandboxManager.registerCommandNetworkLists(registered, { @@ -619,9 +616,7 @@ describe('per-command network allow lists', () => { }) SandboxManager.unregisterCommandNetworkLists(randomCommandId()) - // Registering refuses what is not a string; unregistering has nothing to - // refuse, since no list can be registered under it, and lets it pass so - // that a cleanup path never throws. + // Unregistering lets a non-string pass, so a cleanup path never throws. for (const notAString of [null, undefined, 42]) { expect(() => SandboxManager.unregisterCommandNetworkLists( @@ -734,9 +729,8 @@ describe('per-command network allow lists', () => { expect(message).not.toContain(almost) }) - // Accepted and refused by one predicate, so the two can only agree; this - // pins that a list arriving at run time cannot carry an entry the - // configuration would have refused, and the other way round. + // A list arriving at run time cannot carry an entry the configuration + // would have refused, and the other way round. it.each([ ['example.com', true], ['*.example.com', true], @@ -806,9 +800,8 @@ describe('per-command network allow lists', () => { }) // An id is normalised before it keys the map, and what the proxy receives - // is decoded rather than compared as bytes. Each case here is an id or a - // username that differs from the registered one byte for byte and reaches - // its list all the same, or looks alike and does not. + // is decoded rather than compared as bytes, so an id can differ from the + // registered one byte for byte and reach its list all the same. describe('ids that differ byte for byte from the registered one', () => { it('share a list when they agree on their first 100 characters', async () => { const port = await start({}) @@ -818,8 +811,7 @@ describe('per-command network allow lists', () => { allowedDomains: [loopback.target], }) - // A wrap given this other id mints the same username: the key is cut - // to 100 characters before it is encoded. + // Both ids mint one username: the key is cut to 100 characters. expect(usernameFor(`${prefix}-second`)).toBe( usernameFor(`${prefix}-first`), ) @@ -827,8 +819,7 @@ describe('per-command network allow lists', () => { await connectAs(port, loopback.target, usernameFor(`${prefix}-second`)), ).toBe(200) - // A username carrying all 105 characters was not minted here; it - // decodes to a longer key, which nothing is registered under. + // An uncut username decodes to a longer key, which has no list. const uncut = Buffer.from(`${prefix}-first`).toString('base64') expect(await connectAs(port, loopback.target, `srt.${uncut}`)).toBe(403) @@ -976,7 +967,7 @@ describe('ask callback answers', () => { }) }) - // Every one of these was an allow while the filter tested truthiness. + // A filter that tested truthiness would allow every one of these. it.each([ ['the number 1', 1], ['a non-empty string', 'yes'], @@ -984,8 +975,7 @@ describe('ask callback answers', () => { ['an empty array', []], ['an object that says allow: true', { allow: true }], ['the string "true"', 'true'], - // A reason is reported only for an answer that says allow: false. One - // that says otherwise was not written as the explanation of a denial. + // A reason is reported only for an answer that says allow: false. [ 'an object that says allow: true and gives a reason', { allow: true, reason: 'approved for this session' }, @@ -1083,8 +1073,7 @@ describe('ask callback answers', () => { }) it('counts the cut in what is displayed, not in what was stripped', async () => { - // 600 control characters collapse to one space, so nothing is cut off - // the text that follows them. + // 600 control characters collapse to one space, so `after` is not cut. const { lines } = await decideWith({ allow: false, reason: `before${'\x07'.repeat(600)}after`, @@ -1109,8 +1098,7 @@ describe('ask callback answers', () => { expect(violationLines()).toEqual(deniedBecause('permission prompt failed')) }) - // The debug log is where an embedder caught by the change from "any truthy - // answer allows" finds out why its connections are now refused. + // The debug log tells an embedder why a truthy answer did not allow. it('warns in the debug log about an answer that is neither a boolean nor { allow: false }, and about no other', async () => { const before = process.env.SRT_DEBUG process.env.SRT_DEBUG = '1' From 8cc4b19f3e634f63d7edaf987ddcc3893b232714 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 28 Sep 2026 17:17:14 +0000 Subject: [PATCH 5/5] Per-command network lists: say where none applies, and test SOCKS and a real wrap Under an external `network.httpProxyPort` the wrap hands the proxy no username, so no connection names an id and a registered list never applies. It fails closed; the README and the doc comment now say so, and registering in that state logs one debug line. The 22-character floor is measured on the key the map holds, with U+FFFD left out: every unpaired surrogate becomes one, so an id made of them passed the floor while carrying nothing. New tests: a list applies over SOCKS5 as over CONNECT, and it reaches a sandboxed curl through the commandId its wrap was given. --- README.md | 6 +- src/sandbox/sandbox-manager.ts | 32 ++++-- test/sandbox/update-config.test.ts | 157 ++++++++++++++++++++++++++++- 3 files changed, 184 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index f1cd04b50..023159250 100644 --- a/README.md +++ b/README.md @@ -328,7 +328,7 @@ child.once('exit', unregister) child.once('error', unregister) // the child never started ``` -Entries use the grammar and the matcher of `network.allowedDomains` and get the same validation. `registerCommandNetworkLists` throws on an invalid entry, and on a `commandId` shorter than 22 characters. Registering an id again replaces its list; `unregisterCommandNetworkLists` is a no-op for an id that has none; `reset()` removes every registration. +Entries use the grammar and the matcher of `network.allowedDomains` and get the same validation. `registerCommandNetworkLists` throws on an invalid entry, and on a `commandId` shorter than 22 characters (an unpaired surrogate does not count). Registering an id again replaces its list; `unregisterCommandNetworkLists` is a no-op for an id that has none; `reset()` removes every registration. The order of evaluation for a connection is: @@ -340,7 +340,9 @@ The order of evaluation for a connection is: So a per-command entry never overrides a configured `deniedDomains` entry, and is ignored entirely under `strictAllowlist`. It never enters `network.*` either: `getConfig()` and `getNetworkRestrictionConfig()` do not show it, and the default `injectHosts` scope of a masked credential, which is `network.allowedDomains`, does not grow. A host allowed this way still goes through the resolved-address check when it is dialed, and an IP literal in a per-command list adds no exemption there. -What the id has to be, and what this feature is not: the proxy learns which invocation a connection belongs to from the proxy username, and the username is presented by the client inside the sandbox. The id is therefore the **only** thing binding a connection to an allow list. It **must** be unguessable: at least 128 bits of randomness, never a counter, a timestamp or anything derived from the command. A sandboxed process that presents another live invocation's id gets that invocation's allows, so this is attribution, not a boundary between concurrent commands of one session. Register just before spawning the wrapped command, and unregister when the child exits or never started: a registration that outlives its command widens the window in which its id is worth presenting. As with every attribution key, only the first 100 characters of an id take part. +What the id has to be, and what this feature is not: the proxy learns which invocation a connection belongs to from the proxy username, and the username is presented by the client inside the sandbox. The id is therefore the **only** thing binding a connection to an allow list. It **must** be unguessable: at least 128 bits of randomness, never a counter, a timestamp or anything derived from the command. A sandboxed process that presents another live invocation's id gets that invocation's allows, so this is attribution, not a boundary between concurrent commands of one session. Register just before spawning the wrapped command, and unregister when the child exits or never started: a registration that outlives its command widens the window in which its id is worth presenting. As with every attribution key, only the first 100 characters of an id take part. Keep ids ASCII: a list under a non-ASCII id whose encoded form does not fit in the proxy username (255 bytes) registers and never applies. + +No list applies while `network.httpProxyPort` names an external proxy: the wrap gives that proxy no username, so no connection carries an id. `registerCommandNetworkLists` still succeeds (and says so in one line under `SRT_DEBUG`), and every connection is decided as if no list existed. #### Available exports diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 0d4c25bed..b2b1112c3 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -275,15 +275,19 @@ function registerCommandNetworkLists( commandId: string, lists: { allowedDomains?: string[] }, ): void { + // The floor is measured on the key the map holds, not on the raw id, and + // less U+FFFD: every unpaired surrogate becomes one, so it carries nothing. + const key = + typeof commandId === 'string' + ? decodeSandboxedCommand(encodeSandboxedCommand(commandId)) + : '' // The id is not echoed in either message: it is what stands between a // sandboxed process and this list, and error text ends up in logs. - if ( - typeof commandId !== 'string' || - commandId.length < MIN_NETWORK_LISTS_COMMAND_ID_LENGTH - ) { + if (key.replace(/\ufffd/g, '').length < MIN_NETWORK_LISTS_COMMAND_ID_LENGTH) { throw new Error( `registerCommandNetworkLists: commandId must be a string of at least ` + - `${MIN_NETWORK_LISTS_COMMAND_ID_LENGTH} characters. The id is the ` + + `${MIN_NETWORK_LISTS_COMMAND_ID_LENGTH} characters, unpaired ` + + `surrogates not counted. The id is the ` + `only thing that binds a proxy connection to this allow list, and ` + `the sandboxed process chooses which id it presents, so it must be ` + `random (at least 128 bits, e.g. 16 random bytes as base64url) and ` + @@ -305,10 +309,18 @@ function registerCommandNetworkLists( } } commandNetworkListsByKey.set( - decodeSandboxedCommand(encodeSandboxedCommand(commandId)), + key, // Copied, so the caller cannot change what was validated. { allowedDomains: [...allowedDomains] }, ) + // An external HTTP proxy is handed no username, so no connection names an id. + if (config?.network.httpProxyPort !== undefined) { + logForDebugging( + 'registerCommandNetworkLists: network.httpProxyPort is set (an ' + + 'external proxy), so this list will never apply', + { level: 'warn' }, + ) + } } function unregisterCommandNetworkLists(commandId: string): void { @@ -2612,7 +2624,13 @@ export interface ISandboxManager { * * As with every attribution key, only the first 100 characters of an id * take part, and a list under an id whose encoded form does not fit in the - * proxy username (possible only with non-ASCII ids) never applies. + * proxy username (possible only with non-ASCII ids) never applies. An + * unpaired surrogate does not count towards the 22 characters. + * + * No list applies while `network.httpProxyPort` names an external proxy: + * the wrap gives that proxy no username, so no connection carries an id. + * Registering still succeeds, and every connection is decided as if no list + * existed. */ registerCommandNetworkLists( commandId: string, diff --git a/test/sandbox/update-config.test.ts b/test/sandbox/update-config.test.ts index c7c55ca12..fc3ef856f 100644 --- a/test/sandbox/update-config.test.ts +++ b/test/sandbox/update-config.test.ts @@ -432,14 +432,58 @@ function connectAs( }) } +/** + * The same over SOCKS5: the reply code to a CONNECT for `target` under + * `username` (0 is granted, 2 is refused by the rules). + */ +function socksConnectAs( + proxyPort: number, + target: string, + username: string, +): Promise { + return new Promise(resolve => { + const [host, port] = target.split(':') as [string, string] + const field = (text: string): Buffer => + Buffer.concat([Buffer.from([Buffer.byteLength(text)]), Buffer.from(text)]) + const socket = connect(proxyPort, '127.0.0.1', () => { + socket.write( + Buffer.concat([ + // Offer username/password only, authenticate, then CONNECT by name. + Buffer.from([0x05, 0x01, 0x02, 0x01]), + field(username), + field(SandboxManager.getProxyAuthToken() ?? ''), + Buffer.from([0x05, 0x01, 0x00, 0x03]), + field(host), + Buffer.from([Number(port) >> 8, Number(port) & 0xff]), + ]), + ) + }) + let reply = Buffer.alloc(0) + socket.on('data', chunk => { + reply = Buffer.concat([reply, chunk]) + // Method selection (2 bytes), auth status (2), then VER and REP. + if (reply.length >= 6) socket.destroy() + }) + socket.on('close', () => resolve(reply[5])) + socket.on('error', () => {}) + socket.setTimeout(2000, () => socket.destroy()) + }) +} + /** * A listener on loopback for the proxy to dial, so an allow is observed as a * 200 without leaving the machine: an IP literal has no name to resolve and - * nothing for the resolved-address check to refuse. + * nothing for the resolved-address check to refuse. It answers a request with + * the body `ok`. */ function listenOnLoopback(): Promise<{ server: Server; target: string }> { return new Promise(resolve => { - const server = createServer(socket => socket.on('error', () => {})) + const server = createServer(socket => { + socket.on('error', () => {}) + socket.once('data', () => + socket.end('HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok'), + ) + }) server.listen(0, '127.0.0.1', () => { const { port } = server.address() as AddressInfo resolve({ server, target: `127.0.0.1:${port}` }) @@ -516,6 +560,56 @@ describe('per-command network allow lists', () => { ]) }) + it('applies over SOCKS5 as it does over CONNECT', async () => { + await start({}) + const port = SandboxManager.getSocksProxyPort()! + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect( + await socksConnectAs(port, loopback.target, usernameFor(registered)), + ).toBe(0) + expect( + await socksConnectAs( + port, + loopback.target, + usernameFor(randomCommandId()), + ), + ).toBe(2) + }) + + // The one case where the wrap, not usernameFor(), mints the username. + it.if(isMacOS || isLinux)( + 'reaches a sandboxed command through the commandId its wrap was given', + async () => { + await start({}) + const curlAs = async (commandId: string): Promise => { + const wrapped = await SandboxManager.wrapWithSandbox( + // NO_PROXY in the sandbox exempts loopback; this goes to the proxy. + `curl -s --noproxy '' --max-time 3 http://${loopback.target}/`, + undefined, + undefined, + undefined, + { commandId }, + ) + return (await spawnAsync('bash', ['-c', wrapped])).stdout + } + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + + expect(await curlAs(registered)).toBe('ok') + expect(await curlAs(randomCommandId())).not.toBe('ok') + expect(violationLines()).toEqual([ + `deny network-outbound ${loopback.target} (host is not on the allow list)`, + ]) + }, + 30000, + ) + it('is consulted before the ask callback, which still decides every host it does not list', async () => { const port = await start({}, denyAndRecord) const registered = randomCommandId() @@ -679,6 +773,48 @@ describe('per-command network allow lists', () => { ).toBe(403) }) + it('registers under an external network.httpProxyPort, where no list applies, and says so in the debug log', async () => { + const before = process.env.SRT_DEBUG + process.env.SRT_DEBUG = '1' + const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}) + const errorSpy = spyOn(console, 'error').mockImplementation(() => {}) + const warnings = (): string[] => + warnSpy.mock.calls + .map(call => String(call[0])) + .filter(message => message.includes('registerCommandNetworkLists')) + try { + await start({}) + SandboxManager.registerCommandNetworkLists(randomCommandId(), {}) + expect(warnings()).toEqual([]) + + await SandboxManager.reset() + await start({ httpProxyPort: 8888 }) + const registered = randomCommandId() + SandboxManager.registerCommandNetworkLists(registered, { + allowedDomains: [loopback.target], + }) + expect(warnings()).toHaveLength(1) + expect(warnings()[0]).toContain('this list will never apply') + expect(warnings()[0]).not.toContain(registered) + // The library's own port still serves SOCKS and CONNECT, and reads no id. + expect( + await connectAs( + SandboxManager.getSocksProxyPort()!, + loopback.target, + usernameFor(registered), + ), + ).toBe(403) + } finally { + warnSpy.mockRestore() + errorSpy.mockRestore() + if (before === undefined) { + delete process.env.SRT_DEBUG + } else { + process.env.SRT_DEBUG = before + } + } + }) + it('refuses an id shorter than 22 characters, and says what an id has to be', () => { const lists = { allowedDomains: ['example.com'] } for (const tooShort of ['', '1', 'inv-0001', 'a'.repeat(21)]) { @@ -729,6 +865,23 @@ describe('per-command network allow lists', () => { expect(message).not.toContain(almost) }) + it('measures the floor on the key it stores, where an unpaired surrogate counts for nothing', () => { + // Each is 22 characters long, and is stored with U+FFFD in place of every + // unpaired surrogate. U+FFFD is not counted. + for (const hollow of [ + '\ud800'.repeat(22), + `${'a'.repeat(21)}\udc00`, + '\ufffd'.repeat(22), + ]) { + expect(hollow).toHaveLength(22) + expect(() => + SandboxManager.registerCommandNetworkLists(hollow, {}), + ).toThrow(/at least 22 characters, unpaired surrogates not counted/) + } + // A pair is a character like any other, counted as String.length does. + SandboxManager.registerCommandNetworkLists('\u{1f600}'.repeat(11), {}) + }) + // A list arriving at run time cannot carry an entry the configuration // would have refused, and the other way round. it.each([