diff --git a/cmd/ingestor/stats_file.go b/cmd/ingestor/stats_file.go index 4c2bea85..17bcb4e8 100644 --- a/cmd/ingestor/stats_file.go +++ b/cmd/ingestor/stats_file.go @@ -61,6 +61,25 @@ func statsFilePath() string { // writeStatsAtomic writes b to path via a tmp-then-rename, refusing to follow // symlinks on the tmp file. Returns nil on success, an error otherwise. +// +// Symlink semantics (refs #1170): +// +// - tmp side (path+".tmp"): protected by O_NOFOLLOW below. If tmp is a +// pre-planted symlink, openat fails with ELOOP instead of writing +// through it. This is the defensive-coding path that matters when the +// default stats path lives under world-writable /tmp. +// +// - rename side (path): NOT protected by O_NOFOLLOW. Instead, os.Rename's +// semantics are relied upon — rename atomically replaces any existing +// entry at path (including a symlink) with the new regular file. The +// symlink's target is NEVER written through, because all writes happened +// to the unrelated tmp file before rename. Post-rename, path is a +// regular file (not a symlink) and any prior symlink target's contents +// are unchanged. The regression guardrail +// TestWriteStatsAtomic_SymlinkAtDestIsReplaced pins this behavior so a +// future refactor that swaps os.Rename for a destination-symlink- +// following primitive (e.g. an open(path, O_WRONLY) without O_NOFOLLOW) +// fails loudly. func writeStatsAtomic(path string, b []byte) error { tmp := path + ".tmp" // O_NOFOLLOW: if tmp is a pre-existing symlink, openat fails with ELOOP diff --git a/cmd/ingestor/stats_file_test.go b/cmd/ingestor/stats_file_test.go index ae0d5b26..29ba4b22 100644 --- a/cmd/ingestor/stats_file_test.go +++ b/cmd/ingestor/stats_file_test.go @@ -96,3 +96,73 @@ func TestStatsFileWriter_PublishesProcIO(t *testing.T) { } } } + +// TestWriteStatsAtomic_SymlinkAtDestIsReplaced is a regression guardrail for +// #1170. The tmp side of writeStatsAtomic uses O_NOFOLLOW so a pre-planted +// symlink at path+".tmp" cannot redirect the write — but the rename target +// (`path` itself) is not protected by O_NOFOLLOW. Instead, os.Rename's +// semantics are relied upon: rename atomically replaces any existing entry +// at the destination, including a symlink, with the new regular file. The +// original symlink's target is never written through (because the write +// happened to the unrelated tmp file). +// +// This test pre-plants a symlink at `path` pointing to an unrelated target +// file and asserts: +// (a) post-write, path is a regular file (not a symlink), and +// (b) the original target's contents are unchanged. +// +// If a future refactor swaps os.Rename for something that follows the +// destination symlink (e.g. ioutil.WriteFile, or an open(path, O_WRONLY) +// without O_NOFOLLOW), this test will fail loudly. +func TestWriteStatsAtomic_SymlinkAtDestIsReplaced(t *testing.T) { + dir := t.TempDir() + + // Unrelated target file with sentinel bytes. If writeStatsAtomic ever + // followed the symlink at `path`, it would overwrite this file. + target := filepath.Join(dir, "unrelated-target.bin") + sentinel := []byte("DO-NOT-OVERWRITE-ME-#1170") + if err := os.WriteFile(target, sentinel, 0o600); err != nil { + t.Fatalf("seed target: %v", err) + } + + // Pre-plant a symlink at the destination path. + path := filepath.Join(dir, "stats.json") + if err := os.Symlink(target, path); err != nil { + t.Fatalf("symlink: %v", err) + } + + payload := []byte(`{"sampledAt":"2026-01-01T00:00:00Z"}`) + if err := writeStatsAtomic(path, payload); err != nil { + t.Fatalf("writeStatsAtomic: %v", err) + } + + // (a) post-write, path must NOT be a symlink. + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("lstat path: %v", err) + } + if info.Mode()&os.ModeSymlink != 0 { + t.Errorf("post-write path is still a symlink (mode=%v); os.Rename should have atomically replaced it with a regular file", info.Mode()) + } + if !info.Mode().IsRegular() { + t.Errorf("post-write path is not a regular file (mode=%v)", info.Mode()) + } + + // Path now contains the new payload. + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read path: %v", err) + } + if string(got) != string(payload) { + t.Errorf("path contents: want %q, got %q", payload, got) + } + + // (b) the original symlink target must be unchanged. + gotTarget, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read target: %v", err) + } + if string(gotTarget) != string(sentinel) { + t.Errorf("symlink target was clobbered: want %q, got %q", sentinel, gotTarget) + } +}