Fix devcontainer localEnv/containerEnv substitution when variable is undefined

[im-0]

Existing code replaces only the variables that are present in the environment. It ignores references to undefined variables and leaves them as is.

Consider following devcontainer.json:

{
  ...
  "remoteEnv": {
    "RUSTFLAGS": "${localEnv:RUSTFLAGS}"
  },
  ...
}

Existing code will set RUSTFLAGS inside the dev container to a correct value only if RUSTFLAGS is defined outside the dev container. If RUSTFLAGS is not defined outside, Zed will erroneously set RUSTFLAGS to ${localEnv:RUSTFLAGS}.

This patch fixes this by replacing such references with either empty strings or provided default values (${localEnv:VARNAME:default}).

This patch also removes replacement of \ with / for environment variables. Existing code makes no sense as simple replacement will not magically make Windows paths valid on *nix. It is also guaranteed to break things: think about passing passwords via env vars, for example.

I also noticed that, for some reason, existing code serializes k-v maps to JSON, then tries to replace the references, and after that deserializes modified JSON back. I believe that this is horribly wrong and may have some security implications. This is out of the scope of this patch. And I hope that somebody from the Zed team will provide the reasoning behind this code.

Self-Review Checklist:

• I've reviewed my own diff for quality, security, and reliability
• Unsafe blocks (if any) have justifying comments
• The content is consistent with the UI/UX checklist
• Tests cover the new/changed behavior
• Performance impact has been considered and is acceptable

Closes #ISSUE This pull request is also a bug report.

Release Notes:

• Fixed substitution of ${localEnv:VARNAME} and ${containerEnv:VARNAME} in devcontainer.json when variable is not defined

[im-0]

v0.232.2 is still affected.

[KyleBarton]

Hey, thanks for this! Left a few comments on implementation.

Also re:

I also noticed that, for some reason, existing code serializes k-v maps to JSON, then > tries to replace the references, and after that deserializes modified JSON back. I
believe that this is horribly wrong and may have some security implications.

Can you expand on your security concerns here? I'm not sure I understand.

And I hope that somebody from the Zed team will provide the reasoning behind this code.

Since these variables can be referenced anywhere in the devcontainer.json object, replacing and re-deserializing seems expeditious. The alternatives would be to perform replacements field-by-field (which I think would amount to the same work, just more verbose) or create a custom "printing" layer that performs the replacement just-in-time, which was complexity that didn't seem necessary. Open to feedback if you think there's a better approach though.

[im-0]

Can you expand on your security concerns here? I'm not sure I understand.

I have not reviewed your code thoroughly, but it looks like local environment variables referenced in devcontainer.json may affect how Zed parses devcontainer.json and add arbitrary new structures there. Like, sneak in a new onCreateCommand that does not exist in the original devcontainer.json. This is not catastrophic on its own, but pretty bad.

Also, environment values may contain backslashes and quotes and new lines that will break JSON structure and lead to an unexpected errors.

Since these variables can be referenced anywhere in the devcontainer.json object, replacing and re-deserializing seems expeditious.

Not anywhere, but only on the value side. And only inside the existing JSON strings.

Open to feedback if you think there's a better approach though.

I think it is possible to deserialize into serde_json::Value, then traverse it and replace all localEnv (and localWorkspaceFolder etc.) references inside any Value::String. Then deserialize modified serde_json::Value into the "final" struct. containerEnv replacement is trivial as you just need to do something like remote_env.values_mut().for_each(...).

Another option is to create a simple String wrapper struct with custom deserializer that deserializes the inner String and then substitutes localEnv there. But this will probably require adding unwrapping all over the place.

[im-0]

I think it is possible to deserialize into serde_json::Value, then traverse it and replace all localEnv (and localWorkspaceFolder etc.) references inside any Value::String. Then deserialize modified serde_json::Value into the "final" struct. containerEnv replacement is trivial as you just need to do something like remote_env.values_mut().for_each(...).

Tried to implement this ^, please see new commit.

[KyleBarton]

Awesome, yes I really like this direction, thanks for the contribution. My one blocking concern is the syntax around replace_environment_variables, as mentioned in my comment below. A more generic ${} parser would work, as you've suggested. But an alternative smaller change could be to just make the third argument a prefix string which the function handles, so that the caller can pass in:

Self::replace_environment_variables(
                        string,
                        "localEnv", // <-- a prefix which the function will then appropriately wrap in `${...`
                        &self.local_environment,
                    );

Let me know what you think!

[im-0]

But an alternative smaller change could be to just make the third argument a prefix string which the function handles, so that the caller can pass in:

Done. Now replace_environment_variables() accepts environment_source string and constructs the prefix internally:

        let prefix = format!("${{{environment_source}:");

[im-0]

Rebased on current main to resolve a merge conflict.