From 67cded2e2aa5b923eb68abfbdf0bd45482e55bd7 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Fri, 13 Jan 2023 17:29:38 +0100 Subject: [PATCH] Add optional URL protection to createRegisterHandler (#148) * validateAllowSaleorUrls impl * Implement in handler and add test * update codeowners * Apply suggestions from code review Co-authored-by: Krzysztof Wolski * Rename param Co-authored-by: Krzysztof Wolski --- CODEOWNERS | 2 +- .../next/create-app-register-handler.test.ts | 71 +++++++++++++------ .../next/create-app-register-handler.ts | 29 +++++++- .../next/validate-allow-saleor-urls.test.ts | 42 +++++++++++ .../next/validate-allow-saleor-urls.ts | 22 ++++++ 5 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 src/handlers/next/validate-allow-saleor-urls.test.ts create mode 100644 src/handlers/next/validate-allow-saleor-urls.ts diff --git a/CODEOWNERS b/CODEOWNERS index 022a85a..9df6a49 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1 +1 @@ -* @saleor/devtools \ No newline at end of file +* @saleor/marketplace \ No newline at end of file diff --git a/src/handlers/next/create-app-register-handler.test.ts b/src/handlers/next/create-app-register-handler.test.ts index 08e43a9..5e1208f 100644 --- a/src/handlers/next/create-app-register-handler.test.ts +++ b/src/handlers/next/create-app-register-handler.test.ts @@ -4,29 +4,29 @@ import { describe, expect, it, vi } from "vitest"; import { APL } from "../../APL"; import { createAppRegisterHandler } from "./create-app-register-handler"; +vi.mock("../../get-app-id", () => ({ + getAppId: vi.fn().mockResolvedValue("42"), +})); + +vi.mock("../../fetch-remote-jwks", () => ({ + fetchRemoteJwks: vi.fn().mockResolvedValue("{}"), +})); + +const mockApl: APL = { + get: vi.fn(), + set: vi.fn(), + delete: vi.fn(), + getAll: vi.fn(), + isReady: vi.fn().mockImplementation(async () => ({ + ready: true, + })), + isConfigured: vi.fn().mockImplementation(async () => ({ + configured: true, + })), +}; + describe("create-app-register-handler", () => { it("Sets auth data for correct request", async () => { - vi.mock("../../get-app-id", () => ({ - getAppId: vi.fn().mockResolvedValue("42"), - })); - - vi.mock("../../fetch-remote-jwks", () => ({ - fetchRemoteJwks: vi.fn().mockResolvedValue("{}"), - })); - - const mockApl: APL = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getAll: vi.fn(), - isReady: vi.fn().mockImplementation(async () => ({ - ready: true, - })), - isConfigured: vi.fn().mockImplementation(async () => ({ - configured: true, - })), - }; - const { res, req } = createMocks({ /** * Use body, instead of params, otherwise - for some reason - param is not accessible in mock request @@ -61,4 +61,33 @@ describe("create-app-register-handler", () => { jwks: "{}", }); }); + + it("Returns 403 if configured to work only for specific saleor URL and try to install on prohibited one", async () => { + const { res, req } = createMocks({ + /** + * Use body, instead of params, otherwise - for some reason - param is not accessible in mock request + * Maybe this is a bug https://github.com/howardabrams/node-mocks-http/blob/master/lib/mockRequest.js + */ + body: { + auth_token: "mock-auth-token", + }, + headers: { + host: "some-saleor-host.cloud", + "x-forwarded-proto": "https", + "saleor-api-url": "https://wrong-saleor-domain.saleor.cloud/graphql/", + "saleor-domain": "https://wrong-saleor-domain.saleor.cloud/", + }, + method: "POST", + }); + + const handler = createAppRegisterHandler({ + apl: mockApl, + allowedSaleorUrls: [(url: string) => url === "https://mock-saleor-domain.saleor.cloud"], + }); + + await handler(req, res); + + expect(res._getStatusCode()).toBe(403); + expect(res._getData().success).toBe(false); + }); }); diff --git a/src/handlers/next/create-app-register-handler.ts b/src/handlers/next/create-app-register-handler.ts index 85995b6..86c22b5 100644 --- a/src/handlers/next/create-app-register-handler.ts +++ b/src/handlers/next/create-app-register-handler.ts @@ -9,23 +9,48 @@ import { fetchRemoteJwks } from "../../fetch-remote-jwks"; import { getAppId } from "../../get-app-id"; import { withAuthTokenRequired, withSaleorDomainPresent } from "../../middleware"; import { HasAPL } from "../../saleor-app"; +import { validateAllowSaleorUrls } from "./validate-allow-saleor-urls"; const debug = createDebug("createAppRegisterHandler"); -export type CreateAppRegisterHandlerOptions = HasAPL; +export type CreateAppRegisterHandlerOptions = HasAPL & { + /** + * Protect app from being registered in Saleor other than specific. + * By default, allow everything. + * + * Provide array of either a full Saleor API URL (eg. my-shop.saleor.cloud/graphql/) + * or a function that receives a full Saleor API URL ad returns true/false. + */ + allowedSaleorUrls?: Array boolean)>; +}; /** * Creates API handler for Next.js. Creates handler called by Saleor that registers app. * Hides implementation details if possible * In the future this will be extracted to separate sdk/next package */ -export const createAppRegisterHandler = ({ apl }: CreateAppRegisterHandlerOptions) => { +export const createAppRegisterHandler = ({ + apl, + allowedSaleorUrls, +}: CreateAppRegisterHandlerOptions) => { const baseHandler: Handler = async (request) => { debug("Request received"); const authToken = request.params.auth_token; const saleorDomain = request.headers[SALEOR_DOMAIN_HEADER] as string; const saleorApiUrl = request.headers[SALEOR_API_URL_HEADER] as string; + if (!validateAllowSaleorUrls(saleorApiUrl, allowedSaleorUrls)) { + debug("Validation of URL %s against allowSaleorUrls param resolves to false, throwing"); + + return Response.Forbidden({ + success: false, + error: { + code: "SALEOR_URL_PROHIBITED", + message: "This app expects to be installed only in allowed saleor instances", + }, + }); + } + const { configured: aplConfigured } = await apl.isConfigured(); if (!aplConfigured) { diff --git a/src/handlers/next/validate-allow-saleor-urls.test.ts b/src/handlers/next/validate-allow-saleor-urls.test.ts new file mode 100644 index 0000000..d4cfbbc --- /dev/null +++ b/src/handlers/next/validate-allow-saleor-urls.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "vitest"; + +import { validateAllowSaleorUrls } from "./validate-allow-saleor-urls"; + +const saleorCloudUrlMock = "https://my-shop.saleor.cloud/graphql/"; +const onPremiseSaleorUrlMock = "https://my-shop-123.aws-services.com/graphql/"; + +const saleorCloudRegexValidator = (url: string) => /https:\/\/.*.saleor.cloud\/graphql\//.test(url); + +describe("validateAllowSaleorUrls", () => { + it("Passes any URL if allow list is empty", () => { + expect(validateAllowSaleorUrls(saleorCloudUrlMock, [])).toBe(true); + expect(validateAllowSaleorUrls(onPremiseSaleorUrlMock, [])).toBe(true); + }); + + it("Passes only for URL that was exactly matched in provided allow list array", () => { + expect(validateAllowSaleorUrls(saleorCloudUrlMock, [saleorCloudUrlMock])).toBe(true); + expect(validateAllowSaleorUrls(onPremiseSaleorUrlMock, [saleorCloudUrlMock])).toBe(false); + }); + + it("Validates against custom function provided to allow list", () => { + expect(validateAllowSaleorUrls(saleorCloudUrlMock, [saleorCloudRegexValidator])).toBe(true); + expect(validateAllowSaleorUrls(onPremiseSaleorUrlMock, [saleorCloudRegexValidator])).toBe( + false + ); + }); + + it("Validates against more than one argument in allow list", () => { + expect( + validateAllowSaleorUrls(saleorCloudUrlMock, [ + saleorCloudRegexValidator, + onPremiseSaleorUrlMock, + ]) + ).toBe(true); + expect( + validateAllowSaleorUrls(onPremiseSaleorUrlMock, [ + saleorCloudRegexValidator, + onPremiseSaleorUrlMock, + ]) + ).toBe(true); + }); +}); diff --git a/src/handlers/next/validate-allow-saleor-urls.ts b/src/handlers/next/validate-allow-saleor-urls.ts new file mode 100644 index 0000000..9b7c39c --- /dev/null +++ b/src/handlers/next/validate-allow-saleor-urls.ts @@ -0,0 +1,22 @@ +import { CreateAppRegisterHandlerOptions } from "./create-app-register-handler"; + +export const validateAllowSaleorUrls = ( + saleorApiUrl: string, + allowedUrls: CreateAppRegisterHandlerOptions["allowedSaleorUrls"] +) => { + if (!allowedUrls || allowedUrls.length === 0) { + return true; + } + + for (const urlOrFn of allowedUrls) { + if (typeof urlOrFn === "string" && urlOrFn === saleorApiUrl) { + return true; + } + + if (typeof urlOrFn === "function" && urlOrFn(saleorApiUrl)) { + return true; + } + } + + return false; +};