fix: address PR review issues in GitLab webhook handler

edouard-mangel · claude · edouard-mangel · commit cf2f1778466d · 2026-03-29T10:05:20.000+02:00
- Guard `hasExistingSecurityMRNote` against non-ok HTTP responses before
  parsing JSON (was a silent failure path)
- Separate "Tag Push Hook" from "Push Hook" into distinct handlers; tag
  deploys now parse `refs/tags/`, filter by `triggerType = "tag"`, and
  omit the branch constraint — matching the GitHub handler pattern
- Add `triggerType = "push"` filter to push-hook app and compose queries
  so tag-triggered repos are not accidentally deployed on push events
- Remove `unlabeled` from the MR action allowlist; the event previously
  triggered security checks and the full app loop while doing nothing
  (shouldCreateDeployment already excluded it)
- Hoist `checkGitlabMemberPermissions` before the per-app loop — the
  check is per-MR-author so N apps no longer make N API calls
- Drop explicit `{ gitProvider: any }` return type on
  `findGitlabByWebhookSecret`; TypeScript now infers the Drizzle type
- Use `encode(gen_random_bytes(21), 'base64')` in migration instead of
  `gen_random_uuid()::text` for consistency with the rest of the schema

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/apps/dokploy/drizzle/0152_gitlab_preview_webhook.sql b/apps/dokploy/drizzle/0152_gitlab_preview_webhook.sql
@@ -1,5 +1,5 @@
 ALTER TABLE "gitlab" ADD COLUMN "webhook_secret" text;
 --> statement-breakpoint
-UPDATE "gitlab" SET "webhook_secret" = gen_random_uuid()::text WHERE "webhook_secret" IS NULL;
+UPDATE "gitlab" SET "webhook_secret" = encode(gen_random_bytes(21), 'base64') WHERE "webhook_secret" IS NULL;
 --> statement-breakpoint
 ALTER TABLE "gitlab" ALTER COLUMN "webhook_secret" SET NOT NULL;
diff --git a/apps/dokploy/pages/api/deploy/gitlab.ts b/apps/dokploy/pages/api/deploy/gitlab.ts
@@ -38,7 +38,92 @@ export default async function handler(
 	const event = req.headers["x-gitlab-event"] as string;
 	const body = req.body;
 
-	if (event === "Push Hook" || event === "Tag Push Hook") {
+	if (event === "Tag Push Hook") {
+		try {
+			const tagName = body?.ref?.replace("refs/tags/", "");
+			const deploymentHash = body?.checkout_sha;
+			const pathNamespace = body?.project?.path_with_namespace;
+
+			const apps = await db.query.applications.findMany({
+				where: and(
+					eq(applications.sourceType, "gitlab"),
+					eq(applications.autoDeploy, true),
+					eq(applications.triggerType, "tag"),
+					eq(applications.gitlabPathNamespace, pathNamespace),
+					eq(applications.gitlabId, gitlabProvider.gitlabId),
+				),
+			});
+
+			for (const app of apps) {
+				const jobData: DeploymentJob = {
+					applicationId: app.applicationId as string,
+					titleLog: `Tag created: ${tagName}`,
+					descriptionLog: `Hash: ${deploymentHash}`,
+					type: "deploy",
+					applicationType: "application",
+					server: !!app.serverId,
+				};
+
+				if (IS_CLOUD && app.serverId) {
+					jobData.serverId = app.serverId;
+					deploy(jobData).catch((error) => {
+						console.error("Background deployment failed:", error);
+					});
+					continue;
+				}
+				await myQueue.add("deployments", { ...jobData }, {
+					removeOnComplete: true,
+					removeOnFail: true,
+				});
+			}
+
+			const composeApps = await db.query.compose.findMany({
+				where: and(
+					eq(compose.sourceType, "gitlab"),
+					eq(compose.autoDeploy, true),
+					eq(compose.triggerType, "tag"),
+					eq(compose.gitlabPathNamespace, pathNamespace),
+					eq(compose.gitlabId, gitlabProvider.gitlabId),
+				),
+			});
+
+			for (const composeApp of composeApps) {
+				const jobData: DeploymentJob = {
+					composeId: composeApp.composeId as string,
+					titleLog: `Tag created: ${tagName}`,
+					descriptionLog: `Hash: ${deploymentHash}`,
+					type: "deploy",
+					applicationType: "compose",
+					server: !!composeApp.serverId,
+				};
+
+				if (IS_CLOUD && composeApp.serverId) {
+					jobData.serverId = composeApp.serverId;
+					deploy(jobData).catch((error) => {
+						console.error("Background deployment failed:", error);
+					});
+					continue;
+				}
+				await myQueue.add("deployments", { ...jobData }, {
+					removeOnComplete: true,
+					removeOnFail: true,
+				});
+			}
+
+			const totalApps = apps.length + composeApps.length;
+			res.status(200).json({
+				message:
+					totalApps === 0
+						? "No apps configured to deploy on tag"
+						: `Deployed ${totalApps} apps based on tag ${tagName}`,
+			});
+		} catch (error) {
+			res.status(400).json({ message: "Error deploying application", error });
+		}
+		return;
+	}
+
+	if (event === "Push Hook") {
 		try {
 			const branchName = body?.ref?.replace("refs/heads/", "");
 			const deploymentHash = body?.checkout_sha;
@@ -55,6 +140,7 @@ export default async function handler(
 				where: and(
 					eq(applications.sourceType, "gitlab"),
 					eq(applications.autoDeploy, true),
+					eq(applications.triggerType, "push"),
 					eq(applications.gitlabPathNamespace, pathNamespace),
 					eq(applications.gitlabBranch, branchName),
 					eq(applications.gitlabId, gitlabProvider.gitlabId),
@@ -92,6 +178,7 @@ export default async function handler(
 				where: and(
 					eq(compose.sourceType, "gitlab"),
 					eq(compose.autoDeploy, true),
+					eq(compose.triggerType, "push"),
 					eq(compose.gitlabPathNamespace, pathNamespace),
 					eq(compose.gitlabBranch, branchName),
 					eq(compose.gitlabId, gitlabProvider.gitlabId),
@@ -126,7 +213,12 @@ export default async function handler(
 			}
 
 			const totalApps = apps.length + composeApps.length;
-			res.status(200).json({ message: totalApps === 0 ? "No apps to deploy" : `Deployed ${totalApps} apps` });
+			res.status(200).json({
+				message:
+					totalApps === 0
+						? "No apps to deploy"
+						: `Deployed ${totalApps} apps`,
+			});
 		} catch (error) {
 			res.status(400).json({ message: "Error deploying application", error });
 		}
@@ -173,15 +265,8 @@ export default async function handler(
 			action === "open" ||
 			action === "update" ||
 			action === "reopen" ||
-			action === "labeled" ||
-			action === "unlabeled"
+			action === "labeled"
 		) {
-			const shouldCreateDeployment =
-				action === "open" ||
-				action === "update" ||
-				action === "reopen" ||
-				action === "labeled";
-
 			const targetBranch = body?.object_attributes?.target_branch as string;
 			const sourceBranch = body?.object_attributes?.source_branch as string;
 			const mrTitle = body?.object_attributes?.title as string;
@@ -202,36 +287,46 @@ export default async function handler(
 				},
 			});
 
-			// Security: check member permissions per app
+			// Permission check is per-MR-author, not per-app — check once before the loop
+			const requiresPermissionCheck = apps.some(
+				(app) => app.previewRequireCollaboratorPermissions !== false,
+			);
+			let permissionResult: Awaited<
+				ReturnType<typeof checkGitlabMemberPermissions>
+			> | null = null;
+			let permissionError: unknown = null;
+
+			if (requiresPermissionCheck) {
+				try {
+					permissionResult = await checkGitlabMemberPermissions(
+						gitlabProvider.gitlabId,
+						projectId,
+						mrAuthor,
+					);
+				} catch (error) {
+					permissionError = error;
+					console.error("Error validating MR author permissions:", error);
+				}
+			}
+
 			const secureApps: typeof apps = [];
 			let blockedAccessLevel: number | null = null;
 			let blocked = false;
 
 			for (const app of apps) {
 				if (app.previewRequireCollaboratorPermissions !== false) {
-					try {
-						const { hasWriteAccess, accessLevel } =
-							await checkGitlabMemberPermissions(
-								gitlabProvider.gitlabId,
-								projectId,
-								mrAuthor,
-							);
-
-						if (!hasWriteAccess) {
-							console.warn(
-								`🚨 SECURITY: Blocked preview deployment for ${app.name} from ${mrAuthor}. Access level: ${accessLevel}`,
-							);
-							if (!blocked) {
-								blockedAccessLevel = accessLevel;
-								blocked = true;
-							}
-							continue;
-						}
-					} catch (error) {
-						console.error(
-							`Error validating MR author permissions for ${app.name}:`,
-							error,
+					if (permissionError) {
+						continue;
+					}
+					const { hasWriteAccess, accessLevel } = permissionResult!;
+					if (!hasWriteAccess) {
+						console.warn(
+							`🚨 SECURITY: Blocked preview deployment for ${app.name} from ${mrAuthor}. Access level: ${accessLevel}`,
 						);
+						if (!blocked) {
+							blockedAccessLevel = accessLevel;
+							blocked = true;
+						}
 						continue;
 					}
 				}
@@ -275,7 +370,7 @@ export default async function handler(
 				let previewDeploymentId =
 					existingDeployment?.previewDeploymentId ?? "";
 
-				if (!existingDeployment && shouldCreateDeployment) {
+				if (!existingDeployment) {
 					const newDeployment = await createPreviewDeployment({
 						applicationId: app.applicationId as string,
 						branch: sourceBranch,
diff --git a/packages/server/src/services/gitlab.ts b/packages/server/src/services/gitlab.ts
@@ -65,7 +65,7 @@ export const findGitlabById = async (gitlabId: string) => {
 
 export const findGitlabByWebhookSecret = async (
 	webhookSecret: string,
-): Promise<(typeof gitlab.$inferSelect & { gitProvider: any }) | null> => {
+) => {
 	const result = await db.query.gitlab.findFirst({
 		where: eq(gitlab.webhookSecret, webhookSecret),
 		with: {
diff --git a/packages/server/src/utils/providers/gitlab.ts b/packages/server/src/utils/providers/gitlab.ts
@@ -350,6 +350,9 @@ export const hasExistingSecurityMRNote = async (
 		`${baseUrl}/api/v4/projects/${projectId}/merge_requests/${mrIid}/notes`,
 		{ headers: { Authorization: `Bearer ${gitlabProvider.accessToken}` } },
 	);
+	if (!response.ok) {
+		return false;
+	}
 	const notes: { id: number; body: string }[] = await response.json();
 	return notes.some((note) => note.body.includes(SECURITY_SENTINEL));
 };
