[dborup]

Summary

Relates to #407.

Resolve the selected config path before reading and atomically replacing it. A config symlink remains a symlink, while its persistent target receives the geo-filter update or removal. Existing unique temporary files, permissions, best-effort ownership preservation, fsync, cleanup and wrapped errors remain intact.

Verification

Baseline: master 4a5ec5e231dd97ed7a4392e50e5f409f4dc5fab0.

• Regression tests failed on unchanged production: valid symlinks were overwritten, the temporary file/rename used the alias directory, and an unwritable target directory incorrectly reported success.
• Focused SaveGeoFilter tests pass normally and under the race detector: 21 top-level groups; no skips in the focused local run.
• Parent independently ran the full server suite with -race: PASS (504.391 s). Unchanged ingestor suite: PASS (128.895 s). Existing JavaScript baseline: 238/238 files.
• Parent browser check against a local Go server: successful authenticated save, symlink retained, target JSON updated, 0600 mode preserved, and saved polygon retained after server restart.
• go vet, formatting and diff checks pass.

Independent review

A separate reviewer inspected the immutable candidate tree and ran the focused race tests in an isolated archive export: PASS, no actionable findings.

Integration and limitations

The proposed non-root PR #404's create/rename error wrappers are not replaced; its changes are separate from this insertion. Filesystem permissions are checked at the effective target. A genuinely unwritable/read-only target still fails.

A file's read-only mode is not a write-policy guarantee when its parent directory permits atomic replacement. Foreign ownership restoration remains best-effort. This change does not add protection against an external actor retargeting links/directories during a save. An actual Docker bind-mount rehearsal has not been performed.

Test environment: macOS arm64, Go 1.26.3, Node 24.19.0. GitHub CI is separate evidence and pending at creation.

Performance and scope

One bounded filesystem path-resolution operation on an admin-save path, not the packet hot path. No database/schema, workflow, configuration option, customizer or dependency changes. No staging/production access, deployment, merge or upstream writes.

[dborup-agent]

Review — CS-pve-agent1 PR#422 — head 42aea67

Dom: APPROVE med nits

Note: despite the task framing, #422 is the first PR for issue #407, not a follow-up closing earlier review nits. I checked each point of the issue's "Proposed work" and acceptance criteria. No author report was posted, so the PR body is the only author claim.

Findings

#	Severity	Finding	Evidence
1	nit	The resolve config: %w error branch is not pinned by any test. Mutant M2 (ignore the EvalSymlinks error and fall back to the link path) survived. Dangling and loop links are rejected earlier by the existing os.Stat selection loop, so the branch is near-unreachable today. It is cheap defensive code and the behaviour is correct.	[T] mutant run
2	nit	The PR body says "21 top-level groups" and cites a macOS arm64 run. I could only cross-check the Linux run. The unwritable-directory test skips as root, which does not affect CI as a non-root user.	[A]
3	info	A read-only mount that leaves the directory writable is not detected as read-only. The PR states this limitation honestly.	[A] PR body
4	info	The retarget-during-save race (TOCTOU between EvalSymlinks and rename) is not addressed. The PR states this explicitly as out of scope.	[A] PR body

No blocking findings.

Issue points, one by one

Issue point	Status
Resolve the effective target without replacing the link	Done. filepath.EvalSymlinks runs before open, temp creation and rename. Tests cover absolute, relative, chained and symlinked-directory links, and assert the link is unchanged [T].
Preserve unique temp file, mode, ownership, sync and error handling	Done. The diff is +10 lines in config.go and no existing lines changed. Mode and owner are asserted in the tests [T][A].
Dangling, chained and relative links	Done. Dangling and loop links return an error with no files created. The dangling link falls back to data/config.json as before [T].
Read-only mounts and atomic-rename failures explicit	Done. Unwritable target directory returns os.ErrPermission. Rename failure is wrapped, leaves no temp file and leaves the target unchanged [T].
Coordinate with #404, keep deliberate permission restrictions	Done. No create/rename wrappers are replaced. The permission outcome is explicit [A].
Regression tests inspect target contents, link intact, persistence/reopen	Done. Contents are compared field by field and the link is checked with Readlink. The "reopen" case is covered by re-reading the target. I also did a live PUT (below) [T].
Acceptance: update target, keep link and invariants, explicit errors	Met [T].

Tests and mutants

On the merged tree (git merge-tree of origin/master 4c7a1ca5 and the full head sha; clean merge, no conflicts), exported with git archive into scratch:

• cmd/server go test -race ./...: PASS (725 s). A first run hit Go's default 10-minute timeout in TestRouteMaskChanges_ConcurrentIngestAndPoll while I ran another suite in parallel. The rerun alone with -timeout 30m passed. That test is unrelated to this diff [T].
• cmd/ingestor go test ./...: PASS (220 s) [T].
• sh test-all.sh: 242 passed, 0 failed [T].
• node test-frontend-helpers.js: 713 passed, 0 failed [T].
• Focused SaveGeoFilter tests with -race: PASS [T].
• The PR's own tests are red without the production change. Mutant M0 (remove the configPath = resolvedPath assignment) fails 4 tests [T].

Mutants I ran against the PR's tests:

Mutant	Result
M0: do not use the resolved path	Killed. 4 tests fail.
M1: temp file created next to the link, not the target	Killed by UsesTargetDirectory407.
M2: ignore the EvalSymlinks error	Survived (finding 1).
M3: rename onto the link path	Killed. 3 tests fail.

Live check: a local Go server with a fixture DB, prepared with corescope-migrate and seeds 2073, 199 and 245 (not the CI "freshen" step). config.json was a relative symlink into a data directory, with a locally invented test key. PUT /api/config/geo-filter returned 200, the symlink stayed a symlink, the target held the polygon, mode 0600 and _comment were preserved. A PUT with an empty polygon removed it. [T]

Scope and policy checks: the diff is two files (cmd/server/config.go +10 and the new test, +252) [T]. There are no new map[string]interface{} in the diff [T]. cmd/server stays read-only for the DB; the diff adds no DB write [T]. There are no colour, public/ or workflow changes, and check-xss-sinks.sh --diff origin/master reports nothing to scan [T]. The workflow fork-guard grep count is identical on master and head (10 with my pattern; I did not reproduce your "9 and 1" counting). The commit author and committer are dborup <kontakt@meshview.dk>. The PR body uses "Relates to #407", and the diff has no closing keywords. The head sha was the same before and after my run [T].

CI: Go Build & Test, Playwright E2E and Docker image all pass. The jobs that were skipped are the expected fork-guarded ones [T]. No flaky test (#256, #267) was involved.

Not verified

• Affected E2E against a local server: I ran test-e2e-playwright.js and it stopped fail-fast at "Version info lives on Perf dashboard, not in navbar" (a wait timeout), before reaching any geo-filter test. I did not rerun the fixture "freshen" step, so I read this as an environment difference. The diff touches no frontend or Perf code. I have not ruled it out, so CI's passing Playwright job is the only E2E evidence.
• Docker bind-mount behaviour, macOS runs and foreign-owner restoration as root.
• The "9 and 1" fork-guard counts; I only showed the count is unchanged.