docs(24): mark code review findings fixed (commit e057255)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
e057255f4f
commit
3465192ab5
1 changed files with 294 additions and 0 deletions
|
|
@ -0,0 +1,294 @@
|
|||
---
|
||||
phase: 24-aws-route-53-dns-sync-track-changes-crud-operations-full-aud
|
||||
reviewed: 2026-08-06T03:01:44Z
|
||||
depth: standard
|
||||
files_reviewed: 27
|
||||
files_reviewed_list:
|
||||
- app/admin/sync/page.tsx
|
||||
- app/admin/sync/route53/page.tsx
|
||||
- app/api/route53/sync/route.ts
|
||||
- app/api/route53/zones/route.ts
|
||||
- app/api/route53/zones/[zoneId]/records/[recordId]/history/route.ts
|
||||
- app/api/route53/zones/[zoneId]/records/[recordId]/route.ts
|
||||
- app/api/route53/zones/[zoneId]/records/route.ts
|
||||
- CLAUDE.md
|
||||
- components/admin/route53/record-editor-dialog.tsx
|
||||
- lib/services/integration-health.ts
|
||||
- lib/services/route53-change-submit.test.ts
|
||||
- lib/services/route53-change-submit.ts
|
||||
- lib/services/route53-dns-delegation.test.ts
|
||||
- lib/services/route53-dns-delegation.ts
|
||||
- lib/services/route53-factory.test.ts
|
||||
- lib/services/route53-factory.ts
|
||||
- lib/services/route53-record-key.test.ts
|
||||
- lib/services/route53-record-key.ts
|
||||
- lib/services/route53-record-validation.test.ts
|
||||
- lib/services/route53-record-validation.ts
|
||||
- lib/services/route53-sync-service.test.ts
|
||||
- lib/services/route53-sync-service.ts
|
||||
- lib/services/route53-write-persistence.test.ts
|
||||
- lib/services/route53-write-persistence.ts
|
||||
- lib/services/sync-scheduler.ts
|
||||
- lib/types/route53.ts
|
||||
- migrations/102_route53_tables.sql
|
||||
- public/logos/route53.svg
|
||||
findings:
|
||||
critical: 2
|
||||
warning: 4
|
||||
info: 1
|
||||
total: 7
|
||||
status: fixed
|
||||
fixed_commit: e057255
|
||||
fixes_applied: [CR-01, tombstone-empty-array-guard, WR-02-health-check-timeout, IN-01]
|
||||
backlog_deferred: [WR-03-alias-records, WR-04-audit-log-ui]
|
||||
---
|
||||
|
||||
# Phase 24: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-08-06T03:01:44Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 27 (package.json / package-lock.json excluded per scope rules — dependency bump only, no logic)
|
||||
**Status:** fixed (commit `e057255`)
|
||||
|
||||
**Post-review action:** CR-01, the tombstone empty-array guard, WR-02 (health-check timeout),
|
||||
and IN-01 (`buildRecordKey` normalization) were all fixed and deployed. WR-03 (alias records)
|
||||
and WR-04 (no audit-log admin UI) were deliberately left as backlog items — see notes inline
|
||||
below each finding.
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the full AWS Route 53 DNS sync/CRUD/audit implementation: factory, sync service,
|
||||
change-submit + validation + write-persistence libraries, NS-delegation health check, four
|
||||
API route files, the admin detail page, and the record editor dialog, against the phase's
|
||||
CONTEXT/RESEARCH decisions (D-01 through D-12) and this codebase's established sibling
|
||||
patterns (PAX8, Veeam, IT Glue write-back).
|
||||
|
||||
The architecture and defense-in-depth discipline are generally strong — the pending →
|
||||
committed/failed audit lifecycle, TXT quoting (already fixed per the known checkpoint bug),
|
||||
error sanitization, D-01 allowlist enforcement at three independent layers, and the bounded
|
||||
DNS-resolver/GetChange polling are all implemented carefully and match the codebase's own
|
||||
sibling conventions.
|
||||
|
||||
Two issues rise to Critical: (1) the record **update (PATCH)** route never verifies the
|
||||
submitted `name`/`type` actually match the record identified by the URL's `recordId` before
|
||||
submitting an AWS `UPSERT` — the client UI disables those fields, but the server has no
|
||||
matching check, so a direct API call (or a future UI bug) can silently create an orphaned
|
||||
duplicate DNS record in AWS while leaving the record the caller thought they were editing
|
||||
untouched, and corrupt the Postgres mirror's `record_key` invariant. (2) the sync service's
|
||||
zone/record tombstone queries use `<> ALL($1)` with no guard for an empty result set — this
|
||||
exact class of bug was already found and explicitly fixed in this repo's own
|
||||
`pax8-sync-service.ts` (`seen.length === 0 ? 0 : ...`), and the Route 53 sync service does not
|
||||
carry the same guard, so a transient empty AWS response would soft-delete every previously
|
||||
synced zone/record.
|
||||
|
||||
Four Warnings and one Info round out the rest: the D-12 health check's AWS auth probe has no
|
||||
timeout (unlike every other integration's `liveCheck()`), alias-type A/AAAA records can never
|
||||
pass `validateRecordWrite`'s non-empty-`resourceRecords` rule (so they're silently
|
||||
un-editable/un-deletable from Pulse with a generic 400), the `route53_audit_log` table
|
||||
(the "failures too" ledger required by D-07/SC-3) has no admin-UI surface at all — the
|
||||
checkpoint verified it via raw `psql`, not the page — and `buildRecordKey` relies on callers
|
||||
having already case-normalized `name`/`type` rather than enforcing it itself.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: PATCH does not verify the request body's name/type match the record being edited — can silently duplicate a live DNS record and corrupt the mirror
|
||||
|
||||
**File:** `app/api/route53/zones/[zoneId]/records/[recordId]/route.ts:73-146`
|
||||
**Issue:**
|
||||
`recordId` (the record's stable identity, `${zoneId}:${name}:${type}:${setIdentifier}`) is
|
||||
decoded into `recordKey` and used only for a zone-prefix check (`recordKeyMatchesZone`) and to
|
||||
load the "before" mirror row. The actual AWS submission is built entirely from the **request
|
||||
body**:
|
||||
|
||||
```ts
|
||||
const { name, type, ttl, resourceRecords } = validated.value; // from body, not from `existing`
|
||||
...
|
||||
const { changeId, awsResponse } = await submitRecordChange({
|
||||
zoneId,
|
||||
action: 'UPSERT',
|
||||
recordSet: { name, type, ttl, resourceRecords, setIdentifier },
|
||||
});
|
||||
...
|
||||
await upsertMirrorRecord({
|
||||
recordKey, // the OLD key from the URL
|
||||
zoneId,
|
||||
name, // the NEW name from the body
|
||||
type, // the NEW type from the body
|
||||
...
|
||||
});
|
||||
```
|
||||
|
||||
Route 53 identifies a resource record set by `Name`+`Type`+`SetIdentifier`, not by any
|
||||
Pulse-internal id. If the body's `name`/`type` differ from the record `recordId` actually
|
||||
denotes, `ChangeResourceRecordSetsCommand` with `Action: 'UPSERT'` **creates a brand-new
|
||||
record set** at AWS — the original record (matching `recordId`) is never touched, so it stays
|
||||
live in DNS unmodified. `upsertMirrorRecord` then writes the *new* name/type into the row keyed
|
||||
by the *old* `record_key`, breaking the invariant (documented in `migrations/102_route53_
|
||||
tables.sql`) that `record_key = zoneId:name:type:setIdentifier`.
|
||||
|
||||
The UI (`components/admin/route53/record-editor-dialog.tsx`) disables the name/type fields in
|
||||
edit mode and states "renaming a record set is a delete-plus-create, not an update" — but this
|
||||
is UI-only. The same codebase's own doc comments elsewhere in this phase explicitly call out
|
||||
"never rely on the UI hiding a control" (T-24-02) as the reason CRUD is re-validated
|
||||
server-side; this exact principle isn't applied here. Any admin session (already gated by
|
||||
`requireAdmin()`, so this isn't a privilege-escalation bug, but it is a correctness/data-
|
||||
integrity bug with live-DNS blast radius) hitting the API directly — or a future UI bug that
|
||||
re-enables the fields — triggers a duplicate/orphaned live DNS record plus a corrupted mirror
|
||||
row, both invisible until the next scheduled sync silently "fixes" the mirror (but not the
|
||||
orphaned AWS record, which just sits there as stray DNS).
|
||||
|
||||
**Fix:**
|
||||
```ts
|
||||
const existing = await loadMirrorRecord(recordKey);
|
||||
if (!existing) {
|
||||
return NextResponse.json({ error: 'Record not found' }, { status: 404 });
|
||||
}
|
||||
|
||||
// NEW: name/type/setIdentifier are immutable via PATCH — reject any attempt to
|
||||
// change them instead of silently UPSERTing a different record set.
|
||||
const normalizedSetId = setIdentifier ?? null;
|
||||
if (
|
||||
name !== existing.name ||
|
||||
type !== existing.type ||
|
||||
normalizedSetId !== existing.setIdentifier
|
||||
) {
|
||||
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 }
|
||||
);
|
||||
}
|
||||
```
|
||||
(Apply the same guard before constructing `afterValue`/calling `submitRecordChange`.)
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Route 53 sync tombstones every zone/record on an empty AWS response — same bug class already fixed elsewhere in this repo
|
||||
|
||||
**File:** `lib/services/route53-sync-service.ts:282-289` (zones), `lib/services/route53-sync-service.ts:398-404` (records)
|
||||
**Issue:**
|
||||
```ts
|
||||
// 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]
|
||||
);
|
||||
```
|
||||
If `seenIds` (or `seenKeys` in `syncRecords`) is `[]` — e.g. AWS's `ListHostedZonesCommand`/
|
||||
`ListResourceRecordSetsCommand` returns a successful-but-empty page due to a transient API
|
||||
quirk, rather than throwing — Postgres's `id <> ALL('{}')` is vacuously true for every row, so
|
||||
**every previously synced zone (and, in the records case, every record in that zone) gets
|
||||
soft-deleted** in one query. This exact class of bug was already discovered and explicitly
|
||||
guarded against in this same codebase's `lib/services/pax8-sync-service.ts`:
|
||||
```ts
|
||||
const tombstoned = seen.length === 0
|
||||
? 0
|
||||
: (await postgresClient.query(`UPDATE ... WHERE id <> ALL($1::uuid[])`, [seen])).rowCount ?? 0;
|
||||
```
|
||||
Route 53's sync service does not carry the same guard, and the sync-service test file
|
||||
(`route53-sync-service.test.ts`) only exercises the pure `buildDriftHistoryRows` helper — it
|
||||
never exercises `syncZones()`/`syncRecords()`, so this gap isn't caught by the test suite.
|
||||
**Fix:**
|
||||
```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]
|
||||
);
|
||||
}
|
||||
```
|
||||
Apply the same `seenKeys.length > 0` guard in `syncRecords()`.
|
||||
|
||||
### WR-02: Route 53 health check's AWS auth probe has no timeout, unlike every other integration
|
||||
|
||||
**File:** `lib/services/integration-health.ts:234-240`
|
||||
**Issue:** Every other integration's health check goes through `liveCheck()`, which wraps its
|
||||
`fetch()` in an `AbortController` with an 8-second timeout. `checkRoute53()` instead calls the
|
||||
AWS SDK directly with no bound:
|
||||
```ts
|
||||
await getRoute53Client().send(new ListHostedZonesCommand({ MaxItems: 1 }));
|
||||
```
|
||||
The AWS SDK v3 client has its own internal retry policy (multiple attempts) but no
|
||||
caller-supplied deadline here. `checkIntegrationHealth()` fans out via `Promise.all` across
|
||||
every integration (`lib/services/integration-health.ts:428`); a slow/hanging Route 53 call
|
||||
extends the whole aggregate's latency with no cap. (24-07-SUMMARY.md's checkpoint noted the
|
||||
whole `/admin/integrations` page felt slow and attributed it to "some other" integration's
|
||||
`liveCheck()` lacking a timeout — worth re-checking against this codepath specifically, since
|
||||
this one is also unbounded and wasn't a `liveCheck()` caller to begin with.)
|
||||
**Fix:** Pass an `AbortSignal`/timeout through to the SDK command (v3 commands accept
|
||||
`abortSignal` in `send()`'s options), or wrap the call in the same `Promise.race`-with-timeout
|
||||
pattern used elsewhere in this file.
|
||||
|
||||
### WR-03: Alias-type A/AAAA records can never pass validation — silently un-editable/un-deletable with a generic error
|
||||
|
||||
**File:** `lib/services/route53-record-validation.ts:112-117`, exercised via `app/api/route53/zones/[zoneId]/records/[recordId]/route.ts:196-204` (DELETE)
|
||||
**Issue:** `validateRecordWrite` requires `resourceRecords` to be a non-empty array:
|
||||
```ts
|
||||
if (!Array.isArray(input.resourceRecords) || input.resourceRecords.length === 0) {
|
||||
return fail('resourceRecords must be a non-empty array');
|
||||
}
|
||||
```
|
||||
An AWS Route 53 alias record (common for ALB/CloudFront/S3-website targets on A/AAAA types)
|
||||
has no `ResourceRecords` at all — only an `AliasTarget` — and `normalizeRecordSet` correctly
|
||||
reflects that (`resourceRecords: []`). The DELETE handler re-runs `validateRecordWrite` against
|
||||
the *existing* mirror row before submitting the delete:
|
||||
```ts
|
||||
const validated = validateRecordWrite({
|
||||
name: existing.name, type: existing.type,
|
||||
ttl: existing.ttl ?? undefined, resourceRecords: existing.resourceRecords ?? [],
|
||||
});
|
||||
if (!validated.ok) return 400; // always hits this branch for an alias record
|
||||
```
|
||||
Any synced alias A/AAAA record can therefore never be deleted (or edited — the same shape
|
||||
issue applies via the PATCH/POST path once a user tries to submit an empty `resourceRecords`
|
||||
array) through Pulse; it always returns a generic `400 resourceRecords must be a non-empty
|
||||
array`, with no indication to the admin that the underlying reason is "this is an alias
|
||||
record, which Pulse doesn't support editing." D-01/RESEARCH.md never discuss alias records at
|
||||
all, so this looks like an unconsidered gap rather than an intentional exclusion.
|
||||
**Fix:** Either (a) explicitly detect and reject alias records earlier with a clear message
|
||||
("Alias records are not supported for CRUD from Pulse — manage in the AWS console"), or (b)
|
||||
extend `validateRecordWrite`/the CRUD routes to handle `aliasTarget`-only recordsets if alias
|
||||
support is actually wanted. At minimum, surface a specific error message instead of the current
|
||||
generic one.
|
||||
|
||||
### WR-04: `route53_audit_log` (the D-07 "failures too" ledger) has no admin-UI surface
|
||||
|
||||
**File:** `app/admin/sync/route53/page.tsx` (Zones/Records/History/Schedule tabs), `lib/services/route53-write-persistence.ts`
|
||||
**Issue:** D-07/SC-3 require every sync/CRUD *attempt*, including failures, to be logged and
|
||||
auditable. `route53_audit_log` faithfully captures this (status `pending`/`committed`/`failed`,
|
||||
sanitized `error_message`, before/after). However, the only history-facing UI on
|
||||
`/admin/sync/route53` is the **History** tab, which reads `route53_record_history` — the
|
||||
resolved-changes ledger, not the attempt ledger. There is no route or tab that lists
|
||||
`route53_audit_log` rows (including failed attempts) anywhere in this phase's UI. The 24-07
|
||||
checkpoint verified audit completeness by querying Postgres directly with `psql`, not through
|
||||
the product — which means, in production, an operator has no way to see "who tried to change
|
||||
this record and it failed" without shelling into the database. This meaningfully undercuts the
|
||||
practical value of a phase whose stated goal is "full audit."
|
||||
**Fix:** Add a lightweight "Attempts" or "Audit Log" tab (or fold failed attempts into the
|
||||
existing History tab with a distinct visual treatment) backed by a new
|
||||
`GET /api/route53/audit-log` (or per-record) route reading `route53_audit_log`.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `buildRecordKey` doesn't normalize its own inputs — relies on every caller to pre-normalize case
|
||||
|
||||
**File:** `lib/services/route53-record-key.ts:28-35`
|
||||
**Issue:** `buildRecordKey` concatenates `name`/`type` verbatim without lower/upper-casing them
|
||||
itself; every current call site happens to pre-normalize (via `normalizeRecordSet` during sync,
|
||||
or via `validateRecordWrite`'s lowercasing/uppercasing during CRUD) before calling it, so this
|
||||
isn't causing a bug today — but it's a fragile invariant with no guard at the point where it
|
||||
would actually matter (a mismatched-case call would silently produce a different `record_key`
|
||||
than the canonical one, splitting a single AWS record across two mirror rows).
|
||||
**Fix:** Have `buildRecordKey` normalize `name`/`type` itself (lowercase name, uppercase type)
|
||||
so the invariant holds regardless of caller discipline, and drop the redundant normalization at
|
||||
call sites once that's in place.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-08-06T03:01:44Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue