From 14390f919a1f94d0a222561acd6eed289322d09d Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Tue, 2 Dec 2025 13:06:29 +0000 Subject: [PATCH 1/2] fix(firestore-send-email): make attachment validation more lenient and more robust --- .../__tests__/prepare-payload.test.ts | 21 ++++++++++-- .../functions/__tests__/validation.test.ts | 14 +++----- .../functions/src/prepare-payload.ts | 32 ++++++++++++++++--- .../functions/src/validation.ts | 15 +++++++-- 4 files changed, 63 insertions(+), 19 deletions(-) diff --git a/firestore-send-email/functions/__tests__/prepare-payload.test.ts b/firestore-send-email/functions/__tests__/prepare-payload.test.ts index c9a7be394..2dff9dc04 100644 --- a/firestore-send-email/functions/__tests__/prepare-payload.test.ts +++ b/firestore-send-email/functions/__tests__/prepare-payload.test.ts @@ -398,7 +398,7 @@ describe("preparePayload Template Merging", () => { await expect(preparePayload(payload)).rejects.toThrow(); }); - it("should handle null attachments", async () => { + it("should handle null attachments as no attachments", async () => { const payload = { to: "test@example.com", message: { @@ -408,7 +408,24 @@ describe("preparePayload Template Merging", () => { }, }; - await expect(preparePayload(payload)).rejects.toThrow(); + const result = await preparePayload(payload); + expect(result.message.attachments).toBeUndefined(); + }); + + it("should normalize single attachment object to array", async () => { + const payload = { + to: "test@example.com", + message: { + subject: "Test Subject", + text: "Test text", + attachments: { filename: "test.txt", content: "test content" }, + }, + }; + + const result = await preparePayload(payload); + expect(result.message.attachments).toEqual([ + { filename: "test.txt", content: "test content" }, + ]); }); it("should handle undefined attachments", async () => { diff --git a/firestore-send-email/functions/__tests__/validation.test.ts b/firestore-send-email/functions/__tests__/validation.test.ts index 1d74fca27..731498ccc 100644 --- a/firestore-send-email/functions/__tests__/validation.test.ts +++ b/firestore-send-email/functions/__tests__/validation.test.ts @@ -601,22 +601,16 @@ describe("validatePayload", () => { ); }); - it("should throw error for non-array attachments", () => { - const invalidPayload = { + it("should accept single attachment object and normalize to array", () => { + const payload = { to: "test@example.com", message: { subject: "Test Subject", text: "Test message", - attachments: { - filename: "test.txt", - content: "test", - }, + attachments: { filename: "test.txt", content: "test" }, }, }; - expect(() => validatePayload(invalidPayload)).toThrow(ValidationError); - expect(() => validatePayload(invalidPayload)).toThrow( - "Invalid message configuration: Field 'message.attachments' must be an array" - ); + expect(() => validatePayload(payload)).not.toThrow(); }); }); }); diff --git a/firestore-send-email/functions/src/prepare-payload.ts b/firestore-send-email/functions/src/prepare-payload.ts index 5574c732d..ffa987bbf 100644 --- a/firestore-send-email/functions/src/prepare-payload.ts +++ b/firestore-send-email/functions/src/prepare-payload.ts @@ -3,6 +3,7 @@ import { validatePayload, attachmentSchema, attachmentsSchema, + ValidationError, } from "./validation"; import * as logs from "./logs"; import config from "./config"; @@ -41,11 +42,19 @@ export async function preparePayload( const templateRender = await templates.render(template.name, template.data); const mergeMessage = payload.message || {}; - let attachments = attachmentsSchema.parse( - templateRender.attachments - ? templateRender.attachments - : mergeMessage.attachments - ); + const attachmentsInput = templateRender.attachments + ? templateRender.attachments + : mergeMessage.attachments; + + const attachmentsResult = attachmentsSchema.safeParse(attachmentsInput); + if (!attachmentsResult.success) { + throw new ValidationError( + `Invalid attachments: ${attachmentsResult.error.issues + .map((i) => i.message) + .join(", ")}` + ); + } + let attachments = attachmentsResult.data; const handleTemplateValue = (value: any) => { if (value === null) { @@ -75,6 +84,19 @@ export async function preparePayload( }); payload.message = Object.assign(mergeMessage, templateContent); + } else if (payload.message?.attachments !== undefined) { + // Normalize attachments for non-template messages + const attachmentsResult = attachmentsSchema.safeParse( + payload.message.attachments + ); + if (!attachmentsResult.success) { + throw new ValidationError( + `Invalid attachments: ${attachmentsResult.error.issues + .map((i) => i.message) + .join(", ")}` + ); + } + payload.message.attachments = attachmentsResult.data; } let to: string[] = []; diff --git a/firestore-send-email/functions/src/validation.ts b/firestore-send-email/functions/src/validation.ts index 0d13af707..8a6be41bf 100644 --- a/firestore-send-email/functions/src/validation.ts +++ b/firestore-send-email/functions/src/validation.ts @@ -53,8 +53,19 @@ export const attachmentSchema = z }); export const attachmentsSchema = z - .array(attachmentSchema) - .optional() + .preprocess( + // Normalize inputs before validation: + // - null/undefined → undefined (no attachments) + // - single object → wrap in array + // - array → pass through + (val) => { + if (val === undefined || val === null) return undefined; + if (Array.isArray(val)) return val; + if (typeof val === "object") return [val]; + return val; // Let validation handle invalid types + }, + z.array(attachmentSchema).optional() + ) .transform((attachments) => attachments ? attachments.filter((attachment) => Object.keys(attachment).length > 0) From 963cb7b583e391d4247b35b97acdb5ab3ca1604f Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Tue, 2 Dec 2025 13:23:27 +0000 Subject: [PATCH 2/2] test(firestore-send-email): add edge case tests --- .../functions/__tests__/e2e/setup.ts | 32 ++++ .../__tests__/e2e/validation.test.ts | 144 ++++++++++++++++++ .../__tests__/prepare-payload.test.ts | 108 +++++++++++-- .../functions/__tests__/validation.test.ts | 89 +++++++++++ firestore-send-email/functions/jest.config.js | 1 + .../functions/jest.e2e.config.js | 13 ++ firestore-send-email/functions/package.json | 4 +- 7 files changed, 374 insertions(+), 17 deletions(-) create mode 100644 firestore-send-email/functions/__tests__/e2e/setup.ts create mode 100644 firestore-send-email/functions/__tests__/e2e/validation.test.ts create mode 100644 firestore-send-email/functions/jest.e2e.config.js diff --git a/firestore-send-email/functions/__tests__/e2e/setup.ts b/firestore-send-email/functions/__tests__/e2e/setup.ts new file mode 100644 index 000000000..be85783fe --- /dev/null +++ b/firestore-send-email/functions/__tests__/e2e/setup.ts @@ -0,0 +1,32 @@ +import * as admin from "firebase-admin"; + +export const TEST_COLLECTIONS = ["mail", "templates"] as const; + +// Initialize Firebase Admin once for all e2e tests +beforeAll(() => { + if (!admin.apps.length) { + admin.initializeApp({ projectId: "demo-test" }); + } + process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; +}); + +/** + * Clears all documents from test collections. + * Call this in beforeEach to ensure clean state between tests. + */ +export async function clearCollections() { + const db = admin.firestore(); + for (const collection of TEST_COLLECTIONS) { + const snapshot = await db.collection(collection).get(); + const batch = db.batch(); + snapshot.docs.forEach((doc) => batch.delete(doc.ref)); + await batch.commit(); + } +} + +/** + * Gets the test email address from environment or returns default. + */ +export function getTestEmail() { + return process.env.TEST_EMAIL || "test@example.com"; +} diff --git a/firestore-send-email/functions/__tests__/e2e/validation.test.ts b/firestore-send-email/functions/__tests__/e2e/validation.test.ts new file mode 100644 index 000000000..760da45c9 --- /dev/null +++ b/firestore-send-email/functions/__tests__/e2e/validation.test.ts @@ -0,0 +1,144 @@ +/** + * E2E tests for attachment validation edge cases. + * + * Tests that the extension handles various attachment formats gracefully: + * - Missing attachments field + * - Null attachments + * - Single attachment object (should normalize to array) + * - Empty objects in attachments array (should be filtered out) + * + * Run with: npm run test:e2e + */ + +import * as admin from "firebase-admin"; +import { clearCollections, getTestEmail } from "./setup"; + +const TEST_TEMPLATE = { + name: "validation_test_template", + subject: "Test Subject {{id}}", + text: "Test content for {{id}}", + html: "

Test content for {{id}}

", +}; + +describe.skip("Attachment validation edge cases", () => { + beforeEach(async () => { + await clearCollections(); + + const db = admin.firestore(); + await db.collection("templates").doc(TEST_TEMPLATE.name).set({ + subject: TEST_TEMPLATE.subject, + text: TEST_TEMPLATE.text, + html: TEST_TEMPLATE.html, + }); + }); + + test("should process template email without attachments field", async () => { + const db = admin.firestore(); + + const testData = { + template: { + name: TEST_TEMPLATE.name, + data: { id: "test-1" }, + }, + to: getTestEmail(), + }; + + const docRef = db.collection("mail").doc("test-no-attachments"); + await docRef.set(testData); + + await new Promise((resolve) => setTimeout(resolve, 2000)); + + const doc = await docRef.get(); + const updatedData = doc.data(); + + expect(updatedData?.delivery.state).toBe("SUCCESS"); + expect(updatedData?.delivery.error).toBeNull(); + }); + + test("should process template email with null message attachments", async () => { + const db = admin.firestore(); + + const testData = { + template: { + name: TEST_TEMPLATE.name, + data: { id: "test-2" }, + }, + message: { + attachments: null, + }, + to: getTestEmail(), + }; + + const docRef = db + .collection("emailCollection") + .doc("test-null-attachments"); + await docRef.set(testData); + + await new Promise((resolve) => setTimeout(resolve, 2000)); + + const doc = await docRef.get(); + const updatedData = doc.data(); + + expect(updatedData?.delivery.state).toBe("SUCCESS"); + expect(updatedData?.delivery.error).toBeNull(); + }); + + test("should normalize single attachment object to array", async () => { + const db = admin.firestore(); + + const testData = { + template: { + name: TEST_TEMPLATE.name, + data: { id: "test-3" }, + }, + message: { + attachments: { + filename: "test.txt", + content: "test content", + }, + }, + to: getTestEmail(), + }; + + const docRef = db + .collection("emailCollection") + .doc("test-object-attachment"); + await docRef.set(testData); + + await new Promise((resolve) => setTimeout(resolve, 2000)); + + const doc = await docRef.get(); + const updatedData = doc.data(); + + expect(updatedData?.delivery.state).toBe("SUCCESS"); + expect(updatedData?.delivery.error).toBeNull(); + }); + + test("should filter out empty objects in attachments array", async () => { + const db = admin.firestore(); + + const testData = { + template: { + name: TEST_TEMPLATE.name, + data: { id: "test-4" }, + }, + message: { + attachments: [{}], + }, + to: getTestEmail(), + }; + + const docRef = db + .collection("emailCollection") + .doc("test-empty-attachment"); + await docRef.set(testData); + + await new Promise((resolve) => setTimeout(resolve, 2000)); + + const doc = await docRef.get(); + const updatedData = doc.data(); + + expect(updatedData?.delivery.state).toBe("SUCCESS"); + expect(updatedData?.delivery.error).toBeNull(); + }); +}); diff --git a/firestore-send-email/functions/__tests__/prepare-payload.test.ts b/firestore-send-email/functions/__tests__/prepare-payload.test.ts index 2dff9dc04..2822c8c3a 100644 --- a/firestore-send-email/functions/__tests__/prepare-payload.test.ts +++ b/firestore-send-email/functions/__tests__/prepare-payload.test.ts @@ -48,6 +48,19 @@ class MockTemplates { text: undefined, subject: "Template Subject", }; + case "template-with-object-attachment": + // Simulates a template that returns attachments as an object instead of array + return { + html: "

Template HTML

", + subject: "Template Subject", + attachments: { filename: "report.pdf" }, + }; + case "template-with-null-attachments": + return { + html: "

Template HTML

", + subject: "Template Subject", + attachments: null, + }; default: return {}; } @@ -351,18 +364,13 @@ describe("preparePayload Template Merging", () => { expect(result.message.subject).toBe("Template Subject"); }); - it("should handle incorrectly formatted attachments object", async () => { + it("should filter out empty attachment objects with only null values", async () => { const payload = { - to: "tester@gmx.at", + to: "test@example.com", template: { - name: "med_order_reply_greimel", + name: "html-only-template", data: { - address: "Halbenrain 140 Graz", - doctorName: "Dr. Andreas", - openingHours: "Mo., Mi., Fr. 8:00-12:00Di., Do. 10:30-15:30", - orderText: "Some stuff i need", - userName: "Pfeiler ", - name: "med_order_reply_greimel", + name: "Test User", }, }, message: { @@ -372,20 +380,20 @@ describe("preparePayload Template Merging", () => { text: null, }, ], - subject: "Bestellbestätigung", + subject: "Test Subject", }, }; const result = await preparePayload(payload); - // Should convert attachments to an empty array since the format is incorrect + // Empty attachment objects should be filtered out expect(result.message.attachments).toEqual([]); - expect(result.message.subject).toBe("Bestellbestätigung"); - expect(result.to).toEqual(["tester@gmx.at"]); + expect(result.message.subject).toBe("Template Subject"); + expect(result.to).toEqual(["test@example.com"]); }); describe("attachment validation", () => { - it("should handle non-array attachments", async () => { + it("should throw clear error for string attachments", async () => { const payload = { to: "test@example.com", message: { @@ -395,7 +403,30 @@ describe("preparePayload Template Merging", () => { }, }; - await expect(preparePayload(payload)).rejects.toThrow(); + await expect(preparePayload(payload)).rejects.toThrow( + "Invalid message configuration: Field 'message.attachments' must be an array" + ); + }); + + it("should throw clear error for invalid attachment httpHeaders", async () => { + const payload = { + to: "test@example.com", + message: { + subject: "Test Subject", + text: "Test text", + attachments: [ + { + filename: "test.txt", + href: "https://example.com", + httpHeaders: "invalid", + }, + ], + }, + }; + + await expect(preparePayload(payload)).rejects.toThrow( + "Invalid message configuration: Field 'message.attachments.0.httpHeaders' must be a map" + ); }); it("should handle null attachments as no attachments", async () => { @@ -456,4 +487,51 @@ describe("preparePayload Template Merging", () => { expect(result.message.attachments).toEqual([]); }); }); + + describe("template-rendered attachments", () => { + it("should normalize template-returned attachment object to array", async () => { + // This tests the exact scenario from issue #2550 where a template + // returns attachments as an object instead of an array + const payload = { + to: "test@example.com", + template: { + name: "template-with-object-attachment", + data: {}, + }, + }; + + const result = await preparePayload(payload); + expect(result.message.attachments).toEqual([{ filename: "report.pdf" }]); + }); + + it("should handle template-returned null attachments", async () => { + const payload = { + to: "test@example.com", + template: { + name: "template-with-null-attachments", + data: {}, + }, + }; + + const result = await preparePayload(payload); + expect(result.message.attachments).toEqual([]); + }); + + it("should process template-only payload without message field", async () => { + // Matches the user's payload structure - template only, no message field + const payload = { + to: "test@example.com", + template: { + name: "html-only-template", + data: { + someField: "value", + }, + }, + }; + + const result = await preparePayload(payload); + expect(result.message.html).toBe("

Template HTML

"); + expect(result.message.subject).toBe("Template Subject"); + }); + }); }); diff --git a/firestore-send-email/functions/__tests__/validation.test.ts b/firestore-send-email/functions/__tests__/validation.test.ts index 731498ccc..5a1180a75 100644 --- a/firestore-send-email/functions/__tests__/validation.test.ts +++ b/firestore-send-email/functions/__tests__/validation.test.ts @@ -612,6 +612,95 @@ describe("validatePayload", () => { }; expect(() => validatePayload(payload)).not.toThrow(); }); + + it("should throw error for string attachments", () => { + const payload = { + to: "test@example.com", + message: { + subject: "Test Subject", + text: "Test message", + attachments: "invalid-string", + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid message configuration: Field 'message.attachments' must be an array" + ); + }); + + it("should throw error for number attachments", () => { + const payload = { + to: "test@example.com", + message: { + subject: "Test Subject", + text: "Test message", + attachments: 123, + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid message configuration: Field 'message.attachments' must be an array" + ); + }); + }); + }); + + describe("error messages", () => { + it("should provide clear error for empty template name", () => { + const payload = { + to: "test@example.com", + template: { + name: "", + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid template configuration: Field 'template.name' cannot be empty" + ); + }); + + it("should provide clear error for empty UID in array", () => { + const payload = { + toUids: ["valid-uid", ""], + message: { + subject: "Test", + text: "Test", + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid email configuration: Field 'toUids.1' cannot be empty" + ); + }); + + it("should provide clear error for cc as number", () => { + const payload = { + to: "test@example.com", + cc: 123, + message: { + subject: "Test", + text: "Test", + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid email configuration: Field 'cc' must be either a string or an array of strings" + ); + }); + + it("should provide clear error for bcc as boolean", () => { + const payload = { + to: "test@example.com", + bcc: true, + message: { + subject: "Test", + text: "Test", + }, + }; + expect(() => validatePayload(payload)).toThrow(ValidationError); + expect(() => validatePayload(payload)).toThrow( + "Invalid email configuration: Field 'bcc' must be either a string or an array of strings" + ); }); }); }); diff --git a/firestore-send-email/functions/jest.config.js b/firestore-send-email/functions/jest.config.js index d9dc12340..aeb573231 100644 --- a/firestore-send-email/functions/jest.config.js +++ b/firestore-send-email/functions/jest.config.js @@ -5,6 +5,7 @@ module.exports = { displayName: packageJson.name, rootDir: "./", preset: "ts-jest", + testPathIgnorePatterns: ["/node_modules/", "/__tests__/e2e/"], globals: { "ts-jest": { tsConfig: "/__tests__/tsconfig.json", diff --git a/firestore-send-email/functions/jest.e2e.config.js b/firestore-send-email/functions/jest.e2e.config.js new file mode 100644 index 000000000..beaf7bb1a --- /dev/null +++ b/firestore-send-email/functions/jest.e2e.config.js @@ -0,0 +1,13 @@ +module.exports = { + displayName: "e2e", + rootDir: "./", + preset: "ts-jest", + testMatch: ["**/__tests__/e2e/**/*.test.ts"], + testEnvironment: "node", + setupFilesAfterEnv: ["/__tests__/e2e/setup.ts"], + globals: { + "ts-jest": { + tsconfig: "/__tests__/tsconfig.json", + }, + }, +}; diff --git a/firestore-send-email/functions/package.json b/firestore-send-email/functions/package.json index f2eae28d6..5cb9c08bd 100644 --- a/firestore-send-email/functions/package.json +++ b/firestore-send-email/functions/package.json @@ -9,12 +9,12 @@ "clean": "rimraf lib", "compile": "tsc", "local:emulator": "cd ../../_emulator && firebase emulators:start -P demo-test", - "test": "cd ../../_emulator && firebase emulators:exec --only extensions jest -P demo-test", + "test": "cd ../../_emulator && firebase emulators:exec --project=demo-test \"cd ../firestore-send-email/functions && npx jest\"", "testIfEmulatorRunning": "wait-on tcp:4001 && jest", "test:local": "concurrently --kill-others \"npm run local:emulator\" \"npm run testIfEmulatorRunning\"", "test:watch": "concurrently \"npm run local:emulator\" \"jest --watch\"", "test:coverage": "concurrently --kill-others \"npm run local:emulator\" \"wait-on tcp:4001 && jest --coverage\"", - "test:e2e:sendgrid": "cd ../../_emulator && firebase emulators:exec --project=demo-test \" cd ../firestore-send-email/functions && E2E_SENDGRID=true jest __tests__/e2e/sendgrid.test.ts\"", + "test:e2e": "cd ../../_emulator && firebase emulators:exec --project=demo-test \"cd ../firestore-send-email/functions && npx jest --config jest.e2e.config.js\"", "generate-readme": "firebase ext:info .. --markdown > ../README.md" }, "keywords": [],