fix(dns): make the Cloudflare DNS provider boot, validate, and delete safely (#37) - #539
Closed
Hydralerne wants to merge 5 commits into
Closed
fix(dns): make the Cloudflare DNS provider boot, validate, and delete safely (#37)#539Hydralerne wants to merge 5 commits into
Hydralerne wants to merge 5 commits into
Conversation
…rd certs, and tunnels (#37)
… safely The DNS provider work needed three kinds of fix before it could ship: the control plane did not start, the two write endpoints rejected every request, and removing a domain could delete DNS records Openship never created. Boot and validation - packages/db/src/repos/index.ts referenced createDnsCredentialRepo from a re-export block, which creates no local binding — importing @repo/db threw ReferenceError, so nothing that touches the DB started. Added the import. - Added migration 0098_dns_credential (+ journal entry). The table had a schema but no SQL, and runtime DDL here is journal-driven only, so every /api/dns call hit "relation does not exist". - Ported dns.schema.ts from Zod to TypeBox. secureRouter feeds `body` to tbValidator, which throws ValueCheckUnknownTypeError on a Zod object — both POST routes 500'd on valid input. Bounded name, token and hostname while there. - Replaced the org:read / org:admin tags with settings:read / settings:admin. `org` is not a root resource, so parsePermissionTag threw at route registration — a second, independent boot failure. - domain.service no longer imports the ../dns barrel. That barrel re-exported dns.routes, so importing a service mounted the HTTP route table and pulled the auth graph into every consumer; 83 test files failed on it. - addDomain now states its return type. Adding a field to one of its two returns silently dropped preexistingEdgeSite from the other, breaking the edge-takeover notice in the controller. Record safety - Deletion is gated on the "Managed by Openship" comment marker. Matching on name alone meant that for an apex domain — where the record name IS the zone apex — removing a domain also deleted the operator's MX, SPF TXT and CAA. - Moved cleanup above the service-scoped early return, which it sat below, so per-service domains no longer leak their records forever. - upsertRecord preserves existing Cloudflare proxying instead of resetting it, compares against the effective TTL, and refuses (409) to rewrite a multi-record set it does not own rather than half-rewriting a round-robin. - findZone propagates 429/5xx instead of reporting "no zone". The lookup now answers matched | none | unauthorized | unavailable, so "we could not ask" stops being reported as "this domain is not yours". - provisionRecords attempts every record and reports per-record outcomes; a partial write no longer looks identical to nothing happening. Dropped the t=0 verify: the row stays pending for the existing domains:verify-pending sweep instead of burning a Let's Encrypt validation failure on records that have not propagated. - listRecords paginates; the zone walk is bounded. Secrets, audit, surface - No read path decrypts. tokenMasked is the constant ENV_MASK, never a token prefix, and a rotated key surfaces as status:"invalid" instead of 500ing the list. Added dns_credential.connected/.disconnected audit events carrying the label and provider only. - Added the Settings -> DNS tab (connect, remove, masked status, zone check) with strings in all nine locales, and docs/api/dns.mdx. - Reverted the tunnel rewrite and the DNS-01 certbot plumbing to main. Both were unreachable: nothing passed the challenge hooks and assertValidDomain rejects a wildcard before the branch could run, while the tunnel provider's preflight claimed success without probing for cloudflared and discarded the token its named-tunnel path needed. They belong in their own change. Resolves #37.
Three adjacent-insertion conflicts, no semantic disagreement: - SettingsSidebar.tsx: main reworked the `email` tab into a productView-aware object while this branch inserted the `dns` tab at the same line. Both kept. - i18n/locales/en/settings.json: main added `systemSender`, this branch added `dns` — adjacent keys. Both kept. - drizzle/meta/_journal.json: both claimed idx 98. main keeps 0098-0101. The DNS migration is renumbered 0098 -> 0103, not 0102: 0102 is taken by project_workload_axes, which lands first. `when` is 1787869307325, one slot after that migration's 1787782907325 — drizzle's migrator gates on `created_at < folderMillis` (pg-core/dialect.cjs) and ignores `idx`, so the timestamp is what has to be ordered, and the idx gap at 102 is filled when project_workload_axes merges. Verified on the merged tree: tsc --noEmit clean for apps/api, apps/dashboard, packages/core and packages/db; i18n drift byte-identical to main (no new missing keys); every journal tag resolves to a .sql file and back.
One conflict: drizzle/meta/_journal.json, where main's 0102_project_workload_axes and this branch's DNS migration both appended after 0101. Both kept, in `when` order — 0102 @1787782907325 then 0103 @1787869307325 — which also closes the idx gap this branch was holding open for it. The migrator gates on `created_at < folderMillis` and never reads idx (drizzle-orm pg-core/dialect.cjs), so the timestamps are what had to be ordered: 0102 applies, then 0103. Merging in the other order would have left 0102 with a timestamp below the last applied one and silently skipped it.
…imed hostnames Three defects found reviewing the branch against main, plus the follow-ups worth landing in the same pass. upsertRecord adopted the operator's record. With exactly one pre-existing record at a name+type, `target` was `existing[0]` regardless of ownership, and the PUT always stamped OPENSHIP_RECORD_COMMENT — so an unmarked record went in `isOpenshipManaged === false` and came out `true`, and releaseRecords hard-deleted it on domain removal. Connecting an apex that already had an A record therefore destroyed that record on the way out, with no copy of its original value kept. Repointing is the feature and stays; claiming is not. An adopted record now keeps whatever comment it already had, so it is never ours to delete. The >=2 branch already refused correctly. This also makes two statements the branch ships true rather than aspirational (types.ts rule 1, docs/api/dns.mdx). www records were written for a hostname another project owns. addWwwSibling swallows its failure — including the cross-project "already in use" conflict — and returns `wwwError`, but buildRecords was still fed `!!data.includeWww`, so provisioning wrote `www.<apex>` at this project's box anyway. removeDomain releases only the apex and its challenge name, so that record was orphaned with nothing on this side able to take it back. Provisioning now filters to the hostnames the call actually claimed; the display panel still lists the sibling's record, which is what includeWww reaches buildRecords for. The two write routes had no requireRole. `settings:admin` is a token scope, not an org role, so any member could connect or delete the org-wide credential and silently revert every later domain add to manual records. Added requireRole("admin"), per-route, matching the sidebar's requiresRole on this tab and the pairing every other admin route uses. Also: bound every Cloudflare call with a 15s abort (the deadline covers the body read, not just the handshake) — this runs inline on POST /domains, where an unbounded call outlived the operator's own client deadline while the server kept writing. Registered dns_credential in dump.ts TABLES *and* ENCRYPTED_COLUMNS plus secret-registry together: instance export carried zero credentials without the first, and cataloguing without the second would let the restore-side null pass skip api_token_enc. Dropped ZoneNotFoundError, which nothing threw, and its documented-but-unreachable 404 row. Verified: tsc --noEmit clean for apps/api, apps/dashboard, packages/core and packages/db; full monorepo suite 7/7 tasks, 0 failures. 5 new regression tests (58 DNS tests, up from 54) covering the adopted-record, comment-less-record and abort-signal paths, and one asserting www is listed but not written.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #528, keeping its commit so @thisisrakshith's work stays attributed. Resolves #37.
The first commit is #528 unchanged. The second is the fix pass — reviewing that commit on its own shows exactly what changed and why.
Why this wasn't mergeable as-is
Three independent problems, each verified by running the branch rather than reading it:
packages/db/src/repos/index.ts:322usedcreateDnsCredentialRepo, but the symbol was only added to anexport { … } fromblock — a re-export creates no local binding. Importing@repo/dbthrewReferenceError, so nothing that touches the DB booted.secureRouterfeedsbodyto TypeBox'stbValidator; the schemas were Zod. A fully validPOST /api/dns/credentialsreturned 500 (ValueCheckUnknownTypeError). Separately,org:read/org:adminare not valid tags —orgisn't a root resource, soparsePermissionTagthrew at route registration, a second boot failure.MX, SPFTXTandCAAlive — so removing a domain from Openship could take the org's mail with it.There was also no SQL migration for
dns_credential(runtime DDL here is journal-driven only, so every endpoint would 500 onrelation does not exist), and the../dnsbarrel re-exporteddns.routes, which meant importing a service mounted the HTTP route table — that alone broke 83 test files.What changed
Boot + validation — added the missing import; added
0098_dns_credential.sqland its journal entry (hand-written, matching every migration since 0061; verified against PGlite: 9 columns, 3 indexes, FK CASCADE); ported the schemas to TypeBox with boundedname/apiToken/hostname; moved tosettings:read/settings:admin; fixedbuildBackgroundContext, which was called with two positional args instead of one options object; stopped the barrel leaking routes into services; and gaveaddDomainan explicit return type — adding a field to one of its two returns had silently droppedpreexistingEdgeSitefrom the other, breaking the edge-takeover notice.Record safety — deletion is gated on a
Managed by Openshipprovider-side comment, so we only ever remove records we wrote. Cleanup moved above the service-scoped early return it was sitting below, so per-service domains stop leaking records.upsertRecordpreserves your Cloudflare proxying instead of resetting it, compares against the effective TTL, and returns 409 rather than half-rewriting a round-robin A set it doesn't own.Honest failure modes —
findZonepropagates 429/5xx instead of reporting "no zone", and the lookup answersmatched | none | unauthorized | unavailable, so "we couldn't ask" stops reading as "this domain isn't yours".provisionRecordsattempts every record and reports per-record outcomes, so a partial write isn't indistinguishable from nothing happening. Thet=0verify is gone: the row stays pending for the existingdomains:verify-pendingsweep instead of burning a Let's Encrypt validation failure on records that haven't propagated.listRecordspaginates; the zone walk is bounded.Secrets + audit — no read path decrypts.
tokenMaskedis the constantENV_MASK, never a token prefix, and a rotatedBETTER_AUTH_SECRETsurfaces asstatus: "invalid"instead of 500ing the list. Addeddns_credential.connected/.disconnectedaudit events carrying the label and provider only.Surface — added the Settings → DNS tab (connect, remove, masked status, zone check) with strings in all nine locales (parity ratchet stays green), 54 DNS tests in place of the original 12, and
docs/api/dns.mdxregistered inmeta.jsonsince every other API module has a page.Deliberately out of scope
The tunnel rewrite and the DNS-01 / wildcard certbot plumbing are reverted byte-identical to
main. Both were unreachable rather than incomplete: nothing anywhere passedchallenge/dnsAuthHook, andassertValidDomainrejects a*.name before that branch can run; the tunnel provider'spreflightreturnedok: truewithout probing forcloudflared, discarded the token its named-tunnel path needed, and emittederroron an EventEmitter with no listener attached. Each deserves its own PR now that the provider abstraction exists to build on.Verification
apps/apitypecheckapps/dashboardtypecheckReferenceErroron importPOST /api/dns/credentialsorigin/mainwas confirmed green at the same commit first, so the before/after numbers are attributable to the branch and not the environment.Before merging
origin/mainis at0097, so the migration is0098. If another migration lands first, this needs renumbering.