[SECURITY] Credential storage audit - replace plaintext writes in pt_hub.py - closes #52 by Ibrahim-3d · Pull Request #93 · sjackson0109/PowerTraderAI

Ibrahim-3d · 2026-05-18T16:11:09Z

Summary

Full codebase audit for direct r_key.txt/r_secret.txt plaintext reads/writes. Found and fixed direct writes in pt_hub.py GUI wizard.

Findings

File	Issue	Action
`pt_hub.py` `do_save()`	Wrote plaintext via `_atomic_write_text(key_path, api_key)`	Replaced with `SecureCredentialManager.encrypt_credentials()`
`pt_hub.py` `_read_api_files()`	Opened `r_key.txt` directly	Replaced with `decrypt_credentials()` + plaintext fallback for legacy installs
`pt_hub.py` `_clear_api_files()`	Deleted only `.txt` files	Now deletes encrypted files (`.enc`, `.pt_salt`) too
`pt_thinker.py`	References filename in error message only	No change needed (string literal, not file op)
`pt_credentials.py`	All references are authorised migration/fallback code	No change

Changes

Modified: app/pt_hub.py — 3 functions updated
New: app/test_credential_audit.py — 6 audit tests that will catch any future regressions

Test plan

6 audit tests pass
Zero direct r_key.txt writes outside pt_credentials.py
Zero direct r_key.txt reads outside pt_credentials.py
pt_hub.py uses encrypt_credentials() and decrypt_credentials()

Closes #52

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Migrates plaintext credential storage in pt_hub.py to use SecureCredentialManager from pt_credentials.py, and adds an audit test suite to enforce that no other module reads/writes plaintext credential files directly.

Changes:

Replace plaintext r_key.txt/r_secret.txt writes in pt_hub.py with SecureCredentialManager.encrypt_credentials().
Add decryption-first read path in _read_api_files() with plaintext fallback for legacy/migration.
Add test_credential_audit.py to statically scan modules for unauthorized plaintext credential I/O.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
app/pt_hub.py	Switch credential save/read to use encrypted vault via `SecureCredentialManager`.
app/test_credential_audit.py	New audit tests validating no module outside `pt_credentials.py` performs direct plaintext credential I/O.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…entialManager pt_hub.py do_save(): encrypt_credentials() replaces _atomic_write_text(r_key.txt) pt_hub.py _read_api_files(): decrypt_credentials() with plaintext fallback for legacy _clear_api_files() now deletes encrypted files (.enc, .pt_salt) alongside plaintext Audit test confirms zero direct r_key.txt / r_secret.txt writes/reads outside pt_credentials.py across entire app/ directory. 6 audit tests pass.

pt_hub.py: - _read_api_files() logs debug message on vault failure before falling back - Falls back only when has_encrypted_credentials() is False (not on any exception) - Deletes stale r_key.txt / r_secret.txt after successful encrypt_credentials() test_credential_audit.py: - Rewrote scanner to use AST (not brittle string/regex matching) - os.walk recursive scan (covers subdirectories) - Removed fragile monkey-patch; uses SecureCredentialManager(tmpdir) directly - Removed brittle substring assertions; AST checks replace them - Added encrypt+decrypt roundtrip test - Added public API method verification - Removed unused imports (threading, patch)

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

Ibrahim-3d · 2026-05-19T09:00:16Z

+            if _mgr.has_encrypted_credentials():
+                try:
+                    creds = _mgr.decrypt_credentials()
+                    if creds:
+                        return creds[0], creds[1]
+                except Exception as _e:
+                    _log.getLogger(__name__).debug(
+                        "Encrypted vault read failed, falling back to plaintext: %s", _e
+                    )
+            # Plaintext fallback for legacy installs


Fixed. _read_api_files raises RuntimeError with a clear message when vault exists but decrypt returns None/fails. The plaintext fallback only executes when has_encrypted_credentials() is False.

Ibrahim-3d · 2026-05-19T09:00:19Z

+                # Remove stale plaintext files so they cannot be read later
+                for _stale in (key_path, secret_path):
+                    try:
+                        if os.path.isfile(_stale):
+                            os.remove(_stale)
+                    except Exception:
+                        pass
+


Fixed. do_save now overwrites plaintext file content with zeros and calls os.fsync() before os.remove (best-effort secure erase). Failure to remove is logged at WARNING level so the operator is informed.

Ibrahim-3d · 2026-05-19T09:00:21Z

+        first_arg = ast.unparse(node.args[0]) if hasattr(ast, "unparse") else ""
+        if "r_key" not in first_arg and "r_secret" not in first_arg:
+            continue


Fixed. Replaced ast.unparse with _unparse_node() which falls back to inspecting ast.Constant, ast.Str, ast.Name, ast.Attribute, ast.JoinedStr on Python < 3.9. The audit is never silently a no-op on older interpreters.

Ibrahim-3d · 2026-05-19T09:00:23Z

+        for arg in node.args[1:]:
+            val = ast.unparse(arg) if hasattr(ast, "unparse") else ""


Fixed. Mode is now extracted only from node.args[1] (the second positional argument, which is mode). The previous loop over node.args[1:] was overwriting with buffering (args[2]) on open(path, mode, buffering) calls.

Ibrahim-3d · 2026-05-19T09:00:25Z

+            if fname in ALLOWLIST or fname.startswith("test_"):
+                continue


Fixed — explicit TEST_FILES set, not prefix matching.

Ibrahim-3d · 2026-05-19T09:00:27Z

+                        private_b64_state[
+                            "value"
+                        ] = priv_b64  # keep UI state consistent


Fixed. Non-standard subscript line wrap at settings['script_neural_trainer'] reformatted to a single line consistent with Black style.

Ibrahim-3d · 2026-05-19T09:00:29Z

+            # Try encrypted vault first, then fall back to plaintext
+            import logging as _log
+            from pt_credentials import SecureCredentialManager as _SCM


Fixed. Import logging and SecureCredentialManager moved to module level (try/except block). Local function imports with _SCM, _log, _mgr aliases eliminated.

pt_hub.py: - Add module-level `import logging` and `from pt_credentials import SecureCredentialManager` (try/except) so local function imports with underscore aliases (_SCM, _log, _mgr) are eliminated - _read_api_files: distinguish vault-broken from no-vault; raise RuntimeError with clear message when vault exists but decrypt fails instead of silently falling back to (now-deleted) plaintext files - do_save: overwrite plaintext files with zeros + fsync before os.remove (best-effort secure erase); log WARNING on remove failure so operator knows a stale file may remain - Fix non-standard subscript line wrap at settings["script_neural_trainer"] test_credential_audit.py: - Replace startswith("test_") exclusion with explicit TEST_FILES set so a production module named test_*.py can never silently skip the audit - Fix mode parsing: only inspect node.args[1] (mode); previously iterated all args[1:] which mistook buffering (args[2]) for the mode string - Replace ast.unparse with _unparse_node() that falls back to inspecting ast.Constant/.Str/.Name/.Attribute/.JoinedStr nodes on Python < 3.9; audit is never silently a no-op on older interpreters - Add test_unparse_fallback_handles_string_constant and test_mode_parsing_ignores_buffering_arg

Ibrahim-3d · 2026-05-19T09:31:11Z

Manual Testing Guide — PR #93: Credential Storage Audit

Prerequisites

git fetch fork && git checkout feat/52-credential-storage-audit
cd app
pip install cryptography  # if not already installed

1. Run the audit tests

python -m pytest test_credential_audit.py -v

Expected: All tests pass. Confirm test_no_direct_plaintext_writes and test_no_direct_plaintext_reads_outside_credentials_module both PASS (not skip).

2. Smoke test — encrypt + decrypt roundtrip

# Run from app/
python -c "
import tempfile, os
from pt_credentials import SecureCredentialManager
with tempfile.TemporaryDirectory() as d:
    m = SecureCredentialManager(d)
    assert not m.has_encrypted_credentials()
    ok = m.encrypt_credentials('my_api_key', 'my_secret_b64')
    assert ok, 'encrypt failed'
    assert m.has_encrypted_credentials()
    creds = m.decrypt_credentials()
    assert creds == ('my_api_key', 'my_secret_b64'), f'got {creds}'
    print('PASS: roundtrip ok')
"

3. Verify secure erase behaviour (key change)

In the GUI (pt_hub.py):

Create dummy r_key.txt and r_secret.txt in the project dir
Open the credential wizard → Save credentials
Expected: Both .txt files are gone after save
If remove fails (e.g. locked by antivirus), check app log for: WARNING: Could not remove stale plaintext credential

4. Verify vault-broken error surfaces (key change)

python -c "
import tempfile, os
from pt_credentials import SecureCredentialManager
with tempfile.TemporaryDirectory() as d:
    m = SecureCredentialManager(d)
    m.encrypt_credentials('k', 's')
    # Corrupt the vault
    open(m.encrypted_key_file, 'wb').write(b'garbage')
    result = m.decrypt_credentials()
    print('decrypt returned:', result)  # should be None, not crash
"

Expected: decrypt_credentials returns None (logged at ERROR). If called from _read_api_files in the GUI, a clear error dialog appears instead of silently returning empty credentials.

5. AST scanner on Python 3.8 (if available)

python3.8 -m pytest test_credential_audit.py::TestCredentialAudit::test_no_direct_plaintext_writes -v

Expected: Test passes (not silently skipped). The _unparse_node fallback handles ast.Constant without ast.unparse.

Rollback

git checkout main -- app/pt_hub.py app/test_credential_audit.py

Regex substitution in previous commit introduced literal newlines into the f-string body. Restored \n escape sequences. Black now passes.

…y Black Regex substitution introduced two bugs into pt_hub.py: 1. Literal newlines in f-string body (should be \n escape sequences) 2. Literal null byte in b'\x00' bytes literal (should be text backslash-x00) Both corrupted during re.subn() backslash handling in the credential audit PR. Fixed via binary patching; Black formatting applied.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

sjackson0109 · 2026-05-19T19:20:50Z

As another linting issue @Ibrahim-3d - can you address this one?

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Ibrahim-3d · 2026-05-20T14:51:27Z

@sjackson0109 lint F821 fixed in 68c339a (added missing logging + SecureCredentialManager imports). Code Quality CI now green. Ready to merge.

Ibrahim-3d requested a review from sjackson0109 as a code owner May 18, 2026 16:11

Copilot AI review requested due to automatic review settings May 18, 2026 16:11

Copilot AI reviewed May 18, 2026

View reviewed changes

Ibrahim-3d added 4 commits May 18, 2026 21:34

fix: Black formatting on pt_hub.py

1ab5fd0

fix: Black formatting on pt_hub.py

c28860d

Copilot AI review requested due to automatic review settings May 18, 2026 18:38

Ibrahim-3d force-pushed the feat/52-credential-storage-audit branch from 161458a to c28860d Compare May 18, 2026 18:38

Copilot AI reviewed May 18, 2026

View reviewed changes

fix: repair malformed f-string in do_save + apply Black formatting

6b4bce7

Regex substitution in previous commit introduced literal newlines into the f-string body. Restored \n escape sequences. Black now passes.

Copilot AI review requested due to automatic review settings May 19, 2026 12:33

Copilot AI reviewed May 19, 2026

sjackson0109 approved these changes May 19, 2026

View reviewed changes

Merge branch 'main' into feat/52-credential-storage-audit

73bf4e3

sjackson0109 self-assigned this May 19, 2026

sjackson0109 added the formatting label May 19, 2026

fix: add missing logging and SecureCredentialManager imports (F821)

68c339a

Copilot AI review requested due to automatic review settings May 20, 2026 12:09

Copilot AI reviewed May 20, 2026

Merge branch 'main' into feat/52-credential-storage-audit

c8fd33b

sjackson0109 merged commit a3a733b into sjackson0109:main May 20, 2026
11 checks passed

		for arg in node.args[1:]:
		val = ast.unparse(arg) if hasattr(ast, "unparse") else ""

Conversation

Ibrahim-3d commented May 18, 2026

Summary

Findings

Changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d commented May 19, 2026

Manual Testing Guide — PR #93: Credential Storage Audit

Prerequisites

1. Run the audit tests

2. Smoke test — encrypt + decrypt roundtrip

3. Verify secure erase behaviour (key change)

4. Verify vault-broken error surfaces (key change)

5. AST scanner on Python 3.8 (if available)

Rollback

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

sjackson0109 commented May 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Ibrahim-3d commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants