-
-
Notifications
You must be signed in to change notification settings - Fork 367
feat: MAIL FROM label override and aggregate domain verification #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| -- AlterTable | ||
| ALTER TABLE "Domain" ADD COLUMN "mailFromLabel" TEXT; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { DomainStatus } from "@prisma/client"; | ||
|
|
||
| /** | ||
| * Severity order: worst first. Used to combine identity, DKIM, and MAIL FROM (SPF) checks. | ||
| */ | ||
| const STATUS_WORST_FIRST: DomainStatus[] = [ | ||
| DomainStatus.FAILED, | ||
| DomainStatus.TEMPORARY_FAILURE, | ||
| DomainStatus.PENDING, | ||
| DomainStatus.NOT_STARTED, | ||
| DomainStatus.SUCCESS, | ||
| ]; | ||
|
|
||
| function parseLooseStatus(value?: string | null): DomainStatus { | ||
| if (!value) { | ||
| return DomainStatus.NOT_STARTED; | ||
| } | ||
| const normalized = value.toUpperCase(); | ||
| if ((Object.values(DomainStatus) as string[]).includes(normalized)) { | ||
| return normalized as DomainStatus; | ||
| } | ||
| return DomainStatus.NOT_STARTED; | ||
| } | ||
|
|
||
| /** | ||
| * Single status for UX: all of SES identity verification, DKIM, and MAIL FROM (SPF) must be SUCCESS | ||
| * for the aggregate to be SUCCESS. | ||
| */ | ||
| export function aggregateDomainStatus(domain: { | ||
| status: DomainStatus; | ||
| dkimStatus?: string | null; | ||
| spfDetails?: string | null; | ||
| }): DomainStatus { | ||
| const parts: DomainStatus[] = [ | ||
| domain.status, | ||
| parseLooseStatus(domain.dkimStatus), | ||
| parseLooseStatus(domain.spfDetails), | ||
| ]; | ||
|
|
||
| let minIdx = STATUS_WORST_FIRST.length - 1; | ||
| for (const p of parts) { | ||
| const idx = STATUS_WORST_FIRST.indexOf(p); | ||
| if (idx !== -1 && idx < minIdx) { | ||
| minIdx = idx; | ||
| } | ||
| } | ||
| return STATUS_WORST_FIRST[minIdx]!; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { DomainStatus } from "@prisma/client"; | ||
| import { aggregateDomainStatus } from "~/lib/domain-aggregate-status"; | ||
|
|
||
| describe("aggregateDomainStatus", () => { | ||
| it("returns SUCCESS only when identity, DKIM, and SPF are all SUCCESS", () => { | ||
| expect( | ||
| aggregateDomainStatus({ | ||
| status: DomainStatus.SUCCESS, | ||
| dkimStatus: DomainStatus.SUCCESS, | ||
| spfDetails: DomainStatus.SUCCESS, | ||
| }), | ||
| ).toBe(DomainStatus.SUCCESS); | ||
| }); | ||
|
|
||
| it("returns the worst status across the three checks", () => { | ||
| expect( | ||
| aggregateDomainStatus({ | ||
| status: DomainStatus.SUCCESS, | ||
| dkimStatus: DomainStatus.SUCCESS, | ||
| spfDetails: DomainStatus.PENDING, | ||
| }), | ||
| ).toBe(DomainStatus.PENDING); | ||
| }); | ||
|
|
||
| it("treats FAILED as worse than PENDING", () => { | ||
| expect( | ||
| aggregateDomainStatus({ | ||
| status: DomainStatus.SUCCESS, | ||
| dkimStatus: DomainStatus.FAILED, | ||
| spfDetails: DomainStatus.PENDING, | ||
| }), | ||
| ).toBe(DomainStatus.FAILED); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,9 +8,10 @@ export const DomainDnsRecordSchema = z.object({ | |||||||
| description: "DNS record type", | ||||||||
| example: "TXT", | ||||||||
| }), | ||||||||
| name: z | ||||||||
| .string() | ||||||||
| .openapi({ description: "DNS record name", example: "mail" }), | ||||||||
| name: z.string().openapi({ | ||||||||
| description: | ||||||||
| "DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.", | ||||||||
|
||||||||
| "DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.", | |
| "DNS record name relative to the domain/zone. This may be a single label or multiple labels (for example, `selector1._domainkey` or a MAIL FROM subdomain such as `bounce`). For custom MAIL FROM MX and SPF TXT records, this is the MAIL FROM host name relative to the domain: `mailFromLabel` if set, otherwise the SES `region` value.", | |
| example: "selector1._domainkey", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify MAIL FROM DNS record names for subdomains.
The schema says this is “the first label,” but buildDnsRecords returns the full relative record name; for sub.example.com, a custom label becomes bounce.sub, not just bounce.
📝 Proposed wording update
name: z.string().openapi({
description:
- "DNS record name (hostname label). For custom MAIL FROM MX and SPF TXT records, this is the first label of the MAIL FROM host: the domain `mailFromLabel` if set, otherwise the SES `region` value.",
+ "DNS record name relative to the domain's DNS zone. For MAIL FROM MX and SPF TXT records, this starts with `mailFromLabel` when set, otherwise the SES `region`, and may include the domain subdomain prefix.",
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/zod/domain-schema.ts` around lines 11 - 14, The openapi
description for the zod schema field name (z.string().openapi(...) in
domain-schema.ts) misleadingly states "the first label" but buildDnsRecords
actually returns the full relative record name including subdomain (e.g., for
sub.example.com a custom label becomes "bounce.sub" not just "bounce"); update
the description text in the name field to clarify it is the full relative DNS
record name (first label plus any subdomain segment) and include a brief example
such as "for sub.example.com and custom label 'bounce' the record name is
'bounce.sub'".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aggregateDomainStatus(domain)here is computed fromteam.domains, but the admin API selection for domains (seeteamAdminSelectioninapps/web/src/server/api/routers/admin.ts) only includesstatus/isVerifyingand does not includedkimStatus/spfDetails. With missing fields,aggregateDomainStatustreats them asNOT_STARTED, so even aSUCCESSdomain will display asnot_started. Fix by includingdkimStatusandspfDetails(and any other required fields) in the admin query selection, or by usingdomain.statusdirectly when those fields aren’t present.