fix(notify): harden init against symlink/TOCTOU writes (#616) - #643
Conversation
Publish sample config through same-dir temp + exclusive link or rename, with POSIX parent fsync, so a raced symlink cannot redirect init --force. Closes #616 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Grader review: APPROVE-WITH-CHANGES. The core mechanism is right - enforcement at the syscall level (link/rename semantics), bounded refusals, fd-based parent fsync matching the run_journal pattern. Two required changes before merge:
Non-blocking: a fsyncParent failure after a successful non-force Link returns an error although the config was published (next init then says 'already exists'); os.Link fails on hard-link-less filesystems - worth a doc note. Add attack-shaped tests for the two required changes (symlinked parent pre-created, and chmod target swapped). Ping for re-grade after push. |
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
|
Re-grade requested.
Verification: |
|
Grader: APPROVE, merging. Both required changes verified: the symlinked-parent refusal now precedes temp creation and publish (d82cba0), with TestWriteFile_RefusesSymlinkedParentBeforePublish proving the target directory is untouched, and the chmod is now on the open descriptor (c489cff), with TestWriteFile_ChmodTargetSwappedMidWrite proving a swapped victim symlink stays 0644. The non-Unix gap is documented rather than silently absent. Receipt 20260801-204537-work-verify-2d51ff, CI green. |
Summary
agent-notify initnow publishes config throughstations/notify/internal/safeioinstead ofos.Stat+os.WriteFile.--forceuses rename so a symlink at the destination is replaced rather than followed.O_DIRECTORY|O_NOFOLLOW, matching the Brigadelocalio/run_journalpatterns ported into notify-only Go code.Acceptance criteria → tests
TestWriteFile_SymlinkCannotRedirectWithoutForce,TestRun_InitSymlinkCannotRedirectWriteTestWriteFile_ForceReplaceAfterSwapCannotClobberVictim,TestWriteFile_ForceReplacesSymlinkWithoutTouchingTarget,TestRun_InitForceReplacesSymlinkWithoutTouchingTarget--forceTestWriteFile_RefusesExistingWithoutForce,TestRun_InitRefusesExistingWithoutForceTestRun_InitRefusesExistingWithoutForce,TestWriteFile_SymlinkCannotRedirectWithoutForceTest plan
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work20260801-034048-work-verify-2fd004brigade work verify run --target ./stations/notify --command "go test ./..." --capture brigade-work20260801-035217-work-verify-cf0e47(covers the acceptance tests above;./scripts/verifydoes not run the Go notify suite)Closes #616
Made with Cursor