Repository navigation
fix(config): keep restrictive config permissions during atomic geo-filter save (#340) - #357
Conversation
…filter saves
SaveGeoFilter wrote config.json.tmp with os.WriteFile(..., 0644) and renamed
it over config.json. A config deliberately restricted to 0600 therefore came
back 0644 after any geo-filter edit in the UI, and config.json can hold broker
credentials. The fixed temp path was also a write target an attacker with
directory access could pre-create as a symlink or hard link.
The replacement now:
- reads mode and contents from the same descriptor, and fails rather than
guessing a default when that metadata is unavailable;
- creates a uniquely named temp file in the destination directory
(os.CreateTemp, O_EXCL), so a pre-existing config.json.tmp is never
opened, written through or removed;
- applies the preserved mode with an explicit chmod on its own descriptor,
which makes the result umask-independent — copying upstream's WriteFile
mode argument would fix neither case;
- carries uid/gid over best-effort, a no-op in the normal case and
deliberately non-fatal so an unprivileged server (#339) can still save.
Relates to #340.
The create-temp seam hands back a closed descriptor, so the first operation on it fails with os.ErrClosed. Asserting that specific error keeps the test from passing on some unrelated failure, and the comment now says "before the rename" rather than "mid-write": the closed fd trips the chmod first. Relates to #340.
Rapport — CS-Minimax PR#357 #340 — head 9906c04Status: Done — issue implemented, 9 tests covering all 8 acceptance points, 7/7 mutants killed, CI green on every job that ran (no reruns needed). Evidence legend
Requirements
7 mutants run, 7 killed, 0 survived. The mutant runner restored the snapshot between every mutation and re-confirmed a green baseline at the start and the end of the run. Approach noteThe issue's warning about upstream's CI per jobAll six jobs of the pipeline run on head
No job failed, so no rerun was needed and the known-flaky #271 did not come up. [K] One note for the record: the first run on head Local verification
Remainders
|
Review — CS-pve-agent1 PR#357 — head 9906c04Dom: APPROVE med nits Independent, read-only review. Evidence: [T] = I ran it myself, [A] = I read the code or artifact, [K] = CI or a known issue. Trees used: PR head Findings
There are no blocking findings. The security-relevant behaviour is correct: mode is preserved independently of umask; it fails closed on stat errors; temp files are Acceptance points
Red-before method: base Owner/group and the non-root container (#339)
Supported config locationsBoth lookup paths are covered: Invariants
Tests and mutantsOn the merged tree
The E2E runs used a local Go server on Mutants, against
Probe, not committed: the Go container symlink layout ( CIRun on Not verified
|
Relates to #340
Problem
SaveGeoFilterincmd/server/config.gowroteconfig.json.tmpwithos.WriteFile(..., 0644)and renamed it overconfig.json. Three consequences:0600came back0644after anygeo-filter edit in the UI.
config.jsoncan hold broker credentials.group-readable
0644config could come back0600under a tight umask andbreak whatever else reads it.
config.json.tmpin the config directory — as a symlink or a hard link —got the full config, credentials included, written through to a path of its
choosing. The old cleanup also removed that path on failure, whether or not
this function had created it.
Plan
issue has a failing assertion before any production line moves.
each mutation.
Approach
Read mode and contents from one descriptor, so the bits written back belong
to the bytes that were parsed, and fail if that metadata is unavailable rather
than falling back to a permissive default.
Create the replacement under a unique name in the destination directory via
os.CreateTemp(which opensO_EXCL), thenrename. A pre-existingconfig.json.tmpis therefore never opened, never written through, and neverremoved — cleanup is scoped to the file this call created.
Apply the preserved mode with an explicit
chmodon our own descriptor. That iswhat makes the result umask-independent. The issue is right that simply copying
the upstream
WriteFilemode argument fixes neither half: the mode argument isignored for a file that already exists, and masked by the umask for one that
does not.
Carry
uid/gidover best-effort. This is a no-op in the normal case, wherethe saving process already owns the file — the only arrangement in which a
non-root server (#339) can write at all. It matters for a privileged process
rewriting a bind-mounted config owned by a host user, which would otherwise
silently become root-owned. The chown failure is deliberately not fatal, so
an unprivileged server can still save; the mutant below shows the suite would
catch it if that changed.
Two helper files (
config_owner_unix.go/_windows.go) follow the existingcmd/ingestor/stats_file_owner_*.gopattern rather than inventing a new one.Three package-level test seams (
configCreateTemp,configRename,configStatFile) make the create/write/rename/stat failure branches reachable;production always holds the
osfunctions. Test seams are an established idiomin this package (
nodeRegionQueryHook,lruClock,distanceBuildHook, …).Tests
New:
cmd/server/config_geofilter_perms_340_test.go(portable) andconfig_geofilter_perms_340_unix_test.go(umask, owner IDs).TestSaveGeoFilterPreservesMode— 0600 / 0640 / 0644 / 0664, on both a set and a removal saveTestSaveGeoFilterModeIsUmaskIndependent— 0644 under umask 0077, 0600 under umask 0000 and 0022TestSaveGeoFilterStatFailureIsFatal,TestSaveGeoFilterFailsSafelyOnUnreadableConfig,TestSaveGeoFilterDanglingSymlinkConfigFailsTestSaveGeoFilterIgnoresPreexistingTempPath(staleconfig.json.tmpis a directory), plus a stray-temp sweep after every successTestSaveGeoFilterDoesNotFollowTempSymlink,TestSaveGeoFilterDoesNotFollowTempHardLinkTestSaveGeoFilterWriteFailureCleansUpOwnTemp,TestSaveGeoFilterRenameFailureCleansUpOwnTemp,TestSaveGeoFilterCreateTempFailureTestSaveGeoFilterPreservesUnrelatedFields— incl._comment, which theConfigstruct does not modelTestSaveGeoFilterConfigLocations— top-level,data/, and precedenceTestSaveGeoFilterPreservesOwnerThe existing
TestSaveGeoFilterinroutes_test.gois unchanged and stillpasses.
Mutants
Seven, run locally against the new suite; all seven were caught.
Chmod(mode)→Chmod(0o644)PreservesMode(3 modes),ModeIsUmaskIndependent,ConfigLocations(3),PreservesUnrelatedFields,IgnoresPreexistingTempPath,DoesNotFollowTempSymlinkStatFailureIsFatalconfigPath + ".tmp"viaOpenFileinstead ofCreateTempDoesNotFollowTempSymlink,DoesNotFollowTempHardLink,IgnoresPreexistingTempPath, both cleanup testsconfigPath + ".tmp"instead of our own tempWriteFailureCleansUpOwnTemp,RenameFailureCleansUpOwnTempdelete(raw, "_comment")before marshalPreservesUnrelatedFieldsdata/config.jsonlookup entryConfigLocations/data_subdirectoryPreservesOwnerVerification
go test ./...green in all 18 modules (cmd/server97s,cmd/ingestor110s); the threeinternal/modules without test files are unchanged.go test . -run TestSaveGeoFilter -racegreen.go vet ./...clean incmd/server.sh test-all.sh— 232/232 files, incl.test-geo-filter.js,test-geofilter-draft.js,test-nodes-geo-scope-filter.js.node test-frontend-helpers.js— 709 passed.node test-e2e-playwright.jsagainst a local Go server one2e-fixture.db, prepared as in CI (freshen, inline seed, migrate, three seed files) — 132/135, 3 skipped, 0 failed.scripts/check-xss-sinks.sh --diff origin/master— clean (nopublic/**changes).deploy.yml, 1 inrelease-fast-path.yml.map[string]interface{}outside tests;cmd/serveradds no DB write.redacted-test-user/redacted-test-secret.Out of scope
The non-root container migration itself (#339). This PR only makes
SaveGeoFilterbehave correctly under it and pins that behaviour with a test.🤖 Generated with Claude Code