fix(sexp): create files at the umask, keep journals owner-only - #674
Open
pauliuszaleckas wants to merge 1 commit into
Open
pauliuszaleckas wants to merge 1 commit into
pauliuszaleckas wants to merge 1 commit into
Conversation
The create-only writer built its scratch file with tempfile::Builder, which creates at 0600 by design and never adjusts it, so that mode survived the rename: every file Konnect created — project, schematic, board, sym-lib-table, the Specctra DSN and the Freerouting SES — landed unreadable to a second user, a CI account or a container uid. The replace path was always right; it copies the destination's mode. A create now states which kind of file it is. Ordinary content asks for 0666 and lets the umask decide, like any other new file. Private content — only the transaction journal, which holds complete before and after images of the project — is created at 0600 and then set to 0600 on the open handle, because a creation mode is only a request: a umask of 0277 would otherwise leave a journal at 0400, and 0677 at 0000. Fixes mixelpixx#538 Co-Authored-By: Zhengzhuo Wang <175673456+wangzhengzhuo05@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
22 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #538
What was wrong
write_new_atomic_unlockedbuilt its scratch file withtempfile::Builder, whichcreates at
0600by design. Nothing adjusted it afterwards — on a create there is nodestination whose mode can be copied — so
0600survivedpersist_noclobberand becamethe created file's permanent mode. Every create-side call site was affected:
create_project,create_schematic,register_symbol_library,schematic_builder,export_specctra_dsnand its manifest, and the Freerouting SES output. The replace pathwas always correct: it copies the destination's mode onto the scratch file.
What it does now
A create-only write states which kind of file it is:
0666and the kernel applies the umask, which is exactly what
File::createwould haveproduced, and what the replace path already gets from
OpenOptions.persist_journal's.konnect-transaction-*.json, which carriescomplete before/after images of the project. It asks for
0600at creation and thensets
0600on the open handle, before any content is written and by descriptor ratherthan by path.
That second step is the correction from #555's review:
Builder::permissionssuppliesonly the creation mode, which the umask still masks, so
umask 0277would have left ajournal at
0400and0677at0000— neither of which is the0600#538 asks for.This follows the issue's stated expectation; the only thing beyond its text is that
enforcement, which the issue's "Not a bug" section implies but does not spell out.
Evidence
The issue's repro, against this branch
Built server,
create_project+create_schematic+register_symbol_libraryinto anempty directory, plus
create_symbolas the control the issue names (a different writepath,
0644before this change too). The umask is genuinely consulted, not0644hardcoded:
umask 022umask 077umask 002permcheck.kicad_propermcheck.kicad_pcbpermcheck.kicad_schsolo.kicad_schsym-lib-tablemy.kicad_sym(control)All five files the issue lists as
0600now match the control in the same directory andprocess.
Sampling the project directory while
create_projectruns catches the journal live at0600in all three runs — beside0644,0600and0664design files respectively.Existing projects
A replacement keeps the destination's mode, verified on this branch: a file chmod-ed to
0600is still0600after anadd_schematic_text, and one at0666is still0666.Anyone who already hit #538 therefore keeps
0600and mustchmodonce — this fixgoverns newly created files only. Recorded in DEV.md alongside the journal's Unix-only
privacy guarantee, which that file previously described without qualification.
Tests
writer::atomic_write_tests::created_file_modes_follow_the_creation_policyandtransaction::tests::transaction_journal_is_created_privateeach drive an#[ignore]dprobe in a child process under an explicit umask, so no test mutates the umask of the
process its parallel neighbours are writing in. The parent names the directory the child
writes into and reads the resulting modes from it directly. Expected ordinary modes are written as
literals — POSIX's
0666 & ~umaskpromise — not recomputed from the writer.Private content is asserted as the two properties themselves — owner read/write present,
and no group, other or execute bits, which together are exactly
0600— through oneshared helper both tests call.
transaction_journal_is_created_privatecompares againsta design file created by
apply_entryin the same run, atumask 000(where an ordinarycreate is
0666) andumask 277(where it is0400), so the two policies are pinnedapart in both directions rather than coinciding by accident. That coincidence is what
made #555's
assert_ne!fail under a validumask 077; nothing here compares the twounder a umask that collapses them.
writer::atomic_write_tests::atomic_create_honors_the_process_umaskkeeps thepublic-API, ambient-umask check from #555 against an
OpenOptionscontrol.Negative controls
0600default returns)atomic_create_honors_the_process_umask,created_file_modes_follow_the_creation_policy,transaction_journal_is_created_privatecreated_file_modes_follow_the_creation_policy,transaction_journal_is_created_private, both atumask 277persist_journalback on the ordinary create pathtransaction_journal_is_created_private, atumask 000Each control kills the guards that own its behaviour and nothing else; the third shows
journal privacy is pinned independently of the ordinary fix.
Validation
Run on this exact head:
cargo test --workspace --locked --lib --tests— pass (39 suites, 0 failed;konnect-sexplib 196 passed, 2 ignored — the two probes)cargo test --workspace --locked --doc— passcargo clippy --workspace --locked --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleancargo test --locked --manifest-path crates/schematic-viewer/Cargo.toml— 20 passedpython -m unittest discover -s plugin/tests— 10 passedNot runnable here: Nix flake check, PCM packaging validation (no
jsonschema), and thedependency-licence check (no
cargo-deny). No dependency or packaging file changes, soCI covers those.
Windows has no
#[cfg(unix)]branch of this code; the non-Unix compile was checked bybuilding with the two
cfg(unix)blocks inverted, with no unused-variable or dead-codewarnings.
Notes
docs/API_MIGRATIONS.mdentry: no tool argument, response field or error shapechanges — only the mode of files on disk.
umask 0277: any directory created under that umask isitself unwritable, so
create_projectcannot get as far as a file. The subprocesstests cover those umasks directly.
Risk and rollback
Contained to the create-only writer. Ordinary creates become wider under a permissive
umask — that is the fix — while the journal becomes strictly narrower or unchanged. No
behaviour depends on the mode of a file Konnect wrote, and reverting the commit restores
0600-on-create exactly.Credit
The diagnosis and the ordinary-path fix are from #555 by @wangzhengzhuo05, closed
unmerged with its review checklist recorded; that author asked for the claim to be
released. This branch answers that checklist — the private-mode enforcement, the
subprocess umask tests, and the controls — and keeps them as a co-author on the commit.
🤖 Generated with Claude Code