From 3f9e1cfe69e71c1611a4f1a4a47a4f792ee7071e Mon Sep 17 00:00:00 2001 From: Mariano Fuentes Date: Mon, 27 Jul 2026 12:25:34 -0400 Subject: [PATCH 1/6] fix(audit): stop logging read endpoints as mutations + name task-item events (#3508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(audit): stop logging read endpoints as mutations + name task-item events Two audit-trail hygiene fixes: - The global AuditLogInterceptor derived the verb purely from the HTTP method, so read endpoints that use POST to carry a body (e.g. `POST .../list`, `POST .../status`) were logged as "Created X" on every page load — spamming the trail with false "Created trust"/"Created integration" entries. Skip logging when the endpoint's declared permission is read-only (`['read']`); `@AuditRead` still opts a read endpoint back in. - Task-item audit descriptions said "created this task" / "deleted this task" — wording written for a single task's own timeline that reads ambiguously in the org-wide feed. Name the task instead: `Created task ""` / `Deleted task "<title>"`, across the audit service and the vendor risk-assessment flow that emits the same events. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(audit): only skip audit when ALL declared permissions are read-only Addresses cubic review: the read-only skip checked only the first @RequirePermission, so a POST declaring multiple requirements (e.g. [read, create]) would drop its mutation audit entry. Skip only when every declared permission is exclusively `read`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --- .../src/audit/audit-log.interceptor.spec.ts | 62 +++++++++++++++++ apps/api/src/audit/audit-log.interceptor.ts | 18 +++++ .../task-item-audit.service.spec.ts | 68 +++++++++++++++++++ .../task-item-audit.service.ts | 10 +-- .../vendor/vendor-risk-assessment-task.ts | 4 +- apps/api/src/vendors/vendors.service.ts | 2 +- 6 files changed, 157 insertions(+), 7 deletions(-) create mode 100644 apps/api/src/task-management/task-item-audit.service.spec.ts diff --git a/apps/api/src/audit/audit-log.interceptor.spec.ts b/apps/api/src/audit/audit-log.interceptor.spec.ts index d33fa1fcb3..6a29fcdcc8 100644 --- a/apps/api/src/audit/audit-log.interceptor.spec.ts +++ b/apps/api/src/audit/audit-log.interceptor.spec.ts @@ -381,6 +381,68 @@ describe('AuditLogInterceptor', () => { }); }); + it('should skip read endpoints that use a mutation verb (POST with read-only permission)', (done) => { + // e.g. POST /v1/trust-portal/documents/list — a list/status read that uses + // POST to carry a filter body. It declares `read`, so it must not be logged + // as "Created trust". + jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => { + if (key === PERMISSIONS_KEY) { + return [{ resource: 'trust', actions: ['read'] }]; + } + if (key === SKIP_AUDIT_LOG_KEY) return false; + return undefined; + }); + + const context = createMockExecutionContext({ + method: 'POST', + url: '/v1/trust-portal/documents/list', + params: {}, + body: { organizationId: 'org_123' }, + }); + const handler = createMockCallHandler([]); + + interceptor.intercept(context, handler).subscribe({ + next: () => { + setTimeout(() => { + expect(mockCreate).not.toHaveBeenCalled(); + done(); + }, 50); + }, + }); + }); + + it('still logs when only ONE of several declared permissions is read-only', (done) => { + // A POST that declares [read, create] is a real mutation — the read + // requirement must not suppress the audit entry. + jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => { + if (key === PERMISSIONS_KEY) { + return [ + { resource: 'trust', actions: ['read'] }, + { resource: 'policy', actions: ['create'] }, + ]; + } + if (key === SKIP_AUDIT_LOG_KEY) return false; + return undefined; + }); + + const context = createMockExecutionContext({ + method: 'POST', + url: '/v1/something', + params: {}, + body: { organizationId: 'org_123' }, + }); + const handler = createMockCallHandler({ id: 'ent_new' }); + + interceptor.intercept(context, handler).subscribe({ + next: () => { + setTimeout(() => { + expect(mockCreate).toHaveBeenCalled(); + done(); + }, 50); + }, + }); + }); + it('should skip requests without userId', (done) => { jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => { if (key === PERMISSIONS_KEY) { diff --git a/apps/api/src/audit/audit-log.interceptor.ts b/apps/api/src/audit/audit-log.interceptor.ts index 4e954cb244..06bf722fac 100644 --- a/apps/api/src/audit/audit-log.interceptor.ts +++ b/apps/api/src/audit/audit-log.interceptor.ts @@ -76,6 +76,24 @@ export class AuditLogInterceptor implements NestInterceptor { } const { resource, actions } = requiredPermissions[0]; + + // Read-only endpoints that use a mutation HTTP verb (e.g. `POST .../list` or + // `POST .../status` that carry a filter body) declare exactly `['read']`. + // The method-derived verb below would log them as "Created X" on every page + // load, so skip them — a required permission of only `read` is definitionally + // not a mutation. `@AuditRead` opts a read endpoint back into logging. + // Only skip when EVERY declared permission is read-only: an endpoint with + // multiple requirements (e.g. [read, create]) still performs a mutation. + if ( + !isAuditRead && + requiredPermissions.every( + (permission) => + permission.actions.length === 1 && permission.actions[0] === 'read', + ) + ) { + return next.handle(); + } + // Derive the actual action from the HTTP method rather than using the first // permission action. This is important when a controller declares multiple // actions (e.g. ['create','read','update','delete']) at the class level. diff --git a/apps/api/src/task-management/task-item-audit.service.spec.ts b/apps/api/src/task-management/task-item-audit.service.spec.ts new file mode 100644 index 0000000000..c862d8d436 --- /dev/null +++ b/apps/api/src/task-management/task-item-audit.service.spec.ts @@ -0,0 +1,68 @@ +const mockCreate = jest.fn(); +jest.mock('@db', () => ({ + db: { auditLog: { create: (...args: unknown[]) => mockCreate(...args) } }, +})); + +import { TaskItemAuditService } from './task-item-audit.service'; + +describe('TaskItemAuditService', () => { + let service: TaskItemAuditService; + + const base = { + taskItemId: 'ti_1', + organizationId: 'org_1', + userId: 'usr_1', + memberId: 'mem_1', + taskTitle: 'Review Vendor X', + entityType: 'vendor', + entityId: 'vnd_1', + }; + + beforeEach(() => { + jest.clearAllMocks(); + mockCreate.mockResolvedValue({}); + service = new TaskItemAuditService(); + }); + + describe('logTaskItemCreated', () => { + it('names the task in the description (readable in the global feed)', async () => { + await service.logTaskItemCreated(base); + + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + entityType: 'task', + entityId: 'ti_1', + description: 'Created task "Review Vendor X"', + }), + }), + ); + }); + + it('marks API-key creation', async () => { + await service.logTaskItemCreated({ ...base, viaApiKey: true }); + + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + description: 'Created task "Review Vendor X" (via API key)', + }), + }), + ); + }); + }); + + describe('logTaskItemDeleted', () => { + it('names the task in the description', async () => { + await service.logTaskItemDeleted(base); + + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + description: 'Deleted task "Review Vendor X"', + }), + }), + ); + }); + }); +}); diff --git a/apps/api/src/task-management/task-item-audit.service.ts b/apps/api/src/task-management/task-item-audit.service.ts index 4f75aeb0a5..9f1f2f9856 100644 --- a/apps/api/src/task-management/task-item-audit.service.ts +++ b/apps/api/src/task-management/task-item-audit.service.ts @@ -26,9 +26,11 @@ export class TaskItemAuditService { memberId: params.memberId, entityType: 'task', entityId: params.taskItemId, + // Name the task so the entry is unambiguous in the org-wide activity + // feed (not just a single task's own timeline). description: params.viaApiKey - ? 'created this task (via API key)' - : 'created this task', + ? `Created task "${params.taskTitle}" (via API key)` + : `Created task "${params.taskTitle}"`, data: { action: 'created', taskItemId: params.taskItemId, @@ -115,8 +117,8 @@ export class TaskItemAuditService { entityType: 'task', entityId: params.taskItemId, description: params.viaApiKey - ? 'deleted this task (via API key)' - : 'deleted this task', + ? `Deleted task "${params.taskTitle}" (via API key)` + : `Deleted task "${params.taskTitle}"`, data: { action: 'deleted', taskItemId: params.taskItemId, diff --git a/apps/api/src/trigger/vendor/vendor-risk-assessment-task.ts b/apps/api/src/trigger/vendor/vendor-risk-assessment-task.ts index fd664a6e54..413c3a06d8 100644 --- a/apps/api/src/trigger/vendor/vendor-risk-assessment-task.ts +++ b/apps/api/src/trigger/vendor/vendor-risk-assessment-task.ts @@ -541,7 +541,7 @@ export const vendorRiskAssessmentTask: Task< memberId: creatorMember.id, entityType: 'task', entityId: verifyTaskItemId, - description: 'created this task', + description: `Created task "${VERIFY_RISK_ASSESSMENT_TASK_TITLE}"`, data: { action: 'created', taskItemId: verifyTaskItemId, @@ -651,7 +651,7 @@ export const vendorRiskAssessmentTask: Task< memberId: creatorMemberId, entityType: 'task', entityId: verifyTaskItemId, - description: 'created this task', + description: `Created task "${VERIFY_RISK_ASSESSMENT_TASK_TITLE}"`, data: { action: 'created', taskItemId: verifyTaskItemId, diff --git a/apps/api/src/vendors/vendors.service.ts b/apps/api/src/vendors/vendors.service.ts index cdf39e5cab..4b2df09af0 100644 --- a/apps/api/src/vendors/vendors.service.ts +++ b/apps/api/src/vendors/vendors.service.ts @@ -471,7 +471,7 @@ export class VendorsService { memberId: creatorMember.id, entityType: 'task', entityId: created.id, - description: 'created this task', + description: `Created task "${VERIFY_RISK_ASSESSMENT_TASK_TITLE}"`, data: { action: 'created', taskItemId: created.id, From 99264081bcb5e9389b79d05f1c733fa23a193a56 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:52:38 -0400 Subject: [PATCH 2/6] fix(app): make invite modal manual rows scrollable so action buttons stay visible (#3510) Co-authored-by: chasprowebdev <chasgarciaprowebdev@gmail.com> Co-authored-by: chasprowebdev <70908289+chasprowebdev@users.noreply.github.com> --- .../all/components/InviteMembersModal.tsx | 100 +++++++++--------- 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/apps/app/src/app/(app)/[orgId]/people/all/components/InviteMembersModal.tsx b/apps/app/src/app/(app)/[orgId]/people/all/components/InviteMembersModal.tsx index 1365a51cfd..de8110de48 100644 --- a/apps/app/src/app/(app)/[orgId]/people/all/components/InviteMembersModal.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/all/components/InviteMembersModal.tsx @@ -455,56 +455,58 @@ export function InviteMembersModal({ </TabsList> <TabsContent value="manual" className="space-y-4 pt-4"> - {fields.map((item, index) => ( - <div key={item.id} className="flex items-start gap-2"> - <FormField - control={form.control} - name={`manualInvites.${index}.email`} - render={({ field }) => ( - <FormItem className="flex-1"> - {index === 0 && <FormLabel>{'Email'}</FormLabel>} - <FormControl> - <Input - className="h-10" - placeholder={'Enter email address'} - {...field} - value={field.value || ''} + <div className="max-h-[280px] space-y-3 overflow-y-auto px-1"> + {fields.map((item, index) => ( + <div key={item.id} className="flex items-start gap-2"> + <FormField + control={form.control} + name={`manualInvites.${index}.email`} + render={({ field }) => ( + <FormItem className="flex-1"> + {index === 0 && <FormLabel>{'Email'}</FormLabel>} + <FormControl> + <Input + className="h-10" + placeholder={'Enter email address'} + {...field} + value={field.value || ''} + /> + </FormControl> + <FormMessage /> + </FormItem> + )} + /> + <Controller + control={form.control} + name={`manualInvites.${index}.roles`} + render={({ field: { onChange, value }, fieldState: { error } }) => ( + <FormItem className="w-[200px]"> + {index === 0 && <FormLabel>{'Role'}</FormLabel>} + <MultiRoleCombobox + selectedRoles={value || []} + onSelectedRolesChange={onChange} + allowedRoles={normalizedAllowedRoles} + customRoles={customRoles} + placeholder={'Select a role'} /> - </FormControl> - <FormMessage /> - </FormItem> - )} - /> - <Controller - control={form.control} - name={`manualInvites.${index}.roles`} - render={({ field: { onChange, value }, fieldState: { error } }) => ( - <FormItem className="w-[200px]"> - {index === 0 && <FormLabel>{'Role'}</FormLabel>} - <MultiRoleCombobox - selectedRoles={value || []} - onSelectedRolesChange={onChange} - allowedRoles={normalizedAllowedRoles} - customRoles={customRoles} - placeholder={'Select a role'} - /> - <FormMessage>{error?.message}</FormMessage> - </FormItem> - )} - /> - <Button - type="button" - variant="ghost" - size="icon" - onClick={() => fields.length > 1 && remove(index)} - disabled={fields.length <= 1} - className={`mt-${index === 0 ? '6' : '0'} self-center ${fields.length <= 1 ? 'cursor-not-allowed opacity-50' : ''}`} - aria-label="Remove invite" - > - <Trash2 className="h-4 w-4" /> - </Button> - </div> - ))} + <FormMessage>{error?.message}</FormMessage> + </FormItem> + )} + /> + <Button + type="button" + variant="ghost" + size="icon" + onClick={() => fields.length > 1 && remove(index)} + disabled={fields.length <= 1} + className={`mt-${index === 0 ? '6' : '0'} self-center ${fields.length <= 1 ? 'cursor-not-allowed opacity-50' : ''}`} + aria-label="Remove invite" + > + <Trash2 className="h-4 w-4" /> + </Button> + </div> + ))} + </div> <Button type="button" variant="outline" From 258ce4570ec6b949ff29746310cdbf6d0b90181b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:56:16 -0400 Subject: [PATCH 3/6] feat(api): email org owners/admins when a portal access request is submitted (CS-522) (#3494) * feat(api): email org owners/admins when a portal access request is submitted * fix(api): fix stale isActive filter assertion in notifier spec --------- Co-authored-by: chasprowebdev <chasgarciaprowebdev@gmail.com> Co-authored-by: chasprowebdev <70908289+chasprowebdev@users.noreply.github.com> Co-authored-by: Tofik Hasanov <72318342+tofikwest@users.noreply.github.com> --- .../evidence-access-request-submitted.tsx | 140 ++++++++++++ .../evidence-forms-notifier.service.spec.ts | 201 ++++++++++++++++++ .../evidence-forms-notifier.service.ts | 157 ++++++++++++++ .../evidence-forms/evidence-forms.module.ts | 3 +- .../evidence-forms.service.spec.ts | 84 +++++++- .../evidence-forms/evidence-forms.service.ts | 19 ++ 6 files changed, 602 insertions(+), 2 deletions(-) create mode 100644 apps/api/src/email/templates/evidence-access-request-submitted.tsx create mode 100644 apps/api/src/evidence-forms/evidence-forms-notifier.service.spec.ts create mode 100644 apps/api/src/evidence-forms/evidence-forms-notifier.service.ts diff --git a/apps/api/src/email/templates/evidence-access-request-submitted.tsx b/apps/api/src/email/templates/evidence-access-request-submitted.tsx new file mode 100644 index 0000000000..d8907be134 --- /dev/null +++ b/apps/api/src/email/templates/evidence-access-request-submitted.tsx @@ -0,0 +1,140 @@ +import * as React from 'react'; +import { + Body, + Button, + Container, + Font, + Heading, + Html, + Link, + Preview, + Section, + Tailwind, + Text, +} from '@react-email/components'; +import { Footer } from '../components/footer'; +import { Logo } from '../components/logo'; +import { getUnsubscribeUrl } from '@trycompai/email'; + +interface Props { + toName: string; + toEmail: string; + organizationName: string; + requesterName: string; + accountsNeeded: string; + permissionsNeeded: string; + reasonForRequest: string; + reviewUrl: string; +} + +export const EvidenceAccessRequestSubmittedEmail = ({ + toName, + toEmail, + organizationName, + requesterName, + accountsNeeded, + permissionsNeeded, + reasonForRequest, + reviewUrl, +}: Props) => { + const unsubscribeUrl = getUnsubscribeUrl(toEmail); + + return ( + <Html> + <Tailwind> + <head> + <Font + fontFamily="Geist" + fallbackFontFamily="Helvetica" + fontWeight={400} + fontStyle="normal" + /> + <Font + fontFamily="Geist" + fallbackFontFamily="Helvetica" + fontWeight={500} + fontStyle="normal" + /> + </head> + <Preview>New access request from {requesterName}</Preview> + + <Body className="mx-auto my-auto bg-[#fff] font-sans"> + <Container + className="mx-auto my-[40px] max-w-[600px] border-transparent p-[20px] md:border-[#E8E7E1]" + style={{ borderStyle: 'solid', borderWidth: 1 }} + > + <Logo /> + <Heading className="mx-0 my-[30px] p-0 text-center text-[24px] font-normal text-[#121212]"> + New Access Request + </Heading> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + Hello {toName}, + </Text> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + <strong>{requesterName}</strong> submitted an access request in{' '} + <strong>{organizationName}</strong>. + </Text> + + <Section + className="mt-[24px] mb-[24px] rounded-[8px] bg-[#f5f5f5] p-[16px]" + style={{ border: '1px solid #e0e0e0' }} + > + <Text className="m-0 text-[12px] font-medium uppercase tracking-wide text-[#666666]"> + Request Details + </Text> + + <Text className="mt-[8px] mb-[4px] text-[14px] font-medium text-[#121212]"> + Accounts Needed: {accountsNeeded} + </Text> + + <Text className="mt-[4px] mb-[4px] text-[14px] font-medium text-[#121212]"> + Permissions Needed: {permissionsNeeded} + </Text> + + <Text className="mt-[12px] mb-0 text-[13px] italic text-[#444444]"> + "{reasonForRequest}" + </Text> + </Section> + + <Section className="mt-[32px] mb-[32px] text-center"> + <Button + className="rounded-[3px] bg-[#121212] px-[20px] py-[12px] text-center text-[14px] font-semibold text-white no-underline" + href={reviewUrl} + > + Review Request + </Button> + </Section> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + or copy and paste this URL into your browser:{' '} + <a href={reviewUrl} className="text-[#121212] underline"> + {reviewUrl} + </a> + </Text> + + <Section className="mt-[30px] mb-[20px]"> + <Text className="text-[12px] leading-[20px] text-[#666666]"> + Don't want to receive access request notifications?{' '} + <Link + href={unsubscribeUrl} + className="text-[#121212] underline" + > + Manage your email preferences + </Link> + . + </Text> + </Section> + + <br /> + + <Footer /> + </Container> + </Body> + </Tailwind> + </Html> + ); +}; + +export default EvidenceAccessRequestSubmittedEmail; diff --git a/apps/api/src/evidence-forms/evidence-forms-notifier.service.spec.ts b/apps/api/src/evidence-forms/evidence-forms-notifier.service.spec.ts new file mode 100644 index 0000000000..10cb01f6d8 --- /dev/null +++ b/apps/api/src/evidence-forms/evidence-forms-notifier.service.spec.ts @@ -0,0 +1,201 @@ +const mockDb = { + organization: { findUnique: jest.fn() }, + member: { findMany: jest.fn() }, +}; + +jest.mock('@db', () => ({ db: mockDb })); + +jest.mock('@trycompai/email', () => ({ + isUserUnsubscribed: jest.fn().mockResolvedValue(false), + getUnsubscribeUrl: jest + .fn() + .mockReturnValue('https://app.trycomp.ai/unsubscribe'), +})); + +jest.mock('../email/trigger-email', () => ({ + triggerEmail: jest.fn().mockResolvedValue({ id: 'email_1' }), +})); + +jest.mock('../email/templates/evidence-access-request-submitted', () => ({ + EvidenceAccessRequestSubmittedEmail: () => null, +})); + +import { isUserUnsubscribed } from '@trycompai/email'; +import { triggerEmail } from '../email/trigger-email'; +import { EvidenceFormsNotifierService } from './evidence-forms-notifier.service'; + +describe('EvidenceFormsNotifierService', () => { + let service: EvidenceFormsNotifierService; + + beforeEach(() => { + jest.clearAllMocks(); + (isUserUnsubscribed as jest.Mock).mockResolvedValue(false); + service = new EvidenceFormsNotifierService(); + }); + + it('emails every active owner/admin, excluding the submitter', async () => { + mockDb.organization.findUnique.mockResolvedValue({ name: 'Acme' }); + mockDb.member.findMany.mockResolvedValue([ + { + role: 'admin', + user: { id: 'usr_admin', email: 'admin@acme.com', name: 'Admin' }, + }, + { + role: 'owner', + user: { id: 'usr_owner', email: 'owner@acme.com', name: 'Owner' }, + }, + { + role: 'employee', + user: { + id: 'usr_submitter', + email: 'submitter@acme.com', + name: 'Submitter', + }, + }, + ]); + + await service.notifyAccessRequestSubmitted({ + organizationId: 'org_1', + submitterUserId: 'usr_submitter', + submitterName: 'Submitter', + submissionId: 'sub_1', + data: { + accountsNeeded: 'GitHub', + permissionsNeeded: 'write', + reasonForRequest: 'New hire', + }, + }); + + expect(triggerEmail).toHaveBeenCalledTimes(2); + const recipients = (triggerEmail as jest.Mock).mock.calls.map( + (call) => call[0].to, + ); + expect(recipients.sort()).toEqual(['admin@acme.com', 'owner@acme.com']); + expect(mockDb.member.findMany).toHaveBeenCalledWith({ + where: { organizationId: 'org_1', deactivated: false, isActive: true }, + select: { + role: true, + user: { select: { id: true, email: true, name: true } }, + }, + }); + }); + + it('does nothing when there are no owner/admin recipients', async () => { + mockDb.member.findMany.mockResolvedValue([ + { + role: 'employee', + user: { + id: 'usr_submitter', + email: 'submitter@acme.com', + name: 'Submitter', + }, + }, + ]); + + await service.notifyAccessRequestSubmitted({ + organizationId: 'org_1', + submitterUserId: 'usr_submitter', + submitterName: 'Submitter', + submissionId: 'sub_1', + data: {}, + }); + + expect(triggerEmail).not.toHaveBeenCalled(); + expect(mockDb.organization.findUnique).not.toHaveBeenCalled(); + }); + + it('excludes deactivated and inactive members from the recipient query', async () => { + mockDb.organization.findUnique.mockResolvedValue({ name: 'Acme' }); + + const allMembers = [ + { + role: 'admin', + deactivated: false, + isActive: true, + user: { id: 'usr_admin', email: 'admin@acme.com', name: 'Admin' }, + }, + { + role: 'owner', + deactivated: false, + isActive: false, + user: { + id: 'usr_inactive_owner', + email: 'inactive-owner@acme.com', + name: 'Inactive Owner', + }, + }, + { + role: 'admin', + deactivated: true, + isActive: true, + user: { + id: 'usr_deactivated_admin', + email: 'deactivated-admin@acme.com', + name: 'Deactivated Admin', + }, + }, + ]; + + mockDb.member.findMany.mockImplementation( + (args: { where: { deactivated: boolean; isActive: boolean } }) => + Promise.resolve( + allMembers.filter( + (m) => + m.deactivated === args.where.deactivated && + m.isActive === args.where.isActive, + ), + ), + ); + + await service.notifyAccessRequestSubmitted({ + organizationId: 'org_1', + submitterUserId: 'usr_other', + submitterName: 'Requester', + submissionId: 'sub_1', + data: {}, + }); + + expect(mockDb.member.findMany).toHaveBeenCalledWith({ + where: { organizationId: 'org_1', deactivated: false, isActive: true }, + select: { + role: true, + user: { select: { id: true, email: true, name: true } }, + }, + }); + expect(triggerEmail).toHaveBeenCalledTimes(1); + expect((triggerEmail as jest.Mock).mock.calls[0][0].to).toBe( + 'admin@acme.com', + ); + }); + + it('excludes admins who unsubscribed from emails', async () => { + mockDb.organization.findUnique.mockResolvedValue({ name: 'Acme' }); + mockDb.member.findMany.mockResolvedValue([ + { + role: 'admin', + user: { id: 'usr_admin', email: 'admin@acme.com', name: 'Admin' }, + }, + { + role: 'owner', + user: { id: 'usr_owner', email: 'owner@acme.com', name: 'Owner' }, + }, + ]); + (isUserUnsubscribed as jest.Mock).mockImplementation( + (_db: unknown, email: string) => + Promise.resolve(email === 'admin@acme.com'), + ); + + await service.notifyAccessRequestSubmitted({ + organizationId: 'org_1', + submitterUserId: 'usr_other', + submitterName: 'Requester', + submissionId: 'sub_1', + data: {}, + }); + + expect(triggerEmail).toHaveBeenCalledTimes(1); + expect((triggerEmail as jest.Mock).mock.calls[0][0].to).toBe( + 'owner@acme.com', + ); + }); +}); diff --git a/apps/api/src/evidence-forms/evidence-forms-notifier.service.ts b/apps/api/src/evidence-forms/evidence-forms-notifier.service.ts new file mode 100644 index 0000000000..bedde398ea --- /dev/null +++ b/apps/api/src/evidence-forms/evidence-forms-notifier.service.ts @@ -0,0 +1,157 @@ +import { db } from '@db'; +import { Injectable, Logger } from '@nestjs/common'; +import { isUserUnsubscribed } from '@trycompai/email'; +import { triggerEmail } from '../email/trigger-email'; +import { EvidenceAccessRequestSubmittedEmail } from '../email/templates/evidence-access-request-submitted'; + +interface Recipient { + userId: string; + email: string; + name: string; +} + +function getAppUrl(): string { + return ( + process.env.NEXT_PUBLIC_APP_URL ?? + process.env.BETTER_AUTH_URL ?? + 'https://app.trycomp.ai' + ); +} + +function stringField(data: Record<string, unknown>, key: string): string { + const value = data[key]; + return typeof value === 'string' ? value : ''; +} + +@Injectable() +export class EvidenceFormsNotifierService { + private readonly logger = new Logger(EvidenceFormsNotifierService.name); + + async notifyAccessRequestSubmitted(params: { + organizationId: string; + submitterUserId: string; + submitterName: string; + submissionId: string; + data: Record<string, unknown>; + }): Promise<void> { + const { + organizationId, + submitterUserId, + submitterName, + submissionId, + data, + } = params; + + const recipients = await this.getOwnersAndAdmins( + organizationId, + submitterUserId, + ); + if (recipients.length === 0) { + this.logger.log( + 'No owner/admin recipients for access request notification', + ); + return; + } + + const organization = await db.organization.findUnique({ + where: { id: organizationId }, + select: { name: true }, + }); + const organizationName = organization?.name ?? 'your organization'; + const reviewUrl = `${getAppUrl()}/${organizationId}/documents/access-request/submissions/${submissionId}`; + + await Promise.allSettled( + recipients.map((recipient) => + this.sendToRecipient({ + recipient, + organizationName, + submitterName, + reviewUrl, + accountsNeeded: stringField(data, 'accountsNeeded'), + permissionsNeeded: stringField(data, 'permissionsNeeded'), + reasonForRequest: stringField(data, 'reasonForRequest'), + }), + ), + ); + } + + private async sendToRecipient(params: { + recipient: Recipient; + organizationName: string; + submitterName: string; + reviewUrl: string; + accountsNeeded: string; + permissionsNeeded: string; + reasonForRequest: string; + }): Promise<void> { + const { recipient, submitterName, organizationName, reviewUrl } = params; + + try { + const isUnsubscribed = await isUserUnsubscribed(db, recipient.email); + if (isUnsubscribed) { + this.logger.log( + `Skipping access request notification: ${recipient.email} unsubscribed`, + ); + return; + } + + await triggerEmail({ + to: recipient.email, + subject: `New access request from ${submitterName}`, + react: EvidenceAccessRequestSubmittedEmail({ + toName: recipient.name, + toEmail: recipient.email, + organizationName, + requesterName: submitterName, + accountsNeeded: params.accountsNeeded, + permissionsNeeded: params.permissionsNeeded, + reasonForRequest: params.reasonForRequest, + reviewUrl, + }), + system: true, + }); + } catch (error) { + this.logger.error( + `Failed to send access request notification to ${recipient.email}:`, + error instanceof Error ? error.message : 'Unknown error', + ); + } + } + + private async getOwnersAndAdmins( + organizationId: string, + excludeUserId: string, + ): Promise<Recipient[]> { + try { + const members = await db.member.findMany({ + where: { organizationId, deactivated: false, isActive: true }, + select: { + role: true, + user: { select: { id: true, email: true, name: true } }, + }, + }); + + const seen = new Set<string>(); + const recipients: Recipient[] = []; + for (const member of members) { + if (!member.role.includes('admin') && !member.role.includes('owner')) { + continue; + } + const { user } = member; + if (user.id === excludeUserId || !user.email || seen.has(user.id)) { + continue; + } + seen.add(user.id); + recipients.push({ + userId: user.id, + email: user.email, + name: user.name || user.email, + }); + } + return recipients; + } catch (error) { + this.logger.error('Failed to resolve owners/admins:', error); + return []; + } + } +} diff --git a/apps/api/src/evidence-forms/evidence-forms.module.ts b/apps/api/src/evidence-forms/evidence-forms.module.ts index a2d69ad34a..fa997430fe 100644 --- a/apps/api/src/evidence-forms/evidence-forms.module.ts +++ b/apps/api/src/evidence-forms/evidence-forms.module.ts @@ -3,12 +3,13 @@ import { AttachmentsModule } from '@/attachments/attachments.module'; import { AuthModule } from '@/auth/auth.module'; import { TimelinesModule } from '../timelines/timelines.module'; import { EvidenceFormsController } from './evidence-forms.controller'; +import { EvidenceFormsNotifierService } from './evidence-forms-notifier.service'; import { EvidenceFormsService } from './evidence-forms.service'; @Module({ imports: [AuthModule, AttachmentsModule, TimelinesModule], controllers: [EvidenceFormsController], - providers: [EvidenceFormsService], + providers: [EvidenceFormsService, EvidenceFormsNotifierService], exports: [EvidenceFormsService], }) export class EvidenceFormsModule {} diff --git a/apps/api/src/evidence-forms/evidence-forms.service.spec.ts b/apps/api/src/evidence-forms/evidence-forms.service.spec.ts index be6c396b1a..9093e6c305 100644 --- a/apps/api/src/evidence-forms/evidence-forms.service.spec.ts +++ b/apps/api/src/evidence-forms/evidence-forms.service.spec.ts @@ -12,7 +12,7 @@ jest.mock( ); jest.mock('../frameworks/frameworks-timeline.helper', () => ({ - checkAutoCompletePhases: jest.fn(), + checkAutoCompletePhases: jest.fn().mockResolvedValue(undefined), })); jest.mock('../timelines/timelines.service', () => ({ @@ -42,6 +42,7 @@ jest.mock('@db', () => { findFirst: jest.fn(), groupBy: jest.fn(), update: jest.fn(), + create: jest.fn(), }, evidenceFormSetting: { findMany: jest.fn(), @@ -56,6 +57,7 @@ type MockDb = { findFirst: jest.Mock; groupBy: jest.Mock; update: jest.Mock; + create: jest.Mock; }; evidenceFormSetting: { findMany: jest.Mock; @@ -82,9 +84,14 @@ describe('EvidenceFormsService', () => { const timelinesServiceMock = {} as unknown as import('../timelines/timelines.service').TimelinesService; + const evidenceFormsNotifierMock = { + notifyAccessRequestSubmitted: jest.fn().mockResolvedValue(undefined), + }; + const service = new EvidenceFormsService( attachmentsServiceMock, timelinesServiceMock, + evidenceFormsNotifierMock as unknown as import('./evidence-forms-notifier.service').EvidenceFormsNotifierService, ); const mockedDb = db as unknown as MockDb; @@ -187,6 +194,81 @@ describe('EvidenceFormsService', () => { }); }); + describe('submitForm', () => { + const accessRequestPayload = { + submissionDate: '2026-01-01T00:00:00.000Z', + userName: 'Jane Employee', + accountsNeeded: 'GitHub (org: engineering)', + permissionsNeeded: 'write', + reasonForRequest: 'Needs write access for a new project', + accessGrantedBy: 'IT Admin', + dateAccessGranted: '2026-01-01', + }; + + it('notifies owners/admins after an access-request submission', async () => { + mockedDb.evidenceSubmission.create.mockResolvedValue({ + id: 'sub_access_1', + formType: 'access_request', + data: accessRequestPayload, + submittedBy: { + id: 'usr_reviewer', + name: 'Jane Employee', + email: 'reviewer@example.com', + }, + }); + + await service.submitForm({ + organizationId: 'org_123', + formType: 'access-request', + payload: accessRequestPayload, + authContext, + }); + + expect( + evidenceFormsNotifierMock.notifyAccessRequestSubmitted, + ).toHaveBeenCalledWith({ + organizationId: 'org_123', + submitterUserId: 'usr_reviewer', + submitterName: 'Jane Employee', + submissionId: 'sub_access_1', + data: accessRequestPayload, + }); + }); + + it('does not notify owners/admins for non access-request submissions', async () => { + const meetingPayload = { + submissionDate: '2026-01-01T00:00:00.000Z', + attendees: 'Board members', + date: '2026-01-01', + meetingMinutes: 'Discussed Q1 roadmap', + meetingMinutesApprovedBy: 'CEO', + approvedDate: '2026-01-02', + }; + + mockedDb.evidenceSubmission.create.mockResolvedValue({ + id: 'sub_meeting_1', + formType: 'meeting', + data: meetingPayload, + submittedBy: { + id: 'usr_reviewer', + name: 'Jane Employee', + email: 'reviewer@example.com', + }, + }); + + await service.submitForm({ + organizationId: 'org_123', + formType: 'meeting', + payload: meetingPayload, + authContext, + }); + + expect( + evidenceFormsNotifierMock.notifyAccessRequestSubmitted, + ).not.toHaveBeenCalled(); + }); + }); + describe('reviewSubmission', () => { it('includes submittedBy and reviewedBy relations on review update', async () => { mockedDb.evidenceSubmission.findFirst.mockResolvedValue({ diff --git a/apps/api/src/evidence-forms/evidence-forms.service.ts b/apps/api/src/evidence-forms/evidence-forms.service.ts index b2ed8afb8c..86050d2c55 100644 --- a/apps/api/src/evidence-forms/evidence-forms.service.ts +++ b/apps/api/src/evidence-forms/evidence-forms.service.ts @@ -23,6 +23,7 @@ import { } from './evidence-forms.definitions'; import { checkAutoCompletePhases } from '../frameworks/frameworks-timeline.helper'; import { TimelinesService } from '../timelines/timelines.service'; +import { EvidenceFormsNotifierService } from './evidence-forms-notifier.service'; const listQuerySchema = z.object({ search: z.string().trim().optional(), @@ -140,6 +141,7 @@ export class EvidenceFormsService { constructor( private readonly attachmentsService: AttachmentsService, private readonly timelinesService: TimelinesService, + private readonly evidenceFormsNotifier: EvidenceFormsNotifierService, ) {} private requireJwtUser(authContext: AuthContext): string { @@ -594,6 +596,23 @@ export class EvidenceFormsService { this.logger.warn('timeline auto-complete check failed', err); }); + if (parsedType.data === 'access-request') { + this.evidenceFormsNotifier + .notifyAccessRequestSubmitted({ + organizationId: params.organizationId, + submitterUserId: params.authContext.userId, + submitterName: + submission.submittedBy?.name ?? + submission.submittedBy?.email ?? + 'A user', + submissionId: submission.id, + data: parsedPayload.data, + }) + .catch((err) => { + this.logger.warn('access request notification failed', err); + }); + } + return submission; } From 2c1118855d326b9e6bd389a07cdcd83bb79b7898 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:05:42 -0400 Subject: [PATCH 4/6] CS-725 [Bug] - Validate comment length against visible text, not raw Tiptap JSON (#3409) * fix(api): validate comment length against visible text, not raw Tiptap JSON * fix(api): only treat content as Tiptap JSON when it has a doc shape * fix(api): reject empty Tiptap documents in comment content validation * fix(api): stop double-counting line breaks inside blockquotes * fix(api): document raw payload maxLength on comment content in OpenAPI schema * fix(api): count Unicode code points to keep the limit aligned with visible text --------- Co-authored-by: chasprowebdev <chasgarciaprowebdev@gmail.com> Co-authored-by: chasprowebdev <70908289+chasprowebdev@users.noreply.github.com> Co-authored-by: Tofik Hasanov <72318342+tofikwest@users.noreply.github.com> --- .../comments/dto/create-comment.dto.spec.ts | 99 ++++++++++++ .../src/comments/dto/create-comment.dto.ts | 15 +- .../src/comments/dto/update-comment.dto.ts | 15 +- .../utils/extract-comment-plain-text.spec.ts | 152 ++++++++++++++++++ .../utils/extract-comment-plain-text.ts | 88 ++++++++++ .../max-comment-text-length.validator.ts | 50 ++++++ 6 files changed, 413 insertions(+), 6 deletions(-) create mode 100644 apps/api/src/comments/dto/create-comment.dto.spec.ts create mode 100644 apps/api/src/comments/utils/extract-comment-plain-text.spec.ts create mode 100644 apps/api/src/comments/utils/extract-comment-plain-text.ts create mode 100644 apps/api/src/comments/validators/max-comment-text-length.validator.ts diff --git a/apps/api/src/comments/dto/create-comment.dto.spec.ts b/apps/api/src/comments/dto/create-comment.dto.spec.ts new file mode 100644 index 0000000000..3b9a775b73 --- /dev/null +++ b/apps/api/src/comments/dto/create-comment.dto.spec.ts @@ -0,0 +1,99 @@ +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; +import { CreateCommentDto } from './create-comment.dto'; + +// create-comment.dto.ts imports the `CommentEntityType` enum from `@db`, +// which eagerly instantiates the Prisma client on import — mock it so this +// spec doesn't need a configured DB connection (mirrors comments.controller.spec.ts). +jest.mock('@db', () => ({ + db: {}, + CommentEntityType: { + task: 'task', + vendor: 'vendor', + risk: 'risk', + policy: 'policy', + finding: 'finding', + }, +})); + +function tiptapDoc(content: unknown[]): string { + return JSON.stringify({ type: 'doc', content }); +} + +function toDto(plain: Record<string, unknown>): CreateCommentDto { + return plainToInstance(CreateCommentDto, plain, { + enableImplicitConversion: true, + }); +} + +const VALID_BASE = { + entityId: 'tsk_abc123', + entityType: 'task', +}; + +describe('CreateCommentDto', () => { + it('accepts a plain-text comment under the limit', async () => { + const dto = toDto({ ...VALID_BASE, content: 'Looks good to me' }); + const errors = await validate(dto); + expect(errors).toHaveLength(0); + }); + + it('rejects a plain-text comment over 2000 visible characters', async () => { + const dto = toDto({ ...VALID_BASE, content: 'x'.repeat(2001) }); + const errors = await validate(dto); + expect(errors.some((e) => e.property === 'content')).toBe(true); + }); + + it('accepts a formatted Tiptap comment whose raw JSON exceeds 2000 chars but whose visible text does not (regression for the reported bug)', async () => { + const words = Array.from({ length: 240 }, (_, i) => ({ + type: 'text', + text: 'word ', + ...(i % 2 === 0 ? { marks: [{ type: 'bold' }] } : {}), + })); + const content = tiptapDoc([{ type: 'paragraph', content: words }]); + expect(content.length).toBeGreaterThan(2000); + + const dto = toDto({ ...VALID_BASE, content }); + const errors = await validate(dto); + expect(errors).toHaveLength(0); + }); + + it('rejects a formatted Tiptap comment whose visible text exceeds 2000 characters', async () => { + const content = tiptapDoc([ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'a'.repeat(2001), + marks: [{ type: 'bold' }], + }, + ], + }, + ]); + + const dto = toDto({ ...VALID_BASE, content }); + const errors = await validate(dto); + expect(errors.some((e) => e.property === 'content')).toBe(true); + }); + + it('rejects an empty comment', async () => { + const dto = toDto({ ...VALID_BASE, content: '' }); + const errors = await validate(dto); + expect(errors.some((e) => e.property === 'content')).toBe(true); + }); + + it('rejects a non-doc JSON payload over the limit instead of treating it as empty (bypass regression)', async () => { + const content = `{"foo": "${'x'.repeat(2001)}"}`; + const dto = toDto({ ...VALID_BASE, content }); + const errors = await validate(dto); + expect(errors.some((e) => e.property === 'content')).toBe(true); + }); + + it('rejects an empty Tiptap document — non-empty JSON string but zero visible text (regression)', async () => { + const content = tiptapDoc([]); + const dto = toDto({ ...VALID_BASE, content }); + const errors = await validate(dto); + expect(errors.some((e) => e.property === 'content')).toBe(true); + }); +}); diff --git a/apps/api/src/comments/dto/create-comment.dto.ts b/apps/api/src/comments/dto/create-comment.dto.ts index f83266ae15..a2edabf69a 100644 --- a/apps/api/src/comments/dto/create-comment.dto.ts +++ b/apps/api/src/comments/dto/create-comment.dto.ts @@ -11,16 +11,25 @@ import { ValidateNested, } from 'class-validator'; import { UploadAttachmentDto } from '../../attachments/upload-attachment.dto'; +import { MaxCommentTextLength } from '../validators/max-comment-text-length.validator'; + +// `content` is serialized Tiptap JSON (or plain text for API callers). The +// 2000-char limit applies to the visible text a user typed, not the raw +// JSON — see MaxCommentTextLength. This raw-string cap only bounds payload +// size against pathologically formatted input. +const RAW_CONTENT_MAX_LENGTH = 50_000; export class CreateCommentDto { @ApiProperty({ - description: 'Content of the comment', + description: + 'Content of the comment (plain text or serialized Tiptap JSON). Limited to 2000 characters of visible text; maxLength bounds the serialized payload size, not the visible text.', example: 'This task needs to be completed by end of week', - maxLength: 2000, + maxLength: RAW_CONTENT_MAX_LENGTH, }) @IsString() @IsNotEmpty() - @MaxLength(2000) + @MaxLength(RAW_CONTENT_MAX_LENGTH) + @MaxCommentTextLength(2000) content: string; @ApiProperty({ diff --git a/apps/api/src/comments/dto/update-comment.dto.ts b/apps/api/src/comments/dto/update-comment.dto.ts index 00b24d3c9e..6215232362 100644 --- a/apps/api/src/comments/dto/update-comment.dto.ts +++ b/apps/api/src/comments/dto/update-comment.dto.ts @@ -1,15 +1,24 @@ import { ApiProperty } from '@nestjs/swagger'; import { IsNotEmpty, IsOptional, IsString, MaxLength } from 'class-validator'; +import { MaxCommentTextLength } from '../validators/max-comment-text-length.validator'; + +// `content` is serialized Tiptap JSON (or plain text for API callers). The +// 2000-char limit applies to the visible text a user typed, not the raw +// JSON — see MaxCommentTextLength. This raw-string cap only bounds payload +// size against pathologically formatted input. +const RAW_CONTENT_MAX_LENGTH = 50_000; export class UpdateCommentDto { @ApiProperty({ - description: 'Updated content of the comment', + description: + 'Updated content of the comment (plain text or serialized Tiptap JSON). Limited to 2000 characters of visible text; maxLength bounds the serialized payload size, not the visible text.', example: 'This task needs to be completed by end of week (updated)', - maxLength: 2000, + maxLength: RAW_CONTENT_MAX_LENGTH, }) @IsString() @IsNotEmpty() - @MaxLength(2000) + @MaxLength(RAW_CONTENT_MAX_LENGTH) + @MaxCommentTextLength(2000) content: string; @ApiProperty({ diff --git a/apps/api/src/comments/utils/extract-comment-plain-text.spec.ts b/apps/api/src/comments/utils/extract-comment-plain-text.spec.ts new file mode 100644 index 0000000000..cb9dd1d9e1 --- /dev/null +++ b/apps/api/src/comments/utils/extract-comment-plain-text.spec.ts @@ -0,0 +1,152 @@ +import { extractCommentPlainText } from './extract-comment-plain-text'; + +function tiptapDoc(content: unknown[]): string { + return JSON.stringify({ type: 'doc', content }); +} + +describe('extractCommentPlainText', () => { + it('returns plain text as-is when content is not JSON', () => { + expect(extractCommentPlainText('Just a plain comment')).toBe( + 'Just a plain comment', + ); + }); + + it('extracts an empty string from an empty Tiptap document', () => { + expect(extractCommentPlainText(tiptapDoc([]))).toBe(''); + }); + + it('returns plain text as-is when it happens to be valid JSON but not a Tiptap doc (bypass regression)', () => { + const longPlainText = 'x'.repeat(3000); + const jsonLookingText = `{"foo": "${longPlainText}"}`; + expect(extractCommentPlainText(jsonLookingText)).toBe(jsonLookingText); + + const jsonArrayLookingText = `["${longPlainText}"]`; + expect(extractCommentPlainText(jsonArrayLookingText)).toBe( + jsonArrayLookingText, + ); + }); + + it('extracts text from a simple paragraph', () => { + const content = tiptapDoc([ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Hello world' }], + }, + ]); + expect(extractCommentPlainText(content)).toBe('Hello world'); + }); + + it('ignores formatting marks — bold text counts the same as plain text', () => { + const content = tiptapDoc([ + { + type: 'paragraph', + content: [ + { type: 'text', text: 'This word is ' }, + { type: 'text', text: 'bold', marks: [{ type: 'bold' }] }, + { type: 'text', text: '.' }, + ], + }, + ]); + expect(extractCommentPlainText(content)).toBe('This word is bold.'); + }); + + it('counts one character per hard break and per paragraph boundary', () => { + const content = tiptapDoc([ + { + type: 'paragraph', + content: [ + { type: 'text', text: 'Line one' }, + { type: 'hardBreak' }, + { type: 'text', text: 'Line two' }, + ], + }, + { + type: 'paragraph', + content: [{ type: 'text', text: 'Second paragraph' }], + }, + ]); + expect(extractCommentPlainText(content)).toBe( + 'Line one\nLine two\nSecond paragraph', + ); + }); + + it('renders a mention as @label', () => { + const content = tiptapDoc([ + { + type: 'paragraph', + content: [ + { type: 'text', text: 'Hey ' }, + { type: 'mention', attrs: { id: 'usr_1', label: 'Jane Doe' } }, + ], + }, + ]); + expect(extractCommentPlainText(content)).toBe('Hey @Jane Doe'); + }); + + it('extracts text from a bullet list', () => { + const content = tiptapDoc([ + { + type: 'bulletList', + content: [ + { + type: 'listItem', + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Item one' }], + }, + ], + }, + { + type: 'listItem', + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Item two' }], + }, + ], + }, + ], + }, + ]); + expect(extractCommentPlainText(content)).toBe('Item one\nItem two'); + }); + + it('does not double-count the line break for a blockquoted paragraph', () => { + const content = tiptapDoc([ + { + type: 'blockquote', + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Quoted line' }], + }, + ], + }, + { + type: 'paragraph', + content: [{ type: 'text', text: 'Next line' }], + }, + ]); + // If blockquote also appended its own newline on top of the paragraph's, + // this would be 'Quoted line\n\nNext line' instead. + expect(extractCommentPlainText(content)).toBe('Quoted line\nNext line'); + }); + + it('matches the reported bug: a ~1,200-char formatted comment exceeds 2000 raw chars but stays under the visible limit', () => { + // Alternating bold/plain 5-char words, like a comment with scattered + // emphasis — each bold run's marks array is pure JSON overhead. + const words = Array.from({ length: 240 }, (_, i) => ({ + type: 'text', + text: 'word ', + ...(i % 2 === 0 ? { marks: [{ type: 'bold' }] } : {}), + })); + const content = tiptapDoc([{ type: 'paragraph', content: words }]); + + // 240 * 5 = 1200 visible characters, well under the 2000 limit. + expect(extractCommentPlainText(content).length).toBe(1200); + // But the raw JSON (what the old @MaxLength(2000) validated) blows past + // it purely from marks/node overhead — this is the bug. + expect(content.length).toBeGreaterThan(2000); + }); +}); diff --git a/apps/api/src/comments/utils/extract-comment-plain-text.ts b/apps/api/src/comments/utils/extract-comment-plain-text.ts new file mode 100644 index 0000000000..438273e5b2 --- /dev/null +++ b/apps/api/src/comments/utils/extract-comment-plain-text.ts @@ -0,0 +1,88 @@ +/** + * Node types whose content represents a separate visual line. A trailing + * newline is appended after their text so line/paragraph breaks the user + * typed count toward the visible length, matching what they see on screen. + * Wrapper types (listItem, tableCell, blockquote, ...) are deliberately + * excluded — their children are typically paragraphs that already + * contribute a newline, and including the wrapper too would double-count + * each line break. + */ +const BLOCK_NODE_TYPES = new Set(['paragraph', 'heading', 'codeBlock']); + +interface TiptapNode { + type?: unknown; + text?: unknown; + attrs?: unknown; + content?: unknown; +} + +function mentionLabel(node: TiptapNode): string { + const attrs = node.attrs as { label?: unknown; id?: unknown } | undefined; + if (typeof attrs?.label === 'string' && attrs.label) return attrs.label; + if (typeof attrs?.id === 'string' && attrs.id) return attrs.id; + return ''; +} + +function nodeToText(node: unknown): string { + if (!node || typeof node !== 'object') return ''; + const n = node as TiptapNode; + + if (n.type === 'text') { + return typeof n.text === 'string' ? n.text : ''; + } + + if (n.type === 'hardBreak') { + return '\n'; + } + + if (n.type === 'mention') { + const label = mentionLabel(n); + return label ? `@${label}` : ''; + } + + if (Array.isArray(n.content)) { + const childText = n.content.map(nodeToText).join(''); + return BLOCK_NODE_TYPES.has(typeof n.type === 'string' ? n.type : '') + ? `${childText}\n` + : childText; + } + + return ''; +} + +function isTiptapDoc(value: unknown): value is TiptapNode { + if (!value || typeof value !== 'object') return false; + const n = value as TiptapNode; + return n.type === 'doc' && Array.isArray(n.content); +} + +/** + * Extracts the visible text a user typed from a comment's stored `content`. + * Comments accept either raw Tiptap/ProseMirror JSON (from the web editor) + * or plain text (from API/MCP callers) — formatting marks, node types, and + * attrs are structural overhead that inflates the raw string but adds no + * visible characters, so length checks must run against this instead of + * `content.length`. + * + * Only parses `content` as Tiptap when it has the expected `{ type: 'doc', + * content: [...] }` shape. A plain-text comment that happens to be valid + * JSON (e.g. `{"foo": "..."}`) would otherwise be walked as a node tree, + * match no known type, and silently extract to `''` — bypassing the length + * check entirely instead of falling back to the raw string. + */ +export function extractCommentPlainText(content: string): string { + if (typeof content !== 'string') return ''; + + let parsed: unknown; + try { + parsed = JSON.parse(content); + } catch { + return content; + } + + if (!isTiptapDoc(parsed)) { + return content; + } + + return nodeToText(parsed).replace(/\n$/, ''); +} diff --git a/apps/api/src/comments/validators/max-comment-text-length.validator.ts b/apps/api/src/comments/validators/max-comment-text-length.validator.ts new file mode 100644 index 0000000000..343b6431bf --- /dev/null +++ b/apps/api/src/comments/validators/max-comment-text-length.validator.ts @@ -0,0 +1,50 @@ +import { + registerDecorator, + ValidationArguments, + ValidationOptions, + ValidatorConstraint, + ValidatorConstraintInterface, +} from 'class-validator'; +import { extractCommentPlainText } from '../utils/extract-comment-plain-text'; + +const DEFAULT_MAX_LENGTH = 2000; + +/** + * Validates comment `content` against the visible text length rather than + * the raw stored string — `content` is serialized Tiptap JSON, so formatting + * (marks, node types, attrs) would otherwise count toward the limit and + * reject short, plainly-visible comments once they include any formatting. + * + * Also rejects zero visible text: `@IsNotEmpty()` only sees the raw string, + * so an empty document (e.g. `{"type":"doc","content":[]}`) is a non-empty + * JSON string that would otherwise sail through as a "valid" empty comment. + */ +@ValidatorConstraint({ name: 'maxCommentTextLength', async: false }) +export class MaxCommentTextLengthConstraint implements ValidatorConstraintInterface { + validate(value: unknown, args: ValidationArguments): boolean { + if (typeof value !== 'string') return false; + const maxLength = (args.constraints[0] as number) ?? DEFAULT_MAX_LENGTH; + const length = [...extractCommentPlainText(value)].length; + return length > 0 && length <= maxLength; + } + + defaultMessage(args: ValidationArguments): string { + const maxLength = (args.constraints[0] as number) ?? DEFAULT_MAX_LENGTH; + return `content must not be empty and must not exceed ${maxLength} characters`; + } +} + +export function MaxCommentTextLength( + maxLength: number = DEFAULT_MAX_LENGTH, + validationOptions?: ValidationOptions, +) { + return function (object: object, propertyName: string) { + registerDecorator({ + target: object.constructor, + propertyName, + options: validationOptions, + constraints: [maxLength], + validator: MaxCommentTextLengthConstraint, + }); + }; +} From d5ac0cb84af50110bc61355984c6a008c0afd1f2 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:25:51 -0400 Subject: [PATCH 5/6] CS-229 [Improvement] - Send notification when DNS record needs to be fixed on the Trust Portal settings (#3247) * fix(api): add daily domain health check scheduled task * fix(api): correct maxDuration and continue batch on per-domain vercel api failure instead of aborting in domain check schedule * fix(api): route domain misconfigured email through trust portal channel * fix(api): remove unused TRUST_PORTAL_PROJECT_ID requirement from vercel config check * fix(api): parallelize domain health checks and email sends * fix(api): isolate per-trust domain health check failures with Promise.allSettled * fix(api): use exact role matching for domain health notifications --------- Co-authored-by: chasprowebdev <chasgarciaprowebdev@gmail.com> Co-authored-by: chasprowebdev <70908289+chasprowebdev@users.noreply.github.com> Co-authored-by: Tofik Hasanov <72318342+tofikwest@users.noreply.github.com> Co-authored-by: Mariano Fuentes <marfuen98@gmail.com> --- .../templates/trust-domain-misconfigured.tsx | 109 ++++++++++ .../check-domain-health-schedule.ts | 196 ++++++++++++++++++ apps/api/src/trust-portal/email.service.ts | 27 +++ 3 files changed, 332 insertions(+) create mode 100644 apps/api/src/email/templates/trust-domain-misconfigured.tsx create mode 100644 apps/api/src/trigger/trust-portal/check-domain-health-schedule.ts diff --git a/apps/api/src/email/templates/trust-domain-misconfigured.tsx b/apps/api/src/email/templates/trust-domain-misconfigured.tsx new file mode 100644 index 0000000000..3ba3806a68 --- /dev/null +++ b/apps/api/src/email/templates/trust-domain-misconfigured.tsx @@ -0,0 +1,109 @@ +import { + Body, + Button, + Container, + Font, + Heading, + Html, + Preview, + Section, + Tailwind, + Text, +} from '@react-email/components'; +import { Footer } from '../components/footer'; +import { Logo } from '../components/logo'; + +interface Props { + toName: string; + organizationName: string; + domain: string; + settingsUrl: string; +} + +export const TrustDomainMisconfiguredEmail = ({ + toName, + organizationName, + domain, + settingsUrl, +}: Props) => { + return ( + <Html> + <Tailwind> + <head> + <Font + fontFamily="Geist" + fallbackFontFamily="Helvetica" + fontWeight={400} + fontStyle="normal" + /> + <Font + fontFamily="Geist" + fallbackFontFamily="Helvetica" + fontWeight={500} + fontStyle="normal" + /> + </head> + <Preview> + Action required: Trust Portal custom domain {domain} is misconfigured + </Preview> + + <Body className="mx-auto my-auto bg-[#fff] font-sans"> + <Container + className="mx-auto my-[40px] max-w-[600px] border-transparent p-[20px] md:border-[#E8E7E1]" + style={{ borderStyle: 'solid', borderWidth: 1 }} + > + <Logo /> + <Heading className="mx-0 my-[30px] p-0 text-center text-[24px] font-normal text-[#121212]"> + Trust Portal Domain Needs Attention + </Heading> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + Hello {toName}, + </Text> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + We detected that the custom domain{' '} + <strong>{domain}</strong> configured for{' '} + <strong>{organizationName}</strong>'s Trust Portal is no longer + resolving correctly. Visitors using this domain may be unable to + access your Trust Portal until the DNS configuration is fixed. + </Text> + + <Section + className="mt-[24px] mb-[24px] rounded-[3px] border-l-4 p-[15px]" + style={{ backgroundColor: '#fff8f0', borderColor: '#f97316' }} + > + <Text className="m-0 text-[14px] leading-[24px] text-[#121212]"> + <strong>What to do:</strong> + <br /> + Visit your Trust Portal settings to review the DNS records and + re-verify your domain. Ensure your CNAME record points to the + correct target and that all required verification records are in + place. + </Text> + </Section> + + <Section className="mt-[32px] mb-[32px] text-center"> + <Button + className="rounded-[3px] bg-[#121212] px-[20px] py-[12px] text-center text-[14px] font-semibold text-white no-underline" + href={settingsUrl} + > + Review Domain Settings + </Button> + </Section> + + <Text className="text-[14px] leading-[24px] text-[#121212]"> + If you need help, please contact our support team. + </Text> + + <br /> + + <Footer /> + </Container> + </Body> + </Tailwind> + </Html> + ); +}; + +export default TrustDomainMisconfiguredEmail; diff --git a/apps/api/src/trigger/trust-portal/check-domain-health-schedule.ts b/apps/api/src/trigger/trust-portal/check-domain-health-schedule.ts new file mode 100644 index 0000000000..a798bd205c --- /dev/null +++ b/apps/api/src/trigger/trust-portal/check-domain-health-schedule.ts @@ -0,0 +1,196 @@ +import { db } from '@db'; +import { logger, schedules } from '@trigger.dev/sdk'; +import { parseRoles } from '../../people/utils/role-authorization'; +import { TrustEmailService } from '../../trust-portal/email.service'; + +const emailService = new TrustEmailService(); + +const NOTIFIABLE_ROLES = ['owner', 'admin']; + +const APP_BASE_URL = + process.env.NEXT_PUBLIC_APP_URL ?? 'https://app.trycomp.ai'; + +/** + * Checks domain config via the Vercel API. Returns null when Vercel is not + * configured on this server (dev/self-host) — callers should skip the check. + */ +async function isDomainMisconfigured( + domain: string, +): Promise<boolean | null> { + const teamId = process.env.VERCEL_TEAM_ID; + const vercelToken = process.env.VERCEL_AUTH_TOKEN; + + if (!teamId || !vercelToken) { + return null; + } + + const url = new URL( + `https://api.vercel.com/v6/domains/${encodeURIComponent(domain)}/config`, + ); + url.searchParams.set('teamId', teamId); + + const res = await fetch(url.toString(), { + headers: { Authorization: `Bearer ${vercelToken}` }, + }); + + if (!res.ok) { + logger.warn(`Vercel config check failed for ${domain}`, { + status: res.status, + }); + return null; + } + + const data = (await res.json()) as { misconfigured?: boolean }; + return data.misconfigured === true; +} + +/** + * Daily health check for Trust Portal custom domains. + * + * Iterates all orgs with a verified custom domain, re-checks Vercel's + * `misconfigured` flag, and — when a domain is broken — marks it unverified + * in the DB and emails the org's admin/owner members so they can act. + * + * Runs at 6:00 AM UTC daily. + */ +export const checkDomainHealthSchedule = schedules.task({ + id: 'trust-portal-check-domain-health', + cron: '0 6 * * *', + maxDuration: 60 * 15, // 15 minutes + run: async (payload) => { + logger.info('Starting Trust Portal domain health check', { + scheduledAt: payload.timestamp, + }); + + const trusts = await db.trust.findMany({ + where: { + domain: { not: null }, + domainVerified: true, + }, + select: { + organizationId: true, + domain: true, + organization: { + select: { + name: true, + members: { + where: { isActive: true }, + select: { + role: true, + user: { + select: { id: true, name: true, email: true }, + }, + }, + }, + }, + }, + }, + }); + + logger.info(`Found ${trusts.length} trusts with verified custom domains`); + + const vercelConfigured = + !!process.env.VERCEL_TEAM_ID && + !!process.env.VERCEL_AUTH_TOKEN; + + if (!vercelConfigured) { + logger.info( + 'Skipping domain health check — Vercel not configured on this server', + ); + return { checked: 0, misconfigured: 0, notified: 0 }; + } + + const settled = await Promise.allSettled( + trusts.map(async (trust) => { + const domain = trust.domain!; + + const broken = await isDomainMisconfigured(domain); + + if (broken === null) { + logger.warn(`Skipping domain ${domain} — Vercel API request failed`); + return { misconfigured: 0, notified: 0 }; + } + + if (!broken) { + return { misconfigured: 0, notified: 0 }; + } + + logger.warn(`Domain misconfigured: ${domain}`, { + organizationId: trust.organizationId, + }); + + await db.trust.update({ + where: { organizationId: trust.organizationId }, + data: { domainVerified: false }, + }); + + const adminOrOwnerMembers = trust.organization.members.filter( + (m) => + parseRoles(m.role).some((role) => NOTIFIABLE_ROLES.includes(role)) && + m.user?.email, + ); + + const settingsUrl = `${APP_BASE_URL}/${trust.organizationId}/trust/portal-settings`; + + const emailResults = await Promise.allSettled( + adminOrOwnerMembers + .filter((m) => m.user?.email) + .map((member) => + emailService.sendDomainMisconfiguredEmail({ + toEmail: member.user!.email!, + toName: member.user!.name?.trim() || member.user!.email!, + organizationName: trust.organization.name, + domain, + settingsUrl, + }), + ), + ); + + emailResults.forEach((result, i) => { + if (result.status === 'rejected') { + logger.error( + `Failed to send domain misconfigured email to ${adminOrOwnerMembers[i].user?.email}`, + { + error: + result.reason instanceof Error + ? result.reason.message + : String(result.reason), + }, + ); + } + }); + + return { + misconfigured: 1, + notified: emailResults.filter((r) => r.status === 'fulfilled').length, + }; + }), + ); + + const results = settled.map((s, i) => { + if (s.status === 'rejected') { + logger.error( + `Domain health check failed for trust ${trusts[i].organizationId}`, + { + error: + s.reason instanceof Error ? s.reason.message : String(s.reason), + }, + ); + return { misconfigured: 0, notified: 0 }; + } + return s.value; + }); + + const checked = trusts.length; + const misconfigured = results.reduce((sum, r) => sum + r.misconfigured, 0); + const notified = results.reduce((sum, r) => sum + r.notified, 0); + + logger.info('Trust Portal domain health check complete', { + checked, + misconfigured, + notified, + }); + + return { checked, misconfigured, notified }; + }, +}); diff --git a/apps/api/src/trust-portal/email.service.ts b/apps/api/src/trust-portal/email.service.ts index fab01d9e51..1b2f4a4d30 100644 --- a/apps/api/src/trust-portal/email.service.ts +++ b/apps/api/src/trust-portal/email.service.ts @@ -4,6 +4,7 @@ import { AccessGrantedEmail } from '../email/templates/access-granted'; import { AccessReclaimEmail } from '../email/templates/access-reclaim'; import { NdaSigningEmail } from '../email/templates/nda-signing'; import { AccessRequestNotificationEmail } from '../email/templates/access-request-notification'; +import { TrustDomainMisconfiguredEmail } from '../email/templates/trust-domain-misconfigured'; @Injectable() export class TrustEmailService { @@ -131,4 +132,30 @@ export class TrustEmailService { `Access request notification sent to ${toEmail} for requester ${requesterEmail} (ID: ${id})`, ); } + + async sendDomainMisconfiguredEmail(params: { + toEmail: string; + toName: string; + organizationName: string; + domain: string; + settingsUrl: string; + }): Promise<void> { + const { toEmail, toName, organizationName, domain, settingsUrl } = params; + + const { id } = await triggerEmail({ + to: toEmail, + subject: `Action required: Trust Portal domain ${domain} is misconfigured`, + react: TrustDomainMisconfiguredEmail({ + toName, + organizationName, + domain, + settingsUrl, + }), + trustPortal: true, + }); + + this.logger.log( + `Domain misconfigured email sent to ${toEmail} for domain ${domain} (ID: ${id})`, + ); + } } From 55a48fb0abc2a419970356914979575735f076b1 Mon Sep 17 00:00:00 2001 From: Mariano Fuentes <marfuen98@gmail.com> Date: Mon, 27 Jul 2026 15:11:05 -0400 Subject: [PATCH 6/6] fix(audit): stop plaintext secrets leaking into the audit log (#3512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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> --- .../admin-audit-log.interceptor.spec.ts | 24 +++++++ .../admin-audit-log.interceptor.ts | 16 ++++- apps/api/src/audit/audit-log.constants.ts | 18 +++++ .../src/audit/audit-log.interceptor.spec.ts | 39 +++++++++++ apps/api/src/audit/audit-log.interceptor.ts | 7 ++ apps/api/src/audit/audit-log.utils.spec.ts | 66 +++++++++++++++++++ apps/api/src/audit/audit-log.utils.ts | 22 ++++++- apps/api/src/email/email.controller.ts | 5 ++ 8 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 apps/api/src/audit/audit-log.utils.spec.ts diff --git a/apps/api/src/admin-organizations/admin-audit-log.interceptor.spec.ts b/apps/api/src/admin-organizations/admin-audit-log.interceptor.spec.ts index ccdca4374c..e4a6c81aaa 100644 --- a/apps/api/src/admin-organizations/admin-audit-log.interceptor.spec.ts +++ b/apps/api/src/admin-organizations/admin-audit-log.interceptor.spec.ts @@ -54,6 +54,8 @@ jest.mock('@db', () => ({ jest.mock('../audit/audit-log.constants', () => ({ MUTATION_METHODS: new Set(['POST', 'PATCH', 'PUT', 'DELETE']), SENSITIVE_KEYS: new Set(['password', 'token']), + SENSITIVE_KEY_PATTERN: + /secret|password|passphrase|credential|token|api[_-]?key|private[_-]?key|totp|access[_-]?key/i, })); function buildContext(overrides: { @@ -261,6 +263,28 @@ describe('AdminAuditLogInterceptor', () => { }); }); + it('sanitizes credential-shaped keys the exact list misses (clientSecret)', (done) => { + mockPolicyFind.mockResolvedValue({ name: 'Test' }); + + const ctx = buildContext({ + method: 'PATCH', + url: '/v1/admin/organizations/org_1/policies/pol_1', + params: { orgId: 'org_1' }, + body: { status: 'published', clientSecret: 'leak_me' }, + }); + + interceptor.intercept(ctx, nextHandler).subscribe({ + complete: () => { + setTimeout(() => { + const changes = mockCreate.mock.calls[0][0].data.data.changes; + expect(changes.status).toBeDefined(); + expect(changes.clientSecret).toBeUndefined(); + done(); + }, 50); + }, + }); + }); + it('should handle DELETE for invitations', (done) => { const ctx = buildContext({ method: 'DELETE', diff --git a/apps/api/src/admin-organizations/admin-audit-log.interceptor.ts b/apps/api/src/admin-organizations/admin-audit-log.interceptor.ts index 9db723b935..54a9916085 100644 --- a/apps/api/src/admin-organizations/admin-audit-log.interceptor.ts +++ b/apps/api/src/admin-organizations/admin-audit-log.interceptor.ts @@ -8,7 +8,11 @@ import { import { AuditLogEntityType, db, Prisma } from '@db'; import { Reflector } from '@nestjs/core'; import { Observable, tap } from 'rxjs'; -import { MUTATION_METHODS, SENSITIVE_KEYS } from '../audit/audit-log.constants'; +import { + MUTATION_METHODS, + SENSITIVE_KEYS, + SENSITIVE_KEY_PATTERN, +} from '../audit/audit-log.constants'; import { SKIP_ADMIN_AUDIT_LOG_KEY } from './skip-admin-audit-log.decorator'; const SEGMENT_TO_RESOURCE: Record< @@ -261,7 +265,15 @@ export class AdminAuditLogInterceptor implements NestInterceptor { const changes: Changes = {}; for (const [key, value] of Object.entries(body)) { - if (value === undefined || SENSITIVE_KEYS.has(key)) continue; + // Skip exact-match sensitive keys AND credential-shaped names the exact + // list misses (clientSecret, secretAccessKey, …) — shared with the global + // interceptor so both audit paths redact consistently. + if ( + value === undefined || + SENSITIVE_KEYS.has(key) || + SENSITIVE_KEY_PATTERN.test(key) + ) + continue; changes[key] = { previous: null, current: value }; } diff --git a/apps/api/src/audit/audit-log.constants.ts b/apps/api/src/audit/audit-log.constants.ts index 33b7d3a71c..6ae08e863f 100644 --- a/apps/api/src/audit/audit-log.constants.ts +++ b/apps/api/src/audit/audit-log.constants.ts @@ -24,6 +24,24 @@ export const SENSITIVE_KEYS = new Set([ 'totpCode', ]); +/** + * Fallback pattern for credential-ish field names the exact-match set misses + * (e.g. `clientSecret`, `secretAccessKey`, `aws_secret_access_key`). Matched + * case-insensitively against key names anywhere in the audited body, so new + * credential fields are redacted without having to enumerate every name. + */ +export const SENSITIVE_KEY_PATTERN = + /secret|password|passphrase|credential|token|api[_-]?key|private[_-]?key|totp|access[_-]?key/i; + +/** + * Resources whose request body must never be diffed into the audit log at all — + * the field carrying the secret is generically named (e.g. the secret manager's + * `value`) so key-based redaction can't catch it, and reading audit logs needs + * only `app:read`. For these we log the action (Created/Updated/Deleted) with no + * payload, keeping plaintext out of a store that bypasses `secret:read`. + */ +export const REDACT_BODY_RESOURCES = new Set(['secret']); + export const RESOURCE_TO_ENTITY_TYPE: Record< string, AuditLogEntityType | null diff --git a/apps/api/src/audit/audit-log.interceptor.spec.ts b/apps/api/src/audit/audit-log.interceptor.spec.ts index 6a29fcdcc8..3852e6eb4c 100644 --- a/apps/api/src/audit/audit-log.interceptor.spec.ts +++ b/apps/api/src/audit/audit-log.interceptor.spec.ts @@ -443,6 +443,45 @@ describe('AuditLogInterceptor', () => { }); }); + it('logs the action but never the payload for the secret resource', (done) => { + // Reading audit logs needs only app:read; the secret manager's plaintext + // `value` must not be diffed into the log where an auditor (no secret:read) + // could read it. + jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => { + if (key === PERMISSIONS_KEY) { + return [{ resource: 'secret', actions: ['create'] }]; + } + if (key === SKIP_AUDIT_LOG_KEY) return false; + return undefined; + }); + + const context = createMockExecutionContext({ + method: 'POST', + url: '/v1/secrets', + params: {}, + body: { name: 'STRIPE_KEY', value: 'sk_live_super_secret' }, + }); + const handler = createMockCallHandler({ id: 'sec_1' }); + + interceptor.intercept(context, handler).subscribe({ + next: () => { + setTimeout(() => { + expect(mockCreate).toHaveBeenCalled(); + // The action is recorded... + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ description: 'Created secret' }), + }), + ); + // ...but the plaintext value never appears anywhere in the row. + const persisted = JSON.stringify(mockCreate.mock.calls[0][0]); + expect(persisted).not.toContain('sk_live_super_secret'); + done(); + }, 50); + }, + }); + }); + it('should skip requests without userId', (done) => { jest.spyOn(reflector, 'getAllAndOverride').mockImplementation((key) => { if (key === PERMISSIONS_KEY) { diff --git a/apps/api/src/audit/audit-log.interceptor.ts b/apps/api/src/audit/audit-log.interceptor.ts index 06bf722fac..85374fdb67 100644 --- a/apps/api/src/audit/audit-log.interceptor.ts +++ b/apps/api/src/audit/audit-log.interceptor.ts @@ -15,6 +15,7 @@ import { AUDIT_READ_KEY, SKIP_AUDIT_LOG_KEY } from './skip-audit-log.decorator'; import { MEMBER_REF_FIELDS, MUTATION_METHODS, + REDACT_BODY_RESOURCES, RESOURCE_TO_ENTITY_TYPE, } from './audit-log.constants'; import { @@ -268,6 +269,12 @@ export class AuditLogInterceptor implements NestInterceptor { } else if (relationMappingResult) { changes = relationMappingResult.changes; descriptionOverride ??= relationMappingResult.description; + } else if (REDACT_BODY_RESOURCES.has(resource)) { + // Credential resources (e.g. the secret manager) carry their + // secret in a generically-named field. Diffing the body would + // land plaintext in a store readable with only app:read, which + // bypasses <resource>:read. Record the action, not the payload. + changes = null; } else { changes = requestBody ? buildChanges(requestBody, previousValues, memberNames) diff --git a/apps/api/src/audit/audit-log.utils.spec.ts b/apps/api/src/audit/audit-log.utils.spec.ts new file mode 100644 index 0000000000..9c3125b12d --- /dev/null +++ b/apps/api/src/audit/audit-log.utils.spec.ts @@ -0,0 +1,66 @@ +// buildChanges → constants pull Prisma enums at module load; stub @db so the +// pure redaction logic can be tested without a real client. +jest.mock('@db', () => ({ + db: {}, + AuditLogEntityType: new Proxy({}, { get: (_t, p) => p }), + CommentEntityType: new Proxy({}, { get: (_t, p) => p }), +})); + +import { buildChanges } from './audit-log.utils'; + +describe('buildChanges — redaction', () => { + it('redacts the existing exact-match sensitive keys', () => { + const changes = buildChanges( + { password: 'p', apiKey: 'k', name: 'ok' }, + null, + {}, + ); + expect(changes?.password.current).toBe('[REDACTED]'); + expect(changes?.apiKey.current).toBe('[REDACTED]'); + expect(changes?.name.current).toBe('ok'); + }); + + it('redacts credential-named keys the exact list misses (clientSecret, secretAccessKey)', () => { + const changes = buildChanges( + { + clientSecret: 'abc', + secretAccessKey: 'xyz', + aws_secret_access_key: 'q', + name: 'ok', + }, + null, + {}, + ); + expect(changes?.clientSecret.current).toBe('[REDACTED]'); + expect(changes?.secretAccessKey.current).toBe('[REDACTED]'); + expect(changes?.aws_secret_access_key.current).toBe('[REDACTED]'); + expect(changes?.name.current).toBe('ok'); + }); + + it('summarizes object elements inside arrays so secrets in generic fields cannot leak', () => { + // e.g. browserbase `extraFields: [{ label, value }]` — `value` is a secret + // under a non-credential key; the whole element is hidden as [Object]. + const changes = buildChanges( + { extraFields: [{ label: 'workspace', value: 'sekret' }] }, + null, + {}, + ); + expect(changes?.extraFields.current).toEqual(['[Object]']); + }); + + it('keeps primitive array elements (ids, scopes, tags) visible', () => { + const changes = buildChanges({ scopes: ['read', 'write'] }, null, {}); + expect(changes?.scopes.current).toEqual(['read', 'write']); + }); + + it('keeps summarizing nested plain objects as [Object]', () => { + const changes = buildChanges({ config: { a: 1, token: 't' } }, null, {}); + expect(changes?.config.current).toBe('[Object]'); + }); + + it('leaves ordinary values untouched', () => { + const changes = buildChanges({ status: 'active', count: 3 }, null, {}); + expect(changes?.status.current).toBe('active'); + expect(changes?.count.current).toBe(3); + }); +}); diff --git a/apps/api/src/audit/audit-log.utils.ts b/apps/api/src/audit/audit-log.utils.ts index e4a10775bb..de67a3d7a1 100644 --- a/apps/api/src/audit/audit-log.utils.ts +++ b/apps/api/src/audit/audit-log.utils.ts @@ -4,6 +4,7 @@ import { COMMENT_ENTITY_TYPE_MAP, MEMBER_REF_FIELDS, SENSITIVE_KEYS, + SENSITIVE_KEY_PATTERN, } from './audit-log.constants'; export type AuditContextOverride = { @@ -328,14 +329,29 @@ export function buildDescription( } } +function isSensitiveKey(key: string): boolean { + return SENSITIVE_KEYS.has(key) || SENSITIVE_KEY_PATTERN.test(key); +} + function sanitizeValue(key: string, value: unknown): unknown { - if (SENSITIVE_KEYS.has(key)) return '[REDACTED]'; + if (isSensitiveKey(key)) return '[REDACTED]'; if (value instanceof Date) return value.toISOString(); - if (value && typeof value === 'object' && !Array.isArray(value)) - return '[Object]'; + // Arrays are logged rather than summarized, so a secret in a generically-named + // field inside an array element (e.g. `extraFields: [{ label, value }]`) would + // otherwise land in the log verbatim. Keep primitive elements (ids, scopes, + // tags) but summarize object/array elements as '[Object]' — the same way a + // nested object is hidden below. + if (Array.isArray(value)) return value.map(summarizeArrayItem); + if (value && typeof value === 'object') return '[Object]'; return value; } +function summarizeArrayItem(item: unknown): unknown { + if (item instanceof Date) return item.toISOString(); + if (item && typeof item === 'object') return '[Object]'; + return item; +} + export function buildChanges( body: Record<string, unknown>, previousValues: Record<string, unknown> | null, diff --git a/apps/api/src/email/email.controller.ts b/apps/api/src/email/email.controller.ts index 2b15718d12..54dde600ea 100644 --- a/apps/api/src/email/email.controller.ts +++ b/apps/api/src/email/email.controller.ts @@ -10,6 +10,7 @@ import { tasks } from '@trigger.dev/sdk'; import { HybridAuthGuard } from '../auth/hybrid-auth.guard'; import { PermissionGuard } from '../auth/permission.guard'; import { RequirePermission } from '../auth/require-permission.decorator'; +import { SkipAuditLog } from '../audit/skip-audit-log.decorator'; import { SendEmailDto } from './dto/send-email.dto'; import { SendBatchEmailDto } from './dto/send-batch-email.dto'; import type { sendEmailTask } from '../trigger/email/send-email'; @@ -24,6 +25,9 @@ export class EmailController { @Post('send') @HttpCode(200) @RequirePermission('email', 'send') + // The body carries rendered `html` (magic-links / OTP) and full attachment + // bytes — never worth diffing into the audit log, and readable with app:read. + @SkipAuditLog() @ApiOperation({ summary: 'Send an email via the centralized Trigger task (internal)', }) @@ -46,6 +50,7 @@ export class EmailController { @Post('send-batch') @HttpCode(200) @RequirePermission('email', 'send') + @SkipAuditLog() @ApiOperation({ summary: 'Send a batch of emails via the centralized Trigger task (internal)', })