.env-based secret param binding for Functions#10714
Conversation
…e params right now
There was a problem hiding this comment.
Code Review
This pull request introduces experimental support (functionsEnvParams) for storing references to Secrets in .env files for Cloud Functions deployments. It partitions secret references (prefixed with &) from standard environment variables, parses and validates them, and applies overrides to the build's secret environment variables. The review feedback identifies several critical issues in the validation and parsing logic: 1) validateKey will fail on keys starting with & unless the prefix is stripped first; 2) the regex anchors \A and \z are unsupported in JavaScript and will cause version validation to fail; 3) SECRET_REF_RE contains a redundant double ^ anchor and is too restrictive to support common secret names containing uppercase letters or underscores; and 4) overriding an existing secret with an empty reference will incorrectly set the secret name to an empty string instead of defaulting to the key name.
| const validationErrors: (KeyValidationError | SecretValidationError)[] = []; | ||
| for (const key of Object.keys(envs)) { | ||
| try { | ||
| validateKey(key); |
There was a problem hiding this comment.
When parseStrict is called, it iterates over all keys in envs and calls validateKey(key). If a key starts with & (e.g., &API_KEY), validateKey will throw a KeyValidationError because & is not a valid character according to the regex /^[A-Z_][A-Z0-9_]*$/. This prevents any successful deployment when secrets are defined in the .env file. We should strip the leading & before validating the key.
| const validationErrors: (KeyValidationError | SecretValidationError)[] = []; | |
| for (const key of Object.keys(envs)) { | |
| try { | |
| validateKey(key); | |
| const validationErrors: (KeyValidationError | SecretValidationError)[] = []; | |
| for (const key of Object.keys(envs)) { | |
| try { | |
| validateKey(key.startsWith("&") ? key.slice(1) : key); |
| if (!version) { | ||
| return; | ||
| } | ||
| if (version !== "latest" && !/\A\d+\z/.test(version)) { |
There was a problem hiding this comment.
The regular expression \A\d+\z uses \A and \z anchors, which are not supported in JavaScript/TypeScript RegExp. In JS, \A is treated as a literal A and \z as a literal z, meaning this check will always fail for valid integer versions (e.g., 4). Use standard JS anchors ^ and $ instead.
| if (version !== "latest" && !/\A\d+\z/.test(version)) { | |
| if (version !== "latest" && !/^\d+$/.test(version)) { |
There was a problem hiding this comment.
they what??????????
| const SECRET_REF_RE = new RegExp( | ||
| "^" + // begin line | ||
| "^[a-z]([-a-z0-9]*[a-z0-9])?" + // cloud resource ID | ||
| "(?:@(.+))?" + // optional reference to a specific version | ||
| "$", // end line | ||
| ); |
There was a problem hiding this comment.
The SECRET_REF_RE regular expression has a redundant double ^ anchor due to string concatenation ("^" + "^[a-z]..."). Additionally, it is too restrictive as it does not allow uppercase letters or underscores, which are extremely common in secret names (e.g., STRIPE_SECRET). We should simplify the regex and allow alphanumeric characters, underscores, and hyphens.
| const SECRET_REF_RE = new RegExp( | |
| "^" + // begin line | |
| "^[a-z]([-a-z0-9]*[a-z0-9])?" + // cloud resource ID | |
| "(?:@(.+))?" + // optional reference to a specific version | |
| "$", // end line | |
| ); | |
| const SECRET_REF_RE = new RegExp( | |
| "^" + // begin line | |
| "[a-zA-Z0-9_-]+" + // cloud resource ID | |
| "(?:@(.+))?" + // optional reference to a specific version | |
| "$", // end line | |
| ); |
| endpoint.secretEnvironmentVariables?.forEach((envVar) => { | ||
| if (envVar.key === key) { | ||
| needEnvInsert = false; | ||
| envVar.secret = resourceId; |
There was a problem hiding this comment.
When overriding an existing secret environment variable, if secretRef is empty (e.g., &FOO=), resourceId will be "". Setting envVar.secret = resourceId; directly will overwrite the secret name with an empty string, which is invalid. It should default to the key name, matching the behavior used when inserting a new secret environment variable.
| envVar.secret = resourceId; | |
| envVar.secret = resourceId === "" ? key : resourceId; |
Pulls &-defined secrets from the Functions .env files and propagates them into the Functions Build's SecretEnvVars (and from there, into the resolved Backend, and from there, into the JSON uploaded to Cloud Functions/Cloud Run).
Experimental-flagged.
Params support not included in this PR, to reduce the size of the review. This is obviously going to be a terrible user experience before params are supported (hence the flag) but this should be enough to test the actual binding behavior.