Domain Connect#3877
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Domain Connect: TXT discovery and provider mapping, RSA-signed apply URL generation, two workspace-scoped API endpoints (apply and forward-instructions), UI flows and modals for auto-config/forwarding, provider icons, email template, and callback handling. ChangesDomain Connect Auto-Configuration
Sequence DiagramsequenceDiagram
participant User as User
participant UI as DomainConfiguration UI
participant ApplyAPI as POST /domain-connect/apply
participant Discover as discoverDomainConnectIfEligible
participant Signer as buildSignedApplyUrl
participant Provider as Provider (Vercel/Cloudflare)
participant Callback as Callback Handler
User->>UI: Click "Auto configure"
UI->>ApplyAPI: POST { recordType, returnTo? }
ApplyAPI->>Discover: discoverDomainConnectIfEligible(apex, status)
Discover-->>ApplyAPI: DomainConnectDiscovery (urlSyncUX, providerKind)
ApplyAPI->>Signer: buildSignedApplyUrl(...)
Signer-->>ApplyAPI: { applyUrl }
ApplyAPI-->>UI: { applyUrl }
UI->>Provider: Redirect browser to signed applyUrl
Provider->>Callback: Redirect back with domain_connect=callback
Callback->>UI: Show success/error toast and clear params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/web/app/api/domains/[domain]/forward-instructions/route.ts (1)
34-37: ⚡ Quick winRemove the unused
getConfigResponsecall.Line 36 performs an external call whose result is ignored, adding latency with no effect.
♻️ Proposed cleanup
- const [domainJson] = await Promise.all([ - getDomainResponse(domain), - getConfigResponse(domain), - ]); + const domainJson = await getDomainResponse(domain);🤖 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 `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts around lines 34 - 37, The Promise.all currently calls getConfigResponse(domain) but only destructures domainJson, so remove the unused getConfigResponse call to avoid extra latency: replace the const [domainJson] = await Promise.all([ getDomainResponse(domain), getConfigResponse(domain), ]) pattern with a single await for getDomainResponse (e.g., const domainJson = await getDomainResponse(domain)); also remove any now-unused import or reference to getConfigResponse to keep the module clean.apps/web/ui/modals/forward-dns-instructions-modal.tsx (2)
122-128: ⚡ Quick winRemove
setShowModalfromuseMemodependencies.
setShowModalis a React state setter and is guaranteed to be stable across renders. Including it in the dependency array is unnecessary.♻️ Proposed fix
return useMemo( () => ({ setShowForwardDnsModal: setShowModal, ForwardDnsInstructionsModal: Modal, }), - [setShowModal, Modal], + [Modal], );🤖 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 `@apps/web/ui/modals/forward-dns-instructions-modal.tsx` around lines 122 - 128, The useMemo call returning { setShowForwardDnsModal: setShowModal, ForwardDnsInstructionsModal: Modal } incorrectly includes the stable React state setter setShowModal in its dependency array; remove setShowModal from the dependencies so the array only depends on Modal (i.e., change useMemo(..., [Modal])) to avoid unnecessary recomputation while keeping ForwardDnsInstructionsModal and setShowForwardDnsModal correct.
111-120: ⚡ Quick winSimplify
useCallbackdependency array.The dependency array destructures individual props instead of depending on the entire
propsobject. Ifpropshas unstable object identity (recreated on each render), this pattern won't prevent unnecessary re-creation of theModalcomponent. Consider depending onpropsdirectly or ensuringpropsis memoized at the call site.♻️ Proposed fix
const Modal = useCallback( () => ( <ForwardDnsInstructionsModal showModal={showModal} setShowModal={setShowModal} {...props} /> ), - [showModal, props.domain, props.recordType, props.workspaceId], + [showModal, props], );🤖 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 `@apps/web/ui/modals/forward-dns-instructions-modal.tsx` around lines 111 - 120, The useCallback for Modal currently lists individual props (props.domain, props.recordType, props.workspaceId) which defeats the purpose if the props object identity is unstable; update the dependency array to depend on props directly (e.g., [showModal, props]) or ensure the caller memoizes props before passing in; locate the useCallback that defines Modal (wrapping ForwardDnsInstructionsModal and using showModal/setShowModal) and replace the dependency entries for props.domain/props.recordType/props.workspaceId with a single props dependency so the callback only re-creates when the props object actually changes.apps/web/lib/domain-connect/sign-apply-url.ts (1)
16-21: ⚡ Quick winVerify safety of non-null assertion.
Line 19 uses
params[k]!assuming values are always defined. WhileObject.keys(params)only iterates over existing keys, TypeScript'sRecord<string, string>type can still permitundefinedvalues in some configurations. Consider adding a runtime guard or using a more explicit type.🛡️ Proposed defensive check
export function buildSigningString(params: Record<string, string>): string { return Object.keys(params) .sort() - .map((k) => `${encodeURIComponent(k)}=${encodeURIComponent(params[k]!)}`) + .map((k) => { + const value = params[k]; + if (value === undefined) throw new Error(`Missing value for key: ${k}`); + return `${encodeURIComponent(k)}=${encodeURIComponent(value)}`; + }) .join("&"); }🤖 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 `@apps/web/lib/domain-connect/sign-apply-url.ts` around lines 16 - 21, The buildSigningString function uses a non-null assertion on params[k] which can be unsafe; replace it with a runtime-safe approach: either tighten the type or check values before encoding — e.g., filter Object.keys(params) to only keys where params[k] !== undefined (or throw a clear error if a missing value is exceptional), then use the guaranteed string value when calling encodeURIComponent; update the codepaths around buildSigningString to reflect the chosen behavior so callers provide valid data or receive an explicit error.
🤖 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 `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts:
- Around line 52-54: The A-record push always uses name: "@" which is wrong for
subdomains; update the logic in the route handler where recordType and records
are used so the name is computed: if the requested domain is the apex/root
domain use "@", otherwise extract the left-most label of the host (e.g., "go"
from "go.example.com") and use that as the name when calling records.push({
type: "A", name, value: DUB_CUSTOM_DOMAIN_A_RECORD }). Locate the code around
recordType and records.push in route.ts and replace the hard-coded "@" with this
computed name (ensure you handle single-label hosts and trailing dots
consistently).
---
Nitpick comments:
In `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts:
- Around line 34-37: The Promise.all currently calls getConfigResponse(domain)
but only destructures domainJson, so remove the unused getConfigResponse call to
avoid extra latency: replace the const [domainJson] = await Promise.all([
getDomainResponse(domain), getConfigResponse(domain), ]) pattern with a single
await for getDomainResponse (e.g., const domainJson = await
getDomainResponse(domain)); also remove any now-unused import or reference to
getConfigResponse to keep the module clean.
In `@apps/web/lib/domain-connect/sign-apply-url.ts`:
- Around line 16-21: The buildSigningString function uses a non-null assertion
on params[k] which can be unsafe; replace it with a runtime-safe approach:
either tighten the type or check values before encoding — e.g., filter
Object.keys(params) to only keys where params[k] !== undefined (or throw a clear
error if a missing value is exceptional), then use the guaranteed string value
when calling encodeURIComponent; update the codepaths around buildSigningString
to reflect the chosen behavior so callers provide valid data or receive an
explicit error.
In `@apps/web/ui/modals/forward-dns-instructions-modal.tsx`:
- Around line 122-128: The useMemo call returning { setShowForwardDnsModal:
setShowModal, ForwardDnsInstructionsModal: Modal } incorrectly includes the
stable React state setter setShowModal in its dependency array; remove
setShowModal from the dependencies so the array only depends on Modal (i.e.,
change useMemo(..., [Modal])) to avoid unnecessary recomputation while keeping
ForwardDnsInstructionsModal and setShowForwardDnsModal correct.
- Around line 111-120: The useCallback for Modal currently lists individual
props (props.domain, props.recordType, props.workspaceId) which defeats the
purpose if the props object identity is unstable; update the dependency array to
depend on props directly (e.g., [showModal, props]) or ensure the caller
memoizes props before passing in; locate the useCallback that defines Modal
(wrapping ForwardDnsInstructionsModal and using showModal/setShowModal) and
replace the dependency entries for
props.domain/props.recordType/props.workspaceId with a single props dependency
so the callback only re-creates when the props object actually changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55d17398-ec1c-4bce-ba5c-d8fe913989ad
📒 Files selected for processing (17)
apps/web/.env.exampleapps/web/app/api/domains/[domain]/domain-connect/apply/route.tsapps/web/app/api/domains/[domain]/forward-instructions/route.tsapps/web/app/api/domains/[domain]/verify/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/domains/page-client.tsxapps/web/lib/domain-connect/constants.tsapps/web/lib/domain-connect/discover.tsapps/web/lib/domain-connect/sign-apply-url.tsapps/web/lib/domain-connect/types.tsapps/web/ui/domains/domain-card.tsxapps/web/ui/domains/domain-configuration.tsxapps/web/ui/modals/forward-dns-instructions-modal.tsxapps/web/ui/partners/program-link-configuration.tsxpackages/email/src/templates/domain-dns-instructions.tsxpackages/ui/src/icons/cloudflare.tsxpackages/ui/src/icons/index.tsxpackages/ui/src/icons/vercel.tsx
There was a problem hiding this comment.
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 `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts:
- Around line 34-37: The code awaits Promise.all([getDomainResponse(domain),
getConfigResponse(domain)]) but only uses the first result (domainJson), so
remove the unused getConfigResponse call to avoid extra latency and potential
failures: replace the Promise.all call with a single await
getDomainResponse(domain) (keeping the variable name domainJson) and remove any
related imports or references to getConfigResponse in this file (ensure
getDomainResponse is still awaited and error handling remains).
- Around line 69-81: The TXT DNS instruction block can push empty name or value
strings into records (variables: txtVerification, txtHostFqdn, apex, txtHost,
records); change the logic so you only push a TXT record when the resolved
txtHost is non-empty (treat "@" as valid) and txtVerification.value is non-blank
(e.g., value?.trim() !== ""); if either is empty, skip adding to records to
avoid emitting invalid instructions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0481af7f-2b73-46b3-848c-7f3daea3c7ab
📒 Files selected for processing (1)
apps/web/app/api/domains/[domain]/forward-instructions/route.ts
|
@coderabbitai full review. any breaking changes or security issues? |
🧩 Analysis chain🏁 Script executed: Length of output: 12414 🏁 Script executed: Length of output: 27819
🔴 Security Issues1. SSRF in
|
| Severity | Issue | Location |
|---|---|---|
| 🔴 High | SSRF via DNS TXT → arbitrary host fetch | lib/domain-connect/discover.ts |
| 🟠 Medium | Open redirect via server-returned applyUrl |
ui/domains/domain-configuration.tsx + apply/route.ts |
| 🟠 Medium | No rate limiting on email-sending endpoint | api/domains/[domain]/forward-instructions/route.ts |
| 🟡 Low-Bug | Stray export * re-export conflicts |
api/domains/[domain]/verify/route.ts |
| 🟢 Additive | domainConnect field in verify response |
api/domains/[domain]/verify/route.ts |
The stray export * (#4) is the one most likely to cause an immediate breakage in production and should be fixed before merging.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/web/app/api/domains/[domain]/forward-instructions/route.ts (1)
74-74: 💤 Low valueSimplify redundant condition.
The condition
(txtHost || txtHost === "@")is redundant because iftxtHost === "@", thentxtHostis already truthy. This can be simplified to justtxtHost && txtValue.♻️ Proposed simplification
- if ((txtHost || txtHost === "@") && txtValue) { + if (txtHost && txtValue) { records.push({ type: "TXT", name: txtHost, value: txtValue }); }🤖 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 `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts at line 74, The if-condition checking TXT inputs uses a redundant expression: replace the check in the route handler where it currently reads ((txtHost || txtHost === "@") && txtValue) with a simplified truthy check txtHost && txtValue; update the conditional that references variables txtHost and txtValue (inside the forward-instructions route handler) so it just validates both are present without the unnecessary txtHost === "@" clause.apps/web/ui/domains/domain-configuration.tsx (1)
36-38: 💤 Low valueSimplify recordType initialization.
The double negation
!!subdomainis unnecessary in a ternary expression.♻️ Proposed simplification
- const [recordType, setRecordType] = useState<"A" | "CNAME">( - !!subdomain ? "CNAME" : "A", - ); + const [recordType, setRecordType] = useState<"A" | "CNAME">( + subdomain ? "CNAME" : "A", + );🤖 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 `@apps/web/ui/domains/domain-configuration.tsx` around lines 36 - 38, The initialization of the recordType state uses an unnecessary double negation (!!subdomain); update the useState call for recordType to use subdomain directly in the ternary (i.e., subdomain ? "CNAME" : "A") so it remains functionally identical but simpler; locate the useState invocation that declares const [recordType, setRecordType] = useState<"A" | "CNAME">(...) and replace the !!subdomain expression with subdomain.apps/web/app/api/domains/[domain]/domain-connect/apply/route.ts (1)
106-111: 💤 Low valueSimplify redundant groupId assignment.
Line 109 reassigns
groupId = "subdomain"but this is already set on line 102. Theelsebranch can be simplified.♻️ Proposed simplification
if (isApex) { queryParams.groupId = "apex"; } else { - queryParams.groupId = "subdomain"; queryParams.host = (subdomain ?? "www").toLowerCase(); }🤖 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 `@apps/web/app/api/domains/`[domain]/domain-connect/apply/route.ts around lines 106 - 111, The else branch redundantly sets queryParams.groupId = "subdomain" even though queryParams.groupId was already assigned "subdomain" earlier; simplify by removing that redundant assignment and only set queryParams.host = (subdomain ?? "www").toLowerCase() inside the else branch—keep the isApex conditional as-is and ensure queryParams.groupId is only set to "apex" in the if branch and left unchanged otherwise.
🤖 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 `@apps/web/app/api/domains/`[domain]/domain-connect/apply/route.ts:
- Around line 70-78: The code currently uses discovery.urlSyncUX (returned by
discoverDomainConnect(apex)) to build applyUrl and return it to the client,
enabling an open redirect; update the server-side logic that reads
discovery.urlSyncUX so you parse and validate the URL origin before using it (in
the route handler that builds applyUrl) — e.g., ensure the URL's origin matches
an allowlist of known providers (Vercel/Cloudflare) or the discovery apex,
reject or ignore values that fail validation, and fallback to a safe internal
URL; alternatively implement an intermediate redirect endpoint on the server
that performs the same validation before redirecting (and/or add client-side
validation in domain-configuration.tsx before calling window.location.assign) so
untrusted DNS-provided values cannot cause a redirect to an arbitrary host.
In `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts:
- Around line 21-95: The POST handler wrapped by withWorkspace (the async
function that calls getDomainOrThrow, parses bodySchema, builds records and
calls sendEmail) currently allows unlimited emails to arbitrary addresses; add
per-user or per-workspace rate limiting (e.g., max 10 emails per hour) before
calling sendEmail: use a shared store (Redis, cache, or DB table) keyed by
workspace.id or session.user.id + endpoint identifier to increment and expire
counts, check the count and return a 429-style error/throw (instead of
proceeding) when the limit is exceeded, and only call sendEmail when under the
limit; ensure you reference the same identifiers used in the handler (workspace,
session.user.email) and enforce atomic increment/TTL (or transaction) to avoid
race conditions.
In `@apps/web/lib/domain-connect/discover.ts`:
- Around line 46-55: The current return uses candidates.find(... ) ??
candidates[0] ?? null which allows falling back to untrusted DNS TXT values and
enables SSRF; change the logic in discover.ts to remove the fallback to
candidates[0] so only values matching the trusted keywords ("domainconnect",
"vercel", "cloudflare") are returned (i.e., return the result of
candidates.find(...) or null), or alternatively enforce a strict
allowlist/validation step: validate the chosen host from candidates against an
explicit allowlist or perform IP resolution and reject private/internal
IPv4/IPv6 and localhost names before returning/using it (update the code paths
that reference candidates.find and candidates[0] accordingly).
In `@apps/web/ui/domains/domain-configuration.tsx`:
- Around line 68-70: The redirect uses json?.applyUrl with
window.location.assign without origin checks, enabling an open-redirect—before
calling window.location.assign in domain-configuration.tsx, parse and validate
json?.applyUrl: ensure it is an absolute URL whose origin equals
window.location.origin or is in an explicit server-approved whitelist (or else
only allow relative paths), and only call window.location.assign when that
validation passes; update the code path around json?.applyUrl and the early
return to enforce this check.
---
Nitpick comments:
In `@apps/web/app/api/domains/`[domain]/domain-connect/apply/route.ts:
- Around line 106-111: The else branch redundantly sets queryParams.groupId =
"subdomain" even though queryParams.groupId was already assigned "subdomain"
earlier; simplify by removing that redundant assignment and only set
queryParams.host = (subdomain ?? "www").toLowerCase() inside the else
branch—keep the isApex conditional as-is and ensure queryParams.groupId is only
set to "apex" in the if branch and left unchanged otherwise.
In `@apps/web/app/api/domains/`[domain]/forward-instructions/route.ts:
- Line 74: The if-condition checking TXT inputs uses a redundant expression:
replace the check in the route handler where it currently reads ((txtHost ||
txtHost === "@") && txtValue) with a simplified truthy check txtHost &&
txtValue; update the conditional that references variables txtHost and txtValue
(inside the forward-instructions route handler) so it just validates both are
present without the unnecessary txtHost === "@" clause.
In `@apps/web/ui/domains/domain-configuration.tsx`:
- Around line 36-38: The initialization of the recordType state uses an
unnecessary double negation (!!subdomain); update the useState call for
recordType to use subdomain directly in the ternary (i.e., subdomain ? "CNAME" :
"A") so it remains functionally identical but simpler; locate the useState
invocation that declares const [recordType, setRecordType] = useState<"A" |
"CNAME">(...) and replace the !!subdomain expression with subdomain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22b8dba1-70a8-4b46-b5d2-1136a2df6945
📒 Files selected for processing (17)
apps/web/.env.exampleapps/web/app/api/domains/[domain]/domain-connect/apply/route.tsapps/web/app/api/domains/[domain]/forward-instructions/route.tsapps/web/app/api/domains/[domain]/verify/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/domains/page-client.tsxapps/web/lib/domain-connect/constants.tsapps/web/lib/domain-connect/discover.tsapps/web/lib/domain-connect/sign-apply-url.tsapps/web/lib/domain-connect/types.tsapps/web/ui/domains/domain-card.tsxapps/web/ui/domains/domain-configuration.tsxapps/web/ui/modals/forward-dns-instructions-modal.tsxapps/web/ui/partners/program-link-configuration.tsxpackages/email/src/templates/domain-dns-instructions.tsxpackages/ui/src/icons/cloudflare.tsxpackages/ui/src/icons/index.tsxpackages/ui/src/icons/vercel.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/lib/domain-connect/discover.ts`:
- Around line 47-52: The current candidates.find call uses substring matching
(h.includes("domainconnect") etc.) which allows hosts like "vercel.evil.tld" to
pass; replace that check with a strict allowlist or suffix-based hostname
validation: parse the candidate into a URL/hostname (e.g., new URL(...).hostname
or equivalent), then accept only exact matches or suffix matches that ensure a
dot boundary (e.g., hostname === "vercel.com" || hostname ===
"domainconnect.org" || hostname.endsWith(".vercel.com") becomes hostname ===
trusted || hostname.endsWith("." + trusted)); update the code that uses
candidates.find to validate the parsed hostname this way before performing the
fetch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c82a6839-d91d-40a1-8d80-610a3cdc46b7
📒 Files selected for processing (4)
apps/web/app/api/domains/[domain]/domain-connect/apply/route.tsapps/web/app/api/domains/[domain]/forward-instructions/route.tsapps/web/lib/domain-connect/discover.tsapps/web/ui/domains/domain-configuration.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/app/api/domains/[domain]/forward-instructions/route.ts
- apps/web/ui/domains/domain-configuration.tsx
- apps/web/app/api/domains/[domain]/domain-connect/apply/route.ts
Summary by CodeRabbit
New Features
Documentation
Style