diff --git a/docs/appendices/file-formats/app-json.md b/docs/appendices/file-formats/app-json.md index dd55093cc..d5017b82e 100644 --- a/docs/appendices/file-formats/app-json.md +++ b/docs/appendices/file-formats/app-json.md @@ -244,9 +244,9 @@ An autoscaling trigger consists of the following properties: - `initialDelay`: (int, optional) - `listening`: (boolean, optional) - `name`: (string, optional) -- `path`: (string, optional) +- `path`: (string, optional) Must start with a `/` and must not contain whitespace, control characters, quotes, or backslashes. Deploys with an invalid `path` will fail. - `port`: (int, optional) -- `scheme`: (string, optional) +- `scheme`: (string, optional) Must be either `http` or `https`. Deploys with an invalid `scheme` will fail. - `timeout`: (int, optional) - `type`: (string, optional) - `uptime`: (int, optional) diff --git a/plugins/app-json/.gitignore b/plugins/app-json/.gitignore index d39fdc622..029dc59e8 100644 --- a/plugins/app-json/.gitignore +++ b/plugins/app-json/.gitignore @@ -7,5 +7,6 @@ /triggers /triggers/* /app-json-get-content +/app-json-is-valid /app-json-process-deploy-parallelism /core-* diff --git a/plugins/app-json/Makefile b/plugins/app-json/Makefile index 5283fc5db..bcd0530ae 100644 --- a/plugins/app-json/Makefile +++ b/plugins/app-json/Makefile @@ -1,5 +1,5 @@ SUBCOMMANDS = subcommands/report subcommands/set -TRIGGERS = triggers/app-json-process-deploy-parallelism triggers/app-json-get-content triggers/core-post-deploy triggers/core-post-extract triggers/install triggers/post-app-clone-setup triggers/post-app-rename triggers/post-app-rename-setup triggers/post-create triggers/post-delete triggers/post-deploy triggers/post-release-builder triggers/pre-release-builder triggers/report +TRIGGERS = triggers/app-json-process-deploy-parallelism triggers/app-json-get-content triggers/app-json-is-valid triggers/core-post-deploy triggers/core-post-extract triggers/install triggers/post-app-clone-setup triggers/post-app-rename triggers/post-app-rename-setup triggers/post-create triggers/post-delete triggers/post-deploy triggers/post-release-builder triggers/pre-release-builder triggers/report BUILD = commands subcommands triggers PLUGIN_NAME = app-json diff --git a/plugins/app-json/appjson.go b/plugins/app-json/appjson.go index 2aff5c217..7bace19aa 100644 --- a/plugins/app-json/appjson.go +++ b/plugins/app-json/appjson.go @@ -5,7 +5,9 @@ import ( "fmt" "math" "os" + "sort" "strings" + "unicode" "github.com/dokku/dokku/plugins/common" "github.com/tailscale/hujson" @@ -234,6 +236,67 @@ const ( HealthcheckType_Startup HealthcheckType = "startup" ) +// validateHealthchecks ensures healthcheck fields that are passed to schedulers and proxies are well-formed +func validateHealthchecks(appJSON AppJSON) error { + processTypes := make([]string, 0, len(appJSON.Healthchecks)) + for processType := range appJSON.Healthchecks { + processTypes = append(processTypes, processType) + } + sort.Strings(processTypes) + + for _, processType := range processTypes { + for index, healthcheck := range appJSON.Healthchecks[processType] { + name := healthcheck.Name + if name == "" { + name = fmt.Sprintf("#%d", index+1) + } + + if err := validateHealthcheckScheme(healthcheck.Scheme); err != nil { + return fmt.Errorf("Invalid app.json healthcheck %s for process type %s: %w", name, processType, err) + } + + if err := validateHealthcheckPath(healthcheck.Path); err != nil { + return fmt.Errorf("Invalid app.json healthcheck %s for process type %s: %w", name, processType, err) + } + } + } + + return nil +} + +// validateHealthcheckPath ensures a healthcheck path is absolute and contains no whitespace, control, or quote characters +func validateHealthcheckPath(path string) error { + if path == "" { + return nil + } + + if !strings.HasPrefix(path, "/") { + return fmt.Errorf("path %q must start with a /", path) + } + + for _, r := range path { + if unicode.IsSpace(r) || unicode.IsControl(r) || strings.ContainsRune(`"'\`, r) { + return fmt.Errorf("path %q must not contain whitespace, control characters, quotes, or backslashes", path) + } + } + + return nil +} + +// validateHealthcheckScheme ensures a healthcheck scheme is either http or https +func validateHealthcheckScheme(scheme string) error { + if scheme == "" { + return nil + } + + switch strings.ToLower(scheme) { + case "http", "https": + return nil + } + + return fmt.Errorf("scheme %q must be one of http or https", scheme) +} + // HTTPHeader is a struct that represents a single HTTP header associated with a healthcheck type HTTPHeader struct { // Name is the name of the HTTP header diff --git a/plugins/app-json/appjson_test.go b/plugins/app-json/appjson_test.go new file mode 100644 index 000000000..b7005fb2c --- /dev/null +++ b/plugins/app-json/appjson_test.go @@ -0,0 +1,85 @@ +package appjson + +import ( + "strings" + "testing" +) + +func TestValidateHealthchecksAcceptsWellFormedValues(t *testing.T) { + tests := []struct { + name string + healthcheck Healthcheck + }{ + {name: "empty", healthcheck: Healthcheck{}}, + {name: "root path", healthcheck: Healthcheck{Path: "/"}}, + {name: "nested path", healthcheck: Healthcheck{Path: "/health/ready"}}, + {name: "query string", healthcheck: Healthcheck{Path: "/health?full=1&verbose=true"}}, + {name: "percent encoded", healthcheck: Healthcheck{Path: "/health%20check"}}, + {name: "http scheme", healthcheck: Healthcheck{Path: "/", Scheme: "http"}}, + {name: "https scheme", healthcheck: Healthcheck{Path: "/", Scheme: "https"}}, + {name: "uppercase scheme", healthcheck: Healthcheck{Path: "/", Scheme: "HTTPS"}}, + {name: "command check", healthcheck: Healthcheck{Command: []string{"/bin/sh", "-c", "echo ok"}}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + appJSON := AppJSON{Healthchecks: map[string][]Healthcheck{"web": {tt.healthcheck}}} + if err := validateHealthchecks(appJSON); err != nil { + t.Errorf("validateHealthchecks() returned unexpected error: %v", err) + } + }) + } +} + +func TestValidateHealthchecksRejectsMalformedValues(t *testing.T) { + tests := []struct { + name string + healthcheck Healthcheck + wantField string + }{ + {name: "relative path", healthcheck: Healthcheck{Path: "health"}, wantField: "path"}, + {name: "space", healthcheck: Healthcheck{Path: "/health check"}, wantField: "path"}, + {name: "tab", healthcheck: Healthcheck{Path: "/health\tcheck"}, wantField: "path"}, + {name: "newline", healthcheck: Healthcheck{Path: "/health\ncheck"}, wantField: "path"}, + {name: "control character", healthcheck: Healthcheck{Path: "/health\x00"}, wantField: "path"}, + {name: "double quote", healthcheck: Healthcheck{Path: `/health"`}, wantField: "path"}, + {name: "single quote", healthcheck: Healthcheck{Path: `/health'`}, wantField: "path"}, + {name: "backslash", healthcheck: Healthcheck{Path: `/health\ check`}, wantField: "path"}, + {name: "unknown scheme", healthcheck: Healthcheck{Path: "/", Scheme: "tcp"}, wantField: "scheme"}, + {name: "scheme with whitespace", healthcheck: Healthcheck{Path: "/", Scheme: "http s"}, wantField: "scheme"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + appJSON := AppJSON{Healthchecks: map[string][]Healthcheck{"web": {tt.healthcheck}}} + err := validateHealthchecks(appJSON) + if err == nil { + t.Fatalf("validateHealthchecks() returned nil, want error for %s", tt.wantField) + } + + if !strings.Contains(err.Error(), tt.wantField) { + t.Errorf("validateHealthchecks() error = %q, want it to mention %q", err.Error(), tt.wantField) + } + }) + } +} + +func TestValidateHealthchecksNamesTheOffendingCheck(t *testing.T) { + appJSON := AppJSON{Healthchecks: map[string][]Healthcheck{ + "web": { + {Name: "ok", Path: "/ready"}, + {Path: "/health check"}, + }, + }} + + err := validateHealthchecks(appJSON) + if err == nil { + t.Fatal("validateHealthchecks() returned nil, want error") + } + + for _, want := range []string{"#2", "web"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("validateHealthchecks() error = %q, want it to mention %q", err.Error(), want) + } + } +} diff --git a/plugins/app-json/src/triggers/triggers.go b/plugins/app-json/src/triggers/triggers.go index 862982ac5..3243db5b2 100644 --- a/plugins/app-json/src/triggers/triggers.go +++ b/plugins/app-json/src/triggers/triggers.go @@ -22,6 +22,10 @@ func main() { appName := flag.Arg(0) processType := flag.Arg(1) err = appjson.TriggerAppJSONProcessDeployParallelism(appName, processType) + case "app-json-is-valid": + appName := flag.Arg(0) + appJSONPath := flag.Arg(1) + err = appjson.TriggerAppJSONIsValid(appName, appJSONPath) case "app-json-get-content": appName := flag.Arg(0) err = appjson.TriggerAppJSONGetContent(appName) diff --git a/plugins/app-json/triggers.go b/plugins/app-json/triggers.go index 6d78ce53d..6318203ed 100644 --- a/plugins/app-json/triggers.go +++ b/plugins/app-json/triggers.go @@ -35,6 +35,20 @@ func TriggerAppJSONProcessDeployParallelism(appName string, processType string) return nil } +// TriggerAppJSONIsValid validates the healthchecks in an extracted app.json file +func TriggerAppJSONIsValid(appName string, appJSONPath string) error { + if !common.FileExists(appJSONPath) { + return nil + } + + appJSON, err := ReadAppJSON(appJSONPath) + if err != nil { + return err + } + + return validateHealthchecks(appJSON) +} + // TriggerAppJSONGetContent outputs the contents of the app-json file, if any func TriggerAppJSONGetContent(appName string) error { if !hasAppJSON(appName) { diff --git a/plugins/traefik-vhosts/docker-args-process-deploy b/plugins/traefik-vhosts/docker-args-process-deploy index 96937e5e5..29f8a6e45 100755 --- a/plugins/traefik-vhosts/docker-args-process-deploy +++ b/plugins/traefik-vhosts/docker-args-process-deploy @@ -140,17 +140,25 @@ trigger-traefik-vhosts-docker-args-process-deploy() { hc_timeout=$(echo "$app_json" | jq -r "$hc_filter | .timeout // empty") hc_interval=$(echo "$app_json" | jq -r "$hc_filter | .wait // empty") + if ! fn-traefik-healthcheck-path-is-valid "$hc_path"; then + dokku_log_fail "Invalid app.json healthcheck path for traefik: path must start with a / and must not contain whitespace, control characters, quotes, or backslashes" + fi + + if ! fn-traefik-healthcheck-scheme-is-valid "$hc_scheme"; then + dokku_log_fail "Invalid app.json healthcheck scheme for traefik: scheme must be one of http or https" + fi + if [[ -n "$proxy_container_http_port" ]]; then - output="$output --label traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.path=$hc_path" - [[ -n "$hc_scheme" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.scheme=$hc_scheme" + output="$output --label \"traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.path=$hc_path\"" + [[ -n "$hc_scheme" ]] && output="$output --label \"traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.scheme=$hc_scheme\"" [[ -n "$hc_port" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.port=$hc_port" [[ -n "$hc_timeout" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.timeout=${hc_timeout}s" [[ -n "$hc_interval" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-http.loadbalancer.healthcheck.interval=${hc_interval}s" fi if [[ -n "$proxy_container_https_port" ]]; then - output="$output --label traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.path=$hc_path" - [[ -n "$hc_scheme" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.scheme=$hc_scheme" + output="$output --label \"traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.path=$hc_path\"" + [[ -n "$hc_scheme" ]] && output="$output --label \"traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.scheme=$hc_scheme\"" [[ -n "$hc_port" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.port=$hc_port" [[ -n "$hc_timeout" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.timeout=${hc_timeout}s" [[ -n "$hc_interval" ]] && output="$output --label traefik.http.services.$APP-$PROC_TYPE-https.loadbalancer.healthcheck.interval=${hc_interval}s" diff --git a/plugins/traefik-vhosts/internal-functions b/plugins/traefik-vhosts/internal-functions index 49b9afc5c..de94e656d 100755 --- a/plugins/traefik-vhosts/internal-functions +++ b/plugins/traefik-vhosts/internal-functions @@ -243,3 +243,20 @@ fn-traefik-dns-provider-env-vars() { declare desc="returns all dns-provider-* environment variables" fn-plugin-property-get-all-by-prefix "traefik" "--global" "dns-provider-" } + +fn-traefik-healthcheck-path-is-valid() { + declare desc="checks that an app.json healthcheck path is absolute and free of whitespace, control, quote, and backslash characters" + declare HC_PATH="$1" + + [[ "$HC_PATH" == /* ]] || return 1 + [[ "$HC_PATH" =~ [[:space:][:cntrl:]\"\'\\] ]] && return 1 + return 0 +} + +fn-traefik-healthcheck-scheme-is-valid() { + declare desc="checks that an app.json healthcheck scheme is either http or https" + declare HC_SCHEME="$1" + + [[ -z "$HC_SCHEME" ]] && return 0 + [[ "${HC_SCHEME,,}" == "http" ]] || [[ "${HC_SCHEME,,}" == "https" ]] +} diff --git a/tests/unit/app-json-3.bats b/tests/unit/app-json-3.bats index 0f7b79ef9..3b8c75d87 100644 --- a/tests/unit/app-json-3.bats +++ b/tests/unit/app-json-3.bats @@ -75,6 +75,20 @@ teardown() { assert_output "web: 1" } +@test "(app-json) healthcheck path with whitespace fails deploy" { + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP whitespace_healthcheck_path_callback + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains 'Invalid app.json healthcheck web readiness check for process type web: path "/health check" must not contain whitespace, control characters, quotes, or backslashes' 2 + + run /bin/bash -c "docker ps -a -q --filter label=com.dokku.app-name=$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + assert_output_not_exists +} + persist_scale_callback_a() { local APP="$1" local APP_REPO_DIR="$2" @@ -114,3 +128,23 @@ negative_formation_callback() { } EOF } + +whitespace_healthcheck_path_callback() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + + cat >"$APP_REPO_DIR/app.json" < /var/lib/dokku/data/app-json/$TEST_APP/app.json.tmp && mv /var/lib/dokku/data/app-json/$TEST_APP/app.json.tmp /var/lib/dokku/data/app-json/$TEST_APP/app.json" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku ps:restart $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid app.json healthcheck path for traefik" 2 +} + setup_traefik_healthcheck() { local APP="$1" local APP_REPO_DIR="$2" @@ -890,3 +945,21 @@ setup_traefik_healthcheck() { mv "$APP_REPO_DIR/app-traefik.json" "$APP_REPO_DIR/app.json" } + +setup_traefik_healthcheck_whitespace_path() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + APP_REPO_DIR="$(realpath "$APP_REPO_DIR")" + + jq '.healthchecks.web[0].path = "/health check"' "$APP_REPO_DIR/app-traefik.json" >"$APP_REPO_DIR/app.json" +} + +setup_traefik_healthcheck_invalid_scheme() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + APP_REPO_DIR="$(realpath "$APP_REPO_DIR")" + + jq '.healthchecks.web[0].scheme = "tcp"' "$APP_REPO_DIR/app-traefik.json" >"$APP_REPO_DIR/app.json" +}