Repository navigation
fix(harness): create session directories private to the user - #3137
Merged
Merged
Conversation
A sessions/ owned by another user or on a filesystem that refuses chmod started a new session on main; keep it so, as the harness's own path does.
6 of 9 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.
Description
On unix,
RIG_HOME/sessions/and every session directory were created with the default umask, usually 0755. With a traversableRIG_HOMEand the default directory store, other local users could read every conversation, spill file and traffic log.auth/was already 0700.What users see change: on unix,
sessions/and each new session directory are now mode 0700. An existing widersessions/is tightened to 0700 the next time a session starts, by whichever path creates it.RIG_HOMEitself, existing session directories inside it, and file modes are left as they are; a 0700sessions/already keeps them from other users. Windows and wasm are untouched (everything iscfg(unix)).Both creation paths change:
Home::new_session(crates/rig-harness-protocol/src/lib.rs):sessions/is set to 0700 aftercreate_dir_all, and the session directory is created withDirBuildermode 0700. Ifsessions/can't be made 0700 (another owner, or a filesystem that refuses chmod), it keeps its mode and the session starts, as on main; the new session's own directory is still 0700.paths_in(crates/rig-harness/src/host/session.rs): the same two steps. Errors are still ignored there, as before, and show up as a failed log write.The umask can only narrow the
DirBuildermode, so 0700 holds whatever the umask is. Tightening an existing directory needsset_permissions. Nopubitem is added or changed. The two crates can't share one helper without a new public item, so each crate has the few lines inline.rig-harness-protocoltests::a_new_session_directory_is_private_to_the_user(fresh home withsessions/pre-made 0755:sessions/and the new session directory end up& 0o077 == 0);rig-harnesshost::session::tests::a_session_directory_made_without_the_launcher_is_private_to_the_user(paths_inon a fresh home)There is no issue or outside PR for this. This change differs from the audit's repro diff in one way: the repro had only a test, and this test also covers tightening an existing
sessions/.Changelog
sessions/and every session directory underRIG_HOMEare created mode 0700, and an existing widersessions/is tightened when a session starts, so other local users can no longer read conversations and logs.Migration
None.
Type of change
Testing
cargo xtask verify --affected --base origin/mainpasses at the headChecklist:
new_session's# Errors)CHANGELOG.mdorMIGRATING.md(they are generated at release)Notes
Line deltas: production +21 / -2 (2 files), tests +54 / -1 (2 files).
Real use: an independent tester ran real
rig(launcher, interactive, and the agent alone) on main and this PR:sessions/and session directories 755 on main, 700 here; an existing 0755/0777/0750sessions/tightened;RIG_HOME, files and old session directories unchanged; resume works.Second fix commit (from review):
new_sessionignores a failed chmod ofsessions/, aspaths_indoes, so a group-sharedsessions/owned by another user, or one on vfat, still starts a new session as on main.Not verified: macOS.
Follow-ups (not in this PR):
RIG_HOMEitself and session directories from before this change keep their modes. A 0700sessions/already protects those directories. Other parts of the audit's proposal 3 (secret leaks in MCP header expansion and the cloak browser opener) are handled separately.