Tenant scoping, encrypted tenant secrets, Redis rate limiting, and audit logging#1
Conversation
📝 WalkthroughWalkthroughThis change introduces infrastructure for multi-tenant isolation and API request rate limiting. Adds Redis-backed rate limiting with async evaluation, tenant-scoped Prisma operations via middleware, Dr. Green API key encryption and decryption, AsyncLocalStorage-based tenant context propagation, and audit logging for settings updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
![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: 98306132ab
ℹ️ 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".
| if (params.args?.where) { | ||
| params.args.where = applyTenantScope(params.args.where, tenantId, allowNull); | ||
| } else if (params.action !== 'createMany') { |
There was a problem hiding this comment.
The middleware unconditionally rewrites params.args.where for all actions, including update, delete, and upsert, by merging in tenantId. Those actions use Prisma WhereUniqueInput, which only accepts unique fields (e.g., id), so adding tenantId will throw a runtime validation error for models without a composite unique on tenantId (e.g., orders, users). This means routes that set tenant context (like /app/api/orders/route.ts, which calls getTenantFromRequest and then prisma.orders.update({ where: { id } })) will now fail once the context is set. Consider scoping by converting to updateMany/deleteMany or handling scoping in a way that preserves WhereUniqueInput requirements.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nextjs_space/app/api/shop/register/route.ts (1)
35-53: Incomplete handling for missing credentials allows silent failure.The code logs an error at line 49 when credentials are missing but continues to
createClient, which will throw. This defers the failure rather than providing a clear early response.Consider returning a proper error response when credentials are unavailable:
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 store. Please contact support.' }, + { status: 503 } + ); }
🤖 Fix all issues with AI agents
In @nextjs_space/lib/db.ts:
- Around line 107-109: The middleware converts params.action from 'findUnique'
to 'findFirst' (params.action) to inject tenant scoping, which masks a schema
issue: orders.orderNumber and users.email are single-field uniques and could
leak across tenants if middleware is bypassed; fix by adding compound unique
constraints in the schema (e.g., @@unique([tenantId, orderNumber]) on the orders
model and a composite unique that includes tenantId for users' email) or
alternatively ensure all code paths always include tenantId in unique lookups
(update model definitions for orders and users and any unique-lookup usage to
include tenantId).
In @nextjs_space/lib/tenant.ts:
- Around line 165-168: The catch block in the function that fetches tenant from
a request currently logs the error and returns null but does not clear any
previously set tenant context; update the catch handler in getTenantFromRequest
(or the function that calls setTenantContext(tenant.id)) to call
setTenantContext(null) before returning null so you don't leave a stale tenant
in context — alternatively move the context cleanup into a finally block that
always calls setTenantContext(null) if appropriate.
🧹 Nitpick comments (7)
nextjs_space/lib/encryption.ts (1)
41-47: Fallback to returning original text may mask encryption issues.When the format is invalid, returning the original text could silently expose unencrypted sensitive data if a caller assumes the value is decrypted. Consider throwing an error or returning a distinct sentinel value to make the failure explicit.
♻️ Suggested improvement
if (parts.length !== 3) { - // Fallback: If it's not in our format, return as is (maybe it wasn't encrypted yet) - // Or throw error. For migration safety, we can return null or error. - console.warn('Invalid encrypted format, returning original'); - return text; + // Invalid format - throw to make failure explicit + throw new Error('Invalid encrypted format'); }If migration compatibility is required, consider a dedicated migration utility instead.
nextjs_space/app/api/tenant-admin/settings/route.ts (1)
8-14: Consider adding rate limiting to this settings endpoint.Other admin endpoints in this PR use rate limiting, but this settings update endpoint does not. Settings changes are sensitive operations that could benefit from rate limiting to prevent abuse.
nextjs_space/lib/tenant-context.ts (1)
9-11:enterWithcan leak context across unrelated async operations.
enterWith()modifies the current execution context permanently rather than scoping it to a callback. If called outside an existing async context (e.g., at the top level of a request handler before any async work begins), it can cause the tenant context to leak to subsequent unrelated requests sharing the same async resource.Consider using
runWithTenantContextconsistently instead, or ensuresetTenantContextis only called within an already-scoped async context established byrun().♻️ Alternative: Remove setTenantContext and use runWithTenantContext exclusively
If
setTenantContextmust remain for ergonomic reasons, document its constraints clearly:+/** + * Sets the tenant context for the current async execution. + * WARNING: Must only be called within an existing AsyncLocalStorage context + * (e.g., inside a runWithTenantContext callback or Next.js request handler). + * Calling this at module scope or outside async context can leak state. + */ export function setTenantContext(tenantId: string | null) { tenantContextStorage.enterWith({ tenantId }); }nextjs_space/lib/db.ts (1)
125-132: SettingtenantIdon upsert's update path may not behave as expected.Line 130 sets
tenantIdon the update payload, but if the tenant-scopedwhereclause (applied later) doesn't match an existing record (e.g., record exists but belongs to a different tenant), Prisma will execute thecreatepath instead. This is likely the intended security behavior, but it could silently duplicate records across tenants if the unique constraint doesn't includetenantId.Consider removing the
tenantIdassignment from the update path since the where clause already scopes to the correct tenant, and changing tenant ownership via update should probably be an explicit operation.♻️ Suggested change
if (params.action === 'upsert') { if (params.args?.create) { params.args.create.tenantId = params.args.create.tenantId ?? tenantId; } - if (params.args?.update) { - params.args.update.tenantId = params.args.update.tenantId ?? tenantId; - } }nextjs_space/lib/tenant.ts (1)
160-163: Consider clearing context when subdomain tenant is not found.If the subdomain path is taken but no tenant is found (line 159), the function falls through to line 170 returning
nullwithout clearing context. This is fine if no context was set earlier, but for defensive consistency, consider addingsetTenantContext(null)before the final return.♻️ Suggested improvement
} + setTenantContext(null); return null; }nextjs_space/app/api/shop/register/route.ts (1)
40-46: Fallback warning message may be misleading when platform credentials are empty.When
getTenantDrGreenConfigthrows, the warning states "Using platform Dr. Green credentials fallback" but doesn't verify platform credentials actually exist. Consider clarifying the log message.Optional clarity improvement
if (tenant?.id) { try { config = await getTenantDrGreenConfig(tenant.id); } catch (error) { - console.warn('Using platform Dr. Green credentials fallback:', error); + console.warn('Tenant-specific Dr. Green config unavailable, attempting platform fallback:', error); } }nextjs_space/app/tenant-admin/settings/settings-form.tsx (1)
162-173: Consider consistent pattern for Secret Key handling.The
drGreenSecretKeysection usestenant.drGreenSecretKeydirectly in the conditional (line 169, 172), while the API Key section uses a derivedhasApiKeyvariable. For consistency and readability, consider adding ahasSecretKeyvariable:const hasApiKey = Boolean(tenant.drGreenApiKey); +const hasSecretKey = Boolean(tenant.drGreenSecretKey);Then use
hasSecretKeyin lines 169 and 172. This is a minor stylistic nit.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
nextjs_space/app/api/shop/register/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/settings/route.tsnextjs_space/app/tenant-admin/settings/page.tsxnextjs_space/app/tenant-admin/settings/settings-form.tsxnextjs_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.js
🧰 Additional context used
🧬 Code graph analysis (11)
nextjs_space/app/api/tenant-admin/products/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
nextjs_space/app/api/tenant-admin/orders/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
nextjs_space/app/api/super-admin/tenants/bulk/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
nextjs_space/lib/db.ts (1)
nextjs_space/lib/tenant-context.ts (1)
getTenantContext(13-15)
nextjs_space/lib/tenant-config.ts (1)
nextjs_space/lib/encryption.ts (1)
decrypt(38-67)
nextjs_space/app/api/super-admin/tenants/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
nextjs_space/lib/rate-limit.ts (1)
nextjs_space/middleware.ts (1)
config(4-16)
nextjs_space/app/api/tenant-admin/products/reorder/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
nextjs_space/app/api/tenant-admin/analytics/route.ts (2)
nextjs_space/lib/rate-limit.ts (1)
checkRateLimit(50-95)nextjs_space/lib/auth.ts (1)
session(56-63)
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/app/api/shop/register/route.ts (2)
nextjs_space/middleware.ts (1)
config(4-16)nextjs_space/lib/tenant-config.ts (1)
getTenantDrGreenConfig(10-38)
🔇 Additional comments (25)
nextjs_space/app/api/tenant-admin/analytics/route.ts (1)
17-21: LGTM!The rate-limiting check is correctly awaited, aligning with the new async Redis-backed implementation.
nextjs_space/next.config.js (1)
17-17: LGTM!Environment-driven configuration for image optimization is a good practice. Note that the default behavior changes: without
NEXT_IMAGE_UNOPTIMIZED=true, images will now be optimized (previously hardcoded to unoptimized).nextjs_space/lib/encryption.ts (1)
5-5: LGTM - Removing public key fallback is the correct security posture.Encryption keys should never be exposed via
NEXT_PUBLIC_*environment variables.nextjs_space/lib/tenant-config.ts (1)
27-36: LGTM!The implementation correctly decrypts both API key and secret key, with proper validation before returning.
nextjs_space/app/api/tenant-admin/settings/route.ts (3)
82-89: LGTM!API key encryption follows the same established pattern as the secret key encryption, maintaining consistency.
91-95: Good security practice masking sensitive fields in logs.The masked logging correctly hides encrypted credential values.
103-117: Audit logging is well-structured.The implementation correctly captures user context, tenant ID, and client info for the audit trail.
nextjs_space/lib/rate-limit.ts (2)
90-92: Fail-open behavior is intentional but worth documenting.On Redis errors, requests are allowed through. This prioritizes availability over strict rate limiting. Ensure this trade-off is acceptable for your security requirements and consider adding metrics/alerting for Redis failures.
14-17: maxRetriesPerRequest: null does not cause infinite retries on failures—it affects pending command flushing during reconnection.The technical premise needs correction.
maxRetriesPerRequest: nulldoes not retry indefinitely on command failures. Instead, it disables per-request flushing; reconnection behavior is governed byretryStrategy/reconnectOnErroroptions (neither configured here).The lazyConnect concern is partially valid: the first request may incur a connection delay (bounded by
connectTimeout), but this is not due to infinite retries. Commands queue by default until the connection succeeds. The code already handles Redis errors gracefully with try-catch, allowing requests through if Redis is unavailable.If first-request latency is a concern, consider explicitly setting
connectTimeoutor disablinglazyConnect, but the current configuration with graceful error handling is defensible for a rate limiter.Likely an incorrect or invalid review comment.
nextjs_space/app/api/tenant-admin/products/bulk/route.ts (1)
30-34: LGTM!The rate-limiting check is correctly awaited, consistent with the other API routes updated in this PR.
nextjs_space/app/api/tenant-admin/products/reorder/route.ts (1)
19-23: LGTM!The rate limiting check is now correctly awaited to work with the async Redis-backed implementation. The pattern is consistent with other routes in the PR.
nextjs_space/app/api/tenant-admin/orders/bulk/route.ts (1)
32-36: LGTM!The rate limiting check is correctly awaited, consistent with the async Redis-backed rate limiter migration across the codebase.
nextjs_space/lib/tenant-context.ts (1)
13-18: LGTM!
getTenantContextandrunWithTenantContextare well-implemented. The nullish coalescing handles missing store gracefully, andrun()properly scopes the context to the callback's async lifetime.nextjs_space/lib/db.ts (2)
80-96: LGTM!The
applyTenantScopefunction correctly handles both strict tenant scoping and theallowNullcase for shared resources (like email templates). UsingANDwithORfor null-access models is the right approach.
98-104: LGTM on the guard clause and compatibility check.The
'$use' in prismacheck correctly handles the mock client case during builds. The early return for missing context or non-scoped models/actions is efficient.nextjs_space/lib/tenant.ts (2)
131-145: LGTM on path-based tenant resolution.The
/store/{slug}pattern extraction and tenant lookup correctly implement path-based routing. Setting the tenant context on successful resolution ensures downstream code has access to the tenant ID.
20-23: LGTM on tenant context propagation ingetCurrentTenant.Good defensive pattern: setting context to
nullwhen no identifiers are present, setting totenant?.id ?? nullon resolution, and clearing on error. This ensures the context is always in a known state.Also applies to: 52-58
nextjs_space/middleware.ts (2)
22-27: Good security hardening: stripping client-supplied tenant headers.Removing incoming
x-tenant-*headers prevents header spoofing attacks where a malicious client could inject arbitrary tenant context. The middleware now exclusively controls tenant header values based on trusted routing logic.
41-49: Consistent header forwarding pattern across all routes.All code paths now correctly forward the sanitized
requestHeadersviaNextResponse.next({ request: { headers: requestHeaders } }), ensuring tenant context is propagated only when derived from trusted sources (path, subdomain, or custom domain).Also applies to: 62-63, 75-76, 89-93
nextjs_space/app/api/super-admin/tenants/bulk/route.ts (1)
30-34: Correct async rate limit integration.The
awaitoncheckRateLimitaligns with the Redis-backed implementation that returns a Promise. The existing success check and response handling remain valid.nextjs_space/app/api/super-admin/tenants/route.ts (2)
24-28: GET handler correctly awaits async rate limit check.Consistent with the Redis-backed
checkRateLimitimplementation.
124-128: POST handler correctly awaits async rate limit check.Same pattern applied consistently across both handlers.
nextjs_space/app/tenant-admin/settings/page.tsx (1)
22-28: Good: Both API key and secret key are masked before client exposure.Consistent masking pattern prevents credential leakage to the browser. The
'********'placeholder correctly indicates the presence of a configured key without revealing its value.nextjs_space/app/tenant-admin/settings/settings-form.tsx (2)
29-34: Security improvement: API key no longer pre-populated in form.Initializing
drGreenApiKeyas an empty string rather thantenant.drGreenApiKeycorrectly prevents potentially masked or encrypted values from being sent back to the server on save. This aligns with the PR objective of avoiding exposure of credentials in the UI.
156-160: LGTM - Clear UX for masked credential handling.The conditional placeholder and helper text accurately communicate to users whether an API key exists and that leaving the field empty preserves the existing key.
1 participant
Motivation
Description
AsyncLocalStorage(lib/tenant-context.ts) and set it from tenant resolution inlib/tenant.tsandgetTenantFromRequestso the tenant id is available per request.lib/db.ts, which injectstenantIdforcreate/upsertand scopeswhereclauses for tenant-scoped models and actions.middleware.tsto remove incomingx-tenant-*headers, set derivedx-tenant-slug|subdomain|custom-domainon proxied requests, and avoid trusting client-supplied tenant headers.app/api/tenant-admin/settings/route.ts,lib/tenant-config.ts, andlib/encryption.ts, mask keys in the settings UI, and record aSETTINGS_UPDATEDaudit entry when settings are changed.lib/rate-limit.tsand update callers toawait checkRateLimit(...)across affected API routes, and add a toggle for Next.js image optimization innext.config.js.Testing
Codex Task
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.