fix(web): write dashboard Anthropic OAuth creds atomically with 0600 perms#11004
Closed
shaun0927 wants to merge 1 commit into
Closed
fix(web): write dashboard Anthropic OAuth creds atomically with 0600 perms#11004shaun0927 wants to merge 1 commit into
shaun0927 wants to merge 1 commit into
Conversation
…ees as other auth paths The web dashboard's Anthropic OAuth helper wrote the credential file straight to its final destination and relied on the process umask for permissions. That left the dashboard-specific path weaker than the existing auth writers, which already use owner-only permissions and safer write semantics. This change keeps the scope narrow: make the dashboard helper write via a temp file + replace, chmod the final file to owner-only, and add a focused regression test for both permission handling and atomic-write behavior. Constraint: Must preserve the existing dashboard OAuth flow and credential-pool side effects Rejected: Broader auth-storage refactor | unnecessary scope for a single verified inconsistency Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep dashboard credential writes aligned with existing auth storage semantics; do not reintroduce direct write_text() here without matching chmod/atomic behavior Tested: pytest -o addopts='' tests/hermes_cli/test_web_server_oauth_write.py tests/hermes_cli/test_web_server.py -q (78 passed) Not-tested: Cross-platform permission semantics on Windows-managed filesystems
Contributor
Author
|
This is ready for review on my side. If GitHub Actions are waiting on maintainer approval because this is a fork PR from a first-time external contributor, please approve the workflow run so the normal checks can start. |
Contributor
Author
|
Re-validated this in an isolated worktree against current Current diff is still intentionally narrow:
Targeted validation still passes locally:
So the atomic write + |
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.
Summary
Fixes #11003.
The dashboard's Anthropic OAuth helper wrote the credential file directly to its final path and relied on the process umask for permissions. That made this one write path weaker than Hermes's adjacent auth writers, which already use owner-only permissions and safer write semantics.
This PR keeps the scope intentionally narrow:
flush()+fsync()before replaceos.replace()into the final pathchmod(0600)on the final filefinallyblockWhy this shape
Recent merged Hermes fixes that land quickly tend to have three traits:
This follows that pattern. It does not refactor the broader auth stack or change the dashboard flow beyond making its credential write behavior match the repo's existing conventions.
What changed
hermes_cli/web_server.pywrite_text(...)with temp-file write +os.replace()tests/hermes_cli/test_web_server_oauth_write.py0600underumask 022Test plan