fix(names): validate a VM name before it becomes a directory

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`,

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>-<key>`. `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`.

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)

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) {

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)

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)
 		}

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
 }

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.

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 {

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)
+}