fix(firestore-send-email): reject invalid select and regex param values at deploy

kits/firestore-send-email/CHANGELOG.md

@@ -1,3 +1,5 @@
+- fix: invalid values in `.env` now fail the deploy instead of leaving mail unprocessed or quietly changing behaviour. The Firebase CLI only checks values it prompts for, so `TTL_EXPIRE_TYPE=days` deployed and left every document without a `delivery` field, `TTL_EXPIRE_VALUE=-1` set an `expireAt` in the past, and `AUTH_TYPE=oauth2` sent through the SMTP URI. The kit now rejects `DATABASE_REGION`, `AUTH_TYPE`, `OAUTH_SECURE` and `TTL_EXPIRE_TYPE` values that are not one of their options, and `SMTP_CONNECTION_URI`, `MAIL_COLLECTION`, `DEFAULT_FROM` and `TTL_EXPIRE_VALUE` values that fail the extension's format checks, as the extension did at install time. `DATABASE_REGION` is now case sensitive, as it was in the extension. Option labels such as `Day` are still accepted, as the extension accepted them, and run as they did in the extension. An empty `OAUTH_SECURE` now means `true`, as in the extension.
+
 - fix: deploy `processQueue` with the extension's runtime settings: `concurrency: 1`, `cpu: "gcf_gen1"` (0.1666 vCPU at 256MiB) and `ingressSettings: "ALLOW_INTERNAL_ONLY"`. `setGlobalOptions` can't change these. Before, the kit took the 2nd gen defaults of concurrency `80`, 1 vCPU and `ALLOW_ALL`. A burst of mail with attachments landed on one 256MiB instance, which ran out of memory and dropped every event in flight on it: the documents were left in `PENDING` or `PROCESSING`, or with no `delivery` field, and were never retried or marked as failed, so the mail was never sent. Existing kit deployments switch on their next deploy.
 
 - fix: an Eventarc publish failure no longer fails the function. A channel that was deleted or is unreachable answers `PERMISSION_DENIED`, and the publish was awaited before the work the function exists to do, so the mail was never sent. The publish failure is now logged as a warning and the invocation continues. Deploys also declare `eventarcpublishing.googleapis.com`, so the API is enabled on your project whether or not you use events.

kits/firestore-send-email/README.md

@@ -112,6 +112,35 @@ the CLI connects them to the function at deploy time.
 | `ttlExpireValue`      | `TTL_EXPIRE_VALUE`     | no       | `1`                | TTL amount when expire type is set                    |
 | `tlsOptions`          | `TLS_OPTIONS`          | no       | `{}`               | JSON TLS options for the SMTP transport               |
 
+Single-quote JSON values such as `TLS_OPTIONS` in `.env`. In a double-quoted
+value the CLI turns `\n` into a real newline, so a PEM certificate no longer
+parses as JSON, and the kit falls back to `{"rejectUnauthorized":false}`
+(certificate checks off) with only a warning:
+
+```
+TLS_OPTIONS='{"ca":"-----BEGIN CERTIFICATE-----\n...\n-----END CERTIFICATE-----\n"}'
+```
+
+The CLI does not check values that are already in `.env`, so the kit checks
+them itself when the deploy loads it, as the extension did at install time. The
+deploy fails with an error naming the setting if `DATABASE_REGION`,
+`AUTH_TYPE`, `OAUTH_SECURE` or `TTL_EXPIRE_TYPE` is not one of its option values
+or their labels (the error lists the allowed values), or if
+`SMTP_CONNECTION_URI`, `MAIL_COLLECTION`, `DEFAULT_FROM` or `TTL_EXPIRE_VALUE`
+is not in the format the extension accepted (the error gives the extension's
+message). One error lists
+every bad setting, along with any required setting left blank. The values are
+case sensitive. The emulator runs the same check, so a bad value stops the
+function from loading there. An empty `OAUTH_SECURE` means `true`, as it did in
+the extension.
+
+An option's label (such as `Day`) is accepted because the extension's
+installer accepted it, but it runs as it did in the extension: any
+`TTL_EXPIRE_TYPE` label, `Never` included, leaves every document without a
+`delivery` field, `OAUTH_SECURE=Yes` connects without TLS, and a
+`DATABASE_REGION` label is not a region the function can deploy to. Use the
+values instead.
+
 ## Multiple instances
 
 To process several mail queues, add one entry per instance to the `instances`
@@ -177,8 +206,8 @@ location setting. The kit deploys the function to the region derived from
 Firestore locations (`europe-west2`, `us-east1`, ...) are used as-is; the
 multi-region locations map to a Cloud Run region inside them - `nam5` and
 `nam7` to `us-central1`, `eur3` to `europe-west1` - because they are not Cloud
-Run regions themselves and would fail the deploy. The value is matched
-case-insensitively. The Firestore trigger always fires in the database's own
+Run regions themselves and would fail the deploy. The value is case sensitive,
+as in the extension, so `NAM5` fails the deploy. The Firestore trigger always fires in the database's own
 region, whatever region the function runs in.
 
 If you copied `DATABASE_REGION` into your `.env` from an extension install, it
@@ -232,18 +261,11 @@ with `EXT_`, so
 `EVENTARC_CHANNEL` unset, nothing is published and the function is otherwise
 unaffected.
 
-### Settings in `.env` are not validated at deploy time
-
-The extension rejected a malformed `DEFAULT_FROM`, a `MAIL_COLLECTION` that was
-not a valid collection path, an `SMTP_CONNECTION_URI` that was not
-`smtp(s)://...:port`, and a `TTL_EXPIRE_VALUE` that was not a positive integer,
-before it would install. The kit declares the same checks, but they apply only to
-a value entered at the `firebase deploy` prompt, not to one read from `.env`. A
-bad from address or connection URI in `.env` deploys cleanly and every document
-fails at send time with `delivery.state: ERROR` instead. `TTL_EXPIRE_VALUE: 0` is
-silently treated as `1`, and a negative value produces a `delivery.expireAt` in
-the past, which a TTL policy will act on immediately. Only a required setting
-left empty in `.env`, such as `DEFAULT_FROM=`, fails the deploy.
+### An empty required setting fails the deploy
+
+The kit checks `.env` values against the extension's install-time rules (see
+[Configuration](#configuration)), with one difference: a required setting left
+empty, such as `DEFAULT_FROM=`, fails the deploy instead of taking its default.
 
 ### The setup steps and the OAuth2 helper are not in the README
 

kits/firestore-send-email/src/config.ts

@@ -362,7 +362,10 @@ export function configFromEnv(): SendEmailConfig {
     tlsOptions: params.tlsOptions.value(),
     host: params.host.value(),
     oauthPort: params.oauthPort.value(),
-    oauthSecure: params.oauthSecure.value(),
+    // The extension platform swapped a blank optional value for its default
+    // (`true`), but the boolean param reads "" as false.
+    oauthSecure:
+      process.env.OAUTH_SECURE === "" ? undefined : params.oauthSecure.value(),
     user: params.user.value(),
     clientId:
       authType === AuthenticatonType.OAuth2
@@ -433,3 +436,78 @@ export function assertRequiredParams(
     );
   }
 }
+
+/**
+ * Rejects param values the published extension refuses at install time: a
+ * select value that is not one of its option values or labels, or a text value
+ * that fails its `validationRegex`.
+ *
+ * The CLI applies a param's `input` only when it prompts, so a value already
+ * in `.env` reaches the handlers unchecked, and they coerce it
+ * (`TTL_EXPIRE_TYPE=days` leaves every document without a `delivery` field,
+ * and `TTL_EXPIRE_VALUE=-1` sets an `expireAt` in the past, for example).
+ * Option labels such as `Day` are accepted, as the extension's installer
+ * accepted them, and run as the extension ran them. Unset and empty values are
+ * skipped: defaults cover the first, and `assertRequiredParams` decides about
+ * the second. Called at module scope so deploy-time discovery fails before the
+ * function ships.
+ */
+export function assertValidParams(): void {
+  const invalid: string[] = [];
+
+  for (const param of Object.values(params)) {
+    const raw = process.env[param.name];
+    // Secrets declare no `input`.
+    const declared = param.options;
+    const input = declared && "input" in declared ? declared.input : undefined;
+    if (raw === undefined || raw === "" || !input) {
+      continue;
+    }
+
+    const setting = `${param.name}=${JSON.stringify(raw)}`;
+    if ("select" in input) {
+      const { options } = input.select;
+      const allowed = options.map(({ value }) => String(value));
+      const labels = options.map((option) => option.label);
+      if (!allowed.includes(raw) && !labels.includes(raw)) {
+        invalid.push(`${setting} must be one of: ${allowed.join(", ")}`);
+      }
+    } else if ("text" in input && input.text.validationRegex) {
+      const regex = new RegExp(input.text.validationRegex);
+      if (!regex.test(raw)) {
+        const reason =
+          input.text.validationErrorMessage ?? `must match ${regex}`;
+        invalid.push(`${setting}: ${reason}`);
+      }
+    }
+  }
+
+  if (invalid.length > 0) {
+    throw new Error(
+      `Invalid parameter values: ${invalid.join("; ")}. ` +
+        "Fix them in this instance's .env or .env.<projectId> file, or remove " +
+        "the entries to use their defaults."
+    );
+  }
+}
+
+/**
+ * Runs `assertRequiredParams` and `assertValidParams` and throws once with
+ * both messages, so a `.env` with a blank required param and an invalid value
+ * reports both in one deploy rather than one per redeploy.
+ */
+export function assertParams(): void {
+  const errors: string[] = [];
+
+  for (const check of [assertRequiredParams, assertValidParams]) {
+    try {
+      check();
+    } catch (err) {
+      errors.push((err as Error).message);
+    }
+  }
+
+  if (errors.length > 0) {
+    throw new Error(errors.join("\n"));
+  }
+}

kits/firestore-send-email/src/index.ts

@@ -20,7 +20,7 @@ import { onDocumentWritten } from "firebase-functions/firestore";
 import type { Role } from "firebase-functions/v2";
 import { requiresAPI, requiresRole } from "firebase-functions/v2";
 import {
-  assertRequiredParams,
+  assertParams,
   configFromEnv,
   envDeployOptions,
   secretParams,
@@ -32,7 +32,7 @@ import { transportLayer } from "./helpers";
 import * as logs from "./logs";
 import { Templates } from "./templates";
 
-assertRequiredParams();
+assertParams();
 export * from "./lib";
 
 const REQUIRED_ROLES: ReadonlyArray<Role> = [

kits/firestore-send-email/tests/config-runtime.test.ts

@@ -29,6 +29,7 @@
 import { declaredParams } from "firebase-functions/params";
 import { afterEach, beforeEach, describe, expect, test } from "vitest";
 import { configFromEnv } from "../src/config";
+import { resolveConfig } from "../src/export-config";
 
 function declaration(name: string) {
   const param = declaredParams.find((candidate) => candidate.name === name);
@@ -85,4 +86,10 @@ describe("OAUTH_SECURE values inherited from the extension", () => {
   test("treats an unset variable as insecure, as the extension did", () => {
     expect(configFromEnv().oauthSecure).toBe(false);
   });
+
+  // The extension platform replaced a blank optional value with its default.
+  test("treats a blank variable as secure, as the extension did", () => {
+    process.env.OAUTH_SECURE = "";
+    expect(resolveConfig(configFromEnv()).oauthSecure).toBe(true);
+  });
 });

kits/firestore-send-email/tests/index.test.ts

@@ -59,7 +59,6 @@ describe("processQueue options", () => {
     ["nam5", "us-central1"],
     ["nam7", "us-central1"],
     ["eur3", "europe-west1"],
-    ["NAM5", "us-central1"],
   ])(
     "multi-region DATABASE_REGION %s deploys the function to %s",
     async (databaseRegion, expectedRegion) => {
@@ -99,3 +98,41 @@ describe("processQueue options", () => {
     expect(options.ingressSettings).toBe("ALLOW_INTERNAL_ONLY");
   });
 });
+
+describe("module load", () => {
+  afterEach(() => {
+    vi.unstubAllEnvs();
+  });
+
+  // Deploy-time discovery imports the kit, so this is what fails the deploy.
+  test("throws when a param value is invalid", async () => {
+    vi.resetModules();
+    vi.stubEnv("TTL_EXPIRE_TYPE", "days");
+
+    await expect(import("../src/index")).rejects.toThrow(
+      'TTL_EXPIRE_TYPE="days"'
+    );
+  });
+
+  // The extension's select is case sensitive, so a DATABASE_REGION it rejected
+  // no longer reaches the case-insensitive region lookup.
+  test("throws on a DATABASE_REGION in the wrong case", async () => {
+    vi.resetModules();
+    vi.stubEnv("DATABASE_REGION", "NAM5");
+
+    await expect(import("../src/index")).rejects.toThrow(
+      'DATABASE_REGION="NAM5"'
+    );
+  });
+
+  test("reports blank required and invalid params in one error", async () => {
+    vi.resetModules();
+    vi.stubEnv("DEFAULT_FROM", "");
+    vi.stubEnv("TTL_EXPIRE_VALUE", "0");
+
+    const load = import("../src/index");
+
+    await expect(load).rejects.toThrow("DEFAULT_FROM");
+    await expect(load).rejects.toThrow('TTL_EXPIRE_VALUE="0"');
+  });
+});