fix(security): create the remaining plaintext transcript artifacts owner-only - #77655
Closed
ZHJay wants to merge 1 commit into
Closed
fix(security): create the remaining plaintext transcript artifacts owner-only#77655ZHJay wants to merge 1 commit into
ZHJay wants to merge 1 commit into
Conversation
…ner-only Follow-up to NousResearch#77520, which hardened four plaintext transcript-artifact write paths and deliberately scoped these sibling call paths out to stay narrow. Same defect, same mechanism, same two primitives: - plugins/platforms/a2a/protocol.py (persist_message) — verbatim peer conversation turns; mkdir and append open were both at umask (0o755 dir / 0o644 file). Now secure_mkdir + open_private_append. - plugins/platforms/a2a/security.py (audit) — 500-char summaries of each peer exchange, append open at umask (0o644). Now open_private_append. Only the file: path.parent is HERMES_HOME, whose mode belongs to ensure_hermes_home(). - batch_runner.py (_process_batch_worker) — full trajectories appended under the CWD's data/<run>/, 0o644. Closest sibling to agent/trajectory.py. - cli.py (save_conversation) — sessions/saved was created 0o755. NousResearch#77520 fixed the snapshot file inside it and left the directory as an explicit follow-up. hermes_cli/config.py gains secure_mkdir(), the directory analogue of _secure_dir: mode at creation (no chmod-after TOCTOU window), no re-tightening of an existing dir, and skipped in managed mode so the NixOS module's setgid group-sharing (2770 + UMask=0007) survives. agent/agent_runtime_helpers.py is deliberately NOT changed: its atomic_json_write call has no mode=, but mkstemp already creates at 0o600 and _restore_file_mode is a no-op with no pre-existing file, so a fresh request dump already lands 0o600. Measured, not assumed. A test pins that invariant. File modes only — no redaction anywhere (NousResearch#43083).
This was referenced Aug 3, 2026
This was referenced Aug 4, 2026
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.
What does this PR do?
Follow-up to #77520, which hardened four plaintext transcript-artifact write paths. The sites below are the same defect at sibling call paths; they were deliberately scoped out of that PR to keep it narrow, and are called out there as explicit follow-ups.
Measured on
upstream/mainunderumask 022, before -> after:plugins/platforms/a2a/protocol.pypersist_messagea2a_conversations/dir0o7550o700<ctx>.jsonl- verbatim peer turns0o6440o600plugins/platforms/a2a/security.pyaudita2a_audit.jsonl- 500-char exchange summaries0o6440o600batch_runner.py_process_batch_workerdata/<run>/batch_N.jsonl- full trajectories, in the CWD0o6440o600cli.pysave_conversationsessions/saved/dir0o7550o700Scoped to file modes only. No redaction is added anywhere - these artifacts are replayed and audited full-fidelity, and masking a credential in a replayed path poisons the replay (#43083, guarded by
tests/agent/test_tool_call_arg_no_redaction.py). Two tests here assert content stays verbatim, so this can't drift.Severity, stated accurately
The two a2a sites are materially lower severity than #77520's trajectories and I don't want to oversell them: the plugin is opt-in (only when that platform is enabled), and both paths write under
HERMES_HOME, which is0o700by default - so on a stock install the parent already blocks the traversal.The concrete exposure is the documented
HERMES_HOME_MODEhatch:_secure_dir's docstring supports e.g.HERMES_HOME_MODE=0701so a web server can traverseHERMES_HOMEto reach a served subdir. Under that config a0o755child is genuinely world-readable - the execute-only bit on the parent is specifically chosen to allow cd-through, and a0o755a2a_conversations/then lists every peer conversation to any local account.batch_runner'sdata/<run>/batch_N.jsonlneeds no such precondition. Likesave_trajectory()it appends to the CWD, not underHERMES_HOME, so a datagen run in a checkout drops world-readable full trajectories with no protective parent at all. That one is the direct sibling of the file #77520 fixed.Why this is one PR
One logical change: finish the owner-only-at-creation mode class for plaintext transcript artifacts. It is one mechanism (
open_private_append/ mode-at-mkdir) applied at the sibling call paths of an already-open fix, which is whatAGENTS.mdasks for - "fix the whole bug class ... sibling call paths included." Splitting by directory would produce two PRs reviewed against the identical rationale and the same helper, and would leave the class half-finished in the interim. No refactor, no feature, no behavior change beyond permission bits.One site I did NOT change, deliberately
agent/agent_runtime_helpers.pycallsatomic_json_write(dump_file, ...)forrequest_dump_*.jsonwithoutmode=. That looks like the same bug and isn't - measured, not assumed:tempfile.mkstempcreates the temp file0o600, and_restore_file_modeis a no-op when there was no pre-existing file, so a fresh dump already lands0o600. The only case an explicitmode=0o600would change is overwriting an existing relaxed file - and request-dump filenames carry a microsecond timestamp, so that path is unreachable in practice; forcing it would also re-tighten a file the user widened, which is exactly what #77520's create-only contract avoids. Adding the argument would be a no-op. Instead I pinned the invariant the absent argument silently depends on, so a future refactor ofatomic_json_write's temp-file creation can't regress request dumps unnoticed.Also verified and excluded as a weaker class:
hermes_cli/session_export_md.pyandtools/delegation_live_log.py.delegation_live_logredacts unconditionally withforce=Trueat a single choke point (event()), so its content exposure is bounded.session_export_md's line 277 ismanifest.jsonl, which holds onlysession_id/ path / sha256 / counts - no transcript content - and its export path is a directory the user names with--output, which puts it on #74897's side of the line.Managed / NixOS disclosure
sessions/savedanda2a_conversationsare both underHERMES_HOME, so the newsecure_mkdir()skips managed mode, exactly as_secure_dirdoes. This is deliberate and worth flagging because an unconditionalmode=at creation would change managed behavior:nix/nixosModules.nixcreatesHERMES_HOME's subdirs setgid group-writable (2770, lines 711-719 and 743) and runs the service withUMask = "0007"(line 907) so interactive users in the hermes group can share state with the gateway. Measured:Unlike
_secure_dir's chmod, a mode passed at creation is not re-reconciled by a later activation pass, so getting this wrong would be silent._is_container()is not consulted: it gates_secure_fileonly, and this module has no container carve-out for directories.HERMES_HOME_MODEis also not honoured - that hatch is for traversal to a served subdir, and nothing is served out of these two artifact dirs.Related Issue
Follow-up to #77520 (no separate issue). Refs #77472 - the remaining file-mode items; the "exact-value redaction on every persistence path" ask there remains deliberately not implemented, per #43083.
Type of Change
Changes Made
utils.py-open_private_append(). Identical to the helper in fix(security): create plaintext transcript artifacts owner-only #77520; carried here so this branch stands alone onmain. If fix(security): create plaintext transcript artifacts owner-only #77520 lands first, the hunk is byte-identical and drops out on rebase.hermes_cli/config.py- newsecure_mkdir(), the directory analogue of_secure_dir: mode atmkdir(no chmod-after TOCTOU window), never re-applied to an existing dir, skipped in managed mode. Docstring records the_is_container/HERMES_HOME_MODEreasoning above.plugins/platforms/a2a/protocol.py-persist_message()usessecure_mkdir+open_private_append.plugins/platforms/a2a/security.py-audit()usesopen_private_append. File only:path.parentisHERMES_HOMEitself, whose mode belongs toensure_hermes_home()/_secure_dirand is user-overridable - not to this call site.batch_runner.py-_process_batch_worker()usesopen_private_append. File only;data/<run>/is a workspace directory the user names via--run_name, and fix(security): create plaintext transcript artifacts owner-only #77520 set the same precedent foragent/trajectory.py(harden the artifact, not the CWD).tests/test_transcript_artifact_file_modes_remaining.py- new, 12 tests.How to Test
scripts/run_tests.sh tests/test_transcript_artifact_file_modes_remaining.py -q- 12 passed. They assert the contract (no group/other bits on a freshly created artifact) rather than a frozen octal, exercise the real write paths against a tempHERMES_HOME/ temp CWD under a deliberately permissiveumask 022, and areskipif(os.name != "posix").Teeth check, per mechanism. Each hunk reverted individually, suite re-run, then restored. All eight turn something red - none of these is load-bearing only in aggregate:
secure_mkdir-> baremkdir)batch_runnerfile modeclisessions/saveddir modesecure_mkdirmanaged carve-out (-> unconditional0o700)secure_mkdircreate-only (-> chmod-after)open_private_appendcreate-only (-> chmod-after)The harness asserted each revert string matched exactly once before editing, so no "revert" was a silent no-op.
Regression, direct blast radius - every test file mentioning a touched module (
rg -l 'batch_runner|a2a|save_conversation|atomic_json_write|open_private_append|secure_mkdir' tests): 20 files, 321 passed, 0 failed.Regression,
hermes_cli/config.pyblast radius (it is imported nearly everywhere):rg -l 'hermes_cli.config|_secure_dir|ensure_hermes_home' tests-> 300 files, 5579 passed, 7 failed. The same 7 failures reproduce on a cleanupstream/mainworktree - macOS-environmental:test_voice_mode(3, sounddevice/WSL2),test_wake_word,test_web_providers,test_approval, and the known load-flakytest_api_server::test_health_detailed_returns_ok. I did not run the full suite; the two sets above are what I ran.Lint on changed files:
uvx ruff check(ruff 0.16.1) - all checks passed.scripts/check-windows-footguns.py --diff upstream/main- 0 findings (it caught 6 bareread_text()/write_text()calls in my test file on the first pass; fixed withencoding="utf-8").scripts/check_subprocess_stdin.py- passed.Manual, real write paths, temp
HERMES_HOME,umask 022:Precedent for this exact pattern
Verified on
upstream/main, not from memory -gateway/shutdown_flush.pyalready writes a verbatim-content artifact with both primitives:flush_dir.mkdir(parents=True, exist_ok=True, mode=0o700)atomic_json_write(final_path, payload, mode=0o600, default=str)Same modes, same reasoning: pending gateway messages are verbatim user content, so the directory is born
0o700and each payload0o600.atomic_json_write'smode=docstring names the motivation ("avoiding chmod-after-write TOCTOU exposure for secret-bearing files"). #77520 addedopen_private_appendfor the append-mode casesatomic_json_writecan't cover; this PR applies both to the paths that were left.Why this isn't a reversal of #74897
#74897 moved
write_file's new files off0600onto umask-derived0644, so the direction looks opposite. It isn't the same class of file, and the detail matters:_atomic_write's chmod branch only ranif [ -e "$t" ], so new files silently keptmktemp's0600. That0600was an accident of the temp-file mechanism, not a policy - which is why restoring umask-derived permissions was a fix and not a loosening.What #74897 protected was a path the user named, with a documented interop contract: cross-process readers by design (#70856 - Obsidian LiveSync, Docker volumes, NAS mounts). Nothing in it generalizes to artifacts the agent names and the user never types:
a2a_conversations/<ctx>.jsonl,a2a_audit.jsonl,data/<run>/batch_N.jsonl,sessions/saved/. No cross-process reader is implied by any of them, and there is no interop contract to break.Both changes also agree on the invariant #74897 actually defended - never override permissions the user set. Its regression guard is "overwrite 0755 file -> preserved." Everything here applies its mode only at creation, asserted directly by three tests (
..._existing_relaxed_file_is_not_retightenedfor the a2a log and the batch file, plussecure_mkdirpreserving an existing0o755dir). An operator who widens a batch trajectory to feed a training pipeline keeps that.Prior art searched
gh search prsfora2a,owner-only,0600,batch_runner,mkdir mode;gh search issuesfora2a permissions,a2a_conversations,world-readable transcript,file permissions 0644 umask. Nothing overlaps. The a2a plugin landed recently (837003b1e, "closes #514") and the only other open a2a PR is #77526 (feat(a2a): inject conversation history on context resume), which touchespersist_message's module but not itsmkdir/ open - it adds a reader alongside. Adjacent owner-only work in flight is on disjoint files: #77520 (parent), #77579 (browser profile / media cache), #77622 (desktopconnection.json), #77527 (Windows ACLs in_secure_file).Checklist
Code
pytest tests/ -qand all tests pass - not the full suite. I ran the two regression sets in "How to Test" (20 files / 321 passed, and 300 files / 5579 passed with 7 failures confirmed baseline on cleanupstream/main).Documentation & Housekeeping
secure_mkdircarry the managed-mode /HERMES_HOME_MODE/_is_containerreasoning. No user-facing behavior change, so nodocs/edit.cli-config.yaml.example- N/A, no config keys added.CONTRIBUTING.md/AGENTS.md- N/A.os.openhonours only the read-only bit;mkdir(mode=)is ignored), so every mode test isskipif(os.name != "posix")and the docstrings say so. Windows at-rest protection is ACL-based and belongs to fix(security): enforce owner-only ACLs on Windows in _secure_file #77527 - not duplicated here.