diff --git a/internal/apply/process.go b/internal/apply/process.go index e2f37a6..b3c5978 100644 --- a/internal/apply/process.go +++ b/internal/apply/process.go @@ -303,10 +303,11 @@ func unitValue(key, value string) string { // have all vanished is forgotten rather than reported as removed. func removeProcess(ctx context.Context, a store.Applied, run Runner) (string, string, error) { name := a.Target - if name == "" || strings.ContainsAny(name, "/ \t") { - // The name is a unit name and a directory under the mesh's own. One that could climb out - // of either is refused rather than acted on, whatever wrote it into the record. - return "", "", fmt.Errorf("a process recorded under %q cannot be removed by name", name) + if problem := declaration.ProcessNameProblem(name); problem != "" { + // The name is a unit name and a directory under the mesh's own, and what goes is that + // directory, whole. One that could climb out of either — ".." is the mesh's own directory's + // parent — is refused rather than acted on, whatever wrote it into the record. + return "", "", fmt.Errorf("a process recorded under %q cannot be removed by name: %s", name, problem) } found := false for _, unit := range []string{name + ".timer", name + ".service"} { diff --git a/internal/apply/process_test.go b/internal/apply/process_test.go index 93dca73..7f055f7 100644 --- a/internal/apply/process_test.go +++ b/internal/apply/process_test.go @@ -224,3 +224,35 @@ func TestAnUndeclaredProcessIsRemovedWithItsUnitAndBundle(t *testing.T) { t.Errorf("removing a process that is already gone failed: %v", err) } } + +func TestAProcessRecordedUnderAPathlikeNameIsRefusedNotRemoved(t *testing.T) { + // filepath.Join(daemonRoot, "..") is the mesh's own directory, and removal deletes what that + // names, whole. A record is what some host wrote, perhaps under looser rules than today's, so + // the removal holds the name to the declaration's rule again (novox/hq ADR 0118). + root := t.TempDir() + bundles := filepath.Join(root, "daemons") + wasUnits, wasBundles := unitDir, daemonRoot + unitDir, daemonRoot = t.TempDir(), bundles + t.Cleanup(func() { unitDir, daemonRoot = wasUnits, wasBundles }) + precious := filepath.Join(root, "state.json") + if err := os.MkdirAll(filepath.Join(bundles, "other"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(precious, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + for _, name := range []string{"..", ".", "", "-x"} { + known := store.State{Resources: []store.Applied{{ID: "p", Type: "process", Target: name}}} + d := parse(t, `{"declaration":1,"resources":[ + {"id":"f","type":"file","path":"`+filepath.Join(t.TempDir(), "a")+`","content":"a\n"} + ]}`) + if _, _, err := Apply(context.Background(), archHost(t), d, known, store.OriginCarried, noServices, nil, nil); err == nil { + t.Errorf("a process recorded as %q was removed by name", name) + } + for _, still := range []string{precious, filepath.Join(bundles, "other")} { + if _, err := os.Stat(still); err != nil { + t.Fatalf("removing a process recorded as %q took %s with it", name, still) + } + } + } +} diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index 5ba9091..5cc738f 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -575,16 +575,34 @@ func (d *Process) Identity() string { return d.ID } func (d *Process) Kind() Type { return TypeProcess } func (d *Process) Target() string { return d.Name } +// ProcessNameProblem says what is wrong with a process name, or nothing. +// +// The name becomes a unit name, a file under the unit directory and a directory under the mesh's +// own — the one removing the process deletes, whole (novox/hq ADR 0118). So a name that is not one +// plain path element is refused: with a separator it writes somewhere nobody meant, and "." or +// ".." IS the mesh's directory or its parent — removing a process named ".." would delete every +// bundle the mesh has, and more. One with a leading dash is read by the service manager as an +// option, not a unit. Exported because the removal checks the recorded name again: a record is +// what the host wrote, and a host of an older version wrote it under looser rules. +func ProcessNameProblem(name string) string { + switch { + case name == "": + return "a process needs a name, which is what its unit is called" + case strings.ContainsAny(name, "/ \t"): + return "a process name becomes a unit name, so it cannot contain a path separator or a space" + case name == "." || name == "..": + return fmt.Sprintf("a process name becomes a directory under the mesh's own, and %q would be "+ + "that directory or its parent", name) + case strings.HasPrefix(name, "-"): + return "a process name cannot begin with a dash: the service manager would read it as an option" + } + return "" +} + func (d *Process) validate(where string, _ bool) []string { var problems []string - if d.Name == "" { - problems = append(problems, where+": a process needs a name, which is what its unit is called") - } - if strings.ContainsAny(d.Name, "/ \t") { - // It becomes a unit name and a file on disk. A name with a separator in it would write - // somewhere nobody meant. - problems = append(problems, where+": a process name becomes a unit name, so it cannot "+ - "contain a path separator or a space") + if problem := ProcessNameProblem(d.Name); problem != "" { + problems = append(problems, where+": "+problem) } if d.Source == "" { problems = append(problems, where+": a process needs somewhere to fetch its bundle from") diff --git a/internal/declaration/process_test.go b/internal/declaration/process_test.go index c52519f..9a7c7e9 100644 --- a/internal/declaration/process_test.go +++ b/internal/declaration/process_test.go @@ -55,7 +55,10 @@ func TestAProcessMustSayWhatToRun(t *testing.T) { // Its name becomes a unit name and a path, so a separator in it would write somewhere nobody meant. func TestAProcesssNameCannotEscapeItsUnit(t *testing.T) { - for _, bad := range []string{"", "../escape", "two words", "a/b"} { + // "." and ".." are one path element each, and the mesh's own bundle directory and its parent: + // removing a process named ".." would delete every bundle the mesh has, and more (novox/hq ADR + // 0118). A leading dash is an option to the service manager, not a unit. + for _, bad := range []string{"", "../escape", "two words", "a/b", ".", "..", "-", "--now"} { d := aProcess() d.Name = bad if problems := d.validate("a process", false); len(problems) == 0 {