Repository navigation
Create the state directory private - #9
Conversation
`things.log` is opened without an explicit mode and the state directory is created with `create_dir_all`, so on a default umask they land at 0644 in a 0755 directory. That log holds every task title and note in plaintext, readable by any other local user; `auth.json` was the only thing already protected. `create_private_dir` narrows the directory it creates to 0700, tightening an existing one on upgrade too. The auth path applies it to the state directory itself; the sync path applies it to `append-log`, which leaves the state directory a plain intermediate when no `set-auth` has run. Its contents are 0600/0700 either way.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53ceb6124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| perms.set_mode(0o600); | ||
| fs::set_permissions(&path, perms)?; | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
| opts.mode(0o600); |
There was a problem hiding this comment.
When the process umask masks owner bits (for example, umask 0777), OpenOptionsExt::mode(0o600) creates the file with those bits removed because the requested mode is still filtered through the umask. The previous post-rename set_permissions(0o600) restored an exact readable mode, whereas this version can leave auth.json at 000, causing subsequent load_auth calls to fail. Set the staging file's permissions to 0600 after opening but before writing the password so there is no exposure window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4558bf: the mode is restated between open and write. Verified the mode test passes under umask 0777, which previously produced 000.
`write_auth` wrote the payload at the process umask and narrowed the mode only after the rename, leaving the plaintext password world-readable in between. The staging path is also a fixed `.tmp` name, so anything able to write the directory can pre-plant a symlink and have the password written through it. Create the staging file with `create_new` at `0600` instead, then restate the mode before writing: `open` filters the requested mode through the umask, so a restrictive one would otherwise leave the file unreadable to its owner. The rename carries the mode onto the final file. Adds a regression test that plants a symlink at the staging path and asserts the target is untouched. It fails on the previous implementation.
a53ceb6 to
b4558bf
Compare
2 participants
On a stock install the append log lands world-readable:
things.logholds every task title and note in plaintext, so any other localuser can read it.
auth.jsonwas the only thing already protected.create_private_dirnarrows the directory to 0700 when creating it, andtightens an existing one on upgrade, which covers the log, both caches and the
staging files at once.
Separately,
write_authwrote the password at the process umask and narrowedthe mode only after the rename, leaving it briefly world-readable. The staging
path is also a fixed
.tmpname, so anything able to write the directory canpre-plant a symlink and have the password written through it. It now creates the
staging file with
create_newat 0600 and lets the rename carry the mode over.Two tests cover the directory; a third plants a symlink at the staging path and
asserts the target is untouched. That one fails on the current implementation.