feat: accept RegExp values in Expression/thenElse for CORS options - #1944
IzaakGough wants to merge 17 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extends the Expression type and related helper functions to support RegExp and Array<string | RegExp> types, enabling dynamic selection of CORS origins via ternary expressions. It also updates the cors option in HTTPS options to use the shared CorsOption type and adds comprehensive unit tests. However, a high-severity issue was identified in src/params/types.ts where a single RegExp (not wrapped in an array) falls through to the default else block in refOf, resulting in an unquoted string representation that is invalid in CEL. A suggestion has been provided to explicitly handle RegExp and wrap its string representation in JSON.stringify.
A single RegExp (not wrapped in an array) passed to thenElse fell through to arg.toString(), producing an unquoted /pattern/ in the generated CEL string, which is invalid and fails at deploy time.
…pression-for-cors-options
Replace the eight inline copies of the Expression type bound with a single exported ExpressionValue alias, so future additions to the set of values an Expression can resolve to only need editing in one place. Also close two gaps in the new tests: - the nested thenElse case resolved the outer true branch, so the nested expression was never evaluated. Drive it from the false branch instead and assert both inner branches. - the onRequest CORS case only asserted the true branch, so an implementation that always returned ifTrue would have passed. Add the false-branch case, asserting the non-matching origin is not allowed.
…pression-for-cors-options
…pression-for-cors-options
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
Nice work @IzaakGough, solid PR. Checked out the branch, ran the specs and compiled the #1943 repro against it, all good. The refOf fix and the two-branch E2E preflight tests are exactly the right shape. This all makes sense to me, but since it widens the public API (Expression, CorsOption, new ExpressionValue export) we'll run it by the Firebase team before it lands.
Small asks inline. One more on the issue itself: this fully fixes #1943 as filed, but the author also mentions wanting regex CORS patterns read directly from .env files (via #1903), which this doesn't cover (an env var string still won't become a RegExp). Since merge auto-closes the issue, please reply on the thread first clarifying that. Whenever a fix partially addresses what a reporter wants, say so before the close, otherwise they reopen confused.
CorsOption is referenced by HttpsOptions.cors in both providers but was not in either module's export list, so api-extractor reported ae-forgotten-export and the docs rendered the type unlinked.
…pression-for-cors-options
…pression-for-cors-options
…pression-for-cors-options
…pression-for-cors-options
…pression-for-cors-options
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
This looks like the right general design, requesting changes for some things below.
Widening at the Expression level looks forced by the hierarchy (thenElse returns a TernaryExpression, which extends Expression<T>), and I checked the widening can't leak a RegExp into anything serialized: tsc rejects Expression<RegExp> on region, serviceAccount, memory, omit and networkInterface.tags.
ExpressionValue named the wider set an expression can resolve to, but the narrower set a param can hold was still spelled out literally in CompareExpression, ParamSpec, WireParamSpec, ParamOptions and Param. Extract it so both constraints have names and the distinction between them is visible.
CorsOption is public on the v1.https and v2.https namespaces, so it needs its own JSDoc block. Also mention the param ternary form on the cors option itself in both v1 and v2, since that is where a user looks for it.
RegExp has no CEL literal form, so a cors expression holding one is only safe because cors is resolved per request and never serialized. Pin that assumption in v1 and v2 rather than leaving it implicit.
Covariance makes it assignable to Expression<Array<string | RegExp>>, so it reads as removable without the note.
…pression-for-cors-options
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
I need to spend some more time wrapping my head around all this. It's a part of functions I haven't looked at too much. In the meantime I've asked @Berlioz to review as well as he knows more of it. But I want to understand and be confident in this PR.
| | Expression<string[]> | ||
| | Expression<RegExp> | ||
| | Expression<Array<string | RegExp>> | ||
| | boolean |
There was a problem hiding this comment.
Should we also support Expression?
|
Sorry to jump and and say things right before running off for vacation, but my first instinct here is wondering whether doing this kind of invasive surgery on the params type system is worth the risk when there's a perfectly reasonable hack available (we control how options are set by the SDK, so we can just special-case the cors option for anything beginning and ending with If we do decide that type surgery is worth doing in this case, this PR pretty much LGTM though. |
4 participants
Fixes #1943
Expression<T>was constrained tostring | number | boolean | string[], so a param ternary could not select betweenRegExpvalues even thoughcorsaccepts them. Users hit a type error when writing something likeparams.defineBoolean("X").thenElse(/a\.com$/, /b\.com$/)for a v2 HTTPS function'scorsoption.This widens the
Expressiontype parameter to a new exportedExpressionValueunion that also coversRegExpandArray<string | RegExp>, and adds those expression forms toCorsOption.refOfalso gains aRegExpcase. That one is not a deploy fix:corsis resolved per request byresolveCorsOriginand has no entry inoptionsToEndpoint,optionsToTriggeroroptionsToTriggerAnnotations, so a cors expression never reaches the wire manifest. What the case fixes is the diagnostic string, reached via.toCEL(),.value()andtoJSON(), where aRegExppreviously stringified to{}. A test in both v1 and v2 pins that cors stays out of the manifest, since that is the assumption the change rests on.relnote: none