diff --git a/packages/business/__tests__/platform-credential-service.test.ts b/packages/business/__tests__/platform-credential-service.test.ts index 407a99b646..ce10c26583 100644 --- a/packages/business/__tests__/platform-credential-service.test.ts +++ b/packages/business/__tests__/platform-credential-service.test.ts @@ -9,13 +9,37 @@ vi.mock("@chatbotx.io/database/client", () => ({ eq: vi.fn(), isNull: vi.fn(), })) +const credentialSchemas = { + instagram: { name: "instagram-schema" }, + messenger: { name: "messenger-schema" }, +} +const credentialPublicSchemas = { + messenger: { parse: vi.fn((value: unknown) => value) }, +} +const credentialEncryptedSchema = { + parse: vi.fn((value: unknown) => value), +} +// Mirrors the real credentialAad from @chatbotx.io/database/partials inline +// rather than via importOriginal (importing real database-package modules in +// vitest mocks risks opening a DB connection). The exact-string assertions in +// the upsert tests below lock this format to the real implementation's. +const credentialAad = (props: { + userId?: string | null + type: string + livemode: boolean +}) => + props.userId + ? `user:${props.userId}:${props.type}:${props.livemode}` + : `platform:${props.type}:${props.livemode}` vi.mock("@chatbotx.io/database/partials", () => ({ - credentialEncryptedSchema: {}, - credentialPublicSchemas: {}, - credentialSchemas: {}, + credentialAad, + credentialEncryptedSchema, + credentialPublicSchemas, + credentialSchemas, })) vi.mock("@chatbotx.io/database/schema", () => ({ platformCredentialModel: {} })) -vi.mock("@chatbotx.io/encryption", () => ({ encryptUtils: {} })) +const encryptUtils = { decryptObject: vi.fn(), encryptObject: vi.fn() } +vi.mock("@chatbotx.io/encryption", () => ({ encryptUtils })) vi.mock("@chatbotx.io/redis", () => ({ invalidateCacheByTags: vi.fn(async () => undefined), withCache: vi.fn(async (_key: string, fn: () => unknown) => fn()), @@ -39,6 +63,9 @@ beforeEach(() => { afterEach(() => { vi.restoreAllMocks() + encryptUtils.decryptObject.mockReset() + encryptUtils.encryptObject.mockReset() + credentialEncryptedSchema.parse.mockClear() }) describe("resolveForOwner", () => { @@ -229,3 +256,140 @@ describe("resolvePlatformAppAccessToken", () => { ).resolves.toBeUndefined() }) }) + +describe("_decrypt", () => { + // decryptObject takes no aad argument: it reads the aad off the blob + // itself (see packages/encryption/src/encryption.ts decryptText), so + // `_decrypt` doesn't need to re-derive the row-scoped aad the writers + // stamped — it just passes the blob and schema through. + test("decrypts a platform-scoped row", async () => { + encryptUtils.decryptObject.mockResolvedValue({ clientId: "client-1" }) + + await expect( + (platformCredentialService as never)._decrypt({ + id: "platform-1", + userId: null, + type: "instagram", + livemode: false, + value: { encrypted: true }, + publicConfig: { clientId: "client-1" }, + createdAt: new Date("2026-08-13T00:00:00.000Z"), + updatedAt: new Date("2026-08-13T00:00:00.000Z"), + }), + ).resolves.toEqual( + expect.objectContaining({ + id: "platform-1", + userId: null, + type: "instagram", + config: { clientId: "client-1" }, + }), + ) + + expect(encryptUtils.decryptObject).toHaveBeenCalledTimes(1) + expect(encryptUtils.decryptObject).toHaveBeenCalledWith( + { encrypted: true }, + credentialSchemas.instagram, + ) + }) + + test("decrypts a user-scoped row", async () => { + encryptUtils.decryptObject.mockResolvedValue({ clientId: "client-2" }) + + await expect( + (platformCredentialService as never)._decrypt({ + id: "user-1", + userId: "owner-1", + type: "messenger", + livemode: true, + value: { encrypted: true }, + publicConfig: { clientId: "client-2" }, + createdAt: new Date("2026-08-13T00:00:00.000Z"), + updatedAt: new Date("2026-08-13T00:00:00.000Z"), + }), + ).resolves.toEqual( + expect.objectContaining({ + id: "user-1", + userId: "owner-1", + type: "messenger", + config: { clientId: "client-2" }, + }), + ) + + expect(encryptUtils.decryptObject).toHaveBeenCalledTimes(1) + expect(encryptUtils.decryptObject).toHaveBeenCalledWith( + { encrypted: true }, + credentialSchemas.messenger, + ) + }) +}) + +// Fakes a minimal chainable drizzle-style tx sufficient for +// upsertForUser/upsertPlatform's `.insert().values().onConflictDoUpdate()` +// call shape, without needing a real DB client. +const fakeTx = () => { + const tx = { + insert: vi.fn(() => tx), + values: vi.fn(() => tx), + onConflictDoUpdate: vi.fn(() => Promise.resolve()), + } + return tx as unknown as Parameters< + typeof platformCredentialService.upsertForUser + >[0]["tx"] +} + +describe("upsertForUser / upsertPlatform write path", () => { + // Writers still stamp a row-derived aad at encrypt time — only decrypt + // stopped taking one. These assertions lock in that the derivation + // (user::: / platform::) still reaches + // encryptObject unchanged. + test("upsertForUser encrypts with a row-derived aad", async () => { + encryptUtils.encryptObject.mockResolvedValue({ + v: 1, + iv: "iv", + text: "text", + tag: "tag", + aad: "user:owner-1:messenger:false", + }) + vi.spyOn( + platformCredentialService, + "invalidateCacheTags", + ).mockResolvedValue(undefined) + + await platformCredentialService.upsertForUser({ + userId: "owner-1", + type: "messenger", + config: { clientId: "c", clientSecret: "s" } as never, + tx: fakeTx(), + }) + + expect(encryptUtils.encryptObject).toHaveBeenCalledWith( + { clientId: "c", clientSecret: "s" }, + "user:owner-1:messenger:false", + ) + }) + + test("upsertPlatform encrypts with a platform-scoped aad", async () => { + encryptUtils.encryptObject.mockResolvedValue({ + v: 1, + iv: "iv", + text: "text", + tag: "tag", + aad: "platform:messenger:false", + }) + vi.spyOn( + platformCredentialService, + "invalidateCacheTags", + ).mockResolvedValue(undefined) + + await platformCredentialService.upsertPlatform({ + type: "messenger", + config: { clientId: "c", clientSecret: "s" } as never, + tx: fakeTx(), + }) + + expect(encryptUtils.encryptObject).toHaveBeenCalledWith( + { clientId: "c", clientSecret: "s" }, + "platform:messenger:false", + ) + }) +}) diff --git a/packages/business/src/platform-credential/service.ts b/packages/business/src/platform-credential/service.ts index 671ca9ec7c..fe143fa15b 100644 --- a/packages/business/src/platform-credential/service.ts +++ b/packages/business/src/platform-credential/service.ts @@ -9,6 +9,7 @@ import { type CredentialByType, type CredentialPublicByType, type CredentialType, + credentialAad, credentialEncryptedSchema, credentialPublicSchemas, credentialSchemas, @@ -117,7 +118,7 @@ class PlatformCredentialService extends BaseService { tx = db, } = props const publicConfig = this._publicConfig(type, config) - const aad = `user:${userId}:${type}:${livemode}` + const aad = credentialAad({ userId, type, livemode }) const value = await encryptUtils.encryptObject(config, aad) await tx @@ -242,7 +243,7 @@ class PlatformCredentialService extends BaseService { }): Promise { const { type, config, livemode = false, tx = db } = props const publicConfig = this._publicConfig(type, config) - const aad = `platform:${type}:${livemode}` + const aad = credentialAad({ type, livemode }) const value = await encryptUtils.encryptObject(config, aad) await tx diff --git a/packages/database/__tests__/credential.test.ts b/packages/database/__tests__/credential.test.ts index d4611e46f6..3321a9ab69 100644 --- a/packages/database/__tests__/credential.test.ts +++ b/packages/database/__tests__/credential.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "vitest" import { + credentialAad, + credentialEncryptedSchema, giphyCredentialUpdateSchema, googleCredentialUpdateSchema, instagramCredentialUpdateSchema, @@ -141,3 +143,87 @@ describe("credential update schemas", () => { }) }) }) + +describe("credentialEncryptedSchema", () => { + const validBlob = { + v: 1 as const, + iv: "a".repeat(24), + text: "ciphertext-hex", + tag: "b".repeat(32), + aad: "user:1:messenger:false", + } + + test("accepts a blob with a non-empty aad", () => { + const result = credentialEncryptedSchema.safeParse(validBlob) + expect(result.success).toBe(true) + }) + + // aad is optional here, matching the transport schema: the writers stamp + // it and decrypt reads it back off the blob, so no reader needs to + // reconstruct or pass one, and a blob with no aad is a legitimate case + // rather than an error. + test("accepts a blob missing aad entirely", () => { + const { aad: _aad, ...blobWithoutAad } = validBlob + const result = credentialEncryptedSchema.safeParse(blobWithoutAad) + expect(result.success).toBe(true) + }) + + test("kid remains optional for legacy blobs predating key versioning", () => { + const { kid: _kid, ...blobWithoutKid } = { ...validBlob, kid: "k1" } + const result = credentialEncryptedSchema.safeParse(blobWithoutKid) + expect(result.success).toBe(true) + }) + + // These mirror the length/non-empty checks encryptedDataSchema enforces on + // the same fields (packages/encryption/src/encryption.ts). credentialEncryptedSchema + // is intentionally standalone rather than derived from that schema, so + // without these checks a malformed iv/text/tag would pass this parse and + // only fail later inside hexToBytes/WebCrypto with an opaque error instead + // of a clear validation failure at the storage boundary. + test("rejects a blob with a wrong-length iv", () => { + const result = credentialEncryptedSchema.safeParse({ + ...validBlob, + iv: "a".repeat(10), + }) + expect(result.success).toBe(false) + }) + + test("rejects a blob with an empty-string text", () => { + const result = credentialEncryptedSchema.safeParse({ + ...validBlob, + text: "", + }) + expect(result.success).toBe(false) + }) + + test("rejects a blob with a wrong-length tag", () => { + const result = credentialEncryptedSchema.safeParse({ + ...validBlob, + tag: "b".repeat(10), + }) + expect(result.success).toBe(false) + }) +}) + +describe("credentialAad", () => { + // Pins the exact aad format at its source. All writers (upsertForUser, + // upsertPlatform, rotate-encryption-key.ts) go through this helper; the + // business-package tests assert the same strings from the consumer side. + test("derives a user-scoped aad when userId is present", () => { + expect( + credentialAad({ userId: "owner-1", type: "messenger", livemode: true }), + ).toBe("user:owner-1:messenger:true") + }) + + test("derives a platform-scoped aad when userId is absent", () => { + expect(credentialAad({ type: "instagram", livemode: false })).toBe( + "platform:instagram:false", + ) + }) + + test("treats a null userId as platform-scoped", () => { + expect( + credentialAad({ userId: null, type: "messenger", livemode: false }), + ).toBe("platform:messenger:false") + }) +}) diff --git a/packages/database/scripts/rotate-encryption-key.ts b/packages/database/scripts/rotate-encryption-key.ts index 948370306a..f2a892e982 100644 --- a/packages/database/scripts/rotate-encryption-key.ts +++ b/packages/database/scripts/rotate-encryption-key.ts @@ -23,6 +23,7 @@ import { db } from "../src/client" import { type CredentialByType, type CredentialType, + credentialAad, credentialEncryptedSchema, credentialSchemas, } from "../src/partials/credential" @@ -41,15 +42,39 @@ const main = async (): Promise => { const rows = await db.select().from(platformCredentialModel) + const unparseable: (typeof rows)[number][] = [] const toRotate = rows.filter((row) => { const result = credentialEncryptedSchema.safeParse(row.value) - return result.success && result.data.kid !== activeKid + if (!result.success) { + unparseable.push(row) + return false + } + return result.data.kid !== activeKid }) console.log( `Found ${toRotate.length} of ${rows.length} rows to rotate → kid="${activeKid}".`, ) + if (unparseable.length > 0) { + // These rows fail schema validation (e.g. an unexpected `v`, or a + // malformed iv/text/tag) and are excluded from `toRotate` above. Left + // unrotated, they become permanently undecryptable once + // ENCRYPTION_KEY_PREV is removed, so surface them loudly here rather + // than dropping them without a trace. + console.error( + `Warning: ${unparseable.length} row(s) failed schema validation and ` + + "will NOT be rotated. They will become undecryptable once " + + "ENCRYPTION_KEY_PREV is removed. Investigate before completing " + + "rotation:", + ) + for (const row of unparseable) { + console.error( + ` [id:${row.id} userId:${row.userId ?? "platform"} type:${row.type}]`, + ) + } + } + if (isDryRun) { console.log("Dry run — no changes written.") return @@ -62,14 +87,16 @@ const main = async (): Promise => { try { const blob = credentialEncryptedSchema.parse(row.value) const type = row.type as CredentialType - const aad = row.userId - ? `user:${row.userId}:${type}:${row.livemode}` - : `platform:${type}:${row.livemode}` + const aad = credentialAad({ + userId: row.userId, + type, + livemode: row.livemode, + }) const schema = credentialSchemas[type] as unknown as z.ZodType< CredentialByType[CredentialType] > - const config = await encryptUtils.decryptObject(blob, schema, aad) + const config = await encryptUtils.decryptObject(blob, schema) const newValue = await encryptUtils.encryptObject(config, aad) await db diff --git a/packages/database/src/partials/credential.ts b/packages/database/src/partials/credential.ts index e244d305aa..f9354ac482 100644 --- a/packages/database/src/partials/credential.ts +++ b/packages/database/src/partials/credential.ts @@ -351,12 +351,38 @@ export type MakeCredentialUpdate = z.infer // ─── Encrypted blob shape stored in Credential.value ───────────────────────── +// Storage guard for Credential.value. Deliberately standalone rather than +// derived from @chatbotx.io/encryption's encryptedDataSchema (even though +// packages/database already depends on that package for other things), so +// the field constraints below are mirrored by hand and must be kept in sync +// with encryptedDataSchema if that shape ever changes. +// +// `iv`/`text`/`tag` mirror the length/non-empty checks encryptedDataSchema +// already enforces, so a malformed row fails loudly here instead of only +// surfacing later as an opaque hexToBytes/WebCrypto error. `aad` stays +// optional, matching the transport schema: it is stamped by the writers +// (upsertForUser, upsertPlatform, rotate-encryption-key.ts) and read back +// off the blob by decrypt — no caller needs to reconstruct or pass it. export const credentialEncryptedSchema = z.object({ v: z.literal(1), kid: z.string().optional(), - iv: z.string(), - text: z.string(), - tag: z.string(), + iv: z.string().length(24), + text: z.string().min(1), + tag: z.string().length(32), aad: z.string().optional(), }) export type CredentialEncrypted = z.infer + +// Derives the aad stamped onto Credential.value at write time +// (upsertForUser / upsertPlatform / rotate-encryption-key.ts). Decrypt reads +// the aad back off the blob, so this only needs to stay consistent across +// writers — but a drift between them would silently change what future rows +// are bound to, so all writers must go through this helper. +export const credentialAad = (props: { + userId?: string | null + type: CredentialType + livemode: boolean +}): string => + props.userId + ? `user:${props.userId}:${props.type}:${props.livemode}` + : `platform:${props.type}:${props.livemode}` diff --git a/packages/encryption/__tests__/encryption.test.ts b/packages/encryption/__tests__/encryption.test.ts index f3b5e7f989..ff5b4433e1 100644 --- a/packages/encryption/__tests__/encryption.test.ts +++ b/packages/encryption/__tests__/encryption.test.ts @@ -70,56 +70,39 @@ describe("encryptUtils", () => { }) describe("aad binding", () => { - test("encrypts and decrypts with matching aad", async () => { + // decryptText/decryptObject take no aad parameter: the aad a caller + // stamped at encrypt time travels with the blob and is read back + // automatically, so any holder of the blob can decrypt it without + // reconstructing the writer's context. + test("decrypts a blob that carries an aad without any caller input", async () => { const blob = await encryptUtils.encryptText("secret", "org:1:whatsapp") - expect(await encryptUtils.decryptText(blob, "org:1:whatsapp")).toBe( - "secret", - ) - }) - - test("decrypting with wrong aad throws", async () => { - const blob = await encryptUtils.encryptText("secret", "org:1:whatsapp") - await expect( - encryptUtils.decryptText(blob, "org:2:whatsapp"), - ).rejects.toThrow() + expect(blob.aad).toBe("org:1:whatsapp") + expect(await encryptUtils.decryptText(blob)).toBe("secret") }) - test("decrypting without aad falls back to the aad stored on the blob", async () => { - const blob = await encryptUtils.encryptText("secret", "org:1:whatsapp") + test("decrypts a blob with no aad the same way", async () => { + const blob = await encryptUtils.encryptText("secret") + expect(blob.aad).toBeUndefined() expect(await encryptUtils.decryptText(blob)).toBe("secret") }) - test("decrypting with aad throws when no aad was used at encryption", async () => { - const blob = await encryptUtils.encryptText("secret") - await expect( - encryptUtils.decryptText(blob, "org:1:whatsapp"), - ).rejects.toThrow() + // The aad is still authenticated by the GCM tag, not merely echoed back: + // rewriting it after the fact must invalidate decryption. This is the + // guarantee that remains once decrypt stops taking a caller-supplied aad. + test("tampering with the stored aad throws", async () => { + const blob = await encryptUtils.encryptText("secret", "org:1:whatsapp") + const tampered: EncryptedData = { ...blob, aad: "org:2:whatsapp" } + await expect(encryptUtils.decryptText(tampered)).rejects.toThrow() }) - test("encryptObject/decryptObject round-trip with matching aad", async () => { + test("encryptObject/decryptObject round-trip with an aad-carrying blob", async () => { const original = { clientId: "app_123", clientSecret: "s3cr3t" } const schema = z.object({ clientId: z.string(), clientSecret: z.string(), }) const blob = await encryptUtils.encryptObject(original, "org:1:messenger") - expect( - await encryptUtils.decryptObject(blob, schema, "org:1:messenger"), - ).toEqual(original) - }) - - test("decryptObject with wrong aad throws", async () => { - const schema = z.object({ - clientId: z.string(), - clientSecret: z.string(), - }) - const blob = await encryptUtils.encryptObject( - { clientId: "app_123", clientSecret: "s3cr3t" }, - "org:1:messenger", - ) - await expect( - encryptUtils.decryptObject(blob, schema, "org:9:messenger"), - ).rejects.toThrow() + expect(await encryptUtils.decryptObject(blob, schema)).toEqual(original) }) }) diff --git a/packages/encryption/src/appointment-token-utils.ts b/packages/encryption/src/appointment-token-utils.ts index 8b88772683..434c0940f2 100644 --- a/packages/encryption/src/appointment-token-utils.ts +++ b/packages/encryption/src/appointment-token-utils.ts @@ -16,10 +16,15 @@ export async function verifyAppointmentToken( ): Promise { const json = Buffer.from(token, "base64url").toString("utf8") const encrypted = encryptedDataSchema.parse(JSON.parse(json)) + // This comparison is now the SOLE enforcement of purpose separation: + // decryptObject reads its aad off the blob rather than taking one from the + // caller, so a schedule token would otherwise decrypt cleanly when + // verified as a cancel token. See appointment-tokens.test.ts "does not + // allow schedule tokens to be used as cancel tokens". if (encrypted.aad !== aad) { throw new Error("Appointment token type mismatch") } - const payload = await encryptUtils.decryptObject(encrypted, schema, aad) + const payload = await encryptUtils.decryptObject(encrypted, schema) const expiresAt = typeof payload === "object" && payload && "expiresAt" in payload ? payload.expiresAt diff --git a/packages/encryption/src/encryption.ts b/packages/encryption/src/encryption.ts index 650633de88..b0dd855301 100644 --- a/packages/encryption/src/encryption.ts +++ b/packages/encryption/src/encryption.ts @@ -123,10 +123,7 @@ export const encryptUtils = { } }, - decryptText: async ( - encryptedData: EncryptedData, - aad?: string, - ): Promise => { + decryptText: async (encryptedData: EncryptedData): Promise => { assertCurrentVersion(encryptedData.v) const key = await getKey(encryptedData.kid) const iv = hexToBytes(encryptedData.iv) @@ -135,9 +132,14 @@ export const encryptUtils = { hexToBytes(encryptedData.text), hexToBytes(encryptedData.tag), ) - const resolvedAad = aad ?? encryptedData.aad + // The aad travels with the blob, so any holder can decrypt without + // reconstructing the writer's context. AES-GCM still authenticates that + // aad against the ciphertext and tag — a tampered aad fails here. What + // this does NOT detect is an intact blob copied onto a different + // logical row; callers that need that binding must compare the aad + // themselves before decrypting (see appointment-token-utils.ts). const raw = await crypto.subtle.decrypt( - buildAlgorithm(iv, resolvedAad), + buildAlgorithm(iv, encryptedData.aad), key, combined, ) @@ -150,9 +152,8 @@ export const encryptUtils = { decryptObject: async ( encryptedData: EncryptedData, schema: z.ZodType, - aad?: string, ): Promise => { - const text = await encryptUtils.decryptText(encryptedData, aad) + const text = await encryptUtils.decryptText(encryptedData) const parsed: unknown = JSON.parse(text) return schema.parse(parsed) },