From 98409ecce3d3aca23808deb54628da344364637e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 28 Jul 2026 16:00:30 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Keep=20SSRF=20Gu?= =?UTF-8?q?ards=20on=20Label=20Calls,=20Skip=20Mixed=20Handoff=20Batches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-eighteen review: four fixed; the fifth repeats the maintainer-decided packages/api extraction (eighth instance), answered on-thread. - SSRF-safe carrier (host.ts, P1): the sanitize step restored the Anthropic `clientOptions` carrier only when `defaultHeaders` existed, but for user-provided base URLs `getLLMConfig` stores the guarded Undici dispatcher and `redirect: 'error'` there — dropping it reopened DNS-rebinding/redirect paths on label calls to user-controlled URLs. The carrier (client CONSTRUCTION options, not generation params) is now restored whenever present, same reference. - Primary maxTokens (host.ts): top-level `maxTokens` is not in `omitTitleOptions` and survived the strip; the title path deletes it explicitly, and a cap sized for the primary model can be rejected by the substitute. Deleted on the copy. - Bounded keys (runtime.ts): the object branch materialized every key via `Object.keys` and quoted oversized keys in full before the budget check. Enumeration is now lazy (`for..in` + own-property guard) and keys slice to the budget before quoting, like string values. - Mixed handoff batches (runtime.ts, groupToolCalls.ts): the client flushes the block at the transfer card, so a mixed batch's label orphaned exactly like a pure one. The hook now skips ANY batch containing a transfer call, and the renderer drops orphan labels covering one (legacy content). Tests: carrier survival without headers by same reference, maxTokens strip (host.spec); mixed batch claims nothing (runtime.spec); mixed orphan dropped, real-batch orphan kept (groupToolCalls.test). --- .../utils/__tests__/groupToolCalls.test.ts | 25 +++++++++++++++++ client/src/utils/groupToolCalls.ts | 22 ++++++++------- .../activityLabels/__tests__/host.spec.ts | 21 ++++++++++++++ .../activityLabels/__tests__/runtime.spec.ts | 7 +++-- .../api/src/agents/activityLabels/host.ts | 22 +++++++++++---- .../api/src/agents/activityLabels/runtime.ts | 28 +++++++++++++------ 6 files changed, 98 insertions(+), 27 deletions(-) diff --git a/client/src/utils/__tests__/groupToolCalls.test.ts b/client/src/utils/__tests__/groupToolCalls.test.ts index 41244cf733..a785836849 100644 --- a/client/src/utils/__tests__/groupToolCalls.test.ts +++ b/client/src/utils/__tests__/groupToolCalls.test.ts @@ -116,6 +116,31 @@ describe('groupSequentialToolCalls with activity labels', () => { expect((grouped[0] as { part: PartWithIndex }).part.idx).toBe(0); }); + /** Mixed legacy content: an orphan label covering a transfer AND real + * calls is equally headless once the block flushed at the transfer. */ + it('drops an orphan label whose batch mixed a transfer with real calls', () => { + const realTool = { + type: ContentTypes.TOOL_CALL, + [ContentTypes.TOOL_CALL]: { id: 't1', name: 'web_search', args: '{}', output: 'ok' }, + } as unknown as TMessageContentParts; + const transfer = { + type: ContentTypes.TOOL_CALL, + [ContentTypes.TOOL_CALL]: { id: 'x1', name: 'lc_transfer_to_billing', args: '{}' }, + } as unknown as TMessageContentParts; + const mixedLabel = { + type: ContentTypes.ACTIVITY_LABEL, + [ContentTypes.ACTIVITY_LABEL]: 'Looked up the refund policy', + tool_call_ids: ['t1', 'x1'], + pending: false, + } as unknown as TMessageContentParts; + + const grouped = groupSequentialToolCalls(withIndex([realTool, transfer, mixedLabel])); + + /** Tool card + handoff card render; the headless label is dropped. */ + expect(grouped).toHaveLength(2); + expect(grouped.every((entry) => entry.type === 'single')).toBe(true); + }); + it('keeps rendering an orphan label whose batch had real tool calls', () => { const orphanLabel = { type: ContentTypes.ACTIVITY_LABEL, diff --git a/client/src/utils/groupToolCalls.ts b/client/src/utils/groupToolCalls.ts index 9edd9511ca..7f4b365731 100644 --- a/client/src/utils/groupToolCalls.ts +++ b/client/src/utils/groupToolCalls.ts @@ -24,18 +24,20 @@ function isGroupableToolCall(part: TMessageContentParts): boolean { } /** - * True when every tool call the label covers is a `transfer_to_*` handoff. - * Transfer parts are never groupable, so such a label can only orphan into a - * stray line after the handoff card — which already names the destination. - * Content persisted before the server stopped claiming labels for pure - * handoff batches still carries these; they are dropped at render. + * True when the label covers ANY `transfer_to_*` handoff call. Transfer + * parts are never groupable, so the flush at the handoff card leaves such a + * label with nothing to head — it can only orphan into a stray line after + * cards that already show everything (the handoff card names the + * destination; mixed batches keep their tool cards). Content persisted + * before the server stopped claiming labels for handoff batches still + * carries these; they are dropped at render. */ -function isTransferOnlyLabel(labelPart: TMessageContentParts, allParts: PartWithIndex[]): boolean { +function coversTransferCall(labelPart: TMessageContentParts, allParts: PartWithIndex[]): boolean { const ids = (labelPart as { tool_call_ids?: unknown }).tool_call_ids; if (!Array.isArray(ids) || ids.length === 0) { return false; } - return ids.every((id) => + return ids.some((id) => allParts.some(({ part }) => { if (part?.type !== ContentTypes.TOOL_CALL) { return false; @@ -119,10 +121,10 @@ export function groupSequentialToolCalls(parts: PartWithIndex[]): GroupedPart[] flushWithoutLabel(); if (claimed.length > 0) { result.push({ type: 'tool-group', parts: claimed, labelPart: item }); - } else if (!isTransferOnlyLabel(item.part, parts)) { + } else if (!coversTransferCall(item.part, parts)) { /** Orphan label (block parts hidden/filtered): renders standalone — - * UNLESS its batch was only handoff calls, where the transfer card - * already says everything and the label would be a stray line. */ + * UNLESS its batch contained a handoff call, where the cards + * already say everything and the label would be a stray line. */ result.push({ type: 'single', part: item }); } continue; diff --git a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts index ea49989e09..d201ec2d9f 100644 --- a/packages/api/src/agents/activityLabels/__tests__/host.spec.ts +++ b/packages/api/src/agents/activityLabels/__tests__/host.spec.ts @@ -184,6 +184,27 @@ describe('resolveActivityLabelModel model precedence', () => { defaultHeaders: { 'x-proxy-key': 'abc' }, }); }); + + it('strips the primary maxTokens like the title path', async () => { + mockGetOptions.mockResolvedValueOnce({ + llmConfig: { model: 'resolved', maxTokens: 64_000 }, + } as never); + const resolved = await resolve({ activityLabel: true, activityModel: 'label-model' }); + expect((resolved.clientOptions as Record).maxTokens).toBeUndefined(); + }); + + /** The Anthropic carrier holds client CONSTRUCTION options — for + * user-provided base URLs that includes the SSRF-safe fetch dispatcher — + * so it must survive the strip even with no custom headers, and by the + * SAME reference. */ + it('preserves the SSRF-safe carrier even without defaultHeaders', async () => { + const carrier = { fetchOptions: { dispatcher: { kind: 'guarded' }, redirect: 'error' } }; + mockGetOptions.mockResolvedValueOnce({ + llmConfig: { model: 'resolved', thinking: { type: 'enabled' }, clientOptions: carrier }, + } as never); + const resolved = await resolve({ activityLabel: true, activityModel: 'label-model' }); + expect((resolved.clientOptions as Record).clientOptions).toBe(carrier); + }); }); describe('mapCollectedMetadataToUsage cache tokens', () => { diff --git a/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts b/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts index e74e956501..3ba5ac0436 100644 --- a/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts +++ b/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts @@ -307,7 +307,9 @@ describe('createActivityLabelHook', () => { expect(mockInvoke).not.toHaveBeenCalled(); }); - it('still claims for a mixed batch containing a transfer call', async () => { + /** Mixed batches skip too: the client flushes the block at the transfer + * card, so the label would orphan even when real tools ran alongside. */ + it('claims nothing for a mixed batch containing a transfer call', async () => { const hook = createActivityLabelHook({ claimSlot, resolveLLM }); await hook( batchInput({ @@ -331,7 +333,8 @@ describe('createActivityLabelHook', () => { new AbortController().signal, ); await flushDetached(); - expect(slots).toHaveLength(1); + expect(slots).toHaveLength(0); + expect(mockInvoke).not.toHaveBeenCalled(); }); it('skips subagent scopes entirely', async () => { diff --git a/packages/api/src/agents/activityLabels/host.ts b/packages/api/src/agents/activityLabels/host.ts index fdb30c8227..b62672b93f 100644 --- a/packages/api/src/agents/activityLabels/host.ts +++ b/packages/api/src/agents/activityLabels/host.ts @@ -299,25 +299,35 @@ export async function resolveActivityLabelModel({ * `modelKwargs` output caps go for the same reason (copied, not mutated: * `llmConfig` is shared with the memoized provider resolution). */ const rawOptions = { ...(llmConfig ?? {}) } as MaybeAzureConfig & { + maxTokens?: number; modelKwargs?: Record; clientOptions?: { defaultHeaders?: unknown }; }; + /** Top-level `maxTokens` too, exactly like the title path — it is not in + * `omitTitleOptions`, and a primary cap sized for the agent's model can + * be unsupported or absurd on the substitute label model. */ + delete rawOptions.maxTokens; if (rawOptions.modelKwargs != null) { const modelKwargs = { ...rawOptions.modelKwargs }; delete modelKwargs.max_completion_tokens; delete modelKwargs.max_output_tokens; rawOptions.modelKwargs = modelKwargs; } - /** The filter drops the Anthropic `clientOptions` carrier (thinking, - * streaming), which would also drop its `defaultHeaders` — restore the - * SAME object reference so gateway/proxy metadata still reaches label - * requests and `resolveConfigHeaders` mutates the object the client is - * actually built from. */ + /** The filter drops the Anthropic `clientOptions` carrier, so restore it + * WHENEVER it exists — by the SAME reference. It holds client + * CONSTRUCTION options, not generation parameters: proxy + * `defaultHeaders`, and for user-provided base URLs the SSRF-safe + * `fetchOptions` (guarded Undici dispatcher, `redirect: 'error'`). + * Restoring only when headers were present silently stripped those + * guards from label calls to user-controlled URLs, re-opening DNS + * rebinding/redirect paths the endpoint validation exists to block; the + * same-reference restore also lets `resolveConfigHeaders` mutate the + * object the client is actually built from. */ const anthropicCarrier = rawOptions.clientOptions; const clientOptions = Object.fromEntries( Object.entries(rawOptions).filter(([key]) => !omitTitleOptions.has(key)), ) as MaybeAzureConfig & { clientOptions?: { defaultHeaders?: unknown } }; - if (anthropicCarrier?.defaultHeaders != null && clientOptions.clientOptions == null) { + if (anthropicCarrier != null && clientOptions.clientOptions == null) { clientOptions.clientOptions = anthropicCarrier; } if (options.configOptions) { diff --git a/packages/api/src/agents/activityLabels/runtime.ts b/packages/api/src/agents/activityLabels/runtime.ts index f623f75c3d..5b7cec6a3b 100644 --- a/packages/api/src/agents/activityLabels/runtime.ts +++ b/packages/api/src/agents/activityLabels/runtime.ts @@ -258,12 +258,21 @@ function stringifyBounded(value: unknown, limit: number): string { return false; } let first = true; - for (const key of Object.keys(val)) { + /** for-in, not Object.keys: enumeration stops at the budget instead of + * materializing a key array for a million-property object first. The + * own-property guard replaces what Object.keys implied. */ + for (const key in val) { + if (!Object.prototype.hasOwnProperty.call(val, key)) { + continue; + } const entry = (val as Record)[key]; if (entry === undefined || typeof entry === 'function') { continue; } - if ((!first && !push(',')) || !push(`${JSON.stringify(key)}:`) || !walk(entry)) { + /** Keys are untrusted too: slice BEFORE quoting, exactly like string + * values — quoting is what materializes the copy. */ + const boundedKey = key.length > limit + 1 ? key.slice(0, limit + 1) : key; + if ((!first && !push(',')) || !push(`${JSON.stringify(boundedKey)}:`) || !walk(entry)) { return false; } first = false; @@ -455,14 +464,15 @@ export function createActivityLabelHook( ) { return {}; } - /** A batch that is ONLY handoff calls gets no label. The transfer card - * already names the destination, the client renders transfer parts - * standalone (never groupable), so a claimed label could only orphan - * into a stray line after the card — and the model call would be spent - * describing what the card already says. Skipped BEFORE the quota so - * handoffs never consume `maxPerRun`. */ + /** A batch containing ANY handoff call gets no label — mixed batches + * included. Transfer parts are never groupable on the client, so the + * flush at the transfer card leaves the label with nothing to head and + * it could only orphan into a stray line after the cards; the handoff + * card already names the destination and the tool cards still show the + * work. Skipped BEFORE the quota so handoffs never consume + * `maxPerRun`. */ if ( - input.entries.every((entry) => entry.toolName?.startsWith(Constants.LC_TRANSFER_TO_) === true) + input.entries.some((entry) => entry.toolName?.startsWith(Constants.LC_TRANSFER_TO_) === true) ) { return {}; }