From 4095c51875c03e46a2eb2675ecc40a04f42fd572 Mon Sep 17 00:00:00 2001 From: universe-ops Date: Sat, 27 Jun 2026 22:04:18 +0300 Subject: [PATCH] fix(git): open absolute-path files via osfs root "/" (fixes secrets reveal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenFile delegated absolute paths to osfs.New("").OpenFile(...). Under go-billy v5.9.0 the default ChrootOS resolves symlinks relative to its root via filepath.Rel; with an empty root (which cleans to "."), filepath.Rel(".", "/abs/path") always errors, so every absolute open returned "chroot boundary crossed". This broke profile configs that reference keys by absolute/tilde path (e.g. privateKeyPath: ~/.ssh/id_rsa). WithKeysFromScConfig loads the private key first, so the failed open aborted the whole profile load. The error is swallowed by IgnoreConfigDirError at command bootstrap, leaving the public key unset — surfacing later as the misleading "public key is not configured" on `sc secrets reveal` / `add` / `allow`. Root the OS filesystem at "/" for absolute paths so Rel("/", "/abs/path") is well-defined. The relative-path fallback (wdFs) stays rooted at "" so it keeps resolving against cwd. Updated the unit test that had codified the broken "chroot boundary" behavior to assert absolute-path opens now succeed. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Ilya Sadykov --- pkg/api/git/repo.go | 7 ++++++- pkg/api/git/repo_lifecycle_test.go | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/api/git/repo.go b/pkg/api/git/repo.go index 49b52d7c..b834a159 100644 --- a/pkg/api/git/repo.go +++ b/pkg/api/git/repo.go @@ -75,7 +75,12 @@ func (r *repo) OpenFile(filePath string, flag int, perm os.FileMode) (billy.File return nil, errors.Wrapf(err, "failed to replace path %q with home", filePath) } if path.IsAbs(rPath) { - return osfs.New("").OpenFile(rPath, flag, perm) + // Root the OS filesystem at "/" (not "") for absolute paths. go-billy's + // chroot helper (osfs default) resolves symlinks relative to its root via + // filepath.Rel; with an empty root that cleans to ".", and Rel(".", "/abs/path") + // fails, so every absolute open returns "chroot boundary crossed". Rooting at + // "/" makes Rel("/", "/abs/path") well-defined. Regressed with go-billy v5.9.0. + return osfs.New("/").OpenFile(rPath, flag, perm) } return r.wdFs.OpenFile(rPath, flag, perm) } diff --git a/pkg/api/git/repo_lifecycle_test.go b/pkg/api/git/repo_lifecycle_test.go index ab257f0f..c146c636 100644 --- a/pkg/api/git/repo_lifecycle_test.go +++ b/pkg/api/git/repo_lifecycle_test.go @@ -353,13 +353,6 @@ func TestFileOperations(t *testing.T) { Expect(string(b)).To(Equal("relative")) }) - // QUIRK (repo.go:74-75): for an absolute path OpenFile delegates to - // osfs.New("").OpenFile(...). Under go-billy v5.9.0 the default ChrootOS has - // root "" (a *relative* path), and its symlink-follow boundary check runs - // filepath.Rel(".", ) which always errors, so it returns - // "chroot boundary crossed". The absolute-path branch therefore cannot - // actually open any absolute file with this billy version; we assert that - // observed behaviour (the branch is still exercised for coverage). t.Run("OpenFile wraps a tilde-expansion error for an unknown user", func(t *testing.T) { RegisterTestingT(t) wd := t.TempDir() @@ -372,7 +365,11 @@ func TestFileOperations(t *testing.T) { Expect(err.Error()).To(ContainSubstring("failed to replace path")) }) - t.Run("OpenFile with an absolute path hits the chroot boundary error", func(t *testing.T) { + // For an absolute path OpenFile delegates to osfs.New("/").OpenFile(...). + // Rooting the OS filesystem at "/" (rather than "") keeps go-billy's ChrootOS + // symlink-follow boundary check well-defined for absolute paths — see repo.go. + // This is the path that loads keys referenced by ~/.ssh-style profile config. + t.Run("OpenFile opens a file by absolute path", func(t *testing.T) { RegisterTestingT(t) wd := t.TempDir() r := newRepoIn(wd) @@ -380,9 +377,12 @@ func TestFileOperations(t *testing.T) { absPath := filepath.Join(t.TempDir(), "abs.txt") Expect(os.WriteFile(absPath, []byte("absolute"), 0o644)).To(Succeed()) - _, err := r.OpenFile(absPath, os.O_RDONLY, 0) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("boundary")) + f, err := r.OpenFile(absPath, os.O_RDONLY, 0) + Expect(err).ToNot(HaveOccurred()) + defer func() { _ = f.Close() }() + b, err := io.ReadAll(f) + Expect(err).ToNot(HaveOccurred()) + Expect(string(b)).To(Equal("absolute")) }) }