Replace the duplicated withinDir path-containment helpers in internal...

[colonelpanik]

Goal

Replace the duplicated withinDir path-containment helpers in internal/grpcapi/vmimport.go:818-824 and internal/libvirt/vtpmstate.go:373 with the existing internal/safename.Contains, removing both local copies.

Plan

Fold the duplicated withinDir helpers into safename.Contains — Implementation Plan

For agentic workers: REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (- [ ]) syntax for tracking.

Goal: Delete the two local withinDir path-containment helpers (internal/grpcapi/vmimport.go:818-824, internal/libvirt/vtpmstate.go:373-379) and route every call site through the existing internal/safename.Contains.

Architecture: Pure consolidation, no new abstraction. safename.Contains already exists, is already the implementation SafeJoin builds on, and its doc comment already names these copies as the duplication it generalizes. Both local copies are byte-for-byte equivalent to it (proof in Reviewer notes below), so every call site is a mechanical substitution. internal/safename is a stdlib-only leaf package, so neither import can create a cycle.

Tech Stack: Go 1.26, standard go test. No new dependencies.

Global Constraints

• No behavior change at any call site. Contains and withinDir compute the identical filepath.Rel-based check. If any test needs a new assertion to stay green, stop — that means the equivalence claim is wrong.
• No new tests beyond existing tests continuing to pass. One existing test (internal/grpcapi.TestWithinDir) calls the removed helper directly and therefore moves; moving it is not adding coverage.
• Both local copies must be gone when the change lands. grep -rn "withinDir" --include=*.go . returns zero hits at the end.
• Out of scope, deliberately left alone: internal/vmimport/ova.go:24 (safeJoin), internal/storage/btrfs.go:116, internal/safename/tar.go:211. See Reviewer notes.
• Conventional-commit style, scope matching the package touched.

---

File Structure

File	Change
internal/libvirt/vtpmstate.go	Call site at :166 → safename.Contains; delete local withinDir (:373-379). safename is already imported (:11).
internal/grpcapi/vmimport.go	Add safename import; call sites at :462, :787, :808 → safename.Contains; delete local withinDir (:818-824).
internal/grpcapi/migrate.go	Call sites at :60, :73 → safename.Contains. safename is already imported (:28).
internal/grpcapi/vmimport_test.go	Delete TestWithinDir (:86-102) — it tests a helper this change removes.
internal/safename/safename_test.go	Receive that test's table as TestContains_Table, next to the existing TestContains.
internal/safename/path.go	Update the SafeJoin doc comment (:9-12), which names the copies being removed.

No files created. No files deleted.

---

Task 1: internal/libvirt — drop the vtpmstate copy

Start here: this package already imports safename, so it is the smallest self-contained slice and proves the leaf-package claim before the larger edit.

Files:

• Modify: internal/libvirt/vtpmstate.go:166, internal/libvirt/vtpmstate.go:373-379
• Test: internal/libvirt/vtpmstate_test.go (existing, unmodified)

Interfaces:

• Consumes: safename.Contains(dir, path string) bool — already in internal/safename/path.go:28.

• Produces: nothing new. internal/libvirt.withinDir ceases to exist.

• Step 1: Confirm internal/safename is a leaf package (no import cycle)

Run:

go list -f '{{.ImportPath}}: {{join .Imports " "}}' ./internal/safename

Expected — stdlib only, no github.com/litevirt/litevirt/... entry:

github.com/litevirt/litevirt/internal/safename: archive/tar errors fmt io os path/filepath regexp strings syscall time

If any litevirt import appears, stop and report — the cycle premise is broken.

• Step 2: Establish the green baseline

Run: go test ./internal/libvirt/
Expected: ok github.com/litevirt/litevirt/internal/libvirt

• Step 3: Switch the call site

internal/libvirt/vtpmstate.go:166 — replace:

			if !withinDir(swtpmStage, dst) {

with:

			if !safename.Contains(swtpmStage, dst) {

• Step 4: Delete the local helper

Remove these seven lines in full (internal/libvirt/vtpmstate.go:373-379), including the blank line that separated them from fsyncPath:

func withinDir(dir, path string) bool {
	rel, err := filepath.Rel(dir, path)
	if err != nil {
		return false
	}
	return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)))
}

fsyncPath (:364-371) becomes the last function in the file.

Do not touch the import block. strings is still used at :131, :144, :161, :293; os and filepath are used throughout. The build will tell you if that is wrong.

• Step 5: Build and test

Run:

gofmt -l internal/libvirt/ && go build ./internal/libvirt/ && go test ./internal/libvirt/

Expected: no gofmt output, no build output, ok github.com/litevirt/litevirt/internal/libvirt.

If you instead get "strings" imported and not used, you deleted more than the seven lines above — revert the import block and re-check.

• Step 6: Run the firmware-bundle tests explicitly

Run:

go test ./internal/libvirt/ -run 'TestFirmwareBundle_RoundTrip|TestReadFirmwareBundle_RejectsSlip|TestRestore_OverExistingState|TestWipeFirmwareState' -v

Expected: four --- PASS lines.

Note for whoever reads the diff: the check you just rewired at :166 is defense-in-depth and not reachable through ReadFirmwareBundle's inputs — the earlier guard at :127-133 rejects absolute and ..-escaping member names, and rel is derived from an already-cleaned, already-non-escaping name, so dst cannot leave swtpmStage. TestReadFirmwareBundle_RejectsSlip's "escaping swtpm path" case (swtpm/../../etc/evil) cleans to ../etc/evil and dies at :131. That is why Task 2, not this task, carries the mutation check.

• Step 7: Commit

git add internal/libvirt/vtpmstate.go
git commit -m "refactor(libvirt): use safename.Contains for swtpm stage containment"

---

Task 2: internal/grpcapi — drop the vmimport copy and relocate its test

Files:

• Modify: internal/grpcapi/vmimport.go (import block, :462, :787, :808, :818-824)
• Modify: internal/grpcapi/migrate.go:60, internal/grpcapi/migrate.go:73
• Modify: internal/grpcapi/vmimport_test.go:86-102 (delete TestWithinDir)
• Modify: internal/safename/safename_test.go (add TestContains_Table after the existing TestContains)

Interfaces:

• Consumes: safename.Contains(dir, path string) bool.

• Produces: nothing new. internal/grpcapi.withinDir ceases to exist; internal/grpcapi.(*Server).withinDiskArtifactRoot keeps its exact signature and semantics.

• Step 1: Baseline the package

Run: go test ./internal/grpcapi/
Expected: ok github.com/litevirt/litevirt/internal/grpcapi — takes roughly two minutes, that is normal for this package.

• Step 2: Add the import to vmimport.go

internal/grpcapi/vmimport.go — in the third import group, insert safename between qcow2 and tenancy (gofmt sorts by path, so alias lv does not affect ordering):

	"github.com/google/uuid"
	pb "github.com/litevirt/litevirt/gen/litevirt/v1"
	"github.com/litevirt/litevirt/internal/corrosion"
	lv "github.com/litevirt/litevirt/internal/libvirt"
	"github.com/litevirt/litevirt/internal/qcow2"
	"github.com/litevirt/litevirt/internal/safename"
	"github.com/litevirt/litevirt/internal/tenancy"
	"github.com/litevirt/litevirt/internal/vmimport"
	"log/slog"

The stray "log/slog" at the end of the group is pre-existing — leave it exactly where it is.

• Step 3: Switch the three vmimport.go call sites

:462, inside resolveStagedPath — replace:

	if !withinDir(stagingRoot, resolved) {

with:

	if !safename.Contains(stagingRoot, resolved) {

:787, inside the VMDK descriptor scan — replace:

			if filepath.IsAbs(ext) || strings.Contains(ext, "..") || !withinDir(allowedDir, filepath.Join(allowedDir, ext)) {

with:

			if filepath.IsAbs(ext) || strings.Contains(ext, "..") || !safename.Contains(allowedDir, filepath.Join(allowedDir, ext)) {

:808, inside the qemu-img info backing-file check — replace:

			if !withinDir(allowedDir, resolved) {

with:

			if !safename.Contains(allowedDir, resolved) {

• Step 4: Delete the local helper

internal/grpcapi/vmimport.go:818-824 — remove these seven lines, leaving the // ── small helpers ── banner at :816 in place (it still heads fileExists, readHead, and findByExt):

func withinDir(dir, path string) bool {
	rel, err := filepath.Rel(dir, path)
	if err != nil {
		return false
	}
	return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)))
}

fileExists becomes the first helper under the banner. Do not touch the import block again: os is still used by fileExists/readHead/findByExt, strings by :474 and :787, filepath throughout.

• Step 5: Switch the two migrate.go call sites

internal/grpcapi/migrate.go:59-77, withinDiskArtifactRoot. Replace :60:

	if withinDir(filepath.Join(s.dataDir, "disks"), p) {

with:

	if safename.Contains(filepath.Join(s.dataDir, "disks"), p) {

and :73:

		if dir, derr := fileBasedPoolDir(s.dataDir, pr); derr == nil && withinDir(dir, p) {

with:

		if dir, derr := fileBasedPoolDir(s.dataDir, pr); derr == nil && safename.Contains(dir, p) {

migrate.go already imports safename at :28 — no import edit here.

• Step 6: Move the table test to internal/safename

Delete internal/grpcapi/vmimport_test.go:86-102 in full:

func TestWithinDir(t *testing.T) {
	d := "/srv/imports/x"
	cases := []struct {
		path string
		want bool
	}{
		{"/srv/imports/x/disk.qcow2", true},
		{"/srv/imports/x", true},
		{"/srv/imports/x/../y/disk.qcow2", false},
		{"/etc/passwd", false},
	}
	for _, c := range cases {
		if got := withinDir(d, filepath.Clean(c.path)); got != c.want {
			t.Errorf("withinDir(%q, %q) = %v, want %v", d, c.path, got, c.want)
		}
	}
}

Then add this to internal/safename/safename_test.go, immediately after the existing TestContains (which ends at :144) and before the // --- tar extraction --- section marker at :146 — it belongs with the path tests, not the tar tests:

func TestContains_Table(t *testing.T) {
	d := "/srv/imports/x"
	cases := []struct {
		path string
		want bool
	}{
		{"/srv/imports/x/disk.qcow2", true},
		{"/srv/imports/x", true},
		{"/srv/imports/x/../y/disk.qcow2", false},
		{"/etc/passwd", false},
	}
	for _, c := range cases {
		if got := Contains(d, c.path); got != c.want {
			t.Errorf("Contains(%q, %q) = %v, want %v", d, c.path, got, c.want)
		}
	}
}

Two deliberate differences from the original, both no-ops for these inputs:

• package-local Contains instead of withinDir (same package now, so no qualifier);
• the caller's filepath.Clean(c.path) is dropped, because Contains cleans internally. /srv/imports/x/../y/disk.qcow2 still yields rel == "../y/disk.qcow2" → false, identical to before.

After deleting TestWithinDir, check whether filepath is still used elsewhere in vmimport_test.go — it is (:67, :78, :80, :107), so its import stays. The build is the authority.

• Step 7: Build, vet, format, test

Run:

gofmt -l internal/grpcapi/ internal/safename/ && go build ./... && go vet ./internal/grpcapi/ ./internal/safename/ && go test ./internal/safename/ ./internal/grpcapi/

Expected: no gofmt output, no build or vet output, then

ok  	github.com/litevirt/litevirt/internal/safename
ok  	github.com/litevirt/litevirt/internal/grpcapi

• Step 8: Mutation-verify the wiring (this is the step that proves the change)

Everything above is a substitution, so the risk is not "wrong logic", it is "call site silently no longer consulted". Break Contains and confirm red.

Temporarily insert return true as the first statement of Contains in internal/safename/path.go — keep the rest of the body:

func Contains(dir, path string) bool {
	return true // MUTATION - revert
	dirClean := filepath.Clean(dir)
	rel, err := filepath.Rel(dirClean, filepath.Clean(path))
	if err != nil {
		return false
	}
	return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)))
}

Insert it, do not replace the final return — line 34 is the only use of strings in path.go, and replacing it fails the build with "strings" imported and not used instead of failing the test, which proves nothing.

Run:

go test ./internal/safename/ -run 'TestContains|TestSafeJoin'
go test ./internal/grpcapi/ -run 'TestAssertNoExternalDiskRefs_RejectsExternalBacking|TestConvertForeignDisk_VMDKRoundTrip'

Expected: both FAIL. TestContains/TestContains_Table/TestSafeJoin fail on the sibling/parent/escape cases; TestAssertNoExternalDiskRefs_RejectsExternalBacking fails with expected rejection of external backing file, got nil, which is the :808 call site you just rewired reporting that it is genuinely reached. (go vet will now also warn unreachable code — expected, it is the mutation.)

That grpcapi test needs a real qemu-img on PATH. Confirm with which qemu-img first; if it is missing the test passes vacuously and this step is worthless — install it or say so in the handoff rather than recording a pass.

Now revert the mutation and re-run both commands. Expected: both ok.

• Step 9: Commit

git add internal/grpcapi/vmimport.go internal/grpcapi/migrate.go internal/grpcapi/vmimport_test.go internal/safename/safename_test.go
git commit -m "refactor(grpcapi): use safename.Contains for path containment"

---

Task 3: Retire the stale SafeJoin doc comment and gate the whole thing

Files:

• Modify: internal/safename/path.go:9-12

Interfaces: none — comment text and repo-wide verification only.

• Step 1: Update the doc comment

internal/safename/path.go:9-12 names the copies this change just deleted, so it is now wrong. Replace:

// SafeJoin joins root with the given path parts and guarantees the result stays
// within root. It rejects any combination (absolute parts, ".." traversal) that
// would escape, and returns the cleaned path. This generalizes the per-package
// withinDir/safeJoin helpers (internal/grpcapi/vmimport.go, internal/vmimport).

with:

// SafeJoin joins root with the given path parts and guarantees the result stays
// within root. It rejects any combination (absolute parts, ".." traversal) that
// would escape, and returns the cleaned path. Together with Contains this is the
// single implementation of path containment for callers that need a boolean
// check; the former per-package withinDir copies in internal/grpcapi and
// internal/libvirt now call Contains. internal/vmimport/ova.go still keeps its
// own tar-member safeJoin.

The last sentence is load-bearing for the next person: internal/vmimport/ova.go:24 is a different helper (returns (string, error) for tar members, rejects rel == ".") and was left alone on purpose. Do not silently drop that note.

• Step 2: Prove both copies are gone

Run:

grep -rn "withinDir" --include='*.go' . ; echo "exit=$?"

Expected: no matching lines and exit=1. Any hit means a call site or definition was missed.

• Step 3: Full pre-push gate (per CLAUDE.md)

Run each, in order, and read the output:

go build ./... && go vet ./...
go test ./...
make ci-guards

Expected: no build/vet output; every go test line ok or no test files; make ci-guards completes with all sub-checks passing. None of the guards should be affected by this change — no SQL builders, no schema version, no new operator-facing flags or config keys — so any ci-guards failure is either pre-existing on main or a real surprise. If one fails, re-run it on a clean main checkout before assuming this change caused it, and report either way.

• Step 4: Commit

git add internal/safename/path.go
git commit -m "docs(safename): point SafeJoin's comment at the finished consolidation"

---

Reviewer notes

1. The equivalence is exact, not approximate. Both removed copies were:

rel, err := filepath.Rel(dir, path)
if err != nil { return false }
return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)))

safename.Contains differs in exactly two cosmetic ways, and neither changes any result:

• It calls filepath.Clean on both arguments first. filepath.Rel already does this internally — Go 1.26 src/path/filepath/path.go opens Rel with base := Clean(basePath); targ := Clean(targPath) — so the extra Clean cannot change rel.
• It writes filepath.Separator where the copies wrote os.PathSeparator. path/filepath declares Separator = os.PathSeparator, so these are the same constant.

Same error handling (Rel errors → false), same "." acceptance, same .. rejection. No call site changes behavior, which is why no call site needed a new assertion.

2. The change is wider than the two lines in the goal. internal/grpcapi.withinDir had five callers, not three: vmimport.go:462, :787, :808 and migrate.go:60, :73 (withinDiskArtifactRoot, which bounds migration's create/remove to the disks dir and file-backed pool dirs so it can never touch {dataDir}/state.db). Removing the helper forces migrate.go to move too. Worth checking that the reviewer expected five.

3. One existing test had to move, and it is the only test file change. internal/grpcapi.TestWithinDir called the removed helper directly, so it could not stay. It is relocated verbatim (bar the two documented no-ops) into internal/safename as TestContains_Table. Net coverage is unchanged — no assertion added, none dropped.

4. Coverage of the rewired call sites is uneven, and the plan does not pretend otherwise.

Call site	Covered by a test that goes red if Contains breaks?
grpcapi/vmimport.go:808	Yes — TestAssertNoExternalDiskRefs_RejectsExternalBacking (needs real qemu-img; present here at /usr/bin/qemu-img). This is the mutation target in Task 2 Step 8.
grpcapi/vmimport.go:787	Partly — TestConvertForeignDisk_VMDKRoundTrip exercises the descriptor path; the filepath.IsAbs/strings.Contains(ext, "..") terms short-circuit most escapes before Contains is consulted.
grpcapi/vmimport.go:462	No direct unit test (resolveStagedPath gates on RBAC role).
grpcapi/migrate.go:60, :73	No direct unit test for withinDiskArtifactRoot; reached only via MigrateVM/disk-stub paths.
libvirt/vtpmstate.go:166	No — and it is unreachable by construction, see below.

5. vtpmstate.go:166 is unreachable defense-in-depth. ReadFirmwareBundle rejects absolute member names at :127 and ..-escaping cleaned names at :131 before the switch. By the time :165 computes dst = filepath.Join(swtpmStage, rel), rel derives from an already-cleaned non-escaping name, so dst provably cannot leave swtpmStage. TestReadFirmwareBundle_RejectsSlip's swtpm/../../etc/evil case cleans to ../etc/evil and is caught at :131 — it never reaches the line this plan rewires. So a mutated Contains will not turn that test red, and the plan does not claim it will. The line is kept (not deleted) because it is a cheap invariant guard, and CLAUDE.md's "mutation-verify anything you assert" is satisfied through the grpcapi site, which is genuinely reachable.

6. Import-cycle risk is nil, and Task 1 Step 1 checks it rather than assuming. internal/safename imports only stdlib (archive/tar errors fmt io os path/filepath regexp strings syscall time) — a true leaf. internal/libvirt/vtpmstate.go:11 already imports it, so Task 1 adds no import at all; internal/grpcapi already imports it from a dozen files, so Task 2 only adds it to one more file in the same package.

7. Three near-identical helpers survive, on purpose. The goal names two copies; these are not them:

• internal/vmimport/ova.go:24 safeJoin — different shape ((string, error), tar-member semantics, treats rel == "." as an error). Replacing it means using safename.SafeJoin, a separate change with its own behavioral review.
• internal/safename/tar.go:211 — inside safename itself, tar-specific.
• internal/storage/btrfs.go:116 — uses strings.HasPrefix(rel, "..") without the separator, so it is subtly over-broad: it would reject a legitimate sibling-free path whose first component starts with .. (e.g. ..data/x). That is a real (if harmless-in-practice) divergence, not an equivalent copy, and swapping it to Contains would be a behavior change — exactly what this plan's constraints forbid. Flagged here so it is a known follow-up rather than an oversight.

8. Nothing operator-facing changes, so the docs triangulation guard (cmd/litevirt/docs_triangulation_test.go) is unaffected: no command, flag, config key, or litevirt_* identifier is added or removed. grep -rn withinDir docs/ is already empty. make ci-guards is still run in Task 3 because the repo asks for it before every push, not because this change plausibly trips it.

9. Baselines measured in this worktree before planning, so the "expected" outputs above are real, not guessed: go test ./internal/safename/ → ok 0.008s; go test ./internal/libvirt/ → ok 0.020s; go test ./internal/grpcapi/ → ok 120.324s; TestWithinDir, TestAssertNoExternalDiskRefs_RejectsExternalBacking, TestConvertForeignDisk_VMDKRoundTrip all currently PASS.

---

Deviations from the plan as executed

The plan's substance held up — every equivalence claim, line number, call-site
count (five, not three), and mutation prediction was correct. Three procedural
deviations:

1. No commits made. Task 1 Step 7, Task 2 Step 9, and Task 3 Step 4 were
skipped: the execution instructions stated committing is handled externally. All
six files are left modified in the working tree as one reviewable change.

2. gofmt -l is not empty, and that is pre-existing. Task 1 Step 5 and Task
2 Step 7 expect no gofmt output. The repo has 13 gofmt-unclean files on
de655ea — 4 under internal/libvirt/, 8 under internal/grpcapi/, and
internal/safename/safename_test.go (misaligned map literal in
TestProjectRBACPath, ~:68). Each was verified unformatted at HEAD before any
edit. None of the six files this change touches is newly unformatted, and the
added TestContains_Table is gofmt-clean. They were left alone deliberately:
reformatting unrelated files would bury a 30-line refactor in whitespace noise
for the reviewer. Worth a separate chore: gofmt pass.

3. Task 3 Step 2's exit=1 expectation contradicts Task 3 Step 1. Step 1
instructs writing a SafeJoin doc comment that contains the word withinDir
("the former per-package withinDir copies…"), so the Step 2 grep cannot return
zero hits. Final state is one hit —
internal/safename/path.go:13, that new comment. Both definitions and all
five call sites are gone, which is what the Global Constraint actually means;
grep -rn 'func withinDir\|withinDir(' --include='*.go' . returns nothing.

Verification actually run (not asserted)

Check	Result
go list on internal/safename	stdlib only, exactly as predicted — leaf confirmed
go test ./internal/libvirt/	ok 0.020s
Firmware-bundle tests, -v	4 × --- PASS
go build ./... && go vet ./...	clean
go test ./...	every package ok or no test files; zero failures
make ci-guards	exit 0 — schema-bump, ledger-drift, writecheck, stmtshapecheck (336 builders), docs triangulation all OK

Mutation check (Task 2 Step 8) performed as written and it worked.
qemu-img confirmed at /usr/bin/qemu-img before running, so the grpcapi test
was not vacuous. With return true inserted at the top of Contains:

• TestContains, TestContains_Table, TestSafeJoin → FAIL. The relocated
  table failed on both escape cases (../y/disk.qcow2 and /etc/passwd),
  proving the move preserved its teeth.
• TestAssertNoExternalDiskRefs_RejectsExternalBacking → FAIL with
  expected rejection of external backing file, got nil — verbatim the predicted
  message, so the rewired vmimport.go backing-file call site is genuinely
  reached.
• TestConvertForeignDisk_VMDKRoundTrip passed under mutation, consistent with
  Reviewer note 4 rating that site "partly" covered.

Mutation reverted; both commands re-run and ok.

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.