diff --git a/packages/api/src/middleware/__tests__/rateLimit.test.ts b/packages/api/src/middleware/__tests__/rateLimit.test.ts new file mode 100644 index 00000000..4f3db220 --- /dev/null +++ b/packages/api/src/middleware/__tests__/rateLimit.test.ts @@ -0,0 +1,111 @@ +import type { NextFunction, Request, Response } from 'express'; + +jest.mock('@/utils/logger', () => ({ + __esModule: true, + default: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, +})); + +// Dynamic user id per test so we can simulate multiple callers +let mockUserId = 'user-a'; +jest.mock('@/middleware/auth', () => ({ + getNonNullUserWithTeam: () => ({ + teamId: 't', + userId: mockUserId, + email: 'e', + }), +})); + +import { createRateLimiter } from '../rateLimit'; + +function makeReq(): Request { + return {} as Request; +} +function makeRes(): Response { + return {} as Response; +} + +// Invoke the middleware and capture what it passes to next() +function invoke(mw: ReturnType): { + error?: unknown; + passed: boolean; +} { + let captured: unknown; + let passed = false; + const next: NextFunction = err => { + if (err) captured = err; + else passed = true; + }; + mw(makeReq(), makeRes(), next); + return { error: captured, passed }; +} + +describe('createRateLimiter', () => { + beforeEach(() => { + mockUserId = 'user-a'; + }); + + it('allows requests under the limit', () => { + const mw = createRateLimiter({ windowMs: 60_000, max: 3, name: 'test' }); + expect(invoke(mw).passed).toBe(true); + expect(invoke(mw).passed).toBe(true); + expect(invoke(mw).passed).toBe(true); + }); + + it('rejects the (max+1)th request with a 429', () => { + const mw = createRateLimiter({ windowMs: 60_000, max: 2, name: 'test' }); + invoke(mw); + invoke(mw); + const { passed, error } = invoke(mw); + expect(passed).toBe(false); + expect(error).toBeDefined(); + // Api429Error has statusCode 429. The project's BaseError pattern puts + // the detail message in `.name` and a generic description in `.message`. + expect((error as { statusCode?: number }).statusCode).toBe(429); + expect((error as Error).name).toContain('Rate limit'); + }); + + it('resets after the window elapses', () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-01-01T00:00:00Z')); + const mw = createRateLimiter({ windowMs: 1000, max: 1, name: 'test' }); + expect(invoke(mw).passed).toBe(true); + expect(invoke(mw).passed).toBe(false); // over + jest.setSystemTime(new Date('2026-01-01T00:00:01.500Z')); // past window + expect(invoke(mw).passed).toBe(true); + jest.useRealTimers(); + }); + + it('tracks users independently', () => { + const mw = createRateLimiter({ windowMs: 60_000, max: 1, name: 'test' }); + mockUserId = 'user-a'; + expect(invoke(mw).passed).toBe(true); + expect(invoke(mw).passed).toBe(false); // a is over + + mockUserId = 'user-b'; + expect(invoke(mw).passed).toBe(true); // b is fresh + }); + + it('returns independent buckets per limiter instance', () => { + const mwA = createRateLimiter({ windowMs: 60_000, max: 1, name: 'A' }); + const mwB = createRateLimiter({ windowMs: 60_000, max: 1, name: 'B' }); + expect(invoke(mwA).passed).toBe(true); + expect(invoke(mwA).passed).toBe(false); + // different limiter — still fresh + expect(invoke(mwB).passed).toBe(true); + }); + + it('includes the limiter name and retry seconds in the error message', () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-01-01T00:00:00Z')); + const mw = createRateLimiter({ + windowMs: 30_000, + max: 1, + name: 'summarize', + }); + invoke(mw); + const { error } = invoke(mw); + expect((error as Error).name).toContain('summarize'); + expect((error as Error).name).toMatch(/Try again in \d+s/); + jest.useRealTimers(); + }); +}); diff --git a/packages/api/src/routers/api/__tests__/aiSummarize.test.ts b/packages/api/src/routers/api/__tests__/aiSummarize.test.ts index dace2010..d76e4ebd 100644 --- a/packages/api/src/routers/api/__tests__/aiSummarize.test.ts +++ b/packages/api/src/routers/api/__tests__/aiSummarize.test.ts @@ -151,6 +151,30 @@ describe('redactSecrets', () => { 'error: database timeout after 30s', ); }); + + it('redacts JSON-shape secrets (quoted key/value)', () => { + const input = '{"password":"s3cret","user":"alice"}'; + const out = redactSecrets(input); + expect(out).not.toContain('s3cret'); + expect(out).toContain('"password":"[REDACTED]"'); + expect(out).toContain('"user":"alice"'); + }); + + it('redacts JSON-shape with whitespace', () => { + const input = '{ "api_key" : "abc123" }'; + const out = redactSecrets(input); + expect(out).not.toContain('abc123'); + expect(out).toContain('[REDACTED]'); + }); + + it('redacts HTTP-style secret headers', () => { + expect(redactSecrets('X-Api-Key: abc123')).toContain( + 'X-Api-Key: [REDACTED]', + ); + expect(redactSecrets('X-Auth-Token: xyz')).toContain( + 'X-Auth-Token: [REDACTED]', + ); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/api/src/routers/api/aiSummarize.ts b/packages/api/src/routers/api/aiSummarize.ts index 859b02f8..55695e09 100644 --- a/packages/api/src/routers/api/aiSummarize.ts +++ b/packages/api/src/routers/api/aiSummarize.ts @@ -32,6 +32,15 @@ export const summarizeBodySchema = z.object({ tone: z.enum(TONE_VALUES).optional(), // Optional conversation history for future follow-up-question flows. // Each message is bounded; history is bounded; prevents unbounded prompt growth. + // + // SECURITY / TRUST BOUNDARY: messages are client-supplied, which means a + // caller can claim the assistant previously said anything. That is fine for + // the current single-shot summarize flow (no downstream consumer trusts + // these as "authentic model output"). When a real follow-up UI is built, + // move conversation state to the server (keyed by a server-issued + // conversationId) instead of round-tripping messages through the client, or + // restrict `role` to 'user' only and let the server interleave its own + // prior assistant replies from storage. messages: z .array( z.object({ @@ -119,17 +128,41 @@ export function wrapContent(content: string): string { // --------------------------------------------------------------------------- // Secret redaction — scrubs obvious credentials from context before sending. -// Catches common patterns: `password=...`, `token=...`, `api_key=...`, -// `Authorization: Bearer ...`, JWT-shaped strings. +// Best-effort allowlist; not a guarantee. Recipes matched today: +// - key=value pairs: password=secret, api_key=abc +// - JSON key/value pairs: "password": "secret" +// - HTTP Authorization values: Bearer ..., Basic ... +// - HTTP-style secret headers: X-Api-Key: abc, X-Auth-Token: xyz +// - JWT-shaped strings: eyJ...header.payload.signature +// +// Missing recipes (extend when spotted): URL-encoded values, basic-auth URLs +// (user:pass@host), SSH private key blocks. If redaction fires in production, +// count it so we can tell if users are routinely sending secrets. // --------------------------------------------------------------------------- +const SECRET_KEY_TOKENS = + 'password|passwd|pwd|secret|token|api[_-]?key|apikey|access[_-]?key|private[_-]?key|client[_-]?secret|authorization|auth'; + const REDACTION_PATTERNS: { re: RegExp; replace: string }[] = [ - // key=value pairs with secret-ish keys + // key=value pairs { - re: /\b(password|passwd|pwd|secret|token|api[_-]?key|apikey|access[_-]?key|private[_-]?key|client[_-]?secret|authorization|auth)=([^\s,;&"'`]+)/gi, + re: new RegExp(`\\b(${SECRET_KEY_TOKENS})=([^\\s,;&"'\`]+)`, 'gi'), replace: '$1=[REDACTED]', }, - // HTTP Authorization headers + // JSON-shape: "key": "value" or "key":"value" (quoted value) + { + re: new RegExp(`("(?:${SECRET_KEY_TOKENS})"\\s*:\\s*)"[^"]*"`, 'gi'), + replace: '$1"[REDACTED]"', + }, + // HTTP-header shape: X-Api-Key: value (colon-separated on one line) + { + re: new RegExp( + `\\b(x[-_]?(?:api[-_]?key|auth[-_]?token|access[-_]?token)|api[-_]?key)\\s*:\\s*([^\\s,;]+)`, + 'gi', + ), + replace: '$1: [REDACTED]', + }, + // Authorization headers { re: /Bearer\s+[A-Za-z0-9._~+/=-]+/gi, replace: 'Bearer [REDACTED]', diff --git a/packages/app/src/components/__tests__/AISummarize.test.tsx b/packages/app/src/components/__tests__/AISummarize.test.tsx index 204a0fd1..657fc22a 100644 --- a/packages/app/src/components/__tests__/AISummarize.test.tsx +++ b/packages/app/src/components/__tests__/AISummarize.test.tsx @@ -38,8 +38,13 @@ jest.mock('@/source', () => ({ useSource: () => ({ data: undefined, isLoading: false }), })); +const mockUseEventsAroundFocus = jest.fn((_args: unknown) => ({ + rows: [] as unknown[], + meta: [] as unknown[], + isFetching: false, +})); jest.mock('../DBTraceWaterfallChart', () => ({ - useEventsAroundFocus: () => ({ rows: [], meta: [], isFetching: false }), + useEventsAroundFocus: (args: unknown) => mockUseEventsAroundFocus(args), })); let mockEasterEggVisible = true; @@ -470,6 +475,54 @@ describe('AISummarizePatternButton', () => { // Direct import of the panel for isolated tests import AISummaryPanelComponent from '../aiSummarize/AISummaryPanel'; +// --------------------------------------------------------------------------- +// supportsTraceContext gate — a subject without trace context support must +// never enable the trace-span fetch, even when trace props are passed. +// --------------------------------------------------------------------------- + +describe('trace context gate', () => { + beforeEach(() => { + mockUseEventsAroundFocus.mockClear(); + mockMeData = { aiAssistantEnabled: true }; + mockEasterEggVisible = false; + }); + + it('pattern subject never enables trace fetch', () => { + const pattern: Pattern = { + id: 'p1', + pattern: 'GET /api/<*>', + count: 10, + samples: [], + }; + renderWithMantine( + , + ); + expect(mockUseEventsAroundFocus).toHaveBeenCalled(); + // Every call must have enabled=false for a pattern subject, regardless + // of other props — the subject doesn't support trace context. + for (const call of mockUseEventsAroundFocus.mock.calls) { + const args = call[0] as { enabled: boolean }; + expect(args.enabled).toBe(false); + } + }); + + it('event subject without traceId does not enable trace fetch', () => { + renderWithMantine( + , + ); + for (const call of mockUseEventsAroundFocus.mock.calls) { + const args = call[0] as { enabled: boolean }; + expect(args.enabled).toBe(false); + } + }); +}); + describe('AISummaryPanel', () => { const AISummaryPanel = AISummaryPanelComponent; diff --git a/packages/app/src/components/aiSummarize/eventSubject.ts b/packages/app/src/components/aiSummarize/eventSubject.ts index 64585c49..11050dc4 100644 --- a/packages/app/src/components/aiSummarize/eventSubject.ts +++ b/packages/app/src/components/aiSummarize/eventSubject.ts @@ -1,4 +1,5 @@ // Subject: single log or trace event +import { attrToString } from './formatHelpers'; import { SummarySubject } from './subjects'; export interface EventSubjectInput { @@ -6,16 +7,9 @@ export interface EventSubjectInput { severityText?: string; } -function coerceAttrValue(v: unknown): string { - if (v == null) return ''; - if (typeof v === 'string') return v; - if (typeof v === 'number' || typeof v === 'boolean') return String(v); - try { - return JSON.stringify(v); - } catch { - return String(v); - } -} +// Effectively unlimited for event attributes — 500 is far above typical +// attribute lengths but caps pathological cases. +const coerceAttrValue = (v: unknown) => attrToString(v, 500); export function formatEventContent( { rowData, severityText }: EventSubjectInput, diff --git a/packages/app/src/components/aiSummarize/formatHelpers.ts b/packages/app/src/components/aiSummarize/formatHelpers.ts new file mode 100644 index 00000000..0893f0c7 --- /dev/null +++ b/packages/app/src/components/aiSummarize/formatHelpers.ts @@ -0,0 +1,24 @@ +// Shared string-coercion helpers used by the subject formatters and the +// trace context builder. Each of these was previously duplicated with +// slight variations — keeping them here keeps the LLM-prompt output +// consistent across subjects. + +/** + * Turn any attribute value into a short string suitable for the LLM prompt. + * Objects are JSON-stringified (with a try/catch for circular refs). + * Truncates at `maxLen` with an ellipsis. + */ +export function attrToString(v: unknown, maxLen = 100): string { + if (v == null) return ''; + let s: string; + if (typeof v === 'string') s = v; + else if (typeof v === 'number' || typeof v === 'boolean') s = String(v); + else { + try { + s = JSON.stringify(v); + } catch { + s = String(v); + } + } + return s.length > maxLen ? s.slice(0, maxLen - 3) + '...' : s; +} diff --git a/packages/app/src/components/aiSummarize/patternSubject.ts b/packages/app/src/components/aiSummarize/patternSubject.ts index cbcfa7da..63dac445 100644 --- a/packages/app/src/components/aiSummarize/patternSubject.ts +++ b/packages/app/src/components/aiSummarize/patternSubject.ts @@ -5,6 +5,7 @@ import { SEVERITY_TEXT_COLUMN_ALIAS, } from '@/hooks/usePatterns'; +import { attrToString } from './formatHelpers'; import { SummarySubject } from './subjects'; export interface PatternSubjectInput { @@ -19,15 +20,9 @@ const SKIP_KEYS = new Set([ 'SortKey', ]); -function coerceAttr(v: unknown): string { - if (typeof v === 'string') return v; - if (typeof v === 'number' || typeof v === 'boolean') return String(v); - try { - return JSON.stringify(v); - } catch { - return String(v); - } -} +// Per-attribute cap; 200 chars is enough for URLs, status codes, short +// messages — anything longer is likely redacted body content. +const coerceAttr = (v: unknown) => attrToString(v, 200); export function formatPatternContent({ pattern, diff --git a/packages/app/src/components/aiSummarize/traceContext.ts b/packages/app/src/components/aiSummarize/traceContext.ts index 6e16eb11..5619cb49 100644 --- a/packages/app/src/components/aiSummarize/traceContext.ts +++ b/packages/app/src/components/aiSummarize/traceContext.ts @@ -2,18 +2,10 @@ // Includes: summary stats, span groups with duration stats, and error spans. import { isErrorEvent } from './classifiers'; +import { attrToString } from './formatHelpers'; -// SpanAttributes come from ClickHouse Map columns — values can be string, -// number, boolean, or nested objects depending on the source. Normalize -// before using. -export type TraceAttributeValue = - | string - | number - | boolean - | null - | undefined - | unknown; - +// SpanAttributes come from ClickHouse Map columns — values can be any type +// depending on the source schema. export interface TraceSpan { Body?: string; ServiceName?: string; @@ -22,7 +14,7 @@ export interface TraceSpan { SeverityText?: string; SpanId?: string; ParentSpanId?: string; - SpanAttributes?: Record; + SpanAttributes?: Record; } interface SpanGroup { @@ -44,22 +36,6 @@ function fmtMs(ms: number): string { return `${(ms / 1000).toFixed(2)}s`; } -// Coerce any attribute value to a short string for the LLM prompt. -function attrToString(v: TraceAttributeValue, maxLen = 100): string { - if (v == null) return ''; - let s: string; - if (typeof v === 'string') s = v; - else if (typeof v === 'number' || typeof v === 'boolean') s = String(v); - else { - try { - s = JSON.stringify(v); - } catch { - s = String(v); - } - } - return s.length > maxLen ? s.slice(0, maxLen - 3) + '...' : s; -} - function isSpanError(span: TraceSpan): boolean { return isErrorEvent({ severity: span.SeverityText, diff --git a/packages/app/src/components/aiSummarize/useAISummarizeState.ts b/packages/app/src/components/aiSummarize/useAISummarizeState.ts index bea8f85e..0e66e26a 100644 --- a/packages/app/src/components/aiSummarize/useAISummarizeState.ts +++ b/packages/app/src/components/aiSummarize/useAISummarizeState.ts @@ -113,9 +113,13 @@ export function useAISummarizeState({ clearTimeout(timerRef.current); timerRef.current = null; } - // release abort flag synchronously on next microtask — in-flight - // callbacks queued before this point see abortRef=true and bail out; - // any mutate() called *after* this runs works normally. + // Release abort flag on the next microtask — in-flight callbacks queued + // before this point see abortRef=true and bail out; any mutate() called + // *after* this runs works normally. + // + // Uses queueMicrotask (not setTimeout(0)) deliberately: tests that enable + // jest.useFakeTimers() would freeze a setTimeout-based reset and block + // all subsequent onSuccess callbacks. Microtasks are not faked by Jest. queueMicrotask(() => { abortRef.current = false; });