Skip to content

fix(cors): defer parameter resolution in https callables and requests#1903

Open
inlined wants to merge 1 commit into
masterfrom
fix-cors-param-resolution
Open

fix(cors): defer parameter resolution in https callables and requests#1903
inlined wants to merge 1 commit into
masterfrom
fix-cors-param-resolution

Conversation

@inlined

@inlined inlined commented Jun 13, 2026

Copy link
Copy Markdown
Member

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 also 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 that resolveCorsOrigin successfully extracts runtime values from StringParam and ListParam Expressions.
  • 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 ensure no regressions.

@inlined inlined requested a review from wandamora June 13, 2026 01:10

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request adds support for configuring CORS options using parameter expressions (such as defineString and defineList) in both v1 and v2 HTTPS and callable functions. It introduces a resolveCorsOrigin helper to dynamically resolve these expressions per request and includes comprehensive unit tests. The review feedback highlights a critical bug in src/v1/function-builder.ts where arguments is incorrectly used inside an arrow function, which will cause runtime failures. Additionally, it is recommended to use typeof checks instead of arguments.length in src/v1/providers/https.ts for better robustness and consistency.

Comment on lines +345 to +361
onRequest: (
optsOrHandler:
| https.HttpsOptions
| ((req: https.Request, resp: express.Response) => void | Promise<void>),
handler?: (req: https.Request, resp: express.Response) => void | Promise<void>
) => {
let opts: https.HttpsOptions;
let userHandler: (req: https.Request, resp: express.Response) => void | Promise<void>;
if (arguments.length === 1) {
opts = {};
userHandler = optsOrHandler as any;
} else {
opts = optsOrHandler as https.HttpsOptions;
userHandler = handler!;
}
return https._onRequestWithOptions(userHandler, { ...this.options, ...opts } as any);
},

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.

critical

Using arguments inside an arrow function is a critical bug. Arrow functions do not bind their own arguments object; instead, they inherit it from the enclosing scope (in this case, the get https() getter, which has 0 arguments). Consequently, arguments.length === 1 will always evaluate to false, causing the function to incorrectly treat a single handler argument as options and leaving the actual handler undefined, leading to runtime crashes when requests are received.

To fix this, check typeof optsOrHandler === "function" instead of relying on arguments.length.

Suggested change
onRequest: (
optsOrHandler:
| https.HttpsOptions
| ((req: https.Request, resp: express.Response) => void | Promise<void>),
handler?: (req: https.Request, resp: express.Response) => void | Promise<void>
) => {
let opts: https.HttpsOptions;
let userHandler: (req: https.Request, resp: express.Response) => void | Promise<void>;
if (arguments.length === 1) {
opts = {};
userHandler = optsOrHandler as any;
} else {
opts = optsOrHandler as https.HttpsOptions;
userHandler = handler!;
}
return https._onRequestWithOptions(userHandler, { ...this.options, ...opts } as any);
},
onRequest: (
optsOrHandler:
| https.HttpsOptions
| ((req: https.Request, resp: express.Response) => void | Promise<void>),
handler?: (req: https.Request, resp: express.Response) => void | Promise<void>
) => {
let opts: https.HttpsOptions;
let userHandler: (req: https.Request, resp: express.Response) => void | Promise<void>;
if (typeof optsOrHandler === "function") {
opts = {};
userHandler = optsOrHandler;
} else {
opts = optsOrHandler as https.HttpsOptions;
userHandler = handler!;
}
return https._onRequestWithOptions(userHandler, { ...this.options, ...opts } as any);
},

Comment thread src/v1/providers/https.ts
Comment on lines 61 to 77
export function onRequest(
handler: (req: Request, resp: express.Response) => void | Promise<void>
optsOrHandler:
| HttpsOptions
| ((req: Request, resp: express.Response) => void | Promise<void>),
handler?: (req: Request, resp: express.Response) => void | Promise<void>
): HttpsFunction {
return _onRequestWithOptions(handler, {});
let opts: HttpsOptions;
let userHandler: (req: Request, resp: express.Response) => void | Promise<void>;
if (arguments.length === 1) {
opts = {};
userHandler = optsOrHandler as any;
} else {
opts = optsOrHandler as HttpsOptions;
userHandler = handler!;
}
return _onRequestWithOptions(userHandler, opts as DeploymentOptions);
}

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.

medium

For consistency and robustness, it is highly recommended to check typeof optsOrHandler === "function" instead of relying on arguments.length. This avoids potential issues if the function is refactored into an arrow function in the future and aligns with modern TypeScript practices.

Suggested change
export function onRequest(
handler: (req: Request, resp: express.Response) => void | Promise<void>
optsOrHandler:
| HttpsOptions
| ((req: Request, resp: express.Response) => void | Promise<void>),
handler?: (req: Request, resp: express.Response) => void | Promise<void>
): HttpsFunction {
return _onRequestWithOptions(handler, {});
let opts: HttpsOptions;
let userHandler: (req: Request, resp: express.Response) => void | Promise<void>;
if (arguments.length === 1) {
opts = {};
userHandler = optsOrHandler as any;
} else {
opts = optsOrHandler as HttpsOptions;
userHandler = handler!;
}
return _onRequestWithOptions(userHandler, opts as DeploymentOptions);
}
export function onRequest(
optsOrHandler:
| HttpsOptions
| ((req: Request, resp: express.Response) => void | Promise<void>),
handler?: (req: Request, resp: express.Response) => void | Promise<void>
): HttpsFunction {
let opts: HttpsOptions;
let userHandler: (req: Request, resp: express.Response) => void | Promise<void>;
if (typeof optsOrHandler === "function") {
opts = {};
userHandler = optsOrHandler;
} else {
opts = optsOrHandler as HttpsOptions;
userHandler = handler!;
}
return _onRequestWithOptions(userHandler, opts as DeploymentOptions);
}

Comment thread src/common/providers/https.ts Dismissed
Comment thread src/v1/providers/https.ts Dismissed
Comment thread src/v2/providers/https.ts Dismissed
Comment thread src/v2/providers/https.ts Dismissed
### 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 also 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 that `resolveCorsOrigin` successfully extracts runtime values from `StringParam` and `ListParam` Expressions.
- 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 ensure no regressions.

### Sample Commands
`npm test`
@inlined inlined force-pushed the fix-cors-param-resolution branch from 993412b to 60b6849 Compare June 13, 2026 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants