Redact credentials at tool-error construction and in source_errors (#234)

* redact credentials at tool-error construction and in source_errors

* addressed ai review

* addressed timon's review

* addressed ai review: Use an explicit undefined check

---------

Co-authored-by: zhao <jornathanm910923@gmail.com>

Author: jonathanMLDev

docs/SECURITY.md

@@ -11,6 +11,7 @@
 `src/logger.ts` implements `redactApiKey` and recursive redaction for structured log data:
 
 - UUID-shaped tokens (`xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx`) → `***`
+  The UUID mask is intentional: legacy (pre-`pcsk_`) Pinecone API keys are UUID-formatted, so this pattern is a credential mask, not just an incidental document-ID match. Tool-error `message`/`suggestion` therefore use the full `redactApiKey` (including UUID masking), not a document-ID-preserving variant.
 - Modern Pinecone keys (`pcsk_…`) → `***`
 - Substrings after `apiKey` / `api_key` / similar patterns → masked
 - `Authorization: Bearer …` tokens → masked
@@ -19,9 +20,14 @@ Logs go to **stderr**; use `PINECONE_READ_ONLY_MCP_LOG_FORMAT=json` for pipeline
 
 ## MCP response redaction
 
-Tool responses returned to MCP clients (and LLM consumers) are sanitized in `src/core/server/tool-response.ts` via `redactSensitiveFields()` before JSON serialization. Only known sensitive keys are masked (`message`, `suggestion`, `degradation_reason`); document metadata UUIDs and other non-sensitive fields are preserved.
+Tool responses are sanitized at construction and at the serialization boundary:
 
-This covers tool error payloads, hybrid degradation reasons, and SDK error text surfaced in DEBUG log mode — not only stderr logs.
+- `src/core/server/tool-error.ts`: `pineconeToolError` and `timeoutToolError` apply `redactApiKey` to `message` before the `ToolError` object is built; `timeoutToolError` redacts caller-supplied `suggestion` only (the default suggestion is a static string with no secrets).
+- `src/logger.ts`: `redactErrorMessage` redacts credential-shaped substrings from error values; `redactSensitiveFields()` masks known sensitive keys (`message`, `suggestion`, `degradation_reason`) and every string value nested directly under `source_errors` (keyed by source name, not a sensitive field name).
+- `src/core/server/source-registry.ts`: multi-source `source_errors` rejection messages are redacted via `redactErrorMessage` when the per-source error map is built.
+- `src/core/server/tool-response.ts`: `jsonResponse` / `jsonErrorResponse` call `redactSensitiveFields()` before JSON serialization (boundary layer).
+
+Document metadata UUIDs and other non-sensitive fields are preserved throughout MCP response construction, aggregation, and serialization.
 
 ## Private config content (descriptions and schemas)
 

src/core/pinecone/rerank.ts

@@ -3,7 +3,7 @@
  */
 
 import type { Pinecone } from '@pinecone-database/pinecone';
-import { error as logError, redactApiKey } from '../../logger.js';
+import { error as logError, redactErrorMessage } from '../../logger.js';
 import { DEFAULT_REQUEST_TIMEOUT_MS } from '../config.js';
 import { runWithPolicy } from '../server/retry.js';
 import type { MergedHit, SearchResult } from '../../types.js';
@@ -66,7 +66,7 @@ export async function rerankResults(
     return { results: reranked, degraded: false };
   } catch (error) {
     logError('Error reranking results', error);
-    const msg = redactApiKey(error instanceof Error ? error.message : String(error));
+    const msg = redactErrorMessage(error);
     return {
       results: results.slice(0, topN).map((result) => ({
         id: result._id || '',

src/core/server/redaction.test.ts

@@ -15,6 +15,8 @@ import {
   makeNamespaceCacheEntry,
   makeSearchResult,
   parseToolJson,
+  PCSK_KEY,
+  UUID_KEY,
 } from './tools/test-helpers.js';
 
 vi.mock('./client-context.js', () => ({
@@ -33,9 +35,6 @@ vi.mock('./suggestion-flow.js', async (importOriginal) => {
   };
 });
 
-const PCSK_KEY = 'pcsk_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ';
-const UUID_KEY = '12345678-1234-1234-1234-123456789abc';
-
 const flowOk = {
   ok: true as const,
   flow: {
@@ -78,6 +77,19 @@ describe('redactSensitiveFields', () => {
     expect(out.message).not.toContain(PCSK_KEY);
     expect(out.results[0].metadata.document_id).toBe(UUID_KEY);
   });
+
+  it('redacts string values nested under source_errors regardless of key name', () => {
+    const input = {
+      source_errors: {
+        api_key_2: `unreachable: ${PCSK_KEY}`,
+        api_key_3: 'benign message with no token',
+      },
+    };
+    const out = redactSensitiveFields(input) as typeof input;
+    expect(out.source_errors.api_key_2).not.toContain(PCSK_KEY);
+    expect(out.source_errors.api_key_2).toContain('***');
+    expect(out.source_errors.api_key_3).toBe('benign message with no token');
+  });
 });
 
 describe('MCP response redaction', () => {
@@ -99,6 +111,8 @@ describe('MCP response redaction', () => {
     const err = pineconeToolError(`PINECONE_ERROR with ${PCSK_KEY}`, {
       suggestion: `retry with ${PCSK_KEY}`,
     });
+    expect(err.message).not.toContain(PCSK_KEY);
+    expect(err.suggestion).not.toContain(PCSK_KEY);
     const text = jsonErrorResponse(err).content[0]!.text;
     expect(text).not.toContain(PCSK_KEY);
     expect(text).toContain('***');
@@ -107,6 +121,8 @@ describe('MCP response redaction', () => {
   it('redacts classifyToolCatchError output in DEBUG mode', () => {
     setLogLevel('DEBUG');
     const err = classifyToolCatchError(new Error(`SDK auth failed ${PCSK_KEY}`), 'fallback');
+    expect(err.message).not.toContain(PCSK_KEY);
+    expect(err.message).toContain('***');
     const text = jsonErrorResponse(err).content[0]!.text;
     expect(text).not.toContain(PCSK_KEY);
   });

src/core/server/retry.ts

@@ -7,7 +7,7 @@
  * waiter still rejects immediately on timeout.
  */
 
-import { warn, redactApiKey } from '../../logger.js';
+import { warn, redactErrorMessage } from '../../logger.js';
 
 /** Matches {@link withTimeout} rejection message prefix; used by tool-error and callers. */
 export const APP_TIMEOUT_PATTERN = /^Timeout after \d+ms while waiting for /i;
@@ -140,7 +140,7 @@ export function runWithPolicy<T>(
     backoffMs: options.backoffMs,
     shouldRetry: transientShouldRetry,
     onRetry: (attempt, error) => {
-      const msg = redactApiKey(error instanceof Error ? error.message : String(error));
+      const msg = redactErrorMessage(error);
       warn(`Retrying ${options.label} (attempt ${attempt})`, msg);
     },
   });

src/core/server/source-registry.test.ts

@@ -1,6 +1,7 @@
 import { describe, expect, it, vi } from 'vitest';
 import { buildSourceRegistry } from './source-registry.js';
 import type { SourceDefinition } from './source-config.js';
+import { PCSK_KEY, makeFailingSourceClient } from './tools/test-helpers.js';
 
 const sources: SourceDefinition[] = [
   { name: 'api_key_1', apiKey: 'k1', indexName: 'idx-a', sparseIndexName: 'idx-a-sparse' },
@@ -67,14 +68,14 @@ describe('SourceRegistry', () => {
 
   it('returns partial namespaces and source_errors when one source fails', async () => {
     const client1 = mockClient('api_key_1');
-    const client2 = {
-      listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error('api_key_2 unreachable')),
-      checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }),
-      getSparseIndexName: () => 'api_key_2-sparse',
-    };
     const clients = new Map([
       ['api_key_1', client1 as never],
-      ['api_key_2', client2 as never],
+      [
+        'api_key_2',
+        makeFailingSourceClient('api_key_2 unreachable', {
+          sparseIndexName: 'api_key_2-sparse',
+        }) as never,
+      ],
     ]);
     const registry = buildSourceRegistry({
       sources,
@@ -91,6 +92,30 @@ describe('SourceRegistry', () => {
     expect(result.cache_hit).toBe(false);
   });
 
+  it('redacts credential-shaped tokens in source_errors at construction time', async () => {
+    const client1 = mockClient('api_key_1');
+    const clients = new Map([
+      ['api_key_1', client1 as never],
+      [
+        'api_key_2',
+        makeFailingSourceClient(`api_key_2 unreachable: auth failed ${PCSK_KEY}`, {
+          sparseIndexName: 'api_key_2-sparse',
+        }) as never,
+      ],
+    ]);
+    const registry = buildSourceRegistry({
+      sources,
+      defaultSource: 'api_key_1',
+      cacheTtlMs: 60_000,
+      defaultTopK: 10,
+      requestTimeoutMs: 15_000,
+      clients,
+    });
+    const result = await registry.getAllNamespacesWithCache();
+    expect(result.source_errors?.['api_key_2']).not.toContain(PCSK_KEY);
+    expect(result.source_errors?.['api_key_2']).toContain('***');
+  });
+
   it('aggregates warnings across sources in getAllNamespacesWithCache', async () => {
     const client1 = {
       listNamespacesWithMetadata: vi.fn().mockResolvedValue({

src/core/server/source-registry.ts

@@ -3,6 +3,7 @@
  */
 
 import { PineconeClient } from '../pinecone-client.js';
+import { redactErrorMessage } from '../../logger.js';
 import type { NamespaceInfo } from './server-context.js';
 import { fetchNamespacesWithDeclaredConfig, type NamespacesCacheEntry } from './namespace-cache.js';
 import type { SourceDefinition } from './source-config.js';
@@ -151,9 +152,7 @@ export class SourceRegistry {
         maxExpires = Math.max(maxExpires, outcome.value.result.expires_at);
       } else {
         cache_hit = false;
-        const message =
-          outcome.reason instanceof Error ? outcome.reason.message : String(outcome.reason);
-        source_errors[name] = message;
+        source_errors[name] = redactErrorMessage(outcome.reason);
       }
     }
     const expires_at = maxExpires > 0 ? maxExpires : Date.now() + this.cacheTtlMs;

src/core/server/tool-error.test.ts

@@ -1,8 +1,10 @@
 import { describe, expect, it } from 'vitest';
 import {
   classifyToolCatchError,
+  DEFAULT_TIMEOUT_SUGGESTION,
   flowGateToolError,
   lifecycleToolError,
+  timeoutToolError,
   toolErrorSchema,
   validationToolError,
 } from './tool-error.js';
@@ -56,4 +58,16 @@ describe('ToolError schema and builders', () => {
     expect(err.suggestion).toMatch(/retry|timeout/i);
     toolErrorSchema.parse(err);
   });
+
+  it('TIMEOUT: default suggestion is unredacted literal', () => {
+    const err = timeoutToolError('Timeout after 100ms');
+    expect(err.suggestion).toBe(DEFAULT_TIMEOUT_SUGGESTION);
+    toolErrorSchema.parse(err);
+  });
+
+  it('TIMEOUT: explicit empty suggestion is preserved, not replaced by default', () => {
+    const err = timeoutToolError('Timeout after 100ms', { suggestion: '' });
+    expect(err.suggestion).toBe('');
+    toolErrorSchema.parse(err);
+  });
 });

src/core/server/tool-error.ts

@@ -4,7 +4,7 @@
  */
 
 import { z } from 'zod';
-import { getLogLevel, error as logError, info } from '../../logger.js';
+import { getLogLevel, redactApiKey, error as logError, info } from '../../logger.js';
 import { isAppTimeoutError } from './retry.js';
 
 /** User-facing error message: detailed in DEBUG, generic otherwise. */
@@ -60,7 +60,7 @@ const timeoutToolErrorSchema = z.object({
   code: z.literal('TIMEOUT'),
   message: z.string(),
   recoverable: z.literal(true),
-  suggestion: z.string().optional(),
+  suggestion: z.string(),
 });
 
 const lifecycleToolErrorSchema = z.object({
@@ -80,7 +80,8 @@ export const toolErrorSchema = z.discriminatedUnion('code', [
 
 export type ToolError = z.infer<typeof toolErrorSchema>;
 
-const DEFAULT_TIMEOUT_SUGGESTION = 'Retry the request, or increase --request-timeout-ms.';
+/** Default TIMEOUT suggestion when the caller does not supply one. */
+export const DEFAULT_TIMEOUT_SUGGESTION = 'Retry the request, or increase --request-timeout-ms.';
 
 export function flowGateToolError(namespace: string, message: string): ToolError {
   return {
@@ -111,18 +112,21 @@ export function pineconeToolError(
 ): ToolError {
   return {
     code: 'PINECONE_ERROR',
-    message,
+    message: redactApiKey(message),
     recoverable: options?.recoverable ?? false,
-    ...(options?.suggestion ? { suggestion: options.suggestion } : {}),
+    ...(options?.suggestion ? { suggestion: redactApiKey(options.suggestion) } : {}),
   };
 }
 
 export function timeoutToolError(message: string, options?: { suggestion?: string }): ToolError {
   return {
     code: 'TIMEOUT',
-    message,
+    message: redactApiKey(message),
     recoverable: true,
-    suggestion: options?.suggestion ?? DEFAULT_TIMEOUT_SUGGESTION,
+    suggestion:
+      options?.suggestion !== undefined
+        ? redactApiKey(options.suggestion)
+        : DEFAULT_TIMEOUT_SUGGESTION,
   };
 }
 

src/core/server/tools/list-namespaces-tool.context.test.ts

@@ -1,14 +1,16 @@
 import { describe, expect, it, vi } from 'vitest';
+import { setLogLevel } from '../../../logger.js';
 import { registerListNamespacesTool } from './list-namespaces-tool.js';
 import { listNamespacesResponseSchema } from '../response-schemas.js';
 import {
   createMockServer,
   createMultiSourceTestContext,
   createTestServerContext,
   expectMatchesResponseSchema,
-  makeMockPineconeClient,
+  makePartialFailureMultiSourceClients,
   mockNamespacesWithMetadataResult,
   parseToolJson,
+  PCSK_KEY,
 } from './test-helpers.js';
 
 describe('list_namespaces tool handler (ServerContext instance path)', () => {
@@ -73,19 +75,7 @@ describe('list_namespaces tool handler (ServerContext instance path)', () => {
 
 describe('list_namespaces tool handler (multi-source)', () => {
   it('tags namespaces with source and propagates source_errors on partial failure', async () => {
-    const client1 = makeMockPineconeClient(['wg21']);
-    const client2 = {
-      listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error('api_key_2 unreachable')),
-      query: vi.fn(),
-      count: vi.fn(),
-      keywordSearch: vi.fn(),
-      checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }),
-      getSparseIndexName: () => 'sparse',
-    };
-    const clients = new Map([
-      ['api_key_1', client1],
-      ['api_key_2', client2],
-    ]);
+    const clients = makePartialFailureMultiSourceClients('api_key_2 unreachable');
     const { ctx } = createMultiSourceTestContext({ clients });
 
     const server = createMockServer();
@@ -99,6 +89,31 @@ describe('list_namespaces tool handler (multi-source)', () => {
     expect(body['cache_hit']).toBe(false);
   });
 
+  it('redacts a credential-shaped token embedded in a source_errors rejection message at DEBUG log level', async () => {
+    setLogLevel('DEBUG');
+    try {
+      const clients = makePartialFailureMultiSourceClients(
+        `api_key_2 unreachable: auth failed ${PCSK_KEY}`
+      );
+      const { ctx } = createMultiSourceTestContext({ clients });
+
+      const server = createMockServer();
+      registerListNamespacesTool(server as never, ctx);
+      const raw = await server.getHandler('list_namespaces')!({});
+      const text = raw.content[0]!.text;
+      expect(text).not.toContain(PCSK_KEY);
+      expect(text).toContain('***');
+
+      const body = parseToolJson(raw);
+      expectMatchesResponseSchema(listNamespacesResponseSchema, body);
+      expect(body['source_errors']).toMatchObject({
+        api_key_2: expect.not.stringContaining(PCSK_KEY),
+      });
+    } finally {
+      setLogLevel('INFO');
+    }
+  });
+
   it('surfaces schema_source and config_warnings when declarations mismatch live data', async () => {
     const client1 = {
       listNamespacesWithMetadata: vi.fn().mockResolvedValue({

src/core/server/tools/test-helpers.ts

@@ -15,6 +15,12 @@ import {
 import type { ToolError, ToolErrorCode } from '../tool-error.js';
 import { toolErrorSchema } from '../tool-error.js';
 
+/** Credential-shaped test token for redaction assertions. Not a real key. */
+export const PCSK_KEY = 'pcsk_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ';
+
+/** Non-sensitive UUID for regression tests (must not be masked). */
+export const UUID_KEY = '12345678-1234-1234-1234-123456789abc';
+
 /** Handler invoked by MCP tool registration (params shape varies by tool). */
 export type ToolHandler = (params: Record<string, unknown>) => Promise<unknown>;
 
@@ -216,6 +222,35 @@ export function makeMockPineconeClient(
   };
 }
 
+/** Mock client that rejects namespace listing with the given error message. */
+export function makeFailingSourceClient(
+  rejectionMessage: string,
+  options?: { sparseIndexName?: string }
+) {
+  return {
+    listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error(rejectionMessage)),
+    query: vi.fn(),
+    count: vi.fn(),
+    keywordSearch: vi.fn(),
+    checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }),
+    getSparseIndexName: () => options?.sparseIndexName ?? 'sparse',
+  };
+}
+
+/** Two-source client map: api_key_1 succeeds; api_key_2 fails with rejectionMessage. */
+export function makePartialFailureMultiSourceClients(
+  rejectionMessage: string,
+  options?: { successNamespaces?: string[] }
+) {
+  return new Map([
+    ['api_key_1', makeMockPineconeClient(options?.successNamespaces ?? ['wg21'])],
+    [
+      'api_key_2',
+      makeFailingSourceClient(rejectionMessage, { sparseIndexName: 'api_key_2-sparse' }),
+    ],
+  ]);
+}
+
 export type MultiSourceTestContext = {
   ctx: CoreServerContext;
   clients: Map<string, ReturnType<typeof makeMockPineconeClient>>;