From 1a376c362215a5c7044a08b3e3905b1a5fddc821 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sat, 18 Jul 2026 09:22:37 -0400 Subject: [PATCH] fix: prevent command injection via docker options eval Values supplied through docker options, `--ttl-seconds`, and `-e` flowed into a Bash `eval` during build, deploy, and run, letting a low-privileged user execute arbitrary commands on the host as the dokku user. These arguments are now tokenized and passed through to the container verbatim, without shell expansion. A one-time migration repairs stored labels whose backticks were saved with a stray backslash so Traefik-style rules stay valid on the next deploy. --- docs/advanced-usage/docker-options.md | 6 ++ docs/appendices/0.38.0-migration-guide.md | 1 + plugins/builder-dockerfile/builder-build | 7 +- plugins/builder-herokuish/builder-build | 6 +- plugins/builder-nixpacks/builder-build | 7 +- plugins/builder-pack/builder-build | 7 +- plugins/builder-railpack/builder-build | 7 +- plugins/common/functions | 6 ++ plugins/docker-options/dockeroptions.go | 95 +++++++++++++++-- plugins/docker-options/dockeroptions_test.go | 35 ++++++ plugins/docker-options/functions.go | 100 ++++++++++++++++++ plugins/docker-options/migration_test.go | 52 +++++++++ plugins/docker-options/triggers.go | 4 + plugins/run/internal-functions | 4 + .../bin/scheduler-deploy-process | 7 +- .../bin/scheduler-deploy-process-container | 6 +- plugins/scheduler-docker-local/scheduler-run | 6 +- .../traefik-vhosts/docker-args-process-deploy | 6 +- tests/unit/builder-herokuish.bats | 5 + tests/unit/builder-nixpacks.bats | 5 + tests/unit/builder-pack.bats | 5 + tests/unit/builder-railpack.bats | 5 + tests/unit/core_1.bats | 29 +++++ tests/unit/docker-options.bats | 19 ++++ tests/unit/run_2.bats | 43 ++++++++ 25 files changed, 444 insertions(+), 29 deletions(-) diff --git a/docs/advanced-usage/docker-options.md b/docs/advanced-usage/docker-options.md index 2fcabcb6e..054904e34 100644 --- a/docs/advanced-usage/docker-options.md +++ b/docs/advanced-usage/docker-options.md @@ -64,6 +64,12 @@ Multiple docker options can also be specified in a single call. Each `--flag [va dokku docker-options:add node-js-app deploy "--ulimit nofile=12" "--shm-size 256m" ``` +Option values are stored and passed to the container verbatim. Quoting only controls how a value is split into words - no shell expansion is performed, so `$(...)`, backticks, `$VAR`, and globs are treated literally rather than being interpreted by the shell. This is what lets values such as a Traefik router rule be applied as-is: + +```shell +dokku docker-options:add node-js-app deploy '--label "traefik.http.routers.web.rule=Host(`node-js-app.example.com`) && PathPrefix(`/api`)"' +``` + A misplaced `--process PROC` (i.e. one specified after the app name instead of before it) is honored as a subcommand flag rather than stored as a docker option, so the example above and the equivalent process-scoped form below behave identically: ```shell diff --git a/docs/appendices/0.38.0-migration-guide.md b/docs/appendices/0.38.0-migration-guide.md index 3123ff840..254446dc4 100644 --- a/docs/appendices/0.38.0-migration-guide.md +++ b/docs/appendices/0.38.0-migration-guide.md @@ -26,6 +26,7 @@ - A second round of `:report` additions surfaces every remaining settable-but-unreported property under the same raw/global/computed convention so external tooling can verify drift via `:report --format json` without falling back to a generic bash task. The `ps` plugin gains `--ps-dockerfile-start-cmd` and `--ps-computed-dockerfile-start-cmd`, `--ps-start-cmd` and `--ps-computed-start-cmd`, and the `--ps-skip-deploy` / `--ps-global-skip-deploy` / `--ps-computed-skip-deploy` triple (default `false`). The `builder` plugin gains the `--builder-skip-cleanup` triple (default `false`). The `scheduler` plugin gains the `--scheduler-shell` triple. The `proxy` plugin gains the `--proxy-proxy-port` and `--proxy-proxy-ssl-port` triples and exposes the raw `disabled` property as `--proxy-disabled` / `--proxy-computed-disabled`, alongside the existing inverted `--proxy-enabled`. The `openresty` plugin gains `--openresty-global-log-level` and `--openresty-computed-log-level` (default `ERROR`). The `nginx` plugin gains the `--nginx-nginx-service-command` triple. The `scheduler-k3s` plugin gains `--scheduler-k3s-global-token`, but the value is masked as `*******` in default stdout output; the raw value is returned only when the report is requested via `--format json` or when this flag is queried explicitly by name. The traefik `dns-provider-` keys now follow the same explicit-query rule - previously they were unmasked only for `--format json`, but a query like `dokku traefik:report --traefik-dns-provider-cf_api_key` now returns the actual value instead of `*******`. - All Go-implemented plugins (`app-json`, `apps`, `builder`, `buildpacks`, `builds`, `cron`, `docker-options`, `logs`, `network`, `ports`, `proxy`, `ps`, `registry`, `resource`, `scheduler`, `scheduler-k3s`, `storage`) now emit JSON keys from `:report --format json` without the `-` head segment, matching the shape bash plugins have always emitted. For example, `dokku ps:report myapp --format json` now contains `stop-timeout-seconds`, `global-stop-timeout-seconds`, and `computed-stop-timeout-seconds` keys. The CLI flag names (`--ps-stop-timeout-seconds`, etc.) are unchanged, and `:set` semantics are unchanged. For backwards compatibility during the 0.38.x patch series, the old `-` JSON keys are emitted side-by-side with the new keys, so external scripts reading either shape continue to work. The legacy keys will be dropped in a future major release. External JSON consumers should migrate to the new key shape. - The `scheduler-k3s` plugin now manages env config and the dokku-generated image pull Secret as their own helm releases with stable names (`config-{app}` and `pull-secret-{app}`) rather than bundling them into the app helm chart with a per-deploy timestamp suffix (`env-{app}.{ts}` / `ims-{app}.{ts}`). This fixes two bugs: a helm rollback of the app chart no longer deletes Secrets that older ReplicaSets still reference, and the Deployment's `imagePullSecrets` list no longer accumulates references to nonexistent Secrets across deploys. The next deploy of an app switches the Deployment's `envFrom` and `imagePullSecrets` references to the stable names and prunes any leaked entries; existing live Deployments do not need to be patched manually. App rename now also uninstalls the old `tls-{app}`, `config-{app}`, and `pull-secret-{app}` releases under the previous app name; the new name's releases are recreated on the next deploy or certs sync. +- **New in 0.38.25:** Values supplied through docker options, `dokku run`'s `-e`/`--env` flag, and `--ttl-seconds` are no longer evaluated by the shell when assembling a container's arguments; they are now tokenized and passed through verbatim. This closes a command-injection vector where a `$(...)` or backtick expression in one of these values executed on the host as the `dokku` user during build, deploy, or run. As a result, shell metacharacters such as `$(...)`, backticks, `$VAR`, and globs in these values are treated literally instead of being expanded, and `--ttl-seconds` must now be a plain integer. Existing Traefik docker-options labels (those whose label key begins with `traefik.`) whose backticks were stored with a stray backslash are repaired automatically the first time `dokku` runs after the upgrade, so they become valid on the next deploy. - The storage plugin now treats persistent volumes as named, scheduler-aware first-class resources via `storage:create`, `storage:mount`, `storage:set`, and `storage:destroy`. The legacy `storage:mount :` colon form continues to work on docker-local apps but is deprecated; on k3s apps it is rejected. Existing colon-form mounts are migrated automatically the first time the new storage plugin runs (during the install trigger) - they appear as `legacy-` entries in `storage:list-entries`. The migration is idempotent and tied to a per-app flag file at `$DOKKU_LIB_ROOT/config/storage/.migrated/`; deleting that file forces a re-scan on the next install. The `storage:ensure-directory` command keeps working but now emits a deprecation warning - prefer `storage:create []` (the path defaults to the same `$DOKKU_LIB_ROOT/data/storage/` location). Storage entry names must now be DNS-1123 labels of 45 characters or less so they can be used verbatim as Helm release and Kubernetes resource names; underscores and uppercase characters that the older `ensure-directory` validator accepted are rejected for new names. The migration synthesizer always uses lowercase hex hashes so existing data is never locked out. ### TLS handshake behavior change diff --git a/plugins/builder-dockerfile/builder-build b/plugins/builder-dockerfile/builder-build index 405cbfbbc..7bb83d31a 100755 --- a/plugins/builder-dockerfile/builder-build +++ b/plugins/builder-dockerfile/builder-build @@ -36,8 +36,11 @@ trigger-builder-dockerfile-builder-build() { DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-build "$APP" "$BUILDER_TYPE")" DOCKER_ARGS+=" $DOKKU_GLOBAL_BUILD_ARGS" - DOCKER_ARGS=" $DOCKER_ARGS " - eval set -- "$DOCKER_ARGS" + declare -a DOCKER_ARGS_ARRAY=() + while IFS= read -r -d '' arg; do + DOCKER_ARGS_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") + set -- "${DOCKER_ARGS_ARRAY[@]}" declare -a DOCKERFILE_ARGS while true; do diff --git a/plugins/builder-herokuish/builder-build b/plugins/builder-herokuish/builder-build index 73e17deb0..2a800bbf8 100755 --- a/plugins/builder-herokuish/builder-build +++ b/plugins/builder-herokuish/builder-build @@ -71,8 +71,10 @@ trigger-builder-herokuish-builder-build() { fi DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-build "$APP" "$BUILDER_TYPE")" - declare -a ARG_ARRAY - eval "ARG_ARRAY=($DOCKER_ARGS)" + declare -a ARG_ARRAY=() + while IFS= read -r -d '' arg; do + ARG_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") local DOKKU_CONTAINER_EXIT_CODE=0 fn-builder-herokuish-ensure-cache "$APP" diff --git a/plugins/builder-nixpacks/builder-build b/plugins/builder-nixpacks/builder-build index f56e8dc00..413f91617 100755 --- a/plugins/builder-nixpacks/builder-build +++ b/plugins/builder-nixpacks/builder-build @@ -37,8 +37,11 @@ trigger-builder-nixpacks-builder-build() { DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-build "$APP" "$BUILDER_TYPE")" DOCKER_ARGS+=" $DOKKU_GLOBAL_BUILD_ARGS" - DOCKER_ARGS=" $DOCKER_ARGS " - eval set -- "$DOCKER_ARGS" + declare -a DOCKER_ARGS_ARRAY=() + while IFS= read -r -d '' arg; do + DOCKER_ARGS_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") + set -- "${DOCKER_ARGS_ARRAY[@]}" declare -a NIXPACKS_ARGS while true; do diff --git a/plugins/builder-pack/builder-build b/plugins/builder-pack/builder-build index 882b676e3..f3a9213ad 100755 --- a/plugins/builder-pack/builder-build +++ b/plugins/builder-pack/builder-build @@ -41,8 +41,11 @@ trigger-builder-pack-builder-build() { DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-build "$APP" "$BUILDER_TYPE")" [[ "$DOKKU_TRACE" ]] && DOCKER_ARGS+=" --env=TRACE=true " - DOCKER_ARGS=" $DOCKER_ARGS " - eval set -- "$DOCKER_ARGS" + declare -a DOCKER_ARGS_ARRAY=() + while IFS= read -r -d '' arg; do + DOCKER_ARGS_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") + set -- "${DOCKER_ARGS_ARRAY[@]}" declare -a PACK_ARGS while true; do diff --git a/plugins/builder-railpack/builder-build b/plugins/builder-railpack/builder-build index 94bfe957c..278a74d52 100755 --- a/plugins/builder-railpack/builder-build +++ b/plugins/builder-railpack/builder-build @@ -36,8 +36,11 @@ trigger-builder-railpack-builder-build() { local DOCKER_ARGS=$(: | plugn trigger docker-args-build "$APP" "$BUILDER_TYPE") DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-build "$APP" "$BUILDER_TYPE")" - DOCKER_ARGS=" $DOCKER_ARGS " - eval set -- "$DOCKER_ARGS" + declare -a DOCKER_ARGS_ARRAY=() + while IFS= read -r -d '' arg; do + DOCKER_ARGS_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") + set -- "${DOCKER_ARGS_ARRAY[@]}" declare -a RAILPACK_ARGS while true; do diff --git a/plugins/common/functions b/plugins/common/functions index 90bac37e8..aec97aeb4 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -245,6 +245,12 @@ fn-plugn-trigger-exists() { return 1 } +fn-docker-args-split() { + declare desc="tokenizes a docker args string into NUL-delimited fields without shell expansion" + declare DOCKER_ARGS="$1" + printf "%s" "$DOCKER_ARGS" | xargs -r printf "%s\0" +} + is_valid_app_name() { declare desc="verify that the app name matches naming restrictions" local APP="$1" diff --git a/plugins/docker-options/dockeroptions.go b/plugins/docker-options/dockeroptions.go index 8d48d293e..77458d053 100644 --- a/plugins/docker-options/dockeroptions.go +++ b/plugins/docker-options/dockeroptions.go @@ -6,7 +6,7 @@ import ( "strings" "github.com/dokku/dokku/plugins/common" - "mvdan.cc/sh/v3/shell" + "mvdan.cc/sh/v3/syntax" ) // DefaultProcessType is the sentinel process-type key used for options that @@ -15,19 +15,23 @@ import ( const DefaultProcessType = "_default_" // SplitOptionString shell-tokenizes input, groups tokens on flag boundaries, -// and returns one re-serialized option per group. A docker-options subcommand -// flag (currently just --process) that lands inside the option content - -// because the user typed it after the app name, where pflag's -// SetInterspersed(false) hands it back as positional - is lifted into the -// returned processes slice rather than stored as a docker option. The caller -// merges those processes with whatever pflag already captured. Empty or -// whitespace-only input returns empty slices. +// and returns one re-serialized option per group. Tokenization honors quotes +// for word boundaries but performs no expansion, so parameter expansions, +// command substitutions, and other shell metacharacters are stored verbatim +// and passed through to the container as written (the bash scheduler splits +// them the same way, without expansion). A docker-options subcommand flag +// (currently just --process) that lands inside the option content - because +// the user typed it after the app name, where pflag's SetInterspersed(false) +// hands it back as positional - is lifted into the returned processes slice +// rather than stored as a docker option. The caller merges those processes +// with whatever pflag already captured. Empty or whitespace-only input returns +// empty slices. func SplitOptionString(input string) (options []string, processes []string, err error) { if strings.TrimSpace(input) == "" { return nil, nil, nil } - fields, err := shell.Fields(input, func(string) string { return "" }) + fields, err := literalFields(input) if err != nil { return nil, nil, fmt.Errorf("Unable to parse docker option: %s", err.Error()) } @@ -77,6 +81,79 @@ func SplitOptionString(input string) (options []string, processes []string, err return options, processes, nil } +// literalFields splits input into shell words using the parser directly, so +// quotes delimit words and are stripped from the stored value, but nothing is +// expanded. Parameter expansions, command substitutions, and other +// metacharacters are preserved verbatim. Malformed input, such as an unbalanced +// quote, returns the parser error. +func literalFields(input string) ([]string, error) { + parser := syntax.NewParser() + var fields []string + for word, err := range parser.WordsSeq(strings.NewReader(input)) { + if err != nil { + return nil, err + } + fields = append(fields, literalWordValue(input, word.Parts)) + } + return fields, nil +} + +// literalWordValue reconstructs the unquoted, unexpanded value of a shell word. +// Literal and single-quoted parts contribute their literal text; double-quoted +// parts have their surrounding quotes dropped while their contents stay +// literal; every other part - parameter expansions, command substitutions, +// arithmetic expansions - contributes its original source text unchanged. +// Backslash escapes are removed the same way the shell removes them during +// quote removal so a value such as `Host(\`app\`)` round-trips to `Host(`app`)`. +func literalWordValue(input string, parts []syntax.WordPart) string { + var sb strings.Builder + for _, part := range parts { + switch p := part.(type) { + case *syntax.Lit: + sb.WriteString(unquoteBackslashes(p.Value, false)) + case *syntax.SglQuoted: + sb.WriteString(p.Value) + case *syntax.DblQuoted: + for _, inner := range p.Parts { + if lit, ok := inner.(*syntax.Lit); ok { + sb.WriteString(unquoteBackslashes(lit.Value, true)) + continue + } + sb.WriteString(input[inner.Pos().Offset():inner.End().Offset()]) + } + default: + sb.WriteString(input[part.Pos().Offset():part.End().Offset()]) + } + } + return sb.String() +} + +// unquoteBackslashes removes backslashes the way the shell does during quote +// removal. Inside double quotes only \$, \`, \", \\, and an escaped newline +// lose their backslash; unquoted, a backslash escapes any following character. +func unquoteBackslashes(s string, inDblQuotes bool) string { + if !strings.Contains(s, "\\") { + return s + } + var sb strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '\\' && i+1 < len(s) { + next := s[i+1] + if next == '\n' { + i++ + continue + } + if !inDblQuotes || next == '$' || next == '`' || next == '"' || next == '\\' { + sb.WriteByte(next) + i++ + continue + } + } + sb.WriteByte(s[i]) + } + return sb.String() +} + // isFlagToken reports whether tok looks like a CLI flag (long or short) rather // than a value. Treating any token that begins with `-` and has more than one // character as a flag matches docker's flag conventions and avoids the need diff --git a/plugins/docker-options/dockeroptions_test.go b/plugins/docker-options/dockeroptions_test.go index 5237c75c3..965d7a8f9 100644 --- a/plugins/docker-options/dockeroptions_test.go +++ b/plugins/docker-options/dockeroptions_test.go @@ -96,6 +96,41 @@ func TestSplitOptionString(t *testing.T) { input: `--build-arg FOO="bar`, wantErr: true, }, + { + name: "command substitution is stored verbatim", + input: "--label x=$(id)", + wantOptions: []string{"--label 'x=$(id)'"}, + }, + { + name: "backtick command substitution is stored verbatim", + input: "--label x=`id`", + wantOptions: []string{"--label 'x=`id`'"}, + }, + { + name: "parameter expansion is stored verbatim", + input: "--label x=$FOO", + wantOptions: []string{"--label 'x=$FOO'"}, + }, + { + name: "metacharacter value that parses is shell-quoted", + input: `--label 'a;b'`, + wantOptions: []string{"--label 'a;b'"}, + }, + { + name: "escaped backtick inside double quotes is unescaped", + input: "--label \"traefik.rule=Host(\\`app.example.com\\`)\"", + wantOptions: []string{"--label 'traefik.rule=Host(`app.example.com`)'"}, + }, + { + name: "escaped dollar inside double quotes is unescaped", + input: "--label \"k=a\\$b\"", + wantOptions: []string{"--label 'k=a$b'"}, + }, + { + name: "unquoted backslash escape is removed", + input: "--label k=a\\ b", + wantOptions: []string{"--label 'k=a b'"}, + }, } for _, tc := range cases { diff --git a/plugins/docker-options/functions.go b/plugins/docker-options/functions.go index fec61a67f..c0957be78 100644 --- a/plugins/docker-options/functions.go +++ b/plugins/docker-options/functions.go @@ -351,3 +351,103 @@ func readLegacyOptionsFile(path string) ([]string, error) { } return lines, nil } + +// traefikLabelMigrationKey gates the one-time pass that repairs docker-option +// labels whose backticks were stored with a stray leading backslash by an +// earlier release's option tokenizer (e.g. Traefik rules like Host(\`app\`)). +const traefikLabelMigrationKey = "migrated-traefik-backticks" + +// migrateTraefikLabelBackticks repairs stored Traefik label options whose +// backticks carry a stray backslash (Host(\`app\`) instead of Host(`app`)) so +// they become valid on the next deploy. Only Traefik --label options (those +// whose label key begins with "traefik.") are considered, and only when they +// contain a backslash-escaped backtick, which correct storage never produces. +// It runs once, guarded by a global property. +func migrateTraefikLabelBackticks() error { + if common.PropertyGet("docker-options", "--global", traefikLabelMigrationKey) == "true" { + return nil + } + + apps, err := common.DokkuApps() + if err != nil { + if errors.Is(err, common.NoAppsExist) { + return common.PropertyWrite("docker-options", "--global", traefikLabelMigrationKey, "true") + } + return err + } + + for _, appName := range apps { + properties, err := common.PropertyGetAll("docker-options", appName) + if err != nil { + return err + } + + for key := range properties { + processType, phase, ok := splitPropertyKey(key) + if !ok { + continue + } + + options, err := GetDockerOptionsForProcessPhase(appName, processType, phase) + if err != nil { + return err + } + + changed := false + for i, option := range options { + if fixed, ok := repairTraefikLabelBackticks(option); ok { + options[i] = fixed + changed = true + } + } + + if changed { + if err := writeDockerOptionsForProcessPhase(appName, processType, phase, options); err != nil { + return err + } + common.LogInfo1(fmt.Sprintf("Repaired docker-options label backticks for %s (%s %s)", appName, processType, phase)) + } + } + } + + return common.PropertyWrite("docker-options", "--global", traefikLabelMigrationKey, "true") +} + +// repairTraefikLabelBackticks removes the stray backslash before a backtick in +// a Traefik --label option (one whose label key begins with "traefik."), +// returning the repaired option and whether anything changed. Backticks are +// Traefik's rule syntax, so the repair is scoped to Traefik labels; non-label +// options, non-Traefik labels, and labels without a backslash-escaped backtick +// are left untouched so an intentional backslash elsewhere is never rewritten. +func repairTraefikLabelBackticks(option string) (string, bool) { + spec, ok := labelSpec(option) + if !ok || !strings.HasPrefix(spec, "traefik.") || !strings.Contains(option, "\\`") { + return option, false + } + return strings.ReplaceAll(option, "\\`", "`"), true +} + +// labelSpec returns the "key=value" portion of a stored --label/-l option with +// the flag and any surrounding shell-quoting removed so the label key can be +// inspected. It reports ok=false for options that do not set a docker label. +func labelSpec(option string) (string, bool) { + s := strings.TrimLeft(option, " ") + if len(s) >= 2 && s[0] == '\'' && s[len(s)-1] == '\'' { + s = s[1 : len(s)-1] + } + switch { + case strings.HasPrefix(s, "--label="): + s = s[len("--label="):] + case strings.HasPrefix(s, "--label "): + s = s[len("--label "):] + case strings.HasPrefix(s, "-l="): + s = s[len("-l="):] + case strings.HasPrefix(s, "-l "): + s = s[len("-l "):] + default: + return "", false + } + s = strings.TrimPrefix(s, "'") + s = strings.TrimPrefix(s, "\"") + return s, true +} diff --git a/plugins/docker-options/migration_test.go b/plugins/docker-options/migration_test.go index 80995008f..7803c9f20 100644 --- a/plugins/docker-options/migration_test.go +++ b/plugins/docker-options/migration_test.go @@ -226,6 +226,58 @@ func TestMigrateLegacyDockerOptionsFiles_SkipsEmptyContentFiles(t *testing.T) { } } +func TestRepairTraefikLabelBackticks(t *testing.T) { + cases := []struct { + name string + in string + want string + changed bool + }{ + {"traefik rule space form", "--label 'traefik.http.routers.web.rule=Host(\\`app.example.com\\`)'", "--label 'traefik.http.routers.web.rule=Host(`app.example.com`)'", true}, + {"traefik rule whole-quoted equals form", "'--label=traefik.http.routers.web.rule=Host(\\`x\\`)'", "'--label=traefik.http.routers.web.rule=Host(`x`)'", true}, + {"non-traefik label left untouched", "--label 'some.key=Host(\\`x\\`)'", "--label 'some.key=Host(\\`x\\`)'", false}, + {"already valid traefik label", "--label 'traefik.rule=Host(`x`)'", "--label 'traefik.rule=Host(`x`)'", false}, + {"non-label option left untouched", "-v /tmp", "-v /tmp", false}, + {"traefik label without backticks", "--label 'traefik.enable=true'", "--label 'traefik.enable=true'", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, changed := repairTraefikLabelBackticks(tc.in) + if got != tc.want || changed != tc.changed { + t.Errorf("repairTraefikLabelBackticks(%q) = (%q, %v), want (%q, %v)", tc.in, got, changed, tc.want, tc.changed) + } + }) + } +} + +func TestMigrateTraefikLabelBackticks(t *testing.T) { + dokkuRoot := setupMigrationEnv(t) + if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + broken := "--label 'traefik.http.routers.web.rule=Host(\\`app.example.com\\`)'" + if err := common.PropertyListWrite("docker-options", "alpha", "_default_.deploy", []string{broken}); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := migrateTraefikLabelBackticks(); err != nil { + t.Fatalf("migrateTraefikLabelBackticks: %v", err) + } + + got, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "deploy") + if err != nil { + t.Fatalf("GetDockerOptionsForProcessPhase: %v", err) + } + want := "--label 'traefik.http.routers.web.rule=Host(`app.example.com`)'" + if !equalStrings(got, []string{want}) { + t.Errorf("got %q, want [%q]", got, []string{want}) + } + if common.PropertyGet("docker-options", "--global", traefikLabelMigrationKey) != "true" { + t.Errorf("expected %s guard to be set", traefikLabelMigrationKey) + } +} + func equalStrings(a, b []string) bool { if len(a) != len(b) { return false diff --git a/plugins/docker-options/triggers.go b/plugins/docker-options/triggers.go index 936055b99..fbb46937b 100644 --- a/plugins/docker-options/triggers.go +++ b/plugins/docker-options/triggers.go @@ -20,6 +20,10 @@ func TriggerInstall() error { return fmt.Errorf("Unable to migrate legacy docker-options files: %v", err) } + if err := migrateTraefikLabelBackticks(); err != nil { + return fmt.Errorf("Unable to repair docker-options label backticks: %v", err) + } + return nil } diff --git a/plugins/run/internal-functions b/plugins/run/internal-functions index e23ada445..2a15e60ef 100755 --- a/plugins/run/internal-functions +++ b/plugins/run/internal-functions @@ -86,6 +86,10 @@ fn-run() { DOKKU_RUN_TTL_SECONDS="86400" fi + if ! is_number "$DOKKU_RUN_TTL_SECONDS"; then + dokku_log_fail "--ttl-seconds must be a positive integer" + fi + if [[ "$CMD" == "run:detached" ]] && [[ "$DOKKU_FORCE_TTY" != "true" ]]; then export DOKKU_DISABLE_TTY=true fi diff --git a/plugins/scheduler-docker-local/bin/scheduler-deploy-process b/plugins/scheduler-docker-local/bin/scheduler-deploy-process index a98cda979..126fa0e7f 100755 --- a/plugins/scheduler-docker-local/bin/scheduler-deploy-process +++ b/plugins/scheduler-docker-local/bin/scheduler-deploy-process @@ -54,9 +54,10 @@ fn-scheduler-deploy-process() { local DOCKER_ARGS DOCKER_ARGS=$(: | plugn trigger docker-args-deploy "$APP" "$IMAGE_TAG" "$PROC_TYPE") DOCKER_ARGS+=" $(: | plugn trigger docker-args-process-deploy "$APP" "$IMAGE_SOURCE_TYPE" "$IMAGE_TAG" "$PROC_TYPE")" - DOCKER_ARGS=" $DOCKER_ARGS " - declare -a ARG_ARRAY - eval "ARG_ARRAY=($DOCKER_ARGS)" + declare -a ARG_ARRAY=() + while IFS= read -r -d '' arg; do + ARG_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") local port_published=false for arg in "${ARG_ARRAY[@]}"; do diff --git a/plugins/scheduler-docker-local/bin/scheduler-deploy-process-container b/plugins/scheduler-docker-local/bin/scheduler-deploy-process-container index 30c9dc795..9c6a33039 100755 --- a/plugins/scheduler-docker-local/bin/scheduler-deploy-process-container +++ b/plugins/scheduler-docker-local/bin/scheduler-deploy-process-container @@ -67,8 +67,10 @@ main() { DOCKER_ARGS+=" $IMAGE" DOCKER_ARGS+=" $START_CMD" - declare -a ARG_ARRAY - eval "ARG_ARRAY=($DOCKER_ARGS)" + declare -a ARG_ARRAY=() + while IFS= read -r -d '' arg; do + ARG_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") cid=$(fn-scheduler-docker-local-start-app-container "$APP" "${ARG_ARRAY[@]}") plugn trigger post-container-create "app" "$cid" "$APP" "deploy" "$PROC_TYPE" diff --git a/plugins/scheduler-docker-local/scheduler-run b/plugins/scheduler-docker-local/scheduler-run index 05d7bd8ca..d184b0954 100755 --- a/plugins/scheduler-docker-local/scheduler-run +++ b/plugins/scheduler-docker-local/scheduler-run @@ -133,8 +133,10 @@ trigger-scheduler-docker-local-scheduler-run() { DOCKER_ARGS+=" $IMAGE" DOCKER_ARGS+=" $EXEC_CMD" - declare -a ARG_ARRAY - eval "ARG_ARRAY=($DOCKER_ARGS)" + declare -a ARG_ARRAY=() + while IFS= read -r -d '' arg; do + ARG_ARRAY+=("$arg") + done < <(fn-docker-args-split "$DOCKER_ARGS") RUN_COMMAND=("$@") if [[ "${#RUN_COMMAND[@]}" -eq 0 ]]; then diff --git a/plugins/traefik-vhosts/docker-args-process-deploy b/plugins/traefik-vhosts/docker-args-process-deploy index 35dcda750..96937e5e5 100755 --- a/plugins/traefik-vhosts/docker-args-process-deploy +++ b/plugins/traefik-vhosts/docker-args-process-deploy @@ -86,14 +86,14 @@ trigger-traefik-vhosts-docker-args-process-deploy() { if [[ -n "$app_domains" ]]; then # get length of domains if [[ "$(echo "$traefik_domains" | wc -w)" -eq 1 ]]; then - traefik_domains="Host(\\\`$traefik_domains\\\`)" + traefik_domains="Host(\`$traefik_domains\`)" else for domain in $(echo "$app_domains" | xargs); do if [[ -z "$traefik_domains" ]]; then - traefik_domains="Host(\\\`$domain\\\`)" + traefik_domains="Host(\`$domain\`)" continue fi - traefik_domains="$traefik_domains || Host(\\\`$domain\\\`)" + traefik_domains="$traefik_domains || Host(\`$domain\`)" done fi fi diff --git a/tests/unit/builder-herokuish.bats b/tests/unit/builder-herokuish.bats index e01ef454c..c6a7ef2ed 100644 --- a/tests/unit/builder-herokuish.bats +++ b/tests/unit/builder-herokuish.bats @@ -105,6 +105,11 @@ teardown() { } @test "(builder-herouish:build .env)" { + run /bin/bash -c "dokku docker-options:add $TEST_APP build '--label=com.dokku.build-test=safe'" + echo "output: $output" + echo "status: $status" + assert_success + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP echo "output: $output" echo "status: $status" diff --git a/tests/unit/builder-nixpacks.bats b/tests/unit/builder-nixpacks.bats index 2458774c9..d0a1ac1a5 100644 --- a/tests/unit/builder-nixpacks.bats +++ b/tests/unit/builder-nixpacks.bats @@ -146,6 +146,11 @@ teardown() { echo "status: $status" assert_success + run /bin/bash -c "dokku docker-options:add $TEST_APP build '--label=com.dokku.build-test=safe'" + echo "output: $output" + echo "status: $status" + assert_success + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP add_requirements_txt echo "output: $output" echo "status: $status" diff --git a/tests/unit/builder-pack.bats b/tests/unit/builder-pack.bats index 403989785..ef8e64849 100644 --- a/tests/unit/builder-pack.bats +++ b/tests/unit/builder-pack.bats @@ -178,6 +178,11 @@ teardown() { echo "status: $status" assert_success + run /bin/bash -c "dokku docker-options:add $TEST_APP build '--label=com.dokku.build-test=safe'" + echo "output: $output" + echo "status: $status" + assert_success + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP add_requirements_txt_cnb echo "output: $output" echo "status: $status" diff --git a/tests/unit/builder-railpack.bats b/tests/unit/builder-railpack.bats index 4959bc39f..d94cba63b 100644 --- a/tests/unit/builder-railpack.bats +++ b/tests/unit/builder-railpack.bats @@ -157,6 +157,11 @@ teardown() { echo "status: $status" assert_success + run /bin/bash -c "dokku docker-options:add $TEST_APP build '--label=com.dokku.build-test=safe'" + echo "output: $output" + echo "status: $status" + assert_success + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP add_requirements_txt echo "output: $output" echo "status: $status" diff --git a/tests/unit/core_1.bats b/tests/unit/core_1.bats index 1c9d32606..b28d3c510 100644 --- a/tests/unit/core_1.bats +++ b/tests/unit/core_1.bats @@ -91,3 +91,32 @@ teardown() { echo "status: $status" assert_output "false" } + +@test "(common) fn-docker-args-split does not expand injected commands" { + export DOKKU_TEST_PAYLOAD='--label x=$(id)' + run /bin/bash -c "source '$PLUGIN_CORE_AVAILABLE_PATH/common/functions'; fn-docker-args-split \"\$DOKKU_TEST_PAYLOAD\" | tr '\\0' '\\n'" + echo "output: $output" + echo "status: $status" + assert_success + assert_line 1 'x=$(id)' + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - value was expanded" + + export DOKKU_TEST_PAYLOAD='--label x=`id`' + run /bin/bash -c "source '$PLUGIN_CORE_AVAILABLE_PATH/common/functions'; fn-docker-args-split \"\$DOKKU_TEST_PAYLOAD\" | tr '\\0' '\\n'" + echo "output: $output" + echo "status: $status" + assert_success + assert_line 1 'x=`id`' + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - value was expanded" + + unset DOKKU_TEST_PAYLOAD +} + +@test "(common) fn-docker-args-split preserves quoted tokens" { + run /bin/bash -c "source '$PLUGIN_CORE_AVAILABLE_PATH/common/functions'; fn-docker-args-split \"--label 'a b'\" | tr '\\0' '\\n'" + echo "output: $output" + echo "status: $status" + assert_success + assert_line 0 "--label" + assert_line 1 "a b" +} diff --git a/tests/unit/docker-options.bats b/tests/unit/docker-options.bats index b5f8cbac4..0848c7b47 100644 --- a/tests/unit/docker-options.bats +++ b/tests/unit/docker-options.bats @@ -285,6 +285,25 @@ teardown() { assert_output "/tmp:{} /var/tmp:{}" } +@test "(docker-options) deploy does not expand command substitution in option values [buildpacks]" { + run /bin/bash -c "dokku docker-options:add $TEST_APP deploy '--label=com.dokku.test=\$(id)'" + echo "output: $output" + echo "status: $status" + assert_success + + run deploy_app + echo "output: $output" + echo "status: $status" + assert_success + + CID=$(<$DOKKU_ROOT/$TEST_APP/CONTAINER.web.1) + run /bin/bash -c "docker inspect $CID --format '{{ index .Config.Labels \"com.dokku.test\" }}'" + echo "output: $output" + echo "status: $status" + assert_output '$(id)' + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - option value was expanded" +} + @test "(docker-options) deploy with options [dockerfile]" { run /bin/bash -c "dokku docker-options:add $TEST_APP deploy \"-v /var/tmp\"" echo "output: $output" diff --git a/tests/unit/run_2.bats b/tests/unit/run_2.bats index bd461aa31..b80957028 100644 --- a/tests/unit/run_2.bats +++ b/tests/unit/run_2.bats @@ -153,3 +153,46 @@ teardown() { assert_success assert_output "2" } + +@test "(run) docker-options and -e flags are not eval-injected" { + run deploy_app + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku docker-options:add $TEST_APP run '--label=com.dokku.test=safe'" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku run --ttl-seconds=60 -e FOO=bar $TEST_APP env | grep -E '^FOO=bar'" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku run -e 'X=\$(id)' $TEST_APP env" + echo "output: $output" + echo "status: $status" + assert_success + [[ "$output" == *'X=$(id)'* ]] || flunk "expected literal X=\$(id) in container env" + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - env value was expanded" +} + +@test "(run) --ttl-seconds rejects non-numeric input; docker-options stores it verbatim" { + run /bin/bash -c "dokku run --ttl-seconds '\$(id)' $TEST_APP echo hi" + echo "output: $output" + echo "status: $status" + assert_failure + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - ttl value was expanded" + + run /bin/bash -c "dokku docker-options:add $TEST_APP run '--label=x=\$(id)'" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-run" + echo "output: $output" + echo "status: $status" + [[ "$output" == *'x=$(id)'* ]] || flunk "expected literal x=\$(id) stored in docker options" + [[ "$output" != *"uid="* ]] || flunk "id command output leaked - docker option was expanded" +}