Skip to content

Commit 8b3b39b

Browse files
carhartlewisclaude
carhartlewis
and
claude
committed
fix(background-checks): security hardening across payment flow, validation, and logging
P1: Restructure requestForMember to create DB record before charging Stripe, preventing orphaned payments on DB failure and eliminating the TOCTOU race condition on concurrent requests via unique constraint catch. P2: Add @maxlength to base64 fileData field (50MB limit), add @isurl validation to billing redirect DTOs, remove env var names from error messages, and add session metadata org check in handleSetupSuccess. P3: Enhance refund failure logging with structured context for manual intervention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d5df5db commit 8b3b39b

7 files changed

Lines changed: 148 additions & 90 deletions

‎apps/api/src/attachments/upload-attachment.dto.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ export class UploadAttachmentDto {
3535
})
3636
@IsString()
3737
@IsNotEmpty()
38+
@MaxLength(67_108_864)
3839
@IsBase64()
3940
fileData: string;
4041

‎apps/api/src/background-checks/background-check-billing.service.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ export class BackgroundCheckBillingService {
8282
throw new BadRequestException('Checkout session is not complete.');
8383
}
8484

85+
if (session.metadata?.organizationId && session.metadata.organizationId !== organizationId) {
86+
throw new BadRequestException('Checkout session does not belong to this organization.');
87+
}
88+
8589
const stripeCustomerId = this.extractStripeId(session.customer);
8690
if (!stripeCustomerId) {
8791
throw new BadRequestException('Checkout session is missing a customer.');
@@ -191,13 +195,13 @@ export class BackgroundCheckBillingService {
191195
async getBackgroundCheckPrice(): Promise<{ id: string; unitAmount: number; currency: string }> {
192196
const priceId = process.env.STRIPE_BACKGROUND_CHECK_PRICE_ID;
193197
if (!priceId) {
194-
throw new BadRequestException('STRIPE_BACKGROUND_CHECK_PRICE_ID is not configured.');
198+
throw new BadRequestException('Background check pricing is not configured. Contact support.');
195199
}
196200

197201
const stripe = this.stripeService.getClient();
198202
const price = await stripe.prices.retrieve(priceId);
199203
if (price.unit_amount === null || price.unit_amount === undefined) {
200-
throw new BadRequestException('Background check price has no unit amount.');
204+
throw new BadRequestException('Background check pricing is not configured. Contact support.');
201205
}
202206

203207
return {

‎apps/api/src/background-checks/background-check-identity.client.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ export class BackgroundCheckIdentityClient {
1717
}): Promise<IdentityCreateResponse> {
1818
const apiKey = process.env.BACKGROUND_CHECK_API_KEY;
1919
if (!apiKey) {
20-
throw new BadRequestException('BACKGROUND_CHECK_API_KEY is not configured.');
20+
throw new BadRequestException('Background check service is not configured. Contact support.');
2121
}
2222

2323
const baseUrl = this.baseUrl();

‎apps/api/src/background-checks/background-check-payment.service.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,15 @@ export class BackgroundCheckPaymentService {
9797
);
9898
return refund.id;
9999
} catch (error) {
100-
this.logger.error('Failed to refund background check payment', error);
100+
this.logger.error(
101+
'Failed to refund background check payment — manual refund required.',
102+
{
103+
organizationId: params.organizationId,
104+
memberId: params.memberId,
105+
paymentIntentId: params.paymentIntentId,
106+
error: error instanceof Error ? error.message : 'Unknown error',
107+
},
108+
);
101109
return null;
102110
}
103111
}

‎apps/api/src/background-checks/background-checks.service.spec.ts‎

Lines changed: 80 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ jest.mock('@db', () => {
2727
backgroundCheckRequest: {
2828
findUnique: jest.fn(),
2929
findFirst: jest.fn(),
30+
create: jest.fn(),
3031
upsert: jest.fn(),
3132
update: jest.fn(),
3233
},
@@ -219,17 +220,23 @@ describe('background checks', () => {
219220
id: 'mem_1',
220221
organizationId: 'org_1',
221222
} as Awaited<ReturnType<typeof db.member.findFirst>>);
222-
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>>(
223-
mockedDb.backgroundCheckRequest.upsert,
223+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.create>>>(
224+
mockedDb.backgroundCheckRequest.create,
225+
).mockResolvedValueOnce({
226+
id: 'bcr_1',
227+
status: 'invited',
228+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.create>>);
229+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>>(
230+
mockedDb.backgroundCheckRequest.update,
224231
)
225232
.mockResolvedValueOnce({
226233
id: 'bcr_1',
227234
status: 'invited',
228-
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>)
235+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>)
229236
.mockResolvedValueOnce({
230237
id: 'bcr_1',
231238
status: 'failed',
232-
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>);
239+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>);
233240

234241
const identityClient = {
235242
createBackgroundCheck: jest.fn().mockRejectedValue(new Error('identity down')),
@@ -263,9 +270,9 @@ describe('background checks', () => {
263270
memberId: 'mem_1',
264271
paymentIntentId: 'pi_1',
265272
});
266-
expect(mockedDb.backgroundCheckRequest.upsert).toHaveBeenCalledWith(
273+
expect(mockedDb.backgroundCheckRequest.update).toHaveBeenCalledWith(
267274
expect.objectContaining({
268-
create: expect.objectContaining({
275+
data: expect.objectContaining({
269276
status: 'failed',
270277
stripeRefundId: 're_1',
271278
}),
@@ -283,17 +290,23 @@ describe('background checks', () => {
283290
id: 'mem_1',
284291
organizationId: 'org_1',
285292
} as Awaited<ReturnType<typeof db.member.findFirst>>);
286-
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>>(
287-
mockedDb.backgroundCheckRequest.upsert,
293+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.create>>>(
294+
mockedDb.backgroundCheckRequest.create,
295+
).mockResolvedValueOnce({
296+
id: 'bcr_1',
297+
status: 'invited',
298+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.create>>);
299+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>>(
300+
mockedDb.backgroundCheckRequest.update,
288301
)
289302
.mockResolvedValueOnce({
290303
id: 'bcr_1',
291304
status: 'invited',
292-
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>)
305+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>)
293306
.mockResolvedValueOnce({
294307
id: 'bcr_1',
295308
status: 'invited',
296-
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.upsert>>);
309+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.update>>);
297310

298311
const identityClient = {
299312
createBackgroundCheck: jest.fn().mockResolvedValue({
@@ -325,32 +338,34 @@ describe('background checks', () => {
325338
requesterNotes: 'Expedite this check.',
326339
});
327340

328-
expect(mockedDb.backgroundCheckRequest.upsert).toHaveBeenCalledWith(
341+
// Record is created with requester notes before charging
342+
expect(mockedDb.backgroundCheckRequest.create).toHaveBeenCalledWith(
329343
expect.objectContaining({
330-
create: expect.objectContaining({
344+
data: expect.objectContaining({
331345
requesterNotes: 'Expedite this check.',
346+
status: 'invited',
332347
}),
333348
}),
334349
);
335-
expect(mockedDb.backgroundCheckRequest.upsert).toHaveBeenNthCalledWith(
336-
1,
350+
// Payment info is persisted via update
351+
expect(mockedDb.backgroundCheckRequest.update).toHaveBeenCalledWith(
337352
expect.objectContaining({
338-
create: expect.objectContaining({
339-
status: 'invited',
353+
data: expect.objectContaining({
340354
stripePaymentIntentId: 'pi_1',
341355
}),
342356
}),
343357
);
344-
expect(mockedDb.backgroundCheckRequest.upsert).toHaveBeenNthCalledWith(
345-
2,
358+
// Identity result is persisted via update
359+
expect(mockedDb.backgroundCheckRequest.update).toHaveBeenCalledWith(
346360
expect.objectContaining({
347-
update: expect.objectContaining({
361+
data: expect.objectContaining({
348362
identityBackgroundCheckId: 'check_1',
349363
candidateUrl: 'https://identity.trycomp.ai/cand_1',
350364
}),
351365
}),
352366
);
353-
expect(invocationOrder(mockedDb.backgroundCheckRequest.upsert)).toBeLessThan(
367+
// Record is created before Identity API is called
368+
expect(invocationOrder(mockedDb.backgroundCheckRequest.create)).toBeLessThan(
354369
invocationOrder(identityClient.createBackgroundCheck),
355370
);
356371
expect(identityClient.createBackgroundCheck).toHaveBeenCalledWith(
@@ -360,6 +375,51 @@ describe('background checks', () => {
360375
);
361376
});
362377

378+
it('handles concurrent requests by returning existing record on unique constraint', async () => {
379+
const { Prisma } = jest.requireMock<typeof import('@db')>('@db');
380+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.findUnique>>>(
381+
mockedDb.backgroundCheckRequest.findUnique,
382+
)
383+
.mockResolvedValueOnce(null)
384+
.mockResolvedValueOnce({
385+
id: 'bcr_1',
386+
status: 'invited',
387+
} as Awaited<ReturnType<typeof db.backgroundCheckRequest.findUnique>>);
388+
mockAsync<Awaited<ReturnType<typeof db.member.findFirst>>>(
389+
mockedDb.member.findFirst,
390+
).mockResolvedValueOnce({
391+
id: 'mem_1',
392+
organizationId: 'org_1',
393+
} as Awaited<ReturnType<typeof db.member.findFirst>>);
394+
mockAsync<Awaited<ReturnType<typeof db.backgroundCheckRequest.create>>>(
395+
mockedDb.backgroundCheckRequest.create,
396+
).mockRejectedValueOnce(
397+
new Prisma.PrismaClientKnownRequestError('duplicate', {
398+
code: 'P2002',
399+
clientVersion: 'test',
400+
}),
401+
);
402+
403+
const paymentService = { charge: jest.fn(), refund: jest.fn() };
404+
const identityClient = { createBackgroundCheck: jest.fn() };
405+
const service = new BackgroundChecksService(
406+
identityClient as unknown as BackgroundCheckIdentityClient,
407+
paymentService as unknown as BackgroundCheckPaymentService,
408+
);
409+
410+
const result = await service.requestForMember({
411+
organizationId: 'org_1',
412+
memberId: 'mem_1',
413+
employeeName: 'Ada Lovelace',
414+
employeeEmail: 'ada@example.com',
415+
requesterEmail: 'admin@example.com',
416+
});
417+
418+
expect(result).toEqual(expect.objectContaining({ id: 'bcr_1' }));
419+
expect(paymentService.charge).not.toHaveBeenCalled();
420+
expect(identityClient.createBackgroundCheck).not.toHaveBeenCalled();
421+
});
422+
363423
it('uses BETTER_AUTH_URL as the local app URL fallback for setup redirects', async () => {
364424
process.env = {
365425
...process.env,

‎apps/api/src/background-checks/background-checks.service.ts‎

Lines changed: 47 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -65,38 +65,56 @@ export class BackgroundChecksService {
6565
throw new NotFoundException('Member not found.');
6666
}
6767

68+
// Step 1: Claim the record slot before charging. Catches the TOCTOU race
69+
// where two concurrent requests both pass the getForMember check.
70+
try {
71+
await db.backgroundCheckRequest.create({
72+
data: {
73+
organizationId,
74+
memberId,
75+
employeeName,
76+
employeeEmail,
77+
requesterNotes,
78+
status: BackgroundCheckStatus.invited,
79+
lastSyncedAt: new Date(),
80+
},
81+
});
82+
} catch (error) {
83+
if (this.isUniqueConstraintError(error)) {
84+
const raced = await this.getForMember({ organizationId, memberId });
85+
if (raced) return raced;
86+
}
87+
throw error;
88+
}
89+
90+
// Step 2: Charge — record exists so a failure here is recoverable
6891
const payment = await this.paymentService.charge({
6992
organizationId,
7093
memberId,
7194
});
7295

73-
await db.backgroundCheckRequest.upsert({
74-
where: { organizationId_memberId: { organizationId, memberId } },
75-
create: {
96+
// Step 3: Persist payment info. Refund if this write fails.
97+
try {
98+
await db.backgroundCheckRequest.update({
99+
where: { organizationId_memberId: { organizationId, memberId } },
100+
data: {
101+
stripePaymentIntentId: payment.paymentIntentId,
102+
stripePaymentStatus: payment.status,
103+
stripeAmountCents: payment.amount,
104+
stripeCurrency: payment.currency,
105+
lastSyncedAt: new Date(),
106+
},
107+
});
108+
} catch (error) {
109+
await this.paymentService.refund({
76110
organizationId,
77111
memberId,
78-
employeeName,
79-
employeeEmail,
80-
requesterNotes,
81-
status: BackgroundCheckStatus.invited,
82-
stripePaymentIntentId: payment.paymentIntentId,
83-
stripePaymentStatus: payment.status,
84-
stripeAmountCents: payment.amount,
85-
stripeCurrency: payment.currency,
86-
lastSyncedAt: new Date(),
87-
},
88-
update: {
89-
employeeName,
90-
employeeEmail,
91-
requesterNotes,
92-
stripePaymentIntentId: payment.paymentIntentId,
93-
stripePaymentStatus: payment.status,
94-
stripeAmountCents: payment.amount,
95-
stripeCurrency: payment.currency,
96-
lastSyncedAt: new Date(),
97-
},
98-
});
112+
paymentIntentId: payment.paymentIntentId,
113+
});
114+
throw error;
115+
}
99116

117+
// Step 4: Call Identity API — refund on failure
100118
let identityResult;
101119
try {
102120
identityResult = await this.identityClient.createBackgroundCheck({
@@ -107,30 +125,15 @@ export class BackgroundChecksService {
107125
requesterEmail,
108126
});
109127
} catch (error) {
110-
// Only refund when the identity API call itself fails — not on DB errors
111128
const refundId = await this.paymentService.refund({
112129
organizationId,
113130
memberId,
114131
paymentIntentId: payment.paymentIntentId,
115132
});
116133

117-
await db.backgroundCheckRequest.upsert({
134+
await db.backgroundCheckRequest.update({
118135
where: { organizationId_memberId: { organizationId, memberId } },
119-
create: {
120-
organizationId,
121-
memberId,
122-
employeeName,
123-
employeeEmail,
124-
requesterNotes,
125-
status: BackgroundCheckStatus.failed,
126-
stripePaymentIntentId: payment.paymentIntentId,
127-
stripePaymentStatus: payment.status,
128-
stripeRefundId: refundId,
129-
stripeAmountCents: payment.amount,
130-
stripeCurrency: payment.currency,
131-
lastSyncedAt: new Date(),
132-
},
133-
update: {
136+
data: {
134137
status: BackgroundCheckStatus.failed,
135138
stripeRefundId: refundId,
136139
lastSyncedAt: new Date(),
@@ -140,34 +143,13 @@ export class BackgroundChecksService {
140143
throw error;
141144
}
142145

143-
return db.backgroundCheckRequest.upsert({
146+
// Step 5: Persist Identity result
147+
return db.backgroundCheckRequest.update({
144148
where: { organizationId_memberId: { organizationId, memberId } },
145-
create: {
146-
organizationId,
147-
memberId,
148-
employeeName,
149-
employeeEmail,
150-
requesterNotes,
151-
identityBackgroundCheckId: identityResult.id,
152-
candidateUrl: identityResult.candidateUrl ?? null,
153-
status: identityResult.status,
154-
stripePaymentIntentId: payment.paymentIntentId,
155-
stripePaymentStatus: payment.status,
156-
stripeAmountCents: payment.amount,
157-
stripeCurrency: payment.currency,
158-
lastSyncedAt: new Date(),
159-
},
160-
update: {
161-
employeeName,
162-
employeeEmail,
163-
requesterNotes,
149+
data: {
164150
identityBackgroundCheckId: identityResult.id,
165151
candidateUrl: identityResult.candidateUrl ?? null,
166152
status: identityResult.status,
167-
stripePaymentIntentId: payment.paymentIntentId,
168-
stripePaymentStatus: payment.status,
169-
stripeAmountCents: payment.amount,
170-
stripeCurrency: payment.currency,
171153
lastSyncedAt: new Date(),
172154
},
173155
});

0 commit comments

Comments
 (0)