diff --git a/apps/dokploy/__test__/git-provider/git-provider-update-fk-repoint.test.ts b/apps/dokploy/__test__/git-provider/git-provider-update-fk-repoint.test.ts new file mode 100644 index 000000000..c281a71cd --- /dev/null +++ b/apps/dokploy/__test__/git-provider/git-provider-update-fk-repoint.test.ts @@ -0,0 +1,134 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// Service-level guard: the update* services must never write the identity/FK +// columns (giteaId/githubId/gitlabId and gitProviderId). Repointing gitProviderId +// at another org's git_provider is the credential-leak mechanism. We capture the +// argument passed to drizzle's `.set()` and assert the FK/PK keys are absent. + +const capture = vi.hoisted(() => ({ setArgs: [] as unknown[] })); +const setMock = vi.hoisted(() => + vi.fn((values: unknown) => { + capture.setArgs.push(values); + return { where: () => ({ returning: () => Promise.resolve([{}]) }) }; + }), +); +const updateMock = vi.hoisted(() => vi.fn(() => ({ set: setMock }))); + +vi.mock("@dokploy/server/db", () => ({ + db: { + update: updateMock, + }, +})); + +import { updateGitea } from "@dokploy/server/services/gitea"; +import { updateGithub } from "@dokploy/server/services/github"; +import { updateGitlab } from "@dokploy/server/services/gitlab"; + +beforeEach(() => { + vi.clearAllMocks(); + capture.setArgs.length = 0; +}); + +const hijackPayload = { + giteaId: "gitea-victim", + gitProviderId: "gp-attacker", + giteaUrl: "https://evil.gitea.com", + clientSecret: "stolen-secret", + accessToken: "stolen-at", + refreshToken: "stolen-rt", +}; + +describe("updateGitea — FK/PK repoint guard", () => { + it("strips giteaId and gitProviderId from the SET clause", async () => { + await updateGitea("gitea-victim", hijackPayload); + + expect(setMock).toHaveBeenCalledTimes(1); + const setArg = capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("giteaId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.giteaUrl).toBe("https://evil.gitea.com"); + expect(setArg.clientSecret).toBe("stolen-secret"); + }); + + it("preserves OAuth secret columns when they are provided", async () => { + await updateGitea("gitea-victim", { + accessToken: "at", + refreshToken: "rt", + expiresAt: 123, + }); + + const setArg = capture.setArgs[0] as Record; + expect(setArg.accessToken).toBe("at"); + expect(setArg.refreshToken).toBe("rt"); + expect(setArg.expiresAt).toBe(123); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg).not.toHaveProperty("giteaId"); + }); + + it("only sends provided editable columns (omitted fields are not sent)", async () => { + await updateGitea("gitea-victim", { + giteaUrl: "https://gitea.com", + }); + + const setArg = capture.setArgs[0] as Record; + expect(Object.keys(setArg)).toEqual(["giteaUrl"]); + }); +}); + +describe("updateGithub — FK/PK repoint guard", () => { + it("strips githubId and gitProviderId from the SET clause", async () => { + await updateGithub("gh-victim", { + githubId: "gh-victim", + gitProviderId: "gp-attacker", + githubAppName: "stolen-app", + githubUrl: "https://github.corp.evil.com", + githubClientSecret: "stolen-secret", + }); + + expect(setMock).toHaveBeenCalledTimes(1); + const setArg = capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("githubId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.githubAppName).toBe("stolen-app"); + expect(setArg.githubUrl).toBe("https://github.corp.evil.com"); + expect(setArg.githubClientSecret).toBe("stolen-secret"); + }); + + it("only sends provided editable columns", async () => { + await updateGithub("gh-victim", { + githubAppName: "renamed-app", + }); + + const setArg = capture.setArgs[0] as Record; + expect(Object.keys(setArg)).toEqual(["githubAppName"]); + }); +}); + +describe("updateGitlab — FK/PK repoint guard", () => { + it("strips gitlabId and gitProviderId from the SET clause", async () => { + await updateGitlab("gl-victim", { + gitlabId: "gl-victim", + gitProviderId: "gp-attacker", + gitlabUrl: "https://gitlab.corp.evil.com", + secret: "stolen-secret", + accessToken: "stolen-at", + }); + + expect(setMock).toHaveBeenCalledTimes(1); + const setArg = capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("gitlabId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.gitlabUrl).toBe("https://gitlab.corp.evil.com"); + expect(setArg.secret).toBe("stolen-secret"); + expect(setArg.accessToken).toBe("stolen-at"); + }); + + it("only sends provided editable columns", async () => { + await updateGitlab("gl-victim", { + gitlabUrl: "https://gitlab.com", + }); + + const setArg = capture.setArgs[0] as Record; + expect(Object.keys(setArg)).toEqual(["gitlabUrl"]); + }); +}); diff --git a/apps/dokploy/__test__/git-provider/git-provider-update-router.test.ts b/apps/dokploy/__test__/git-provider/git-provider-update-router.test.ts new file mode 100644 index 000000000..844754071 --- /dev/null +++ b/apps/dokploy/__test__/git-provider/git-provider-update-router.test.ts @@ -0,0 +1,312 @@ +import { TRPCError } from "@trpc/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// Router-level IDOR guard for the *.update handlers. +// +// Models the exploit from the bug report: an attacker who has +// `gitProviders:create` in their own org (so withPermission passes) and knows a +// victim's Id calls .update with the victim id plus their own +// gitProviderId. The handler must resolve the row's owning git_provider via +// find*ById and run assertGitProviderAccess BEFORE mutating, so: +// - cross-org updates are rejected (NOT_FOUND); +// - updateGitProvider is no longer fed organizationId (cannot move a provider +// across orgs); +// - the service layer never repoints gitProviderId (FK-repoint mechanism closed). + +const mockFindGiteaById = vi.hoisted(() => vi.fn()); +const mockFindGithubById = vi.hoisted(() => vi.fn()); +const mockFindGitlabById = vi.hoisted(() => vi.fn()); +const mockUpdateGitProvider = vi.hoisted(() => + vi.fn().mockResolvedValue(undefined), +); + +vi.mock("@dokploy/server", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + findGiteaById: mockFindGiteaById, + findGithubById: mockFindGithubById, + findGitlabById: mockFindGitlabById, + updateGitProvider: mockUpdateGitProvider, + }; +}); + +vi.mock("@dokploy/server/services/permission", async (importOriginal) => { + const actual = + await importOriginal< + typeof import("@dokploy/server/services/permission") + >(); + return { ...actual, checkPermission: vi.fn().mockResolvedValue(undefined) }; +}); + +const mockHasValidLicense = vi.hoisted(() => vi.fn().mockResolvedValue(false)); +vi.mock("@dokploy/server/services/proprietary/license-key", () => ({ + hasValidLicense: mockHasValidLicense, +})); + +// Real `updateGitProvider` (kept real) and `assertGitProviderAccess`/`update*` +// (kept real via the barrel) run against this db mock. `update` captures the +// `.set()` arg so we can prove the service never repoints the FK, while `query` +// feeds `getAccessibleGitProviderIds` for the same-org success path. +const mocks = vi.hoisted(() => { + const capture = { setArgs: [] as unknown[] }; + const dbSetMock = vi.fn((values: unknown) => { + capture.setArgs.push(values); + return { where: () => ({ returning: () => Promise.resolve([{}]) }) }; + }); + const gitProviderFindMany = vi.fn(() => + Promise.resolve([ + { + gitProviderId: "gp-1", + userId: "user-1", + sharedWithOrganization: false, + }, + ]), + ); + const memberFindFirst = vi.fn(() => + Promise.resolve({ role: "owner", accessedGitProviders: [] }), + ); + const db = { + update: vi.fn(() => ({ set: dbSetMock })), + query: { + gitProvider: { + findMany: gitProviderFindMany, + findFirst: vi.fn(() => Promise.resolve(undefined)), + }, + member: { + findFirst: memberFindFirst, + findMany: vi.fn(() => Promise.resolve([])), + }, + }, + }; + return { capture, dbSetMock, db }; +}); + +vi.mock("@dokploy/server/db", () => ({ db: mocks.db })); + +vi.mock("@dokploy/server/lib/auth", () => ({ + validateRequest: vi.fn().mockResolvedValue({ session: null, user: null }), +})); + +const mockAudit = vi.hoisted(() => vi.fn().mockResolvedValue(undefined)); +vi.mock("@/server/api/utils/audit", () => ({ audit: mockAudit })); + +import { giteaRouter } from "@/server/api/routers/gitea"; +import { githubRouter } from "@/server/api/routers/github"; +import { gitlabRouter } from "@/server/api/routers/gitlab"; + +const ORG = "org-1"; +const USER = "user-1"; + +const ownProvider = { + gitProviderId: "gp-1", + organizationId: ORG, + userId: USER, + name: "mine", + providerType: "gitea", + createdAt: "", + sharedWithOrganization: false, +}; + +const foreignProvider = { + gitProviderId: "gp-victim", + organizationId: "org-2", + userId: "user-2", + name: "victim", + providerType: "gitea", + createdAt: "", + sharedWithOrganization: false, +}; + +const buildCtx = () => + ({ + session: { userId: USER, activeOrganizationId: ORG }, + user: { id: USER, email: "owner@test.com", role: "owner" }, + db: mocks.db, + req: {}, + res: {}, + }) as unknown as Parameters<(typeof giteaRouter)["createCaller"]>[0]; + +beforeEach(() => { + vi.clearAllMocks(); + mocks.capture.setArgs.length = 0; + mockFindGiteaById.mockResolvedValue({ + giteaId: "gitea-1", + gitProvider: ownProvider, + }); + mockFindGithubById.mockResolvedValue({ + githubId: "gh-1", + gitProvider: ownProvider, + }); + mockFindGitlabById.mockResolvedValue({ + gitlabId: "gl-1", + gitProvider: ownProvider, + }); +}); + +describe("gitea.update — cross-org IDOR guard", () => { + const caller = () => giteaRouter.createCaller(buildCtx()); + + it("rejects a cross-org update (victim giteaId + attacker gitProviderId)", async () => { + mockFindGiteaById.mockResolvedValue({ + giteaId: "gitea-victim", + gitProvider: foreignProvider, + }); + + const err = await caller() + .update({ + giteaId: "gitea-victim", + gitProviderId: "gp-attacker", + name: "stolen", + giteaUrl: "https://gitea.com", + }) + .catch((e) => e); + + expect(err).toBeInstanceOf(TRPCError); + expect(err).toMatchObject({ code: "NOT_FOUND" }); + expect(mockUpdateGitProvider).not.toHaveBeenCalled(); + expect(mocks.dbSetMock).not.toHaveBeenCalled(); + expect(mockAudit).not.toHaveBeenCalled(); + }); + + it("does not pass organizationId to updateGitProvider", async () => { + await caller().update({ + giteaId: "gitea-1", + gitProviderId: "gp-1", + name: "renamed", + giteaUrl: "https://gitea.com", + }); + + expect(mockUpdateGitProvider).toHaveBeenCalledWith("gp-1", { + name: "renamed", + }); + }); + + it("does not repoint the gitea row's gitProviderId via the service", async () => { + await caller().update({ + giteaId: "gitea-1", + gitProviderId: "gp-attacker", + name: "stolen", + giteaUrl: "https://gitea.com", + }); + + expect(mocks.dbSetMock).toHaveBeenCalledTimes(1); + const setArg = mocks.capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("giteaId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.giteaUrl).toBe("https://gitea.com"); + }); + + it("succeeds for a legitimate same-org own-provider update", async () => { + const result = await caller().update({ + giteaId: "gitea-1", + gitProviderId: "gp-1", + name: "renamed", + giteaUrl: "https://gitea.com", + }); + expect(result).toEqual({ success: true }); + expect(mockAudit).toHaveBeenCalledTimes(1); + }); +}); + +describe("github.update — cross-org IDOR guard", () => { + const caller = () => githubRouter.createCaller(buildCtx()); + + it("rejects a cross-org update", async () => { + mockFindGithubById.mockResolvedValue({ + githubId: "gh-victim", + gitProvider: foreignProvider, + }); + + await expect( + caller().update({ + githubId: "gh-victim", + gitProviderId: "gp-attacker", + name: "stolen", + githubAppName: "stolen-app", + }), + ).rejects.toMatchObject({ code: "NOT_FOUND" }); + + expect(mockUpdateGitProvider).not.toHaveBeenCalled(); + expect(mocks.dbSetMock).not.toHaveBeenCalled(); + }); + + it("does not pass organizationId to updateGitProvider", async () => { + await caller().update({ + githubId: "gh-1", + gitProviderId: "gp-1", + name: "renamed", + githubAppName: "my-app", + }); + + expect(mockUpdateGitProvider).toHaveBeenCalledWith("gp-1", { + name: "renamed", + }); + }); + + it("does not repoint the github row's gitProviderId via the service", async () => { + await caller().update({ + githubId: "gh-1", + gitProviderId: "gp-attacker", + name: "stolen", + githubAppName: "stolen-app", + }); + + expect(mocks.dbSetMock).toHaveBeenCalledTimes(1); + const setArg = mocks.capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("githubId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.githubAppName).toBe("stolen-app"); + }); +}); + +describe("gitlab.update — cross-org IDOR guard", () => { + const caller = () => gitlabRouter.createCaller(buildCtx()); + + it("rejects a cross-org update", async () => { + mockFindGitlabById.mockResolvedValue({ + gitlabId: "gl-victim", + gitProvider: foreignProvider, + }); + + await expect( + caller().update({ + gitlabId: "gl-victim", + gitProviderId: "gp-attacker", + name: "stolen", + gitlabUrl: "https://gitlab.com", + }), + ).rejects.toMatchObject({ code: "NOT_FOUND" }); + + expect(mockUpdateGitProvider).not.toHaveBeenCalled(); + expect(mocks.dbSetMock).not.toHaveBeenCalled(); + }); + + it("does not pass organizationId to updateGitProvider", async () => { + await caller().update({ + gitlabId: "gl-1", + gitProviderId: "gp-1", + name: "renamed", + gitlabUrl: "https://gitlab.com", + }); + + expect(mockUpdateGitProvider).toHaveBeenCalledWith("gp-1", { + name: "renamed", + }); + }); + + it("does not repoint the gitlab row's gitProviderId via the service", async () => { + await caller().update({ + gitlabId: "gl-1", + gitProviderId: "gp-attacker", + name: "stolen", + gitlabUrl: "https://gitlab.com", + }); + + expect(mocks.dbSetMock).toHaveBeenCalledTimes(1); + const setArg = mocks.capture.setArgs[0] as Record; + expect(setArg).not.toHaveProperty("gitlabId"); + expect(setArg).not.toHaveProperty("gitProviderId"); + expect(setArg.gitlabUrl).toBe("https://gitlab.com"); + }); +}); diff --git a/apps/dokploy/server/api/routers/gitea.ts b/apps/dokploy/server/api/routers/gitea.ts index 63cfc4e79..5eeb4fb85 100644 --- a/apps/dokploy/server/api/routers/gitea.ts +++ b/apps/dokploy/server/api/routers/gitea.ts @@ -187,10 +187,12 @@ export const giteaRouter = createTRPCRouter({ update: withPermission("gitProviders", "create") .input(apiUpdateGitea) .mutation(async ({ input, ctx }) => { + const gitea = await findGiteaById(input.giteaId); + await assertGitProviderAccess(ctx.session, gitea.gitProvider); + if (input.name) { await updateGitProvider(input.gitProviderId, { name: input.name, - organizationId: ctx.session.activeOrganizationId, }); await updateGitea(input.giteaId, { diff --git a/apps/dokploy/server/api/routers/github.ts b/apps/dokploy/server/api/routers/github.ts index dd765dee8..4b11363bc 100644 --- a/apps/dokploy/server/api/routers/github.ts +++ b/apps/dokploy/server/api/routers/github.ts @@ -106,9 +106,11 @@ export const githubRouter = createTRPCRouter({ update: withPermission("gitProviders", "create") .input(apiUpdateGithub) .mutation(async ({ input, ctx }) => { + const github = await findGithubById(input.githubId); + await assertGitProviderAccess(ctx.session, github.gitProvider); + await updateGitProvider(input.gitProviderId, { name: input.name, - organizationId: ctx.session.activeOrganizationId, }); await updateGithub(input.githubId, { diff --git a/apps/dokploy/server/api/routers/gitlab.ts b/apps/dokploy/server/api/routers/gitlab.ts index 133610763..8d232d810 100644 --- a/apps/dokploy/server/api/routers/gitlab.ts +++ b/apps/dokploy/server/api/routers/gitlab.ts @@ -136,10 +136,12 @@ export const gitlabRouter = createTRPCRouter({ update: withPermission("gitProviders", "create") .input(apiUpdateGitlab) .mutation(async ({ input, ctx }) => { + const gitlab = await findGitlabById(input.gitlabId); + await assertGitProviderAccess(ctx.session, gitlab.gitProvider); + if (input.name) { await updateGitProvider(input.gitProviderId, { name: input.name, - organizationId: ctx.session.activeOrganizationId, }); await updateGitlab(input.gitlabId, { diff --git a/packages/server/src/services/gitea.ts b/packages/server/src/services/gitea.ts index e3897d702..6a7cfad9b 100644 --- a/packages/server/src/services/gitea.ts +++ b/packages/server/src/services/gitea.ts @@ -82,9 +82,14 @@ export const findGiteaById = async (giteaId: string) => { export const updateGitea = async (giteaId: string, input: Partial) => { try { + const { + giteaId: _giteaId, + gitProviderId: _gitProviderId, + ...editable + } = input; const updateResult = await db .update(gitea) - .set(input) + .set(editable) .where(eq(gitea.giteaId, giteaId)) .returning(); diff --git a/packages/server/src/services/github.ts b/packages/server/src/services/github.ts index 07ce8cf4a..c5ecef51a 100644 --- a/packages/server/src/services/github.ts +++ b/packages/server/src/services/github.ts @@ -68,11 +68,14 @@ export const updateGithub = async ( githubId: string, input: Partial, ) => { + const { + githubId: _githubId, + gitProviderId: _gitProviderId, + ...editable + } = input; return await db .update(github) - .set({ - ...input, - }) + .set(editable) .where(eq(github.githubId, githubId)) .returning() .then((response) => response[0]); diff --git a/packages/server/src/services/gitlab.ts b/packages/server/src/services/gitlab.ts index 2b62bdd09..b5c8e60d3 100644 --- a/packages/server/src/services/gitlab.ts +++ b/packages/server/src/services/gitlab.ts @@ -67,11 +67,14 @@ export const updateGitlab = async ( gitlabId: string, input: Partial, ) => { + const { + gitlabId: _gitlabId, + gitProviderId: _gitProviderId, + ...editable + } = input; return await db .update(gitlab) - .set({ - ...input, - }) + .set(editable) .where(eq(gitlab.gitlabId, gitlabId)) .returning() .then((response) => response[0]);