From dcdaaeac6725039453f97e54b860cb6d598a4936 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 7 Jul 2026 07:22:50 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=90=20fix:=20Exclude=20Provider=20Secr?= =?UTF-8?q?ets=20from=20HITL=20Pending=20Actions=20(#14136)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🤐 fix: Exclude Provider Secrets from HITL Pending Actions Sanitize resolved model parameters before persisting them in the pending action's resumeContext, and strip resumeContext/requestFingerprint from every client-facing copy (SSE emit, reconnect gap-fill, resume state, status route). The full record stays server-side for resume replay. * 🤐 fix: Treat Header-Carrier Keys as Sensitive Wholesale Google llmConfig places the Authorization header in customHeaders, and header names like Ocp-Apim-Subscription-Key defeat name heuristics. Match 'header' as a key fragment so every header-carrier object is dropped rather than relying on exact carrier names. * 🤐 fix: Strip Preconfigured Client Instances from Resume Params Bedrock stores a BedrockRuntimeClient on llmConfig.client when PROXY or a bearer token is configured; replaying it would also fold a mangled client object into additionalModelRequestFields on resume. --- api/server/controllers/agents/client.js | 12 +- api/server/routes/agents/index.js | 9 +- packages/api/src/agents/hitl/policy.spec.ts | 100 ++++++++++++++ packages/api/src/agents/hitl/policy.ts | 128 +++++++++++++++++- .../api/src/stream/GenerationJobManager.ts | 11 +- .../stream/__tests__/pendingAction.spec.ts | 24 ++++ 6 files changed, 275 insertions(+), 9 deletions(-) diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 8b639f01a8..9581e27c5c 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -34,8 +34,10 @@ const { getTransactionsConfig, resolveRecursionLimit, buildPendingAction, + toClientPendingAction, computeAgentRequestFingerprint, extractDiscoveredToolsFromHistory, + sanitizeResumeModelParameters, pickResumeContext, getApprovalTtlMs, isHITLEnabled, @@ -1222,9 +1224,13 @@ class AgentClient extends BaseClient { // paused on. The resume payload omits them and they aren't part of the fingerprint, so // without this the rebuilt ephemeral run falls back to defaults. (Saved agents source // these from the DB record server-side, so this is belt-and-suspenders for them.) + // Sanitized: the resolved params are the llmConfig, which carries provider secrets + // (apiKey, credentials) and gateway config — resume re-resolves those server-side. const resumeContext = pickResumeContext(this.options.req?.body); - const resolvedModelParameters = this.options.agent?.model_parameters; - if (resolvedModelParameters && typeof resolvedModelParameters === 'object') { + const resolvedModelParameters = sanitizeResumeModelParameters( + this.options.agent?.model_parameters, + ); + if (resolvedModelParameters) { resumeContext.model_parameters = resolvedModelParameters; } const pendingAction = buildPendingAction(interrupt.payload, { @@ -1311,7 +1317,7 @@ class AgentClient extends BaseClient { } await GenerationJobManager.emitChunk(streamId, { event: ApprovalEvents.ON_PENDING_ACTION, - data: pendingAction, + data: toClientPendingAction(pendingAction), }); logger.debug( `[AgentClient] Paused ${streamId} for ${interrupt.payload.type} (action ${pendingAction.actionId})`, diff --git a/api/server/routes/agents/index.js b/api/server/routes/agents/index.js index fe36869983..5776b31e9e 100644 --- a/api/server/routes/agents/index.js +++ b/api/server/routes/agents/index.js @@ -5,6 +5,7 @@ const { hasPersistableAbortContent, buildAbortedResponseMetadata, isPendingActionStale, + toClientPendingAction, isHITLEnabled, deleteAgentCheckpoint, } = require('@librechat/api'); @@ -222,8 +223,12 @@ router.get('/chat/status/:conversationId', async (req, res) => { resumeState, // Surface the live pending approval so a client rebuilding from /chat/status // (reload / cross-replica) has the action id + payload to render and submit - // the prompt, not just the knowledge that the stream is paused. - pendingAction: job.status === 'requires_action' && pendingLive ? pendingAction : undefined, + // the prompt, not just the knowledge that the stream is paused. Client-safe + // projection only — resumeContext/requestFingerprint stay server-side. + pendingAction: + job.status === 'requires_action' && pendingLive + ? toClientPendingAction(pendingAction) + : undefined, }); }); diff --git a/packages/api/src/agents/hitl/policy.spec.ts b/packages/api/src/agents/hitl/policy.spec.ts index 579b0503ef..3c6d0a79cb 100644 --- a/packages/api/src/agents/hitl/policy.spec.ts +++ b/packages/api/src/agents/hitl/policy.spec.ts @@ -6,7 +6,9 @@ import { buildToolApprovalPayload, buildAskUserQuestionPayload, buildPendingAction, + toClientPendingAction, computeAgentRequestFingerprint, + sanitizeResumeModelParameters, pickResumeContext, applyResumeContext, } from './policy'; @@ -257,6 +259,104 @@ describe('buildPendingAction', () => { }); }); +describe('toClientPendingAction', () => { + const payload: Agents.ToolApprovalInterruptPayload = { + type: 'tool_approval', + action_requests: [{ name: 'shell', arguments: { command: 'ls' }, tool_call_id: 'call_abc' }], + review_configs: [ + { action_name: 'shell', tool_call_id: 'call_abc', allowed_decisions: ['approve', 'reject'] }, + ], + }; + + test('omits server-only replay state, keeping the fields the client renders from', () => { + const full = buildPendingAction(payload, { + streamId: 'stream-1', + conversationId: 'conv-1', + requestFingerprint: 'fp-hash', + resumeContext: { + endpoint: 'agents', + model_parameters: { temperature: 0.5 }, + }, + }); + + const clientSafe = toClientPendingAction(full); + expect(clientSafe).toBeDefined(); + expect(clientSafe?.resumeContext).toBeUndefined(); + expect(clientSafe?.requestFingerprint).toBeUndefined(); + expect(clientSafe?.actionId).toBe(full.actionId); + expect(clientSafe?.streamId).toBe('stream-1'); + expect(clientSafe?.payload).toBe(full.payload); + // Non-mutating: the stored record keeps its replay state for the resume route. + expect(full.resumeContext).toBeDefined(); + expect(full.requestFingerprint).toBe('fp-hash'); + }); + + test('passes through nullish input', () => { + expect(toClientPendingAction(undefined)).toBeUndefined(); + expect(toClientPendingAction(null)).toBeUndefined(); + }); +}); + +describe('sanitizeResumeModelParameters', () => { + test('strips provider credentials and transport config across provider shapes', () => { + const sanitized = sanitizeResumeModelParameters({ + model: 'gpt-5', + temperature: 0.2, + maxTokens: 1024, + max_tokens: 512, + apiKey: 'sk-server-secret', + azureOpenAIApiKey: 'azure-secret', + azureOpenAIApiInstanceName: 'internal-resource', + anthropicApiUrl: 'https://internal-gateway.example', + configuration: { + baseURL: 'https://internal-gateway.example', + defaultHeaders: { Authorization: 'Bearer server-secret' }, + }, + clientOptions: { defaultHeaders: { 'x-api-key': 'anthropic-secret' } }, + customHeaders: { 'Ocp-Apim-Subscription-Key': 'gateway-secret' }, + authOptions: { credentials: { private_key: 'google-secret' } }, + credentials: { accessKeyId: 'aws-id', secretAccessKey: 'aws-secret' }, + client: { config: { token: { token: 'bedrock-bearer' } } }, + endpointHost: 'vpce.internal.example', + baseURL: 'https://internal-gateway.example', + }); + + expect(sanitized).toEqual({ + model: 'gpt-5', + temperature: 0.2, + maxTokens: 1024, + max_tokens: 512, + }); + }); + + test('keeps user-level params while stripping nested secret keys from custom params', () => { + const sanitized = sanitizeResumeModelParameters({ + maxTokens: 2048, + stop: ['a', 'b'], + custom: { safe: true, api_key: 'x', token: 'y' }, + }); + + expect(sanitized).toEqual({ + maxTokens: 2048, + stop: ['a', 'b'], + custom: { safe: true }, + }); + }); + + test('drops function values and returns undefined for non-object input', () => { + const sanitized = sanitizeResumeModelParameters({ + temperature: 1, + fetch: () => undefined, + }); + expect(sanitized).toEqual({ temperature: 1 }); + + expect(sanitizeResumeModelParameters(undefined)).toBeUndefined(); + expect(sanitizeResumeModelParameters(null)).toBeUndefined(); + expect(sanitizeResumeModelParameters('sk-secret')).toBeUndefined(); + expect(sanitizeResumeModelParameters(['sk-secret'])).toBeUndefined(); + }); +}); + describe('computeAgentRequestFingerprint', () => { it('is stable for the same graph-determining fields (ignoring other body keys)', () => { const a = computeAgentRequestFingerprint({ diff --git a/packages/api/src/agents/hitl/policy.ts b/packages/api/src/agents/hitl/policy.ts index 795b435aff..1c65798f31 100644 --- a/packages/api/src/agents/hitl/policy.ts +++ b/packages/api/src/agents/hitl/policy.ts @@ -238,7 +238,113 @@ export const RESUME_CONTEXT_KEYS = [ 'manualSkills', ] as const; -export type ResumeContext = Partial>; +export type ResumeContext = Partial> & { + /** Resolved model params captured at pause (sanitized); replayed by the resume route. */ + model_parameters?: Record; +}; + +/** Exact (lowercased) parameter keys that carry credentials or server transport config. */ +const SENSITIVE_PARAM_KEYS = new Set([ + 'auth', + 'authoptions', + 'auth_options', + 'token', + 'accesstoken', + 'access_token', + 'refreshtoken', + 'refresh_token', + 'idtoken', + 'id_token', + 'sessiontoken', + 'session_token', + 'configuration', + 'client', + 'clientoptions', + 'client_options', + 'fetchoptions', + 'fetch_options', + 'fetch', + 'httpagent', + 'httpsagent', + 'callbacks', + 'endpointhost', + 'endpoint_host', +]); + +/** Key fragments matched anywhere in a (lowercased) key, e.g. `azureOpenAIApiKey`. */ +const SENSITIVE_PARAM_KEY_FRAGMENTS = [ + 'apikey', + 'api_key', + 'api-key', + 'apiurl', + 'api_url', + 'api-url', + 'secret', + 'password', + 'credential', + 'authorization', + 'azureopenai', + 'header', + 'proxy', + 'baseurl', + 'base_url', + 'basepath', + 'base_path', +]; + +function isSensitiveParamKey(key: string): boolean { + const normalized = key.toLowerCase(); + if (SENSITIVE_PARAM_KEYS.has(normalized)) { + return true; + } + return SENSITIVE_PARAM_KEY_FRAGMENTS.some((fragment) => normalized.includes(fragment)); +} + +/** Bounded recursion guard for pathological / cyclic parameter graphs. */ +const MAX_SANITIZE_DEPTH = 8; + +function sanitizeParamValue(value: unknown, depth: number): unknown { + if (typeof value === 'function') { + return undefined; + } + if (Array.isArray(value)) { + return depth >= MAX_SANITIZE_DEPTH + ? undefined + : value.map((item) => sanitizeParamValue(item, depth + 1)); + } + if (value != null && typeof value === 'object') { + if (depth >= MAX_SANITIZE_DEPTH) { + return undefined; + } + const sanitized: Record = {}; + for (const [key, child] of Object.entries(value as Record)) { + if (isSensitiveParamKey(key) || typeof child === 'function') { + continue; + } + sanitized[key] = sanitizeParamValue(child, depth + 1); + } + return sanitized; + } + return value; +} + +/** + * Strip credentials and server transport config from resolved model parameters before + * they are persisted for resume replay. The initialized agent's `model_parameters` are + * the resolved `llmConfig` — they carry provider secrets (`apiKey`, Azure key names, + * Google `authOptions`, Bedrock `credentials`) and gateway config (`configuration`, + * headers, base URLs). Resume re-resolves all of those server-side from env/config, so + * only the user-level generation params (temperature, max tokens, custom endpoint + * params, …) need to survive the round trip. + */ +export function sanitizeResumeModelParameters( + params: unknown, +): Record | undefined { + if (params == null || typeof params !== 'object' || Array.isArray(params)) { + return undefined; + } + return sanitizeParamValue(params, 0) as Record; +} /** Extract the graph-determining fields from a request body for durable replay. */ export function pickResumeContext(body: Record | undefined | null): ResumeContext { @@ -319,3 +425,23 @@ export function buildPendingAction( resumeContext: ctx.resumeContext, }; } + +/** + * Client-facing projection of a pending action. `requestFingerprint` and `resumeContext` + * are server-only replay state — `resumeContext` in particular carries the resolved + * model parameters — so every copy that leaves the server (SSE, status, resume state) + * must go through this. The full record stays in the job store for the resume route. + */ +export function toClientPendingAction( + pendingAction: Agents.PendingAction | undefined | null, +): Agents.PendingAction | undefined { + if (pendingAction == null) { + return undefined; + } + const { + requestFingerprint: _requestFingerprint, + resumeContext: _resumeContext, + ...clientSafe + } = pendingAction; + return clientSafe; +} diff --git a/packages/api/src/stream/GenerationJobManager.ts b/packages/api/src/stream/GenerationJobManager.ts index fa66f0a051..d6d34d8304 100644 --- a/packages/api/src/stream/GenerationJobManager.ts +++ b/packages/api/src/stream/GenerationJobManager.ts @@ -33,6 +33,7 @@ import { isPendingActionStale, isPendingActionExpired } from './interfaces/IJobS import { InMemoryEventTransport } from './implementations/InMemoryEventTransport'; import { InMemoryJobStore } from './implementations/InMemoryJobStore'; import { filterPersistableAbortContent } from './abortContent'; +import { toClientPendingAction } from '~/agents/hitl/policy'; import { ApprovalLifecycle } from './ApprovalLifecycle'; /** Terminal error surfaced to a client still attached when its approval window lapses. */ @@ -1107,7 +1108,10 @@ class GenerationJobManagerClass { ...pendingEvents, { event: ApprovalEvents.ON_PENDING_ACTION, - data: liveJob.pendingAction as unknown as Record, + data: toClientPendingAction(liveJob.pendingAction) as unknown as Record< + string, + unknown + >, }, ]; } @@ -1612,10 +1616,11 @@ class GenerationJobManagerClass { collectedUsage, contextUsage, // Carry the live pending approval in the resume contract so a reloading / - // cross-replica client can rebuild the prompt from resumeState. + // cross-replica client can rebuild the prompt from resumeState. Client-safe + // projection: the stored record's resumeContext/requestFingerprint stay server-only. pendingAction: jobData.status === 'requires_action' && !isPendingActionStale(jobData) - ? jobData.pendingAction + ? toClientPendingAction(jobData.pendingAction) : undefined, }; } diff --git a/packages/api/src/stream/__tests__/pendingAction.spec.ts b/packages/api/src/stream/__tests__/pendingAction.spec.ts index 0098e9865e..67d6258529 100644 --- a/packages/api/src/stream/__tests__/pendingAction.spec.ts +++ b/packages/api/src/stream/__tests__/pendingAction.spec.ts @@ -67,6 +67,30 @@ describe('ApprovalLifecycle via GenerationJobManager.approvals (in-memory)', () test('returns false when the job does not exist', async () => { expect(await manager.approvals.pause('nonexistent', buildAction('nonexistent'))).toBe(false); }); + + test('client-facing copies omit resumeContext/requestFingerprint; the stored record keeps them', async () => { + const streamId = 'stream-redact'; + await manager.createJob(streamId, 'user-1'); + + const action = buildAction(streamId, { + requestFingerprint: 'fp-hash', + resumeContext: { + endpoint: 'agents', + model_parameters: { temperature: 0.7 }, + }, + }); + expect(await manager.approvals.pause(streamId, action)).toBe(true); + + const resumeState = await manager.getResumeState(streamId); + expect(resumeState?.pendingAction?.actionId).toBe(action.actionId); + expect(resumeState?.pendingAction?.resumeContext).toBeUndefined(); + expect(resumeState?.pendingAction?.requestFingerprint).toBeUndefined(); + + // The resume route replays from the job store, which must retain the full record. + const job = await manager.getJob(streamId); + expect(job?.metadata.pendingAction?.resumeContext).toEqual(action.resumeContext); + expect(job?.metadata.pendingAction?.requestFingerprint).toBe('fp-hash'); + }); }); describe('peek', () => {