Skip to content

Commit c56a7ae

Browse files
committed
fix(cors): defer parameter resolution in https callables and requests
### Description Fixes an issue where passing a parameterized CORS origin (`Expression`) to HTTPS functions attempts to evaluate the parameter at global module definition time rather than request runtime. This updates `resolveCorsOrigin` to execute per-request, gracefully catching missing environment variables or empty parameters and disabling CORS (`origin: false`) as a secure fallback. Additionally, this updates V1 `https.onRequest` to accept an options object (`HttpsOptions`) supporting custom `cors` configuration, aligning with V2. ### Scenarios Tested - Added unit tests in `spec/common/providers/https.spec.ts` verifying `resolveCorsOrigin` successfully extracts runtime values from `StringParam` and `ListParam` Expressions, and safely defaults to `false` when params are missing or evaluate to empty lists/strings. - Added unit tests in `spec/v1/providers/https.spec.ts` verifying V1 `onRequest` correctly enforces custom CORS options and does not crash when passed an Expression. - Added unit tests in `spec/v2/providers/https.spec.ts` verifying passing Expressions to `onRequest` and `onCall` CORS options does not cause definition-time crashes. - Ran the full `npm test` suite to confirm zero regressions. ### Sample Commands `npm test`
1 parent 8cb4a5a commit c56a7ae

7 files changed

Lines changed: 302 additions & 38 deletions

File tree

spec/common/providers/https.spec.ts

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { App, initializeApp } from "firebase-admin/app";
33
import * as appCheck from "firebase-admin/app-check";
44
import * as sinon from "sinon";
55
import * as nock from "nock";
6+
import { defineString, defineList } from "../../../src/params";
67

78
import { getApp, setApp } from "../../../src/common/app";
89
import * as debug from "../../../src/common/debug";
@@ -949,6 +950,127 @@ describe("onCallHandler", () => {
949950
});
950951
});
951952
});
953+
954+
describe("CORS resolution", () => {
955+
let oldEnv: NodeJS.ProcessEnv;
956+
957+
beforeEach(() => {
958+
oldEnv = process.env;
959+
process.env = { ...oldEnv };
960+
});
961+
962+
afterEach(() => {
963+
process.env = oldEnv;
964+
});
965+
966+
it("should resolve Expression origin", async () => {
967+
process.env.ALLOWED_ORIGIN = "example.org";
968+
const param = defineString("ALLOWED_ORIGIN");
969+
await runCallableTest({
970+
httpRequest: mockRequest(null, "application/json", {}, {
971+
origin: "example.org",
972+
}),
973+
expectedData: null,
974+
callableOption: {
975+
cors: { origin: param, methods: "POST" },
976+
},
977+
callableFunction: () => null,
978+
callableFunction2: () => null,
979+
expectedHttpResponse: {
980+
status: 200,
981+
headers: {
982+
"Access-Control-Allow-Origin": "example.org",
983+
"Vary": "Origin",
984+
},
985+
body: { result: null },
986+
},
987+
});
988+
});
989+
990+
it("should resolve Expression list origin", async () => {
991+
process.env.ALLOWED_ORIGINS = JSON.stringify(["example.com", "example.org"]);
992+
const param = defineList("ALLOWED_ORIGINS");
993+
await runCallableTest({
994+
httpRequest: mockRequest(null, "application/json", {}, {
995+
origin: "example.org",
996+
}),
997+
expectedData: null,
998+
callableOption: {
999+
cors: { origin: param, methods: "POST" },
1000+
},
1001+
callableFunction: () => null,
1002+
callableFunction2: () => null,
1003+
expectedHttpResponse: {
1004+
status: 200,
1005+
headers: {
1006+
"Access-Control-Allow-Origin": "example.org",
1007+
"Vary": "Origin",
1008+
},
1009+
body: { result: null },
1010+
},
1011+
});
1012+
});
1013+
});
1014+
});
1015+
1016+
describe("resolveCorsOrigin", () => {
1017+
let oldEnv: NodeJS.ProcessEnv;
1018+
1019+
beforeEach(() => {
1020+
oldEnv = process.env;
1021+
process.env = { ...oldEnv };
1022+
});
1023+
1024+
afterEach(() => {
1025+
process.env = oldEnv;
1026+
sinon.restore();
1027+
});
1028+
1029+
it("should resolve Expression string", () => {
1030+
process.env.ALLOWED_ORIGIN = "example.com";
1031+
const param = defineString("ALLOWED_ORIGIN");
1032+
expect(https.resolveCorsOrigin(param)).to.equal("example.com");
1033+
});
1034+
1035+
it("should resolve Expression list", () => {
1036+
process.env.ALLOWED_ORIGINS = JSON.stringify(["example.com", "example.org"]);
1037+
const param = defineList("ALLOWED_ORIGINS");
1038+
expect(https.resolveCorsOrigin(param)).to.deep.equal(["example.com", "example.org"]);
1039+
});
1040+
1041+
it("should flatten single element array", () => {
1042+
expect(https.resolveCorsOrigin(["example.com"])).to.equal("example.com");
1043+
});
1044+
1045+
it("should support enableCors debug feature", () => {
1046+
sinon.stub(debug, "isDebugFeatureEnabled").withArgs("enableCors").returns(true);
1047+
expect(https.resolveCorsOrigin("example.com")).to.equal(true);
1048+
});
1049+
1050+
it("should respect cors: false even with enableCors debug feature", () => {
1051+
sinon.stub(debug, "isDebugFeatureEnabled").withArgs("enableCors").returns(true);
1052+
expect(https.resolveCorsOrigin(false)).to.equal(false);
1053+
});
1054+
1055+
it("should disable CORS when ListParam is missing at runtime", () => {
1056+
delete process.env.MISSING_ORIGINS;
1057+
const param = defineList("MISSING_ORIGINS");
1058+
expect(https.resolveCorsOrigin(param)).to.equal(false);
1059+
});
1060+
1061+
it("should disable CORS when StringParam is missing at runtime", () => {
1062+
delete process.env.MISSING_ORIGIN;
1063+
const param = defineString("MISSING_ORIGIN");
1064+
expect(https.resolveCorsOrigin(param)).to.equal(false);
1065+
});
1066+
1067+
it("should disable CORS when origin is an empty array", () => {
1068+
expect(https.resolveCorsOrigin([])).to.equal(false);
1069+
});
1070+
1071+
it("should disable CORS when origin is an empty string", () => {
1072+
expect(https.resolveCorsOrigin("")).to.equal(false);
1073+
});
9521074
});
9531075

9541076
describe("encoding/decoding", () => {

spec/v1/providers/https.spec.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
// SOFTWARE.
2222

2323
import { expect } from "chai";
24+
import { defineString, defineList } from "../../../src/params";
2425

2526
import * as functions from "../../../src/v1";
2627
import * as https from "../../../src/v1/providers/https";
@@ -91,6 +92,28 @@ describe("CloudHttpsBuilder", () => {
9192
await runHandler(fn, req as any);
9293
expect(hello).to.equal("world");
9394
});
95+
96+
it("should enforce CORS options", async () => {
97+
const func = functions.https.onRequest({ cors: "example.com" }, (req, resp) => {
98+
resp.status(200).send("hello");
99+
});
100+
const req = mockRequest(null, "application/json", {}, { origin: "example.com" });
101+
req.method = "GET";
102+
const resp = await runHandler(func, req as any);
103+
expect(resp.status).to.equal(200);
104+
expect(resp.headers?.["Access-Control-Allow-Origin"]).to.equal("example.com");
105+
});
106+
107+
it("should not crash when a string or list parameter is passed in", () => {
108+
const stringParam = defineString("ALLOWED_ORIGIN");
109+
const listParam = defineList("ALLOWED_ORIGINS");
110+
111+
const funcString = functions.https.onRequest({ cors: stringParam }, () => {});
112+
const funcList = functions.https.onRequest({ cors: listParam }, () => {});
113+
114+
expect(funcString).to.be.a("function");
115+
expect(funcList).to.be.a("function");
116+
});
94117
});
95118
});
96119

spec/v2/providers/https.spec.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import { FULL_ENDPOINT, MINIMAL_V2_ENDPOINT, FULL_OPTIONS, FULL_TRIGGER } from "
3232
import { onInit } from "../../../src/v2/core";
3333
import { Handler } from "express";
3434
import { genkit } from "genkit";
35-
import { clearParams, defineBoolean, defineList, Expression } from "../../../src/params";
35+
import { clearParams, defineBoolean, defineList, defineString, Expression } from "../../../src/params";
3636

3737
function request(args: {
3838
data?: any;
@@ -336,6 +336,17 @@ describe("onRequest", () => {
336336
await runHandler(func, req);
337337
expect(hello).to.equal("world");
338338
});
339+
340+
it("should not crash when a string or list parameter is passed in", () => {
341+
const stringParam = defineString("ALLOWED_ORIGIN");
342+
const listParam = defineList("ALLOWED_ORIGINS");
343+
344+
const funcString = https.onRequest({ cors: stringParam }, () => {});
345+
const funcList = https.onRequest({ cors: listParam }, () => {});
346+
347+
expect(funcString).to.be.a("function");
348+
expect(funcList).to.be.a("function");
349+
});
339350
});
340351

341352
describe("onCall", () => {
@@ -605,6 +616,17 @@ describe("onCall", () => {
605616
expect(hello).to.equal("world");
606617
});
607618

619+
it("should not crash when a string or list parameter is passed in", () => {
620+
const stringParam = defineString("ALLOWED_ORIGIN");
621+
const listParam = defineList("ALLOWED_ORIGINS");
622+
623+
const funcString = https.onCall({ cors: stringParam }, () => 42);
624+
const funcList = https.onCall({ cors: listParam }, () => 42);
625+
626+
expect(funcString).to.be.a("function");
627+
expect(funcList).to.be.a("function");
628+
});
629+
608630
describe("authPolicy", () => {
609631
before(() => {
610632
sinon.stub(debug, "isDebugFeatureEnabled").withArgs("skipTokenVerification").returns(true);

src/common/providers/https.ts

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import { DecodedIdToken, getAuth } from "firebase-admin/auth";
3333
import { getApp } from "../app";
3434
import { isDebugFeatureEnabled } from "../debug";
3535
import { TaskContext } from "./tasks";
36+
import { Expression } from "../../params";
3637

3738
const JWT_REGEX = /^[a-zA-Z0-9\-_=]+?\.[a-zA-Z0-9\-_=]+?\.([a-zA-Z0-9\-_=]+)?$/;
3839

@@ -710,9 +711,63 @@ type v2CallableHandler<Req, Res, Stream> = (
710711
response?: CallableResponse<Stream>
711712
) => Res;
712713

714+
export type CorsOption =
715+
| string
716+
| Expression<string>
717+
| Expression<string[]>
718+
| boolean
719+
| RegExp
720+
| Array<string | RegExp>;
721+
722+
export function resolveCorsOrigin(corsOption?: CorsOption): cors.CorsOptions["origin"] {
723+
let origin: CorsOption | undefined;
724+
if (corsOption instanceof Expression) {
725+
try {
726+
origin = corsOption.value();
727+
} catch (e) {
728+
logger.warn(`Failed to resolve CORS parameter: ${e}`);
729+
origin = false;
730+
}
731+
} else {
732+
origin = corsOption;
733+
}
734+
735+
if (Array.isArray(origin) && origin.length === 0) {
736+
logger.warn("CORS parameter resolved to an empty array. Disabling CORS.");
737+
origin = false;
738+
}
739+
740+
if (origin === "") {
741+
logger.warn("CORS parameter resolved to an empty string. Disabling CORS.");
742+
origin = false;
743+
}
744+
745+
if (isDebugFeatureEnabled("enableCors")) {
746+
// Respect `cors: false` to turn off cors even if debug feature is enabled.
747+
origin = origin === false ? false : true;
748+
}
749+
750+
if (origin === undefined) {
751+
origin = true;
752+
}
753+
754+
// Arrays cause the access-control-allow-origin header to be dynamic based
755+
// on the origin header of the request. If there is only one element in the
756+
// array, this is unnecessary.
757+
if (Array.isArray(origin) && origin.length === 1) {
758+
origin = origin[0];
759+
}
760+
761+
return origin;
762+
}
763+
764+
export type CorsInfo = Omit<cors.CorsOptions, "origin"> & {
765+
origin?: CorsOption;
766+
};
767+
713768
/** @internal **/
714769
export interface CallableOptions<T = any> {
715-
cors: cors.CorsOptions;
770+
cors: CorsInfo;
716771
enforceAppCheck?: boolean;
717772
consumeAppCheckToken?: boolean;
718773
/* @deprecated */
@@ -736,7 +791,12 @@ export function onCallHandler<Req = any, Res = any, Stream = unknown>(
736791
return (req: Request, res: express.Response) => {
737792
return new Promise((resolve) => {
738793
res.on("finish", resolve);
739-
cors(options.cors)(req, res, () => {
794+
const origin = resolveCorsOrigin(options.cors.origin);
795+
const corsOptions = {
796+
...options.cors,
797+
origin,
798+
};
799+
cors(corsOptions as cors.CorsOptions)(req, res, () => {
740800
resolve(wrapped(req, res));
741801
});
742802
});

src/v1/function-builder.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -339,11 +339,26 @@ export class FunctionBuilder {
339339
return {
340340
/**
341341
* Handle HTTP requests.
342-
* @param handler A function that takes a request and response object,
342+
* @param optsOrHandler Options or a function that takes a request and response object,
343343
* same signature as an Express app.
344344
*/
345-
onRequest: (handler: (req: https.Request, resp: express.Response) => void | Promise<void>) =>
346-
https._onRequestWithOptions(handler, this.options),
345+
onRequest: (
346+
optsOrHandler:
347+
| https.HttpsOptions
348+
| ((req: https.Request, resp: express.Response) => void | Promise<void>),
349+
handler?: (req: https.Request, resp: express.Response) => void | Promise<void>
350+
) => {
351+
let opts: https.HttpsOptions;
352+
let userHandler: (req: https.Request, resp: express.Response) => void | Promise<void>;
353+
if (typeof optsOrHandler === "function") {
354+
opts = {};
355+
userHandler = optsOrHandler;
356+
} else {
357+
opts = optsOrHandler as https.HttpsOptions;
358+
userHandler = handler!;
359+
}
360+
return https._onRequestWithOptions(userHandler, { ...this.options, ...opts } as any);
361+
},
347362
/**
348363
* Declares a callable method for clients to call using a Firebase SDK.
349364
* @param handler A method that takes a data and context and returns a value.

0 commit comments

Comments
 (0)