diff --git a/docs/processes/scheduled-cron-tasks.md b/docs/processes/scheduled-cron-tasks.md index 1df1f2537..9be07b666 100644 --- a/docs/processes/scheduled-cron-tasks.md +++ b/docs/processes/scheduled-cron-tasks.md @@ -59,6 +59,7 @@ When running scheduled cron tasks, there are a few items to be aware of: - Each scheduled task is executed within a one-off `run` container, and thus inherit any docker-options specified for `run` containers. Resources are never shared between scheduled tasks. - Scheduled cron tasks are supported on a per-scheduler basis, and are currently only implemented by the `docker-local` scheduler. - Tasks for _all_ apps managed by the `docker-local` scheduler are written to a single crontab file owned by the `dokku` user. The `dokku` user's crontab should be considered reserved for this purpose. +- The `command` is tokenized and exec'd directly inside the container. Shell features such as `;`, `&&`, `|`, and `>` are _not_ interpreted. Commands that contain a bare shell operator are rejected when `app.json` is validated at deploy time, so a malformed cron command will fail the deploy rather than silently fail to run. If shell semantics are required, wrap the command explicitly, for example `"sh -c 'do-thing > /var/log/x.log'"`. ### Changing cron management settings diff --git a/plugins/cron/cron.go b/plugins/cron/cron.go index 89d19f552..ea78eae77 100644 --- a/plugins/cron/cron.go +++ b/plugins/cron/cron.go @@ -10,8 +10,20 @@ import ( "github.com/multiformats/go-base36" cronparser "github.com/robfig/cron/v3" + "mvdan.cc/sh/v3/shell" ) +// ValidateCronCommand returns an error if the command cannot be tokenised +// as a sequence of shell words. cron:run dispatches with the same parser, +// so a command that fails here will also fail to execute - validating at +// deploy time surfaces the error before the schedule fires. +func ValidateCronCommand(command string) error { + _, err := shell.Fields(command, func(name string) string { + return "" + }) + return err +} + var ( // DefaultProperties is a map of all valid cron properties with corresponding default property values DefaultProperties = map[string]string{ @@ -75,7 +87,7 @@ func (t CronTask) DokkuRunCommand() string { return t.AltCommand } - return fmt.Sprintf("dokku run --concurrency-policy %s --cron-id %s %s %s", t.ConcurrencyPolicy, t.ID, t.App, t.Command) + return fmt.Sprintf("dokku cron:run %s %s", t.App, t.ID) } // FetchCronTasksInput is the input for the FetchCronTasks function @@ -138,6 +150,10 @@ func FetchCronTasks(input FetchCronTasksInput) ([]CronTask, error) { return tasks, fmt.Errorf("Invalid cron schedule for app %s (schedule %s): %s", appName, c.Schedule, err.Error()) } + if err := ValidateCronCommand(c.Command); err != nil { + return tasks, fmt.Errorf("Invalid cron command for app %s (command %q): %s", appName, c.Command, err.Error()) + } + cronID := GenerateCommandID(appName, c) maintenance := c.Maintenance if value, ok := properties[MaintenancePropertyPrefix+cronID]; ok { diff --git a/plugins/cron/cron_test.go b/plugins/cron/cron_test.go new file mode 100644 index 000000000..88e6e53fc --- /dev/null +++ b/plugins/cron/cron_test.go @@ -0,0 +1,102 @@ +package cron + +import ( + "strings" + "testing" +) + +func TestDokkuRunCommandAppTaskDispatchesViaCronRun(t *testing.T) { + task := CronTask{ + App: "myapp", + ID: "abc123", + Command: "echo CRON_OK; echo hi > /tmp/appjson-test.txt", + Schedule: "* * * * *", + ConcurrencyPolicy: "allow", + } + + got := task.DokkuRunCommand() + want := "dokku cron:run myapp abc123" + if got != want { + t.Errorf("DokkuRunCommand() = %q, want %q", got, want) + } + + if strings.Contains(got, task.Command) { + t.Errorf("DokkuRunCommand() leaked user command into crontab line: %q", got) + } + if strings.ContainsAny(got, ";>|&`$") { + t.Errorf("DokkuRunCommand() contains shell metacharacters: %q", got) + } +} + +func TestDokkuRunCommandPlainCommandStillUsesCronRun(t *testing.T) { + task := CronTask{ + App: "myapp", + ID: "abc123", + Command: "npm run send-email", + Schedule: "@daily", + ConcurrencyPolicy: "forbid", + } + + got := task.DokkuRunCommand() + want := "dokku cron:run myapp abc123" + if got != want { + t.Errorf("DokkuRunCommand() = %q, want %q", got, want) + } +} + +func TestDokkuRunCommandAltCommandUnchanged(t *testing.T) { + task := CronTask{ + ID: "abc123", + AltCommand: "/usr/bin/some-internal-task --flag", + } + + got := task.DokkuRunCommand() + want := "/usr/bin/some-internal-task --flag" + if got != want { + t.Errorf("DokkuRunCommand() = %q, want %q", got, want) + } +} + +func TestValidateCronCommandAcceptsValidCommands(t *testing.T) { + cases := []string{ + "python3 task.py schedule", + "npm run send-email", + "sh -c 'echo CRON_OK; echo hi > /tmp/x.txt'", + `node -e 'console.log(1)'`, + "true", + } + for _, cmd := range cases { + if err := ValidateCronCommand(cmd); err != nil { + t.Errorf("ValidateCronCommand(%q) returned error: %v", cmd, err) + } + } +} + +func TestValidateCronCommandRejectsShellOperators(t *testing.T) { + cases := []string{ + "echo CRON_OK; echo hi > /tmp/x.txt", + "cmd1 && cmd2", + "cmd | other", + "cmd > file", + "cmd $(other)", + } + for _, cmd := range cases { + if err := ValidateCronCommand(cmd); err == nil { + t.Errorf("ValidateCronCommand(%q) accepted a command containing a shell operator", cmd) + } + } +} + +func TestDokkuRunCommandAltCommandWithLogFile(t *testing.T) { + task := CronTask{ + ID: "abc123", + AltCommand: "/usr/bin/some-internal-task", + LogFile: "/var/log/dokku/internal-task.log", + } + + got := task.DokkuRunCommand() + want := "/usr/bin/some-internal-task &>> /var/log/dokku/internal-task.log" + if got != want { + t.Errorf("DokkuRunCommand() = %q, want %q", got, want) + } +} diff --git a/plugins/cron/subcommands.go b/plugins/cron/subcommands.go index e3d054545..aa2cb08c1 100644 --- a/plugins/cron/subcommands.go +++ b/plugins/cron/subcommands.go @@ -52,7 +52,7 @@ func CommandList(appName string, format string) error { maintenance = "true (app)" } } - output = append(output, fmt.Sprintf("%s | %s | %s | %t | %s", task.ID, task.Schedule, task.ConcurrencyPolicy, maintenance, task.Command)) + output = append(output, fmt.Sprintf("%s | %s | %s | %s | %s", task.ID, task.Schedule, task.ConcurrencyPolicy, maintenance, task.Command)) } result := columnize.SimpleFormat(output) @@ -139,6 +139,7 @@ func CommandRun(appName string, cronID string, detached bool) error { os.Setenv("DOKKU_CONCURRENCY_POLICY", concurrencyPolicy) os.Setenv("DOKKU_CRON_ID", cronID) os.Setenv("DOKKU_RM_CONTAINER", "1") + os.Setenv("DOKKU_RUN_TTL_SECONDS", "86400") scheduler := common.GetAppScheduler(appName) args := append([]string{scheduler, appName, "0", "--"}, fields...) _, err = common.CallPlugnTrigger(common.PlugnTriggerInput{ diff --git a/tests/unit/cron.bats b/tests/unit/cron.bats index c81f69f5b..6170002d7 100644 --- a/tests/unit/cron.bats +++ b/tests/unit/cron.bats @@ -82,11 +82,14 @@ teardown() { echo "status: $status" assert_success + cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + run /bin/bash -c "cat /var/spool/cron/crontabs/dokku" echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py schedule" + assert_output_contains "dokku cron:run $TEST_APP $cron_id" + assert_output_contains "python3 task.py schedule" 0 } @test "(cron) create [single-short]" { @@ -95,11 +98,14 @@ teardown() { echo "status: $status" assert_success + cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + run /bin/bash -c "cat /var/spool/cron/crontabs/dokku" echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py daily" + assert_output_contains "dokku cron:run $TEST_APP $cron_id" + assert_output_contains "python3 task.py daily" 0 } @test "(cron:report) --global --format json" { @@ -253,12 +259,17 @@ teardown() { echo "status: $status" assert_success + first_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + second_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[1].id')" + run /bin/bash -c "cat /var/spool/cron/crontabs/dokku" echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py first" - assert_output_contains "python3 task.py second" + assert_output_contains "dokku cron:run $TEST_APP $first_id" + assert_output_contains "dokku cron:run $TEST_APP $second_id" + assert_output_contains "python3 task.py first" 0 + assert_output_contains "python3 task.py second" 0 } @test "(cron) injected entries" { @@ -417,38 +428,27 @@ teardown() { echo "status: $status" assert_success + first_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + second_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[1].id')" + run /bin/bash -c "cat /var/spool/cron/crontabs/dokku" echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py first" - assert_output_contains "python3 task.py second" + assert_output_contains "dokku cron:run $TEST_APP $first_id" + assert_output_contains "dokku cron:run $TEST_APP $second_id" - cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" - run /bin/bash -c "echo $cron_id" - echo "output: $output" - echo "status: $status" - assert_success - assert_output_exists - - first_command="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].command')" - run /bin/bash -c "echo $first_command" - echo "output: $output" - echo "status: $status" - assert_success - assert_output_exists - - run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id" + run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id" echo "output: $output" echo "status: $status" assert_failure - run /bin/bash -c "dokku cron:suspend $TEST_APP $cron_id" + run /bin/bash -c "dokku cron:suspend $TEST_APP $first_id" echo "output: $output" echo "status: $status" assert_success - run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id" + run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id" echo "output: $output" echo "status: $status" assert_success @@ -458,15 +458,15 @@ teardown() { echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py first" 0 - assert_output_contains "python3 task.py second" + assert_output_contains "dokku cron:run $TEST_APP $first_id" 0 + assert_output_contains "dokku cron:run $TEST_APP $second_id" - run /bin/bash -c "dokku cron:resume $TEST_APP $cron_id" + run /bin/bash -c "dokku cron:resume $TEST_APP $first_id" echo "output: $output" echo "status: $status" assert_success - run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id" + run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id" echo "output: $output" echo "status: $status" assert_failure @@ -475,8 +475,112 @@ teardown() { echo "output: $output" echo "status: $status" assert_success - assert_output_contains "python3 task.py first" - assert_output_contains "python3 task.py second" + assert_output_contains "dokku cron:run $TEST_APP $first_id" + assert_output_contains "dokku cron:run $TEST_APP $second_id" +} + +@test "(cron) invalid [command]" { + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_injection + echo "output: $output" + echo "status: $status" + assert_failure +} + +@test "(cron) crontab format [no raw command leakage]" { + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_valid_single + echo "output: $output" + echo "status: $status" + assert_success + + cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + + run /bin/bash -c "cat /var/spool/cron/crontabs/dokku" + echo "output: $output" + echo "status: $status" + assert_success + assert_output_contains "dokku cron:run $TEST_APP $cron_id" + # User command must never appear verbatim in the crontab - the crontab + # only references the cron ID, and cron:run resolves the command at run + # time and exec's it inside the container. + assert_output_contains "python3 task.py schedule" 0 +} + +@test "(cron) container labels regression" { + run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_long_running + echo "output: $output" + echo "status: $status" + assert_success + + cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')" + + run /bin/bash -c "dokku cron:run $TEST_APP $cron_id --detach" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.cron-id\" }}'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_id" + + run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.concurrency-policy\" }}'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "allow" + + run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.container-type\" }}'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "cron" + + run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.app-name\" }}'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$TEST_APP" + + run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.active-deadline-seconds\" }}'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "86400" +} + +template_cron_file_long_running() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + echo "injecting long-running cron app.json -> $APP_REPO_DIR/app.json" + cat <"$APP_REPO_DIR/app.json" +{ + "cron": [ + { + "command": "sleep 30", + "schedule": "0 0 * * *" + } + ] +} +EOF +} + +template_cron_file_injection() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + echo "injecting injection-attempt cron app.json -> $APP_REPO_DIR/app.json" + cat <"$APP_REPO_DIR/app.json" +{ + "cron": [ + { + "command": "echo CRON_OK; echo hi > /tmp/appjson-injection-test.txt", + "schedule": "* * * * *" + } + ] +} +EOF } template_cron_file_invalid() {