feat: add email forwarding infrastructure#362
Conversation
|
Someone is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR implements inbound email forwarding functionality for Unsend. It adds database models for forwarding rules and inbound emails, creates API endpoints and TRPC procedures to manage forwarding configurations, implements an SNS callback handler for receiving inbound emails from AWS SES, introduces job workers for email processing and cleanup, adds UI components for managing forwarding rules and viewing inbound email logs, and includes AWS SES and S3 integration utilities for receipt rule management and email retrieval. The implementation tracks forwarded emails through status transitions, rewrites email headers, and integrates with the existing email pipeline. Possibly related PRs
Suggested labels
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. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 413f5c329a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const hopCount = getHopCount(parsed); | ||
| if (hopCount > MAX_FORWARDING_HOPS) { |
There was a problem hiding this comment.
The loop guard in processInboundEmail uses hopCount > MAX_FORWARDING_HOPS, which still forwards messages that are already at the configured cap (3 hops) and then increments the header to 4 on the outgoing message. That means loop prevention allows one extra pass, so a message already at the limit is not dropped as intended. Use >= here so emails at the max hop count are rejected before forwarding.
Useful? React with 👍 / 👎.
| const inboundEmail = await db.inboundEmail.create({ | ||
| data: { | ||
| teamId: domain.teamId, | ||
| domainId: domain.id, | ||
| from: fromAddress ?? "unknown", |
There was a problem hiding this comment.
This handler always creates a new InboundEmail row and enqueues a job keyed by that new CUID, but it never uses a stable SES message identifier (such as mail.messageId) to enforce idempotency. Because SNS delivery is at-least-once and retries on 5xx/timeouts, the same inbound message can be delivered more than once and get forwarded multiple times to the destination address. Add a unique dedupe key from the SNS/SES payload and short-circuit duplicate notifications before insert/queue.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
apps/web/src/server/aws/s3-inbound.ts (1)
15-27: Consider caching S3Client instances per region.A new
S3Clientis instantiated on everyfetchRawEmailcall. For high-volume inbound processing, consider caching clients per region to reduce overhead.♻️ Optional: Cache S3Client per region
+const s3ClientCache = new Map<string, S3Client>(); + function getS3Client(region: string) { + const cached = s3ClientCache.get(region); + if (cached) { + return cached; + } + const client = new S3Client({ - return new S3Client({ region, credentials: { accessKeyId: env.AWS_ACCESS_KEY, secretAccessKey: env.AWS_SECRET_KEY, }, }); + s3ClientCache.set(region, client); + return client; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/aws/s3-inbound.ts` around lines 15 - 27, fetchRawEmail currently calls getS3Client which creates a new S3Client on every invocation; change this to reuse S3Client instances per region by adding a module-level cache (e.g., a Map keyed by region) and updating getS3Client (or create a getOrCreateS3Client) to return the cached client if present, otherwise instantiate S3Client, store it in the cache, and return it; ensure the cache keys on the region string and preserve the existing getS3Client/fetchRawEmail signatures so callers are unaffected.apps/web/prisma/schema.prisma (1)
541-562: Consider adding an index onteamIdforInboundEmail.The
InboundEmailmodel has indexes on[domainId, status]andcreatedAt, but team-scoped queries (e.g., listing all inbound emails for a team) would benefit from an index onteamId. TheEmailForwardingRulemodel includes@@index([teamId])for this purpose.♻️ Add teamId index
@@index([domainId, status]) @@index([createdAt(sort: Desc)]) + @@index([teamId]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/prisma/schema.prisma` around lines 541 - 562, Add an index on teamId in the InboundEmail Prisma model to speed up team-scoped queries: update the InboundEmail model (where fields like id, teamId, domainId, etc. are defined) to include an @@index([teamId]) declaration alongside the existing @@index([domainId, status]) and @@index([createdAt(sort: Desc)]) entries so queries filtering by teamId use the new index.docs/plans/2026-02-25-email-forwarding-design.md (1)
41-41: Add language specifier to fenced code block.The ASCII diagram code block is missing a language specifier. Use
textor leave empty with proper configuration to satisfy markdownlint.-``` +```text External Sender🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-02-25-email-forwarding-design.md` at line 41, The fenced code block containing the ASCII diagram line "External Sender" lacks a language specifier; update that block to use a language tag such as text (e.g., change the opening triple backticks to ```text) so markdownlint is satisfied and the diagram is treated as plain text.apps/web/src/app/api/inbound_callback/route.unit.test.ts (1)
265-278: Clean up fetch spy after test.The
fetchspy is not restored after the test, which could affect other tests if they rely on the globalfetch. Consider usingmockRestore()in a cleanup or wrapping withvi.restoreAllMocks()inafterEach.♻️ Suggested fix
it("confirms SNS subscription with awaited fetch", async () => { const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue( new Response("ok", { status: 200 }) ); const res = await POST(makeSubscriptionRequest()); const body = await res.json(); expect(body.data).toBe("Subscription confirmed"); expect(fetchSpy).toHaveBeenCalledWith( "https://sns.us-east-1.amazonaws.com/?Action=ConfirmSubscription", { method: "GET" } ); + + fetchSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/inbound_callback/route.unit.test.ts` around lines 265 - 278, The test creates a global fetch spy (fetchSpy) but never restores it; add cleanup to restore the mock after the test suite so other tests aren't affected—either call fetchSpy.mockRestore() at the end of this test or add an afterEach hook that calls vi.restoreAllMocks(); locate the spy in the "confirms SNS subscription with awaited fetch" test that wraps globalThis.fetch and restore it after calling POST(makeSubscriptionRequest()) (or globally in afterEach) to ensure the fetch mock is removed.apps/web/src/server/public-api/api/domains/list-forwarding-rules.ts (1)
59-65: Response shape may include extra fields.The spread
...rincludes all fields from the database record (id, teamId, domainId, sourceAddress, destinationAddress, enabled, createdAt, updatedAt), but the schema only defines a subset. While extra fields are typically stripped by Zod validation in OpenAPI, consider explicitly mapping only the fields defined in the schema for clarity and to avoid leaking internal fields liketeamIdanddomainId.♻️ Suggested explicit mapping
return c.json({ data: rules.map((r) => ({ - ...r, + id: r.id, + sourceAddress: r.sourceAddress, + destinationAddress: r.destinationAddress, + enabled: r.enabled, createdAt: r.createdAt.toISOString(), updatedAt: r.updatedAt.toISOString(), })), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/public-api/api/domains/list-forwarding-rules.ts` around lines 59 - 65, The response currently spreads the entire DB record (using ...r) which may leak internal fields (teamId, domainId); update the mapping in list-forwarding-rules.ts where rules.map is used to explicitly return only the schema-defined fields (e.g., id, sourceAddress, destinationAddress, enabled) and convert timestamps to strings for createdAt and updatedAt instead of spreading r, so the JSON response contains only those allowed properties.apps/web/src/server/public-api/api/domains/update-domain-inbound.ts (1)
89-100: Consider atomicity of SES rule creation and DB update.If
createReceiptRulesucceeds but the subsequentdb.domain.updatefails, an orphan SES receipt rule will exist. This matches the TRPC implementation behavior, but consider wrapping in a try-catch to delete the SES rule on DB failure, or document this as a known edge case for operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/public-api/api/domains/update-domain-inbound.ts` around lines 89 - 100, The SES receipt rule creation and DB update must be made atomic: after calling createReceiptRule(...) which returns ruleName, wrap the subsequent db.domain.update(...) in a try-catch and if the DB update throws, call the corresponding SES cleanup function (e.g., deleteReceiptRule or an equivalent SES API wrapper) to remove the created ruleName from the rule set (passing domain.name, domain.region, ruleSetName and ruleName), then rethrow or return the error; ensure you reference createReceiptRule, ruleName and db.domain.update when adding the try-catch and cleanup call.apps/web/src/server/aws/ses-receipt-rules.ts (1)
17-25: Consider usingAWS_SES_ENDPOINTfor consistency with other SES clients.Other parts of the codebase may use
env.AWS_SES_ENDPOINTfor local development or testing. If receipt rule operations should be testable locally (e.g., with LocalStack), consider adding the endpoint configuration.♻️ Suggested fix
function getSesV1Client(region: string) { return new SESClient({ region, credentials: { accessKeyId: env.AWS_ACCESS_KEY, secretAccessKey: env.AWS_SECRET_KEY, }, + ...(env.AWS_SES_ENDPOINT && { endpoint: env.AWS_SES_ENDPOINT }), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/aws/ses-receipt-rules.ts` around lines 17 - 25, getSesV1Client currently constructs an SESClient with region and credentials only, which prevents using a custom endpoint (e.g., LocalStack) via env.AWS_SES_ENDPOINT; update getSesV1Client to read env.AWS_SES_ENDPOINT and, when present, pass an endpoint option into the SESClient configuration so receipt rule operations can target a local/test SES endpoint while keeping region/credentials as-is.apps/web/src/server/api/routers/forwarding.trpc.test.ts (1)
66-68: Minor: Explicit block body for clarity.The inner
forEachcallback implicitly returns the result ofmockReset(). While harmless, using braces makes the intent clearer and silences the static analysis warning.♻️ Suggested fix
beforeEach(() => { Object.values(mockDb).forEach((model) => { - Object.values(model).forEach((fn) => (fn as ReturnType<typeof vi.fn>).mockReset()); + Object.values(model).forEach((fn) => { + (fn as ReturnType<typeof vi.fn>).mockReset(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/api/routers/forwarding.trpc.test.ts` around lines 66 - 68, Update the inner forEach callback so it uses an explicit block body instead of an implicit return: in the code iterating Object.values(mockDb).forEach((model) => { Object.values(model).forEach((fn) => { (fn as ReturnType<typeof vi.fn>).mockReset(); }); }); — specifically change the inner arrow function for the model members to use braces and call mockReset() as a statement (no implicit return) to make the intent clear and remove the lint warning.apps/web/src/app/(dashboard)/domains/[domainId]/forwarding-tab.tsx (1)
305-319: Consider disabling rule actions during mutations.The rule toggle switch and delete button don't disable during their respective mutations, which could allow rapid double-clicks causing duplicate requests. This is a minor UX polish.
♻️ Suggested improvement
<div className="flex items-center justify-end gap-2"> <Switch checked={rule.enabled} onCheckedChange={() => handleToggleRule(rule.id, rule.enabled) } className="data-[state=checked]:bg-success scale-75" + disabled={updateRule.isPending} /> <Button variant="ghost" size="sm" onClick={() => handleDeleteRule(rule.id)} className="text-destructive hover:text-destructive" + disabled={deleteRule.isPending} > <Trash2 className="h-4 w-4" /> </Button> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/domains/[domainId]/forwarding-tab.tsx around lines 305 - 319, The Switch and delete Button must be disabled while their respective mutations are in flight to prevent duplicate requests; update the toggle and delete logic (handleToggleRule and handleDeleteRule) to set and clear a per-rule mutation state (e.g., currentTogglingRuleId/currentDeletingRuleId or a map of loading rule IDs) and pass a disabled prop to the Switch and Button (and guard onCheckedChange/onClick to no-op when that rule is loading). Specifically, modify the Switch and Button rendering in forwarding-tab.tsx to derive disabled from those mutation flags for the given rule.id and ensure handleToggleRule/handleDeleteRule check the flag before issuing the mutation and clear it on success/error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/inbound_callback/route.ts`:
- Around line 114-120: The logger.info call is currently emitting raw PII
(fromAddress and recipientAddress); change it to log redacted or non-PII
variants instead by replacing the from/to values with either a hashed value or
domain-only value (e.g., use a helper like redactEmail(email) or
extractDomain(email) or hashEmail(email) and call that inside the logger.info).
Update the logger.info invocation that references inboundEmail.id, domain.id,
fromAddress, recipientAddress to pass the redacted/hash/domain-only results (and
add the helper function in the same module or a util file), so no full email
addresses are logged at info level.
- Around line 90-112: The current logic creates an InboundEmail with
db.inboundEmail.create which allows duplicates when SNS retries; add a nullable
sesMessageId field and a unique constraint on (domainId, sesMessageId) in the
Prisma model, then replace the create call with an upsert (using
db.inboundEmail.upsert or equivalent) keyed on the unique constraint so creating
is idempotent using sesMessageId (populate from snsMessage / messageId), and
ensure the same inboundEmail.id is used when calling inboundEmailQueue.add so
retries reuse the existing row instead of inserting duplicates.
- Around line 159-170: Replace the weak TopicArn check in checkEventValidity
with full AWS SNS signature verification: validate SignatureVersion, download
the SigningCertURL over HTTPS with proper hostname/cert checks, build the
canonical string from message fields per AWS SNS docs, and verify the Signature
using the downloaded certificate (use a crypto/X509 verify flow) while still
allowing a bypass in development; also add an AbortController/timeout to the
fetch of message.SubscribeURL to avoid hangs and redact or avoid logging plain
email addresses (mask local-part or remove PII) where emails are currently
logged to prevent exposing sensitive data.
- Around line 145-147: Replace the bare fetch in the SNS subscription
confirmation block with a timed fetch using an AbortController: create an
AbortController, set a timeout (use setTimeout) to call controller.abort(), pass
controller.signal into fetch(message.SubscribeURL!), await the response and
validate response.ok (or status 200–299) before calling logger.info; clear the
timeout on success and handle AbortError/non-OK responses by logging or throwing
so the handler does not falsely log "Confirmed inbound SNS subscription" for
failed or hung requests (affecting the fetch call and logger.info referenced in
route.ts).
In `@apps/web/src/server/aws/ses-receipt-rules.ts`:
- Around line 67-73: The SES client calls that check
response.$metadata.httpStatusCode (around the client.send(command) calls) must
be wrapped in try-catch so API errors thrown by AWS SDK v3 are caught; modify
the blocks that call client.send(command) (and where response is inspected) to
catch exceptions, detect SES errors by checking instanceof SESServiceException
or error.name, log the error and include error.$metadata.httpStatusCode only as
supplemental info, and rethrow or return a controlled error instead of relying
on a non-2xx response check; update both places that currently inspect
response.$metadata.httpStatusCode to follow this try-catch pattern.
In `@apps/web/src/server/public-api/api/domains/create-forwarding-rule.ts`:
- Around line 83-88: The REST handler in create-forwarding-rule.ts throws an
UnsendApiError with code "BAD_REQUEST" when a duplicate forwarding rule is found
(variable existing); change this to use code "CONFLICT" to match the TRPC
createRule behavior—update the throw to new UnsendApiError({ code: "CONFLICT",
message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`
}) so both API surfaces return HTTP 409 for duplicates.
In `@apps/web/src/server/service/inbound-email-service.ts`:
- Around line 64-76: The forwarding-loop check currently uses `if (hopCount >
MAX_FORWARDING_HOPS)` which is off-by-one for a 3-hop limit; change the
condition to `if (hopCount >= MAX_FORWARDING_HOPS)` (in the block using
`getHopCount(parsed)`, `hopCount`, and `MAX_FORWARDING_HOPS`) so emails hitting
the maximum allowed hops are treated as a loop, keeping the existing
`logger.warn` and `updateInboundEmailStatus(inboundEmailId,
InboundEmailStatus.FAILED, ...)` behavior intact.
- Around line 122-127: The rewritten From header construction in
inbound-email-service.ts uses originalFromName directly which allows double
quotes or control characters to break the header; sanitize originalFromName
before building rewrittenFrom by removing or escaping double quotes and
stripping control characters (CR, LF, non-printables) and use the sanitized
value in the `"${...} via Unsend" <...>` template (e.g., add a helper like
sanitizeDisplayName and call it where originalFromName is referenced to produce
sanitizedFromName, then use sanitizedFromName when creating rewrittenFrom).
- Line 191: The assignment to const snsData = JSON.parse(snsMessage) can throw
on invalid JSON; wrap the JSON.parse(snsMessage) call in a try-catch inside the
same function (the inbound email processing function in
inbound-email-service.ts) to catch SyntaxError, log a clear error including the
raw snsMessage and error.message via your logger, and then handle the failure
path (return an appropriate error response or throw a wrapped/typed error)
instead of letting the raw exception propagate.
---
Nitpick comments:
In `@apps/web/prisma/schema.prisma`:
- Around line 541-562: Add an index on teamId in the InboundEmail Prisma model
to speed up team-scoped queries: update the InboundEmail model (where fields
like id, teamId, domainId, etc. are defined) to include an @@index([teamId])
declaration alongside the existing @@index([domainId, status]) and
@@index([createdAt(sort: Desc)]) entries so queries filtering by teamId use the
new index.
In `@apps/web/src/app/`(dashboard)/domains/[domainId]/forwarding-tab.tsx:
- Around line 305-319: The Switch and delete Button must be disabled while their
respective mutations are in flight to prevent duplicate requests; update the
toggle and delete logic (handleToggleRule and handleDeleteRule) to set and clear
a per-rule mutation state (e.g., currentTogglingRuleId/currentDeletingRuleId or
a map of loading rule IDs) and pass a disabled prop to the Switch and Button
(and guard onCheckedChange/onClick to no-op when that rule is loading).
Specifically, modify the Switch and Button rendering in forwarding-tab.tsx to
derive disabled from those mutation flags for the given rule.id and ensure
handleToggleRule/handleDeleteRule check the flag before issuing the mutation and
clear it on success/error.
In `@apps/web/src/app/api/inbound_callback/route.unit.test.ts`:
- Around line 265-278: The test creates a global fetch spy (fetchSpy) but never
restores it; add cleanup to restore the mock after the test suite so other tests
aren't affected—either call fetchSpy.mockRestore() at the end of this test or
add an afterEach hook that calls vi.restoreAllMocks(); locate the spy in the
"confirms SNS subscription with awaited fetch" test that wraps globalThis.fetch
and restore it after calling POST(makeSubscriptionRequest()) (or globally in
afterEach) to ensure the fetch mock is removed.
In `@apps/web/src/server/api/routers/forwarding.trpc.test.ts`:
- Around line 66-68: Update the inner forEach callback so it uses an explicit
block body instead of an implicit return: in the code iterating
Object.values(mockDb).forEach((model) => { Object.values(model).forEach((fn) =>
{ (fn as ReturnType<typeof vi.fn>).mockReset(); }); }); — specifically change
the inner arrow function for the model members to use braces and call
mockReset() as a statement (no implicit return) to make the intent clear and
remove the lint warning.
In `@apps/web/src/server/aws/s3-inbound.ts`:
- Around line 15-27: fetchRawEmail currently calls getS3Client which creates a
new S3Client on every invocation; change this to reuse S3Client instances per
region by adding a module-level cache (e.g., a Map keyed by region) and updating
getS3Client (or create a getOrCreateS3Client) to return the cached client if
present, otherwise instantiate S3Client, store it in the cache, and return it;
ensure the cache keys on the region string and preserve the existing
getS3Client/fetchRawEmail signatures so callers are unaffected.
In `@apps/web/src/server/aws/ses-receipt-rules.ts`:
- Around line 17-25: getSesV1Client currently constructs an SESClient with
region and credentials only, which prevents using a custom endpoint (e.g.,
LocalStack) via env.AWS_SES_ENDPOINT; update getSesV1Client to read
env.AWS_SES_ENDPOINT and, when present, pass an endpoint option into the
SESClient configuration so receipt rule operations can target a local/test SES
endpoint while keeping region/credentials as-is.
In `@apps/web/src/server/public-api/api/domains/list-forwarding-rules.ts`:
- Around line 59-65: The response currently spreads the entire DB record (using
...r) which may leak internal fields (teamId, domainId); update the mapping in
list-forwarding-rules.ts where rules.map is used to explicitly return only the
schema-defined fields (e.g., id, sourceAddress, destinationAddress, enabled) and
convert timestamps to strings for createdAt and updatedAt instead of spreading
r, so the JSON response contains only those allowed properties.
In `@apps/web/src/server/public-api/api/domains/update-domain-inbound.ts`:
- Around line 89-100: The SES receipt rule creation and DB update must be made
atomic: after calling createReceiptRule(...) which returns ruleName, wrap the
subsequent db.domain.update(...) in a try-catch and if the DB update throws,
call the corresponding SES cleanup function (e.g., deleteReceiptRule or an
equivalent SES API wrapper) to remove the created ruleName from the rule set
(passing domain.name, domain.region, ruleSetName and ruleName), then rethrow or
return the error; ensure you reference createReceiptRule, ruleName and
db.domain.update when adding the try-catch and cleanup call.
In `@docs/plans/2026-02-25-email-forwarding-design.md`:
- Line 41: The fenced code block containing the ASCII diagram line "External
Sender" lacks a language specifier; update that block to use a language tag such
as text (e.g., change the opening triple backticks to ```text) so markdownlint
is satisfied and the diagram is treated as plain text.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.cursor/rules/design-system.mdcapps/web/package.jsonapps/web/prisma/schema.prismaapps/web/src/app/(dashboard)/domains/[domainId]/forwarding-tab.tsxapps/web/src/app/(dashboard)/domains/[domainId]/page.tsxapps/web/src/app/api/inbound_callback/route.tsapps/web/src/app/api/inbound_callback/route.unit.test.tsapps/web/src/env.jsapps/web/src/server/api/root.tsapps/web/src/server/api/routers/forwarding.trpc.test.tsapps/web/src/server/api/routers/forwarding.tsapps/web/src/server/aws/s3-inbound.tsapps/web/src/server/aws/ses-receipt-rules.tsapps/web/src/server/jobs/inbound-email-cleanup-job.tsapps/web/src/server/jobs/inbound-email-worker.tsapps/web/src/server/public-api/api/domains/create-forwarding-rule.tsapps/web/src/server/public-api/api/domains/delete-forwarding-rule.tsapps/web/src/server/public-api/api/domains/list-forwarding-rules.tsapps/web/src/server/public-api/api/domains/update-domain-inbound.tsapps/web/src/server/public-api/index.tsapps/web/src/server/queue/queue-constants.tsapps/web/src/server/service/inbound-email-service.tsapps/web/src/server/service/inbound-email-service.unit.test.tsapps/web/src/server/service/ses-hook-parser.tsdocs/plans/2026-02-25-email-forwarding-design.mddocs/plans/2026-02-25-email-forwarding-scoping.md
| const inboundEmail = await db.inboundEmail.create({ | ||
| data: { | ||
| teamId: domain.teamId, | ||
| domainId: domain.id, | ||
| from: fromAddress ?? "unknown", | ||
| to: recipientAddress, | ||
| subject, | ||
| s3Key, | ||
| status: "RECEIVED", | ||
| }, | ||
| }); | ||
|
|
||
| await inboundEmailQueue.add( | ||
| inboundEmail.id, | ||
| { | ||
| inboundEmailId: inboundEmail.id, | ||
| teamId: domain.teamId, | ||
| domainId: domain.id, | ||
| snsMessage, | ||
| s3Key, | ||
| }, | ||
| { jobId: inboundEmail.id, ...DEFAULT_QUEUE_OPTIONS } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd schema.prismaRepository: usesend/useSend
Length of output: 88
🏁 Script executed:
rg -n --type=prisma -C3 'model InboundEmail|@@unique|messageId|sesMessageId'Repository: usesend/useSend
Length of output: 89
🏁 Script executed:
cd apps/web && cat -n src/app/api/inbound_callback/route.ts | sed -n '85,135p'Repository: usesend/useSend
Length of output: 1685
🏁 Script executed:
cat -n apps/web/prisma/schema.prisma | grep -A 30 'model InboundEmail'Repository: usesend/useSend
Length of output: 1103
🏁 Script executed:
rg -n 'model InboundEmail' -A 30 apps/web/prisma/schema.prismaRepository: usesend/useSend
Length of output: 1037
🏁 Script executed:
rg -n 'mail\?.messageId|parsedMessage.mail' apps/web/src/app/api/inbound_callback/route.tsRepository: usesend/useSend
Length of output: 408
Add idempotency to inbound email creation to prevent duplicates on SNS retries.
Lines 90-112 persist an inboundEmail row before queueing. If the enqueue fails and line 129 returns 500, SNS will retry. Since create() lacks idempotency protection, this causes duplicate rows and duplicate email forwards.
Schema prerequisite: The InboundEmail model currently lacks a sesMessageId field and unique constraint. Add these first:
model InboundEmail {
// ... existing fields
sesMessageId String?
// ... relations
@@unique([domainId, sesMessageId])
}Then use upsert with this constraint instead of create:
Suggested fix
+const sesMessageId = parsedMessage.mail?.messageId;
+if (!sesMessageId) {
+ logger.warn("Missing SES messageId");
+ return Response.json({ data: "Missing messageId" });
+}
+
+const inboundEmail = await db.inboundEmail.upsert({
+ where: { domainId_sesMessageId: { domainId: domain.id, sesMessageId } },
+ update: {},
+ create: {
+ teamId: domain.teamId,
+ domainId: domain.id,
+ from: fromAddress ?? "unknown",
+ to: recipientAddress,
+ subject,
+ s3Key,
+ status: "RECEIVED",
+ sesMessageId,
+ },
+});Also applies to: 125-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/inbound_callback/route.ts` around lines 90 - 112, The
current logic creates an InboundEmail with db.inboundEmail.create which allows
duplicates when SNS retries; add a nullable sesMessageId field and a unique
constraint on (domainId, sesMessageId) in the Prisma model, then replace the
create call with an upsert (using db.inboundEmail.upsert or equivalent) keyed on
the unique constraint so creating is idempotent using sesMessageId (populate
from snsMessage / messageId), and ensure the same inboundEmail.id is used when
calling inboundEmailQueue.add so retries reuse the existing row instead of
inserting duplicates.
There was a problem hiding this comment.
The plan was deliberately simple for MVP. It didn't address SNS retry idempotency because:
- SNS retries are relatively rare (they only happen on 5xx or timeouts from the callback)
- The callback returns 200 quickly in all success paths, so retries should be uncommon
- Duplicate InboundEmail rows are low-impact — the downstream worker processes both, but the actual forwarded email is deduplicated at the SES send level (SES has its own message deduplication)
That said, the feedback is still a valid improvement it's just not a bug that's likely to cause real user-facing issues.
There was a problem hiding this comment.
@KalebJG, thanks for the context! That makes sense for MVP scope. The SES-level deduplication on the send side is a good safety net, and you're right that with quick 200 responses, SNS retries should be uncommon in practice.
Appreciate you walking through the tradeoffs—sounds like a reasonable line to draw for initial delivery. If you ever want to tighten it up post-MVP (especially if retry patterns change or if you want cleaner audit logs), the sesMessageId + upsert approach is there as an option. 👍
| logger.info( | ||
| { | ||
| inboundEmailId: inboundEmail.id, | ||
| domainId: domain.id, | ||
| from: fromAddress, | ||
| to: recipientAddress, | ||
| }, |
There was a problem hiding this comment.
Avoid logging raw sender/recipient addresses at info level.
Lines 118-120 log full email addresses (from, to), which are PII. Prefer redacted/hash/domain-only fields for routine logs.
🧹 Proposed fix
logger.info(
{
inboundEmailId: inboundEmail.id,
domainId: domain.id,
- from: fromAddress,
- to: recipientAddress,
+ fromDomain: fromAddress?.split("@")[1],
+ toDomain: recipientAddress.split("@")[1],
},
"Enqueued inbound email for processing"
);📝 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.
| logger.info( | |
| { | |
| inboundEmailId: inboundEmail.id, | |
| domainId: domain.id, | |
| from: fromAddress, | |
| to: recipientAddress, | |
| }, | |
| logger.info( | |
| { | |
| inboundEmailId: inboundEmail.id, | |
| domainId: domain.id, | |
| fromDomain: fromAddress?.split("@")[1], | |
| toDomain: recipientAddress.split("@")[1], | |
| }, | |
| "Enqueued inbound email for processing" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/inbound_callback/route.ts` around lines 114 - 120, The
logger.info call is currently emitting raw PII (fromAddress and
recipientAddress); change it to log redacted or non-PII variants instead by
replacing the from/to values with either a hashed value or domain-only value
(e.g., use a helper like redactEmail(email) or extractDomain(email) or
hashEmail(email) and call that inside the logger.info). Update the logger.info
invocation that references inboundEmail.id, domain.id, fromAddress,
recipientAddress to pass the redacted/hash/domain-only results (and add the
helper function in the same module or a util file), so no full email addresses
are logged at info level.
There was a problem hiding this comment.
The feedback is valid but shouldn't be fixed in isolation. The entire codebase logs full email addresses at info level (including mailer.ts which logs emails with sign-in tokens), so redacting this one log line would be inconsistent and give a false sense of compliance if PII in logs is a real concern, it needs a global solution like a Pino serializer, not a one-off patch.
There was a problem hiding this comment.
@KalebJG, you make a fair point about consistency—applying redaction to just this line would create a patchwork approach when the logging pattern is used throughout the codebase. A global solution like a Pino serializer (to redact or hash PII fields automatically) would indeed be the right way to handle this systematically if you decide to address it.
If PII in logs becomes a compliance or security requirement down the road, you might want to track this as tech debt and implement a codebase-wide redaction strategy at that time. For now, keeping the logging consistent with the rest of the application makes sense.
✏️ Learnings added
Learnt from: KalebJG
Repo: usesend/useSend PR: 362
File: apps/web/src/app/api/inbound_callback/route.ts:114-120
Timestamp: 2026-02-27T05:51:42.679Z
Learning: The useSend codebase intentionally logs full email addresses at info level throughout (e.g., mailer.ts, inbound_callback route). The team prefers consistency and would address PII redaction globally via a Pino serializer if needed, rather than one-off fixes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| try { | ||
| await fetch(message.SubscribeURL!, { method: "GET" }); | ||
| logger.info({ topicArn }, "Confirmed inbound SNS subscription"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/api/inbound_callback/route.ts | sed -n '140,155p'Repository: usesend/useSend
Length of output: 616
🏁 Script executed:
rg -n 'fetch\(' apps/web/src/app/api/ -A 2 --type=ts -B 1 | head -40Repository: usesend/useSend
Length of output: 716
🏁 Script executed:
fd -t f -e ts -e tsx apps/web/src/ | xargs rg -l 'AbortController|fetch.*timeout|fetchWithTimeout' | head -10Repository: usesend/useSend
Length of output: 480
🏁 Script executed:
rg -n 'AbortController|setTimeout.*abort|fetch.*signal' apps/web/src/server/service/webhook-service.ts -B 2 -A 2Repository: usesend/useSend
Length of output: 211
🏁 Script executed:
rg -n 'AbortController' apps/web/src/server/service/webhook-service.ts -B 2 -A 10Repository: usesend/useSend
Length of output: 463
🏁 Script executed:
rg -n 'WEBHOOK_REQUEST_TIMEOUT_MS' apps/web/src/server/service/webhook-service.ts -B 2 -A 2Repository: usesend/useSend
Length of output: 367
🏁 Script executed:
rg -n 'fetch.*signal|res\\.ok' apps/web/src/server/service/webhook-service.ts -B 2 -A 5Repository: usesend/useSend
Length of output: 41
🏁 Script executed:
sed -n '673,710p' apps/web/src/server/service/webhook-service.tsRepository: usesend/useSend
Length of output: 1132
🏁 Script executed:
sed -n '673,730p' apps/web/src/server/service/webhook-service.tsRepository: usesend/useSend
Length of output: 1630
Add timeout and response validation to SNS subscription confirmation fetch.
Line 146 makes an outbound fetch with no timeout or response validation. A hung upstream request could block handler indefinitely. Add AbortController with timeout and validate the response status.
The codebase establishes this pattern in webhook-service.ts (using AbortController + setTimeout with status check), so applying it here maintains consistency.
⏱️ Proposed fix
try {
- await fetch(message.SubscribeURL!, { method: "GET" });
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ try {
+ const res = await fetch(message.SubscribeURL!, {
+ method: "GET",
+ signal: controller.signal,
+ });
+ if (!res.ok) {
+ throw new Error(`SNS subscription confirmation failed: ${res.status}`);
+ }
+ } finally {
+ clearTimeout(timeout);
+ }
logger.info({ topicArn }, "Confirmed inbound SNS subscription");
} catch (err) {📝 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.
| try { | |
| await fetch(message.SubscribeURL!, { method: "GET" }); | |
| logger.info({ topicArn }, "Confirmed inbound SNS subscription"); | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| try { | |
| const res = await fetch(message.SubscribeURL!, { | |
| method: "GET", | |
| signal: controller.signal, | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`SNS subscription confirmation failed: ${res.status}`); | |
| } | |
| } finally { | |
| clearTimeout(timeout); | |
| } | |
| logger.info({ topicArn }, "Confirmed inbound SNS subscription"); | |
| } catch (err) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/inbound_callback/route.ts` around lines 145 - 147,
Replace the bare fetch in the SNS subscription confirmation block with a timed
fetch using an AbortController: create an AbortController, set a timeout (use
setTimeout) to call controller.abort(), pass controller.signal into
fetch(message.SubscribeURL!), await the response and validate response.ok (or
status 200–299) before calling logger.info; clear the timeout on success and
handle AbortError/non-OK responses by logging or throwing so the handler does
not falsely log "Confirmed inbound SNS subscription" for failed or hung requests
(affecting the fetch call and logger.info referenced in route.ts).
| function checkEventValidity(message: SnsNotification): boolean { | ||
| if (env.NODE_ENV === "development") { | ||
| return true; | ||
| } | ||
|
|
||
| const { TopicArn } = message; | ||
|
|
||
| if (!env.INBOUND_SNS_TOPIC_ARN) { | ||
| return false; | ||
| } | ||
|
|
||
| return TopicArn === env.INBOUND_SNS_TOPIC_ARN; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/api/inbound_callback/route.ts | head -200Repository: usesend/useSend
Length of output: 6885
🏁 Script executed:
rg -n --type=ts 'Signature|SigningCertURL|SignatureVersion|verify' apps/web/src/app/api/inbound_callback/route.tsRepository: usesend/useSend
Length of output: 41
🏁 Script executed:
rg -n --type=ts 'import.*sns|from.*aws|aws-sdk' apps/web/src/app/api/inbound_callback/route.tsRepository: usesend/useSend
Length of output: 41
🏁 Script executed:
rg -i 'signature|signing.*cert|sns.*verify|verifysnsmessage' --type=ts --type=tsx -lRepository: usesend/useSend
Length of output: 86
🏁 Script executed:
rg -i 'aws.*sns|sns.*signature' --type=ts --type=tsx -A2 -B2Repository: usesend/useSend
Length of output: 86
🏁 Script executed:
fd -e ts -e tsx | xargs grep -l 'aws-sdk\|@aws-sdk\|sns' 2>/dev/null | head -20Repository: usesend/useSend
Length of output: 692
🏁 Script executed:
cat -n apps/web/src/server/aws/sns.tsRepository: usesend/useSend
Length of output: 1569
🏁 Script executed:
cat -n apps/web/src/app/api/inbound_callback/route.unit.test.ts | head -150Repository: usesend/useSend
Length of output: 5135
🏁 Script executed:
rg -n 'Signature|SigningCertURL|verify.*sns' apps/web/src/server/aws/sns.ts apps/web/src/app/api/inbound_callback/route.unit.test.tsRepository: usesend/useSend
Length of output: 41
Implement AWS SNS signature verification instead of TopicArn validation alone.
The endpoint validates only TopicArn against env.INBOUND_SNS_TOPIC_ARN, which is insufficient. AWS SNS messages must be cryptographically verified using the Signature, SigningCertURL, and SignatureVersion fields provided in every SNS message. The current implementation completely omits signature verification, making this endpoint vulnerable to forged SNS payloads that can reach database writes and queue operations.
Additionally, the fetch(message.SubscribeURL!) call at line 146 lacks a timeout and could hang indefinitely, and email addresses are logged as plaintext at lines 118–119 (PII exposure).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/inbound_callback/route.ts` around lines 159 - 170,
Replace the weak TopicArn check in checkEventValidity with full AWS SNS
signature verification: validate SignatureVersion, download the SigningCertURL
over HTTPS with proper hostname/cert checks, build the canonical string from
message fields per AWS SNS docs, and verify the Signature using the downloaded
certificate (use a crypto/X509 verify flow) while still allowing a bypass in
development; also add an AbortController/timeout to the fetch of
message.SubscribeURL to avoid hangs and redact or avoid logging plain email
addresses (mask local-part or remove PII) where emails are currently logged to
prevent exposing sensitive data.
| if (response.$metadata.httpStatusCode !== 200) { | ||
| logger.error( | ||
| { response, domainName }, | ||
| "Failed to create SES Receipt Rule" | ||
| ); | ||
| throw new Error("Failed to create SES Receipt Rule"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file to see the context of both locations
cat -n apps/web/src/server/aws/ses-receipt-rules.tsRepository: usesend/useSend
Length of output: 3305
🌐 Web query:
AWS SDK v3 SES CreateReceiptRuleCommand error handling exceptions vs status codes
💡 Result:
In AWS SDK for JavaScript v3, sesClient.send(new CreateReceiptRuleCommand(...)) does not return a non-2xx “status code response” for you to check. If the HTTP response indicates failure, the SDK throws an exception instead. AWS’s v3 docs explicitly describe failure as “it throws an exception,” and show branching on the exception type/name. [1]
What to do instead of “check status codes”
Catch the exception and inspect:
- Modeled SES error type (recommended):
- Use
instanceof SomeSesException(supported since v3.53.0) [2] - Or check
error.name === "RuleSetDoesNotExist"/ etc. [1]
- Use
- HTTP status code (when you really need it):
- Read
error.$metadata.httpStatusCode(it’s in the exception’s metadata) [3]
- Read
SES CreateReceiptRule modeled errors (examples)
CreateReceiptRule can throw things like AlreadyExists, InvalidSnsTopic, InvalidS3Configuration, LimitExceeded, RuleDoesNotExist, RuleSetDoesNotExist, etc. (mostly HTTP 400). [4]
Example (TypeScript)
import {
SESClient,
CreateReceiptRuleCommand,
SESServiceException,
RuleSetDoesNotExist, // if exported by your installed SDK version
} from "@aws-sdk/client-ses";
try {
await new SESClient({}).send(new CreateReceiptRuleCommand(input));
} catch (e: any) {
const status = e?.$metadata?.httpStatusCode; // e.g., 400
if (e instanceof RuleSetDoesNotExist || e?.name === "RuleSetDoesNotExist") {
// handle missing rule set
} else if (e instanceof SESServiceException) {
// other SES service-side errors
} else {
// network/credentials/serialization/etc.
}
throw e;
}Rule of thumb: handle by exception type/name first; use e.$metadata.httpStatusCode only as supplemental info, and keep a fallback branch for unmodeled errors. [2][3]
Sources
[1] https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/the-response-object.html
[2] https://aws.amazon.com/blogs/developer/service-error-handling-modular-aws-sdk-js/
[3] https://lightrun.com/answers/aws-aws-sdk-js-v3-client-s3-getobjectcommand-with-etags-throws-error-without-name-or-details
[4] https://docs.aws.amazon.com/ses/latest/APIReference/API_CreateReceiptRule.html
Use try-catch to handle AWS SDK v3 errors; HTTP status code check cannot catch API failures.
AWS SDK v3 throws exceptions on API errors rather than returning non-2xx status codes. The response.$metadata.httpStatusCode check on lines 67–73 and 94–100 cannot catch failures because when the API call fails, an exception is thrown at client.send() before the response is even returned. The current code lacks error handling for SES API failures.
Wrap the client.send(command) calls in try-catch blocks and handle exceptions by type (e.g., instanceof SESServiceException or by error.name). Use error.$metadata.httpStatusCode only for logging supplemental information, not as the primary error detection mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/aws/ses-receipt-rules.ts` around lines 67 - 73, The SES
client calls that check response.$metadata.httpStatusCode (around the
client.send(command) calls) must be wrapped in try-catch so API errors thrown by
AWS SDK v3 are caught; modify the blocks that call client.send(command) (and
where response is inspected) to catch exceptions, detect SES errors by checking
instanceof SESServiceException or error.name, log the error and include
error.$metadata.httpStatusCode only as supplemental info, and rethrow or return
a controlled error instead of relying on a non-2xx response check; update both
places that currently inspect response.$metadata.httpStatusCode to follow this
try-catch pattern.
| if (existing) { | ||
| throw new UnsendApiError({ | ||
| code: "BAD_REQUEST", | ||
| message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Inconsistent error code for duplicate rule.
The TRPC createRule procedure returns CONFLICT (HTTP 409) for duplicate source addresses, but this REST endpoint returns BAD_REQUEST (HTTP 400). Consider using CONFLICT for consistency across both API surfaces.
🔧 Suggested fix
if (existing) {
throw new UnsendApiError({
- code: "BAD_REQUEST",
+ code: "CONFLICT",
message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`,
});
}📝 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.
| if (existing) { | |
| throw new UnsendApiError({ | |
| code: "BAD_REQUEST", | |
| message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`, | |
| }); | |
| } | |
| if (existing) { | |
| throw new UnsendApiError({ | |
| code: "CONFLICT", | |
| message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/public-api/api/domains/create-forwarding-rule.ts` around
lines 83 - 88, The REST handler in create-forwarding-rule.ts throws an
UnsendApiError with code "BAD_REQUEST" when a duplicate forwarding rule is found
(variable existing); change this to use code "CONFLICT" to match the TRPC
createRule behavior—update the throw to new UnsendApiError({ code: "CONFLICT",
message: `A forwarding rule for ${sourceAddress}@${domain.name} already exists`
}) so both API surfaces return HTTP 409 for duplicates.
| const hopCount = getHopCount(parsed); | ||
| if (hopCount > MAX_FORWARDING_HOPS) { | ||
| logger.warn( | ||
| { inboundEmailId, hopCount }, | ||
| "Forwarding loop detected, dropping email" | ||
| ); | ||
| await updateInboundEmailStatus( | ||
| inboundEmailId, | ||
| InboundEmailStatus.FAILED, | ||
| "Forwarding loop detected" | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Off-by-one in forwarding loop detection.
The condition hopCount > MAX_FORWARDING_HOPS allows 4 hops (0, 1, 2, 3) before triggering the loop detection at hop 4. If the intent is to limit forwarding to exactly 3 hops, use >=.
🐛 Proposed fix
- if (hopCount > MAX_FORWARDING_HOPS) {
+ if (hopCount >= MAX_FORWARDING_HOPS) {📝 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.
| const hopCount = getHopCount(parsed); | |
| if (hopCount > MAX_FORWARDING_HOPS) { | |
| logger.warn( | |
| { inboundEmailId, hopCount }, | |
| "Forwarding loop detected, dropping email" | |
| ); | |
| await updateInboundEmailStatus( | |
| inboundEmailId, | |
| InboundEmailStatus.FAILED, | |
| "Forwarding loop detected" | |
| ); | |
| return; | |
| } | |
| const hopCount = getHopCount(parsed); | |
| if (hopCount >= MAX_FORWARDING_HOPS) { | |
| logger.warn( | |
| { inboundEmailId, hopCount }, | |
| "Forwarding loop detected, dropping email" | |
| ); | |
| await updateInboundEmailStatus( | |
| inboundEmailId, | |
| InboundEmailStatus.FAILED, | |
| "Forwarding loop detected" | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/service/inbound-email-service.ts` around lines 64 - 76,
The forwarding-loop check currently uses `if (hopCount > MAX_FORWARDING_HOPS)`
which is off-by-one for a 3-hop limit; change the condition to `if (hopCount >=
MAX_FORWARDING_HOPS)` (in the block using `getHopCount(parsed)`, `hopCount`, and
`MAX_FORWARDING_HOPS`) so emails hitting the maximum allowed hops are treated as
a loop, keeping the existing `logger.warn` and
`updateInboundEmailStatus(inboundEmailId, InboundEmailStatus.FAILED, ...)`
behavior intact.
| const originalFrom = | ||
| parsed.from?.value?.[0]?.address ?? inboundEmail.from; | ||
| const originalFromName = | ||
| parsed.from?.value?.[0]?.name ?? originalFrom; | ||
|
|
||
| const rewrittenFrom = `"${originalFromName} via Unsend" <${forwardingRule.sourceAddress}@${domain.name}>`; |
There was a problem hiding this comment.
Sanitize originalFromName to prevent header injection.
If the original sender's display name contains double quotes or control characters, it could malform the rewritten From header. Consider escaping or removing problematic characters.
🛡️ Proposed fix
const originalFromName =
parsed.from?.value?.[0]?.name ?? originalFrom;
- const rewrittenFrom = `"${originalFromName} via Unsend" <${forwardingRule.sourceAddress}@${domain.name}>`;
+ // Escape quotes and remove control characters to prevent header injection
+ const sanitizedName = originalFromName
+ .replace(/[\x00-\x1f\x7f]/g, "")
+ .replace(/"/g, '\\"');
+ const rewrittenFrom = `"${sanitizedName} via Unsend" <${forwardingRule.sourceAddress}@${domain.name}>`;📝 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.
| const originalFrom = | |
| parsed.from?.value?.[0]?.address ?? inboundEmail.from; | |
| const originalFromName = | |
| parsed.from?.value?.[0]?.name ?? originalFrom; | |
| const rewrittenFrom = `"${originalFromName} via Unsend" <${forwardingRule.sourceAddress}@${domain.name}>`; | |
| const originalFrom = | |
| parsed.from?.value?.[0]?.address ?? inboundEmail.from; | |
| const originalFromName = | |
| parsed.from?.value?.[0]?.name ?? originalFrom; | |
| // Escape quotes and remove control characters to prevent header injection | |
| const sanitizedName = originalFromName | |
| .replace(/[\x00-\x1f\x7f]/g, "") | |
| .replace(/"/g, '\\"'); | |
| const rewrittenFrom = `"${sanitizedName} via Unsend" <${forwardingRule.sourceAddress}@${domain.name}>`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/service/inbound-email-service.ts` around lines 122 - 127,
The rewritten From header construction in inbound-email-service.ts uses
originalFromName directly which allows double quotes or control characters to
break the header; sanitize originalFromName before building rewrittenFrom by
removing or escaping double quotes and stripping control characters (CR, LF,
non-printables) and use the sanitized value in the `"${...} via Unsend" <...>`
template (e.g., add a helper like sanitizeDisplayName and call it where
originalFromName is referenced to produce sanitizedFromName, then use
sanitizedFromName when creating rewrittenFrom).
| ): Promise<ParsedMail> { | ||
| let rawContent: string; | ||
|
|
||
| const snsData = JSON.parse(snsMessage); |
There was a problem hiding this comment.
Add error handling for JSON.parse.
If snsMessage contains invalid JSON, this will throw an unhandled exception that propagates up, potentially causing confusing error messages. Wrap in try-catch for clearer error handling.
🛡️ Proposed fix
async function parseEmail(
snsMessage: string,
s3Key: string | undefined,
region: string
): Promise<ParsedMail> {
let rawContent: string;
+ let snsData: { content?: string };
+ try {
- const snsData = JSON.parse(snsMessage);
+ snsData = JSON.parse(snsMessage);
+ } catch {
+ throw new Error("Invalid SNS message format");
+ }
if (snsData.content) {📝 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.
| const snsData = JSON.parse(snsMessage); | |
| async function parseEmail( | |
| snsMessage: string, | |
| s3Key: string | undefined, | |
| region: string | |
| ): Promise<ParsedMail> { | |
| let rawContent: string; | |
| let snsData: { content?: string }; | |
| try { | |
| snsData = JSON.parse(snsMessage); | |
| } catch { | |
| throw new Error("Invalid SNS message format"); | |
| } | |
| if (snsData.content) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/service/inbound-email-service.ts` at line 191, The
assignment to const snsData = JSON.parse(snsMessage) can throw on invalid JSON;
wrap the JSON.parse(snsMessage) call in a try-catch inside the same function
(the inbound email processing function in inbound-email-service.ts) to catch
SyntaxError, log a clear error including the raw snsMessage and error.message
via your logger, and then handle the failure path (return an appropriate error
response or throw a wrapped/typed error) instead of letting the raw exception
propagate.
|
too broader for external folks to handle |
2 participants
feat: Add email forwarding for inbound emails
Closes #113
Summary
Add inbound email forwarding so users can forward incoming mail from their verified domains to any address (e.g.
[email protected]→[email protected]).What's Included
listRules,createRule,updateRule,deleteRule,enableInbound,disableInbound,listInboundEmailsGET/POST /v1/domains/:id/forwarding-rules,DELETE /v1/domains/:id/forwarding-rules/:ruleId,PUT /v1/domains/:id/inboundArchitecture
Emailmodel and send pipeline for forwarded messagesMigration Required
After pulling this branch, run:
ash
pnpm db:migrate-dev
Summary by cubic
Adds inbound email forwarding so verified domains can forward specific addresses to any destination, with a full SES/SNS/S3 pipeline, dashboard UI, and APIs. Includes safe bounce handling and background cleanup.
New Features
Migration
Written for commit 8d7251e. Summary will update on new commits.
Summary by CodeRabbit
Release Notes