diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 0a5a501008..a77ae17999 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -472,10 +472,9 @@ def _emit_event_from_thread(self, event: Event) -> None: from callbacks that may run in different threads. Events are emitted through the conversation's normal event flow to ensure they are persisted. """ - if self._main_loop and self._main_loop.is_running() and self._conversation: - # Capture conversation reference for closure - conversation = self._conversation - + main_loop = self._main_loop + conversation = self._conversation + if main_loop and main_loop.is_running() and conversation: # Wrap _on_event with lock acquisition to ensure thread-safe access # to conversation state and event log during concurrent operations def locked_on_event(): @@ -500,13 +499,16 @@ def log_callback( filename: str, log_data: str, uid=usage_id, model=model_name ) -> None: """Callback to emit LLM completion logs as events.""" - event = LLMCompletionLogEvent( - filename=filename, - log_data=log_data, - model_name=model, - usage_id=uid, - ) - self._emit_event_from_thread(event) + try: + event = LLMCompletionLogEvent( + filename=filename, + log_data=log_data, + model_name=model, + usage_id=uid, + ) + self._emit_event_from_thread(event) + except Exception: + logger.exception("Failed to emit LLM completion log event") llm.telemetry.set_log_completions_callback(log_callback) diff --git a/openhands-tools/openhands/tools/apply_patch/core.py b/openhands-tools/openhands/tools/apply_patch/core.py index 34381024d0..8ed54ac420 100644 --- a/openhands-tools/openhands/tools/apply_patch/core.py +++ b/openhands-tools/openhands/tools/apply_patch/core.py @@ -59,7 +59,9 @@ def assemble_changes( old_content=old_content, ) else: - assert False + raise DiffError( + f"Unexpected state: path '{path}' exists in neither orig nor dest" + ) return commit @@ -95,13 +97,15 @@ def is_done(self, prefixes: tuple[str, ...] | None = None) -> bool: return False def startswith(self, prefix: str | tuple[str, ...]) -> bool: - assert self.index < len(self.lines), f"Index: {self.index} >= {len(self.lines)}" + if self.index >= len(self.lines): + raise DiffError(f"Unexpected end of patch at index {self.index}") if self.lines[self.index].startswith(prefix): return True return False def read_str(self, prefix: str = "", return_everything: bool = False) -> str: - assert self.index < len(self.lines), f"Index: {self.index} >= {len(self.lines)}" + if self.index >= len(self.lines): + raise DiffError(f"Unexpected end of patch at index {self.index}") line = self.lines[self.index] if line.startswith(prefix): text = line if return_everything else line[len(prefix) :] @@ -116,11 +120,15 @@ def parse(self): if path in self.patch.actions: raise DiffError(f"Update File Error: Duplicate Path: {path}") move_to = self.read_str("*** Move to: ") + if move_to and (".." in move_to.split("/") or move_to.startswith("/")): + raise DiffError( + f"Update File Error: Invalid move path '{move_to}': " + "must be a relative path without '..' components" + ) if path not in self.current_files: raise DiffError(f"Update File Error: Missing File: {path}") text = self.current_files[path] action = self.parse_update_file(text) - # TODO: Check move_to is valid action.move_path = move_to self.patch.actions[path] = action continue @@ -359,7 +367,10 @@ def identify_files_needed(text: str) -> list[str]: def _get_updated_file(text: str, action: PatchAction, path: str) -> str: - assert action.type == ActionType.UPDATE + if action.type != ActionType.UPDATE: + raise DiffError( + f"_get_updated_file: expected UPDATE action for '{path}', got {action.type}" + ) orig_lines = text.split("\n") dest_lines = [] orig_index = 0 @@ -375,7 +386,11 @@ def _get_updated_file(text: str, action: PatchAction, path: str) -> str: f"_get_updated_file: {path}: orig_index {orig_index} > " f"chunk.orig_index {chunk.orig_index}" ) - assert orig_index <= chunk.orig_index + if orig_index > chunk.orig_index: + raise DiffError( + f"_get_updated_file: {path}: orig_index {orig_index} advanced past " + f"chunk.orig_index {chunk.orig_index}" + ) dest_lines.extend(orig_lines[orig_index : chunk.orig_index]) delta = chunk.orig_index - orig_index orig_index += delta @@ -389,8 +404,16 @@ def _get_updated_file(text: str, action: PatchAction, path: str) -> str: delta = len(orig_lines) - orig_index orig_index += delta dest_index += delta - assert orig_index == len(orig_lines) - assert dest_index == len(dest_lines) + if orig_index != len(orig_lines): + raise DiffError( + f"_get_updated_file: {path}: did not consume all original lines " + f"(orig_index={orig_index}, len={len(orig_lines)})" + ) + if dest_index != len(dest_lines): + raise DiffError( + f"_get_updated_file: {path}: dest line count mismatch " + f"(dest_index={dest_index}, len={len(dest_lines)})" + ) return "\n".join(dest_lines) @@ -449,10 +472,14 @@ def apply_commit( if change.type == ActionType.DELETE: remove_fn(path) elif change.type == ActionType.ADD: - assert change.new_content is not None + if change.new_content is None: + raise DiffError(f"apply_commit: ADD change for '{path}' has no content") write_fn(path, change.new_content) elif change.type == ActionType.UPDATE: - assert change.new_content is not None + if change.new_content is None: + raise DiffError( + f"apply_commit: UPDATE change for '{path}' has no content" + ) if change.move_path: write_fn(change.move_path, change.new_content) remove_fn(path) @@ -470,7 +497,8 @@ def process_patch( Returns (message, fuzz, commit) """ - assert text.startswith("*** Begin Patch") + if not text.startswith("*** Begin Patch"): + raise DiffError("Invalid patch: must start with '*** Begin Patch'") paths = identify_files_needed(text) orig = load_files(paths, open_fn) patch, fuzz = text_to_patch(text, orig) diff --git a/openhands-tools/openhands/tools/gemini/edit/impl.py b/openhands-tools/openhands/tools/gemini/edit/impl.py index ea0448bfcd..30e08c40e6 100644 --- a/openhands-tools/openhands/tools/gemini/edit/impl.py +++ b/openhands-tools/openhands/tools/gemini/edit/impl.py @@ -1,6 +1,5 @@ """Edit tool executor implementation.""" -import os from pathlib import Path from typing import TYPE_CHECKING @@ -21,7 +20,7 @@ def __init__(self, workspace_root: str): Args: workspace_root: Root directory for file operations """ - self.workspace_root = Path(workspace_root) + self.workspace_root = Path(workspace_root).resolve() def __call__( self, @@ -43,11 +42,13 @@ def __call__( new_string = action.new_string expected_replacements = action.expected_replacements - # Resolve path relative to workspace - if not os.path.isabs(file_path): - resolved_path = self.workspace_root / file_path - else: - resolved_path = Path(file_path) + # Resolve path relative to workspace and guard against path traversal + resolved_path = (self.workspace_root / file_path).resolve() + if not str(resolved_path).startswith(str(self.workspace_root)): + return EditObservation.from_text( + is_error=True, + text=f"Error: Path traversal not allowed: {file_path}", + ) # Handle file creation (old_string is empty) if old_string == "":