feat: add domain-based access control for API keys #198
Conversation
Add functionality to limit API key usage to specific domains for enhanced security: - Add domainId field to ApiKey model with foreign key to Domain - Update API key creation to support domain selection (defaults to "All Domains") - Add domain access dropdown in frontend with verified domains list - Display domain access in API keys table - Implement domain validation for API requests - Filter domain endpoints based on API key access - Maintain backward compatibility (existing keys have access to all domains) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds optional domain scoping for API keys: DB migration adds nullable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Dashboard UI
participant API as tRPC Router
participant SVC as Api Service
participant DB as Prisma/DB
User->>UI: Submit Create API Key (name, permission, domainId?)
UI->>API: createToken({ name, permission, domainId? })
API->>SVC: addApiKey({ teamId, name, permission, domainId? })
SVC->>DB: INSERT ApiKey { teamId, name, permission, domainId? }
DB-->>SVC: Created ApiKey (incl. domain relation)
SVC-->>API: ApiKey
API-->>UI: Created key (with domain info if present)
sequenceDiagram
autonumber
actor Client
participant Pub as Public API
participant Dom as Domains Route
participant DB as Prisma/DB
Client->>Pub: GET /domains (with API key)
Pub->>DB: resolve team + apiKey (domainId?)
Pub->>Dom: getDomains(team with apiKey)
alt apiKey has domainId
Dom->>DB: findMany({ teamId, id: domainId })
else no domain restriction
Dom->>DB: findMany({ teamId })
end
DB-->>Dom: Domains
Dom-->>Client: Domain list (scoped if restricted)
sequenceDiagram
autonumber
actor Client
participant Email as Public Email Endpoint
participant Svc as Domain Service
participant DB as Prisma/DB
Client->>Email: POST /send (from, teamId, apiKeyId?)
alt apiKeyId provided
Email->>DB: getTeamAndApiKey(apiKeyId) (includes domain)
opt API key not found
Email-->>Client: 400 BAD_REQUEST (Invalid API key)
end
Email->>Svc: validateApiKeyDomainAccess(from, teamId, apiKey)
else no apiKeyId
Email->>Svc: validateDomainFromEmail(from, teamId)
end
Svc-->>Email: Resolved domain
Email-->>Client: Proceed with send using domain.id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/server/service/email-service.ts (2)
490-499: Use domain-aware validation when recording suppressed bulk emails.Even when all TOs are suppressed, we still create an Email row. Use the same domain resolution rules (respecting apiKeyId) to avoid creating records under the wrong domain.
- // Validate domain for suppressed email too - const domain = await validateDomainFromEmail(from, teamId); + // Validate domain for suppressed email with API key awareness + let domain: Awaited<ReturnType<typeof validateDomainFromEmail>>; + if (apiKeyId) { + const apiKey = await db.apiKey.findUnique({ + where: { id: apiKeyId }, + include: { domain: true }, + }); + if (!apiKey) { + throw new UnsendApiError({ code: "BAD_REQUEST", message: "Invalid API key" }); + } + domain = await validateApiKeyDomainAccess(from, teamId, apiKey); + } else { + domain = await validateDomainFromEmail(from, teamId); + }Also applies to: 522-554
1-10: Fix invalidfindUniquecall invalidateDomainFromEmailThe call in
apps/web/src/server/service/domain-service.tsat line 37 uses:const domain = await db.domain.findUnique({ where: { name: fromDomain, teamId }, });However, your Prisma schema defines
nameas globally unique (String @unique) and does not define a composite unique constraint on(teamId, name). As a result,findUnique({ where: { name, teamId } })is an invalid call and will fail at runtime.Choose one of the following resolutions:
• Quick fix with runtime check
– Change to
ts const domain = await db.domain.findUnique({ where: { name: fromDomain }, }); if (!domain || domain.teamId !== teamId) { throw new UnsendApiError("Domain not found or access denied"); }
– No schema changes required.• Schema-backed composite unique
- In
apps/web/prisma/schema.prisma, update theDomainmodel:model Domain { id Int @id @default(autoincrement()) name String @unique teamId Int + @@unique([teamId, name]) // …other fields… }- Run
prisma migrate devto add the composite index.- Use a named unique input for
findUnique, e.g.:const domain = await db.domain.findUnique({ where: { teamId_name: { teamId, name: fromDomain } }, });Alternatively, you can opt for
findFirst({ where: { name: fromDomain, teamId } })if you prefer not to modify the schema—though without a composite index this may be less efficient.Impacted locations:
apps/web/src/server/service/domain-service.ts@ line 37 (findUniqueusage invalidateDomainFromEmail)- (If choosing the schema-backed approach)
apps/web/prisma/schema.prisma– add@@unique([teamId, name])apps/web/src/server/service/domain-service.ts (1)
13-55: Critical: Prisma Domain model missing composite unique on (name, teamId)I inspected
apps/web/prisma/schema.prismaand found that theDomainmodel declaresname String @uniquebut does not include a composite unique constraint on(name, teamId). As written, the call:await db.domain.findUnique({ where: { name: fromDomain, teamId }, });will fail to compile under Prisma’s generated typings (and error at runtime), since
DomainWhereUniqueInputonly accepts unique fields (idorname), not bothnameandteamId.Please address this by either:
Adding a composite unique index to the
Domainmodel:model Domain { id Int @id @default(autoincrement()) name String teamId Int …
- @@unique([name, teamId])
}Then run `prisma migrate dev` (or equivalent) to apply the change. - **Or** changing the query to use a non-unique finder, for example: ```ts const domain = await db.domain.findFirst({ where: { name: fromDomain, teamId }, });Whichever approach you choose, ensure the Prisma schema and client typings align with the intended lookup.
🧹 Nitpick comments (16)
apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx (2)
56-58: Handle deleted or missing domains explicitly; keep wording consistent.
- With ON DELETE SET NULL, a formerly restricted key can display “All Domains” after its domain is deleted, which may mask a privilege broadening. Prefer showing an explicit state.
- Consistency: either “All domains” or “All Domains” across the UI.
Apply a safer display fallback:
- <TableCell> - {apiKey.domainId ? apiKey.domain?.name : "All Domains"} - </TableCell> + <TableCell> + {apiKey.domainId + ? apiKey.domain?.name ?? "Domain removed" + : "All domains"} + </TableCell>Also consider a small tooltip: “All domains: this key can send from any verified domain.”
60-62: Consistent relative time formatting.createdAt uses addSuffix; lastUsed doesn’t. Align for consistency.
- {apiKey.lastUsed - ? formatDistanceToNow(apiKey.lastUsed) - : "Never"} + {apiKey.lastUsed + ? formatDistanceToNow(apiKey.lastUsed, { addSuffix: true }) + : "Never"}apps/web/src/server/service/email-service.ts (1)
73-94: Type the domain variable to satisfy strict TS and prevent implicit any.With let domain; TypeScript infers any. Annotate with the Prisma Domain type (or the return type of the validator) for safety.
-import { EmailRenderer } from "@unsend/email-editor/src/renderer"; +import { EmailRenderer } from "@unsend/email-editor/src/renderer"; +import type { Domain as PrismaDomain } from "@prisma/client"; @@ - let domain; + let domain: PrismaDomain;Alternatively:
- let domain; + let domain: Awaited<ReturnType<typeof validateDomainFromEmail>>;apps/web/src/server/public-api/hono.ts (1)
25-38: Minor typing cleanup: avoid casting to any.c as any hides type errors. getTeamFromToken can accept Context and return the sanitized type so the cast isn’t needed.
Proposed adjustment (sketch):
- Change getTeamFromToken signature to accept Context.
- Here: const team = await getTeamFromToken(c);
apps/web/src/server/service/api-service.ts (1)
51-53: Avoid over-fetching: select only needed domain fieldsYou only use domain metadata downstream; fetching the entire domain object is unnecessary. Narrow the include to selected fields.
- include: { - domain: true, - }, + include: { + domain: { + select: { id: true, name: true }, + }, + },apps/web/src/server/api/routers/api.ts (1)
13-17: Harden input schema: use native Prisma enum and stronger number constraints
- Replace string literal enum with
z.nativeEnum(ApiPermission)to avoid drift if enum values change in Prisma.- Constrain
domainIdas an integer and positive.-import { z } from "zod"; +import { z } from "zod"; +import { ApiPermission } from "@prisma/client"; @@ - z.object({ - name: z.string(), - permission: z.enum(["FULL", "SENDING"]), - domainId: z.number().optional() - }) + z.object({ + name: z.string(), + permission: z.nativeEnum(ApiPermission), + domainId: z.number().int().positive().optional(), + })apps/web/prisma/migrations/20250822125136_add_domain_access_to_api_keys/migration.sql (1)
1-5: Consider indexing ApiKey.domainIdQueries that filter or join by
domainId(e.g., audits, admin views) will benefit from an index. Postgres doesn’t auto-index FKs.ALTER TABLE "ApiKey" ADD COLUMN "domainId" INTEGER; @@ ALTER TABLE "ApiKey" ADD CONSTRAINT "ApiKey_domainId_fkey" FOREIGN KEY ("domainId") REFERENCES "Domain"("id") ON DELETE SET NULL ON UPDATE CASCADE; +-- Optional: add index for faster filters/joins on domainId +CREATE INDEX IF NOT EXISTS "ApiKey_domainId_idx" ON "ApiKey" ("domainId");apps/web/src/server/service/domain-service.ts (1)
9-9: Minor: Narrow the parameter type or use the embedded domainYou don’t use
apiKey.domainhere. Either drop it from the type to justApiKey, or incorporate it in the error message (e.g., to show the key’s allowed domain on mismatch). Keeping the surface minimal reduces confusion.apps/web/src/server/public-api/api/domains/get-domains.ts (2)
26-38: Simplify branching and avoid implicit any ondomains
let domains;infersanyunder strict settings. You can keep strong typing and readability by computing a singleconstwith a conditional.- // If API key is restricted to a specific domain, only return that domain - let domains; - if (team.apiKey.domainId) { - domains = await db.domain.findMany({ - where: { - teamId: team.id, - id: team.apiKey.domainId - } - }); - } else { - // If API key has access to all domains, return all team domains - domains = await db.domain.findMany({ where: { teamId: team.id } }); - } + // If API key is restricted to a specific domain, only return that domain; else return all team domains + const domains = team.apiKey.domainId + ? await db.domain.findMany({ + where: { teamId: team.id, id: team.apiKey.domainId }, + }) + : await db.domain.findMany({ where: { teamId: team.id } });
7-20: Minor: OpenAPI description mismatchThe 200 response description says “Retrieve the user”. This route returns domains. Update the description to avoid confusing SDK generators or consumers.
apps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx (6)
30-36: Reorder imports to follow repo import grouping/alphabetization guidelines.Per guidelines, group external libs (react, zod, react-hook-form, lucide-react), then internal packages (e.g., @unsend/ui, ~/trpc/*), and alphabetize within groups. This improves readability and reduces churn in future diffs.
42-42: Tighten schema: restrict domainId to 'all' or a numeric ID string.Strengthens validation against unexpected values while keeping the form’s string-based Select model intact.
Apply this diff:
- domainId: z.string().optional(), + // "all" means unrestricted; otherwise a numeric id (as string) + domainId: z + .union([z.literal("all"), z.string().regex(/^\d+$/, "Invalid domain id")]) + .optional(),
52-53: Handle domains query loading/error and confirm verified-domain filtering.
- UX: When the domains query is loading or errors, the Select should reflect that state (disabled + inline item).
- Data: PR objective mentions “specific verified domain.” Verify this query only surfaces verified domains (or filter client-side if the API doesn’t yet). Also consider empty-state messaging when there are no domains.
Would you confirm whether
api.domain.domains.useQuery()returns only verified domains? If not, should we filter to verified in the UI until the API adds a filter?
69-69: Safer conversion and guard for undefined values.Prevents accidental
NaNifdomainIdis ever undefined and makes the intent explicit.Apply this diff:
- domainId: values.domainId === "all" ? undefined : Number(values.domainId), + domainId: + values.domainId && values.domainId !== "all" + ? Number.parseInt(values.domainId, 10) + : undefined,
64-79: Add error handling for API key creation to surface failures to the user.Currently only
onSuccessis handled; addonErrorto show a toast and keep the dialog open. This improves UX and aligns with the error-handling guideline.You can update the mutation call like this (illustrative snippet outside the changed lines):
createApiKeyMutation.mutate( { name: values.name, permission: "FULL", domainId: values.domainId && values.domainId !== "all" ? Number.parseInt(values.domainId, 10) : undefined, }, { onSuccess: (data) => { utils.apiKey.invalidate(); setApiKey(data); apiKeyForm.reset(); }, onError: (err) => { toast.error(err?.message ?? "Failed to create API key"); }, } );
81-87: Handle clipboard write errors to avoid silent failures.
navigator.clipboard.writeTextcan reject (permissions, insecure context). Catch and notify the user.You can wrap it as:
async function handleCopy() { try { await navigator.clipboard.writeText(apiKey); setIsCopied(true); setTimeout(() => setIsCopied(false), 2000); } catch { toast.error("Could not copy to clipboard. Please copy manually."); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/web/prisma/migrations/20250822125136_add_domain_access_to_api_keys/migration.sql(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx(4 hunks)apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx(4 hunks)apps/web/src/server/api/routers/api.ts(2 hunks)apps/web/src/server/public-api/api/domains/get-domains.ts(1 hunks)apps/web/src/server/public-api/auth.ts(1 hunks)apps/web/src/server/public-api/hono.ts(1 hunks)apps/web/src/server/service/api-service.ts(3 hunks)apps/web/src/server/service/domain-service.ts(2 hunks)apps/web/src/server/service/email-service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
{apps,packages}/**/*.{js,jsx,ts,tsx,css,scss,md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with the Tailwind plugin for code formatting
Files:
apps/web/src/server/service/email-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/server/public-api/api/domains/get-domains.tsapps/web/src/server/public-api/hono.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/auth.tsapps/web/src/server/service/api-service.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
{apps,packages}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{js,jsx,ts,tsx}: Group imports by source (internal/external) and alphabetize them
Use camelCase for variables and functions, PascalCase for components and classes
Use try/catch with specific error types for error handling
Files:
apps/web/src/server/service/email-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/server/public-api/api/domains/get-domains.tsapps/web/src/server/public-api/hono.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/auth.tsapps/web/src/server/service/api-service.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Use strong typing in TypeScript, avoidany, and use Zod for validation
Follow Vercel style guides with strict TypeScript
Files:
apps/web/src/server/service/email-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/server/public-api/api/domains/get-domains.tsapps/web/src/server/public-api/hono.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/auth.tsapps/web/src/server/service/api-service.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use tRPC for internal API endpoints
Files:
apps/web/src/server/service/email-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/server/public-api/api/domains/get-domains.tsapps/web/src/server/public-api/hono.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/auth.tsapps/web/src/server/service/api-service.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/service/email-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/server/public-api/api/domains/get-domains.tsapps/web/src/server/public-api/hono.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/auth.tsapps/web/src/server/service/api-service.tsapps/web/src/server/service/domain-service.tsapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
{apps,packages}/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{jsx,tsx}: Use functional React components with hooks and group related hooks together
In React components, structure code with props at the top, hooks next, helper functions, then JSX
Files:
apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsxapps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-09T10:37:58.146Z
Learnt from: CR
PR: unsend-dev/unsend#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-09T10:37:58.146Z
Learning: Applies to apps/smtp-server/**/*.{ts,tsx} : Use Hono for public API endpoints
Applied to files:
apps/web/src/server/public-api/hono.ts
🧬 Code graph analysis (6)
apps/web/src/server/service/email-service.ts (2)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/service/domain-service.ts (2)
validateApiKeyDomainAccess(58-80)validateDomainFromEmail(13-56)
apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx (1)
packages/ui/src/table.tsx (2)
TableHead(113-113)TableCell(115-115)
apps/web/src/server/public-api/api/domains/get-domains.ts (1)
apps/web/src/server/db.ts (1)
db(20-20)
apps/web/src/server/api/routers/api.ts (1)
apps/web/src/server/service/api-service.ts (1)
addApiKey(8-42)
apps/web/src/server/service/domain-service.ts (1)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)
apps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx (2)
packages/ui/src/form.tsx (5)
FormField(176-176)FormItem(171-171)FormLabel(172-172)FormControl(173-173)FormDescription(174-174)packages/ui/src/select.tsx (5)
Select(150-150)SelectTrigger(153-153)SelectValue(152-152)SelectContent(154-154)SelectItem(156-156)
🔇 Additional comments (5)
apps/web/prisma/schema.prisma (1)
193-194: Relation back-reference looks correct.Domain.apiKeys establishes the expected 1:N link to ApiKey and aligns with the PR’s intent.
apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx (2)
28-28: Good UX addition.The “Domain Access” column provides clear visibility into scoping.
37-37: Colspan updates are correct.colSpan=7 matches the new column count in the header.
Also applies to: 46-46
apps/web/src/server/api/routers/api.ts (1)
40-45: LGTM: exposing domain scope in list payloadSelecting
domainIdanddomain.nameenables the UI to render “All Domains” vs the specific domain cleanly. This aligns with the PR objective and keeps sensitive fields out.apps/web/src/app/(dashboard)/dev-settings/api-keys/add-api-key.tsx (1)
60-60: Good default: preserves backward compatibility.Defaulting to "all" aligns with the objective that existing keys remain unrestricted by domain.
apps/web/prisma/schema.prisma
Outdated
| domainId Int? | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| lastUsed DateTime? | ||
| teamId Int | ||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prevent cross-team association and clarify deletion semantics for security.
- Today, ApiKey.domainId references Domain(id) without asserting the same team across both sides. An ApiKey could (accidentally) reference a Domain from another team, and validateApiKeyDomainAccess only compares IDs, not team membership. Enforce the invariant teamId(apiKey) === teamId(domain) at DB or service layer.
- ON DELETE SET NULL silently broadens a restricted key to “All Domains” if its domain is deleted. That’s a surprising privilege escalation.
Actionable suggestions:
- If you can accept a composite relation, model the relation on [domainId, teamId] → [id, teamId] and check team membership at the DB level. Otherwise, add a strict service-level check in key creation/update to ensure selected domain belongs to the same team.
- Consider ON DELETE RESTRICT (or disabling the key) instead of SET NULL to avoid unexpectedly broadening access. If SET NULL is desired, surface this clearly in the UI and audit logs.
Apply an index for query performance and prepare for service-side checks:
model ApiKey {
id Int @id @default(autoincrement())
clientId String @unique
tokenHash String
partialToken String
name String
permission ApiPermission @default(SENDING)
domainId Int?
+ @@index([domainId])
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
lastUsed DateTime?
teamId Int
team Team @relation(fields: [teamId], references: [id], onDelete: Cascade)
- domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull)
+ domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull)
}If you prefer DB-level invariants and can tolerate a composite optional relation, we can draft the exact Prisma schema changes for [domainId, teamId] → [id, teamId].
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Finding ApiKey model in Prisma schema ==="
rg -n -C3 "model ApiKey" apps/web/prisma/schema.prisma || true
echo
echo "=== Displaying lines 180–260 around ApiKey definition ==="
sed -n '180,260p' apps/web/prisma/schema.prisma || true
echo
echo "=== Searching for validateApiKeyDomainAccess usage ==="
rg -n -C3 "validateApiKeyDomainAccess" -g "*.ts" || true
echo
echo "=== Searching for any domain.teamId equality checks ==="
rg -n -C3 "domain.*teamId" -g "*.ts" || trueLength of output: 8491
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Domain model definition ==="
rg -n -C5 "model Domain" apps/web/prisma/schema.prisma || true
echo
echo "=== validateDomainFromEmail implementation ==="
sed -n '1,200p' apps/web/src/server/service/domain-service.ts || true
echo
echo "=== Listing public API endpoints ==="
find apps/web/src/server/public-api/api -type f -maxdepth 2 || true
echo
echo "=== Searching for ApiKey in public API endpoints ==="
rg -n -C5 "ApiKey" -g "apps/web/src/server/public-api/api/**" || true
echo
echo "=== Searching for domainId in ApiKey endpoints ==="
rg -n -C5 "domainId" -g "apps/web/src/server/public-api/api/**" || trueLength of output: 14073
Mandatory: Enforce domain-team invariant and tighten deletion semantics for ApiKey
The current schema and service code still allow three gaps:
- DB allows an ApiKey’s domainId to be set to any Domain.id (even from another team).
- ON DELETE SET NULL silently lifts a formerly restricted key to unrestricted if its domain is deleted.
- No index on
domainIdmeans domain-scoped lookups won’t use an index.
To address these:
• schema.prisma (apps/web/prisma/schema.prisma):
– Add an index on domainId for lookup performance.
– Change the referential action on domain from SetNull to Restrict (or NoAction) to avoid privilege escalation.
– (Optional, if you can tolerate the complexity) introduce a composite relation on [domainId, teamId] → [id, teamId] to enforce at the DB level that every ApiKey.domain must belong to the same team.
model ApiKey {
id Int @id @default(autoincrement())
clientId String @unique
tokenHash String
partialToken String
name String
permission ApiPermission @default(SENDING)
domainId Int?
+ @@index([domainId])
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
lastUsed DateTime?
teamId Int
team Team @relation(fields: [teamId], references: [id], onDelete: Cascade)
- domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull)
+ domain Domain? @relation(fields: [domainId], references: [id], onDelete: Restrict)
}• Service layer (in your ApiKey-creation/update endpoints):
Before persisting a new or updated ApiKey, explicitly verify domainId, if provided, belongs to the same teamId. For example, reuse validateDomainFromEmail (or add a similar check) in createApiKey/updateApiKey to throw if the selected domain’s teamId mismatches the ApiKey’s teamId.
• Runtime enforcement (already in place):
validateApiKeyDomainAccess calls validateDomainFromEmail(email, teamId) to ensure team membership, then checks apiKey.domainId === domain.id. This is good, but it only runs when sending mail—creation and other operations still need safeguards.
Please update the schema and service code accordingly to close these security gaps.
📝 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.
| domainId Int? | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| lastUsed DateTime? | |
| teamId Int | |
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | |
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull) | |
| } | |
| model ApiKey { | |
| id Int @id @default(autoincrement()) | |
| clientId String @unique | |
| tokenHash String | |
| partialToken String | |
| name String | |
| permission ApiPermission @default(SENDING) | |
| domainId Int? | |
| @@index([domainId]) | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| lastUsed DateTime? | |
| teamId Int | |
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | |
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: Restrict) | |
| } |
🤖 Prompt for AI Agents
In apps/web/prisma/schema.prisma around lines 208 to 215, the ApiKey model
currently allows domainId from any Domain, uses onDelete: SetNull (which can
escalate privileges), and lacks an index on domainId; update the schema to add
an index on domainId, change the domain relation's onDelete from SetNull to
Restrict (or NoAction) to prevent silent privilege escalation, and optionally
add a composite relation/constraint tying [domainId, teamId] → [id, teamId] to
enforce at the DB level that the domain belongs to the same team; additionally,
update the ApiKey create/update service endpoints to explicitly validate that
the provided domainId (when present) belongs to the same teamId before
persisting (reuse validateDomainFromEmail or add an equivalent check) so
mismatched domain/team combinations are rejected.
| <FormField | ||
| control={apiKeyForm.control} | ||
| name="domainId" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Domain access</FormLabel> | ||
| <Select onValueChange={field.onChange} defaultValue={field.value}> | ||
| <FormControl> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select domain access" /> | ||
| </SelectTrigger> | ||
| </FormControl> | ||
| <SelectContent> | ||
| <SelectItem value="all">All Domains</SelectItem> | ||
| {domainsQuery.data?.map((domain: { id: number; name: string }) => ( | ||
| <SelectItem key={domain.id} value={domain.id.toString()}> | ||
| {domain.name} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| <FormDescription> | ||
| Choose which domain this API key can send emails from. | ||
| </FormDescription> | ||
| </FormItem> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Select controlled (not defaultValue) to ensure form resets work reliably.
Using defaultValue makes the Select uncontrolled; after apiKeyForm.reset() the UI can show a stale value, confusing users. Control it via value, disable during loading/error, and add basic loading/empty states.
Apply this diff:
- <Select onValueChange={field.onChange} defaultValue={field.value}>
+ <Select
+ onValueChange={field.onChange}
+ value={field.value ?? "all"}
+ disabled={domainsQuery.isLoading || domainsQuery.isError}
+ >
<FormControl>
<SelectTrigger>
<SelectValue placeholder="Select domain access" />
</SelectTrigger>
</FormControl>
- <SelectContent>
- <SelectItem value="all">All Domains</SelectItem>
- {domainsQuery.data?.map((domain: { id: number; name: string }) => (
- <SelectItem key={domain.id} value={domain.id.toString()}>
- {domain.name}
- </SelectItem>
- ))}
- </SelectContent>
+ <SelectContent>
+ <SelectItem value="all">All Domains</SelectItem>
+ {domainsQuery.isLoading && (
+ <SelectItem value="loading" disabled>
+ Loading domains…
+ </SelectItem>
+ )}
+ {domainsQuery.isError && (
+ <SelectItem value="error" disabled>
+ Failed to load domains
+ </SelectItem>
+ )}
+ {!domainsQuery.isLoading &&
+ !domainsQuery.isError &&
+ (domainsQuery.data?.length ?? 0) === 0 && (
+ <SelectItem value="none" disabled>
+ No domains available
+ </SelectItem>
+ )}
+ {domainsQuery.data?.map((domain: { id: number; name: string }) => (
+ <SelectItem key={domain.id} value={domain.id.toString()}>
+ {domain.name}
+ </SelectItem>
+ ))}
+ </SelectContent>
</Select>
- <FormDescription>
- Choose which domain this API key can send emails from.
- </FormDescription>
+ {apiKeyForm.formState.errors.domainId ? (
+ <FormMessage />
+ ) : (
+ <FormDescription>
+ Choose which domain this API key can send emails from.
+ </FormDescription>
+ )}📝 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.
| <FormField | |
| control={apiKeyForm.control} | |
| name="domainId" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Domain access</FormLabel> | |
| <Select onValueChange={field.onChange} defaultValue={field.value}> | |
| <FormControl> | |
| <SelectTrigger> | |
| <SelectValue placeholder="Select domain access" /> | |
| </SelectTrigger> | |
| </FormControl> | |
| <SelectContent> | |
| <SelectItem value="all">All Domains</SelectItem> | |
| {domainsQuery.data?.map((domain: { id: number; name: string }) => ( | |
| <SelectItem key={domain.id} value={domain.id.toString()}> | |
| {domain.name} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| <FormDescription> | |
| Choose which domain this API key can send emails from. | |
| </FormDescription> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={apiKeyForm.control} | |
| name="domainId" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormLabel>Domain access</FormLabel> | |
| <Select | |
| onValueChange={field.onChange} | |
| value={field.value ?? "all"} | |
| disabled={domainsQuery.isLoading || domainsQuery.isError} | |
| > | |
| <FormControl> | |
| <SelectTrigger> | |
| <SelectValue placeholder="Select domain access" /> | |
| </SelectTrigger> | |
| </FormControl> | |
| <SelectContent> | |
| <SelectItem value="all">All Domains</SelectItem> | |
| {domainsQuery.isLoading && ( | |
| <SelectItem value="loading" disabled> | |
| Loading domains… | |
| </SelectItem> | |
| )} | |
| {domainsQuery.isError && ( | |
| <SelectItem value="error" disabled> | |
| Failed to load domains | |
| </SelectItem> | |
| )} | |
| {!domainsQuery.isLoading && | |
| !domainsQuery.isError && | |
| (domainsQuery.data?.length ?? 0) === 0 && ( | |
| <SelectItem value="none" disabled> | |
| No domains available | |
| </SelectItem> | |
| )} | |
| {domainsQuery.data?.map((domain: { id: number; name: string }) => ( | |
| <SelectItem key={domain.id} value={domain.id.toString()}> | |
| {domain.name} | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| {apiKeyForm.formState.errors.domainId ? ( | |
| <FormMessage /> | |
| ) : ( | |
| <FormDescription> | |
| Choose which domain this API key can send emails from. | |
| </FormDescription> | |
| )} | |
| </FormItem> | |
| )} | |
| /> |
| return addApiKey({ | ||
| name: input.name, | ||
| permission: input.permission, | ||
| teamId: ctx.team.id, | ||
| domainId: input.domainId, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate domain ownership (and optionally verification) at the API boundary
Before calling addApiKey, ensure the provided domainId exists and belongs to ctx.team.id. Without this, a client can associate a key with another team’s domain; the FK won’t stop that. Also return a clear 400/403 instead of a generic 500 from a DB FK violation.
+import { TRPCError } from "@trpc/server";
@@
- .mutation(async ({ ctx, input }) => {
- return addApiKey({
+ .mutation(async ({ ctx, input }) => {
+ // Validate domain ownership (and optionally verification)
+ if (input.domainId !== undefined) {
+ const domain = await ctx.db.domain.findUnique({
+ where: { id: input.domainId },
+ select: { id: true, teamId: true, status: true },
+ });
+ if (!domain || domain.teamId !== ctx.team.id) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Invalid domainId for this team",
+ });
+ }
+ // Optional: enforce only verified domains can be scoped
+ // if (domain.status !== "SUCCESS") {
+ // throw new TRPCError({ code: "BAD_REQUEST", message: "Domain is not verified" });
+ // }
+ }
+ return addApiKey({
name: input.name,
permission: input.permission,
teamId: ctx.team.id,
domainId: input.domainId,
});
}),If you’d like, I can add unit/integration tests that cover:
- Creating a key with another team’s domainId → 400
- Creating a key with non-existent domainId → 400
- Creating a key with verified domainId → 200
- (Optional) Unverified domainId → 400
📝 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.
| return addApiKey({ | |
| name: input.name, | |
| permission: input.permission, | |
| teamId: ctx.team.id, | |
| domainId: input.domainId, | |
| }); | |
| }), | |
| // At the top of the file, alongside your existing imports: | |
| import { TRPCError } from "@trpc/server"; | |
| ... | |
| // In your router definition: | |
| .mutation("createApiKey", async ({ ctx, input }) => { | |
| // Validate domain ownership (and optionally verification) | |
| if (input.domainId !== undefined) { | |
| const domain = await ctx.db.domain.findUnique({ | |
| where: { id: input.domainId }, | |
| select: { id: true, teamId: true, status: true }, | |
| }); | |
| if (!domain || domain.teamId !== ctx.team.id) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid domainId for this team", | |
| }); | |
| } | |
| // Optional: enforce only verified domains can be scoped | |
| // if (domain.status !== "SUCCESS") { | |
| // throw new TRPCError({ code: "BAD_REQUEST", message: "Domain is not verified" }); | |
| // } | |
| } | |
| return addApiKey({ | |
| name: input.name, | |
| permission: input.permission, | |
| teamId: ctx.team.id, | |
| domainId: input.domainId, | |
| }); | |
| }), |
| @@ -59,5 +59,5 @@ export const getTeamFromToken = async (c: Context) => { | |||
| logger.error({ err }, "Failed to update lastUsed on API key") | |||
| ); | |||
|
|
|||
| return { ...team, apiKeyId: apiKey.id }; | |||
| return { ...team, apiKeyId: apiKey.id, apiKey }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing sensitive ApiKey fields in request context.
c.set("team", ...) now carries the full ApiKey, which includes tokenHash. If any handler logs c.var.team or accidentally serializes it, secrets may leak.
Recommend returning a safe, minimal shape (id, name, permission, domainId, domain?.name) and update hono.ts typing accordingly.
Apply this localized sanitization here:
- return { ...team, apiKeyId: apiKey.id, apiKey };
+ const safeApiKey = {
+ id: apiKey.id,
+ name: apiKey.name,
+ permission: apiKey.permission,
+ domainId: apiKey.domainId,
+ domain: apiKey.domain ? { name: apiKey.domain.name } : null,
+ } as const;
+ return { ...team, apiKeyId: apiKey.id, apiKey: safeApiKey };Optional: also validate the scheme prefix to harden auth parsing.
- const token = authHeader.split(" ")[1];
+ const [scheme, token] = authHeader.split(" ");
+ if (scheme?.toLowerCase() !== "bearer" || !token) {
+ throw new UnsendApiError({ code: "UNAUTHORIZED", message: "Invalid Authorization header" });
+ }🤖 Prompt for AI Agents
In apps/web/src/server/public-api/auth.ts around line 62, the code currently
sets c.set("team", ...) with the full ApiKey (including tokenHash); replace this
with a sanitized team object containing only { id, name, permission, domainId,
domainName } (or domain?.name as domainName) so no secret fields are attached to
the request context, and update the hono.ts request/context typing to reflect
the new minimal shape; additionally, optionally validate the authorization
scheme prefix (e.g., "Bearer ") before parsing the token to harden auth parsing.
There was a problem hiding this comment.
@enemyrr can you take a look at this, seems like valida comment, should only have need fields in context
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -7,13 +7,13 @@ import { getRedis } from "~/server/redis"; | |||
| import { getTeamFromToken } from "~/server/public-api/auth"; | |||
| import { isSelfHosted } from "~/utils/common"; | |||
| import { UnsendApiError } from "./api-error"; | |||
| import { Team } from "@prisma/client"; | |||
| import { Team, ApiKey } from "@prisma/client"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t expose full ApiKey type to route handlers; define a safe shape.
Using ApiKey in AppEnv makes tokenHash and other internals available to every handler. Adopt a sanitized type consistent with auth.ts.
-import { Team, ApiKey } from "@prisma/client";
+import { Team } from "@prisma/client";
+type PublicApiKey = {
+ id: number;
+ name: string;
+ permission: import("@prisma/client").ApiPermission;
+ domainId: number | null;
+ domain: { name: string } | null;
+};
@@
- team: Team & { apiKeyId: number; apiKey: ApiKey & { domain?: { name: string } | null } };
+ team: Team & { apiKeyId: number; apiKey: PublicApiKey };Follow-up: this pairs with the sanitization in apps/web/src/server/public-api/auth.ts.
Also applies to: 16-16
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/hono.ts around lines 10 and 16, the AppEnv
currently imports and exposes the full ApiKey Prisma type (which includes
tokenHash and other sensitive internals) to route handlers; replace that with a
sanitized ApiKey shape (only the public fields used by handlers) that matches
the sanitization defined in apps/web/src/server/public-api/auth.ts, update the
import/definition to use that safe type instead of ApiKey, and ensure AppEnv
references the sanitized type so tokenHash and other internals are not
accessible to handlers.
| export async function validateApiKeyDomainAccess( | ||
| email: string, | ||
| teamId: number, | ||
| apiKey: ApiKey & { domain?: { name: string } | null } | ||
| ) { | ||
| // First validate the domain exists and is verified | ||
| const domain = await validateDomainFromEmail(email, teamId); | ||
|
|
||
| // If API key has no domain restriction (domainId is null), allow all domains | ||
| if (!apiKey.domainId) { | ||
| return domain; | ||
| } | ||
|
|
||
| // If API key is restricted to a specific domain, check if it matches | ||
| if (apiKey.domainId !== domain.id) { | ||
| throw new UnsendApiError({ | ||
| code: "FORBIDDEN", | ||
| message: `API key does not have access to domain: ${domain.name}`, | ||
| }); | ||
| } | ||
|
|
||
| return domain; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add team ownership assertion for the provided API key
validateApiKeyDomainAccess trusts the caller to pass an API key that belongs to teamId. Add a guard to prevent accidental cross-team use if this function ever gets reused elsewhere.
export async function validateApiKeyDomainAccess(
email: string,
teamId: number,
apiKey: ApiKey & { domain?: { name: string } | null }
) {
// First validate the domain exists and is verified
const domain = await validateDomainFromEmail(email, teamId);
+ // Ensure the API key belongs to the same team (defense-in-depth)
+ if (apiKey.teamId !== teamId) {
+ throw new UnsendApiError({
+ code: "FORBIDDEN",
+ message: "API key does not belong to this team",
+ });
+ }
+
// If API key has no domain restriction (domainId is null), allow all domains
if (!apiKey.domainId) {
return domain;
}
// If API key is restricted to a specific domain, check if it matches
if (apiKey.domainId !== domain.id) {
throw new UnsendApiError({
code: "FORBIDDEN",
message: `API key does not have access to domain: ${domain.name}`,
});
}
return domain;
}📝 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.
| export async function validateApiKeyDomainAccess( | |
| email: string, | |
| teamId: number, | |
| apiKey: ApiKey & { domain?: { name: string } | null } | |
| ) { | |
| // First validate the domain exists and is verified | |
| const domain = await validateDomainFromEmail(email, teamId); | |
| // If API key has no domain restriction (domainId is null), allow all domains | |
| if (!apiKey.domainId) { | |
| return domain; | |
| } | |
| // If API key is restricted to a specific domain, check if it matches | |
| if (apiKey.domainId !== domain.id) { | |
| throw new UnsendApiError({ | |
| code: "FORBIDDEN", | |
| message: `API key does not have access to domain: ${domain.name}`, | |
| }); | |
| } | |
| return domain; | |
| } | |
| export async function validateApiKeyDomainAccess( | |
| email: string, | |
| teamId: number, | |
| apiKey: ApiKey & { domain?: { name: string } | null } | |
| ) { | |
| // First validate the domain exists and is verified | |
| const domain = await validateDomainFromEmail(email, teamId); | |
| // Ensure the API key belongs to the same team (defense-in-depth) | |
| if (apiKey.teamId !== teamId) { | |
| throw new UnsendApiError({ | |
| code: "FORBIDDEN", | |
| message: "API key does not belong to this team", | |
| }); | |
| } | |
| // If API key has no domain restriction (domainId is null), allow all domains | |
| if (!apiKey.domainId) { | |
| return domain; | |
| } | |
| // If API key is restricted to a specific domain, check if it matches | |
| if (apiKey.domainId !== domain.id) { | |
| throw new UnsendApiError({ | |
| code: "FORBIDDEN", | |
| message: `API key does not have access to domain: ${domain.name}`, | |
| }); | |
| } | |
| return domain; | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/server/service/domain-service.ts around lines 58 to 80, the
function validateApiKeyDomainAccess does not assert that the passed apiKey
belongs to the provided teamId, allowing potential cross-team use; add an early
guard after fetching/validating the domain (or immediately at function start)
that checks apiKey.teamId === teamId and if not, throw an UnsendApiError with a
FORBIDDEN code and a clear message about cross-team API key usage; keep the
existing domainId/null logic unchanged and place this ownership assertion before
any domain-specific access checks.
There was a problem hiding this comment.
nah, it's already checked in validateDomainFromEmail
There was a problem hiding this comment.
@KMKoushik, understood! You're right that the domain ownership is validated in validateDomainFromEmail. I acknowledge your decision to keep the current implementation as is.
- Fix critical Prisma query issue: use findFirst instead of findUnique for Domain lookup - Remove unused imports and improve type safety - Update OpenAPI description for domains endpoint - Improve error handling for deleted domains in UI - Optimize API service to select only needed domain fields - Add consistent time formatting and better validation schemas
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/src/server/service/domain-service.ts (1)
58-80: Add team-ownership assertion to prevent cross-team API key usage.This function trusts that
apiKeybelongs toteamId. Add an early guard to block cross-team use before any domain checks. This mirrors feedback from an earlier review.export async function validateApiKeyDomainAccess( email: string, teamId: number, apiKey: ApiKey & { domain?: { name: string } | null } ) { - // First validate the domain exists and is verified + // Ensure the API key belongs to the same team (defense-in-depth) + if (apiKey.teamId !== teamId) { + throw new UnsendApiError({ + code: "FORBIDDEN", + message: "API key does not belong to this team", + }); + } + + // First validate the domain exists and is verified const domain = await validateDomainFromEmail(email, teamId);Follow-up tests to add (happy to draft):
- apiKey.teamId ≠ teamId → throws FORBIDDEN
- apiKey.domainId set and not equal to resolved domain → throws FORBIDDEN
- apiKey.domainId unset → any verified team domain allowed
apps/web/src/server/api/routers/api.ts (1)
20-27: Add server-side validation fordomainId(ownership and optionally verification).Validate that
input.domainIdbelongs toctx.team.idbefore callingaddApiKey. This prevents cross-team association and yields a clean 400/403 instead of a generic failure. This was raised previously as well.+import { TRPCError } from "@trpc/server"; @@ createToken: teamProcedure .input( z.object({ name: z.string(), permission: z.nativeEnum(ApiPermission), domainId: z.number().int().positive().optional(), }) ) - .mutation(async ({ ctx, input }) => { - return addApiKey({ + .mutation(async ({ ctx, input }) => { + // Validate domain ownership (and optional verification) at the boundary + if (input.domainId !== undefined) { + const domain = await ctx.db.domain.findUnique({ + where: { id: input.domainId }, + select: { id: true, teamId: true, status: true }, + }); + if (!domain || domain.teamId !== ctx.team.id) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Invalid domainId for this team", + }); + } + // Optional: only allow verified domains to be scoped + // if (domain.status !== "SUCCESS") { + // throw new TRPCError({ code: "BAD_REQUEST", message: "Domain is not verified" }); + // } + } + return addApiKey({ name: input.name, permission: input.permission, teamId: ctx.team.id, domainId: input.domainId, }); }),If you want stricter semantics, flip the optional check to require verified domains when
domainIdis provided.
🧹 Nitpick comments (2)
apps/web/src/server/public-api/api/domains/get-domains.ts (1)
25-30: Unify query (minor) and consider exposing only verified domains.
- Minor: build a single
whereand callfindManyonce; keeps things simpler.- Optional: if this endpoint is used by API clients to select “sendable” domains, consider filtering to verified domains only (
status: "SUCCESS").- // If API key is restricted to a specific domain, only return that domain; else return all team domains - const domains = team.apiKey.domainId - ? await db.domain.findMany({ - where: { teamId: team.id, id: team.apiKey.domainId }, - }) - : await db.domain.findMany({ where: { teamId: team.id } }); + // If API key is restricted to a specific domain, only return that domain; else return all team domains + const where = { + teamId: team.id, + ...(team.apiKey.domainId ? { id: team.apiKey.domainId } : {}), + // Optional: uncomment to return only verified domains + // status: "SUCCESS", + }; + const domains = await db.domain.findMany({ where });apps/web/src/server/service/domain-service.ts (1)
37-39: Case-insensitive match and normalization for domain lookup.Email domains are case-insensitive. Use a case-insensitive comparison (and normalize input) to avoid false negatives. If you routinely query by
(teamId, name), consider a composite unique index for integrity and performance.- const domain = await db.domain.findFirst({ - where: { name: fromDomain, teamId }, - }); + const domain = await db.domain.findFirst({ + where: { + teamId, + name: { equals: fromDomain.toLowerCase(), mode: "insensitive" }, + }, + });Architecture note:
- Add a Prisma unique constraint (migration) to enforce uniqueness per team:
@@unique([teamId, name], name: "domains_teamId_name_key")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx(4 hunks)apps/web/src/server/api/routers/api.ts(3 hunks)apps/web/src/server/public-api/api/domains/get-domains.ts(2 hunks)apps/web/src/server/service/api-service.ts(3 hunks)apps/web/src/server/service/domain-service.ts(3 hunks)apps/web/src/server/service/email-service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/app/(dashboard)/dev-settings/api-keys/api-list.tsx
- apps/web/src/server/service/email-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
{apps,packages}/**/*.{js,jsx,ts,tsx,css,scss,md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with the Tailwind plugin for code formatting
Files:
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/api-service.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/api/domains/get-domains.ts
{apps,packages}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{js,jsx,ts,tsx}: Group imports by source (internal/external) and alphabetize them
Use camelCase for variables and functions, PascalCase for components and classes
Use try/catch with specific error types for error handling
Files:
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/api-service.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/api/domains/get-domains.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Use strong typing in TypeScript, avoidany, and use Zod for validation
Follow Vercel style guides with strict TypeScript
Files:
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/api-service.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/api/domains/get-domains.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use tRPC for internal API endpoints
Files:
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/api-service.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/api/domains/get-domains.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/service/domain-service.tsapps/web/src/server/service/api-service.tsapps/web/src/server/api/routers/api.tsapps/web/src/server/public-api/api/domains/get-domains.ts
🧬 Code graph analysis (3)
apps/web/src/server/service/domain-service.ts (1)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)
apps/web/src/server/api/routers/api.ts (1)
apps/web/src/server/service/api-service.ts (1)
addApiKey(8-42)
apps/web/src/server/public-api/api/domains/get-domains.ts (1)
apps/web/src/server/db.ts (1)
db(20-20)
🔇 Additional comments (5)
apps/web/src/server/public-api/api/domains/get-domains.ts (1)
16-16: Good update to the route description.Accurately reflects the new domain-scoped behavior.
apps/web/src/server/service/domain-service.ts (1)
9-9: Type import is appropriate.Using
ApiKeyfrom Prisma here clarifies the contract ofvalidateApiKeyDomainAccess.apps/web/src/server/service/api-service.ts (1)
51-55: Including domain info with the API key is useful.This enables downstream handlers (e.g., public API context) to enforce domain-scoped behavior without extra queries.
apps/web/src/server/api/routers/api.ts (2)
2-18: Good switch toz.nativeEnum(ApiPermission)and optionaldomainIdinput.Type safety improves and matches the Prisma enum; the input contract is clear.
41-46: LGTM on enriching list output with domain info.Selecting
domainIdanddomain.namewill make the UI “Domain Access” column straightforward to render.
- Implement detailed error handling for domain ownership and verification during API key creation. - Introduce TRPCError responses for better client feedback on domain-related issues. - Update addApiKey function to validate domain ownership and status before proceeding with key creation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/server/service/api-service.ts (1)
20-35: Great defense-in-depth: ownership + verification checks added (resolves prior feedback).Validating
domainIdand enforcing verified domains here is the right call and aligns with the PR objectives. This addresses the earlier recommendation to guard at the service layer as well.
🧹 Nitpick comments (7)
apps/web/src/server/service/api-service.ts (4)
22-29: Fold team ownership into the DB predicate (simpler and avoids a second check).Query the domain with both
idandteamIdso the DB enforces ownership in one step. This reduces branching and keeps the “doesn’t exist vs not owned” ambiguity intact.Apply this diff:
- const domain = await db.domain.findUnique({ - where: { id: domainId }, - select: { id: true, teamId: true, status: true }, - }); - - if (!domain || domain.teamId !== teamId) { - throw new Error("DOMAIN_NOT_OWNED"); - } + const domain = await db.domain.findFirst({ + where: { id: domainId, teamId }, + select: { id: true, status: true }, + }); + if (!domain) { + throw new Error("DOMAIN_NOT_OWNED"); + }
31-34: Avoid magic strings for status; use the Prisma enum for type-safety.Leverage
DomainStatus.SUCCESSto prevent typos and benefit from compiler checks.Apply this diff within the selected lines:
- if (domain.status !== "SUCCESS") { + if (domain.status !== DomainStatus.SUCCESS) { throw new Error("DOMAIN_NOT_VERIFIED"); }Add this import (outside the selected range):
// at top of file import { ApiPermission, DomainStatus } from "@prisma/client";
20-35: Prefer typed service errors over string-matching downstream.Throw a typed error so the router can
instanceofcheck instead of matching.message. This follows the guideline to use specific error types.Apply this diff within the selected lines:
- throw new Error("DOMAIN_NOT_OWNED"); + throw new ApiServiceError("DOMAIN_NOT_OWNED"); @@ - throw new Error("DOMAIN_NOT_VERIFIED"); + throw new ApiServiceError("DOMAIN_NOT_VERIFIED");And define the error class (outside the selected range):
// near the top of the file export class ApiServiceError extends Error { constructor(public code: "DOMAIN_NOT_OWNED" | "DOMAIN_NOT_VERIFIED") { super(code); this.name = "ApiServiceError"; } }
61-63: Early-validate API key format before hitting the DB.Guard against malformed keys to avoid unnecessary queries and potential undefined access.
Apply this diff:
export async function getTeamAndApiKey(apiKey: string) { - const [, clientId, token] = apiKey.split("_") as [string, string, string]; + const parts = apiKey.split("_"); + if (parts.length !== 3 || parts[0] !== "us") return null; + const [, clientId, token] = parts as [string, string, string];apps/web/src/server/api/routers/api.ts (3)
1-3: Order imports per guideline and prepare for typed error mapping.Group externals and alphabetize; also import the typed service error for robust error handling.
Apply this diff:
-import { z } from "zod"; -import { ApiPermission } from "@prisma/client"; -import { TRPCError } from "@trpc/server"; +import { ApiPermission } from "@prisma/client"; +import { TRPCError } from "@trpc/server"; +import { z } from "zod"; +import { ApiServiceError } from "~/server/service/api-service";
15-19: Tighten Zod validation forname.Protect against empty or overly long names at the boundary.
Apply this diff:
- z.object({ - name: z.string(), + z.object({ + name: z.string().min(1).max(64), permission: z.nativeEnum(ApiPermission), domainId: z.number().int().positive().optional(), })
21-45: Use typed error mapping instead of string matching; default to sanitized 500.Avoid brittle
.messagechecks. MapApiServiceErrorcodes, and return a generic 500 for unknown errors.Apply this diff:
- .mutation(async ({ ctx, input }) => { - try { - return await addApiKey({ + .mutation(async ({ ctx, input }) => { + try { + return await addApiKey({ name: input.name, permission: input.permission, teamId: ctx.team.id, domainId: input.domainId, }); - } catch (error) { - if (error instanceof Error) { - if (error.message === "DOMAIN_NOT_OWNED") { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Domain does not belong to your team", - }); - } - if (error.message === "DOMAIN_NOT_VERIFIED") { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Domain is not verified. Only verified domains can be used for API key restrictions", - }); - } - } - throw error; - } + } catch (error) { + if (error instanceof ApiServiceError) { + if (error.code === "DOMAIN_NOT_OWNED") { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Domain does not belong to your team", + }); + } + if (error.code === "DOMAIN_NOT_VERIFIED") { + throw new TRPCError({ + code: "BAD_REQUEST", + message: + "Domain is not verified. Only verified domains can be used for API key restrictions", + }); + } + } + throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" }); + } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/src/server/api/routers/api.ts(3 hunks)apps/web/src/server/service/api-service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
{apps,packages}/**/*.{js,jsx,ts,tsx,css,scss,md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with the Tailwind plugin for code formatting
Files:
apps/web/src/server/api/routers/api.tsapps/web/src/server/service/api-service.ts
{apps,packages}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{js,jsx,ts,tsx}: Group imports by source (internal/external) and alphabetize them
Use camelCase for variables and functions, PascalCase for components and classes
Use try/catch with specific error types for error handling
Files:
apps/web/src/server/api/routers/api.tsapps/web/src/server/service/api-service.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Use strong typing in TypeScript, avoidany, and use Zod for validation
Follow Vercel style guides with strict TypeScript
Files:
apps/web/src/server/api/routers/api.tsapps/web/src/server/service/api-service.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use tRPC for internal API endpoints
Files:
apps/web/src/server/api/routers/api.tsapps/web/src/server/service/api-service.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/api/routers/api.tsapps/web/src/server/service/api-service.ts
🧬 Code graph analysis (1)
apps/web/src/server/api/routers/api.ts (1)
apps/web/src/server/service/api-service.ts (1)
addApiKey(8-59)
🔇 Additional comments (3)
apps/web/src/server/service/api-service.ts (2)
48-48: LGTM: domain association is persisted correctly.Passing
domainIdthrough ensures the key is scoped as intended; omitting it keeps existing “all domains” behavior.
68-72: LGTM: eager-loading domain context is useful for callers.Selecting only
idandnameis lean and sufficient for display/use.apps/web/src/server/api/routers/api.ts (1)
60-65: LGTM: exposing domain context in listings.Returning both
domainIdanddomain.nameis exactly what the UI needs to show scope clearly.
apps/web/prisma/schema.prisma
Outdated
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| lastUsed DateTime? | ||
| teamId Int | ||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull) |
There was a problem hiding this comment.
this might be a security problem, i'd rather delete the api key
| @@ -59,5 +59,5 @@ export const getTeamFromToken = async (c: Context) => { | |||
| logger.error({ err }, "Failed to update lastUsed on API key") | |||
| ); | |||
|
|
|||
| return { ...team, apiKeyId: apiKey.id }; | |||
| return { ...team, apiKeyId: apiKey.id, apiKey }; | |||
There was a problem hiding this comment.
@enemyrr can you take a look at this, seems like valida comment, should only have need fields in context
| // Validate domain ownership if domainId is provided | ||
| if (domainId !== undefined) { | ||
| const domain = await db.domain.findUnique({ | ||
| where: { id: domainId }, |
There was a problem hiding this comment.
@enemyrr instead, teamId should be added to the where condition and error should just domainId not found instead of DOMAIN_NOT_OWNED, this will give out that the domain id is valid. i don't want to leak that out
| } | ||
|
|
||
| // Only allow verified domains to be scoped | ||
| if (domain.status !== "SUCCESS") { |
There was a problem hiding this comment.
not sure if we really need this,
| }); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| if (error.message === "DOMAIN_NOT_OWNED") { |
There was a problem hiding this comment.
yep, i don't want this error to be show to user
| @@ -33,7 +34,7 @@ export async function validateDomainFromEmail(email: string, teamId: number) { | |||
| }); | |||
| } | |||
|
|
|||
| const domain = await db.domain.findUnique({ | |||
| const domain = await db.domain.findFirst({ | |||
There was a problem hiding this comment.
@enemyrr why is this changed
There was a problem hiding this comment.
@KMKoushik The change from findUnique to findFirst was necessary because findUnique can only be used with fields that have a unique constraint.
In this case, we're querying with { name: fromDomain, teamId }, while name is unique, the combination of name + teamId is not defined as a unique constraint in the schema.
Using findUnique with non-unique field combinations causes Prisma runtime errors. findFirst is the correct method when filtering by multiple fields that don't form a unique constraint.
| export async function validateApiKeyDomainAccess( | ||
| email: string, | ||
| teamId: number, | ||
| apiKey: ApiKey & { domain?: { name: string } | null } | ||
| ) { | ||
| // First validate the domain exists and is verified | ||
| const domain = await validateDomainFromEmail(email, teamId); | ||
|
|
||
| // If API key has no domain restriction (domainId is null), allow all domains | ||
| if (!apiKey.domainId) { | ||
| return domain; | ||
| } | ||
|
|
||
| // If API key is restricted to a specific domain, check if it matches | ||
| if (apiKey.domainId !== domain.id) { | ||
| throw new UnsendApiError({ | ||
| code: "FORBIDDEN", | ||
| message: `API key does not have access to domain: ${domain.name}`, | ||
| }); | ||
| } | ||
|
|
||
| return domain; | ||
| } |
There was a problem hiding this comment.
nah, it's already checked in validateDomainFromEmail
| @@ -2,7 +2,6 @@ import { createRoute, z } from "@hono/zod-openapi"; | |||
| import { DomainSchema } from "~/lib/zod/domain-schema"; | |||
There was a problem hiding this comment.
@enemyrr verify-domain api should also verify this!
| @@ -2,7 +2,6 @@ import { createRoute, z } from "@hono/zod-openapi"; | |||
| import { DomainSchema } from "~/lib/zod/domain-schema"; | |||
| import { PublicAPIApp } from "~/server/public-api/hono"; | |||
There was a problem hiding this comment.
along with email apis should also be restricted, we are now allowing to fetch emails by other domains as well.
get-email
list-emails
update-email
cancel-email
- Introduce checkIsValidEmailIdWithDomainRestriction function to validate email access based on API key domain restrictions. - Update email-related API endpoints to utilize the new validation function, ensuring proper domain checks for email actions. - Modify API key model to include domainId and adjust related logic for domain verification during email operations. - Enhance error handling for domain access issues in email APIs, providing clearer feedback for unauthorized access.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/server/public-api/api/emails/list-emails.ts (1)
115-124: Bug: createdAt filter overwritten — combine gte/lte instead of replacingSetting
whereClause.createdAttwice drops the first bound. Merge both conditions.Apply:
- if (startDate) { - whereClause.createdAt = { - gte: new Date(startDate), - }; - } - if (endDate) { - whereClause.createdAt = { - lte: new Date(endDate), - }; - } + if (startDate || endDate) { + whereClause.createdAt = { + ...(startDate ? { gte: new Date(startDate) } : {}), + ...(endDate ? { lte: new Date(endDate) } : {}), + }; + }apps/web/src/server/public-api/api/emails/update-email.ts (1)
35-43: Fix OpenAPI description (copy-paste error)The response description says “Retrieve the user” for an email scheduling endpoint. Update to reflect the actual operation for accurate public API docs.
- description: "Retrieve the user", + description: "Update the email's scheduled time",apps/web/src/server/public-api/api/emails/cancel-email.ts (1)
24-33: Fix OpenAPI description (copy-paste error)This endpoint cancels a scheduled email, but the description says “Retrieve the user.” Update to reflect the correct operation.
- description: "Retrieve the user", + description: "Cancel a scheduled email",
♻️ Duplicate comments (2)
apps/web/prisma/schema.prisma (1)
208-215: Optional: Enforce domain–team invariant at DB or service layerCurrently nothing prevents
ApiKey.domainIdfrom referencing a Domain of another team purely at the DB level. You already validate at service/router layers elsewhere; consider strengthening:
- DB-level (requires composite relation):
[domainId, teamId] -> Domain.[id, teamId]with a corresponding unique/index onDomain(teamId, id)or@@id([id, teamId]).- Or keep service-layer validation but add explicit tests.
If you want a DB-level approach, I can draft the exact Prisma schema changes in a follow-up.
apps/web/src/server/api/routers/api.ts (1)
22-27: Wrap service errors and validate domainId at the API boundary to return proper 400s (and use TRPCError import)Currently this blindly forwards to
addApiKey, and any validation error bubbles up as a generic 500. Per guidelines (“Use try/catch with specific error types”) and product comment not to surface raw errors, wrap and normalize them. Also, validatedomainIdbelongs toctx.team.idup front to fail fast.Apply:
- .mutation(async ({ ctx, input }) => { - return await addApiKey({ - name: input.name, - permission: input.permission, - teamId: ctx.team.id, - domainId: input.domainId, - }); - }), + .mutation(async ({ ctx, input }) => { + try { + if (input.domainId !== undefined) { + const domain = await ctx.db.domain.findFirst({ + where: { id: input.domainId, teamId: ctx.team.id }, + select: { id: true }, + }); + if (!domain) { + throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid domainId for this team" }); + } + } + return await addApiKey({ + name: input.name, + permission: input.permission, + teamId: ctx.team.id, + domainId: input.domainId, + }); + } catch (err: any) { + // Map known service sentinel errors + if (err?.message === "DOMAIN_NOT_FOUND") { + throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid domainId for this team" }); + } + throw err; + } + }),Note: This also makes the
TRPCErrorimport used. If you choose not to handle here, remove the import. Also,return awaitis only needed because we catch; otherwise it’s redundant.
🧹 Nitpick comments (7)
apps/web/src/server/public-api/api/domains/verify-domain.ts (5)
28-29: Nit: clarify description to match response message.The success message says “Domain verification started”. Consider making the description consistent.
- description: "Verify domain", + description: "Start domain verification",
59-80: Early-return on domain restriction to avoid unnecessary DB work and simplify logic.Short-circuit when the API key is scoped to a different domain. This also removes the need for the mutable
let domain = null;(which widens toanyunless annotated).- let domain = null; - - if (team.apiKey.domainId) { - // If API key is restricted to a specific domain, verify the requested domain matches - if (domainId === team.apiKey.domainId) { - domain = await db.domain.findFirst({ - where: { - teamId: team.id, - id: domainId - }, - }); - } - // If domainId doesn't match the API key's restriction, domain remains null - } else { - // API key has access to all team domains - domain = await db.domain.findFirst({ - where: { - teamId: team.id, - id: domainId - } - }); - } + const restrictedDomainId = team.apiKey.domainId; + if (restrictedDomainId != null && domainId !== restrictedDomainId) { + return c.json({ error: "API key doesn't have access to this domain" }, 403); + } + const domain = await db.domain.findFirst({ + where: { teamId: team.id, id: domainId }, + });
90-93: Scope the update by teamId (defense-in-depth) and verify affected rows.We validated team ownership earlier, but scoping the write and checking the result prevents accidental cross-tenant updates in case of future refactors and avoids TOCTOU edges.
- await db.domain.update({ - where: { id: domainId }, - data: { isVerifying: true }, - }); + const { count } = await db.domain.updateMany({ + where: { id: domainId, teamId: team.id }, + data: { isVerifying: true }, + }); + if (count === 0) { + return c.json({ error: "Domain not found" }, 404); + }
54-98: Add structured error handling for Prisma errors.Wrap DB calls in try/catch and map known Prisma errors to 4xx vs 5xx. This follows the repo guideline to use try/catch with specific error types and prevents leaking internals on exceptions.
// At top-level imports (illustrative, outside this hunk) import { Prisma } from "@prisma/client"; // Inside handler try { // existing handler logic... } catch (err) { if (err instanceof Prisma.PrismaClientKnownRequestError) { // e.g., map P2025 (record not found) if it can surface here return c.json({ error: "Domain not found" }, 404); } // fallback return c.json({ error: "Internal server error" }, 500); }
58-80: Consider centralizing domain-access checks in a shared service to keep behavior consistent across routes.The PR adds a domain-access validation pathway in the domain service. Reusing it here would reduce duplication and ensure consistent 403/404 semantics across public API routes.
Happy to refactor this handler to use the shared validator if you point me to the exact helper signature you want standardized.
apps/web/src/server/public-api/api-utils.ts (1)
29-35: Optional: Unify email-id validation helpersTo reduce duplication and ensure consistent domain enforcement, have
checkIsValidEmailIdcall the new function withapiKeyDomainId = null.Proposed (outside current range):
export const checkIsValidEmailId = async (emailId: string, teamId: number) => { await checkIsValidEmailIdWithDomainRestriction(emailId, teamId, null); };apps/web/src/server/public-api/api/emails/update-email.ts (1)
27-29: Optional: validate schedule is in the future and pass a Date to the serviceTo prevent accidental back-dating, validate
scheduledAtis in the future. IfupdateEmailexpects aDate, convert here.- scheduledAt: z.string().datetime(), + scheduledAt: z + .string() + .datetime() + .refine((s) => !Number.isNaN(Date.parse(s)) && new Date(s) > new Date(), { + message: "scheduledAt must be a future datetime (ISO 8601)", + }),- await updateEmail(emailId, { - scheduledAt: c.req.valid("json").scheduledAt, - }); + const { scheduledAt } = c.req.valid("json"); + await updateEmail(emailId, { + // If updateEmail accepts string, keep it as-is; otherwise convert to Date: + scheduledAt: new Date(scheduledAt), + });Confirm what type
updateEmail(...).scheduledAtexpects. If it’s a string today, I’ll adjust the diff to avoid theDateconversion and only keep the future-date validation.Also applies to: 54-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/web/prisma/schema.prisma(2 hunks)apps/web/src/server/api/routers/api.ts(3 hunks)apps/web/src/server/public-api/api-utils.ts(1 hunks)apps/web/src/server/public-api/api/domains/verify-domain.ts(1 hunks)apps/web/src/server/public-api/api/emails/cancel-email.ts(2 hunks)apps/web/src/server/public-api/api/emails/get-email.ts(1 hunks)apps/web/src/server/public-api/api/emails/list-emails.ts(1 hunks)apps/web/src/server/public-api/api/emails/update-email.ts(2 hunks)apps/web/src/server/public-api/auth.ts(1 hunks)apps/web/src/server/public-api/hono.ts(1 hunks)apps/web/src/server/service/api-service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/server/public-api/auth.ts
- apps/web/src/server/public-api/hono.ts
- apps/web/src/server/service/api-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
{apps,packages}/**/*.{js,jsx,ts,tsx,css,scss,md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with the Tailwind plugin for code formatting
Files:
apps/web/src/server/public-api/api/emails/get-email.tsapps/web/src/server/public-api/api/emails/list-emails.tsapps/web/src/server/public-api/api/domains/verify-domain.tsapps/web/src/server/public-api/api-utils.tsapps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.tsapps/web/src/server/api/routers/api.ts
{apps,packages}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{js,jsx,ts,tsx}: Group imports by source (internal/external) and alphabetize them
Use camelCase for variables and functions, PascalCase for components and classes
Use try/catch with specific error types for error handling
Files:
apps/web/src/server/public-api/api/emails/get-email.tsapps/web/src/server/public-api/api/emails/list-emails.tsapps/web/src/server/public-api/api/domains/verify-domain.tsapps/web/src/server/public-api/api-utils.tsapps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.tsapps/web/src/server/api/routers/api.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}: Use strong typing in TypeScript, avoidany, and use Zod for validation
Follow Vercel style guides with strict TypeScript
Files:
apps/web/src/server/public-api/api/emails/get-email.tsapps/web/src/server/public-api/api/emails/list-emails.tsapps/web/src/server/public-api/api/domains/verify-domain.tsapps/web/src/server/public-api/api-utils.tsapps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.tsapps/web/src/server/api/routers/api.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use tRPC for internal API endpoints
Files:
apps/web/src/server/public-api/api/emails/get-email.tsapps/web/src/server/public-api/api/emails/list-emails.tsapps/web/src/server/public-api/api/domains/verify-domain.tsapps/web/src/server/public-api/api-utils.tsapps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.tsapps/web/src/server/api/routers/api.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/public-api/api/emails/get-email.tsapps/web/src/server/public-api/api/emails/list-emails.tsapps/web/src/server/public-api/api/domains/verify-domain.tsapps/web/src/server/public-api/api-utils.tsapps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.tsapps/web/src/server/api/routers/api.ts
🧠 Learnings (1)
📚 Learning: 2025-08-09T10:37:58.146Z
Learnt from: CR
PR: unsend-dev/unsend#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-09T10:37:58.146Z
Learning: Applies to apps/smtp-server/**/*.{ts,tsx} : Use Hono for public API endpoints
Applied to files:
apps/web/src/server/public-api/api/emails/update-email.tsapps/web/src/server/public-api/api/emails/cancel-email.ts
🧬 Code graph analysis (6)
apps/web/src/server/public-api/api/emails/get-email.ts (1)
apps/web/src/server/db.ts (1)
db(20-20)
apps/web/src/server/public-api/api/domains/verify-domain.ts (2)
apps/web/src/server/public-api/hono.ts (1)
PublicAPIApp(136-136)apps/web/src/server/db.ts (1)
db(20-20)
apps/web/src/server/public-api/api-utils.ts (2)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)
apps/web/src/server/public-api/api/emails/update-email.ts (1)
apps/web/src/server/public-api/api-utils.ts (1)
checkIsValidEmailIdWithDomainRestriction(37-58)
apps/web/src/server/public-api/api/emails/cancel-email.ts (1)
apps/web/src/server/public-api/api-utils.ts (1)
checkIsValidEmailIdWithDomainRestriction(37-58)
apps/web/src/server/api/routers/api.ts (1)
apps/web/src/server/service/api-service.ts (1)
addApiKey(8-57)
🔇 Additional comments (5)
apps/web/prisma/schema.prisma (1)
193-194: LGTM: Back-reference from Domain to ApiKeys is correctThe
apiKeys: ApiKey[]relation is consistent and improves discoverability from Domain. No issues here.apps/web/src/server/api/routers/api.ts (1)
42-47: LGTM: Exposing domain context in getApiKeysSelecting
domainIdanddomain.nameis useful for the UI; preserves backward compatibility whendomainIdis null.apps/web/src/server/public-api/api/emails/list-emails.ts (1)
126-129: LGTM: Domain scoping precedenceWhen the API key is domain-scoped, you correctly force
whereClause.domainId = apiKey.domainId; otherwise, you honor the query filter. Matches the access-control intent.apps/web/src/server/public-api/api/emails/update-email.ts (1)
5-5: LGTM: switched to domain-aware validatorImporting
checkIsValidEmailIdWithDomainRestrictionaligns this endpoint with the new domain-scoped access control.apps/web/src/server/public-api/api/emails/cancel-email.ts (1)
5-5: LGTM: switched to domain-aware validatorThe import aligns this endpoint with domain-scoped access control.
| domainId Int? | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| lastUsed DateTime? | ||
| teamId Int | ||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: Cascade) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Index missing for ApiKey.domainId; add for query performance and future constraints
Domain-scoped lookups (listing keys by domain, enforcing access, etc.) will benefit from an index. Add @@index([domainId]) to ApiKey.
Apply:
model ApiKey {
id Int @id @default(autoincrement())
clientId String @unique
tokenHash String
partialToken String
name String
permission ApiPermission @default(SENDING)
domainId Int?
+ @@index([domainId])
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
lastUsed DateTime?
teamId Int
team Team @relation(fields: [teamId], references: [id], onDelete: Cascade)
domain Domain? @relation(fields: [domainId], references: [id], onDelete: Cascade)
}Additionally, given frequent filters by teamId and domainId (and sorts by createdAt), consider in a follow-up migration:
model Email {
// ...
@@index([teamId, domainId, createdAt(sort: Desc)])
}🤖 Prompt for AI Agents
In apps/web/prisma/schema.prisma around lines 208 to 215, the ApiKey model is
missing an index on domainId which will hurt domain-scoped lookups and future
constraints; add an @@index([domainId]) declaration to the ApiKey model (and
then run the Prisma migration to apply it). Also consider as a follow-up adding
a composite index for Email on teamId + domainId + createdAt (sorted Desc) to
optimize common filtered-and-sorted queries.
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| lastUsed DateTime? | ||
| teamId Int | ||
| team Team @relation(fields: [teamId], references: [id], onDelete: Cascade) | ||
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: Cascade) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema/migration mismatch on ApiKey.domain referential action (Cascade vs SetNull) — pick one and align everywhere
The Prisma schema uses onDelete: Cascade for ApiKey.domain, but the migration (20250822125136...) sets the FK to ON DELETE SET NULL. This discrepancy can lead to drift warnings and unexpected runtime behavior. Given the prior product preference (“i'd rather delete the api key”), prefer Cascade and update the migration accordingly. Otherwise, switch the schema back to SetNull.
Action:
- Update the existing migration or create a follow-up migration to align the FK action with the schema choice.
- Add an entry in the changelog describing the semantics on domain deletion.
Apply this change in the Prisma schema only if you decide to keep SetNull instead:
- domain Domain? @relation(fields: [domainId], references: [id], onDelete: Cascade)
+ domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull)If keeping Cascade (recommended per product comment), alter the FK in the migration to ON DELETE CASCADE and re-generate the client.
📝 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.
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: Cascade) | |
| domain Domain? @relation(fields: [domainId], references: [id], onDelete: SetNull) |
🤖 Prompt for AI Agents
In apps/web/prisma/schema.prisma around line 214 there is a referential-action
mismatch: the schema currently declares ApiKey.domain with onDelete: Cascade
while the existing migration SQL uses ON DELETE SET NULL; choose one and align
everywhere — recommended: keep Cascade, update the migration to use ON DELETE
CASCADE (or if you prefer SetNull, change the schema back to SetNull), then
either edit the existing migration SQL to replace SET NULL with CASCADE or
create a new follow-up migration that changes the FK to ON DELETE CASCADE, run
prisma migrate/resolve as appropriate, regenerate the Prisma client (prisma
generate), and add a changelog entry describing the domain-deletion semantics
for ApiKey.
| export const checkIsValidEmailIdWithDomainRestriction = async ( | ||
| emailId: string, | ||
| teamId: number, | ||
| apiKeyDomainId?: number | ||
| ) => { | ||
| const whereClause: { id: string; teamId: number; domainId?: number } = { | ||
| id: emailId, | ||
| teamId, | ||
| }; | ||
|
|
||
| if (apiKeyDomainId !== undefined) { | ||
| whereClause.domainId = apiKeyDomainId; | ||
| } | ||
|
|
||
| const email = await db.email.findUnique({ where: whereClause }); | ||
|
|
||
| if (!email) { | ||
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | ||
| } | ||
|
|
||
| return email; | ||
| }; |
There was a problem hiding this comment.
Fix two issues: (1) null vs undefined check causes unintended filtering; (2) findUnique with non-unique where
- If callers pass
null(they do:team.apiKey.domainIdisnumber | null), the current!== undefinedcheck setsdomainId: null, restricting to emails withNULLdomainId — wrong. findUnique({ where: { id, teamId, domainId? }})is invalid unless there’s a composite unique; usefindFirst.
Apply:
-export const checkIsValidEmailIdWithDomainRestriction = async (
- emailId: string,
- teamId: number,
- apiKeyDomainId?: number
-) => {
- const whereClause: { id: string; teamId: number; domainId?: number } = {
- id: emailId,
- teamId,
- };
-
- if (apiKeyDomainId !== undefined) {
- whereClause.domainId = apiKeyDomainId;
- }
-
- const email = await db.email.findUnique({ where: whereClause });
+export const checkIsValidEmailIdWithDomainRestriction = async (
+ emailId: string,
+ teamId: number,
+ apiKeyDomainId: number | null
+) => {
+ const whereClause = {
+ id: emailId,
+ teamId,
+ ...(apiKeyDomainId !== null ? { domainId: apiKeyDomainId } : {}),
+ };
+
+ const email = await db.email.findFirst({ where: whereClause });
if (!email) {
throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" });
}
return email;
};Follow-up: consider refactoring existing checkIsValidEmailId to delegate to this function for consistency.
📝 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.
| export const checkIsValidEmailIdWithDomainRestriction = async ( | |
| emailId: string, | |
| teamId: number, | |
| apiKeyDomainId?: number | |
| ) => { | |
| const whereClause: { id: string; teamId: number; domainId?: number } = { | |
| id: emailId, | |
| teamId, | |
| }; | |
| if (apiKeyDomainId !== undefined) { | |
| whereClause.domainId = apiKeyDomainId; | |
| } | |
| const email = await db.email.findUnique({ where: whereClause }); | |
| if (!email) { | |
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | |
| } | |
| return email; | |
| }; | |
| export const checkIsValidEmailIdWithDomainRestriction = async ( | |
| emailId: string, | |
| teamId: number, | |
| apiKeyDomainId: number | null | |
| ) => { | |
| const whereClause = { | |
| id: emailId, | |
| teamId, | |
| ...(apiKeyDomainId !== null ? { domainId: apiKeyDomainId } : {}), | |
| }; | |
| const email = await db.email.findFirst({ where: whereClause }); | |
| if (!email) { | |
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | |
| } | |
| return email; | |
| }; |
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api-utils.ts around lines 37 to 58, the
function incorrectly treats a null apiKeyDomainId as a valid filter and uses
findUnique with a non-unique where clause; update the guard to only add domainId
when apiKeyDomainId is not null or undefined (e.g., if (apiKeyDomainId != null)
whereClause.domainId = apiKeyDomainId), and replace db.email.findUnique(...)
with db.email.findFirst(...) so the query works without a composite unique;
afterwards consider refactoring checkIsValidEmailId to call this function for
consistency.
| description: "Verify domain", | ||
| }, | ||
| 403: { | ||
| content: { | ||
| "application/json": { | ||
| schema: z.object({ | ||
| error: z.string(), | ||
| }), | ||
| }, | ||
| }, | ||
| description: "Forbidden - API key doesn't have access to this domain", | ||
| }, | ||
| 404: { | ||
| content: { | ||
| "application/json": { | ||
| schema: z.object({ | ||
| error: z.string(), | ||
| }), | ||
| }, | ||
| }, | ||
| description: "Domain not found", | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
OpenAPI declares 403 but handler never returns it — align the contract and implementation.
You document a 403 (“Forbidden - API key doesn't have access to this domain”), but the handler currently always returns 404 in the failure branch. Either change the handler to return 403 for access-denied or remove the 403 from the spec. I’ve proposed a handler fix in another comment (Lines 82–88).
Align OpenAPI 403 response with handler implementation
The OpenAPI spec in apps/web/src/server/public-api/api/domains/verify-domain.ts (lines 28–49) declares a 403 (“Forbidden – API key doesn’t have access to this domain”), but the handler’s error branch (around lines 82–88) always returns a 404. To keep contract and code in sync, please choose one:
- Update the handler so that when the API key lacks access it returns a 403 status with the
{ error: string }payload as documented. - Or, if access checks are consolidated elsewhere, remove the 403 entry from the OpenAPI definition.
Files to update:
- apps/web/src/server/public-api/api/domains/verify-domain.ts
- Remove or adjust the 403 response schema (lines 28–49)
- Change the error branch to
res.status(403).send({ error })(around lines 82–88)
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api/domains/verify-domain.ts around lines
28–49 and the handler around lines 82–88, the OpenAPI spec declares a 403
response but the handler currently always returns 404 for access-denied cases;
fix by either (A) updating the handler error branch to return
res.status(403).send({ error }) when the API key lacks access so it matches the
documented 403 schema, or (B) removing the 403 response block from the OpenAPI
definition if access checks are enforced elsewhere; apply the corresponding
change in this file so the contract and implementation are consistent.
| if (!domain) { | ||
| return c.json({ | ||
| error: team.apiKey.domainId | ||
| ? "API key doesn't have access to this domain" | ||
| : "Domain not found" | ||
| }, 404); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return 403 for access-denied and 404 for missing domain.
This matches the OpenAPI and provides clearer semantics.
- if (!domain) {
- return c.json({
- error: team.apiKey.domainId
- ? "API key doesn't have access to this domain"
- : "Domain not found"
- }, 404);
- }
+ if (!domain) {
+ const restricted = team.apiKey.domainId != null;
+ return c.json(
+ {
+ error: restricted
+ ? "API key doesn't have access to this domain"
+ : "Domain not found",
+ },
+ restricted ? 403 : 404,
+ );
+ }📝 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 (!domain) { | |
| return c.json({ | |
| error: team.apiKey.domainId | |
| ? "API key doesn't have access to this domain" | |
| : "Domain not found" | |
| }, 404); | |
| } | |
| if (!domain) { | |
| const restricted = team.apiKey.domainId != null; | |
| return c.json( | |
| { | |
| error: restricted | |
| ? "API key doesn't have access to this domain" | |
| : "Domain not found", | |
| }, | |
| restricted ? 403 : 404, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api/domains/verify-domain.ts around lines 82
to 88, the handler currently returns a 404 for both missing domains and
access-denied cases; update the response so that if domain is falsy and
team.apiKey.domainId is set you return a 403 status with the access-denied
message, otherwise return a 404 with the "Domain not found" message, preserving
the same JSON shape.
| @@ -37,7 +37,7 @@ function cancelScheduledEmail(app: PublicAPIApp) { | |||
| app.openapi(route, async (c) => { | |||
| const team = c.var.team; | |||
| const emailId = c.req.param("emailId"); | |||
| await checkIsValidEmailId(emailId, team.id); | |||
| await checkIsValidEmailIdWithDomainRestriction(emailId, team.id, team.apiKey.domainId); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: same null domainId handling and validator misuse apply here
This call will also pass null for legacy keys, which the current validator treats as an explicit domainId = null filter, denying access to emails for verified domains. Please apply the helper fix proposed in the other comment to restore backward compatibility and to emit a proper 403 when access is denied by domain.
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api/emails/cancel-email.ts around line 40, the
call to checkIsValidEmailIdWithDomainRestriction(emailId, team.id,
team.apiKey.domainId) will pass null for legacy keys which the validator treats
as an explicit null filter and denies access; replace the direct pass with the
same helper used in the other comment that converts legacy null domainId to
undefined (restoring backward compatibility), and ensure the validator's
rejection is caught and converted to an HTTP 403 response when access is denied
by domain.
| const whereClause: { id: string; teamId: number; domainId?: number } = { | ||
| id: emailId, | ||
| teamId: team.id, | ||
| }; | ||
|
|
||
| if (team.apiKey.domainId !== null) { | ||
| whereClause.domainId = team.apiKey.domainId; | ||
| } | ||
|
|
||
| const email = await db.email.findUnique({ | ||
| where: { | ||
| id: emailId, | ||
| teamId: team.id, | ||
| }, | ||
| where: whereClause, | ||
| select: { |
There was a problem hiding this comment.
Bug: Using findUnique with non-unique composite where (id + teamId [+ domainId]) will not compile/run
findUnique only accepts a unique selector (e.g., { id }) or a declared composite unique. Adding teamId/domainId makes it an invalid UniqueWhere. Use findFirst with a normal where filter and preserve domain scoping.
Apply:
- const whereClause: { id: string; teamId: number; domainId?: number } = {
- id: emailId,
- teamId: team.id,
- };
-
- if (team.apiKey.domainId !== null) {
- whereClause.domainId = team.apiKey.domainId;
- }
+ const whereClause = {
+ id: emailId,
+ teamId: team.id,
+ ...(team.apiKey.domainId !== null ? { domainId: team.apiKey.domainId } : {}),
+ };
@@
- const email = await db.email.findUnique({
- where: whereClause,
+ const email = await db.email.findFirst({
+ where: whereClause,
select: {Optional: import and use Prisma.EmailWhereInput for stricter typing of whereClause.
📝 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 whereClause: { id: string; teamId: number; domainId?: number } = { | |
| id: emailId, | |
| teamId: team.id, | |
| }; | |
| if (team.apiKey.domainId !== null) { | |
| whereClause.domainId = team.apiKey.domainId; | |
| } | |
| const email = await db.email.findUnique({ | |
| where: { | |
| id: emailId, | |
| teamId: team.id, | |
| }, | |
| where: whereClause, | |
| select: { | |
| const whereClause = { | |
| id: emailId, | |
| teamId: team.id, | |
| ...(team.apiKey.domainId !== null ? { domainId: team.apiKey.domainId } : {}), | |
| }; | |
| const email = await db.email.findFirst({ | |
| where: whereClause, | |
| select: { |
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api/emails/get-email.ts around lines 63–74,
the code builds a composite whereClause (id + teamId [+ domainId]) but calls
db.email.findUnique, which requires a unique selector; change to
db.email.findFirst and pass the built whereClause as the normal where filter so
non-unique composite filters compile and run correctly. Keep the domain scoping
logic (set domainId only when team.apiKey.domainId !== null), and optionally
import and type whereClause as Prisma.EmailWhereInput for stricter typing.
| @@ -48,7 +48,7 @@ function updateEmailScheduledAt(app: PublicAPIApp) { | |||
| const team = c.var.team; | |||
| const emailId = c.req.param("emailId"); | |||
|
|
|||
| await checkIsValidEmailId(emailId, team.id); | |||
| await checkIsValidEmailIdWithDomainRestriction(emailId, team.id, team.apiKey.domainId); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: null domainId is treated as a filter (breaking “All Domains”) and validator likely misuses Prisma findUnique
Passing team.apiKey.domainId directly will be null for legacy “All Domains” keys. The current validator (see api-utils.ts snippet in the PR) checks apiKeyDomainId !== undefined, which is true for null and will set whereClause.domainId = null. That restricts queries to emails with domainId IS NULL, inadvertently denying access to emails belonging to any domain and breaking backward compatibility.
Additionally, the validator uses db.email.findUnique({ where: { id, teamId, domainId? } }). In Prisma, findUnique only accepts a unique selector (e.g., by id, or a defined composite unique). If id is already unique, adding teamId/domainId will not match the UniqueWhereInput type, causing a type error or being rejected by the client. The correct approach is either findFirst with a where filter, or fetch by { id, teamId } and then enforce domain.
Action:
- Treat
nullthe same as “no domain restriction” (don’t adddomainIdto the filter). - Replace
findUniquewithfindFirst(or fetch by{ id, teamId }then check domain). - Return a clear 403/“FORBIDDEN” error when the email exists but the key’s domain doesn’t match, to meet the PR objective “returns clear error messages when a key lacks domain access.”
Proposed fix for apps/web/src/server/public-api/api-utils.ts:
-export const checkIsValidEmailIdWithDomainRestriction = async (
- emailId: string,
- teamId: number,
- apiKeyDomainId?: number
-) => {
- const whereClause: { id: string; teamId: number; domainId?: number } = {
- id: emailId,
- teamId,
- };
-
- if (apiKeyDomainId !== undefined) {
- whereClause.domainId = apiKeyDomainId;
- }
-
- const email = await db.email.findUnique({ where: whereClause });
-
- if (!email) {
- throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" });
- }
-
- return email;
-};
+export const checkIsValidEmailIdWithDomainRestriction = async (
+ emailId: string,
+ teamId: number,
+ apiKeyDomainId?: number | null
+) => {
+ // Fetch by id+teamId first (single query) then enforce domain restriction.
+ const email = await db.email.findFirst({
+ where: { id: emailId, teamId },
+ select: { id: true, teamId: true, domainId: true },
+ });
+
+ if (!email) {
+ throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" });
+ }
+
+ // Only enforce domain check when the API key is actually scoped.
+ if (apiKeyDomainId != null && email.domainId !== apiKeyDomainId) {
+ throw new UnsendApiError({
+ code: "FORBIDDEN",
+ message: "API key does not have access to this email's domain",
+ });
+ }
+
+ return email;
+};No changes are needed at this call site after updating the helper.
I can push a follow-up commit with this fix and add tests covering: legacy key (null domain), scoped key allowed domain, scoped key disallowed domain (403), and non-existent email (404).
📝 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.
| await checkIsValidEmailIdWithDomainRestriction(emailId, team.id, team.apiKey.domainId); | |
| // apps/web/src/server/public-api/api-utils.ts | |
| export const checkIsValidEmailIdWithDomainRestriction = async ( | |
| emailId: string, | |
| teamId: number, | |
| apiKeyDomainId?: number | null | |
| ) => { | |
| // Fetch by id+teamId first (single query) then enforce domain restriction. | |
| const email = await db.email.findFirst({ | |
| where: { id: emailId, teamId }, | |
| select: { id: true, teamId: true, domainId: true }, | |
| }); | |
| if (!email) { | |
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | |
| } | |
| // Only enforce domain check when the API key is actually scoped. | |
| if (apiKeyDomainId != null && email.domainId !== apiKeyDomainId) { | |
| throw new UnsendApiError({ | |
| code: "FORBIDDEN", | |
| message: "API key does not have access to this email's domain", | |
| }); | |
| } | |
| return email; | |
| }; |
Summary
Changes
domainIdfield toApiKeymodel with foreign key relation toDomainFeatures
Test plan
Summary by CodeRabbit
New Features
Behavior Changes