From 783cc3bb34bd434d651ff6d1ccc9386eb957d1ab Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 11 Aug 2026 05:10:32 +0200 Subject: [PATCH] fix(thothctl): fail closed on partial secret loads --- tools/thothctl/cmd/thothctl/main.go | 6 ++++ tools/thothctl/cmd/thothctl/main_test.go | 17 ++++++++++ tools/thothctl/internal/safeio/files.go | 10 ++---- .../thothctl/internal/safeio/files_darwin.go | 6 +++- tools/thothctl/internal/safeio/files_linux.go | 6 +++- ...test.go => files_replacement_unix_test.go} | 31 ++++++++++++++----- .../thothctl/internal/safeio/files_windows.go | 2 +- 7 files changed, 60 insertions(+), 18 deletions(-) rename tools/thothctl/internal/safeio/{files_linux_test.go => files_replacement_unix_test.go} (62%) diff --git a/tools/thothctl/cmd/thothctl/main.go b/tools/thothctl/cmd/thothctl/main.go index ebaff926..fb79461c 100644 --- a/tools/thothctl/cmd/thothctl/main.go +++ b/tools/thothctl/cmd/thothctl/main.go @@ -307,6 +307,12 @@ func containsDeclaredSecretBytes(contents []byte, secrets []string) bool { // deterministic safe line exists it emits empty stderr rather than risk a // declared-secret collision. func writeWorkspaceError(stderr io.Writer, err error, secrets []string, code int) int { + // A nil set means the complete declared-secret set was not loaded. No + // nonempty diagnostic is safe because a successfully read secret may equal + // the fixed error chrome. Callers must fail closed without output. + if secrets == nil { + return code + } message := "workspace operation failed" if err != nil { message = output.Sanitize(err.Error(), secrets) diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index a39c29d7..1443e385 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -218,6 +218,23 @@ func TestRunWorkspaceRejectsDeclaredSecretInHumanChrome(t *testing.T) { } } +func TestRunWorkspacePartialSecretLoadEmitsNoOutput(t *testing.T) { + fixture := newCLIFixture(t, "") + firstSecret := filepath.Join(fixture.root, "first-secret") + missingSecret := filepath.Join(fixture.root, "missing-secret") + if err := os.WriteFile(firstSecret, []byte("workspace operation failed"), 0o600); err != nil { + t.Fatal(err) + } + fixture.setEnvContents(t, "FIRST_SECRET_FILE="+firstSecret+"\nSECOND_SECRET_FILE="+missingSecret+"\n") + + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "inspect", "--workspace", "psd"}, &stdout, &stderr) + if code != 2 || stdout.Len() != 0 || stderr.Len() != 0 { + t.Fatalf("exit=%d stdout=%q stderr=%q, want exit 2 and no output", code, stdout.String(), stderr.String()) + } + assertDockerNotInvoked(t, fixture) +} + func TestRunWorkspaceErrorPrefixNeverLeaksDeclaredSecret(t *testing.T) { fixture := newCLIFixture(t, "UNLABELLED_SECRET_FILE=%s\n") secretPath := filepath.Join(fixture.root, "secret") diff --git a/tools/thothctl/internal/safeio/files.go b/tools/thothctl/internal/safeio/files.go index 2ae10adb..810ec7ea 100644 --- a/tools/thothctl/internal/safeio/files.go +++ b/tools/thothctl/internal/safeio/files.go @@ -18,10 +18,6 @@ var ErrUnsafeFile = errors.New("unsafe file") // private stages before retrying; it is never a blind-retry-safe failure. var ErrIndeterminateFile = errors.New("indeterminate file state") -// beforeBoundedRead is an internal test seam used to deterministically suspend -// a read between opening the file and resolving its final pathname. -var beforeBoundedRead func() - // ValidateCanonicalPath rejects relative or lexically non-canonical paths before they are opened. func ValidateCanonicalPath(path string) error { if !filepath.IsAbs(path) || filepath.Clean(path) != path || strings.Contains(path, string(filepath.Separator)+".."+string(filepath.Separator)) { @@ -33,7 +29,7 @@ func ValidateCanonicalPath(path string) error { return nil } -func readBoundedRegularFile(file *os.File, maximum int64) ([]byte, error) { +func readBoundedRegularFile(file *os.File, maximum int64, before func()) ([]byte, error) { if maximum < 0 || maximum == int64(^uint64(0)>>1) { return nil, ErrUnsafeFile } @@ -41,8 +37,8 @@ func readBoundedRegularFile(file *os.File, maximum int64) ([]byte, error) { if err != nil || !info.Mode().IsRegular() { return nil, ErrUnsafeFile } - if beforeBoundedRead != nil { - beforeBoundedRead() + if before != nil { + before() } contents, err := io.ReadAll(io.LimitReader(file, maximum+1)) if err != nil || int64(len(contents)) > maximum { diff --git a/tools/thothctl/internal/safeio/files_darwin.go b/tools/thothctl/internal/safeio/files_darwin.go index 55010834..cba28d2a 100644 --- a/tools/thothctl/internal/safeio/files_darwin.go +++ b/tools/thothctl/internal/safeio/files_darwin.go @@ -16,6 +16,10 @@ import ( // descriptor. O_NOFOLLOW rejects symlinks at every component, and the open directory descriptors // prevent later parent replacement from redirecting the final open. func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { + return readCanonicalRegularWithHook(path, maximum, nil) +} + +func readCanonicalRegularWithHook(path string, maximum int64, beforeRead func()) ([]byte, error) { if err := ValidateCanonicalPath(path); err != nil { return nil, err } @@ -54,7 +58,7 @@ func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { if err := unix.Fstat(int(file.Fd()), &before); err != nil || before.Nlink > 1 || before.Mode&unix.S_IFMT != unix.S_IFREG { return nil, ErrUnsafeFile } - contents, err := readBoundedRegularFile(file, maximum) + contents, err := readBoundedRegularFile(file, maximum, beforeRead) if err != nil { return nil, ErrUnsafeFile } diff --git a/tools/thothctl/internal/safeio/files_linux.go b/tools/thothctl/internal/safeio/files_linux.go index 31dde734..a2060f9b 100644 --- a/tools/thothctl/internal/safeio/files_linux.go +++ b/tools/thothctl/internal/safeio/files_linux.go @@ -16,6 +16,10 @@ import ( // descriptor. O_NOFOLLOW rejects symlinks at every component, and the open directory descriptors // prevent later parent replacement from redirecting the final open. func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { + return readCanonicalRegularWithHook(path, maximum, nil) +} + +func readCanonicalRegularWithHook(path string, maximum int64, beforeRead func()) ([]byte, error) { if err := ValidateCanonicalPath(path); err != nil { return nil, err } @@ -54,7 +58,7 @@ func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { if err := unix.Fstat(int(file.Fd()), &before); err != nil || before.Nlink > 1 || before.Mode&unix.S_IFMT != unix.S_IFREG { return nil, ErrUnsafeFile } - contents, err := readBoundedRegularFile(file, maximum) + contents, err := readBoundedRegularFile(file, maximum, beforeRead) if err != nil { return nil, ErrUnsafeFile } diff --git a/tools/thothctl/internal/safeio/files_linux_test.go b/tools/thothctl/internal/safeio/files_replacement_unix_test.go similarity index 62% rename from tools/thothctl/internal/safeio/files_linux_test.go rename to tools/thothctl/internal/safeio/files_replacement_unix_test.go index e1326677..63a14e07 100644 --- a/tools/thothctl/internal/safeio/files_linux_test.go +++ b/tools/thothctl/internal/safeio/files_replacement_unix_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build darwin || linux package safeio @@ -10,7 +10,7 @@ import ( ) func TestReadCanonicalRegularRejectsReplacementDuringRead(t *testing.T) { - root := t.TempDir() + root := canonicalSafeioTempDir(t) path := filepath.Join(root, "schema.sql") replacement := filepath.Join(root, "replacement.sql") parked := filepath.Join(root, "parked.sql") @@ -24,13 +24,14 @@ func TestReadCanonicalRegularRejectsReplacementDuringRead(t *testing.T) { entered := make(chan struct{}) proceed := make(chan struct{}) - beforeBoundedRead = func() { - close(entered) - <-proceed - } - t.Cleanup(func() { beforeBoundedRead = nil }) result := make(chan error, 1) - go func() { _, err := ReadCanonicalRegular(path, int64(len(contents))); result <- err }() + go func() { + _, err := readCanonicalRegularWithHook(path, int64(len(contents)), func() { + close(entered) + <-proceed + }) + result <- err + }() <-entered if err := os.Rename(path, parked); err != nil { t.Fatal(err) @@ -44,3 +45,17 @@ func TestReadCanonicalRegularRejectsReplacementDuringRead(t *testing.T) { t.Fatalf("replacement during read error = %v, want ErrUnsafeFile", err) } } + +func canonicalSafeioTempDir(t *testing.T) string { + t.Helper() + root, err := filepath.EvalSymlinks(os.TempDir()) + if err != nil { + t.Fatal(err) + } + directory, err := os.MkdirTemp(root, "thothctl-safeio-test-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(directory) }) + return directory +} diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go index abbb9ab1..8fac0a14 100644 --- a/tools/thothctl/internal/safeio/files_windows.go +++ b/tools/thothctl/internal/safeio/files_windows.go @@ -71,7 +71,7 @@ func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { if windows.GetFileInformationByHandle(handle, &before) != nil || before.NumberOfLinks > 1 { return nil, ErrUnsafeFile } - contents, err := readBoundedRegularFile(file, maximum) + contents, err := readBoundedRegularFile(file, maximum, nil) if err != nil { return nil, ErrUnsafeFile }