From 8e5ab571d5267970082fd249610cff70a4b641d1 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:49:00 +0000 Subject: [PATCH 1/2] feat(service): Take origins and value patterns from the run A run now sends its own `allow` origins and value patterns, and the service config no longer has them. A tenant that calls a new origin or uses a new pattern changes only its own config: no Roach PR, no Terraform apply, and no VM restart. Two checks replace the fixed lists, so one run cannot harm the others: - The proxy sends requests only to public addresses. It checks the address of each connection after DNS, so neither an IP address nor a host name can point a run at the metadata server or the VPC. `privateNetworks` turns this off for tests. - Value patterns have a time limit, enforced with the `vm` timeout: 1 s to compile when the run starts, and 1 s plus 0.5 s per MiB of body for the key of each request. A slow pattern fails only its own run or request with HTTP 400. The certificate authority keeps at most 1000 host certificates and deletes its temporary files, because runs now name their own hosts. `allow` is now required in a run config. Co-Authored-By: David Cramer --- README.md | 50 ++++++++++++------- deploy/gcp/main.tf | 2 - deploy/gcp/roach.tfvars | 18 +------ deploy/gcp/variables.tf | 11 ----- src/addresses.ts | 103 +++++++++++++++++++++++++++++++++++++++ src/certificates.ts | 20 ++++++-- src/request-key.ts | 42 +++++++++++++--- src/server.ts | 31 +++++++++--- src/service.ts | 105 +++++++++++++++++++++++++--------------- src/values.ts | 39 +++++++++++++++ tests/deployed.test.ts | 9 +++- tests/recording.test.ts | 3 +- tests/service.test.ts | 93 ++++++++++++++++++++++++++++++++--- 13 files changed, 412 insertions(+), 114 deletions(-) create mode 100644 src/addresses.ts diff --git a/README.md b/README.md index 65a4396..88cab13 100644 --- a/README.md +++ b/README.md @@ -71,11 +71,12 @@ A repository adds one step and a `roach.json` file: run: pnpm test ``` -`roach.json` is at the root of the repository. It has the `rules` of the -run, and can have `allow` (`RunConfig` in `src/service.ts`): +`roach.json` is at the root of the repository. It has the `allow` origins +and the `rules` of the run (`RunConfig` in `src/service.ts`): ```json { + "allow": ["https://ai-gateway.vercel.sh"], "rules": [ { "name": "model", @@ -108,13 +109,16 @@ A run takes `RunConfig` (`src/service.ts`): `tenant`, `mode`, `rules`, recordings and fails a miss with HTTP 412, so nothing goes live. `record` sends every request live and records it again. `off` records and replays nothing. -- `allow`: the only origins that the proxy sends requests to. It refuses all - other origins with HTTP 403. The upstream host always comes from this - list, not from the client. +- `allow`: the only origins that the proxy sends requests to, 1 to 32 of + them. It refuses all other origins with HTTP 403. The upstream host + always comes from this list, not from the request. The service sends + requests only to public addresses, so an origin that resolves to a + private, loopback, or link-local address also gets HTTP 403. - `rules`: the traffic to record. `name` uses letters, digits, `_`, `.`, and `-`. `match` has `method`, `url` (a prefix), and `headers`. `keyHeaders` adds request headers to the key. `values` - names the values that change from run to run. + names the values that change from run to run. A run can have 64 rules, + and a rule can have 32 values. - Credentials: the proxy never writes the `secrets` of the service config or of the run. Use `secrets` of the run for the credentials of a CI job that a response can show but no request header has, such as a token that @@ -183,6 +187,14 @@ it, and the provider refuses a changed thinking block. Keep the patterns narrow. A broad pattern makes requests that differ in a real way use the same recording. +The service takes any pattern that compiles in 1 s, up to 2048 +characters. A run without the token must use the same patterns as the run +that recorded, or its keys do not match. All runs share one process, so +the key of each request has a time limit: 1 s, and 0.5 s for each MiB of +the body. The built-in patterns take less than 0.1 s for each MiB. A +request whose key takes longer fails with HTTP 400, and other runs go on. +Avoid nested quantifiers, such as `(a+)+`. + ## Misses The proxy adds each request without a recording to `stats().misses`, with @@ -214,10 +226,16 @@ first miss of each test, which is the one to fix. stats. A proxied request names its run in `Proxy-Authorization`, as `Basic base64(:)`. The proxy URL of the run has them, so `HTTPS_PROXY` sends them. A request without them gets HTTP 407. -- **Value patterns.** A pattern runs for every run in the process, so a - run can only use the patterns in `VALUE_PATTERNS` and the ones in - `valuePatterns` of the config. Check each one for slow backtracking - before you add it. +- **Config of a run.** Each run sends its own `allow` and `rules`, with + their value patterns. A tenant that calls a new origin or uses a new + pattern changes only its own config. The service needs no change and no + restart. Two checks keep one run from harming the others: + - The proxy sends requests only to public addresses (`addresses.ts`). + It checks the address of each connection after DNS, so a host name + cannot point a run at the metadata server, at the VPC, or at the VM. + Only tests and local development set `privateNetworks`. + - The key of a request has a time limit (`request-key.ts`), so a slow + pattern fails only its own request. - **Sentry.** With `sentryDsn`, each read and write of a recording sends the metric `roach.recording`. Its attributes are `tenant`, `rule`, `key`, `run` (the `name` of the run), and `result`: `replayed`, `missed`, or @@ -261,9 +279,8 @@ The `Image` workflow builds the image on each pull request and pushes it on 1. Make the package `ghcr.io/getsentry/roach` public once, after the first push to `main`, so the VM can pull it without credentials. 2. `deploy/gcp/roach.tfvars` has the values of the Sentry deployment: the - GCP project `roach-511216`, the domain, `allow`, and `value_patterns`. - `allow` and `value_patterns` must cover the rules of every tenant. Another - deployment changes these values. + GCP project `roach-511216` and the domain. Another deployment changes + these values. It has no origins or patterns: runs send them. 3. The Terraform state is in the private bucket that `backend "gcs"` in `versions.tf` names. The state holds the CA key and the write token, so the bucket must stay private. Make the bucket before the first @@ -296,10 +313,8 @@ To deploy a new image, restart the VM: To add a project, add its repository to the repositories of the `ROACH_TOKEN` organization secret, in the GitHub settings of the -organization. Its CI jobs set `tenant` to `GITHUB_REPOSITORY`. When the -project calls origins or uses value patterns that are not in -`roach.tfvars` yet, add them and apply again. Terraform writes a new config version, and the -VM reads it at its next start. +organization. Its CI jobs set `tenant` to `GITHUB_REPOSITORY`, and send +their own origins and patterns. The service needs no change. To change the write token, run `terraform apply -var-file=roach.tfvars -replace=random_password.write_token`, @@ -376,6 +391,7 @@ The proxy is in `src/`. Tests are in `tests/`. The production setup is in - `types.ts`: rules, modes, and the results of a run. - `server.ts`: sockets, HTTPS interception, the allow list, and the control API. +- `addresses.ts`: the public addresses that the proxy can send requests to. - `service.ts`: the shared service: tenants, runs, access, and metrics. - `recorder.ts`: modes, sessions, replay, and misses. - `request-key.ts`: the key of a request. diff --git a/deploy/gcp/main.tf b/deploy/gcp/main.tf index e51ae74..6ea2804 100644 --- a/deploy/gcp/main.tf +++ b/deploy/gcp/main.tf @@ -84,9 +84,7 @@ resource "google_secret_manager_secret_version" "config" { port = local.port publicUrl = "https://${var.domain}" bucket = google_storage_bucket.recordings.name - allow = var.allow writeTokenHash = sha256(random_password.write_token.result) - valuePatterns = var.value_patterns ca = { cert = tls_self_signed_cert.ca.cert_pem key = tls_private_key.ca.private_key_pem diff --git a/deploy/gcp/roach.tfvars b/deploy/gcp/roach.tfvars index 7a6aa47..8a60a8d 100644 --- a/deploy/gcp/roach.tfvars +++ b/deploy/gcp/roach.tfvars @@ -7,21 +7,7 @@ project = "roach-511216" # The record is in getsentry/ops (getsentry/ops#24232). domain = "roach-proxy.getsentry.net" -# The origins of Junior's evals (packages/junior-evals/src/recording-rules.ts). -allow = [ - "https://ai-gateway.vercel.sh", - "https://vercel.com", - "https://api.vercel.com", - "https://oidc.vercel.com", - "https://docs.slack.dev", -] - -# Junior's own value patterns, in addition to the built-in ones. Each one must -# be equal to its source in packages/junior-evals/src/recording-rules.ts, or -# the service refuses the run. -value_patterns = [ - "(? + Object.assign( + new Error( + `Roach does not send requests to ${host}, because ${address} is not a public address`, + ), + { status: 403, code: "ERR_ROACH_NOT_PUBLIC" }, + ); + +/** + * A DNS lookup for `http.request()` that fails when the host has an + * address that is not public. It checks all addresses of the host, so the + * choice of the connection does not matter. + */ +export const publicLookup: LookupFunction = (hostname, options, callback) => { + dnsLookup(hostname, { ...options, all: true }, (error, addresses) => { + if (error) { + callback(error, "", 0); + return; + } + const blocked = addresses.find(({ address }) => !isPublicAddress(address)); + if (blocked) { + callback(notPublicError(hostname, blocked.address), "", 0); + return; + } + if (options.all) { + // The type has only the single-address form of the callback. + (callback as unknown as (e: null, list: LookupAddress[]) => void)( + null, + addresses, + ); + return; + } + callback(null, addresses[0]!.address, addresses[0]!.family); + }); +}; diff --git a/src/certificates.ts b/src/certificates.ts index f10a5cc..1356bc4 100644 --- a/src/certificates.ts +++ b/src/certificates.ts @@ -3,7 +3,9 @@ * * The proxy intercepts HTTPS. It signs one certificate for each host when a * client first connects to that host, and signs it again before it - * expires. Clients must trust the authority certificate. The service loads + * expires. Runs name their own hosts, so the authority keeps the + * certificates of at most `MAX_HOSTS` hosts, and keeps no files for them. + * Clients must trust the authority certificate. The service loads * a fixed one, so that clients can trust it across restarts. */ import { execFile } from "node:child_process"; @@ -44,6 +46,8 @@ export interface CertificateAuthorityPem { const HOST_DAYS = 7; /** Sign a host again when its certificate is older than this. */ const RESIGN_MS = (HOST_DAYS - 1) * 24 * 60 * 60 * 1000; +/** The hosts with a certificate in memory. More hosts drop the oldest one. */ +const MAX_HOSTS = 1000; /** * Create a certificate authority in a new temporary directory. Without @@ -109,10 +113,11 @@ export async function createCertificateAuthority( "-out", cert, ]); - return tls.createSecureContext({ - cert: await readFile(cert, "utf8"), - key: hostKeyPem, - }); + const certPem = await readFile(cert, "utf8"); + await Promise.all( + [csr, extensions, cert].map((file) => rm(file, { force: true })), + ); + return tls.createSecureContext({ cert: certPem, key: hostKeyPem }); }; return { @@ -126,7 +131,12 @@ export async function createCertificateAuthority( fresh.context.catch(() => { if (contexts.get(host) === fresh) contexts.delete(host); }); + contexts.delete(host); contexts.set(host, fresh); + // A map keeps the order of insertion, so the first key is the oldest. + if (contexts.size > MAX_HOSTS) { + contexts.delete(contexts.keys().next().value!); + } entry = fresh; } return entry.context; diff --git a/src/request-key.ts b/src/request-key.ts index 6106296..eefa7fb 100644 --- a/src/request-key.ts +++ b/src/request-key.ts @@ -5,6 +5,10 @@ * and the body. A JSON body has sorted object keys, so key order does not * matter. Each changing value of the rule is `<>` in the key * (`values.ts`). + * + * The patterns of `values` come from the client, and all runs share one + * process. A slow pattern would block every run, so a key has a time limit + * (`KEY_TIME_LIMIT`). */ import { createHash } from "node:crypto"; import type { IncomingHttpHeaders } from "node:http"; @@ -15,11 +19,27 @@ import { type KnownValues, type RequestValues, type ValuePatterns, + withTimeLimit, } from "./values.ts"; /** Change this to make every recording a miss. */ const KEY_VERSION = "http-v1"; +/** + * The time that the key of one request can take: 1 s, and 0.5 s for each + * MiB of the body. The built-in patterns take less than 0.1 s for each + * MiB. A key that takes longer fails its request, and other runs go on. + */ +const KEY_TIME_LIMIT = { baseMs: 1000, msPerMiB: 500 }; + +/** The parts of a request that make its key. */ +interface RequestToKey { + method: string; + url: string; + headers: IncomingHttpHeaders; + body: string; +} + /** A request as the key sees it. */ export interface KeyedRequest { /** The name of the recording file, without `.json`. */ @@ -100,16 +120,24 @@ const sha256 = (text: string) => createHash("sha256").update(text).digest("hex"); /** * The key and the changing values of a request. `known` holds - * the values of earlier requests in the same session (`values.ts`). + * the values of earlier requests in the same session (`values.ts`). It + * throws an error with HTTP status 400 when it takes too long. */ export function keyRequest( rule: RecordingRule, - request: { - method: string; - url: string; - headers: IncomingHttpHeaders; - body: string; - }, + request: RequestToKey, + known?: KnownValues, +): KeyedRequest { + const mib = Buffer.byteLength(request.body) / (1024 * 1024); + return withTimeLimit( + Math.ceil(KEY_TIME_LIMIT.baseMs + KEY_TIME_LIMIT.msPerMiB * mib), + () => keyRequestNow(rule, request, known), + ); +} + +function keyRequestNow( + rule: RecordingRule, + request: RequestToKey, known?: KnownValues, ): KeyedRequest { const headers = Object.fromEntries( diff --git a/src/server.ts b/src/server.ts index c46b12b..7a8747c 100644 --- a/src/server.ts +++ b/src/server.ts @@ -6,9 +6,11 @@ * rule matches to the recorder (`recorder.ts`). Other requests go live * without a change, and their responses stream. * - * It sends requests only to the `allow` origins. It refuses other origins - * with HTTP 403 and sends nothing. The upstream host always comes from - * `allow`, never from the client. Only the path comes from the client. + * It sends requests only to the `allow` origins of the run. It refuses + * other origins with HTTP 403 and sends nothing. The upstream host always + * comes from `allow`, never from the request. Only the path comes from the + * request. It also refuses an origin whose address is not public + * (`addresses.ts`), unless the service config has `privateNetworks`. * * Each response has an `x-roach` header: `replayed`, `live`, * `missed` (`replay` mode had no recording), or `passthrough` (no rule @@ -19,6 +21,7 @@ import http from "node:http"; import https from "node:https"; import { isIP, type Socket } from "node:net"; import tls from "node:tls"; +import { isPublicAddress, notPublicError, publicLookup } from "./addresses.ts"; import type { CertificateAuthority } from "./certificates.ts"; import type { ProxyRequest, Recorder } from "./recorder.ts"; @@ -122,11 +125,13 @@ export function sameToken( /** * Send a request to an allowed origin. The host, port, and scheme come from - * the configuration. Only the path comes from the client. + * the configuration. Only the path comes from the client. Without + * `privateNetworks`, only public addresses get the request. */ function sendUpstream( origin: URL, request: ProxyRequest, + privateNetworks: boolean, extraHeaders: http.OutgoingHttpHeaders = {}, ): Promise { const headers: http.OutgoingHttpHeaders = {}; @@ -143,12 +148,19 @@ function sendUpstream( const client = origin.protocol === "https:" ? https : http; // A URL keeps the brackets of an IPv6 host. A request option has none. const hostname = origin.hostname.replace(/^\[|\]$/g, ""); + // Node does not call `lookup` for an IP address, so check it here. + if (!privateNetworks && isIP(hostname) && !isPublicAddress(hostname)) { + return Promise.reject(notPublicError(hostname, hostname)); + } return new Promise((resolve, reject) => { const upstream = client.request( { protocol: origin.protocol, hostname, port: origin.port || undefined, + // The check is on the address of the connection. A kept-alive + // connection had the check when it opened. + lookup: privateNetworks ? undefined : publicLookup, servername: isIP(hostname) ? undefined : hostname, path: `${request.url.pathname}${request.url.search}`, method: request.method, @@ -163,7 +175,7 @@ function sendUpstream( /** Proxy one request to an allowed origin, through the recorder. */ async function proxyRequest( - { recorder, origins, replayOnly }: ProxyTarget, + { recorder, origins, replayOnly, privateNetworks = false }: ProxyTarget, incoming: http.IncomingMessage, outgoing: http.ServerResponse, tunnelOrigin: string | undefined, @@ -196,7 +208,7 @@ async function proxyRequest( } if (!rule) { recorder.countPassthrough(origin.origin, request.headers); - const upstream = await sendUpstream(origin, request); + const upstream = await sendUpstream(origin, request, privateNetworks); outgoing.writeHead(upstream.statusCode ?? 502, upstream.statusMessage, { ...upstream.headers, "x-roach": "passthrough", @@ -208,7 +220,7 @@ async function proxyRequest( const response = await recorder.respond(rule, request, async () => { // Ask for a plain body, so the recording is readable. - const upstream = await sendUpstream(origin, request, { + const upstream = await sendUpstream(origin, request, privateNetworks, { "accept-encoding": "identity", }); return { @@ -287,6 +299,11 @@ export interface ProxyTarget { * service sets this for a run without the write token. */ replayOnly?: boolean; + /** + * Send requests to addresses that are not public too, such as + * `127.0.0.1`. Only for tests and local development. + */ + privateNetworks?: boolean; /** Set when the target stops. Its tunnels then refuse new requests. */ closed?: boolean; } diff --git a/src/service.ts b/src/service.ts index 415289d..46547c2 100644 --- a/src/service.ts +++ b/src/service.ts @@ -16,6 +16,13 @@ * - The service gives each run its own id and token. A proxied request * names its run in `Proxy-Authorization`, as `Basic base64(:)`. * The proxy URL of the run has them, so `HTTPS_PROXY` sends them. + * - A run sends its own config: its origins (`allow`), rules, and value + * patterns. So a tenant changes what it records without a change to the + * service. Two checks keep that safe for the other tenants. The proxy + * sends requests only to public addresses (`addresses.ts`), so a run + * cannot reach the metadata server or the VPC. The key of a request has + * a time limit (`request-key.ts`), so a slow pattern fails only its own + * request. * - The recordings of a tenant are under `/`, in a directory or a * GCS bucket (`gcs.ts`). Tenants never share recordings, because a key * does not include credentials. @@ -46,7 +53,7 @@ import { } from "./server.ts"; import { RULE_NAME, type RecordingStore } from "./store.ts"; import type { RecordingMode, RecordingRule, RecordingStats } from "./types.ts"; -import { VALUE_PATTERNS } from "./values.ts"; +import { invalidValuePatterns } from "./values.ts"; /** The configuration of the service. The operator owns it. */ export interface RoachServiceConfig { @@ -63,21 +70,19 @@ export interface RoachServiceConfig { directory?: string; /** A GCS bucket for the recordings. Set this or `directory`. */ bucket?: string; - /** The only origins that runs can reach. A run can allow fewer. */ - allow: string[]; /** * The SHA-256 of the write token, in hex. All tenants share the token. * Give it only to CI jobs that you trust. */ writeTokenHash: string; - /** - * More regular expression sources that rules can use in `values`, in - * addition to `VALUE_PATTERNS`. A slow pattern blocks all runs, so check - * each one for catastrophic backtracking before you add it. - */ - valuePatterns?: string[]; /** Credentials to redact in all recordings. */ secrets?: string[]; + /** + * Let runs reach addresses that are not public, such as `127.0.0.1` and + * the metadata server. Only for tests and local development. Default: + * `false`. + */ + privateNetworks?: boolean; /** * The certificate authority that signs intercepted hosts. Clients trust * it, so a deployed service must keep it across restarts. Default: a new @@ -97,8 +102,12 @@ export interface RunConfig { tenant: string; mode: RecordingMode; rules: RecordingRule[]; - /** Default: all origins of the service. */ - allow?: string[]; + /** + * The only origins that the run can reach, such as + * `https://ai-gateway.vercel.sh`. The service sends requests only to + * their public addresses. + */ + allow: string[]; /** * The name of the run in Sentry, such as * `${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}`. Default: the run id. @@ -150,6 +159,15 @@ const RUN_TIMEOUT_MS = 6 * 60 * 60 * 1000; */ const MAX_PUBLIC_RUNS = 100; const MAX_RUN_NAME = 128; +/** + * Limits of the config of a run. Anyone can start a run, so they keep the + * work of a run small: the service makes a certificate for each host that + * a run tunnels to, and runs each value pattern on each request. + */ +const MAX_RUN_ORIGINS = 32; +const MAX_RULES = 64; +const MAX_RULE_VALUES = 32; +const MAX_PATTERN_LENGTH = 2048; /** Enough for the API keys and OIDC tokens of a CI job. */ const MAX_RUN_SECRETS = 32; const MAX_SECRET_LENGTH = 16 * 1024; @@ -184,11 +202,7 @@ function proxyCredentials( } /** Check the run config of a client. Returns the error, or `undefined`. */ -function invalidRunConfig( - config: Partial, - serviceOrigins: Map, - valuePatterns: Set, -): string | undefined { +function invalidRunConfig(config: Partial): string | undefined { if (!config.mode || !MODES.has(config.mode)) { return "mode must be auto, off, record, or replay"; } @@ -209,7 +223,9 @@ function invalidRunConfig( return `secrets must be a list of at most ${MAX_RUN_SECRETS} strings of at most ${MAX_SECRET_LENGTH} characters`; } } - if (!Array.isArray(config.rules)) return "rules must be a list"; + if (!Array.isArray(config.rules) || config.rules.length > MAX_RULES) { + return `rules must be a list of at most ${MAX_RULES} rules`; + } const names = new Set(); for (const rule of config.rules) { if (typeof rule?.name !== "string" || !RULE_NAME.test(rule.name)) { @@ -220,29 +236,41 @@ function invalidRunConfig( if (typeof rule.match !== "object" || rule.match === null) { return `rule ${rule.name} must have match`; } - // A pattern runs on the requests of all runs in one process, so a - // client can only use patterns that the operator knows to be fast. + // A slow pattern fails only its own requests (`request-key.ts`), so + // the service takes any pattern that compiles. if (rule.values !== undefined) { if (typeof rule.values !== "object" || rule.values === null) { return `values of rule ${rule.name} must be an object`; } - for (const [name, source] of Object.entries(rule.values)) { - if (typeof source !== "string" || !valuePatterns.has(source)) { - return `value ${name} of rule ${rule.name} is not a pattern that the service allows`; + const values = Object.entries(rule.values); + if (values.length > MAX_RULE_VALUES) { + return `rule ${rule.name} can have at most ${MAX_RULE_VALUES} values`; + } + for (const [name, source] of values) { + if (typeof source !== "string" || source.length > MAX_PATTERN_LENGTH) { + return `value ${name} of rule ${rule.name} must be a pattern of at most ${MAX_PATTERN_LENGTH} characters`; } } } } - if (config.allow !== undefined) { - if (!Array.isArray(config.allow)) return "allow must be a list"; - for (const origin of config.allow) { - // The client sends JSON, so check the type at run time. - if ( - typeof origin !== "string" || - !serviceOrigins.has(origin.replace(/\/$/, "")) - ) { - return `the service does not allow ${origin}`; - } + const patternError = invalidValuePatterns( + config.rules.flatMap((rule) => (rule.values ? [rule.values] : [])), + ); + if (patternError) return patternError; + if ( + !Array.isArray(config.allow) || + config.allow.length === 0 || + config.allow.length > MAX_RUN_ORIGINS + ) { + return `allow must be a list of 1 to ${MAX_RUN_ORIGINS} origins`; + } + for (const origin of config.allow) { + // The client sends JSON, so check the type at run time. + if (typeof origin !== "string") return "allow must be a list of origins"; + try { + parseOrigins([origin.replace(/\/$/, "")]); + } catch { + return `${origin} is not an http or https origin`; } } return undefined; @@ -297,13 +325,8 @@ export async function startRoachService( if (config.sentryDsn) { Sentry.init({ dsn: config.sentryDsn, tracesSampleRate: 0 }); } - const serviceOrigins = parseOrigins(config.allow); const runs = new Map(); const authority = await createCertificateAuthority(config.ca); - const valuePatterns = new Set([ - ...Object.values(VALUE_PATTERNS), - ...(config.valuePatterns ?? []), - ]); const storeOf = (tenant: string): RecordingStore => config.bucket ? createGcsStore(config.bucket, `${tenant}/`) @@ -320,7 +343,6 @@ export async function startRoachService( const createRun = (runConfig: RunConfig, canWrite: boolean): RemoteRun => { const id = randomBytes(8).toString("hex"); const token = randomBytes(24).toString("hex"); - const allow = runConfig.allow ?? config.allow; const store = runStore( storeOf(runConfig.tenant), { tenant: runConfig.tenant, run: runConfig.name ?? id }, @@ -330,9 +352,12 @@ export async function startRoachService( id, token, started: Date.now(), - origins: parseOrigins(allow.map((origin) => origin.replace(/\/$/, ""))), + origins: parseOrigins( + runConfig.allow.map((origin) => origin.replace(/\/$/, "")), + ), // A public run must not be an open proxy to the allowed origins. replayOnly: !canWrite, + privateNetworks: config.privateNetworks === true, // Each run learns only the credentials of its own requests. recorder: createRecorder( { @@ -387,7 +412,7 @@ export async function startRoachService( sendJson(outgoing, 400, { error: "tenant must be owner/repo" }); return; } - const error = invalidRunConfig(runConfig, serviceOrigins, valuePatterns); + const error = invalidRunConfig(runConfig); if (error) { sendJson(outgoing, 400, { error }); return; diff --git a/src/values.ts b/src/values.ts index 2613430..c2a64ee 100644 --- a/src/values.ts +++ b/src/values.ts @@ -16,6 +16,7 @@ * A value that the response makes itself, such as a date that the model * calculates, stays as it was recorded. */ +import vm from "node:vm"; /** * Lookbehind: the value does not continue a word or number. The proxy @@ -102,6 +103,44 @@ function compile(patterns: ValuePatterns): CompiledValues { return result; } +/** + * Run `fn` with a time limit. A slow pattern cannot stop itself, but the + * `timeout` of `vm` stops any code that runs too long, also code outside + * the context that the script calls. A timeout throws an error with HTTP + * status 400. + */ +export function withTimeLimit(ms: number, fn: () => T): T { + try { + return vm.runInContext("fn()", vm.createContext({ fn }), { + timeout: ms, + }) as T; + } catch (error) { + if ((error as { code?: string }).code === "ERR_SCRIPT_EXECUTION_TIMEOUT") { + throw Object.assign( + new Error(`the value patterns took over ${ms} ms. Make them simpler.`), + { status: 400 }, + ); + } + throw error; + } +} + +/** + * Check that the patterns of each rule compile. Returns the error, or + * `undefined`. A pattern can be slow to compile, so all of them together + * have 1 s. + */ +export function invalidValuePatterns( + list: ValuePatterns[], +): string | undefined { + try { + withTimeLimit(1000, () => list.forEach((patterns) => compile(patterns))); + return undefined; + } catch (error) { + return (error as Error).message; + } +} + /** Values that an earlier request of the same session had, by value. */ export type KnownValues = ReadonlyMap; diff --git a/tests/deployed.test.ts b/tests/deployed.test.ts index 1ba5b1a..9215ef3 100644 --- a/tests/deployed.test.ts +++ b/tests/deployed.test.ts @@ -93,7 +93,10 @@ async function ciJob(job: { summary: string; }> { const workspace = await mkdtemp(path.join(directory, "job-")); - const config = { rules: [{ name: "model", match: { method: "POST" } }] }; + const config = { + allow: [upstreamOrigin], + rules: [{ name: "model", match: { method: "POST" } }], + }; await writeFile(path.join(workspace, "roach.json"), JSON.stringify(config)); await writeFile( path.join(workspace, "job.mjs"), @@ -273,7 +276,9 @@ beforeAll(async () => { port: servicePort, publicUrl: frontUrl, bucket: BUCKET, - allow: [upstreamOrigin], + // The upstream is on 127.0.0.2 here. A deployed service has no such + // option, so runs reach only public addresses. + privateNetworks: true, writeTokenHash: createHash("sha256").update(TOKEN).digest("hex"), ca: { cert: caCert, key: await readFile(caKeyFile, "utf8") }, sentryDsn: `http://public@127.0.0.1:${sentryPort}/1`, diff --git a/tests/recording.test.ts b/tests/recording.test.ts index c085243..8e6c1c6 100644 --- a/tests/recording.test.ts +++ b/tests/recording.test.ts @@ -43,6 +43,7 @@ async function start(mode: RecordingMode): Promise { { tenant: TENANT, mode, + allow: [origin], rules: [ { name: "model", @@ -119,7 +120,7 @@ beforeEach(async () => { directory = await mkdtemp(path.join(tmpdir(), "roach-")); service = await startRoachService({ directory, - allow: [origin], + privateNetworks: true, writeTokenHash: createHash("sha256").update(TOKEN).digest("hex"), }); }); diff --git a/tests/service.test.ts b/tests/service.test.ts index 8bbb8e5..eb05664 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -39,6 +39,7 @@ async function startRun( { tenant: "acme/alpha", mode, + allow: [origin], rules: [ { name: "model", match: { method: "POST", url: `${origin}/v1/` } }, ], @@ -103,7 +104,8 @@ beforeEach(async () => { directory = await mkdtemp(path.join(tmpdir(), "roach-service-")); service = await startRoachService({ directory, - allow: [origin], + // The upstream of the tests is on 127.0.0.1. + privateNetworks: true, writeTokenHash: hash("write-token"), secrets: ["service-secret-value-123"], }); @@ -221,19 +223,38 @@ describe("roach service", () => { ); } await expect( - startRun("write-token", "auto", { allow: ["https://example.com"] }), - ).rejects.toThrow(/does not allow/); + startRun("write-token", "auto", { allow: ["file:///etc/passwd"] }), + ).rejects.toThrow(/not an http or https origin/); + await expect( + startRun("write-token", "auto", { allow: [] }), + ).rejects.toThrow(/allow must be a list/); await expect( startRun("write-token", "auto", { rules: [{ name: "../escape", match: {} }], }), ).rejects.toThrow(/rule name/); - // A pattern runs for all runs, so only known patterns are allowed. await expect( startRun("write-token", "auto", { - rules: [{ name: "model", match: {}, values: { slow: "(a+)+$" } }], + rules: [{ name: "model", match: {}, values: { broken: "(" } }], }), - ).rejects.toThrow(/not a pattern that the service allows/); + ).rejects.toThrow(/Invalid regular expression/); + // Patterns can also be slow to compile. They have a time limit too. + await expect( + startRun("write-token", "auto", { + rules: [ + { + name: "model", + match: {}, + values: Object.fromEntries( + Array.from({ length: 32 }, (_, i) => [ + `v${i}`, + `(?:(?:(?:(?:a?){30}){30}){30}){${30 + i}}`, + ]), + ), + }, + ], + }), + ).rejects.toThrow(/value patterns took over/); await expect( startRun("write-token", "auto", { secrets: [42 as unknown as string] }), ).rejects.toThrow(/secrets must be a list/); @@ -285,6 +306,66 @@ describe("roach service", () => { expect(liveRequests).toBe(1); }); + it("takes origins and patterns from the run, and keeps the service safe", async () => { + // A run can use any pattern, also one that backtracks a lot. Its key + // has a time limit, so only its own request fails. + const slow = await startRun("write-token", "auto", { + rules: [ + { + name: "model", + match: { method: "POST", url: `${origin}/v1/` }, + values: { slow: "(a+)+$" }, + }, + ], + }); + const started = Date.now(); + const stuck = await send(slow.url, { prompt: `${"a".repeat(40)}!` }); + expect(stuck.status).toBe(400); + expect(stuck.body).toMatch(/value patterns took over/); + expect(Date.now() - started).toBeLessThan(5000); + // The process goes on for other runs. + const other = await startRun("write-token", "auto"); + expect(await send(other.url, { prompt: "hi" })).toMatchObject({ + status: 200, + source: "live", + }); + expect(liveRequests).toBe(1); + + // A deployed service sends requests only to public addresses. A run + // names its own origins, so it must not reach the metadata server or + // the network of the service, by IP address or by host name. + const port = new URL(origin).port; + const guarded = await startRoachService({ + directory, + writeTokenHash: hash("write-token"), + }); + try { + for (const target of [origin, `http://localhost:${port}`]) { + const run = await startRemoteRun( + { url: guarded.url, token: "write-token" }, + { + tenant: "acme/alpha", + mode: "auto", + allow: [target], + rules: [{ name: "model", match: { url: `${target}/v1/` } }], + }, + ); + runs.push(run); + const agent = new ProxyAgent({ uri: run.url, proxyTunnel: true }); + agents.push(agent); + // One request that a rule records, and one that goes through. + for (const url of [`${target}/v1/messages`, `${target}/other`]) { + const response = await request(url, { dispatcher: agent }); + expect(response.statusCode).toBe(403); + expect(await response.body.text()).toMatch(/not a public address/); + } + } + } finally { + await guarded.close(); + } + expect(liveRequests).toBe(1); + }); + it("redacts request credentials and the secrets of the service and the run", async () => { // A run secret is not in a credential header, so only the run config // tells the service about it. From 8e1e03ba4d2009ecde8fd1ca3b835bb6a5644089 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 01:05:41 +0000 Subject: [PATCH 2/2] test(service): Make the slow compile case slow on fast machines --- tests/service.test.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/service.test.ts b/tests/service.test.ts index eb05664..bce2c27 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -239,20 +239,20 @@ describe("roach service", () => { }), ).rejects.toThrow(/Invalid regular expression/); // Patterns can also be slow to compile. They have a time limit too. + // Without it, these take over a minute. Each pattern is different, + // so V8 cannot reuse an earlier one. await expect( startRun("write-token", "auto", { - rules: [ - { - name: "model", - match: {}, - values: Object.fromEntries( - Array.from({ length: 32 }, (_, i) => [ - `v${i}`, - `(?:(?:(?:(?:a?){30}){30}){30}){${30 + i}}`, - ]), - ), - }, - ], + rules: Array.from({ length: 64 }, (_, rule) => ({ + name: `model${rule}`, + match: {}, + values: Object.fromEntries( + Array.from({ length: 32 }, (__, value) => [ + `v${value}`, + `(?:(?:(?:(?:a?){30}){30}){${30 + rule}}){${30 + value}}`, + ]), + ), + })), }), ).rejects.toThrow(/value patterns took over/); await expect(