Step back from a rendered resolver file only, and compare a declared path cleaned (issue 412, review 80)

A resource on the resolver file was left out by the check and still composed by the declaration, so an
uplink declaring it as a resource beside the resolver passed with two writers; it is refused again, as
before. And /etc//hosts or /etc/../etc/hosts is /etc/hosts to the machine, so it is compared cleaned.
A seat names no root or top-level directory.

internal/catalogue/node_resolver.go

@@ -1,5 +1,7 @@
 package catalogue
 
+import "path"
+
 // The machine's own resolver (novox/hq ADR 0247).
 //
 // **Most machines have none.** Every machine lists the mesh's resolvers in /etc/resolv.conf and nothing
@@ -66,10 +68,13 @@ func holdsSeat(m Manifest, seat string) bool {
 	return false
 }
 
-// stepsBack is the module as it composes among the modules of one machine (novox/hq ADR 0287, ADR 0247):
+// stepsBack is the module as it composes among the modules of one machine (novox/hq ADR 0287,
+// ADR 0247):
 // **where it holds a seat that names a path stepping back for another seat, and that other seat's holder
-// is on the machine and declares the path, its own declaration of the path is left out**, and nothing else
-// of it changes. The stepping back is the seat's definition — node-uplink stepping back from the resolver
+// is on the machine and declares the path, its own rendering of the path is left out**, and nothing else
+// of it changes. Only a file the mesh renders for it steps back, as before: a resource on the path is
+// composed as declared (the declaration copies a module's resources whole), so an uplink holder declaring
+// the resolver file as a resource beside the resolver's holder is two owners of one path, and refused. The stepping back is the seat's definition — node-uplink stepping back from the resolver
 // file for node-resolver — so any other module declaring the path beside the owner's is still two owners
 // of one path, and is refused as before.
 func stepsBack(m Manifest, modules []Manifest) Manifest {
@@ -95,20 +100,12 @@ func stepsBack(m Manifest, modules []Manifest) Manifest {
 	}
 	facts := make(map[string]RosterFile, len(m.Facts))
 	for name, f := range m.Facts {
-		if !f.Home && drop[f.Path] {
+		if !f.Home && drop[path.Clean(f.Path)] {
 			continue
 		}
 		facts[name] = f
 	}
 	m.Facts = facts
-	var resources []map[string]any
-	for _, r := range m.Resources {
-		if p, ok := r["path"].(string); ok && drop[p] {
-			continue
-		}
-		resources = append(resources, r)
-	}
-	m.Resources = resources
 	return m
 }
 

internal/catalogue/seat_paths.go

@@ -45,6 +45,10 @@ func seatPathProblem(seat, scope, p string) string {
 	case !strings.HasPrefix(p, "/") || path.Clean(p) != p:
 		return fmt.Sprintf("the seat %s names the path %q, which is not a clean absolute path (novox/hq ADR 0287)",
 			seat, p)
+	case path.Dir(p) == "/":
+		// The root or a top-level directory is every role's, never one role's own (ADR 0287).
+		return fmt.Sprintf("the seat %s names the path %q, the root or a top-level directory, which no one "+
+			"role owns (novox/hq ADR 0287)", seat, p)
 	}
 	return ""
 }
@@ -113,16 +117,20 @@ func namesWithoutSteppingBack(s Seat, p string) bool {
 
 // declaredPaths is every machine path a module declares it owns: a resource's literal path, and a file
 // the mesh renders for it outside the operator's home. An access is not here — reading is not owning.
+//
+// **Cleaned, because the machine writes the cleaned path**: /etc//hosts, /etc/./hosts, /etc/../etc/hosts and
+// /etc/hosts/ are all /etc/hosts to it, and a seat's path is clean by its own rule, so a declaration compared
+// as written would pass for a path nobody named.
 func declaredPaths(m Manifest) map[string]bool {
 	out := map[string]bool{}
 	for _, r := range m.Resources {
 		if p, ok := r["path"].(string); ok && strings.HasPrefix(p, "/") {
-			out[p] = true
+			out[path.Clean(p)] = true
 		}
 	}
 	for _, f := range m.Facts {
 		if !f.Home && strings.HasPrefix(f.Path, "/") {
-			out[f.Path] = true
+			out[path.Clean(f.Path)] = true
 		}
 	}
 	return out

internal/catalogue/seat_paths_test.go

@@ -137,6 +137,12 @@ func TestOnlyAClaimantDeclaresASeatsPath(t *testing.T) {
 	if !strings.Contains(got, `stranger declares the path "/etc/hosts", which the seat node-hostname names`) {
 		t.Errorf("a module declaring /etc/hosts without claiming node-hostname was not refused: %q", got)
 	}
+	// However the path is spelled (novox/hq issue 412 review).
+	unclean := Manifest{Module: "unclean", Version: "1", Resources: []map[string]any{
+		{"id": "hosts", "type": "file", "path": "/etc/../etc/hosts", "content": "10.0.0.1 x\n"}}}
+	if got := strings.Join(CatalogueProblems(Shelf{"unclean": unclean}), "; "); !strings.Contains(got, "node-hostname") {
+		t.Errorf("/etc/../etc/hosts from a module not claiming node-hostname was not refused: %q", got)
+	}
 	reader := Manifest{Module: "reader", Version: "1", Accesses: []Access{{ID: "hosts", Path: "/etc/hosts"}}}
 	if got := CatalogueProblems(Shelf{"reader": reader}); len(got) != 0 {
 		t.Errorf("reading /etc/hosts was refused: %v", got)
@@ -237,19 +243,56 @@ func TestPathOwnersApplyTheSteppingBack(t *testing.T) {
 	}
 }
 
-// The stepping back comes from the seat: an uplink holder declaring the resolver file as a resource rather
-// than a rendered file steps back too, and keeps everything else.
-func TestTheUplinkStepsBackByTheSeatsDefinition(t *testing.T) {
-	uplink := Manifest{Module: "uplink", Version: "1", Claims: []Claim{{Name: "node-uplink"}},
-		Resources: []map[string]any{
-			{"id": "resolvers", "type": "file", "path": ResolverFile, "content": "nameserver 10.42.0.1\n"},
-			{"id": "conf", "type": "file", "path": "/etc/uplink.conf", "content": "x"}}}
-	got := stepsBack(uplink, []Manifest{uplink, localResolver()})
-	if len(got.Resources) != 1 || got.Resources[0]["id"] != "conf" {
-		t.Errorf("the uplink's holder composes %v beside the resolver", got.Resources)
+// The stepping back comes from the seat, and is of a rendered file only: an uplink holder declaring the
+// resolver file as a resource beside a node-resolver holder is two writers of one path, refused when the
+// machine is resolved — never composed twice — as before the seat named its paths.
+func TestAnUplinkDeclaringTheResolverFileAsAResourceIsRefusedBesideTheResolver(t *testing.T) {
+	shelf := resolverShelf(t)
+	uplink := shelf["systemd-networkd"]
+	uplink.Facts = nil
+	uplink.Resources = append(append([]map[string]any(nil), uplink.Resources...),
+		map[string]any{"id": "resolvers", "type": "file", "path": ResolverFile, "content": "nameserver 10.42.0.1\n"})
+	shelf["systemd-networkd"] = uplink
+	shelf["local-resolver"] = localResolver()
+	_, err := Resolve(shelf, []string{"dnsmasq", "systemd-networkd", "local-resolver"}, splitDNSLaptop(), World{})
+	if err == nil || !strings.Contains(err.Error(), `both declare the path "/etc/resolv.conf"`) {
+		t.Fatalf("an uplink declaring the resolver file as a resource beside the resolver was not refused: %v", err)
 	}
-	if p := checkResources([]Manifest{uplink, localResolver()}); len(p) != 0 {
-		t.Errorf("the uplink's holder and the resolver's were refused together: %v", p)
+	// Its rendered file steps back, by the seat's definition, and nothing else of it does.
+	rendered := Manifest{Module: "uplink", Version: "1", Claims: []Claim{{Name: "node-uplink"}},
+		Facts: map[string]RosterFile{"resolvers": {Path: "/etc//resolv.conf", Template: "x"},
+			"other": {Path: "/etc/uplink.conf", Template: "x"}}}
+	got := stepsBack(rendered, []Manifest{rendered, localResolver()})
+	if len(got.Facts) != 1 || got.Facts["other"].Path == "" {
+		t.Errorf("the uplink's holder renders %v beside the resolver", got.Facts)
+	}
+}
+
+// A path is compared as the machine writes it: cleaned. Each spelling of /etc/hosts is /etc/hosts.
+func TestAnUncleanSpellingOfASeatsPathIsTheSeatsPath(t *testing.T) {
+	for _, spelled := range []string{"/etc//hosts", "/etc/./hosts", "/etc/../etc/hosts", "/etc/hosts/"} {
+		stranger := Manifest{Module: "stranger", Version: "1", Resources: []map[string]any{
+			{"id": "hosts", "type": "file", "path": spelled, "content": "10.0.0.1 x\n"}}}
+		if got := strings.Join(CatalogueProblems(Shelf{"stranger": stranger}), "; "); !strings.Contains(got,
+			`stranger declares the path "/etc/hosts", which the seat node-hostname names`) {
+			t.Errorf("%s from a module not claiming node-hostname was not refused: %q", spelled, got)
+		}
+		holder := hostnameHolder(map[string]any{"id": "hosts", "type": "file", "path": spelled, "content": "x"},
+			map[string]any{"id": "name", "type": "file", "path": "/etc/hostname", "content": "x"})
+		if err := CanHold(holder, hostnameSeat(t)); err != nil {
+			t.Errorf("a holder declaring %s was refused: %v", spelled, err)
+		}
+	}
+}
+
+// A seat names a role's own path, never the root or a top-level directory.
+func TestASeatNamesNoRootOrTopLevelDirectory(t *testing.T) {
+	for _, p := range []string{"/", "/etc"} {
+		m := Manifest{Module: "a", Version: "1", DefinesSeats: []SeatDeclaration{
+			{Name: "a-seat", Scope: ScopeNode, Paths: []string{p}}}}
+		if got := strings.Join(declaredSeatProblems(m), "; "); !strings.Contains(got, "top-level directory") {
+			t.Errorf("a module's seat naming %s was accepted: %q", p, got)
+		}
 	}
 }