security: harden filesystem path operations against path traversal

[Copilot]

Summary

Defense-in-depth hardening for three code paths that build a filesystem path with filepath.Join(base, userInfluenced) and then read from it without verifying the result stays within base. No exploit path exists today (inputs originate from developer-specified repositories), but the patterns would become exploitable if ever refactored to accept less-trusted input.

Follows the same pattern as the existing zip extractor in logs_download.go:364-380.

Changes

New helper: pkg/fileutil/fileutil.go — MustBeWithin(base, candidate string) error

Resolves both paths to absolute form via filepath.Abs, then uses filepath.Rel + filepath.IsLocal (Go 1.20+) to assert the candidate is contained within the base directory tree. Returns a descriptive error if it escapes.

pkg/cli/download_workflow.go (line 140)

filePath := filepath.Join(tmpDir, path)
// NEW: boundary check before reading
if err := fileutil.MustBeWithin(tmpDir, filePath); err != nil {
    return nil, fmt.Errorf("refusing to read file outside clone directory: %w", err)
}
content, err := os.ReadFile(filePath)

pkg/parser/remote_fetch.go (line 547)

Identical guard applied after filepath.Join(tmpDir, path).

pkg/fileutil/tar.go — ExtractFileFromTar

Uses filepath.IsLocal to:

1. Reject the caller-supplied search target before iterating the archive.
2. Skip any archive entry whose name is not local (absolute paths, .. components).

filepath.IsLocal avoids false positives for legitimate filenames like file..backup.txt that contain .. as a substring rather than a path component.

pkg/fileutil/fileutil_test.go

• TestMustBeWithin: traversal via .., deeply nested traversal, absolute path outside base, valid paths within base.
• TestExtractFileFromTar_UnsafePaths: absolute search target, .. in search target, archive entry with absolute name, archive entry with .. name, and a filename containing .. as a substring (confirms no false positive).

Testing

go test -v -run "TestMustBeWithin|TestExtractFileFromTar" ./pkg/fileutil/
# All 10 sub-tests PASS

References

• Affected code identified in security hardening request (DiD, no active exploit)
• Reference implementation: logs_download.go:364-380 (zip extractor)

[Copilot]

Pull request overview

Defense-in-depth hardening to prevent path traversal in code paths that build filesystem paths from a trusted base plus user-influenced components, and to reject unsafe paths when scanning tar archives.

Changes:

• Added fileutil.MustBeWithin(base, candidate) helper to assert joined paths remain under a base directory.
• Applied the helper before reading files from sparse-cloned repositories in CLI and parser code paths.
• Hardened ExtractFileFromTar by rejecting non-local search targets and skipping non-local tar entry names; added focused tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/fileutil/fileutil.go	Adds MustBeWithin path boundary check helper.
pkg/cli/download_workflow.go	Uses MustBeWithin to guard reads from the clone directory.
pkg/parser/remote_fetch.go	Uses MustBeWithin to guard reads from the clone directory.
pkg/fileutil/tar.go	Rejects unsafe tar search targets and skips unsafe tar entry names using filepath.IsLocal.
pkg/fileutil/fileutil_test.go	Adds tests for MustBeWithin and unsafe tar path handling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The docstring claims symlinks cannot be used to escape base, but filepath.Abs does not resolve symlinks. A repo can contain a symlink inside tmpDir pointing outside; MustBeWithin will currently allow tmpDir/link/passwd and os.ReadFile will follow the symlink and read outside the clone directory. Either update the comment to avoid claiming symlink safety, or make the check symlink-aware (e.g., EvalSymlinks on base and candidate before computing Rel, with appropriate handling when the candidate doesn’t exist).

[Copilot]

TestMustBeWithin exercises .. and absolute-path traversal, but it doesn’t cover the symlink-escape case (a symlink within base pointing outside). Since MustBeWithin is used to guard os.ReadFile, adding a symlink traversal test (skipping when os.Symlink isn’t supported, e.g. some Windows setups) would prevent regressions once the helper is made symlink-aware.

Suggested change

	}
	}
	t.Run("symlink escape", func(t *testing.T) {
	// Create a real file outside the base directory.
	outsideFile, err := os.CreateTemp("", "mustbewithin-outside-*")
	require.NoError(t, err, "failed to create outside file")
	defer os.Remove(outsideFile.Name())
	outsidePath := outsideFile.Name()
	require.NoError(t, outsideFile.Close())
	// Create a symlink inside base that points to the outside file.
	linkPath := filepath.Join(base, "link-to-outside")
	if err := os.Symlink(outsidePath, linkPath); err != nil {
	// Some platforms (or configurations) do not support symlinks; skip in that case.
	t.Skipf("symlinks not supported on this platform or configuration: %v", err)
	}
	defer os.Remove(linkPath)
	err = MustBeWithin(base, linkPath)
	require.Error(t, err, "MustBeWithin should reject symlink that escapes base directory")
	assert.Contains(t, err.Error(), "escapes base directory", "Error should describe the escape for symlink")
	})

[pelikhan]

@copilot review comments

[Copilot]

@copilot review comments

Addressed both review comments in commit 70c9ca7:

• MustBeWithin now calls filepath.EvalSymlinks on both base and candidate (falling back to filepath.Abs when a path does not exist on disk) before computing filepath.Rel. This ensures a symlink inside tmpDir that points outside is resolved to its real target and caught by the containment check.
• Updated the docstring to accurately describe the symlink-aware behaviour.
• Added TestMustBeWithin/symlink_escape to cover this attack vector.