From e057255f4f9caf03cd90f12401af3c3df50a076e Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 5 Aug 2026 23:15:08 -0400 Subject: [PATCH] =?UTF-8?q?fix(24):=20address=20code-review=20findings=20?= =?UTF-8?q?=E2=80=94=20PATCH=20identity=20guard,=20empty-array=20tombstone?= =?UTF-8?q?,=20health-check=20timeout,=20record-key=20normalization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two critical issues from the post-phase code review: - PATCH /api/route53/zones/[zoneId]/records/[recordId] never verified the request body's name/type/setIdentifier matched the record identified by the URL. A mismatch would silently UPSERT a brand-new AWS recordset (leaving the original live and untouched) while corrupting the mirror's record_key invariant. Now rejects with 400 if any of those three fields differ from the existing record — renaming/retyping is delete-plus-create, not an update. - route53-sync-service.ts's syncZones()/syncRecords() tombstone queries used "id <> ALL(seenIds)" style queries with no empty-array guard — a successful-but-empty AWS response would soft-delete every previously synced zone/record in one shot. Same bug class already fixed in pax8-sync-service.ts; now guarded the same way here. Two smaller fixes: - checkRoute53()'s AWS auth probe had no timeout, unlike every other integration's liveCheck() (8s AbortController). Added the same bound via the SDK's abortSignal option. - buildRecordKey() relied on every caller to pre-normalize name/type case before calling it. Now normalizes internally (lowercase name, uppercase type) so the record_key invariant holds regardless of caller discipline. Full REVIEW.md findings in 24-REVIEW.md. Two remaining Warnings (alias records un-editable/undeletable, no admin-UI surface for route53_audit_log) deliberately left as backlog items for a follow-up phase — out of scope for a post-execution fix pass. Co-Authored-By: Claude Sonnet 5 --- .../[zoneId]/records/[recordId]/route.ts | 20 +++++++++++ lib/services/integration-health.ts | 15 +++++++- lib/services/route53-record-key.ts | 9 ++++- lib/services/route53-sync-service.ts | 36 ++++++++++++------- 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/app/api/route53/zones/[zoneId]/records/[recordId]/route.ts b/app/api/route53/zones/[zoneId]/records/[recordId]/route.ts index b6f7942..84474c3 100644 --- a/app/api/route53/zones/[zoneId]/records/[recordId]/route.ts +++ b/app/api/route53/zones/[zoneId]/records/[recordId]/route.ts @@ -91,6 +91,26 @@ export async function PATCH( if (!existing) { return NextResponse.json({ error: 'Record not found' }, { status: 404 }); } + + // name/type/setIdentifier are immutable via PATCH — Route 53 identifies a + // recordset by Name+Type+SetIdentifier, not by any Pulse-internal id, so a + // mismatch here would UPSERT a brand-new AWS recordset (leaving the one + // `recordId` actually denotes untouched) while corrupting the mirror's + // record_key = zoneId:name:type:setIdentifier invariant. The UI disables + // these fields in edit mode, but per T-24-02 the server never relies on + // the UI hiding a control — reject any attempt to rename/retype instead of + // silently targeting a different record set. + const existingSetIdentifier = existing.setIdentifier ?? null; + if (name !== existing.name || type !== existing.type || setIdentifier !== existingSetIdentifier) { + return NextResponse.json( + { + error: 'Cannot change name/type/setIdentifier via update', + message: 'Renaming or retyping a record is a delete-plus-create, not an update.', + }, + { status: 400 } + ); + } + const beforeValue = { name: existing.name, type: existing.type, diff --git a/lib/services/integration-health.ts b/lib/services/integration-health.ts index 91ce43e..416ae6c 100644 --- a/lib/services/integration-health.ts +++ b/lib/services/integration-health.ts @@ -218,6 +218,7 @@ function isAwsAuthError(err: unknown): boolean { } const ROUTE53_ZONE_CHECK_LIMIT = 50; +const ROUTE53_AUTH_PROBE_TIMEOUT_MS = 8000; async function checkRoute53(): Promise { const checkedAt = new Date().toISOString(); @@ -232,7 +233,19 @@ async function checkRoute53(): Promise { let status: HealthStatus; let error: string | null = null; try { - await getRoute53Client().send(new ListHostedZonesCommand({ MaxItems: 1 })); + // Bounded the same way liveCheck() bounds every other integration's + // fetch() (8s) — the AWS SDK's own retry policy has no caller-supplied + // deadline, and checkIntegrationHealth() fans out via Promise.all, so an + // unbounded call here would extend the whole aggregate's latency. + const ctrl = new AbortController(); + const timeout = setTimeout(() => ctrl.abort(), ROUTE53_AUTH_PROBE_TIMEOUT_MS); + try { + await getRoute53Client().send(new ListHostedZonesCommand({ MaxItems: 1 }), { + abortSignal: ctrl.signal, + }); + } finally { + clearTimeout(timeout); + } status = 'ok'; } catch (err) { status = isAwsAuthError(err) ? 'auth_failed' : 'unreachable'; diff --git a/lib/services/route53-record-key.ts b/lib/services/route53-record-key.ts index 60c7dcb..a9c145c 100644 --- a/lib/services/route53-record-key.ts +++ b/lib/services/route53-record-key.ts @@ -24,6 +24,13 @@ export interface NormalizedRecordSet { /** * Build the `route53_records.record_key` primary key / `recordId` URL * segment: `${zoneId}:${name}:${type}:${setIdentifier ?? ''}`. + * + * Normalizes `name` (lowercase) and `type` (uppercase) itself rather than + * trusting every caller to pre-normalize — every current call site happens + * to normalize before calling this, but that's a fragile invariant with no + * guard at the point where it actually matters: a mismatched-case call would + * silently produce a different `record_key` than the canonical one, splitting + * a single AWS record across two mirror rows. */ export function buildRecordKey(input: { zoneId: string; @@ -31,7 +38,7 @@ export function buildRecordKey(input: { type: string; setIdentifier?: string | null; }): string { - return `${input.zoneId}:${input.name}:${input.type}:${input.setIdentifier ?? ''}`; + return `${input.zoneId}:${input.name.toLowerCase()}:${input.type.toUpperCase()}:${input.setIdentifier ?? ''}`; } /** diff --git a/lib/services/route53-sync-service.ts b/lib/services/route53-sync-service.ts index 60d4da5..d6e80dc 100644 --- a/lib/services/route53-sync-service.ts +++ b/lib/services/route53-sync-service.ts @@ -279,12 +279,19 @@ export class Route53SyncService { ); } - // Soft-delete zones no longer returned by AWS. - await postgresClient.query( - `UPDATE route53_zones SET is_deleted = true, deleted_at = NOW(), updated_at = NOW() - WHERE is_deleted = false AND id <> ALL($1)`, - [seenIds] - ); + // Soft-delete zones no longer returned by AWS. Guard against an empty + // seenIds — `id <> ALL('{}')` is vacuously true for every row, so a + // successful-but-empty AWS response (transient API quirk, not "the + // account has zero zones") would otherwise soft-delete every previously + // synced zone in one shot. Same bug class already fixed in + // pax8-sync-service.ts. + if (seenIds.length > 0) { + await postgresClient.query( + `UPDATE route53_zones SET is_deleted = true, deleted_at = NOW(), updated_at = NOW() + WHERE is_deleted = false AND id <> ALL($1)`, + [seenIds] + ); + } return seenIds.length; } @@ -396,12 +403,17 @@ export class Route53SyncService { } // Soft-delete this zone's records no longer present in AWS. Never - // hard-delete — the history ledger references record_key. - await postgresClient.query( - `UPDATE route53_records SET is_deleted = true, deleted_at = NOW(), updated_at = NOW() - WHERE zone_id = $1 AND is_deleted = false AND record_key <> ALL($2)`, - [zoneId, seenKeys] - ); + // hard-delete — the history ledger references record_key. Guard + // against an empty seenKeys for the same reason as syncZones() above — + // every real zone has at least apex NS/SOA records, so an empty page + // here is a transient AWS API quirk, not "this zone has zero records." + if (seenKeys.length > 0) { + await postgresClient.query( + `UPDATE route53_records SET is_deleted = true, deleted_at = NOW(), updated_at = NOW() + WHERE zone_id = $1 AND is_deleted = false AND record_key <> ALL($2)`, + [zoneId, seenKeys] + ); + } } return totalUpserted;