From 3cd1871a53c55e44b534e7c95335deb38f8a5429 Mon Sep 17 00:00:00 2001 From: benbenbang Date: Fri, 9 Oct 2026 15:59:11 +0800 Subject: [PATCH] feat(config): add safe editor command for config files Add `config edit` with explicit editor override, selected TOML editor support, and Vim on PATH as the default. Execute the editor directly without shell or argument parsing, passing the absolute selected filename as one argument and forwarding terminal streams through the command context. Reject missing, nonregular, and final-symlink targets, while allowing Vim repair of invalid TOML and explicit-editor repair of invalid, mistyped, or oversized configuration files. Ignore `EDITOR`, `VISUAL`, and decision timeouts, and bound post-exit pipe waits without managing descendants. Document the security boundaries and cover editor selection, repair, process errors, pipe handling, native smoke tests, full gates, and race tests. --- README.md | 8 +- docs/internal/config-commands.md | 2 + internal/cli/config.go | 2 +- internal/cli/config_edit.go | 65 +++++++++++ internal/cli/config_edit_test.go | 185 +++++++++++++++++++++++++++++++ internal/config/editor.go | 35 ++++++ internal/config/editor_test.go | 29 +++++ 7 files changed, 324 insertions(+), 2 deletions(-) create mode 100644 internal/cli/config_edit.go create mode 100644 internal/cli/config_edit_test.go create mode 100644 internal/config/editor.go create mode 100644 internal/config/editor_test.go diff --git a/README.md b/README.md index 744bff2..e50cce8 100644 --- a/README.md +++ b/README.md @@ -121,17 +121,23 @@ Precedence: **changed flags > nonblank environment > TOML > defaults**. - Trust the current directory before running decision/MCP commands: local settings can change the upstream destination. Environment/changed flags still win. Invalid, oversized (over 1 MiB), linked or nonregular local files fail without global fallback; help/version do not load them. - Start with [config.example.toml](config.example.toml). Prefer environment credentials rather than command-line secrets. -### Initialize and inspect +### Initialize, inspect and edit ```sh jevwise config init # Global XDG/HOME config, even if local exists jevwise config init --local # ./jevwise.toml, automatically selected in this cwd jevwise config view # Selected file/defaults, API key redacted jevwise config view --config /path/to/config.toml +jevwise config edit # Selected local/global file, vim by default +jevwise config edit --editor nvim # One-shot override, also for repairs ``` - Init never overwrites existing files, directories or links. It writes a template, not your environment credentials; Unix file permissions are requested as `0600`, new parent directories as `0700`. Use a trusted parent with safe ownership/ACLs: mode bits alone do not guarantee privacy, including inherited Unix ACLs. Windows users must manage ACLs separately. - `--config PATH` also selects an init destination; do not combine it with `--local` or pass a blank path. View ignores environment/decision flag overrides, unknown keys and comments; invalid files fail safely. It is not a dump of effective runtime settings. Only the `api_key` field is automatically redacted; do not store credentials in other displayed settings. +- Set `editor = "nvim"` (or another executable name/path) in TOML to change the editor. Selection is `config edit --editor` > selected file's `editor` > `vim` on PATH; `$EDITOR`/`$VISUAL` are not read. No editor is bundled and no shell is used. Arguments are not parsed: use a trusted wrapper executable if options such as `--wait` are needed. +- Edit forwards terminal input/output and waits for the direct editor process. It uses the command context, not the decision timeout; missing editors/nonzero exits fail safely. Invalid TOML falls back to Vim for repair; wrong-typed editor settings or oversized files can be repaired using `--editor`. It refuses missing files, directories and final symlinks. +- Go-copy pipe waits are bounded after process exit/cancellation. Only the direct editor process is managed, not wrapper descendants; arbitrary blocking custom readers/writers cannot be forcibly interrupted. +- Trust local editor settings and wrappers before editing: they select a program to execute. Editor terminal output intentionally may show the file you are editing. macOS/Linux support assumes an installed editor; no Windows editor availability/behavior is promised. - Add `jevwise.toml` to your project's ignore rules before storing credentials. Prefer environment API keys. ## Skill setup diff --git a/docs/internal/config-commands.md b/docs/internal/config-commands.md index 6ee6e2d..b5369c0 100644 --- a/docs/internal/config-commands.md +++ b/docs/internal/config-commands.md @@ -16,3 +16,5 @@ - 29 local QA passed after correcting blank selectors and short output writes: root fresh full gates, six builds and isolated native init/view smoke; independent full tests and CLI config race repeated 20 times. Public docs qualify ACL/privacy assumptions and field-only redaction. - Follow-up outside this scope: shared Cobra help-template write errors may be printed directly to stderr; harden the shared HelpFunc separately, not just config commands. + +- 30 local QA passed: root fresh full gates/six builds and fake-editor native smoke (PATH/default/custom spaced path/repair/override/help); independent final full tests and editor/config race repeated 10 times. Bounded inherited-pipe waits and owned-helper completion are regression-tested. This cohesive security regression justifies a small soft-300 exception; hard 450 remains satisfied. No real editor, API, agent, push, tag or release used. diff --git a/internal/cli/config.go b/internal/cli/config.go index a5ec855..15b6a71 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -90,6 +90,6 @@ func newConfig() *cobra.Command { } return nil }} - cmd.AddCommand(init, view) + cmd.AddCommand(init, view, newConfigEdit(runConfigEditor)) return cmd } diff --git a/internal/cli/config_edit.go b/internal/cli/config_edit.go new file mode 100644 index 0000000..0abecf7 --- /dev/null +++ b/internal/cli/config_edit.go @@ -0,0 +1,65 @@ +package cli + +import ( + "context" + "errors" + "io" + "os" + "os/exec" + "strings" + "time" + + "github.com/bitbrew-dev/jevwise/internal/config" + "github.com/spf13/cobra" +) + +type configEditor func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error + +func runConfigEditor(ctx context.Context, editor, path string, in io.Reader, out, stderr io.Writer) error { + process := exec.CommandContext(ctx, editor, path) // No shell; absolute file is one argument. + process.WaitDelay = 250 * time.Millisecond // Bound inherited Go-copy pipe waits. + process.Stdin, process.Stdout, process.Stderr = in, out, stderr + err := process.Run() + if ctx.Err() != nil { + return ctx.Err() + } + return err +} + +func newConfigEdit(run configEditor) *cobra.Command { + var override string + cmd := &cobra.Command{Use: "edit", Short: "Edit selected configuration (default: vim on PATH)", Args: configNoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + if err := cmd.Context().Err(); err != nil { + return err + } + path, err := configPathFlag(cmd) + if err != nil { + return err + } + path, _, err = config.ResolvePath(path) + if err != nil { + return err + } + info, err := os.Lstat(path) + if err != nil || !info.Mode().IsRegular() { + return errors.New("config edit requires an existing regular file, not a link; initialize it first") + } + editor := strings.TrimSpace(override) + if cmd.Flags().Changed("editor") && editor == "" { + return errors.New("editor executable must not be blank") + } + if editor == "" { + editor, err = config.Editor(path) + if err != nil { + return err + } + } + if err := run(cmd.Context(), editor, path, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr()); err != nil { + return &decisionError{"cannot run editor; install vim or select one editor executable name/path", err} + } + return nil + }} + cmd.Flags().StringVar(&override, "editor", "", "Override editor executable for this edit (no shell or arguments)") + return cmd +} diff --git a/internal/cli/config_edit_test.go b/internal/cli/config_edit_test.go new file mode 100644 index 0000000..61e345f --- /dev/null +++ b/internal/cli/config_edit_test.go @@ -0,0 +1,185 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + "github.com/bitbrew-dev/jevwise/internal/config" + "github.com/spf13/cobra" +) + +func editorRoot(t *testing.T, run configEditor) *cobra.Command { + root := configRoot(t) + group, _, _ := root.Find([]string{"config"}) + edit, _, _ := root.Find([]string{"config", "edit"}) + group.RemoveCommand(edit) + group.AddCommand(newConfigEdit(run)) + return root +} + +func TestConfigEditSelectionStreamsAndRepair(t *testing.T) { + for _, mode := range []string{"default", "local", "global", "explicit", "malformed", "override"} { + t.Run(mode, func(t *testing.T) { + configEnvironment(t) + path, _ := filepath.Abs(config.LocalName) + content, want := "", "vim" + args := []string{"config", "edit"} + if mode == "global" { + path, _ = config.GlobalPath() + } + if mode == "explicit" { + path = filepath.Join(t.TempDir(), "selected file.toml") + args = append(args, "--config", path) + _ = os.WriteFile(config.LocalName, []byte("editor='wrong-local'"), 0600) + } + if mode == "local" || mode == "global" || mode == "explicit" { + content, want = "editor='configured editor'", "configured editor" + } + if mode == "malformed" { + content = "api_key='private-secret'\n[" + } + if mode == "override" { + content, want = "editor=42", "override-editor" + args = append(args, "--editor", want) + } + _ = os.MkdirAll(filepath.Dir(path), 0700) + _ = os.WriteFile(path, []byte(content), 0600) + calls := 0 + root := editorRoot(t, func(ctx context.Context, editor, selected string, in io.Reader, out, stderr io.Writer) error { + calls++ + input, _ := io.ReadAll(in) + if editor != want || selected != path || string(input) != "typed" || ctx.Err() != nil { + t.Fatal("wrong editor/path/streams/context") + } + _, _ = io.WriteString(out, "editor-output") + _, _ = io.WriteString(stderr, "editor-diagnostic") + return nil + }) + root.SetIn(strings.NewReader("typed")) + out, stderr, err := execute(root, args...) + if err != nil || calls != 1 || out != "editor-output" || stderr != "editor-diagnostic" { + t.Fatal("editor not invoked correctly", err) + } + }) + } +} + +func TestConfigEditInvalidRequestsHelpAndSafeFailure(t *testing.T) { + configEnvironment(t) + for _, args := range [][]string{{"config", "edit", "--help", "--config", "/missing"}, {"config", "edit"}, {"config", "edit", "private-secret"}, {"config", "edit", "--config="}} { + out, _, err := execute(editorRoot(t, func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error { + t.Fatal("invalid/help started editor") + return nil + }), args...) + if strings.Contains(strings.Join(args, " "), "--help") { + if err != nil || !strings.Contains(out, "--editor") { + t.Fatal("edit help failed", err) + } + } else if err == nil || out != "" || strings.Contains(err.Error(), "private-secret") { + t.Fatal("invalid edit accepted or leaked", err) + } + } + _ = os.WriteFile(config.LocalName, []byte("editor='vim'"), 0600) + if _, _, err := execute(editorRoot(t, func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error { + return errors.New("private-secret") + }), "config", "edit"); err == nil || strings.Contains(err.Error(), "private-secret") { + t.Fatal("editor error leaked", err) + } + if runtime.GOOS != "windows" { + _ = os.Symlink(config.LocalName, "linked.toml") + if _, _, err := execute(editorRoot(t, func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error { + t.Fatal("linked editor target accepted") + return nil + }), "config", "edit", "--config", "linked.toml"); err == nil { + t.Fatal("symlink accepted") + } + } +} + +func TestConfigEditorProcessNoShellAndCancellation(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("installed editor behavior supported/tested on Unix") + } + directory := t.TempDir() + editor := filepath.Join(directory, "vim") + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + _ = os.WriteFile(editor, []byte("#!/bin/sh\nprintf '%s\\n' \"$#\" \"$1\"\ncat\nprintf 'diagnostic' >&2\n"), 0700) + path := filepath.Join(directory, "config ; not a command.toml") + var out, stderr bytes.Buffer + if err := runConfigEditor(context.Background(), "vim", path, strings.NewReader("typed"), &out, &stderr); err != nil || out.String() != "1\n"+path+"\ntyped" || stderr.String() != "diagnostic" { + t.Fatal("process arguments/streams changed", err) + } + marker := filepath.Join(directory, "not-created") + if err := runConfigEditor(context.Background(), "sh -c 'touch "+marker+"'", path, nil, io.Discard, io.Discard); err == nil { + t.Fatal("shell fragment executed") + } + if _, err := os.Stat(marker); !errors.Is(err, os.ErrNotExist) { + t.Fatal("shell side effect") + } + _ = os.WriteFile(editor, []byte("#!/bin/sh\nexit 7\n"), 0700) + if err := runConfigEditor(context.Background(), editor, path, nil, io.Discard, io.Discard); err == nil { + t.Fatal("nonzero editor exit accepted") + } + _ = os.WriteFile(editor, []byte("#!/bin/sh\nexec sleep 30\n"), 0700) + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + if err := runConfigEditor(ctx, editor, path, nil, io.Discard, io.Discard); !errors.Is(err, context.DeadlineExceeded) { + t.Fatal("editor cancellation ignored", err) + } +} + +func TestConfigEditRejectsBadEditorAndCancelledRequests(t *testing.T) { + configEnvironment(t) + _ = os.WriteFile(config.LocalName, []byte("editor=42"), 0600) + for _, args := range [][]string{{"config", "edit"}, {"config", "edit", "--editor="}, {"config", "edit", "--config", "."}} { + if _, _, err := execute(editorRoot(t, func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error { + t.Fatal("invalid editor request launched") + return nil + }), args...); err == nil { + t.Fatal("invalid editor accepted") + } + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + root := editorRoot(t, func(context.Context, string, string, io.Reader, io.Writer, io.Writer) error { + t.Fatal("cancelled edit launched") + return nil + }) + root.SetArgs([]string{"config", "edit", "--editor", "vim"}) + if err := root.ExecuteContext(ctx); !errors.Is(err, context.Canceled) { + t.Fatal("cancelled edit accepted", err) + } +} + +func TestConfigEditorBoundsInheritedPipeWait(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix synthetic editor helper") + } + dir := t.TempDir() + path, editor := filepath.Join(dir, "config.toml"), filepath.Join(dir, "helper") + _ = os.WriteFile(editor, []byte("#!/bin/sh\n(sleep 1; printf done > \"$1.finished\") &\nexit 0\n"), 0700) + var out, stderr bytes.Buffer + if err := runConfigEditor(context.Background(), editor, path, nil, &out, &stderr); !errors.Is(err, exec.ErrWaitDelay) { + t.Fatal("inherited output pipe was not bounded", err) + } + // Wait for the owned, bounded helper to finish before deleting its files. + deadline := time.Now().Add(5 * time.Second) + for { + if _, err := os.Stat(path + ".finished"); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("synthetic helper did not finish") + } + time.Sleep(10 * time.Millisecond) + } +} diff --git a/internal/config/editor.go b/internal/config/editor.go new file mode 100644 index 0000000..3a54960 --- /dev/null +++ b/internal/config/editor.go @@ -0,0 +1,35 @@ +package config + +import ( + "bytes" + "errors" + "strings" + + "github.com/spf13/viper" +) + +// Editor reads only the selected file's editor setting, not environment/SDK +// configuration. Invalid TOML falls back to vim so the file can be repaired. +func Editor(path string) (string, error) { + data, err := ReadFile(path) + if err != nil { + return "", errors.New("cannot read configuration to choose editor; use --editor to repair") + } + v := viper.New() + v.SetConfigType("toml") + if err := v.ReadConfig(bytes.NewReader(data)); err != nil { + return "vim", nil + } + value := v.Get("editor") + if value == nil { + return "vim", nil + } + editor, ok := value.(string) + if !ok { + return "", errors.New("editor must be one executable name or path; use --editor to repair") + } + if strings.TrimSpace(editor) == "" { + return "vim", nil + } + return strings.TrimSpace(editor), nil +} diff --git a/internal/config/editor_test.go b/internal/config/editor_test.go new file mode 100644 index 0000000..cdfdcda --- /dev/null +++ b/internal/config/editor_test.go @@ -0,0 +1,29 @@ +package config + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestEditorUsesOnlyFileAndCanRepairInvalidTOML(t *testing.T) { + cleanEnv(t) + t.Setenv("EDITOR", "environment-editor") + t.Setenv("VISUAL", "environment-visual") + t.Setenv("TS_JEV_EDITOR", "environment-editor") + for _, tc := range []struct { + content, want string + invalid bool + }{ + {"", "vim", false}, {"editor='nvim'", "nvim", false}, {"editor=' '", "vim", false}, + {"editor='/some/editor with spaces'", "/some/editor with spaces", false}, + {"model=42\neditor='nano'", "nano", false}, {"api_key='private-secret'\n[", "vim", false}, + {"editor=42", "", true}, + } { + path := writeConfig(t, filepath.Join(t.TempDir(), "config.toml"), tc.content) + got, err := Editor(path) + if got != tc.want || (err != nil) != tc.invalid || err != nil && strings.Contains(err.Error(), "private-secret") { + t.Fatal("editor selection or safe error failed", err) + } + } +}