From 755acf20d7f29354321870a57a61b33d38b18ceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 15:32:42 +0200 Subject: [PATCH] fix: write TOML escapes instead of Go escapes for config values strconv.Quote writes Go literals: BEL as \a, VT as \v, other control characters as \xHH. TOML 1.0 has none of these, so a config.toml holding a BEL or VT no longer loaded, and an invalid UTF-8 byte written as \xff came back as a different character. quoteTOML writes the same text as before for everything valid in both languages and \u00XX for the rest, and rejects invalid UTF-8. --- CHANGELOG.md | 6 + internal/cli/set_toml_test.go | 67 +++++++++ internal/config/edit.go | 10 +- internal/config/literal_test.go | 253 ++++++++++++++++++++++++++++++++ internal/config/quote.go | 68 +++++++++ 5 files changed, 401 insertions(+), 3 deletions(-) create mode 100644 internal/cli/set_toml_test.go create mode 100644 internal/config/literal_test.go create mode 100644 internal/config/quote.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a0de665..f890177 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). output. `list --json` was not valid JSON on hosts where the package manager writes to stdout. `apply`, `doctor --fix`, `remove` and `uninstall` still show installation and hook output. +- `omnishell set` no longer writes a `config.toml` that no command can load + when a value contains a BEL or vertical-tab character: control characters are + now written as `\u00XX`, the escape TOML defines, instead of Go's `\a`, `\v` + and `\x..`. Values that were not valid UTF-8 used to come back as different + characters; they are now rejected with exit code 2 and the file is left + untouched. Values that worked before are written exactly as before. ## [0.6.0] - 2026-10-07 diff --git a/internal/cli/set_toml_test.go b/internal/cli/set_toml_test.go new file mode 100644 index 0000000..d31f658 --- /dev/null +++ b/internal/cli/set_toml_test.go @@ -0,0 +1,67 @@ +package cli_test + +import ( + "bytes" + "os" + "strings" + "testing" + + "github.com/JtheGunner/omnishell/internal/cli" + "github.com/JtheGunner/omnishell/internal/config" +) + +// A control character in a value used to be written with a Go-only escape +// (BEL as \a, VT as \v), after which no command could load config.toml. +func TestSetWithControlCharactersKeepsTheConfigLoadable(t *testing.T) { + cases := map[string]string{"BEL": "a\x07b", "VT": "a\x0bb", "ESC": "a\x1bb", "DEL": "a\x7fb", "NUL": "a\x00b"} + for name, value := range cases { + t.Run(name, func(t *testing.T) { + cfgPath := setTestInit(t) + + var out, errb bytes.Buffer + if code := cli.Execute([]string{"set", "direnv.log_format", value}, &out, &errb); code != 0 { + t.Fatalf("set exit %d: %s", code, errb.String()) + } + + out.Reset() + errb.Reset() + if code := cli.Execute([]string{"list"}, &out, &errb); code != 0 { + t.Fatalf("the next command cannot load the config: exit %d: %s", code, errb.String()) + } + c, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got := c.Modules["direnv"].Options["log_format"]; got != value { + t.Fatalf("log_format = %q, want %q", got, value) + } + }) + } +} + +// A value that is not valid UTF-8 cannot be written to a TOML file; it must be +// refused up front instead of coming back as different characters. +func TestSetWithInvalidUTF8IsRejectedAndLeavesTheConfigUntouched(t *testing.T) { + cfgPath := setTestInit(t) + before, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("read config: %v", err) + } + + var out, errb bytes.Buffer + code := cli.Execute([]string{"set", "direnv.log_format", "a\xffb"}, &out, &errb) + + if code != 2 { + t.Fatalf("exit = %d, want 2 (stderr: %s)", code, errb.String()) + } + if !strings.Contains(errb.String(), "direnv.log_format") || !strings.Contains(errb.String(), "not valid UTF-8") { + t.Fatalf("stderr should name the option and the problem, got: %s", errb.String()) + } + if out.Len() != 0 { + t.Fatalf("nothing to stdout on error, got: %s", out.String()) + } + after, err := os.ReadFile(cfgPath) + if err != nil || !bytes.Equal(before, after) { + t.Fatalf("config.toml must be untouched (err=%v)", err) + } +} diff --git a/internal/config/edit.go b/internal/config/edit.go index 779d234..8ca1317 100644 --- a/internal/config/edit.go +++ b/internal/config/edit.go @@ -32,7 +32,7 @@ func SetEnabled(path, moduleID string, enabled bool) error { func SetOption(path, moduleID, key string, value any) error { lit, err := tomlLiteral(value) if err != nil { - return err + return Error{Path: path, Msg: fmt.Sprintf("invalid value for %s.%s: %v", moduleID, key, err)} } return editTable(path, moduleID, "options", key, lit) } @@ -46,11 +46,15 @@ func tomlLiteral(value any) (string, error) { case int64: return strconv.FormatInt(v, 10), nil case string: - return strconv.Quote(v), nil + return quoteTOML(v) case []string: quoted := make([]string, len(v)) for i, s := range v { - quoted[i] = strconv.Quote(s) + q, err := quoteTOML(s) + if err != nil { + return "", err + } + quoted[i] = q } return "[" + strings.Join(quoted, ", ") + "]", nil default: diff --git a/internal/config/literal_test.go b/internal/config/literal_test.go new file mode 100644 index 0000000..ca87fb5 --- /dev/null +++ b/internal/config/literal_test.go @@ -0,0 +1,253 @@ +package config_test + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/JtheGunner/omnishell/internal/config" +) + +// newConfigFile writes the default config into a temp dir and returns its path. +func newConfigFile(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "config.toml") + if err := os.WriteFile(p, config.RenderDefault(), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// writtenLine returns the line `key = ...` that SetOption wrote into the file. +func writtenLine(t *testing.T, path, key string) string { + t.Helper() + src, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + for _, line := range strings.Split(string(src), "\n") { + if strings.HasPrefix(line, key+" = ") { + return strings.TrimPrefix(line, key+" = ") + } + } + t.Fatalf("no line for %q in:\n%s", key, src) + return "" +} + +// escapesIn returns the character after every backslash in a TOML basic string +// literal, skipping the character that a backslash itself escapes. +func escapesIn(literal string) []byte { + var out []byte + for i := 0; i < len(literal); i++ { + if literal[i] == '\\' && i+1 < len(literal) { + out = append(out, literal[i+1]) + i++ + } + } + return out +} + +// toml10Escapes are the only escape sequences TOML 1.0 defines for basic +// strings: \b \t \n \f \r \" \\ and the two Unicode forms. +const toml10Escapes = `btnfr"\uU` + +func assertOnlyTOMLEscapes(t *testing.T, input, literal string) { + t.Helper() + for _, c := range escapesIn(literal) { + if !strings.ContainsRune(toml10Escapes, rune(c)) { + t.Fatalf("input %q was written as %s, which uses the escape \\%c that TOML 1.0 does not have", input, literal, c) + } + } +} + +// Every ASCII character, in the middle of a value: the file must load again, +// the value must come back unchanged, and only TOML escapes may be written. +func TestSetOptionRoundTripsEveryAsciiCharacter(t *testing.T) { + for r := rune(0); r <= 0x7f; r++ { + input := "a" + string(r) + "b" + p := newConfigFile(t) + + if err := config.SetOption(p, "m", "k", input); err != nil { + t.Fatalf("%q: SetOption: %v", input, err) + } + c, err := config.Load(p) + if err != nil { + t.Fatalf("%q: the written file does not load: %v\nwritten: %s", input, err, writtenLine(t, p, "k")) + } + if got := c.Modules["m"].Options["k"]; got != input { + t.Fatalf("%q: came back as %q", input, got) + } + assertOnlyTOMLEscapes(t, input, writtenLine(t, p, "k")) + } +} + +func TestSetOptionRoundTripsOtherAwkwardCharacters(t *testing.T) { + for _, r := range []rune{0x80, 0x85, 0x9f, 0xa0, 0xad, 0x200b, 0x2028, 0x2029, 0x202e, 0xfeff, 0xfffd, 0x1f600, 0x10ffff} { + input := "a" + string(r) + "b" + p := newConfigFile(t) + + if err := config.SetOption(p, "m", "k", input); err != nil { + t.Fatalf("U+%04X: SetOption: %v", r, err) + } + c, err := config.Load(p) + if err != nil { + t.Fatalf("U+%04X: the written file does not load: %v", r, err) + } + if got := c.Modules["m"].Options["k"]; got != input { + t.Fatalf("U+%04X: came back as %q", r, got) + } + assertOnlyTOMLEscapes(t, input, writtenLine(t, p, "k")) + } +} + +func TestSetOptionRoundTripsListElementsWithControlCharacters(t *testing.T) { + p := newConfigFile(t) + in := []string{"a\x07b", "c\x0bd", "e\x01f", "plain", `q"uo\te`} + + if err := config.SetOption(p, "m", "k", in); err != nil { + t.Fatalf("SetOption: %v", err) + } + c, err := config.Load(p) + if err != nil { + t.Fatalf("the written file does not load: %v\nwritten: %s", err, writtenLine(t, p, "k")) + } + + got, ok := c.Modules["m"].Options["k"].([]any) + if !ok || len(got) != len(in) { + t.Fatalf("list = %#v, want %d elements", c.Modules["m"].Options["k"], len(in)) + } + for i, want := range in { + if got[i] != want { + t.Fatalf("element %d = %q, want %q", i, got[i], want) + } + } + assertOnlyTOMLEscapes(t, fmt.Sprint(in), writtenLine(t, p, "k")) +} + +// What worked before must be written exactly as before, so existing files and +// diffs do not change; only the Go-only escapes are replaced. +func TestSetOptionWritesTheSameLiteralsAsBefore(t *testing.T) { + cases := []struct{ input, literal string }{ + {"--height 40%", `"--height 40%"`}, + {"a\"b\\c\nd\te", `"a\"b\\c\nd\te"`}, + {"\b\f\r", `"\b\f\r"`}, + {"é", `"é"`}, + {"日本語", `"日本語"`}, + {"😀", `"😀"`}, + {"\u200b", `"\u200b"`}, + {"\u2028", `"\u2028"`}, + {"\ufeff", `"\ufeff"`}, + {"\u0085", `"\u0085"`}, + {"\U0010ffff", `"\U0010ffff"`}, + {"", `""`}, + } + for _, c := range cases { + p := newConfigFile(t) + if err := config.SetOption(p, "m", "k", c.input); err != nil { + t.Fatalf("%q: SetOption: %v", c.input, err) + } + if got := writtenLine(t, p, "k"); got != c.literal { + t.Fatalf("%q written as %s, want %s", c.input, got, c.literal) + } + } +} + +// The characters Go writes with escapes TOML lacks become \u00XX. +func TestSetOptionWritesGoOnlyEscapesAsUnicodeEscapes(t *testing.T) { + cases := []struct{ input, literal string }{ + {"\x07", `"\u0007"`}, // BEL, Go: \a + {"\x0b", `"\u000b"`}, // VT, Go: \v + {"\x00", `"\u0000"`}, + {"\x01", `"\u0001"`}, // Go: \x01 + {"\x1b", `"\u001b"`}, // ESC + {"\x1f", `"\u001f"`}, + {"\x7f", `"\u007f"`}, // DEL, Go: \x7f + } + for _, c := range cases { + p := newConfigFile(t) + if err := config.SetOption(p, "m", "k", c.input); err != nil { + t.Fatalf("%q: SetOption: %v", c.input, err) + } + if got := writtenLine(t, p, "k"); got != c.literal { + t.Fatalf("%q written as %s, want %s", c.input, got, c.literal) + } + } +} + +// TOML text must be valid UTF-8; a value that is not cannot be written, and +// silently turning each stray byte into another character would change it. +func TestSetOptionRejectsInvalidUTF8AndLeavesTheFileUntouched(t *testing.T) { + for _, bad := range []string{"a\xffb", "a\xc3b", "\xe2\x82"} { + p := newConfigFile(t) + before, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + + err = config.SetOption(p, "m", "k", bad) + + var cfgErr config.Error + if !errors.As(err, &cfgErr) || !strings.Contains(cfgErr.Msg, "m.k") || !strings.Contains(cfgErr.Msg, "not valid UTF-8") { + t.Fatalf("%q: err = %v, want a config.Error naming m.k and the UTF-8 problem", bad, err) + } + after, err := os.ReadFile(p) + if err != nil || string(before) != string(after) { + t.Fatalf("%q: the file must be untouched (err=%v)", bad, err) + } + } +} + +func TestSetOptionRejectsInvalidUTF8InAListElementAndWritesNothing(t *testing.T) { + p := newConfigFile(t) + before, _ := os.ReadFile(p) + + err := config.SetOption(p, "m", "k", []string{"fine", "bad\xff"}) + + var cfgErr config.Error + if !errors.As(err, &cfgErr) || !strings.Contains(cfgErr.Msg, "not valid UTF-8") { + t.Fatalf("err = %v, want a config.Error about UTF-8", err) + } + if after, _ := os.ReadFile(p); string(before) != string(after) { + t.Fatal("the file must be untouched") + } +} + +// Writing a value with a control character must not disturb the rest of the +// file: comments, other tables and other keys stay as they were. +func TestSetOptionWithAControlCharacterLeavesTheRestOfTheFileAlone(t *testing.T) { + p := filepath.Join(t.TempDir(), "config.toml") + original := `# my omnishell config +[omnishell] +version = 1 + +[modules.direnv] +# keep this comment +enabled = true + +[modules.direnv.options] +whitelist = ["a", "b"] +` + if err := os.WriteFile(p, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + + if err := config.SetOption(p, "direnv", "log_format", "x\x07y"); err != nil { + t.Fatalf("SetOption: %v", err) + } + + got, _ := os.ReadFile(p) + for _, line := range strings.Split(strings.TrimRight(original, "\n"), "\n") { + if !strings.Contains(string(got), line) { + t.Fatalf("line %q is gone:\n%s", line, got) + } + } + if !strings.Contains(string(got), `log_format = "x\u0007y"`) { + t.Fatalf("the new option is missing:\n%s", got) + } + if _, err := config.Load(p); err != nil { + t.Fatalf("the file must still load: %v", err) + } +} diff --git a/internal/config/quote.go b/internal/config/quote.go new file mode 100644 index 0000000..763191f --- /dev/null +++ b/internal/config/quote.go @@ -0,0 +1,68 @@ +package config + +import ( + "errors" + "fmt" + "strconv" + "strings" + "unicode/utf8" +) + +// errInvalidUTF8 is returned for a value TOML cannot represent: a TOML file is +// UTF-8 text, so stray bytes have no spelling that reads back as the same value. +var errInvalidUTF8 = errors.New("value is not valid UTF-8") + +// quoteTOML renders s as a TOML basic string. +// +// It is not strconv.Quote: that writes Go literals, and Go's escapes are not +// TOML's. Go spells BEL as \a, VT as \v and other control characters as \xHH; +// TOML 1.0 has none of these, so a file holding them no longer loads (or, for +// \xHH, only loads with a parser that also knows TOML 1.1). Everything that is +// valid in both languages is written exactly as strconv.Quote writes it, so +// existing files and diffs do not change; the rest becomes \u00XX. +func quoteTOML(s string) (string, error) { + var b strings.Builder + b.Grow(len(s) + 2) + b.WriteByte('"') + for i := 0; i < len(s); { + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size == 1 { + return "", errInvalidUTF8 + } + i += size + + switch r { + case '"': + b.WriteString(`\"`) + case '\\': + b.WriteString(`\\`) + case '\b': + b.WriteString(`\b`) + case '\t': + b.WriteString(`\t`) + case '\n': + b.WriteString(`\n`) + case '\f': + b.WriteString(`\f`) + case '\r': + b.WriteString(`\r`) + default: + switch { + case r < 0x20 || r == 0x7f: + // Control characters TOML forbids unescaped. + fmt.Fprintf(&b, `\u%04x`, r) + case !strconv.IsPrint(r): + // Invisible or unassigned characters stay visible in the file. + if r <= 0xffff { + fmt.Fprintf(&b, `\u%04x`, r) + } else { + fmt.Fprintf(&b, `\U%08x`, r) + } + default: + b.WriteRune(r) + } + } + } + b.WriteByte('"') + return b.String(), nil +}