-
Notifications
You must be signed in to change notification settings - Fork 0
Harden orchestrator/auth error surfaces and remove CodeQL-flagged unsafe patterns #8
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
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 |
|---|---|---|
|
|
@@ -2684,6 +2684,7 @@ class OrchestrationAcceptedResponse(BaseModel): | |
|
|
||
|
|
||
| _ORCHESTRATION_PROGRESS_STORE: Dict[str, Dict[str, Any]] = {} | ||
| _ORCHESTRATION_PROGRESS_FILE_LOCK = threading.Lock() | ||
|
|
||
|
|
||
| def _runtime_progress_root() -> Path: | ||
|
Comment on lines
+2687
to
2690
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. issue (bug_risk): Process-local lock does not protect against concurrent writers across multiple processes or workers. This means concurrent workers (e.g., multiple gunicorn/uvicorn processes or containers sharing the same volume) can still interleave writes and corrupt the JSON file. If the file is shared across processes, use an inter-process–safe mechanism instead (e.g., OS-level file locking like |
||
|
|
@@ -2694,9 +2695,8 @@ def _runtime_progress_root() -> Path: | |
| return progress_root | ||
|
|
||
|
|
||
| def _orchestration_progress_path(run_id: str) -> Path: | ||
| safe_run_id = re.sub(r"[^a-zA-Z0-9_.-]+", "-", str(run_id or "unknown")).strip("-") or "unknown" | ||
| return _runtime_progress_root() / f"{safe_run_id}.json" | ||
| def _orchestration_progress_store_path() -> Path: | ||
| return _runtime_progress_root() / "progress_store.json" | ||
|
|
||
|
|
||
| def _build_progress_poll_url(run_id: str) -> str: | ||
|
|
@@ -2712,23 +2712,42 @@ def _save_orchestration_progress(run_id: str, payload: Dict[str, Any]) -> Dict[s | |
| normalized["run_id"] = str(run_id or normalized.get("run_id") or "") | ||
| normalized.setdefault("updated_at", datetime.utcnow().isoformat() + "Z") | ||
| _ORCHESTRATION_PROGRESS_STORE[normalized["run_id"]] = normalized | ||
| progress_path = _orchestration_progress_path(normalized["run_id"]) | ||
| progress_path.write_text(json.dumps(normalized, ensure_ascii=False, indent=2), encoding="utf-8") | ||
| progress_path = _orchestration_progress_store_path() | ||
| with _ORCHESTRATION_PROGRESS_FILE_LOCK: | ||
| persisted_payload: Dict[str, Any] = {} | ||
| try: | ||
| if progress_path.exists() and progress_path.is_file(): | ||
| existing_payload = json.loads(progress_path.read_text(encoding="utf-8")) | ||
| if isinstance(existing_payload, dict): | ||
| persisted_payload = dict(existing_payload) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed to read orchestration progress store from %s before write", | ||
| str(progress_path), | ||
| exc_info=True, | ||
| ) | ||
| persisted_payload = {} | ||
| persisted_payload[normalized["run_id"]] = normalized | ||
| progress_path.write_text(json.dumps(persisted_payload, ensure_ascii=False, indent=2), encoding="utf-8") | ||
|
Comment on lines
+2715
to
+2731
|
||
| return normalized | ||
|
|
||
|
|
||
| def _load_orchestration_progress(run_id: str) -> Dict[str, Any]: | ||
| cached = _ORCHESTRATION_PROGRESS_STORE.get(str(run_id or "")) | ||
| if isinstance(cached, dict) and cached: | ||
| return dict(cached) | ||
| progress_path = _orchestration_progress_path(run_id) | ||
| progress_path = _orchestration_progress_store_path() | ||
| try: | ||
| if progress_path.exists() and progress_path.is_file(): | ||
| payload = json.loads(progress_path.read_text(encoding="utf-8")) | ||
| if isinstance(payload, dict): | ||
| _ORCHESTRATION_PROGRESS_STORE[str(run_id or "")] = dict(payload) | ||
| return dict(payload) | ||
| with _ORCHESTRATION_PROGRESS_FILE_LOCK: | ||
| if progress_path.exists() and progress_path.is_file(): | ||
| payload = json.loads(progress_path.read_text(encoding="utf-8")) | ||
| if isinstance(payload, dict): | ||
| stored = payload.get(str(run_id or "")) | ||
| if isinstance(stored, dict): | ||
| _ORCHESTRATION_PROGRESS_STORE[str(run_id or "")] = dict(stored) | ||
| return dict(stored) | ||
| except Exception: | ||
| logger.error("Failed to load orchestration progress for run_id=%s", str(run_id or ""), exc_info=True) | ||
| return {} | ||
| return {} | ||
|
|
||
|
|
||
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.
issue (bug_risk): Swallowing all exceptions when reading SECRET_KEY_FILE makes misconfigurations hard to diagnose.
If
SECRET_KEY_FILEexists but is unreadable (permissions, encoding, etc.), thisexcept Exceptionwill silently fall back to the "not configured" path, masking the real problem. Instead, log the exception (e.g.logger.exception("Failed to read SECRET_KEY_FILE: %s", configured_file)) before falling back so file-related issues are visible in logs.