Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #103: Escape double quotes in environment variables #104

Closed
wants to merge 3 commits into from

Conversation

MoChilia
Copy link
Member

@MoChilia MoChilia commented Jun 2, 2023

Description

This PR is going to escape double quotes in environment variables to fix #103.

Un-escaped double quotes will break parsing of the docker run command and results in a misleading message an error. In the previous version, we didn't check whether there are double quotes in the environment variables.

cli/src/main.ts

Lines 53 to 59 in 68f0d36

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]}" `;
}
}

In this version, we aim to scan the incoming environment variables and escape the double quotes first with the function:

cli/src/utils.ts

Lines 97 to 99 in ee92c5a

export const escapeDoubleQuote = (value: string): string => {
return value.replace(/"/g, '\\"');
}

Test workflows

Test with double quotes in env

@MoChilia MoChilia requested a review from jiasli June 2, 2023 03:51
@MoChilia MoChilia self-assigned this Jun 2, 2023
@MoChilia MoChilia marked this pull request as draft June 2, 2023 04:01
src/utils.ts Outdated
@@ -93,3 +93,7 @@ export const checkIfEnvironmentVariableIsOmitted = (key: string): boolean => {

return omitEnvironmentVariablesWithPrefix.some((prefix: string) => key.toUpperCase().startsWith(prefix));
}

export const escapeDoubleQuote = (value: string): string => {
return value.replace(/"/g, '\\"');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex replacement can be much slower than a simply substring replace in some browsers. Alternatively, we can consider

Suggested change
return value.replace(/"/g, '\\"');
return value.replaceAll('"', '\\"');

See https://khef.co/2019-javascript-string-replace-regex-vs-substring/

@@ -93,3 +93,7 @@ export const checkIfEnvironmentVariableIsOmitted = (key: string): boolean => {

return omitEnvironmentVariablesWithPrefix.some((prefix: string) => key.toUpperCase().startsWith(prefix));
}

export const escapeDoubleQuote = (value: string): string => {
return value.replaceAll('"', '\\"');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation uses double quotes to quote the environment variable:

environmentVariables += ` -e "${key}=${process.env[key]}" `;

However, only escaping double quotes may not be sufficient. Command Substitution can happen in double quotes, which can also cause shell injection:

$ docker run -e "MY_KEY=$(whoami)" -it mcr.microsoft.com/azure-cli env
...
MY_KEY=user2

Note that whoami gets executed.

Using single quotes to quote the environment variable is safer as this usage preserves the literal value of each character within the quotes.

However, this is still not the optimal solution as we need to care about single quotes' escaping. To make things more complicated:

A single quote may not occur between single quotes, even when preceded by a backslash.
- https://www.gnu.org/software/bash/manual/bash.html#Single-Quotes

See how weird it becomes:

# test.py
import shlex
print(shlex.quote("Single quote ' in string"))

# output
'Single quote '"'"' in string'

Is there a method in TypeScript that is similar to Python's subprocess.Popen which takes args as a list, instead of string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it tested in action test, both environment variables with Command Substitution and single quote can pass the test.

  1. About Command Substitution: Not sure why Command Substitution didn't happen in the test, but it won't be a concern in this case. Since whether it happens or not, as long as there are no double quotes input in docker run -e which ensured by the function escapeDoubleQuote, the command won't fail.
  2. About using single quotes to quote the environment variables: It is not working in this situation. We have to use double quotes to quote the env.
  3. About single quotes: Single quotes work well in this situation with no need to escape.
  4. About taking args as a list: I agree with this great suggestion. It is a safer way to pass env. Will investigate on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exported environment variables w/quotes break the docker run command
2 participants