Clear tenant context on errors; add tenant-scoping, Redis rate limiter, and secrets handling#2
Conversation
📝 WalkthroughWalkthroughIntroduces multi-tenant enforcement and context propagation (AsyncLocalStorage), converts Prisma reads to tenant-aware findFirst via middleware, moves rate limiting to an async Redis-backed implementation, adjusts encryption/key handling and masking, and updates various API routes and pages to use tenant- or id-based lookups and audit logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant TenantContext
participant RateLimiter as Redis
participant AppServer as API
participant PrismaDB as Database
Client->>Middleware: HTTP request (headers, path)
Middleware->>TenantContext: determine tenant (slug/subdomain/custom-domain) & set header
Middleware->>AppServer: forward request with sanitized x-tenant-* headers
AppServer->>RateLimiter: await checkRateLimit(session.user.id)
RateLimiter-->>AppServer: allow / reject (with Retry-After)
AppServer->>PrismaDB: tenant-scoped query (middleware applies tenantId, findFirst)
PrismaDB-->>AppServer: result (user/tenant/data)
AppServer-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
nextjs_space/app/tenant-admin/branding/page.tsx (1)
10-15: Guard check doesn't validate the field being used.The guard on line 10 checks for
session?.user?.email, but line 15 now queries bysession.user.id. Ifidis missing from the session (e.g., token didn't populate it), the query will fail or return no results.Update the guard to also check for
id:Proposed fix
- if (!session?.user?.email) { + if (!session?.user?.email || !session.user.id) { redirect('/auth/login'); }nextjs_space/prisma/schema.prisma (1)
437-458: Tenant-scoped email uniqueness is appropriate for multi-tenancy.The composite
@@unique([tenantId, email])correctly enables the same email across different tenants while preventing duplicates within a tenant.Production authentication in
lib/auth.tscorrectly implements tenant-aware email lookups. However,scripts/debug-auth.tshas a bug at lines 15-16: it performs a findUnique lookup by email without tenant context (where: { email }), which bypasses the unique constraint. While this is a debug script and won't affect production, it should be fixed to usefindFirstwith tenant filtering to match the production pattern.As noted for the migration file, ensure NULL
tenantIdusers (super-admins) are handled correctly at the application layer, since PostgreSQL won't enforce email uniqueness among them.nextjs_space/app/api/reset-password/route.ts (2)
6-6: Use shared Prisma client from@/lib/dbinstead of creating a new instance.Creating a new
PrismaClientper request:
- Bypasses the tenant-scoping middleware added in
lib/db.ts- Creates unnecessary database connections (connection pool exhaustion risk)
- Is inconsistent with other routes in this PR
🔧 Suggested fix
-import { PrismaClient } from '@prisma/client'; +import { prisma } from '@/lib/db'; ... -const prisma = new PrismaClient();Also remove the
prisma.$disconnect()in the finally block (line 83) as the shared client manages its own lifecycle.
24-58: Hardcoded test data and unexpected user creation in password reset endpoint.This password reset endpoint creates a new user with hardcoded values (
name: 'Gerard Kavanagh', tenant:'healingbuds') when the user doesn't exist. This is problematic:
- Password reset should not create users - return 404 instead
- Hardcoded name and tenant are clearly debug/test artifacts
- Security risk: allows account enumeration and unauthorized account creation
🔧 Suggested fix
if (!user) { - // Get tenant for healingbuds - const tenant = await prisma.tenants.findUnique({ - where: { subdomain: 'healingbuds' }, - }); - - if (!tenant) { - return NextResponse.json( - { error: 'Tenant not found' }, - { status: 404 } - ); - } - - // Create user - const hashedPassword = await bcrypt.hash(password, 10); - const newUser = await prisma.users.create({ - data: { - id: crypto.randomUUID(), - email: email.toLowerCase(), - password: hashedPassword, - name: 'Gerard Kavanagh', - role: 'PATIENT', - tenantId: tenant.id, - updatedAt: new Date(), - }, - }); - - return NextResponse.json({ - success: true, - message: 'User created', - user: { - email: newUser.email, - name: newUser.name, - }, - }); + return NextResponse.json( + { error: 'User not found' }, + { status: 404 } + ); }nextjs_space/lib/tenant.ts (1)
152-171: MissingsetTenantContext(null)in subdomain fallthrough path.If the subdomain check at line 152 passes but
findFirstreturnsnull(tenant not found/inactive), execution falls through to line 171 without clearing the tenant context. AddsetTenantContext(null)before the final return:🔧 Suggested fix
if (tenant) { setTenantContext(tenant.id); return tenant; } } } catch (error) { console.error('Error fetching tenant from request:', error); setTenantContext(null); return null; } + setTenantContext(null); return null; }nextjs_space/app/api/super-admin/platform-settings/route.ts (1)
12-18: Guard checksid— potential type mismatch.Line 12 validates
session?.user?.email, but line 18 queries bysession.user.id. While the auth setup populates both, this creates two issues:
- Type safety:
session.user.idis accessed viaas anycast in auth.ts, so TypeScript won't catch ifidis undefined.- Semantic mismatch: If
idis somehow missing,findUnique({ where: { id: undefined } })will throw a Prisma error.Consider updating the guard to check what you actually use:
- if (!session?.user?.email) { + if (!session?.user?.id) {Or check both if email is needed elsewhere in the flow.
nextjs_space/app/super-admin/settings/page.tsx (1)
9-16: MissingauthOptionsparameter may causesession.user.idto be undefined.
getServerSession()is called withoutauthOptions, but the custom session properties (id,role,tenantId) are only populated by the callbacks defined inauthOptions. Without passing it, the session may only contain the default next-auth properties (name,image).This would cause line 16 to query with
id: undefined, which will throw a Prisma validation error.- const session = await getServerSession(); + const session = await getServerSession(authOptions);Also requires importing
authOptions:import { authOptions } from '@/lib/auth';Additionally, the guard on line 11 checks
id— consider aligning the check with the usage.nextjs_space/app/super-admin/platform-settings/page.tsx (1)
9-16: MissingauthOptions—session.user.idwill be undefined.Same issue as
nextjs_space/app/super-admin/settings/page.tsx:getServerSession()is called withoutauthOptions, so the custom session properties (id,role,tenantId) won't be populated.+import { authOptions } from '@/lib/auth'; ... - const session = await getServerSession(); + const session = await getServerSession(authOptions);Without this fix,
session.user.idwill beundefined, andprisma.users.findUnique({ where: { id: undefined } })will fail.nextjs_space/app/api/signup/route.ts (1)
47-57: Tenant-scoping missing in email duplicate check.The query checks for existing users across all tenants, but the user is created with
tenantId: tenant.id. This prevents the same email from being used across different tenants, which contradicts the multi-tenant model implied by the composite unique constraint on(tenantId, email)in the schema.🐛 Proposed fix
// Check if user already exists const existingUser = await prisma.users.findFirst({ - where: { email }, + where: { + email, + tenantId: tenant.id, + }, });nextjs_space/app/api/shop/register/route.ts (1)
77-83: Add validation forsession.user.idbefore database update.The authorization check at line 13 validates
session?.user?.email, but the update at line 79 usessession.user.idwithout validation. While the auth flow should populate both together, a defensive check prevents potential Prisma errors if the token is corrupted or incomplete.♻️ Suggested defensive check
- if (!session?.user?.email) { + if (!session?.user?.email || !session?.user?.id) { return NextResponse.json( { error: 'Unauthorized' }, { status: 401 } ); }nextjs_space/app/api/auth/reset-password/route.ts (1)
18-28: Inconsistent tenant handling in password reset lookup.The email lookup uses
findFirstwithout tenant context, unlike the login flow which filters by tenant. Since the schema allows duplicate emails across tenants (@@unique([tenantId, email])), this may reset the password for the wrong user if the same email exists in multiple tenants. The reset token is sent without explicit tenant confirmation, creating potential confusion. Align with the login pattern by:
- Extracting tenant context (already available via headers, see
lib/tenant.ts)- Filtering email lookup by both email and tenantId
- Or clarify if platform-wide password resets are intentional
nextjs_space/app/api/user/profile/route.ts (1)
10-15: Auth check is inconsistent with the lookup field.The authorization check validates
session?.user?.emailbut the update usessession.user.id. Ifidis missing from the session butidinstead.🐛 Suggested fix
- if (!session?.user?.email) { + if (!session?.user?.id) { return NextResponse.json( { error: 'Unauthorized' }, { status: 401 } ); }nextjs_space/app/api/tenant-admin/select-template/route.ts (1)
12-17: Auth check is inconsistent with the lookup field.Same pattern as the profile route: validates
session?.user?.emailbut then queries bysession.user.idat line 20. If the session has email but not id, the query will fail.🐛 Suggested fix
- if (!session?.user?.email) { + if (!session?.user?.id) { return NextResponse.json( { error: 'Unauthorized' }, { status: 401 } ); }nextjs_space/app/api/super-admin/tenants/route.ts (2)
162-171: Clarify email uniqueness intent and add ordering or filtering.The users table has a composite unique constraint on
(tenantId, email), meaning the same email can exist across different tenants. The currentfindFirstlookup on email alone is non-deterministic and unclear in intent:
- If checking tenant-scoped uniqueness: filter by the current tenant's ID
- If checking global uniqueness: this contradicts the schema design
- Either way: add an
orderByclause (e.g.,orderBy: { createdAt: 'asc' }) to ensure deterministic results in case of edge cases with NULL tenantIds
178-211: Critical: Transaction uses incorrect model names that don't match the schema.The transaction code uses singular and camelCase model names (
tx.tenant,tx.user,tx.tenantBranding) but your Prisma schema defines them in plural form:tenants,users,tenant_branding. Update the transaction to use the correct model names:tx.tenants.create(...) tx.users.create(...) tx.tenant_branding.create(...)Prisma delegates (inside transactions) use the exact model names as defined in the schema, not variations. These incorrect names will cause runtime errors.
🤖 Fix all issues with AI agents
In @nextjs_space/lib/auth.ts:
- Around line 20-26: When calling prisma.users.findFirst in the authentication
path, add an orderBy clause for the branch where tenant?.id is falsy so matching
by email only returns deterministically (e.g., orderBy: { createdAt: 'asc' } or
{ tenantId: 'asc' }); update the query that uses getCurrentTenant() and
prisma.users.findFirst (the branch that uses credentials.email and include: {
tenants: true }) to include orderBy to prevent nondeterministic selection when
multiple users share the same email across tenants.
In @nextjs_space/lib/db.ts:
- Around line 131-133: The upsert update branch should not modify tenant
ownership; remove the assignment to params.args.update.tenantId (or at minimum
avoid setting it during the update path) and instead ensure tenantId is applied
only on creation and enforced via the query filter (use
params.args.create.tenantId = params.args.create.tenantId ?? tenantId and ensure
the where clause includes tenantId). Update the code references: stop changing
params.args.update.tenantId, set tenantId only on params.args.create, and verify
the upsert where/filter uses tenantId to prevent cross-tenant updates.
- Around line 112-114: The middleware unconditionally rewrites params.action
from 'findUnique' to 'findFirst' (params.action = 'findFirst') which is unsafe
for models without composite unique constraints; update the logic to only
rewrite for models that actually have a tenant-scoped unique constraint (i.e.,
check the model name against the tenantScopedModels list or a new allowlist of
models with @@unique including tenantId) or alternatively add composite @@unique
constraints including tenantId to the affected models (audit_logs,
consultations, drgreen_webhook_logs, email_logs, email_templates, webhooks), so
that only models guaranteed unique per tenant are converted to findFirst.
In @nextjs_space/lib/rate-limit.ts:
- Around line 59-70: The current rate-limit logic resets the TTL on every
request because pexpire(key, windowMs) is called unconditionally inside the
redis multi, turning fixed-window into sliding-window behavior; change the logic
so pexpire is only set when the key is first created (i.e., when the increment
result shows count === 1). Concretely, keep using getRedisClient(),
redis.multi() and .incr(key) but remove the unconditional .pexpire from the
multi and instead call .pexpire(key, windowMs) only when the computed count
(from results?.[0]?.[1] or the local count variable) equals 1; leave ttlMs/pttl
and retryAfter calculation as-is.
In @nextjs_space/prisma/migrations/init_complete_schema.sql:
- Line 407: The composite unique index users_tenantId_email_key does not prevent
duplicate emails when tenantId IS NULL; add a partial unique index on
users(email) WHERE tenantId IS NULL (e.g., users_email_global_key) to enforce
uniqueness for platform/global users; update the migration to create that
partial index and ensure any rollback/drop uses the same index name so DB-level
uniqueness is enforced for NULL tenantId rows.
🧹 Nitpick comments (5)
nextjs_space/app/super-admin/templates/page.tsx (1)
15-21: Guard check doesn't validate the field used in query.Line 15 checks for
session?.user?.email, but the query on line 20 usessession.user.id. Sinceidis a custom property added via the session callback (not part of NextAuth's default types), there's a type-safety gap—ifidis somehow missing, this query could fail or return unexpected results.Consider updating the guard to check for
id:- if (!session?.user?.email) { + if (!session?.user?.id) { redirect('/auth/login'); }nextjs_space/lib/db.ts (1)
64-77:deleteManyappears in both action sets - consider consolidating.
deleteManyis included in bothtenantScopedReadActions(line 71) andtenantScopedWriteManyActions(line 76). While functionally harmless (both apply where-clause filtering),deleteManyis semantically a write operation. Consider removing it fromtenantScopedReadActionsfor clarity.nextjs_space/app/api/onboarding/route.ts (1)
60-70: Consider normalizing email before lookup to ensure consistent duplicate detection.The switch to
findFirstis appropriate for the multi-tenant model. However, the email lookup doesn't normalize the input (e.g., lowercase), which could allow bypassing duplicate detection if emails are stored with different casing.♻️ Suggested fix
+ const normalizedEmail = email.toLowerCase().trim(); + // Check if email already exists const existingUser = await prisma.users.findFirst({ - where: { email }, + where: { email: normalizedEmail }, });Also ensure email is normalized when creating the user at line 141.
nextjs_space/app/api/shop/register/route.ts (1)
48-53: Consider early return when credentials are missing.The code logs an error but continues even with missing credentials, deferring failure to
createClient. An early return with a clear error message would provide better UX and avoid unnecessary downstream calls.♻️ Suggested improvement
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: 'Dr. Green API is not configured for this tenant' }, + { status: 503 } + ); }nextjs_space/lib/rate-limit.ts (1)
12-20: Redis client lacks connection error handling and lifecycle management.The lazy-initialized Redis client doesn't handle connection errors or provide a way to gracefully shut down. Consider adding error event handlers and an optional
closeRedisClientfunction for graceful shutdown.Additionally,
maxRetriesPerRequest: nullcauses infinite retries on failed commands, which could block requests indefinitely during Redis outages.♻️ Suggested improvement
const getRedisClient = () => { if (!redisClient) { redisClient = new Redis(REDIS_URL, { - maxRetriesPerRequest: null, + maxRetriesPerRequest: 3, lazyConnect: true, + retryStrategy: (times) => Math.min(times * 50, 2000), }); + redisClient.on('error', (err) => { + console.error('[RateLimit] Redis connection error:', err); + }); } return redisClient; }; + +export const closeRedisClient = async () => { + if (redisClient) { + await redisClient.quit(); + redisClient = null; + } +};
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
nextjs_space/app/api/auth/reset-password/route.tsnextjs_space/app/api/consultation/submit/route.tsnextjs_space/app/api/onboarding/route.tsnextjs_space/app/api/reset-password/route.tsnextjs_space/app/api/shop/register/route.tsnextjs_space/app/api/signup/route.tsnextjs_space/app/api/super-admin/platform-settings/route.tsnextjs_space/app/api/super-admin/templates/[id]/route.tsnextjs_space/app/api/super-admin/tenants/bulk/route.tsnextjs_space/app/api/super-admin/tenants/route.tsnextjs_space/app/api/tenant-admin/analytics/route.tsnextjs_space/app/api/tenant-admin/orders/bulk/route.tsnextjs_space/app/api/tenant-admin/products/bulk/route.tsnextjs_space/app/api/tenant-admin/products/reorder/route.tsnextjs_space/app/api/tenant-admin/select-template/route.tsnextjs_space/app/api/tenant-admin/settings/route.tsnextjs_space/app/api/user/profile/route.tsnextjs_space/app/super-admin/platform-settings/page.tsxnextjs_space/app/super-admin/settings/page.tsxnextjs_space/app/super-admin/templates/page.tsxnextjs_space/app/tenant-admin/branding/page.tsxnextjs_space/app/tenant-admin/settings/page.tsxnextjs_space/app/tenant-admin/settings/settings-form.tsxnextjs_space/lib/auth.tsnextjs_space/lib/db.tsnextjs_space/lib/encryption.tsnextjs_space/lib/rate-limit.tsnextjs_space/lib/tenant-config.tsnextjs_space/lib/tenant-context.tsnextjs_space/lib/tenant.tsnextjs_space/middleware.tsnextjs_space/next.config.jsnextjs_space/prisma/migrations/init_complete_schema.sqlnextjs_space/prisma/schema.prisma
🧰 Additional context used
🧬 Code graph analysis (25)
nextjs_space/app/api/tenant-admin/select-template/route.ts (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/super-admin/platform-settings/route.ts (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/super-admin/tenants/route.ts (3)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/app/api/super-admin/tenants/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/signup/route.ts (1)
nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/app/api/tenant-admin/orders/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/tenant-admin/analytics/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/tenant-admin/products/reorder/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/super-admin/templates/page.tsx (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/reset-password/route.ts (1)
nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/lib/auth.ts (2)
nextjs_space/lib/tenant.ts (1)
getCurrentTenant(14-59)nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/app/api/auth/reset-password/route.ts (1)
nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/app/api/tenant-admin/products/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-97)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/super-admin/templates/[id]/route.ts (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/tenant-admin/branding/page.tsx (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/lib/db.ts (1)
nextjs_space/lib/tenant-context.ts (1)
getTenantContext(13-15)
nextjs_space/lib/tenant.ts (2)
nextjs_space/lib/tenant-context.ts (1)
setTenantContext(9-11)nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/lib/tenant-config.ts (1)
nextjs_space/lib/encryption.ts (1)
decrypt(38-67)
nextjs_space/app/api/shop/register/route.ts (3)
nextjs_space/middleware.ts (1)
config(4-16)nextjs_space/lib/tenant-config.ts (1)
getTenantDrGreenConfig(10-38)nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/super-admin/platform-settings/page.tsx (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/app/api/tenant-admin/settings/route.ts (2)
nextjs_space/lib/encryption.ts (1)
encrypt(20-33)nextjs_space/lib/audit-log.ts (3)
getClientInfo(121-128)createAuditLog(42-62)AUDIT_ACTIONS(67-116)
nextjs_space/app/super-admin/settings/page.tsx (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
nextjs_space/lib/rate-limit.ts (1)
nextjs_space/middleware.ts (1)
config(4-16)
nextjs_space/app/api/consultation/submit/route.ts (1)
nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/app/api/user/profile/route.ts (1)
nextjs_space/lib/auth.ts (1)
session(60-67)
🔇 Additional comments (35)
nextjs_space/next.config.js (1)
17-17: LGTM!The strict equality check against
'true'is the correct pattern for boolean-like environment variables. Image optimization remains enabled by default when the env var is unset, which is sensible behavior.nextjs_space/app/tenant-admin/branding/page.tsx (1)
14-23: ID-based lookup is correct and properly typed.The switch from email-based to ID-based user lookup is correct and consistent with the broader tenant-context refactor. The next-auth types are already properly augmented in
nextjs_space/types/next-auth.d.tswithid: stringon both theUserandSessioninterfaces, sosession.user.idhas full TypeScript support.nextjs_space/lib/encryption.ts (1)
5-5: Good security hardening.Removing the
NEXT_PUBLIC_ENCRYPTION_KEYfallback is the right call—NEXT_PUBLIC_*environment variables are bundled into client-side JavaScript in Next.js, which would have exposed the encryption key to browsers.nextjs_space/prisma/migrations/init_complete_schema.sql (1)
416-416: LGTM for tenant-scoped order numbers.The composite unique constraint on
(tenantId, orderNumber)correctly ensures order numbers are unique within each tenant while allowing the same order number across different tenants.nextjs_space/prisma/schema.prisma (1)
232-248: LGTM – tenant-scoped order uniqueness.The composite unique constraint on
(tenantId, orderNumber)is the correct approach for multi-tenant order isolation. This ensures order numbers are unique per tenant.nextjs_space/lib/tenant-context.ts (1)
1-19: Well-implemented request-scoped tenant context.The use of
AsyncLocalStorageis appropriate for propagating tenant context across async boundaries in Next.js server-side code. The dual API (setTenantContextfor imperative use,runWithTenantContextfor callback-scoped use) covers common patterns.A brief JSDoc on each export would help maintainers understand the distinction (e.g.,
enterWithmodifies the current context vsruncreates an isolated scope), but this is optional.nextjs_space/lib/tenant-config.ts (1)
27-36: LGTM! Proper decryption validation for both credentials.The change to decrypt the API key (in addition to the secret key) and validate both decrypted values before returning is a solid security improvement. The falsy check correctly catches decrypt failures since
decrypt()returns an empty string on error.nextjs_space/lib/db.ts (1)
103-151: Well-structured tenant-scoping middleware.The middleware correctly:
- Guards registration with
'$use' in prismacheck- Falls through when no tenant context or model isn't scoped
- Handles both read filtering and write defaulting appropriately
- Supports null-tenant access for shared templates
nextjs_space/app/api/consultation/submit/route.ts (1)
78-83: LGTM! Tenant-aware user lookup with email normalization.The change from
findUniquetofindFirstwith optional tenant scoping is correct for multi-tenant scenarios. Email lowercasing ensures consistent lookups regardless of input case.nextjs_space/app/api/reset-password/route.ts (1)
20-22: ThefindFirstand id-based update changes are correct.Using
findFirstfor user lookup and updating byuser.idinstead of email aligns with the multi-tenant approach where email alone may not be unique across tenants.Also applies to: 63-66
nextjs_space/lib/tenant.ts (2)
21-22: LGTM! Consistent tenant context management on all exit paths.The pattern of calling
setTenantContext(null)on early exits and errors, andsetTenantContext(tenant?.id ?? null)on success ensures the context is always in a known state. This prevents stale tenant IDs from persisting across requests.Also applies to: 52-52, 56-57
131-145: Good addition of path-based tenant resolution.The
/store/{slug}path matching complements the existing subdomain logic and aligns withgetTenantUrl()which generates these URLs. Setting tenant context immediately upon match ensures downstream code has access to the tenant ID.nextjs_space/app/api/super-admin/templates/[id]/route.ts (1)
14-24: LGTM — correctly usesauthOptions.Unlike the page components, this API route correctly passes
authOptionstogetServerSession(), ensuringsession.user.idis populated.Minor note: The guard on line 16 checks
id. For consistency with the actual usage, consider updating the guard:- if (!session?.user?.email) { + if (!session?.user?.id) {This is a minor inconsistency rather than a bug since
authOptionsensures both are populated together.nextjs_space/middleware.ts (2)
22-26: Good security hardening: Sanitizing incoming tenant headers.Stripping
x-tenant-slug,x-tenant-subdomain, andx-tenant-custom-domainfrom incoming requests prevents clients from spoofing tenant context. This is a solid defense-in-depth measure for multi-tenant isolation.
40-93: LGTM!The consistent use of
NextResponse.next({ request: { headers: requestHeaders } })across all routing paths ensures tenant headers are properly propagated (or sanitized) downstream. The routing priority (path → subdomain → custom domain → platform) is logically sound.nextjs_space/app/api/auth/reset-password/route.ts (1)
46-57: LGTM!The email sending logic appropriately falls back to
'SYSTEM'for the tenantId when the user doesn't have one. The await with catch pattern ensures the response isn't blocked by email failures.nextjs_space/app/api/shop/register/route.ts (1)
34-46: Good defensive pattern for tenant credential resolution.The fallback to platform environment credentials when tenant-specific config fails is a sensible approach. The warning log helps with debugging without breaking the flow.
nextjs_space/lib/rate-limit.ts (1)
106-141: LGTM!The
getRateLimitStatusfunction correctly handles edge cases: returns safe defaults on Redis errors and properly handles TTL values. The fail-open strategy aligns withcheckRateLimit.nextjs_space/app/api/user/profile/route.ts (1)
33-42: LGTM!The switch from email-based to ID-based lookup is a correct improvement. Using the primary key (
id) for updates is more efficient and stable than using email, which can change.nextjs_space/app/api/super-admin/tenants/route.ts (2)
25-28: LGTM!The rate limit check is correctly awaited to match the new async signature.
125-128: LGTM!Correctly awaits the async
checkRateLimitcall.nextjs_space/app/api/tenant-admin/analytics/route.ts (1)
17-21: LGTM!The rate limit check is correctly awaited to match the new async Redis-backed implementation.
nextjs_space/app/api/tenant-admin/products/reorder/route.ts (1)
19-23: LGTM!The rate limit check is correctly awaited.
nextjs_space/app/api/tenant-admin/products/bulk/route.ts (1)
30-34: LGTM!The rate limit check is correctly awaited to match the new async signature.
nextjs_space/app/api/tenant-admin/select-template/route.ts (1)
19-22: LGTM!The switch from email-based to ID-based lookup is correct and aligns with the PR's broader pattern of using stable identifiers for user lookups.
nextjs_space/app/api/super-admin/tenants/bulk/route.ts (2)
30-34: LGTM! Rate limit check correctly awaited.The migration to the async Redis-backed rate limiter is properly implemented. The
awaitensures the Promise resolves before checkingsuccess.
76-96: Good audit logging implementation.The audit logs capture relevant context including previous/new status, bulk operation flag, and batch size. Using
crypto.randomUUID()for IDs is appropriate.nextjs_space/app/api/tenant-admin/orders/bulk/route.ts (2)
32-36: LGTM! Async rate limiting properly integrated.The
awaitis correctly added for the Redis-backed rate limiter, and the success check pattern remains consistent with other endpoints.
71-97: Good tenant isolation pattern.The code correctly enforces tenant boundaries by filtering orders on
tenantIdduring both fetch and update operations, preventing cross-tenant data access.nextjs_space/app/tenant-admin/settings/page.tsx (1)
22-28: LGTM! Consistent API key masking.Both
drGreenSecretKeyanddrGreenApiKeyare now properly masked before being passed to the client component, preventing sensitive credential exposure in the browser.nextjs_space/app/tenant-admin/settings/settings-form.tsx (2)
29-34: LGTM! Proper handling of masked API key in form state.The
hasApiKeyflag correctly detects whether a key exists (the masked'********'value is truthy), and initializingdrGreenApiKeyto empty string prevents accidentally submitting the mask placeholder back to the server.
156-160: Good UX for credential management.The conditional placeholder and helper text provide clear guidance—users see "Leave empty to keep existing key" when a key is already configured, avoiding confusion about whether to re-enter credentials.
nextjs_space/app/api/tenant-admin/settings/route.ts (3)
82-89: LGTM! Consistent encryption for API key.The
drGreenApiKeyencryption follows the same pattern asdrGreenSecretKey, only encrypting when a non-empty value is provided. This preserves existing keys when the form field is left empty.
103-117: Good audit logging implementation.The audit log captures relevant metadata including updated field names (not values), SMTP update status, and client information. Field names like
drGreenApiKeyappearing inupdatedFieldsis acceptable since only the key names are logged, not the actual values.
91-95: Good practice: sensitive keys masked in logs.The console output correctly masks
drGreenSecretKeyanddrGreenApiKeyvalues, preventing accidental credential exposure in server logs while still providing useful debugging information about which fields were updated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed24090292
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @nextjs_space/lib/db.ts:
- Around line 116-133: The upsert handling is incomplete: ensure the upsert's
lookup and update are tenant-scoped and avoid the early return that skips read
scoping; specifically, when params.action === 'upsert' set
params.args.where.tenantId = params.args.where?.tenantId ?? tenantId and also
set params.args.update.tenantId = params.args.update?.tenantId ?? tenantId (in
addition to the existing params.args.create handling), and move or modify the
return logic so the subsequent read-scoping logic that scopes params.args.where
for other actions still runs or is applied to the upsert case as well before
calling next(params).
- Around line 64-77: The middleware is missing tenant scoping for single-record
write actions: add 'update' and 'delete' to the tenant-scoped write set (or
create a new tenantScopedWriteSingleActions = new Set(['update','delete'])) and
remove the duplicate 'deleteMany' from tenantScopedReadActions; then update the
request handling in the Prisma middleware (the function that inspects
action/params) to inject or merge the tenant filter into params.where for single
writes (e.g., ensure params.where = { AND: [params.where || {}, { tenantId:
currentTenantId }] }) and validate/throw if a write would run without a tenant
constraint so single-record update/delete cannot bypass tenant scoping.
In @nextjs_space/lib/rate-limit.ts:
- Around line 12-20: The Redis client in getRedisClient currently sets
maxRetriesPerRequest: null and lazyConnect: true which can cause indefinite
hangs when Redis is down; update the Redis constructor options for redisClient
to use a bounded retry policy (e.g., set maxRetriesPerRequest to a finite number
or supply a retryStrategy), add a connection timeout (e.g., connectTimeout or
socket.connectTimeout), and disable offline queueing (enableOfflineQueue: false)
so commands fail fast; ensure getRedisClient (and any caller) handles connection
errors from lazyConnect() or explicitly connect with a timeout and surface the
error instead of letting requests hang.
- Around line 59-72: The INCR and PEXPIRE are separate calls causing a
non-atomic race where a crash after INCR can leave the key without a TTL; change
the pipeline in the rate limiter (where getRedisClient() is used and
multi().incr(key).exec() is called) to include PEXPIRE in the same MULTI/EXEC
and also request PTTL in that same transaction so you can parse the incremented
count and TTL from the single exec() result; remove the separate await
redis.pexpire(...) and await redis.pttl(...) calls and instead extract count and
ttlMs from the multi exec response, using the existing key and windowMs values.
🧹 Nitpick comments (1)
nextjs_space/lib/rate-limit.ts (1)
94-98: Consider returning rate limit headers on successful requests.Currently,
X-RateLimit-*headers are only returned when rate limited (429). Returning these headers on success would allow clients to monitor their usage proactively.Suggested enhancement
- return { success: true }; + return { + success: true as const, + headers: { + 'X-RateLimit-Limit': maxRequests.toString(), + 'X-RateLimit-Remaining': Math.max(0, maxRequests - count).toString(), + 'X-RateLimit-Reset': new Date(now + ttlMs).toISOString(), + } + };Note: This would require updating callers to propagate these headers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nextjs_space/lib/auth.tsnextjs_space/lib/db.tsnextjs_space/lib/rate-limit.ts
🧰 Additional context used
🧬 Code graph analysis (3)
nextjs_space/lib/auth.ts (2)
nextjs_space/lib/tenant.ts (1)
getCurrentTenant(14-59)nextjs_space/lib/db.ts (1)
prisma(31-37)
nextjs_space/lib/db.ts (1)
nextjs_space/lib/tenant-context.ts (1)
getTenantContext(13-15)
nextjs_space/lib/rate-limit.ts (1)
nextjs_space/middleware.ts (1)
config(4-16)
🔇 Additional comments (5)
nextjs_space/lib/auth.ts (1)
4-4: LGTM!Import added to support tenant-aware authentication flow.
nextjs_space/lib/db.ts (3)
1-2: LGTM!Import of
getTenantContextcorrectly added to support the new tenant-scoping middleware.
85-101: LGTM!The
applyTenantScopehelper correctly handles both strict tenant scoping and the null-access variant for shared templates. Overwriting any existingtenantIdin the spread is actually secure—it prevents callers from bypassing tenant isolation.
135-147: LGTM!The read and write-many action scoping logic correctly applies tenant constraints to the
whereclause, handling both existing and missing where conditions.nextjs_space/lib/rate-limit.ts (1)
108-143: LGTM!The implementation correctly handles concurrent Redis queries and provides sensible defaults on errors. The TTL fallback logic properly addresses edge cases where the key doesn't exist or has no expiry.
| const getRedisClient = () => { | ||
| if (!redisClient) { | ||
| redisClient = new Redis(REDIS_URL, { | ||
| maxRetriesPerRequest: null, | ||
| lazyConnect: true, | ||
| }); | ||
| } | ||
| }, 5 * 60 * 1000); | ||
| return redisClient; | ||
| }; |
There was a problem hiding this comment.
Risk of indefinite request hangs with current Redis configuration.
Setting maxRetriesPerRequest: null disables the per-request retry limit, causing commands to retry indefinitely if Redis is unavailable. Combined with lazyConnect: true and no explicit timeouts, this can cause API requests to hang forever when Redis is down.
Consider adding connection timeouts and a bounded retry strategy:
Suggested fix
const getRedisClient = () => {
if (!redisClient) {
redisClient = new Redis(REDIS_URL, {
- maxRetriesPerRequest: null,
+ maxRetriesPerRequest: 3,
+ connectTimeout: 5000,
+ commandTimeout: 2000,
lazyConnect: true,
});
+ redisClient.on('error', (err) => {
+ console.error('[RateLimit] Redis connection error:', err);
+ });
}
return redisClient;
};🤖 Prompt for AI Agents
In @nextjs_space/lib/rate-limit.ts around lines 12 - 20, The Redis client in
getRedisClient currently sets maxRetriesPerRequest: null and lazyConnect: true
which can cause indefinite hangs when Redis is down; update the Redis
constructor options for redisClient to use a bounded retry policy (e.g., set
maxRetriesPerRequest to a finite number or supply a retryStrategy), add a
connection timeout (e.g., connectTimeout or socket.connectTimeout), and disable
offline queueing (enableOfflineQueue: false) so commands fail fast; ensure
getRedisClient (and any caller) handles connection errors from lazyConnect() or
explicitly connect with a timeout and surface the error instead of letting
requests hang.
AutomatosAI
deleted the
codex/perform-full-review-of-saas-platform-x7zylq
branch
1 participant
Motivation
Description
nextjs_space/lib/tenant-context.ts) and clear it on errors by callingsetTenantContext(null)ingetTenantFromRequest/getCurrentTenant.nextjs_space/lib/db.tsso reads/writes are annotated withtenantIdand creation defaults to the current tenant.nextjs_space/lib/rate-limit.ts, makecheckRateLimit/getRateLimitStatusasynchronous, and switch call sites toawait checkRateLimit(...).findUniquetofindFirst, change session lookups to usesession.user.id, requireENCRYPTION_KEYinlib/encryption.ts, decrypt tenant Dr. Green keys viagetTenantDrGreenConfig, mask API keys in settings UI, and add audit logging on tenant settings updates; also update Prisma schema unique indexes to be tenant-scoped.Testing
Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Security
✏️ Tip: You can customize this high-level summary in your review settings.