diff --git a/.bumpy/proxy-skip-inert-placeholders.md b/.bumpy/proxy-skip-inert-placeholders.md new file mode 100644 index 000000000..e821776c3 --- /dev/null +++ b/.bumpy/proxy-skip-inert-placeholders.md @@ -0,0 +1,7 @@ +--- +varlock: minor +--- + +Proxy: a placeholder appearing in a request surface its rule doesn't substitute in (e.g. the body under the default header-only targets) is now skipped (forwarded unsubstituted) and logged as a skipped-placeholder audit event, instead of blocking the request. Blocking still applies to occurrences off the named path/param within a body: or query: target. + +The `maxOccurrences` option has been removed. Each `substituteIn` target is now worth one substitution per request, so listing a target is what grants it an occurrence: an API that carries the secret in two places just names both (`substituteIn=["header:authorization", "body:signature"]`) instead of raising a count. A repeat at the same target still blocks. Setting `maxOccurrences` is now a schema error that points at the replacement. diff --git a/packages/varlock-website/src/content/docs/guides/proxy/rules.mdx b/packages/varlock-website/src/content/docs/guides/proxy/rules.mdx index 985717baa..676ee404d 100644 --- a/packages/varlock-website/src/content/docs/guides/proxy/rules.mdx +++ b/packages/varlock-website/src/content/docs/guides/proxy/rules.mdx @@ -15,7 +15,6 @@ A `@proxy(...)` rule supports more than just a domain: | `block` | `block=true` denies matching requests outright (fail closed). | | `keys` | Array of additional item names to inject for this rule, e.g. `keys=[STRIPE_KEY, WEBHOOK_SECRET]`. | | `substituteIn` | Where the secret may be substituted: `header` (default), `header:`, `query`, `query:`, `body:`, e.g. `substituteIn=[header, "body:client_secret"]` (see [Substitution surface](#substitution-surface)). | -| `maxOccurrences` | How many times the placeholder may appear in one request before it's blocked (default `1`) (see [Substitution surface](#substitution-surface)). | | `rules` | Array of per-path/method policy refinements that share this rule's `domain` (see [Grouping rules for one domain](#grouping-rules-for-one-domain)). | `domain` and `method` take either a single value or an **array literal** for lists: @@ -46,7 +45,7 @@ When one host needs several path/method policies, write the `domain` once and li STRIPE_SECRET_KEY=yourPreferredPlugin() ``` -This injects `STRIPE_SECRET_KEY` across `api.stripe.com` and blocks refunds and payouts. The parent `@proxy(...)` still controls injection (where the secret goes); each `rules` entry is a policy-only refinement that inherits the `domain` and injects nothing on its own, so [precedence](#routing-rules) (block over allow) does the rest. An entry may set `path`, `method`, `block`, `substituteIn`, and `maxOccurrences`, but not `domain` or `keys` (those stay on the parent). +This injects `STRIPE_SECRET_KEY` across `api.stripe.com` and blocks refunds and payouts. The parent `@proxy(...)` still controls injection (where the secret goes); each `rules` entry is a policy-only refinement that inherits the `domain` and injects nothing on its own, so [precedence](#routing-rules) (block over allow) does the rest. An entry may set `path`, `method`, `block`, and `substituteIn`, but not `domain` or `keys` (those stay on the parent). ### Attached vs detached rules @@ -68,9 +67,9 @@ Even in `permissive` mode, if a request carries a placeholder that **no rule inj ### Substitution surface -Matching a rule decides **which host** a secret may go to. Two more guards decide **where inside the request** the placeholder gets swapped for the real value, and **how many times**. They exist because the proxy substitutes by finding the placeholder in the outbound bytes: without limits, an agent that was prompt-injected could place the placeholder somewhere the real value then leaks. The classic case is a request to an allowed host that forwards the value onward, e.g. asking a mail API to send an email whose body contains the placeholder. +Matching a rule decides **which host** a secret may go to. `substituteIn` decides **where inside the request** the placeholder is swapped for the real value, and each place you name is worth one swap. Without that, a prompt-injected agent could put the placeholder somewhere the real value then leaks: the classic case is asking a mail API on an allowed host to send an email whose body contains it. -**`substituteIn`, where the swap may happen.** By default a secret is only substituted into request **headers** (any header). That covers the common case, since most APIs authenticate with an `Authorization` or `X-Api-Key` header. If the placeholder shows up anywhere a target doesn't allow, the request is **blocked** rather than substituted, so the real value never lands somewhere it could be exfiltrated. Targets can be as broad or as specific as you want: +By default a secret is only substituted into request **headers** (any header), which covers most APIs. Targets can be as broad or as specific as you want: | Target | Allows substitution in | |---|---| @@ -82,9 +81,13 @@ Matching a rule decides **which host** a secret may go to. Two more guards decid | `body:client_secret` | only the value at that body path (see below) | | `body:*` | anywhere in the body (escape hatch for unparseable bodies, see below) | -Pin as tightly as the API allows: `header:authorization` blocks the secret being swapped into any other header (some providers forward custom headers onward), and a body path blocks it landing in any other field. +Pin as tightly as the API allows: `header:authorization` keeps the secret out of every other header (some providers forward custom ones onward), and a body path pins it to one field. -The bare `header` default still excludes a handful of headers that are never a legitimate secret and are common forward/log sinks: `cookie`, `host`, `x-forwarded-*`, `forwarded`, `via`, `referer`, `origin`, and `user-agent`. A placeholder landing in one of those is blocked even under the any-header default. If an API genuinely authenticates through one (a session cookie, say), name it explicitly with `substituteIn=[header:cookie]` and the explicit target wins. +The bare `header` default excludes headers that are never a legitimate secret and are common forward/log sinks: `cookie`, `host`, `x-forwarded-*`, `forwarded`, `via`, `referer`, `origin`, and `user-agent`. If an API really authenticates through one, name it explicitly (`substituteIn=[header:cookie]`) and the explicit target wins. + +**Placeholders outside your targets are left alone.** An occurrence in a part of the request no target covers (the body, under the header-only default) is **skipped**: those bytes are never rewritten, and the request is forwarded with the literal placeholder, which is inert. This is routine with agents, which quote their own env var into the conversation transcript they send with every call. Every item with a skipped placeholder gets one `skipped-placeholder` [audit event](/guides/proxy/running/#auditing) per request, naming the item and the parts of the request its placeholder turned up in, so probing stays visible. + +**Body substitution always requires a path.** `substituteIn=[body]` is a schema error, deliberately: "anywhere in the body" is the easiest surface to exfiltrate from, and a placeholder placed once in the wrong field would pass any count check. A path is a dotted path into a JSON body (`client_secret`, `data.token`, `items[0].key`) or a field name in an `application/x-www-form-urlencoded` body. The content type selects the parser, and a body that can't be parsed as declared fails closed. ```env-spec title=".env.schema" # OAuth token exchange carries the secret in a form field: @@ -92,20 +95,20 @@ The bare `header` default still excludes a handful of headers that are never a l CLIENT_SECRET=yourPreferredPlugin() ``` -**Body substitution always requires a path.** There is no bare `body` target: `substituteIn=[body]` is a schema error. This is deliberate. "Anywhere in the body" is the easiest surface to exfiltrate from (the email-body attack above), and `maxOccurrences` alone doesn't close it: the placeholder placed **once** in the wrong field still passes the count check. Naming the path (`body:client_secret`) is what actually pins the secret to the field it belongs in. - -A body path is a dotted path into a JSON body (`client_secret`, `data.token`, `items[0].key`) or a field name in an `application/x-www-form-urlencoded` body. The content type selects the parser; a body that can't be parsed as declared fails closed. +For a body varlock can't parse into a path (XML/SOAP, protobuf, plain text, a signed blob), `body:*` allows the placeholder anywhere in it. That reopens the surface a path exists to close, so scope the rule tightly with `path` and `method`, and don't use it on an endpoint that echoes, forwards, or stores body content. -For a body format varlock can't parse into a path (XML/SOAP, protobuf, plain text, a signed blob), use the wildcard `body:*`. It allows the placeholder anywhere in the body, so it reopens the "anywhere in the body" surface: only reach for it when a path won't work, scope the rule tightly with `path` and `method` to the one endpoint that needs it, and keep `maxOccurrences` low. Don't use it on an endpoint that echoes, forwards, or stores body content (a mail-send or note-create endpoint), where it would let a secret leak. - -**`maxOccurrences`, how many copies.** A valid request uses a secret a fixed number of times (almost always once). By default the placeholder may appear at most **once** per request; a second copy is treated as an exfiltration attempt (duplicate the token into an attacker-visible field while still making a working call) and the request is blocked. Raise it only for an API that legitimately repeats the same secret: +**One substitution per target.** A second occurrence at the *same* target is blocked, since the proxy can't tell the real use from an exfiltration copy. Skipped occurrences belong to no target, so they never count against it. Note that the bare `header` target is a single target covering every header, so the default allows the secret in one header, not one per header. An API that carries it in two places just names both, which tightens the rule rather than loosening it: ```env-spec title=".env.schema" -# @proxy(domain="api.example.com", substituteIn=["header:authorization", "body:signature"], maxOccurrences=2) +# One substitution in the auth header, one in the body's signature field: +# @proxy(domain="api.example.com", substituteIn=["header:authorization", "body:signature"]) SIGNING_KEY=yourPreferredPlugin() ``` -Both guards fail closed and, like a route mismatch, produce a message naming the item, where it was found, and how to widen the rule if the placement is legitimate. +Two things still fail closed, and both return a `403` naming the item, where the placeholder was found, and how to adjust the rule: + +- **A repeat at one target**, as above. +- **An occurrence off the named spot inside a targeted body or query.** With a `body:` or `query:` target, substitution is one find-and-replace across that whole surface, so a stray occurrence elsewhere in it can't be skipped without rewriting the body. ## Controlling what the agent sees diff --git a/packages/varlock-website/src/content/docs/guides/proxy/running.mdx b/packages/varlock-website/src/content/docs/guides/proxy/running.mdx index d55e30351..625d14f34 100644 --- a/packages/varlock-website/src/content/docs/guides/proxy/running.mdx +++ b/packages/varlock-website/src/content/docs/guides/proxy/running.mdx @@ -116,7 +116,7 @@ These flags apply only when **starting** a proxy. They also work on `proxy run` ## Auditing -Every request through the proxy is appended to a per-session, secrets-free audit log (host, method, path, a request hash, the matched rule, the decision, and which key names were injected, never any values). +Every request through the proxy is appended to a per-session, secrets-free audit log (host, method, path, a request hash, the matched rule, the decision, and which key names were injected, never any values). A [skipped](/guides/proxy/rules/#substitution-surface) placeholder adds one `skipped-placeholder` line per item, naming the key and the parts of the request its placeholder appeared in. ```bash varlock proxy audit # current/most-recent session diff --git a/packages/varlock-website/src/content/docs/reference/item-decorators.mdx b/packages/varlock-website/src/content/docs/reference/item-decorators.mdx index c9f20e087..f8db0027a 100644 --- a/packages/varlock-website/src/content/docs/reference/item-decorators.mdx +++ b/packages/varlock-website/src/content/docs/reference/item-decorators.mdx @@ -332,7 +332,7 @@ OPENAI_API_KEY=yourPreferredPlugin() Routes an item's secret through the [credential proxy](/guides/proxy/) so an untrusted child process only ever sees a placeholder, while the real value is injected into matching outbound requests at the network boundary. Using `@proxy(...)` on an item implies [`@sensitive`](#sensitive). -**Function form** `@proxy(domain=..., [path], [method], [block], [approval], [keys], [substituteIn], [maxOccurrences], [rules])`: +**Function form** `@proxy(domain=..., [path], [method], [block], [approval], [keys], [substituteIn], [rules])`: | Option | Meaning | |---|---| @@ -342,9 +342,8 @@ Routes an item's secret through the [credential proxy](/guides/proxy/) so an unt | `block` | `block=true` denies matching requests outright. | | `approval` | `approval=true` holds matching requests for an interactive yes/no in the `proxy start` terminal before they proceed. A self-contained one-shot `proxy run` has no terminal to prompt in and denies them. | | `keys` | Array of additional item names to inject for this rule, e.g. `keys=[OTHER_KEY]`. | -| `substituteIn` | Where the placeholder may be swapped for the real value: `header` (default), `header:`, `path`, `query`, `query:`, or `body:`, e.g. `[header, "body:client_secret"]`. Body always requires a path (`body:*` allows anywhere, for bodies that can't be parsed into a path). A placeholder anywhere no target allows blocks the request instead of substituting. See [Substitution surface](/guides/proxy/rules/#substitution-surface). | -| `maxOccurrences` | Max times the placeholder may appear in one request before it's blocked (default `1`). See [Substitution surface](/guides/proxy/rules/#substitution-surface). | -| `rules` | Array of policy refinements sharing this rule's `domain`, e.g. `rules=[{path="/v1/**", block=true}]`. Each entry may set `path`/`method`/`block`/`approval`/`substituteIn`/`maxOccurrences` (not `domain`/`keys`) and injects nothing on its own. See the [Grouping rules guide](/guides/proxy/rules/#grouping-rules-for-one-domain). | +| `substituteIn` | Where the placeholder may be swapped for the real value: `header` (default), `header:`, `path`, `query`, `query:`, or `body:`, e.g. `[header, "body:client_secret"]`. Body always requires a path (`body:*` allows anywhere, for bodies that can't be parsed into one). Each target is worth one substitution per request. A placeholder outside every target is skipped: forwarded unsubstituted and audited. A repeat at one target, or an occurrence off the named path/param inside a targeted body or query, blocks the request. See [Substitution surface](/guides/proxy/rules/#substitution-surface). | +| `rules` | Array of policy refinements sharing this rule's `domain`, e.g. `rules=[{path="/v1/**", block=true}]`. Each entry may set `path`/`method`/`block`/`approval`/`substituteIn` (not `domain`/`keys`) and injects nothing on its own. See the [Grouping rules guide](/guides/proxy/rules/#grouping-rules-for-one-domain). | The same decorator in the **header** creates a _detached_ policy rule (no injection unless it lists `keys`). diff --git a/packages/varlock/src/cli/commands/proxy.command.ts b/packages/varlock/src/cli/commands/proxy.command.ts index f5e271fa9..9b64569e7 100644 --- a/packages/varlock/src/cli/commands/proxy.command.ts +++ b/packages/varlock/src/cli/commands/proxy.command.ts @@ -17,6 +17,7 @@ import { createProxyAuditLog, readProxyAuditLines, type ProxyActivity, + type ProxyAuditSkippedPlaceholder, type ProxyAuditEntry, type ProxyAuditLog, } from '../../proxy/audit'; @@ -633,7 +634,12 @@ function formatProxyRequestLog(a: ProxyActivity): string { const inject = a.injectedKeys?.length ? ` ${ansis.dim('inject:')} ${ansis.yellow(a.injectedKeys.join(', '))}` : ''; - return `${arrow} ${formatProxyTarget(a.method, a.host, a.path)}${decision}${inject}`; + // A placeholder left inert in an untargeted surface (usually benign, e.g. an + // agent quoting its own placeholder), surfaced so probing stays visible. + const skipped = a.skippedPlaceholders?.length + ? ` ${ansis.dim('skipped:')} ${ansis.yellow(a.skippedPlaceholders.map((c) => `${c.key} (${c.locations.join(', ')})`).join(', '))}` + : ''; + return `${arrow} ${formatProxyTarget(a.method, a.host, a.path)}${decision}${inject}${skipped}`; } /** A one-line live log of a forwarded response: `← POST host/path 200 scrubbed: KEY`. */ @@ -2260,7 +2266,11 @@ export async function pruneAction(ctx: any) { console.log(`Pruned ${removed.length} ended proxy session${removed.length === 1 ? '' : 's'}.`); } -function formatAuditEntry(entry: ProxyAuditEntry): string { +function formatAuditEntry(entry: ProxyAuditEntry | ProxyAuditSkippedPlaceholder): string { + if (entry.type === 'skipped-placeholder') { + const rule = entry.ruleId ? ` rule="${entry.ruleId}"` : ''; + return `${entry.ts} ${'skipped'.padEnd(16)} ${entry.method.padEnd(7)} ${entry.host}${entry.path} key=${entry.key} in=${entry.locations.join(',')}${rule}`; + } const injected = entry.injected && entry.injectedKeys?.length ? ` injected=${entry.injectedKeys.join(',')}` : ''; @@ -2297,7 +2307,9 @@ export async function auditAction(ctx: any) { return; } - const entries = lines.filter((line): line is ProxyAuditEntry => line.type === 'request'); + const entries = lines.filter( + (line): line is ProxyAuditEntry | ProxyAuditSkippedPlaceholder => line.type === 'request' || line.type === 'skipped-placeholder', + ); if (!entries.length) { console.log('No audit entries for this session.'); return; diff --git a/packages/varlock/src/env-graph/lib/decorators.ts b/packages/varlock/src/env-graph/lib/decorators.ts index 447e834bc..e27f50a8d 100644 --- a/packages/varlock/src/env-graph/lib/decorators.ts +++ b/packages/varlock/src/env-graph/lib/decorators.ts @@ -14,7 +14,7 @@ import { ResolutionError, SchemaError, type VarlockError } from './errors'; import type { EnvGraph } from './env-graph'; import { parseKeyFilterArgs, applyKeyFilter, type KeyFilter } from './key-filter'; import { parseDuration } from '../../lib/duration'; -import { PROXY_APPROVAL_EACH_VALUES, parseProxySubstitutionTarget } from '../../proxy/types'; +import { PROXY_APPROVAL_EACH_VALUES, REMOVED_PROXY_RULE_OPTIONS, parseProxySubstitutionTarget } from '../../proxy/types'; export abstract class DecoratorInstance { @@ -379,11 +379,20 @@ function assertProxyStringListArg( * literal and `keys` as an array literal; rejects positional args; validates the * approval options. */ -const VALID_PROXY_OPTIONS = ['domain', 'path', 'method', 'keys', 'block', 'approval', 'substituteIn', 'maxOccurrences', 'rules'] as const; +const VALID_PROXY_OPTIONS = ['domain', 'path', 'method', 'keys', 'block', 'approval', 'substituteIn', 'rules'] as const; /** Per-entry options inside the `rules=[{...}]` array form. Each entry is a * policy refinement for the parent's `domain`, so it cannot re-set `domain` or * `keys` (injection is controlled by the parent rule). */ -const VALID_PROXY_RULE_ENTRY_OPTIONS = ['path', 'method', 'block', 'approval', 'substituteIn', 'maxOccurrences'] as const; +const VALID_PROXY_RULE_ENTRY_OPTIONS = ['path', 'method', 'block', 'approval', 'substituteIn'] as const; + +/** Reject an option that used to exist with its migration, before the generic + * unknown-option sweep turns it into a bare "unknown option" error. */ +function assertNoRemovedProxyOptions(keys: Array): void { + for (const key of keys) { + const removed = REMOVED_PROXY_RULE_OPTIONS[key]; + if (removed) throw new SchemaError(removed); + } +} /** Inner options of the `approval={...}` object form. */ const VALID_APPROVAL_OPTIONS = ['enabled', 'each', 'maxDuration'] as const; @@ -430,15 +439,6 @@ function assertProxySubstituteInArg(resolver: Resolver | undefined): void { if (resolver.isStatic) check(resolver.staticValue); } -/** A static `maxOccurrences` must be an integer >= 1. */ -function assertProxyMaxOccurrencesArg(resolver: Resolver | undefined): void { - if (!resolver?.isStatic) return; - const val = resolver.staticValue; - if (typeof val !== 'number' || !Number.isInteger(val) || val < 1) { - throw new SchemaError(`@proxy: maxOccurrences must be an integer >= 1, not ${JSON.stringify(val)}`); - } -} - /** * `approval` accepts either a boolean (`approval=true`) or an options object * (`approval={each=request, maxDuration=15m}`); the object form implies approval @@ -493,6 +493,7 @@ function assertProxyRulesArg(resolver: Resolver | undefined): void { throw new SchemaError('@proxy: each rules entry must be an object, e.g. {path="/v1/**", block=true}'); } const inner = entry.objArgs ?? {}; + assertNoRemovedProxyOptions(Object.keys(inner)); for (const key of Object.keys(inner)) { if (!VALID_PROXY_RULE_ENTRY_OPTIONS.includes(key as typeof VALID_PROXY_RULE_ENTRY_OPTIONS[number])) { throw new SchemaError( @@ -506,7 +507,6 @@ function assertProxyRulesArg(resolver: Resolver | undefined): void { assertProxyBooleanArg(inner.block, 'block'); assertProxyApprovalArg(inner.approval); assertProxySubstituteInArg(inner.substituteIn); - assertProxyMaxOccurrencesArg(inner.maxOccurrences); } } @@ -517,6 +517,7 @@ function validateProxyFunctionArgs(argsVal: Resolver): void { // Reject unknown options so a typo (e.g. `aproval=true`, `blok=true`) fails loudly // instead of silently producing a permissive rule. + assertNoRemovedProxyOptions(Object.keys(argsVal.objArgs)); for (const key of Object.keys(argsVal.objArgs)) { if (!VALID_PROXY_OPTIONS.includes(key as typeof VALID_PROXY_OPTIONS[number])) { throw new SchemaError( @@ -532,7 +533,6 @@ function validateProxyFunctionArgs(argsVal: Resolver): void { assertProxyBooleanArg(argsVal.objArgs?.block, 'block'); assertProxyApprovalArg(argsVal.objArgs?.approval); assertProxySubstituteInArg(argsVal.objArgs?.substituteIn); - assertProxyMaxOccurrencesArg(argsVal.objArgs?.maxOccurrences); assertProxyRulesArg(argsVal.objArgs?.rules); if (argsVal.arrArgs?.length) { diff --git a/packages/varlock/src/env-graph/lib/env-graph.ts b/packages/varlock/src/env-graph/lib/env-graph.ts index 2e0a86206..5b4644a84 100644 --- a/packages/varlock/src/env-graph/lib/env-graph.ts +++ b/packages/varlock/src/env-graph/lib/env-graph.ts @@ -33,7 +33,7 @@ import { isVarlockReservedKey } from './reserved-vars'; import { normalizeOverrideKeys } from '../../lib/injected-env-provenance'; import { generateProxyPlaceholderForItem } from '../../proxy/placeholder'; import { - PROXY_APPROVAL_EACH_VALUES, + PROXY_APPROVAL_EACH_VALUES, REMOVED_PROXY_RULE_OPTIONS, parseProxySubstitutionTarget, type ProxyApprovalEach, type ProxyEgressMode, type ProxyManagedItem, type ProxyRule, } from '../../proxy/types'; @@ -1206,8 +1206,10 @@ export class EnvGraph { // doesn't fire for header/root @proxy decorators), so a typo like `blok=true` // fails loudly instead of silently producing a permissive rule. Entries that // reach the recursive call have already been filtered to the per-entry set. - const validOptions = ['domain', 'path', 'method', 'keys', 'block', 'approval', 'substituteIn', 'maxOccurrences', 'rules']; + const validOptions = ['domain', 'path', 'method', 'keys', 'block', 'approval', 'substituteIn', 'rules']; for (const key of Object.keys(obj ?? {})) { + const removed = REMOVED_PROXY_RULE_OPTIONS[key]; + if (removed) throw new SchemaError(removed); if (!validOptions.includes(key)) { throw new SchemaError(`@proxy: unknown option "${key}". Valid options: ${validOptions.join(', ')}`); } @@ -1257,13 +1259,6 @@ export class EnvGraph { if (!parsed.ok) throw new SchemaError(`@proxy: ${parsed.error}`); } } - if (obj?.maxOccurrences !== undefined) { - const val = obj.maxOccurrences; - if (!_.isNumber(val) || !Number.isInteger(val) || val < 1) { - throw new SchemaError(`@proxy: maxOccurrences must resolve to an integer >= 1, got ${JSON.stringify(val)}`); - } - } - // `rules=[{...}]`: each entry is a policy refinement for the parent's domain. if (obj?.rules !== undefined) { if (!Array.isArray(obj.rules)) { @@ -1274,8 +1269,10 @@ export class EnvGraph { throw new SchemaError(`@proxy: each rules entry must be an object, got ${JSON.stringify(entry)}`); } for (const key of Object.keys(entry)) { - if (!['path', 'method', 'block', 'approval', 'substituteIn', 'maxOccurrences'].includes(key)) { - throw new SchemaError(`@proxy: unknown option "${key}" in a rules entry. Valid entry options: path, method, block, approval, substituteIn, maxOccurrences (domain and keys are set on the parent @proxy)`); + const removed = REMOVED_PROXY_RULE_OPTIONS[key]; + if (removed) throw new SchemaError(removed); + if (!['path', 'method', 'block', 'approval', 'substituteIn'].includes(key)) { + throw new SchemaError(`@proxy: unknown option "${key}" in a rules entry. Valid entry options: path, method, block, approval, substituteIn (domain and keys are set on the parent @proxy)`); } } // reuse the per-option type checks for the entry (path/method/block/approval) @@ -1329,7 +1326,6 @@ export class EnvGraph { ...(method.length ? { method } : {}), ...(_.isBoolean(obj?.block) ? { block: obj.block } : {}), ...(substituteIn.length ? { substituteIn } : {}), - ...(_.isNumber(obj?.maxOccurrences) ? { maxOccurrences: obj.maxOccurrences } : {}), ...EnvGraph.buildProxyApprovalFields(obj), }; } diff --git a/packages/varlock/src/env-graph/test/proxy-mode.test.ts b/packages/varlock/src/env-graph/test/proxy-mode.test.ts index 5d9f3f733..e613ba132 100644 --- a/packages/varlock/src/env-graph/test/proxy-mode.test.ts +++ b/packages/varlock/src/env-graph/test/proxy-mode.test.ts @@ -172,49 +172,53 @@ describe('proxy decorators', () => { expect(errors.some((e) => /path takes no argument/.test(e.message))).toBe(true); }); - test('maxOccurrences parses onto the rule', async () => { + test('the removed maxOccurrences option is rejected with migration guidance', async () => { const graph = await loadGraph(outdent` # @defaultSensitive=false # --- # @proxy(domain="api.a.com", maxOccurrences=2) API_KEY=secret `); - expect(await graph.getProxyRules()).toMatchObject([{ domain: ['api.a.com'], maxOccurrences: 2 }]); + const errors = graph.configSchema.API_KEY.decoratorSchemaErrors; + // Not the generic "unknown option" error: the message has to say what replaced it. + expect(errors.some((e) => /maxOccurrences has been removed/.test(e.message))).toBe(true); + expect(errors.some((e) => /name each one/.test(e.message))).toBe(true); }); - test('an invalid substituteIn target is rejected', async () => { + test('the removed maxOccurrences option is rejected inside a rules entry too', async () => { const graph = await loadGraph(outdent` # @defaultSensitive=false # --- - # @proxy(domain="api.a.com", substituteIn=[header, cookie]) + # @proxy(domain="api.a.com", rules=[{path="/v1/**", maxOccurrences=2}]) API_KEY=secret `); const errors = graph.configSchema.API_KEY.decoratorSchemaErrors; - expect(errors.some((e) => /invalid substituteIn target "cookie"/.test(e.message))).toBe(true); + expect(errors.some((e) => /maxOccurrences has been removed/.test(e.message))).toBe(true); }); - test('bare body (no path) is rejected — body substitution must name a path', async () => { + test('an invalid substituteIn target is rejected', async () => { const graph = await loadGraph(outdent` # @defaultSensitive=false # --- - # @proxy(domain="api.a.com", substituteIn=[header, body]) + # @proxy(domain="api.a.com", substituteIn=[header, cookie]) API_KEY=secret `); const errors = graph.configSchema.API_KEY.decoratorSchemaErrors; - expect(errors.some((e) => /body substitution requires a path/.test(e.message))).toBe(true); + expect(errors.some((e) => /invalid substituteIn target "cookie"/.test(e.message))).toBe(true); }); - test('a non-integer maxOccurrences is rejected', async () => { + test('bare body (no path) is rejected — body substitution must name a path', async () => { const graph = await loadGraph(outdent` # @defaultSensitive=false # --- - # @proxy(domain="api.a.com", maxOccurrences=0) + # @proxy(domain="api.a.com", substituteIn=[header, body]) API_KEY=secret `); const errors = graph.configSchema.API_KEY.decoratorSchemaErrors; - expect(errors.some((e) => /maxOccurrences must be an integer >= 1/.test(e.message))).toBe(true); + expect(errors.some((e) => /body substitution requires a path/.test(e.message))).toBe(true); }); + test('a header-level (detached) @proxy is not rejected as a misplaced item decorator', async () => { const graph = await loadGraph(outdent` # @proxyConfig={egress="strict"} diff --git a/packages/varlock/src/proxy/audit.test.ts b/packages/varlock/src/proxy/audit.test.ts index 1f37469e9..178916e47 100644 --- a/packages/varlock/src/proxy/audit.test.ts +++ b/packages/varlock/src/proxy/audit.test.ts @@ -80,6 +80,35 @@ describe('proxy audit log', () => { expect((lines[2] as ProxyAuditEntry).injectedKeys).toBeUndefined(); }); + test('emits one skipped-placeholder line per skipped item, sharing the request fingerprint', async () => { + const uuid = 'skipped-lines'; + const log = createProxyAuditLog(uuid); + log.record(allowActivity({ + skippedPlaceholders: [ + { key: 'API_KEY', locations: ['body'] }, + { key: 'OTHER_KEY', locations: ['header:x-debug', 'body'] }, + ], + })); + await log.flush(); + + const lines = await readProxyAuditLines(uuid); + expect(lines).toHaveLength(3); + const entry = lines[0] as ProxyAuditEntry; + expect(entry).toMatchObject({ type: 'request', decision: 'allow' }); + expect(lines[1]).toMatchObject({ + type: 'skipped-placeholder', + key: 'API_KEY', + locations: ['body'], + requestHash: entry.requestHash, + ruleId: entry.ruleId, + }); + expect(lines[2]).toMatchObject({ + type: 'skipped-placeholder', + key: 'OTHER_KEY', + locations: ['header:x-debug', 'body'], + }); + }); + test('never persists a secret value, even when injectedKeys are present', async () => { const uuid = 'no-secrets'; const log = createProxyAuditLog(uuid); diff --git a/packages/varlock/src/proxy/audit.ts b/packages/varlock/src/proxy/audit.ts index d048c5904..90d7c8767 100644 --- a/packages/varlock/src/proxy/audit.ts +++ b/packages/varlock/src/proxy/audit.ts @@ -38,6 +38,12 @@ export type ProxyActivity = { ruleId?: string; /** Keys (names, never values) of the managed items actually injected into this request. */ injectedKeys?: Array; + /** + * Injected items whose placeholder also appeared in a surface their rule doesn't + * substitute in, forwarded unsubstituted (inert). Each produces a + * `skipped-placeholder` audit line alongside the request entry. + */ + skippedPlaceholders?: Array<{ key: string; locations: Array }>; }; /** First line of every audit file — makes the file self-describing after the session record is gone. */ @@ -68,7 +74,29 @@ export type ProxyAuditEntry = { ruleId?: string; }; -export type ProxyAuditLine = ProxyAuditHeader | ProxyAuditEntry; +/** + * One skipped-placeholder event: an injected item's placeholder appeared in a + * request surface its rule has no substitution targets on, and was forwarded + * unsubstituted (an unswapped placeholder is inert). Usually benign (an agent + * quoting its own placeholder), but logged per item so probing stays visible. + */ +export type ProxyAuditSkippedPlaceholder = { + type: 'skipped-placeholder'; + ts: string; + host: string; + method: string; + /** Path only, no query, placeholder form. */ + path: string; + /** Matches the accompanying request entry's fingerprint. */ + requestHash: string; + /** Key (name, never value) of the managed item whose placeholder was skipped. */ + key: string; + /** Where the unsubstituted occurrences sat, e.g. `body`, `path`, `query`, `header:`. */ + locations: Array; + ruleId?: string; +}; + +export type ProxyAuditLine = ProxyAuditHeader | ProxyAuditEntry | ProxyAuditSkippedPlaceholder; // Resolved lazily (not a module-load const) so it honors the active // XDG_CONFIG_HOME / legacy-dir resolution at call time. Co-located in the @@ -128,7 +156,24 @@ export function createProxyAuditLog(uuid: string, header?: Omit Promise }; +export type UpstreamHandler = (req: http.IncomingMessage, res: http.ServerResponse) => void; + +export type MitmHarness = { + /** Start a stub HTTPS upstream on an ephemeral port, holding a cert the proxy trusts. */ + startUpstream: (handler: UpstreamHandler) => Promise; + /** The stub upstream's CA, for a test that needs to mint a cert of its own. */ + upstreamCa: () => EphemeralCa; +}; + +/** + * Register the harness for one test file. Call it at the top level: it installs + * the `beforeAll`/`afterAll` hooks that mint the stub upstream's CA and make the + * proxy's outbound requests trust it. + */ +export function setupMitmHarness(): MitmHarness { + let ca: EphemeralCa | undefined; + let certPem = ''; + let keyPem = ''; + let restoreGlobalCa: (() => void) | undefined; + + beforeAll(async () => { + // Stub upstream's own CA + leaf (IP SAN, since we connect by 127.0.0.1). + ca = await createEphemeralCa(); + const leaf = await createHostCert(ca, UPSTREAM_HOST); + certPem = leaf.certPem; + keyPem = leaf.keyPem; + + // The proxy dials upstreams through the global agent, so add the stub CA + // there (alongside the real roots) and restore afterwards. + const previousCa = https.globalAgent.options.ca; + https.globalAgent.options.ca = [...tls.rootCertificates, ca.certPem]; + restoreGlobalCa = () => { + https.globalAgent.options.ca = previousCa; + }; + }); + + afterAll(() => { + restoreGlobalCa?.(); + }); + + return { + startUpstream(handler: UpstreamHandler) { + const server = https.createServer({ key: keyPem, cert: certPem }, handler); + return new Promise((resolve) => { + server.listen(0, UPSTREAM_HOST, () => { + const addr = server.address(); + if (!addr || typeof addr === 'string') throw new Error('no upstream addr'); + resolve({ + port: addr.port, + close: () => new Promise((r) => { + server.close(() => r()); + }), + }); + }); + }); + }, + upstreamCa() { + if (!ca) throw new Error('mitm harness is not ready: call setupMitmHarness() at the top level of the test file'); + return ca; + }, + }; +} + +/** + * Open a CONNECT tunnel through the proxy and TLS-handshake against the proxy's + * minted leaf, trusting only the proxy CA. Resolving at all proves CA trust. + */ +export async function openMitmTunnel( + proxyUrl: string, + proxyCaPem: string, + targetPort: number, +): Promise { + const proxy = new URL(proxyUrl); + const rawSocket = net.connect(Number(proxy.port), proxy.hostname); + await new Promise((resolve, reject) => { + rawSocket.once('error', reject); + rawSocket.once('connect', () => resolve()); + }); + await new Promise((resolve, reject) => { + rawSocket.once('data', (chunk: Buffer) => { + const statusLine = chunk.toString('utf8').split('\r\n')[0] ?? ''; + if (/^HTTP\/1\.\d 200/.test(statusLine)) resolve(); + else reject(new Error(`CONNECT failed: ${statusLine}`)); + }); + rawSocket.write(`CONNECT ${UPSTREAM_HOST}:${targetPort} HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${targetPort}\r\n\r\n`); + }); + + const tlsSocket = tls.connect({ socket: rawSocket, host: UPSTREAM_HOST, ca: [proxyCaPem] }); + await new Promise((resolve, reject) => { + tlsSocket.once('error', reject); + tlsSocket.once('secureConnect', () => { + if (tlsSocket.authorized) resolve(); + else reject(tlsSocket.authorizationError ?? new Error('client did not authorize proxy leaf')); + }); + }); + return tlsSocket; +} + +/** + * Write a raw HTTP request over the tunnel and read the response. The MITM + * connection may stay keep-alive, so settle on idle rather than socket close. + */ +export async function sendAndRead(tlsSocket: tls.TLSSocket, rawRequest: string): Promise { + return new Promise((resolve, reject) => { + let buf = ''; + let idle: ReturnType; + tlsSocket.on('data', (c: Buffer) => { + buf += c.toString('utf8'); + clearTimeout(idle); + idle = setTimeout(() => resolve(buf), 250); + }); + tlsSocket.on('end', () => resolve(buf)); + tlsSocket.on('error', reject); + tlsSocket.write(rawRequest); + }); +} diff --git a/packages/varlock/src/proxy/policy.test.ts b/packages/varlock/src/proxy/policy.test.ts index 0633aa5d1..5831831bd 100644 --- a/packages/varlock/src/proxy/policy.test.ts +++ b/packages/varlock/src/proxy/policy.test.ts @@ -295,17 +295,17 @@ describe('getRequestScopedManagedItems — per-rule key scoping', () => { }); }); - describe('substitution policy (targets + occurrence cap)', () => { - test('defaults to any-header, once, when the rule sets nothing', () => { + describe('substitution policy (targets)', () => { + test('defaults to any-header when the rule sets nothing', () => { const rules = [rule({ domain: ['api.x.com'], itemKeys: ['A'] })]; const scoped = getRequestScopedManagedItems(facts('api.x.com', 'GET', '/'), rules, items); - expect(scoped[0]).toMatchObject({ key: 'A', targets: [{ location: 'header' }], maxOccurrences: 1 }); + expect(scoped[0]).toMatchObject({ key: 'A', targets: [{ location: 'header' }] }); }); - test('parses named targets (header:name, body:path) + maxOccurrences onto the scoped item', () => { + test('parses named targets (header:name, body:path) onto the scoped item', () => { const rules = [ rule({ - domain: ['api.x.com'], itemKeys: ['A'], substituteIn: ['header:authorization', 'body:client_secret'], maxOccurrences: 3, + domain: ['api.x.com'], itemKeys: ['A'], substituteIn: ['header:authorization', 'body:client_secret'], }), ]; const scoped = getRequestScopedManagedItems(facts('api.x.com', 'GET', '/'), rules, items); @@ -313,14 +313,13 @@ describe('getRequestScopedManagedItems — per-rule key scoping', () => { { location: 'header', name: 'authorization' }, { location: 'body', path: 'client_secret' }, ]); - expect(scoped[0]!.maxOccurrences).toBe(3); }); - test('merges targets (union) and maxOccurrences (max) across matching rules', () => { + test('merges targets (union) across matching rules', () => { const rules = [ rule({ domain: ['api.x.com'], itemKeys: ['A'], substituteIn: ['header'] }), rule({ - domain: ['api.x.com'], path: '/**', itemKeys: ['A'], substituteIn: ['query:api_key'], maxOccurrences: 2, + domain: ['api.x.com'], path: '/**', itemKeys: ['A'], substituteIn: ['query:api_key'], }), ]; const scoped = getRequestScopedManagedItems(facts('api.x.com', 'GET', '/x'), rules, items); @@ -328,7 +327,6 @@ describe('getRequestScopedManagedItems — per-rule key scoping', () => { { location: 'header' }, { location: 'query', name: 'api_key' }, ]); - expect(scoped[0]!.maxOccurrences).toBe(2); }); test('a withheld approval rule does not widen an unconditional key’s targets', () => { diff --git a/packages/varlock/src/proxy/policy.ts b/packages/varlock/src/proxy/policy.ts index ab24d697a..c02e44281 100644 --- a/packages/varlock/src/proxy/policy.ts +++ b/packages/varlock/src/proxy/policy.ts @@ -1,5 +1,5 @@ import { - DEFAULT_PROXY_MAX_OCCURRENCES, DEFAULT_PROXY_SUBSTITUTION_TARGETS, + DEFAULT_PROXY_SUBSTITUTION_TARGETS, parseProxySubstitutionTarget, proxySubstitutionTargetKey, type ProxyEgressMode, type ProxyManagedItem, type ProxyRule, type ProxySubstitutionTarget, } from './types'; @@ -7,13 +7,11 @@ import { /** * A managed item scoped to a single request, carrying the merged substitution * policy from the matching rules that inject it: the `targets` its placeholder may - * be substituted at, and the per-request `maxOccurrences` cap. Both are the union / - * max across the active contributing rules (any rule that adds a target or raises - * the cap wins), defaulting to any-header / once. + * be substituted at. The union across the active contributing rules (any rule that + * adds a target wins), defaulting to any header. */ export type RequestScopedManagedItem = ProxyManagedItem & { targets: Array; - maxOccurrences: number; }; /** @@ -198,13 +196,13 @@ export function getRequestScopedManagedItems( } if (allowedKeys.size === 0) return []; - // Merge the substitution policy (targets + occurrence cap) for each allowed key, - // but only from rules whose contribution is *active* for this request: plain-allow - // rules always; approval rules only when the approval gate runs - // (`includeApprovalGatedKeys`). This mirrors the key-scoping above so a withheld - // approval rule can't quietly widen where a key may be substituted. + // Merge the substitution targets for each allowed key, but only from rules whose + // contribution is *active* for this request: plain-allow rules always; approval + // rules only when the approval gate runs (`includeApprovalGatedKeys`). This + // mirrors the key-scoping above so a withheld approval rule can't quietly widen + // where a key may be substituted. Each merged target carries one substitution's + // worth of budget, so unioning targets is also what grants cardinality. const targetsByKey = new Map>(); - const maxOccByKey = new Map(); for (const rule of rules) { if (rule.block) continue; if (rule.approval && !opts?.includeApprovalGatedKeys) continue; @@ -214,7 +212,6 @@ export function getRequestScopedManagedItems( .map((raw) => parseProxySubstitutionTarget(raw)) .flatMap((r) => (r.ok ? [r.target] : [])) : DEFAULT_PROXY_SUBSTITUTION_TARGETS; - const ruleMaxOcc = rule.maxOccurrences ?? DEFAULT_PROXY_MAX_OCCURRENCES; for (const key of rule.itemKeys) { if (!allowedKeys.has(key)) continue; let targets = targetsByKey.get(key); @@ -223,7 +220,6 @@ export function getRequestScopedManagedItems( targetsByKey.set(key, targets); } for (const target of ruleTargets) targets.set(proxySubstitutionTargetKey(target), target); - maxOccByKey.set(key, Math.max(maxOccByKey.get(key) ?? 0, ruleMaxOcc)); } } @@ -232,6 +228,5 @@ export function getRequestScopedManagedItems( .map((item) => ({ ...item, targets: [...(targetsByKey.get(item.key)?.values() ?? DEFAULT_PROXY_SUBSTITUTION_TARGETS)], - maxOccurrences: maxOccByKey.get(item.key) ?? DEFAULT_PROXY_MAX_OCCURRENCES, })); } diff --git a/packages/varlock/src/proxy/proxy-substitution.test.ts b/packages/varlock/src/proxy/proxy-substitution.test.ts new file mode 100644 index 000000000..54690d055 --- /dev/null +++ b/packages/varlock/src/proxy/proxy-substitution.test.ts @@ -0,0 +1,390 @@ +import { describe, expect, test } from 'vitest'; +import { readFileSync } from 'node:fs'; + +import { startLocalProxyRuntime } from './runtime-proxy'; +import { + openMitmTunnel, sendAndRead, setupMitmHarness, UPSTREAM_HOST, +} from './mitm-test-harness'; + +// End-to-end coverage of the substitution surface: which placeholder occurrences +// are swapped for the real value, which are skipped and forwarded inert, and +// which block the request. The unit tests in runtime-proxy.test.ts cover +// checkSubstitutionGuards directly; these prove the whole pipeline agrees, down +// to the bytes the upstream receives. +// +// They run over the MITM harness rather than plain http because the proxy +// refuses to inject a secret into a cleartext connection, so any request that +// substitutes anything has to be TLS. + +const { startUpstream } = setupMitmHarness(); + +describe('proxy substitution surface (end-to-end)', () => { + test('skips a body placeholder under the header-only default (forwarded, unsubstituted, audited)', async () => { + let upstreamBody = ''; + let upstreamAuthHeader = ''; + const upstream = await startUpstream((req, res) => { + upstreamAuthHeader = String(req.headers.authorization ?? ''); + const chunks: Array = []; + req.on('data', (c) => chunks.push(c)); + req.on('end', () => { + upstreamBody = Buffer.concat(chunks).toString('utf8'); + res.statusCode = 200; + res.end('ok'); + }); + }); + + const activities: Array = []; + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'API_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + // No substituteIn → header-only default: the body is never a substitution + // surface, so a body occurrence is inert and must not brick the request. + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['API_KEY'] }], + egressMode: 'permissive', + onActivity: (a) => activities.push(a), + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + // The agent quoted its own placeholder in the body (e.g. echoed the env var) + // while also using it legitimately in the auth header. + const payload = JSON.stringify({ note: 'my key is sk-stub-PLACEHOLDER' }); + const response = await sendAndRead( + tlsSocket, + `POST /send HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Authorization: Bearer sk-stub-PLACEHOLDER\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + + // Forwarded: the header got the real value, the body bytes are untouched (the + // upstream sees the literal placeholder, which is inert). + expect(response.split('\r\n')[0]).toContain('200'); + expect(upstreamAuthHeader).toBe('Bearer sk-stub-REALKEY'); + expect(upstreamBody).toBe(payload); + expect(upstreamBody).not.toContain('sk-stub-REALKEY'); + expect(JSON.stringify(activities)).not.toContain('sk-stub-REALKEY'); + expect(activities.at(-1)).toMatchObject({ + decision: 'allow', + blocked: false, + injectedKeys: ['API_KEY'], + skippedPlaceholders: [{ key: 'API_KEY', locations: ['body'] }], + }); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('an overlapping placeholder skipped in one surface is not clobbered by a shorter one substituted there', async () => { + // SHORT's placeholder is a strict prefix of LONG's (the shape `ensureUnique` + // produces on a collision). They are targeted at DIFFERENT surfaces: SHORT only + // in the body at `note`, LONG only in headers. A per-surface replace that looked + // at just that surface's own items would match SHORT inside LONG's body bytes + // and emit `REAL_SHORT_1`, modifying text the guard called skipped and leaking + // the wrong secret into the body. + let upstreamBody = ''; + let upstreamAuthHeader = ''; + const upstream = await startUpstream((req, res) => { + upstreamAuthHeader = String(req.headers.authorization ?? ''); + const chunks: Array = []; + req.on('data', (c) => chunks.push(c)); + req.on('end', () => { + upstreamBody = Buffer.concat(chunks).toString('utf8'); + res.statusCode = 200; + res.end('ok'); + }); + }); + + const runtime = await startLocalProxyRuntime({ + managedItems: [ + { key: 'SHORT', placeholder: 'sk-stub-PH', realValue: 'sk-stub-REALSHORT' }, + { key: 'LONG', placeholder: 'sk-stub-PH_1', realValue: 'sk-stub-REALLONG' }, + ], + rules: [ + { domain: [UPSTREAM_HOST], itemKeys: ['LONG'] }, // header-only default + { domain: [UPSTREAM_HOST], itemKeys: ['SHORT'], substituteIn: ['body:note'] }, + ], + egressMode: 'permissive', + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + // LONG's placeholder in both the auth header (its own allowed surface) and the + // body field that SHORT (but not LONG) may be substituted into. + const payload = JSON.stringify({ note: 'sk-stub-PH_1' }); + const response = await sendAndRead( + tlsSocket, + `POST /send HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Authorization: Bearer sk-stub-PH_1\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + + expect(response.split('\r\n')[0]).toContain('200'); + // The header swapped LONG's own real value; the body is untouched, with LONG's + // placeholder still literal and neither real value spliced in. + expect(upstreamAuthHeader).toBe('Bearer sk-stub-REALLONG'); + expect(upstreamBody).toBe(payload); + expect(upstreamBody).not.toContain('sk-stub-REALSHORT'); + expect(upstreamBody).not.toContain('sk-stub-REALLONG'); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('still blocks an off-path body placeholder when the rule has a body: target', async () => { + let upstreamHit = false; + const upstream = await startUpstream((_req, res) => { + upstreamHit = true; + res.statusCode = 200; + res.end('ok'); + }); + + const activities: Array = []; + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'CLIENT_SECRET', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + // The body IS a substitution surface here (body:client_secret), so a stray + // occurrence at another path can't be skipped (substitution is a blind + // replace across the body) and the request fails closed. + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['CLIENT_SECRET'], substituteIn: ['body:client_secret'] }], + egressMode: 'permissive', + onActivity: (a) => activities.push(a), + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + // The blocked MITM path tears the tunnel down (see the DNS-poison test), so + // assert on the security properties + audit decision rather than reading a body. + tlsSocket.on('error', () => { /* expected: connection torn down on block */ }); + // The agent is tricked into moving the placeholder to an exfil-friendly field. + const payload = JSON.stringify({ note: 'sk-stub-PLACEHOLDER' }); + tlsSocket.write( + `POST /send HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Content-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + + // Blocked before forwarding: the upstream never saw the request, and the real + // value was never substituted (so it can't have leaked into the note field). + expect(upstreamHit).toBe(false); + expect(JSON.stringify(activities)).not.toContain('sk-stub-REALKEY'); + expect(activities.at(-1)).toMatchObject({ decision: 'blocked-location', blocked: true }); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('a Claude-style transcript request round-trips: real use in the auth header, quoted placeholder in the body', async () => { + let upstreamBody = ''; + let upstreamAuthHeader = ''; + const upstream = await startUpstream((req, res) => { + upstreamAuthHeader = String(req.headers.authorization ?? ''); + const chunks: Array = []; + req.on('data', (c) => chunks.push(c)); + req.on('end', () => { + upstreamBody = Buffer.concat(chunks).toString('utf8'); + res.setHeader('content-type', 'application/json'); + res.statusCode = 200; + res.end(JSON.stringify({ id: 'msg_1', content: [{ type: 'text', text: 'ok' }] })); + }); + }); + + const activities: Array = []; + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'ANTHROPIC_API_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['ANTHROPIC_API_KEY'] }], + egressMode: 'permissive', + onActivity: (a) => activities.push(a), + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + // The flagship `proxy run -- claude` shape: the whole conversation transcript + // travels in the JSON body, and once the agent has echoed its env var the + // placeholder appears there on EVERY subsequent request. Multiple quoted + // copies in the body must not trip the occurrence cap either. + const payload = JSON.stringify({ + model: 'claude-fable-5', + messages: [ + { role: 'user', content: 'what is ANTHROPIC_API_KEY set to?' }, + { role: 'assistant', content: 'ANTHROPIC_API_KEY=sk-stub-PLACEHOLDER' }, + { role: 'user', content: 'again?' }, + { role: 'assistant', content: 'still sk-stub-PLACEHOLDER' }, + ], + }); + const response = await sendAndRead( + tlsSocket, + `POST /v1/messages HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Authorization: Bearer sk-stub-PLACEHOLDER\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + + expect(response.split('\r\n')[0]).toContain('200'); + expect(response).toContain('msg_1'); + expect(upstreamAuthHeader).toBe('Bearer sk-stub-REALKEY'); + expect(upstreamBody).toBe(payload); // transcript bytes untouched, placeholder still literal + expect(upstreamBody).not.toContain('sk-stub-REALKEY'); + expect(activities.at(-1)).toMatchObject({ + decision: 'allow', + blocked: false, + injectedKeys: ['ANTHROPIC_API_KEY'], + skippedPlaceholders: [{ key: 'ANTHROPIC_API_KEY', locations: ['body'] }], + }); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('substitutes into the body only at the opted-in path (substituteIn=[body:client_secret])', async () => { + let upstreamBody = ''; + const upstream = await startUpstream((req, res) => { + const chunks: Array = []; + req.on('data', (c) => chunks.push(c)); + req.on('end', () => { + upstreamBody = Buffer.concat(chunks).toString('utf8'); + res.statusCode = 200; + res.end('ok'); + }); + }); + + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'CLIENT_SECRET', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['CLIENT_SECRET'], substituteIn: ['body:client_secret'] }], + egressMode: 'permissive', + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + // OAuth-style token exchange: the secret legitimately travels in the form body. + const payload = 'grant_type=client_credentials&client_secret=sk-stub-PLACEHOLDER'; + const response = await sendAndRead( + tlsSocket, + `POST /oauth/token HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Content-Type: application/x-www-form-urlencoded\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + + expect(response.split('\r\n')[0]).toContain('200'); + expect(upstreamBody).toContain('client_secret=sk-stub-REALKEY'); + expect(upstreamBody).not.toContain('PLACEHOLDER'); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('substitutes a token carried in the URL path (substituteIn=[path])', async () => { + let upstreamPath = ''; + const upstream = await startUpstream((req, res) => { + upstreamPath = req.url ?? ''; + res.statusCode = 200; + res.end('ok'); + }); + + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'PATH_TOKEN', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['PATH_TOKEN'], substituteIn: ['path'] }], + egressMode: 'permissive', + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + const response = await sendAndRead( + tlsSocket, + `GET /v1/sk-stub-PLACEHOLDER/data HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n\r\n`, + ); + + expect(response.split('\r\n')[0]).toContain('200'); + expect(upstreamPath).toBe('/v1/sk-stub-REALKEY/data'); + expect(upstreamPath).not.toContain('PLACEHOLDER'); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('blocks a request that repeats the placeholder at the same substitution target', async () => { + let upstreamHit = false; + const upstream = await startUpstream((_req, res) => { + upstreamHit = true; + res.statusCode = 200; + res.end('ok'); + }); + + const activities: Array = []; + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'API_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + // Header-only default: every header is covered by the one `header` target, so + // two headers are two substitutions at the same target. + rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['API_KEY'] }], + egressMode: 'permissive', + onActivity: (a) => activities.push(a), + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + tlsSocket.on('error', () => { /* expected: connection torn down on block */ }); + // A valid call uses the token once (the auth header); the copy in a second + // header is an exfiltration attempt that still makes a working request. + tlsSocket.write( + `GET /data HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + 'Authorization: Bearer sk-stub-PLACEHOLDER\r\nX-Duplicate: sk-stub-PLACEHOLDER\r\n\r\n', + ); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + + expect(upstreamHit).toBe(false); + expect(JSON.stringify(activities)).not.toContain('sk-stub-REALKEY'); + expect(activities.at(-1)).toMatchObject({ decision: 'blocked-occurrences', blocked: true }); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); + + test('substitutes into two separately named targets in one request (no cap to configure)', async () => { + let upstreamAuthHeader = ''; + let upstreamBody = ''; + const upstream = await startUpstream((req, res) => { + upstreamAuthHeader = String(req.headers.authorization ?? ''); + const chunks: Array = []; + req.on('data', (c) => chunks.push(c)); + req.on('end', () => { + upstreamBody = Buffer.concat(chunks).toString('utf8'); + res.statusCode = 200; + res.end('ok'); + }); + }); + + const runtime = await startLocalProxyRuntime({ + managedItems: [{ key: 'SIGNING_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], + // Two named targets: the author declared both places, so each gets one + // substitution. This used to require maxOccurrences=2. + rules: [ + { + domain: [UPSTREAM_HOST], + itemKeys: ['SIGNING_KEY'], + substituteIn: ['header:authorization', 'body:signature'], + }, + ], + egressMode: 'permissive', + }); + const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); + + const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); + const payload = JSON.stringify({ signature: 'sk-stub-PLACEHOLDER' }); + const response = await sendAndRead( + tlsSocket, + `POST /sign HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` + + `Authorization: Bearer sk-stub-PLACEHOLDER\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, + ); + + expect(response.split('\r\n')[0]).toContain('200'); + expect(upstreamAuthHeader).toBe('Bearer sk-stub-REALKEY'); + expect(upstreamBody).toBe(JSON.stringify({ signature: 'sk-stub-REALKEY' })); + + tlsSocket.destroy(); + await runtime.stop(); + await upstream.close(); + }); +}); diff --git a/packages/varlock/src/proxy/proxy-tls.test.ts b/packages/varlock/src/proxy/proxy-tls.test.ts index f56e68567..ec8f4d853 100644 --- a/packages/varlock/src/proxy/proxy-tls.test.ts +++ b/packages/varlock/src/proxy/proxy-tls.test.ts @@ -1,113 +1,22 @@ -import { - afterAll, beforeAll, describe, expect, test, -} from 'vitest'; +import { describe, expect, test } from 'vitest'; import { readFileSync } from 'node:fs'; import https from 'node:https'; -import net from 'node:net'; -import tls from 'node:tls'; -import { URL } from 'node:url'; import { startLocalProxyRuntime } from './runtime-proxy'; -import { createEphemeralCa, createHostCert, type EphemeralCa } from './cert-authority'; - -// End-to-end exercise of the HTTPS MITM path: a real TLS client, trusting only -// the proxy's CA, opens a CONNECT tunnel and handshakes against the proxy's -// minted leaf; the proxy injects the real secret and forwards to a stub HTTPS -// upstream. Covers the cert-trust + CONNECT + injection + streaming mechanics -// that the plain-HTTP unit tests can't reach. - -const UPSTREAM_HOST = '127.0.0.1'; -let upstreamCa: EphemeralCa; -let upstreamCertPem: string; -let upstreamKeyPem: string; -let restoreGlobalCa: () => void; - -beforeAll(async () => { - // Stub upstream's own CA + leaf (IP SAN, since we connect by 127.0.0.1). - upstreamCa = await createEphemeralCa(); - const leaf = await createHostCert(upstreamCa, UPSTREAM_HOST); - upstreamCertPem = leaf.certPem; - upstreamKeyPem = leaf.keyPem; - - // Make the proxy's outbound https.request trust the stub upstream. The proxy - // uses the global agent, so inject the upstream CA there (alongside the real - // roots) and restore afterwards. - const previousCa = https.globalAgent.options.ca; - https.globalAgent.options.ca = [...tls.rootCertificates, upstreamCa.certPem]; - restoreGlobalCa = () => { - https.globalAgent.options.ca = previousCa; - }; -}); - -afterAll(() => { - restoreGlobalCa?.(); -}); +import { createHostCert } from './cert-authority'; +import { + openMitmTunnel, sendAndRead, setupMitmHarness, UPSTREAM_HOST, +} from './mitm-test-harness'; -function startUpstream(handler: (req: import('node:http').IncomingMessage, res: import('node:http').ServerResponse) => void) { - const server = https.createServer({ key: upstreamKeyPem, cert: upstreamCertPem }, handler); - return new Promise<{ port: number; close: () => Promise }>((resolve) => { - server.listen(0, UPSTREAM_HOST, () => { - const addr = server.address(); - if (!addr || typeof addr === 'string') throw new Error('no upstream addr'); - resolve({ - port: addr.port, - close: () => new Promise((r) => { - server.close(() => r()); - }), - }); - }); - }); -} - -// Open a CONNECT tunnel through the proxy and TLS-handshake against the proxy's -// minted leaf, trusting only the proxy CA. Resolving at all proves CA trust. -async function openMitmTunnel( - proxyUrl: string, - proxyCaPem: string, - targetPort: number, -): Promise { - const proxy = new URL(proxyUrl); - const rawSocket = net.connect(Number(proxy.port), proxy.hostname); - await new Promise((resolve, reject) => { - rawSocket.once('error', reject); - rawSocket.once('connect', () => resolve()); - }); - await new Promise((resolve, reject) => { - rawSocket.once('data', (chunk: Buffer) => { - const statusLine = chunk.toString('utf8').split('\r\n')[0] ?? ''; - if (/^HTTP\/1\.\d 200/.test(statusLine)) resolve(); - else reject(new Error(`CONNECT failed: ${statusLine}`)); - }); - rawSocket.write(`CONNECT ${UPSTREAM_HOST}:${targetPort} HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${targetPort}\r\n\r\n`); - }); +// End-to-end exercise of the HTTPS MITM transport: a real TLS client, trusting +// only the proxy's CA, opens a CONNECT tunnel and handshakes against the proxy's +// minted leaf, and the proxy forwards to a stub HTTPS upstream. Covers the +// cert-trust + CONNECT + streaming + response-scrubbing mechanics that the +// plain-HTTP unit tests can't reach. Which parts of a request get substituted is +// covered in proxy-substitution.test.ts. - const tlsSocket = tls.connect({ socket: rawSocket, host: UPSTREAM_HOST, ca: [proxyCaPem] }); - await new Promise((resolve, reject) => { - tlsSocket.once('error', reject); - tlsSocket.once('secureConnect', () => { - if (tlsSocket.authorized) resolve(); - else reject(tlsSocket.authorizationError ?? new Error('client did not authorize proxy leaf')); - }); - }); - return tlsSocket; -} - -// Write a raw HTTP request over the tunnel and read the response (the MITM -// connection may stay keep-alive, so settle on idle rather than socket close). -async function sendAndRead(tlsSocket: tls.TLSSocket, rawRequest: string): Promise { - return new Promise((resolve, reject) => { - let buf = ''; - let idle: ReturnType; - tlsSocket.on('data', (c: Buffer) => { - buf += c.toString('utf8'); - clearTimeout(idle); - idle = setTimeout(() => resolve(buf), 250); - }); - tlsSocket.on('end', () => resolve(buf)); - tlsSocket.on('error', reject); - tlsSocket.write(rawRequest); - }); -} +const harness = setupMitmHarness(); +const { startUpstream } = harness; describe('proxy HTTPS MITM (end-to-end)', () => { test('client trusts the minted leaf and the real key is injected upstream', async () => { @@ -218,7 +127,7 @@ describe('proxy HTTPS MITM (end-to-end)', () => { // The upstream listens on 127.0.0.1 but presents a cert for a DIFFERENT // name — exactly what a DNS-poisoned / rebound host does, since it cannot // obtain a valid cert for the host the rule targets. - const wrongLeaf = await createHostCert(upstreamCa, 'wrong.example'); + const wrongLeaf = await createHostCert(harness.upstreamCa(), 'wrong.example'); let upstreamGotRequest = false; let upstreamAuth = ''; const server = https.createServer({ key: wrongLeaf.keyPem, cert: wrongLeaf.certPem }, (req, res) => { @@ -413,163 +322,4 @@ describe('proxy HTTPS MITM (end-to-end)', () => { await runtime.stop(); await upstream.close(); }); - - test('blocks (does not substitute) a placeholder placed in the body under the header-only default', async () => { - let upstreamHit = false; - let upstreamBody = ''; - const upstream = await startUpstream((req, res) => { - upstreamHit = true; - const chunks: Array = []; - req.on('data', (c) => chunks.push(c)); - req.on('end', () => { - upstreamBody = Buffer.concat(chunks).toString('utf8'); - res.statusCode = 200; - res.end('ok'); - }); - }); - - const activities: Array = []; - const runtime = await startLocalProxyRuntime({ - managedItems: [{ key: 'API_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], - // No substituteIn → header-only default. - rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['API_KEY'] }], - egressMode: 'permissive', - onActivity: (a) => activities.push(a), - }); - const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); - - const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); - // The blocked MITM path tears the tunnel down (see the DNS-poison test), so - // assert on the security properties + audit decision rather than reading a body. - tlsSocket.on('error', () => { /* expected: connection torn down on block */ }); - // The agent is tricked into putting the placeholder in the request body (e.g. an - // email body on an allowed host) instead of the auth header. - const payload = JSON.stringify({ to: 'attacker@evil.test', text: 'sk-stub-PLACEHOLDER' }); - tlsSocket.write( - `POST /send HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` - + `Content-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, - ); - await new Promise((resolve) => { - setTimeout(resolve, 500); - }); - - // Blocked before forwarding: the upstream never saw the request, and the real - // value was never substituted (so it can't have leaked into the email body). - expect(upstreamHit).toBe(false); - expect(upstreamBody).toBe(''); - expect(JSON.stringify(activities)).not.toContain('sk-stub-REALKEY'); - expect(activities.at(-1)).toMatchObject({ decision: 'blocked-location', blocked: true }); - - tlsSocket.destroy(); - await runtime.stop(); - await upstream.close(); - }); - - test('substitutes into the body only at the opted-in path (substituteIn=[body:client_secret])', async () => { - let upstreamBody = ''; - const upstream = await startUpstream((req, res) => { - const chunks: Array = []; - req.on('data', (c) => chunks.push(c)); - req.on('end', () => { - upstreamBody = Buffer.concat(chunks).toString('utf8'); - res.statusCode = 200; - res.end('ok'); - }); - }); - - const runtime = await startLocalProxyRuntime({ - managedItems: [{ key: 'CLIENT_SECRET', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], - rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['CLIENT_SECRET'], substituteIn: ['body:client_secret'] }], - egressMode: 'permissive', - }); - const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); - - const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); - // OAuth-style token exchange: the secret legitimately travels in the form body. - const payload = 'grant_type=client_credentials&client_secret=sk-stub-PLACEHOLDER'; - const response = await sendAndRead( - tlsSocket, - `POST /oauth/token HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` - + `Content-Type: application/x-www-form-urlencoded\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, - ); - - expect(response.split('\r\n')[0]).toContain('200'); - expect(upstreamBody).toContain('client_secret=sk-stub-REALKEY'); - expect(upstreamBody).not.toContain('PLACEHOLDER'); - - tlsSocket.destroy(); - await runtime.stop(); - await upstream.close(); - }); - - test('substitutes a token carried in the URL path (substituteIn=[path])', async () => { - let upstreamPath = ''; - const upstream = await startUpstream((req, res) => { - upstreamPath = req.url ?? ''; - res.statusCode = 200; - res.end('ok'); - }); - - const runtime = await startLocalProxyRuntime({ - managedItems: [{ key: 'PATH_TOKEN', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], - rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['PATH_TOKEN'], substituteIn: ['path'] }], - egressMode: 'permissive', - }); - const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); - - const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); - const response = await sendAndRead( - tlsSocket, - `GET /v1/sk-stub-PLACEHOLDER/data HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n\r\n`, - ); - - expect(response.split('\r\n')[0]).toContain('200'); - expect(upstreamPath).toBe('/v1/sk-stub-REALKEY/data'); - expect(upstreamPath).not.toContain('PLACEHOLDER'); - - tlsSocket.destroy(); - await runtime.stop(); - await upstream.close(); - }); - - test('blocks a request that repeats the placeholder more than the occurrence cap', async () => { - let upstreamHit = false; - const upstream = await startUpstream((_req, res) => { - upstreamHit = true; - res.statusCode = 200; - res.end('ok'); - }); - - const activities: Array = []; - const runtime = await startLocalProxyRuntime({ - managedItems: [{ key: 'API_KEY', placeholder: 'sk-stub-PLACEHOLDER', realValue: 'sk-stub-REALKEY' }], - // Both placements are allowed (header + body:leak), but the default cap of 1 - // still stops the duplicated copy. - rules: [{ domain: [UPSTREAM_HOST], itemKeys: ['API_KEY'], substituteIn: ['header', 'body:leak'] }], - egressMode: 'permissive', - onActivity: (a) => activities.push(a), - }); - const proxyCaPem = readFileSync(runtime.env.NODE_EXTRA_CA_CERTS!, 'utf8'); - - const tlsSocket = await openMitmTunnel(runtime.env.HTTP_PROXY!, proxyCaPem, upstream.port); - tlsSocket.on('error', () => { /* expected: connection torn down on block */ }); - // A valid call uses the token once (header); the second copy in the body is an - // exfiltration attempt while still making a working request. - const payload = JSON.stringify({ leak: 'sk-stub-PLACEHOLDER' }); - tlsSocket.write( - `POST /send HTTP/1.1\r\nHost: ${UPSTREAM_HOST}:${upstream.port}\r\nConnection: close\r\n` - + `Authorization: Bearer sk-stub-PLACEHOLDER\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}`, - ); - await new Promise((resolve) => { - setTimeout(resolve, 500); - }); - - expect(upstreamHit).toBe(false); - expect(JSON.stringify(activities)).not.toContain('sk-stub-REALKEY'); - expect(activities.at(-1)).toMatchObject({ decision: 'blocked-occurrences', blocked: true }); - - tlsSocket.destroy(); - await runtime.stop(); - await upstream.close(); - }); }); diff --git a/packages/varlock/src/proxy/runtime-proxy.test.ts b/packages/varlock/src/proxy/runtime-proxy.test.ts index 29df54247..003732990 100644 --- a/packages/varlock/src/proxy/runtime-proxy.test.ts +++ b/packages/varlock/src/proxy/runtime-proxy.test.ts @@ -11,7 +11,7 @@ import { URL } from 'node:url'; import type { ProxyActivity } from './audit'; import { checkSubstitutionGuards, dataPlaneAuthOk, findUninjectedPlaceholder, parseProxyAuthToken, - replacePlaceholdersWithReal, startLocalProxyRuntime, + startLocalProxyRuntime, substitutePlaceholdersInSurface, type SubstitutionGuardRequest, } from './runtime-proxy'; import type { RequestScopedManagedItem } from './policy'; @@ -59,16 +59,50 @@ describe('findUninjectedPlaceholder (helpful-failure guard)', () => { }); }); -describe('replacePlaceholdersWithReal', () => { +describe('substitutePlaceholdersInSurface', () => { + // A's placeholder is a strict prefix of B's: the shape `ensureUnique` produces + // when two items' derived placeholders collide (it appends `_1`). + const overlapping = [ + { key: 'A', placeholder: 'vlk_x', realValue: 'REAL_A' }, + { key: 'B', placeholder: 'vlk_x_1', realValue: 'REAL_B' }, + ] as any; + const allKeys = new Set(['A', 'B']); + test('substitutes the longest placeholder first so substring placeholders are not corrupted', () => { - // P1 is a prefix of P2 — naive left-to-right replacement would splice R1 into - // P2's text and never match P2 correctly. - const managedItems = [ - { key: 'A', placeholder: 'vlk_x', realValue: 'REAL_A' }, - { key: 'B', placeholder: 'vlk_x_1', realValue: 'REAL_B' }, - ]; + // Naive left-to-right replacement would splice REAL_A into B's text and never + // match B correctly. const input = 'a=vlk_x&b=vlk_x_1'; - expect(replacePlaceholdersWithReal(input, managedItems as any)).toBe('a=REAL_A&b=REAL_B'); + expect(substitutePlaceholdersInSurface(input, overlapping, allKeys)).toBe('a=REAL_A&b=REAL_B'); + }); + + test('leaves a skipped placeholder untouched even when a substitutable one is its prefix', () => { + // B is skipped in this surface (not in substituteKeys) while A, a prefix of B's + // placeholder, is substitutable. Matching only A's list would rewrite B's bytes + // into `REAL_A_1`, modifying supposedly inert text AND emitting the wrong secret. + const input = 'note=vlk_x_1'; + expect(substitutePlaceholdersInSurface(input, overlapping, new Set(['A']))).toBe('note=vlk_x_1'); + }); + + test('still substitutes the shorter placeholder where it stands on its own', () => { + const input = 'auth=vlk_x¬e=vlk_x_1'; + expect(substitutePlaceholdersInSurface(input, overlapping, new Set(['A']))).toBe('auth=REAL_A¬e=vlk_x_1'); + }); + + test('substitutes nothing when the surface allows no items', () => { + expect(substitutePlaceholdersInSurface('a=vlk_x&b=vlk_x_1', overlapping, new Set())).toBe('a=vlk_x&b=vlk_x_1'); + }); + + test('never rescans a substituted value, so a real value containing a placeholder is left alone', () => { + const items = [ + { key: 'A', placeholder: 'PH_A', realValue: 'real-with-PH_B-inside' }, + { key: 'B', placeholder: 'PH_B', realValue: 'REAL_B' }, + ] as any; + expect(substitutePlaceholdersInSurface('x=PH_A', items, new Set(['A', 'B']))).toBe('x=real-with-PH_B-inside'); + }); + + test('ignores empty placeholders and returns the input unchanged when there are none', () => { + expect(substitutePlaceholdersInSurface('anything', [{ key: 'C', placeholder: '', realValue: 'RC' }] as any, new Set(['C']))) + .toBe('anything'); }); }); @@ -81,127 +115,265 @@ describe('checkSubstitutionGuards', () => { placeholder: 'vlk_ph_key', realValue: 'sk-real', targets: [{ location: 'header' }], - maxOccurrences: 1, ...over, }); const jsonBody = (obj: unknown): Partial => ({ body: JSON.stringify(obj), contentType: 'application/json', }); + const ok = { violation: undefined }; + test('allows a placeholder in an allowed header within the occurrence cap', () => { const req = { ...emptyReq, headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key' }] }; - expect(checkSubstitutionGuards(req, [item()])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item()])).toMatchObject({ ...ok, injectedKeys: ['API_KEY'], skipped: [] }); }); - test('blocks a placeholder in the body under the any-header default', () => { + test('skips (does not substitute) a body placeholder under the any-header default', () => { + // The agent quoted its own placeholder in the body (e.g. a conversation + // transcript echoing the env var). The body is never a substitution surface for + // this item, so the occurrence is inert: forward it, report it as skipped. const req = { ...emptyReq, ...jsonBody({ note: 'vlk_ph_key' }) }; - expect(checkSubstitutionGuards(req, [item()])).toMatchObject({ kind: 'location', location: 'body' }); + const result = checkSubstitutionGuards(req, [item()]); + expect(result.violation).toBeUndefined(); + expect(result.injectedKeys).toEqual([]); // nothing at an allowed target, so nothing injected + expect(result.skipped).toMatchObject([{ item: { key: 'API_KEY' }, locations: ['body'] }]); + }); + + test('a header use plus skipped body occurrences: forwarded, body spends no target budget', () => { + const req = { + ...emptyReq, + headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key' }], + ...jsonBody({ note: 'vlk_ph_key', quoted: 'echo vlk_ph_key' }), + }; + // Header-only default: the two body copies belong to no target, so the single + // `header` substitution is still within budget. + const result = checkSubstitutionGuards(req, [item()]); + expect(result.violation).toBeUndefined(); + expect(result.injectedKeys).toEqual(['API_KEY']); + expect(result.skipped).toMatchObject([{ item: { key: 'API_KEY' }, locations: ['body'] }]); }); test('allows a body placeholder only at the exact path it was widened to', () => { const req = { ...emptyReq, ...jsonBody({ client_secret: 'vlk_ph_key' }) }; - expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: 'client_secret' }] })])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: 'client_secret' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); }); test('blocks a body placeholder at a DIFFERENT path than the one allowed (the exfil case)', () => { // body:client_secret is allowed, but the agent put the placeholder in `note` - // instead — a path-level guard catches this; a coarse "body" bucket would not. + // instead. Since the body IS a substitution surface for this item, the blind + // body replace can't skip the stray occurrence: fail closed, no skipping. const req = { ...emptyReq, ...jsonBody({ note: 'vlk_ph_key' }) }; expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: 'client_secret' }] })])) - .toMatchObject({ kind: 'location', location: 'body' }); + .toMatchObject({ violation: { kind: 'location', location: 'body' } }); }); - test('the any-header default excludes denylisted forward/log headers (cookie, x-forwarded-*, ...)', () => { - // Placeholder redirected into a header the upstream might forward/log — blocked - // even though the item allows "any header". + test('the any-header default skips denylisted forward/log headers (cookie, x-forwarded-*, ...)', () => { + // Placeholder redirected into a header the upstream might forward/log: not + // substituted there (so it stays inert), and reported as skipped. for (const name of ['cookie', 'x-forwarded-for', 'host', 'referer', 'user-agent']) { const req = { ...emptyReq, headers: [{ name, value: 'x vlk_ph_key y' }] }; - expect(checkSubstitutionGuards(req, [item()])).toMatchObject({ kind: 'location', location: 'header' }); + expect(checkSubstitutionGuards(req, [item()])) + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ locations: [`header:${name}`] }] }); } }); test('an explicit header: target overrides the denylist', () => { const req = { ...emptyReq, headers: [{ name: 'cookie', value: 'session=vlk_ph_key' }] }; - expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'header', name: 'cookie' }] })])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'header', name: 'cookie' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'], skipped: [] }); }); - test('pins to a specific header name', () => { + test('pins to a specific header name; other headers are skipped', () => { const allowed = item({ targets: [{ location: 'header', name: 'authorization' }] }); const inAuth = { ...emptyReq, headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key' }] }; - expect(checkSubstitutionGuards(inAuth, [allowed])).toBeUndefined(); + expect(checkSubstitutionGuards(inAuth, [allowed])).toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); const inOther = { ...emptyReq, headers: [{ name: 'x-evil', value: 'vlk_ph_key' }] }; - expect(checkSubstitutionGuards(inOther, [allowed])).toMatchObject({ kind: 'location', location: 'header' }); + expect(checkSubstitutionGuards(inOther, [allowed])) + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ locations: ['header:x-evil'] }] }); }); - test('blocks a placeholder in the URL path by default, allows it with substituteIn=[path]', () => { + test('skips a URL-path placeholder by default, substitutes it with substituteIn=[path]', () => { const req = { ...emptyReq, requestTarget: '/v1/vlk_ph_key/data' }; - expect(checkSubstitutionGuards(req, [item()])).toMatchObject({ kind: 'location', location: 'path' }); - expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'path' }] })])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item()])) + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ locations: ['path'] }] }); + expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'path' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'], skipped: [] }); }); test('path and query are distinct: a path token is not covered by bare query (and vice versa)', () => { const inPath = { ...emptyReq, requestTarget: '/v1/vlk_ph_key/data?page=2' }; expect(checkSubstitutionGuards(inPath, [item({ targets: [{ location: 'query' }] })])) - .toMatchObject({ kind: 'location', location: 'path' }); + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ locations: ['path'] }] }); const inQuery = { ...emptyReq, requestTarget: '/v1/data?token=vlk_ph_key' }; expect(checkSubstitutionGuards(inQuery, [item({ targets: [{ location: 'path' }] })])) - .toMatchObject({ kind: 'location', location: 'query' }); + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ locations: ['query'] }] }); // ...and bare query does cover the query string - expect(checkSubstitutionGuards(inQuery, [item({ targets: [{ location: 'query' }] })])).toBeUndefined(); + expect(checkSubstitutionGuards(inQuery, [item({ targets: [{ location: 'query' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'], skipped: [] }); }); - test('allows a placeholder in a named query param', () => { + test('allows a placeholder in a named query param, blocks it in a different param', () => { const req = { ...emptyReq, requestTarget: '/v1?api_key=vlk_ph_key' }; - expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'query', name: 'api_key' }] })])).toBeUndefined(); - // ...but not in a different param + expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'query', name: 'api_key' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); + // The query IS a substitution surface for this item (query:api_key), so a stray + // occurrence in another param fails closed: the query is substituted as one + // string and can't skip it. const other = { ...emptyReq, requestTarget: '/v1?leak=vlk_ph_key' }; expect(checkSubstitutionGuards(other, [item({ targets: [{ location: 'query', name: 'api_key' }] })])) - .toMatchObject({ kind: 'location', location: 'query' }); + .toMatchObject({ violation: { kind: 'location', location: 'query' } }); }); - test('blocks when a placeholder appears more times than the occurrence cap', () => { - // Valid use in the header PLUS an exfil copy at the same body path (both allowed). + test('blocks two occurrences that land on the SAME target (bare header covers both)', () => { + // Legit auth header + attacker copy in another header: both are covered by the + // one `header` target, so both would be substituted and which copy is the real + // use is ambiguous. Fail closed. const req = { ...emptyReq, - headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key' }], - ...jsonBody({ client_secret: 'vlk_ph_key' }), + headers: [ + { name: 'authorization', value: 'Bearer vlk_ph_key' }, + { name: 'x-duplicate', value: 'vlk_ph_key' }, + ], }; - const allowed = item({ targets: [{ location: 'header' }, { location: 'body', path: 'client_secret' }] }); - expect(checkSubstitutionGuards(req, [allowed])).toMatchObject({ kind: 'occurrences', count: 2 }); + expect(checkSubstitutionGuards(req, [item()])) + .toMatchObject({ violation: { kind: 'occurrences', target: 'header', count: 2 } }); }); - test('allows repeated occurrences when maxOccurrences is raised', () => { + test('blocks two occurrences in the same header value', () => { + const req = { ...emptyReq, headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key vlk_ph_key' }] }; + expect(checkSubstitutionGuards(req, [item()])) + .toMatchObject({ violation: { kind: 'occurrences', target: 'header', count: 2 } }); + }); + + test('allows one occurrence per DISTINCT target with no extra configuration', () => { + // The author declared both places legitimate, so each gets its own substitution. + // This is the case that used to need maxOccurrences=2. const req = { ...emptyReq, headers: [{ name: 'authorization', value: 'Bearer vlk_ph_key' }], - ...jsonBody({ client_secret: 'vlk_ph_key' }), + ...jsonBody({ signature: 'vlk_ph_key' }), }; - const allowed = item({ targets: [{ location: 'header' }, { location: 'body', path: 'client_secret' }], maxOccurrences: 2 }); - expect(checkSubstitutionGuards(req, [allowed])).toBeUndefined(); + const allowed = item({ + targets: [{ location: 'header', name: 'authorization' }, { location: 'body', path: 'signature' }], + }); + expect(checkSubstitutionGuards(req, [allowed])).toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); + }); + + test('allows the same secret in two separately named headers', () => { + const req = { + ...emptyReq, + headers: [ + { name: 'authorization', value: 'Bearer vlk_ph_key' }, + { name: 'x-api-key', value: 'vlk_ph_key' }, + ], + }; + const allowed = item({ + targets: [{ location: 'header', name: 'authorization' }, { location: 'header', name: 'x-api-key' }], + }); + expect(checkSubstitutionGuards(req, [allowed])).toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); + }); + + test('the broadest allowing target wins, so declaring header AND header: grants no extra budget', () => { + // Without this, an occurrence in `authorization` could bill to `header:authorization` + // while a second in `x-evil` billed to `header`, letting two copies through. + const req = { + ...emptyReq, + headers: [ + { name: 'authorization', value: 'Bearer vlk_ph_key' }, + { name: 'x-evil', value: 'vlk_ph_key' }, + ], + }; + const allowed = item({ targets: [{ location: 'header' }, { location: 'header', name: 'authorization' }] }); + expect(checkSubstitutionGuards(req, [allowed])) + .toMatchObject({ violation: { kind: 'occurrences', target: 'header', count: 2 } }); }); test('fails closed when a body:path target is set but the body cannot be parsed', () => { const req = { ...emptyReq, body: 'vlk_ph_key not-json', contentType: 'application/json' }; expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: 'client_secret' }] })])) - .toMatchObject({ kind: 'location', location: 'body' }); + .toMatchObject({ violation: { kind: 'location', location: 'body' } }); + }); + + describe('overlapping placeholders are classified exactly as substitution matches them', () => { + // SHORT's placeholder is a strict prefix of LONG's. Counting per item with a + // plain substring search would charge SHORT for bytes that belong to LONG and + // that substitution never touches, blocking requests where nothing would have + // been substituted at all. + const SHORT = item({ key: 'SHORT', placeholder: 'vlk_x', realValue: 'REAL_SHORT' }); + const LONG = item({ key: 'LONG', placeholder: 'vlk_x_1', realValue: 'REAL_LONG' }); + const bodyTargeted = { ...SHORT, targets: [{ location: 'body' as const, path: 'note' }] }; + + test('a longer skipped placeholder is not counted as an occurrence of its shorter prefix', () => { + // Two of LONG's placeholders in the body; LONG has no body target, so both are + // skipped. SHORT may substitute at body:note but has no occurrence of its own. + const req = { ...emptyReq, ...jsonBody({ note: 'vlk_x_1', other: 'vlk_x_1' }) }; + const result = checkSubstitutionGuards(req, [bodyTargeted, LONG]); + expect(result.violation).toBeUndefined(); + expect(result.injectedKeys).toEqual([]); + expect(result.skipped).toMatchObject([{ item: { key: 'LONG' }, locations: ['body'] }]); + }); + + test('a longer placeholder outside the shorter item’s named body path is not an off-path violation', () => { + const req = { ...emptyReq, ...jsonBody({ elsewhere: 'vlk_x_1' }) }; + expect(checkSubstitutionGuards(req, [bodyTargeted, LONG])) + .toMatchObject({ ...ok, injectedKeys: [], skipped: [{ item: { key: 'LONG' }, locations: ['body'] }] }); + }); + + test('the shorter placeholder still counts where it stands on its own', () => { + const req = { ...emptyReq, ...jsonBody({ note: 'vlk_x', other: 'vlk_x_1' }) }; + const result = checkSubstitutionGuards(req, [bodyTargeted, LONG]); + expect(result.violation).toBeUndefined(); + expect(result.injectedKeys).toEqual(['SHORT']); + expect(result.skipped).toMatchObject([{ item: { key: 'LONG' }, locations: ['body'] }]); + }); + + test('a longer skipped placeholder in a header is not charged to its shorter prefix', () => { + // SHORT allows any header; the header holds LONG's placeholder, and LONG's + // rule is header-only too, so LONG is the one injected. + const req = { ...emptyReq, headers: [{ name: 'authorization', value: 'Bearer vlk_x_1' }] }; + const result = checkSubstitutionGuards(req, [SHORT, LONG]); + expect(result.violation).toBeUndefined(); + expect(result.injectedKeys).toEqual(['LONG']); + }); + + test('two longer placeholders in headers do not trip the shorter item’s per-target budget', () => { + const req = { + ...emptyReq, + headers: [ + { name: 'authorization', value: 'Bearer vlk_x_1' }, + { name: 'x-other', value: 'vlk_x_1' }, + ], + }; + // LONG legitimately blocks (two copies at its own `header` target), but the + // violation must be LONG's, never SHORT's. + expect(checkSubstitutionGuards(req, [SHORT, LONG])) + .toMatchObject({ + violation: { + kind: 'occurrences', item: { key: 'LONG' }, target: 'header', count: 2, + }, + }); + }); }); test('body:* allows the placeholder anywhere in an unparseable (e.g. XML) body', () => { const xml = 'vlk_ph_key'; const req = { ...emptyReq, body: xml, contentType: 'application/xml' }; - expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: '*' }] })])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: '*' }] })])) + .toMatchObject({ ...ok, injectedKeys: ['API_KEY'] }); }); test('body:* still respects the occurrence cap', () => { // Two copies in an unstructured body — anywhere is allowed, but the default cap is 1. const req = { ...emptyReq, body: 'sig=vlk_ph_key&dup=vlk_ph_key', contentType: 'text/plain' }; expect(checkSubstitutionGuards(req, [item({ targets: [{ location: 'body', path: '*' }] })])) - .toMatchObject({ kind: 'occurrences', count: 2 }); + .toMatchObject({ violation: { kind: 'occurrences', count: 2 } }); }); test('ignores items with an empty placeholder', () => { const req = { ...emptyReq, body: 'anything' }; - expect(checkSubstitutionGuards(req, [item({ placeholder: '' })])).toBeUndefined(); + expect(checkSubstitutionGuards(req, [item({ placeholder: '' })])) + .toMatchObject({ ...ok, injectedKeys: [], skipped: [] }); }); }); diff --git a/packages/varlock/src/proxy/runtime-proxy.ts b/packages/varlock/src/proxy/runtime-proxy.ts index de4c2be48..63b307603 100644 --- a/packages/varlock/src/proxy/runtime-proxy.ts +++ b/packages/varlock/src/proxy/runtime-proxy.ts @@ -146,7 +146,7 @@ export type SessionEnvPayloadMeta = { type HostInfo = { host: string, port: number }; -type HeaderTransformFn = (value: string) => string; +type HeaderTransformFn = (value: string, name: string) => string; function parseHostPort(value: string): HostInfo | null { // Parse via URL so bracketed IPv6 literals (`[::1]:443`) are handled — a plain @@ -270,21 +270,65 @@ async function runApprovalGate(input: { } } +/** One managed placeholder occurrence located in a request surface. */ +type PlaceholderMatch = { item: ProxyManagedItem; index: number }; + /** - * Number of non-overlapping occurrences of `needle` in `haystack`. Uses an - * indexOf scan rather than `split` so it stays O(n) time / O(1) extra space: an - * untrusted agent controls the request and could repeat a placeholder many times, - * and `split` would allocate an array proportional to the match count. + * Every managed placeholder occurrence in `value`, left to right, matched + * **leftmost-longest** across all of `allItems` and never overlapping. + * + * This is the single source of truth for "where are the placeholders", shared by + * the guard and the substitution so the two can never disagree. That matters + * because placeholders can contain one another (`ensureUnique` resolves a + * collision by appending `_1`, and explicit `@placeholder` values can overlap + * freely): a per-item substring search would charge the shorter item for bytes + * that belong to the longer one, and substitution would then leave those bytes + * alone, so the guard could block a request in which nothing would have been + * substituted at all. + * + * Scanning is an indexOf sweep per placeholder with each item's next match cached, + * rather than a `split`, so an agent repeating a placeholder many times can't make + * this allocate proportionally to the match count in the common (no-match) case. */ -function countOccurrences(haystack: string, needle: string): number { - if (!needle) return 0; - let count = 0; - let idx = haystack.indexOf(needle); - while (idx !== -1) { - count += 1; - idx = haystack.indexOf(needle, idx + needle.length); +function findPlaceholderMatches(value: string, allItems: Array): Array { + // Longest first, so on a tie at the same index the longer placeholder wins. + const items = allItems + .filter((item) => !!item.placeholder) + .sort((a, b) => b.placeholder.length - a.placeholder.length); + const matches: Array = []; + if (!items.length) return matches; + + const nextIndex = items.map((item) => value.indexOf(item.placeholder)); + let pos = 0; + while (true) { + let bestIdx = -1; + let bestItem = -1; + for (let i = 0; i < items.length; i += 1) { + // A cached index inside an already-consumed match is stale; re-scan from pos. + if (nextIndex[i] !== -1 && nextIndex[i]! < pos) { + nextIndex[i] = value.indexOf(items[i]!.placeholder, pos); + } + const idx = nextIndex[i]!; + if (idx === -1) continue; + if (bestIdx === -1 || idx < bestIdx) { + bestIdx = idx; + bestItem = i; + } + } + if (bestIdx === -1) return matches; + const item = items[bestItem]!; + matches.push({ item, index: bestIdx }); + pos = bestIdx + item.placeholder.length; } - return count; +} + +/** Per-item-key occurrence counts in one surface, using the shared matcher. */ +function countPlaceholderMatches(value: string, allItems: Array): Map { + const counts = new Map(); + for (const match of findPlaceholderMatches(value, allItems)) { + counts.set(match.item.key, (counts.get(match.item.key) ?? 0) + 1); + } + return counts; } /** A request decomposed into the parts the substitution guards inspect. */ @@ -300,7 +344,28 @@ export type SubstitutionGuardRequest = { }; export type SubstitutionGuardViolation = | { kind: 'location'; item: RequestScopedManagedItem; location: ProxySubstitutionLocation; suggestion: string } - | { kind: 'occurrences'; item: RequestScopedManagedItem; count: number }; + /** More than one occurrence at the same substitution target (`target` is its key). */ + | { kind: 'occurrences'; item: RequestScopedManagedItem; target: string; count: number }; + +/** + * An injected item's placeholder found in a surface its rule has no targets on. + * The occurrences are forwarded unsubstituted (an unswapped placeholder is inert + * by design) and surfaced in the audit log so probing stays visible. + */ +export type SkippedPlaceholder = { + item: RequestScopedManagedItem; + /** Where the skipped occurrences sat: `body`, `path`, `query`, or `header:`. */ + locations: Array; +}; + +export type SubstitutionGuardResult = { + /** Fail-closed violation: the caller blocks the request instead of substituting. */ + violation?: SubstitutionGuardViolation; + /** Keys of items with at least one occurrence at an allowed target (these get substituted). */ + injectedKeys: Array; + /** Per-item placeholder occurrences left inert in untargeted surfaces (empty on violation). */ + skipped: Array; +}; /** A string value in a request body, with the dotted path that locates it. */ type BodyLeaf = { path: string; value: string }; @@ -355,160 +420,271 @@ function locationSuggestion(location: ProxySubstitutionLocation, targets: Array< return `currently allowed: [${current.join(', ')}]. To allow it in the ${location}, set ${substituteInExample(targets, entry)} on the @proxy rule${extra}`; } -/** Header-specific hint: names the offending header, the exact substituteIn edit, and any denylist note. */ -function headerSuggestion(name: string | undefined, denied: boolean, targets: Array): string { - const current = targets.map(proxySubstitutionTargetKey); - const where = name ? `the "${name}" header` : 'that header'; - const entry = name ? `header:${name}` : 'header:'; - const deniedNote = denied - ? ` (${name} is excluded from the any-header default because it's commonly forwarded or logged)` - : ''; - return `currently allowed: [${current.join(', ')}]${deniedNote}. To allow it in ${where}, set ${substituteInExample(targets, entry)} on the @proxy rule`; +/** + * Which of an item's targets allows substitution in the named (lower-cased) header, + * as a target key, or undefined if none does. The any-header default excludes a + * denylist of never-secret forward/log headers; an explicit `header:` target + * covers a denied header (so a named denylisted header is still allowed). + * + * The broadest allowing target wins, so declaring both `header` and `header:x` can't + * split one header's occurrences across two targets and double its budget. + */ +function headerTargetKey(item: RequestScopedManagedItem, name: string): string | undefined { + let anyHeader = false; + let named = false; + for (const t of item.targets) { + if (t.location !== 'header') continue; + if (t.name) { + if (t.name === name) named = true; + } else { + anyHeader = true; + } + } + if (anyHeader && !isNeverAutoSubstituteHeader(name)) return 'header'; + if (named) return `header:${name}`; + return undefined; +} + +/** Whether an item's targets allow substitution in the named (lower-cased) header. */ +export function itemAllowsHeader(item: RequestScopedManagedItem, name: string): boolean { + return headerTargetKey(item, name) !== undefined; } /** - * Enforce the substitution guards on the injected items for a request, *before* - * any placeholder is swapped for its real value. Returns the first violation, or - * undefined if every injected placeholder sits only where its rule allows and - * within its occurrence cap. - * - * - placement guard: a placeholder occurrence anywhere the item's `targets` don't - * allow is an anomaly (default: any header). Each occurrence is checked against - * the exact target (specific header name, query param, or body path), which is - * what stops an injected secret from being swapped into a request body/query — a - * placeholder the agent was tricked into placing in, say, an email body on an - * otherwise-allowed host, even one whose body IS a substitution target at a - * different path. - * - cardinality guard: a valid request uses the secret a fixed number of times - * (default 1). An extra occurrence suggests an exfiltration copy (duplicate the - * token into an attacker-visible field while still making a valid call). + * Evaluate the substitution guards on the injected items for a request, *before* + * any placeholder is swapped for its real value. * - * Because placeholders are unique high-entropy tokens, the guard alone decides - * placement; the actual substitution can stay a blind string-replace, since a - * passing request has every occurrence at an allowed spot. - * - * Both fail closed: the caller blocks the request rather than substituting. + * - skip: a placeholder occurrence in a surface the item has NO targets on + * (the body under the default header-only targets, a denylisted header, the URL + * path without a `path` target, ...) is left alone: substitution is scoped per + * surface, so the occurrence reaches the upstream as the literal placeholder, + * which is inert by design. Reported as `skipped` so the audit log keeps probing + * visible. Blocking here would add no protection (the value is never substituted + * there anyway) and bricks legitimate flows where an agent quotes its own + * placeholder, e.g. a conversation transcript echoing an env var. + * - placement guard (fail closed): when the item DOES have targets on a surface + * that can't be substituted surgically (`body:`, `query:`), an + * occurrence off the named path/param blocks the request. Substituting there is + * a blind string-replace across the whole surface, so a stray occurrence would + * either get the real value or require re-serializing the body/query to skip it. + * This is what stops an injected secret being swapped into, say, an email-body + * field on an otherwise-allowed host whose body IS a target at a different path. + * - cardinality guard (fail closed): each target may be substituted at most ONCE + * per request. A second occurrence at the same target is an exfiltration copy + * (duplicate the token into an attacker-visible field while still making a valid + * call), and the proxy can't tell which copy is the real use, so it blocks. The + * budget is per target rather than per request because listing a target is the + * author declaring that spot legitimate: `substituteIn=[header:authorization, + * body:signature]` gets one substitution in each with nothing further to + * configure, while `[header]` still allows only one header in total. Skipped + * occurrences belong to no target and never count. */ export function checkSubstitutionGuards( req: SubstitutionGuardRequest, hostItems: Array, -): SubstitutionGuardViolation | undefined { + /** + * Every managed placeholder in the session, so occurrences are attributed exactly + * as substitution will attribute them (see `findPlaceholderMatches`). Defaults to + * the injected items, which is right only when no other managed placeholder could + * appear; the runtime passes the full list. + */ + allItems: Array = hostItems, +): SubstitutionGuardResult { + const injectedKeys: Array = []; + const skipped: Array = []; + + // Split the request target into the URL path and the query string: they are + // separate substitution locations (`path` vs `query`/`query:`). + const queryStart = req.requestTarget.indexOf('?'); + const pathPart = queryStart === -1 ? req.requestTarget : req.requestTarget.slice(0, queryStart); + const queryPart = queryStart === -1 ? '' : req.requestTarget.slice(queryStart + 1); + + // Tokenize each surface ONCE, against every managed placeholder, then read + // per-item counts out of the result. Doing this per item with a plain substring + // search would disagree with substitution whenever one placeholder contains + // another. + const headerCounts = req.headers.map((h) => ({ name: h.name, counts: countPlaceholderMatches(h.value, allItems) })); + const pathCounts = countPlaceholderMatches(pathPart, allItems); + const queryCounts = countPlaceholderMatches(queryPart, allItems); + const bodyCounts = countPlaceholderMatches(req.body, allItems); + + // Named query params and body paths are only needed when some item targets them, + // and parsing is the expensive part, so both are computed at most once and only + // on demand. `bodyLeafCounts === null` means the body could not be parsed for its + // content type, which allows nothing (fail closed). + let queryParamCounts: Map> | undefined; + const getQueryParamCounts = () => { + if (!queryParamCounts) { + queryParamCounts = new Map(); + for (const [name, value] of new URLSearchParams(queryPart)) { + const existing = queryParamCounts.get(name); + const counts = countPlaceholderMatches(value, allItems); + if (!existing) { + queryParamCounts.set(name, counts); + } else { + // A repeated param (?a=1&a=2) contributes all of its values. + for (const [key, n] of counts) existing.set(key, (existing.get(key) ?? 0) + n); + } + } + } + return queryParamCounts; + }; + let bodyLeafCounts: Array<{ path: string; counts: Map }> | null | undefined; + const getBodyLeafCounts = () => { + if (bodyLeafCounts === undefined) { + const leaves = bodyStringLeaves(req.body, req.contentType); + bodyLeafCounts = leaves + ? leaves.map((leaf) => ({ path: leaf.path, counts: countPlaceholderMatches(leaf.value, allItems) })) + : null; + } + return bodyLeafCounts; + }; + for (const item of hostItems) { - const ph = item.placeholder; - if (!ph) continue; + if (!item.placeholder) continue; const { targets } = item; - const anyHeader = targets.some((t) => t.location === 'header' && !t.name); - const headerNames = new Set(targets.flatMap((t) => (t.location === 'header' && t.name ? [t.name] : []))); + const countFor = (counts: Map) => counts.get(item.key) ?? 0; const anyPath = targets.some((t) => t.location === 'path'); const anyQuery = targets.some((t) => t.location === 'query' && !t.name); const queryNames = targets.flatMap((t) => (t.location === 'query' && t.name ? [t.name] : [])); const bodyPaths = targets.flatMap((t) => (t.location === 'body' ? [t.path] : [])); // `body:*` is the explicit escape hatch for bodies we can't parse into a path. const bodyAnywhere = bodyPaths.includes('*'); + const skippedLocations: Array = []; + const violation = (v: SubstitutionGuardViolation): SubstitutionGuardResult => ( + { violation: v, injectedKeys: [], skipped: [] } + ); + // Occurrences that will be substituted, tallied per target key. Each target + // gets a budget of one, so this is also the cardinality check. + const perTarget = new Map(); + const countAt = (targetKey: string, n: number) => { + if (n > 0) perTarget.set(targetKey, (perTarget.get(targetKey) ?? 0) + n); + }; - // Split the request target into the URL path and the query string: they are - // separate substitution locations (`path` vs `query`/`query:`). - const queryStart = req.requestTarget.indexOf('?'); - const pathPart = queryStart === -1 ? req.requestTarget : req.requestTarget.slice(0, queryStart); - const queryPart = queryStart === -1 ? '' : req.requestTarget.slice(queryStart + 1); - - // Headers: total occurrences vs. those in an allowed header. The any-header - // default excludes a denylist of never-secret forward/log headers; an explicit - // header: target still wins (so a named denied header is allowed). - let headerTotal = 0; - let headerAllowed = 0; - let offendingHeader: string | undefined; - for (const h of req.headers) { - const c = countOccurrences(h.value, ph); + // Headers: substitution is applied per header value, so a disallowed header + // is skipped (left inert) with no surgery needed. Occurrences land on whichever + // target allowed that header, so two headers covered by the bare `header` target + // share one budget while `header:a` + `header:b` get one each. + for (const h of headerCounts) { + const c = countFor(h.counts); if (!c) continue; - headerTotal += c; - const allowed = headerNames.has(h.name) || (anyHeader && !isNeverAutoSubstituteHeader(h.name)); - if (allowed) headerAllowed += c; - else offendingHeader ||= h.name; - } - if (headerAllowed < headerTotal) { - const denied = anyHeader && !!offendingHeader && isNeverAutoSubstituteHeader(offendingHeader); - return { - kind: 'location', item, location: 'header', suggestion: headerSuggestion(offendingHeader, denied, targets), - }; + const targetKey = headerTargetKey(item, h.name); + if (targetKey) countAt(targetKey, c); + else skippedLocations.push(`header:${h.name}`); } // URL path: all-or-nothing (`path` allows a token anywhere in the path). - const pathTotal = countOccurrences(pathPart, ph); - if (pathTotal > 0 && !anyPath) { - return { - kind: 'location', item, location: 'path', suggestion: locationSuggestion('path', targets), - }; + const pathTotal = countFor(pathCounts); + if (pathTotal > 0) { + if (anyPath) countAt('path', pathTotal); + else skippedLocations.push('path'); } - // Query string: total occurrences vs. those in an allowed param. - const queryTotal = countOccurrences(queryPart, ph); - let queryAllowed = 0; - if (queryTotal) { + // Query string: `query` allows anywhere; `query:` only the named + // param's value. An occurrence off the named param fails closed: the query + // is substituted as one string, so skipping it would need re-serialization. + // No query targets at all ⇒ the query is never substituted ⇒ skip (leave inert). + const queryTotal = countFor(queryCounts); + if (queryTotal > 0) { if (anyQuery) { - queryAllowed = queryTotal; + countAt('query', queryTotal); } else if (queryNames.length) { - const params = new URLSearchParams(queryPart); - for (const name of queryNames) for (const v of params.getAll(name)) queryAllowed += countOccurrences(v, ph); + const params = getQueryParamCounts(); + let queryAllowed = 0; + for (const name of queryNames) { + const perParam = countFor(params.get(name) ?? new Map()); + countAt(`query:${name}`, perParam); + queryAllowed += perParam; + } + if (queryAllowed < queryTotal) { + return violation({ + kind: 'location', item, location: 'query', suggestion: locationSuggestion('query', targets), + }); + } + } else { + skippedLocations.push('query'); } } - if (queryAllowed < queryTotal) { - return { - kind: 'location', item, location: 'query', suggestion: locationSuggestion('query', targets), - }; - } - // Body: total occurrences vs. those at an allowed path. `body:*` allows anywhere - // (no parse needed); otherwise an unparseable body (leaves === null) allows - // nothing, so a `body:` target fails closed on a body we can't parse. - const bodyTotal = countOccurrences(req.body, ph); - let bodyAllowed = 0; - if (bodyTotal && bodyAnywhere) { - bodyAllowed = bodyTotal; - } else if (bodyTotal && bodyPaths.length) { - const leaves = bodyStringLeaves(req.body, req.contentType); - if (leaves) { - for (const leaf of leaves) if (bodyPaths.includes(leaf.path)) bodyAllowed += countOccurrences(leaf.value, ph); + // Body: `body:*` allows anywhere (no parse needed). With `body:` targets, + // an occurrence off an allowed path fails closed (same surgery argument as + // query params), and an unparseable body (leaves === null) allows nothing. With + // no body targets the body bytes are never touched, so occurrences are skipped. + const bodyTotal = countFor(bodyCounts); + if (bodyTotal > 0) { + if (bodyAnywhere) { + countAt('body:*', bodyTotal); + } else if (bodyPaths.length) { + const leaves = getBodyLeafCounts(); + let bodyAllowed = 0; + if (leaves) { + for (const leaf of leaves) { + if (!bodyPaths.includes(leaf.path)) continue; + const c = countFor(leaf.counts); + countAt(`body:${leaf.path}`, c); + bodyAllowed += c; + } + } + if (bodyAllowed < bodyTotal) { + return violation({ + kind: 'location', item, location: 'body', suggestion: locationSuggestion('body', targets), + }); + } + } else { + skippedLocations.push('body'); } } - if (bodyAllowed < bodyTotal) { - return { - kind: 'location', item, location: 'body', suggestion: locationSuggestion('body', targets), - }; - } - const total = headerTotal + pathTotal + queryTotal + bodyTotal; - if (total > item.maxOccurrences) return { kind: 'occurrences', item, count: total }; + let allowedTotal = 0; + for (const [targetKey, count] of perTarget) { + // One substitution per target: a second copy at the same spot is ambiguous + // (which one is the real use?) and would put the secret in both. + if (count > 1) { + return violation({ + kind: 'occurrences', item, target: targetKey, count, + }); + } + allowedTotal += count; + } + if (allowedTotal > 0) injectedKeys.push(item.key); + if (skippedLocations.length) skipped.push({ item, locations: skippedLocations }); } - return undefined; -} - -export function replacePlaceholdersWithReal(value: string, managedItems: Array): string { - let next = value; - // Longest placeholder first, mirroring the scrub direction: if one placeholder - // is a substring of another (e.g. `vlk_x` and `vlk_x_1`), replacing the shorter - // one first would corrupt the longer one and splice in the wrong real value. - const sortedByPlaceholderLength = [...managedItems] - .filter((item) => !!item.placeholder) - .sort((a, b) => b.placeholder.length - a.placeholder.length); - for (const item of sortedByPlaceholderLength) { - next = next.split(item.placeholder).join(item.realValue); - } - return next; + return { violation: undefined, injectedKeys, skipped }; } /** - * Which managed items' placeholders actually appear in this request — i.e. the - * secrets that will really be injected. Used for the audit log so it records - * what was injected (keys only), not merely what was in scope. + * Substitute placeholder → real value within ONE request surface (a single header + * value, the URL path, the query string, or the body). + * + * `allItems` is *every* managed placeholder, not just the substitutable ones, and + * `substituteKeys` selects which of them this surface may swap. Matching is + * leftmost-longest across all of them, which is what keeps overlapping placeholders + * honest: one placeholder can be a substring of another (`ensureUnique` resolves a + * collision by appending `_1`, and explicit `@placeholder` values can overlap too), + * so a shorter placeholder must never match inside its longer sibling. Filtering the + * list down to the surface's own items before replacing would do exactly that, and + * would rewrite bytes the guard classified as skipped. + * + * Every match outside `substituteKeys` is re-emitted verbatim, so a skipped + * placeholder reaches the upstream byte-for-byte unchanged. Matched regions are + * never rescanned, so a real value that happens to contain another placeholder's + * text can't be substituted a second time. */ -function detectInjectedKeys(parts: Array, hostItems: Array): Array { - const keys: Array = []; - for (const item of hostItems) { - if (!item.placeholder) continue; - if (parts.some((part) => part.includes(item.placeholder))) keys.push(item.key); +export function substitutePlaceholdersInSurface( + value: string, + allItems: Array, + substituteKeys: ReadonlySet, +): string { + const matches = findPlaceholderMatches(value, allItems); + if (!matches.length) return value; + let out = ''; + let pos = 0; + for (const match of matches) { + out += value.slice(pos, match.index); + out += substituteKeys.has(match.item.key) ? match.item.realValue : match.item.placeholder; + pos = match.index + match.item.placeholder.length; } - return keys; + return out + value.slice(pos); } /** @@ -651,10 +827,12 @@ function transformHeaders( const out: Record> = {}; for (const [key, val] of Object.entries(headers)) { if (val === undefined) continue; + // node lower-cases incoming header names, so `key` is already the canonical + // (lower-cased) name the substitution targets match on. if (Array.isArray(val)) { - out[key] = val.map((v) => transformValue(v)); + out[key] = val.map((v) => transformValue(v, key)); } else { - out[key] = transformValue(String(val)); + out[key] = transformValue(String(val), key); } } return out; @@ -1101,7 +1279,6 @@ export async function startLocalProxyRuntime({ const body = await readBody(req); const bodyText = body.toString('utf8'); const scanParts = [t.requestTarget, JSON.stringify(req.headers), bodyText]; - const injectedKeys = shouldRewrite ? detectInjectedKeys(scanParts, hostItems) : []; // Helpful-failure guard: when NO rule injects anything on this route yet the // request carries a managed placeholder, the real value won't be substituted @@ -1124,12 +1301,16 @@ export async function startLocalProxyRuntime({ } // Substitution guards: before any placeholder is swapped for its real value, - // enforce *where* (target: header / header:name / query:param / body:path) and - // *how often* (occurrence cap) each injected secret may appear. Default is any - // header, once. This is what keeps a clever request from moving the real secret - // into an exfiltration-friendly spot (an email body, a duplicated field) on an - // otherwise-allowed host — the secret is only ever substituted where the rule - // explicitly allows. + // enforce *where* (target: header / header:name / query:param / body:path) each + // injected secret may appear, and that it appears at most once per target. + // Default is any header, once. This is what keeps a clever request from moving + // the real secret into an exfiltration-friendly spot (an email body, a + // duplicated field) on an otherwise-allowed host: the secret is only ever + // substituted where the rule explicitly allows. Occurrences in a surface the + // rule has no targets on are skipped (forwarded unsubstituted, inert) rather + // than blocked; see checkSubstitutionGuards. + let injectedKeys: Array = []; + let skippedPlaceholders: Array<{ key: string; locations: Array }> = []; if (shouldRewrite && hostItems.length > 0) { const guardReq: SubstitutionGuardRequest = { headers: Object.entries(req.headers).map(([name, value]) => ({ @@ -1140,21 +1321,27 @@ export async function startLocalProxyRuntime({ body: bodyText, contentType: getHeaderValue(req.headers, 'content-type'), }; - const violation = checkSubstitutionGuards(guardReq, hostItems); + // Pass every managed placeholder, not just the injected ones, so the guard + // attributes occurrences exactly as the substitution below will. + const guardResult = checkSubstitutionGuards(guardReq, hostItems, managedItems); + const { violation } = guardResult; if (violation) { const decision = violation.kind === 'location' ? 'blocked-location' : 'blocked-occurrences'; onActivity?.({ ...baseActivity, ...ruleId, matched: true, blocked: true, decision, }); const message = violation.kind === 'location' - ? `Blocked by the varlock credential proxy: ${violation.item.key}'s placeholder appears in the ${violation.location} of this request, which its @proxy rule doesn't allow. ` + ? `Blocked by the varlock credential proxy: ${violation.item.key}'s placeholder appears in the ${violation.location} of this request, off the exact spot its @proxy rule allows. ` + `${violation.suggestion}. ` + 'If that placement was not intentional, it may be an attempt to place the secret somewhere it could leak.' - : `Blocked by the varlock credential proxy: ${violation.item.key}'s placeholder appears ${violation.count} times in this request, but at most ${violation.item.maxOccurrences} is allowed. ` - + 'A valid request uses the secret once; extra copies can exfiltrate it. If this API legitimately repeats it, raise maxOccurrences on the @proxy rule.'; + : `Blocked by the varlock credential proxy: ${violation.item.key}'s placeholder appears ${violation.count} times at the same substitution target (${violation.target}) in this request, but each target may be substituted only once. ` + + 'A valid request uses the secret once per place it belongs; an extra copy at the same target can exfiltrate it, and the proxy cannot tell which copy is the real use. ' + + `If this API genuinely carries it in more than one place, name each one so each gets its own substitution: ${substituteInExample(violation.item.targets, '')}.`; respondBlocked(res, 403, message, t.tunnelTeardown); return; } + injectedKeys = guardResult.injectedKeys; + skippedPlaceholders = guardResult.skipped.map((c) => ({ key: c.item.key, locations: c.locations })); } // Invariant #8: a require-approval rule holds the request for an out-of-band, @@ -1187,24 +1374,42 @@ export async function startLocalProxyRuntime({ blocked: false, decision: policyDecision?.verdict === 'require-approval' ? 'approval-granted' : 'allow', ...(injectedKeys.length ? { injectedKeys } : {}), + ...(skippedPlaceholders.length ? { skippedPlaceholders } : {}), }); - // Substitute placeholder → real value. The guards above already proved every - // occurrence sits at an allowed target for its item, and placeholders are unique - // per item, so a blind string-replace across all three parts only ever hits the - // approved spot — no need to re-scope per location (which would also risk - // re-serializing/altering the body). + // Substitute placeholder → real value, scoped per surface: a surface only swaps + // the items whose targets cover it, so a skipped occurrence (in a surface the + // rule doesn't target) stays the literal, inert placeholder. Within a targeted + // surface the guard above already proved every occurrence sits at an allowed + // spot, so no body/query re-serialization is needed. Every call still matches + // against ALL managed placeholders, not just the surface's own, so an + // overlapping placeholder can't be clobbered from inside its longer sibling + // (see substitutePlaceholdersInSurface). + const keysForLocation = (location: ProxySubstitutionLocation) => new Set( + hostItems.filter((item) => item.targets.some((tg) => tg.location === location)).map((item) => item.key), + ); const rewrittenBody = shouldRewrite - ? Buffer.from(replacePlaceholdersWithReal(bodyText, hostItems), 'utf8') + ? Buffer.from(substitutePlaceholdersInSurface(bodyText, managedItems, keysForLocation('body')), 'utf8') : body; - const rewrittenPath = shouldRewrite - ? replacePlaceholdersWithReal(t.requestTarget, hostItems) - : t.requestTarget; + let rewrittenPath = t.requestTarget; + if (shouldRewrite) { + const queryStart = t.requestTarget.indexOf('?'); + const pathPart = queryStart === -1 ? t.requestTarget : t.requestTarget.slice(0, queryStart); + const queryPart = queryStart === -1 ? undefined : t.requestTarget.slice(queryStart + 1); + rewrittenPath = substitutePlaceholdersInSurface(pathPart, managedItems, keysForLocation('path')) + + (queryPart === undefined + ? '' + : `?${substitutePlaceholdersInSurface(queryPart, managedItems, keysForLocation('query'))}`); + } const upstreamHeaders = transformHeaders( req.headers, shouldRewrite - ? (value) => replacePlaceholdersWithReal(value, hostItems) + ? (value, name) => substitutePlaceholdersInSurface( + value, + managedItems, + new Set(hostItems.filter((item) => itemAllowsHeader(item, name)).map((item) => item.key)), + ) : (value) => value, ); delete upstreamHeaders['proxy-connection']; diff --git a/packages/varlock/src/proxy/types.ts b/packages/varlock/src/proxy/types.ts index 5e0884d43..3a0301e39 100644 --- a/packages/varlock/src/proxy/types.ts +++ b/packages/varlock/src/proxy/types.ts @@ -57,12 +57,12 @@ export function proxySubstitutionTargetKey(target: ProxySubstitutionTarget): str /** * Headers the bare `header` (any-header) default will NOT substitute into: they're - * never a legitimate place for a managed secret and are common forward/log sinks, - * so a placeholder landing here is almost always an attempt to redirect the one - * allowed substitution somewhere it leaks (e.g. a header the upstream forwards to a - * webhook). Any `x-forwarded-*` header is covered by prefix. This narrows only the - * default — an explicit `header:` target (even for one of these) still wins, - * for the rare API that genuinely authenticates via, say, a cookie. + * never a legitimate place for a managed secret and are common forward/log sinks + * (e.g. a header the upstream forwards to a webhook), so a placeholder landing here + * is skipped (left unsubstituted) instead. Any `x-forwarded-*` header is covered + * by prefix. This narrows only the default: an explicit `header:` target + * (even for one of these) still wins, for the rare API that genuinely authenticates + * via, say, a cookie. */ export const PROXY_NEVER_AUTO_SUBSTITUTE_HEADERS: ReadonlyArray = ['cookie', 'host', 'forwarded', 'via', 'referer', 'origin', 'user-agent']; @@ -111,13 +111,17 @@ export function parseProxySubstitutionTarget(raw: string): ParsedProxySubstituti } /** - * Default cardinality cap when a rule doesn't set `maxOccurrences`: a placeholder - * may appear at most once per request. A valid request uses the secret a fixed - * number of times, so an extra occurrence is usually an exfiltration copy (the - * secret duplicated into an attacker-visible field while a valid call is still - * made). + * `@proxy(...)` options that no longer exist, mapped to the error explaining what + * replaced them. Checked before the unknown-option sweep so an upgrade gets the + * migration, not just "unknown option". */ -export const DEFAULT_PROXY_MAX_OCCURRENCES = 1; +export const REMOVED_PROXY_RULE_OPTIONS: Record = { + maxOccurrences: + '@proxy: maxOccurrences has been removed. A placeholder may now appear at most once per substitution ' + + 'target, so a separate cap is no longer needed: listing a target in substituteIn is what grants it an ' + + 'occurrence. If this API carries the secret in more than one place, name each one ' + + '(e.g. substituteIn=["header:authorization", "body:signature"]) instead of raising a count.', +}; export type ProxyRule = { domain: Array; @@ -146,15 +150,15 @@ export type ProxyRule = { * as raw `substituteIn` entries (`header`, `header:authorization`, `query`, * `query:api_key`, `body:client_secret`). Validated at schema load; parsed into * structured targets at request time. Omitted ⇒ `DEFAULT_PROXY_SUBSTITUTION_TARGETS` - * (any header). A placeholder that reaches a spot no target allows is treated as - * an anomaly and the request is blocked, rather than silently substituted. + * (any header). A placeholder in a surface with no targets is skipped, forwarded + * unsubstituted (inert) and audited; one that lands off the exact spot within a + * `body:`/`query:`-targeted surface blocks the request (fail closed). + * + * Each target also carries its own cardinality: a placeholder may be substituted + * at most once per target per request, so a second copy at the same target blocks + * the request (see `checkSubstitutionGuards`). */ substituteIn?: Array; - /** - * Max times a single injected placeholder may appear in one request. Omitted ⇒ - * `DEFAULT_PROXY_MAX_OCCURRENCES` (`1`). Exceeding it blocks the request. - */ - maxOccurrences?: number; }; export type ProxyManagedItem = {