From 7414acaa85dd81b80132e09ec970c15ef2ab4d9e Mon Sep 17 00:00:00 2001 From: Mariano Fuentes Date: Wed, 20 May 2026 16:42:48 -0400 Subject: [PATCH] fix(people): address cubic review findings for offboarding feature - prevent CSV formula injection in export by prefixing dangerous chars - sanitize filenames in ZIP paths to prevent path traversal - add member ID to export folder names to prevent collisions - scope revocation lookup by organizationId to prevent cross-tenant access - rollback completion record if evidence upload fails - filter completion counts to enabled template items only - use @IsInt() for sortOrder DTO validation - validate date query params before building DB filters - scope attachment deletion to member and event type - show "Until {date}" when only end date is set in filter - clear file input in finally block for retry support - validate tab param against dynamically available tabs - show error state in TodosOverview on fetch failure Co-Authored-By: Claude Opus 4.6 (1M context) --- .../access-revocation.service.ts | 4 +-- .../dto/update-template-item.dto.ts | 4 +-- .../offboarding-checklist.service.ts | 36 ++++++++++++------- .../offboarding-export.service.ts | 18 +++++++--- apps/api/src/people/people.controller.ts | 27 +++++++++++--- .../overview/components/TodosOverview.tsx | 8 ++++- .../[employeeId]/components/Employee.tsx | 12 +++++-- .../components/OffboardingChecklistItem.tsx | 7 ++-- .../all/components/TeamMembersClient.tsx | 4 ++- 9 files changed, 89 insertions(+), 31 deletions(-) diff --git a/apps/api/src/offboarding-checklist/access-revocation.service.ts b/apps/api/src/offboarding-checklist/access-revocation.service.ts index 2ed8c002a..a0cdb719d 100644 --- a/apps/api/src/offboarding-checklist/access-revocation.service.ts +++ b/apps/api/src/offboarding-checklist/access-revocation.service.ts @@ -132,8 +132,8 @@ export class AccessRevocationService { memberId: string; vendorId: string; }) { - const revocation = await db.offboardingAccessRevocation.findUnique({ - where: { memberId_vendorId: { memberId, vendorId } }, + const revocation = await db.offboardingAccessRevocation.findFirst({ + where: { memberId, vendorId, organizationId }, }); if (!revocation) { diff --git a/apps/api/src/offboarding-checklist/dto/update-template-item.dto.ts b/apps/api/src/offboarding-checklist/dto/update-template-item.dto.ts index 5d7594fda..d246ec5d5 100644 --- a/apps/api/src/offboarding-checklist/dto/update-template-item.dto.ts +++ b/apps/api/src/offboarding-checklist/dto/update-template-item.dto.ts @@ -1,5 +1,5 @@ import { ApiProperty } from '@nestjs/swagger'; -import { IsString, IsOptional, IsBoolean, IsNumber } from 'class-validator'; +import { IsString, IsOptional, IsBoolean, IsInt } from 'class-validator'; export class UpdateTemplateItemDto { @ApiProperty({ required: false }) @@ -19,7 +19,7 @@ export class UpdateTemplateItemDto { @ApiProperty({ required: false }) @IsOptional() - @IsNumber() + @IsInt() sortOrder?: number; @ApiProperty({ required: false }) diff --git a/apps/api/src/offboarding-checklist/offboarding-checklist.service.ts b/apps/api/src/offboarding-checklist/offboarding-checklist.service.ts index 185857efd..268dc521f 100644 --- a/apps/api/src/offboarding-checklist/offboarding-checklist.service.ts +++ b/apps/api/src/offboarding-checklist/offboarding-checklist.service.ts @@ -195,17 +195,24 @@ export class OffboardingChecklistService { }); if (dto.fileName && dto.fileData && dto.fileType) { - await this.attachmentsService.uploadAttachment( - organizationId, - completion.id, - AttachmentEntityType.offboarding_checklist, - { - fileName: dto.fileName, - fileData: dto.fileData, - fileType: dto.fileType, - }, - completedById, - ); + try { + await this.attachmentsService.uploadAttachment( + organizationId, + completion.id, + AttachmentEntityType.offboarding_checklist, + { + fileName: dto.fileName, + fileData: dto.fileData, + fileType: dto.fileType, + }, + completedById, + ); + } catch (err) { + await db.offboardingChecklistCompletion.delete({ + where: { id: completion.id }, + }); + throw err; + } } return completion; @@ -327,7 +334,12 @@ export class OffboardingChecklistService { }, include: { user: { select: { id: true, name: true, email: true, image: true } }, - offboardingChecklistCompletions: { select: { id: true } }, + offboardingChecklistCompletions: { + where: { + templateItem: { organizationId, isEnabled: true }, + }, + select: { id: true }, + }, }, orderBy: { offboardDate: 'desc' }, }); diff --git a/apps/api/src/offboarding-checklist/offboarding-export.service.ts b/apps/api/src/offboarding-checklist/offboarding-export.service.ts index fd09c8e40..a63fc7601 100644 --- a/apps/api/src/offboarding-checklist/offboarding-export.service.ts +++ b/apps/api/src/offboarding-checklist/offboarding-export.service.ts @@ -111,8 +111,9 @@ export class OffboardingExportService { for (const file of vendor.evidence) { const buffer = await this.getAttachmentBuffer(organizationId, file.id); if (!buffer) continue; + const safeName = sanitizeFileName(file.name); archive.append(buffer, { - name: `${prefix}vendor-access-revocations/evidence/${file.name}`, + name: `${prefix}vendor-access-revocations/evidence/${safeName}`, }); } } @@ -135,8 +136,9 @@ export class OffboardingExportService { for (const file of item.evidence) { const buffer = await this.getAttachmentBuffer(organizationId, file.id); if (!buffer) continue; + const safeName = sanitizeFileName(file.name); archive.append(buffer, { - name: `${prefix}checklist-items/${folderNum}-${folderName}/${file.name}`, + name: `${prefix}checklist-items/${folderNum}-${folderName}/${safeName}`, }); } } @@ -163,7 +165,7 @@ export class OffboardingExportService { .replace(/[^a-zA-Z0-9 ]/g, '') .replace(/\s+/g, '-') .toLowerCase(); - const prefix = `offboarded-employees/${safeName}/`; + const prefix = `offboarded-employees/${safeName}-${member.id}/`; const checklist = await this.offboardingChecklistService.getMemberChecklist( organizationId, @@ -199,6 +201,14 @@ export class OffboardingExportService { } } +function sanitizeFileName(name: string): string { + return name.replace(/.*[/\\]/, '').replace(/[/\\]/g, '_') || 'file'; +} + function escapeCsvField(value: string): string { - return value.replace(/"/g, '""'); + const escaped = value.replace(/"/g, '""'); + if (/^[=+\-@\t\r]/.test(escaped)) { + return `'${escaped}`; + } + return escaped; } diff --git a/apps/api/src/people/people.controller.ts b/apps/api/src/people/people.controller.ts index aa506661f..ecc345b73 100644 --- a/apps/api/src/people/people.controller.ts +++ b/apps/api/src/people/people.controller.ts @@ -117,11 +117,20 @@ export class PeopleController { @Query('offboardAfter') offboardAfter?: string, @Query('offboardBefore') offboardBefore?: string, ) { + const parseDateParam = (param: string | undefined, name: string): Date | undefined => { + if (!param) return undefined; + const date = new Date(param); + if (isNaN(date.getTime())) { + throw new BadRequestException(`Invalid date value for "${name}": ${param}`); + } + return date; + }; + const filters = { - ...(onboardAfter ? { onboardAfter: new Date(onboardAfter) } : {}), - ...(onboardBefore ? { onboardBefore: new Date(onboardBefore) } : {}), - ...(offboardAfter ? { offboardAfter: new Date(offboardAfter) } : {}), - ...(offboardBefore ? { offboardBefore: new Date(offboardBefore) } : {}), + ...(onboardAfter ? { onboardAfter: parseDateParam(onboardAfter, 'onboardAfter') } : {}), + ...(onboardBefore ? { onboardBefore: parseDateParam(onboardBefore, 'onboardBefore') } : {}), + ...(offboardAfter ? { offboardAfter: parseDateParam(offboardAfter, 'offboardAfter') } : {}), + ...(offboardBefore ? { offboardBefore: parseDateParam(offboardBefore, 'offboardBefore') } : {}), }; const hasFilters = Object.keys(filters).length > 0; @@ -610,8 +619,16 @@ export class PeopleController { @Param('attachmentId') attachmentId: string, @OrganizationId() organizationId: string, ) { - this.resolveEventType(eventType); + const entityType = this.resolveEventType(eventType); await this.peopleService.findById(memberId, organizationId); + const attachments = await this.attachmentsService.getAttachments( + organizationId, + memberId, + entityType, + ); + if (!attachments.some((a) => a.id === attachmentId)) { + throw new BadRequestException('Attachment not found for this member and event type'); + } await this.attachmentsService.deleteAttachment(organizationId, attachmentId); return { success: true }; } diff --git a/apps/app/src/app/(app)/[orgId]/overview/components/TodosOverview.tsx b/apps/app/src/app/(app)/[orgId]/overview/components/TodosOverview.tsx index 0253ca17f..ed1aa80f1 100644 --- a/apps/app/src/app/(app)/[orgId]/overview/components/TodosOverview.tsx +++ b/apps/app/src/app/(app)/[orgId]/overview/components/TodosOverview.tsx @@ -23,7 +23,7 @@ interface PendingResponse { export function TodosOverview() { const params = useParams<{ orgId: string }>(); const organizationId = params.orgId; - const { data, isLoading } = useApiSWR( + const { data, isLoading, error } = useApiSWR( '/v1/offboarding-checklist/pending', ); const members = data?.data?.members ?? []; @@ -44,6 +44,12 @@ export function TodosOverview() {
Loading...
+ ) : error ? ( +
+ + Failed to load todos + +
) : members.length === 0 ? (
diff --git a/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/Employee.tsx b/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/Employee.tsx index 92cd35b74..e5757f902 100644 --- a/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/Employee.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/Employee.tsx @@ -74,7 +74,15 @@ export function Employee({ const pathname = usePathname(); const router = useRouter(); - const VALID_TABS: EmployeeTab[] = ['details', 'policies', 'training', 'hipaa', 'device', 'offboarding', 'background-check']; + const availableTabs: EmployeeTab[] = [ + 'details', + 'policies', + 'training', + ...(hasHipaaFramework ? (['hipaa'] as EmployeeTab[]) : []), + 'device', + ...(backgroundCheckStepEnabled ? (['background-check'] as EmployeeTab[]) : []), + ...(employee.offboardDate ? (['offboarding'] as EmployeeTab[]) : []), + ]; const resolveTab = (): EmployeeTab => { if ( @@ -84,7 +92,7 @@ export function Employee({ return 'background-check'; } const tabParam = searchParams.get('tab'); - if (tabParam && VALID_TABS.includes(tabParam as EmployeeTab)) { + if (tabParam && availableTabs.includes(tabParam as EmployeeTab)) { return tabParam as EmployeeTab; } return 'details'; diff --git a/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx b/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx index 88fa5f209..3ccd207a7 100644 --- a/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx @@ -198,8 +198,11 @@ export function OffboardingChecklistItem({ const handleFileSelect = async (e: React.ChangeEvent) => { const file = e.target.files?.[0]; if (!file) return; - await handleFileUpload(file); - if (dropzoneInputRef.current) dropzoneInputRef.current.value = ''; + try { + await handleFileUpload(file); + } finally { + if (dropzoneInputRef.current) dropzoneInputRef.current.value = ''; + } }; const handleFileUpload = async (file: File) => { diff --git a/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx b/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx index 07dca17c4..b0432e8a1 100644 --- a/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx +++ b/apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx @@ -646,7 +646,9 @@ function DateRangeFilter({ ? `${format(from, 'MMM d')} – ${format(to, 'MMM d, yyyy')}` : from ? `From ${format(from, 'MMM d, yyyy')}` - : 'Any time'; + : to + ? `Until ${format(to, 'MMM d, yyyy')}` + : 'Any time'; return (