feat(sessions): optional AES-256-GCM encryption for saved sessions#8
feat(sessions): optional AES-256-GCM encryption for saved sessions#8JessicaMulein wants to merge 13 commits into
Conversation
Port Vision integration (session, http_api, git_workspace, todos) from aider_vision_core with async bridge to cecli coders. Adds bright-vision-core-serve entrypoint, fastapi/uvicorn deps, and basic HTTP/workspace pytest. Co-authored-by: Cursor <cursoragent@cursor.com>
Add --session-encrypt with CECLI_SESSION_KEY or --session-key-file, wire encrypt/decrypt through SessionManager save/load/list, and document usage. Plaintext JSON remains the default when encryption is off. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Static Code Review 📊 🛑 10 quality checks failed!
|
📝 WalkthroughWalkthroughThis PR adds optional AES-256-GCM encryption for cecli session files, enabling encrypted on-disk persistence. A new ChangesSession Encryption Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd optional AES-256-GCM encryption for saved sessions
WalkthroughsDescription• Adds optional AES-256-GCM encryption for session files on disk • Introduces --session-encrypt and --session-key-file CLI arguments • Implements session_crypto module with encrypt/decrypt utilities • Integrates encryption into SessionManager save/load/list operations • Maintains backward compatibility with plaintext JSON sessions • Adds 26 comprehensive tests for crypto, args, and session management Diagramflowchart LR
CLI["CLI Arguments<br/>--session-encrypt<br/>--session-key-file"]
KEY["Key Resolution<br/>CECLI_SESSION_KEY<br/>or key file"]
CRYPTO["session_crypto module<br/>AES-256-GCM<br/>MAGIC header"]
MANAGER["SessionManager<br/>save/load/list"]
DISK["Session Files<br/>Encrypted or Plaintext"]
CLI --> KEY
KEY --> CRYPTO
CRYPTO --> MANAGER
MANAGER --> DISK
File Changes2. cecli/session_crypto.py
|
Code Review by Qodo
1. UnicodeDecodeError not handled
|
| parsed = json.loads(data.decode("utf-8")) | ||
| if not isinstance(parsed, dict): | ||
| self.io.tool_error("Invalid session format.") | ||
| return None | ||
| return parsed | ||
| except session_crypto.SessionCryptoError as e: | ||
| self.io.tool_error(str(e)) | ||
| return None | ||
| except json.JSONDecodeError as e: | ||
| self.io.tool_error(f"Error loading session: {e}") | ||
| return None |
There was a problem hiding this comment.
1. Unicodedecodeerror not handled 🐞 Bug ☼ Reliability
SessionManager._read_session_file decodes bytes as UTF-8 but does not catch UnicodeDecodeError, so a non-UTF8/corrupted plaintext session file can crash session loading instead of producing a user-facing error and returning False.
Agent Prompt
## Issue description
`SessionManager._read_session_file()` decodes plaintext session bytes via `data.decode("utf-8")` and only catches `json.JSONDecodeError` and `SessionCryptoError`. If the file contains invalid UTF-8, `UnicodeDecodeError` will propagate and can terminate `/load-session`.
## Issue Context
- This affects plaintext session files (or corrupted files) and results in an unhandled exception instead of a clean `tool_error(...)` + `return None` path.
- `session_crypto.decrypt_session_bytes()` has the same plaintext decode pattern and also misses `UnicodeDecodeError`.
## Fix Focus Areas
- cecli/sessions.py[33-62]
- cecli/session_crypto.py[69-80]
## Suggested fix
1. In `SessionManager._read_session_file()`, add an `except UnicodeDecodeError as e:` handler (ideally alongside JSON errors) that emits a clear message (eg "Invalid session file encoding") and returns `None`.
2. Also update `session_crypto.decrypt_session_bytes()` plaintext branch to catch `UnicodeDecodeError` and raise `SessionCryptoError("Invalid session file encoding.")` (defensive for other call sites).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cecli/session_crypto.py (1)
95-96: 💤 Low valueMinimum payload length check could be more precise.
GCM produces a 16-byte authentication tag appended to the ciphertext. For an empty plaintext, the minimum blob size would be
12 (nonce) + 16 (tag) = 28bytes. The current check (< 13) allows through payloads that will definitely fail decryption due to a truncated tag.The decryption will still fail gracefully (caught at line 100-101), but the error message "payload is too short" would be more accurate with a threshold of 28.
Suggested fix
- if len(blob) < 13: + if len(blob) < 28: # 12-byte nonce + 16-byte GCM tag minimum raise SessionCryptoError("Encrypted session payload is too short.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cecli/session_crypto.py` around lines 95 - 96, The minimum-length check for the encrypted payload is too low: update the condition that raises SessionCryptoError when validating blob (the variable named blob in session_crypto.py) so it requires at least 28 bytes (12-byte nonce + 16-byte GCM tag) instead of 13; adjust the check and error message raised by SessionCryptoError accordingly so truncated-tag cases are rejected early with the accurate "Encrypted session payload is too short." message.tests/basic/test_sessions_manager.py (1)
154-172: ⚡ Quick winAdd a regression test for
list_sessionswithsession_encrypt=Falseand key present.You already cover this mode for
load_session; adding the list-path equivalent would prevent behavior drift between listing and loading encrypted sessions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/basic/test_sessions_manager.py` around lines 154 - 172, Add a new regression test mirroring the load_session case to ensure list_sessions respects session_encrypt=False when a key is present: create a test similar to test_list_encrypted_placeholder_without_key (or add a new test function) that sets up SessionManager and workspace, ensures the encryption key env var (session_crypto.KEY_ENV) is present, sets encrypt_coder.args.session_encrypt=False (and session_key_file=None), calls manager.list_sessions(), and asserts the returned row still shows ["encrypted"] model and encrypted True; target SessionManager.list_sessions behavior to prevent divergence from load_session handling of the session_encrypt flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cecli/sessions.py`:
- Around line 51-61: The _read_session_file code can raise UnicodeDecodeError
when decoding non-UTF8 plaintext; add an except UnicodeDecodeError handler
alongside the existing except json.JSONDecodeError and
session_crypto.SessionCryptoError handlers in the function (the block that calls
json.loads(data.decode("utf-8"))), call self.io.tool_error with the error (e.g.
"Error loading session: {e}") and return None so non-UTF8 files are treated as
invalid sessions rather than letting the exception bubble up.
- Around line 129-133: The list_sessions function currently checks for
encryption using _session_encrypt_settings(), tying key resolution to the
session_encrypt flag and causing encrypted session files to appear locked even
when a valid key is available (mismatching load_session behavior); update
list_sessions to resolve the decrypt key the same way load_session does (i.e.,
use the same key-resolution path rather than relying on
_session_encrypt_settings()'s session_encrypt boolean), so when
session_crypto.is_encrypted_payload(raw) is true you call the same key lookup
used by load_session and treat the session as unlocked if that lookup returns a
usable key.
In `@cecli/website/docs/usage/sessions.md`:
- Around line 163-170: Update the sessions.md text near the session-encrypt
example (the block using --session-encrypt and CECLI_SESSION_KEY) to add a clear
note that encrypted session files are not readable JSON on disk and require the
same key to be provided to read them; explicitly state that the
CECLI_SESSION_KEY environment variable or the --session-key-file must be
available when using commands that read sessions (e.g., /load-session and
/list-sessions), so users won’t mistake normal encrypted files for corrupted
JSON.
In `@tests/basic/test_session_crypto.py`:
- Around line 88-100: The test test_cryptography_import_error may not trigger
ImportError if the target module is already cached; before patching
builtins.__import__ in the test, remove
"cryptography.hazmat.primitives.ciphers.aead" (and optionally its parent
packages like "cryptography.hazmat.primitives.ciphers" and
"cryptography.hazmat.primitives") from sys.modules, then apply the monkeypatch
and call session_crypto.encrypt_session_dict to ensure the fake_import runs;
after the assertion, restore any removed modules or avoid persistent mutation so
other tests are unaffected.
---
Nitpick comments:
In `@cecli/session_crypto.py`:
- Around line 95-96: The minimum-length check for the encrypted payload is too
low: update the condition that raises SessionCryptoError when validating blob
(the variable named blob in session_crypto.py) so it requires at least 28 bytes
(12-byte nonce + 16-byte GCM tag) instead of 13; adjust the check and error
message raised by SessionCryptoError accordingly so truncated-tag cases are
rejected early with the accurate "Encrypted session payload is too short."
message.
In `@tests/basic/test_sessions_manager.py`:
- Around line 154-172: Add a new regression test mirroring the load_session case
to ensure list_sessions respects session_encrypt=False when a key is present:
create a test similar to test_list_encrypted_placeholder_without_key (or add a
new test function) that sets up SessionManager and workspace, ensures the
encryption key env var (session_crypto.KEY_ENV) is present, sets
encrypt_coder.args.session_encrypt=False (and session_key_file=None), calls
manager.list_sessions(), and asserts the returned row still shows ["encrypted"]
model and encrypted True; target SessionManager.list_sessions behavior to
prevent divergence from load_session handling of the session_encrypt flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3386c5a-318c-48e1-a0f3-c90ad4ef4099
📒 Files selected for processing (9)
cecli/args.pycecli/session_crypto.pycecli/sessions.pycecli/website/docs/usage/sessions.mdrequirements/requirements.intests/basic/conftest.pytests/basic/test_session_args.pytests/basic/test_session_crypto.pytests/basic/test_sessions_manager.py
| parsed = json.loads(data.decode("utf-8")) | ||
| if not isinstance(parsed, dict): | ||
| self.io.tool_error("Invalid session format.") | ||
| return None | ||
| return parsed | ||
| except session_crypto.SessionCryptoError as e: | ||
| self.io.tool_error(str(e)) | ||
| return None | ||
| except json.JSONDecodeError as e: | ||
| self.io.tool_error(f"Error loading session: {e}") | ||
| return None |
There was a problem hiding this comment.
Handle non-UTF8 plaintext files in _read_session_file.
Line 51 can raise UnicodeDecodeError, which is currently uncaught and can bubble up during load. Treat it as an invalid session read and return None like JSON parse failures.
Proposed fix
- except json.JSONDecodeError as e:
+ except (UnicodeDecodeError, json.JSONDecodeError) as e:
self.io.tool_error(f"Error loading session: {e}")
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parsed = json.loads(data.decode("utf-8")) | |
| if not isinstance(parsed, dict): | |
| self.io.tool_error("Invalid session format.") | |
| return None | |
| return parsed | |
| except session_crypto.SessionCryptoError as e: | |
| self.io.tool_error(str(e)) | |
| return None | |
| except json.JSONDecodeError as e: | |
| self.io.tool_error(f"Error loading session: {e}") | |
| return None | |
| parsed = json.loads(data.decode("utf-8")) | |
| if not isinstance(parsed, dict): | |
| self.io.tool_error("Invalid session format.") | |
| return None | |
| return parsed | |
| except session_crypto.SessionCryptoError as e: | |
| self.io.tool_error(str(e)) | |
| return None | |
| except (UnicodeDecodeError, json.JSONDecodeError) as e: | |
| self.io.tool_error(f"Error loading session: {e}") | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cecli/sessions.py` around lines 51 - 61, The _read_session_file code can
raise UnicodeDecodeError when decoding non-UTF8 plaintext; add an except
UnicodeDecodeError handler alongside the existing except json.JSONDecodeError
and session_crypto.SessionCryptoError handlers in the function (the block that
calls json.loads(data.decode("utf-8"))), call self.io.tool_error with the error
(e.g. "Error loading session: {e}") and return None so non-UTF8 files are
treated as invalid sessions rather than letting the exception bubble up.
| raw = session_file.read_bytes() | ||
| if session_crypto.is_encrypted_payload(raw): | ||
| _, key = self._session_encrypt_settings() | ||
| if not key: | ||
| sessions.append( |
There was a problem hiding this comment.
Key resolution in list_sessions is incorrectly tied to session_encrypt.
Line 131 uses _session_encrypt_settings(), so when session_encrypt=False, encrypted sessions are always treated as locked even if a valid env/key-file key exists. This diverges from load_session behavior and can hide metadata unnecessarily.
Proposed fix
- _, key = self._session_encrypt_settings()
+ args = getattr(self.coder, "args", None)
+ key_file = getattr(args, "session_key_file", None) if args else None
+ key = session_crypto.resolve_key(key_file=key_file)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raw = session_file.read_bytes() | |
| if session_crypto.is_encrypted_payload(raw): | |
| _, key = self._session_encrypt_settings() | |
| if not key: | |
| sessions.append( | |
| raw = session_file.read_bytes() | |
| if session_crypto.is_encrypted_payload(raw): | |
| args = getattr(self.coder, "args", None) | |
| key_file = getattr(args, "session_key_file", None) if args else None | |
| key = session_crypto.resolve_key(key_file=key_file) | |
| if not key: | |
| sessions.append( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cecli/sessions.py` around lines 129 - 133, The list_sessions function
currently checks for encryption using _session_encrypt_settings(), tying key
resolution to the session_encrypt flag and causing encrypted session files to
appear locked even when a valid key is available (mismatching load_session
behavior); update list_sessions to resolve the decrypt key the same way
load_session does (i.e., use the same key-resolution path rather than relying on
_session_encrypt_settings()'s session_encrypt boolean), so when
session_crypto.is_encrypted_payload(raw) is true you call the same key lookup
used by load_session and treat the session as unlocked if that lookup returns a
usable key.
| When enabled, session files on disk are encrypted (plaintext JSON is unchanged when disabled). | ||
|
|
||
| ```bash | ||
| export CECLI_SESSION_KEY="$(python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(32)).decode())')" | ||
| cecli --session-encrypt --auto-save | ||
| ``` | ||
|
|
||
| Or use `--session-key-file` pointing at a file with the same urlsafe-base64 32-byte key. BrightVision stores the key in the OS keychain and sets `CECLI_SESSION_KEY` for the Vision API process. |
There was a problem hiding this comment.
Clarify that encrypted session files are not JSON and require a key to read.
This section should explicitly note that encrypted session files won’t be valid JSON on disk and that /load-session or /list-sessions needs CECLI_SESSION_KEY or --session-key-file; otherwise users may misdiagnose normal encrypted files as corrupted.
Suggested doc patch
### Optional encryption (AES-256-GCM)
When enabled, session files on disk are encrypted (plaintext JSON is unchanged when disabled).
```bash
export CECLI_SESSION_KEY="$(python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(32)).decode())')"
cecli --session-encrypt --auto-saveOr use --session-key-file pointing at a file with the same urlsafe-base64 32-byte key. BrightVision stores the key in the OS keychain and sets CECLI_SESSION_KEY for the Vision API process.
+
+> Note: encrypted session files are intentionally not readable JSON on disk.
+> To load or list encrypted sessions, provide the same key via CECLI_SESSION_KEY
+> or --session-key-file.
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @cecli/website/docs/usage/sessions.md around lines 163 - 170, Update the
sessions.md text near the session-encrypt example (the block using
--session-encrypt and CECLI_SESSION_KEY) to add a clear note that encrypted
session files are not readable JSON on disk and require the same key to be
provided to read them; explicitly state that the CECLI_SESSION_KEY environment
variable or the --session-key-file must be available when using commands that
read sessions (e.g., /load-session and /list-sessions), so users won’t mistake
normal encrypted files for corrupted JSON.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| def test_cryptography_import_error(monkeypatch): | ||
| import builtins | ||
|
|
||
| real_import = builtins.__import__ | ||
|
|
||
| def fake_import(name, *args, **kwargs): | ||
| if name == "cryptography.hazmat.primitives.ciphers.aead": | ||
| raise ImportError("blocked for test") | ||
| return real_import(name, *args, **kwargs) | ||
|
|
||
| monkeypatch.setattr(builtins, "__import__", fake_import) | ||
| with pytest.raises(session_crypto.SessionCryptoError, match="cryptography"): | ||
| session_crypto.encrypt_session_dict({"version": 1}, os.urandom(32)) |
There was a problem hiding this comment.
Test may not trigger ImportError if cryptography is already imported.
Python caches imported modules in sys.modules. If cryptography.hazmat.primitives.ciphers.aead was imported by an earlier test (e.g., test_roundtrip_encrypted), the patched __import__ won't be called—Python returns the cached module directly. This could make the test pass spuriously or become order-dependent.
To reliably test the ImportError path, also clear the module from sys.modules before the patched call.
Suggested fix
def test_cryptography_import_error(monkeypatch):
import builtins
+ import sys
real_import = builtins.__import__
def fake_import(name, *args, **kwargs):
if name == "cryptography.hazmat.primitives.ciphers.aead":
raise ImportError("blocked for test")
return real_import(name, *args, **kwargs)
monkeypatch.setattr(builtins, "__import__", fake_import)
+ # Remove cached module so __import__ is actually called
+ monkeypatch.delitem(sys.modules, "cryptography.hazmat.primitives.ciphers.aead", raising=False)
with pytest.raises(session_crypto.SessionCryptoError, match="cryptography"):
session_crypto.encrypt_session_dict({"version": 1}, os.urandom(32))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_cryptography_import_error(monkeypatch): | |
| import builtins | |
| real_import = builtins.__import__ | |
| def fake_import(name, *args, **kwargs): | |
| if name == "cryptography.hazmat.primitives.ciphers.aead": | |
| raise ImportError("blocked for test") | |
| return real_import(name, *args, **kwargs) | |
| monkeypatch.setattr(builtins, "__import__", fake_import) | |
| with pytest.raises(session_crypto.SessionCryptoError, match="cryptography"): | |
| session_crypto.encrypt_session_dict({"version": 1}, os.urandom(32)) | |
| def test_cryptography_import_error(monkeypatch): | |
| import builtins | |
| import sys | |
| real_import = builtins.__import__ | |
| def fake_import(name, *args, **kwargs): | |
| if name == "cryptography.hazmat.primitives.ciphers.aead": | |
| raise ImportError("blocked for test") | |
| return real_import(name, *args, **kwargs) | |
| monkeypatch.setattr(builtins, "__import__", fake_import) | |
| # Remove cached module so __import__ is actually called | |
| monkeypatch.delitem(sys.modules, "cryptography.hazmat.primitives.ciphers.aead", raising=False) | |
| with pytest.raises(session_crypto.SessionCryptoError, match="cryptography"): | |
| session_crypto.encrypt_session_dict({"version": 1}, os.urandom(32)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/basic/test_session_crypto.py` around lines 88 - 100, The test
test_cryptography_import_error may not trigger ImportError if the target module
is already cached; before patching builtins.__import__ in the test, remove
"cryptography.hazmat.primitives.ciphers.aead" (and optionally its parent
packages like "cryptography.hazmat.primitives.ciphers" and
"cryptography.hazmat.primitives") from sys.modules, then apply the monkeypatch
and call session_crypto.encrypt_session_dict to ensure the fake_import runs;
after the assertion, restore any removed modules or avoid persistent mutation so
other tests are unaffected.
Summary
cecli/session_crypto.py(AES-256-GCM, magic headerCECLI_ENCRYPTED_SESSION_v1)--session-encrypt/--no-session-encryptand--session-key-fileon the CLISessionManagerencrypts on save and decrypts on load/list when enabled; plaintext JSON unchanged when offCECLI_SESSION_KEY(urlsafe base64, 32 bytes) or key filecryptography>=42in requirementstests/basic/test_session_{crypto,args,sessions_manager}.pyBrightVision
Parent repo PR will pin this commit and add Vision API + Tauri keychain wiring.
Test plan
python -m pytest tests/basic/test_session_crypto.py tests/basic/test_session_args.py tests/basic/test_sessions_manager.py -qMade with Cursor
PR Summary by Typo
Overview
This PR introduces optional AES-256-GCM encryption for saved sessions, enhancing data security for sensitive information stored on disk. It allows users to encrypt session files using a provided key, with backward compatibility for unencrypted sessions.
Key Changes
--session-encryptand--session-key-filecommand-line arguments to enable and configure session encryption.cecli/session_crypto.pycontaining AES-256-GCM encryption and decryption logic.cecli/sessions.pyto handle encrypted and unencrypted session files during saving, loading, and listing.cryptographyas a new dependency for encryption functionality.Work Breakdown
To turn off PR summary, please visit Notification settings.
Summary by CodeRabbit
Release Notes
New Features
--session-encryptCLI flag to enable session encryption--session-key-fileCLI option to specify a file containing the encryption keyDocumentation