Skip to content

Commit 8a6ea99

Browse files
committed
fix: address PR #391 review (tracking UX, SES idempotency, schema uniqueness)
- Case-insensitive unsubscribe pre-check; reject apex tracking hostnames - PutConfigurationSetTrackingOptions throws on non-200; addWebhookConfiguration treats AlreadyExists as success and still returns boolean - Poll custom tracking verification on 6h cadence when tracking DNS is pending - Apply HTTPS policy to SES before persisting DB; create new tracking identity before removing old SES resources - Unique customTrackingHostname to avoid cross-domain SES cleanup clashes - Tests: mixed-case unsubscribe, verification due with pending tracking Made-with: Cursor
1 parent 1117082 commit 8a6ea99

7 files changed

Lines changed: 117 additions & 20 deletions

File tree

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
-- CreateIndex
2+
CREATE UNIQUE INDEX "Domain_customTrackingHostname_key" ON "Domain"("customTrackingHostname");

apps/web/prisma/schema.prisma

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ model Domain {
197197
sesTenantId String?
198198
isVerifying Boolean @default(false)
199199
/// Self-hosted: custom hostname for SES click/open tracking (e.g. track.example.com). Requires DNS + verification in SES.
200-
customTrackingHostname String?
200+
customTrackingHostname String? @unique
201201
customTrackingPublicKey String?
202202
customTrackingDkimSelector String? @default("utrack")
203203
customTrackingDkimStatus String?

apps/web/src/server/aws/ses.ts

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,12 @@ export async function putConfigurationSetHttpsTracking(
217217
HttpsPolicy: httpsPolicy,
218218
});
219219
const response = await sesClient.send(cmd);
220-
return response.$metadata.httpStatusCode === 200;
220+
const code = response.$metadata.httpStatusCode;
221+
if (code !== 200) {
222+
throw new Error(
223+
`PutConfigurationSetTrackingOptions failed for ${configurationSetName}: HTTP ${code ?? "unknown"}`,
224+
);
225+
}
221226
}
222227

223228
export async function deleteConfigurationSet(
@@ -379,6 +384,10 @@ export async function getAccount(region: string) {
379384
return response;
380385
}
381386

387+
function isAlreadyExistsError(error: unknown): boolean {
388+
return (error as { name?: string })?.name === "AlreadyExistsException";
389+
}
390+
382391
export async function addWebhookConfiguration(
383392
configName: string,
384393
topicArn: string,
@@ -391,10 +400,19 @@ export async function addWebhookConfiguration(
391400
ConfigurationSetName: configName,
392401
});
393402

394-
const configSetResponse = await sesClient.send(configSetCommand);
395-
396-
if (configSetResponse.$metadata.httpStatusCode !== 200) {
397-
throw new Error("Failed to create configuration set");
403+
try {
404+
const configSetResponse = await sesClient.send(configSetCommand);
405+
if (configSetResponse.$metadata.httpStatusCode !== 200) {
406+
throw new Error("Failed to create configuration set");
407+
}
408+
} catch (error: unknown) {
409+
if (!isAlreadyExistsError(error)) {
410+
throw error;
411+
}
412+
logger.debug(
413+
{ configName, region },
414+
"SES configuration set already exists; continuing",
415+
);
398416
}
399417

400418
const command = new CreateConfigurationSetEventDestinationCommand({
@@ -409,8 +427,22 @@ export async function addWebhookConfiguration(
409427
},
410428
});
411429

412-
const response = await sesClient.send(command);
413-
return response.$metadata.httpStatusCode === 200;
430+
try {
431+
const response = await sesClient.send(command);
432+
if (response.$metadata.httpStatusCode !== 200) {
433+
throw new Error("Failed to create configuration set event destination");
434+
}
435+
} catch (error: unknown) {
436+
if (!isAlreadyExistsError(error)) {
437+
throw error;
438+
}
439+
logger.debug(
440+
{ configName, region },
441+
"SES event destination already exists; continuing",
442+
);
443+
}
444+
445+
return true;
414446
}
415447

416448
/**

apps/web/src/server/service/domain-service.ts

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,15 @@ export async function setCustomTrackingHostname(
435435

436436
assertTrackingHostnameAllowed(domain.name, normalized);
437437

438+
const parsedHost = tldts.parse(normalized);
439+
if (!parsedHost.subdomain) {
440+
throw new UnsendApiError({
441+
code: "BAD_REQUEST",
442+
message:
443+
"Tracking hostname must be a subdomain (for example track.example.com), not the zone apex.",
444+
});
445+
}
446+
438447
if (
439448
domain.customTrackingHostname === normalized &&
440449
domain.customTrackingPublicKey
@@ -443,27 +452,26 @@ export async function setCustomTrackingHostname(
443452
trackingHttpsRequired !== undefined &&
444453
trackingHttpsRequired !== domain.trackingHttpsRequired
445454
) {
455+
const domainForSes: Domain = {
456+
...domain,
457+
trackingHttpsRequired,
458+
};
459+
await reapplyCustomTrackingSesPolicy(domainForSes);
446460
const updated = await db.domain.update({
447461
where: { id: domainId },
448462
data: { trackingHttpsRequired },
449463
});
450-
try {
451-
await reapplyCustomTrackingSesPolicy(updated);
452-
} catch (error) {
453-
logger.error(
454-
{ err: error, domainId },
455-
"[DomainService]: Failed to reapply custom tracking HTTPS policy",
456-
);
457-
}
458464
await emitDomainEvent(updated, "domain.updated");
459465
return updated;
460466
}
461467
return domain;
462468
}
463469

464-
if (domain.customTrackingHostname) {
465-
await removeCustomTrackingResources(domain);
466-
}
470+
const previousForCleanup =
471+
domain.customTrackingHostname &&
472+
domain.customTrackingHostname !== normalized
473+
? domain
474+
: null;
467475

468476
const selector = domain.customTrackingDkimSelector ?? "utrack";
469477
const publicKey = await ses.addTrackingEmailIdentity(
@@ -489,6 +497,10 @@ export async function setCustomTrackingHostname(
489497
},
490498
});
491499

500+
if (previousForCleanup) {
501+
await removeCustomTrackingResources(previousForCleanup);
502+
}
503+
492504
await emitDomainEvent(updated, "domain.updated");
493505
return updated;
494506
}
@@ -1134,6 +1146,15 @@ export async function isDomainVerificationDue(domain: Domain) {
11341146
return false;
11351147
}
11361148

1149+
if (shouldPollCustomTrackingVerification(domain)) {
1150+
const now = Date.now();
1151+
const lastCheckedAt = verificationState.lastCheckedAt?.getTime() ?? 0;
1152+
if (!verificationState.lastCheckedAt) {
1153+
return true;
1154+
}
1155+
return now - lastCheckedAt >= DOMAIN_UNVERIFIED_RECHECK_MS;
1156+
}
1157+
11371158
const now = Date.now();
11381159
const lastCheckedAt = verificationState.lastCheckedAt?.getTime() ?? 0;
11391160
const intervalMs =

apps/web/src/server/service/domain-service.unit.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ vi.mock("~/server/email-templates", () => ({
7070
renderDomainVerificationStatusEmail: mockRenderDomainVerificationStatusEmail,
7171
}));
7272

73+
vi.mock("~/env", () => ({
74+
env: {
75+
NEXT_PUBLIC_IS_CLOUD: false,
76+
NEXTAUTH_URL: "http://localhost:3000",
77+
},
78+
}));
79+
7380
import {
7481
DOMAIN_UNVERIFIED_RECHECK_MS,
7582
DOMAIN_VERIFIED_RECHECK_MS,
@@ -421,4 +428,32 @@ describe("domain-service", () => {
421428

422429
await expect(isDomainVerificationDue(domain)).resolves.toBe(false);
423430
});
431+
432+
it("uses unverified cadence when custom tracking is still pending even if the sending domain is verified", async () => {
433+
const domain = createDomain({
434+
status: DomainStatus.SUCCESS,
435+
customTrackingHostname: "track.example.com",
436+
customTrackingPublicKey: "pk",
437+
customTrackingStatus: DomainStatus.PENDING,
438+
});
439+
mockRedis.mget.mockResolvedValue([
440+
new Date(
441+
Date.now() - DOMAIN_UNVERIFIED_RECHECK_MS + 5 * 60 * 1000,
442+
).toISOString(),
443+
DomainStatus.SUCCESS,
444+
"1",
445+
]);
446+
447+
await expect(isDomainVerificationDue(domain)).resolves.toBe(false);
448+
449+
mockRedis.mget.mockResolvedValue([
450+
new Date(
451+
Date.now() - DOMAIN_UNVERIFIED_RECHECK_MS - 5 * 60 * 1000,
452+
).toISOString(),
453+
DomainStatus.SUCCESS,
454+
"1",
455+
]);
456+
457+
await expect(isDomainVerificationDue(domain)).resolves.toBe(true);
458+
});
424459
});

apps/web/src/server/utils/ses-tracking-html.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* @see https://docs.aws.amazon.com/ses/latest/dg/faqs-metrics.html
66
*/
77
export function addSesNoTrackToUnsubscribeLinks(html: string): string {
8-
if (!html.includes("unsubscribe")) {
8+
if (!/unsubscribe/i.test(html)) {
99
return html;
1010
}
1111

apps/web/src/server/utils/ses-tracking-html.unit.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,11 @@ describe("addSesNoTrackToUnsubscribeLinks", () => {
2222
const html = '<a href="https://example.com/">Home</a>';
2323
expect(addSesNoTrackToUnsubscribeLinks(html)).toBe(html);
2424
});
25+
26+
it("handles mixed-case unsubscribe in href", () => {
27+
const html =
28+
'<a href="https://app.example.com/api/Unsubscribe?id=1">x</a>';
29+
const out = addSesNoTrackToUnsubscribeLinks(html);
30+
expect(out).toContain("ses:no-track");
31+
});
2532
});

0 commit comments

Comments
 (0)