Skip to content

Commit

Permalink
Add optional URL protection to createRegisterHandler (#148)
Browse files Browse the repository at this point in the history
* validateAllowSaleorUrls impl

* Implement in handler and add test

* update codeowners

* Apply suggestions from code review

Co-authored-by: Krzysztof Wolski <[email protected]>

* Rename param

Co-authored-by: Krzysztof Wolski <[email protected]>
  • Loading branch information
lkostrowski and krzysztofwolski authored Jan 13, 2023
1 parent 715eb6a commit 67cded2
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 23 deletions.
2 changes: 1 addition & 1 deletion CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -1 +1 @@
* @saleor/devtools
* @saleor/marketplace
69 changes: 49 additions & 20 deletions src/handlers/next/create-app-register-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,29 @@ import { describe, expect, it, vi } from "vitest";
import { APL } from "../../APL";
import { createAppRegisterHandler } from "./create-app-register-handler";

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("../../get-app-id", () => ({
getAppId: vi.fn().mockResolvedValue("42"),
}));

vi.mock("../../fetch-remote-jwks", () => ({
fetchRemoteJwks: vi.fn().mockResolvedValue("{}"),
}));
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 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 () => {
const { res, req } = createMocks({
/**
* Use body, instead of params, otherwise - for some reason - param is not accessible in mock request
Expand Down Expand Up @@ -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);
});
});
29 changes: 27 additions & 2 deletions src/handlers/next/create-app-register-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string | ((saleorApiUrl: string) => 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) {
Expand Down
42 changes: 42 additions & 0 deletions src/handlers/next/validate-allow-saleor-urls.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
22 changes: 22 additions & 0 deletions src/handlers/next/validate-allow-saleor-urls.ts
Original file line number Diff line number Diff line change
@@ -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;
};

0 comments on commit 67cded2

Please sign in to comment.