diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index ac6045975..ebd9fb8f8 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -254,6 +254,20 @@ at: confirm it now REQUIRES the authentication prompt to return data instead of returning it silently — the regression test for the finding this goal exists to close. + - **The real sudo askpass / Touch ID escalation prompt** (goal 0240 + S5, `wrapArgvForAdmin`/`materializeAskpass`) — sudo's PAM + conversation (pam_tid's Touch ID sheet, or the osascript + hidden-answer password dialog) is out-of-process system UI no + headless harness can trigger or dismiss; unit tests pin the argv + wrapping, askpass content/mode, and the secrets-refusal, and the + guardrail tests pin the always-asks policy — never the real + prompt. Verify on an installed build: configure the seeded "Run + from clipboard" shell step with "Run with admin rights", run + `whoami`, approve the forced ask, confirm the Touch ID sheet + appears (pam_tid configured) and the output reads `root`; cancel + the prompt on a second run and confirm the step fails with sudo's + own error rather than hanging; then confirm an allow-listed + command STILL parked for approval while the toggle was on. - **The real browser-tab approval notification** (goal 0132 slice A) — requires a real granted browser permission and a real OS compositor; verify via a server-mode instance reached from a real browser tab: diff --git a/internal/adapters/procexec/procexec.go b/internal/adapters/procexec/procexec.go index de4e91845..e9862bedc 100644 --- a/internal/adapters/procexec/procexec.go +++ b/internal/adapters/procexec/procexec.go @@ -27,6 +27,7 @@ import ( "errors" "io" "os/exec" + "strings" "sync" "time" ) @@ -116,6 +117,14 @@ type Spec struct { // which case output is discarded but LastOutputAt still advances // (idle-timeout tracking doesn't require a caller-supplied sink). Output io.Writer + + // Stdin, when non-empty, is written to the child's standard input + // (then closed, so a reader sees EOF after it). Empty keeps + // exec.Cmd's own default: the child reads from the null device. + // A string, not a Reader, deliberately: every caller's input is an + // already-materialized payload, and a bounded value can never hold + // the child open waiting on a slow producer. + Stdin string } // Result is one Start call's terminal outcome. @@ -184,6 +193,9 @@ func Start(spec Spec) (*Handle, error) { fw := newFanWriter(spec.Output) cmd.Stdout = fw cmd.Stderr = fw + if spec.Stdin != "" { + cmd.Stdin = strings.NewReader(spec.Stdin) + } if err := cmd.Start(); err != nil { return nil, err diff --git a/internal/adapters/procexec/procexec_test.go b/internal/adapters/procexec/procexec_test.go index eae97ead3..a1fa93e33 100644 --- a/internal/adapters/procexec/procexec_test.go +++ b/internal/adapters/procexec/procexec_test.go @@ -286,3 +286,46 @@ func TestCancel_EscalatesToSIGKILL_WhenSIGTERMIsTrapped(t *testing.T) { } waitForGroupGone(t, h.PGID()) } + +// TestStart_StdinPipedToChild pins Spec.Stdin's contract (goal 0240 +// S5): a non-empty Stdin reaches the child verbatim and is followed by +// EOF, so a stdin-reading command terminates on its own. +func TestStart_StdinPipedToChild(t *testing.T) { + var out bytes.Buffer + h, err := Start(Spec{ + Argv: []string{"cat"}, + Stdin: "line one\nline two\n", + Output: &out, + }) + if err != nil { + t.Fatalf("Start() error: %v", err) + } + result := h.Wait() + if result.Outcome != OutcomeExited || result.ExitCode != 0 { + t.Fatalf("Outcome/ExitCode = %q/%d, want exited/0", result.Outcome, result.ExitCode) + } + if got := out.String(); got != "line one\nline two\n" { + t.Errorf("output = %q, want the stdin content verbatim", got) + } +} + +// TestStart_EmptyStdin_ChildSeesEOF pins the zero value's behavior: +// no Stdin means the child reads the null device (immediate EOF), the +// pre-S5 behavior unchanged. +func TestStart_EmptyStdin_ChildSeesEOF(t *testing.T) { + var out bytes.Buffer + h, err := Start(Spec{ + Argv: []string{"cat"}, + Output: &out, + }) + if err != nil { + t.Fatalf("Start() error: %v", err) + } + result := h.Wait() + if result.Outcome != OutcomeExited || result.ExitCode != 0 { + t.Fatalf("Outcome/ExitCode = %q/%d, want exited/0 (cat must see EOF, not hang)", result.Outcome, result.ExitCode) + } + if got := out.String(); got != "" { + t.Errorf("output = %q, want empty", got) + } +} diff --git a/internal/contract/contract.json b/internal/contract/contract.json index c17ff3c73..83ff8c5f8 100644 --- a/internal/contract/contract.json +++ b/internal/contract/contract.json @@ -1862,6 +1862,23 @@ "Multiline": true, "SystemManaged": false }, + { + "Key": "passInput", + "Label": "Pass input", + "Type": "options", + "Required": false, + "Default": "stdin", + "Description": "How a literal script receives the upstream payload: piped to stdin, or one argument per line ($1, $2, …).", + "Options": [ + "stdin", + "arguments" + ], + "Suggestions": null, + "Secret": false, + "RefKind": "", + "Multiline": false, + "SystemManaged": false + }, { "Key": "timeoutSeconds", "Label": "Timeout (seconds)", @@ -2654,6 +2671,20 @@ "OptionalRef": true, "Multiline": false, "SystemManaged": false + }, + { + "Key": "runWithAdmin", + "Label": "Run with admin rights", + "Type": "boolean", + "Required": false, + "Default": "false", + "Description": "Runs each command with administrator rights. macOS asks you to approve every run — Touch ID when it's set up for sudo, your password otherwise.", + "Options": null, + "Suggestions": null, + "Secret": false, + "RefKind": "", + "Multiline": false, + "SystemManaged": false } ], "Output": "combined stdout+stderr from every sub-command that ran", @@ -4075,6 +4106,23 @@ "Multiline": true, "SystemManaged": false }, + { + "Key": "passInput", + "Label": "Pass input", + "Type": "options", + "Required": false, + "Default": "stdin", + "Description": "How a literal script receives the upstream payload: piped to stdin, or one argument per line ($1, $2, …).", + "Options": [ + "stdin", + "arguments" + ], + "Suggestions": null, + "Secret": false, + "RefKind": "", + "Multiline": false, + "SystemManaged": false + }, { "Key": "timeoutSeconds", "Label": "Timeout (seconds)", @@ -4867,6 +4915,20 @@ "OptionalRef": true, "Multiline": false, "SystemManaged": false + }, + { + "Key": "runWithAdmin", + "Label": "Run with admin rights", + "Type": "boolean", + "Required": false, + "Default": "false", + "Description": "Runs each command with administrator rights. macOS asks you to approve every run — Touch ID when it's set up for sudo, your password otherwise.", + "Options": null, + "Suggestions": null, + "Secret": false, + "RefKind": "", + "Multiline": false, + "SystemManaged": false } ], "Output": "combined stdout+stderr from every sub-command that ran", diff --git a/internal/domain/composition/builtinworkflows.go b/internal/domain/composition/builtinworkflows.go index c1d509e41..a015d596d 100644 --- a/internal/domain/composition/builtinworkflows.go +++ b/internal/domain/composition/builtinworkflows.go @@ -372,7 +372,7 @@ func BuiltInWorkflows() []Workflow { {ID: "example-mcp-e0", Source: mcpTriggerID, Target: mcpCallID}, }, BuiltIn: true, - Seed: seedorigin.Stamp(2), + Seed: seedorigin.Stamp(3), }, { ID: "example-codeexec-workflow", diff --git a/internal/domain/composition/builtinworkflows_codingloop.go b/internal/domain/composition/builtinworkflows_codingloop.go index ed5a48a3c..06bf17440 100644 --- a/internal/domain/composition/builtinworkflows_codingloop.go +++ b/internal/domain/composition/builtinworkflows_codingloop.go @@ -45,7 +45,7 @@ func codingLoopBuiltInWorkflow() []Workflow { {ID: "coding-loop-e2", Source: applyID, Target: notifyID}, }, BuiltIn: true, - Seed: seedorigin.Stamp(2), + Seed: seedorigin.Stamp(3), }, } } diff --git a/internal/domain/composition/builtinworkflows_secretguard.go b/internal/domain/composition/builtinworkflows_secretguard.go index 1e9a934db..b00a32b34 100644 --- a/internal/domain/composition/builtinworkflows_secretguard.go +++ b/internal/domain/composition/builtinworkflows_secretguard.go @@ -55,7 +55,7 @@ func builtInSecretGuardWorkflows() []Workflow { {ID: "example-secret-guard-e0", Source: secretGuardTriggerID, Target: ExampleSecretGuardStepID}, }, BuiltIn: true, - Seed: seedorigin.Stamp(1), + Seed: seedorigin.Stamp(2), }, } } diff --git a/internal/domain/composition/codeexec.go b/internal/domain/composition/codeexec.go index dfb0ddd21..588c797ff 100644 --- a/internal/domain/composition/codeexec.go +++ b/internal/domain/composition/codeexec.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strconv" "strings" "time" @@ -175,6 +176,41 @@ func shellArgv(shell, profile, script string) []string { } } +// inputArgs turns an upstream payload into the argument list a +// pass-input=arguments script receives -- one argument per line, the +// Shortcuts convention for list-shaped input. Lines arrive VERBATIM +// (interior empty lines stay empty arguments); only a single trailing +// newline's empty remainder is dropped, so "a\nb\n" is two arguments, +// not three. CR is stripped per line for CRLF payloads. Empty input +// means no arguments at all. +func inputArgs(input string) []string { + if input == "" { + return nil + } + lines := strings.Split(strings.TrimSuffix(input, "\n"), "\n") + for i, l := range lines { + lines[i] = strings.TrimSuffix(l, "\r") + } + return lines +} + +// appendShellArgs extends a shellArgv-built `-c` invocation with +// positional arguments. POSIX `-c` semantics: the first operand after +// the command string becomes $0, the rest $1..$N -- so the shell's own +// name is inserted as the $0 placeholder and the input lines land as +// "$@" exactly the way a terminal invocation would deliver them. A nil +// args returns argv unchanged (no stray $0 operand for the common +// no-arguments case). +func appendShellArgs(argv []string, args []string) []string { + if len(args) == 0 { + return argv + } + out := make([]string, 0, len(argv)+1+len(args)) + out = append(out, argv...) + out = append(out, filepath.Base(argv[0])) + return append(out, args...) +} + // resolveDir turns an ExecEnv's Dir into a real, existing directory -- // TempDirSentinel mints a fresh one per run (os.MkdirTemp), matching // the seeded "Safe sandbox" env's own design (nothing this env @@ -220,6 +256,11 @@ func init() { Description: "The command to run when \"Command source\" is literal. Ignored when source is payload.", Default: "", Type: FieldText, }, + { + Key: "passInput", Label: "Pass input", Type: FieldOptions, + Description: "How a literal script receives the upstream payload: piped to stdin, or one argument per line ($1, $2, …).", + Default: "stdin", Options: []string{"stdin", "arguments"}, + }, { Key: "timeoutSeconds", Label: "Timeout (seconds)", Type: FieldNumber, Description: "Kills the command if it hasn't finished within this many seconds.", @@ -233,8 +274,20 @@ func init() { } script := ctx.Payload + // A literal script receives the upstream payload as INPUT -- + // the Shortcuts/Automator pass-input convention (goal 0240 S5): + // piped to stdin (default), or one argument per line. Source + // "payload" runs the payload AS the script, so there is no + // separate input to route and both modes are no-ops there. + var stdin string + var extraArgs []string if node.Config["source"] == "literal" { script = node.Config["script"] + if node.Config["passInput"] == "arguments" { + extraArgs = inputArgs(ctx.Payload) + } else { + stdin = ctx.Payload + } } if strings.TrimSpace(script) == "" { return ctx, fmt.Errorf("code-execution: nothing to run (empty command)") @@ -263,10 +316,11 @@ func init() { var out strings.Builder handle, err := startProcessFn(procexec.Spec{ - Argv: shellArgv(re.Shell, re.ProfileMode, script), + Argv: appendShellArgs(shellArgv(re.Shell, re.ProfileMode, script), extraArgs), Dir: dir, Env: env, HardTimeout: timeout, + Stdin: stdin, Output: &out, }) if err != nil { diff --git a/internal/domain/composition/codeexec_test.go b/internal/domain/composition/codeexec_test.go index e5a0f7cf7..e64213d37 100644 --- a/internal/domain/composition/codeexec_test.go +++ b/internal/domain/composition/codeexec_test.go @@ -206,3 +206,99 @@ func TestShellArgv_CleanAndLoginModesPerShell(t *testing.T) { } } } + +// TestCodeExecution_LiteralPassInputStdin_PipesThePayload pins goal +// 0240 S5's pass-input default (the Shortcuts convention): a literal +// script receives the upstream payload on stdin. +func TestCodeExecution_LiteralPassInputStdin_PipesThePayload(t *testing.T) { + swapExecEnvLookupForTest(t, func(id string) (ResolvedExecEnv, error) { return testExecEnv(t), nil }) + + out, err := runCodeExecution(t, Node{ID: "n1", Config: map[string]string{ + "envId": "e1", "source": "literal", "script": `cat`, "timeoutSeconds": "10", + }}, "payload on stdin\n") + if err != nil { + t.Fatalf("exec: %v", err) + } + if got := strings.TrimSpace(out.Payload); got != "payload on stdin" { + t.Errorf("Payload = %q, want the piped input", got) + } +} + +// TestCodeExecution_LiteralPassInputArguments_OneArgPerLine pins the +// "as arguments" half: each payload line lands as its own positional +// argument, reachable as "$@". +func TestCodeExecution_LiteralPassInputArguments_OneArgPerLine(t *testing.T) { + swapExecEnvLookupForTest(t, func(id string) (ResolvedExecEnv, error) { return testExecEnv(t), nil }) + + out, err := runCodeExecution(t, Node{ID: "n1", Config: map[string]string{ + "envId": "e1", "source": "literal", "script": `printf '%s|' "$@"`, "passInput": "arguments", "timeoutSeconds": "10", + }}, "alpha\nbeta\ngamma\n") + if err != nil { + t.Fatalf("exec: %v", err) + } + if got := strings.TrimSpace(out.Payload); got != "alpha|beta|gamma|" { + t.Errorf("Payload = %q, want one argument per input line", got) + } +} + +// TestCodeExecution_SourcePayload_PassInputIgnored pins the no-op: +// source "payload" runs the payload AS the script, so there is no +// separate input to route and the passInput setting changes nothing. +func TestCodeExecution_SourcePayload_PassInputIgnored(t *testing.T) { + swapExecEnvLookupForTest(t, func(id string) (ResolvedExecEnv, error) { return testExecEnv(t), nil }) + + out, err := runCodeExecution(t, Node{ID: "n1", Config: map[string]string{ + "envId": "e1", "source": "payload", "passInput": "arguments", "timeoutSeconds": "10", + }}, `echo unrouted`) + if err != nil { + t.Fatalf("exec: %v", err) + } + if got := strings.TrimSpace(out.Payload); got != "unrouted" { + t.Errorf("Payload = %q, want %q", got, "unrouted") + } +} + +func TestInputArgs_LineSplitting(t *testing.T) { + cases := []struct { + in string + want []string + }{ + {"", nil}, + {"one", []string{"one"}}, + {"a\nb\n", []string{"a", "b"}}, + {"a\n\nb", []string{"a", "", "b"}}, + {"a\r\nb\r\n", []string{"a", "b"}}, + } + for _, c := range cases { + got := inputArgs(c.in) + if len(got) != len(c.want) { + t.Errorf("inputArgs(%q) = %v, want %v", c.in, got, c.want) + continue + } + for i := range got { + if got[i] != c.want[i] { + t.Errorf("inputArgs(%q)[%d] = %q, want %q", c.in, i, got[i], c.want[i]) + } + } + } +} + +// TestAppendShellArgs_DollarZeroPlaceholder pins the POSIX -c operand +// convention: the first appended operand becomes $0, so the shell's +// own basename is inserted ahead of the real arguments. +func TestAppendShellArgs_DollarZeroPlaceholder(t *testing.T) { + base := []string{"/bin/sh", "-c", "printf '%s' \"$1\""} + got := appendShellArgs(base, []string{"first"}) + want := []string{"/bin/sh", "-c", "printf '%s' \"$1\"", "sh", "first"} + if len(got) != len(want) { + t.Fatalf("appendShellArgs = %v, want %v", got, want) + } + for i := range got { + if got[i] != want[i] { + t.Fatalf("appendShellArgs[%d] = %q, want %q", i, got[i], want[i]) + } + } + if out := appendShellArgs(base, nil); len(out) != len(base) { + t.Fatalf("appendShellArgs with no args = %v, want argv unchanged", out) + } +} diff --git a/internal/domain/composition/executeshellcommand.go b/internal/domain/composition/executeshellcommand.go index 77e9174ce..88718fa02 100644 --- a/internal/domain/composition/executeshellcommand.go +++ b/internal/domain/composition/executeshellcommand.go @@ -3,6 +3,7 @@ package composition import ( "fmt" "os" + "path/filepath" "strings" "sync" "time" @@ -179,6 +180,60 @@ type shellStepOutcome struct { cancelled bool } +// AdminForcedAsk reports whether node is a shell step configured to run +// with administrator rights (goal 0240 S5). Read by the guardrail +// evaluation seams as well as this file's own exec path: an admin run +// ALWAYS asks -- an allow rule matching the command text never +// auto-grants privilege (the goal's recorded fail-safe policy) -- so +// every evaluator upgrades an allow verdict to ask for such a node, +// while a deny still wins unchanged. +func AdminForcedAsk(node Node) bool { + return node.NodeTypeID == "process-shell-command" && node.Config["runWithAdmin"] == "true" +} + +// adminWrapFn wraps one step's argv for an admin run -- overridable so +// tests assert the wrapping without a real sudo prompt. +var adminWrapFn = wrapArgvForAdmin + +// wrapArgvForAdmin escalates via sudo's own documented GUI hook (goal +// 0240 S5's owner-decided mechanism): `sudo -A` invokes the program +// named by SUDO_ASKPASS to collect the password when no terminal +// exists, and where pam_tid is configured for sudo the system Touch ID +// prompt satisfies authentication before the askpass is ever consulted +// -- Mill never handles the credential on that path at all. The +// returned env is the real environment plus SUDO_ASKPASS (the caller +// guarantees no resolved-secret env reaches here). Headless (server +// mode) the askpass's dialog cannot appear, sudo's auth fails, and the +// step errors -- fail-closed, never a hang: sudo -A exits rather than +// waiting on a TTY. +func wrapArgvForAdmin(argv []string) ([]string, []string, error) { + askpass, err := materializeAskpass() + if err != nil { + return nil, nil, err + } + wrapped := append([]string{"/usr/bin/sudo", "-A"}, argv...) + env := append(os.Environ(), "SUDO_ASKPASS="+askpass) + return wrapped, env, nil +} + +// materializeAskpass writes the askpass helper sudo -A executes: a +// two-line shell script showing the standard macOS password dialog +// (osascript `display dialog ... with hidden answer` -- the +// ssh-askpass ecosystem shape; NOT the deprecated +// administrator-privileges AppleScript API) and printing the entered +// text to stdout for sudo to consume. Rewritten on every call so the +// content is always exactly this script; 0700 in the per-user temp dir +// so no other user can swap it. +func materializeAskpass() (string, error) { + const script = "#!/bin/sh\n" + + "exec /usr/bin/osascript -e 'display dialog \"Mill needs an administrator password to run this command.\" default answer \"\" with hidden answer with title \"Mill\" with icon caution' -e 'text returned of result'\n" + path := filepath.Join(os.TempDir(), "mill-sudo-askpass.sh") + if err := os.WriteFile(path, []byte(script), 0o700); err != nil { //nolint:gosec // 0700, not 0600: sudo -A EXECUTES this file; owner-only exec is the askpass contract + return "", fmt.Errorf("write askpass helper: %w", err) + } + return path, nil +} + // resolveShellSecretEnv resolves every env-var-style secret placeholder // referenced anywhere in steps through the goal 0240 S2 chain // (shellSecretResolverFn: typed-stash -> vault -> shell env), for this @@ -255,8 +310,23 @@ func runShellStep(node Node, step ParsedCommandStep, total int, target ResolvedS }) } + argv := target.argvFor(step.Text) + if AdminForcedAsk(node) { + // Secret env values cannot survive sudo's own env_reset -- the + // escalated child would silently run WITHOUT the resolved + // secrets, which is exactly the silent-divergence the verbatim + // contract forbids. Refuse the combination honestly instead. + if env != nil { + return shellStepOutcome{}, fmt.Errorf("process-shell-command: a block referencing secrets can't run with admin rights (sudo strips the resolved environment)") + } + var wrapErr error + argv, env, wrapErr = adminWrapFn(argv) + if wrapErr != nil { + return shellStepOutcome{}, fmt.Errorf("process-shell-command: %w", wrapErr) + } + } handle, err := startShellProcessFn(procexec.Spec{ - Argv: target.argvFor(step.Text), + Argv: argv, Dir: target.Dir, // Env nil (the common case, resolveShellSecretEnv's own doc // comment) falls back to the calling process's real environment @@ -367,6 +437,11 @@ func init() { Description: "Runs the block inside a Configure-authored environment. Empty runs your real login shell.", Default: "", Type: FieldText, RefKind: "execenv", OptionalRef: true, }, + { + Key: "runWithAdmin", Label: "Run with admin rights", Type: FieldBoolean, + Description: "Runs each command with administrator rights. macOS asks you to approve every run — Touch ID when it's set up for sudo, your password otherwise.", + Default: "false", + }, }, }, func(node Node, ctx ExecContext) (ExecContext, error) { steps := ParseShellCommandBlock(ctx.Payload) diff --git a/internal/domain/composition/executeshellcommand_test.go b/internal/domain/composition/executeshellcommand_test.go index 1a6b57da8..c189bc96c 100644 --- a/internal/domain/composition/executeshellcommand_test.go +++ b/internal/domain/composition/executeshellcommand_test.go @@ -1,8 +1,11 @@ package composition import ( + "os" "strings" "testing" + + "github.com/alicoding/mill/internal/adapters/procexec" ) func runShellCommand(t *testing.T, payload string) (ExecContext, error) { @@ -107,3 +110,104 @@ func TestTailLines_ShorterThanCapIsUnchanged(t *testing.T) { t.Errorf("tailLines = %q, want %q", got, want) } } + +func TestAdminForcedAsk_ReadsNodeConfig(t *testing.T) { + if AdminForcedAsk(Node{NodeTypeID: "process-shell-command", Config: map[string]string{"runWithAdmin": "true"}}) != true { + t.Fatal("admin shell node must force ask") + } + if AdminForcedAsk(Node{NodeTypeID: "process-shell-command"}) { + t.Fatal("default shell node must not force ask") + } + if AdminForcedAsk(Node{NodeTypeID: "code-execution", Config: map[string]string{"runWithAdmin": "true"}}) { + t.Fatal("only the shell step carries the admin mode") + } +} + +// TestWrapArgvForAdmin_SudoAskpassShape pins the escalation mechanism +// (goal 0240 S5): sudo's own -A/SUDO_ASKPASS hook, an executable +// askpass helper materialized 0700, and NEVER the deprecated +// administrator-privileges AppleScript API. +func TestWrapArgvForAdmin_SudoAskpassShape(t *testing.T) { + argv, env, err := wrapArgvForAdmin([]string{"/bin/zsh", "-c", "whoami"}) + if err != nil { + t.Fatalf("wrapArgvForAdmin: %v", err) + } + if argv[0] != "/usr/bin/sudo" || argv[1] != "-A" || argv[2] != "/bin/zsh" { + t.Fatalf("argv = %v, want the original argv behind sudo -A", argv) + } + var askpass string + for _, kv := range env { + if strings.HasPrefix(kv, "SUDO_ASKPASS=") { + askpass = strings.TrimPrefix(kv, "SUDO_ASKPASS=") + } + } + if askpass == "" { + t.Fatal("env carries no SUDO_ASKPASS") + } + info, err := os.Stat(askpass) + if err != nil { + t.Fatalf("askpass helper missing: %v", err) + } + if info.Mode().Perm() != 0o700 { + t.Fatalf("askpass mode = %v, want 0700", info.Mode().Perm()) + } + content, err := os.ReadFile(askpass) //nolint:gosec // the path under test comes from this test's own env assertion, not user input + if err != nil { + t.Fatalf("read askpass: %v", err) + } + if !strings.Contains(string(content), "hidden answer") || strings.Contains(string(content), "administrator privileges") { + t.Fatalf("askpass content = %q, want a hidden-answer dialog and never the admin-privileges API", content) + } +} + +// TestProcessShellCommand_AdminRun_WrapsEveryStep proves the exec path +// consults the node's own runWithAdmin config: each step's Spec argv is +// wrapped and its env carries SUDO_ASKPASS, without any real sudo +// spawn (runner stubbed). +func TestProcessShellCommand_AdminRun_WrapsEveryStep(t *testing.T) { + var specs []procexec.Spec + orig := startShellProcessFn + SetShellCommandRunner(func(s procexec.Spec) (*procexec.Handle, error) { + specs = append(specs, s) + return procexec.Start(procexec.Spec{Argv: []string{"true"}, Output: s.Output}) + }) + t.Cleanup(func() { startShellProcessFn = orig }) + + entry := nodeTypeRegistry["process-shell-command"] + node := Node{ID: "n1", NodeTypeID: "process-shell-command", Config: map[string]string{"runWithAdmin": "true"}} + if _, err := entry.exec(node, ExecContext{Payload: "echo a\necho b", Attributes: map[string]any{}}); err != nil { + t.Fatalf("exec: %v", err) + } + if len(specs) != 2 { + t.Fatalf("got %d specs, want 2", len(specs)) + } + for i, s := range specs { + if s.Argv[0] != "/usr/bin/sudo" || s.Argv[1] != "-A" { + t.Fatalf("step %d argv = %v, want sudo -A wrapping", i, s.Argv) + } + found := false + for _, kv := range s.Env { + if strings.HasPrefix(kv, "SUDO_ASKPASS=") { + found = true + } + } + if !found { + t.Fatalf("step %d env carries no SUDO_ASKPASS", i) + } + } +} + +// TestProcessShellCommand_AdminWithSecrets_RefusedHonestly pins the +// recorded seam: sudo's env_reset would strip a resolved secret from +// the escalated child, so the combination fails with a clear message +// instead of silently running without the secret. +func TestProcessShellCommand_AdminWithSecrets_RefusedHonestly(t *testing.T) { + entry := nodeTypeRegistry["process-shell-command"] + node := Node{ID: "n1", NodeTypeID: "process-shell-command", Config: map[string]string{"runWithAdmin": "true"}} + // $MILL_S5_TOKEN matches the secret-shaped env-ref pattern, so the + // block resolves a secret env and the admin combination must refuse. + _, err := entry.exec(node, ExecContext{Payload: "echo $MILL_S5_TOKEN", Attributes: map[string]any{}}) + if err == nil || !strings.Contains(err.Error(), "can't run with admin rights") { + t.Fatalf("err = %v, want the honest secrets-with-admin refusal", err) + } +} diff --git a/internal/services/executionsvc/executionservice_guardrail.go b/internal/services/executionsvc/executionservice_guardrail.go index 4bd638b92..c6aa1fc3b 100644 --- a/internal/services/executionsvc/executionservice_guardrail.go +++ b/internal/services/executionsvc/executionservice_guardrail.go @@ -142,18 +142,27 @@ func (e *ExecutionService) evaluateVerdict(workflowID string, node composition.N // EvaluateStep is the same core guardrailsvc.RequestGuardedAction's // EvaluateAction calls -- this is the execution gate's own call // site, never a second evaluation of these rules. - if node.NodeTypeID != shellCommandNodeTypeID { - return e.guard.EvaluateStep(guardrailsvc.GuardrailStep(workflowID, node, ec), class) - } - steps := composition.ParseShellCommandBlock(ec.Payload) - if len(steps) == 0 { - return guardrail.Evaluate(e.guard.Rules(), guardrailsvc.GuardrailStep(workflowID, node, ec), class) - } - commands := make([]string, len(steps)) - for i, s := range steps { - commands[i] = s.Text + v := func() guardrail.Verdict { + if node.NodeTypeID != shellCommandNodeTypeID { + return e.guard.EvaluateStep(guardrailsvc.GuardrailStep(workflowID, node, ec), class) + } + steps := composition.ParseShellCommandBlock(ec.Payload) + if len(steps) == 0 { + return guardrail.Evaluate(e.guard.Rules(), guardrailsvc.GuardrailStep(workflowID, node, ec), class) + } + commands := make([]string, len(steps)) + for i, s := range steps { + commands[i] = s.Text + } + return guardrail.WorstVerdict(e.guard.ShellCommandVerdicts(commands)) + }() + // An admin run always asks (composition.AdminForcedAsk's fail-safe + // policy): an allow verdict -- a matching allow rule included -- + // upgrades to ask; deny keeps winning unchanged. + if composition.AdminForcedAsk(node) && v.Effect == guardrail.EffectAllow { + v = guardrail.Verdict{Effect: guardrail.EffectAsk, RuleLabel: "Runs with admin rights"} } - return guardrail.WorstVerdict(e.guard.ShellCommandVerdicts(commands)) + return v } // guardrailGate is installed as composition.SetGuardrailGate at @@ -374,6 +383,11 @@ func (e *ExecutionService) mayRequireApproval(workflowID string, nodes []composi if composition.NodeAlwaysParks(n) { return true } + // An admin shell step always asks -- same fail-safe policy the + // gate's evaluateVerdict applies. + if composition.AdminForcedAsk(n) { + return true + } step := guardrail.Step{ NodeTypeID: n.NodeTypeID, RequestID: n.Config["requestId"], diff --git a/internal/services/guardrailsvc/guardrailservice.go b/internal/services/guardrailsvc/guardrailservice.go index b5461c093..1367136b5 100644 --- a/internal/services/guardrailsvc/guardrailservice.go +++ b/internal/services/guardrailsvc/guardrailservice.go @@ -257,6 +257,12 @@ func (g *GuardrailService) TestRules(workflowID, nodeID string) (RuleTestResult, verdict := guardrail.Evaluate(g.Rules(), GuardrailStep(workflowID, *target, composition.ExecContext{ Attributes: composition.AttributesEnv(wf.Attributes, nil), }), class) + // An admin shell step always asks (composition.AdminForcedAsk's + // fail-safe policy) -- the dry-run must report exactly what the + // execution gate will do, never an allow the gate would upgrade. + if composition.AdminForcedAsk(*target) && verdict.Effect == guardrail.EffectAllow { + verdict = guardrail.Verdict{Effect: guardrail.EffectAsk, RuleLabel: "Runs with admin rights"} + } return RuleTestResult{ Effect: string(verdict.Effect), RuleID: verdict.RuleID, @@ -351,8 +357,30 @@ func GuardrailStep(workflowID string, node composition.Node, ec composition.Exec // granularity. func (g *GuardrailService) ShellCommandVerdicts(commands []string) []guardrail.Verdict { node := composition.Node{ID: composition.CodingLoopShellStepID, NodeTypeID: "process-shell-command"} + // The REAL seeded node's config, when present -- its runWithAdmin + // answer must reach the admin upgrade below, or the Confirm + // preview's per-line verdicts would promise an unattended run the + // execution gate (which shares this same function) won't give. + for _, w := range g.comp.Workflows() { + if w.ID != composition.CodingLoopWorkflowID { + continue + } + for i := range w.Nodes { + if w.Nodes[i].ID == composition.CodingLoopShellStepID { + node = w.Nodes[i] + } + } + } base := GuardrailStep(composition.CodingLoopWorkflowID, node, composition.ExecContext{}) - return guardrail.EvaluateCommandSteps(g.Rules(), base, commands, guardrail.ClassExternal) + verdicts := guardrail.EvaluateCommandSteps(g.Rules(), base, commands, guardrail.ClassExternal) + if composition.AdminForcedAsk(node) { + for i := range verdicts { + if verdicts[i].Effect == guardrail.EffectAllow { + verdicts[i] = guardrail.Verdict{Effect: guardrail.EffectAsk, RuleLabel: "Runs with admin rights"} + } + } + } + return verdicts } // WorkflowVerdicts dry-runs the current rule set against every @@ -388,6 +416,11 @@ func (g *GuardrailService) WorkflowVerdicts(workflowID string) (map[string]RuleT } class := composition.EffectForNode(n) v := guardrail.Evaluate(rules, GuardrailStep(workflowID, n, composition.ExecContext{Attributes: attrs}), class) + // Same admin upgrade the execution gate applies -- the canvas + // badge must never promise an unattended run the gate won't give. + if composition.AdminForcedAsk(n) && v.Effect == guardrail.EffectAllow { + v = guardrail.Verdict{Effect: guardrail.EffectAsk, RuleLabel: "Runs with admin rights"} + } out[n.ID] = RuleTestResult{ Effect: string(v.Effect), RuleID: v.RuleID, RuleLabel: v.RuleLabel, EffectClass: string(class), Source: v.Source, } diff --git a/internal/services/guardrailsvc/guardrailservice_shellcommand_test.go b/internal/services/guardrailsvc/guardrailservice_shellcommand_test.go index e9bec770f..1aa4421dd 100644 --- a/internal/services/guardrailsvc/guardrailservice_shellcommand_test.go +++ b/internal/services/guardrailsvc/guardrailservice_shellcommand_test.go @@ -3,6 +3,7 @@ package guardrailsvc import ( "testing" + "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/guardrail" ) @@ -47,3 +48,43 @@ func TestShellCommandVerdicts_UnlistedCommand_FallsBackToTheClassDefault(t *test t.Fatalf("verdict = %+v, want the plain class default (ask, no rule)", verdicts[0]) } } + +// TestShellCommandVerdicts_AdminRun_UpgradesAllowToAsk pins goal 0240 +// S5's fail-safe policy at the shared preview/gate seam: with the +// seeded shell step configured runWithAdmin, even a command matching a +// seeded ALLOW rule reports ask -- privilege is never auto-granted by a +// pattern list -- while a deny-listed shape keeps its own rule +// attribution unchanged. +func TestShellCommandVerdicts_AdminRun_UpgradesAllowToAsk(t *testing.T) { + g, comp := newTestGuardrailService(t) + g.rules = guardrail.BuiltIn() + + var loop composition.Workflow + for _, w := range comp.Workflows() { + if w.ID == composition.CodingLoopWorkflowID { + loop = w + } + } + if loop.ID == "" { + t.Fatal("seeded coding-loop workflow missing from the test composition service") + } + for i := range loop.Nodes { + if loop.Nodes[i].ID == composition.CodingLoopShellStepID { + if loop.Nodes[i].Config == nil { + loop.Nodes[i].Config = map[string]string{} + } + loop.Nodes[i].Config["runWithAdmin"] = "true" + } + } + if _, err := comp.UpdateWorkflow(loop.ID, loop.Label, loop.Description, loop.Nodes, loop.Edges); err != nil { + t.Fatalf("UpdateWorkflow: %v", err) + } + + verdicts := g.ShellCommandVerdicts([]string{"curl -I https://example.test", "rm -rf /tmp/whatever"}) + if verdicts[0].Effect != guardrail.EffectAsk || verdicts[0].RuleLabel != "Runs with admin rights" { + t.Fatalf("allow-listed verdict = %+v, want the admin ask upgrade", verdicts[0]) + } + if verdicts[1].Effect != guardrail.EffectAsk || verdicts[1].RuleID != guardrail.ShellDenyRmRfRuleID { + t.Fatalf("deny-listed verdict = %+v, want the deny rule's own attribution unchanged", verdicts[1]) + } +} diff --git a/internal/services/seeding/seed_fingerprints.json b/internal/services/seeding/seed_fingerprints.json index 450777bfd..a631eb577 100644 --- a/internal/services/seeding/seed_fingerprints.json +++ b/internal/services/seeding/seed_fingerprints.json @@ -196,8 +196,8 @@ "fingerprint": "cbd789a974956ceea36271f5240de1d118b6966e4a7c9637fb00467f07f03d00" }, "workflow:coding-loop-run-copied-command-workflow": { - "seedRevision": 2, - "fingerprint": "ebf1af11ee05a917b9a32c405acf2f1648bca1727d01f8764f4d41ac5c68bef6" + "seedRevision": 3, + "fingerprint": "921927c388734f3530778d090134ad9b5900b77eb6cdfb5573036cb7fba10c7a" }, "workflow:example-ai-classify-branch-workflow": { "seedRevision": 2, @@ -229,7 +229,7 @@ }, "workflow:example-codeexec-workflow": { "seedRevision": 2, - "fingerprint": "885b37e75639c22e576436b7272754c86854bb0d896fffd677c43c61c1e13586" + "fingerprint": "224be4f17aca05b95354151cf5f718c0902c6a193dcd288a79065bee7ce1da8a" }, "workflow:example-confluence-to-markdown-workflow": { "seedRevision": 2, @@ -288,7 +288,7 @@ "fingerprint": "4ed20ba60b7602a75b0bb7490f3a2780faab1be1c7906618f1c578408be174d3" }, "workflow:example-mcp-echo-workflow": { - "seedRevision": 2, + "seedRevision": 3, "fingerprint": "a24f6a0a19fa008c61e7f8baa641b1de67b9b80ad2a0cab642d9853af0de7666" }, "workflow:example-parent-workflow": { @@ -312,8 +312,8 @@ "fingerprint": "58474919963eb4da78947e7f46802b2c75d2c0e51f4e685b5c7e94774d00550c" }, "workflow:example-secret-guard-workflow": { - "seedRevision": 1, - "fingerprint": "e30cb4264a5bc4557450609fe2fa0af0eadd9715cc6de756b84498e4ba195f68" + "seedRevision": 2, + "fingerprint": "78cc6d87d6308837ab1da6c316e430f65c60af174aa9ab1c6d1f36e636fa5eb7" }, "workflow:example-step-failure-workflow": { "seedRevision": 1, diff --git a/userdocs/llms-full.txt b/userdocs/llms-full.txt index d28e470ee..9eff0b632 100644 --- a/userdocs/llms-full.txt +++ b/userdocs/llms-full.txt @@ -860,6 +860,7 @@ Runs the captured payload exactly as written -- in your real login shell by defa - Effect: external — parks for approval by default - Settings: - **Execution environment** — Runs the block inside a Configure-authored environment. Empty runs your real login shell. (references an Execution environment) + - **Run with admin rights** — Runs each command with administrator rights. macOS asks you to approve every run — Touch ID when it's set up for sudo, your password otherwise. ### Run a command @@ -871,6 +872,7 @@ Runs one command locally, inside a configured execution environment (pinned shel - **Execution environment** — Which Configure-authored environment (shell, working directory, env vars) this command runs inside. (references an Execution environment) - **Command source** — "payload" runs the captured/upstream payload as the command; "literal" runs the script below instead. - **Script** — The command to run when "Command source" is literal. Ignored when source is payload. + - **Pass input** — How a literal script receives the upstream payload: piped to stdin, or one argument per line ($1, $2, …). - **Timeout (seconds)** — Kills the command if it hasn't finished within this many seconds. ### Run another workflow diff --git a/userdocs/reference/steps.md b/userdocs/reference/steps.md index 1e3fae563..4ade54de5 100644 --- a/userdocs/reference/steps.md +++ b/userdocs/reference/steps.md @@ -245,6 +245,7 @@ Runs the captured payload exactly as written -- in your real login shell by defa - Effect: external — parks for approval by default - Settings: - **Execution environment** — Runs the block inside a Configure-authored environment. Empty runs your real login shell. (references an Execution environment) + - **Run with admin rights** — Runs each command with administrator rights. macOS asks you to approve every run — Touch ID when it's set up for sudo, your password otherwise. ### Run a command @@ -256,6 +257,7 @@ Runs one command locally, inside a configured execution environment (pinned shel - **Execution environment** — Which Configure-authored environment (shell, working directory, env vars) this command runs inside. (references an Execution environment) - **Command source** — "payload" runs the captured/upstream payload as the command; "literal" runs the script below instead. - **Script** — The command to run when "Command source" is literal. Ignored when source is payload. + - **Pass input** — How a literal script receives the upstream payload: piped to stdin, or one argument per line ($1, $2, …). - **Timeout (seconds)** — Kills the command if it hasn't finished within this many seconds. ### Run another workflow