14 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 261008-dts-modul-domains-autodns-anbindung-kontakte | 2026-10-08T00:00:00Z | quick | 37 |
|
|
issues_found |
Quick 261008-dts: Code Review Report
Reviewed: 2026-10-08 Depth: quick (focus area 1, money safety, traced in full) Files Reviewed: 37 Status: issues_found
Summary
The core money-safety design holds up. The DRAFT -> SUBMITTING claim is an atomic updateMany that includes the environment in its where, and it happens before any network call. autodnsRequest has no retry and there is exactly one POST /domain. Timeouts and network errors end in UNKNOWN. The web client has no automatic retry and locks the confirm button after an error. Concurrent confirms, a second tab and a double click cannot produce two POSTs.
Secrets, tenant isolation, permission levels and route order are clean:
- Secrets: The password never leaves the settings service. Views only carry
hasPassword. No log line or error text contains headers or credentials. - Tenant isolation: Every Prisma
wherenamestenantId, RLS is on for all four tables, and the cache key includes tenant, environment and config version. - Permissions: Writes are
@ModuleManage, reads use the class-level@UseModule, and there is no@Roles. - Route order:
orders/refreshcomes beforeorders/:id/*.
One real hole remains in the "never guess" rule. A non-2xx answer that carries an envelope is treated as "AutoDNS rejected the order", even for 5xx (CR-01). Several smaller gaps around the discard and reconcile path make that hole easier to reach.
Fix Status (2026-10-08)
| ID | Status | Commit | Note |
|---|---|---|---|
| CR-01 | fixed | cc1c83a |
5xx, 408, 425 (with or without envelope) now classify as http; only 4xx with envelope, 401/403 and 2xx+ERROR are definitive. Submit: http, timeouts, network, tls and 429 all end in UNKNOWN (429 origin is not proven to be AutoDNS pre-processing). Reconcile treats non-business failures as inconclusive. Tests: client spec table, order spec for 408/425/429/500/502/503/504 on submit, reconcile and job search. Requires human verification (logic change). |
| WR-01 | fixed | cc1c83a |
cancelOrder re-runs the reconcile at discard time; only an explicit not-found discards. Found -> 409 orderFound, inconclusive -> 409 checkFirst, other system -> 409 environmentChanged. Requires human verification. |
| WR-02 | fixed | cc1c83a |
Summary carries version (draft updatedAt); submit DTO requires it, claim where includes updatedAt, mismatch -> 409 orderChanged (German message shown by the register dialog). Requires human verification. |
| WR-03 | fixed | cc1c83a |
Jobs for the domain without readable date, jobs without readable object and a full result page (10) without match are inconclusive: lastCheckedAt stays empty. Note: the object filter key is still unverified against the live API. |
| WR-04 | fixed | 268d6d5 |
@ValidateIf(v !== undefined) instead of @IsOptional(); context numbers stay nullable. DTO spec added. |
| WR-05 | fixed | 9ecf191 |
prices[0] fallback removed; no verifiable one-year entry -> no price. |
| WR-06 | fixed | 9eada2e, e1dd996 |
ModuleGuard stores request.moduleAccessLevel; ?refresh is ignored unless MANAGE. Refresh buttons hidden for non-managers. |
| IN-01 | skipped | - | Out of scope (needs grace-period design for openKey on SUCCESS). |
| IN-02 | skipped | - | Out of scope (needs a migration with a partial unique index). |
| IN-03 | fixed | cc1c83a |
Post-POST write wrapped: logs order id and job number, falls back to UNKNOWN (never DRAFT/FAILED), returns an UNKNOWN view instead of 500; if that fails too the row stays SUBMITTING and becomes UNKNOWN after 2 minutes. |
| IN-04 | skipped | - | Out of scope (needs a migration). |
Critical Issues
CR-01: HTTP 5xx with an AutoDNS envelope is classified as definitive rejection, so the order is FAILED and the domain key is freed (double-registration path)
File: apps/api/src/domains/autodns-client.ts:150-155, used at apps/api/src/domains/domains-orders.service.ts:546-556 and :718
Issue: failureKindForStatus returns 'business' for every non-2xx status other than 401, 403 and 429 whenever the body looks like an envelope. The envelope test is loose: 'messages' in parsed is enough. autodns-client.spec.ts:187 pins 500 + envelope -> business as intended.
outcomeOfSubmit maps business to FAILED with openKey: null. A 500, 502, 503, 504 or 408 with a JSON envelope therefore marks the order as not placed. That is the case the class header says must never be guessed. A gateway or backend that times out after the job was created, but still answers with an AutoDNS-style error envelope, will:
- save the order as FAILED and free
openKey, and - let the user (or a second user) create and submit a new order for the same domain, which is a second
POST /domainand a second charge.
The same classification weakens reconcile. Line 718 reads !domain.ok && domain.kind !== 'business' as "AutoDNS said: domain does not exist". A 500 with an envelope is then taken as "not found" and the order can move toward discardable.
Fix: Treat only unambiguous rejections as FAILED. That means HTTP 4xx other than 408 and 425 with an envelope, plus 2xx with status.type === 'ERROR'. Everything with httpStatus >= 500 becomes UNKNOWN. In parseAutodnsEnvelope, return 'http' for a non-2xx status that is 408, 425 or 5xx, regardless of the envelope:
function failureKindForStatus(httpStatus: number, hasEnvelope: boolean): AutodnsFailureKind {
if (httpStatus === 401) return 'auth';
if (httpStatus === 403) return 'forbidden';
if (httpStatus === 429) return 'rate-limit';
if (httpStatus >= 500 || httpStatus === 408 || httpStatus === 425) return 'http';
return hasEnvelope ? 'business' : 'http';
}
In outcomeOfSubmit, also keep rate-limit out of the definitive-FAILED group unless it is known to come from AutoDNS. Update autodns-client.spec.ts:187 ([500, 'business'] becomes [500, 'http']) and add an order-service test: submit answered with 500 + envelope must end in UNKNOWN with openKey kept.
Warnings
WR-01: "Discard" of an UNKNOWN order relies on a check of arbitrary age
File: apps/api/src/domains/domains-orders.service.ts:754-767, apps/web/src/components/domains/order-status.ts:53-55
Issue: After one reconcile that found nothing, lastCheckedAt is set. From then on the order is discardable forever. The web client also stops polling it, because isOpen is false once lastCheckedAt is set. Registrar jobs can be delayed, so a check that was true ten minutes ago may no longer be true. A later discard frees openKey, and a re-order then runs a second POST /domain while the first can still complete.
Fix: In cancelOrder, require a recent check. For example, reject with checkFirst when lastCheckedAt is older than 5 minutes. Better, run reconcile inside cancelOrder and discard only if it still finds nothing.
WR-02: Registration is claimed with a payload the user did not confirm (lost update on a shared DRAFT)
File: apps/api/src/domains/domains-orders.service.ts:386-393 and :466-488
Issue: createOrder for an existing DRAFT overwrites payload (contacts, name servers) with updateMany. submitOrder reads the payload first and claims afterwards, with no version check. If manager B re-runs "Show summary" for the same domain after manager A has seen the summary, A confirms and registers with B's owner contact and name servers. The same happens if B's update lands between A's read and A's claim.
Fix: Return order.updatedAt or a payload hash in the summary. The client sends it back on submit, and the claim becomes where: { id, tenantId, status: 'DRAFT', environment, updatedAt: before.updatedAt }. Alternatively refuse to overwrite a DRAFT that another user created.
WR-03: Job matching in reconcile can silently miss an existing job and then allow discard
File: apps/api/src/domains/domains-orders.service.ts:727-742, apps/api/src/domains/autodns-parse.ts:287-306
Issue: Candidates need j.object === row.domainName && j.created && created >= since. If AutoDNS returns a matching job with no readable created (the parser also falls back to started and added), or with the object in another form, the job is dropped. The result is "nothing found", lastCheckedAt is set, and the order becomes discardable although a job exists. The filters key object is also unverified against the live API.
Fix: Fail towards caution. If a job for the domain exists but cannot be dated, treat it as a match, or leave lastCheckedAt unset and do not make the order discardable.
WR-04: @IsOptional() lets null through, which causes 500 instead of 400
File: apps/api/src/domains/dto/domains-settings.dto.ts:25-69, apps/api/src/domains/domains-settings.service.ts:194-203
Issue: @IsOptional() skips validation for null as well as undefined. Sending {"demoUser": null}, {"defaultNameServers": null} or {"environment": null} passes validation. The service only checks !== undefined and then calls .trim() or .map() on null, which is a TypeError, or writes null into a non-null column. This needs a manager account, but it is a bug and not a validation response.
Fix: Use @ValidateIf((_o, v) => v !== undefined) in place of @IsOptional() for these fields. Alternatively check != null in the service.
WR-05: Price shown for one year can actually be another period's price
File: apps/api/src/domains/autodns-parse.ts:195-196
Issue: price = parsePriceEntry(oneYear) ?? parsePriceEntry(prices[0]). If no entry matches one year, the first entry is used and then displayed as the price for 1 year in the confirmation of a binding order. The fallback could be a multi-year or transfer price.
Fix: Drop the prices[0] fallback and return null. The UI already says "price unknown" for null.
WR-06: Any USE user can force full AutoDNS re-reads with ?refresh=1
File: apps/api/src/domains/domains.controller.ts:105-129, apps/api/src/domains/domains-directory.service.ts:160-197
Issue: GET contacts?refresh=1 and GET domains?refresh=1 skip the cache for read-only users. Each call is up to 20 sequential POSTs on the shared 3 requests/second limiter, in addition to any refresh cycle. A read-only user can starve order submission and reconcile, and since the limiter is shared per process, that affects the whole instance.
Fix: Honour refresh only for managers. Add @ModuleManage on a separate refresh route, or ignore the flag without MANAGE. A minimum interval per tenant (for example 10 seconds) also helps.
Info
IN-01: SUCCESS keeps openKey for good
File: apps/api/prisma/migrations/20261008120000_domains_autodns/migration.sql (header comment), apps/api/src/domains/domains-orders.service.ts:362
Issue: A registered domain that is later deleted or expires can never be ordered again through Tessera in that environment. The user sees orderOpen for good.
Fix: Clear openKey on SUCCESS after a grace period (for example 24 hours), or let createOrder ignore SUCCESS rows older than that.
IN-02: "Own company" is not enforced in the database
File: apps/api/src/domains/domains-directory.service.ts:286-311
Issue: Two concurrent createCustomer calls with isOwnCompany: true can both succeed, leaving two own companies. The register tab then picks the first one.
Fix: Add a partial unique index: CREATE UNIQUE INDEX ... ON "DomainsCustomer"("tenantId") WHERE "isOwnCompany".
IN-03: Failure of the final status write is not handled
File: apps/api/src/domains/domains-orders.service.ts:522-533
Issue: If the database write after POST /domain throws, the user gets a 500 although the order may have been placed. The state is still safe (SUBMITTING becomes UNKNOWN after 2 minutes). The error is neither logged with the order id nor turned into a clear UNKNOWN answer.
Fix: Wrap the write, log the order id, and return an UNKNOWN view or a dedicated error code. The web client already locks the button.
IN-04: Redundant index
File: apps/api/prisma/migrations/20261008120000_domains_autodns/migration.sql (DomainsConfig_tenantId_idx)
Issue: DomainsConfig_tenantId_key already covers tenantId, so the extra index is redundant.
Fix: Remove @@index([tenantId]) on DomainsConfig in a later migration.
Reviewed: 2026-10-08 Reviewer: Claude (gsd-code-reviewer) Depth: quick (money-safety path traced in full)