fix: restrict file permissions on auth, config, and log files to prevent local credential theft#244
Merged
aviatco merged 13 commits intoJun 18, 2026
Conversation
…ent local credential theft
Auth files (auth.json, config.json) and debug log directories were created
with default OS permissions (0o644/0o755), making them world-readable on
multi-user systems. This exposed MSAL token caches, auth metadata, and
debug logs containing sensitive API responses to other local users.
Changes:
- fab_auth.py: Write auth.json with 0o600 via os.open() instead of open()
- fab_state_config.py: Create ~/.config/fab/ with mode 0o700; write
config.json with 0o600 via os.open() instead of open()
- fab_logger.py: Create log directory with mode 0o700
- fab_context.py: Write context-{pid}.json with 0o600 via os.open()
Note: On Windows the mode parameter is a no-op (Windows uses ACLs, not
POSIX permissions). Default Windows ACLs on user profile directories
already restrict access to the owner, so the original behavior was not
vulnerable there.
CWE-276: Incorrect Default Permissions
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens filesystem permissions for Fabric CLI’s config/auth/context/log artifacts and adds test coverage to validate that newly created directories/files are owner-restricted.
Changes:
- Restrict created directories to
0o700(config + log directories). - Restrict created files to
0o600by switching writes toos.open(..., mode=0o600)+os.fdopen. - Add tests asserting restricted permissions for config/auth/log paths and overwrites.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/fabric_cli/core/fab_state_config.py |
Create config dir with 0o700; write config via restricted file helper. |
src/fabric_cli/core/fab_logger.py |
Create log directory with 0o700. |
src/fabric_cli/core/fab_context.py |
Save context file using restricted 0o600 open. |
src/fabric_cli/core/fab_auth.py |
Save auth file using restricted 0o600 open. |
tests/test_core/test_fab_state_config.py |
Add permission tests for config dir + config file. |
tests/test_core/test_fab_logger.py |
Add permission test for log directory creation. |
tests/test_core/test_fab_auth.py |
Add permission tests for auth.json creation + overwrite. |
…s, TOCTOU fix, Windows test skip - Guard os.fdopen failures with try/except to close fd on error - Add os.chmod() after writes to tighten permissions on pre-existing files - Fix TOCTOU race in config_location() with exist_ok=True - Centralize _write_restricted_file() and reuse in auth/context modules - Skip POSIX permission assertions on Windows (os.name == 'nt') - Rewrite config_location test to exercise real function via expanduser patch - Add tests for tightening permissions on pre-existing permissive files - Update context persistence tests for _write_restricted_file usage
…ure, test cleanup - Add _chmod_if_posix() to fab_logger.py to tighten pre-existing log directories (mirrors config_location behavior) - Emit warning via logging.warning when chmod fails on POSIX instead of silently swallowing the error - Fix changelog text to accurately describe scope (directories + files) - Remove unused sys import and config_dir variable in tests
aviatco
reviewed
Jun 15, 2026
aviatco
reviewed
Jun 15, 2026
Move _write_restricted_file, _chmod_if_posix, and directory creation logic into a dedicated fabric_cli.utils.fab_file_permissions module. This addresses review feedback that classes should not depend on fab_state_config just for file permission functionality. Changes: - New utils/fab_file_permissions.py with _IS_POSIX constant, chmod_if_posix(), write_restricted_file(), create_restricted_dir() - fab_state_config.py: remove local helpers, import from utils - fab_auth.py: import from utils instead of fab_state_config - fab_context.py: import from utils instead of fab_state_config - fab_logger.py: remove local _chmod_if_posix, use shared helpers, add _RestrictedRotatingFileHandler to enforce 0o600 on rotated log files, add warning on chmod failure - Update test mocks to reference new module path Assisted-by: GitHub Copilot:claude-opus-4.6
Explicitly set encoding='utf-8' in os.fdopen for consistent JSON persistence across platforms and locales. Assisted-by: GitHub Copilot:claude-opus-4.6
aviatco
reviewed
Jun 16, 2026
…on tests - Rename fab_file_permissions.py to fab_secure_io.py for clarity - Change chmod_if_posix log level from warning to debug - Add _success suffix to all permission test names - Add log file permission test (0o600 + owner readability) - Add log rotation permission test (rotated files maintain 0o600) - Clarify 'old CLI version' comments in test assertions Assisted-by: GitHub Copilot:claude-opus-4.6
…ndler cleanup - Tighten permissions before truncation in write_restricted_file() to close the race window on pre-existing files - Rename _IS_POSIX to IS_POSIX (public constant) in fab_secure_io - Add handler cleanup in log file permission test to prevent accumulation on the global logger singleton Assisted-by: GitHub Copilot:claude-opus-4.6
Black does not read configuration from tox.toml. The target-version, line-length, and exclude settings defined there were silently ignored. Move them to pyproject.toml where Black actually reads them. Assisted-by: GitHub Copilot:claude-opus-4.6
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
…elper Move restricted file opener logic from fab_logger.py into fab_secure_io.py as get_restricted_file_opener(), which returns the POSIX opener or None on Windows. fab_logger.py no longer imports IS_POSIX directly. Assisted-by: GitHub Copilot:claude-opus-4.6
…//github.com/iemejia/fabric-cli into fix/restrict-file-permissions-sensitive-data
aviatco
reviewed
Jun 18, 2026
This reverts commit f649865.
aviatco
approved these changes
Jun 18, 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.
Summary
Auth files (
auth.json,config.json,context-{pid}.json) and debug log directories were created with default OS permissions (0o644for files,0o755for directories), making them world-readable on multi-user Linux/macOS systems. This exposed MSAL token caches, auth metadata, and debug logs containing sensitive API responses to other local users.Security Impact
~/.config/fab/0o755(world-traversable)0o700(owner-only)~/.config/fab/auth.json0o644(world-readable)0o600(owner-only)~/.config/fab/config.json0o644(world-readable)0o600(owner-only)~/.config/fab/context-{pid}.json0o644(world-readable)0o600(owner-only)0o755(world-traversable)0o700(owner-only)CWE-276: Incorrect Default Permissions
Changes
fab_auth.py: Writeauth.jsonwith0o600viaos.open()instead ofopen()fab_state_config.py: Create~/.config/fab/withmode=0o700; writeconfig.jsonwith0o600viaos.open()with a new_write_restricted_file()helperfab_logger.py: Create log directory withmode=0o700fab_context.py: Writecontext-{pid}.jsonwith0o600viaos.open()Cross-Platform Behavior
0o600/0o700correctly — resolves the vulnerabilitymodeparameter is a no-op (Windows uses ACLs, not POSIX permissions). Default Windows ACLs on user profile directories already restrict access to the owner, so the original behavior was not vulnerable there. These changes are safely a no-op on Windows.Tests
6 new tests verifying:
0o700config.jsonwritten with0o600(initial write and overwrite)auth.jsonwritten with0o600(initial write and overwrite)0o700All existing tests pass (488 total).