Repository navigation
Fix variable interpolation in .env files loaded through envFile - #14828
Abdelhadi Salmaoui (abdelhadi703) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Interpolation incorrectly expands quoted or escaped literals, and the tests are incomplete and environment-dependent.
2 open findings
What changed in this PR
Adds dotenv-style ${VAR} interpolation for debugger envFile parsing.
Changes:
- Resolves references from prior entries and the process environment.
- Preserves unknown references.
- Adds tests and changelog documentation.
| File | Description |
|---|---|
Extension/src/Debugger/ParsedEnvironmentFile.ts |
Implements interpolation. |
Extension/test/scenarios/SingleRootProject/tests/ParsedEnvironmentFile.test.ts |
Adds interpolation tests. |
Extension/CHANGELOG.md |
Documents the fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The quotes come off before the references are resolved, so VALUE='${SECRET}' was expanded and
a \${SECRET} reference kept its backslash while losing its meaning. Both are literal in dotenv,
and both were literal before the interpolation existed.
Adds the missing test for the process-environment fallback the changelog advertises.
|
Both addressed in 66ea67b. The quotes came off before the references were resolved, so you are right on both counts: The process-environment fallback has a test now as well: it sets a name of its own, parses a reference to it, and restores the previous value in a 13 passing against the first commit of this branch. The two literal tests fail there -- |
| if (!singleQuoted) { | ||
| value = value.replace(/(\\)?\$\{([\w\.\-]+)\}/g, (full: string, escape: string | undefined, name: string): string => { | ||
| if (escape) { | ||
| return full.substring(1); |
| * Reduce CPU usage when resolving headers in large Windows workspaces. | ||
|
|
||
| ### Bug Fixes | ||
| * Fix `${VAR}` interpolation in `.env` files used by `envFile`: references to variables defined earlier in the same file (or in the OS environment) are now resolved. Single-quoted values and `\${VAR}` are left as written. [#13781](https://github.com/microsoft/vscode-cpptools/issues/13781) |
The parser's output goes through expandAllStrings straight after it (configurationProvider.expand),
which substitutes every ${...} it knows and warns about the rest. Writing a literal back as a bare
${VAR} therefore did not keep it literal: the expansion either replaced it from the launch
configuration or reported "Invalid variable reference" -- the very symptom this change set out to
remove. Both literal forms now come out as ${dollar}{VAR}, the one form that expansion leaves alone
and collapses to a plain $ once it is finished.
Drops the changelog entry, since those are written separately before a release.
|
You are right, and I had this wrong. I went looking for a later expansion before writing the previous commit and concluded there was none, because Both literal forms are now written back as Run through the expansion rule from On the wording finding: the changelog entry is gone rather than corrected, for the same reason as the other PR -- those are written separately before a release. 14 tests pass. The path through |
.envfiles are supposed to support variable interpolation, but when such a file isloaded through
envFile, the values keep their${...}references and the variable isnever substituted.
With a file like
EMAILcame out as the literal${USER}@example.org. Later, when the debugconfiguration gets expanded,
${USER}is looked up as alaunch.jsonvariable, isn'tfound, and prints "Invalid variable reference ${USER} in string". The env var that was
meant to land in the debuggee doesn't.
I made
ParsedEnvironmentFileresolve${VAR}while it parses each line, against thevariables already read from the file plus the process environment. A reference to a name we
don't know is left untouched, so
${workspaceFolder}and the other launch.json variableskeep working exactly as before.
Only a reference the value actually asked to expand is expanded. A single-quoted value and a
\${VAR}reference are literal in dotenv, and both were literal here before this change, sothey stay literal now.
Keeping them literal takes more than emitting them unchanged. This parser's output goes through
expandAllStringsimmediately afterwards (configurationProvider.expand), which substitutesevery
${...}it knows and warns about the rest, so a bare${VAR}either got replaced fromthe launch configuration or printed the same "Invalid variable reference" warning the issue is
about. Both literal forms are written back as
${dollar}{VAR}instead -- the one form thatexpansion leaves alone and collapses to a plain
$once it is done (expand.ts), so thedebuggee is the only place that sees the literal.
Seven tests were added to the existing
ParsedEnvironmentFilesuite: a reference to a variabledefined earlier in the same file, a reference coming from the initial environment, an unknown
reference staying literal, a reference resolved from the process environment, the two literal
forms, and a resolved reference still coming out bare.
Testing: ran the
ParsedEnvironmentFilesuite against the parser alone, compiled with therepo's TypeScript and a local
vscode-nlsstand-in (mocha 10 does not start on Node 26, so thecompiled file runs under a short TDD shim). 14 passing with this change.
I also ran that output through the expansion rule from
expand.ts, which is what the extensiondoes next. Both literal forms reach the debuggee as
${USER}with no warning, while${workspaceFolder}still resolves from the debug configuration. On the previous commit of thisbranch the single-quoted value came out as
admin@example.org, because the expansion took${USER}from the launch variables.The path through
extendDebugConfigurationitself needs the extension host and the packagesfrom the private feed, so it is not covered here.
Fixes #13781.