diff --git a/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts b/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts index c29e1ead18..797e5c049f 100644 --- a/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts +++ b/apps/api/src/security-penetration-tests/pentest-lineage.util.spec.ts @@ -109,5 +109,17 @@ describe('collapsedStatus', () => { }), ).toBe('failed'); }); + + it('reveals failed immediately when not retry-eligible (e.g. no scan params)', () => { + expect( + collapsedStatus({ + activeStatus: 'failed', + attemptNumber: 1, + retryEligible: false, + failedAtMs: now - 5_000, // well within grace, but can't retry + nowMs: now, + }), + ).toBe('failed'); + }); }); }); diff --git a/apps/api/src/security-penetration-tests/pentest-lineage.util.ts b/apps/api/src/security-penetration-tests/pentest-lineage.util.ts index 92134dc0bd..b2e3cebd8b 100644 --- a/apps/api/src/security-penetration-tests/pentest-lineage.util.ts +++ b/apps/api/src/security-penetration-tests/pentest-lineage.util.ts @@ -34,7 +34,9 @@ export const RETRY_GRACE_MS = 10 * 60 * 1000; * - `completed` / in-progress statuses pass through unchanged. * - `cancelled` is terminal and never retried → always revealed. * - `failed`: - * - lineage exhausted (`attemptNumber >= maxAttempts`) → revealed. + * - not retry-eligible (lineage exhausted, or the run can't actually be + * re-run — e.g. a pre-feature row with no stored scan params) → revealed + * immediately, so it's never masked as in-progress pointlessly. * - otherwise masked as `provisioning` (a retry is expected) while inside * the grace window; revealed once the window elapses without a retry * appearing, or when the failure time is unknown (can't bound the mask). @@ -44,19 +46,24 @@ export function collapsedStatus(params: { attemptNumber: number; failedAtMs: number | null; nowMs: number; + /** Whether this run can actually be auto-retried (has re-runnable params). */ + retryEligible?: boolean; maxAttempts?: number; graceMs?: number; }): PentestRunStatus { const { activeStatus, attemptNumber, failedAtMs, nowMs } = params; const maxAttempts = params.maxAttempts ?? MAX_ATTEMPTS; const graceMs = params.graceMs ?? RETRY_GRACE_MS; + const retryEligible = params.retryEligible ?? true; // Only `failed` triggers an auto-retry, so only `failed` is ever masked. if (activeStatus !== 'failed') { return activeStatus; } - if (attemptNumber >= maxAttempts) { + // Reveal immediately if no retry can happen — lineage exhausted or the run + // isn't re-runnable — so a failure that will never retry isn't masked. + if (attemptNumber >= maxAttempts || !retryEligible) { return 'failed'; } if (failedAtMs === null || nowMs - failedAtMs >= graceMs) { diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts index 63fa802dd8..114bf22eca 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.spec.ts @@ -68,6 +68,22 @@ describe('toCustomerFacingError', () => { expect(message).not.toContain('1 minutes'); }); + it('falls back to the generic message for a malformed zero cap', () => { + const message = toCustomerFacingError( + 'Hard timeout: workflow ran 0m (cap 0m)', + ); + expect(message).not.toContain('maximum runtime'); + expect(message).toContain("The scan couldn't be completed"); + }); + + it('falls back to the generic message for an absurd oversized cap', () => { + const message = toCustomerFacingError( + 'Hard timeout: workflow ran 1m (cap 999999999999999999999999m)', + ); + expect(message).not.toContain('maximum runtime'); + expect(message).toContain("The scan couldn't be completed"); + }); + it('classifies the "; Daytona unreachable" variant as infra, not runtime cap', () => { const message = toCustomerFacingError( 'Hard timeout: 4m (cap 120m); Daytona unreachable', diff --git a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts index 5662e7c046..1c863b6c67 100644 --- a/apps/api/src/security-penetration-tests/pentest-run-error.util.ts +++ b/apps/api/src/security-penetration-tests/pentest-run-error.util.ts @@ -76,8 +76,13 @@ export function toCustomerFacingError( // has already claimed the "; Daytona unreachable" variant. const capMatch = /hard timeout:\s*workflow ran\s+\d+m\s*\(cap\s+(\d+)m\)/i.exec(raw); - if (capMatch) { - const cap = humanizeMinutes(Number(capMatch[1])); + const capMinutes = capMatch ? Number(capMatch[1]) : NaN; + // Only use the runtime-cap message for a sane, positive whole-minute cap — a + // malformed "cap 0m" would render "maximum runtime of its time limit", and an + // absurd oversized cap would render e.g. "1e+24 minutes". Anything else falls + // through to the generic message. + if (Number.isSafeInteger(capMinutes) && capMinutes > 0) { + const cap = humanizeMinutes(capMinutes); return ( `Your scan reached its maximum runtime of ${cap} before completing. ` + "You won't be charged for this scan. Large targets may need a lighter " + diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts index a062f465f4..f04e7ab1d3 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.spec.ts @@ -164,6 +164,8 @@ describe('SecurityPenetrationTestsService', () => { return Promise.resolve({ providerRunId: where.rootRunId, attemptNumber: 1, + // Retry-eligible by default (has stored scan params). + scanParams: { targetUrl: 'https://app.example.com' }, }); } return Promise.resolve(null); @@ -215,6 +217,13 @@ describe('SecurityPenetrationTestsService', () => { return input instanceof Request ? input.url : String(input); } + function getRequestHeader(name: string, callIndex = 0): string | null { + const [input, init] = fetchMock.mock.calls[callIndex]; + const headers = + input instanceof Request ? input.headers : new Headers(init?.headers); + return headers.get(name); + } + it('lists reports with organization context', async () => { const expectedPayload = [ { @@ -1157,6 +1166,20 @@ describe('SecurityPenetrationTestsService', () => { pipelineTesting: true, }), ); + // Deterministic idempotency key tied to the parent → Maced dedupes + // concurrent duplicate failure webhooks into one provider scan. + expect(getRequestHeader('Idempotency-Key')).toBe('retry:run_orig'); + // Deterministic billing reservation id (idempotent on sourceResourceId) + // so concurrent duplicates debit the allowance exactly once. + expect( + mockBillingEntitlementsService.tryConsumeIncludedUsageForProduct, + ).toHaveBeenCalledWith( + expect.objectContaining({ + organizationId: 'org_123', + productKey: 'pentest', + sourceResourceId: 'pending:retry:run_orig', + }), + ); // The user's briefing survives the round-trip (re-persisted for any // further retry). expect(mockedDb.securityPenetrationTestRun.upsert).toHaveBeenCalledWith( @@ -1175,6 +1198,43 @@ describe('SecurityPenetrationTestsService', () => { ); }); + it('preserves an explicit empty check selection on retry', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValueOnce({ + organizationId: 'org_123', + attemptNumber: 1, + rootRunId: 'run_orig', + scanParams: { targetUrl: 'https://app.example.com', checks: [] }, + }); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ id: 'run_retry', status: 'provisioning' }), + { status: 200 }, + ), + ); + + await service['maybeAutoRetry']('run_orig'); + + // Empty selection is sent through, not dropped (which would let the + // provider fall back to its default check set). + const createBody = await getRequestBody(); + expect(createBody.checks).toEqual([]); + }); + + it('does not send an idempotency key for user-initiated creates', async () => { + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ id: 'run_new', status: 'provisioning' }), + { status: 200 }, + ), + ); + + await service.createReport('org_123', { + targetUrl: 'https://app.example.com', + }); + + expect(getRequestHeader('Idempotency-Key')).toBeNull(); + }); + it('blocks auto-retry across the whole lineage when a run is cancelled', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValueOnce({ rootRunId: 'run_orig', @@ -1329,6 +1389,7 @@ describe('SecurityPenetrationTestsService', () => { mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ providerRunId: 'run_orig', attemptNumber: 1, + scanParams: { targetUrl: 'https://a.com' }, // retry-eligible }); const recent = new Date().toISOString(); fetchMock.mockResolvedValueOnce( @@ -1352,6 +1413,69 @@ describe('SecurityPenetrationTestsService', () => { expect(report.failedReason).toBeNull(); }); + it('reveals a legacy failed run (no scan params) immediately instead of masking', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ + organizationId: 'org_123', + rootRunId: 'run_legacy', + }); + // Pre-feature row: attempt 1 but no scanParams → not retry-eligible. + mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ + providerRunId: 'run_legacy', + attemptNumber: 1, + scanParams: null, + }); + const recent = new Date().toISOString(); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: 'run_legacy', + status: 'failed', + targetUrl: 'https://a.com', + createdAt: recent, + updatedAt: recent, + error: 'some failure', + }), + { status: 200 }, + ), + ); + + const report = await service.getReport('org_123', 'run_legacy'); + + expect(report.status).toBe('failed'); + }); + + it('reveals a failed run with malformed (non-null) scan params instead of masking', async () => { + mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ + organizationId: 'org_123', + rootRunId: 'run_bad', + }); + // Non-null but invalid params (no targetUrl) → fromScanParams rejects → + // not retry-eligible. + mockedDb.securityPenetrationTestRun.findFirst.mockResolvedValueOnce({ + providerRunId: 'run_bad', + attemptNumber: 1, + scanParams: { nonsense: true }, + }); + const recent = new Date().toISOString(); + fetchMock.mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: 'run_bad', + status: 'failed', + targetUrl: 'https://a.com', + createdAt: recent, + updatedAt: recent, + error: 'some failure', + }), + { status: 200 }, + ), + ); + + const report = await service.getReport('org_123', 'run_bad'); + + expect(report.status).toBe('failed'); + }); + it('reveals a clean, white-labeled failure once the lineage is exhausted', async () => { mockedDb.securityPenetrationTestRun.findUnique.mockResolvedValue({ organizationId: 'org_123', diff --git a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts index b5faac7bb6..bdd49010de 100644 --- a/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts +++ b/apps/api/src/security-penetration-tests/security-penetration-tests.service.ts @@ -285,7 +285,12 @@ export class SecurityPenetrationTestsService { ): Promise { const rows = await db.securityPenetrationTestRun.findMany({ where: { organizationId }, - select: { providerRunId: true, rootRunId: true, attemptNumber: true }, + select: { + providerRunId: true, + rootRunId: true, + attemptNumber: true, + scanParams: true, + }, }); if (rows.length === 0) { return []; @@ -295,7 +300,7 @@ export class SecurityPenetrationTestsService { // customer sees one entry per scan — retries never appear as separate rows. const activeByRoot = new Map< string, - { providerRunId: string; attemptNumber: number } + { providerRunId: string; attemptNumber: number; retryable: boolean } >(); for (const row of rows) { const root = row.rootRunId ?? row.providerRunId; @@ -304,6 +309,9 @@ export class SecurityPenetrationTestsService { activeByRoot.set(root, { providerRunId: row.providerRunId, attemptNumber: row.attemptNumber, + // Re-runnable only if the stored params actually validate (same check + // as the retry path), not merely non-null. + retryable: this.fromScanParams(row.scanParams) != null, }); } } @@ -324,6 +332,8 @@ export class SecurityPenetrationTestsService { this.collapseRun(report, { rootRunId, attemptNumber: active.attemptNumber, + retryEligible: + active.attemptNumber < MAX_ATTEMPTS && active.retryable, }), ); } @@ -344,7 +354,15 @@ export class SecurityPenetrationTestsService { organizationId, payload, ); - const billingUsageSourceId = `pending:${randomUUID()}`; + // For a retry, derive a DETERMINISTIC reservation id from the parent run so + // concurrent duplicate failure webhooks reserve the same allowance. Billing + // consumption is idempotent on this id (billingUsageEvent.idempotencyKey), + // so duplicates debit exactly once — otherwise two random ids would debit + // twice and only one would land on the shared ownership row, orphaning the + // other. User-initiated creates keep a unique random id. + const billingUsageSourceId = lineage.retryOfProviderRunId + ? `pending:retry:${lineage.retryOfProviderRunId}` + : `pending:${randomUUID()}`; let consumedSubscriptionAllowance = false; // Reserve subscription allowance before calling Maced so fast double-clicks @@ -436,10 +454,22 @@ export class SecurityPenetrationTestsService { }, }; + // For an auto-retry, use a deterministic idempotency key tied to the parent + // run so concurrent duplicate `pentest.failed` webhooks dedupe AT THE + // PROVIDER — both create calls return the same run instead of launching two + // scans (one of which would be orphaned). User-initiated creates pass none. + const idempotencyKey = lineage.retryOfProviderRunId + ? `retry:${lineage.retryOfProviderRunId}` + : undefined; + let createdReport: PentestCreated; try { createdReport = await this.callMaced( - () => this.macedClient.pentests.create(body), + () => + this.macedClient.pentests.create( + body, + idempotencyKey ? { idempotencyKey } : undefined, + ), 'creating penetration test', ); } catch (error) { @@ -586,14 +616,18 @@ export class SecurityPenetrationTestsService { id: string, ): Promise<{ run: SecurityPenetrationTest; activeProviderRunId: string }> { await this.assertRunOwnership(organizationId, id); - const { rootRunId, activeProviderRunId, attemptNumber } = + const { rootRunId, activeProviderRunId, attemptNumber, retryEligible } = await this.resolveActiveAttempt(organizationId, id); const report = await this.callMaced( () => this.macedClient.pentests.get(activeProviderRunId), `fetching penetration test ${activeProviderRunId}`, ); return { - run: this.collapseRun(report, { rootRunId, attemptNumber }), + run: this.collapseRun(report, { + rootRunId, + attemptNumber, + retryEligible, + }), activeProviderRunId, }; } @@ -1199,7 +1233,7 @@ export class SecurityPenetrationTestsService { */ private collapseRun( report: Pentest | PentestWithProgress, - ctx: { rootRunId: string; attemptNumber: number }, + ctx: { rootRunId: string; attemptNumber: number; retryEligible: boolean }, ): SecurityPenetrationTest { const mapped = this.mapMacedRunToSecurityPenetrationTest(report); const activeStatus: PentestRunStatus = mapped.status; @@ -1210,6 +1244,7 @@ export class SecurityPenetrationTestsService { const status = collapsedStatus({ activeStatus, attemptNumber: ctx.attemptNumber, + retryEligible: ctx.retryEligible, failedAtMs: Number.isNaN(parsedFailedAt) ? null : parsedFailedAt, nowMs: Date.now(), }); @@ -1301,14 +1336,14 @@ export class SecurityPenetrationTestsService { dto.evidenceLevel = raw.evidenceLevel; } if (Array.isArray(raw.checks)) { - const checks = raw.checks.filter((check): check is PentestCheck => + // Preserve the original selection faithfully: assign the validated array + // whenever checks were stored, including an explicit empty selection + // (`[]`) — omitting it would let the provider fall back to its default + // check set. Stale entries (e.g. a check enum value removed between + // deploys) are filtered out rather than dropping the whole selection. + dto.checks = raw.checks.filter((check): check is PentestCheck => this.isPentestCheck(check), ); - // Keep whatever survived validation rather than dropping the whole - // selection if a single entry is stale (e.g. a check enum value removed - // between deploys) — a retry with the valid subset beats silently - // running the full default check set. - if (checks.length > 0) dto.checks = checks; } if (typeof raw.additionalContext === 'string') { dto.additionalContext = raw.additionalContext; @@ -1601,6 +1636,7 @@ export class SecurityPenetrationTestsService { rootRunId: string; activeProviderRunId: string; attemptNumber: number; + retryEligible: boolean; }> { const row = await db.securityPenetrationTestRun.findUnique({ where: { providerRunId: requestedId }, @@ -1610,12 +1646,20 @@ export class SecurityPenetrationTestsService { const active = await db.securityPenetrationTestRun.findFirst({ where: { organizationId, rootRunId }, orderBy: { attemptNumber: 'desc' }, - select: { providerRunId: true, attemptNumber: true }, + select: { providerRunId: true, attemptNumber: true, scanParams: true }, }); + const attemptNumber = active?.attemptNumber ?? 1; return { rootRunId, activeProviderRunId: active?.providerRunId ?? requestedId, - attemptNumber: active?.attemptNumber ?? 1, + attemptNumber, + // A failure can only be masked as in-progress if a retry could actually + // happen: under the attempt cap AND re-runnable. Reuse the same params + // validation `maybeAutoRetry` uses, so malformed (not just null) scan + // params are treated as ineligible and revealed immediately. + retryEligible: + attemptNumber < MAX_ATTEMPTS && + this.fromScanParams(active?.scanParams) != null, }; }