Repository navigation
Stop the token file inheriting permissions, and three FFI fixes - #2
Merged
Merged
Conversation
The refresh token is written to `tokens.tmp` and renamed into place. Both platforms apply the permissions we ask for when the file is created and only then: `CREATE_ALWAYS` over an existing file on Windows keeps that file's access-control list and ignores the descriptor passed to it, and `OpenOptions::mode` on unix applies to a create, not to an open. A temporary left behind by a crashed run -- or one another local account pre-created, which the working-directory config fallback makes reachable -- therefore lent its own permissions to the secret, and the rename carried them onto the store. Remove the temporary first, then create exclusively, so a race fails with an error instead of a readable token. Also in this crate: - Conditional access-control entries were dropped when a descriptor was read back, because the parser cut each entry at its first `)` and a conditional entry carries brackets of its own. A file shared with everyone through one read as private. Count brackets instead, and treat an entry that never closes as an entry rather than as nothing. - `libraries()` looked only at `HOME`, which Windows does not set, and gave up before trying the Program Files roots. It now asks the same way `discover()` does and keeps going without a home directory. - Bound the `TOKEN_USER` read by the buffer Windows filled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag
Three things across the C surface: - `tapline_job_next` took the held message, read the queue and put the message back in three separate critical sections, so two threads polling one job could both find the slot empty, both take a message, and both write the slot -- dropping one. The header promises every function is safe to call from any thread, so hold one lock for the whole call. - `tapline_set_total_concurrency` latched on a flag set when a job is spawned, but the budget is frozen the first time the pool is built, which asking for the current concurrency already does. Calling `tapline_total_concurrency()` first therefore made every later `set_total_concurrency` answer OK and change nothing. Latch where the budget is actually frozen, and check before storing so a rejected call leaves the value alone. - Asking `tapline_last_error` for the length with a null buffer answered TAPLINE_BUFFER_TOO_SMALL when there was no error to report. Out-pointer writes now go through `write_unaligned`, which costs nothing and removes an alignment precondition C has no way to promise us, and the safety comments say what is actually checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag
`Steam::connect` set `SteamAppId` with a comment claiming the process was still single-threaded. A library linked into someone else's game-server host cannot know that, and on glibc `setenv` can reallocate `environ` under a concurrent `getenv`. There is no safe way to set an environment variable in Rust and no way to hand the app id to the Steam client except through one, so the call stays -- but it is now skipped whenever the host has already set the variable, and `connect` documents the requirement rather than asserting something it cannot know. Two more in the same function: the error buffer was `[i8; 1024]`, which does not compile where `c_char` is unsigned, and it was read with `CStr::from_ptr`, which would scan past the end if Steam ever filled it without a terminator. Both are now bounded and target-independent, in safe Rust. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced 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 |
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
2 participants
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.
Requested by beingsuz · project thread
Before: saving a refresh token wrote
tokens.tmpand renamed it into place. On Windows that file was opened withCREATE_ALWAYS, which applies the access-control list we build only when the call actually creates the file; on unixOpenOptions::mode(0o600)applies at creation and not when an existing file is opened. So a temporary left behind by a crashed run, or one another local account pre-created, kept its own permissions, took the secret, and handed those permissions to the real store through the rename. A descriptor read back with a conditional access-control entry in it lost that entry entirely, so a file shared with everyone read as private.libraries()asked only forHOME, which Windows does not set, and gave up before looking in Program Files, so it came back empty on every Windows machine. Across the C surface, two threads polling one job could each take a message and each write the held slot, silently dropping one;tapline_set_total_concurrencyanswered OK and did nothing whenever anything had already built the pool; and askingtapline_last_errorfor a length with a null buffer reported the buffer too small when there was no error at all.After: the temporary is removed before the token is written and the file is created exclusively, so a race is an error rather than a readable secret — with a test that fails on the old code and passes on the new. Conditional entries are parsed by counting brackets, and an entry that never closes counts as an entry rather than as nothing.
libraries()looks the same waydiscover()does and keeps going without a home directory.tapline_job_nextholds one lock for the whole call, the concurrency latch is set where the budget is actually frozen and checked before the value is stored, and an empty error message reads as success.tapline-steamworksalso claimed in a safety comment that the process was still single-threaded when it setSteamAppId. A library linked into someone else's host cannot know that. The call stays — there is no safe way to set an environment variable in Rust and no other way to tell the Steam client which app it is — but it is skipped when the host has already set the variable, andSteam::connectnow documents the requirement instead of asserting it. Its error buffer was[i8; 1024], which does not compile wherec_charis unsigned, and was read withCStr::from_ptr, which would scan past the end of an unterminated buffer; both are fixed in safe Rust.How:
clear_temporaryruns before bothcreate_privatearms,create_with_daclusesCREATE_NEWand says why in its doc comment, and the unix arm usescreate_new(true).aces()walks the DACL body tracking bracket depth.roots()takesOption<&Path>so a machine with no home still gets the install roots. In the FFI crate the held-message lock is taken once at the top oftapline_job_next,STARTEDis latched inside the pool'sget_or_init, and out-pointer writes go throughwrite_unaligned.On the
unsafeaudit: nothing here could be moved to safe Rust without changing the C ABI or dropping the Windows access-control list, which was a deliberate choice.tapline-ffiis a C surface, so its entry points must take raw pointers;tapline-auth/src/windows_acl.rsstays the singlecfg(windows)exception it was. What changed is that the safety comments now say what is actually checked, and two of them no longer assert things that were not true.Verified:
cargo clippy --workspace --all-targetsand the same with--target x86_64-pc-windows-gnuare both clean,cargo test --workspacepasses,cargo fmt --all --checkis clean. Not verified: nothing here has been run on a real Windows machine — the Windows paths are typechecked by the cross-compile and nothing more.