Exported environment variables w/quotes break the docker run command

[MT-Jacobs]

Let's start with my branch of Azure/cli here as the diff from source shows how I enabled logging of the docker run command to find the issue: https://github.com/MT-Jacobs/azure-cli

The file I edited is in src/main.ts.

Anyhow -

If you run Azure CLI commands, you might choose to pass along an environment variable to the docker run command like so:

docker run ...
-e "COMMIT_MESSAGE=Message here"
...
mcr.microsoft.com/azure-cli:2.48.1  bash --noprofile --norc
...

However, if the variable has quotes in it - like can often happen if you, say, supplied a commit message for a revert in Github - you get the following:

docker run ...
-e "COMMIT_MESSAGE=Revert "Original commit message" (#882)"
...
mcr.microsoft.com/azure-cli:2.48.1  bash --noprofile --norc
...

these un-escaped quotes break parsing of the docker run command and results in a misleading message:

Error: Error: docker: invalid reference format: repository name must be lowercase.
See 'docker run --help'.

Environment variables need to be properly escaped. The issue is in this area of the script, I believe:

        let environmentVariables = '';
        for (let key in process.env) {
            // if (key.toUpperCase().startsWith("GITHUB_") && key.toUpperCase() !== 'GITHUB_WORKSPACE' && process.env[key]){
            if (!checkIfEnvironmentVariableIsOmitted(key) && process.env[key]) {
                environmentVariables += ` -e "${key}=${process.env[key]}" `;
            }
        }

[jiasli]

This is a nice finding.

environmentVariables string is constructed by appending process.env without proper escaping:

cli/src/main.ts

Lines 54 to 59 in 68f0d36

	for (let key in process.env) {
	// if (key.toUpperCase().startsWith("GITHUB_") && key.toUpperCase() !== 'GITHUB_WORKSPACE' && process.env[key]){
	if (!checkIfEnvironmentVariableIsOmitted(key) && process.env[key]) {
	environmentVariables += ` -e "${key}=${process.env[key]}" `;
	}
	}

Then the dockerCommand gets executed through exec.exec:

cli/src/main.ts

Line 143 in 68f0d36

exitCode = await exec.exec(`"${dockerTool}" ${dockerCommand}`, [], execOptions);

I am not an expert on TypeScript, but I can provide some insights from Python's perspective (Azure CLI itself is written in Python).

Python's subprocess.Popen accepts args as either a list or string:

1. If args is a list, the subprocess module takes care of escaping and quoting of arguments.
2. If args is a string, the caller must ensure arguments are correctly escaped and quoted to avoid shell injection vulnerabilities, possibly with shlex.quote().

See

• https://docs.python.org/3/library/subprocess.html#frequently-used-arguments
• https://docs.python.org/3/library/subprocess.html#security-considerations

Apparently, in Azure/cli GitHub Action's implementation, it is the second case - args is a string, but not escaped correctly. We need to investigate how to achieve the same in TypeScript to avoid such shell injection.