Skip to content

fix(https): defer CORS param resolution until runtime#1887

Open
CorieW wants to merge 3 commits into
masterfrom
@invertase/fix-cors-param-deploy
Open

fix(https): defer CORS param resolution until runtime#1887
CorieW wants to merge 3 commits into
masterfrom
@invertase/fix-cors-param-deploy

Conversation

@CorieW

@CorieW CorieW commented May 14, 2026

Copy link
Copy Markdown
Member

What This PR Solves

Fixes #1773.

CORS options can be backed by params such as defineList, but v2 HTTPS handlers were resolving those params while the Functions control API was analyzing source code. At that point runtime env values may not exist yet, which can trigger deploy-time warnings or failures like parsing undefined.

This PR keeps the param-backed CORS option unresolved during function definition and resolves it from the CORS middleware at request time instead. It preserves debug CORS behavior and the existing single-origin normalization.

Testing Details

  • Updated v2 HTTPS unit coverage for onRequest({ cors: defineList(...) }) so function analysis can run with FUNCTIONS_CONTROL_API=true before the param value exists, then runtime request handling resolves ORIGINS.
  • Updated v2 HTTPS unit coverage for onCall({ cors: defineList(...) }) with the same deploy-time/runtime split.
  • Manually tested with a local test-project/functions app exporting both corsParamOnRequest and corsParamOnCall, each using cors: defineList("ALLOWED_ORIGINS").
  • Manual test loaded the functions while FUNCTIONS_CONTROL_API=true and ALLOWED_ORIGINS was unset, confirming module analysis no longer resolves/parses the param during deploy-time loading.
  • Manual test then set ALLOWED_ORIGINS=["https://app.example.com","https://admin.example.com"] and verified CORS preflight behavior: allowed origins returned the matching Access-Control-Allow-Origin header, and https://evil.example.com did not.

@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 refactors CORS origin resolution for onRequest and onCall by introducing normalizeCorsOrigin and resolveCorsOrigin helper functions. It also adds support for resolving CORS origins from Expression objects at runtime using a middleware callback. Feedback suggests wrapping the runtime resolution of Expression values in a try-catch block to ensure that any errors encountered while retrieving the value are properly propagated to the Express error handler.

Comment thread src/v2/providers/https.ts
Comment on lines +281 to +286
return (
_requestOrigin: string | undefined,
callback: (err: Error | null, origin?: ResolvedCorsOrigin) => void
): void => {
callback(null, normalizeCorsOrigin(corsOption.value()));
};

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

When corsOption is an Expression, its value is resolved at runtime within the CORS middleware callback. Since corsOption.value() can throw an error (e.g., if a required parameter is missing or invalid at runtime), it is safer to wrap this call in a try-catch block and pass any error to the callback. This ensures the cors middleware can handle the error gracefully by propagating it to the Express error handler via next(err).

    return (
      _requestOrigin: string | undefined,
      callback: (err: Error | null, origin?: ResolvedCorsOrigin) => void
    ): void => {
      try {
        callback(null, normalizeCorsOrigin(corsOption.value()));
      } catch (err) {
        callback(err instanceof Error ? err : new Error(String(err)));
      }
    };

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.

This might be worth doing just to make the error more explicit if corsOption.value() throws.

Comment thread src/v2/providers/https.ts Outdated
@CorieW CorieW force-pushed the @invertase/fix-cors-param-deploy branch from 8a3f9a3 to 20505bc Compare May 14, 2026 15:58
@IzaakGough IzaakGough marked this pull request as ready for review June 11, 2026 12:30

@IzaakGough IzaakGough 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.

Lgtm!

@inlined

inlined commented Jun 12, 2026

Copy link
Copy Markdown
Member

Thanks for doing this. I see the bug that needs to be corrected. But this only fixes the V2 API and not the V1 API. I'm going to try a different approach where we can accept a param in the common library and it resolves the param inside the callback for the express server (since that is only run during live execution it is safe there)

@inlined

inlined commented Jun 13, 2026

Copy link
Copy Markdown
Member

#1903 tries a different approach. By consolidating more logic into /common we can avoid global executions of the param access code as well as removing duplicate code

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.

params.defineList is always parsed as undefined in v6

5 participants