From 063ad24c77bd00b744e885c46fcb3e9d028a0edf Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 08:22:40 +0100 Subject: [PATCH 1/6] Require deleteMetadata to metadata manager --- .changeset/tangy-rings-retire.md | 5 ++++ src/auth/verify-signature.test.ts | 24 +++++++++++++++++++ src/auth/verify-signature.ts | 6 ++--- .../shared/saleor-webhook-validator.test.ts | 1 - src/settings-manager/metadata-manager.ts | 6 +---- 5 files changed, 32 insertions(+), 10 deletions(-) create mode 100644 .changeset/tangy-rings-retire.md create mode 100644 src/auth/verify-signature.test.ts diff --git a/.changeset/tangy-rings-retire.md b/.changeset/tangy-rings-retire.md new file mode 100644 index 00000000..27a936f4 --- /dev/null +++ b/.changeset/tangy-rings-retire.md @@ -0,0 +1,5 @@ +--- +"@saleor/app-sdk": major +--- + +MetadataManager now requires deleteMetadata to be defined diff --git a/src/auth/verify-signature.test.ts b/src/auth/verify-signature.test.ts new file mode 100644 index 00000000..d1ff3327 --- /dev/null +++ b/src/auth/verify-signature.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; + +import { verifySignatureWithJwks } from "@/auth/verify-signature"; + +/** + * Actual signature copied from testing webhook payload. + */ +const testSignature = + "eyJhbGciOiJSUzI1NiIsImI2NCI6ZmFsc2UsImNyaXQiOlsiYjY0Il0sImlzcyI6Imh0dHBzOi8vaGFja2F0aG9uLXNoaXBwaW5nLmV1LnNhbGVvci5jbG91ZC9ncmFwaHFsLyIsImtpZCI6InlLdzlYeUVmOHgyYnd5SkdCWVlTckhzeXpHaHhKaktLdlpkb2tOZTlGSWMiLCJ0eXAiOiJKV1QifQ..ShPbfvYc_A5Aq3hiT6sisDclKikDkhxOvGXT2ZWgdsGRjZpg9ukiHRZym0kbfMfDqU5C3Pfo6n7am0ExwbnFWBfOil3pfe3uJOcOn_UGRj76Fy-59TB0JdS_WuTgNQcYM8Yjvlq2sNK4jdAfJVRTTx8FVgEpFrHBKmcMPfD7zuDozswIDMZOkklYqBcyQ76DJYIRVhl3QsktYPPrxDoqf-GJ--e9FuNqtqNDksP1weiDSraqXCF4-Ie7UWZsMIFxkPF8jdKjF_s1UmNS8Xel8soFQQ9L6Gps-NEv7xcHicGt5lgohH4mqhz1YIxCR7v_NTQgWImu_GQ6ELBiBSIZ2Q"; + +const rawContent = "{\"__typename\": \"OrderCreated\"}"; + +const jwks = + "{\"keys\": [{\"kty\": \"RSA\", \"key_ops\": [\"verify\"], \"n\": \"uDhbbpspufsQiqHsmC4kvmFQ5l2mGZsGcWhKVSQKQubSdXMedPpLnPD3Z3DsY76DILTm6WfOtSp5rr4KzF5wjurlOEhuFsB1HUfK9ZZB2nEDCQbweoIv3SOdclaNB__pYvQ0nmQHwsAeqH1QUuFUIvOL3t31rhjvzX6wvS49fGNb7rDlqQjufCvaX_n-ADJTgEAg6y1Mzn5NhgoTV1KTBeviyZqCdwvD6bk1ghN2XXWpNcARTzu3WHrmzIzkTwQeIMG8efwIddjfCaMGiOzAfzdQlqHlHPL1Xb5kV9AVX3kiSy-9shaQY23HdWwwiodrb4k2w34Z9ZZN-MHp8i6JdQ\", \"e\": \"AQAB\", \"use\": \"sig\", \"kid\": \"yKw9XyEf8x2bwyJGBYYSrHsyzGhxJjKKvZdokNe9FIc\"}]}"; + +describe("verifySignatureWithJwks", () => { + it("Returns empty promise if signature is valid", () => + expect(verifySignatureWithJwks(jwks, testSignature, rawContent)).resolves.not.toThrow()); + + it("Throws if signature is invalid", () => + expect( + verifySignatureWithJwks(jwks, testSignature, "{\"forged\": \"payload\"}"), + ).rejects.toThrow()); +}); diff --git a/src/auth/verify-signature.ts b/src/auth/verify-signature.ts index 0bd966a8..78c3c469 100644 --- a/src/auth/verify-signature.ts +++ b/src/auth/verify-signature.ts @@ -7,8 +7,6 @@ const debug = createDebug("verify-signature"); /** * Verify the Webhook payload signature from provided JWKS string. * JWKS can be cached to avoid unnecessary calls. - * - * TODO: Add test */ export const verifySignatureWithJwks = async (jwks: string, signature: string, rawBody: string) => { const [header, , jwsSignature] = signature.split("."); @@ -22,14 +20,14 @@ export const verifySignatureWithJwks = async (jwks: string, signature: string, r try { const parsedJWKS = JSON.parse(jwks); + localJwks = jose.createLocalJWKSet(parsedJWKS) as jose.FlattenedVerifyGetKey; } catch { debug("Could not create local JWKSSet from given data: %s", jwks); + throw new Error("JWKS verification failed - could not parse given JWKS"); } - debug("Created remote JWKS"); - try { await jose.flattenedVerify(jws, localJwks); debug("JWKS verified"); diff --git a/src/handlers/shared/saleor-webhook-validator.test.ts b/src/handlers/shared/saleor-webhook-validator.test.ts index 2d67446a..3c63278e 100644 --- a/src/handlers/shared/saleor-webhook-validator.test.ts +++ b/src/handlers/shared/saleor-webhook-validator.test.ts @@ -230,7 +230,6 @@ describe("SaleorWebhookValidator", () => { }); }); - // TODO: This should be required it("Fallbacks to null if version is missing in payload", async () => { vi.spyOn(adapter, "getRawBody").mockResolvedValue(JSON.stringify({})); vi.spyOn(requestProcessor, "getSaleorHeaders").mockReturnValue(validHeaders); diff --git a/src/settings-manager/metadata-manager.ts b/src/settings-manager/metadata-manager.ts index 439749ac..a338f8da 100644 --- a/src/settings-manager/metadata-manager.ts +++ b/src/settings-manager/metadata-manager.ts @@ -41,10 +41,6 @@ const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): Met export interface MetadataManagerConfig { fetchMetadata: FetchMetadataCallback; mutateMetadata: MutateMetadataCallback; - /** - * Keep it optional, to avoid breaking changes. If not provided, delete will throw. - * TODO: Make it required in next major version - */ deleteMetadata?: DeleteMetadataCallback; } @@ -98,7 +94,7 @@ export class MetadataManager implements SettingsManager { async delete(args: DeleteSettingsValue | DeleteSettingsValue[] | string | string[]) { if (!this.deleteMetadata) { throw new Error( - "Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor" + "Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor", ); } From 5317e6454df48fc69004608897eccd0953863797 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 10:59:04 +0100 Subject: [PATCH 2/6] Update metadata-manager.ts --- src/settings-manager/metadata-manager.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/settings-manager/metadata-manager.ts b/src/settings-manager/metadata-manager.ts index a338f8da..c4df261a 100644 --- a/src/settings-manager/metadata-manager.ts +++ b/src/settings-manager/metadata-manager.ts @@ -41,7 +41,7 @@ const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): Met export interface MetadataManagerConfig { fetchMetadata: FetchMetadataCallback; mutateMetadata: MutateMetadataCallback; - deleteMetadata?: DeleteMetadataCallback; + deleteMetadata: DeleteMetadataCallback; } /** @@ -58,7 +58,7 @@ export class MetadataManager implements SettingsManager { private mutateMetadata: MutateMetadataCallback; - private deleteMetadata?: DeleteMetadataCallback; + private deleteMetadata: DeleteMetadataCallback; constructor({ fetchMetadata, mutateMetadata, deleteMetadata }: MetadataManagerConfig) { this.fetchMetadata = fetchMetadata; @@ -85,7 +85,7 @@ export class MetadataManager implements SettingsManager { } // changes should update cache const metadata = await this.mutateMetadata(serializedMetadata); - this.settings = metadata.map(deserializeMetadata); + this.settings = metadata.map(deserializeMetadata);? } /** From 0658a0ae2434d48c390f65588461da5680a4a11c Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 12:50:51 +0100 Subject: [PATCH 3/6] Update src/settings-manager/metadata-manager.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Krzysztof Żuraw <9116238+krzysztofzuraw@users.noreply.github.com> --- src/settings-manager/metadata-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/settings-manager/metadata-manager.ts b/src/settings-manager/metadata-manager.ts index c4df261a..78164148 100644 --- a/src/settings-manager/metadata-manager.ts +++ b/src/settings-manager/metadata-manager.ts @@ -85,7 +85,7 @@ export class MetadataManager implements SettingsManager { } // changes should update cache const metadata = await this.mutateMetadata(serializedMetadata); - this.settings = metadata.map(deserializeMetadata);? + this.settings = metadata.map(deserializeMetadata); } /** From fbc7492bbf44acf10006a267f7c6d7993bd54dc7 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 13:40:04 +0100 Subject: [PATCH 4/6] fix lint --- src/settings-manager/encrypted-metadata-manager.test.ts | 6 ++++-- src/settings-manager/encrypted-metadata-manager.ts | 4 ++-- src/settings-manager/metadata-manager.test.ts | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/settings-manager/encrypted-metadata-manager.test.ts b/src/settings-manager/encrypted-metadata-manager.test.ts index b6b2fc0e..52269392 100644 --- a/src/settings-manager/encrypted-metadata-manager.test.ts +++ b/src/settings-manager/encrypted-metadata-manager.test.ts @@ -40,9 +40,9 @@ describe("settings-manager", () => { mutateMetadata: mutateMock, // @ts-expect-error encryptionKey: undefined, - }) + }), ).toThrowError( - "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager" + "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager", ); }); @@ -70,6 +70,7 @@ describe("settings-manager", () => { fetchMetadata: fetchMock, mutateMetadata: mutateMock, encryptionKey: "key", + deleteMetadata: deleteMetadataMock, }); beforeEach(() => { @@ -112,6 +113,7 @@ describe("settings-manager", () => { encryptionKey: "key", encryptionMethod: customEncrypt, decryptionMethod: customDecrypt, + deleteMetadata: deleteMetadataMock, }); await customManager.set(newEntry); diff --git a/src/settings-manager/encrypted-metadata-manager.ts b/src/settings-manager/encrypted-metadata-manager.ts index 83789f13..62618d44 100644 --- a/src/settings-manager/encrypted-metadata-manager.ts +++ b/src/settings-manager/encrypted-metadata-manager.ts @@ -80,11 +80,11 @@ export class EncryptedMetadataManager implements SettingsManager { if (process.env.NODE_ENV === "production") { console.error("Can't start the application without the secret key."); throw new Error( - "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager" + "Encryption key for the EncryptedMetadataManager has not been set. Setting it for the production environments is necessary. You can find more in the documentation: https://docs.saleor.io/docs/3.x/developer/extending/apps/developing-apps/app-sdk/settings-manager", ); } console.warn( - "WARNING: Encrypted Metadata Manager encryption key has not been set. For production deployments, it need's to be set" + "WARNING: Encrypted Metadata Manager encryption key has not been set. For production deployments, it need's to be set", ); console.warn("Using placeholder value for the development."); this.encryptionKey = "CHANGE_ME"; diff --git a/src/settings-manager/metadata-manager.test.ts b/src/settings-manager/metadata-manager.test.ts index 431cbd0a..65596c95 100644 --- a/src/settings-manager/metadata-manager.test.ts +++ b/src/settings-manager/metadata-manager.test.ts @@ -76,7 +76,7 @@ describe("settings-manager", () => { }); await expect(manager.delete("test")).rejects.toThrowErrorMatchingInlineSnapshot( - "[Error: Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor]" + "[Error: Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor]", ); }); From c3e1695d9a6f27b41ce88e8bdf6368434f58f670 Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 13:43:55 +0100 Subject: [PATCH 5/6] fix build --- src/settings-manager/metadata-manager.test.ts | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/settings-manager/metadata-manager.test.ts b/src/settings-manager/metadata-manager.test.ts index 65596c95..9583d4a9 100644 --- a/src/settings-manager/metadata-manager.test.ts +++ b/src/settings-manager/metadata-manager.test.ts @@ -21,7 +21,11 @@ describe("settings-manager", () => { }); it("Get method - using cached values", async () => { - const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock }); + const manager = new MetadataManager({ + fetchMetadata: fetchMock, + mutateMetadata: mutateMock, + deleteMetadata: deleteMetadataMock, + }); expect(fetchMock).toBeCalledTimes(0); // Fetch should be called just after getting a first value @@ -38,7 +42,11 @@ describe("settings-manager", () => { }); it("Get method - return values for chosen domain", async () => { - const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock }); + const manager = new MetadataManager({ + fetchMetadata: fetchMock, + mutateMetadata: mutateMock, + deleteMetadata: deleteMetadataMock, + }); expect(await manager.get("a", "x.com")).toBe(entryForDomainX.value); expect(await manager.get("a", "y.com")).toBe(entryForDomainY.value); @@ -46,7 +54,11 @@ describe("settings-manager", () => { }); it("Set method - return values for chosen domain", async () => { - const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock }); + const manager = new MetadataManager({ + fetchMetadata: fetchMock, + mutateMetadata: mutateMock, + deleteMetadata: deleteMetadataMock, + }); const newEntry = { key: "new", value: "new value" }; await manager.set(newEntry); @@ -64,6 +76,7 @@ describe("settings-manager", () => { const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock, + deleteMetadata: deleteMetadataMock, }); expect(manager).toBeDefined(); @@ -73,6 +86,7 @@ describe("settings-manager", () => { const manager = new MetadataManager({ fetchMetadata: fetchMock, mutateMetadata: mutateMock, + deleteMetadata: deleteMetadataMock, }); await expect(manager.delete("test")).rejects.toThrowErrorMatchingInlineSnapshot( From 9165c95307a46db7d0bc4e2e575161f0575ea02c Mon Sep 17 00:00:00 2001 From: Lukasz Ostrowski Date: Thu, 6 Mar 2025 13:45:01 +0100 Subject: [PATCH 6/6] fix build --- src/settings-manager/metadata-manager.test.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/settings-manager/metadata-manager.test.ts b/src/settings-manager/metadata-manager.test.ts index 9583d4a9..c90e3b06 100644 --- a/src/settings-manager/metadata-manager.test.ts +++ b/src/settings-manager/metadata-manager.test.ts @@ -82,18 +82,6 @@ describe("settings-manager", () => { expect(manager).toBeDefined(); }); - it("Throws if \"delete\" method is called, but deleteMetadata was not passed to constructor", async () => { - const manager = new MetadataManager({ - fetchMetadata: fetchMock, - mutateMetadata: mutateMock, - deleteMetadata: deleteMetadataMock, - }); - - await expect(manager.delete("test")).rejects.toThrowErrorMatchingInlineSnapshot( - "[Error: Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor]", - ); - }); - it("Calls deleteMetadata constructor param when \"delete\" method called", async () => { const manager = new MetadataManager({ fetchMetadata: fetchMock,