fix(backend): security hardening for environment, cookies, oauth, and task locking#1311
fix(backend): security hardening for environment, cookies, oauth, and task locking#1311eren-karakus0 wants to merge 3 commits intoeigent-ai:mainfrom
Conversation
… task locking - Restrict environment variable access to an explicit allowlist - Add SameSite/Secure/HttpOnly flags to cookie_manager - Add CSRF token validation to oauth_state_manager - Fix race condition in task service with proper lock/mutex handling - Sanitize tool_controller path parameter - Add tests for all changes
|
could @eren-karakus0 link the issue? |
|
Linked - closes #1328. Sorry for the delay! |
bytecii
left a comment
bytecii
left a comment
There was a problem hiding this comment.
In general LGTM. Left some comments. Thanks!
backend/app/component/environment.py
Outdated
| logger.warning( | ||
| f"Security: Rejected absolute env_path. Path: {env_path}" | ||
| ) | ||
| return None |
There was a problem hiding this comment.
why we also reject the absolute path such as ~/.eigent/some-user/.env which is within the .eigent?
There was a problem hiding this comment.
Restored the original logic. Absolute paths within ~/.eigent/ are now accepted again. The boundary check via resolved_path.relative_to(base_resolved) still prevents escapes outside the safe directory. Updated the corresponding test as well.
| return create_task_lock(id) | ||
|
|
||
|
|
||
| async def delete_task_lock(id: str): |
There was a problem hiding this comment.
Should we also apply the mutex for other functions such as delete_task_lock, get_task_lock, and get_task_lock_if_exists?
There was a problem hiding this comment.
Applied _task_locks_mutex to all three: get_task_lock, get_task_lock_if_exists, and delete_task_lock. For delete_task_lock, used a split-mutex approach - holds the lock for dict lookup and deletion, but releases it during await task_lock.cleanup() to avoid blocking other threads during async cleanup.
There was a problem hiding this comment.
Let's add the tests within the service/test_task.py directly for now
There was a problem hiding this comment.
Done - merged all 4 mutex tests into test_task.py using def test_xxx style with a clean_task_locks_with_mutex fixture for proper cleanup. Deleted test_task_lock_mutex.py.
- Restore original absolute path handling in environment.py (allow absolute paths within env_base_dir boundary) - Add _task_locks_mutex to get_task_lock, get_task_lock_if_exists, and delete_task_lock for thread safety - Merge mutex tests from test_task_lock_mutex.py into test_task.py using def test_xxx style - Update test_environment.py to match restored path behavior
- Remove unused tempfile import in test_cookie_manager.py - Fix line length formatting in task.py and test_oauth_state_manager.py
3 participants
Related Issue
Closes #1328
Split from #1299 as requested by @bytecii.
Summary
Security hardening for the backend codebase.
Changes
~/.eigent/allowed, path traversal blocked)Tests
test_environment.py- path validation: traversal, absolute within base, symlink escapetest_task.py- mutex tests merged in: thread safety, idempotent get_or_create, duplicate detectiontest_cookie_manager.py- secure cookie flag validationtest_oauth_state_manager.py- CSRF token validationFiles (9)
backend/app/component/environment.pybackend/app/controller/tool_controller.pybackend/app/service/task.pybackend/app/utils/cookie_manager.pybackend/app/utils/oauth_state_manager.pybackend/tests/app/component/test_environment.pybackend/tests/app/service/test_task.pybackend/tests/app/utils/test_cookie_manager.pybackend/tests/app/utils/test_oauth_state_manager.py