Skip to content

Commit 55a48fb

Browse files
Marfuenclaude
andauthored
fix(audit): stop plaintext secrets leaking into the audit log (#3512)
* fix(audit): stop plaintext secrets/credentials leaking into the audit log Audit rows are readable with app:read (e.g. the auditor role), but the interceptor diffed request bodies into AuditLog.data with only exact-match, top-level key redaction — so credentials landed in cleartext where roles denied the resource's own read permission could see them. Confirmed leak: POST/PUT /v1/secrets stored the plaintext `value` (key not in the denylist), readable by an auditor who is intentionally denied secret:read. Defense in depth, all layers: - Per-resource body skip (REDACT_BODY_RESOURCES = {secret}): log the action, not the payload, for credential resources whose secret rides in a generic field. - Regex key matching: redact clientSecret, secretAccessKey, accessKeyId, etc. that the exact-match SENSITIVE_KEYS set missed. - Summarize object elements inside arrays as [Object] (as nested objects already are), so secrets in generic array fields (browserbase extraFields[].value, email attachments[].content) can't be logged verbatim. - @SkipAuditLog the internal mailer (html/attachments carry OTP/magic-links). Tests: new audit-log.utils.spec (regex, arrays, nested); interceptor asserts a secret mutation logs the action but never the plaintext value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(audit): apply credential regex in the admin audit interceptor too Addresses cubic review: AdminAuditLogInterceptor.sanitizeBody only skipped exact-match SENSITIVE_KEYS, so credential-shaped fields (clientSecret, secretAccessKey, …) could still leak through the platform-admin audit path. Reuse the shared SENSITIVE_KEY_PATTERN so both interceptors redact consistently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent d5ac0cb commit 55a48fb

8 files changed

Lines changed: 192 additions & 5 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: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,45 @@ describe('AuditLogInterceptor', () => {
443443
});
444444
});
445445

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+
446485
it('should skip requests without userId', (done) => {
447486
jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => {
448487
if (key === PERMISSIONS_KEY) {

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

Lines changed: 7 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 {
@@ -268,6 +269,12 @@ export class AuditLogInterceptor implements NestInterceptor {
268269
} else if (relationMappingResult) {
269270
changes = relationMappingResult.changes;
270271
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;
271278
} else {
272279
changes = requestBody
273280
? 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,

‎apps/api/src/email/email.controller.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { tasks } from '@trigger.dev/sdk';
1010
import { HybridAuthGuard } from '../auth/hybrid-auth.guard';
1111
import { PermissionGuard } from '../auth/permission.guard';
1212
import { RequirePermission } from '../auth/require-permission.decorator';
13+
import { SkipAuditLog } from '../audit/skip-audit-log.decorator';
1314
import { SendEmailDto } from './dto/send-email.dto';
1415
import { SendBatchEmailDto } from './dto/send-batch-email.dto';
1516
import type { sendEmailTask } from '../trigger/email/send-email';
@@ -24,6 +25,9 @@ export class EmailController {
2425
@Post('send')
2526
@HttpCode(200)
2627
@RequirePermission('email', 'send')
28+
// The body carries rendered `html` (magic-links / OTP) and full attachment
29+
// bytes — never worth diffing into the audit log, and readable with app:read.
30+
@SkipAuditLog()
2731
@ApiOperation({
2832
summary: 'Send an email via the centralized Trigger task (internal)',
2933
})
@@ -46,6 +50,7 @@ export class EmailController {
4650
@Post('send-batch')
4751
@HttpCode(200)
4852
@RequirePermission('email', 'send')
53+
@SkipAuditLog()
4954
@ApiOperation({
5055
summary: 'Send a batch of emails via the centralized Trigger task (internal)',
5156
})

0 commit comments

Comments
 (0)