chore: Phase 3 code review - API routes#7
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA large codebase-wide normalization of string quotes plus targeted functional changes: password-reset token validation and reset, order submission flow (Dr. Green integration, webhook + email triggers, response shape), new/rewritten webhook handlers, rate-limit and authorization enhancements, pagination and tenant-scoped safeguards across many admin routes. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Order Submit Endpoint
participant DB as Database
participant DrGreen as Dr. Green API
participant Webhook as Webhook Service
participant Email as Email Service
Client->>API: POST /store/[slug]/orders/submit (order + shipping)
API->>API: Validate auth, body, tenant, shipping
API->>DrGreen: submitOrder(payload, credentials)
DrGreen-->>API: drGreenOrderId / error
API->>DB: create order record (items, productName, totals)
API->>DB: create audit log
API->>Webhook: trigger ORDER_CREATED (orderId, drGreenOrderId, total, userEmail)
API->>Email: send confirmation email
API-->>Client: 201 { success: true, order }
sequenceDiagram
actor External as External Payment Webhook
participant API as Webhook Handler
participant DB as Database
participant Audit as Audit Log
participant BudStack as BudStack Webhook
External->>API: POST payment notification (fiat/crypto)
API->>API: Parse payload, validate nonce/reference
API->>DB: find order by reference/nonce
alt order found
API->>DB: update paymentStatus, invoiceNum, clear nonce
API->>Audit: record webhook receipt
alt payment PAID
API->>DB: set order status CONFIRMED
API->>BudStack: trigger ORDER_CONFIRMED webhook
else payment FAILED
API->>BudStack: trigger ORDER_CANCELLED webhook
end
API-->>External: 200 { status, orderId, paymentStatus }
else not found
API->>Audit: log failed lookup
API-->>External: 404 { error }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
nextjs_space/app/api/consultation/submit/route.ts (3)
38-53: Fallback returns invalid format for unmapped countries.When a country code isn't in the mapping, the function returns the original Alpha-2 code. Since the Dr. Green API requires Alpha-3 codes (per the docstring), this will cause API failures for customers from unmapped countries.
Consider either:
- Throwing an error for unsupported countries with a clear message
- Using a comprehensive mapping library like
i18n-iso-countries💡 Option 1: Fail explicitly for unmapped countries
function convertToAlpha3CountryCode(alpha2: string): string { const mapping: Record<string, string> = { PT: "PRT", // Portugal // ... existing mappings }; - return mapping[alpha2.toUpperCase()] || alpha2; + const alpha3 = mapping[alpha2.toUpperCase()]; + if (!alpha3) { + throw new Error(`Unsupported country code: ${alpha2}. Please add mapping for this country.`); + } + return alpha3; }
70-81: Missing input validation before processing.The handler directly accesses
body.password,body.email, etc. without validation. If any required field is missing or malformed:
bcrypt.hash(body.password, 10)will throw ifbody.passwordisundefinedbody.emailcould beundefinedcausing thefindUniqueto failConsider adding validation at the start of the handler or using a schema validation library (e.g., Zod).
💡 Example validation
export async function POST(request: NextRequest) { try { const body = await request.json(); // Basic validation if (!body.email || !body.password || !body.firstName || !body.lastName) { return NextResponse.json( { success: false, error: "Missing required fields" }, { status: 400 } ); } // ... rest of handler
115-117: DefaultingdateOfBirthto current date is incorrect.If
body.dateOfBirthis falsy, the code defaults tonew Date()(today). For a medical consultation system, date of birth should be a required field, not silently defaulted to an incorrect value. This could lead to incorrect age calculations in medical records.💡 Suggested fix
- dateOfBirth: body.dateOfBirth ? new Date(body.dateOfBirth) : new Date(), + dateOfBirth: new Date(body.dateOfBirth), // Ensure validation requires this fieldAdd to validation:
if (!body.dateOfBirth) { return NextResponse.json( { success: false, error: "Date of birth is required" }, { status: 400 } ); }nextjs_space/app/api/doctor-green/sync-products.disabled/route.ts (2)
10-23: Security: Missing authentication allows arbitrary tenant impersonation.The
tenantIdis taken directly from the request body without any authentication or authorization check. Any caller can sync products for any tenant by simply providing a differenttenantId. The comment on line 11 acknowledges this needs to be fixed, but this is a critical security gap if the route is ever enabled.Additionally, the dynamic import on line 22 is unnecessary—
getTenantDrGreenConfigcan be imported statically at the top of the file.🔐 Recommended fix: Add authentication and use static import
import { NextRequest, NextResponse } from "next/server"; import { fetchProducts } from "@/lib/doctor-green-api"; import { prisma } from "@/lib/db"; +import { getTenantDrGreenConfig } from "@/lib/tenant-config"; +import { getServerSession } from "next-auth"; // or your auth method +import { authOptions } from "@/lib/auth"; export async function POST(request: NextRequest) { try { - // Get tenant ID from request (will be from auth session in production) - const { tenantId } = await request.json(); + // Authenticate and get tenant ID from session + const session = await getServerSession(authOptions); + if (!session?.user?.tenantId) { + return NextResponse.json( + { success: false, error: "Unauthorized" }, + { status: 401 }, + ); + } + const tenantId = session.user.tenantId; - if (!tenantId) { - return NextResponse.json( - { success: false, error: "Tenant ID required" }, - { status: 400 }, - ); - } - - // Fetch tenant-specific Dr Green Config - const { getTenantDrGreenConfig } = await import("@/lib/tenant-config"); const doctorGreenConfig = await getTenantDrGreenConfig(tenantId);
54-76: Critical: Code uses non-existent database fields and has TOCTOU race condition.The code attempts to use fields (
doctorGreenId,doctorGreenData,image,inStock,stockQuantity,lastSyncedAt) that don't exist in theproductsschema. This will cause runtime Prisma validation errors. Additionally, thefindFirst+ conditionalupdate/createpattern introduces a TOCTOU race condition and N+1 query inefficiency (2 queries per product).Before optimization, the schema must be updated to include the missing doctor-green integration fields. Once added, replace the manual check-then-write pattern with Prisma's atomic
upsertmethod wrapped in a transaction.nextjs_space/app/api/tenant-admin/products/reorder/route.ts (1)
29-39: SUPER_ADMIN users will receive a 404 error due to missing tenantId.SUPER_ADMIN users created in the seed lack a
tenantId(the field is optional in the user model), but the code requires it and returns a 404 if missing. This is a systemic issue across tenant-admin routes (products/bulk, orders, etc.). Either assigntenantIdto SUPER_ADMIN users during creation, or add conditional logic to handle SUPER_ADMIN users differently—for example, allowing them to reorder products across any tenant or requiringtenantIdas a request parameter.nextjs_space/app/api/tenant-admin/orders/route.ts (1)
396-408: Fix relation names in PATCH handler to match Prisma schema.The PATCH handler uses incorrect relation names:
itemsanduserinstead of the schema-definedorder_itemsandusers. This will cause a runtime error when updating orders. The GET handler already uses the correct names.Update lines 401-402 to:
order_items: true, users: {nextjs_space/app/api/super-admin/tenants/[id]/route.ts (1)
223-246: Missing audit log for tenant deletion.The DELETE handler doesn't create an audit log entry, unlike PATCH which logs
TENANT_UPDATED. Deleting a tenant is a significant action that should be tracked for compliance and debugging purposes.Suggested fix
export async function DELETE( req: NextRequest, { params }: { params: { id: string } }, ) { try { const session = await getServerSession(authOptions); if (!session || session.user.role !== "SUPER_ADMIN") { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } + // Get tenant info before deletion for audit log + const tenant = await prisma.tenants.findUnique({ + where: { id: params.id }, + select: { businessName: true, subdomain: true }, + }); + + if (!tenant) { + return NextResponse.json({ error: "Tenant not found" }, { status: 404 }); + } + // Delete tenant (cascade will handle related records) await prisma.tenants.delete({ where: { id: params.id }, }); + // Create audit log + await prisma.audit_logs.create({ + data: { + id: crypto.randomUUID(), + action: "TENANT_DELETED", + entityType: "Tenant", + entityId: params.id, + userId: session.user.id, + userEmail: session.user.email, + metadata: { + businessName: tenant.businessName, + subdomain: tenant.subdomain, + }, + }, + }); + return NextResponse.json({ success: true }); } catch (error) {nextjs_space/app/api/super-admin/templates/upload/route.ts (1)
106-121: Error message shows wrong HTTP status code when master branch download fails.When both
mainandmasterbranch downloads fail, the error message incorrectly referencesresponse.status(from the main request) instead ofmasterResponse.status(from the master request that actually failed).Suggested fix
const masterResponse = await fetch(masterUrl); if (!masterResponse.ok) { throw new Error( - `Failed to download repository. Status: ${response.status}`, + `Failed to download repository. Tried main (${response.status}) and master (${masterResponse.status})`, ); }
🤖 Fix all issues with AI agents
In @nextjs_space/app/api/consultation/submit/route.ts:
- Line 197: The shipping address object currently sets address2: body.address2
which is the wrong field name and causes address line 2 to be blank; update the
assignment to use body.addressLine2 (matching the questionnaire creation and
form) wherever the shippingAddress is built in the submit route (e.g., in the
route handler that constructs the shippingAddress object) so the API sends the
correct addressLine2 value to Dr. Green.
In @nextjs_space/app/api/reset-password/route.ts:
- Line 44: The code currently hardcodes name: "Gerard Kavanagh" for new users;
replace this by deriving the name from the incoming request (e.g., read a name
field from the request body) or compute a fallback from the email local-part,
and update the object that contains name: "Gerard Kavanagh" so it uses the
request-provided name (or a sanitized email-derived string) instead; locate the
literal "Gerard Kavanagh" in route.ts and modify the user-creation payload there
to use the request body param (e.g., req.body.name / parsed JSON) or a helper
that extracts the part before '@' from the email.
- Around line 8-88: This endpoint's POST handler allows unauthenticated password
resets and account creation (see function POST) and contains hardcoded values
("Gerard Kavanagh", role "PATIENT", tenant subdomain "healingbuds"), enabling
account takeover and user enumeration; remove the entire route file immediately
(delete the POST handler / file) or disable its export so it is not mounted, and
ensure no references remain; if temporary access is required, replace with
proper guards by delegating to the existing secure flows (the token-based reset
endpoints) rather than performing direct create/update operations.
In @nextjs_space/app/api/super-admin/platform-settings/route.ts:
- Around line 109-131: The GET handler lacks the super-admin auth guard; mirror
the POST auth check by calling getServerSession(req) at the top of GET, validate
the session and that session.user.role === SUPER_ADMIN (use the same SUPER_ADMIN
constant and options used in the POST), and return an appropriate NextResponse
(401/403 JSON) when the check fails before querying prisma.platform_settings;
refer to the existing POST function in this file for the exact session-check
logic and error responses and reuse the same behavior.
In @nextjs_space/app/api/super-admin/tenants/route.ts:
- Around line 172-207: Inside the prisma.$transaction callback, replace the
incorrect model references tx.tenant, tx.user, and tx.tenantBranding with the
schema-correct model names tx.tenants, tx.users, and tx.tenant_branding
respectively (so newTenant creation, admin user creation, and default branding
use those models), and change the tx parameter type from any to
Prisma.TransactionClient to restore type safety and proper validation.
In @nextjs_space/app/api/tenant-admin/posts/[id]/route.ts:
- Around line 109-121: The loop that regenerates uniqueSlug calls
prisma.posts.findFirst with tenantId set to user!.tenant!.id which will throw
because the user relation is tenants (plural) and the user row already has
tenantId; change the tenant lookup to use user!.tenantId instead (update the
prisma.posts.findFirst call inside the slug uniqueness loop and the assignment
to dataToUpdate.slug), ensuring you reference the same user variable used
earlier and keep the rest of the uniqueness logic intact.
In @nextjs_space/app/api/tenant-admin/products/bulk/route.ts:
- Around line 104-116: The current bulk handler overwrites real inventory counts
by setting the Int `stock` to 1/0 via the `newStock` variable and
`prisma.products.updateMany`, which destroys actual quantities; change the
update to toggle a boolean availability flag instead (e.g., set `isAvailable`
based on `action === "set-in-stock"`) and leave `stock` untouched, or if you
must preserve/restore counts first call `prisma.products.findMany` for the
target IDs to capture current `stock` values and persist them to an audit/backup
table/field before running the update; update references to `newStock`, the
`prisma.products.updateMany` call, and the `auditAction` assignment accordingly
so you no longer overwrite `stock`.
In @nextjs_space/app/api/tenant-admin/webhooks/[id]/deliveries/route.ts:
- Around line 11-14: The GET handler currently assumes params is synchronous;
update the signature to accept params as a Promise and await it before use:
change the parameter type to { params: Promise<{ id: string }> } (i.e., export
async function GET(req: NextRequest, { params }: { params: Promise<{ id: string
}> }) ) and then do const { id } = await params (or await params wherever params
is accessed) so the dynamic route works on Next.js 15 where params is a Promise.
In @nextjs_space/app/api/tenant-admin/webhooks/[id]/route.ts:
- Around line 12-14: The PATCH and DELETE handlers (export async function PATCH
and export async function DELETE) access params synchronously but must support
Next.js 15 where params is a Promise; change the handlers to await params (e.g.,
const { id } = await params) before using it and update the params typing to
Promise<{ id: string }> so both PATCH and DELETE correctly await and extract id.
In @nextjs_space/app/api/webhooks/drgreen/crypto/route.ts:
- Around line 154-167: The code attempts to call request.json() again inside the
error-logging block (used by prisma.drGreenWebhookLog.create) but the request
body stream was already consumed earlier, so capture and reuse the parsed body
instead: parse the body once into a variable (e.g., parsedBody) in the outer
scope where the request is first read, then reference that variable in the
catch/error-logging path for prisma.drGreenWebhookLog.create (and provide a safe
fallback value like "unavailable" or null if parsing failed) instead of calling
request.json() a second time.
In @nextjs_space/app/api/webhooks/drgreen/fiat/route.ts:
- Around line 96-127: The webhook handler uses order.user.email and
parseFloat(amount || "0") unsafely; update the triggerWebhook calls (the blocks
invoking triggerWebhook for WEBHOOK_EVENTS.ORDER_CONFIRMED and ORDER_CANCELLED)
to read the customer email via a null-safe expression (e.g., get a local
customerEmail = order.user?.email ?? "" or a clearly named fallback) and pass
that instead of order.user.email, and validate/normalize amount before sending
(e.g., parseFloat(amount) with an isNaN check or Number() and fallback to 0) so
the data object always contains a safe string for customerEmail and a finite
numeric amount for the payment payload.
🟠 Major comments (13)
nextjs_space/app/api/shop/register/route.ts-37-42 (1)
37-42: Fail early when credentials are missing.The code logs an error when API credentials are missing but continues execution, deferring failure to
createClient. This makes debugging harder and returns a generic error message. Resolve the TODO-like comment and fail fast with a clear error.🐛 Proposed fix
if (!config.apiKey || !config.secretKey) { console.error("Missing Dr. Green API credentials for registration"); - // Continue anyway? The createClient might fail or we should block. - // createClient throws 'MISSING_CREDENTIALS` if keys are missing. - // We'll let it proceed and fail inside if needed, or handle it here. + return NextResponse.json( + { error: "Service configuration error. Please contact support." }, + { status: 500 }, + ); }nextjs_space/app/api/doctor-green/products/route.ts-12-12 (1)
12-12: Dead code: tenant country fallback logic is unreachable.Line 12 ensures
countryis always truthy (defaults to"PT"), which means the fallback logic on line 36 (tenant.countryCode || "SA") will never execute. The tenant-specific country configuration is effectively ignored.If the intent is to respect tenant country settings when no query parameter is provided, remove the default from line 12:
- const country = searchParams.get("country") || "PT"; + const country = searchParams.get("country");Then line 36 will correctly fall back through: query param → tenant config → "SA" default.
Also applies to: 36-36
nextjs_space/app/api/tenant-admin/email-mappings/route.ts-185-195 (1)
185-195: Unexpected template deletion - potential data loss.Deleting a mapping also deletes the associated template (lines 190-194). This is unexpected behavior for an endpoint managing "mappings" and could cause data loss if:
- The template is used by other mappings
- The user intends to reassign the template later
- The template contains significant customization work
Additionally, these operations lack transaction safety - if the template deletion fails after the mapping is deleted, data is left inconsistent.
🔧 Suggested fix - remove template deletion or make it explicit
Option 1: Remove automatic template deletion (recommended):
if (mapping) { // Delete Mapping await prisma.email_event_mappings.delete({ where: { id: mapping.id } }); - - // Delete Template ONLY IF it belongs to tenant (Safety check) - if (mapping.template && mapping.template.tenantId === tenantId) { - await prisma.email_templates.delete({ - where: { id: mapping.template.id }, - }); - } }Option 2: If template deletion is intentional, use a transaction and make it opt-in via query param:
+ const deleteTemplate = searchParams.get("deleteTemplate") === "true"; + - if (mapping) { - await prisma.email_event_mappings.delete({ where: { id: mapping.id } }); - if (mapping.template && mapping.template.tenantId === tenantId) { - await prisma.email_templates.delete({ - where: { id: mapping.template.id }, - }); - } - } + if (mapping) { + await prisma.$transaction(async (tx) => { + await tx.email_event_mappings.delete({ where: { id: mapping.id } }); + if (deleteTemplate && mapping.template?.tenantId === tenantId) { + await tx.email_templates.delete({ where: { id: mapping.template.id } }); + } + }); + }nextjs_space/app/api/tenant-admin/upload/route.ts-17-22 (1)
17-22: Add file type and size validation for upload security.The route lacks validation for file type and size, which poses security risks:
- Unrestricted file types could allow upload of malicious content (HTML, SVG with scripts)
- No size limit enables potential DoS through large uploads
Additionally, the filename is not sanitized before upload, unlike the similar
branding/upload/route.tsroute which sanitizes withfile.name.replace(/[^a-zA-Z0-9.-]/g, "_").🔒 Proposed fix with validation
const formData = await req.formData(); const file = formData.get("file") as File; if (!file) { return NextResponse.json({ error: "No file provided" }, { status: 400 }); } + + // Validate file type + const allowedTypes = ["image/jpeg", "image/png", "image/gif", "image/webp"]; + if (!allowedTypes.includes(file.type)) { + return NextResponse.json( + { error: "Invalid file type. Only images are allowed." }, + { status: 400 } + ); + } + + // Validate file size (e.g., 5MB limit) + const maxSize = 5 * 1024 * 1024; + if (file.size > maxSize) { + return NextResponse.json( + { error: "File too large. Maximum size is 5MB." }, + { status: 400 } + ); + }nextjs_space/app/api/tenant-admin/upload/route.ts-62-70 (1)
62-70: Return only the key from upload endpoint, or use signed URLs if returning URLs.The bucket is private (confirmed by
getFileUrlgenerating signed URLs with 1-hour expiration), but the code returns an unsigned public S3 URL that will fail on access. This causes:
- Broken images when the returned URL is used directly
- Inconsistency with
branding/upload/route.ts, which returns only the keyChoose one approach:
- Return only the
keyand let consumers callgetFileUrl()for signed URLs (matchesbranding/upload/route.ts)- Return a signed URL by calling
getFileUrl(key)before respondingCurrently the "Optimistic public URL" comment is misleading for a private bucket.
nextjs_space/app/api/customer/change-password/route.ts-46-57 (1)
46-57: Handle users without a password (OAuth-only accounts).If a user registered via OAuth/social login,
user.passwordmay benull. Callingbcrypt.compare(oldPassword, null)will throw an error or behave unexpectedly. Add a null check before comparison.🔧 Proposed fix
if (!user) { return NextResponse.json({ error: "User not found" }, { status: 404 }); } + // Check if user has a password (OAuth-only users may not) + if (!user.password) { + return NextResponse.json( + { error: "Password change not available. You may have signed up using a social login." }, + { status: 400 }, + ); + } + // Verify old password const isValidPassword = await bcrypt.compare(oldPassword, user.password);nextjs_space/app/api/super-admin/tenants/[id]/reset-password/route.ts-60-84 (1)
60-84: Misleading success response: email is not actually sent.The response at line 82 claims "Password reset email sent successfully", but the actual email sending is commented out (lines 60-62). This will mislead both the caller and the admin user who won't receive the reset link.
Either implement the email sending or update the response message to accurately reflect what happened.
Suggested fix
- // TODO: Send email with reset link - // const resetLink = `${process.env.NEXTAUTH_URL}/auth/reset-password/${resetToken}`; - // await sendPasswordResetEmail(adminUser.email, resetLink); + // Send email with reset link + const resetLink = `${process.env.NEXT_PUBLIC_APP_URL || "http://localhost:3000"}/auth/reset-password/${resetToken}`; + const html = await emailTemplates.passwordReset( + adminUser.name || "User", + resetLink, + "BudStack", + ); + await sendEmail({ + to: adminUser.email, + subject: "Password Reset Request", + html, + tenantId: params.id, + templateName: "passwordReset", + }); // Create audit log ... return NextResponse.json({ - message: "Password reset email sent successfully", + message: "Password reset initiated successfully", email: adminUser.email, });Based on the relevant code snippet from
nextjs_space/app/api/auth/reset-password/route.ts(lines 42-53), the codebase already has an implementation pattern for sending password reset emails usingemailTemplates.passwordResetandsendEmail.nextjs_space/app/api/webhooks/drgreen/fiat/route.ts-10-29 (1)
10-29: Add PayIn webhook signature verification.This endpoint accepts payment notifications without verifying authenticity. PayIn webhooks include an
X-PayIn-Signatureheader (format:t=<timestamp>,v1=<hex_signature>) that must be validated against the webhook secret using HMAC-SHA256 to prevent spoofed payloads.Implement signature verification by:
- Reading the raw request body as Buffer (before JSON parsing)
- Extracting the timestamp and signature from the header
- Computing HMAC-SHA256 over
"<timestamp>.<raw_body>"using your PayIn webhook secret- Comparing with the v1 value using constant-time comparison
Also consider rejecting requests with timestamps older than 5 minutes to mitigate replay attacks.
nextjs_space/app/api/signup/route.ts-30-35 (1)
30-35: Terms acceptance should be persisted for compliance.The
acceptTermsfield is validated but not stored in the database. For GDPR/CCPA compliance, you should typically record when and that the user accepted the terms. Consider adding fields liketermsAcceptedAtto the user record.🔧 Suggested fix
// Create user const user = await prisma.users.create({ data: { email, password: hashedPassword, name: `${firstName} ${lastName}`, role: "PATIENT", tenantId: tenant.id, + termsAcceptedAt: new Date(), }, });Note: This requires adding a
termsAcceptedAt DateTime?field to your Prisma schema.nextjs_space/app/api/tenant-admin/tenant/route.ts-14-18 (1)
14-18: Missing role authorization check.This endpoint only verifies authentication (
session?.user?.id) but doesn't verify the user hasTENANT_ADMINorSUPER_ADMINrole. All other routes under/api/tenant-admin/consistently check for these roles—includingcustomers/route.ts,settings/route.ts,branding/route.ts,orders/route.ts, andemail-templates/route.ts.This endpoint's docstring states it's "for the authenticated tenant admin," yet any authenticated user (including patients) could access it. Add an explicit role check to match the authorization pattern used throughout the rest of the tenant-admin endpoints.
Suggested fix: Add role check
if (!session?.user?.id) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } + + if (!["TENANT_ADMIN", "SUPER_ADMIN"].includes(session.user.role || "")) { + return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + }nextjs_space/app/api/super-admin/email-templates/[id]/route.ts-100-107 (1)
100-107: Incomplete handling for system template deletion.The code fetches the template to check
isSystembut then does nothing with this information (lines 100-103 are a no-op). System templates may be referenced by email_event_mappings, and deleting them could break email functionality.Either prevent deletion of system templates or ensure referential integrity by checking for mappings first.
🛡️ Suggested: Prevent system template deletion
if (template?.isSystem) { - // Option: prevent delete, or allow but with warning. - // For now, let's allow it but maybe frontend warns. + return NextResponse.json( + { error: "Cannot delete system templates" }, + { status: 403 }, + ); }nextjs_space/app/api/tenant-admin/email-templates/route.ts-88-102 (1)
88-102: Missing access validation when copying from source template.When a
sourceTemplateIdis provided, the code fetches the template without validating that the requesting tenant has access to it. This could allow a tenant to copy content from another tenant's private template by knowing/guessing the ID.The source template should be validated to ensure it's either owned by the current tenant or is a system template.
🔒 Proposed fix: Validate source template access
if (sourceTemplateId) { const source = await prisma.email_templates.findUnique({ where: { id: sourceTemplateId }, }); - if (source) { + // Validate access: must be system template or owned by this tenant + if (source && (source.isSystem || source.tenantId === user.tenants.id)) { data = { ...data, name: name || `${source.name} (Copy)`, subject: source.subject, contentHtml: source.contentHtml, category: source.category, description: source.description || `Copy of ${source.name}`, }; + } else if (source) { + return NextResponse.json( + { error: "Access denied to source template" }, + { status: 403 }, + ); } }nextjs_space/app/api/webhooks/drgreen/crypto/route.ts-14-18 (1)
14-18: Implement authentication for CoinRemitter webhook endpoint.This endpoint accepts and processes payments without any authentication. CoinRemitter's public documentation does not provide built-in webhook signature verification, but the endpoint should still implement protection. An attacker who discovers the URL could send forged payloads to mark orders as paid (lines 103-111 mark orders as "CONFIRMED" on PAID status).
Implement one or more of the following:
- Require a secret token (configured in CoinRemitter settings or as a query parameter) and validate it server-side
- Verify payment status by calling CoinRemitter's API using your API key before updating the order
- Use IP whitelisting if CoinRemitter publishes their webhook server IPs
- Add HTTPS + authentication headers as defense-in-depth
🟡 Minor comments (17)
nextjs_space/app/api/tenant-admin/seo/pages/route.ts-58-59 (1)
58-59: Add error handling for JSON parsing.Same issue as the products route -
request.json()can throw on malformed JSON.Proposed fix
- const body = await request.json(); - const { pageKey, seo } = body; + let body; + try { + body = await request.json(); + } catch { + return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); + } + const { pageKey, seo } = body;nextjs_space/app/api/tenant-admin/seo/products/[id]/route.ts-77-78 (1)
77-78: Add error handling for JSON parsing.
request.json()can throw aSyntaxErrorif the request body contains malformed JSON. Without a try/catch, this results in an unhandled exception returning a 500 error instead of a user-friendly 400 response.Proposed fix
- const body = await request.json(); - const { title, description, ogImage } = body; + let body; + try { + body = await request.json(); + } catch { + return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); + } + const { title, description, ogImage } = body;nextjs_space/app/api/consultation/submit/route.ts-307-315 (1)
307-315: External API call lacks timeout configuration.The
fetchcall to Dr. Green API has no timeout. If the external service is slow or unresponsive, this request will hang indefinitely, potentially causing resource exhaustion.💡 Add timeout with AbortController
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout + const response = await fetch(`${drGreenApiUrl}/dapp/clients`, { method: "POST", headers: { "Content-Type": "application/json", "x-auth-apikey": apiKey, "x-auth-signature": signature, }, body: payloadStr, + signal: controller.signal, }); + + clearTimeout(timeoutId);nextjs_space/app/api/tenant-admin/email-mappings/route.ts-104-111 (1)
104-111: ValidateeventTypeagainst allowed values.The
eventTypefrom user input is not validated againstSYSTEM_EVENTS. This allows creation of mappings for arbitrary event types, which could lead to data inconsistency.🔒 Suggested fix
const { eventType, templateId } = await req.json(); - if (!eventType || !templateId) { + if (!eventType || !templateId) { + return NextResponse.json( + { error: "Missing required fields" }, + { status: 400 }, + ); + } + + if (!SYSTEM_EVENTS.includes(eventType)) { return NextResponse.json( - { error: "Missing required fields" }, + { error: "Invalid event type" }, { status: 400 }, ); }nextjs_space/app/api/customer/change-password/route.ts-68-81 (1)
68-81: AddtenantIdto audit log for consistency.Other audit log entries in the codebase (e.g., in
tenant-admin/customers/[id]/reset-password/route.ts) includetenantId. This audit log is missing it, which could affect filtering and querying audit trails.🔧 Proposed fix
First, update the user query to include
tenantId(around line 43):where: { id: session.user.id }, - select: { id: true, password: true, email: true }, + select: { id: true, password: true, email: true, tenantId: true }, });Then add it to the audit log:
await prisma.audit_logs.create({ data: { id: crypto.randomUUID(), action: "PASSWORD_CHANGED", entityType: "User", entityId: session.user.id, userId: session.user.id, userEmail: session.user.email, + tenantId: user.tenantId || undefined, metadata: { method: "self-service", }, }, });nextjs_space/app/api/doctor-green/sync-products.disabled/route.ts-38-41 (1)
38-41: Slug generation may cause collisions for products with identical names.The slug is derived solely from
dgProduct.name. If two products have the same name (or names that normalize to the same slug), this will cause conflicts on upsert or create operations ifslughas a unique constraint. Consider appending thedoctorGreenIdor a hash to ensure uniqueness.💡 Suggested fix
slug: dgProduct.name .toLowerCase() .replace(/\s+/g, "-") - .replace(/[^\w-]/g, ""), + .replace(/[^\w-]/g, "") + `-${dgProduct.id}`,nextjs_space/app/api/onboarding/route.ts-169-170 (1)
169-170: Remove duplicate comment.Line 169 and 170 contain the same comment.
Proposed fix
// Send tenant welcome email (don't wait for it) - // Send tenant welcome email (don't wait for it) const html = await emailTemplates.tenantWelcome(nextjs_space/app/api/super-admin/tenants/route.ts-29-30 (1)
29-30: Add validation for pagination parameters.
parseIntcan returnNaNfor invalid input, which would cause the Prisma query to fail or behave unexpectedly. Consider adding validation.Suggested fix
- const page = parseInt(searchParams.get("page") || "1"); - const limit = parseInt(searchParams.get("limit") || "10"); + const page = Math.max(1, parseInt(searchParams.get("page") || "1", 10) || 1); + const limit = Math.min(100, Math.max(1, parseInt(searchParams.get("limit") || "10", 10) || 10));nextjs_space/app/api/signup/route.ts-73-74 (1)
73-74: Remove duplicate comment.Line 73 and 74 contain the same comment
// Send welcome email (don't wait for it).🧹 Fix
// Send welcome email (don't wait for it) - // Send welcome email (don't wait for it) const html = await emailTemplates.welcome(nextjs_space/app/api/user/profile/route.ts-27-38 (1)
27-38: Address object excludesaddressLine2-only updates.The condition
addressLine1 || city || state || postalCode || countrydoesn't includeaddressLine2. If a user only providesaddressLine2, the address object won't be built, and the field won't be updated.If this is intentional (requiring at least one "primary" address field), consider documenting it. Otherwise:
Suggested fix
const address = - addressLine1 || city || state || postalCode || country + addressLine1 || addressLine2 || city || state || postalCode || country ? { addressLine1: addressLine1 || "", addressLine2: addressLine2 || "",nextjs_space/app/api/tenant-admin/settings/route.ts-59-66 (1)
59-66: Inconsistent error handling for encryption failures.SMTP password encryption failure is silently caught (lines 63-65) while
drGreenSecretKeyencryption failure throws (line 81). This inconsistency could lead to:
- SMTP settings saved without password, causing later authentication failures
- Different user experience for similar failures
Consider either throwing for both or handling both gracefully with user feedback.
Suggested fix for consistent handling
if (smtpPassword && smtpPassword.trim() !== "") { try { smtpSettings.password = encrypt(smtpPassword); } catch (e) { console.error("SMTP Password Encryption failed:", e); + return NextResponse.json( + { error: "Failed to encrypt SMTP password" }, + { status: 500 }, + ); } }nextjs_space/app/api/tenant-admin/customers/[id]/reset-password/route.ts-81-87 (1)
81-87: Email send failure is silent - user receives success response.If
sendEmailfails, the operation appears successful to the admin, but the customer never receives the reset email. The token is already saved to the database at this point.Consider either:
- Awaiting email and returning an appropriate error if it fails
- Implementing a retry mechanism
- At minimum, logging when email fails and including a warning in the response
Suggested improvement
- await sendEmail({ - to: customer.email, - subject: "Password Reset Request", - html, - tenantId: customer.tenantId || "SYSTEM", - templateName: "passwordReset", - }); + try { + await sendEmail({ + to: customer.email, + subject: "Password Reset Request", + html, + tenantId: customer.tenantId || "SYSTEM", + templateName: "passwordReset", + }); + } catch (emailError) { + console.error("Failed to send password reset email:", emailError); + // Consider: return error or include warning in response + }nextjs_space/app/api/super-admin/email-mappings/route.ts-56-78 (1)
56-78: Potential race condition in upsert logic and missing templateId validation.
Race condition: Two concurrent requests with the same
eventTypecould both passfindFirst(returning null) and then both attemptcreate, causing a constraint violation if the schema enforces uniqueness. Consider wrapping in a transaction with a retry or usingupsertwith a compound unique constraint if supported.Missing templateId validation: The
templateIdis not verified to exist before creating/updating the mapping. If the template doesn't exist, this will fail with a foreign key error at the database level, which may result in a less informative 500 error.💡 Suggested improvement: Validate templateId exists
+ // Verify template exists + const templateExists = await prisma.email_templates.findUnique({ + where: { id: templateId }, + select: { id: true }, + }); + + if (!templateExists) { + return NextResponse.json( + { error: "Template not found" }, + { status: 404 }, + ); + } + // Safer approach for nullable unique: const existing = await prisma.email_event_mappings.findFirst({nextjs_space/app/api/super-admin/email-templates/[id]/route.ts-60-72 (1)
60-72: Missing template existence check before update.If the template with the given ID doesn't exist,
prisma.email_templates.updatewill throw aRecordNotFoundError, resulting in a generic 500 response instead of a clear 404. Consider checking existence first or handling the specific Prisma error.💡 Suggested improvement
try { const body = await req.json(); const { name, subject, contentHtml, description, category, isSystem, isActive } = body; + const existing = await prisma.email_templates.findUnique({ + where: { id: params.id }, + select: { id: true }, + }); + + if (!existing) { + return NextResponse.json({ error: "Template not found" }, { status: 404 }); + } + const updated = await prisma.email_templates.update({nextjs_space/app/api/webhooks/drgreen/crypto/route.ts-136-136 (1)
136-136: Potential NaN in webhook payload.
parseFloat(usd_amount || "0")will returnNaNifusd_amountis a non-numeric string. Consider adding validation or using a fallback.Suggested fix
- amount: parseFloat(usd_amount || "0"), + amount: parseFloat(usd_amount) || 0,nextjs_space/app/api/super-admin/templates/upload/route.ts-313-321 (1)
313-321: Error details may leak sensitive information.Including
error.messagedirectly in the API response could expose internal paths, database errors, or other sensitive implementation details to clients. Consider logging the full error server-side but returning a generic message to clients.Suggested approach
} catch (error: any) { console.error("[Template Upload] Error:", error); + // Determine if error message is safe to expose + const safeErrors = [ + "Template name is required", + "GitHub URL is required", + "Invalid GitHub URL format", + "template.config.json not found", + "Template with slug", + ]; + const isSafeError = safeErrors.some(msg => error.message?.startsWith(msg)); return NextResponse.json( { error: "Failed to upload template", - details: error.message || "Unknown error", + details: isSafeError ? error.message : "An unexpected error occurred", }, { status: 500 }, ); }Alternatively, use custom error classes to distinguish user-facing errors from internal errors.
nextjs_space/app/api/super-admin/templates/upload/route.ts-80-91 (1)
80-91: GitHub repository names can contain dots, which the regex currently rejects.GitHub repositories allow periods (
.) in their names (e.g.,user/repo.name). The current pattern[\w-]+only matches alphanumeric, underscore, and hyphen characters, causing valid repository names with dots to fail validation.Suggested fix to support dots in repository names
- const githubPattern = /^https:\/\/github\.com\/([\w-]+)\/([\w-]+)(\.git)?$/; + const githubPattern = /^https:\/\/github\.com\/([\w-]+)\/([\w.-]+)(\.git)?$/;
| export async function GET( | ||
| request: NextRequest, | ||
| { params }: { params: { slug: string } } | ||
| request: NextRequest, | ||
| { params }: { params: { slug: string } }, | ||
| ) { | ||
| try { | ||
| const { slug } = params; | ||
| try { | ||
| const { slug } = params; | ||
|
|
||
| if (!slug) { | ||
| return NextResponse.json( | ||
| { error: 'Slug is required' }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Find tenant by subdomain | ||
| const tenant = await prisma.tenants.findUnique({ | ||
| where: { | ||
| subdomain: slug | ||
| }, | ||
| include: { | ||
| template: true, | ||
| tenant_branding: true | ||
| } | ||
| }); | ||
| if (!slug) { | ||
| return NextResponse.json({ error: "Slug is required" }, { status: 400 }); | ||
| } | ||
|
|
||
| if (!tenant) { | ||
| return NextResponse.json( | ||
| { error: 'Tenant not found' }, | ||
| { status: 404 } | ||
| ); | ||
| } | ||
| // Find tenant by subdomain | ||
| const tenant = await prisma.tenants.findUnique({ | ||
| where: { | ||
| subdomain: slug, | ||
| }, | ||
| include: { | ||
| template: true, | ||
| tenant_branding: true, | ||
| }, | ||
| }); | ||
|
|
||
| return NextResponse.json({ | ||
| tenant: { | ||
| id: tenant.id, | ||
| businessName: tenant.businessName, | ||
| subdomain: tenant.subdomain, | ||
| settings: tenant.settings, | ||
| template: tenant.template, | ||
| branding: tenant.tenant_branding | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| console.error('Error fetching tenant:', error); | ||
| return NextResponse.json( | ||
| { error: 'Internal server error' }, | ||
| { status: 500 } | ||
| ); | ||
| if (!tenant) { | ||
| return NextResponse.json({ error: "Tenant not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| return NextResponse.json({ | ||
| tenant: { | ||
| id: tenant.id, | ||
| businessName: tenant.businessName, | ||
| subdomain: tenant.subdomain, | ||
| settings: tenant.settings, | ||
| template: tenant.template, | ||
| branding: tenant.tenant_branding, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Error fetching tenant:", error); | ||
| return NextResponse.json( | ||
| { error: "Internal server error" }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the Prisma schema
find . -name "*.prisma" -o -name "schema.prisma" | head -5Repository: AutomatosAI/budstack-saas
Length of output: 104
🏁 Script executed:
# Search for tenant model definition in schema files
rg "model\s+tenants" --type prisma -A 30Repository: AutomatosAI/budstack-saas
Length of output: 99
🏁 Script executed:
# Check if there are other tenant-related routes and their authentication patterns
find . -path "**/tenant/**" -name "*.ts" -o -path "**/tenant/**" -name "*.js" | grep -E "(route\.|api)" | head -20Repository: AutomatosAI/budstack-saas
Length of output: 320
🏁 Script executed:
# Check for authentication usage in tenant routes
rg -n "getServerSession|getSession|auth|middleware" --type ts -g '**/tenant/**'Repository: AutomatosAI/budstack-saas
Length of output: 608
🏁 Script executed:
# Look for patterns in settings field usage across the codebase
rg -n "\.settings\b" --type ts -C 2 | head -100Repository: AutomatosAI/budstack-saas
Length of output: 7578
🏁 Script executed:
cat -n ./nextjs_space/prisma/schema.prisma | head -200Repository: AutomatosAI/budstack-saas
Length of output: 7614
🏁 Script executed:
# Check other tenant routes for authentication
rg -n "getServerSession|getSession|auth" --type ts ./nextjs_space/app/api/tenant/current/route.ts ./nextjs_space/app/api/tenant/conditions/route.ts -A 5Repository: AutomatosAI/budstack-saas
Length of output: 51
🏁 Script executed:
# Look at the full tenant/current route to understand its purpose
cat -n ./nextjs_space/app/api/tenant/current/route.tsRepository: AutomatosAI/budstack-saas
Length of output: 711
🏁 Script executed:
# Find and show the tenants model in the Prisma schema
rg "^model tenants" --type ts -A 40 ./nextjs_space/prisma/schema.prismaRepository: AutomatosAI/budstack-saas
Length of output: 3236
🏁 Script executed:
# Check the getCurrentTenant implementation
find . -name "*.ts" -path "**/lib/**" | xargs grep -l "getCurrentTenant" | head -3Repository: AutomatosAI/budstack-saas
Length of output: 132
🏁 Script executed:
# Read the getCurrentTenant function
fd -e ts -path "**/lib/**" | xargs grep -l "export.*getCurrentTenant" | head -1 | xargs catRepository: AutomatosAI/budstack-saas
Length of output: 237
🏁 Script executed:
# Read the tenant.ts file to find getCurrentTenant
cat -n ./nextjs_space/lib/tenant.tsRepository: AutomatosAI/budstack-saas
Length of output: 5842
🏁 Script executed:
# Also check tenant-utils.ts
cat -n ./nextjs_space/lib/tenant-utils.tsRepository: AutomatosAI/budstack-saas
Length of output: 1325
Filter sensitive fields from the public tenant endpoint.
This route exposes the full settings field publicly, which contains SMTP credentials (host, user, password). These secrets should never be returned to unauthenticated clients. Either require authentication for this endpoint, or filter sensitive data before returning—only expose public tenant information like businessName, subdomain, template, and branding.
| // Trigger BudStack webhook | ||
| if (paymentStatus === "PAID") { | ||
| await triggerWebhook({ | ||
| event: WEBHOOK_EVENTS.ORDER_CONFIRMED, | ||
| tenantId: order.tenantId, | ||
| data: { | ||
| orderId: order.id, | ||
| orderNumber: order.orderNumber, | ||
| drGreenOrderId: order.drGreenOrderId, | ||
| paymentMethod: "FIAT", | ||
| amount: parseFloat(amount || "0"), | ||
| currency: currency || "USD", | ||
| invoiceId: payment_id, | ||
| customerEmail: order.user.email, | ||
| }, | ||
| }); | ||
|
|
||
| console.log("[Fiat Webhook] Order paid successfully:", order.id); | ||
| } else if (paymentStatus === "FAILED") { | ||
| await triggerWebhook({ | ||
| event: WEBHOOK_EVENTS.ORDER_CANCELLED, | ||
| tenantId: order.tenantId, | ||
| data: { | ||
| orderId: order.id, | ||
| orderNumber: order.orderNumber, | ||
| reason: "Payment failed", | ||
| customerEmail: order.user.email, | ||
| }, | ||
| }); | ||
|
|
||
| console.log("[Fiat Webhook] Payment failed for order:", order.id); | ||
| } |
There was a problem hiding this comment.
Potential null reference on order.user.email.
If an order exists without an associated user (e.g., guest checkout or data inconsistency), accessing order.user.email at lines 109 and 122 will throw a runtime error, causing the webhook to fail after the order has already been updated.
🐛 Add null safety
invoiceId: payment_id,
- customerEmail: order.user.email,
+ customerEmail: order.user?.email ?? null,
},
});Apply the same fix at line 122.
Additionally, parseFloat(amount || "0") at line 106 can return NaN for malformed strings—consider validating or defaulting:
- amount: parseFloat(amount || "0"),
+ amount: Number.isFinite(parseFloat(amount)) ? parseFloat(amount) : 0,🤖 Prompt for AI Agents
In @nextjs_space/app/api/webhooks/drgreen/fiat/route.ts around lines 96 - 127,
The webhook handler uses order.user.email and parseFloat(amount || "0")
unsafely; update the triggerWebhook calls (the blocks invoking triggerWebhook
for WEBHOOK_EVENTS.ORDER_CONFIRMED and ORDER_CANCELLED) to read the customer
email via a null-safe expression (e.g., get a local customerEmail =
order.user?.email ?? "" or a clearly named fallback) and pass that instead of
order.user.email, and validate/normalize amount before sending (e.g.,
parseFloat(amount) with an isNaN check or Number() and fallback to 0) so the
data object always contains a safe string for customerEmail and a finite numeric
amount for the payment payload.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.