From af669438ffe177f6a8046ce595c1baecf6375678 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Thu, 4 Jun 2026 18:48:23 -0700 Subject: [PATCH] docs+test(ingestor): document writeStatsAtomic symlink-replace semantics + regression test (#1170) (#1588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1170. ## What 1. **Doc comment** on `writeStatsAtomic` (`cmd/ingestor/stats_file.go`) spelling out the two-sided symlink story: - tmp side (`path+".tmp"`): protected by `O_NOFOLLOW` (existing behavior, already noted). - rename side (`path` itself): NOT protected by `O_NOFOLLOW`; instead `os.Rename` semantics are relied upon — rename atomically replaces any existing entry at `path` (including a symlink) with the new regular file. The symlink target is never written through because all writes happened to the unrelated tmp file before rename. 2. **Regression guardrail test** `TestWriteStatsAtomic_SymlinkAtDestIsReplaced` in `cmd/ingestor/stats_file_test.go` that pre-plants a symlink at the destination path pointing to an unrelated target file, calls `writeStatsAtomic`, and asserts: - (a) `os.Lstat(path).Mode()&os.ModeSymlink == 0` (post-write path is a regular file, not a symlink) - (b) the original symlink target's sentinel bytes are unchanged. If a future refactor swaps `os.Rename` for a destination-symlink-following primitive (e.g. `open(path, O_WRONLY)` without `O_NOFOLLOW`, or a copy-then-truncate), the test fails loudly. ## TDD note (red-commit exemption) The current `writeStatsAtomic` ALREADY satisfies the new test's assertions — `os.Rename` does the right thing today. Per the fix-issue skill's exemption for pure-documentation / guardrail tests on already-correct behavior, no fabricated red commit was constructed; the test stands as a pinning regression guard. The two commits are therefore: (1) test addition, (2) doc comment. ## Scope - `cmd/ingestor/stats_file.go` — doc comment only - `cmd/ingestor/stats_file_test.go` — one new test function No production behavior change. No public API change. No new dependencies. No CI workflow changes. `O_NOFOLLOW` and the existing tmp-side behavior are untouched. ## Preflight All hard gates pass (PII, branch scope, red commit, CSS vars, LIKE-on-JSON, sync/async migration, XSS sinks). No warnings. --------- Co-authored-by: meshcore-bot --- cmd/ingestor/stats_file.go | 19 +++++++++ cmd/ingestor/stats_file_test.go | 70 +++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) 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) + } +}