From c26357c34750155dea1ac7b141faa8075d9dbc70 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Sat, 15 Feb 2025 18:54:16 +0000 Subject: [PATCH 1/4] ensure app ownership middleware --- packages/server/src/api/routes/backup.ts | 3 + .../middleware/ensureTenantAppOwnership.ts | 16 +++++ .../tests/ensureTenantAppOwnership.spec.js | 61 +++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 packages/server/src/middleware/ensureTenantAppOwnership.ts create mode 100644 packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js diff --git a/packages/server/src/api/routes/backup.ts b/packages/server/src/api/routes/backup.ts index 94e4cf957f..c7f32d6043 100644 --- a/packages/server/src/api/routes/backup.ts +++ b/packages/server/src/api/routes/backup.ts @@ -2,12 +2,15 @@ import Router from "@koa/router" import * as controller from "../controllers/backup" import authorized from "../../middleware/authorized" import { permissions } from "@budibase/backend-core" +import { UserCtx } from "@budibase/types" +import ensureTenantAppOwnership from "../../middleware/ensureTenantAppOwnership" const router: Router = new Router() router.post( "/api/backups/export", authorized(permissions.BUILDER), + ensureTenantAppOwnership((ctx: UserCtx) => ctx.query.appId as string), controller.exportAppDump ) diff --git a/packages/server/src/middleware/ensureTenantAppOwnership.ts b/packages/server/src/middleware/ensureTenantAppOwnership.ts new file mode 100644 index 0000000000..3ad5ff2c97 --- /dev/null +++ b/packages/server/src/middleware/ensureTenantAppOwnership.ts @@ -0,0 +1,16 @@ +import { tenancy } from "@budibase/backend-core" +import { UserCtx } from "@budibase/types" + +function ensureTenantAppOwnership(appIdGetter: (ctx: UserCtx) => string) { + return async (ctx: UserCtx, next: any) => { + const appId = appIdGetter(ctx) + const exportAppId = tenancy.getTenantIDFromAppID(appId) + const tenantId = tenancy.getTenantId() + if (exportAppId !== tenantId) { + ctx.throw(403, `Cannot export app from another tenant`) + } + await next() + } +} + +export default ensureTenantAppOwnership diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js new file mode 100644 index 0000000000..ff53ed0b8f --- /dev/null +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -0,0 +1,61 @@ +import ensureTenantAppOwnership from "../ensureTenantAppOwnership" +import { tenancy } from "@budibase/backend-core" + +jest.mock("@budibase/backend-core", () => ({ + tenancy: { + getTenantId: jest.fn(), + getTenantIDFromAppID: jest.fn(), + }, +})) + +class TestConfiguration { + constructor() { + this.next = jest.fn() + this.throw = jest.fn() + this.middleware = ensureTenantAppOwnership(() => "app_123") + + this.ctx = { + next: this.next, + throw: this.throw, + } + } + + executeMiddleware() { + return this.middleware(this.ctx, this.next) + } + + afterEach() { + jest.clearAllMocks() + } +} + +describe("Ensure Tenant Ownership Middleware", () => { + let config + + beforeEach(() => { + config = new TestConfiguration() + }) + + afterEach(() => { + config.afterEach() + }) + + it("calls next() when tenant IDs match", async () => { + tenancy.getTenantIDFromAppID.mockReturnValue("tenant_1") + tenancy.getTenantId.mockReturnValue("tenant_1") + + await config.executeMiddleware() + expect(config.next).toHaveBeenCalled() + }) + + it("throws 403 when tenant IDs do not match", async () => { + tenancy.getTenantIDFromAppID.mockReturnValue("tenant_2") + tenancy.getTenantId.mockReturnValue("tenant_1") + + await config.executeMiddleware() + expect(config.throw).toHaveBeenCalledWith( + 403, + "Cannot export app from another tenant" + ) + }) +}) From b85f198fc2acb9341f17a49f1e05669edc99b80f Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Sat, 15 Feb 2025 19:06:21 +0000 Subject: [PATCH 2/4] useAppIdFromCtx helper --- packages/server/src/api/routes/backup.ts | 3 +-- .../middleware/ensureTenantAppOwnership.ts | 21 ++++++++-------- .../tests/ensureTenantAppOwnership.spec.js | 25 ++++++++++++++++--- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/packages/server/src/api/routes/backup.ts b/packages/server/src/api/routes/backup.ts index c7f32d6043..e94aeb8874 100644 --- a/packages/server/src/api/routes/backup.ts +++ b/packages/server/src/api/routes/backup.ts @@ -2,7 +2,6 @@ import Router from "@koa/router" import * as controller from "../controllers/backup" import authorized from "../../middleware/authorized" import { permissions } from "@budibase/backend-core" -import { UserCtx } from "@budibase/types" import ensureTenantAppOwnership from "../../middleware/ensureTenantAppOwnership" const router: Router = new Router() @@ -10,7 +9,7 @@ const router: Router = new Router() router.post( "/api/backups/export", authorized(permissions.BUILDER), - ensureTenantAppOwnership((ctx: UserCtx) => ctx.query.appId as string), + ensureTenantAppOwnership, controller.exportAppDump ) diff --git a/packages/server/src/middleware/ensureTenantAppOwnership.ts b/packages/server/src/middleware/ensureTenantAppOwnership.ts index 3ad5ff2c97..8f609f682a 100644 --- a/packages/server/src/middleware/ensureTenantAppOwnership.ts +++ b/packages/server/src/middleware/ensureTenantAppOwnership.ts @@ -1,16 +1,17 @@ -import { tenancy } from "@budibase/backend-core" +import { tenancy, utils } from "@budibase/backend-core" import { UserCtx } from "@budibase/types" -function ensureTenantAppOwnership(appIdGetter: (ctx: UserCtx) => string) { - return async (ctx: UserCtx, next: any) => { - const appId = appIdGetter(ctx) - const exportAppId = tenancy.getTenantIDFromAppID(appId) - const tenantId = tenancy.getTenantId() - if (exportAppId !== tenantId) { - ctx.throw(403, `Cannot export app from another tenant`) - } - await next() +async function ensureTenantAppOwnership(ctx: UserCtx, next: any) { + const appId = await utils.getAppIdFromCtx(ctx) + if (!appId) { + ctx.throw(400, "appId must be provided") } + const exportAppId = tenancy.getTenantIDFromAppID(appId) + const tenantId = tenancy.getTenantId() + if (exportAppId !== tenantId) { + ctx.throw(403, `Cannot export app from another tenant`) + } + await next() } export default ensureTenantAppOwnership diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js index ff53ed0b8f..b600c5a2f2 100644 --- a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -1,26 +1,31 @@ import ensureTenantAppOwnership from "../ensureTenantAppOwnership" -import { tenancy } from "@budibase/backend-core" +import { tenancy, utils } from "@budibase/backend-core" jest.mock("@budibase/backend-core", () => ({ tenancy: { getTenantId: jest.fn(), getTenantIDFromAppID: jest.fn(), }, + utils: { + getAppIdFromCtx: jest.fn(), + }, })) class TestConfiguration { - constructor() { + constructor(appId = "app_123") { this.next = jest.fn() this.throw = jest.fn() - this.middleware = ensureTenantAppOwnership(() => "app_123") + this.middleware = ensureTenantAppOwnership this.ctx = { next: this.next, throw: this.throw, } + + utils.getAppIdFromCtx.mockResolvedValue(appId) } - executeMiddleware() { + async executeMiddleware() { return this.middleware(this.ctx, this.next) } @@ -45,6 +50,8 @@ describe("Ensure Tenant Ownership Middleware", () => { tenancy.getTenantId.mockReturnValue("tenant_1") await config.executeMiddleware() + + expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) expect(config.next).toHaveBeenCalled() }) @@ -53,9 +60,19 @@ describe("Ensure Tenant Ownership Middleware", () => { tenancy.getTenantId.mockReturnValue("tenant_1") await config.executeMiddleware() + + expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) expect(config.throw).toHaveBeenCalledWith( 403, "Cannot export app from another tenant" ) }) + + it("throws 400 when appId is missing", async () => { + utils.getAppIdFromCtx.mockResolvedValue(null) + + await config.executeMiddleware() + + expect(config.throw).toHaveBeenCalledWith(400, "appId must be provided") + }) }) From d933e47754a17fbcd9421e0a4f652f349bc2aa51 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Sat, 15 Feb 2025 19:07:26 +0000 Subject: [PATCH 3/4] update error message to be more generic --- .../src/middleware/ensureTenantAppOwnership.ts | 5 ++--- .../tests/ensureTenantAppOwnership.spec.js | 16 +++++----------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/server/src/middleware/ensureTenantAppOwnership.ts b/packages/server/src/middleware/ensureTenantAppOwnership.ts index 8f609f682a..511342a0c2 100644 --- a/packages/server/src/middleware/ensureTenantAppOwnership.ts +++ b/packages/server/src/middleware/ensureTenantAppOwnership.ts @@ -6,10 +6,9 @@ async function ensureTenantAppOwnership(ctx: UserCtx, next: any) { if (!appId) { ctx.throw(400, "appId must be provided") } - const exportAppId = tenancy.getTenantIDFromAppID(appId) const tenantId = tenancy.getTenantId() - if (exportAppId !== tenantId) { - ctx.throw(403, `Cannot export app from another tenant`) + if (appId !== tenantId) { + ctx.throw(403, `App does not belong to tenant`) } await next() } diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js index b600c5a2f2..cd2696c40d 100644 --- a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -4,7 +4,6 @@ import { tenancy, utils } from "@budibase/backend-core" jest.mock("@budibase/backend-core", () => ({ tenancy: { getTenantId: jest.fn(), - getTenantIDFromAppID: jest.fn(), }, utils: { getAppIdFromCtx: jest.fn(), @@ -12,7 +11,7 @@ jest.mock("@budibase/backend-core", () => ({ })) class TestConfiguration { - constructor(appId = "app_123") { + constructor(appId = "tenant_1") { this.next = jest.fn() this.throw = jest.fn() this.middleware = ensureTenantAppOwnership @@ -45,8 +44,7 @@ describe("Ensure Tenant Ownership Middleware", () => { config.afterEach() }) - it("calls next() when tenant IDs match", async () => { - tenancy.getTenantIDFromAppID.mockReturnValue("tenant_1") + it("calls next() when appId matches tenant ID", async () => { tenancy.getTenantId.mockReturnValue("tenant_1") await config.executeMiddleware() @@ -55,17 +53,13 @@ describe("Ensure Tenant Ownership Middleware", () => { expect(config.next).toHaveBeenCalled() }) - it("throws 403 when tenant IDs do not match", async () => { - tenancy.getTenantIDFromAppID.mockReturnValue("tenant_2") - tenancy.getTenantId.mockReturnValue("tenant_1") + it("throws 403 when appId does not match tenant ID", async () => { + tenancy.getTenantId.mockReturnValue("tenant_2") await config.executeMiddleware() expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) - expect(config.throw).toHaveBeenCalledWith( - 403, - "Cannot export app from another tenant" - ) + expect(config.throw).toHaveBeenCalledWith(403, "App does not belong to tenant") }) it("throws 400 when appId is missing", async () => { From 267a915b9b3f24b76327d3d8039bfe8a0a242389 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Sat, 15 Feb 2025 19:13:10 +0000 Subject: [PATCH 4/4] lint --- .../src/middleware/tests/ensureTenantAppOwnership.spec.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js index cd2696c40d..817893681b 100644 --- a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -59,7 +59,10 @@ describe("Ensure Tenant Ownership Middleware", () => { await config.executeMiddleware() expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) - expect(config.throw).toHaveBeenCalledWith(403, "App does not belong to tenant") + expect(config.throw).toHaveBeenCalledWith( + 403, + "App does not belong to tenant" + ) }) it("throws 400 when appId is missing", async () => {