From c26357c34750155dea1ac7b141faa8075d9dbc70 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Sat, 15 Feb 2025 18:54:16 +0000 Subject: [PATCH 1/8] 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/8] 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/8] 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/8] 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 () => { From 257151dfa5b785244d46865e1e1c409a7214f6f7 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Sat, 15 Feb 2025 19:29:36 +0000 Subject: [PATCH 5/8] Bump version to 3.4.10 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index afcb33918b..e99b03a1ae 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "3.4.9", + "version": "3.4.10", "npmClient": "yarn", "concurrency": 20, "command": { From 06866111d3381a8c95876cb28cca412301d1d017 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Mon, 17 Feb 2025 10:32:12 +0000 Subject: [PATCH 6/8] fix error message and extract tenantId from appId --- packages/backend-core/src/utils/tests/utils.spec.ts | 9 +++++++++ packages/backend-core/src/utils/utils.ts | 5 +++++ .../server/src/middleware/ensureTenantAppOwnership.ts | 9 ++++++--- .../middleware/tests/ensureTenantAppOwnership.spec.js | 7 +++++-- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/utils/tests/utils.spec.ts b/packages/backend-core/src/utils/tests/utils.spec.ts index b5c6283ce2..c7c1e4334f 100644 --- a/packages/backend-core/src/utils/tests/utils.spec.ts +++ b/packages/backend-core/src/utils/tests/utils.spec.ts @@ -67,6 +67,15 @@ describe("utils", () => { }) }) + it("gets appId from query params", async () => { + const ctx = structures.koa.newContext() + const expected = db.generateAppID() + ctx.query = { appId: expected } + + const actual = await utils.getAppIdFromCtx(ctx) + expect(actual).toBe(expected) + }) + it("doesn't get appId from url when previewing", async () => { const ctx = structures.koa.newContext() const appId = db.generateAppID() diff --git a/packages/backend-core/src/utils/utils.ts b/packages/backend-core/src/utils/utils.ts index 7f2e25b6d4..fcec61179f 100644 --- a/packages/backend-core/src/utils/utils.ts +++ b/packages/backend-core/src/utils/utils.ts @@ -101,6 +101,11 @@ export async function getAppIdFromCtx(ctx: Ctx) { appId = confirmAppId(pathId) } + // look in queryParams + if (!appId && ctx.query?.appId) { + appId = confirmAppId(ctx.query?.appId as string) + } + // lookup using custom url - prod apps only // filter out the builder preview path which collides with the prod app path // to ensure we don't load all apps excessively diff --git a/packages/server/src/middleware/ensureTenantAppOwnership.ts b/packages/server/src/middleware/ensureTenantAppOwnership.ts index 511342a0c2..23f35f5cb8 100644 --- a/packages/server/src/middleware/ensureTenantAppOwnership.ts +++ b/packages/server/src/middleware/ensureTenantAppOwnership.ts @@ -1,4 +1,4 @@ -import { tenancy, utils } from "@budibase/backend-core" +import { tenancy, utils, context } from "@budibase/backend-core" import { UserCtx } from "@budibase/types" async function ensureTenantAppOwnership(ctx: UserCtx, next: any) { @@ -6,9 +6,12 @@ async function ensureTenantAppOwnership(ctx: UserCtx, next: any) { if (!appId) { ctx.throw(400, "appId must be provided") } + + const appTenantId = context.getTenantIDFromAppID(appId) const tenantId = tenancy.getTenantId() - if (appId !== tenantId) { - ctx.throw(403, `App does not belong to tenant`) + + if (appTenantId !== tenantId) { + ctx.throw(403, "Unauthorized") } await next() } diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js index 817893681b..e81dfc0398 100644 --- a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -2,6 +2,7 @@ import ensureTenantAppOwnership from "../ensureTenantAppOwnership" import { tenancy, utils } from "@budibase/backend-core" jest.mock("@budibase/backend-core", () => ({ + ...jest.requireActual("@budibase/backend-core"), tenancy: { getTenantId: jest.fn(), }, @@ -53,7 +54,9 @@ describe("Ensure Tenant Ownership Middleware", () => { expect(config.next).toHaveBeenCalled() }) - it("throws 403 when appId does not match tenant ID", async () => { + it("throws when tenant appId does not match tenant ID", async () => { + const appId = "app_dev_tenant3_fce449c4d75b4e4a9c7a6980d82a3e22" + utils.getAppIdFromCtx.mockResolvedValue(appId) tenancy.getTenantId.mockReturnValue("tenant_2") await config.executeMiddleware() @@ -61,7 +64,7 @@ describe("Ensure Tenant Ownership Middleware", () => { expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) expect(config.throw).toHaveBeenCalledWith( 403, - "App does not belong to tenant" + "Unauthorized", ) }) From 268f67b17cd0938d7a8be442b6852893f9a8635e Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Mon, 17 Feb 2025 10:36:26 +0000 Subject: [PATCH 7/8] lint --- .../src/middleware/tests/ensureTenantAppOwnership.spec.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js index e81dfc0398..5c500f8723 100644 --- a/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js +++ b/packages/server/src/middleware/tests/ensureTenantAppOwnership.spec.js @@ -62,10 +62,7 @@ describe("Ensure Tenant Ownership Middleware", () => { await config.executeMiddleware() expect(utils.getAppIdFromCtx).toHaveBeenCalledWith(config.ctx) - expect(config.throw).toHaveBeenCalledWith( - 403, - "Unauthorized", - ) + expect(config.throw).toHaveBeenCalledWith(403, "Unauthorized") }) it("throws 400 when appId is missing", async () => { From 4c54a58db43a457280fa3a6401c540c73ffa63be Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Mon, 17 Feb 2025 10:37:12 +0000 Subject: [PATCH 8/8] Bump version to 3.4.11 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index e99b03a1ae..d9d9e01bef 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "3.4.10", + "version": "3.4.11", "npmClient": "yarn", "concurrency": 20, "command": {