[Dotenv] Don't truncate external env vars containing $ when referenced via ${...} indirection#64386
Merged
Merged
[Dotenv] Don't truncate external env vars containing $ when referenced via ${...} indirection#64386
Conversation
…d via ${...} indirection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #64148, covering the indirect-propagation variant of the same regression family.
When a Dotenv-loaded var references an OS-provided var via
${VAR}indirection (e.g.LOCK_DSN=${REDIS_DSN}withREDIS_DSN=secret$wordin the OS environment), the substituted value is silently truncated at the first literal$.resolveLoadedVars()is a multi-pass loop: pass 1 substitutes${REDIS_DSN}and writessecret$wordback to$_ENV[LOCK_DSN]; pass 2 re-scans the value, treats$wordas an undefined reference, substitutes the empty string, and the final value becomessecret.#64148 prevented externally-present host vars from being added to
loadedRawVars(so they are not themselves re-scanned). It did not protect external values pulled in via substitution. This patch closes that gap inresolveVariables(): when the looked-up name is not in$loadedVars, the substituted value's$chars are converted to the existing\x00literal marker before being spliced in. The terminalstr_replaceat the end ofresolveLoadedVars()(and the equivalents inresolveValue()andresolveEnvKey()) turns them back into$. The gate on!isset($loadedVars[$name])preserves the multi-pass design for chained Dotenv refs likeA=${B},B=${C},C=literal.Two adjacent issues on the same code path are fixed in the same patch rather than as follow-ups.
The first: with
.envcontainingFOO=${FOO:-default}, OS-providedFOO='value$(id)', and the user callingDotenv::overload()(orloadEnv(..., overrideExistingVars: true)), the self-referencing branch ofresolveLoadedVars()temporarily restores the OS value back into$_ENV[FOO]so the:-defaultlookup can see it. Pass 1 substitutes it intoFOO; pass 2 reads$_ENV[FOO]='value$(id)',resolveCommands()matches$(id)and runs it throughProcess::fromShellCommandline(). The regression test below demonstrates this concretely: pre-fix,idis actually invoked and its real output ends up in$_ENV[FOO]. The fix applies the same\x00protection to the OS value when exposing it back to$_ENV/$_SERVER.The second:
$this->overriddenValues = [](and the parser cursor properties) is reset only in the success path ofresolveLoadedVars(). On aVariableCircularReferenceException, the staleoverriddenValuescarries into the nextload()call on the same Dotenv instance and can resurrect a value the OS no longer provides (concretely, when the next.envuses a self-referencing default for the same name). The fix moves the cleanup into afinally, and also resets$this->pendingPutenvfor symmetry.Best reviewed ignoring whitespaces.