Skip to content

Comments

fix: comprehensive security hardening and bug fixes across all codebases#1299

Closed
eren-karakus0 wants to merge 1 commit intoeigent-ai:mainfrom
eren-karakus0:fix/comprehensive-security-hardening-and-bug-fixes
Closed

fix: comprehensive security hardening and bug fixes across all codebases#1299
eren-karakus0 wants to merge 1 commit intoeigent-ai:mainfrom
eren-karakus0:fix/comprehensive-security-hardening-and-bug-fixes

Conversation

@eren-karakus0
Copy link
Contributor

Summary

Comprehensive security audit and bug fix pass across all four codebases (server, electron, frontend, backend). This PR addresses 22 issues including XSS vulnerabilities, IDOR access control gaps, authentication bypasses, race conditions, path traversal vectors, and runtime crashes.

Server Fixes (8)

  • Reflected XSS in redirect callback — user-controlled code parameter injected unescaped into HTML response. Fixed with html.escape().
  • KeyError crash in MCP install — bracket access on optional dict keys. Fixed with .get() defaults.
  • IDOR on GET /mcp/users/{id} — missing user_id ownership filter allows any authenticated user to read other users' MCP configs. Added McpUser.user_id == user_id filter.
  • IDOR on DELETE /mcp/users/{id} — no ownership check before deletion. Added 403 response for non-owners.
  • Blocked user login bypassby_password and dev_login endpoints don't check Status.Block. Added status check to both.
  • auth_must None token crashoauth2_scheme(auto_error=False) can pass None token, causing unhandled exception. Added None guard + user existence check.
  • KeyError in grouped historydefaultdict factory missing total_failed_tasks key. Added with default 0.
  • Path traversal in snapshot saveapi_task_id used directly in os.path.join without sanitization. Added regex whitelist + realpath boundary check.

Electron Fixes (5)

  • Disabled code signature verificationverifyUpdateCodeSignature was set to false, allowing unsigned updates. Changed to true.
  • XSS in file parsers — xlsx, csv, pptx cell content rendered as raw HTML. Added escapeHtml() utility function.
  • Event listener accumulationstartDownload adds new listeners each call without cleanup, causing memory leaks. Added removeAllListeners() + .once().
  • Arbitrary file executionshell.openPath() executes files; changed to shell.showItemInFolder() for directory reveal.
  • OAuth token leak in logs — authorization code logged in plaintext. Replaced with [REDACTED].

Frontend Fixes (5)

  • Division by zerotaskProgress calculation crashes when taskRunning is empty. Added denominator === 0 guard.
  • XSS via dangerouslySetInnerHTMLinput.tsx and textarea.tsx render user-influenced HTML without sanitization. Wrapped with DOMPurify.sanitize().
  • Weak RNG for IDsgenerateUniqueId() used Math.random(). Replaced with crypto.getRandomValues().
  • Secret key client exposurehasStackKeys() checked VITE_STACK_SECRET_SERVER_KEY, leaking server secret to browser bundle. Removed.

Backend Fixes (4)

  • TOCTOU race conditiontask_locks dict accessed from multiple threads without synchronization. Added threading.Lock mutex to create_task_lock and get_or_create_task_lock.
  • Predictable temp filecookie_manager used fixed .tmp suffix, vulnerable to symlink attacks. Replaced with tempfile.mkstemp().
  • Absolute path traversalsanitize_env_path accepted absolute paths that could escape ~/.eigent. Now rejects all absolute paths outright.
  • Lock bypass in OAuthStateManagertool_controller.py accessed _states dict directly, bypassing the threading lock. Added remove_state() method.

Test Plan

  • 13 new pytest tests for server security fixes (server/tests/test_security_fixes.py)
  • 4 new pytest tests for task lock mutex thread safety (backend/tests/app/service/test_task_lock_mutex.py)
  • 6 new pytest tests for OAuth state manager lifecycle (backend/tests/app/utils/test_oauth_state_manager.py)
  • 2 new pytest tests for cookie manager temp file uniqueness (backend/tests/app/utils/test_cookie_manager.py)
  • 1 existing test updated for absolute path rejection (backend/tests/app/component/test_environment.py)
  • 4 new vitest tests for CSPRNG ID generation and secret key removal (test/unit/lib/securityFixes.test.ts)
  • 7 new vitest tests for escapeHtml utility (test/unit/electron/fileReader.test.ts)
  • 6 new vitest tests for division-by-zero guard (test/unit/store/chatStore-divisionByZero.test.ts)

Total: 43 new tests across pytest and vitest

Server (8 fixes):
- Prevent reflected XSS in redirect callback via html.escape
- Fix KeyError crash in MCP install using .get() with defaults
- Add IDOR protection to GET/DELETE /mcp/users endpoints
- Block login for users with Status.Block in both password and OAuth flows
- Guard auth_must against None tokens and deleted users
- Add missing total_failed_tasks key to grouped history defaultdict
- Prevent path traversal in snapshot image save via regex + realpath check

Electron (5 fixes):
- Enable code signature verification for auto-updater
- Add escapeHtml utility to sanitize xlsx/csv/pptx parser output
- Fix event listener accumulation in update download flow
- Replace shell.openPath with shell.showItemInFolder for directories
- Redact OAuth tokens from log output

Frontend (5 fixes):
- Guard against division by zero in task progress calculation
- Sanitize dangerouslySetInnerHTML with DOMPurify in input and textarea
- Replace Math.random with crypto.getRandomValues for ID generation
- Remove VITE_STACK_SECRET_SERVER_KEY from client-side hasStackKeys check

Backend (4 fixes):
- Add threading.Lock mutex to protect task_locks from race conditions
- Use tempfile.mkstemp for unique temp files in cookie manager
- Reject absolute paths in sanitize_env_path to prevent traversal
- Add remove_state() method to OAuthStateManager for proper lock usage

Includes 43 new tests covering all fixes across pytest and vitest.
Copy link
Collaborator

@bytecii bytecii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split the pr? Thanks.

@eren-karakus0
Copy link
Contributor Author

Closed in favor of split PRs as requested:

Each PR is now scoped to a single codebase for easier review.

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.

2 participants