From 0c489aa25e760690cb4ec849e4d17c57c0e945a8 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 12:20:17 +1300 Subject: [PATCH 01/12] feat: add cmdenv, which wraps a command to run with only the env vars it may have The rack splits a command into words and runs them without a shell, in a process holding every env var of the app's release, RACK_URL (the rack's API password) included. cmdenv builds the plan of what a command gets (every env var less RACK_URL, or only the listed ones, with read-only substitutes such as DATABASE_URL from READ_ONLY_DATABASE_URL) and wraps the command in a fixed sh script that runs it with exactly that environment. Parse reads a wrapper back, for the mock rack. The tests run the wrapper with sh, dash, bash and busybox. --- internal/cmdenv/cmdenv.go | 301 +++++++++++++++++++++++++++++++ internal/cmdenv/cmdenv_test.go | 318 +++++++++++++++++++++++++++++++++ 2 files changed, 619 insertions(+) create mode 100644 internal/cmdenv/cmdenv.go create mode 100644 internal/cmdenv/cmdenv_test.go diff --git a/internal/cmdenv/cmdenv.go b/internal/cmdenv/cmdenv.go new file mode 100644 index 00000000..dcdc9c68 --- /dev/null +++ b/internal/cmdenv/cmdenv.go @@ -0,0 +1,301 @@ +// Package cmdenv decides the environment of a command run through the gateway (an exec's, or a process start's) +// and wraps the command so it runs with only that environment. +// +// A command travels as one header line, and the rack splits it into words the way a shell would (go-shellquote) +// and runs them with no shell, in a process that has every env var of the app's release (see scriptrun). The +// gateway sends the command inside a fixed sh script that starts it with only the env vars it may have: +// +// sh -c 'exec env -i -- ${A+"A=$A"} ${RO+"DATABASE_URL=$RO"} "$@"' rack-gateway-env +// +// Each name the command gets is passed on only when it's set, with its value untouched; a read-only var takes the +// value of its substitute instead. A command given every env var drops RACK_URL and the read-only vars, then adds +// the substitutes back: +// +// sh -c 'unset RACK_URL DATABASE_URL; exec env -- ${RO+"DATABASE_URL=$RO"} "$@"' rack-gateway-env +// +// The image needs sh and env. +package cmdenv + +import ( + "errors" + "fmt" + "regexp" + "sort" + "strings" + + "github.com/kballard/go-shellquote" + + "github.com/DocSpring/rack-gateway/internal/gateway/envutil" +) + +// RackURL is the env var Convox gives every process of every app: the rack API's URL, with the rack's API password +// in it. A command holding it could use the rack's API without the gateway, so no command gets it, and no setting +// or flag can give it one. +const RackURL = "RACK_URL" + +// argv0 names the wrapper in process listings ($0). +const argv0 = "rack-gateway-env" + +// Plan is the environment a command gets. The zero Plan gives it no env vars at all. +type Plan struct { + // All gives the command every env var its process has, except RACK_URL. Otherwise it gets only Vars. + All bool + // Vars are the env vars the command gets when All is false, sorted. + Vars []string + // ReadOnly maps an env var the command gets to the env var it takes its value from instead (DATABASE_URL to + // READ_ONLY_DATABASE_URL, say). When the substitute isn't set, the command doesn't get the var at all. + ReadOnly map[string]string +} + +// New returns the plan that gives a command every env var (all), or only vars, with the substitutes in readOnly +// applied to the vars it gets. It refuses a name that isn't a valid env var name, RACK_URL anywhere, and a +// substitute that is itself replaced by one. +func New(all bool, vars []string, readOnly map[string]string) (Plan, error) { + if err := CheckReadOnly(readOnly); err != nil { + return Plan{}, err + } + plan := Plan{All: all} + if !all { + if err := CheckNames(vars); err != nil { + return Plan{}, err + } + plan.Vars = sortedUnique(vars) + } + for name, source := range readOnly { + if plan.gets(name) { + if plan.ReadOnly == nil { + plan.ReadOnly = map[string]string{} + } + plan.ReadOnly[name] = source + } + } + return plan, nil +} + +// gets reports whether the command gets the env var name (from its own value or a substitute's). +func (p Plan) gets(name string) bool { + if p.All { + return true + } + i := sort.SearchStrings(p.Vars, name) + return i < len(p.Vars) && p.Vars[i] == name +} + +// CheckNames refuses a list of env var names with one that isn't a valid env var name, or RACK_URL. +func CheckNames(names []string) error { + for _, name := range names { + if err := checkName(name); err != nil { + return err + } + } + return nil +} + +// CheckReadOnly refuses read-only substitutes (var to substitute) with a name that isn't a valid env var name, +// RACK_URL, or a substitute that is itself replaced: the substitutes all read the process's own values. +func CheckReadOnly(readOnly map[string]string) error { + for _, name := range sortedKeys(readOnly) { + source := readOnly[name] + if err := CheckNames([]string{name, source}); err != nil { + return err + } + if name == source { + return fmt.Errorf("%s can't be its own read-only substitute", name) + } + if _, replaced := readOnly[source]; replaced { + return fmt.Errorf("%s is the read-only substitute for %s, so it can't have one of its own", source, name) + } + } + return nil +} + +func checkName(name string) error { + if !envutil.ValidName(name) { + return fmt.Errorf("%q isn't an env var name (letters, digits and underscores, not starting with a digit)", + name) + } + if name == RackURL { + return fmt.Errorf("no command can have %s: it holds the rack's API password", RackURL) + } + return nil +} + +// Build returns the command the rack runs to run command with plan's environment. plan must be one New returns, +// or the zero Plan; command must be one CheckCommand accepts. +func Build(plan Plan, command string) string { + return "sh -c '" + plan.script() + "' " + argv0 + " " + command +} + +// script is the sh script that runs the command words ("$@") with the plan's environment. It has no single quotes, +// so it can be single-quoted as one word. +func (p Plan) script() string { + if !p.All { + return "exec env -i -- " + p.assignments(p.Vars) + `"$@"` + } + replaced := sortedKeys(p.ReadOnly) + return "unset " + strings.Join(append([]string{RackURL}, replaced...), " ") + "; exec env -- " + + p.assignments(replaced) + `"$@"` +} + +// assignments passes each of names to env as NAME=value when its source (the name, or its read-only substitute) is +// set. The value is double-quoted, so it stays one word whatever it holds. +func (p Plan) assignments(names []string) string { + var b strings.Builder + for _, name := range names { + source := name + if substitute, ok := p.ReadOnly[name]; ok { + source = substitute + } + fmt.Fprintf(&b, `${%s+"%s=$%s"} `, source, name, source) + } + return b.String() +} + +var ( + errEmptyCommand = errors.New("the command is empty") + errAssignmentFirst = errors.New("the command can't start with NAME=value: a command gets env vars from the " + + "gateway (see --env)") +) + +// CheckCommand refuses a command the gateway can't wrap: one the rack can't split into words, one with no words +// (a process start without a command would run the service's own command, with every env var), and one starting +// with NAME=value, which env would read as an env var of its own rather than the program to run. +func CheckCommand(command string) error { + words, err := shellquote.Split(command) + if err != nil { + return fmt.Errorf("the rack can't split the command into words: %w", err) + } + if len(words) == 0 { + return errEmptyCommand + } + if strings.Contains(words[0], "=") { + return errAssignmentFirst + } + return nil +} + +const ( + namePattern = `[A-Za-z_][A-Za-z0-9_]*` + scriptPrefix = "sh -c '" + scriptEnd = "' " + argv0 + " " +) + +var ( + assignment = regexp.MustCompile(`^\$\{(` + namePattern + `)\+"(` + namePattern + `)=\$(` + namePattern + `)"\} `) + unsetLine = regexp.MustCompile(`^unset ` + RackURL + `((?: ` + namePattern + `)*); exec env -- `) +) + +// Parse reads a command Build made back into its plan and the command it runs. ok is false for anything else. +func Parse(wrapped string) (plan Plan, command string, ok bool) { + rest, found := strings.CutPrefix(wrapped, scriptPrefix) + if !found { + return Plan{}, "", false + } + script, command, found := strings.Cut(rest, scriptEnd) + if !found { + return Plan{}, "", false + } + parsed, ok := parseScript(script) + if !ok { + return Plan{}, "", false + } + // New sorts the names and checks them, so only the wrapper Build makes for a valid plan reads back. + plan, err := New(parsed.All, parsed.Vars, parsed.ReadOnly) + if err != nil || Build(plan, command) != wrapped { + return Plan{}, "", false + } + return plan, command, true +} + +// parseScript reads a script Plan.script made back into its plan. +func parseScript(script string) (Plan, bool) { + var plan Plan + if rest, found := strings.CutPrefix(script, "exec env -i -- "); found { + script = rest + } else if m := unsetLine.FindStringSubmatch(script); m != nil { + plan.All = true + script = script[len(m[0]):] + } else { + return Plan{}, false + } + for { + m := assignment.FindStringSubmatch(script) + if m == nil { + return plan, script == `"$@"` + } + if !plan.add(m[1], m[2], m[3]) { + return Plan{}, false + } + script = script[len(m[0]):] + } +} + +// add adds an assignment ${source+"target=$value"} the script passes to env, or reports false when its value isn't +// its source. +func (p *Plan) add(source, target, value string) bool { + if value != source { + return false + } + if !p.All { + p.Vars = append(p.Vars, target) + } + if source != target { + if p.ReadOnly == nil { + p.ReadOnly = map[string]string{} + } + p.ReadOnly[target] = source + } + return true +} + +// Describe says what env vars the plan gives a command, for a person: "every env var except RACK_URL", or the +// names, each read-only one with its substitute. +func (p Plan) Describe() string { + if p.All { + description := "every env var except " + RackURL + if len(p.ReadOnly) > 0 { + description += "; read-only " + strings.Join(p.described(sortedKeys(p.ReadOnly)), ", ") + } + return description + } + if len(p.Vars) == 0 { + return "no env vars" + } + return strings.Join(p.described(p.Vars), ", ") +} + +// described names each of names, with its read-only substitute: "DATABASE_URL (from READ_ONLY_DATABASE_URL)". +func (p Plan) described(names []string) []string { + described := make([]string, 0, len(names)) + for _, name := range names { + if source, ok := p.ReadOnly[name]; ok { + name += " (from " + source + ")" + } + described = append(described, name) + } + return described +} + +func sortedUnique(names []string) []string { + sorted := append([]string(nil), names...) + sort.Strings(sorted) + unique := sorted[:0] + for i, name := range sorted { + if i == 0 || name != sorted[i-1] { + unique = append(unique, name) + } + } + if len(unique) == 0 { + return nil + } + return unique +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for key := range m { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} diff --git a/internal/cmdenv/cmdenv_test.go b/internal/cmdenv/cmdenv_test.go new file mode 100644 index 00000000..a4367200 --- /dev/null +++ b/internal/cmdenv/cmdenv_test.go @@ -0,0 +1,318 @@ +package cmdenv + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/kballard/go-shellquote" + "github.com/stretchr/testify/require" +) + +// helperArg makes the test binary print its environment and arguments as JSON and exit with the code in its next +// argument, so a test can run it as the wrapped command and see exactly what it got. +const helperArg = "cmdenv-print-env" + +type helperOutput struct { + Env map[string]string `json:"env"` + Args []string `json:"args"` +} + +func TestMain(m *testing.M) { + if len(os.Args) > 2 && os.Args[1] == helperArg { + out := helperOutput{Env: map[string]string{}, Args: os.Args[3:]} + for _, entry := range os.Environ() { + name, value, _ := strings.Cut(entry, "=") + out.Env[name] = value + } + _ = json.NewEncoder(os.Stdout).Encode(out) + code, _ := strconv.Atoi(os.Args[2]) + os.Exit(code) + } + os.Exit(m.Run()) +} + +func mustPlan(t *testing.T, all bool, vars []string, readOnly map[string]string) Plan { + t.Helper() + plan, err := New(all, vars, readOnly) + require.NoError(t, err) + return plan +} + +func TestNewSortsNamesAndKeepsOnlyTheSubstitutesOfVarsTheCommandGets(t *testing.T) { + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL", "ADMIN_URL": "READ_ONLY_DATABASE_URL"} + plan := mustPlan(t, false, []string{"PATH", "DATABASE_URL", "PATH", "HOME"}, readOnly) + require.Equal(t, Plan{ + Vars: []string{"DATABASE_URL", "HOME", "PATH"}, + ReadOnly: map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}, + }, plan) + + require.Equal(t, Plan{All: true, ReadOnly: readOnly}, mustPlan(t, true, []string{"IGNORED"}, readOnly)) + require.Equal(t, Plan{}, mustPlan(t, false, nil, readOnly)) +} + +func TestNewRefusesBadNamesRackURLAndChainedSubstitutes(t *testing.T) { + for name, tc := range map[string]struct { + vars []string + readOnly map[string]string + want string + }{ + "invalid name": {vars: []string{"PATH", "BAD-NAME"}, want: `"BAD-NAME" isn't an env var name`}, + "empty name": {vars: []string{""}, want: `"" isn't an env var name`}, + "RACK_URL": {vars: []string{"RACK_URL"}, want: "no command can have RACK_URL"}, + "RACK_URL substituted": {readOnly: map[string]string{"RACK_URL": "X"}, want: "no command can have RACK_URL"}, + "RACK_URL substitute": {readOnly: map[string]string{"X": "RACK_URL"}, want: "no command can have RACK_URL"}, + "invalid substitute": {readOnly: map[string]string{"X": "Y Z"}, want: `"Y Z" isn't an env var name`}, + "own substitute": {readOnly: map[string]string{"X": "X"}, want: "X can't be its own read-only"}, + "chained": { + readOnly: map[string]string{"A": "B", "B": "C"}, + want: "B is the read-only substitute for A, so it can't have one of its own", + }, + } { + for _, all := range []bool{false, true} { + if all && tc.readOnly == nil { + continue // every env var: the names aren't used + } + _, err := New(all, tc.vars, tc.readOnly) + require.ErrorContains(t, err, tc.want, name) + } + } +} + +func TestBuildAndParseRoundTrip(t *testing.T) { + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + for _, plan := range []Plan{ + {}, + mustPlan(t, false, []string{"PATH", "DATABASE_URL"}, readOnly), + mustPlan(t, false, []string{"PATH"}, nil), + mustPlan(t, true, nil, nil), + mustPlan(t, true, nil, readOnly), + } { + for _, command := range []string{"sleep 3600", `bin/rails runner 'puts "it'"'"'s $HOME"'`, "a b"} { + wrapped := Build(plan, command) + parsed, original, ok := Parse(wrapped) + require.True(t, ok, wrapped) + require.Equal(t, plan, parsed) + require.Equal(t, command, original) + } + } + require.Equal(t, + `sh -c 'exec env -i -- ${READ_ONLY_DATABASE_URL+"DATABASE_URL=$READ_ONLY_DATABASE_URL"} ${PATH+"PATH=$PATH"} `+ + `"$@"' rack-gateway-env rake db:migrate`, + Build(mustPlan(t, false, []string{"PATH", "DATABASE_URL"}, readOnly), "rake db:migrate")) + require.Equal(t, + `sh -c 'unset RACK_URL DATABASE_URL; exec env -- `+ + `${READ_ONLY_DATABASE_URL+"DATABASE_URL=$READ_ONLY_DATABASE_URL"} "$@"' rack-gateway-env rake db:migrate`, + Build(mustPlan(t, true, nil, readOnly), "rake db:migrate")) +} + +// Only the wrapper Build makes for a valid plan reads back. +func TestParseRefusesWrappersBuildDoesntMake(t *testing.T) { + for _, wrapped := range []string{ + "rake db:migrate", + `sh -c 'exec "$@"' rack-gateway-env rake`, + `sh -c 'exec env -i -- "$@"' rack-gateway-script rake`, + `sh -c 'exec env -i -- ${PATH+"PATH=$PATH"} ${HOME+"HOME=$HOME"} "$@"' rack-gateway-env rake`, // unsorted + `sh -c 'exec env -i -- ${PATH+"PATH=$PATH"} ${PATH+"PATH=$PATH"} "$@"' rack-gateway-env rake`, // repeated + `sh -c 'exec env -i -- ${RACK_URL+"RACK_URL=$RACK_URL"} "$@"' rack-gateway-env rake`, + `sh -c 'exec env -i -- ${A+"B=$C"} "$@"' rack-gateway-env rake`, + `sh -c 'exec env -i -- ${A+"B=$A"} ${B+"C=$B"} "$@"' rack-gateway-env rake`, // chained substitutes + `sh -c 'exec env -i -- ${PATH+"PATH=$PATH"}"$@"' rack-gateway-env rake`, + `sh -c 'exec env -i -- "$@"; id' rack-gateway-env rake`, + `sh -c 'unset RACK_URL; exec env -- ${A+"A=$A"} "$@"' rack-gateway-env rake`, + `sh -c 'unset RACK_URL B; exec env -- ${A+"C=$A"} "$@"' rack-gateway-env rake`, + `sh -c 'unset RACK_URL; exec env -- "$@"'`, + } { + _, _, ok := Parse(wrapped) + require.False(t, ok, wrapped) + } +} + +func TestCheckCommand(t *testing.T) { + for command, want := range map[string]error{ + "rake db:migrate": nil, + `sh -c 'A=1 rake'`: nil, + `'' rake`: nil, // runs "", which fails as it would unwrapped + "": errEmptyCommand, + " ": errEmptyCommand, + "FOO=bar rake db:migrate": errAssignmentFirst, + `"FOO=bar" rake`: errAssignmentFirst, + `rake 'unbalanced`: errors.New("the rack can't split the command into words"), + "bin/rails runner 'puts 1' \\": errors.New("the rack can't split the command into words"), + } { + err := CheckCommand(command) + if want == nil { + require.NoError(t, err, command) + } else { + require.ErrorContains(t, err, want.Error(), command) + } + } +} + +func TestDescribe(t *testing.T) { + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + require.Equal(t, "no env vars", Plan{}.Describe()) + require.Equal(t, "DATABASE_URL (from READ_ONLY_DATABASE_URL), PATH", + mustPlan(t, false, []string{"PATH", "DATABASE_URL"}, readOnly).Describe()) + require.Equal(t, "every env var except RACK_URL", mustPlan(t, true, nil, nil).Describe()) + require.Equal(t, "every env var except RACK_URL; read-only DATABASE_URL (from READ_ONLY_DATABASE_URL)", + mustPlan(t, true, nil, readOnly).Describe()) +} + +// shell is a shell the rack's image might have: how to run it, and the directory holding the env it uses ("" +// for the one on PATH). +type shell struct { + name, envDir string + argv []string +} + +// shells returns the shells on this machine to run the wrapper with: sh, and dash, bash and busybox's ash (with +// busybox's env) when they're installed. +func shells(t *testing.T) []shell { + t.Helper() + found := []shell{{name: "sh", argv: []string{"sh"}}} + for _, name := range []string{"dash", "bash"} { + if path, err := exec.LookPath(name); err == nil { + found = append(found, shell{name: name, argv: []string{path}}) + } + } + if busybox, err := exec.LookPath("busybox"); err == nil { + dir := t.TempDir() + require.NoError(t, os.Symlink(busybox, filepath.Join(dir, "env"))) + found = append(found, shell{name: "busybox", argv: []string{busybox, "sh"}, envDir: dir}) + } + return found +} + +// run runs the wrapper Build makes for plan the way the rack does (split into words, run without a shell), with sh +// replaced by the shell, in a process whose environment is env. The command is the test binary, printing what it +// got and exiting with code. +func run(t *testing.T, sh shell, plan Plan, env map[string]string, code int, args ...string) (helperOutput, int) { + t.Helper() + command := shellquote.Join(append([]string{os.Args[0], helperArg, strconv.Itoa(code)}, args...)...) + argv, err := shellquote.Split(Build(plan, command)) + require.NoError(t, err) + require.Equal(t, "sh", argv[0]) + argv = append(append([]string(nil), sh.argv...), argv[1:]...) + + cmd := exec.Command(argv[0], argv[1:]...) // #nosec G702 -- the test runs the command it built + cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + if sh.envDir != "" { + cmd.Env[0] = "PATH=" + sh.envDir + string(os.PathListSeparator) + os.Getenv("PATH") + } + for name, value := range env { + cmd.Env = append(cmd.Env, name+"="+value) + } + stdout, err := cmd.Output() + exitCode := 0 + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exitCode = exitErr.ExitCode() + } else { + require.NoError(t, err) + } + var out helperOutput + require.NoError(t, json.Unmarshal(stdout, &out), string(stdout)) + return out, exitCode +} + +// rackURLValue is a RACK_URL value, built from parts: gosec takes a literal one for a hardcoded credential (G101). +const rackURLValue = "https://convox:" + "secret@api" + +// tricky values: each must reach the command exactly as set. +var tricky = map[string]string{ + "SPACES": "a b c", + "QUOTES": `it's "quoted"`, + "DOLLARS": "$HOME ${PATH} $(id) `id`", + "NEWLINES": "line 1\nline 2\n", + "EMPTY": "", + "BACKSLASH": `a\nb\\c\`, + "GLOB": "* ? [a-z]", + "EQUALS": "a=b=c", + "UNICODE": "naïve 日本", +} + +func TestTheCommandGetsOnlyTheListedVarsWithTheirValuesIntact(t *testing.T) { + env := map[string]string{ + "RACK_URL": rackURLValue, "OTHER": "x", + "READ_ONLY_DATABASE_URL": "postgres://ro", "DATABASE_URL": "postgres://rw", + } + names := make([]string, 0, len(tricky)+2) + names = append(names, "DATABASE_URL", "UNSET_VAR") + for name, value := range tricky { + env[name] = value + names = append(names, name) + } + plan := mustPlan(t, false, names, map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}) + for _, sh := range shells(t) { + out, code := run(t, sh, plan, env, 0) + require.Zero(t, code, sh.name) + want := map[string]string{"DATABASE_URL": "postgres://ro"} + for name, value := range tricky { + want[name] = value + } + require.Equal(t, want, out.Env, sh.name) + } +} + +func TestAReadOnlyVarWithoutItsSubstituteIsLeftOut(t *testing.T) { + env := map[string]string{"DATABASE_URL": "postgres://rw", "PATH_EXTRA": "x"} + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + for _, sh := range shells(t) { + out, _ := run(t, sh, mustPlan(t, false, []string{"DATABASE_URL", "PATH_EXTRA"}, readOnly), env, 0) + require.Equal(t, map[string]string{"PATH_EXTRA": "x"}, out.Env, sh.name) + + out, _ = run(t, sh, mustPlan(t, true, nil, readOnly), env, 0) + require.NotContains(t, out.Env, "DATABASE_URL", sh.name) + require.Equal(t, "x", out.Env["PATH_EXTRA"], sh.name) + } +} + +func TestEveryEnvVarLeavesOutRackURLAndAppliesTheSubstitutes(t *testing.T) { + env := map[string]string{ + "RACK_URL": rackURLValue, "DATABASE_URL": "postgres://rw", + "READ_ONLY_DATABASE_URL": "postgres://ro", + } + for name, value := range tricky { + env[name] = value + } + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + for _, sh := range shells(t) { + out, _ := run(t, sh, mustPlan(t, true, nil, readOnly), env, 0) + require.NotContains(t, out.Env, "RACK_URL", sh.name) + require.Equal(t, "postgres://ro", out.Env["DATABASE_URL"], sh.name) + require.Equal(t, "postgres://ro", out.Env["READ_ONLY_DATABASE_URL"], sh.name) + for name, value := range tricky { + require.Equal(t, value, out.Env[name], fmt.Sprintf("%s: %s", sh.name, name)) + } + + out, _ = run(t, sh, mustPlan(t, true, nil, nil), env, 0) + require.NotContains(t, out.Env, "RACK_URL", sh.name) + require.Equal(t, "postgres://rw", out.Env["DATABASE_URL"], sh.name) + } +} + +// The zero Plan (the attached run's placeholder) gives the command nothing. +func TestTheZeroPlanGivesNoEnvVars(t *testing.T) { + for _, sh := range shells(t) { + out, _ := run(t, sh, Plan{}, map[string]string{"RACK_URL": "x", "DATABASE_URL": "y"}, 0) + require.Empty(t, out.Env, sh.name) + } +} + +func TestTheCommandsArgumentsAndExitCodePassThrough(t *testing.T) { + args := []string{"two spaces", `it's "quoted"`, "$HOME", "", "-x", "--", "a=b"} + for _, sh := range shells(t) { + for _, plan := range []Plan{{}, mustPlan(t, true, nil, nil)} { + out, code := run(t, sh, plan, nil, 3, args...) + require.Equal(t, 3, code, sh.name) + require.Equal(t, args, out.Args, sh.name) + } + } +} From 858e89dea24cf2df706cc16890a53576bbccce68 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 12:46:03 +1300 Subject: [PATCH 02/12] feat(gateway): run each command with only the env vars it may have Every exec and process start reaches the rack wrapped by cmdenv, so the command runs without RACK_URL (the rack's API password, which Convox gives every process) and, in an app that sets command_env_vars, with only those env vars and the ones it asks for (X-Command-Env, or X-Command-All-Env-Vars for every one). command_env_read_only gives read-only vars their substitute's value (DATABASE_URL from READ_ONLY_DATABASE_URL, say) unless the command asks to write (X-Command-Write). The audit log records the command as sent, with the env vars it got. approved_deploy_commands entries are objects now ({"command", "env", "all_env_vars", "write"}): under a deploy approval the entry decides the command's env vars, and a command asking for any itself is refused. A migration turns each stored command string into an object. Also refused before anything runs: a command the rack can't split into words, an empty one (a process start without a command runs the service's own command, which the gateway can't wrap), and one starting with NAME=value. `convox run`'s placeholder gets no env vars at all. Slack messages say which env vars a command got, and warn when it ran with --write. --- internal/cmdenv/request.go | 110 ++++++ internal/cmdenv/request_test.go | 45 +++ internal/gateway/audit/command_env.go | 48 +++ internal/gateway/audit/logger.go | 30 +- internal/gateway/db/export_test.go | 10 + .../db/migration_approved_commands_test.go | 41 +++ ...000000_approved_deploy_command_objects.sql | 16 + .../gateway/handlers/settings_app_values.go | 72 +++- .../gateway/openapi/generated/swagger.json | 274 ++++++++++++++- .../gateway/proxy/approval_window_test.go | 2 +- internal/gateway/proxy/command_env.go | 96 ++++++ internal/gateway/proxy/command_env_test.go | 245 +++++++++++++ internal/gateway/proxy/command_length_test.go | 5 +- .../proxy/deploy_approval_binding_test.go | 6 +- internal/gateway/proxy/env_filters.go | 23 +- internal/gateway/proxy/env_protected_test.go | 16 + internal/gateway/proxy/forward.go | 3 + internal/gateway/proxy/handler.go | 3 + internal/gateway/proxy/script_command_test.go | 6 +- internal/gateway/proxy/websocket.go | 3 + internal/gateway/rbac/gateway_routes.go | 20 ++ internal/gateway/rbac/mfa_requirements.go | 2 + internal/gateway/routes/route_registration.go | 4 + internal/gateway/settings/command_env.go | 173 ++++++++++ internal/gateway/settings/command_env_test.go | 172 ++++++++++ internal/gateway/settings/defaults.go | 10 +- internal/gateway/settings/env.go | 2 + internal/gateway/settings/env_test.go | 8 +- internal/gateway/settings/service_commands.go | 36 ++ .../gateway/settings/service_env_helpers.go | 6 - internal/gateway/settings/validate.go | 32 +- internal/gateway/settings/validate_test.go | 5 - .../gateway/slack/process_notifications.go | 27 +- .../slack/process_notifications_test.go | 36 ++ scripts/lib/cli-e2e/db.sh | 2 +- web/src/api/generated.ts | 124 ++++++- web/src/api/openapi.json | 324 +++++++++++++++++- ...eteAppsAppSettingsCommandEnvReadOnly200.ts | 12 + .../deleteAppsAppSettingsCommandEnvVars200.ts | 12 + web/src/api/schemas/index.ts | 6 + ...putAppsAppSettingsCommandEnvReadOnly200.ts | 12 + ...utAppsAppSettingsCommandEnvReadOnlyBody.ts | 11 + .../putAppsAppSettingsCommandEnvVars200.ts | 12 + .../api/schemas/settingsApprovedCommand.ts | 18 + web/src/lib/generated/mfa-requirements.ts | 24 ++ 45 files changed, 2077 insertions(+), 67 deletions(-) create mode 100644 internal/cmdenv/request.go create mode 100644 internal/cmdenv/request_test.go create mode 100644 internal/gateway/audit/command_env.go create mode 100644 internal/gateway/db/migration_approved_commands_test.go create mode 100644 internal/gateway/db/migrations/20261026000000_approved_deploy_command_objects.sql create mode 100644 internal/gateway/proxy/command_env.go create mode 100644 internal/gateway/proxy/command_env_test.go create mode 100644 internal/gateway/settings/command_env.go create mode 100644 internal/gateway/settings/command_env_test.go create mode 100644 web/src/api/schemas/deleteAppsAppSettingsCommandEnvReadOnly200.ts create mode 100644 web/src/api/schemas/deleteAppsAppSettingsCommandEnvVars200.ts create mode 100644 web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnly200.ts create mode 100644 web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnlyBody.ts create mode 100644 web/src/api/schemas/putAppsAppSettingsCommandEnvVars200.ts create mode 100644 web/src/api/schemas/settingsApprovedCommand.ts diff --git a/internal/cmdenv/request.go b/internal/cmdenv/request.go new file mode 100644 index 00000000..de0abb84 --- /dev/null +++ b/internal/cmdenv/request.go @@ -0,0 +1,110 @@ +package cmdenv + +import ( + "errors" + "fmt" + "net/http" + "strings" +) + +// The headers that carry what a command asks for, from the CLI to the gateway (exec and run's --env, +// --all-env-vars and --write). The gateway reads them on an exec or a process start and doesn't forward them to the +// rack. +const ( + // EnvHeader names the env vars the command asks for, comma-separated. + EnvHeader = "X-Command-Env" + // AllEnvVarsHeader is "true" when the command asks for every env var. + AllEnvVarsHeader = "X-Command-All-Env-Vars" + // WriteHeader is "true" when the command asks for the real values of the app's read-only env vars. + WriteHeader = "X-Command-Write" +) + +// Request is what a command asks for: env vars besides the app's, every env var, or the real values of the read-only +// ones. +type Request struct { + // Env names the env vars the command asks for besides the ones every command in the app gets. + Env []string + // AllEnvVars asks for every env var (except RACK_URL). + AllEnvVars bool + // Write asks for the real values of the app's read-only env vars, so the command can change data. + Write bool +} + +// IsZero reports whether r asks for nothing besides what every command in the app gets. +func (r Request) IsZero() bool { + return len(r.Env) == 0 && !r.AllEnvVars && !r.Write +} + +var errEnvAndAll = errors.New("--all-env-vars gives the command every env var, so it can't name some with --env too") + +// Check refuses a request with a name that isn't a valid env var name, RACK_URL, or names given with every env var. +func (r Request) Check() error { + if err := CheckNames(r.Env); err != nil { + return err + } + if r.AllEnvVars && len(r.Env) > 0 { + return errEnvAndAll + } + return nil +} + +// Headers returns the headers that carry r. +func (r Request) Headers() map[string]string { + headers := map[string]string{} + if len(r.Env) > 0 { + headers[EnvHeader] = strings.Join(r.Env, ",") + } + if r.AllEnvVars { + headers[AllEnvVarsHeader] = "true" + } + if r.Write { + headers[WriteHeader] = "true" + } + return headers +} + +// RequestFrom reads the request header carries, checked (see Request.Check). Each header may be given once, and the +// flags only as "true". +func RequestFrom(header http.Header) (Request, error) { + var r Request + env, err := single(header, EnvHeader) + if err != nil { + return Request{}, err + } + if env != "" { + for _, name := range strings.Split(env, ",") { + r.Env = append(r.Env, strings.TrimSpace(name)) + } + } + if r.AllEnvVars, err = flag(header, AllEnvVarsHeader); err != nil { + return Request{}, err + } + if r.Write, err = flag(header, WriteHeader); err != nil { + return Request{}, err + } + return r, r.Check() +} + +// single returns the one value of header name, or "" when it isn't given. +func single(header http.Header, name string) (string, error) { + values := header.Values(name) + if len(values) > 1 { + return "", fmt.Errorf("the request gives %s more than once", name) + } + if len(values) == 0 { + return "", nil + } + return strings.TrimSpace(values[0]), nil +} + +// flag returns whether header name is given, as "true". +func flag(header http.Header, name string) (bool, error) { + value, err := single(header, name) + if err != nil || value == "" { + return false, err + } + if value != "true" { + return false, fmt.Errorf("%s must be true, got %q", name, value) + } + return true, nil +} diff --git a/internal/cmdenv/request_test.go b/internal/cmdenv/request_test.go new file mode 100644 index 00000000..830d1f27 --- /dev/null +++ b/internal/cmdenv/request_test.go @@ -0,0 +1,45 @@ +package cmdenv + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRequestHeadersRoundTrip(t *testing.T) { + for _, r := range []Request{ + {}, + {Env: []string{"STRIPE_KEY", "http_proxy"}}, + {Env: []string{"STRIPE_KEY"}, Write: true}, + {AllEnvVars: true, Write: true}, + } { + header := http.Header{} + for name, value := range r.Headers() { + header.Set(name, value) + } + read, err := RequestFrom(header) + require.NoError(t, err) + require.Equal(t, r, read) + require.Equal(t, r.IsZero(), len(header) == 0) + } + + read, err := RequestFrom(http.Header{EnvHeader: {" A , B "}}) + require.NoError(t, err) + require.Equal(t, Request{Env: []string{"A", "B"}}, read) +} + +func TestRequestFromRefusesMalformedHeaders(t *testing.T) { + for want, header := range map[string]http.Header{ + "gives X-Command-Env more than once": {EnvHeader: {"A", "B"}}, + "X-Command-Write must be true": {WriteHeader: {"yes"}}, + "X-Command-All-Env-Vars must be": {AllEnvVarsHeader: {"1"}}, + "isn't an env var name": {EnvHeader: {"A,B-C"}}, + `"" isn't an env var name`: {EnvHeader: {"A,,B"}}, + "no command can have RACK_URL": {EnvHeader: {"RACK_URL"}}, + "can't name some with --env too": {EnvHeader: {"A"}, AllEnvVarsHeader: {"true"}}, + } { + _, err := RequestFrom(header) + require.ErrorContains(t, err, want) + } +} diff --git a/internal/gateway/audit/command_env.go b/internal/gateway/audit/command_env.go new file mode 100644 index 00000000..c7721918 --- /dev/null +++ b/internal/gateway/audit/command_env.go @@ -0,0 +1,48 @@ +package audit + +import ( + "context" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" +) + +// CommandEnv is the environment the command a request runs gets (an exec's, or a process start's), which the +// request's audit entries record (see BuildDetailsJSON). +type CommandEnv struct { + // Plan is the env vars the command gets. + Plan cmdenv.Plan + // Write is set when the command was given the real values of the app's read-only env vars. + Write bool +} + +type commandEnvContextKey struct{} + +// WithCommandEnv returns ctx carrying the environment of the command its request runs. +func WithCommandEnv(ctx context.Context, env CommandEnv) context.Context { + return context.WithValue(ctx, commandEnvContextKey{}, env) +} + +// CommandEnvFromContext returns the environment WithCommandEnv gave ctx, and whether it gave one. +func CommandEnvFromContext(ctx context.Context) (CommandEnv, bool) { + env, ok := ctx.Value(commandEnvContextKey{}).(CommandEnv) + return env, ok +} + +// details are the audit details that record the environment: all_env_vars when the command got every env var +// (except RACK_URL), or env_vars, the names it got (only those that were set reached it); env_read_only, each +// read-only var it got with the var it took its value from; and write when it was given their real values. +func (e CommandEnv) details() map[string]interface{} { + details := map[string]interface{}{} + if e.Plan.All { + details["all_env_vars"] = true + } else { + details["env_vars"] = append([]string{}, e.Plan.Vars...) + } + if len(e.Plan.ReadOnly) > 0 { + details["env_read_only"] = e.Plan.ReadOnly + } + if e.Write { + details["write"] = true + } + return details +} diff --git a/internal/gateway/audit/logger.go b/internal/gateway/audit/logger.go index 4c85a92c..6480da7c 100644 --- a/internal/gateway/audit/logger.go +++ b/internal/gateway/audit/logger.go @@ -315,16 +315,7 @@ func (_ *Logger) BuildDetailsJSON(r *http.Request) string { details["query"] = debugredact.RedactQuery(r.URL.RawQuery) } - // An exec or a process start records its command (the Command header, never WebSocket frames). A command line - // can carry credentials, which the audit log mustn't keep (see RedactCommand). - if cmd, ok := RequestCommand(r); ok && cmd != "" { - for key, value := range commandDetails(cmd) { - details[key] = value - } - } - if justification := JustificationFromContext(r.Context()); justification != "" { - details["justification"] = justification - } + addCommandDetails(details, r) if strings.HasSuffix(r.URL.Path, "/exec") { parts := strings.Split(strings.TrimPrefix(r.URL.Path, "/"), "/") for i, p := range parts { @@ -348,6 +339,25 @@ func (_ *Logger) BuildDetailsJSON(r *http.Request) string { return string(data) } +// addCommandDetails adds what an exec or a process start records: its command (the Command header, never WebSocket +// frames), its justification and its env vars. A command line can carry credentials, which the audit log mustn't +// keep (see RedactCommand). +func addCommandDetails(details map[string]interface{}, r *http.Request) { + if cmd, ok := RequestCommand(r); ok && cmd != "" { + for key, value := range commandDetails(cmd) { + details[key] = value + } + } + if justification := JustificationFromContext(r.Context()); justification != "" { + details["justification"] = justification + } + if env, ok := CommandEnvFromContext(r.Context()); ok { + for key, value := range env.details() { + details[key] = value + } + } +} + // MapHTTPStatusToStatus converts HTTP status code to audit status string func (_ *Logger) MapHTTPStatusToStatus(httpStatus int) string { switch { diff --git a/internal/gateway/db/export_test.go b/internal/gateway/db/export_test.go index b40ca1cf..18b23c23 100644 --- a/internal/gateway/db/export_test.go +++ b/internal/gateway/db/export_test.go @@ -12,3 +12,13 @@ func (d *Database) SetDeployApprovalCreatedAtForTest(id int64, createdAt time.Ti _, err := d.exec(`UPDATE deploy_approval_requests SET created_at = ? WHERE id = ?`, createdAt, id) return err } + +// RunMigrationForTest runs the migration file name again, on a database that has run every migration. +func (d *Database) RunMigrationForTest(name string) error { + sql, err := migrationsFS.ReadFile("migrations/" + name) + if err != nil { + return err + } + _, err = d.db.Exec(string(sql)) + return err +} diff --git a/internal/gateway/db/migration_approved_commands_test.go b/internal/gateway/db/migration_approved_commands_test.go new file mode 100644 index 00000000..3476f16d --- /dev/null +++ b/internal/gateway/db/migration_approved_commands_test.go @@ -0,0 +1,41 @@ +package db_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" +) + +// Each approved deploy command stored as a string becomes an object with just its command, in the same order. An +// entry that's already an object, other settings and values that aren't lists are left alone. +func TestApprovedDeployCommandsMigrationMakesEachCommandAnObject(t *testing.T) { + database := dbtest.NewDatabase(t) + web, worker, empty, odd := "web", "worker", "empty", "odd" + require.NoError(t, database.UpsertSetting(&web, "approved_deploy_commands", + []interface{}{ + "bin/migrate", + map[string]interface{}{"command": "rake stripe:prepare", "write": true}, + `bin/rails "db:seed"`, + }, nil)) + require.NoError(t, database.UpsertSetting(&worker, "protected_env_vars", []string{"DATABASE_URL"}, nil)) + require.NoError(t, database.UpsertSetting(&empty, "approved_deploy_commands", []string{}, nil)) + require.NoError(t, database.UpsertSetting(&odd, "approved_deploy_commands", "bin/migrate", nil)) + + for range 2 { + require.NoError(t, database.RunMigrationForTest("20261026000000_approved_deploy_command_objects.sql")) + } + + read := func(app, key string) string { + raw, found, err := database.GetSetting(&app, key) + require.NoError(t, err) + require.True(t, found) + return string(raw) + } + require.JSONEq(t, `[{"command":"bin/migrate"},{"command":"rake stripe:prepare","write":true},`+ + `{"command":"bin/rails \"db:seed\""}]`, read(web, "approved_deploy_commands")) + require.JSONEq(t, `["DATABASE_URL"]`, read(worker, "protected_env_vars")) + require.JSONEq(t, `[]`, read(empty, "approved_deploy_commands")) + require.JSONEq(t, `"bin/migrate"`, read(odd, "approved_deploy_commands")) +} diff --git a/internal/gateway/db/migrations/20261026000000_approved_deploy_command_objects.sql b/internal/gateway/db/migrations/20261026000000_approved_deploy_command_objects.sql new file mode 100644 index 00000000..902beb29 --- /dev/null +++ b/internal/gateway/db/migrations/20261026000000_approved_deploy_command_objects.sql @@ -0,0 +1,16 @@ +-- Each approved_deploy_commands entry is an object now ({"command": ..., "env": [...], "all_env_vars": ..., +-- "write": ...}), so it can say which env vars its command gets. Each command stored as a string becomes an object +-- with just its command, which gets the env vars any command in the app gets. Entries that are already objects stay +-- as they are, so the update can run again. +UPDATE settings +SET value = ( + SELECT COALESCE( + jsonb_agg( + CASE jsonb_typeof(entry) WHEN 'string' THEN jsonb_build_object('command', entry) ELSE entry END + ORDER BY position + ), + '[]'::jsonb + ) + FROM jsonb_array_elements(value) WITH ORDINALITY AS entries(entry, position) +) +WHERE key = 'approved_deploy_commands' AND jsonb_typeof(value) = 'array'; diff --git a/internal/gateway/handlers/settings_app_values.go b/internal/gateway/handlers/settings_app_values.go index 9315e41a..f3de06d6 100644 --- a/internal/gateway/handlers/settings_app_values.go +++ b/internal/gateway/handlers/settings_app_values.go @@ -176,7 +176,7 @@ func (h *SettingsHandler) DeleteAppAuditVisibleEnvVars(c *gin.Context) { // @Accept json // @Produce json // @Param app path string true "App name" -// @Param body body []string true "Approved command list" +// @Param body body []settings.ApprovedCommand true "Approved commands, each with the env vars it gets" // @Success 200 {object} map[string]settings.Setting // @Failure 400 {object} ErrorResponse // @Failure 409 {object} ErrorResponse @@ -204,6 +204,76 @@ func (h *SettingsHandler) DeleteAppApprovedDeployCommands(c *gin.Context) { h.deleteAppSettingValue(c, settings.KeyApprovedDeployCommands) } +// UpdateAppCommandEnvVars godoc +// @Summary Update the env vars commands get +// @Description Replaces the env vars every command run in the specified app (exec, run) gets. Empty: every env var +// @Tags Settings +// @Accept json +// @Produce json +// @Param app path string true "App name" +// @Param body body []string true "Env var names" +// @Success 200 {object} map[string]settings.Setting +// @Failure 400 {object} ErrorResponse +// @Failure 409 {object} ErrorResponse +// @Failure 500 {object} ErrorResponse +// @Security SessionCookie +// @Security CSRFToken +// @Router /apps/{app}/settings/command-env-vars [put] +func (h *SettingsHandler) UpdateAppCommandEnvVars(c *gin.Context) { + h.updateAppSettingValue(c, settings.KeyCommandEnvVars) +} + +// DeleteAppCommandEnvVars godoc +// @Summary Delete the env vars commands get +// @Description Removes the stored list, so every command run in the specified app gets every env var +// @Tags Settings +// @Produce json +// @Param app path string true "App name" +// @Success 200 {object} map[string]settings.Setting +// @Failure 400 {object} ErrorResponse +// @Failure 500 {object} ErrorResponse +// @Security SessionCookie +// @Security CSRFToken +// @Router /apps/{app}/settings/command-env-vars [delete] +func (h *SettingsHandler) DeleteAppCommandEnvVars(c *gin.Context) { + h.deleteAppSettingValue(c, settings.KeyCommandEnvVars) +} + +// UpdateAppCommandEnvReadOnly godoc +// @Summary Update the read-only env vars of commands +// @Description Replaces the env vars commands in the specified app get from another one unless run with --write +// @Tags Settings +// @Accept json +// @Produce json +// @Param app path string true "App name" +// @Param body body map[string]string true "Env var name to the env var it reads instead" +// @Success 200 {object} map[string]settings.Setting +// @Failure 400 {object} ErrorResponse +// @Failure 409 {object} ErrorResponse +// @Failure 500 {object} ErrorResponse +// @Security SessionCookie +// @Security CSRFToken +// @Router /apps/{app}/settings/command-env-read-only [put] +func (h *SettingsHandler) UpdateAppCommandEnvReadOnly(c *gin.Context) { + h.updateAppSettingValue(c, settings.KeyCommandEnvReadOnly) +} + +// DeleteAppCommandEnvReadOnly godoc +// @Summary Delete the read-only env vars of commands +// @Description Removes the stored substitutes, so commands in the specified app get every env var's own value +// @Tags Settings +// @Produce json +// @Param app path string true "App name" +// @Success 200 {object} map[string]settings.Setting +// @Failure 400 {object} ErrorResponse +// @Failure 500 {object} ErrorResponse +// @Security SessionCookie +// @Security CSRFToken +// @Router /apps/{app}/settings/command-env-read-only [delete] +func (h *SettingsHandler) DeleteAppCommandEnvReadOnly(c *gin.Context) { + h.deleteAppSettingValue(c, settings.KeyCommandEnvReadOnly) +} + // UpdateAppServiceImagePatterns godoc // @Summary Update service image patterns // @Description Replaces service image constraints for the specified app diff --git a/internal/gateway/openapi/generated/swagger.json b/internal/gateway/openapi/generated/swagger.json index 245fab16..a1612539 100644 --- a/internal/gateway/openapi/generated/swagger.json +++ b/internal/gateway/openapi/generated/swagger.json @@ -676,14 +676,14 @@ "required": true }, { - "description": "Approved command list", + "description": "Approved commands, each with the env vars it gets", "name": "body", "in": "body", "required": true, "schema": { "type": "array", "items": { - "type": "string" + "$ref": "#/definitions/settings.ApprovedCommand" } } } @@ -892,6 +892,252 @@ } } }, + "/apps/{app}/settings/command-env-read-only": { + "put": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Replaces the env vars commands in the specified app get from another one unless run with --write", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "Settings" + ], + "summary": "Update the read-only env vars of commands", + "parameters": [ + { + "type": "string", + "description": "App name", + "name": "app", + "in": "path", + "required": true + }, + { + "description": "Env var name to the env var it reads instead", + "name": "body", + "in": "body", + "required": true, + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/settings.Setting" + } + } + }, + "400": { + "description": "Bad Request", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "409": { + "description": "Conflict", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + } + } + }, + "delete": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Removes the stored substitutes, so commands in the specified app get every env var's own value", + "produces": [ + "application/json" + ], + "tags": [ + "Settings" + ], + "summary": "Delete the read-only env vars of commands", + "parameters": [ + { + "type": "string", + "description": "App name", + "name": "app", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/settings.Setting" + } + } + }, + "400": { + "description": "Bad Request", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + } + } + } + }, + "/apps/{app}/settings/command-env-vars": { + "put": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Replaces the env vars every command run in the specified app (exec, run) gets. Empty: every env var", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "Settings" + ], + "summary": "Update the env vars commands get", + "parameters": [ + { + "type": "string", + "description": "App name", + "name": "app", + "in": "path", + "required": true + }, + { + "description": "Env var names", + "name": "body", + "in": "body", + "required": true, + "schema": { + "type": "array", + "items": { + "type": "string" + } + } + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/settings.Setting" + } + } + }, + "400": { + "description": "Bad Request", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "409": { + "description": "Conflict", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + } + } + }, + "delete": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Removes the stored list, so every command run in the specified app gets every env var", + "produces": [ + "application/json" + ], + "tags": [ + "Settings" + ], + "summary": "Delete the env vars commands get", + "parameters": [ + { + "type": "string", + "description": "App name", + "name": "app", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/settings.Setting" + } + } + }, + "400": { + "description": "Bad Request", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + } + } + } + }, "/apps/{app}/settings/protected-env-vars": { "put": { "security": [ @@ -6446,6 +6692,30 @@ } } }, + "settings.ApprovedCommand": { + "type": "object", + "properties": { + "all_env_vars": { + "description": "AllEnvVars gives it every env var (except RACK_URL), as --all-env-vars does.", + "type": "boolean" + }, + "command": { + "description": "Command is the command, as it's run.", + "type": "string" + }, + "env": { + "description": "Env names the env vars it gets besides the app's command_env_vars.", + "type": "array", + "items": { + "type": "string" + } + }, + "write": { + "description": "Write gives it the real values of the app's read-only env vars (command_env_read_only), as --write does.", + "type": "boolean" + } + } + }, "settings.Setting": { "type": "object", "properties": { diff --git a/internal/gateway/proxy/approval_window_test.go b/internal/gateway/proxy/approval_window_test.go index f467c1b0..7ee7c231 100644 --- a/internal/gateway/proxy/approval_window_test.go +++ b/internal/gateway/proxy/approval_window_test.go @@ -66,7 +66,7 @@ func TestApprovalRecordsEveryExecOnAProcess(t *testing.T) { e := newApprovalExecHarness(t) app := "docspring" commands := []string{"bin/pre_release_checks migrate", "bin/pre_release_checks seed"} - require.NoError(t, e.database.UpsertSetting(&app, "approved_deploy_commands", commands, nil)) + require.NoError(t, e.database.UpsertSetting(&app, "approved_deploy_commands", approvedCommands(commands...), nil)) _, err := e.database.DB().Exec( `UPDATE deploy_approval_requests SET exec_commands = '{"p1": "bin/earlier"}'::jsonb WHERE id = $1`, e.approval.ID, diff --git a/internal/gateway/proxy/command_env.go b/internal/gateway/proxy/command_env.go new file mode 100644 index 00000000..f05f9e30 --- /dev/null +++ b/internal/gateway/proxy/command_env.go @@ -0,0 +1,96 @@ +package proxy + +import ( + "errors" + "fmt" + "net/http" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" + "github.com/DocSpring/rack-gateway/internal/gateway/audit" + "github.com/DocSpring/rack-gateway/internal/gateway/settings" +) + +// errApprovedCommandEnv refuses a command under a deploy approval that asks for env vars of its own. +var errApprovedCommandEnv = errors.New("under a deploy approval, the command's entry in approved_deploy_commands " + + "decides its env vars: run it without --env, --all-env-vars and --write") + +// errEnvUndecided refuses to forward a command whose env vars weren't decided (checkCommandEnv): the gateway never +// runs a command with env vars nothing chose. +var errEnvUndecided = errors.New("the command's env vars weren't decided, so it isn't run") + +// checkCommandEnv decides the env vars of the command r runs (an exec's, or a process start's) and puts them on r, +// for the command forwarded to the rack (wrapCommand) and the request's audit entries. The command gets the app's +// command_env_vars and the ones it asks for (cmdenv.Request), or every env var, with the read-only vars' substitutes +// unless it asks to write. Under a deploy approval its entry in approved_deploy_commands asks instead, and a command +// that asks for anything itself is refused; a command that isn't approved is left undecided, and refused before +// it's forwarded. `convox run`'s placeholder gets no env vars at all: it only sleeps, and it needs no MFA proof. +func (h *Handler) checkCommandEnv(r *http.Request, tracker *deployApprovalTracker) error { + command, ok := audit.RequestCommand(r) + if !ok { + return nil + } + if len(r.Header.Values("Command")) > 1 { + return refuse(http.StatusBadRequest, errors.New("the request gives more than one command")) + } + req, err := cmdenv.RequestFrom(r.Header) + if err != nil { + return refuse(http.StatusBadRequest, err) + } + if err := cmdenv.CheckCommand(command); err != nil { + return refuse(http.StatusBadRequest, err) + } + if hasApproval(tracker) && !req.IsZero() { + return refuse(http.StatusBadRequest, errApprovedCommandEnv) + } + + if isAttachedRunPlaceholder(r) { + *r = *r.WithContext(audit.WithCommandEnv(r.Context(), audit.CommandEnv{})) + return nil + } + app := extractAppFromPath(r.URL.Path) + if hasApproval(tracker) { + approved, ok := h.approvedCommand(app, command) + if !ok { + return nil + } + req = approved.Request() + } + plan, err := h.commandPlan(app, req) + if err != nil { + return err + } + *r = *r.WithContext(audit.WithCommandEnv(r.Context(), audit.CommandEnv{Plan: plan, Write: req.Write})) + return nil +} + +// commandPlan returns the env vars a command in app that asks for req gets (settings.CommandEnv.Plan). +func (h *Handler) commandPlan(app string, req cmdenv.Request) (cmdenv.Plan, error) { + var env settings.CommandEnv + if h.settingsService != nil { + var err error + if env, err = h.settingsService.GetCommandEnv(app); err != nil { + return cmdenv.Plan{}, fmt.Errorf("failed to read the env vars of %s's commands: %w", app, err) + } + } + plan, err := env.Plan(req) + if err != nil { + return cmdenv.Plan{}, refuse(http.StatusBadRequest, err) + } + return plan, nil +} + +// wrapCommand replaces the command in header, the headers forwarded to the rack for r, with the wrapper that runs it +// with the env vars checkCommandEnv decided (cmdenv.Build). The audit log, the deploy approval checks and the +// approval's record all read the command r carries, as it was sent. +func wrapCommand(r *http.Request, header http.Header) error { + command, ok := audit.RequestCommand(r) + if !ok { + return nil + } + env, decided := audit.CommandEnvFromContext(r.Context()) + if !decided { + return refuse(http.StatusForbidden, errEnvUndecided) + } + header.Set("Command", cmdenv.Build(env.Plan, command)) + return nil +} diff --git a/internal/gateway/proxy/command_env_test.go b/internal/gateway/proxy/command_env_test.go new file mode 100644 index 00000000..b3af9cef --- /dev/null +++ b/internal/gateway/proxy/command_env_test.go @@ -0,0 +1,245 @@ +package proxy + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/websocket" + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" + "github.com/DocSpring/rack-gateway/internal/convox" + "github.com/DocSpring/rack-gateway/internal/gateway/audit" + "github.com/DocSpring/rack-gateway/internal/gateway/auth" + "github.com/DocSpring/rack-gateway/internal/gateway/settings" +) + +// approvedCommands is approved_deploy_commands listing commands, each with no env vars of its own. +func approvedCommands(commands ...string) []settings.ApprovedCommand { + approved := make([]settings.ApprovedCommand, 0, len(commands)) + for _, command := range commands { + approved = append(approved, settings.ApprovedCommand{Command: command}) + } + return approved +} + +var ( + readOnlyDatabase = map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + stripeVars = []string{"DATABASE_URL", "PATH", "STRIPE_KEY"} +) + +// limitCommandEnv gives app's commands only vars, and DATABASE_URL from READ_ONLY_DATABASE_URL unless they write. +func (f *protectedEnvFixture) limitCommandEnv(t *testing.T, vars ...string) { + t.Helper() + require.NoError(t, settings.NewService(f.database).SetAppSettings("app", map[string]interface{}{ + settings.KeyCommandEnvVars: vars, settings.KeyCommandEnvReadOnly: readOnlyDatabase, + }, nil)) +} + +// forwarded reads the command the rack got back into its plan and the command it runs. +func forwarded(t *testing.T, header http.Header) (cmdenv.Plan, string) { + t.Helper() + require.Empty(t, header.Values(cmdenv.EnvHeader)) + require.Empty(t, header.Values(cmdenv.AllEnvVarsHeader)) + require.Empty(t, header.Values(cmdenv.WriteHeader)) + plan, command, ok := cmdenv.Parse(header.Get("Command")) + require.True(t, ok, "the rack got the command unwrapped: %s", header.Get("Command")) + return plan, command +} + +// A command gets the app's env vars and the ones it asks for, with the read-only ones from their substitutes. The +// rack gets the command wrapped to run that way; the audit log records the command as sent, and the env vars. +func TestACommandRunsWithTheAppsEnvVarsAndTheOnesItAsksFor(t *testing.T) { + f := newProtectedEnvFixture(t) + f.limitCommandEnv(t, "PATH", "DATABASE_URL") + rr := f.processRequest(t, http.MethodPost, processStartPath, f.totpCode(t), map[string]string{ + "Command": "rake db:migrate", cmdenv.EnvHeader: "STRIPE_KEY", + }) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + + starts := f.processStarts() + require.Len(t, starts, 1) + plan, command := forwarded(t, starts[0]) + require.Equal(t, "rake db:migrate", command) + require.Equal(t, cmdenv.Plan{Vars: stripeVars, ReadOnly: readOnlyDatabase}, plan) + + details := f.processStartDetails(t, "success") + require.Equal(t, "rake db:migrate", details["command"]) + require.Equal(t, []interface{}{"DATABASE_URL", "PATH", "STRIPE_KEY"}, details["env_vars"]) + require.Equal(t, map[string]interface{}{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}, details["env_read_only"]) + require.NotContains(t, details, "write") + require.NotContains(t, details, "all_env_vars") +} + +// --write gives a command the real values of the read-only vars, and the audit log says so. +func TestAWriteCommandGetsTheRealValues(t *testing.T) { + f := newProtectedEnvFixture(t) + f.limitCommandEnv(t, "PATH", "DATABASE_URL") + rr := f.processRequest(t, http.MethodPost, processStartPath, f.totpCode(t), map[string]string{ + "Command": "rake db:migrate", cmdenv.WriteHeader: "true", + }) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + + plan, _ := forwarded(t, f.processStarts()[0]) + require.Equal(t, cmdenv.Plan{Vars: []string{"DATABASE_URL", "PATH"}}, plan) + details := f.processStartDetails(t, "success") + require.Equal(t, true, details["write"]) + require.NotContains(t, details, "env_read_only") +} + +// An app that doesn't limit its commands' env vars gives each one every env var except RACK_URL, which no command +// gets. +func TestACommandInAnAppWithoutLimitsGetsEveryEnvVarButRackURL(t *testing.T) { + f := newProtectedEnvFixture(t) + rr := f.processRequest(t, http.MethodPost, processStartPath, f.totpCode(t), + map[string]string{"Command": "rake db:migrate"}) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + + plan, command := forwarded(t, f.processStarts()[0]) + require.Equal(t, cmdenv.Plan{All: true}, plan) + require.Equal(t, "rake db:migrate", command) + require.Equal(t, true, f.processStartDetails(t, "success")["all_env_vars"]) +} + +// `convox run`'s placeholder gets no env vars at all, whatever it asks for: it needs no MFA proof, and a command +// exec'd into its container could read its environment. +func TestTheRunPlaceholderGetsNoEnvVars(t *testing.T) { + f := newProtectedEnvFixture(t) + headers := placeholderStart("") + headers[cmdenv.AllEnvVarsHeader] = "true" + headers[cmdenv.WriteHeader] = "true" + rr := f.processRequest(t, http.MethodPost, processStartPath, "", headers) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + + plan, command := forwarded(t, f.processStarts()[0]) + require.Equal(t, cmdenv.Plan{}, plan) + require.Equal(t, convox.AttachedRunPlaceholder, command) + details := f.processStartDetails(t, "success") + require.Equal(t, []interface{}{}, details["env_vars"]) + require.NotContains(t, details, "write") +} + +// A command the gateway can't wrap, or one asking for env vars it can't have, is refused before it reaches the rack, +// and the refusal is audited. +func TestCommandsTheGatewayCantWrapAreRefused(t *testing.T) { + for _, c := range []struct { + reason string + headers map[string]string + }{ + {"no command can have RACK_URL", map[string]string{"Command": "rake", cmdenv.EnvHeader: "RACK_URL"}}, + {"isn't an env var name", map[string]string{"Command": "rake", cmdenv.EnvHeader: "A B"}}, + {"X-Command-Write must be true", map[string]string{"Command": "rake", cmdenv.WriteHeader: "1"}}, + { + "can't name some with --env too", + map[string]string{"Command": "rake", cmdenv.EnvHeader: "A", cmdenv.AllEnvVarsHeader: "true"}, + }, + {"the command is empty", map[string]string{"Command": " "}}, + {"the command is empty", map[string]string{}}, + {"can't start with NAME=value", map[string]string{"Command": "DATABASE_URL=postgres://x rake db:migrate"}}, + {"can't split the command", map[string]string{"Command": "rake 'unbalanced"}}, + } { + t.Run(c.reason, func(t *testing.T) { + f := newProtectedEnvFixture(t) + rr := f.processRequest(t, http.MethodPost, processStartPath, f.totpCode(t), c.headers) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + require.Contains(t, rr.Body.String(), c.reason) + require.Empty(t, f.processStarts()) + require.Contains(t, f.processStartDetails(t, "denied")["reason"], c.reason) + }) + } +} + +// A command in an app whose command settings can't be read is refused: running it with every env var would give it +// what the settings keep from it. +func TestACommandIsRefusedWhenTheAppsCommandSettingsCantBeRead(t *testing.T) { + f := newProtectedEnvFixture(t) + app := "app" + require.NoError(t, f.database.UpsertSetting(&app, settings.KeyCommandEnvVars, "PATH", nil)) + rr := f.processRequest(t, http.MethodPost, processStartPath, f.totpCode(t), map[string]string{"Command": "rake"}) + require.Equal(t, http.StatusInternalServerError, rr.Code, rr.Body.String()) + require.Empty(t, f.processStarts()) +} + +// An exec is wrapped the same way, and its audit entries record its env vars. +func TestAnExecRunsWithTheAppsEnvVars(t *testing.T) { + f := newProtectedEnvFixture(t) + f.limitCommandEnv(t, "PATH", "DATABASE_URL") + envReq, _ := f.request(t, f.admin, "", "", f.totpCode(t), stepUpAgeFresh) + authUser, ok := auth.GetAuthUser(envReq.Context()) + require.True(t, ok) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.h.ProxyToRack(w, r.WithContext(context.WithValue(r.Context(), auth.UserContextKey, authUser))) + })) + t.Cleanup(server.Close) + + header := http.Header{"Command": {"bin/rails console"}, cmdenv.EnvHeader: {"STRIPE_KEY"}} + conn, resp, err := websocket.DefaultDialer.Dial(websocketURL(server.URL)+"/apps/app/processes/p-1/exec", header) + require.NoError(t, err) + require.Equal(t, http.StatusSwitchingProtocols, resp.StatusCode) + // The rack ends the session at once; once the client's connection closes, the exec is on record. + _, _, err = conn.ReadMessage() + require.Error(t, err) + _ = conn.Close() + + f.rack.mu.Lock() + execs := append([]http.Header(nil), f.rack.execs...) + f.rack.mu.Unlock() + require.Len(t, execs, 1) + plan, command := forwarded(t, execs[0]) + require.Equal(t, "bin/rails console", command) + require.Equal(t, cmdenv.Plan{Vars: stripeVars, ReadOnly: readOnlyDatabase}, plan) + details := f.processStartDetails(t, "success") + require.Equal(t, "bin/rails console", details["command"]) + require.Equal(t, []interface{}{"DATABASE_URL", "PATH", "STRIPE_KEY"}, details["env_vars"]) +} + +// approvedCommandRequest is a request running command in docspring under the harness's approval, with headers. +func (e *approvalExecHarness) approvedCommandRequest(command string, headers map[string]string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/apps/docspring/processes/p1/exec", nil) + r.Header.Set("Command", command) + for name, value := range headers { + r.Header.Set(name, value) + } + tracker := &deployApprovalTracker{request: e.approval, app: "docspring"} + return r.WithContext(context.WithValue(r.Context(), deployApprovalContextKey, tracker)) +} + +// Under a deploy approval a command's entry in approved_deploy_commands decides its env vars, and a command that +// asks for any itself is refused. A command that isn't approved is left undecided, so it's never forwarded. +func TestAnApprovedCommandsEntryDecidesItsEnvVars(t *testing.T) { + e := newApprovalExecHarness(t) + app := "docspring" + require.NoError(t, e.database.UpsertSetting(&app, settings.KeyCommandEnvVars, []string{"PATH"}, nil)) + require.NoError(t, e.database.UpsertSetting(&app, settings.KeyCommandEnvReadOnly, readOnlyDatabase, nil)) + require.NoError(t, e.database.UpsertSetting(&app, settings.KeyApprovedDeployCommands, []settings.ApprovedCommand{ + {Command: "bin/pre_release_checks migrate", Env: []string{"DATABASE_URL"}, Write: true}, + {Command: "rake stripe:prepare", Env: []string{"DATABASE_URL", "STRIPE_KEY"}}, + {Command: "bin/pre_release_checks", AllEnvVars: true}, + }, nil)) + + for command, want := range map[string]audit.CommandEnv{ + "bin/pre_release_checks migrate": {Plan: cmdenv.Plan{Vars: []string{"DATABASE_URL", "PATH"}}, Write: true}, + "rake stripe:prepare": {Plan: cmdenv.Plan{ + Vars: []string{"DATABASE_URL", "PATH", "STRIPE_KEY"}, ReadOnly: readOnlyDatabase, + }}, + "bin/pre_release_checks": {Plan: cmdenv.Plan{All: true, ReadOnly: readOnlyDatabase}}, + } { + r := e.approvedCommandRequest(command, nil) + require.NoError(t, e.h.checkCommandEnv(r, getDeployApprovalTracker(r.Context())), command) + env, decided := audit.CommandEnvFromContext(r.Context()) + require.True(t, decided, command) + require.Equal(t, want, env, command) + } + + r := e.approvedCommandRequest("bin/pre_release_checks migrate", map[string]string{cmdenv.WriteHeader: "true"}) + err := e.h.checkCommandEnv(r, getDeployApprovalTracker(r.Context())) + require.ErrorIs(t, err, errApprovedCommandEnv) + + r = e.approvedCommandRequest("bin/rails console", nil) + require.NoError(t, e.h.checkCommandEnv(r, getDeployApprovalTracker(r.Context()))) + _, decided := audit.CommandEnvFromContext(r.Context()) + require.False(t, decided) + require.ErrorIs(t, wrapCommand(r, http.Header{}), errEnvUndecided) +} diff --git a/internal/gateway/proxy/command_length_test.go b/internal/gateway/proxy/command_length_test.go index 9e7a3a07..8319893b 100644 --- a/internal/gateway/proxy/command_length_test.go +++ b/internal/gateway/proxy/command_length_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/DocSpring/rack-gateway/internal/cmdenv" "github.com/DocSpring/rack-gateway/internal/gateway/audit" "github.com/DocSpring/rack-gateway/internal/gateway/db" ) @@ -67,7 +68,9 @@ func TestCommandsAtTheLimitAreRecordedInFull(t *testing.T) { rr := httptest.NewRecorder() h.ProxyToRack(rr, req) require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) - require.Equal(t, command, rack.last(t).Header.Get("Command"), route.path) + _, forwarded, ok := cmdenv.Parse(rack.last(t).Header.Get("Command")) + require.True(t, ok, route.path) + require.Equal(t, command, forwarded, route.path) attempt := auditDetailsFor(t, database, "admin@test.com", route.path, audit.StatusAttempted) require.Equal(t, command, attempt["command"], route.path) diff --git a/internal/gateway/proxy/deploy_approval_binding_test.go b/internal/gateway/proxy/deploy_approval_binding_test.go index 5e3a04a9..7e876755 100644 --- a/internal/gateway/proxy/deploy_approval_binding_test.go +++ b/internal/gateway/proxy/deploy_approval_binding_test.go @@ -342,7 +342,7 @@ func TestApprovedDeployCommandsAllowOnlyListedCommands(t *testing.T) { h, database := newProxyForDeployApprovalTest(t) app := "docspring" require.NoError(t, database.UpsertSetting( - &app, "approved_deploy_commands", []string{"bin/pre_release_checks migrate"}, nil, + &app, "approved_deploy_commands", approvedCommands("bin/pre_release_checks migrate"), nil, )) require.True(t, h.isCommandApproved("docspring", "bin/pre_release_checks migrate")) require.False(t, h.isCommandApproved("docspring", "bin/rails console")) @@ -363,7 +363,7 @@ func newApprovalExecHarness(t *testing.T) *approvalExecHarness { h, database := newProxyForDeployApprovalTest(t) app := "docspring" require.NoError(t, database.UpsertSetting( - &app, "approved_deploy_commands", []string{"bin/pre_release_checks migrate"}, nil, + &app, "approved_deploy_commands", approvedCommands("bin/pre_release_checks migrate"), nil, )) deployer := createUserWithRole(t, database, "deployer@example.com", "deployer") token, err := database.CreateAPIToken(strings.Repeat("e", 64), "ci", deployer.ID, nil, nil, nil) @@ -406,7 +406,7 @@ func TestApprovalRecordsCommandsWithoutTheirCredentials(t *testing.T) { e := newApprovalExecHarness(t) const command = "curl -H 'X-API-Key: s3cret' https://example.com/hooks/release" app := "docspring" - require.NoError(t, e.database.UpsertSetting(&app, "approved_deploy_commands", []string{command}, nil)) + require.NoError(t, e.database.UpsertSetting(&app, "approved_deploy_commands", approvedCommands(command), nil)) require.Zero(t, e.exec(command)) recorded, err := e.database.GetDeployApprovalRequest(e.approval.ID) diff --git a/internal/gateway/proxy/env_filters.go b/internal/gateway/proxy/env_filters.go index 4f7f2258..5f572ef3 100644 --- a/internal/gateway/proxy/env_filters.go +++ b/internal/gateway/proxy/env_filters.go @@ -10,6 +10,7 @@ import ( gtwlog "github.com/DocSpring/rack-gateway/internal/gateway/logging" "github.com/DocSpring/rack-gateway/internal/gateway/logutil" "github.com/DocSpring/rack-gateway/internal/gateway/rbac" + "github.com/DocSpring/rack-gateway/internal/gateway/settings" ) func (h *Handler) filterReleaseEnvForUser(r *http.Request, body []byte, app string) []byte { @@ -164,26 +165,32 @@ func (h *Handler) captureProcessCreation(_ *http.Request, body []byte, tracker * }) } -// isCommandApproved reports whether command is in the app's approved_deploy_commands. -// An empty list allows no commands under a deploy approval (fail closed). -func (h *Handler) isCommandApproved(app, command string) bool { +// approvedCommand returns command's entry in the app's approved_deploy_commands, and whether it has one. An empty +// list approves no commands under a deploy approval (fail closed), and so does one the gateway can't read. +func (h *Handler) approvedCommand(app, command string) (settings.ApprovedCommand, bool) { if h.settingsService == nil { - return false + return settings.ApprovedCommand{}, false } approvedCommands, err := h.settingsService.GetApprovedDeployCommands(app) if err != nil { log.Printf("Failed to get approved commands for app %s: %v", logutil.SanitizeForLog(app), err) - return false + return settings.ApprovedCommand{}, false } for _, approved := range approvedCommands { - if strings.TrimSpace(command) == strings.TrimSpace(approved) { - return true + if strings.TrimSpace(command) == strings.TrimSpace(approved.Command) { + return approved, true } } - return false + return settings.ApprovedCommand{}, false +} + +// isCommandApproved reports whether command is in the app's approved_deploy_commands (see approvedCommand). +func (h *Handler) isCommandApproved(app, command string) bool { + _, ok := h.approvedCommand(app, command) + return ok } // filterEnvironmentMapResponse masks values in the GET /apps/{app}/environment response. diff --git a/internal/gateway/proxy/env_protected_test.go b/internal/gateway/proxy/env_protected_test.go index 615d3b1c..385d9710 100644 --- a/internal/gateway/proxy/env_protected_test.go +++ b/internal/gateway/proxy/env_protected_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/gorilla/websocket" "github.com/pquerna/otp" "github.com/pquerna/otp/totp" "github.com/stretchr/testify/require" @@ -46,6 +47,7 @@ type protectedEnvRack struct { created []createdRelease buildStatus map[string]string processStarts []http.Header + execs []http.Header } type createdRelease struct { @@ -79,6 +81,8 @@ func (rk *protectedEnvRack) handler(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"id":"p-1","app":"app","name":"web"}`)) case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/apps/app/builds/"): rk.serveBuild(w, r) + case r.URL.Path == "/apps/app/processes/p-1/exec": + rk.serveExec(w, r) default: http.NotFound(w, r) } @@ -97,6 +101,18 @@ func (rk *protectedEnvRack) serveBuild(w http.ResponseWriter, r *http.Request) { _, _ = w.Write(body) } +// serveExec records an exec's headers and ends the session at once. +func (rk *protectedEnvRack) serveExec(w http.ResponseWriter, r *http.Request) { + rk.mu.Lock() + rk.execs = append(rk.execs, r.Header.Clone()) + rk.mu.Unlock() + conn, err := (&websocket.Upgrader{}).Upgrade(w, r, nil) + if err != nil { + return + } + _ = conn.Close() +} + // setBuildStatus gives the rack a build with status. func (rk *protectedEnvRack) setBuildStatus(id, status string) { rk.mu.Lock() diff --git a/internal/gateway/proxy/forward.go b/internal/gateway/proxy/forward.go index f3a825da..f4fed5a4 100644 --- a/internal/gateway/proxy/forward.go +++ b/internal/gateway/proxy/forward.go @@ -336,6 +336,9 @@ func (h *Handler) rackRequest( if err != nil { return nil, nil, err } + if err := wrapCommand(r, proxyReq.Header); err != nil { + return nil, nil, err + } client, err := h.httpClient(r.Context(), forwardTimeout(r, path)) if err != nil { log.Printf(`{"level":"error","event":"rack_tls_config_error","message":%q}`, err.Error()) diff --git a/internal/gateway/proxy/handler.go b/internal/gateway/proxy/handler.go index ea16d748..8e8dd0f3 100644 --- a/internal/gateway/proxy/handler.go +++ b/internal/gateway/proxy/handler.go @@ -189,6 +189,9 @@ func (h *Handler) checkBeforeRecording( if err := h.checkCommandJustification(r, approvalTracker); err != nil { return err } + if err := h.checkCommandEnv(r, approvalTracker); err != nil { + return err + } if err := audit.CheckCommandRecordable(r); err != nil { return refuse(http.StatusBadRequest, err) } diff --git a/internal/gateway/proxy/script_command_test.go b/internal/gateway/proxy/script_command_test.go index bceb1825..d6a775e6 100644 --- a/internal/gateway/proxy/script_command_test.go +++ b/internal/gateway/proxy/script_command_test.go @@ -9,7 +9,8 @@ import ( "github.com/DocSpring/rack-gateway/internal/scriptrun" ) -// A script command reaches the rack as sent, and its audit entry holds the script readably, not its base64. +// A script command reaches the rack as sent (inside the env wrapper), and its audit entry holds the script readably, +// not its base64. func TestScriptCommandIsForwardedAsSentAndRecordedReadably(t *testing.T) { f := newProtectedEnvFixture(t) text := "accounts = Account.where(plan: 'pro')\nputs accounts.count\n" @@ -20,7 +21,8 @@ func TestScriptCommandIsForwardedAsSentAndRecordedReadably(t *testing.T) { starts := f.processStarts() require.Len(t, starts, 1) - require.Equal(t, command, starts[0].Get("Command")) + _, forwardedCommand := forwarded(t, starts[0]) + require.Equal(t, command, forwardedCommand) details := f.processStartDetails(t, "success") require.Equal(t, "rails runner count.rb", details["command"]) require.Equal(t, text, details["script"]) diff --git a/internal/gateway/proxy/websocket.go b/internal/gateway/proxy/websocket.go index 9eff5697..c485c922 100644 --- a/internal/gateway/proxy/websocket.go +++ b/internal/gateway/proxy/websocket.go @@ -50,6 +50,9 @@ func (h *Handler) proxyWebSocket( } header := buildWebSocketHeaders(r, rack, authUser, wsURL) + if err := wrapCommand(r, header); err != nil { + return 0, err + } upstreamConn, resp, err := h.dialUpstreamWebSocket(r.Context(), wsURL, header, rack.URL) if err != nil { diff --git a/internal/gateway/rbac/gateway_routes.go b/internal/gateway/rbac/gateway_routes.go index 58e71b23..cfd2b802 100644 --- a/internal/gateway/rbac/gateway_routes.go +++ b/internal/gateway/rbac/gateway_routes.go @@ -279,6 +279,26 @@ var httpRouteSpecs = []RouteSpec{ appSettingPath(settings.AppSettingApprovedDeployCommands), GatewayAppSetting(settings.AppSettingApprovedDeployCommands), ), + newHTTPRoute( + "PUT", + appSettingPath(settings.AppSettingCommandEnvVars), + GatewayAppSetting(settings.AppSettingCommandEnvVars), + ), + newHTTPRoute( + "DELETE", + appSettingPath(settings.AppSettingCommandEnvVars), + GatewayAppSetting(settings.AppSettingCommandEnvVars), + ), + newHTTPRoute( + "PUT", + appSettingPath(settings.AppSettingCommandEnvReadOnly), + GatewayAppSetting(settings.AppSettingCommandEnvReadOnly), + ), + newHTTPRoute( + "DELETE", + appSettingPath(settings.AppSettingCommandEnvReadOnly), + GatewayAppSetting(settings.AppSettingCommandEnvReadOnly), + ), newHTTPRoute( "PUT", appSettingPath(settings.AppSettingServiceImagePatterns), diff --git a/internal/gateway/rbac/mfa_requirements.go b/internal/gateway/rbac/mfa_requirements.go index 3a569a3d..c33fbf56 100644 --- a/internal/gateway/rbac/mfa_requirements.go +++ b/internal/gateway/rbac/mfa_requirements.go @@ -149,6 +149,8 @@ var MFARequirements = map[string]MFALevel{ GatewayAppSetting(settings.AppSettingCIProvider): MFANone, // Just configuration GatewayAppSetting(settings.AppSettingCircleCIApprovalJobName): MFANone, // Just configuration GatewayAppSetting(settings.AppSettingCircleCIAutoApproveOnApproval): MFAStepUp, // Auto-approving + GatewayAppSetting(settings.AppSettingCommandEnvReadOnly): MFAAlways, // Read-only database access + GatewayAppSetting(settings.AppSettingCommandEnvVars): MFAAlways, // Env vars commands get GatewayAppSetting(settings.AppSettingDefaultBranch): MFANone, // Just configuration GatewayAppSetting(settings.AppSettingGitHubPostPRComment): MFANone, // Just notification preference GatewayAppSetting(settings.AppSettingGitHubVerification): MFAAlways, // Disabling GitHub verification diff --git a/internal/gateway/routes/route_registration.go b/internal/gateway/routes/route_registration.go index 4830ca5e..97f805d2 100644 --- a/internal/gateway/routes/route_registration.go +++ b/internal/gateway/routes/route_registration.go @@ -92,6 +92,10 @@ func registerAPIRoutes(authenticated *gin.RouterGroup, cfg *Config, h *handlerSe appSettings.DELETE("/audit-visible-env-vars", h.settings.DeleteAppAuditVisibleEnvVars) appSettings.PUT("/approved-deploy-commands", h.settings.UpdateAppApprovedDeployCommands) appSettings.DELETE("/approved-deploy-commands", h.settings.DeleteAppApprovedDeployCommands) + appSettings.PUT("/command-env-vars", h.settings.UpdateAppCommandEnvVars) + appSettings.DELETE("/command-env-vars", h.settings.DeleteAppCommandEnvVars) + appSettings.PUT("/command-env-read-only", h.settings.UpdateAppCommandEnvReadOnly) + appSettings.DELETE("/command-env-read-only", h.settings.DeleteAppCommandEnvReadOnly) appSettings.PUT("/service-image-patterns", h.settings.UpdateAppServiceImagePatterns) appSettings.DELETE("/service-image-patterns", h.settings.DeleteAppServiceImagePatterns) diff --git a/internal/gateway/settings/command_env.go b/internal/gateway/settings/command_env.go new file mode 100644 index 00000000..ccea5dee --- /dev/null +++ b/internal/gateway/settings/command_env.go @@ -0,0 +1,173 @@ +package settings + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "strings" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" +) + +// ApprovedCommand is a command a deploy approval may run (an entry of approved_deploy_commands), with the env vars +// it runs with. The command's own flags can't change them: under a deploy approval, its entry decides. +type ApprovedCommand struct { + // Command is the command, as it's run. + Command string `json:"command"` + // Env names the env vars it gets besides the app's command_env_vars. + Env []string `json:"env,omitempty"` + // AllEnvVars gives it every env var (except RACK_URL), as --all-env-vars does. + AllEnvVars bool `json:"all_env_vars,omitempty"` + // Write gives it the real values of the app's read-only env vars (command_env_read_only), as --write does. + Write bool `json:"write,omitempty"` +} + +// CommandEnv is what an app's settings say about the env vars of the commands run in it. +type CommandEnv struct { + // Vars are the env vars every command gets (command_env_vars). When there are none, every command gets every + // env var (except RACK_URL). + Vars []string + // ReadOnly maps an env var to the one a command gets its value from unless it's run with --write + // (command_env_read_only). + ReadOnly map[string]string +} + +// Request is what the entry gives its command, as the flags of a command run by hand would ask for it. +func (c ApprovedCommand) Request() cmdenv.Request { + return cmdenv.Request{Env: c.Env, AllEnvVars: c.AllEnvVars, Write: c.Write} +} + +// Plan returns the env vars a command that asks for req gets: the app's and the ones it names, or every env var +// when it asks for them all or the app doesn't limit them; the substitutes of the read-only vars unless it asks to +// write. +func (e CommandEnv) Plan(req cmdenv.Request) (cmdenv.Plan, error) { + if err := req.Check(); err != nil { + return cmdenv.Plan{}, err + } + readOnly := e.ReadOnly + if req.Write { + readOnly = nil + } + vars := append(append([]string(nil), e.Vars...), req.Env...) + return cmdenv.New(req.AllEnvVars || len(e.Vars) == 0, vars, readOnly) +} + +// commandEnvVarNames returns command_env_vars's value as env var names, or ErrInvalidSettingValue when it isn't a +// list of env var names (surrounding spaces aside), or it names RACK_URL. Names keep their case: a process's env +// var names are case-sensitive. +func commandEnvVarNames(value interface{}) ([]string, error) { + names, err := stringList(KeyCommandEnvVars, value) + if err != nil { + return nil, err + } + trimmed := make([]string, 0, len(names)) + for _, name := range names { + trimmed = append(trimmed, strings.TrimSpace(name)) + } + if err := cmdenv.CheckNames(trimmed); err != nil { + return nil, fmt.Errorf("%w: %s: %v", ErrInvalidSettingValue, KeyCommandEnvVars, err) + } + return trimmed, nil +} + +// readOnlyEnvMap returns command_env_read_only's value, or ErrInvalidSettingValue when it isn't an object of env +// var name to env var name that cmdenv.CheckReadOnly accepts. +func readOnlyEnvMap(value interface{}) (map[string]string, error) { + readOnly, err := stringMap(KeyCommandEnvReadOnly, value, "env var name to the env var it reads instead") + if err != nil { + return nil, err + } + if err := cmdenv.CheckReadOnly(readOnly); err != nil { + return nil, fmt.Errorf("%w: %s: %v", ErrInvalidSettingValue, KeyCommandEnvReadOnly, err) + } + return readOnly, nil +} + +// stringMap returns value as an object of strings, or ErrInvalidSettingValue (saying the object holds what) when +// it has any other shape. +func stringMap(key string, value interface{}, what string) (map[string]string, error) { + switch v := value.(type) { + case map[string]string: + return v, nil + case map[string]interface{}: + m := make(map[string]string, len(v)) + for name, raw := range v { + s, ok := raw.(string) + if !ok { + return nil, fmt.Errorf("%w: %s: the value for %s must be a string", ErrInvalidSettingValue, key, name) + } + m[name] = s + } + return m, nil + default: + return nil, fmt.Errorf("%w: %s must be an object of %s", ErrInvalidSettingValue, key, what) + } +} + +// approvedCommandList returns approved_deploy_commands's value, or ErrInvalidSettingValue when it isn't a JSON +// array of approved commands: objects with a command and only the fields ApprovedCommand has, each command one the +// gateway can run (see cmdenv.CheckCommand) and listed once, env naming env vars, and not both env and +// all_env_vars. +func approvedCommandList(value interface{}) ([]ApprovedCommand, error) { + commands, err := decodeApprovedCommands(value) + if err == nil { + err = checkApprovedCommands(commands) + } + if err != nil { + return nil, fmt.Errorf("%w: %s: %v", ErrInvalidSettingValue, KeyApprovedDeployCommands, err) + } + return commands, nil +} + +// decodeApprovedCommands reads value (as JSON decodes it, or already typed) as approved commands, refusing a field +// ApprovedCommand doesn't have. +func decodeApprovedCommands(value interface{}) ([]ApprovedCommand, error) { + if commands, ok := value.([]ApprovedCommand); ok { + return commands, nil + } + data, err := json.Marshal(value) + if err != nil || value == nil || !bytes.HasPrefix(data, []byte("[")) { + return nil, errors.New(`must be a JSON array of objects like {"command": "bin/migrate"}`) + } + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + var commands []ApprovedCommand + if err := decoder.Decode(&commands); err != nil { + return nil, fmt.Errorf("each entry must be an object with a command, and optionally env (a list of env var "+ + "names), all_env_vars and write (true or false): %w", err) + } + return commands, nil +} + +// checkApprovedCommands refuses a command the gateway can't run or that's listed twice, an env var name that isn't +// one, and env with all_env_vars. +func checkApprovedCommands(commands []ApprovedCommand) error { + seen := map[string]bool{} + for i, command := range commands { + text := strings.TrimSpace(command.Command) + if err := cmdenv.CheckCommand(text); err != nil { + return fmt.Errorf("entry %d: %w", i+1, err) + } + if seen[text] { + return fmt.Errorf("%q is listed more than once", text) + } + seen[text] = true + if err := cmdenv.CheckNames(command.Env); err != nil { + return fmt.Errorf("%q: %w", text, err) + } + if command.AllEnvVars && len(command.Env) > 0 { + return fmt.Errorf("%q: all_env_vars gives it every env var, so it can't have env as well", text) + } + } + return nil +} + +// parseEnvApprovedCommands reads approved_deploy_commands from its environment variable: a JSON array of objects. +func parseEnvApprovedCommands(raw string) ([]ApprovedCommand, error) { + var value interface{} + if err := json.Unmarshal([]byte(raw), &value); err != nil { + return nil, fmt.Errorf(`must be a JSON array of objects like {"command": "bin/migrate"}: %w`, err) + } + return approvedCommandList(value) +} diff --git a/internal/gateway/settings/command_env_test.go b/internal/gateway/settings/command_env_test.go new file mode 100644 index 00000000..04da3719 --- /dev/null +++ b/internal/gateway/settings/command_env_test.go @@ -0,0 +1,172 @@ +package settings + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" + "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" +) + +func TestValidateCommandEnvVars(t *testing.T) { + web := "web" + for _, value := range []interface{}{ + []interface{}{"PATH", " HOME ", "http_proxy"}, + []string{}, + } { + require.NoErrorf(t, ValidateSettingValue(&web, KeyCommandEnvVars, value), "%#v", value) + } + for _, value := range []interface{}{ + []interface{}{"PATH", "RACK_URL"}, + []interface{}{"BAD-NAME"}, + []interface{}{""}, + []interface{}{"PATH", 1}, + "PATH", + nil, + } { + require.ErrorIsf(t, ValidateSettingValue(&web, KeyCommandEnvVars, value), ErrInvalidSettingValue, "%#v", value) + } +} + +func TestValidateCommandEnvReadOnly(t *testing.T) { + web := "web" + valid := map[string]interface{}{ + "DATABASE_URL": "READ_ONLY_DATABASE_URL", "DATABASE_URL_DIRECT": "READ_ONLY_DATABASE_URL", + } + require.NoError(t, ValidateSettingValue(&web, KeyCommandEnvReadOnly, valid)) + require.NoError(t, ValidateSettingValue(&web, KeyCommandEnvReadOnly, map[string]interface{}{})) + for _, value := range []interface{}{ + map[string]interface{}{"RACK_URL": "X"}, + map[string]interface{}{"X": "RACK_URL"}, + map[string]interface{}{"A": "B", "B": "C"}, + map[string]interface{}{"A B": "C"}, + map[string]interface{}{"A": 1}, + []interface{}{"DATABASE_URL"}, + nil, + } { + err := ValidateSettingValue(&web, KeyCommandEnvReadOnly, value) + require.ErrorIsf(t, err, ErrInvalidSettingValue, "%#v", value) + } +} + +func TestValidateApprovedDeployCommands(t *testing.T) { + web := "web" + for _, value := range []interface{}{ + []interface{}{}, + []interface{}{map[string]interface{}{"command": "bin/pre_release_checks migrate", "write": true}}, + []interface{}{ + map[string]interface{}{"command": "rake stripe:prepare", "env": []interface{}{"STRIPE_SECRET_KEY"}}, + map[string]interface{}{"command": "bin/pre_release_checks", "all_env_vars": true, "write": false}, + }, + []ApprovedCommand{{Command: "bin/migrate", Write: true}}, + } { + require.NoErrorf(t, ValidateSettingValue(&web, KeyApprovedDeployCommands, value), "%#v", value) + } + for want, value := range map[string]interface{}{ + "must be a JSON array of objects": "bin/migrate", + "each entry must be an object": []interface{}{"bin/migrate"}, + `unknown field "cmd"`: []interface{}{map[string]interface{}{"cmd": "bin/migrate"}}, + "env": []interface{}{map[string]interface{}{"command": "x", "env": "A"}}, + "write": []interface{}{map[string]interface{}{"command": "x", "write": "yes"}}, + "entry 1: the command is empty": []interface{}{map[string]interface{}{"command": " "}}, + "can't start with NAME=value": []interface{}{map[string]interface{}{"command": "A=1 rake"}}, + "listed more than once": []interface{}{ + map[string]interface{}{"command": "bin/migrate"}, map[string]interface{}{"command": " bin/migrate"}, + }, + "no command can have RACK_URL": []interface{}{ + map[string]interface{}{"command": "x", "env": []interface{}{"RACK_URL"}}, + }, + "can't have env as well": []interface{}{ + map[string]interface{}{"command": "x", "env": []interface{}{"A"}, "all_env_vars": true}, + }, + } { + err := ValidateSettingValue(&web, KeyApprovedDeployCommands, value) + require.ErrorIsf(t, err, ErrInvalidSettingValue, "%#v", value) + require.ErrorContainsf(t, err, want, "%#v", value) + } + require.ErrorIs(t, ValidateSettingValue(&web, KeyApprovedDeployCommands, nil), ErrInvalidSettingValue) +} + +// The deployment config sets the command settings like any other: names as a list, the substitutes and the +// approved commands as JSON. +func TestCommandSettingsFromTheEnvironment(t *testing.T) { + svc := NewService(dbtest.NewDatabase(t)) + t.Setenv("RGW_APP_WEB_SETTING_COMMAND_ENV_VARS", "PATH, HOME") + t.Setenv("RGW_APP_WEB_SETTING_COMMAND_ENV_READ_ONLY", `{"DATABASE_URL":"READ_ONLY_DATABASE_URL"}`) + t.Setenv("RGW_APP_WEB_SETTING_APPROVED_DEPLOY_COMMANDS", + `[{"command":"bin/migrate","write":true},{"command":"rake stripe:prepare","env":["STRIPE_KEY"]}]`) + + env, err := svc.GetCommandEnv("web") + require.NoError(t, err) + require.Equal(t, CommandEnv{ + Vars: []string{"PATH", "HOME"}, ReadOnly: map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}, + }, env) + commands, err := svc.GetApprovedDeployCommands("web") + require.NoError(t, err) + require.Equal(t, []ApprovedCommand{ + {Command: "bin/migrate", Write: true}, {Command: "rake stripe:prepare", Env: []string{"STRIPE_KEY"}}, + }, commands) + + require.NoError(t, ValidateEnvironment([]string{ + "RGW_APP_WEB_SETTING_APPROVED_DEPLOY_COMMANDS=[{\"command\":\"bin/migrate\"}]", + })) + // The list of strings older releases took is refused, so the gateway won't start with it. + require.ErrorContains(t, ValidateEnvironment([]string{ + `RGW_APP_WEB_SETTING_APPROVED_DEPLOY_COMMANDS=["bin/migrate"]`, + }), "each entry must be an object") + require.ErrorContains(t, ValidateEnvironment([]string{"RGW_APP_WEB_SETTING_COMMAND_ENV_VARS=PATH,RACK_URL"}), + "no command can have RACK_URL") +} + +// Unset, the settings let every command have every env var, as before they existed. A stored value of the wrong +// shape is an error, which refuses the app's commands. +func TestGetCommandEnv(t *testing.T) { + database := dbtest.NewDatabase(t) + svc := NewService(database) + env, err := svc.GetCommandEnv("web") + require.NoError(t, err) + require.Equal(t, CommandEnv{Vars: []string{}, ReadOnly: map[string]string{}}, env) + + app := "web" + require.NoError(t, database.UpsertSetting(&app, KeyCommandEnvVars, []string{"PATH", "RACK_URL"}, nil)) + _, err = svc.GetCommandEnv(app) + require.ErrorIs(t, err, ErrInvalidSettingValue) + + require.NoError(t, database.UpsertSetting(&app, KeyCommandEnvVars, []string{"PATH"}, nil)) + require.NoError(t, database.UpsertSetting(&app, KeyCommandEnvReadOnly, []string{"DATABASE_URL"}, nil)) + _, err = svc.GetCommandEnv(app) + require.ErrorIs(t, err, ErrInvalidSettingValue) +} + +func TestCommandEnvPlan(t *testing.T) { + readOnly := map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"} + unlimited := CommandEnv{ReadOnly: readOnly} + limited := CommandEnv{Vars: []string{"PATH", "DATABASE_URL"}, ReadOnly: readOnly} + for _, c := range []struct { + env CommandEnv + names []string + all, write bool + want cmdenv.Plan + }{ + {env: CommandEnv{}, want: cmdenv.Plan{All: true}}, + {env: unlimited, want: cmdenv.Plan{All: true, ReadOnly: readOnly}}, + {env: unlimited, write: true, want: cmdenv.Plan{All: true}}, + {env: unlimited, names: []string{"STRIPE_KEY"}, want: cmdenv.Plan{All: true, ReadOnly: readOnly}}, + {env: limited, want: cmdenv.Plan{Vars: []string{"DATABASE_URL", "PATH"}, ReadOnly: readOnly}}, + { + env: limited, names: []string{"STRIPE_KEY"}, write: true, + want: cmdenv.Plan{Vars: []string{"DATABASE_URL", "PATH", "STRIPE_KEY"}}, + }, + {env: limited, all: true, want: cmdenv.Plan{All: true, ReadOnly: readOnly}}, + } { + plan, err := c.env.Plan(cmdenv.Request{Env: c.names, AllEnvVars: c.all, Write: c.write}) + require.NoError(t, err) + require.Equalf(t, c.want, plan, "%+v", c) + } + // A name the command asks for is checked whether or not the app limits its env vars. + for _, env := range []CommandEnv{limited, unlimited} { + _, err := env.Plan(cmdenv.Request{Env: []string{"RACK_URL"}}) + require.ErrorContains(t, err, "no command can have RACK_URL") + } +} diff --git a/internal/gateway/settings/defaults.go b/internal/gateway/settings/defaults.go index 50c6073f..34ce7eff 100644 --- a/internal/gateway/settings/defaults.go +++ b/internal/gateway/settings/defaults.go @@ -87,6 +87,8 @@ const ( AppSettingCIProvider AppSettingCircleCIApprovalJobName AppSettingCircleCIAutoApproveOnApproval + AppSettingCommandEnvReadOnly + AppSettingCommandEnvVars AppSettingDefaultBranch AppSettingGitHubPostPRComment AppSettingGitHubVerification @@ -108,6 +110,8 @@ const ( KeyCIProvider = "ci_provider" KeyCircleCIApprovalJobName = "circleci_approval_job_name" KeyCircleCIAutoApproveOnApproval = "circleci_auto_approve_on_approval" + KeyCommandEnvReadOnly = "command_env_read_only" + KeyCommandEnvVars = "command_env_vars" KeyDefaultBranch = "default_branch" KeyGitHubPostPRComment = "github_post_pr_comment" KeyGitHubVerification = "github_verification" @@ -127,6 +131,8 @@ var appSettingKeyToString = [...]string{ KeyCIProvider, KeyCircleCIApprovalJobName, KeyCircleCIAutoApproveOnApproval, + KeyCommandEnvReadOnly, + KeyCommandEnvVars, KeyDefaultBranch, KeyGitHubPostPRComment, KeyGitHubVerification, @@ -197,11 +203,13 @@ var DefaultGlobalSettings = map[string]interface{}{ // DefaultAppSettings defines all valid app-specific settings with their default values. var DefaultAppSettings = map[string]interface{}{ KeyAllowDeployFromDefaultBranch: false, - KeyApprovedDeployCommands: []string(nil), + KeyApprovedDeployCommands: []ApprovedCommand(nil), KeyAuditVisibleEnvVars: []string(nil), KeyCIProvider: nil, // nil means use global default KeyCircleCIApprovalJobName: "", KeyCircleCIAutoApproveOnApproval: false, + KeyCommandEnvReadOnly: map[string]string(nil), + KeyCommandEnvVars: []string(nil), KeyDefaultBranch: "main", KeyGitHubPostPRComment: true, KeyGitHubVerification: true, diff --git a/internal/gateway/settings/env.go b/internal/gateway/settings/env.go index d10d6137..c35085cb 100644 --- a/internal/gateway/settings/env.go +++ b/internal/gateway/settings/env.go @@ -106,6 +106,8 @@ func parseTyped(defaultValue interface{}, raw string) (interface{}, error) { return nil, fmt.Errorf("must be a JSON object of strings: %w", err) } return m, nil + case []ApprovedCommand: + return parseEnvApprovedCommands(raw) default: // strings, including optional ones with a nil default return parseEnvString(raw) } diff --git a/internal/gateway/settings/env_test.go b/internal/gateway/settings/env_test.go index 12b9e94e..a689e325 100644 --- a/internal/gateway/settings/env_test.go +++ b/internal/gateway/settings/env_test.go @@ -238,9 +238,11 @@ func TestMalformedStoredListsFailClosed(t *testing.T) { require.Nil(t, names) } - require.NoError(t, database.UpsertSetting(&app, KeyApprovedDeployCommands, "bin/migrate", nil)) - _, err := svc.GetApprovedDeployCommands(app) - require.ErrorIs(t, err, ErrInvalidSettingValue) + for _, value := range []interface{}{"bin/migrate", []string{"bin/migrate"}, nil} { + require.NoError(t, database.UpsertSetting(&app, KeyApprovedDeployCommands, value, nil)) + _, err := svc.GetApprovedDeployCommands(app) + require.ErrorIsf(t, err, ErrInvalidSettingValue, "stored %#v", value) + } commands, err := svc.GetApprovedDeployCommands("unset-app") require.NoError(t, err) require.Empty(t, commands) diff --git a/internal/gateway/settings/service_commands.go b/internal/gateway/settings/service_commands.go index e777d5b1..20a3c9f5 100644 --- a/internal/gateway/settings/service_commands.go +++ b/internal/gateway/settings/service_commands.go @@ -20,3 +20,39 @@ func (s *Service) GetRequireCommandJustification(appName string) (bool, error) { } return required, nil } + +// GetApprovedDeployCommands returns the commands a deploy approval may run in the app (approved_deploy_commands). +// A stored value that isn't a list of approved commands is an error, which allows none. +func (s *Service) GetApprovedDeployCommands(appName string) ([]ApprovedCommand, error) { + setting, err := s.GetAppSetting(appName, KeyApprovedDeployCommands, []ApprovedCommand{}) + if err != nil { + return nil, err + } + commands, err := approvedCommandList(setting.Value) + if err != nil { + return nil, fmt.Errorf("app %s: %w", appName, err) + } + return commands, nil +} + +// GetCommandEnv returns what the app's settings say about the env vars of its commands (command_env_vars and +// command_env_read_only). A stored value of the wrong shape is an error, which refuses the app's commands: running +// one with every env var would give it what the settings keep from it. +func (s *Service) GetCommandEnv(appName string) (CommandEnv, error) { + vars, err := s.GetAppSetting(appName, KeyCommandEnvVars, []string{}) + if err != nil { + return CommandEnv{}, err + } + readOnly, err := s.GetAppSetting(appName, KeyCommandEnvReadOnly, map[string]string{}) + if err != nil { + return CommandEnv{}, err + } + var env CommandEnv + if env.Vars, err = commandEnvVarNames(vars.Value); err != nil { + return CommandEnv{}, fmt.Errorf("app %s: %w", appName, err) + } + if env.ReadOnly, err = readOnlyEnvMap(readOnly.Value); err != nil { + return CommandEnv{}, fmt.Errorf("app %s: %w", appName, err) + } + return env, nil +} diff --git a/internal/gateway/settings/service_env_helpers.go b/internal/gateway/settings/service_env_helpers.go index 511a3d28..b10e6bde 100644 --- a/internal/gateway/settings/service_env_helpers.go +++ b/internal/gateway/settings/service_env_helpers.go @@ -63,12 +63,6 @@ func normalizeEnvVars(vars []string) []string { return result } -// GetApprovedDeployCommands returns approved commands for an app. A stored value that isn't a list of strings is -// an error, which allows no commands. -func (s *Service) GetApprovedDeployCommands(appName string) ([]string, error) { - return s.getListSetting(appName, KeyApprovedDeployCommands, stringList) -} - // GetServiceImagePatterns returns service image patterns for an app. A stored value that isn't an object of // compiling patterns is an error, which refuses the app's builds: an empty map would leave its images unchecked. func (s *Service) GetServiceImagePatterns(appName string) (map[string]string, error) { diff --git a/internal/gateway/settings/validate.go b/internal/gateway/settings/validate.go index fa9460b0..130c2f12 100644 --- a/internal/gateway/settings/validate.go +++ b/internal/gateway/settings/validate.go @@ -56,6 +56,9 @@ func checkSettingValue(isApp bool, key string, value interface{}) error { return err case []string, map[string]string: return validateCollection(key, value) + case []ApprovedCommand: + _, err := approvedCommandList(value) + return err default: // strings, including optional ones with a nil default return validateString(key, value) } @@ -105,10 +108,9 @@ func validateString(key string, value interface{}) error { } // stringListKeys are app settings stored as a JSON array of strings. Reading one back fails on any other shape -// (the gateway then treats every env var as secret and protected, and allows no deploy commands), so a value of -// another shape is refused before it is saved. +// (the gateway then treats every env var as secret and protected), so a value of another shape is refused before +// it is saved. var stringListKeys = map[string]bool{ - KeyApprovedDeployCommands: true, KeyAuditVisibleEnvVars: true, KeyProtectedEnvVars: true, AppSettingSecretEnvVars.String(): true, @@ -131,6 +133,12 @@ func validateCollection(key string, value interface{}) error { case stringListKeys[key]: _, err := stringList(key, value) return err + case key == KeyCommandEnvVars: + _, err := commandEnvVarNames(value) + return err + case key == KeyCommandEnvReadOnly: + _, err := readOnlyEnvMap(value) + return err case key == KeyServiceImagePatterns: return validateImagePatterns(value) default: @@ -195,21 +203,9 @@ func validateImagePatterns(value interface{}) error { // imagePatternMap returns value as a map of service name to image pattern, or ErrInvalidSettingValue when it isn't // an object of strings or a pattern doesn't compile (with {{GIT_COMMIT}} standing for a commit). func imagePatternMap(value interface{}) (map[string]string, error) { - patterns := map[string]string{} - switch v := value.(type) { - case map[string]string: - patterns = v - case map[string]interface{}: - for service, raw := range v { - pattern, ok := raw.(string) - if !ok { - return nil, fmt.Errorf("%w: image pattern for %s must be a string", ErrInvalidSettingValue, service) - } - patterns[service] = pattern - } - default: - return nil, fmt.Errorf("%w: %s must be an object of service name to pattern", ErrInvalidSettingValue, - KeyServiceImagePatterns) + patterns, err := stringMap(KeyServiceImagePatterns, value, "service name to pattern") + if err != nil { + return nil, err } services := make([]string, 0, len(patterns)) for service := range patterns { diff --git a/internal/gateway/settings/validate_test.go b/internal/gateway/settings/validate_test.go index 525fe818..55daa28b 100644 --- a/internal/gateway/settings/validate_test.go +++ b/internal/gateway/settings/validate_test.go @@ -8,7 +8,6 @@ import ( func TestValidateCollection(t *testing.T) { valid := map[string]interface{}{ - KeyApprovedDeployCommands: []interface{}{"bin/pre_release_checks migrate"}, KeyAuditVisibleEnvVars: []interface{}{"PORT"}, KeyProtectedEnvVars: []string{"DATABASE_URL"}, AppSettingSecretEnvVars.String(): []interface{}{}, @@ -22,8 +21,6 @@ func TestValidateCollection(t *testing.T) { } invalid := map[string]interface{}{ - // The shape the CLI E2E seed used to store: it read back as an empty list. - KeyApprovedDeployCommands: map[string]interface{}{"commands": []interface{}{"echo rake db:migrate"}}, KeyAuditVisibleEnvVars: map[string]interface{}{"PORT": true}, KeyProtectedEnvVars: "DATABASE_URL", AppSettingSecretEnvVars.String(): []interface{}{"A", 1}, @@ -55,8 +52,6 @@ func TestValidateCollectionRefusesInvalidEnvVarNames(t *testing.T) { require.ErrorIsf(t, err, ErrInvalidSettingValue, "key %s, names %q", key, names) } } - // Approved deploy commands are commands, not names. - require.NoError(t, validateCollection(KeyApprovedDeployCommands, []string{`bin/rails "db:migrate"`})) } // Each setting takes only values of its own type: a boolean sent as a string, a fractional or out-of-range number, diff --git a/internal/gateway/slack/process_notifications.go b/internal/gateway/slack/process_notifications.go index 7f7e775a..1cfcf0d4 100644 --- a/internal/gateway/slack/process_notifications.go +++ b/internal/gateway/slack/process_notifications.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/DocSpring/rack-gateway/internal/cmdenv" "github.com/DocSpring/rack-gateway/internal/convox" "github.com/DocSpring/rack-gateway/internal/gateway/audit" "github.com/DocSpring/rack-gateway/internal/gateway/db" @@ -33,6 +34,11 @@ type processCommand struct { Script string `json:"script"` ScriptName string `json:"script_name"` ScriptSHA256 string `json:"script_sha256"` + // The env vars the command got (see audit.CommandEnv). + EnvVars []string `json:"env_vars"` + AllEnvVars bool `json:"all_env_vars"` + EnvReadOnly map[string]string `json:"env_read_only"` + Write bool `json:"write"` } // parseProcessCommand reads the command of a process action's audit entry. ok is false for any other entry. @@ -79,6 +85,20 @@ func (p processCommand) where(action string) string { return "in a new process" } +// env describes the env vars the command got, as escaped mrkdwn, with a warning when it was given the real values of +// the app's read-only ones, or "" when its entry doesn't say (an entry from before the gateway recorded them). +func (p processCommand) env() string { + if p.EnvVars == nil && !p.AllEnvVars { + return "" + } + text := escapeMrkdwn(cmdenv.Plan{All: p.AllEnvVars, Vars: p.EnvVars, ReadOnly: p.EnvReadOnly}.Describe()) + if p.Write { + text = ":warning: *Can change data:* run with --write, so it got the real values of the read-only env " + + "vars\n" + text + } + return truncateMrkdwn(text, maxSectionValue) +} + // size describes the command's length and its script, without their text. func (p processCommand) size() string { text := "Command: " + strconv.Itoa(len(p.Command)) + " bytes" @@ -119,8 +139,13 @@ func (n *Notifier) formatProcessMessage( }, }, mrkdwnSection("*Justification:*\n" + justification), - {"type": "context", "elements": []map[string]interface{}{mrkdwnElement(command.size())}}, } + if env := command.env(); env != "" { + blocks = append(blocks, mrkdwnSection("*Env vars:*\n"+env)) + } + blocks = append(blocks, map[string]interface{}{ + "type": "context", "elements": []map[string]interface{}{mrkdwnElement(command.size())}, + }) if link := n.auditEntryLink(auditLog.ID); link != "" { blocks = append(blocks, mrkdwnSection(link)) } diff --git a/internal/gateway/slack/process_notifications_test.go b/internal/gateway/slack/process_notifications_test.go index 2b1b8f24..1d12edde 100644 --- a/internal/gateway/slack/process_notifications_test.go +++ b/internal/gateway/slack/process_notifications_test.go @@ -114,3 +114,39 @@ func TestOnlyProcessCommandsGetTheProcessFormat(t *testing.T) { _, ok = parseProcessCommand(&db.AuditLog{Action: actionProcessStart, Details: "not json"}) require.False(t, ok) } + +// A command's message says which env vars it got, and warns when it was given the real values of the read-only ones. +// An entry from before the gateway recorded them says nothing about them. +func TestProcessMessageSaysWhichEnvVarsTheCommandGot(t *testing.T) { + for want, env := range map[string]map[string]interface{}{ + "*Env vars:*\\nDATABASE_URL (from READ_ONLY_DATABASE_URL), PATH": { + "env_vars": []string{"DATABASE_URL", "PATH"}, + "env_read_only": map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}, + }, + "*Env vars:*\\n:warning: *Can change data:* run with --write": { + "env_vars": []string{"DATABASE_URL", "PATH"}, "write": true, + }, + "*Env vars:*\\nevery env var except RACK_URL": {"all_env_vars": true}, + "*Env vars:*\\nno env vars": {"env_vars": []string{}}, + } { + details := map[string]interface{}{ + "path": "/apps/myapp/processes/p1/exec", "process_id": "p1", "command": "rake", + } + for key, value := range env { + details[key] = value + } + data, err := json.Marshal(details) + require.NoError(t, err) + entry := processEntry(t, actionProcessExecStart, audit.StatusSuccess, nil) + entry.Details = string(data) + + text, blocks := NewNotifier(nil, testGateway).formatAuditLogMessage(entry) + require.Contains(t, messageText(t, text, blocks), want) + } + + entry := processEntry(t, actionProcessExecStart, audit.StatusSuccess, map[string]string{ + "path": "/apps/myapp/processes/p1/exec", "process_id": "p1", "command": "rake", + }) + text, blocks := NewNotifier(nil, testGateway).formatAuditLogMessage(entry) + require.NotContains(t, messageText(t, text, blocks), "Env vars") +} diff --git a/scripts/lib/cli-e2e/db.sh b/scripts/lib/cli-e2e/db.sh index 15929eb4..6e40bc7c 100644 --- a/scripts/lib/cli-e2e/db.sh +++ b/scripts/lib/cli-e2e/db.sh @@ -137,7 +137,7 @@ INSERT INTO settings (app_name, key, value, updated_at) VALUES (NULL, 'allow_destructive_actions', 'false'::jsonb, NOW()), (NULL, 'deploy_approvals_enabled', 'true'::jsonb, NOW()), (NULL, 'deploy_approval_window_minutes', '15'::jsonb, NOW()), - ('rack-gateway', 'approved_deploy_commands', '["echo rake db:migrate"]'::jsonb, NOW()), + ('rack-gateway', 'approved_deploy_commands', '[{"command": "echo rake db:migrate"}]'::jsonb, NOW()), ('rack-gateway', 'github_verification', 'false'::jsonb, NOW()), ('rack-gateway', 'service_image_patterns', '{"gateway": "docspringcom/rack-gateway:{{GIT_COMMIT}}-amd64"}'::jsonb, NOW()) ON CONFLICT (COALESCE(app_name, ''), key) DO UPDATE diff --git a/web/src/api/generated.ts b/web/src/api/generated.ts index 63405674..165bf1b2 100644 --- a/web/src/api/generated.ts +++ b/web/src/api/generated.ts @@ -14,6 +14,8 @@ import type { DeleteAppsAppSettings200, DeleteAppsAppSettingsApprovedDeployCommands200, DeleteAppsAppSettingsAuditVisibleEnvVars200, + DeleteAppsAppSettingsCommandEnvReadOnly200, + DeleteAppsAppSettingsCommandEnvVars200, DeleteAppsAppSettingsParams, DeleteAppsAppSettingsProtectedEnvVars200, DeleteAppsAppSettingsRequireCommandJustification200, @@ -94,6 +96,9 @@ import type { PutAppsAppSettingsApprovedDeployCommands200, PutAppsAppSettingsAuditVisibleEnvVars200, PutAppsAppSettingsBody, + PutAppsAppSettingsCommandEnvReadOnly200, + PutAppsAppSettingsCommandEnvReadOnlyBody, + PutAppsAppSettingsCommandEnvVars200, PutAppsAppSettingsProtectedEnvVars200, PutAppsAppSettingsRequireCommandJustification200, PutAppsAppSettingsSecretEnvVars200, @@ -101,6 +106,7 @@ import type { PutAppsAppSettingsServiceImagePatternsBody, PutSettings200, PutSettingsBody, + SettingsApprovedCommand, SettingsSetting, } from "./schemas"; @@ -311,7 +317,7 @@ export const getRackGatewayAPI = () => { */ const putAppsAppSettingsApprovedDeployCommands = ( app: string, - putAppsAppSettingsApprovedDeployCommandsBody: string[], + settingsApprovedCommand: SettingsApprovedCommand[], options?: SecondParameter< typeof createGatewayClient >, @@ -321,7 +327,7 @@ export const getRackGatewayAPI = () => { url: `/apps/${app}/settings/approved-deploy-commands`, method: "PUT", headers: { "Content-Type": "application/json" }, - data: putAppsAppSettingsApprovedDeployCommandsBody, + data: settingsApprovedCommand, }, options, ); @@ -385,6 +391,82 @@ export const getRackGatewayAPI = () => { ); }; + /** + * Replaces the env vars commands in the specified app get from another one unless run with --write + * @summary Update the read-only env vars of commands + */ + const putAppsAppSettingsCommandEnvReadOnly = ( + app: string, + putAppsAppSettingsCommandEnvReadOnlyBody: PutAppsAppSettingsCommandEnvReadOnlyBody, + options?: SecondParameter< + typeof createGatewayClient + >, + ) => { + return createGatewayClient( + { + url: `/apps/${app}/settings/command-env-read-only`, + method: "PUT", + headers: { "Content-Type": "application/json" }, + data: putAppsAppSettingsCommandEnvReadOnlyBody, + }, + options, + ); + }; + + /** + * Removes the stored substitutes, so commands in the specified app get every env var's own value + * @summary Delete the read-only env vars of commands + */ + const deleteAppsAppSettingsCommandEnvReadOnly = ( + app: string, + options?: SecondParameter< + typeof createGatewayClient + >, + ) => { + return createGatewayClient( + { url: `/apps/${app}/settings/command-env-read-only`, method: "DELETE" }, + options, + ); + }; + + /** + * Replaces the env vars every command run in the specified app (exec, run) gets. Empty: every env var + * @summary Update the env vars commands get + */ + const putAppsAppSettingsCommandEnvVars = ( + app: string, + putAppsAppSettingsCommandEnvVarsBody: string[], + options?: SecondParameter< + typeof createGatewayClient + >, + ) => { + return createGatewayClient( + { + url: `/apps/${app}/settings/command-env-vars`, + method: "PUT", + headers: { "Content-Type": "application/json" }, + data: putAppsAppSettingsCommandEnvVarsBody, + }, + options, + ); + }; + + /** + * Removes the stored list, so every command run in the specified app gets every env var + * @summary Delete the env vars commands get + */ + const deleteAppsAppSettingsCommandEnvVars = ( + app: string, + options?: SecondParameter< + typeof createGatewayClient + >, + ) => { + return createGatewayClient( + { url: `/apps/${app}/settings/command-env-vars`, method: "DELETE" }, + options, + ); + }; + /** * Replaces the list of protected environment variables for the specified app * @summary Update protected environment variables @@ -1640,6 +1722,10 @@ export const getRackGatewayAPI = () => { deleteAppsAppSettingsApprovedDeployCommands, putAppsAppSettingsAuditVisibleEnvVars, deleteAppsAppSettingsAuditVisibleEnvVars, + putAppsAppSettingsCommandEnvReadOnly, + deleteAppsAppSettingsCommandEnvReadOnly, + putAppsAppSettingsCommandEnvVars, + deleteAppsAppSettingsCommandEnvVars, putAppsAppSettingsProtectedEnvVars, deleteAppsAppSettingsProtectedEnvVars, putAppsAppSettingsRequireCommandJustification, @@ -1797,6 +1883,40 @@ export type DeleteAppsAppSettingsAuditVisibleEnvVarsResult = NonNullable< > > >; +export type PutAppsAppSettingsCommandEnvReadOnlyResult = NonNullable< + Awaited< + ReturnType< + ReturnType< + typeof getRackGatewayAPI + >["putAppsAppSettingsCommandEnvReadOnly"] + > + > +>; +export type DeleteAppsAppSettingsCommandEnvReadOnlyResult = NonNullable< + Awaited< + ReturnType< + ReturnType< + typeof getRackGatewayAPI + >["deleteAppsAppSettingsCommandEnvReadOnly"] + > + > +>; +export type PutAppsAppSettingsCommandEnvVarsResult = NonNullable< + Awaited< + ReturnType< + ReturnType["putAppsAppSettingsCommandEnvVars"] + > + > +>; +export type DeleteAppsAppSettingsCommandEnvVarsResult = NonNullable< + Awaited< + ReturnType< + ReturnType< + typeof getRackGatewayAPI + >["deleteAppsAppSettingsCommandEnvVars"] + > + > +>; export type PutAppsAppSettingsProtectedEnvVarsResult = NonNullable< Awaited< ReturnType< diff --git a/web/src/api/openapi.json b/web/src/api/openapi.json index 11fcb24f..0d39f6c3 100644 --- a/web/src/api/openapi.json +++ b/web/src/api/openapi.json @@ -827,12 +827,12 @@ "schema": { "type": "array", "items": { - "type": "string" + "$ref": "#/components/schemas/settings.ApprovedCommand" } } } }, - "description": "Approved command list", + "description": "Approved commands, each with the env vars it gets", "required": true }, "responses": { @@ -1091,6 +1091,302 @@ } } }, + "/apps/{app}/settings/command-env-read-only": { + "put": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Replaces the env vars commands in the specified app get from another one unless run with --write", + "tags": [ + "Settings" + ], + "summary": "Update the read-only env vars of commands", + "parameters": [ + { + "description": "App name", + "name": "app", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + } + }, + "description": "Env var name to the env var it reads instead", + "required": true + }, + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/settings.Setting" + } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + } + } + }, + "delete": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Removes the stored substitutes, so commands in the specified app get every env var's own value", + "tags": [ + "Settings" + ], + "summary": "Delete the read-only env vars of commands", + "parameters": [ + { + "description": "App name", + "name": "app", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/settings.Setting" + } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + } + } + } + }, + "/apps/{app}/settings/command-env-vars": { + "put": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Replaces the env vars every command run in the specified app (exec, run) gets. Empty: every env var", + "tags": [ + "Settings" + ], + "summary": "Update the env vars commands get", + "parameters": [ + { + "description": "App name", + "name": "app", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "type": "array", + "items": { + "type": "string" + } + } + } + }, + "description": "Env var names", + "required": true + }, + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/settings.Setting" + } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + } + } + }, + "delete": { + "security": [ + { + "SessionCookie": [] + }, + { + "CSRFToken": [] + } + ], + "description": "Removes the stored list, so every command run in the specified app gets every env var", + "tags": [ + "Settings" + ], + "summary": "Delete the env vars commands get", + "parameters": [ + { + "description": "App name", + "name": "app", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/settings.Setting" + } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + } + } + } + }, "/apps/{app}/settings/protected-env-vars": { "put": { "security": [ @@ -7589,6 +7885,30 @@ } } }, + "settings.ApprovedCommand": { + "type": "object", + "properties": { + "all_env_vars": { + "description": "AllEnvVars gives it every env var (except RACK_URL), as --all-env-vars does.", + "type": "boolean" + }, + "command": { + "description": "Command is the command, as it's run.", + "type": "string" + }, + "env": { + "description": "Env names the env vars it gets besides the app's command_env_vars.", + "type": "array", + "items": { + "type": "string" + } + }, + "write": { + "description": "Write gives it the real values of the app's read-only env vars (command_env_read_only), as --write does.", + "type": "boolean" + } + } + }, "settings.Setting": { "type": "object", "properties": { diff --git a/web/src/api/schemas/deleteAppsAppSettingsCommandEnvReadOnly200.ts b/web/src/api/schemas/deleteAppsAppSettingsCommandEnvReadOnly200.ts new file mode 100644 index 00000000..73cc7845 --- /dev/null +++ b/web/src/api/schemas/deleteAppsAppSettingsCommandEnvReadOnly200.ts @@ -0,0 +1,12 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ +import type { SettingsSetting } from "./settingsSetting"; + +export type DeleteAppsAppSettingsCommandEnvReadOnly200 = { + [key: string]: SettingsSetting; +}; diff --git a/web/src/api/schemas/deleteAppsAppSettingsCommandEnvVars200.ts b/web/src/api/schemas/deleteAppsAppSettingsCommandEnvVars200.ts new file mode 100644 index 00000000..9a4b2625 --- /dev/null +++ b/web/src/api/schemas/deleteAppsAppSettingsCommandEnvVars200.ts @@ -0,0 +1,12 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ +import type { SettingsSetting } from "./settingsSetting"; + +export type DeleteAppsAppSettingsCommandEnvVars200 = { + [key: string]: SettingsSetting; +}; diff --git a/web/src/api/schemas/index.ts b/web/src/api/schemas/index.ts index 00c4a7be..d80ae994 100644 --- a/web/src/api/schemas/index.ts +++ b/web/src/api/schemas/index.ts @@ -21,6 +21,8 @@ export * from "./dbUser"; export * from "./deleteAppsAppSettings200"; export * from "./deleteAppsAppSettingsApprovedDeployCommands200"; export * from "./deleteAppsAppSettingsAuditVisibleEnvVars200"; +export * from "./deleteAppsAppSettingsCommandEnvReadOnly200"; +export * from "./deleteAppsAppSettingsCommandEnvVars200"; export * from "./deleteAppsAppSettingsParams"; export * from "./deleteAppsAppSettingsProtectedEnvVars200"; export * from "./deleteAppsAppSettingsRequireCommandJustification200"; @@ -137,6 +139,9 @@ export * from "./putAppsAppSettings200"; export * from "./putAppsAppSettingsApprovedDeployCommands200"; export * from "./putAppsAppSettingsAuditVisibleEnvVars200"; export * from "./putAppsAppSettingsBody"; +export * from "./putAppsAppSettingsCommandEnvReadOnly200"; +export * from "./putAppsAppSettingsCommandEnvReadOnlyBody"; +export * from "./putAppsAppSettingsCommandEnvVars200"; export * from "./putAppsAppSettingsProtectedEnvVars200"; export * from "./putAppsAppSettingsRequireCommandJustification200"; export * from "./putAppsAppSettingsSecretEnvVars200"; @@ -144,6 +149,7 @@ export * from "./putAppsAppSettingsServiceImagePatterns200"; export * from "./putAppsAppSettingsServiceImagePatternsBody"; export * from "./putSettings200"; export * from "./putSettingsBody"; +export * from "./settingsApprovedCommand"; export * from "./settingsSetting"; export * from "./settingsSettingSource"; export * from "./valueBody"; diff --git a/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnly200.ts b/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnly200.ts new file mode 100644 index 00000000..27416e30 --- /dev/null +++ b/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnly200.ts @@ -0,0 +1,12 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ +import type { SettingsSetting } from "./settingsSetting"; + +export type PutAppsAppSettingsCommandEnvReadOnly200 = { + [key: string]: SettingsSetting; +}; diff --git a/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnlyBody.ts b/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnlyBody.ts new file mode 100644 index 00000000..4fa94029 --- /dev/null +++ b/web/src/api/schemas/putAppsAppSettingsCommandEnvReadOnlyBody.ts @@ -0,0 +1,11 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ + +export type PutAppsAppSettingsCommandEnvReadOnlyBody = { + [key: string]: string; +}; diff --git a/web/src/api/schemas/putAppsAppSettingsCommandEnvVars200.ts b/web/src/api/schemas/putAppsAppSettingsCommandEnvVars200.ts new file mode 100644 index 00000000..db866922 --- /dev/null +++ b/web/src/api/schemas/putAppsAppSettingsCommandEnvVars200.ts @@ -0,0 +1,12 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ +import type { SettingsSetting } from "./settingsSetting"; + +export type PutAppsAppSettingsCommandEnvVars200 = { + [key: string]: SettingsSetting; +}; diff --git a/web/src/api/schemas/settingsApprovedCommand.ts b/web/src/api/schemas/settingsApprovedCommand.ts new file mode 100644 index 00000000..f4aeb2b5 --- /dev/null +++ b/web/src/api/schemas/settingsApprovedCommand.ts @@ -0,0 +1,18 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ + +export interface SettingsApprovedCommand { + /** AllEnvVars gives it every env var (except RACK_URL), as --all-env-vars does. */ + all_env_vars?: boolean; + /** Command is the command, as it's run. */ + command?: string; + /** Env names the env vars it gets besides the app's command_env_vars. */ + env?: string[]; + /** Write gives it the real values of the app's read-only env vars (command_env_read_only), as --write does. */ + write?: boolean; +} diff --git a/web/src/lib/generated/mfa-requirements.ts b/web/src/lib/generated/mfa-requirements.ts index 86f9bc9f..9a720d22 100644 --- a/web/src/lib/generated/mfa-requirements.ts +++ b/web/src/lib/generated/mfa-requirements.ts @@ -30,6 +30,18 @@ export const HTTP_ROUTE_MFA_REQUIREMENTS: HttpRouteMfaRequirement[] = [ permissions: ["gateway:setting:audit_visible_env_vars"], mfaLevel: "step_up", }, + { + method: "DELETE", + pattern: "/api/v1/apps/:app/settings/command-env-read-only", + permissions: ["gateway:setting:command_env_read_only"], + mfaLevel: "always", + }, + { + method: "DELETE", + pattern: "/api/v1/apps/:app/settings/command-env-vars", + permissions: ["gateway:setting:command_env_vars"], + mfaLevel: "always", + }, { method: "DELETE", pattern: "/api/v1/apps/:app/settings/protected-env-vars", @@ -504,6 +516,18 @@ export const HTTP_ROUTE_MFA_REQUIREMENTS: HttpRouteMfaRequirement[] = [ permissions: ["gateway:setting:audit_visible_env_vars"], mfaLevel: "step_up", }, + { + method: "PUT", + pattern: "/api/v1/apps/:app/settings/command-env-read-only", + permissions: ["gateway:setting:command_env_read_only"], + mfaLevel: "always", + }, + { + method: "PUT", + pattern: "/api/v1/apps/:app/settings/command-env-vars", + permissions: ["gateway:setting:command_env_vars"], + mfaLevel: "always", + }, { method: "PUT", pattern: "/api/v1/apps/:app/settings/protected-env-vars", From deaeccdf50e93925a9578a247887f20c0e0ec6c3 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 12:51:52 +1300 Subject: [PATCH 03/12] test(mock-convox): run only commands the gateway wrapped, and print their env vars The mock rack reads the env wrapper back (cmdenv.Parse) and prints the command and the env vars it gets. A process start or exec the gateway did not wrap fails, so the CLI E2E tests catch a command reaching the rack with every env var. --- cmd/mock-convox/handlers_processes.go | 31 ++++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/cmd/mock-convox/handlers_processes.go b/cmd/mock-convox/handlers_processes.go index e7b30035..ab5f23e6 100644 --- a/cmd/mock-convox/handlers_processes.go +++ b/cmd/mock-convox/handlers_processes.go @@ -11,6 +11,7 @@ import ( "github.com/gorilla/websocket" mclog "github.com/DocSpring/rack-gateway/cmd/mock-convox/logging" + "github.com/DocSpring/rack-gateway/internal/cmdenv" ) func getProcesses(w http.ResponseWriter, r *http.Request) { @@ -83,6 +84,13 @@ func serviceProcesses(w http.ResponseWriter, r *http.Request) { rawBody, ) + // A rack runs whatever command it gets. This one refuses a command the gateway didn't wrap (cmdenv), so the + // CLI E2E tests catch a command reaching the rack with every env var. + if _, _, ok := cmdenv.Parse(r.Header.Get("Command")); !ok { + http.Error(w, "mock rack: the gateway didn't wrap the command", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusAccepted) writeJSON(w, map[string]interface{}{ @@ -115,17 +123,20 @@ func execProcess(w http.ResponseWriter, r *http.Request) { }() _ = conn.WriteMessage(websocket.TextMessage, []byte("Connected to mock exec for app="+app+" pid="+id+"\n")) - cmd := r.Header.Get("command") - if cmd == "" { - cmd = r.URL.Query().Get("command") + // The rack runs the command the gateway wrapped (cmdenv): this one prints the command and the env vars it gets. + // A command the gateway didn't wrap fails, so the CLI E2E tests catch it. + plan, cmd, ok := cmdenv.Parse(r.Header.Get("Command")) + if !ok { + _ = conn.WriteMessage(websocket.TextMessage, []byte("mock rack: the gateway didn't wrap the command\n")) + _ = conn.WriteMessage(websocket.TextMessage, []byte(execExitMarker+"1\n")) + return } - if cmd != "" { - _ = conn.WriteMessage(websocket.TextMessage, []byte("$ "+cmd+"\n")) - if strings.HasPrefix(cmd, "echo ") { - _ = conn.WriteMessage(websocket.TextMessage, []byte(strings.TrimPrefix(cmd, "echo ")+"\n")) - } else { - _ = conn.WriteMessage(websocket.TextMessage, []byte("(mock output)\n")) - } + _ = conn.WriteMessage(websocket.TextMessage, []byte("$ "+cmd+"\n")) + _ = conn.WriteMessage(websocket.TextMessage, []byte("env: "+plan.Describe()+"\n")) + if strings.HasPrefix(cmd, "echo ") { + _ = conn.WriteMessage(websocket.TextMessage, []byte(strings.TrimPrefix(cmd, "echo ")+"\n")) + } else { + _ = conn.WriteMessage(websocket.TextMessage, []byte("(mock output)\n")) } // A rack ends every exec stream with this marker and the exit code. The CLI strips the line, exits with the // code, and fails when the marker is missing. From 48f190d35d55cc0f35c90a1ce180b463078c2fe8 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 13:19:39 +1300 Subject: [PATCH 04/12] feat(cli): choose a command's env vars with --env, --all-env-vars and --write exec and run send what the flags ask for in the X-Command-Env, X-Command-All-Env-Vars and X-Command-Write headers, which the gateway reads to decide the command's env vars. The CLI checks the names and the command (cmdenv.CheckCommand) before anything is sent. The PIN dialog lists the env vars the command gets, read from the app's settings, and shows warnings in red: --write gives the real value of a read-only env var, --all-env-vars gives every env var where the app limits them, and an exec can read its process's environment. The touch prompt says when the command can change data. The script review and --dry-run show the same warnings. The PIN and script dialogs move from AppleScript to a JXA NSAlert (internal/cli/macdialog) so they can show the warnings with a red icon. --- internal/cli/convox_process_approval.go | 79 +++++++---- internal/cli/convox_process_command.go | 30 ++++- internal/cli/convox_process_command_test.go | 1 + internal/cli/convox_process_env.go | 108 +++++++++++++++ internal/cli/convox_process_env_test.go | 125 ++++++++++++++++++ internal/cli/convox_process_review.go | 1 + internal/cli/convox_processes.go | 1 - internal/cli/macdialog/macdialog.go | 76 +++++++++++ internal/cli/pinentry/pinentry.go | 89 ++++++++----- internal/cli/pinentry/pinentry_test.go | 39 ++++-- internal/cli/scriptreview/scriptreview.go | 74 ++++++----- .../cli/scriptreview/scriptreview_test.go | 27 +++- internal/cli/webauthn/types.go | 3 + internal/cli/webauthn/webauthn.go | 4 +- internal/gateway/settings/command_env.go | 20 +++ internal/gateway/settings/service_commands.go | 7 +- 16 files changed, 567 insertions(+), 117 deletions(-) create mode 100644 internal/cli/convox_process_env.go create mode 100644 internal/cli/convox_process_env_test.go create mode 100644 internal/cli/macdialog/macdialog.go diff --git a/internal/cli/convox_process_approval.go b/internal/cli/convox_process_approval.go index b0a1c7e4..7efadd1d 100644 --- a/internal/cli/convox_process_approval.go +++ b/internal/cli/convox_process_approval.go @@ -25,9 +25,9 @@ func addJustificationFlag(cmd *cobra.Command) { } // commandJustification returns --justification, checked, or "" without one. A command without one in an app that -// requires one is refused here, before anything starts: the gateway refuses it too, but the CLI can't show why the -// gateway refused an exec's WebSocket handshake. -func commandJustification(cmd *cobra.Command, app string) (string, error) { +// requires one (required) is refused here, before anything starts: the gateway refuses it too, but the CLI can't +// show why the gateway refused an exec's WebSocket handshake. +func commandJustification(cmd *cobra.Command, app string, required bool) (string, error) { text, err := cmd.Flags().GetString(justificationFlag) if err != nil { return "", err @@ -38,10 +38,6 @@ func commandJustification(cmd *cobra.Command, app string) (string, error) { } return strings.TrimSpace(text), nil } - required, err := appRequiresJustification(cmd, app) - if err != nil { - return "", err - } if required { return "", fmt.Errorf("app %s requires a justification for every command: add --justification, saying "+ "why the command is run and what it does (at least %d characters)", @@ -50,30 +46,58 @@ func commandJustification(cmd *cobra.Command, app string) (string, error) { return "", nil } -// appRequiresJustification reads require_command_justification: the gateway-wide setting, then the app's. Either -// one requires a justification. API tokens can't read settings; the gateway still refuses their commands without -// one (deploy approvals justify theirs with the approval). -func appRequiresJustification(cmd *cobra.Command, app string) (bool, error) { +// commandSettings is what the gateway's settings say about the commands run in an app: whether each needs a +// justification (the gateway-wide require_command_justification, or the app's own), and the env vars they get. +// API tokens can't read settings, so known is false; the gateway still checks their commands. +type commandSettings struct { + known bool + justificationRequired bool + env settings.CommandEnv +} + +// settingValues is a settings response: each setting's value. +type settingValues map[string]struct { + Value interface{} `json:"value"` +} + +// readCommandSettings reads the settings that apply to commands run in app. +func readCommandSettings(cmd *cobra.Command, app string) (commandSettings, error) { if apiTokenOverride() != "" { - return false, nil + return commandSettings{}, nil } rack, err := SelectedRack() if err != nil { - return false, err + return commandSettings{}, err } - for _, path := range []string{"/settings", "/apps/" + url.PathEscape(app) + "/settings"} { - var values map[string]struct { - Value interface{} `json:"value"` - } - if err := gatewayRequest(cmd, rack, http.MethodGet, path, nil, &values); err != nil { - return false, fmt.Errorf("failed to read whether app %s requires a justification: %w", + var global, values settingValues + for path, into := range map[string]*settingValues{ + "/settings": &global, "/apps/" + url.PathEscape(app) + "/settings": &values, + } { + if err := gatewayRequest(cmd, rack, http.MethodGet, path, nil, into); err != nil { + return commandSettings{}, fmt.Errorf("failed to read the settings of app %s's commands: %w", displayText(app), err) } - if required, _ := values[settings.KeyRequireCommandJustification].Value.(bool); required { - return true, nil - } } - return false, nil + required := func(values settingValues) bool { + on, _ := values[settings.KeyRequireCommandJustification].Value.(bool) + return on + } + env, err := settings.CommandEnvFromValues( + valueOr(values, settings.KeyCommandEnvVars, []interface{}{}), + valueOr(values, settings.KeyCommandEnvReadOnly, map[string]interface{}{}), + ) + if err != nil { + return commandSettings{}, fmt.Errorf("app %s: %w", displayText(app), err) + } + return commandSettings{known: true, justificationRequired: required(global) || required(values), env: env}, nil +} + +// valueOr returns the value of setting key, or empty when it's unset (null). +func valueOr(values settingValues, key string, empty interface{}) interface{} { + if value := values[key].Value; value != nil { + return value + } + return empty } // commandPrompt describes a command for the person approving it with MFA: where it runs, the command (a script's @@ -92,7 +116,14 @@ func commandPrompt(process processCommand) webauthn.Prompt { summary = append(summary, fmt.Sprintf("Script: %s, %d lines, SHA-256 %s", script.Name, script.Lines(), script.SHA256)) } - return webauthn.Prompt{Summary: summary, Touch: "run a command on " + process.where()} + if env := process.envSummary(); env != "" { + summary = append(summary, env) + } + touch := "run a command on " + if process.canChangeData() { + touch = "run a command that can change data on " + } + return webauthn.Prompt{Summary: summary, Warnings: process.envWarnings(), Touch: touch + process.where()} } // freshMFAAuth collects an MFA proof for the selected rack (one prompt or key touch, or --mfa-code), whatever the diff --git a/internal/cli/convox_process_command.go b/internal/cli/convox_process_command.go index 6e5a1224..9fc8937b 100644 --- a/internal/cli/convox_process_command.go +++ b/internal/cli/convox_process_command.go @@ -11,6 +11,7 @@ import ( "github.com/convox/stdcli" "github.com/spf13/cobra" + "github.com/DocSpring/rack-gateway/internal/cmdenv" "github.com/DocSpring/rack-gateway/internal/gateway/audit" "github.com/DocSpring/rack-gateway/internal/scriptrun" ) @@ -24,9 +25,11 @@ const dryRunFlag = "dry-run" // maxScriptFileBytes caps the script file read: the audit log keeps at most 50 KB of command and script anyway. const maxScriptFileBytes = 1 << 20 -// addProcessCommandFlags adds the flags exec and run share: --justification, --script and --dry-run. +// addProcessCommandFlags adds the flags exec and run share: --justification, --script, --dry-run and the env var +// flags (--env, --all-env-vars and --write). func addProcessCommandFlags(cmd *cobra.Command) { addJustificationFlag(cmd) + addEnvFlags(cmd) cmd.Flags().String(scriptFlag, "", "a local script file to run: the command gets the file's path as its last "+ "argument (e.g. rails runner), and you review the script before it runs. Needs --justification") cmd.Flags().Bool(dryRunFlag, false, "check the command and review its script, then show what would be sent, "+ @@ -43,6 +46,10 @@ type processCommand struct { justification string // script is the --script file, as the gateway will record it (nil without one). script *scriptrun.Script + // env is what the command asks for: env vars besides the app's, every env var, or write access. + env cmdenv.Request + // settings are the app's settings for commands. + settings commandSettings // dryRun stops the command before anything is sent (--dry-run). dryRun bool // pin is the security key PIN entered in the script review dialog, for the command's MFA proof ("" when the @@ -69,6 +76,9 @@ func (p processCommand) headers() map[string]string { if p.justification != "" { headers[audit.JustificationHeader] = audit.EncodeJustification(p.justification) } + for name, value := range p.env.Headers() { + headers[name] = value + } return headers } @@ -126,7 +136,11 @@ func (p processCommand) connect(cobraCmd *cobra.Command, args []string) (*proces // printDryRun says what the command would send: what the PIN dialog would show, and the command's size. func printDryRun(out io.Writer, process processCommand) error { - lines := append([]string{"Dry run: nothing was sent to the rack."}, commandPrompt(process).Summary...) + prompt := commandPrompt(process) + lines := append([]string{"Dry run: nothing was sent to the rack."}, prompt.Summary...) + for _, warning := range prompt.Warnings { + lines = append(lines, "WARNING: "+warning) + } size := fmt.Sprintf("The command sent would be %d bytes", len(process.command)) if process.script != nil { size += " (the script travels base64-encoded, with a sh runner that writes it to a temporary file)" @@ -150,10 +164,20 @@ func newProcessCommand(cobraCmd *cobra.Command, args []string, action string) (p if err := audit.CheckCommandLength(process.command); err != nil { return process, err } + if err := cmdenv.CheckCommand(process.command); err != nil { + return process, err + } + if process.env, err = envRequest(cobraCmd); err != nil { + return process, err + } if process.app, err = cobraCmd.Flags().GetString("app"); err != nil { return process, err } - if process.justification, err = commandJustification(cobraCmd, process.app); err != nil { + if process.settings, err = readCommandSettings(cobraCmd, process.app); err != nil { + return process, err + } + process.justification, err = commandJustification(cobraCmd, process.app, process.settings.justificationRequired) + if err != nil { return process, err } if process.script != nil && process.justification == "" { diff --git a/internal/cli/convox_process_command_test.go b/internal/cli/convox_process_command_test.go index 1dd000a5..23363620 100644 --- a/internal/cli/convox_process_command_test.go +++ b/internal/cli/convox_process_command_test.go @@ -235,6 +235,7 @@ func TestProcessCommandDryRun(t *testing.T) { "On app myapp (rack us), run in a new web process: rails runner count.rb", "Justification: " + testJustification, "Script: count.rb, 2 lines, SHA-256 " + script.SHA256, + "Env vars: every env var except RACK_URL", fmt.Sprintf("The command sent would be %d bytes (the script travels base64-encoded, with a sh runner that "+ "writes it to a temporary file).", len(scriptrun.Build("count.rb", []byte(script.Text), "rails runner"))), }, "\n")+"\n", out.String()) diff --git a/internal/cli/convox_process_env.go b/internal/cli/convox_process_env.go new file mode 100644 index 00000000..ec56a45e --- /dev/null +++ b/internal/cli/convox_process_env.go @@ -0,0 +1,108 @@ +package cli + +import ( + "slices" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" +) + +// The flags of exec and run that choose a command's env vars (see cmdenv.Request). An app's command_env_vars +// setting lists the env vars every command gets; without one, every command gets every env var except RACK_URL. +const ( + envFlag = "env" + allEnvVarsFlag = "all-env-vars" + writeFlag = "write" +) + +// addEnvFlags adds --env, --all-env-vars and --write to the exec or run command cmd. +func addEnvFlags(cmd *cobra.Command) { + cmd.Flags().StringSlice(envFlag, nil, "an env var the command needs besides the ones every command in the "+ + "app gets (repeat the flag, or separate names with commas)") + cmd.Flags().Bool(allEnvVarsFlag, false, "give the command every env var of the app (except RACK_URL)") + cmd.Flags().Bool(writeFlag, false, "give the command the real values of the app's read-only env vars (such "+ + "as a database URL), so it can change data") +} + +// envRequest returns what the command's flags ask for, checked. +func envRequest(cmd *cobra.Command) (cmdenv.Request, error) { + var req cmdenv.Request + var err error + if req.Env, err = cmd.Flags().GetStringSlice(envFlag); err != nil { + return req, err + } + for i, name := range req.Env { + req.Env[i] = strings.TrimSpace(name) + } + if req.AllEnvVars, err = cmd.Flags().GetBool(allEnvVarsFlag); err != nil { + return req, err + } + if req.Write, err = cmd.Flags().GetBool(writeFlag); err != nil { + return req, err + } + return req, req.Check() +} + +// envPlan returns the env vars the command gets, as the gateway decides them from the app's settings, and whether +// the settings could be read (API tokens can't read them). +func (p processCommand) envPlan() (cmdenv.Plan, bool) { + if !p.settings.known { + return cmdenv.Plan{}, false + } + plan, err := p.settings.env.Plan(p.env) + return plan, err == nil +} + +// envSummary describes the env vars the command gets, for the PIN dialog, or "" when the app's settings couldn't +// be read. +func (p processCommand) envSummary() string { + plan, ok := p.envPlan() + if !ok { + return "" + } + return "Env vars: " + plan.Describe() +} + +// writtenVars are the app's read-only env vars the command gets the real values of (--write), sorted. +func (p processCommand) writtenVars() []string { + plan, ok := p.envPlan() + if !ok || !p.env.Write { + return nil + } + var names []string + for name := range p.settings.env.ReadOnly { + if plan.All || slices.Contains(plan.Vars, name) { + names = append(names, name) + } + } + sort.Strings(names) + return names +} + +// canChangeData reports whether --write gives the command the real value of a read-only env var. +func (p processCommand) canChangeData() bool { + return len(p.writtenVars()) > 0 +} + +// envWarnings say what the person approving the command must notice about its env vars: it can change data +// (--write), it gets every env var where the app gives commands only some (--all-env-vars), or it's an exec, which +// can read the environment of the process it runs in, whatever env vars it gets itself. +func (p processCommand) envWarnings() []string { + var warnings []string + if written := p.writtenVars(); len(written) > 0 { + warnings = append(warnings, "This command can change data: --write gives it the real "+ + strings.Join(written, ", ")+", not the read-only one.") + } + if p.env.AllEnvVars && len(p.settings.env.Vars) > 0 { + warnings = append(warnings, "--all-env-vars gives this command every env var of the app (except "+ + cmdenv.RackURL+"), credentials included.") + } + if p.action == "exec" && p.settings.env.Limits() { + warnings = append(warnings, "exec runs inside a running process, so the command can read that process's "+ + "environment: every env var, with its real value.") + } + return warnings +} diff --git a/internal/cli/convox_process_env_test.go b/internal/cli/convox_process_env_test.go new file mode 100644 index 00000000..df95fcdd --- /dev/null +++ b/internal/cli/convox_process_env_test.go @@ -0,0 +1,125 @@ +package cli + +import ( + "bytes" + "testing" + + "github.com/convox/convox/pkg/structs" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/cmdenv" + "github.com/DocSpring/rack-gateway/internal/convox" + "github.com/DocSpring/rack-gateway/internal/gateway/settings" +) + +// limitedEnv is an app whose commands get PATH and DATABASE_URL, which is read-only. +const limitedEnv = `{"command_env_vars":{"value":["PATH","DATABASE_URL"]},` + + `"command_env_read_only":{"value":{"DATABASE_URL":"READ_ONLY_DATABASE_URL"}}}` + +// --env, --all-env-vars and --write go with each of the command's requests, as the headers the gateway reads. +func TestProcessCommandSendsWhatItAsksFor(t *testing.T) { + gateway := newEnvChangeGateway(t) + cmd := processCobraCommand(t, RunCommand(), map[string]string{envFlag: "STRIPE_KEY", writeFlag: "true"}) + require.NoError(t, cmd.Flags().Set(envFlag, " SENTRY_DSN,http_proxy")) + client, _, err := setupProcessCommand(t, cmd, []string{"web", "rake", "stripe:prepare"}, "run") + require.NoError(t, err) + placeholder := convox.AttachedRunPlaceholder + _, _ = client.ProcessRun("myapp", "web", structs.ProcessRunOptions{Command: &placeholder}) + + proxied := gateway.requests() + require.Len(t, proxied, 1) + require.Equal(t, "STRIPE_KEY,SENTRY_DSN,http_proxy", proxied[0].Get(cmdenv.EnvHeader)) + require.Equal(t, "true", proxied[0].Get(cmdenv.WriteHeader)) + require.Empty(t, proxied[0].Get(cmdenv.AllEnvVarsHeader)) +} + +// A command the gateway would refuse is refused before anything is sent: env vars it can't have, a command it +// can't wrap. +func TestProcessCommandRefusesWhatTheGatewayWould(t *testing.T) { + for want, c := range map[string]struct { + flags map[string]string + args []string + }{ + "no command can have RACK_URL": {map[string]string{envFlag: "RACK_URL"}, []string{"web", "env"}}, + "isn't an env var name": {map[string]string{envFlag: "A-B"}, []string{"web", "env"}}, + "can't name some with --env too": { + map[string]string{envFlag: "A", allEnvVarsFlag: "true"}, []string{"web", "env"}, + }, + "can't start with NAME=value": {nil, []string{"web", "DATABASE_URL=postgres://x", "rake"}}, + "the command is empty": {nil, []string{"web"}}, + } { + gateway := newEnvChangeGateway(t) + for _, cmd := range []*cobra.Command{ExecCommand(), RunCommand()} { + cmd = processCobraCommand(t, cmd, c.flags) + _, _, err := setupProcessCommand(t, cmd, c.args, cmd.Name()) + require.ErrorContains(t, err, want) + } + require.Empty(t, gateway.requests(), want) + } +} + +// limitedProcess is a command in an app whose commands get PATH and DATABASE_URL, which is read-only. +func limitedProcess(action string, req cmdenv.Request) processCommand { + return processCommand{ + action: action, app: "myapp", rack: "us", target: "web", command: "rake db:migrate", env: req, + settings: commandSettings{known: true, env: settings.CommandEnv{ + Vars: []string{"PATH", "DATABASE_URL"}, + ReadOnly: map[string]string{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}, + }}, + } +} + +// The PIN dialog says which env vars the command gets, and warns in red when it can change data (--write), gets +// every env var where the app gives commands only some (--all-env-vars), or is an exec, which can read its process's +// environment. The touch says when it can change data. +func TestCommandPromptDescribesTheCommandsEnvVars(t *testing.T) { + prompt := commandPrompt(limitedProcess("run", cmdenv.Request{Env: []string{"STRIPE_KEY"}})) + require.Contains(t, prompt.Summary, "Env vars: DATABASE_URL (from READ_ONLY_DATABASE_URL), PATH, STRIPE_KEY") + require.Empty(t, prompt.Warnings) + require.Equal(t, "run a command on app myapp (rack us)", prompt.Touch) + + prompt = commandPrompt(limitedProcess("run", cmdenv.Request{Write: true})) + require.Contains(t, prompt.Summary, "Env vars: DATABASE_URL, PATH") + require.Equal(t, []string{ + "This command can change data: --write gives it the real DATABASE_URL, not the read-only one.", + }, prompt.Warnings) + require.Equal(t, "run a command that can change data on app myapp (rack us)", prompt.Touch) + + prompt = commandPrompt(limitedProcess("run", cmdenv.Request{AllEnvVars: true})) + require.Contains(t, prompt.Summary, "Env vars: every env var except RACK_URL; read-only DATABASE_URL "+ + "(from READ_ONLY_DATABASE_URL)") + require.Len(t, prompt.Warnings, 1) + require.Contains(t, prompt.Warnings[0], "--all-env-vars gives this command every env var of the app") + + prompt = commandPrompt(limitedProcess("exec", cmdenv.Request{})) + require.Len(t, prompt.Warnings, 1) + require.Contains(t, prompt.Warnings[0], "exec runs inside a running process") + + // An app that keeps nothing from its commands has nothing to warn about: --write gives no command more. + unlimited := limitedProcess("exec", cmdenv.Request{Write: true, AllEnvVars: true}) + unlimited.settings.env = settings.CommandEnv{} + prompt = commandPrompt(unlimited) + require.Contains(t, prompt.Summary, "Env vars: every env var except RACK_URL") + require.Empty(t, prompt.Warnings) + + // An API token can't read settings, and has no PIN dialog. + token := limitedProcess("run", cmdenv.Request{Write: true}) + token.settings = commandSettings{} + prompt = commandPrompt(token) + require.NotContains(t, prompt.Summary, "Env vars") + require.Empty(t, prompt.Warnings) +} + +// The CLI reads the app's command settings from the gateway, and a dry run shows the env vars and the warnings. +func TestProcessCommandDryRunShowsTheEnvVars(t *testing.T) { + gateway := newEnvChangeGateway(t) + gateway.appSettings = limitedEnv + cmd := processCobraCommand(t, RunCommand(), map[string]string{writeFlag: "true", dryRunFlag: "true"}) + var out bytes.Buffer + cmd.SetOut(&out) + require.NoError(t, runProcessCommand(cmd, []string{"web", "rake", "db:migrate"}, "run", nil)) + require.Contains(t, out.String(), "Env vars: DATABASE_URL, PATH") + require.Contains(t, out.String(), "WARNING: This command can change data: --write gives it the real DATABASE_URL") + require.Empty(t, gateway.requests()) +} diff --git a/internal/cli/convox_process_review.go b/internal/cli/convox_process_review.go index 3d2bdc67..59637d4b 100644 --- a/internal/cli/convox_process_review.go +++ b/internal/cli/convox_process_review.go @@ -27,6 +27,7 @@ var reviewScriptWith = func(process processCommand) (string, error) { } return scriptreview.Review(scriptreview.Request{ Where: process.where(), Justification: process.justification, Script: *process.script, CheckPIN: checkPIN, + Warnings: process.envWarnings(), }) } diff --git a/internal/cli/convox_processes.go b/internal/cli/convox_processes.go index 4b3de58b..a7220454 100644 --- a/internal/cli/convox_processes.go +++ b/internal/cli/convox_processes.go @@ -127,7 +127,6 @@ func RunCommand() *cobra.Command { cmd.Flags().BoolP("detach", "d", false, "run in detached mode") cmd.Flags().String("entrypoint", "", "override entrypoint") cmd.Flags().String("release", "", "release to run") - cmd.Flags().StringSlice("env", []string{}, "environment variables") addProcessCommandFlags(cmd) return cmd diff --git a/internal/cli/macdialog/macdialog.go b/internal/cli/macdialog/macdialog.go new file mode 100644 index 00000000..732e3a23 --- /dev/null +++ b/internal/cli/macdialog/macdialog.go @@ -0,0 +1,76 @@ +// Package macdialog runs the CLI's macOS dialogs: JavaScript for Automation (JXA) scripts run with osascript. A +// script gets its texts as arguments, never inside the script, so nothing shown can change what the script does. +package macdialog + +import ( + "bytes" + "context" + "os/exec" +) + +// Runner runs a JXA script with osascript, passing args to its run handler, and returns what it printed. +type Runner func(ctx context.Context, script string, args ...string) (stdout, stderr string, err error) + +// Run runs script with osascript (a Runner). +func Run(ctx context.Context, script string, args ...string) (string, string, error) { + var stdout, stderr bytes.Buffer + cmd := exec.CommandContext(ctx, "osascript", append([]string{"-l", "JavaScript", "-e", script}, args...)...) + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + return stdout.String(), stderr.String(), err +} + +// Helpers is JXA the dialog scripts include (after ObjC.import('Cocoa')): +// +// - alertIcon(warning): the alert's 48-point icon, a lock.shield symbol, or a red exclamation mark triangle when +// warning is set, instead of osascript's folder icon. +// - warningsView(text, width): a label showing text (the warnings, one per line) in bold red, wrapped to width +// points, with its height fitted to the text; null when text is empty. +// - stacked(views): a view holding views from the bottom up, 12 points apart, skipping nulls (an alert's +// accessory view). +const Helpers = ` +function alertIcon(warning) { + var config = $.NSImageSymbolConfiguration.configurationWithPointSizeWeightScale(48, 0, 3); + var name = 'lock.shield'; + if (warning) { + name = 'exclamationmark.triangle.fill'; + config = config.configurationByApplyingConfiguration( + $.NSImageSymbolConfiguration.configurationWithHierarchicalColor($.NSColor.systemRedColor)); + } + return $.NSImage.imageWithSystemSymbolNameAccessibilityDescription(name, 'rack-gateway') + .imageWithSymbolConfiguration(config); +} + +function warningsView(text, width) { + if (!text) { + return null; + } + var label = $.NSTextField.wrappingLabelWithString(text); + label.textColor = $.NSColor.systemRedColor; + label.font = $.NSFont.boldSystemFontOfSize(14); + label.preferredMaxLayoutWidth = width; + label.setFrame($.NSMakeRect(0, 0, width, label.fittingSize.height)); + return label; +} + +function stacked(views) { + var container = $.NSView.alloc.initWithFrame($.NSMakeRect(0, 0, 0, 0)); + var width = 0; + var y = 0; + views.forEach(function (view) { + if (!view) { + return; + } + if (y > 0) { + y += 12; + } + view.setFrameOrigin($.NSMakePoint(0, y)); + container.addSubview(view); + y += view.frame.size.height; + width = Math.max(width, view.frame.size.width); + }); + container.setFrameSize($.NSMakeSize(width, y)); + return container; +} +` diff --git a/internal/cli/pinentry/pinentry.go b/internal/cli/pinentry/pinentry.go index 1eee60dc..93913a15 100644 --- a/internal/cli/pinentry/pinentry.go +++ b/internal/cli/pinentry/pinentry.go @@ -7,13 +7,11 @@ package pinentry import ( - "bytes" "context" "errors" "fmt" "io" "os" - "os/exec" "runtime" "strconv" "strings" @@ -22,6 +20,7 @@ import ( "golang.org/x/term" + "github.com/DocSpring/rack-gateway/internal/cli/macdialog" "github.com/DocSpring/rack-gateway/internal/cli/terminal" ) @@ -34,21 +33,37 @@ const ( maxLineLength = 160 // maxSummaryLines keeps a long summary readable; the lines past it are counted instead of shown. maxSummaryLines = 12 - // appleScriptUserCanceled is the AppleScript error number for a canceled dialog. - appleScriptUserCanceled = "(-128)" ) -// dialogScript shows the message passed as the first argument (never interpolated into the script) -// and prints the entered PIN to stdout. `activate` brings osascript to the front first: when the CLI -// runs in the background (e.g. from an agent) the dialog otherwise opens without keyboard focus. -const dialogScript = `on run argv - activate - set reply to display dialog (item 1 of argv) with title "rack-gateway" default answer "" ¬ - with hidden answer buttons {"Cancel", "Approve"} default button "Approve" cancel button "Cancel" ¬ - with icon caution giving up after 170 - if gave up of reply then error "PIN dialog timed out" number -128 - return text returned of reply -end run` +// dialogScript shows an alert with the message (its first argument) and a security key PIN field, which has the +// focus, and prints "approve", a line break and the PIN when Approve is clicked (or Return pressed). The warnings +// (its second argument, one per line) show in red above the field, with a red warning icon. `activate` brings the +// dialog to the front: when the CLI runs in the background (from an agent, say) it would otherwise open without +// keyboard focus. +const dialogScript = ` +ObjC.import('Cocoa'); +function run(argv) { + var app = $.NSApplication.sharedApplication; + app.setActivationPolicy($.NSApplicationActivationPolicyAccessory); + app.activateIgnoringOtherApps(true); + + var field = $.NSSecureTextField.alloc.initWithFrame($.NSMakeRect(0, 0, 240, 24)); + var warnings = warningsView(argv[1], 460); + + var alert = $.NSAlert.alloc.init; + alert.icon = alertIcon(!!warnings); + alert.messageText = 'Approve this rack-gateway command?'; + alert.informativeText = argv[0]; + alert.accessoryView = stacked([field, warnings]); + alert.addButtonWithTitle('Approve'); + alert.addButtonWithTitle('Cancel'); + alert.window.initialFirstResponder = field; + if (alert.runModal != $.NSAlertFirstButtonReturn) { + return 'cancel'; + } + return 'approve\n' + field.stringValue.js; +} +` + macdialog.Helpers // ErrCanceled is returned when the user cancels the PIN dialog or lets it time out. var ErrCanceled = errors.New("PIN entry canceled") @@ -57,9 +72,6 @@ var ErrCanceled = errors.New("PIN entry canceled") // The values of other flags (--app, --rack, --commit, ...) are shown, so the user can see what they approve. var secretFlagWords = []string{"token", "secret", "password", "passwd", "passphrase", "key", "credential", "code"} -// dialogRunner runs the AppleScript with the message as its only argument. -type dialogRunner func(ctx context.Context, script, message string) (stdout, stderr string, err error) - // Request describes what the PIN will authorize. It is shown in the dialog. type Request struct { // Gateway is the gateway origin, e.g. https://gateway-us.example.com. @@ -69,21 +81,26 @@ type Request struct { // Summary optionally describes what the command approves, one item per line (for example each deploy // approval it makes). The dialog lists it above the command line. Summary []string + // Warnings say what the command can do that the person approving it must notice. The dialog shows them in red. + Warnings []string } // ReadPIN asks for the security key PIN on the terminal, or in a macOS dialog when stdin is not a terminal. func ReadPIN(req Request) (string, error) { if fd, ok := terminal.StdinFD(); ok { - return readFromTerminal(fd, os.Stderr) + return readFromTerminal(fd, os.Stderr, req.Warnings) } if runtime.GOOS != "darwin" { return "", errors.New("security key PIN required but stdin is not a terminal; run the command in a terminal") } _, _ = fmt.Fprintln(os.Stderr, "Waiting for the security key PIN in the rack-gateway dialog...") - return readFromDialog(context.Background(), runOsascript, req) + return readFromDialog(context.Background(), macdialog.Run, req) } -func readFromTerminal(fd int, prompt io.Writer) (string, error) { +func readFromTerminal(fd int, prompt io.Writer, warnings []string) (string, error) { + for _, warning := range warnings { + _, _ = fmt.Fprintln(prompt, "WARNING: "+SingleLine(warning, maxLineLength)) + } _, _ = fmt.Fprint(prompt, "Enter your security key PIN: ") pinBytes, err := term.ReadPassword(fd) _, _ = fmt.Fprintln(prompt) @@ -94,32 +111,34 @@ func readFromTerminal(fd int, prompt io.Writer) (string, error) { } // readFromDialog shows the PIN dialog and returns the entered PIN. -func readFromDialog(ctx context.Context, run dialogRunner, req Request) (string, error) { +func readFromDialog(ctx context.Context, run macdialog.Runner, req Request) (string, error) { ctx, cancel := context.WithTimeout(ctx, dialogTimeout) defer cancel() - stdout, stderr, err := run(ctx, dialogScript, dialogMessage(req)) + stdout, stderr, err := run(ctx, dialogScript, dialogMessage(req), dialogWarnings(req.Warnings)) + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return "", ErrCanceled + } if err != nil { - if strings.Contains(stderr, appleScriptUserCanceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) { - return "", ErrCanceled - } return "", fmt.Errorf("failed to show PIN dialog: %w: %s", err, strings.TrimSpace(stderr)) } - - pin := strings.TrimSuffix(stdout, "\n") + answer, pin, _ := strings.Cut(strings.TrimSuffix(stdout, "\n"), "\n") + if answer != "approve" { + return "", ErrCanceled + } if pin == "" { return "", errors.New("no PIN entered") } return pin, nil } -func runOsascript(ctx context.Context, script, message string) (string, string, error) { - var stdout, stderr bytes.Buffer - cmd := exec.CommandContext(ctx, "osascript", "-e", script, message) - cmd.Stdout = &stdout - cmd.Stderr = &stderr - err := cmd.Run() - return stdout.String(), stderr.String(), err +// dialogWarnings renders the warnings for the dialog, one per line, each made safe for one line (see SingleLine). +func dialogWarnings(warnings []string) string { + lines := make([]string, 0, len(warnings)) + for _, warning := range warnings { + lines = append(lines, "⚠ "+SingleLine(warning, maxLineLength)) + } + return strings.Join(lines, "\n") } // dialogMessage renders the dialog text. Everything that comes from outside the dialog (the summary, the diff --git a/internal/cli/pinentry/pinentry_test.go b/internal/cli/pinentry/pinentry_test.go index c3a585cf..311e8f04 100644 --- a/internal/cli/pinentry/pinentry_test.go +++ b/internal/cli/pinentry/pinentry_test.go @@ -7,12 +7,14 @@ import ( "strings" "testing" "unicode/utf8" + + "github.com/DocSpring/rack-gateway/internal/cli/macdialog" ) -// fakeDialog returns a dialogRunner that records the message it was shown and replies with the given output. -func fakeDialog(shown *string, stdout, stderr string, err error) dialogRunner { - return func(_ context.Context, _, message string) (string, string, error) { - *shown = message +// fakeDialog returns a dialog runner that records the message it was shown and replies with the given output. +func fakeDialog(shown *string, stdout, stderr string, err error) macdialog.Runner { + return func(_ context.Context, _ string, args ...string) (string, string, error) { + *shown = args[0] return stdout, stderr, err } } @@ -20,7 +22,7 @@ func fakeDialog(shown *string, stdout, stderr string, err error) dialogRunner { func TestReadFromDialogReturnsEnteredPIN(t *testing.T) { var shown string - pin, err := readFromDialog(context.Background(), fakeDialog(&shown, "12 34\n", "", nil), Request{}) + pin, err := readFromDialog(context.Background(), fakeDialog(&shown, "approve\n12 34\n", "", nil), Request{}) if err != nil { t.Fatalf("readFromDialog: %v", err) } @@ -36,7 +38,7 @@ func TestReadFromDialogShowsGatewayAndMaskedCommand(t *testing.T) { Args: []string{"/usr/local/bin/rack-gateway", "env", "set", "SECRET=hunter2", "-r", "us"}, } - if _, err := readFromDialog(context.Background(), fakeDialog(&shown, "1234\n", "", nil), req); err != nil { + if _, err := readFromDialog(context.Background(), fakeDialog(&shown, "approve\n1234\n", "", nil), req); err != nil { t.Fatalf("readFromDialog: %v", err) } @@ -52,7 +54,7 @@ func TestReadFromDialogShowsGatewayAndMaskedCommand(t *testing.T) { func TestReadFromDialogCanceled(t *testing.T) { var shown string - run := fakeDialog(&shown, "", "execution error: User canceled. (-128)", errors.New("exit status 1")) + run := fakeDialog(&shown, "cancel\n", "", nil) _, err := readFromDialog(context.Background(), run, Request{}) if !errors.Is(err, ErrCanceled) { @@ -76,11 +78,28 @@ func TestReadFromDialogFailure(t *testing.T) { func TestReadFromDialogEmptyPIN(t *testing.T) { var shown string - if _, err := readFromDialog(context.Background(), fakeDialog(&shown, "\n", "", nil), Request{}); err == nil { + run := fakeDialog(&shown, "approve\n\n", "", nil) + if _, err := readFromDialog(context.Background(), run, Request{}); err == nil { t.Fatal("expected an error for an empty PIN") } } +// The warnings reach the dialog as its second argument, one per line, each kept to one line. +func TestReadFromDialogShowsTheWarnings(t *testing.T) { + var warnings string + run := func(_ context.Context, _ string, args ...string) (string, string, error) { + warnings = args[1] + return "approve\n1234\n", "", nil + } + req := Request{Warnings: []string{"This command can change data.", "exec reads\nevery env var."}} + if _, err := readFromDialog(context.Background(), run, req); err != nil { + t.Fatalf("readFromDialog: %v", err) + } + if want := "⚠ This command can change data.\n⚠ exec reads?every env var."; warnings != want { + t.Fatalf("warnings = %q, want %q", warnings, want) + } +} + func TestDescribeCommandMasksCredentialFlags(t *testing.T) { args := []string{"rg", "deploy-approval", "approve", "--api-token", "tok_123", "--mfa-code", "999999"} @@ -281,8 +300,8 @@ func TestDescribeSummaryCountsLinesPastTheLimit(t *testing.T) { } func TestDialogScriptActivatesBeforeShowingDialog(t *testing.T) { - activate := strings.Index(dialogScript, "\tactivate\n") - dialog := strings.Index(dialogScript, "display dialog") + activate := strings.Index(dialogScript, "app.activateIgnoringOtherApps(true);") + dialog := strings.Index(dialogScript, "alert.runModal") if activate < 0 || activate > dialog { t.Fatalf("dialog script must activate osascript before the dialog so the PIN field has focus:\n%s", dialogScript) diff --git a/internal/cli/scriptreview/scriptreview.go b/internal/cli/scriptreview/scriptreview.go index 514c8310..e72fef35 100644 --- a/internal/cli/scriptreview/scriptreview.go +++ b/internal/cli/scriptreview/scriptreview.go @@ -6,18 +6,17 @@ package scriptreview import ( "bufio" - "bytes" "context" "encoding/json" "errors" "fmt" "io" "os" - "os/exec" "runtime" "strings" "time" + "github.com/DocSpring/rack-gateway/internal/cli/macdialog" "github.com/DocSpring/rack-gateway/internal/cli/terminal" "github.com/DocSpring/rack-gateway/internal/scriptrun" "github.com/DocSpring/rack-gateway/internal/util/textcheck" @@ -34,6 +33,9 @@ type Request struct { // CheckPIN, when set, has the dialog ask for the security key PIN as well, and checks each PIN entered. A // terminal ignores it: the PIN is asked for there when the command runs. CheckPIN PINCheck + // Warnings say what the command can do that the person approving it must notice (change data, read every env + // var). They show in red. + Warnings []string } // PINCheck checks a PIN entered in the review dialog. again is a reason to ask for the PIN again (a wrong PIN); @@ -59,13 +61,20 @@ func Review(req Request) (string, error) { "the command in a terminal") } _, _ = fmt.Fprintln(os.Stderr, "Waiting for the script to be approved in the rack-gateway dialog...") - return reviewInDialog(context.Background(), runOsascript, req) + return reviewInDialog(context.Background(), macdialog.Run, req) } // reviewInTerminal prints the script's listing to out, in color when color is set, and approves it when the // answer read from in is y or yes. func reviewInTerminal(req Request, in io.Reader, out io.Writer, color bool) error { var text strings.Builder + for _, warning := range req.Warnings { + line := "WARNING: " + oneLine(warning) + if color { + line = "\x1b[1;31m" + line + "\x1b[0m" // bold red + } + text.WriteString(line + "\n") + } text.WriteString(Header(req) + "\n\n") spans, _ := listing(req.Script) if color { @@ -119,11 +128,8 @@ func oneLine(text string) string { }, text) } -// dialogRunner runs a JavaScript for Automation script with osascript, passing args to its run handler. -type dialogRunner func(ctx context.Context, script string, args ...string) (stdout, stderr string, err error) - // pinInstructions tell the person what the PIN in the review dialog is for. -const pinInstructions = "Enter your security key PIN to run it. You'll touch the key once the command starts." +const pinInstructions = "Enter your security key PIN to run it, then touch the key." // enterPIN asks again for a PIN when Run was clicked without one. const enterPIN = "Enter your security key PIN to run the script." @@ -131,7 +137,7 @@ const enterPIN = "Enter your security key PIN to run the script." // reviewInDialog shows the script in a macOS dialog and approves it when the person clicks Run, after entering a // PIN that req.CheckPIN accepts when it's set. It returns that PIN. A dialog left open for reviewTimeout (all its // tries together) is closed, and the script isn't approved. -func reviewInDialog(ctx context.Context, run dialogRunner, req Request) (string, error) { +func reviewInDialog(ctx context.Context, run macdialog.Runner, req Request) (string, error) { ctx, cancel := context.WithTimeout(ctx, reviewTimeout) defer cancel() spans, _ := listing(req.Script) @@ -139,7 +145,7 @@ func reviewInDialog(ctx context.Context, run dialogRunner, req Request) (string, if err != nil { return "", fmt.Errorf("failed to show the script review dialog: %w", err) } - dialog := reviewDialog{run: run, header: Header(req), listing: string(listingJSON)} + dialog := reviewDialog{run: run, header: Header(req), listing: string(listingJSON), warnings: warningLines(req)} if req.CheckPIN == nil { _, err := dialog.show(ctx, "") return "", err @@ -164,17 +170,26 @@ func reviewInDialog(ctx context.Context, run dialogRunner, req Request) (string, } } -// reviewDialog is the review dialog's arguments: the header, the listing (its spans, as JSON) and the mode ("pin" -// to ask for the security key PIN too, or ""). +// reviewDialog is the review dialog's arguments: the header, the listing (its spans, as JSON), the mode ("pin" to +// ask for the security key PIN too, or "") and the warnings (one per line). type reviewDialog struct { - run dialogRunner - header, listing, mode string + run macdialog.Runner + header, listing, mode, warnings string +} + +// warningLines are req's warnings for the dialog, one per line, each made safe for one line (see oneLine). +func warningLines(req Request) string { + lines := make([]string, 0, len(req.Warnings)) + for _, warning := range req.Warnings { + lines = append(lines, "⚠ "+oneLine(warning)) + } + return strings.Join(lines, "\n") } // show shows the dialog, with problem (why it's shown again, or "") as its title. It returns the PIN entered once // Run is clicked, or ErrDeclined. func (d reviewDialog) show(ctx context.Context, problem string) (string, error) { - stdout, stderr, err := d.run(ctx, dialogScript, d.header, d.listing, d.mode, problem) + stdout, stderr, err := d.run(ctx, dialogScript, d.header, d.listing, d.mode, problem, d.warnings) if errors.Is(ctx.Err(), context.DeadlineExceeded) { return "", fmt.Errorf("%w: the review dialog timed out", ErrDeclined) } @@ -188,21 +203,13 @@ func (d reviewDialog) show(ctx context.Context, problem string) (string, error) return pin, nil } -func runOsascript(ctx context.Context, script string, args ...string) (string, string, error) { - var stdout, stderr bytes.Buffer - cmd := exec.CommandContext(ctx, "osascript", append([]string{"-l", "JavaScript", "-e", script}, args...)...) - cmd.Stdout = &stdout - cmd.Stderr = &stderr - err := cmd.Run() - return stdout.String(), stderr.String(), err -} - // dialogScript shows an alert, with a lock icon and the header as its text, and the listing (its spans, as JSON) in // a scrollable, read-only, monospaced text view, colored by style in the system's colors for the current appearance // (light or dark). It prints "run" when Run is clicked. Cancel is the default button (Return), so approving takes a // click. In "pin" mode it has a security key PIN field under the listing, which has the focus, and Run is the // default button: the PIN is the approval. It then prints "run", a line break and the PIN. A problem (a wrong PIN) -// replaces the title. The texts arrive as arguments, never as part of the script. +// replaces the title. The warnings (one per line) show in red above the listing, with a red warning icon. The +// texts arrive as arguments, never as part of the script. const dialogScript = ` ObjC.import('Cocoa'); function run(argv) { @@ -225,29 +232,26 @@ function run(argv) { view.textStorage.setAttributedString(listing(JSON.parse(argv[1]))); scroll.documentView = view; + var warnings = warningsView(argv[4], 820); var alert = $.NSAlert.alloc.init; - // A 48-point lock.shield symbol, regular weight (0), large scale (3), instead of osascript's folder icon. - alert.icon = $.NSImage.imageWithSystemSymbolNameAccessibilityDescription('lock.shield', 'rack-gateway') - .imageWithSymbolConfiguration($.NSImageSymbolConfiguration.configurationWithPointSizeWeightScale(48, 0, 3)); + alert.icon = alertIcon(!!warnings); alert.messageText = argv[3] || 'Approve this rack-gateway script?'; alert.informativeText = argv[0]; if (argv[2] !== 'pin') { - alert.accessoryView = scroll; + alert.accessoryView = stacked([scroll, warnings]); alert.addButtonWithTitle('Cancel'); alert.addButtonWithTitle('Run'); alert.window.initialFirstResponder = view; return alert.runModal == $.NSAlertSecondButtonReturn ? 'run' : 'cancel'; } - var content = $.NSView.alloc.initWithFrame($.NSMakeRect(0, 0, 820, 500)); - scroll.setFrameOrigin($.NSMakePoint(0, 40)); - content.addSubview(scroll); + var pinRow = $.NSView.alloc.initWithFrame($.NSMakeRect(0, 0, 820, 30)); var label = $.NSTextField.labelWithString('Security key PIN:'); label.setFrame($.NSMakeRect(0, 9, 120, 18)); - content.addSubview(label); + pinRow.addSubview(label); var field = $.NSSecureTextField.alloc.initWithFrame($.NSMakeRect(124, 6, 220, 24)); - content.addSubview(field); - alert.accessoryView = content; + pinRow.addSubview(field); + alert.accessoryView = stacked([pinRow, scroll, warnings]); alert.addButtonWithTitle('Run'); alert.addButtonWithTitle('Cancel'); alert.window.initialFirstResponder = field; @@ -287,4 +291,4 @@ function listing(spans) { }); return text; } -` +` + macdialog.Helpers diff --git a/internal/cli/scriptreview/scriptreview_test.go b/internal/cli/scriptreview/scriptreview_test.go index bf5479cf..ed78d5f6 100644 --- a/internal/cli/scriptreview/scriptreview_test.go +++ b/internal/cli/scriptreview/scriptreview_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/DocSpring/rack-gateway/internal/cli/macdialog" "github.com/DocSpring/rack-gateway/internal/scriptrun" ) @@ -48,6 +49,26 @@ func TestReviewInTerminal(t *testing.T) { } } +// The warnings show first, in the terminal (in red with color) and in the dialog (its last argument, one per line, +// each kept to one line). +func TestReviewShowsTheWarnings(t *testing.T) { + req := testRequest(t, "puts 1\n") + req.Warnings = []string{"This command can change data.", "exec reads\nevery env var."} + var out bytes.Buffer + require.NoError(t, reviewInTerminal(req, strings.NewReader("y\n"), &out, true)) + require.True(t, strings.HasPrefix(out.String(), "\x1b[1;31mWARNING: This command can change data.\x1b[0m\n"+ + "\x1b[1;31mWARNING: exec reads?every env var.\x1b[0m\nReview this script"), out.String()) + + var warnings string + run := func(_ context.Context, _ string, args ...string) (string, string, error) { + warnings = args[4] + return "run\n", "", nil + } + _, err := reviewInDialog(context.Background(), run, req) + require.NoError(t, err) + require.Equal(t, "⚠ This command can change data.\n⚠ exec reads?every env var.", warnings) +} + // Characters that would make the script read differently from what runs (a right-to-left override, a zero-width // space, a lone carriage return) are written out and counted, with a warning. CRLF line endings aren't. func TestListingShowsHiddenCharacters(t *testing.T) { @@ -88,7 +109,7 @@ func TestReviewInDialog(t *testing.T) { } require.Empty(t, pin) require.Equal(t, dialogScript, gotScript) - require.Equal(t, []string{Header(req), gotArgs[1], "", ""}, gotArgs) + require.Equal(t, []string{Header(req), gotArgs[1], "", "", ""}, gotArgs) var spans []span require.NoError(t, json.Unmarshal([]byte(gotArgs[1]), &spans)) require.Equal(t, "1 | puts 'run'", plainText(spans)) @@ -102,11 +123,11 @@ func TestReviewInDialog(t *testing.T) { } // answers is a dialog that gives each of its answers in turn, and records the problem each was asked with. -func answers(t *testing.T, stdouts ...string) (dialogRunner, *[]string) { +func answers(t *testing.T, stdouts ...string) (macdialog.Runner, *[]string) { t.Helper() var problems []string return func(_ context.Context, _ string, args ...string) (string, string, error) { - require.Len(t, args, 4) + require.Len(t, args, 5) require.Equal(t, "pin", args[2]) require.Contains(t, args[0], pinInstructions) problems = append(problems, args[3]) diff --git a/internal/cli/webauthn/types.go b/internal/cli/webauthn/types.go index de8068f3..0ee96f18 100644 --- a/internal/cli/webauthn/types.go +++ b/internal/cli/webauthn/types.go @@ -25,6 +25,9 @@ type AssertionOptions struct { type Prompt struct { // Summary lists what the command approves, one item per line, for the PIN dialog. Summary []string + // Warnings say what the command can do that a person approving it must notice (change data, read every env + // var). The PIN dialog shows them in red. + Warnings []string // Touch says what this touch is for, e.g. "approve api on staging (2 of 4)". Without it the touch is to // "authenticate to" the gateway. Touch string diff --git a/internal/cli/webauthn/webauthn.go b/internal/cli/webauthn/webauthn.go index ae52aa13..f4a7fc43 100644 --- a/internal/cli/webauthn/webauthn.go +++ b/internal/cli/webauthn/webauthn.go @@ -48,7 +48,9 @@ func requestAssertion(options AssertionOptions, cachedPIN string) (*AssertionRes return nil, "", err } - pinRequest := pinentry.Request{Gateway: options.Origin, Args: os.Args, Summary: options.Prompt.Summary} + pinRequest := pinentry.Request{ + Gateway: options.Origin, Args: os.Args, Summary: options.Prompt.Summary, Warnings: options.Prompt.Warnings, + } flow := assertionFlow{ key: chooseKey(keys, options.RPID, allowList), readPIN: func() (string, error) { return pinentry.ReadPIN(pinRequest) }, diff --git a/internal/gateway/settings/command_env.go b/internal/gateway/settings/command_env.go index ccea5dee..16df104e 100644 --- a/internal/gateway/settings/command_env.go +++ b/internal/gateway/settings/command_env.go @@ -53,6 +53,26 @@ func (e CommandEnv) Plan(req cmdenv.Request) (cmdenv.Plan, error) { return cmdenv.New(req.AllEnvVars || len(e.Vars) == 0, vars, readOnly) } +// CommandEnvFromValues reads command_env_vars and command_env_read_only from their values, as the settings API +// returns them (JSON-decoded). A value of the wrong shape is an error (ErrInvalidSettingValue). +func CommandEnvFromValues(vars, readOnly interface{}) (CommandEnv, error) { + var env CommandEnv + var err error + if env.Vars, err = commandEnvVarNames(vars); err != nil { + return CommandEnv{}, err + } + if env.ReadOnly, err = readOnlyEnvMap(readOnly); err != nil { + return CommandEnv{}, err + } + return env, nil +} + +// Limits reports whether the app keeps anything from its commands: env vars they don't get, or the real values of +// read-only ones. +func (e CommandEnv) Limits() bool { + return len(e.Vars) > 0 || len(e.ReadOnly) > 0 +} + // commandEnvVarNames returns command_env_vars's value as env var names, or ErrInvalidSettingValue when it isn't a // list of env var names (surrounding spaces aside), or it names RACK_URL. Names keep their case: a process's env // var names are case-sensitive. diff --git a/internal/gateway/settings/service_commands.go b/internal/gateway/settings/service_commands.go index 20a3c9f5..c612dcd3 100644 --- a/internal/gateway/settings/service_commands.go +++ b/internal/gateway/settings/service_commands.go @@ -47,11 +47,8 @@ func (s *Service) GetCommandEnv(appName string) (CommandEnv, error) { if err != nil { return CommandEnv{}, err } - var env CommandEnv - if env.Vars, err = commandEnvVarNames(vars.Value); err != nil { - return CommandEnv{}, fmt.Errorf("app %s: %w", appName, err) - } - if env.ReadOnly, err = readOnlyEnvMap(readOnly.Value); err != nil { + env, err := CommandEnvFromValues(vars.Value, readOnly.Value) + if err != nil { return CommandEnv{}, fmt.Errorf("app %s: %w", appName, err) } return env, nil From 60926269eb41218b75c093a35fa4204dfb00c9ad Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 13:38:09 +1300 Subject: [PATCH 05/12] refactor(cmdenv): check env var names itself, so the mock rack builds without the gateway cmdenv used envutil.ValidName, which pulled the gateway's config and database packages into everything that imports cmdenv, and broke the mock rack's image (it copies only the packages it needs). ValidName moves to cmdenv, and envutil and the settings checks call it there. --- Dockerfile.mock-convox | 1 + internal/cmdenv/cmdenv.go | 11 ++++++++--- internal/gateway/envutil/envutil.go | 12 ++---------- internal/gateway/settings/validate.go | 4 ++-- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/Dockerfile.mock-convox b/Dockerfile.mock-convox index bca253eb..fd6ec073 100644 --- a/Dockerfile.mock-convox +++ b/Dockerfile.mock-convox @@ -16,6 +16,7 @@ RUN --mount=type=cache,target=/go/pkg/mod,sharing=locked \ # Copy only the source needed for mock-convox COPY cmd/mock-convox ./cmd/mock-convox +COPY internal/cmdenv ./internal/cmdenv COPY internal/logging ./internal/logging # Build mock binary with caches diff --git a/internal/cmdenv/cmdenv.go b/internal/cmdenv/cmdenv.go index dcdc9c68..fb47402d 100644 --- a/internal/cmdenv/cmdenv.go +++ b/internal/cmdenv/cmdenv.go @@ -24,8 +24,6 @@ import ( "strings" "github.com/kballard/go-shellquote" - - "github.com/DocSpring/rack-gateway/internal/gateway/envutil" ) // RackURL is the env var Convox gives every process of every app: the rack API's URL, with the rack's API password @@ -110,7 +108,7 @@ func CheckReadOnly(readOnly map[string]string) error { } func checkName(name string) error { - if !envutil.ValidName(name) { + if !ValidName(name) { return fmt.Errorf("%q isn't an env var name (letters, digits and underscores, not starting with a digit)", name) } @@ -181,10 +179,17 @@ const ( ) var ( + validName = regexp.MustCompile(`^` + namePattern + `$`) assignment = regexp.MustCompile(`^\$\{(` + namePattern + `)\+"(` + namePattern + `)=\$(` + namePattern + `)"\} `) unsetLine = regexp.MustCompile(`^unset ` + RackURL + `((?: ` + namePattern + `)*); exec env -- `) ) +// ValidName reports whether name is a valid env var name: letters, digits and underscores, not starting with a +// digit. +func ValidName(name string) bool { + return validName.MatchString(name) +} + // Parse reads a command Build made back into its plan and the command it runs. ok is false for anything else. func Parse(wrapped string) (plan Plan, command string, ok bool) { rest, found := strings.CutPrefix(wrapped, scriptPrefix) diff --git a/internal/gateway/envutil/envutil.go b/internal/gateway/envutil/envutil.go index 495e932d..a1051c4a 100644 --- a/internal/gateway/envutil/envutil.go +++ b/internal/gateway/envutil/envutil.go @@ -9,13 +9,13 @@ import ( "fmt" "net/http" "net/url" - "regexp" "sort" "strings" "time" "unicode" "unicode/utf8" + "github.com/DocSpring/rack-gateway/internal/cmdenv" "github.com/DocSpring/rack-gateway/internal/gateway/config" "github.com/DocSpring/rack-gateway/internal/gateway/httpclient" ) @@ -43,14 +43,6 @@ var ( ErrInvalidEnvEntry = errors.New("invalid environment variable name or value") ) -var envKeyPattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) - -// ValidName reports whether name is a valid env var name: letters, digits and underscores, not starting with a -// digit. -func ValidName(name string) bool { - return envKeyPattern.MatchString(name) -} - // maxShownKeyBytes is how much of a key an error message shows: the message goes back to the client and into the // logs, and a key can be most of a posted env. const maxShownKeyBytes = 40 @@ -59,7 +51,7 @@ const maxShownKeyBytes = 40 // than tab. An invalid name isn't shown in the error (it may be a line of a multi-line secret value); a value error // names its key, cut to 40 bytes. func ValidateEnvEntry(key, value string) error { - if !ValidName(key) { + if !cmdenv.ValidName(key) { return fmt.Errorf("%w: names must be letters, digits and underscores, not starting with a digit", ErrInvalidEnvEntry) } diff --git a/internal/gateway/settings/validate.go b/internal/gateway/settings/validate.go index 130c2f12..b0c5dc37 100644 --- a/internal/gateway/settings/validate.go +++ b/internal/gateway/settings/validate.go @@ -10,7 +10,7 @@ import ( "strconv" "strings" - "github.com/DocSpring/rack-gateway/internal/gateway/envutil" + "github.com/DocSpring/rack-gateway/internal/cmdenv" ) // ErrInvalidSettingValue means a setting value has the wrong shape for its key. @@ -187,7 +187,7 @@ func envVarNameList(key string, value interface{}) ([]string, error) { func invalidEnvVarNames(names []string) string { var invalid []string for _, name := range names { - if !envutil.ValidName(strings.TrimSpace(name)) { + if !cmdenv.ValidName(strings.TrimSpace(name)) { invalid = append(invalid, strconv.Quote(name)) } } From 46cfdf2065beba2fd2e1ee0ced8ac3e7d804d542 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 13:38:23 +1300 Subject: [PATCH 06/12] test(cli-e2e): check the env vars run and exec give a command The mock rack prints the env vars the gateway's wrapper gives each command. The stages check the default (every env var except RACK_URL), an app's command_env_vars and command_env_read_only with --env and --write, and the refusal of --env RACK_URL. --- scripts/lib/cli-e2e/stages.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/scripts/lib/cli-e2e/stages.sh b/scripts/lib/cli-e2e/stages.sh index f5a01a15..46e21b27 100644 --- a/scripts/lib/cli-e2e/stages.sh +++ b/scripts/lib/cli-e2e/stages.sh @@ -1,5 +1,30 @@ # shellcheck shell=bash +# An app's command_env_vars and command_env_read_only decide the env vars its commands get, with --env and --write. +# The mock rack prints the env vars the gateway's wrapper gives the command. +run_command_env_tests() { + psql_exec "INSERT INTO settings (app_name, key, value, updated_at) VALUES ('rack-gateway', 'command_env_vars', '[\"PATH\", \"DATABASE_URL\"]'::jsonb, NOW()) ON CONFLICT (COALESCE(app_name, ''), key) DO UPDATE SET value = EXCLUDED.value, updated_at = NOW();" + psql_exec "INSERT INTO settings (app_name, key, value, updated_at) VALUES ('rack-gateway', 'command_env_read_only', '{\"DATABASE_URL\": \"READ_ONLY_DATABASE_URL\"}'::jsonb, NOW()) ON CONFLICT (COALESCE(app_name, ''), key) DO UPDATE SET value = EXCLUDED.value, updated_at = NOW();" + + local mfa_code + clear_mfa_replay_protection + mfa_code=$(generate_totp_code "$(get_totp_secret 'admin@example.com')") + verify_rgw_command "run web --mfa-code $mfa_code --env SENTRY_DSN 'echo rake db:migrate'" \ + '$ echo rake db:migrate' \ + 'env: DATABASE_URL (from READ_ONLY_DATABASE_URL), PATH, SENTRY_DSN' + + clear_mfa_replay_protection + mfa_code=$(generate_totp_code "$(get_totp_secret 'admin@example.com')") + verify_rgw_command "exec p-worker-1 --mfa-code $mfa_code --write 'echo rake db:migrate'" \ + '$ echo rake db:migrate' \ + 'env: DATABASE_URL, PATH' + + verify_rgw_command_failure "run web --env RACK_URL 'echo rake db:migrate'" \ + "no command can have RACK_URL" + + psql_exec "DELETE FROM settings WHERE app_name = 'rack-gateway' AND key IN ('command_env_vars', 'command_env_read_only');" +} + run_admin_tests() { if [[ -n "$SKIP_ADMIN_TESTS" ]]; then return @@ -26,6 +51,7 @@ run_admin_tests() { verify_rgw_command "run web --mfa-code $run_mfa_code 'echo rake db:migrate'" \ 'Connected to mock exec for app=rack-gateway pid=proc-123456' \ '$ echo rake db:migrate' \ + 'env: every env var except RACK_URL' \ 'rake db:migrate' clear_mfa_replay_protection @@ -35,6 +61,8 @@ run_admin_tests() { '$ echo rake db:migrate' \ 'rake db:migrate' + run_command_env_tests + # A --script is reviewed before anything is sent. Without a terminal there's nowhere to show it (macOS would # open the review dialog instead, so the check only runs elsewhere). if [[ "$(uname)" != "Darwin" ]]; then From 8024844040e167bb54365af9980e5c42209ca587 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 13:38:24 +1300 Subject: [PATCH 07/12] chore(gateway): delete unused settings request types Nothing used UpdateProtectedEnvVarsRequest, UpdateApprovedCommandsRequest, UpdateAppImagePatternsRequest, UpdateAllowDestructiveActionsRequest or UpdateMFASettingsRequest, and the OpenAPI spec no longer had them. Their generated web schemas were left over from an older spec. --- internal/gateway/handlers/dto.go | 25 ------------------- ...ersUpdateAllowDestructiveActionsRequest.ts | 11 -------- .../handlersUpdateAppImagePatternsRequest.ts | 12 --------- .../handlersUpdateApprovedCommandsRequest.ts | 11 -------- .../handlersUpdateMFASettingsRequest.ts | 11 -------- .../handlersUpdateProtectedEnvVarsRequest.ts | 11 -------- web/src/api/schemas/index.ts | 5 ---- 7 files changed, 86 deletions(-) delete mode 100644 web/src/api/schemas/handlersUpdateAllowDestructiveActionsRequest.ts delete mode 100644 web/src/api/schemas/handlersUpdateAppImagePatternsRequest.ts delete mode 100644 web/src/api/schemas/handlersUpdateApprovedCommandsRequest.ts delete mode 100644 web/src/api/schemas/handlersUpdateMFASettingsRequest.ts delete mode 100644 web/src/api/schemas/handlersUpdateProtectedEnvVarsRequest.ts diff --git a/internal/gateway/handlers/dto.go b/internal/gateway/handlers/dto.go index 75fac692..436bc7a0 100644 --- a/internal/gateway/handlers/dto.go +++ b/internal/gateway/handlers/dto.go @@ -375,31 +375,6 @@ type RevokeAllSessionsResponse struct { RevokedCount int64 `json:"revoked_count" validate:"required"` } -// UpdateProtectedEnvVarsRequest defines the payload for updating protected environment variables. -type UpdateProtectedEnvVarsRequest struct { - ProtectedEnvVars []string `json:"protected_env_vars"` -} - -// UpdateApprovedCommandsRequest defines the payload for updating approved commands for CI/CD exec. -type UpdateApprovedCommandsRequest struct { - ApprovedCommands []string `json:"approved_commands"` -} - -// UpdateAppImagePatternsRequest defines the payload for updating app image tag patterns. -type UpdateAppImagePatternsRequest struct { - AppImagePatterns map[string]string `json:"app_image_patterns"` -} - -// UpdateAllowDestructiveActionsRequest defines the payload for toggling destructive actions. -type UpdateAllowDestructiveActionsRequest struct { - AllowDestructiveActions bool `json:"allow_destructive_actions"` -} - -// UpdateMFASettingsRequest defines the payload for updating MFA enforcement defaults. -type UpdateMFASettingsRequest struct { - RequireAllUsers bool `json:"require_all_users"` -} - // UpdatePreferredMFAMethodRequest defines the payload for updating user's preferred MFA method. type UpdatePreferredMFAMethodRequest struct { PreferredMethod *string `json:"preferred_method"` // "totp", "webauthn", or null to clear diff --git a/web/src/api/schemas/handlersUpdateAllowDestructiveActionsRequest.ts b/web/src/api/schemas/handlersUpdateAllowDestructiveActionsRequest.ts deleted file mode 100644 index 7dcde831..00000000 --- a/web/src/api/schemas/handlersUpdateAllowDestructiveActionsRequest.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * Generated by orval v7.13.0 🍺 - * Do not edit manually. - * Rack Gateway API - * API for the Rack Gateway administration and proxy services. - * OpenAPI spec version: 1.0 - */ - -export interface HandlersUpdateAllowDestructiveActionsRequest { - allow_destructive_actions?: boolean; -} diff --git a/web/src/api/schemas/handlersUpdateAppImagePatternsRequest.ts b/web/src/api/schemas/handlersUpdateAppImagePatternsRequest.ts deleted file mode 100644 index 5e4da69a..00000000 --- a/web/src/api/schemas/handlersUpdateAppImagePatternsRequest.ts +++ /dev/null @@ -1,12 +0,0 @@ -/** - * Generated by orval v7.13.0 🍺 - * Do not edit manually. - * Rack Gateway API - * API for the Rack Gateway administration and proxy services. - * OpenAPI spec version: 1.0 - */ -import type { HandlersUpdateAppImagePatternsRequestAppImagePatterns } from "./handlersUpdateAppImagePatternsRequestAppImagePatterns"; - -export interface HandlersUpdateAppImagePatternsRequest { - app_image_patterns?: HandlersUpdateAppImagePatternsRequestAppImagePatterns; -} diff --git a/web/src/api/schemas/handlersUpdateApprovedCommandsRequest.ts b/web/src/api/schemas/handlersUpdateApprovedCommandsRequest.ts deleted file mode 100644 index 755a2c5b..00000000 --- a/web/src/api/schemas/handlersUpdateApprovedCommandsRequest.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * Generated by orval v7.13.0 🍺 - * Do not edit manually. - * Rack Gateway API - * API for the Rack Gateway administration and proxy services. - * OpenAPI spec version: 1.0 - */ - -export interface HandlersUpdateApprovedCommandsRequest { - approved_commands?: string[]; -} diff --git a/web/src/api/schemas/handlersUpdateMFASettingsRequest.ts b/web/src/api/schemas/handlersUpdateMFASettingsRequest.ts deleted file mode 100644 index f46c8ecd..00000000 --- a/web/src/api/schemas/handlersUpdateMFASettingsRequest.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * Generated by orval v7.13.0 🍺 - * Do not edit manually. - * Rack Gateway API - * API for the Rack Gateway administration and proxy services. - * OpenAPI spec version: 1.0 - */ - -export interface HandlersUpdateMFASettingsRequest { - require_all_users?: boolean; -} diff --git a/web/src/api/schemas/handlersUpdateProtectedEnvVarsRequest.ts b/web/src/api/schemas/handlersUpdateProtectedEnvVarsRequest.ts deleted file mode 100644 index 1c49fb2a..00000000 --- a/web/src/api/schemas/handlersUpdateProtectedEnvVarsRequest.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** - * Generated by orval v7.13.0 🍺 - * Do not edit manually. - * Rack Gateway API - * API for the Rack Gateway administration and proxy services. - * OpenAPI spec version: 1.0 - */ - -export interface HandlersUpdateProtectedEnvVarsRequest { - protected_env_vars?: string[]; -} diff --git a/web/src/api/schemas/index.ts b/web/src/api/schemas/index.ts index d80ae994..5b241aba 100644 --- a/web/src/api/schemas/index.ts +++ b/web/src/api/schemas/index.ts @@ -106,20 +106,15 @@ export * from "./handlersStartYubiOTPEnrollmentResponse"; export * from "./handlersStatusResponse"; export * from "./handlersTokenPermissionMetadata"; export * from "./handlersTrustedDeviceResponse"; -export * from "./handlersUpdateAllowDestructiveActionsRequest"; export * from "./handlersUpdateAPITokenRequest"; -export * from "./handlersUpdateAppImagePatternsRequest"; export * from "./handlersUpdateAppImagePatternsRequestAppImagePatterns"; -export * from "./handlersUpdateApprovedCommandsRequest"; export * from "./handlersUpdateDeployApprovalRequestStatusRequest"; export * from "./handlersUpdateEnvValuesRequest"; export * from "./handlersUpdateEnvValuesRequestSet"; export * from "./handlersUpdateEnvValuesResponse"; export * from "./handlersUpdateEnvValuesResponseEnv"; export * from "./handlersUpdateMFAMethodRequest"; -export * from "./handlersUpdateMFASettingsRequest"; export * from "./handlersUpdatePreferredMFAMethodRequest"; -export * from "./handlersUpdateProtectedEnvVarsRequest"; export * from "./handlersUpdateUserNameRequest"; export * from "./handlersUpdateUserProfileRequest"; export * from "./handlersUpdateUserRequest"; From 79f184a6e7f2cb776ca75664f3372fee2b54e027 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 13:38:39 +1300 Subject: [PATCH 08/12] feat(web): edit approved deploy commands and the command env var settings, and show a command's env vars Approved deploy commands are objects now, so the card edits them as JSON, with the card the service image patterns used (now JsonSettingCard). New cards edit command_env_vars (a list) and command_env_read_only (a JSON object). The audit log shows the env vars each command got, each read-only one with its source, and says when a command ran with --write. --- web/e2e/db.ts | 2 +- .../audit-logs/command-details.test.tsx | 48 +++++++++++++++ .../components/audit-logs/command-details.tsx | 45 +++++++++++++- web/src/pages/app-settings-page.tsx | 59 +++++++++++++++++-- ...rd.test.tsx => json-setting-card.test.tsx} | 48 +++++++++++---- ...atterns-card.tsx => json-setting-card.tsx} | 47 +++++++++------ 6 files changed, 211 insertions(+), 38 deletions(-) rename web/src/pages/app-settings/{service-image-patterns-card.test.tsx => json-setting-card.test.tsx} (52%) rename web/src/pages/app-settings/{service-image-patterns-card.tsx => json-setting-card.tsx} (77%) diff --git a/web/e2e/db.ts b/web/e2e/db.ts index 56d4bca6..5c717a26 100644 --- a/web/e2e/db.ts +++ b/web/e2e/db.ts @@ -415,7 +415,7 @@ export async function clearAllGlobalSettings() { await client.query( `DELETE FROM settings WHERE app_name IS NULL - AND key NOT IN ('mfa', 'approved_commands', 'service_image_patterns')` + AND key NOT IN ('mfa', 'service_image_patterns')` ) }) } diff --git a/web/src/components/audit-logs/command-details.test.tsx b/web/src/components/audit-logs/command-details.test.tsx index e40a8a26..d9557706 100644 --- a/web/src/components/audit-logs/command-details.test.tsx +++ b/web/src/components/audit-logs/command-details.test.tsx @@ -41,6 +41,54 @@ describe('CommandDetails', () => { } }) + // The env vars the command got, with each read-only one's source, and a warning when it could change data. + it('shows the env vars the command got', () => { + const readOnly = { DATABASE_URL: 'READ_ONLY_DATABASE_URL' } + const { rerender } = render( + + ) + expect(screen.getByTestId('audit-command-env')).toHaveTextContent( + 'Env vars: DATABASE_URL (from READ_ONLY_DATABASE_URL), PATH' + ) + expect(screen.queryByTestId('audit-command-write')).toBeNull() + + rerender( + + ) + expect(screen.getByTestId('audit-command-env')).toHaveTextContent( + 'Env vars: every env var except RACK_URL' + ) + expect(screen.getByTestId('audit-command-write')).toHaveTextContent('Can change data') + + rerender( + + ) + expect(screen.getByTestId('audit-command-env')).toHaveTextContent( + 'Env vars: every env var except RACK_URL; read-only DATABASE_URL (from READ_ONLY_DATABASE_URL)' + ) + + rerender() + expect(screen.getByTestId('audit-command-env')).toHaveTextContent('Env vars: no env vars') + + // An entry from before the gateway recorded env vars says nothing about them. + rerender() + expect(screen.queryByTestId('audit-command-env')).toBeNull() + }) + // Characters that would make the script read differently from what ran are written out and counted, with a // warning. CRLF line endings aren't. it('writes out hidden characters', () => { diff --git a/web/src/components/audit-logs/command-details.tsx b/web/src/components/audit-logs/command-details.tsx index 4d0d6936..8885f71e 100644 --- a/web/src/components/audit-logs/command-details.tsx +++ b/web/src/components/audit-logs/command-details.tsx @@ -7,6 +7,8 @@ interface CommandAuditDetails { script?: string scriptName?: string scriptSHA256?: string + env?: string + write: boolean } function stringField(record: Record, key: string): string | undefined { @@ -14,6 +16,34 @@ function stringField(record: Record, key: string): string | und return typeof value === 'string' && value !== '' ? value : undefined } +function stringMap(value: unknown): Record { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return {} + } + return Object.fromEntries( + Object.entries(value).filter((entry): entry is [string, string] => typeof entry[1] === 'string') + ) +} + +// envDescription describes the env vars the command got, as the CLI's PIN dialog does: every env var except +// RACK_URL, or the names it got, each read-only one with the env var it took its value from. Entries from before +// the gateway recorded env vars have none. +function envDescription(record: Record): string | undefined { + const readOnly = stringMap(record.env_read_only) + const described = (vars: string[]) => + vars.map((name) => (readOnly[name] ? `${name} (from ${readOnly[name]})` : name)).join(', ') + if (record.all_env_vars === true) { + const names = Object.keys(readOnly).sort() + const all = 'every env var except RACK_URL' + return names.length > 0 ? `${all}; read-only ${described(names)}` : all + } + if (!Array.isArray(record.env_vars)) { + return + } + const names = record.env_vars.filter((name): name is string => typeof name === 'string') + return names.length > 0 ? described(names) : 'no env vars' +} + // parseCommandDetails reads the command fields from an audit entry's details JSON, or returns null when it has no // command. function parseCommandDetails(details: string | null | undefined): CommandAuditDetails | null { @@ -37,6 +67,8 @@ function parseCommandDetails(details: string | null | undefined): CommandAuditDe script: stringField(record, 'script'), scriptName: stringField(record, 'script_name'), scriptSHA256: stringField(record, 'script_sha256'), + env: envDescription(record), + write: record.write === true, } } @@ -92,7 +124,8 @@ function ScriptListing({ details }: { details: CommandAuditDetails }) { } // CommandDetails shows the command an exec or process start ran, from its audit details: its justification, the -// command, and a script's text with numbered lines (the details JSON would show it as one escaped string). +// command, the env vars it got, and a script's text with numbered lines (the details JSON would show it as one +// escaped string). export function CommandDetails({ details }: { details: string | null | undefined }) { const command = useMemo(() => parseCommandDetails(details), [details]) if (!command) { @@ -109,6 +142,16 @@ export function CommandDetails({ details }: { details: string | null | undefined Command:{' '} {command.command} + {command.env && ( +
+ Env vars: {command.env} +
+ )} + {command.write && ( +
+ Can change data: run with --write, so it got the real values of the read-only env vars. +
+ )} {command.script && } ) diff --git a/web/src/pages/app-settings-page.tsx b/web/src/pages/app-settings-page.tsx index faaa1300..5ed38975 100644 --- a/web/src/pages/app-settings-page.tsx +++ b/web/src/pages/app-settings-page.tsx @@ -6,7 +6,7 @@ import { useAuth } from '@/contexts/auth-context' import { api } from '@/lib/api' import { apiPath } from '@/lib/api-path' import { CommandJustificationCard } from '@/pages/app-settings/command-justification-card' -import { ServiceImagePatternsCard } from '@/pages/app-settings/service-image-patterns-card' +import { JsonSettingCard } from '@/pages/app-settings/json-setting-card' import { StringArrayCard } from '@/pages/app-settings/string-array-card' import type { AppSettingsResponse } from '@/pages/app-settings/types' import { VCSCIProvidersCard } from '@/pages/app-settings/vcs-ci-providers-card' @@ -97,17 +97,66 @@ export function AppSettingsPage() { />
- + The exact commands a CI/CD token can run during an approved deploy request, each + with the env vars it gets: "env" lists the ones it needs besides the + app's command env vars, "all_env_vars" gives it every env var, and + "write" gives it the real values of the read-only env vars. An empty list + allows none. + + } disabled={!isAdmin} + label="JSON list of commands" pathSegment="approved-deploy-commands" - placeholder="e.g. bundle exec rake db:migrate" + placeholder='[{"command": "bundle exec rake db:migrate", "write": true}]' settingKey="approved_deploy_commands" settings={appSettings} title="Approved Deploy Commands" /> - + + Per-service regex patterns for the Docker images in convox.yml, matched against the + whole image reference. Required for builds made under a deploy approval: every + service needs a pattern (its own or "*") that names the image repository.{' '} + {'{{GIT_COMMIT}}'} is replaced with the approved commit. + + } + disabled={!isAdmin} + label="JSON object (service name → regex)" + pathSegment="service-image-patterns" + placeholder='{"*": "docker\\.io/myorg/app:{{GIT_COMMIT}}-amd64"}' + settingKey="service_image_patterns" + settings={appSettings} + title="Service Image Patterns" + /> +
+
+ +
vi.fn()) @@ -16,42 +16,64 @@ vi.mock('@/components/ui/use-toast', () => ({ toast: { error: toastError, success: vi.fn() }, })) -function renderCard() { +function renderCard(settings: AppSettingsResponse = {}) { const client = new QueryClient({ defaultOptions: { mutations: { retry: false } } }) return render( - + ) } function save(json: string) { - fireEvent.change(screen.getByLabelText(/JSON object/), { target: { value: json } }) + fireEvent.change(screen.getByLabelText('JSON list of commands'), { target: { value: json } }) fireEvent.click(screen.getByRole('button', { name: 'Save' })) } -describe('ServiceImagePatternsCard', () => { +describe('JsonSettingCard', () => { beforeEach(() => { mockPut.mockReset() toastError.mockReset() }) + it('shows the current value as indented JSON', () => { + renderCard({ + approved_deploy_commands: { + value: [{ command: 'rake db:migrate', write: true }], + source: 'db', + }, + } as AppSettingsResponse) + expect(screen.getByLabelText('JSON list of commands')).toHaveValue( + JSON.stringify([{ command: 'rake db:migrate', write: true }], null, 2) + ) + }) + it('reports invalid JSON without calling the API', async () => { renderCard() - save('{not json') + save('[{not json') await waitFor(() => expect(toastError).toHaveBeenCalledWith('Invalid JSON format')) expect(mockPut).not.toHaveBeenCalled() }) // Only the parse is about the JSON: a failed request must show the server's error, not "Invalid JSON format". - it("shows the API's error when saving valid JSON fails", async () => { - mockPut.mockRejectedValue(new Error('admin role required')) + it("saves the parsed value to the setting's path, and shows the API's error when that fails", async () => { + mockPut.mockRejectedValue(new Error('unknown field "comand"')) renderCard() - save('{"*": "docker\\\\.io/acme/app:{{GIT_COMMIT}}"}') - await waitFor(() => expect(toastError).toHaveBeenCalledWith('admin role required')) - expect(mockPut).toHaveBeenCalledWith('/api/v1/apps/web/settings/service-image-patterns', { - '*': 'docker\\.io/acme/app:{{GIT_COMMIT}}', - }) + save('[{"command": "rake db:migrate", "env": ["STRIPE_KEY"]}]') + await waitFor(() => expect(toastError).toHaveBeenCalledWith('unknown field "comand"')) + expect(mockPut).toHaveBeenCalledWith('/api/v1/apps/web/settings/approved-deploy-commands', [ + { command: 'rake db:migrate', env: ['STRIPE_KEY'] }, + ]) expect(toastError).not.toHaveBeenCalledWith('Invalid JSON format') }) }) diff --git a/web/src/pages/app-settings/service-image-patterns-card.tsx b/web/src/pages/app-settings/json-setting-card.tsx similarity index 77% rename from web/src/pages/app-settings/service-image-patterns-card.tsx rename to web/src/pages/app-settings/json-setting-card.tsx index 49469ecc..e25b1705 100644 --- a/web/src/pages/app-settings/service-image-patterns-card.tsx +++ b/web/src/pages/app-settings/json-setting-card.tsx @@ -1,5 +1,5 @@ import { useQueryClient } from '@tanstack/react-query' -import { useState } from 'react' +import { type ReactNode, useState } from 'react' import { getSettingValue, @@ -18,22 +18,37 @@ import { apiPath } from '@/lib/api-path' import type { AppSettingsResponse } from '@/pages/app-settings/types' import { extractErrorMessage } from '@/pages/app-settings/utils' -interface ServiceImagePatternsCardProps { +interface JsonSettingCardProps { app: string settings: AppSettingsResponse | undefined disabled: boolean + settingKey: string + pathSegment: string + title: string + description: ReactNode + // label names the value's shape, e.g. "JSON object (service name → regex)". + label: string + placeholder: string } -export function ServiceImagePatternsCard({ +// JsonSettingCard edits an app setting whose value is JSON (an object, or a list of objects) in a text area. The +// gateway checks the value's shape and refuses one it can't use. +export function JsonSettingCard({ app, settings, disabled, -}: ServiceImagePatternsCardProps) { + settingKey, + pathSegment, + title, + description, + label, + placeholder, +}: JsonSettingCardProps) { const qc = useQueryClient() const [value, setValue] = useState(null) - const setting = settings?.service_image_patterns - const currentValue = getSettingValue | null>(setting, null) + const setting = settings?.[settingKey] + const currentValue = getSettingValue(setting, null) let displayValue = '' if (value !== null) { @@ -55,7 +70,7 @@ export function ServiceImagePatternsCard({ } catch (error) { throw new Error('Invalid JSON format', { cause: error }) } - await api.put(apiPath`/api/v1/apps/${app}/settings/service-image-patterns`, parsed) + await api.put(apiPath`/api/v1/apps/${app}/settings/${pathSegment}`, parsed) }, onSuccess: async () => { // Refetch before dropping local edits, so the card never shows the old cached value @@ -71,7 +86,7 @@ export function ServiceImagePatternsCard({ const clearMutation = useMutation({ mutationFn: async () => { - await api.delete(apiPath`/api/v1/apps/${app}/settings/service-image-patterns`) + await api.delete(apiPath`/api/v1/apps/${app}/settings/${pathSegment}`) }, onSuccess: async () => { // Refetch before dropping local edits, so the card never shows the old cached value @@ -98,28 +113,24 @@ export function ServiceImagePatternsCard({ } const hasDbSetting = hasStoredValue(setting) + const inputId = `setting-${pathSegment}` return ( - Service Image Patterns + {title} -

- Per-service regex patterns for the Docker images in convox.yml, matched against the whole - image reference. Required for builds made under a deploy approval: every service needs a - pattern (its own or "*") that names the image repository. {'{{GIT_COMMIT}}'} is - replaced with the approved commit. -

+

{description}

- +