Skip to content

Refuse replacing a file the server's user cannot write - #44

Merged
barelyworkingcode merged 5 commits into
mainfrom
fix/29-unwritable-target
Oct 10, 2026
Merged

barelyworkingcode merged 5 commits into
mainfrom
fix/29-unwritable-target

Conversation

@barelyworkingcode

Copy link
Copy Markdown
Owner

Closes #29

fs_write and fs_replace now refuse a file that the server's user cannot write. Before, they replaced it and the owner changed. AtomicReplace opens an existing target O_WRONLY through the *Root before creating any temp file. EACCES becomes errTargetNotWritable, which returns io_error with a message naming write permission. EPERM falls through to the rename, so a uchg/uappnd file still names chflags.

Non-obvious decisions:

  • TestAtomicReplaceNoTempFileLeftAfterAttrFailure now plants a 0o200 target instead of 0o000, with the same assertions. A 0o000 file is now refused before the attribute copy, so a write-only file is the only way to reach the path that test covers. Both reviewers confirmed this keeps the test's intent.
  • Ownership of a writable file owned by someone else still changes on replace. That's inherent to rename. It's documented as a non-guarantee (DESIGN, ACCEPTANCE C13, README).
  • The foreign-owner tests plant a root-owned file with sudo -n and skip without passwordless sudo. The 0444 and deny-write ACL cases need no sudo and also go red without the fix.

Fix attempts: 1. Tokens: about 230k across subagents (test-writer, dev, two reviewers).

Notes (minor or theoretical, from review):

  • A file that is both uchg and 0444 still reports a bare "permission denied". That's unchanged from main, and the file is untouched.
  • A FIFO swapped in between the Lstat and the open would block the open. The existing cp path has the same window, and TOCTOU is a stated non-goal.

Review: no blocking findings (Sonnet and default-model rounds; three minors fixed in the second commit)
Red on main: TestAtomicReplaceRefusesTargetOwnedByAnotherUser, TestReplaceRefusesTargetOwnedByAnotherUser and TestWriteRefusesUnwritableTarget fail on main's code at the disk assertions (bytes "original" -> "replacement", owner uid 0 -> 501); the 0444 and ACL cases fail at the message assertion ("permission denied")
Real-app check: real binary on the devbox, with a root:wheel file in the root that the server's user cannot append to. On main, fs_write returned ok:true, replaced the bytes and changed the owner to uid 501. On this branch it returns io_error "this file is not writable…", uid stays 0, bytes are unchanged, and no temp file is left.
Break-and-restore: disabled the pre-check (if false && exists); all five new cases went red; restored byte-identical
Journey: n/a: runs in the final full verify

🤖 Generated with Claude Code

barelyworkingcode and others added 2 commits October 10, 2026 15:01
rename(2) consults only the directory, so fs_write and fs_replace replaced
a file whose mode, owner or ACL denied the server's user write access, and
the replacement came back owned by that user. AtomicReplace now opens an
existing target O_WRONLY through the root first; EACCES is refused as
errTargetNotWritable (io_error naming write permission) before any temp
file exists. EPERM is left to the rename so a file flag still names
chflags.

The attribute-failure temp-cleanup test now plants a write-only target,
since an unwritable one no longer reaches the attribute copy.

Docs: the refusal, its non-boundary status, and the ownership
non-guarantee for a writable file owned by someone else.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review minors: a deny-write ACL case needs no sudo, the foreign-owned
fixture is 0644 so group membership cannot make it writable, and the
refusal moves out of the 'consequences' list it is not part of.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@barelyworkingcode barelyworkingcode added the skip-approved PR adds a test skip or focus, on purpose label Oct 10, 2026
@barelyworkingcode

Copy link
Copy Markdown
Owner Author

skip-approved added with the watcher's approval (board decision 163). The two t.Skip calls are environmental guards in the fixture helper (running as root, no passwordless sudo). They aren't loosened assertions.

Proof that CI runs the foreign-owner cases rather than skipping them: probe commit 62e15ec turned both skips into t.Fatal, and the test run 38093476222 on it was green (ok fsmcp/internal/fsapi 12.819s). So the runner is non-root with passwordless sudo, and TestAtomicReplaceRefusesTargetOwnedByAnotherUser, TestReplaceRefusesTargetOwnedByAnotherUser and TestWriteRefusesUnwritableTarget/owned_by_another_user ran and passed. The CI job runs go test without -v, so this is the only way its log can show it. Commit f41a4fe reverts the probe, and git diff 62e15ec~1 f41a4fe is empty.

Guards on head f41a4fe: skip-focus: added test skip or focus, allowed by the skip-approved label: internal/fsapi/atomic_test.go:714, :717; tests-only ok; hygiene ok.

@barelyworkingcode
barelyworkingcode merged commit d7eb964 into main Oct 10, 2026
2 of 3 checks passed
@barelyworkingcode
barelyworkingcode deleted the fix/29-unwritable-target branch October 10, 2026 23:02
@barelyworkingcode

Copy link
Copy Markdown
Owner Author

Merged under merge-check MERGEABLE: closes #29, proven bug, CI and guards green, inside the contract, no blocking review findings, proof present, no gate paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-approved PR adds a test skip or focus, on purpose

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs_write/fs_edit overwrite a read-only (0444) file and change its owner: the rename-based replace never consults the target permission

1 participant