From 478f74a452b5d3a9764da7f405defdd5cd592e8b Mon Sep 17 00:00:00 2001 From: nullrouten0 Date: Mon, 12 Oct 2026 23:44:49 -0700 Subject: [PATCH] fix(config): write the geo filter through a symlinked config.json (#2137) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while reworking #2125, unrelated to that PR's change. In the Docker image the entrypoint symlinks `/app/config.json` to the bind-mounted `/app/data/config.json`. `SaveGeoFilter` takes the first path `os.Stat` resolves, so `configPath` stays the symlink, and the tmp+rename then **replaces the link with a regular file inside the container**: - the mounted `config.json` keeps the old `geo_filter`, so the filter the operator just saved appears nowhere on the host; - server and ingestor read the detached file from then on, so the UI keeps showing the new filter and nothing looks wrong; - the next container start reverts it: the entrypoint's `ln -sf` puts the link back, and re-creating the container drops the file anyway. **Fix:** resolve a symlink at `configPath` before the write, so the save lands in the file it points at and the link stays a link. Only a regular file is followed — a link to a device, fifo or directory is left alone and handled as before. This is deliberately the opposite of `writeStatsAtomic` (refs #1170), where replacing a destination symlink is the point because the default stats path can be world-writable `/tmp`. `config.json` is different: the link is one the image itself creates, and the directory is not world-writable. Happy to narrow it further (e.g. only follow a link that stays inside `configDir`) if you'd rather keep the two paths consistent. **Verification:** `TestSaveGeoFilterSymlinkedConfig` fails on master — ``` --- FAIL: TestSaveGeoFilterSymlinkedConfig config_symlink_test.go:47: .../config.json is no longer a symlink (mode -rw-------) — the rename replaced it config_symlink_test.go:60: geo_filter missing from .../data/config.json: {"port":3000,"_comment":"keep me"} ``` — and passes here. `TestSaveGeoFilterKeepsOwner` covers keeping the file's owner, and `TestSaveGeoFilterFifoSymlinkTarget` checks that only regular files are followed. `go test ./...` in `cmd/server` passes. **Single-file mounts:** when `config.json` is mounted as a single file, the save now fails with 500 (`EBUSY`) instead of writing a copy that the next start discards. Our own instance has no geo filter configured, which is why this went unnoticed there; its `/app/config.json` is still a symlink. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 --- cmd/server/config.go | 33 +++++++++- cmd/server/config_owner_unix.go | 17 +++++ cmd/server/config_owner_unix_test.go | 99 ++++++++++++++++++++++++++++ cmd/server/config_owner_windows.go | 9 +++ cmd/server/config_symlink_test.go | 81 +++++++++++++++++++++++ 5 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 cmd/server/config_owner_unix.go create mode 100644 cmd/server/config_owner_unix_test.go create mode 100644 cmd/server/config_owner_windows.go create mode 100644 cmd/server/config_symlink_test.go diff --git a/cmd/server/config.go b/cmd/server/config.go index 5c4de116..4fa6f543 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -1052,6 +1052,26 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { return fmt.Errorf("config.json not found in %s", configDir) } + // Follow a symlink at configPath to the file it points at before the + // tmp+rename below. In the Docker image /app/config.json is a symlink to + // the bind-mounted /app/data/config.json (docker/entrypoint-go.sh), and + // os.Rename onto the link path would replace the link with a regular + // file inside the container: the mounted file would keep the old + // geo_filter, the saved one would be invisible on the host and would go + // away with the container. Resolving first keeps the write atomic and + // the result where the operator (and the next container) will read it. + // + // Unlike the stats file (writeStatsAtomic, refs #1170), replacing a + // symlink at the destination is not wanted here: this path is not + // world-writable, and the link is one the image itself creates. Only a + // regular file is followed, so a link to a device or a fifo is left + // alone and handled as before. + if resolved, err := filepath.EvalSymlinks(configPath); err == nil && resolved != configPath { + if fi, err := os.Lstat(resolved); err == nil && fi.Mode().IsRegular() { + configPath = resolved + } + } + data, err := os.ReadFile(configPath) if err != nil { return fmt.Errorf("read config: %w", err) @@ -1083,13 +1103,22 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { // config.json holds the API key and broker passwords, and an operator // who made it 0600 should not find it 0644 after a geo-filter save. mode := os.FileMode(0644) - if fi, err := os.Stat(configPath); err == nil { - mode = fi.Mode().Perm() + orig, origErr := os.Stat(configPath) + if origErr == nil { + mode = orig.Mode().Perm() } tmp := configPath + ".tmp" if err := os.WriteFile(tmp, out, mode); err != nil { return fmt.Errorf("write config: %w", err) } + // Keep the owner as well. The image runs the server as root by default, + // and without this the rename would hand the operator's bind-mounted + // config.json to root:root, which at 0600 they could no longer edit + // without sudo. Best effort: a server that may not chown leaves the file + // as it was created. + if origErr == nil { + copyOwner(tmp, orig) + } if err := os.Rename(tmp, configPath); err != nil { os.Remove(tmp) return fmt.Errorf("rename config: %w", err) diff --git a/cmd/server/config_owner_unix.go b/cmd/server/config_owner_unix.go new file mode 100644 index 00000000..46eba93f --- /dev/null +++ b/cmd/server/config_owner_unix.go @@ -0,0 +1,17 @@ +//go:build !windows + +package main + +import ( + "os" + "syscall" +) + +// copyOwner gives path the owner and group recorded in fi, best effort: a +// process that may not chown (not root, or not in the group) leaves path as +// it was created. On Windows there is no uid/gid; see config_owner_windows.go. +func copyOwner(path string, fi os.FileInfo) { + if st, ok := fi.Sys().(*syscall.Stat_t); ok { + _ = os.Chown(path, int(st.Uid), int(st.Gid)) + } +} diff --git a/cmd/server/config_owner_unix_test.go b/cmd/server/config_owner_unix_test.go new file mode 100644 index 00000000..4d610a2e --- /dev/null +++ b/cmd/server/config_owner_unix_test.go @@ -0,0 +1,99 @@ +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +// TestSaveGeoFilterKeepsOwner checks that a save keeps the owner and group of +// config.json. The image runs the server as root, and a bind-mounted config +// that came back as root:root 0600 would lock the operator out of their own +// file. +func TestSaveGeoFilterKeepsOwner(t *testing.T) { + // Pick an owner the save would not produce by itself. Root can use any + // uid and gid; anyone else can only switch to a supplementary group. + uid, gid := -1, -1 + if os.Geteuid() == 0 { + uid, gid = 4242, 4242 + } else if groups, err := os.Getgroups(); err == nil { + for _, g := range groups { + if g != os.Getegid() { + gid = g + break + } + } + } + if gid == -1 { + t.Skip("not root and no supplementary group to chown to") + } + + dir := t.TempDir() + cfg := filepath.Join(dir, "config.json") + if err := os.WriteFile(cfg, []byte(`{"port":3000}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chown(cfg, uid, gid); err != nil { + t.Fatalf("chown %s: %v", cfg, err) + } + + gf := &GeoFilterConfig{Polygon: [][2]float64{{51.0, 4.0}, {51.0, 5.0}, {50.5, 4.0}}} + if err := SaveGeoFilter(dir, gf); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + fi, err := os.Stat(cfg) + if err != nil { + t.Fatal(err) + } + st := fi.Sys().(*syscall.Stat_t) + if uid != -1 && int(st.Uid) != uid { + t.Errorf("owner of %s = %d, want %d", cfg, st.Uid, uid) + } + if int(st.Gid) != gid { + t.Errorf("group of %s = %d, want %d", cfg, st.Gid, gid) + } + if perm := fi.Mode().Perm(); perm != 0o600 { + t.Errorf("mode of %s = %#o, want 0600", cfg, perm) + } +} + +// TestSaveGeoFilterFifoSymlinkTarget pins the regular-file check on the +// symlink target: a config.json that links to a fifo is not followed, so the +// save never replaces the fifo with a regular file. A directory target would +// not test this, since reading the config already fails there. +func TestSaveGeoFilterFifoSymlinkTarget(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "feed") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Skipf("mkfifo unavailable: %v", err) + } + link := filepath.Join(dir, "config.json") + if err := os.Symlink(fifo, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + // SaveGeoFilter is the fifo's reader; feed it one config. + go func() { + f, err := os.OpenFile(fifo, os.O_WRONLY, 0) + if err != nil { + return + } + f.WriteString(`{"port":3000}`) + f.Close() + }() + if err := SaveGeoFilter(dir, nil); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + fi, err := os.Lstat(fifo) + if err != nil { + t.Fatalf("lstat %s: %v", fifo, err) + } + if fi.Mode()&os.ModeNamedPipe == 0 { + t.Errorf("%s was replaced (mode %s) — the link to a fifo was followed", fifo, fi.Mode()) + } +} diff --git a/cmd/server/config_owner_windows.go b/cmd/server/config_owner_windows.go new file mode 100644 index 00000000..ffcec731 --- /dev/null +++ b/cmd/server/config_owner_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package main + +import "os" + +// copyOwner is a no-op on Windows: there is no uid/gid to copy, and the new +// file takes the ACL of its directory. +func copyOwner(path string, fi os.FileInfo) {} diff --git a/cmd/server/config_symlink_test.go b/cmd/server/config_symlink_test.go new file mode 100644 index 00000000..240c3429 --- /dev/null +++ b/cmd/server/config_symlink_test.go @@ -0,0 +1,81 @@ +package main + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// TestSaveGeoFilterSymlinkedConfig pins the behavior the Docker image depends +// on: /app/config.json is a symlink to the bind-mounted /app/data/config.json, +// so a geo-filter save has to land in the mounted file and leave the symlink a +// symlink. A tmp+rename on the link path instead replaces the link with a +// regular file in the container layer, and the operator's config silently +// stops being the one in use. +func TestSaveGeoFilterSymlinkedConfig(t *testing.T) { + dir := t.TempDir() + dataDir := filepath.Join(dir, "data") + if err := os.Mkdir(dataDir, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dataDir, "config.json") + if err := os.WriteFile(target, []byte(`{"port":3000,"_comment":"keep me"}`), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "config.json") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + gf := &GeoFilterConfig{ + Polygon: [][2]float64{{51.0, 4.0}, {51.0, 5.0}, {50.5, 4.0}}, + BufferKm: 20, + } + if err := SaveGeoFilter(dir, gf); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + // The symlink must survive, or the next container start reads a different + // file than the one that was written. + fi, err := os.Lstat(link) + if err != nil { + t.Fatalf("lstat %s: %v", link, err) + } + if fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("%s is no longer a symlink (mode %s) — the rename replaced it", link, fi.Mode()) + } + + // The geo filter must be in the mounted file, next to what was there. + data, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read %s: %v", target, err) + } + var raw map[string]interface{} + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("parse saved config: %v", err) + } + if _, ok := raw["geo_filter"]; !ok { + t.Errorf("geo_filter missing from %s: %s", target, data) + } + if raw["_comment"] != "keep me" { + t.Errorf("unrelated keys not preserved: %s", data) + } + if !bytes.Contains(data, []byte(`"bufferKm"`)) { + t.Errorf("bufferKm missing from saved config: %s", data) + } + + // The 0600 the operator chose is kept (#2126), and no tmp file is left in + // either directory. + if fi, err := os.Stat(target); err != nil { + t.Fatal(err) + } else if perm := fi.Mode().Perm(); perm != 0o600 { + t.Errorf("mode of %s = %#o, want 0600", target, perm) + } + for _, leftover := range []string{link + ".tmp", target + ".tmp"} { + if _, err := os.Lstat(leftover); err == nil { + t.Errorf("leftover temp file %s", leftover) + } + } +}