fix: harden security headers and webhook body handling#555
Conversation
|
@kenedybok3 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe PR refactors Prisma client setup to resolve database URLs at runtime, refresh clients when settings change, and add middleware instrumentation. It also updates Helmet security headers and switches webhook/general JSON parsing to ChangesDatabase client refresh and instrumentation
HTTP security and request parsing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
src/config/database.ts (1)
69-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd jitter to pool-exhaustion retries.
Every request retries on the same 200/400ms schedule, which can create synchronized retry bursts against an already saturated pool.
Proposed change
- const backoff = BASE_BACKOFF_MS * 2 ** (attempt - 1); + const backoffCap = BASE_BACKOFF_MS * 2 ** (attempt - 1); + const delay = Math.floor(Math.random() * backoffCap); logger.warn("Prisma connection pool exhausted, retrying", { model: params.model, action: params.action, attempt, maxRetries: MAX_RETRIES, - backoffMs: backoff, + delayMs: delay, }); - await new Promise((r) => setTimeout(r, backoff)); + await new Promise((r) => setTimeout(r, delay));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/database.ts` around lines 69 - 77, Add jitter to the Prisma pool-exhaustion retry path in the retry/backoff logic so requests do not all retry on the same schedule. Update the backoff calculation used in the logger.warn and setTimeout flow to introduce randomized variance around BASE_BACKOFF_MS * 2 ** (attempt - 1), while keeping MAX_RETRIES and the existing retry loop behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/database.ts`:
- Around line 201-218: refreshPrismaClientsIfNeeded() currently replaces
basePrisma and basePrismaReplica before confirming the new Prisma clients can
connect, which can leave the app pointed at broken clients after a bad rotation.
Update the flow in refreshPrismaClientsIfNeeded() to build and connect the
candidate clients first, only assign them to basePrisma/basePrismaReplica after
successful connection, and then disconnect the previous clients afterward. Use
the existing buildPrismaClient, applyPrismaClientMiddleware, and $disconnect
handling to preserve the current lifecycle.
- Around line 195-199: The logging in the database refresh path is exposing
sensitive connection strings; update the logger.info call in the database
refresh logic so it no longer includes raw runtimeUrl or replicaUrl values.
Replace those fields with redacted host/mode metadata only, keeping the existing
context from the refresh flow and the resolved settings object but ensuring any
identifiers in the DatabaseConfig/Prisma client refresh path are safe to emit.
- Around line 152-156: The current database initialization uses one global
useAccelerate flag for both client instances, but runtime and replica clients
need protocol-specific Accelerate detection based on their resolved URLs. Update
the database setup in the init path that builds the runtime and replica clients
so each derives its own runtimeUseAccelerate and replicaUseAccelerate from the
final URL via ACCELERATE_PROTOCOL_RE, and pass those per-client flags into
withAccelerate() instead of the shared flag. Make sure the client construction
logic around the runtime/replica initialization sites uses the resolved
URL-specific flag for each instance.
---
Nitpick comments:
In `@src/config/database.ts`:
- Around line 69-77: Add jitter to the Prisma pool-exhaustion retry path in the
retry/backoff logic so requests do not all retry on the same schedule. Update
the backoff calculation used in the logger.warn and setTimeout flow to introduce
randomized variance around BASE_BACKOFF_MS * 2 ** (attempt - 1), while keeping
MAX_RETRIES and the existing retry loop behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25b3a652-617a-4028-9887-1ba41e52d1ed
📒 Files selected for processing (3)
src/config/database.tssrc/index.tssrc/middleware/securityHeaders.ts
| const useAccelerate = Boolean(configuredAccelerateUrl); | ||
| const runtimeUrl = useAccelerate | ||
| ? configuredAccelerateUrl! | ||
| : appendStatementTimeout(configuredDatabaseUrl, latestStatementTimeoutMs); | ||
| const replicaUrl = configuredReplicaUrl || runtimeUrl; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
For @prisma/extension-accelerate 3.0.1, should a Prisma Client using a prisma:// or prisma+postgres:// connection URL be extended with withAccelerate(), and should that decision be made per PrismaClient instance?
💡 Result:
Yes, a Prisma Client instance using a prisma:// or prisma+postgres:// connection URL (for Prisma Accelerate) should generally be extended with the withAccelerate extension [1][2]. Extending with withAccelerate provides two primary benefits [1]: 1. It enables access to the cacheStrategy parameter within model queries, allowing for per-query caching configurations (such as ttl and swr) [1][3]. 2. It facilitates the use of advanced Accelerate features, such as withAccelerateInfo to inspect cache status [3]. While the Prisma Client will route queries through the Accelerate connection pool automatically if the connection string starts with prisma:// or prisma+postgres:// [1][4], applying the extension is standard practice to leverage the full suite of Accelerate's features [1][2]. Regarding instance management, this decision should be made per PrismaClient instance [1][4]. Because the extension is applied using the.$extends method on a specific instance, you must apply it to every instance that you intend to use with the Accelerate-specific features [1][3]. If you have multiple instances of PrismaClient, each one must be explicitly extended to utilize the Accelerate extension [1][5]. Note that for migration operations or other administrative tasks that require a direct database connection, you should typically use a separate, non-extended PrismaClient instance configured with your direct database URL, rather than the Accelerate URL [2][6].
Citations:
- 1: https://www.prisma.io/docs/accelerate/getting-started
- 2: https://www.prisma.io/docs/guides/integrations/neon-accelerate
- 3: https://www.prisma.io/docs/accelerate/reference/api-reference
- 4: https://www.prisma.io/docs/accelerate/local-development
- 5: https://www.prisma.io/docs/accelerate/evaluating
- 6: https://www.prisma.io/docs/postgres/database/connecting-to-your-database
Per-client Accelerate flag for URL-specific configuration.
Derive runtimeUseAccelerate and replicaUseAccelerate independently from their resolved URLs using ACCELERATE_PROTOCOL_RE, rather than relying on a single global useAccelerate flag derived from PRISMA_ACCELERATE_URL. Applying withAccelerate() to a client initialized with a direct database URL (e.g., postgresql://) when a global flag is true can cause configuration errors or runtime failures, as the extension must match the specific connection protocol of the instance.
Ensure the initialization at lines 225–227 and 272–274 consumes the specific flag for each client instance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/database.ts` around lines 152 - 156, The current database
initialization uses one global useAccelerate flag for both client instances, but
runtime and replica clients need protocol-specific Accelerate detection based on
their resolved URLs. Update the database setup in the init path that builds the
runtime and replica clients so each derives its own runtimeUseAccelerate and
replicaUseAccelerate from the final URL via ACCELERATE_PROTOCOL_RE, and pass
those per-client flags into withAccelerate() instead of the shared flag. Make
sure the client construction logic around the runtime/replica initialization
sites uses the resolved URL-specific flag for each instance.
| logger.info("[database] Refreshing Prisma clients with updated connection settings", { | ||
| runtimeUrl: resolved.runtimeUrl, | ||
| replicaUrl: resolved.replicaUrl, | ||
| useAccelerate: resolved.useAccelerate, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log raw database URLs.
runtimeUrl and replicaUrl can contain usernames, passwords, API keys, or tokens. Log only redacted host/mode metadata.
Proposed change
logger.info("[database] Refreshing Prisma clients with updated connection settings", {
- runtimeUrl: resolved.runtimeUrl,
- replicaUrl: resolved.replicaUrl,
+ runtimeConnection: resolved.useAccelerate ? "Prisma Accelerate (pooled)" : "direct PostgreSQL",
+ runtimeUrlChanged: resolved.runtimeUrl !== currentRuntimeUrl,
+ replicaUrlChanged: resolved.replicaUrl !== currentReplicaUrl,
useAccelerate: resolved.useAccelerate,
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/database.ts` around lines 195 - 199, The logging in the database
refresh path is exposing sensitive connection strings; update the logger.info
call in the database refresh logic so it no longer includes raw runtimeUrl or
replicaUrl values. Replace those fields with redacted host/mode metadata only,
keeping the existing context from the refresh flow and the resolved settings
object but ensuring any identifiers in the DatabaseConfig/Prisma client refresh
path are safe to emit.
| const previousBasePrisma = basePrisma; | ||
| const previousReplicaPrisma = basePrismaReplica; | ||
|
|
||
| basePrismaReplica.$use(async (params, next) => { | ||
| let lastError: unknown; | ||
| for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { | ||
| try { | ||
| return await next(params); | ||
| } catch (err) { | ||
| if (!isPoolExhaustionError(err)) { | ||
| throw err; | ||
| } | ||
| if (attempt < MAX_RETRIES) { | ||
| lastError = err; | ||
| const backoff = BASE_BACKOFF_MS * 2 ** (attempt - 1); | ||
| logger.warn("Prisma replica connection pool exhausted, retrying", { | ||
| model: params.model, | ||
| action: params.action, | ||
| attempt, | ||
| maxRetries: MAX_RETRIES, | ||
| backoffMs: backoff, | ||
| }); | ||
| await new Promise((r) => setTimeout(r, backoff)); | ||
| } else { | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
| throw lastError; | ||
| }); | ||
| basePrisma = buildPrismaClient(resolved.runtimeUrl); | ||
| basePrismaReplica = buildPrismaClient(resolved.replicaUrl); | ||
| currentRuntimeUrl = resolved.runtimeUrl; | ||
| currentReplicaUrl = resolved.replicaUrl; | ||
| currentUseAccelerate = resolved.useAccelerate; | ||
|
|
||
| applyPrismaClientMiddleware(basePrisma); | ||
| applyPrismaClientMiddleware(basePrismaReplica); | ||
|
|
||
| void previousBasePrisma.$disconnect().catch((err: unknown) => { | ||
| logger.warn("[database] Failed to disconnect previous Prisma client", { error: err }); | ||
| }); | ||
| void previousReplicaPrisma.$disconnect().catch((err: unknown) => { | ||
| logger.warn("[database] Failed to disconnect previous Prisma replica client", { error: err }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Swap clients only after the replacements connect successfully.
refreshPrismaClientsIfNeeded() mutates basePrisma/basePrismaReplica and starts disconnecting the previous clients before Line 275 proves the new clients can connect. A bad credential rotation can drop the working pool and leave exports pointed at failed clients. Build/connect candidates first, then commit the swap and disconnect old clients.
Also applies to: 272-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/database.ts` around lines 201 - 218,
refreshPrismaClientsIfNeeded() currently replaces basePrisma and
basePrismaReplica before confirming the new Prisma clients can connect, which
can leave the app pointed at broken clients after a bad rotation. Update the
flow in refreshPrismaClientsIfNeeded() to build and connect the candidate
clients first, only assign them to basePrisma/basePrismaReplica after successful
connection, and then disconnect the previous clients afterward. Use the existing
buildPrismaClient, applyPrismaClientMiddleware, and $disconnect handling to
preserve the current lifecycle.
2 participants
Closes #441
Closes #429
Closes #426
Summary
Related issues
helmet.crossOriginOpenerPolicynot set #426express.json({ verify })raw body not preserved for webhook signature verification #441Summary by CodeRabbit
Security Improvements
Bug Fixes