From 93a453e901bff65304eb067577ff9dfb7cc9040d Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Mon, 19 May 2025 20:36:53 +0200 Subject: [PATCH 1/3] expose verifySignatureFn to be injected --- .changeset/fuzzy-rocks-kiss.md | 5 +++++ src/handlers/shared/generic-saleor-webhook.ts | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/fuzzy-rocks-kiss.md diff --git a/.changeset/fuzzy-rocks-kiss.md b/.changeset/fuzzy-rocks-kiss.md new file mode 100644 index 00000000..5f38cf8c --- /dev/null +++ b/.changeset/fuzzy-rocks-kiss.md @@ -0,0 +1,5 @@ +--- +"@saleor/app-sdk": patch +--- + +Allow to override signature verification method in SaleorWebhook class (likely for testing) diff --git a/src/handlers/shared/generic-saleor-webhook.ts b/src/handlers/shared/generic-saleor-webhook.ts index f031cece..c97f124f 100644 --- a/src/handlers/shared/generic-saleor-webhook.ts +++ b/src/handlers/shared/generic-saleor-webhook.ts @@ -1,6 +1,7 @@ import { ASTNode } from "graphql"; import { APL } from "@/APL"; +import { verifySignatureWithJwks } from "@/auth"; import { createDebug } from "@/debug"; import { gqlAstToString } from "@/gql-ast-to-string"; import { @@ -32,6 +33,7 @@ export interface GenericWebhookConfig< request: RequestType, ): Promise; query: string | ASTNode; + verifySignatureFn?: typeof verifySignatureWithJwks; } export abstract class GenericSaleorWebhook { @@ -55,6 +57,8 @@ export abstract class GenericSaleorWebhook { formatErrorResponse: GenericWebhookConfig["formatErrorResponse"]; + verifySignatureFn: typeof verifySignatureWithJwks; + protected constructor(configuration: GenericWebhookConfig) { const { name, webhookPath, event, query, apl, isActive = true } = configuration; @@ -66,6 +70,7 @@ export abstract class GenericSaleorWebhook { this.apl = apl; this.onError = configuration.onError; this.formatErrorResponse = configuration.formatErrorResponse; + this.verifySignatureFn = configuration.verifySignatureFn ?? verifySignatureWithJwks; } /** Gets webhook absolute URL based on baseUrl of app From 909cb5fd1068c32f7f44d559020a077650fcf7dc Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Mon, 19 May 2025 21:17:55 +0200 Subject: [PATCH 2/3] use injected values --- src/handlers/shared/generic-saleor-webhook.ts | 8 ++++++-- src/handlers/shared/saleor-webhook-validator.ts | 10 ++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/handlers/shared/generic-saleor-webhook.ts b/src/handlers/shared/generic-saleor-webhook.ts index c97f124f..9e3dffa4 100644 --- a/src/handlers/shared/generic-saleor-webhook.ts +++ b/src/handlers/shared/generic-saleor-webhook.ts @@ -37,8 +37,6 @@ export interface GenericWebhookConfig< } export abstract class GenericSaleorWebhook { - private webhookValidator = new SaleorWebhookValidator(); - protected abstract eventType: "async" | "sync"; name: string; @@ -59,6 +57,8 @@ export abstract class GenericSaleorWebhook { verifySignatureFn: typeof verifySignatureWithJwks; + private webhookValidator: SaleorWebhookValidator; + protected constructor(configuration: GenericWebhookConfig) { const { name, webhookPath, event, query, apl, isActive = true } = configuration; @@ -71,6 +71,10 @@ export abstract class GenericSaleorWebhook { this.onError = configuration.onError; this.formatErrorResponse = configuration.formatErrorResponse; this.verifySignatureFn = configuration.verifySignatureFn ?? verifySignatureWithJwks; + + this.webhookValidator = new SaleorWebhookValidator({ + verifySignatureFn: this.verifySignatureFn, + }); } /** Gets webhook absolute URL based on baseUrl of app diff --git a/src/handlers/shared/saleor-webhook-validator.ts b/src/handlers/shared/saleor-webhook-validator.ts index 681fbea4..c445effa 100644 --- a/src/handlers/shared/saleor-webhook-validator.ts +++ b/src/handlers/shared/saleor-webhook-validator.ts @@ -17,6 +17,12 @@ type WebhookValidationResult = | { result: "failure"; error: WebhookError | Error }; export class SaleorWebhookValidator { + constructor( + private params: { verifySignatureFn: typeof verifySignatureWithJwks } = { + verifySignatureFn: verifySignatureWithJwks, + }, + ) {} + private debug = createDebug("processProtectedHandler"); private tracer = getOtelTracer(); @@ -157,7 +163,7 @@ export class SaleorWebhookValidator { throw new Error("JWKS not found in AuthData"); } - await verifySignatureWithJwks(authData.jwks, signature, rawBody); + await this.params.verifySignatureFn(authData.jwks, signature, rawBody); } catch { this.debug("Request signature check failed. Refresh the JWKS cache and check again"); @@ -177,7 +183,7 @@ export class SaleorWebhookValidator { "Second attempt to validate the signature JWKS, using fresh tokens from the API", ); - await verifySignatureWithJwks(newJwks, signature, rawBody); + await this.params.verifySignatureFn(newJwks, signature, rawBody); this.debug("Verification successful - update JWKS in the AuthData"); From e77d3aec0a574e23cd9372e60a3532b77ca91150 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Fri, 23 May 2025 10:29:04 +0200 Subject: [PATCH 3/3] add tests --- .../saleor-sync-webhook.test.ts | 26 +++++++ src/handlers/shared/generic-saleor-webhook.ts | 5 ++ .../shared/saleor-webhook-validator.test.ts | 67 ++++++++++++++++--- .../shared/saleor-webhook-validator.ts | 18 ++--- 4 files changed, 98 insertions(+), 18 deletions(-) diff --git a/src/handlers/platforms/fetch-api/saleor-webhooks/saleor-sync-webhook.test.ts b/src/handlers/platforms/fetch-api/saleor-webhooks/saleor-sync-webhook.test.ts index bf0c29ee..25c5a920 100644 --- a/src/handlers/platforms/fetch-api/saleor-webhooks/saleor-sync-webhook.test.ts +++ b/src/handlers/platforms/fetch-api/saleor-webhooks/saleor-sync-webhook.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { buildSyncWebhookResponsePayload, FormatWebhookErrorResult } from "@/handlers/shared"; import { SaleorWebhookValidator } from "@/handlers/shared/saleor-webhook-validator"; +import { SALEOR_API_URL_HEADER, SALEOR_EVENT_HEADER, SALEOR_SIGNATURE_HEADER } from "@/headers"; import { MockAPL } from "@/test-utils/mock-apl"; import { SaleorSyncWebhook, WebApiSyncWebhookHandler } from "./saleor-sync-webhook"; @@ -125,4 +126,29 @@ describe("Web API SaleorSyncWebhook", () => { await expect(response.text()).resolves.toBe("Custom error"); expect(handler).not.toHaveBeenCalled(); }); + + it("should call externally injected signature verification function", async () => { + const mockVerifySignatureFn = vi.fn().mockImplementationOnce(async () => {}); + + const saleorSyncWebhook = new SaleorSyncWebhook({ + ...webhookConfiguration, + verifySignatureFn: mockVerifySignatureFn, + }); + + const handler = saleorSyncWebhook.createHandler(() => new Response("OK", { status: 200 })); + + const request = new Request(`${baseUrl}/webhook`, { + method: "POST", + headers: { + [SALEOR_API_URL_HEADER]: "https://example.com/graphql/", + [SALEOR_SIGNATURE_HEADER]: "random-signature-test", + [SALEOR_EVENT_HEADER]: "checkout_calculate_taxes", + }, + body: JSON.stringify({}), + }); + const response = await handler(request); + + expect(response.status).toBe(200); + expect(mockVerifySignatureFn).toHaveBeenCalledOnce(); + }); }); diff --git a/src/handlers/shared/generic-saleor-webhook.ts b/src/handlers/shared/generic-saleor-webhook.ts index 9e3dffa4..57c2947c 100644 --- a/src/handlers/shared/generic-saleor-webhook.ts +++ b/src/handlers/shared/generic-saleor-webhook.ts @@ -33,6 +33,11 @@ export interface GenericWebhookConfig< request: RequestType, ): Promise; query: string | ASTNode; + /** + * Allows to overwrite the default signature verification function. + * + * This is useful for testing purposes, when you want to fabricate a payload, or to opt-out from the default behavior from the library + */ verifySignatureFn?: typeof verifySignatureWithJwks; } diff --git a/src/handlers/shared/saleor-webhook-validator.test.ts b/src/handlers/shared/saleor-webhook-validator.test.ts index 3c63278e..e184ff49 100644 --- a/src/handlers/shared/saleor-webhook-validator.test.ts +++ b/src/handlers/shared/saleor-webhook-validator.test.ts @@ -9,17 +9,9 @@ import { MockAPL } from "@/test-utils/mock-apl"; import { SaleorRequestProcessor } from "./saleor-request-processor"; import { SaleorWebhookValidator } from "./saleor-webhook-validator"; -vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockImplementation( - async (domain, signature) => { - if (signature !== "mocked_signature") { - throw new Error("Wrong signature"); - } - }, -); - describe("SaleorWebhookValidator", () => { const mockAPL = new MockAPL(); - const validator = new SaleorWebhookValidator(); + let adapter: MockAdapter; let requestProcessor: SaleorRequestProcessor; @@ -38,6 +30,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on non-POST request method", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(adapter, "method", "get").mockReturnValue("GET"); const result = await validator.validateRequest({ @@ -57,6 +53,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on missing base URL", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(adapter, "getBaseUrl").mockReturnValue(""); const result = await validator.validateRequest({ allowedEvent: "PRODUCT_UPDATED", @@ -75,6 +75,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on missing api url header", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue({ ...validHeaders, // @ts-expect-error testing missing saleorApiUrl @@ -98,6 +102,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on missing event header", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue({ // @ts-expect-error testing missing event event: null, @@ -122,6 +130,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on mismatched event header", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue({ ...validHeaders, event: "different_event", @@ -144,6 +156,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on missing signature header", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue({ ...validHeaders, // @ts-expect-error testing missing signature @@ -167,6 +183,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on missing request body", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(adapter, "getRawBody").mockResolvedValue(""); vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders); @@ -187,6 +207,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on unparsable request body", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(adapter, "getRawBody").mockResolvedValue("{ "); // broken JSON vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders); @@ -207,6 +231,10 @@ describe("SaleorWebhookValidator", () => { }); it("Throws error on unregistered app", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + const unregisteredApiUrl = "https://not-registered.example.com/graphql/"; vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue({ @@ -231,6 +259,10 @@ describe("SaleorWebhookValidator", () => { }); it("Fallbacks to null if version is missing in payload", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(adapter, "getRawBody").mockResolvedValue(JSON.stringify({})); vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders); @@ -250,6 +282,10 @@ describe("SaleorWebhookValidator", () => { }); it("Returns success on valid request with signature passing validation against jwks in auth data", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); + + const validator = new SaleorWebhookValidator(); + vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders); const result = await validator.validateRequest({ @@ -284,10 +320,12 @@ describe("SaleorWebhookValidator", () => { }); it("Triggers JWKS refresh when initial auth data contains empty JWKS", async () => { + vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValue(undefined); vi.spyOn(mockAPL, "get").mockResolvedValue(authDataNoJwks); - vi.spyOn(verifySignatureModule, "verifySignatureWithJwks").mockResolvedValueOnce(undefined); vi.spyOn(fetchRemoteJwksModule, "fetchRemoteJwks").mockResolvedValue("new-jwks"); + const validator = new SaleorWebhookValidator(); + const result = await validator.validateRequest({ allowedEvent: "PRODUCT_UPDATED", apl: mockAPL, @@ -321,8 +359,11 @@ describe("SaleorWebhookValidator", () => { vi.spyOn(verifySignatureModule, "verifySignatureWithJwks") .mockRejectedValueOnce(new Error("Signature verification failed")) // First: reject validation due to stale jwks .mockResolvedValueOnce(undefined); // Second: resolve validation because jwks is now correct + vi.spyOn(fetchRemoteJwksModule, "fetchRemoteJwks").mockResolvedValue("new-jwks"); + const validator = new SaleorWebhookValidator(); + const result = await validator.validateRequest({ allowedEvent: "PRODUCT_UPDATED", apl: mockAPL, @@ -330,6 +371,8 @@ describe("SaleorWebhookValidator", () => { requestProcessor, }); + console.error(result); + expect(result).toMatchObject({ result: "ok", context: expect.objectContaining({ @@ -360,6 +403,8 @@ describe("SaleorWebhookValidator", () => { new Error("JWKS fetch failed"), ); + const validator = new SaleorWebhookValidator(); + const result = await validator.validateRequest({ allowedEvent: "PRODUCT_UPDATED", apl: mockAPL, @@ -383,6 +428,8 @@ describe("SaleorWebhookValidator", () => { .mockRejectedValueOnce(new Error("Fresh JWKS mismatch")); // Second attempt fails vi.spyOn(fetchRemoteJwksModule, "fetchRemoteJwks").mockResolvedValue("{}"); + const validator = new SaleorWebhookValidator(); + const result = await validator.validateRequest({ allowedEvent: "PRODUCT_UPDATED", apl: mockAPL, diff --git a/src/handlers/shared/saleor-webhook-validator.ts b/src/handlers/shared/saleor-webhook-validator.ts index c445effa..3ed61245 100644 --- a/src/handlers/shared/saleor-webhook-validator.ts +++ b/src/handlers/shared/saleor-webhook-validator.ts @@ -2,12 +2,12 @@ import { SpanKind, SpanStatusCode } from "@opentelemetry/api"; import { APL } from "@/APL"; import { fetchRemoteJwks } from "@/auth/fetch-remote-jwks"; -import { verifySignatureWithJwks } from "@/auth/verify-signature"; import { createDebug } from "@/debug"; import { getOtelTracer } from "@/open-telemetry"; import { SaleorSchemaVersion } from "@/types"; import { parseSchemaVersion } from "@/util/schema-version"; +import { verifySignatureWithJwks } from "../../auth/verify-signature"; import { PlatformAdapterInterface } from "./generic-adapter-use-case-types"; import { SaleorRequestProcessor } from "./saleor-request-processor"; import { WebhookContext, WebhookError } from "./saleor-webhook"; @@ -17,11 +17,13 @@ type WebhookValidationResult = | { result: "failure"; error: WebhookError | Error }; export class SaleorWebhookValidator { - constructor( - private params: { verifySignatureFn: typeof verifySignatureWithJwks } = { - verifySignatureFn: verifySignatureWithJwks, - }, - ) {} + private verifySignatureWithJwks = verifySignatureWithJwks.bind(this); + + constructor(params?: { verifySignatureFn: typeof verifySignatureWithJwks }) { + if (params?.verifySignatureFn) { + this.verifySignatureWithJwks = params.verifySignatureFn; + } + } private debug = createDebug("processProtectedHandler"); @@ -163,7 +165,7 @@ export class SaleorWebhookValidator { throw new Error("JWKS not found in AuthData"); } - await this.params.verifySignatureFn(authData.jwks, signature, rawBody); + await this.verifySignatureWithJwks(authData.jwks, signature, rawBody); } catch { this.debug("Request signature check failed. Refresh the JWKS cache and check again"); @@ -183,7 +185,7 @@ export class SaleorWebhookValidator { "Second attempt to validate the signature JWKS, using fresh tokens from the API", ); - await this.params.verifySignatureFn(newJwks, signature, rawBody); + await this.verifySignatureWithJwks(newJwks, signature, rawBody); this.debug("Verification successful - update JWKS in the AuthData");