Skip to content

feat: add Cloudflare domain registration CLI#167

Merged
Fermionic-Lyu merged 5 commits into
mainfrom
codex/cloudflare-domains
Jun 16, 2026
Merged

feat: add Cloudflare domain registration CLI#167
Fermionic-Lyu merged 5 commits into
mainfrom
codex/cloudflare-domains

Conversation

@Fermionic-Lyu

@Fermionic-Lyu Fermionic-Lyu commented Jun 16, 2026

Copy link
Copy Markdown
Member

Summary

  • add domains CLI commands for Cloudflare OAuth login, search/check/buy, attach, DNS sync, verify, resume, and buy-and-attach
  • add Cloudflare OAuth/Registrar/DNS helper with account discovery, PKCE login, auto-renew, and WHOIS redaction
  • add domain telemetry that avoids sending raw domains or error messages

Validation

  • npm run lint:fix
  • npm run lint
  • npm run build

Summary by cubic

Adds a Cloudflare domains CLI to @insforge/cli (v0.1.90) to search, buy, attach, and verify custom domains end-to-end. Includes OAuth login with account discovery, zone creation, DNS sync, safe telemetry, and token auto-refresh.

  • New Features

    • New domains commands: cloudflare login, search, check, buy, attach, dns sync, verify, status, resume, buy-and-attach.
    • Cloudflare OAuth with PKCE and local callback on http://127.0.0.1:8787/callback; tokens saved to ~/.insforge/cloudflare.json; auto account discovery with selection; supports --account-id, --skip-browser, and CLOUDFLARE_ACCOUNT_ID/CLOUDFLARE_ACCESS_TOKEN; refreshes tokens on 401.
    • Safe purchase flow requiring explicit flags (--confirm-*); optional --poll-seconds to wait for registration (defaults to 90s for buy-and-attach); registrations enable auto-renew and WHOIS redaction.
    • Attach to the linked InsForge deployment, auto-create the Cloudflare zone, and upsert A/CNAME + TXT verification records (proxied off); status --cloudflare shows registration details; resume completes attach/DNS/verify after registration.
    • Telemetry emits cli_domains_invoked via trackDomains with sanitized fields (e.g., TLD, states, structured error codes); never sends raw domains or error messages.
  • Bug Fixes

    • Request account-settings.read in OAuth scopes for reliable Cloudflare account discovery.
    • Safer DNS updates: upsert without overwriting unrelated TXT records; fail early if InsForge hasn’t produced DNS records yet.
    • Reject OAuth login on callback state mismatch to prevent CLI hangs; adds a regression test to verify immediate failure.

Written for commit eb36866. Summary will update on new commits.

Review in cubic

Note

Add Cloudflare domain registration CLI with OAuth, DNS sync, and InsForge attach

  • Adds a domains command group to the CLI covering the full domain lifecycle: OAuth login, search, availability check, purchase, DNS sync, InsForge attach/verify, status, and end-to-end buy-and-attach.
  • Implements Cloudflare OAuth with PKCE, a local callback server (port 8787, 5-minute timeout), token refresh on 401, and credential storage at ~/.insforge/cloudflare.json.
  • Domain purchases require explicit confirmation flags (exact domain, price, currency, and two boolean acknowledgements) in non-interactive mode; interactive mode prompts the user.
  • DNS sync ensures a Cloudflare zone exists for the apex and upserts A/CNAME/TXT records; fails early if InsForge has not yet produced records.
  • Domain command invocations emit sanitized analytics events via a new trackDomains function in analytics.ts.

Changes since #167 opened

  • Enhanced OAuth callback security and error handling in cloudflare module [4b65967]
  • Modified DNS record upsert behavior for TXT records in cloudflare.upsertCloudflareDnsRecord [4b65967]
  • Improved domain registration error handling and helper functions in domains command [4b65967]
  • Hardened credentials file handling in cloudflare module [4b65967]
  • Added test coverage for confirmPurchase in non-interactive mode, DNS record upsert scenarios, and cleanup hooks [4b65967]
  • Modified startCloudflareCallbackServer HTTP callback handler to reject the pending result promise immediately with an error when OAuth state mismatch is detected in the /callback request, instead of only responding with a 400 HTML page and leaving the promise pending until timeout [43dee83]
  • Bumped package version [eb36866]
  • Updated dependency lock file [eb36866]

Macroscope summarized c6b0dce.

Summary by CodeRabbit

  • New Features
    • Added a new domains CLI command group for Cloudflare-backed domain search, availability checks, purchase, attach, DNS sync, verification, status, and resume, including an end-to-end buy-and-attach option.
  • Documentation
    • Documented the domains workflow and Cloudflare OAuth setup, including automation credential overrides, non-browser login handling, and explicit purchase-confirmation behavior.
  • Bug Fixes
    • Improved Cloudflare DNS TXT upsert behavior to update existing records only when needed.
  • Tests
    • Added unit tests for DNS setup record generation, purchase confirmation enforcement, and domains telemetry.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Fermionic-Lyu, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 52 minutes and 9 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 80a8226a-7cb4-4cb5-9b9b-e3aa22b89540

📥 Commits

Reviewing files that changed from the base of the PR and between 43dee83 and eb36866.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json

Walkthrough

Adds a complete domains CLI feature: a new src/lib/cloudflare.ts library covering Cloudflare OAuth PKCE flow, credential persistence, and API operations (domain search/check/register, zone management, DNS upsert); a src/commands/domains/ module with all CLI subcommands, purchase confirmation, InsForge attachment/verify/DNS-sync lifecycle, and telemetry; a trackDomains analytics event; CLI entrypoint wiring; and README documentation.

Changes

Domains CLI Feature

Layer / File(s) Summary
Cloudflare types, OAuth flow, and credential management
src/lib/cloudflare.ts, src/lib/cloudflare.test.ts
Defines all Cloudflare TypeScript interfaces and constants; implements PKCE generation, local HTTP callback server for OAuth, token exchange and refresh, credential persistence to ~/.insforge/cloudflare.json (with env-var override), and requireCloudflareCredentials with auto-refresh. Tests validate OAuth URL parameters, client-id env override, and state mismatch error handling.
Cloudflare API wrapper, domain, zone, and DNS operations
src/lib/cloudflare.ts, src/lib/cloudflare.test.ts
Implements cloudflareFetch with Bearer auth and single-retry on 401 with token refresh; exports domain search/check/register, registration status fetch, zone find/create/ensure, DNS record list, and DNS upsert (PUT existing or POST new). Tests stub fetch to assert registration request body, accounts authorization header, and DNS record create/update flows.
Analytics trackDomains and domain telemetry module
src/lib/analytics.ts, src/commands/domains/telemetry.ts, src/commands/domains/telemetry.test.ts
Adds trackDomains PostHog event emitter with project-id and OSS-mode logic; defines DomainCommandTelemetry type with key whitelists and sanitization helpers; implements trackDomainUsage with CLIError field extraction, try/catch wrapping, and shutdownAnalytics in finally. Tests verify error field mapping and exclusion of sensitive properties.
Domain command helpers, types, purchase confirmation, DNS records, InsForge lifecycle
src/commands/domains/index.ts, src/commands/domains/index.test.ts
Defines internal InsForge domain types and CLI option interfaces; utility helpers (normalization, TLD parsing, price formatting, registrability assertion); purchase confirmation (explicit --confirm-* flags or interactive prompt, error in non-interactive mode); buildDnsSetupRecords (apex A/TXT or subdomain CNAME); InsForge attachment/verify API helpers with idempotent recovery; DNS sync to Cloudflare; output renderers; registration polling loop. Tests cover buildDnsSetupRecords cases, hasExplicitPurchaseConfirmation strict matching, and confirmPurchase non-interactive rejection.
Full domains subcommand tree and CLI entrypoint registration
src/commands/domains/index.ts, src/index.ts
Wires registerDomainsCommands with all subcommands (cloudflare login, search, check, buy, attach, dns sync, verify, status, resume, buy-and-attach), each tracking telemetry on success/failure, using requireAuth where needed, and routing errors through handleError. Registers the domain command group on the root CLI program.
README domains section
README.md
Adds user-facing documentation for the domains command: OAuth flow, credential storage, env-var overrides, all subcommands, TLD limitations, explicit purchase confirmation requirements, and the resume flow.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested reviewers

  • tonychang04
  • jwfing

Poem

🐰 Hop, hop, hooray for domains today!
OAuth tokens tucked safely away,
A PKCE challenge, a zone ensured,
DNS records upserted, all secured.
From search to verify, the rabbit runs free —
Custom domains for all, from Cloudflare to thee! 🌐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add Cloudflare domain registration CLI' clearly and accurately summarizes the main change—adding Cloudflare domain registration functionality to the CLI.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/cloudflare-domains

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a full domains command group to the CLI covering Cloudflare OAuth login, domain search/check/buy, InsForge attach, DNS sync, verify, status, resume, and an end-to-end buy-and-attach. All previously identified issues (state-mismatch rejection, HTML injection in error page, URLSearchParams.size on Node 18, closeAllConnections guard, TXT-record overwrite) have been addressed in this revision.

  • src/lib/cloudflare.ts: PKCE OAuth flow with local callback server on port 8787, token auto-refresh on 401, credentials stored at ~/.insforge/cloudflare.json (mode 0600); DNS upsert now POSTs new TXT records instead of overwriting unrelated ones.
  • src/commands/domains/index.ts: All nine subcommands with explicit purchase confirmation flags required in non-interactive mode; assertRegistrationSucceeded error message is shared between buy-and-attach and resume, incorrectly suggesting resume for terminal failed/blocked states.
  • src/commands/domains/telemetry.ts: Sanitization allowlist ensures raw domains and error messages never reach the analytics backend.

Confidence Score: 4/5

Safe to merge with the assertRegistrationSucceeded error-message issue addressed; the defect misleads users into retrying resume for permanently failed registrations.

The bulk of previously identified bugs are fixed in this revision. The remaining issue is assertRegistrationSucceeded giving a "run domains resume" hint even when the registration state is failed or blocked — permanently terminal states where no amount of retrying will help. When this function is also called directly from resume, the suggestion is circular. This is a present, user-visible correctness defect on the resume and buy-and-attach error paths.

src/commands/domains/index.ts — specifically assertRegistrationSucceeded and its callers in the resume and buy-and-attach actions.

Important Files Changed

Filename Overview
src/lib/cloudflare.ts New Cloudflare OAuth, registrar, and DNS helper; all previously flagged issues (state mismatch rejection, HTML injection, URLSearchParams.size, closeAllConnections guard, TXT upsert overwrite) are addressed in this revision.
src/commands/domains/index.ts New domains command group; assertRegistrationSucceeded shares an error message across buy-and-attach and resume that is circular and misleading for terminal failed/blocked states.
src/commands/domains/telemetry.ts New telemetry wrapper; sanitization allowlist is correctly implemented, shutdownAnalytics is guarded against double-flush, and raw domains/error messages are excluded.
src/lib/analytics.ts Adds trackDomains function following the same structure as existing track functions; accepts null config correctly unlike other track functions that require a non-null config.
src/lib/cloudflare.test.ts Good test coverage for OAuth URL, account listing, domain registration payload, TXT upsert behaviour (new/existing), and state-mismatch immediate rejection.
src/commands/domains/index.test.ts Tests cover DNS record building for apex/subdomain, purchase confirmation flag matching, and non-interactive rejection.
src/commands/domains/telemetry.test.ts Verifies sanitization allowlist, structured error fields, and that raw domain names/error messages are excluded from the analytics payload.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant User
    participant CLI
    participant CallbackServer as Callback Server (8787)
    participant Cloudflare as Cloudflare OAuth/API
    participant InsForge as InsForge API

    User->>CLI: domains cloudflare login
    CLI->>CallbackServer: start (port 8787)
    CLI->>User: print OAuth URL (stderr)
    User->>Cloudflare: browser authorize
    Cloudflare->>CallbackServer: "GET /callback?code=...&state=..."
    CallbackServer->>CLI: resolve result (code, state)
    CLI->>Cloudflare: POST /oauth2/token (PKCE exchange)
    Cloudflare-->>CLI: access_token, refresh_token
    CLI->>Cloudflare: GET /accounts (account discovery)
    Cloudflare-->>CLI: accounts[]
    CLI->>CLI: save ~/.insforge/cloudflare.json

    User->>CLI: domains buy-and-attach example.com
    CLI->>InsForge: requireAuth()
    CLI->>Cloudflare: POST /registrar/domain-check
    Cloudflare-->>CLI: pricing
    CLI->>User: confirm purchase
    CLI->>Cloudflare: POST /registrar/registrations
    Cloudflare-->>CLI: workflow (in_progress)
    CLI->>Cloudflare: poll /registration-status (up to 90s)
    Cloudflare-->>CLI: workflow (succeeded)
    CLI->>InsForge: POST /api/deployments/domains
    InsForge-->>CLI: CustomDomain (DNS records)
    CLI->>Cloudflare: ensureZone + upsertDnsRecords
    CLI->>InsForge: POST /api/deployments/domains/:domain/verify
    InsForge-->>CLI: verified domain
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant User
    participant CLI
    participant CallbackServer as Callback Server (8787)
    participant Cloudflare as Cloudflare OAuth/API
    participant InsForge as InsForge API

    User->>CLI: domains cloudflare login
    CLI->>CallbackServer: start (port 8787)
    CLI->>User: print OAuth URL (stderr)
    User->>Cloudflare: browser authorize
    Cloudflare->>CallbackServer: "GET /callback?code=...&state=..."
    CallbackServer->>CLI: resolve result (code, state)
    CLI->>Cloudflare: POST /oauth2/token (PKCE exchange)
    Cloudflare-->>CLI: access_token, refresh_token
    CLI->>Cloudflare: GET /accounts (account discovery)
    Cloudflare-->>CLI: accounts[]
    CLI->>CLI: save ~/.insforge/cloudflare.json

    User->>CLI: domains buy-and-attach example.com
    CLI->>InsForge: requireAuth()
    CLI->>Cloudflare: POST /registrar/domain-check
    Cloudflare-->>CLI: pricing
    CLI->>User: confirm purchase
    CLI->>Cloudflare: POST /registrar/registrations
    Cloudflare-->>CLI: workflow (in_progress)
    CLI->>Cloudflare: poll /registration-status (up to 90s)
    Cloudflare-->>CLI: workflow (succeeded)
    CLI->>InsForge: POST /api/deployments/domains
    InsForge-->>CLI: CustomDomain (DNS records)
    CLI->>Cloudflare: ensureZone + upsertDnsRecords
    CLI->>InsForge: POST /api/deployments/domains/:domain/verify
    InsForge-->>CLI: verified domain
Loading

Reviews (6): Last reviewed commit: "bump CLI version" | Re-trigger Greptile

Comment thread src/lib/cloudflare.ts
Comment thread src/lib/cloudflare.ts Outdated
Comment thread src/lib/cloudflare.ts Outdated
Comment thread src/commands/domains/index.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
src/lib/cloudflare.test.ts (1)

22-31: ⚡ Quick win

Move env/global cleanup to afterEach to prevent state leakage on test failures.

Current in-test cleanup can be skipped when assertions throw, causing cross-test pollution.

🔧 Suggested refactor
-import { describe, expect, it, vi } from 'vitest';
+import { afterEach, describe, expect, it, vi } from 'vitest';
@@
 describe('Cloudflare OAuth helpers', () => {
+  afterEach(() => {
+    vi.unstubAllEnvs();
+    vi.unstubAllGlobals();
+  });
+
   it('builds the Cloudflare OAuth URL with PKCE and the fixed CLI callback', () => {
@@
-    vi.unstubAllEnvs();
   });
@@
-    vi.unstubAllEnvs();
-    vi.unstubAllGlobals();
   });
@@
-    vi.unstubAllGlobals();
   });
 });

Also applies to: 56-58, 95-95

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/cloudflare.test.ts` around lines 22 - 31, The vi.unstubAllEnvs()
cleanup is called inside the test function after the expect statements, which
means it will be skipped if an assertion throws, causing environment variable
state to leak to subsequent tests. Move all vi.unstubAllEnvs() calls from the
end of each test function (in the test for buildCloudflareAuthorizeUrl and other
affected tests) into a shared afterEach hook that runs after every test,
ensuring cleanup happens regardless of test pass/fail status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/commands/domains/index.ts`:
- Around line 555-557: The `.catch(() => null)` handler on the
`getCloudflareRegistration(domain)` call is masking all errors indiscriminately,
including authentication, network, and API failures that should be reported.
Instead of catching all errors, modify the catch handler to check the error type
and only suppress the known "not found" case (likely a specific error code or
message), then rethrow all other errors so they properly trigger failure
telemetry and error handling. This ensures only expected "not found" scenarios
are silently handled while unexpected failures propagate for proper diagnosis.

In `@src/lib/cloudflare.ts`:
- Around line 175-180: The error_description parameter is being directly
injected into the HTML response without escaping, creating a security
vulnerability where crafted OAuth callbacks could inject malicious scripts. In
the error handling block where the res.end() method is called with the HTML
string containing the Cloudflare authorization failed message, escape the desc
variable using HTML entity encoding (such as replacing special characters like
<, >, and & with their HTML entity equivalents) before inserting it into the
HTML template to prevent script injection attacks.
- Around line 321-324: The JSON.parse call that reads and parses the
CLOUDFLARE_FILE can throw an exception when the file contains invalid JSON,
causing the CLI to crash instead of following the friendly auth recovery paths.
Wrap the JSON.parse and readFileSync statement in a try-catch block, and return
null in the catch block to gracefully handle malformed credential files the same
way the code currently handles missing accountId or accessToken.
- Around line 186-189: In the Cloudflare callback handler at the code block
checking for missing code or state parameters, the 400 response is being sent to
the client but the underlying result promise is not being rejected. This causes
the login process to hang until timeout instead of returning immediately. Add a
reject call for the result promise in this error path (where the 400 response is
written) to ensure the promise is properly rejected when the callback is
invalid, allowing the login to fail fast rather than waiting for the timeout.

---

Nitpick comments:
In `@src/lib/cloudflare.test.ts`:
- Around line 22-31: The vi.unstubAllEnvs() cleanup is called inside the test
function after the expect statements, which means it will be skipped if an
assertion throws, causing environment variable state to leak to subsequent
tests. Move all vi.unstubAllEnvs() calls from the end of each test function (in
the test for buildCloudflareAuthorizeUrl and other affected tests) into a shared
afterEach hook that runs after every test, ensuring cleanup happens regardless
of test pass/fail status.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e50a3a0-11a5-411a-9df9-8659a35561e0

📥 Commits

Reviewing files that changed from the base of the PR and between 21c8bd0 and 8dcf7d2.

📒 Files selected for processing (9)
  • README.md
  • src/commands/domains/index.test.ts
  • src/commands/domains/index.ts
  • src/commands/domains/telemetry.test.ts
  • src/commands/domains/telemetry.ts
  • src/index.ts
  • src/lib/analytics.ts
  • src/lib/cloudflare.test.ts
  • src/lib/cloudflare.ts

Comment thread src/commands/domains/index.ts
Comment thread src/lib/cloudflare.ts
Comment thread src/lib/cloudflare.ts
Comment thread src/lib/cloudflare.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 9 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/lib/cloudflare.ts Outdated
Comment thread src/lib/cloudflare.ts Outdated
@Fermionic-Lyu Fermionic-Lyu force-pushed the codex/cloudflare-domains branch from 8dcf7d2 to 8472808 Compare June 16, 2026 17:59
@Fermionic-Lyu Fermionic-Lyu force-pushed the codex/cloudflare-domains branch from 8472808 to 5a3b0f0 Compare June 16, 2026 18:05
Comment thread src/lib/cloudflare.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/cloudflare.ts`:
- Line 3: The writeFileSync mode option only applies permissions when the file
is newly created, leaving existing files with potentially broader permissions
when tokens are refreshed or re-logged. Add chmodSync to the import statement
from 'node:fs', then locate all writeFileSync calls that save the cloudflare
credentials file (likely in a function that saves access/refresh tokens), and
add a chmodSync call immediately after each writeFileSync to enforce 0o600
permissions on every save operation regardless of whether the file already
exists.
- Around line 593-601: The current logic at line 593 uses a fallback to
existing[0] when exact content match is not found, which is problematic for TXT
records since multiple TXT records can coexist at the same domain (SPF, DKIM,
DMARC, etc.) and overwriting the first one will replace unrelated records.
Modify the logic to check the record type before deciding whether to update or
create: if the record type is TXT and no exact content match is found, call
cloudflareFetch with method POST to create a new record instead of falling back
to updating existing[0]; preserve the current fallback behavior (method PUT) for
singleton record types like A, CNAME, and MX where existing[0] is an acceptable
default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5fbf8705-e4dc-48fc-84e7-29c7e4f78a67

📥 Commits

Reviewing files that changed from the base of the PR and between 8dcf7d2 and 5a3b0f0.

📒 Files selected for processing (9)
  • README.md
  • src/commands/domains/index.test.ts
  • src/commands/domains/index.ts
  • src/commands/domains/telemetry.test.ts
  • src/commands/domains/telemetry.ts
  • src/index.ts
  • src/lib/analytics.ts
  • src/lib/cloudflare.test.ts
  • src/lib/cloudflare.ts
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/lib/analytics.ts
  • src/index.ts
  • src/commands/domains/index.test.ts
  • src/lib/cloudflare.test.ts
  • src/commands/domains/telemetry.ts
  • src/commands/domains/telemetry.test.ts
  • src/commands/domains/index.ts

Comment thread src/lib/cloudflare.ts Outdated
Comment thread src/lib/cloudflare.ts Outdated
Comment thread src/lib/cloudflare.ts Outdated

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat — add Cloudflare domain registration CLI

Summary: A clean, well-scoped, all-additive feature that implements the full Cloudflare domain lifecycle (OAuth/PKCE login → search/check/buy → attach → DNS sync → verify → resume/buy-and-attach) with strong security hygiene and good unit coverage of the core helpers; the items below are non-blocking.

Requirements context: No matching spec/plan found — docs/specs/ contains only 2026-03-27-diagnose-* and 2026-04-17-db-migrations-*, none of which cover domains. Assessed against the PR description, DEVELOPMENT.md, and the repo's cli-development skill conventions.

I verified the four review dimensions across src/lib/cloudflare.ts, src/commands/domains/{index,telemetry}.ts, src/lib/analytics.ts, the wiring in src/index.ts, and the three test files.

What's good (worth calling out)

  • Security hygiene is strong: OAuth uses PKCE (S256) and a random state that's validated before token exchange (cloudflare.ts:376-378); credentials are written mode: 0o600 (cloudflare.ts:308); the callback server binds 127.0.0.1 only with a 5-minute unref() timeout.
  • Telemetry is privacy-safe and proven so: sanitizeDomainTelemetry uses strict key allow-lists and a TLD regex, drops everything else, and never forwards raw domains or error messages — and telemetry.test.ts asserts exactly that (domain and error_message absent).
  • The purchase money-guard is conservative: non-interactive buy requires 5 exactly-matching --confirm-* flags against the live candidate from checkCloudflareDomains (index.ts:118-161, 308-319); registrations enable auto_renew + privacy_mode: redaction.
  • Conventions followed: command/registration layout, CLIError + handleError, outputJson/Table/Success, .js ESM imports, analytics flushed in finally via shutdownAnalytics(), and trackDomains correctly mirrors the null-config/OSS-fallback pattern of trackConfig.

Critical

(none)

Suggestion

  • [Security] src/lib/cloudflare.ts:177-182 — the OAuth callback error page interpolates error_description straight into HTML (res.end(\

    ${desc}

    …`)). It's untrusted query input rendered unescaped → reflected-injection on the 127.0.0.1:8787page during the ~5-min login window. Blast radius is low (transient localhost origin, no tokens stored in the browser), but please HTML-escapedesc`.
  • [Software engineering / convention] agent-skills syncDEVELOPMENT.md §3 says adding a user-facing command group almost certainly requires updating the insforge-cli skill in InsForge/agent-skills (and mentioning that PR here). README.md is updated (good), but the agent skill will be stale and agents will emit wrong domains … commands until it's updated. Please open/link the companion PR.
  • [Functionality] src/lib/cloudflare.ts:579-611 upsertCloudflareDnsRecord — when a name holds multiple records of the same type with differing content, the ?? existing[0] fallback PUTs over an unrelated existing record. It's fine for the single A / single verification-TXT shapes this PR produces, but since ensureCloudflareZone may adopt a zone the user already owns, consider POST-ing a new record (rather than overwriting existing[0]) when no content match is found, to avoid clobbering pre-existing DNS.
  • [Testing] src/commands/domains/index.test.ts — the highest-risk behaviors lack direct tests: the non-interactive confirmPurchase rejection path (the spend guard) and the OAuth state mismatch abort. hasExplicitPurchaseConfirmation is tested as a pure helper, but a test asserting confirmPurchase throws DOMAIN_PURCHASE_CONFIRMATION_REQUIRED when flags are absent/mismatched would lock down the money path.

Information

  • [Functionality] index.ts:440-448, 638-646 — the confirmed telemetry field builds a synthetic candidate from the user's own --confirm-* flags, so hasExplicitPurchaseConfirmation compares those values to themselves. The metric therefore reports "confirm flags were passed," not "they matched the real price." Harmless (the actual gate in registerAfterCheck uses the live candidate), but the analytics signal is misleading.
  • [Security/robustness] cloudflare.ts:185-196 — the callback server resolves on the first request that merely contains code+state, validating state only afterward in performCloudflareOAuthLogin. A local process could pre-empt the real callback and force a STATE_MISMATCH abort (login DoS). Low risk; consider ignoring callbacks whose state doesn't match rather than resolving on the first one.
  • [Functionality] Cloudflare Registrar endpoints (/registrar/domain-search, /domain-check, /registrations with Prefer: respond-async) are relatively new/beta API surface I can't verify from here — worth a manual end-to-end smoke test against the current Cloudflare API before release.
  • Performance: no concerns — pollRegistration sleeps async (3s interval, bounded by deadline), DNS upserts iterate a tiny record set, no N+1 or blocking I/O on the event loop.

Verdict

approved (informational — human approval still required via the approve flow). No Critical findings; the Suggestions above are worth addressing before merge, particularly the HTML-escaping and the agent-skills sync, but none block.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/lib/cloudflare.ts
Comment thread src/lib/cloudflare.ts
The state-mismatch branch in the OAuth callback server sent a 400
response but never settled the result promise, so a mismatched-state
callback (stale tab, browser refresh, CSRF redirect) left the CLI
hanging until the 5-minute timeout. Reject immediately, mirroring the
missing-code/state and error branches, and add a regression test that
drives the real callback server with a bad state.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/cloudflare.ts (1)

137-144: 💤 Low value

Static analysis context: manual HTML escaping is acceptable here.

The static analyzer flags manual HTML entity encoding, but for this specific use case—a localhost-only OAuth callback page displaying error text inside a <p> tag—the escaping covers all necessary characters (&, <, >, ", '). Adding a full sanitization library for this minimal, non-internet-facing use would be over-engineering.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/cloudflare.ts` around lines 137 - 144, The static analyzer is
flagging the manual HTML entity escaping in the escapeHtml function as a code
quality issue. Suppress the analyzer warning for this function by adding an
appropriate suppression comment (such as a linter or analyzer directive) above
the escapeHtml function definition, since this localhost-only OAuth callback use
case with all necessary entity characters covered does not warrant the overhead
of a full sanitization library. No changes to the actual escaping logic are
needed.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/cloudflare.test.ts`:
- Around line 202-225: The stderrSpy.mockRestore() call at the end of the test
'rejects login immediately when the OAuth callback state does not match' will
not execute if any earlier assertion fails, causing the spy to leak into
subsequent tests. Restructure the test by wrapping the test body (starting from
the stderrSpy creation through the expect(delivered).toBe(true) and await
rejection statements) in a try block, and place the stderrSpy.mockRestore() call
in a finally block to guarantee cleanup regardless of whether assertions pass or
fail.

---

Nitpick comments:
In `@src/lib/cloudflare.ts`:
- Around line 137-144: The static analyzer is flagging the manual HTML entity
escaping in the escapeHtml function as a code quality issue. Suppress the
analyzer warning for this function by adding an appropriate suppression comment
(such as a linter or analyzer directive) above the escapeHtml function
definition, since this localhost-only OAuth callback use case with all necessary
entity characters covered does not warrant the overhead of a full sanitization
library. No changes to the actual escaping logic are needed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 45628a68-f58a-4038-86c6-0a8439f0655e

📥 Commits

Reviewing files that changed from the base of the PR and between 5a3b0f0 and 43dee83.

📒 Files selected for processing (4)
  • src/commands/domains/index.test.ts
  • src/commands/domains/index.ts
  • src/lib/cloudflare.test.ts
  • src/lib/cloudflare.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/commands/domains/index.test.ts
  • src/commands/domains/index.ts

Comment on lines +202 to +225
it('rejects login immediately when the OAuth callback state does not match', async () => {
// Silence the OAuth URL banner the login flow writes to stderr.
const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);

// Without the fix this never settles and the test times out instead of
// hanging the real CLI for the full 5-minute callback timeout.
const loginPromise = performCloudflareOAuthLogin({ skipBrowser: true });
const rejection = expect(loginPromise).rejects.toThrow('Cloudflare OAuth state mismatch.');

// Wait for the callback server to bind, then deliver a mismatched state.
let delivered = false;
for (let attempt = 0; attempt < 50 && !delivered; attempt += 1) {
try {
await fetch('http://127.0.0.1:8787/callback?code=test-code&state=wrong-state');
delivered = true;
} catch {
await new Promise((resolve) => setTimeout(resolve, 20));
}
}
expect(delivered).toBe(true);

await rejection;
stderrSpy.mockRestore();
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

fd -t f "cloudflare.test.ts" --exec cat -n {}

Repository: InsForge/CLI

Length of output: 9037


Guarantee stderr spy cleanup even when assertions fail.

At line 224, stderrSpy.mockRestore() only runs on the success path. If expect(delivered).toBe(true) (line 221) or await rejection (line 223) fails first, the spy leaks into later tests. Unlike the vi.stubGlobal() calls in other tests that are cleaned up by the afterEach hook, vi.spyOn() requires explicit restoration. Wrap the test body in try/finally to guarantee cleanup.

Suggested fix
 it('rejects login immediately when the OAuth callback state does not match', async () => {
   // Silence the OAuth URL banner the login flow writes to stderr.
   const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);

-  // Without the fix this never settles and the test times out instead of
-  // hanging the real CLI for the full 5-minute callback timeout.
-  const loginPromise = performCloudflareOAuthLogin({ skipBrowser: true });
-  const rejection = expect(loginPromise).rejects.toThrow('Cloudflare OAuth state mismatch.');
-
-  // Wait for the callback server to bind, then deliver a mismatched state.
-  let delivered = false;
-  for (let attempt = 0; attempt < 50 && !delivered; attempt += 1) {
-    try {
-      await fetch('http://127.0.0.1:8787/callback?code=test-code&state=wrong-state');
-      delivered = true;
-    } catch {
-      await new Promise((resolve) => setTimeout(resolve, 20));
-    }
-  }
-  expect(delivered).toBe(true);
-
-  await rejection;
-  stderrSpy.mockRestore();
+  try {
+    // Without the fix this never settles and the test times out instead of
+    // hanging the real CLI for the full 5-minute callback timeout.
+    const loginPromise = performCloudflareOAuthLogin({ skipBrowser: true });
+    const rejection = expect(loginPromise).rejects.toThrow('Cloudflare OAuth state mismatch.');
+
+    // Wait for the callback server to bind, then deliver a mismatched state.
+    let delivered = false;
+    for (let attempt = 0; attempt < 50 && !delivered; attempt += 1) {
+      try {
+        await fetch('http://127.0.0.1:8787/callback?code=test-code&state=wrong-state');
+        delivered = true;
+      } catch {
+        await new Promise((resolve) => setTimeout(resolve, 20));
+      }
+    }
+    expect(delivered).toBe(true);
+    await rejection;
+  } finally {
+    stderrSpy.mockRestore();
+  }
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('rejects login immediately when the OAuth callback state does not match', async () => {
// Silence the OAuth URL banner the login flow writes to stderr.
const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);
// Without the fix this never settles and the test times out instead of
// hanging the real CLI for the full 5-minute callback timeout.
const loginPromise = performCloudflareOAuthLogin({ skipBrowser: true });
const rejection = expect(loginPromise).rejects.toThrow('Cloudflare OAuth state mismatch.');
// Wait for the callback server to bind, then deliver a mismatched state.
let delivered = false;
for (let attempt = 0; attempt < 50 && !delivered; attempt += 1) {
try {
await fetch('http://127.0.0.1:8787/callback?code=test-code&state=wrong-state');
delivered = true;
} catch {
await new Promise((resolve) => setTimeout(resolve, 20));
}
}
expect(delivered).toBe(true);
await rejection;
stderrSpy.mockRestore();
});
it('rejects login immediately when the OAuth callback state does not match', async () => {
// Silence the OAuth URL banner the login flow writes to stderr.
const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);
try {
// Without the fix this never settles and the test times out instead of
// hanging the real CLI for the full 5-minute callback timeout.
const loginPromise = performCloudflareOAuthLogin({ skipBrowser: true });
const rejection = expect(loginPromise).rejects.toThrow('Cloudflare OAuth state mismatch.');
// Wait for the callback server to bind, then deliver a mismatched state.
let delivered = false;
for (let attempt = 0; attempt < 50 && !delivered; attempt += 1) {
try {
await fetch('http://127.0.0.1:8787/callback?code=test-code&state=wrong-state');
delivered = true;
} catch {
await new Promise((resolve) => setTimeout(resolve, 20));
}
}
expect(delivered).toBe(true);
await rejection;
} finally {
stderrSpy.mockRestore();
}
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/cloudflare.test.ts` around lines 202 - 225, The
stderrSpy.mockRestore() call at the end of the test 'rejects login immediately
when the OAuth callback state does not match' will not execute if any earlier
assertion fails, causing the spy to leak into subsequent tests. Restructure the
test by wrapping the test body (starting from the stderrSpy creation through the
expect(delivered).toBe(true) and await rejection statements) in a try block, and
place the stderrSpy.mockRestore() call in a finally block to guarantee cleanup
regardless of whether assertions pass or fail.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - approved.

@Fermionic-Lyu Fermionic-Lyu merged commit 9c44624 into main Jun 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants