Skip to content

Enhance security & Code Quality#25

Merged
daocha merged 3 commits into
mainfrom
security-improvement
Mar 27, 2026
Merged

Enhance security & Code Quality#25
daocha merged 3 commits into
mainfrom
security-improvement

Conversation

@daocha
Copy link
Copy Markdown
Owner

@daocha daocha commented Mar 27, 2026

security: harden session_id/branch validation, workspace locking, sec)ret scrubbing, and add 29 tests

Security fixes:

  • validate session_id from LLM JSONL output with strict regex; reject flag-like values (e.g. '--exec') before they reach subprocess args
  • workspace-based asyncio.Lock (per project_folder, not per chat_id) so multiple chat IDs sharing a workspace are serialized; second concurrent call receives 'already running' message immediately
  • validate branch names in checkout_branch() and prepare_branch() with a regex allowlist; reject names starting with '-'
  • _scrub_secrets() redacts Telegram tokens, GitHub PATs, AWS keys, etc. from agent prose output before sending to Telegram; _sanitize_agent_error() strips absolute filesystem paths from error messages shown to users
  • resolve symlinks in _path_within_project() so an in-project symlink pointing outside the workspace root is rejected by /commit path validation

Code quality:

  • implement RotatingFileHandler (10 MB / 3 backups) in logging_utils.py — log_file config was previously ignored
  • wrap all 5 portalocker.Lock sites in session_store.py with SessionStoreError; bot.py handles it with a user-friendly retry message
  • watchdog thread kills the agent subprocess after AGENT_HARD_TIMEOUT_SECONDS (default 0 = disabled) — configurable for operators who need a hard cap; normal Copilot long-running jobs are unaffected
  • _sanitize_agent_error() strips internal paths from error messages
  • log OSError in diff_utils._read_snapshot_text() instead of silently swallowing it
  • structured audit log entry at INFO level for every agent invocation (chat_id, project_folder, provider, truncated prompt)
  • router/base.py: refactor _run_with_typing → _execute_with_typing to allow the workspace lock guard to wrap it cleanly
  • .gitignore: explicitly list .env (was only covered by glob patterns before)

Tests (29 new across 5 files + 1 new file):

  • test_agent_runner.py: _validate_session_id (valid, flag-like, special chars, empty), JSONL rejects flag session_id, JSONL accepts valid session_id
  • test_git_utils.py: _validate_branch_name (valid, flag-like, special chars, too long), checkout_branch rejects invalid name, prepare_branch rejects invalid new_branch and origin_branch
  • test_command_router.py: _path_within_project allows normal files, blocks symlink-to-dir outside, blocks symlink-to-file outside; workspace lock blocks concurrent call on same project, allows different projects concurrently
  • test_session_store.py: load() and save() raise SessionStoreError on lock timeout (portalocker.LockException monkeypatched)
  • test_session_runtime_security.py (new): _scrub_secrets redacts Telegram token, GitHub PAT, AWS key, leaves normal text unchanged, handles multiple secrets; _sanitize_agent_error redacts Unix path, Windows path, leaves relative paths and plain messages unchanged

daocha added 3 commits March 27, 2026 19:11
…ret scrubbing, and add 29 tests

   Security fixes:
   - validate session_id from LLM JSONL output with strict regex; reject
     flag-like values (e.g. '--exec') before they reach subprocess args
   - workspace-based asyncio.Lock (per project_folder, not per
     chat_id) so multiple chat IDs sharing a workspace are serialized; second
     concurrent call receives 'already running' message immediately
   - validate branch names in checkout_branch() and prepare_branch() with
     a regex allowlist; reject names starting with '-'
   - _scrub_secrets() redacts Telegram tokens, GitHub PATs, AWS keys, etc.
     from agent prose output before sending to Telegram; _sanitize_agent_error()
     strips absolute filesystem paths from error messages shown to users
   - resolve symlinks in _path_within_project() so an in-project symlink
     pointing outside the workspace root is rejected by /commit path validation

   Code quality:
   - implement RotatingFileHandler (10 MB / 3 backups) in
     logging_utils.py — log_file config was previously ignored
   - wrap all 5 portalocker.Lock sites in session_store.py with
     SessionStoreError; bot.py handles it with a user-friendly retry message
   - watchdog thread kills the agent subprocess after
     AGENT_HARD_TIMEOUT_SECONDS (default 0 = disabled) — configurable for
     operators who need a hard cap; normal Copilot long-running jobs are unaffected
   - _sanitize_agent_error() strips internal paths from error messages
   - log OSError in diff_utils._read_snapshot_text() instead of
     silently swallowing it
   - structured audit log entry at INFO level for every agent
     invocation (chat_id, project_folder, provider, truncated prompt)
   - router/base.py: refactor _run_with_typing → _execute_with_typing to allow
     the workspace lock guard to wrap it cleanly
   - .gitignore: explicitly list .env (was only covered by glob patterns before)

   Tests (29 new across 5 files + 1 new file):
   - test_agent_runner.py: _validate_session_id (valid, flag-like, special chars,
     empty), JSONL rejects flag session_id, JSONL accepts valid session_id
   - test_git_utils.py: _validate_branch_name (valid, flag-like, special chars,
     too long), checkout_branch rejects invalid name, prepare_branch rejects
     invalid new_branch and origin_branch
   - test_command_router.py: _path_within_project allows normal files, blocks
     symlink-to-dir outside, blocks symlink-to-file outside; workspace lock blocks
     concurrent call on same project, allows different projects concurrently
   - test_session_store.py: load() and save() raise SessionStoreError on lock
     timeout (portalocker.LockException monkeypatched)
   - test_session_runtime_security.py (new): _scrub_secrets redacts Telegram
     token, GitHub PAT, AWS key, leaves normal text unchanged, handles multiple
     secrets; _sanitize_agent_error redacts Unix path, Windows path, leaves
     relative paths and plain messages unchanged
   - Add test_logging_utils.py (setup_logging, RotatingFileHandler)
   - Add test_session_runtime_security.py (_scrub_secrets, _sanitize_agent_error)
   - Expand test_telegram_sender.py to 100% coverage (send_markdown_text,
     BadRequest re-raise, markdownish HTTP links, split edge cases,
     shell command parsing, send_code_block no-language)
   - Expand test_git_utils.py to 95% (prepare_branch branches, refresh,
     default_branch origin/HEAD parsing)
   - Expand test_session_store.py to 99% (rebind_session, replace_session,
     switch_session, _load_unlocked error cases, lock timeout on all methods)
   - Expand test_command_router.py, test_agent_runner.py, test_config.py,
     test_filters.py, test_session_runtime_diff_merge.py with targeted tests
@daocha daocha merged commit 224aef8 into main Mar 27, 2026
1 check passed
@daocha daocha deleted the security-improvement branch March 27, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant