Skip to content

Fix security, correctness, and performance issues#1

Open
hyunyul-XCENA wants to merge 1 commit into
xcena-dev:mainfrom
hyunyul-XCENA:main
Open

Fix security, correctness, and performance issues#1
hyunyul-XCENA wants to merge 1 commit into
xcena-dev:mainfrom
hyunyul-XCENA:main

Conversation

@hyunyul-XCENA

Copy link
Copy Markdown

Summary

Competitive code review (3 reviewers: Security / Performance / Correctness) identified 23 issues. This PR fixes all Critical and Warning items plus most Suggestions.

Security (Critical)

  • CSRF prevention: Remove CORS * on POST, add Origin validation, default bind to 127.0.0.1
  • Glob injection: Validate all user-supplied IDs with SAFE_ID_RE regex; replace rglob(user_input) with exact file matching
  • Path traversal: Add resolve() + prefix checks on all 6 delete endpoints (consistent with _serve_static pattern)
  • Error message leakage: Replace str(e) with generic error messages

Correctness (Critical + Warning)

  • Hook delete index mismatch: Pass handler_index/sub_hook_index pair instead of flat index
  • macOS cwd detection: Add lsof -d cwd fallback (/proc doesn't exist on macOS)
  • Session sort TypeError: Normalize mixed int/string timestamps before sorting
  • Falsy filter: {k:v if v}if v is not None and v != "" to preserve timeout: 0
  • Fork dedup key 50→200 chars, delete-session returns 404 when nothing found

Performance (Critical + Warning)

  • TTL cache: @ttl_cache decorator on 7 expensive data-collection functions
  • get_plugins loop: Move get_skills/get_agents/get_connectors calls outside per-plugin loop
  • Full-file reads: Replace read_text() with _read_last_n_lines() in 3 functions
  • Session detail: Reduce from 5-6 file opens to 2 per session (single-pass)
  • grep subprocess elimination: Replace subprocess.run(["grep", ...]) with inline regex for customTitle
  • Stream compact counting in session X-ray instead of loading entire file
  • _read_last_n_lines buffer 10x increase + discard partial first line
  • @lru_cache on decode_project_path, optimized read_text(max_chars)

Test plan

  • Python syntax check passes
  • All API endpoints return valid JSON (health, sessions, activity, hooks, skills, plugins, alerts)
  • CSRF blocked: POST with Origin: http://evil.com → 403
  • Glob injection blocked: id=* → 400
  • Hook entries include handler_index/sub_hook_index metadata
  • Session sort works without TypeError (46 sessions sorted correctly)
  • macOS cwd detection works via lsof (verified on Darwin)
  • Frontend integration: hook delete UI should pass handler_index + sub_hook_index params

🤖 Generated with Claude Code

…de review

Security:
- Remove CORS wildcard on POST + add Origin validation to block CSRF
- Default bind to 127.0.0.1 instead of 0.0.0.0
- Add SAFE_ID_RE regex validation on all user-supplied IDs (prevent glob injection)
- Add resolve() + prefix checks on all delete endpoints (prevent symlink traversal)
- Replace str(e) error responses with generic messages

Correctness:
- Fix hook delete index mismatch: pass handler_index/sub_hook_index instead of flat index
- Add macOS lsof fallback for process cwd detection (/proc doesn't exist on macOS)
- Normalize timestamps before sorting to prevent TypeError on mixed int/string types
- Fix falsy filter ({k:v if v} → if v is not None and v != "") to preserve timeout:0
- Increase fork dedup key from 50 to 200 chars
- Return 404 when delete-session finds nothing

Performance:
- Add TTL cache decorator on 7 expensive data-collection functions
- Move get_skills/get_agents/get_connectors calls outside per-plugin loop in get_plugins
- Replace full-file read_text with _read_last_n_lines in get_activity, get_projects_summary, get_forks
- Reduce get_session_detail from 5-6 file opens to 2 per session (single-pass)
- Replace grep subprocess with inline regex for customTitle extraction
- Stream compact counting in get_session_xray instead of loading entire file
- Increase _read_last_n_lines buffer 10x and discard partial first line
- Add @lru_cache to decode_project_path
- Optimize read_text to read only max_chars bytes when limit is set
- Wrap socket.gethostbyname in try/except for VPN/hostname edge cases

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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