-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: critical session cache, async tools, and pairing security gaps #1862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,6 +200,20 @@ async def handle_approval_callback( | |
| success=False, | ||
| message="Invalid or tampered callback data" | ||
| ) | ||
|
|
||
| # Only the configured bot owner may approve or deny pairing requests | ||
| config = getattr(bot_adapter, "config", None) | ||
| expected_owner = getattr(config, "owner_user_id", None) if config else None | ||
| if expected_owner and str(expected_owner) != str(owner_user_id): | ||
| logger.warning( | ||
| "Rejected pairing callback from non-owner user %s (expected %s)", | ||
| owner_user_id, | ||
| expected_owner, | ||
| ) | ||
| return PairingApprovalResult( | ||
| success=False, | ||
| message="Only the bot owner can approve pairing requests", | ||
| ) | ||
|
Comment on lines
+207
to
+216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The guard |
||
|
|
||
| action = parsed["action"] | ||
| channel = parsed["channel"] | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -131,6 +131,7 @@ def __init__(self, session_dir: Optional[Path] = None): | |||||
| self.session_dir = Path(session_dir) if session_dir else DEFAULT_SESSION_DIR | ||||||
| self.session_dir.mkdir(parents=True, exist_ok=True) | ||||||
| self._cache: Dict[str, UnifiedSession] = {} | ||||||
| self._cache_mtimes: Dict[str, float] = {} | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type annotation mismatch with
🔧 Proposed fix- self._cache_mtimes: Dict[str, float] = {}
+ self._cache_mtimes: Dict[str, int] = {}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| self._last_session_id: Optional[str] = None | ||||||
|
|
||||||
| def _get_session_path(self, session_id: str) -> Path: | ||||||
|
|
@@ -140,6 +141,20 @@ def _get_session_path(self, session_id: str) -> Path: | |||||
| def _get_last_session_path(self) -> Path: | ||||||
| """Get the path to the last session marker file.""" | ||||||
| return self.session_dir / ".last_session" | ||||||
|
|
||||||
| def _is_cache_valid(self, session_id: str) -> bool: | ||||||
| """Return True if the in-memory cache matches the on-disk file.""" | ||||||
| if session_id not in self._cache: | ||||||
| return False | ||||||
| path = self._get_session_path(session_id) | ||||||
| if not path.exists(): | ||||||
| return False | ||||||
| try: | ||||||
| current_mtime = path.stat().st_mtime_ns | ||||||
| cached_mtime = self._cache_mtimes.get(session_id, 0) | ||||||
| return current_mtime <= cached_mtime | ||||||
| except OSError: | ||||||
| return False | ||||||
|
|
||||||
| def save(self, session: UnifiedSession) -> None: | ||||||
| """ | ||||||
|
|
@@ -200,8 +215,12 @@ def save(self, session: UnifiedSession) -> None: | |||||
| json_data = json.dumps(session.to_dict(), indent=2).encode('utf-8') | ||||||
| f.write(json_data) | ||||||
|
|
||||||
| # Update cache | ||||||
| # Update cache and track file mtime for cross-process invalidation | ||||||
| self._cache[session.session_id] = session | ||||||
| try: | ||||||
|
Comment on lines
215
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is a narrow window between the |
||||||
| self._cache_mtimes[session.session_id] = path.stat().st_mtime_ns | ||||||
| except OSError: | ||||||
| pass | ||||||
|
|
||||||
| # Update last session marker | ||||||
| self._update_last_session(session.session_id) | ||||||
|
|
@@ -221,8 +240,8 @@ def load(self, session_id: str) -> Optional[UnifiedSession]: | |||||
| Returns: | ||||||
| Session if found, None otherwise | ||||||
| """ | ||||||
| # Check cache first | ||||||
| if session_id in self._cache: | ||||||
| # Return cached session only when the on-disk file has not changed | ||||||
| if self._is_cache_valid(session_id): | ||||||
| return self._cache[session_id] | ||||||
|
|
||||||
| path = self._get_session_path(session_id) | ||||||
|
|
@@ -258,6 +277,10 @@ def load(self, session_id: str) -> Optional[UnifiedSession]: | |||||
|
|
||||||
| session = UnifiedSession.from_dict(data) | ||||||
| self._cache[session_id] = session | ||||||
| try: | ||||||
| self._cache_mtimes[session_id] = path.stat().st_mtime_ns | ||||||
| except OSError: | ||||||
| pass | ||||||
| logger.debug(f"Loaded session: {session_id}") | ||||||
| return session | ||||||
| except Exception as e: | ||||||
|
|
@@ -299,6 +322,7 @@ def delete(self, session_id: str) -> bool: | |||||
| if path.exists(): | ||||||
| path.unlink() | ||||||
| self._cache.pop(session_id, None) | ||||||
| self._cache_mtimes.pop(session_id, None) | ||||||
| logger.debug(f"Deleted session: {session_id}") | ||||||
| return True | ||||||
| return False | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider requiring owner_user_id when pairing approval is enabled.
When
expected_ownerisNoneor empty, the authorization check is skipped (line 207), allowing any user to approve pairing requests. While the test suite shows this enables CLI fallback for deployments without a configured owner, it creates a security gap ifowner_user_idis accidentally omitted in production.Consider adding validation during bot initialization to enforce that
owner_user_idis set whenunknown_user_policy="pair", rather than silently allowing unauthenticated approvals at runtime.🤖 Prompt for AI Agents