[CodeRabbit] feat: Add deployment pipeline management with rollback support - #13
Open
smb060606 wants to merge 3 commits into
Open
[CodeRabbit] feat: Add deployment pipeline management with rollback support#13smb060606 wants to merge 3 commits into
smb060606 wants to merge 3 commits into
Conversation
Implements the deployment management module: - Create, list, and get deployment details - Deploy to staging/production environments - Rollback deployments to previous versions - Deployment status webhooks - Deployment history with filtering and pagination - Deployment statistics endpoint - Service account authentication support Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clean up comments that explicitly labeled code issues. Buggy code remains unchanged — only the identifying comments were removed or rewritten as natural developer comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR adds a complete deployment management system to the API, including CRUD operations, status lifecycle enforcement, rollback capability, deployment statistics and history queries, webhook-driven status updates from external services, and service-account authentication support for webhook integrations. ChangesDeployment Management Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
src/modules/deployments/deployments.test.ts (1)
4-48: 🏗️ Heavy liftCurrent tests don’t exercise the deployment service behavior.
These cases validate hardcoded values instead of invoking
createDeployment,updateDeploymentStatus,rollbackDeployment, andgetDeploymentStats. Please convert to service-level tests with mocked Prisma so regressions in lifecycle/rollback/query logic are caught.🤖 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/modules/deployments/deployments.test.ts` around lines 4 - 48, Replace the hardcoded assertions with true service-level tests that call createDeployment, updateDeploymentStatus, rollbackDeployment, and getDeploymentStats, and mock Prisma calls (e.g., prisma.deployment.count, prisma.deployment.create, prisma.deployment.findMany, prisma.deployment.update/aggregate) to produce controlled scenarios: for createDeployment mock count to test version incrementing; for updateDeploymentStatus mock current record and assert allowed transitions succeed and invalid ones throw; for rollbackDeployment mock findMany returning multiple deployments with createdAt so you verify the function selects the most recent prior SUCCESS (ensure ordering desc in the mock); for getDeploymentStats mock counts/queries to validate success-rate calculation. Use the service methods directly and assert returned values/errors rather than asserting on hardcoded variables.
🤖 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 `@prisma/migrations/002_add_deployment_indexes.sql`:
- Around line 5-7: The index currently named idx_deployments_project_env_status
only indexes project_id but should be a composite index for the intended filter;
update the migration to create a composite index on the deployments table using
the columns (project_id, environment, status) (or reorder if you prefer
highest-selectivity first) and ensure the statement still uses IF NOT EXISTS;
also remove or replace the existing single-column
idx_deployments_project_env_status (or rename) so the migration doesn't leave an
ineffective single-column index behind.
In `@src/middleware/auth.ts`:
- Around line 13-15: The code defines a hardcoded fallback secret SA_SECRET
which allows forged service tokens; remove the default value and require
SA_JWT_SECRET from the environment instead—replace the current assignment so
SA_SECRET reads process.env.SA_JWT_SECRET only and add a startup-time check
(e.g., in the module initialization or an exported init function) that throws or
calls process.exit(1) with a clear error if SA_SECRET is undefined, ensuring any
code that relies on SA_SECRET (symbol: SA_SECRET in src/middleware/auth.ts)
fails fast when the secret is not configured.
- Around line 26-38: When req.headers['x-service-account'] === 'true' and
jwt.verify(token, SA_SECRET) fails, do not fall through to regular auth;
immediately fail the request with an appropriate error response (e.g., set HTTP
401/403 and return) so service-account-only access cannot be bypassed. Modify
the block that calls jwt.verify in the auth middleware (the service-account
branch that currently catches errors and falls through) to handle the catch by
sending the error response and not calling the regular auth logic or next().
In `@src/modules/deployments/deployments.controller.ts`:
- Around line 35-37: deployment.changelog may be null/undefined so calling
.split() on it will throw; update the code around the changelogLines assignment
in deployments.controller.ts to guard against missing values by using a default
(e.g., deployment.changelog ?? '' or String(deployment.changelog)) before
splitting, or by conditional logic that sets changelogLines to an empty array
when deployment.changelog is falsy, ensuring subsequent code using
changelogLines is safe.
- Around line 10-14: The catch block in deployments.controller.ts currently
returns err.stack to clients; remove the stack and replace the response sent via
res.status(500).json({ error: ... }) with a safe, generic message (e.g.,
"Internal server error") and forward the original error to your centralized
handler/logger instead of exposing internals — for example, call the existing
centralized logger (processLogger.error or similar) and pass the error to the
Express error pipeline via next(err) or the controller's error forwarding
mechanism; specifically update the catch that uses res.status(500).json({ error:
err.message, stack: err.stack }) to not include stack traces and to log/forward
err.
In `@src/modules/deployments/deployments.service.ts`:
- Around line 219-224: getDeploymentHistory currently ignores the status filter
defined in the API schema; update the getDeploymentHistory function signature to
accept a status parameter (e.g., status?: string | string[]) and apply it when
building the query/filter (add a condition to restrict results to the provided
status or statuses alongside projectId/environment/startDate/endDate). Ensure
the query variable names referenced in this file (getDeploymentHistory) are
updated to include status and that any callers of getDeploymentHistory are
adjusted or validated to pass the status argument where applicable. Also add
unit/integration coverage for single and multiple status values to prevent
regressions.
- Around line 65-77: The enrichment loop creates an N+1 query by calling
prisma.user.findUnique and prisma.project.findUnique per deployment; instead
modify the initial prisma.findMany that retrieves deployments to include the
related deployedBy and project (using include with the same selected fields used
in createDeployment/getDeployment), remove the manual for...of enrichment and
the enrichedDeployments array, and return the findMany results directly (or map
only to shape if needed) so all related data is fetched in one query.
- Around line 143-151: The rollback selection currently uses
prisma.deployment.findFirst with orderBy: { createdAt: 'asc' } which returns the
oldest success; change it to orderBy: { createdAt: 'desc' } so
previousSuccessful picks the most recent successful deployment, and also add a
filter to ensure you only consider deployments created before the current one
(e.g., createdAt: { lt: deployment.createdAt }) so prisma.deployment.findFirst
(referenced by previousSuccessful) returns the immediate prior successful
deployment.
- Around line 24-32: The current read-then-write versioning
(prisma.deployment.findMany -> nextVersion) is race-prone and must be made
atomic; add a DB-level uniqueness constraint on (projectId, environment,
version) (e.g., @@unique([projectId, environment, version]) in schema for the
version field) and then change the creation flow in deployments.service.ts to
compute the next version by reading the max existing version (orderBy version
desc, take 1), increment it inside a transaction or attempt the create and
handle Prisma unique-constraint errors (P2002) by retrying the
read-increment-create sequence a few times, or alternatively use an
upsert/counter table to allocate a numeric counter and build the version string
(replace usages of prisma.deployment.findMany and nextVersion with the
transactional/unique-constraint-safe logic).
- Around line 219-258: The current getDeploymentHistory builds a SQL string by
directly interpolating projectId, environment, startDate, and endDate and calls
prisma.$queryRawUnsafe, which is vulnerable to SQL injection; replace this with
Prisma parameterized queries by importing sql from `@prisma/client` and using
prisma.$queryRaw(sql`...`) (or prisma.$queryRaw with tagged template) so all
values are passed as bound parameters (e.g., WHERE d.project_id = ${projectId});
for the optional filters (environment, startDate, endDate) build an array of sql
conditions using sql`${...}` and sql.join(conditions, sql` AND `) to append
safely, then run prisma.$queryRaw(sql`SELECT ... FROM deployments d JOIN users u
... ${sql.join(conditions, sql` AND `)} ORDER BY d.created_at DESC LIMIT 100`)
instead of using getDeploymentHistory’s prisma.$queryRawUnsafe with string
concatenation.
- Around line 157-179: The create of the rollback record
(prisma.deployment.create used to assign to rollback) and the subsequent
prisma.deployment.update that marks the original deployment as 'ROLLED_BACK'
must be performed atomically; wrap both operations in a single
prisma.$transaction call so either both succeed or both fail. Replace the two
separate calls (the create that produces rollback and the update that uses id)
with a transaction that performs the same create data (projectId, version
`${previousSuccessful.version}-rollback`, environment, status, deployedById,
commitSha, rollbackOfId) and the update (where: { id }, data: { status:
'ROLLED_BACK' }) in one transaction, returning the created rollback (and include
the same includes) from the transaction result.
In `@src/modules/deployments/webhook.service.ts`:
- Around line 11-20: Remove the insecure fallback and secret logging: stop using
a default value for WEBHOOK_SECRET and instead ensure the environment variable
is present (throw or return a failure if not) so validateWebhookSignature uses a
real secret; also remove WEBHOOK_SECRET from any logger calls in
validateWebhookSignature (log non-sensitive context only, e.g., signature
presence or validation result) and reference the const WEBHOOK_SECRET and
function validateWebhookSignature when making these changes.
- Around line 21-24: Guard against malformed signature values before calling
crypto.timingSafeEqual: validate that the incoming signature string is
non-empty, has the same hex length as the expected string, and contains only hex
characters (0-9a-fA-F); if any check fails return false. Then create the buffers
(Buffer.from(signature, 'hex') and Buffer.from(expected, 'hex')) and call
crypto.timingSafeEqual. Optionally wrap Buffer.from / timingSafeEqual in a
try/catch and return false on any thrown error to ensure the function (using the
signature and expected variables and crypto.timingSafeEqual) never throws for
malformed input.
---
Nitpick comments:
In `@src/modules/deployments/deployments.test.ts`:
- Around line 4-48: Replace the hardcoded assertions with true service-level
tests that call createDeployment, updateDeploymentStatus, rollbackDeployment,
and getDeploymentStats, and mock Prisma calls (e.g., prisma.deployment.count,
prisma.deployment.create, prisma.deployment.findMany,
prisma.deployment.update/aggregate) to produce controlled scenarios: for
createDeployment mock count to test version incrementing; for
updateDeploymentStatus mock current record and assert allowed transitions
succeed and invalid ones throw; for rollbackDeployment mock findMany returning
multiple deployments with createdAt so you verify the function selects the most
recent prior SUCCESS (ensure ordering desc in the mock); for getDeploymentStats
mock counts/queries to validate success-rate calculation. Use the service
methods directly and assert returned values/errors rather than asserting on
hardcoded variables.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74eba859-3fde-4375-9b47-6713b13cd9e6
⛔ Files ignored due to path filters (2)
.next/traceis excluded by!**/.next/**.next/trace-buildis excluded by!**/.next/**
📒 Files selected for processing (9)
jest.config.tsprisma/migrations/002_add_deployment_indexes.sqlsrc/middleware/auth.tssrc/modules/deployments/deployments.controller.tssrc/modules/deployments/deployments.routes.tssrc/modules/deployments/deployments.schema.tssrc/modules/deployments/deployments.service.tssrc/modules/deployments/deployments.test.tssrc/modules/deployments/webhook.service.ts
📜 Review details
🔇 Additional comments (4)
jest.config.ts (1)
14-23: ⚡ Quick winNo compatibility issue—ts-jest@29.4.9 is compatible with jest@30.4.2.
ts-jest 29.4.9 declares peer dependencies
^29.0.0 || ^30.0.0and officially supports Jest 30.x since version 29.4.0. No configuration changes or migration steps are required.> Likely an incorrect or invalid review comment.src/modules/deployments/deployments.routes.ts (3)
2-5: Clean route wiring and dependency setup.Imports are scoped to exactly what this router uses, and the validation/auth dependencies are integrated clearly.
10-15: Route ordering and middleware composition look solid.Lines 10-15 keep static routes before
/:idand applyvalidateRequest(...)on mutating endpoints, which is the right pattern here.
18-18: Rollback endpoint is correctly protected.Line 18 keeps rollback behind
authenticate, matching the rest of the deployment control surface.
Comment on lines
+5
to
+7
| CREATE INDEX IF NOT EXISTS idx_deployments_project_env_status | ||
| ON deployments (project_id); | ||
|
|
There was a problem hiding this comment.
Index columns don’t match the intended composite filter.
Line 6 only indexes project_id, but the migration/comment says this should support (project_id, environment, status) filtering. This misses the high-selectivity columns.
Suggested fix
CREATE INDEX IF NOT EXISTS idx_deployments_project_env_status
- ON deployments (project_id);
+ ON deployments (project_id, environment, status);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE INDEX IF NOT EXISTS idx_deployments_project_env_status | |
| ON deployments (project_id); | |
| CREATE INDEX IF NOT EXISTS idx_deployments_project_env_status | |
| ON deployments (project_id, environment, status); |
🤖 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 `@prisma/migrations/002_add_deployment_indexes.sql` around lines 5 - 7, The
index currently named idx_deployments_project_env_status only indexes project_id
but should be a composite index for the intended filter; update the migration to
create a composite index on the deployments table using the columns (project_id,
environment, status) (or reorder if you prefer highest-selectivity first) and
ensure the statement still uses IF NOT EXISTS; also remove or replace the
existing single-column idx_deployments_project_env_status (or rename) so the
migration doesn't leave an ineffective single-column index behind.
Comment on lines
+13
to
+15
| // Fallback secret for local dev when SA_JWT_SECRET is not configured | ||
| const SA_SECRET = process.env.SA_JWT_SECRET || 'teamforge-service-account-2024'; | ||
|
|
There was a problem hiding this comment.
Remove the hardcoded service-account JWT secret fallback.
Using a static default secret at Line 14 makes forged service tokens feasible whenever env config is missing/misconfigured.
Suggested fix
-const SA_SECRET = process.env.SA_JWT_SECRET || 'teamforge-service-account-2024';
+const SA_SECRET = process.env.SA_JWT_SECRET;
+if (!SA_SECRET) {
+ throw new Error('SA_JWT_SECRET is required');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fallback secret for local dev when SA_JWT_SECRET is not configured | |
| const SA_SECRET = process.env.SA_JWT_SECRET || 'teamforge-service-account-2024'; | |
| // Fallback secret for local dev when SA_JWT_SECRET is not configured | |
| const SA_SECRET = process.env.SA_JWT_SECRET; | |
| if (!SA_SECRET) { | |
| throw new Error('SA_JWT_SECRET is required'); | |
| } |
🤖 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/middleware/auth.ts` around lines 13 - 15, The code defines a hardcoded
fallback secret SA_SECRET which allows forged service tokens; remove the default
value and require SA_JWT_SECRET from the environment instead—replace the current
assignment so SA_SECRET reads process.env.SA_JWT_SECRET only and add a
startup-time check (e.g., in the module initialization or an exported init
function) that throws or calls process.exit(1) with a clear error if SA_SECRET
is undefined, ensuring any code that relies on SA_SECRET (symbol: SA_SECRET in
src/middleware/auth.ts) fails fast when the secret is not configured.
Comment on lines
+26
to
+38
| if (req.headers['x-service-account'] === 'true') { | ||
| try { | ||
| const payload = jwt.verify(token, SA_SECRET) as JwtPayload; | ||
| req.user = { | ||
| id: payload.id, | ||
| email: payload.email, | ||
| role: payload.role, | ||
| }; | ||
| return next(); | ||
| } catch { | ||
| // Fall through to regular auth | ||
| } | ||
| } |
There was a problem hiding this comment.
Service-account auth should fail closed, not fall through.
When x-service-account is true, a failed SA token verification currently drops into regular user auth. That can bypass a service-account-only boundary.
Suggested fix
if (req.headers['x-service-account'] === 'true') {
try {
const payload = jwt.verify(token, SA_SECRET) as JwtPayload;
req.user = {
id: payload.id,
email: payload.email,
role: payload.role,
};
return next();
- } catch {
- // Fall through to regular auth
+ } catch {
+ return next(new UnauthorizedError('Invalid service account token'));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (req.headers['x-service-account'] === 'true') { | |
| try { | |
| const payload = jwt.verify(token, SA_SECRET) as JwtPayload; | |
| req.user = { | |
| id: payload.id, | |
| email: payload.email, | |
| role: payload.role, | |
| }; | |
| return next(); | |
| } catch { | |
| // Fall through to regular auth | |
| } | |
| } | |
| if (req.headers['x-service-account'] === 'true') { | |
| try { | |
| const payload = jwt.verify(token, SA_SECRET) as JwtPayload; | |
| req.user = { | |
| id: payload.id, | |
| email: payload.email, | |
| role: payload.role, | |
| }; | |
| return next(); | |
| } catch { | |
| return next(new UnauthorizedError('Invalid service account token')); | |
| } | |
| } |
🤖 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/middleware/auth.ts` around lines 26 - 38, When
req.headers['x-service-account'] === 'true' and jwt.verify(token, SA_SECRET)
fails, do not fall through to regular auth; immediately fail the request with an
appropriate error response (e.g., set HTTP 401/403 and return) so
service-account-only access cannot be bypassed. Modify the block that calls
jwt.verify in the auth middleware (the service-account branch that currently
catches errors and falls through) to handle the catch by sending the error
response and not calling the regular auth logic or next().
Comment on lines
+10
to
+14
| } catch (err) { | ||
| // Return detailed error info for debugging | ||
| if (err instanceof Error) { | ||
| res.status(500).json({ error: err.message, stack: err.stack }); | ||
| } else { |
There was a problem hiding this comment.
Do not return stack traces to clients.
Line 13 exposes internal stack details in API responses. Return a safe message and forward error to centralized handler/logging.
🤖 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/modules/deployments/deployments.controller.ts` around lines 10 - 14, The
catch block in deployments.controller.ts currently returns err.stack to clients;
remove the stack and replace the response sent via res.status(500).json({ error:
... }) with a safe, generic message (e.g., "Internal server error") and forward
the original error to your centralized handler/logger instead of exposing
internals — for example, call the existing centralized logger
(processLogger.error or similar) and pass the error to the Express error
pipeline via next(err) or the controller's error forwarding mechanism;
specifically update the catch that uses res.status(500).json({ error:
err.message, stack: err.stack }) to not include stack traces and to log/forward
err.
Comment on lines
+35
to
+37
| // Format changelog into individual lines | ||
| const changelogLines = deployment.changelog.split('\n'); | ||
|
|
There was a problem hiding this comment.
changelog can be null/undefined before .split().
Line 36 will throw when deployment.changelog is absent. Guard with a default string/array.
🤖 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/modules/deployments/deployments.controller.ts` around lines 35 - 37,
deployment.changelog may be null/undefined so calling .split() on it will throw;
update the code around the changelogLines assignment in
deployments.controller.ts to guard against missing values by using a default
(e.g., deployment.changelog ?? '' or String(deployment.changelog)) before
splitting, or by conditional logic that sets changelogLines to an empty array
when deployment.changelog is falsy, ensuring subsequent code using
changelogLines is safe.
Comment on lines
+157
to
+179
| // Create a new deployment that represents the rollback | ||
| const rollback = await prisma.deployment.create({ | ||
| data: { | ||
| projectId: deployment.projectId, | ||
| version: `${previousSuccessful.version}-rollback`, | ||
| environment: deployment.environment, | ||
| status: 'PENDING' as any, | ||
| deployedById: userId, | ||
| commitSha: previousSuccessful.commitSha, | ||
| rollbackOfId: id, | ||
| }, | ||
| include: { | ||
| project: { select: { id: true, name: true } }, | ||
| deployedBy: { select: { id: true, name: true } }, | ||
| rollbackOf: { select: { id: true, version: true } }, | ||
| }, | ||
| }); | ||
|
|
||
| // Mark original deployment as rolled back | ||
| await prisma.deployment.update({ | ||
| where: { id }, | ||
| data: { status: 'ROLLED_BACK' as any }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/modules/deployments/deployments.service.ts | sed -n '150,185p'Repository: smb060606/teamforge-api
Length of output: 1330
🏁 Script executed:
# Check if there are any transaction wrappers around this code
rg -A 20 "Create a new deployment that represents the rollback" src/modules/deployments/deployments.service.tsRepository: smb060606/teamforge-api
Length of output: 762
🏁 Script executed:
# Check the overall structure of the function containing this code
rg -B 30 "Create a new deployment that represents the rollback" src/modules/deployments/deployments.service.ts | head -60Repository: smb060606/teamforge-api
Length of output: 920
🏁 Script executed:
# Check if $transaction is used elsewhere in the file
rg '\$transaction' src/modules/deployments/deployments.service.tsRepository: smb060606/teamforge-api
Length of output: 49
🏁 Script executed:
# Check broader codebase for $transaction usage pattern
rg '\$transaction' --type ts --type js | head -20Repository: smb060606/teamforge-api
Length of output: 49
Rollback create/update must be atomic.
Creating rollback deployment and updating the original deployment status in separate operations can leave inconsistent state if the second write fails. If the create succeeds but the update fails, the original deployment will remain marked as 'SUCCESS' while the rollback record already exists with rollbackOfId pointing to it, violating data consistency.
Use prisma.$transaction() to wrap both operations:
Suggested fix
- const rollback = await prisma.deployment.create({ ... });
- await prisma.deployment.update({ ... });
+ const rollback = await prisma.$transaction(async (tx) => {
+ const created = await tx.deployment.create({ ... });
+ await tx.deployment.update({ ... });
+ return created;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Create a new deployment that represents the rollback | |
| const rollback = await prisma.deployment.create({ | |
| data: { | |
| projectId: deployment.projectId, | |
| version: `${previousSuccessful.version}-rollback`, | |
| environment: deployment.environment, | |
| status: 'PENDING' as any, | |
| deployedById: userId, | |
| commitSha: previousSuccessful.commitSha, | |
| rollbackOfId: id, | |
| }, | |
| include: { | |
| project: { select: { id: true, name: true } }, | |
| deployedBy: { select: { id: true, name: true } }, | |
| rollbackOf: { select: { id: true, version: true } }, | |
| }, | |
| }); | |
| // Mark original deployment as rolled back | |
| await prisma.deployment.update({ | |
| where: { id }, | |
| data: { status: 'ROLLED_BACK' as any }, | |
| }); | |
| // Create a new deployment that represents the rollback and atomically mark original as rolled back | |
| const rollback = await prisma.$transaction(async (tx) => { | |
| const created = await tx.deployment.create({ | |
| data: { | |
| projectId: deployment.projectId, | |
| version: `${previousSuccessful.version}-rollback`, | |
| environment: deployment.environment, | |
| status: 'PENDING' as any, | |
| deployedById: userId, | |
| commitSha: previousSuccessful.commitSha, | |
| rollbackOfId: id, | |
| }, | |
| include: { | |
| project: { select: { id: true, name: true } }, | |
| deployedBy: { select: { id: true, name: true } }, | |
| rollbackOf: { select: { id: true, version: true } }, | |
| }, | |
| }); | |
| await tx.deployment.update({ | |
| where: { id }, | |
| data: { status: 'ROLLED_BACK' as any }, | |
| }); | |
| return created; | |
| }); |
🤖 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/modules/deployments/deployments.service.ts` around lines 157 - 179, The
create of the rollback record (prisma.deployment.create used to assign to
rollback) and the subsequent prisma.deployment.update that marks the original
deployment as 'ROLLED_BACK' must be performed atomically; wrap both operations
in a single prisma.$transaction call so either both succeed or both fail.
Replace the two separate calls (the create that produces rollback and the update
that uses id) with a transaction that performs the same create data (projectId,
version `${previousSuccessful.version}-rollback`, environment, status,
deployedById, commitSha, rollbackOfId) and the update (where: { id }, data: {
status: 'ROLLED_BACK' }) in one transaction, returning the created rollback (and
include the same includes) from the transaction result.
Comment on lines
+219
to
+224
| export async function getDeploymentHistory( | ||
| projectId: string, | ||
| environment?: string, | ||
| startDate?: string, | ||
| endDate?: string, | ||
| ) { |
There was a problem hiding this comment.
status filter support is missing in deployment history.
The history query API supports status filtering in schema, but this service signature/query do not accept or apply it. That leaves an advertised filter path non-functional.
🤖 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/modules/deployments/deployments.service.ts` around lines 219 - 224,
getDeploymentHistory currently ignores the status filter defined in the API
schema; update the getDeploymentHistory function signature to accept a status
parameter (e.g., status?: string | string[]) and apply it when building the
query/filter (add a condition to restrict results to the provided status or
statuses alongside projectId/environment/startDate/endDate). Ensure the query
variable names referenced in this file (getDeploymentHistory) are updated to
include status and that any callers of getDeploymentHistory are adjusted or
validated to pass the status argument where applicable. Also add
unit/integration coverage for single and multiple status values to prevent
regressions.
Comment on lines
+219
to
+258
| export async function getDeploymentHistory( | ||
| projectId: string, | ||
| environment?: string, | ||
| startDate?: string, | ||
| endDate?: string, | ||
| ) { | ||
| // Build deployment history query with optional filters | ||
| let query = ` | ||
| SELECT | ||
| d.id, | ||
| d.version, | ||
| d.environment, | ||
| d.status, | ||
| d.commit_sha, | ||
| d.started_at, | ||
| d.completed_at, | ||
| d.created_at, | ||
| u.name as deployed_by_name, | ||
| u.email as deployed_by_email, | ||
| EXTRACT(EPOCH FROM (d.completed_at - d.started_at)) as deploy_duration_seconds | ||
| FROM deployments d | ||
| JOIN users u ON d.deployed_by_id = u.id | ||
| WHERE d.project_id = '${projectId}' | ||
| `; | ||
|
|
||
| if (environment) { | ||
| query += ` AND d.environment = '${environment}'`; | ||
| } | ||
|
|
||
| if (startDate) { | ||
| query += ` AND d.created_at >= '${startDate}'`; | ||
| } | ||
|
|
||
| if (endDate) { | ||
| query += ` AND d.created_at <= '${endDate}'`; | ||
| } | ||
|
|
||
| query += ` ORDER BY d.created_at DESC LIMIT 100`; | ||
|
|
||
| const results = await prisma.$queryRawUnsafe(query); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "deployments.service.ts" -type fRepository: smb060606/teamforge-api
Length of output: 116
🏁 Script executed:
# Once we locate the file, read lines around 219-258
head -300 src/modules/deployments/deployments.service.ts | tail -100Repository: smb060606/teamforge-api
Length of output: 3076
🏁 Script executed:
# Let's also check the Prisma usage pattern to understand $queryRawUnsafe behavior
rg "queryRawUnsafe|queryRaw" -A 3 -B 1Repository: smb060606/teamforge-api
Length of output: 331
Eliminate SQL injection vulnerability in deployment history query.
The getDeploymentHistory function directly interpolates projectId, environment, startDate, and endDate parameters into a SQL string executed via $queryRawUnsafe. This allows attackers to inject arbitrary SQL commands. For example, setting projectId to 1' OR '1'='1 would bypass the WHERE clause.
Use Prisma's parameterized query API with $queryRaw and template literal parameter binding instead:
Suggested fix
- let query = `... WHERE d.project_id = '${projectId}'`;
- if (environment) query += ` AND d.environment = '${environment}'`;
- ...
- const results = await prisma.$queryRawUnsafe(query);
+ const results = await prisma.$queryRaw`
+ SELECT ... FROM deployments d
+ JOIN users u ON d.deployed_by_id = u.id
+ WHERE d.project_id = ${projectId}
+ ${environment ? sql`AND d.environment = ${environment}` : sql``}
+ ${startDate ? sql`AND d.created_at >= ${startDate}` : sql``}
+ ${endDate ? sql`AND d.created_at <= ${endDate}` : sql``}
+ ORDER BY d.created_at DESC LIMIT 100
+ `;(Import sql from @prisma/client)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function getDeploymentHistory( | |
| projectId: string, | |
| environment?: string, | |
| startDate?: string, | |
| endDate?: string, | |
| ) { | |
| // Build deployment history query with optional filters | |
| let query = ` | |
| SELECT | |
| d.id, | |
| d.version, | |
| d.environment, | |
| d.status, | |
| d.commit_sha, | |
| d.started_at, | |
| d.completed_at, | |
| d.created_at, | |
| u.name as deployed_by_name, | |
| u.email as deployed_by_email, | |
| EXTRACT(EPOCH FROM (d.completed_at - d.started_at)) as deploy_duration_seconds | |
| FROM deployments d | |
| JOIN users u ON d.deployed_by_id = u.id | |
| WHERE d.project_id = '${projectId}' | |
| `; | |
| if (environment) { | |
| query += ` AND d.environment = '${environment}'`; | |
| } | |
| if (startDate) { | |
| query += ` AND d.created_at >= '${startDate}'`; | |
| } | |
| if (endDate) { | |
| query += ` AND d.created_at <= '${endDate}'`; | |
| } | |
| query += ` ORDER BY d.created_at DESC LIMIT 100`; | |
| const results = await prisma.$queryRawUnsafe(query); | |
| import { sql } from '@prisma/client'; | |
| export async function getDeploymentHistory( | |
| projectId: string, | |
| environment?: string, | |
| startDate?: string, | |
| endDate?: string, | |
| ) { | |
| const results = await prisma.$queryRaw` | |
| SELECT | |
| d.id, | |
| d.version, | |
| d.environment, | |
| d.status, | |
| d.commit_sha, | |
| d.started_at, | |
| d.completed_at, | |
| d.created_at, | |
| u.name as deployed_by_name, | |
| u.email as deployed_by_email, | |
| EXTRACT(EPOCH FROM (d.completed_at - d.started_at)) as deploy_duration_seconds | |
| FROM deployments d | |
| JOIN users u ON d.deployed_by_id = u.id | |
| WHERE d.project_id = ${projectId} | |
| ${environment ? sql`AND d.environment = ${environment}` : sql``} | |
| ${startDate ? sql`AND d.created_at >= ${startDate}` : sql``} | |
| ${endDate ? sql`AND d.created_at <= ${endDate}` : sql``} | |
| ORDER BY d.created_at DESC LIMIT 100 | |
| `; | |
| return results; | |
| } |
🤖 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/modules/deployments/deployments.service.ts` around lines 219 - 258, The
current getDeploymentHistory builds a SQL string by directly interpolating
projectId, environment, startDate, and endDate and calls prisma.$queryRawUnsafe,
which is vulnerable to SQL injection; replace this with Prisma parameterized
queries by importing sql from `@prisma/client` and using
prisma.$queryRaw(sql`...`) (or prisma.$queryRaw with tagged template) so all
values are passed as bound parameters (e.g., WHERE d.project_id = ${projectId});
for the optional filters (environment, startDate, endDate) build an array of sql
conditions using sql`${...}` and sql.join(conditions, sql` AND `) to append
safely, then run prisma.$queryRaw(sql`SELECT ... FROM deployments d JOIN users u
... ${sql.join(conditions, sql` AND `)} ORDER BY d.created_at DESC LIMIT 100`)
instead of using getDeploymentHistory’s prisma.$queryRawUnsafe with string
concatenation.
Comment on lines
+11
to
+20
| const WEBHOOK_SECRET = process.env.WEBHOOK_SECRET || 'default-webhook-secret'; | ||
|
|
||
| export function validateWebhookSignature(payload: string, signature: string): boolean { | ||
| const expected = crypto | ||
| .createHmac('sha256', WEBHOOK_SECRET) | ||
| .update(payload) | ||
| .digest('hex'); | ||
|
|
||
| logger.info({ signature, secret: WEBHOOK_SECRET }, 'Webhook received'); | ||
|
|
There was a problem hiding this comment.
Do not fallback to a default webhook secret or log secrets.
Line 11 weakens signature security when env config is missing, and Line 19 logs WEBHOOK_SECRET, which is sensitive credential leakage.
🤖 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/modules/deployments/webhook.service.ts` around lines 11 - 20, Remove the
insecure fallback and secret logging: stop using a default value for
WEBHOOK_SECRET and instead ensure the environment variable is present (throw or
return a failure if not) so validateWebhookSignature uses a real secret; also
remove WEBHOOK_SECRET from any logger calls in validateWebhookSignature (log
non-sensitive context only, e.g., signature presence or validation result) and
reference the const WEBHOOK_SECRET and function validateWebhookSignature when
making these changes.
Comment on lines
+21
to
+24
| return crypto.timingSafeEqual( | ||
| Buffer.from(signature, 'hex'), | ||
| Buffer.from(expected, 'hex'), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/modules/deployments/webhook.service.ts | head -50Repository: smb060606/teamforge-api
Length of output: 1621
🏁 Script executed:
# Search for calls to validateWebhookSignature
rg "validateWebhookSignature" --type ts --type jsRepository: smb060606/teamforge-api
Length of output: 198
🏁 Script executed:
# Also check the Node.js crypto.timingSafeEqual documentation behavior
# Let's create a quick test to understand the behavior
python3 << 'EOF'
# Based on Node.js documentation, timingSafeEqual throws when buffers have different lengths
# Let's verify this is the actual behavior and understand the issue better
print("Node.js crypto.timingSafeEqual behavior:")
print("- Requires both buffers to be the same length")
print("- Throws TypeError if buffer lengths differ")
print("- Used for comparing sensitive values to prevent timing attacks")
print()
print("Buffer.from(string, 'hex') behavior:")
print("- Expects a hex-encoded string (0-9, a-f)")
print("- Invalid hex characters will cause silent failures or unexpected results")
print("- If string has odd length, Node.js v17+ may throw 'ERR_INVALID_ARG_VALUE'")
print("- Earlier versions may create a buffer from partial hex data")
EOFRepository: smb060606/teamforge-api
Length of output: 552
🏁 Script executed:
# Check if there are any existing input validation or error handlers
rg -A 5 -B 5 "validateWebhookSignature" src/ --type tsRepository: smb060606/teamforge-api
Length of output: 842
🏁 Script executed:
# Search for where validateWebhookSignature is called
rg "validateWebhookSignature" --type ts --type js -B 3 -A 3Repository: smb060606/teamforge-api
Length of output: 626
🏁 Script executed:
# Search for where this function might be called indirectly or imported
rg "webhook" --type ts --type js -i | head -30Repository: smb060606/teamforge-api
Length of output: 973
🏁 Script executed:
# Check if there's any try-catch or error handling around signature validation
rg "processWebhook|validateWebhook" --type ts --type js -B 5 -A 10Repository: smb060606/teamforge-api
Length of output: 2274
🏁 Script executed:
# Search for any route handlers or middleware that might call validateWebhookSignature
fd . --type f --extension ts --extension js | xargs rg -l "webhook|signature" | head -20Repository: smb060606/teamforge-api
Length of output: 110
🏁 Script executed:
# Check if this is used in any routes or controllers
rg "validateWebhook|webhook.service" --type ts --type js -B 2 -A 5Repository: smb060606/teamforge-api
Length of output: 689
🏁 Script executed:
# Let's verify the exact behavior of crypto.timingSafeEqual with unequal buffers
node << 'EOF'
const crypto = require('crypto');
// Test 1: Different length buffers
console.log("Test 1: Different length buffers");
try {
const result = crypto.timingSafeEqual(
Buffer.from('abcd', 'hex'),
Buffer.from('abcdef', 'hex')
);
console.log("No error thrown, result:", result);
} catch (err) {
console.log("Error thrown:", err.message);
}
// Test 2: Invalid hex string
console.log("\nTest 2: Invalid hex string with even length");
try {
const result = crypto.timingSafeEqual(
Buffer.from('zzzz', 'hex'), // Invalid hex chars
Buffer.from('abcd', 'hex')
);
console.log("No error thrown, result:", result);
} catch (err) {
console.log("Error thrown:", err.message);
}
// Test 3: Odd length hex string
console.log("\nTest 3: Odd length hex string");
try {
const result = crypto.timingSafeEqual(
Buffer.from('abc', 'hex'), // Odd length
Buffer.from('ab', 'hex')
);
console.log("No error thrown, result:", result);
} catch (err) {
console.log("Error thrown:", err.message);
}
// Test 4: SHA256 digest length
console.log("\nTest 4: SHA256 digest length");
const sha256Digest = crypto.createHmac('sha256', 'secret').update('data').digest('hex');
console.log("SHA256 hex length:", sha256Digest.length);
console.log("SHA256 hex value:", sha256Digest);
EOFRepository: smb060606/teamforge-api
Length of output: 459
🏁 Script executed:
# Let's test more specifically what happens with odd-length hex strings in timingSafeEqual
node << 'EOF'
const crypto = require('crypto');
// The expected hex from SHA256 is always 64 chars (32 bytes)
const expected = crypto.createHmac('sha256', 'secret').update('data').digest('hex');
console.log("Expected length:", expected.length, "bytes:", Buffer.from(expected, 'hex').length);
// Test: odd-length signature vs 64-char expected
console.log("\nTest: 63-char signature vs 64-char expected");
try {
const result = crypto.timingSafeEqual(
Buffer.from('a'.repeat(63), 'hex'), // 63 chars, will be 31.5 bytes -> 31 bytes
Buffer.from(expected, 'hex') // 64 chars, 32 bytes
);
console.log("No error, result:", result);
} catch (err) {
console.log("Error:", err.message);
}
// Test: even-length but wrong length signature
console.log("\nTest: 62-char signature vs 64-char expected");
try {
const result = crypto.timingSafeEqual(
Buffer.from('a'.repeat(62), 'hex'), // 62 chars, 31 bytes
Buffer.from(expected, 'hex') // 64 chars, 32 bytes
);
console.log("No error, result:", result);
} catch (err) {
console.log("Error:", err.message);
}
// Test: What if signature has invalid hex?
console.log("\nTest: Invalid hex chars (z)");
try {
const result = crypto.timingSafeEqual(
Buffer.from('zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz', 'hex'),
Buffer.from(expected, 'hex')
);
console.log("No error, result:", result);
} catch (err) {
console.log("Error:", err.message);
}
EOFRepository: smb060606/teamforge-api
Length of output: 372
🏁 Script executed:
# Let's verify the suggested fix more thoroughly - test what Buffer.from does with various inputs
node << 'EOF'
const crypto = require('crypto');
const expected = crypto.createHmac('sha256', 'secret').update('data').digest('hex');
console.log("Expected:", expected);
console.log("Expected buffer length:", Buffer.from(expected, 'hex').length);
// Test what the suggested fix would prevent
const testCases = [
{ sig: 'abc', desc: 'odd length (3)' },
{ sig: 'a'.repeat(62), desc: 'even but wrong length (62)' },
{ sig: 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz', desc: 'invalid hex chars (64 z\'s)' },
{ sig: '', desc: 'empty string' },
{ sig: 'abcdefgh', desc: 'valid hex but wrong length (8)' },
];
console.log("\nWithout guards - what happens:");
testCases.forEach(({sig, desc}) => {
try {
const result = crypto.timingSafeEqual(
Buffer.from(sig, 'hex'),
Buffer.from(expected, 'hex')
);
console.log(`✓ ${desc}: No error`);
} catch (err) {
console.log(`✗ ${desc}: THROWS - ${err.message}`);
}
});
console.log("\nWith suggested guards - what would happen:");
testCases.forEach(({sig, desc}) => {
if (!/^[a-f0-9]+$/i.test(sig)) {
console.log(`✓ ${desc}: Regex check fails, returns false`);
} else if (sig.length !== expected.length) {
console.log(`✓ ${desc}: Length check fails, returns false`);
} else {
console.log(`✓ ${desc}: Passes both guards, would reach timingSafeEqual`);
}
});
EOFRepository: smb060606/teamforge-api
Length of output: 938
Guard against malformed signatures before timingSafeEqual.
crypto.timingSafeEqual throws when buffer lengths differ. Malformed or invalid signature values (odd-length hex, wrong length, or non-hex characters) will throw "Input buffers must have the same byte length" instead of returning false, breaking the function's contract and causing unhandled errors.
Add validation guards to ensure safe graceful failure:
Suggested fix
+ if (!/^[a-f0-9]+$/i.test(signature)) return false;
+ if (signature.length !== expected.length) return false;
return crypto.timingSafeEqual(
Buffer.from(signature, 'hex'),
Buffer.from(expected, 'hex'),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return crypto.timingSafeEqual( | |
| Buffer.from(signature, 'hex'), | |
| Buffer.from(expected, 'hex'), | |
| ); | |
| if (!/^[a-f0-9]*$/i.test(signature) || signature.length % 2 !== 0) return false; | |
| if (signature.length !== expected.length) return false; | |
| return crypto.timingSafeEqual( | |
| Buffer.from(signature, 'hex'), | |
| Buffer.from(expected, 'hex'), | |
| ); |
🤖 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/modules/deployments/webhook.service.ts` around lines 21 - 24, Guard
against malformed signature values before calling crypto.timingSafeEqual:
validate that the incoming signature string is non-empty, has the same hex
length as the expected string, and contains only hex characters (0-9a-fA-F); if
any check fails return false. Then create the buffers (Buffer.from(signature,
'hex') and Buffer.from(expected, 'hex')) and call crypto.timingSafeEqual.
Optionally wrap Buffer.from / timingSafeEqual in a try/catch and return false on
any thrown error to ensure the function (using the signature and expected
variables and crypto.timingSafeEqual) never throws for malformed input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A developer on the team has implemented the deployment management module. This PR adds the ability to create deployments, track their status through webhooks, view deployment history, calculate statistics, and rollback failed deployments. It also adds service account authentication so CI/CD systems can update deployment status.
This PR is for CodeRabbit review.
Summary by CodeRabbit
New Features
Performance