fix(tools): resolve {{ENV_VAR}} references in user-only params for copilot tool executions (#5679)

* fix(tools): resolve {{ENV_VAR}} references in user-only params for copilot tool executions Chat-invoked integration tools with API-key auth have been broken since the mothership tool-dispatch rewrite (#4090) removed the orchestrator's env reference resolution. Agents pass {{VAR}} references (the VFS exposes env var names only), and the literal placeholder was sent to the provider, producing auth failures like Sentry's 401 Invalid token. Resolution is deliberately narrower than the removed deep resolver: only whole-value references on params declared visibility user-only, gated to copilot executions, resolving against the same personal+workspace env merge workflow runs use. LLM-writable params (urls, headers, bodies) never resolve, so references cannot be used to extract secrets. Missing variables fail fast with an actionable error instead of a provider-side 401. * refactor(tools): delegate copilot env reference resolution to the shared executor resolver Replaces the hand-rolled exact-match regex with resolveEnvVarReferences (allowEmbedded: false), the same resolver used by workflow runs, MCP config, and webhooks — one set of reference semantics instead of two that can drift. Behavior is identical; all existing tests pass unchanged. * fix(tools): fail fast on env references without user context, clarify personal-only scope errors Addresses review: a copilot execution missing userId now errors explicitly instead of forwarding the literal placeholder upstream, and a missing-variable error without a workspace context explains that only personal variables are in scope there (matching workflow-run resolution semantics).
2026-09-24 15:45:35 +08:00 · 2026-07-14 16:38:42 -07:00
parent 515dafa274
commit c203f51545
2 changed files with 271 additions and 0 deletions
@@ -31,6 +31,7 @@ const {
  mockGetCustomToolByIdOrTitle,
  mockGenerateInternalToken,
  mockResolveWorkspaceFileReference,
+  mockGetEffectiveDecryptedEnv,
 } = vi.hoisted(() => ({
  mockIsHosted: { value: false },
  mockEnv: { NEXT_PUBLIC_APP_URL: 'http://localhost:3000' } as Record<string, string | undefined>,
@@ -46,6 +47,7 @@ const {
  mockGetCustomToolByIdOrTitle: vi.fn(),
  mockGenerateInternalToken: vi.fn(),
  mockResolveWorkspaceFileReference: vi.fn(),
+  mockGetEffectiveDecryptedEnv: vi.fn(),
 }))

 const mockSecureFetchWithPinnedIP = inputValidationMockFns.mockSecureFetchWithPinnedIP
@@ -107,6 +109,10 @@ vi.mock('@/lib/core/rate-limiter/hosted-key', () => ({
  getHostedKeyRateLimiter: () => mockRateLimiterFns,
 }))

+vi.mock('@/lib/environment/utils', () => ({
+  getEffectiveDecryptedEnv: (...args: unknown[]) => mockGetEffectiveDecryptedEnv(...args),
+}))
+
 vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
  resolveWorkspaceFileReference: (...args: unknown[]) => mockResolveWorkspaceFileReference(...args),
 }))
@@ -234,6 +240,26 @@ vi.mock('@/tools/registry', () => {
        return { success: true, output: data }
      },
    },
+    test_env_ref_tool: {
+      id: 'test_env_ref_tool',
+      name: 'Test Env Reference Tool',
+      description: 'Accepts a user-only API key and an llm-writable note',
+      version: '1.0.0',
+      params: {
+        apiKey: { type: 'string', required: true, visibility: 'user-only' },
+        note: { type: 'string', required: false, visibility: 'user-or-llm' },
+      },
+      request: {
+        url: '/api/tools/test/env-ref',
+        method: 'POST',
+        headers: () => ({ 'Content-Type': 'application/json' }),
+        body: (p: any) => ({ apiKey: p.apiKey, note: p.note }),
+      },
+      transformResponse: async (response: any) => {
+        const data = await response.json()
+        return { success: true, output: data }
+      },
+    },
    test_file_array_tool: {
      id: 'test_file_array_tool',
      name: 'Test File Array Tool',
@@ -1259,6 +1285,184 @@ describe('Copilot OAuth Credential Enforcement', () => {
  })
 })

+describe('Copilot Env Variable Reference Resolution', () => {
+  let cleanupEnvVars: () => void
+
+  function mockJsonFetch() {
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      status: 200,
+      statusText: 'OK',
+      headers: new Headers(),
+      json: () => Promise.resolve({ ok: true }),
+      text: () => Promise.resolve(JSON.stringify({ ok: true })),
+      clone: vi.fn().mockReturnThis(),
+    })
+    global.fetch = Object.assign(fetchMock, { preconnect: vi.fn() }) as typeof fetch
+    return fetchMock
+  }
+
+  function sentRequestBody(fetchMock: ReturnType<typeof vi.fn>): Record<string, unknown> {
+    return JSON.parse(fetchMock.mock.calls[0][1]?.body as string)
+  }
+
+  const copilotContext = () =>
+    createToolExecutionContext({
+      workspaceId: 'workspace-456',
+      userId: 'user-123',
+      copilotToolExecution: true,
+    } as any)
+
+  beforeEach(() => {
+    process.env.NEXT_PUBLIC_APP_URL = 'http://localhost:3000'
+    cleanupEnvVars = setupEnvVars({ NEXT_PUBLIC_APP_URL: 'http://localhost:3000' })
+    mockGetEffectiveDecryptedEnv.mockReset()
+    mockGetEffectiveDecryptedEnv.mockResolvedValue({ SENTRY_AUTH_TOKEN: 'sntrys_real_token' })
+  })
+
+  afterEach(() => {
+    vi.resetAllMocks()
+    cleanupEnvVars()
+  })
+
+  it('resolves a whole-value {{VAR}} reference in a user-only param', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{SENTRY_AUTH_TOKEN}}' },
+      { executionContext: copilotContext() }
+    )
+
+    expect(result.success).toBe(true)
+    expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('user-123', 'workspace-456')
+    expect(sentRequestBody(fetchMock).apiKey).toBe('sntrys_real_token')
+  })
+
+  it('trims whitespace inside the braces like the executor resolver', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{ SENTRY_AUTH_TOKEN }}' },
+      { executionContext: copilotContext() }
+    )
+
+    expect(result.success).toBe(true)
+    expect(sentRequestBody(fetchMock).apiKey).toBe('sntrys_real_token')
+  })
+
+  it('never resolves references in llm-writable (user-or-llm) params', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{SENTRY_AUTH_TOKEN}}', note: '{{SENTRY_AUTH_TOKEN}}' },
+      { executionContext: copilotContext() }
+    )
+
+    expect(result.success).toBe(true)
+    expect(sentRequestBody(fetchMock).note).toBe('{{SENTRY_AUTH_TOKEN}}')
+  })
+
+  it('leaves embedded references untouched in user-only params', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: 'Bearer {{SENTRY_AUTH_TOKEN}}' },
+      { executionContext: copilotContext() }
+    )
+
+    expect(result.success).toBe(true)
+    expect(sentRequestBody(fetchMock).apiKey).toBe('Bearer {{SENTRY_AUTH_TOKEN}}')
+    expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled()
+  })
+
+  it('fails with a clear error before any request when the variable is missing', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{MISSING_VAR}}' },
+      { executionContext: copilotContext() }
+    )
+
+    expect(result.success).toBe(false)
+    expect(result.error).toContain('MISSING_VAR')
+    expect(result.error).toContain('apiKey')
+    expect(fetchMock).not.toHaveBeenCalled()
+  })
+
+  it('fails fast instead of forwarding the placeholder when user context is missing', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{SENTRY_AUTH_TOKEN}}' },
+      {
+        executionContext: createToolExecutionContext({
+          workspaceId: 'workspace-456',
+          userId: undefined,
+          copilotToolExecution: true,
+        } as any),
+      }
+    )
+
+    expect(result.success).toBe(false)
+    expect(result.error).toContain('authenticated user context')
+    expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled()
+    expect(fetchMock).not.toHaveBeenCalled()
+  })
+
+  it('explains the personal-only scope when a variable is missing without a workspace context', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{MISSING_VAR}}' },
+      {
+        executionContext: createToolExecutionContext({
+          workspaceId: undefined,
+          userId: 'user-123',
+          copilotToolExecution: true,
+        } as any),
+      }
+    )
+
+    expect(result.success).toBe(false)
+    expect(result.error).toContain('only personal variables are available')
+    expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('user-123', undefined)
+    expect(fetchMock).not.toHaveBeenCalled()
+  })
+
+  it('does not resolve references outside copilot execution', async () => {
+    const fetchMock = mockJsonFetch()
+
+    const result = await executeTool(
+      'test_env_ref_tool',
+      { apiKey: '{{SENTRY_AUTH_TOKEN}}' },
+      { executionContext: createToolExecutionContext({ userId: 'user-123' } as any) }
+    )
+
+    expect(result.success).toBe(true)
+    expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled()
+    expect(sentRequestBody(fetchMock).apiKey).toBe('{{SENTRY_AUTH_TOKEN}}')
+  })
+
+  it('never mutates the caller-owned params object (log-leak guard)', async () => {
+    mockJsonFetch()
+    const callerParams = { apiKey: '{{SENTRY_AUTH_TOKEN}}' }
+
+    const result = await executeTool('test_env_ref_tool', callerParams, {
+      executionContext: copilotContext(),
+    })
+
+    expect(result.success).toBe(true)
+    expect(callerParams.apiKey).toBe('{{SENTRY_AUTH_TOKEN}}')
+  })
+})
+
 describe('Centralized Error Handling', () => {
  let cleanupEnvVars: () => void

@@ -33,6 +33,7 @@ import { assertPermissionsAllowed } from '@/ee/access-control/utils/permission-c
 import { isCustomTool, isMcpTool } from '@/executor/constants'
 import { resolveSkillContent } from '@/executor/handlers/agent/skills-resolver'
 import type { ExecutionContext, UserFile } from '@/executor/types'
+import { resolveEnvVarReferences } from '@/executor/utils/reference-validation'
 import type { ErrorInfo } from '@/tools/error-extractors'
 import { extractErrorMessage } from '@/tools/error-extractors'
 import type {
@@ -183,6 +184,71 @@ async function normalizeCopilotFileParams(
  }
 }

+/**
+ * Resolves whole-value {{ENV_VAR}} references in user-only params for copilot
+ * tool executions. Chat agents never see secret values (the workspace VFS
+ * exposes env var names only), so they pass references; workflow runs resolve
+ * these in the executor, and this is the equivalent step for direct tool
+ * calls, delegating to the executor's resolver so both paths share one set of
+ * reference semantics. Resolution is deliberately restricted to params
+ * declared `visibility: 'user-only'` (API keys and other operator-supplied
+ * secrets) and to values that are exactly one reference, so LLM-writable
+ * params (URLs, headers, bodies) can never be used to extract secret values.
+ *
+ * Mutates only the given params object — callers pass the per-execution copy,
+ * never the copilot-side tool-call state, so decrypted values cannot leak
+ * into failure logs or persisted chat state.
+ */
+async function resolveCopilotEnvReferences(
+  tool: ToolConfig,
+  params: Record<string, unknown>,
+  scope: ToolExecutionScope
+): Promise<void> {
+  if (!scope.copilotToolExecution) {
+    return
+  }
+
+  const pending: Array<{ paramId: string; value: string }> = []
+  for (const [paramId, paramDef] of Object.entries(tool.params || {})) {
+    if (paramDef?.visibility !== 'user-only') continue
+    const value = params[paramId]
+    if (typeof value === 'string' && value.startsWith('{{') && value.endsWith('}}')) {
+      pending.push({ paramId, value })
+    }
+  }
+
+  if (pending.length === 0) {
+    return
+  }
+
+  if (!scope.userId) {
+    throw new Error(
+      `Cannot resolve environment variable reference in parameter "${pending[0].paramId}" without an authenticated user context.`
+    )
+  }
+
+  const { getEffectiveDecryptedEnv } = await import('@/lib/environment/utils')
+  const envVars = await getEffectiveDecryptedEnv(scope.userId, scope.workspaceId)
+
+  for (const { paramId, value } of pending) {
+    const missingKeys: string[] = []
+    const resolved = resolveEnvVarReferences(value, envVars, {
+      allowEmbedded: false,
+      missingKeys,
+    })
+    if (missingKeys.length > 0) {
+      const scopeHint = scope.workspaceId
+        ? ''
+        : ' (no workspace context — only personal variables are available here)'
+      throw new Error(
+        `Environment variable "${missingKeys[0]}" referenced by parameter "${paramId}" was not found${scopeHint}. ` +
+          `Check environment/variables.json for available variable names.`
+      )
+    }
+    params[paramId] = resolved as string
+  }
+}
+
 function readExplicitCredentialSelector(params: Record<string, unknown>): string | undefined {
  for (const key of ['credentialId', 'oauthCredential', 'credential'] as const) {
    const value = params[key]
@@ -1025,6 +1091,7 @@ export async function executeTool(
    await normalizeCopilotFileParams(tool, contextParams, scope)
    normalizeCopilotCredentialParams(contextParams)
    enforceCopilotCredentialSelection(toolId, tool, contextParams, scope)
+    await resolveCopilotEnvReferences(tool, contextParams, scope)

    // Inject hosted API key if tool supports it and user didn't provide one
    const hostedKeyInfo = await injectHostedKeyIfNeeded(