-
Notifications
You must be signed in to change notification settings - Fork 6
fix: prevent /generate 502 caused by event loop mismatch + add e2e tests #33
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 |
|---|---|---|
|
|
@@ -86,6 +86,16 @@ def _setup_routes(self) -> None: | |
| ) | ||
|
|
||
| async def _start_background_health_check(self) -> None: | ||
| # Probe capability for pre-registered workers in the active server loop. | ||
| unknown_workers = [ | ||
| url for url, support in self.worker_video_support.items() if support is None | ||
| ] | ||
| if unknown_workers: | ||
| await asyncio.gather( | ||
| *(self.refresh_worker_video_support(url) for url in unknown_workers), | ||
| return_exceptions=True, | ||
| ) | ||
|
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. I still feel like it shouldn't be an one-off check, we might want to have one specific loop task to check it periodically. Also putting this refresh here changes the semnaitc of
Collaborator
Author
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. agree. Maybe we can open a separate PR to implement this, and keep this PR focused on decoupling the video support logic from |
||
|
|
||
| if self._health_task is None or self._health_task.done(): | ||
| self._health_task = asyncio.create_task(self._health_check_loop()) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| """Pytest configuration: force local src import precedence.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| src_str = str(Path(__file__).resolve().parent.parent / "src") | ||
| while src_str in sys.path: | ||
| sys.path.remove(src_str) | ||
| sys.path.insert(0, src_str) |
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.
The
_start_background_health_checkfunction initiates network requests to worker URLs that are not sufficiently validated. The validation logic innormalize_worker_url(used when workers are registered) does not block private IP addresses (RFC 1918) and can be bypassed using a trailing dot in the hostname (e.g.,169.254.169.254.). This allows an attacker to probe internal network services or cloud metadata endpoints by registering malicious worker URLs via the/add_workerendpoint.To remediate this, ensure that
normalize_worker_urlstrictly validates that the hostname is not a private or loopback IP address and correctly handles trailing dots in hostnames.