Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Validate `timeoutSeconds` per v2 trigger type (540s for events, 3600s for HTTPS/callable, 1800s for task queues) so misconfigured values fail at function-definition or manifest-extraction time instead of at deploy time. (#1874)
142 changes: 140 additions & 2 deletions spec/v2/options.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,15 @@
// SOFTWARE.

import { expect } from "chai";
import { defineJsonSecret, defineSecret } from "../../src/params";
import { GlobalOptions, optionsToEndpoint, RESET_VALUE } from "../../src/v2/options";
import { defineInt, defineJsonSecret, defineSecret } from "../../src/params";
import {
assertTimeoutSecondsValid,
GlobalOptions,
optionsToEndpoint,
optionsToTriggerAnnotations,
RESET_VALUE,
setGlobalOptions,
} from "../../src/v2/options";

describe("GlobalOptions", () => {
it("should accept all valid secret types in secrets array (type test)", () => {
Expand Down Expand Up @@ -92,3 +99,134 @@ describe("optionsToEndpoint", () => {
expect(endpoint.vpc).to.equal(RESET_VALUE);
});
});

describe("assertTimeoutSecondsValid", () => {
afterEach(() => {
setGlobalOptions({});
});

it("is a no-op when timeoutSeconds is undefined", () => {
expect(() => assertTimeoutSecondsValid({}, "event")).to.not.throw();
expect(() => assertTimeoutSecondsValid({}, "https")).to.not.throw();
expect(() => assertTimeoutSecondsValid({}, "task")).to.not.throw();
});

it("accepts values within each kind's limit", () => {
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 540 }, "event")).to.not.throw();
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 3600 }, "https")).to.not.throw();
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 1800 }, "task")).to.not.throw();
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 0 }, "event")).to.not.throw();
});

it("throws when timeoutSeconds exceeds the event-handler limit", () => {
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 3600 }, "event")).to.throw(
/between 0 and 540 for event-handling functions/
);
});

it("throws when timeoutSeconds exceeds the HTTPS limit", () => {
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 3601 }, "https")).to.throw(
/between 0 and 3600 for HTTPS and callable functions/
);
});

it("throws when timeoutSeconds exceeds the task-queue limit", () => {
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 1801 }, "task")).to.throw(
/between 0 and 1800 for task queue functions/
);
});

it("throws when timeoutSeconds is negative", () => {
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: -1 }, "event")).to.throw(
/between 0 and 540/
);
});

it("skips validation for Expression timeouts", () => {
const expr = { timeoutSeconds: defineInt("TIMEOUT") };
expect(() => assertTimeoutSecondsValid(expr, "event")).to.not.throw();
});

it("skips validation for RESET_VALUE timeouts", () => {
const opts = { timeoutSeconds: RESET_VALUE as unknown as number };
expect(() => assertTimeoutSecondsValid(opts, "event")).to.not.throw();
});

it("falls back to the global timeoutSeconds when the function-level option is absent", () => {
setGlobalOptions({ timeoutSeconds: 3600 });
expect(() => assertTimeoutSecondsValid({}, "event")).to.throw(
/between 0 and 540 for event-handling functions/
);
expect(() => assertTimeoutSecondsValid({}, "https")).to.not.throw();
});

it("prefers the function-level timeoutSeconds over the global one", () => {
setGlobalOptions({ timeoutSeconds: 60 });
expect(() => assertTimeoutSecondsValid({ timeoutSeconds: 1000 }, "event")).to.throw(
/between 0 and 540/
);
});

it("treats a function-level RESET_VALUE as a clear of an out-of-range global", () => {
setGlobalOptions({ timeoutSeconds: 3600 });
expect(() =>
assertTimeoutSecondsValid({ timeoutSeconds: RESET_VALUE as unknown as number }, "event")
).to.not.throw();
});
});

describe("optionsToEndpoint timeout validation", () => {
afterEach(() => {
setGlobalOptions({});
});

it("does not validate when kind is omitted (backwards compatibility)", () => {
expect(() => optionsToEndpoint({ timeoutSeconds: 9999 })).to.not.throw();
});

it("throws when kind is provided and timeoutSeconds exceeds the limit", () => {
expect(() => optionsToEndpoint({ timeoutSeconds: 3600 }, "event")).to.throw(
/between 0 and 540/
);
expect(() => optionsToEndpoint({ timeoutSeconds: 3601 }, "https")).to.throw(
/between 0 and 3600/
);
expect(() => optionsToEndpoint({ timeoutSeconds: 1801 }, "task")).to.throw(
/between 0 and 1800/
);
});

it("is a no-op for in-range timeouts when kind is provided", () => {
expect(() => optionsToEndpoint({ timeoutSeconds: 540 }, "event")).to.not.throw();
expect(() => optionsToEndpoint({ timeoutSeconds: 3600 }, "https")).to.not.throw();
expect(() => optionsToEndpoint({ timeoutSeconds: 1800 }, "task")).to.not.throw();
});
});

describe("optionsToTriggerAnnotations timeout validation", () => {
afterEach(() => {
setGlobalOptions({});
});

it("does not validate when kind is omitted (backwards compatibility)", () => {
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 9999 })).to.not.throw();
});

it("throws when kind is provided and timeoutSeconds exceeds the limit", () => {
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 3600 }, "event")).to.throw(
/between 0 and 540/
);
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 3601 }, "https")).to.throw(
/between 0 and 3600/
);
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 1801 }, "task")).to.throw(
/between 0 and 1800/
);
});

it("is a no-op for in-range timeouts when kind is provided", () => {
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 540 }, "event")).to.not.throw();
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 3600 }, "https")).to.not.throw();
expect(() => optionsToTriggerAnnotations({ timeoutSeconds: 1800 }, "task")).to.not.throw();
});
});
14 changes: 14 additions & 0 deletions spec/v2/providers/https.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,14 @@ describe("onRequest", () => {
await runHandler(func, req);
expect(hello).to.equal("world");
});

it("rejects timeoutSeconds above the 3600s HTTPS limit", () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we're testing some of the providers, but I think we need more direct test of the wiring across providers

a parameterized suite like "each v2 provider rejects out-of-range timeout" would be a cheap way to get coverage

expect(() =>
https.onRequest({ timeoutSeconds: 3601 }, (_req, res) => {
res.end();
})
).to.throw(/between 0 and 3600 for HTTPS and callable functions/);
});
});

describe("onCall", () => {
Expand Down Expand Up @@ -605,6 +613,12 @@ describe("onCall", () => {
expect(hello).to.equal("world");
});

it("rejects timeoutSeconds above the 3600s HTTPS limit", () => {
expect(() => https.onCall({ timeoutSeconds: 3601 }, () => 42)).to.throw(
/between 0 and 3600 for HTTPS and callable functions/
);
});

describe("authPolicy", () => {
before(() => {
sinon.stub(debug, "isDebugFeatureEnabled").withArgs("skipTokenVerification").returns(true);
Expand Down
13 changes: 13 additions & 0 deletions spec/v2/providers/pubsub.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,19 @@ describe("onMessagePublished", () => {
expect(res).to.equal("input");
});

it("rejects timeoutSeconds above the 540s event-handler limit", () => {
expect(() =>
pubsub.onMessagePublished({ topic: "topic", timeoutSeconds: 3600 }, () => 42)
).to.throw(/between 0 and 540 for event-handling functions/);
});

it("rejects a global timeoutSeconds above the 540s event-handler limit", () => {
options.setGlobalOptions({ timeoutSeconds: 3600 });
Comment thread
cabljac marked this conversation as resolved.
expect(() => pubsub.onMessagePublished("topic", () => 42)).to.throw(
/between 0 and 540 for event-handling functions/
);
});

it("should parse pubsub messages", async () => {
let json: unknown;
const messageJSON = {
Expand Down
8 changes: 8 additions & 0 deletions spec/v2/providers/storage.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,14 @@ describe("v2/storage", () => {
await storage.onObjectFinalized("bucket", () => null)(event);
expect(hello).to.equal("world");
});

it("rejects timeoutSeconds above the 540s event-handler limit on __endpoint access", () => {
const func = storage.onObjectFinalized(
{ bucket: "my-bucket", timeoutSeconds: 3600 },
() => 42
);
expect(() => func.__endpoint).to.throw(/between 0 and 540 for event-handling functions/);
});
});

describe("onObjectDeleted", () => {
Expand Down
6 changes: 6 additions & 0 deletions spec/v2/providers/tasks.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,12 @@ describe("onTaskDispatched", () => {
expect(hello).to.equal("world");
});

it("rejects timeoutSeconds above the 1800s task-queue limit", () => {
expect(() => onTaskDispatched({ timeoutSeconds: 1801 }, () => null)).to.throw(
/between 0 and 1800 for task queue functions/
);
});

describe("v1-compatible getters", () => {
it("should provide v1-compatible context on the request object", async () => {
let capturedRequest: any;
Expand Down
77 changes: 75 additions & 2 deletions src/v2/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,31 @@ import * as logger from "../logger";

export { RESET_VALUE } from "../common/options";

/**
* Maximum timeout in seconds for event-handling functions (e.g. Firestore,
* Realtime Database, Pub/Sub, Storage, Eventarc, alerts, scheduler).
* @internal
*/
export const MAX_EVENT_TIMEOUT_SECONDS = 540;

/**
* Maximum timeout in seconds for HTTPS and callable functions.
* @internal
*/
export const MAX_HTTPS_TIMEOUT_SECONDS = 3600;

/**
* Maximum timeout in seconds for Task Queue functions.
* @internal
*/
export const MAX_TASK_TIMEOUT_SECONDS = 1800;

/**
* Function category used to pick a `timeoutSeconds` upper bound.
* @internal
*/
export type TimeoutKind = "event" | "https" | "task";

/**
* List of all regions supported by Cloud Functions (2nd gen).
*/
Expand Down Expand Up @@ -334,13 +359,57 @@ export interface EventHandlerOptions extends Omit<GlobalOptions, "enforceAppChec
channel?: string;
}

/**
* Validate that `timeoutSeconds` (resolved with global option fallback) does
* not exceed the maximum allowed value for the given function kind. Throws a
* plain `Error` matching the v1 `assertRuntimeOptionsValid` shape so the
* problem surfaces at function-definition time instead of at deploy time.
* `Expression`, `RESET_VALUE`, and `undefined` are skipped — only literal
* numbers are checked.
* @internal
*/
export function assertTimeoutSecondsValid(
opts: GlobalOptions | EventHandlerOptions | HttpsOptions | undefined,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't know if we need the | undefined

kind: TimeoutKind
): void {
const timeoutSeconds = opts?.timeoutSeconds ?? getGlobalOptions().timeoutSeconds;
if (typeof timeoutSeconds !== "number") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if timeoutSeconds is NaN here, i think we might have an issue, i think it might slip through?

return;
}
let max: number;
let label: string;
switch (kind) {
case "https":
max = MAX_HTTPS_TIMEOUT_SECONDS;
label = "HTTPS and callable";
break;
case "task":
max = MAX_TASK_TIMEOUT_SECONDS;
label = "task queue";
break;
default:
max = MAX_EVENT_TIMEOUT_SECONDS;
label = "event-handling";
break;
}
if (timeoutSeconds < 0 || timeoutSeconds > max) {
throw new Error(
`timeoutSeconds must be between 0 and ${max} for ${label} functions. Got ${timeoutSeconds}.`
);
}
}

/**
* Apply GlobalOptions to trigger definitions.
* @internal
*/
export function optionsToTriggerAnnotations(
opts: GlobalOptions | EventHandlerOptions | HttpsOptions
opts: GlobalOptions | EventHandlerOptions | HttpsOptions,
kind?: TimeoutKind
): TriggerAnnotation {
if (kind !== undefined) {
assertTimeoutSecondsValid(opts, kind);
}
const annotation: TriggerAnnotation = {};
copyIfPresent(
annotation,
Expand Down Expand Up @@ -398,8 +467,12 @@ export function optionsToTriggerAnnotations(
* @internal
*/
export function optionsToEndpoint(
opts: GlobalOptions | EventHandlerOptions | HttpsOptions
opts: GlobalOptions | EventHandlerOptions | HttpsOptions,
kind?: TimeoutKind
): ManifestEndpoint {
if (kind !== undefined) {
assertTimeoutSecondsValid(opts, kind);
}
const endpoint: ManifestEndpoint = {};
copyIfPresent(
endpoint,
Expand Down
4 changes: 2 additions & 2 deletions src/v2/providers/ai/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export function beforeGenerateContent(
func = wrapTraceContext(withInit(func));

const baseOpts = options.optionsToEndpoint(options.getGlobalOptions());
const specificOpts = options.optionsToEndpoint(opts);
const specificOpts = options.optionsToEndpoint(opts, "https");
func.__endpoint = {
...initV2Endpoint(options.getGlobalOptions(), opts),
platform: "gcfv2",
Expand Down Expand Up @@ -344,7 +344,7 @@ export function afterGenerateContent(
func = wrapTraceContext(withInit(func));

const baseOpts = options.optionsToEndpoint(options.getGlobalOptions());
const specificOpts = options.optionsToEndpoint(opts);
const specificOpts = options.optionsToEndpoint(opts, "https");
func.__endpoint = {
...initV2Endpoint(options.getGlobalOptions(), opts),
platform: "gcfv2",
Expand Down
2 changes: 1 addition & 1 deletion src/v2/providers/alerts/alerts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export function getEndpointAnnotation(
appId?: string
): ManifestEndpoint {
const baseOpts = options.optionsToEndpoint(options.getGlobalOptions());
const specificOpts = options.optionsToEndpoint(opts);
const specificOpts = options.optionsToEndpoint(opts, "event");
const endpoint: ManifestEndpoint = {
...initV2Endpoint(options.getGlobalOptions(), opts),
platform: "gcfv2",
Expand Down
2 changes: 1 addition & 1 deletion src/v2/providers/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ export function makeEndpoint(
instance: PathPattern
): ManifestEndpoint {
const baseOpts = options.optionsToEndpoint(options.getGlobalOptions());
const specificOpts = options.optionsToEndpoint(opts);
const specificOpts = options.optionsToEndpoint(opts, "event");

const eventFilters: Record<string, string> = {};
const eventFilterPathPatterns: Record<string, string> = {
Expand Down
2 changes: 1 addition & 1 deletion src/v2/providers/dataconnect/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function onGraphRequest(opts: GraphqlServerOptions): HttpsFunction {
const baseOpts = options.optionsToEndpoint(globalOpts);
// global options calls region a scalar and https allows it to be an array,
// but optionsToTriggerAnnotations handles both cases.
const specificOpts = options.optionsToEndpoint(opts as options.GlobalOptions);
const specificOpts = options.optionsToEndpoint(opts as options.GlobalOptions, "https");
const endpoint: Partial<ManifestEndpoint> = {
...initV2Endpoint(globalOpts, opts),
platform: "gcfv2",
Expand Down
2 changes: 1 addition & 1 deletion src/v2/providers/dataconnect/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ function makeEndpoint(
operation: PathPattern | undefined
): ManifestEndpoint {
const baseOpts = optionsToEndpoint(getGlobalOptions());
const specificOpts = optionsToEndpoint(opts);
const specificOpts = optionsToEndpoint(opts, "event");

const eventFilters: Record<string, string> = {};
const eventFilterPathPatterns: Record<string, string> = {};
Expand Down
2 changes: 1 addition & 1 deletion src/v2/providers/eventarc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export function onCustomEventPublished<T = any>(
const channel = opts.channel ?? "locations/us-central1/channels/firebase";

const baseOpts = options.optionsToEndpoint(options.getGlobalOptions());
const specificOpts = options.optionsToEndpoint(opts);
const specificOpts = options.optionsToEndpoint(opts, "event");

const endpoint: ManifestEndpoint = {
...initV2Endpoint(options.getGlobalOptions(), opts),
Expand Down
Loading
Loading