Skip to content

Commit 1c4d6a5

Browse files
fix: address code review issues in GitLab preview deployments
- checkGitlabMemberPermissions: guard against userId=undefined when GitLab username lookup returns no results (prevents malformed API URL) - gitlab.one tRPC endpoint: add ownership check (organizationId + userId) so webhookSecret cannot be read by other authenticated users - Push Hook: extract added/modified/removed files from commits and pass to shouldDeploy so watchPaths filtering works correctly - Push Hook: add shouldDeploy filtering to compose apps loop (was missing) - Add tests: userId=undefined guard, watchPaths filtering on push Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent cf52193 commit 1c4d6a5

5 files changed

Lines changed: 72 additions & 6 deletions

File tree

apps/dokploy/__test__/deploy/gitlab.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,30 @@ describe("GitLab webhook handler — Push Hook", () => {
416416
expect(res.status).toHaveBeenCalledWith(200);
417417
});
418418

419+
it("skips app when watchPaths is set and none of the modified files match", async () => {
420+
const appWithWatchPaths = {
421+
...FAKE_APP,
422+
watchPaths: ["src/**"],
423+
};
424+
// First call (applications) returns app with watchPaths; second (compose) returns []
425+
vi.mocked(db.query.applications.findMany).mockResolvedValueOnce([
426+
appWithWatchPaths as any,
427+
]);
428+
vi.mocked(db.query.applications.findMany).mockResolvedValueOnce([] as any);
429+
430+
// Push only touches docs/ — does not match src/**
431+
const pushPayload = {
432+
...makePushPayload(),
433+
commits: [{ added: ["docs/readme.md"], modified: [], removed: [] }],
434+
};
435+
const req = makeReq("Push Hook", pushPayload);
436+
const res = makeRes();
437+
438+
await handler(req, res);
439+
440+
expect(myQueue.add).not.toHaveBeenCalled();
441+
});
442+
419443
it("returns 200 with no-ops message when no apps match the push", async () => {
420444
vi.mocked(db.query.applications.findMany).mockResolvedValue([]);
421445
vi.mocked(db.query.compose.findMany).mockResolvedValue([]);

apps/dokploy/__test__/utils/gitlab-preview-utils.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,25 @@ describe("checkGitlabMemberPermissions", () => {
140140
expect(result).toEqual({ hasWriteAccess: false, accessLevel: null });
141141
});
142142

143+
it("returns hasWriteAccess=false when the username lookup returns no users", async () => {
144+
// GitLab /users?username=ghost returns [] when user does not exist
145+
vi.stubGlobal(
146+
"fetch",
147+
vi.fn().mockResolvedValueOnce({
148+
ok: true,
149+
json: async () => [], // no matching user
150+
}),
151+
);
152+
153+
const result = await checkGitlabMemberPermissions(
154+
FAKE_GITLAB_ID,
155+
123,
156+
"ghost-user",
157+
);
158+
159+
expect(result).toEqual({ hasWriteAccess: false, accessLevel: null });
160+
});
161+
143162
it("throws when the members API returns a server error (500)", async () => {
144163
vi.stubGlobal(
145164
"fetch",

apps/dokploy/pages/api/deploy/gitlab.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@ export default async function handler(
4343
const branchName = body?.ref?.replace("refs/heads/", "");
4444
const deploymentHash = body?.checkout_sha;
4545
const pathNamespace = body?.project?.path_with_namespace;
46+
const modifiedFiles: string[] = (body?.commits ?? []).flatMap(
47+
(commit: any) => [
48+
...(commit.added ?? []),
49+
...(commit.modified ?? []),
50+
...(commit.removed ?? []),
51+
],
52+
);
4653

4754
const apps = await db.query.applications.findMany({
4855
where: and(
@@ -64,9 +71,7 @@ export default async function handler(
6471
server: !!app.serverId,
6572
};
6673

67-
const shouldDeployPaths = shouldDeploy(app.watchPaths, []);
68-
69-
if (!shouldDeployPaths) {
74+
if (!shouldDeploy(app.watchPaths, modifiedFiles)) {
7075
continue;
7176
}
7277

@@ -103,6 +108,10 @@ export default async function handler(
103108
server: !!composeApp.serverId,
104109
};
105110

111+
if (!shouldDeploy(composeApp.watchPaths, modifiedFiles)) {
112+
continue;
113+
}
114+
106115
if (IS_CLOUD && composeApp.serverId) {
107116
jobData.serverId = composeApp.serverId;
108117
deploy(jobData).catch((error) => {

apps/dokploy/server/api/routers/gitlab.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,19 @@ export const gitlabRouter = createTRPCRouter({
5050
});
5151
}
5252
}),
53-
one: protectedProcedure.input(apiFindOneGitlab).query(async ({ input }) => {
54-
return await findGitlabById(input.gitlabId);
55-
}),
53+
one: protectedProcedure
54+
.input(apiFindOneGitlab)
55+
.query(async ({ input, ctx }) => {
56+
const result = await findGitlabById(input.gitlabId);
57+
if (
58+
result.gitProvider.organizationId !==
59+
ctx.session.activeOrganizationId ||
60+
result.gitProvider.userId !== ctx.session.userId
61+
) {
62+
throw new TRPCError({ code: "FORBIDDEN", message: "Access denied" });
63+
}
64+
return result;
65+
}),
5666
gitlabProviders: protectedProcedure.query(async ({ ctx }) => {
5767
let result = await db.query.gitlab.findMany({
5868
with: {

packages/server/src/utils/providers/gitlab.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ export const checkGitlabMemberPermissions = async (
278278
const users = await userResponse.json();
279279
const userId = users[0]?.id;
280280

281+
if (!userId) {
282+
return { hasWriteAccess: false, accessLevel: null };
283+
}
284+
281285
// Check project membership
282286
const memberResponse = await fetch(
283287
`${baseUrl}/api/v4/projects/${projectId}/members/all/${userId}`,

0 commit comments

Comments
 (0)