Skip to content

Commit 81a0de7

Browse files
authored
Merge pull request #3511 from trycompai/main
[comp] Production Deploy
2 parents 98314a9 + 55a48fb commit 81a0de7

28 files changed

Lines changed: 1747 additions & 69 deletions

‎apps/api/src/admin-organizations/admin-audit-log.interceptor.spec.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ jest.mock('@db', () => ({
5454
jest.mock('../audit/audit-log.constants', () => ({
5555
MUTATION_METHODS: new Set(['POST', 'PATCH', 'PUT', 'DELETE']),
5656
SENSITIVE_KEYS: new Set(['password', 'token']),
57+
SENSITIVE_KEY_PATTERN:
58+
/secret|password|passphrase|credential|token|api[_-]?key|private[_-]?key|totp|access[_-]?key/i,
5759
}));
5860

5961
function buildContext(overrides: {
@@ -261,6 +263,28 @@ describe('AdminAuditLogInterceptor', () => {
261263
});
262264
});
263265

266+
it('sanitizes credential-shaped keys the exact list misses (clientSecret)', (done) => {
267+
mockPolicyFind.mockResolvedValue({ name: 'Test' });
268+
269+
const ctx = buildContext({
270+
method: 'PATCH',
271+
url: '/v1/admin/organizations/org_1/policies/pol_1',
272+
params: { orgId: 'org_1' },
273+
body: { status: 'published', clientSecret: 'leak_me' },
274+
});
275+
276+
interceptor.intercept(ctx, nextHandler).subscribe({
277+
complete: () => {
278+
setTimeout(() => {
279+
const changes = mockCreate.mock.calls[0][0].data.data.changes;
280+
expect(changes.status).toBeDefined();
281+
expect(changes.clientSecret).toBeUndefined();
282+
done();
283+
}, 50);
284+
},
285+
});
286+
});
287+
264288
it('should handle DELETE for invitations', (done) => {
265289
const ctx = buildContext({
266290
method: 'DELETE',

‎apps/api/src/admin-organizations/admin-audit-log.interceptor.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import {
88
import { AuditLogEntityType, db, Prisma } from '@db';
99
import { Reflector } from '@nestjs/core';
1010
import { Observable, tap } from 'rxjs';
11-
import { MUTATION_METHODS, SENSITIVE_KEYS } from '../audit/audit-log.constants';
11+
import {
12+
MUTATION_METHODS,
13+
SENSITIVE_KEYS,
14+
SENSITIVE_KEY_PATTERN,
15+
} from '../audit/audit-log.constants';
1216
import { SKIP_ADMIN_AUDIT_LOG_KEY } from './skip-admin-audit-log.decorator';
1317

1418
const SEGMENT_TO_RESOURCE: Record<
@@ -261,7 +265,15 @@ export class AdminAuditLogInterceptor implements NestInterceptor {
261265
const changes: Changes = {};
262266

263267
for (const [key, value] of Object.entries(body)) {
264-
if (value === undefined || SENSITIVE_KEYS.has(key)) continue;
268+
// Skip exact-match sensitive keys AND credential-shaped names the exact
269+
// list misses (clientSecret, secretAccessKey, …) — shared with the global
270+
// interceptor so both audit paths redact consistently.
271+
if (
272+
value === undefined ||
273+
SENSITIVE_KEYS.has(key) ||
274+
SENSITIVE_KEY_PATTERN.test(key)
275+
)
276+
continue;
265277
changes[key] = { previous: null, current: value };
266278
}
267279

‎apps/api/src/audit/audit-log.constants.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ export const SENSITIVE_KEYS = new Set([
2424
'totpCode',
2525
]);
2626

27+
/**
28+
* Fallback pattern for credential-ish field names the exact-match set misses
29+
* (e.g. `clientSecret`, `secretAccessKey`, `aws_secret_access_key`). Matched
30+
* case-insensitively against key names anywhere in the audited body, so new
31+
* credential fields are redacted without having to enumerate every name.
32+
*/
33+
export const SENSITIVE_KEY_PATTERN =
34+
/secret|password|passphrase|credential|token|api[_-]?key|private[_-]?key|totp|access[_-]?key/i;
35+
36+
/**
37+
* Resources whose request body must never be diffed into the audit log at all —
38+
* the field carrying the secret is generically named (e.g. the secret manager's
39+
* `value`) so key-based redaction can't catch it, and reading audit logs needs
40+
* only `app:read`. For these we log the action (Created/Updated/Deleted) with no
41+
* payload, keeping plaintext out of a store that bypasses `secret:read`.
42+
*/
43+
export const REDACT_BODY_RESOURCES = new Set(['secret']);
44+
2745
export const RESOURCE_TO_ENTITY_TYPE: Record<
2846
string,
2947
AuditLogEntityType | null

‎apps/api/src/audit/audit-log.interceptor.spec.ts‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,107 @@ describe('AuditLogInterceptor', () => {
381381
});
382382
});
383383

384+
it('should skip read endpoints that use a mutation verb (POST with read-only permission)', (done) => {
385+
// e.g. POST /v1/trust-portal/documents/list — a list/status read that uses
386+
// POST to carry a filter body. It declares `read`, so it must not be logged
387+
// as "Created trust".
388+
jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => {
389+
if (key === PERMISSIONS_KEY) {
390+
return [{ resource: 'trust', actions: ['read'] }];
391+
}
392+
if (key === SKIP_AUDIT_LOG_KEY) return false;
393+
return undefined;
394+
});
395+
396+
const context = createMockExecutionContext({
397+
method: 'POST',
398+
url: '/v1/trust-portal/documents/list',
399+
params: {},
400+
body: { organizationId: 'org_123' },
401+
});
402+
const handler = createMockCallHandler([]);
403+
404+
interceptor.intercept(context, handler).subscribe({
405+
next: () => {
406+
setTimeout(() => {
407+
expect(mockCreate).not.toHaveBeenCalled();
408+
done();
409+
}, 50);
410+
},
411+
});
412+
});
413+
414+
it('still logs when only ONE of several declared permissions is read-only', (done) => {
415+
// A POST that declares [read, create] is a real mutation — the read
416+
// requirement must not suppress the audit entry.
417+
jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => {
418+
if (key === PERMISSIONS_KEY) {
419+
return [
420+
{ resource: 'trust', actions: ['read'] },
421+
{ resource: 'policy', actions: ['create'] },
422+
];
423+
}
424+
if (key === SKIP_AUDIT_LOG_KEY) return false;
425+
return undefined;
426+
});
427+
428+
const context = createMockExecutionContext({
429+
method: 'POST',
430+
url: '/v1/something',
431+
params: {},
432+
body: { organizationId: 'org_123' },
433+
});
434+
const handler = createMockCallHandler({ id: 'ent_new' });
435+
436+
interceptor.intercept(context, handler).subscribe({
437+
next: () => {
438+
setTimeout(() => {
439+
expect(mockCreate).toHaveBeenCalled();
440+
done();
441+
}, 50);
442+
},
443+
});
444+
});
445+
446+
it('logs the action but never the payload for the secret resource', (done) => {
447+
// Reading audit logs needs only app:read; the secret manager's plaintext
448+
// `value` must not be diffed into the log where an auditor (no secret:read)
449+
// could read it.
450+
jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => {
451+
if (key === PERMISSIONS_KEY) {
452+
return [{ resource: 'secret', actions: ['create'] }];
453+
}
454+
if (key === SKIP_AUDIT_LOG_KEY) return false;
455+
return undefined;
456+
});
457+
458+
const context = createMockExecutionContext({
459+
method: 'POST',
460+
url: '/v1/secrets',
461+
params: {},
462+
body: { name: 'STRIPE_KEY', value: 'sk_live_super_secret' },
463+
});
464+
const handler = createMockCallHandler({ id: 'sec_1' });
465+
466+
interceptor.intercept(context, handler).subscribe({
467+
next: () => {
468+
setTimeout(() => {
469+
expect(mockCreate).toHaveBeenCalled();
470+
// The action is recorded...
471+
expect(mockCreate).toHaveBeenCalledWith(
472+
expect.objectContaining({
473+
data: expect.objectContaining({ description: 'Created secret' }),
474+
}),
475+
);
476+
// ...but the plaintext value never appears anywhere in the row.
477+
const persisted = JSON.stringify(mockCreate.mock.calls[0][0]);
478+
expect(persisted).not.toContain('sk_live_super_secret');
479+
done();
480+
}, 50);
481+
},
482+
});
483+
});
484+
384485
it('should skip requests without userId', (done) => {
385486
jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => {
386487
if (key === PERMISSIONS_KEY) {

‎apps/api/src/audit/audit-log.interceptor.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { AUDIT_READ_KEY, SKIP_AUDIT_LOG_KEY } from './skip-audit-log.decorator';
1515
import {
1616
MEMBER_REF_FIELDS,
1717
MUTATION_METHODS,
18+
REDACT_BODY_RESOURCES,
1819
RESOURCE_TO_ENTITY_TYPE,
1920
} from './audit-log.constants';
2021
import {
@@ -76,6 +77,24 @@ export class AuditLogInterceptor implements NestInterceptor {
7677
}
7778

7879
const { resource, actions } = requiredPermissions[0];
80+
81+
// Read-only endpoints that use a mutation HTTP verb (e.g. `POST .../list` or
82+
// `POST .../status` that carry a filter body) declare exactly `['read']`.
83+
// The method-derived verb below would log them as "Created X" on every page
84+
// load, so skip them — a required permission of only `read` is definitionally
85+
// not a mutation. `@AuditRead` opts a read endpoint back into logging.
86+
// Only skip when EVERY declared permission is read-only: an endpoint with
87+
// multiple requirements (e.g. [read, create]) still performs a mutation.
88+
if (
89+
!isAuditRead &&
90+
requiredPermissions.every(
91+
(permission) =>
92+
permission.actions.length === 1 && permission.actions[0] === 'read',
93+
)
94+
) {
95+
return next.handle();
96+
}
97+
7998
// Derive the actual action from the HTTP method rather than using the first
8099
// permission action. This is important when a controller declares multiple
81100
// actions (e.g. ['create','read','update','delete']) at the class level.
@@ -250,6 +269,12 @@ export class AuditLogInterceptor implements NestInterceptor {
250269
} else if (relationMappingResult) {
251270
changes = relationMappingResult.changes;
252271
descriptionOverride ??= relationMappingResult.description;
272+
} else if (REDACT_BODY_RESOURCES.has(resource)) {
273+
// Credential resources (e.g. the secret manager) carry their
274+
// secret in a generically-named field. Diffing the body would
275+
// land plaintext in a store readable with only app:read, which
276+
// bypasses <resource>:read. Record the action, not the payload.
277+
changes = null;
253278
} else {
254279
changes = requestBody
255280
? buildChanges(requestBody, previousValues, memberNames)
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
// buildChanges → constants pull Prisma enums at module load; stub @db so the
2+
// pure redaction logic can be tested without a real client.
3+
jest.mock('@db', () => ({
4+
db: {},
5+
AuditLogEntityType: new Proxy({}, { get: (_t, p) => p }),
6+
CommentEntityType: new Proxy({}, { get: (_t, p) => p }),
7+
}));
8+
9+
import { buildChanges } from './audit-log.utils';
10+
11+
describe('buildChanges — redaction', () => {
12+
it('redacts the existing exact-match sensitive keys', () => {
13+
const changes = buildChanges(
14+
{ password: 'p', apiKey: 'k', name: 'ok' },
15+
null,
16+
{},
17+
);
18+
expect(changes?.password.current).toBe('[REDACTED]');
19+
expect(changes?.apiKey.current).toBe('[REDACTED]');
20+
expect(changes?.name.current).toBe('ok');
21+
});
22+
23+
it('redacts credential-named keys the exact list misses (clientSecret, secretAccessKey)', () => {
24+
const changes = buildChanges(
25+
{
26+
clientSecret: 'abc',
27+
secretAccessKey: 'xyz',
28+
aws_secret_access_key: 'q',
29+
name: 'ok',
30+
},
31+
null,
32+
{},
33+
);
34+
expect(changes?.clientSecret.current).toBe('[REDACTED]');
35+
expect(changes?.secretAccessKey.current).toBe('[REDACTED]');
36+
expect(changes?.aws_secret_access_key.current).toBe('[REDACTED]');
37+
expect(changes?.name.current).toBe('ok');
38+
});
39+
40+
it('summarizes object elements inside arrays so secrets in generic fields cannot leak', () => {
41+
// e.g. browserbase `extraFields: [{ label, value }]` — `value` is a secret
42+
// under a non-credential key; the whole element is hidden as [Object].
43+
const changes = buildChanges(
44+
{ extraFields: [{ label: 'workspace', value: 'sekret' }] },
45+
null,
46+
{},
47+
);
48+
expect(changes?.extraFields.current).toEqual(['[Object]']);
49+
});
50+
51+
it('keeps primitive array elements (ids, scopes, tags) visible', () => {
52+
const changes = buildChanges({ scopes: ['read', 'write'] }, null, {});
53+
expect(changes?.scopes.current).toEqual(['read', 'write']);
54+
});
55+
56+
it('keeps summarizing nested plain objects as [Object]', () => {
57+
const changes = buildChanges({ config: { a: 1, token: 't' } }, null, {});
58+
expect(changes?.config.current).toBe('[Object]');
59+
});
60+
61+
it('leaves ordinary values untouched', () => {
62+
const changes = buildChanges({ status: 'active', count: 3 }, null, {});
63+
expect(changes?.status.current).toBe('active');
64+
expect(changes?.count.current).toBe(3);
65+
});
66+
});

‎apps/api/src/audit/audit-log.utils.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
COMMENT_ENTITY_TYPE_MAP,
55
MEMBER_REF_FIELDS,
66
SENSITIVE_KEYS,
7+
SENSITIVE_KEY_PATTERN,
78
} from './audit-log.constants';
89

910
export type AuditContextOverride = {
@@ -328,14 +329,29 @@ export function buildDescription(
328329
}
329330
}
330331

332+
function isSensitiveKey(key: string): boolean {
333+
return SENSITIVE_KEYS.has(key) || SENSITIVE_KEY_PATTERN.test(key);
334+
}
335+
331336
function sanitizeValue(key: string, value: unknown): unknown {
332-
if (SENSITIVE_KEYS.has(key)) return '[REDACTED]';
337+
if (isSensitiveKey(key)) return '[REDACTED]';
333338
if (value instanceof Date) return value.toISOString();
334-
if (value && typeof value === 'object' && !Array.isArray(value))
335-
return '[Object]';
339+
// Arrays are logged rather than summarized, so a secret in a generically-named
340+
// field inside an array element (e.g. `extraFields: [{ label, value }]`) would
341+
// otherwise land in the log verbatim. Keep primitive elements (ids, scopes,
342+
// tags) but summarize object/array elements as '[Object]' — the same way a
343+
// nested object is hidden below.
344+
if (Array.isArray(value)) return value.map(summarizeArrayItem);
345+
if (value && typeof value === 'object') return '[Object]';
336346
return value;
337347
}
338348

349+
function summarizeArrayItem(item: unknown): unknown {
350+
if (item instanceof Date) return item.toISOString();
351+
if (item && typeof item === 'object') return '[Object]';
352+
return item;
353+
}
354+
339355
export function buildChanges(
340356
body: Record<string, unknown>,
341357
previousValues: Record<string, unknown> | null,

0 commit comments

Comments
 (0)