From 0469ebe473c8e175b5221619b8433ea666661e00 Mon Sep 17 00:00:00 2001 From: NovusEdge Date: Sat, 19 Sep 2026 17:11:31 +0300 Subject: [PATCH] fix(names): validate a VM name before it becomes a directory A VM name goes straight into a directory under the data root. create accepted anything without a space or a slash, so "nul" reached a Windows device and ".." reached the parent. internal/vmname holds the rule. core's create path, project's stoat.toml loader and mcpsrv's tool guards all call it, and each rejection wraps ErrInvalidSpec, which wire reports as invalid_spec. init now writes the same slug Load falls back to. A checkout named "My_Repo" produced a stoat.toml that failed to load. Closes #114 Signed-off-by: NovusEdge --- docs/reference/cli.md | 8 ++++ docs/reference/project-file.md | 9 +++- internal/cli/run_init.go | 5 ++- internal/cli/run_init_test.go | 34 ++++++++++++++ internal/core/core.go | 8 ++-- internal/core/core_test.go | 4 +- internal/mcpsrv/guards.go | 27 +++--------- internal/project/project.go | 31 +++++++++++-- internal/project/project_test.go | 24 ++++++++++ internal/vmname/vmname.go | 76 ++++++++++++++++++++++++++++++++ internal/vmname/vmname_test.go | 65 +++++++++++++++++++++++++++ 11 files changed, 258 insertions(+), 33 deletions(-) create mode 100644 internal/vmname/vmname.go create mode 100644 internal/vmname/vmname_test.go diff --git a/docs/reference/cli.md b/docs/reference/cli.md index dc3914a0..57367740 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -30,6 +30,14 @@ for the documented one-letter forms: `-q`, `-y`, `-n`, and `-h`. For example, `stoat --json --version` is a usage error. Scripts should use the `version` subcommand, which supports `--json` normally. +## VM names + +A VM name becomes a directory under the data root, so `create` and `stoat.toml` hold it to one grammar: letters, digits, dot, dash and underscore, starting with a letter or digit. `stoat` also refuses `CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9` and `LPT1`-`LPT9`, with or without an extension and in any case. Windows resolves each of those to a device at every path level, and a data root is portable. The rule applies on every platform for that reason. + +A rejected name reports `invalid_spec` under `--json` and to MCP. A VM created before this rule keeps working; the check runs at create time only. + +`stoat.toml` is stricter still: a declaration key, a `name` override and `project.name` are lower-case letters, digits and dashes. See [project-file.md](project-file.md). + ## Project scope A `stoat.toml` in the current directory activates project scope. `up`, diff --git a/docs/reference/project-file.md b/docs/reference/project-file.md index 59b16c62..2de86d01 100644 --- a/docs/reference/project-file.md +++ b/docs/reference/project-file.md @@ -57,7 +57,14 @@ See the [sample file](samples/stoat.toml) on its own. A VM's global name is its declaration's `name` field, if set, otherwise `-`. `project.name` defaults to the repository directory -name. +name, lower-cased, with every character outside the grammar replaced by a +dash. + +A declaration key, a `name` override and `project.name` are lower-case +letters, digits and dashes, starting with a letter or a digit. The global +name must also clear the rule in [cli.md](cli.md#vm-names): it becomes a +directory, so a Windows device name such as `nul` is refused there even +though the grammar accepts it. A bare command argument resolves to the declaration key first, then to a global name. `stoat ssh dev` reaches `shared-dev`. diff --git a/internal/cli/run_init.go b/internal/cli/run_init.go index 234cd0a9..99d23b10 100644 --- a/internal/cli/run_init.go +++ b/internal/cli/run_init.go @@ -61,7 +61,10 @@ func runInit(a *Args, stdout, stderr io.Writer) int { name := a.Tag if name == "" { - name = strings.ToLower(filepath.Base(dir)) + name = project.DefaultName(dir) + } + if err := project.ValidateProjectName(name); err != nil { + return a.fail(stdout, stderr, err) } if err := os.WriteFile(path, []byte(initTemplate(name)), 0o644); err != nil { return a.fail(stdout, stderr, err) diff --git a/internal/cli/run_init_test.go b/internal/cli/run_init_test.go index e5f24912..1ae3b938 100644 --- a/internal/cli/run_init_test.go +++ b/internal/cli/run_init_test.go @@ -51,6 +51,40 @@ func TestInitDefaultsTheNameToTheDirectory(t *testing.T) { } } +// project.name prefixes every generated VM directory. init refuses a bad one +// here, where the user typed it, and not at the next command that loads the +// file it wrote. +func TestInitRefusesABadName(t *testing.T) { + cliRoot(t) + for _, name := range []string{"../evil", "My_Repo", "-lead", "a b"} { + dir := t.TempDir() + t.Chdir(dir) + if code, _ := runJSON(t, "init", "--name", name); code == ExitOK { + t.Errorf("init --name %q = ExitOK, want a failure", name) + } + if _, err := os.Stat(filepath.Join(dir, project.FileName)); err == nil { + t.Errorf("init --name %q wrote %s", name, project.FileName) + } + } +} + +// A checkout named "My_Repo" lower-cases to "my_repo", which the name grammar +// rejects. init writes the same slug Load falls back to. +func TestInitSlugsTheDirectoryName(t *testing.T) { + cliRoot(t) + dir := filepath.Join(t.TempDir(), "My_Repo") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + t.Chdir(dir) + if code, _ := runJSON(t, "init"); code != ExitOK { + t.Fatalf("exit = %d", code) + } + if _, err := project.Load(dir); err != nil { + t.Errorf("the file init wrote does not load: %v", err) + } +} + // .stoat holds the recipe cache and the secrets file. Committing either is a // mistake init prevents once, in a git checkout only. func TestInitAppendsTheCacheDirToGitignore(t *testing.T) { diff --git a/internal/core/core.go b/internal/core/core.go index 61fc2786..b628beb8 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -24,6 +24,7 @@ import ( _ "github.com/novusedge/stoat/internal/provider/qemu" "github.com/novusedge/stoat/internal/recipes" "github.com/novusedge/stoat/internal/settings" + "github.com/novusedge/stoat/internal/vmname" ) // Typed errors, because every caller branches on them and string matching is @@ -203,11 +204,8 @@ func Plan(s Spec) (*config.VM, error) { return plan(s) } func plan(s Spec) (*config.VM, error) { name := strings.TrimSpace(s.Name) - if name == "" { - return nil, fmt.Errorf("%w: name is required", ErrInvalidSpec) - } - if strings.ContainsAny(name, "/ ") { - return nil, fmt.Errorf("%w: name cannot contain spaces or slashes", ErrInvalidSpec) + if err := vmname.Validate(name); err != nil { + return nil, err } if config.Exists(name) { return nil, fmt.Errorf("%w: %s", ErrNameTaken, name) diff --git a/internal/core/core_test.go b/internal/core/core_test.go index 8d662444..9467362d 100644 --- a/internal/core/core_test.go +++ b/internal/core/core_test.go @@ -255,7 +255,9 @@ func TestPlanUnknownImage(t *testing.T) { func TestPlanRejectsBadNames(t *testing.T) { dir := root(t) haveImage(t, dir, "alpine-virt-3.24.1-x86_64.iso") - for _, name := range []string{"", " ", "a/b", "a b"} { + // internal/vmname owns the rule and covers each class; this holds the + // wiring and the error code plan reports. + for _, name := range []string{"", " ", "a/b", "a b", `a\b`, "..", "-lead", "nul", "COM1.txt"} { if _, err := plan(Spec{Name: name, Image: "alpine-virt-3.24.1-x86_64.iso"}); !errors.Is(err, ErrInvalidSpec) { t.Errorf("plan(name=%q) err = %v, want ErrInvalidSpec", name, err) } diff --git a/internal/mcpsrv/guards.go b/internal/mcpsrv/guards.go index e80c1d75..c4227cb4 100644 --- a/internal/mcpsrv/guards.go +++ b/internal/mcpsrv/guards.go @@ -13,13 +13,9 @@ import ( "github.com/novusedge/stoat/internal/cli/wire" "github.com/novusedge/stoat/internal/config" "github.com/novusedge/stoat/internal/core" + "github.com/novusedge/stoat/internal/vmname" ) -// A VM name becomes a directory name under the data root, so the pattern is -// what keeps an operation inside it. Rejecting beats sanitizing: a rewrite -// hides the attempt. -var vmNameRE = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) - // A catalog image id never contains a separator. An absolute path here is an // arbitrary host file read, booted as a disk. var imageIDRE = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) @@ -36,24 +32,11 @@ var ( // grants an arbitrary host directory into a guest read-write. var forbiddenPatchKeys = []string{"share", "image", "base", "iso", "console_password"} +// checkVMName keeps an operation inside the data root: the name becomes a +// directory there. func checkVMName(name string) (string, error) { - if strings.TrimSpace(name) == "" { - return "", fmt.Errorf("vm name is required") - } - if name != strings.TrimSpace(name) { - return "", fmt.Errorf("vm name %q has leading or trailing whitespace", name) - } - if name == "." || name == ".." { - return "", fmt.Errorf("vm name %q is a path traversal", name) - } - if strings.ContainsAny(name, `/\`) { - return "", fmt.Errorf("vm name %q contains a path separator", name) - } - if strings.ContainsRune(name, 0) { - return "", fmt.Errorf("vm name contains a null byte") - } - if !vmNameRE.MatchString(name) { - return "", fmt.Errorf("invalid vm name %q: must match %s", name, vmNameRE) + if err := vmname.Validate(name); err != nil { + return "", err } return name, nil } diff --git a/internal/project/project.go b/internal/project/project.go index bd8f6910..7d1103b5 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -11,8 +11,10 @@ import ( "sort" "strings" + "github.com/novusedge/stoat/internal/coreerr" "github.com/novusedge/stoat/internal/settings" "github.com/novusedge/stoat/internal/tomlx" + "github.com/novusedge/stoat/internal/vmname" ) // FileName is the declaration file. Its presence in os.Getwd() is the whole @@ -121,10 +123,10 @@ func Load(dir string) (*Project, error) { p := &Project{Dir: abs, Recipes: f.Recipes, Limits: f.Limits, byKey: make(map[string]VM, len(f.VMs))} p.Name = f.Project.Name if p.Name == "" { - p.Name = slug(filepath.Base(abs)) + p.Name = DefaultName(abs) } - if !nameRE.MatchString(p.Name) { - return nil, fmt.Errorf("%s: project.name %q must match %s", FileName, p.Name, nameRE) + if err := ValidateProjectName(p.Name); err != nil { + return nil, fmt.Errorf("%s: project.name %w", FileName, err) } for _, key := range order(path, f.VMs) { @@ -148,6 +150,10 @@ func Load(dir string) (*Project, error) { seen := map[string]string{} for _, v := range p.VMs { g := p.GlobalName(v.Key) + // nameRE passes "nul", which is a Windows device at every path level. + if err := vmname.Validate(g); err != nil { + return nil, fmt.Errorf("%s: vms.%s: %w", FileName, v.Key, err) + } if first, dup := seen[g]; dup { a, b := sortPair(first, v.Key) return nil, fmt.Errorf("%s: vms.%s and vms.%s both resolve to %q", FileName, a, b, g) @@ -157,6 +163,25 @@ func Load(dir string) (*Project, error) { return p, nil } +// DefaultName is the project name a directory implies, for a file that +// declares none. stoat init writes it and Load falls back to it, so both must +// call this and not lower-case the base name themselves: a checkout called +// "my_repo" lower-cases to a name the grammar rejects. +func DefaultName(dir string) string { return slug(filepath.Base(dir)) } + +// ValidateProjectName reports why name cannot be project.name, or nil. The +// name prefixes every generated VM directory, so stoat init checks it before +// it writes the file, not only Load after the fact. +// +// The device-name rule does not apply here. A prefix never stands alone as a +// directory; GlobalName's result carries that check. +func ValidateProjectName(name string) error { + if !nameRE.MatchString(name) { + return fmt.Errorf("%w: %q must match %s", coreerr.ErrInvalidSpec, name, nameRE) + } + return nil +} + // Find loads the project in the current directory, if there is one. There is // no walk-up: a command's scope must be readable from the directory the user // typed it in, not from a parent three levels up. diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 1bb2333d..82175a65 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -3,6 +3,7 @@ package project import ( "os" "path/filepath" + "strings" "testing" ) @@ -120,6 +121,29 @@ func TestDuplicateGlobalNameIsAnError(t *testing.T) { } } +// The vm key grammar accepts "nul", which is a Windows device at every path +// level. The global name is what becomes a directory, so the check belongs +// there. +func TestReservedGlobalNameIsRejected(t *testing.T) { + dir := write(t, "schema = 1\n\n[project]\nname = \"myrepo\"\n\n[vms.dev]\nname = \"nul\"\nimage = \"a\"\n") + _, err := Load(dir) + if err == nil { + t.Fatal("Load accepted a reserved device name") + } + if !strings.Contains(err.Error(), "reserved device name") { + t.Errorf("err = %q, want it to name the reserved word", err) + } +} + +// A prefix never stands alone as a directory, so the device rule does not +// reach it: "nul-dev" is a fine directory. +func TestReservedProjectNameIsAccepted(t *testing.T) { + dir := write(t, "schema = 1\n\n[project]\nname = \"nul\"\n\n[vms.dev]\nimage = \"a\"\n") + if _, err := Load(dir); err != nil { + t.Fatalf("Load = %v, want a project named nul to load", err) + } +} + func TestUnknownFieldIsRejected(t *testing.T) { dir := write(t, "schema = 1\n\n[vms.dev]\nimage = \"a\"\ncpu = 4\n") if _, err := Load(dir); err == nil { diff --git a/internal/vmname/vmname.go b/internal/vmname/vmname.go new file mode 100644 index 00000000..ac6d7cb3 --- /dev/null +++ b/internal/vmname/vmname.go @@ -0,0 +1,76 @@ +// Package vmname owns the rule for a VM name. A name becomes a directory +// under the data root, and filepath.Base of that directory reads it back, so +// every entry point that accepts a new name validates it here: internal/core's +// create path, internal/project's stoat.toml loader, and internal/mcpsrv's +// tool guards. +// +// The rule applies on every platform. A data root is portable, and a VM +// created on Linux must stay openable on Windows. +package vmname + +import ( + "fmt" + "regexp" + "strings" + + "github.com/novusedge/stoat/internal/coreerr" +) + +// nameRE is the grammar. It excludes an empty name, a leading dash and a +// space by construction; the checks before it name the specific problem +// instead of pointing at the pattern. +var nameRE = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) + +// hint states what passes, appended to every rejection. +const hint = "a name is letters, digits, dot, dash or underscore, and starts with a letter or digit" + +// reserved are the Windows device names. Windows resolves one at every path +// level, with or without an extension, so a VM directory called "nul" opens a +// device instead of a directory. +var reserved = map[string]bool{ + "CON": true, "PRN": true, "AUX": true, "NUL": true, + "COM1": true, "COM2": true, "COM3": true, "COM4": true, "COM5": true, + "COM6": true, "COM7": true, "COM8": true, "COM9": true, + "LPT1": true, "LPT2": true, "LPT3": true, "LPT4": true, "LPT5": true, + "LPT6": true, "LPT7": true, "LPT8": true, "LPT9": true, +} + +// Validate reports why name cannot be a VM name, or nil. Every error wraps +// coreerr.ErrInvalidSpec, which internal/cli/wire reports as invalid_spec. +// +// Validate never rewrites a name. A rewrite hides the attempt from the user +// who typed it. +func Validate(name string) error { + switch { + case strings.TrimSpace(name) == "": + return errf("a vm name is required") + case name != strings.TrimSpace(name): + return errf("vm name %q has leading or trailing whitespace", name) + case name == "." || name == "..": + return errf("vm name %q is a path traversal", name) + case strings.ContainsAny(name, `/\`): + return errf("vm name %q contains a path separator", name) + case strings.ContainsRune(name, 0): + return errf("vm name %q contains a null byte", name) + case strings.HasPrefix(name, "-"): + return errf("vm name %q starts with a dash, which reads as a flag", name) + case isReserved(name): + return errf("vm name %q is a reserved device name on Windows", name) + case !nameRE.MatchString(name): + return errf("vm name %q must match %s", name, nameRE) + } + return nil +} + +// isReserved matches a device name case-insensitively, with or without an +// extension. Windows also drops a trailing dot or space before it resolves a +// path, so "nul." and "nul " reach the same device. +func isReserved(name string) bool { + stem, _, _ := strings.Cut(name, ".") + stem = strings.TrimRight(stem, ". ") + return reserved[strings.ToUpper(stem)] +} + +func errf(format string, args ...any) error { + return fmt.Errorf("%w: %s; %s", coreerr.ErrInvalidSpec, fmt.Sprintf(format, args...), hint) +} diff --git a/internal/vmname/vmname_test.go b/internal/vmname/vmname_test.go new file mode 100644 index 00000000..8a535df0 --- /dev/null +++ b/internal/vmname/vmname_test.go @@ -0,0 +1,65 @@ +package vmname + +import ( + "errors" + "strings" + "testing" + + "github.com/novusedge/stoat/internal/coreerr" +) + +func TestValidateAccepts(t *testing.T) { + for _, name := range []string{ + "work", "Work", "w", "1", "work-1", "work_1", "work.1", "a.b-c_d", + "console", "communicator", "lpt", "com0", "com10", "nullx", + } { + if err := Validate(name); err != nil { + t.Errorf("Validate(%q) = %v; want nil", name, err) + } + } +} + +func TestValidateRejects(t *testing.T) { + cases := []struct { + name string + want string // a fragment of the message that names the problem + }{ + {"", "required"}, + {" ", "required"}, + {"work ", "whitespace"}, + {" work", "whitespace"}, + {".", "path traversal"}, + {"..", "path traversal"}, + {"work/evil", "path separator"}, + {`work\evil`, "path separator"}, + {"/etc/passwd", "path separator"}, + {"work\x00", "null byte"}, + {"-lead", "dash"}, + {"nul", "reserved device name"}, + {"NUL", "reserved device name"}, + {"Nul.txt", "reserved device name"}, + {"con", "reserved device name"}, + {"com1", "reserved device name"}, + {"LPT9.log", "reserved device name"}, + {"nul.", "reserved device name"}, + {".hidden", "must match"}, + {"wörk!", "must match"}, + {"work⁄evil", "must match"}, + } + for _, c := range cases { + err := Validate(c.name) + if err == nil { + t.Errorf("Validate(%q) = nil; want an error", c.name) + continue + } + if !errors.Is(err, coreerr.ErrInvalidSpec) { + t.Errorf("Validate(%q) = %v; want it to wrap ErrInvalidSpec", c.name, err) + } + if !strings.Contains(err.Error(), c.want) { + t.Errorf("Validate(%q) = %q; want it to mention %q", c.name, err, c.want) + } + if !strings.Contains(err.Error(), hint) { + t.Errorf("Validate(%q) = %q; want it to state what a valid name looks like", c.name, err) + } + } +}