RTSP installer changes - #13
Closed
Kirusthiya wants to merge 1 commit into
Closed
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughChangesThe PR adds disabled-camera management, live camera administration, persistent pause and resume behavior, connector uninstall handling, source synchronization, installer update support, and connector version 1.1.12 metadata across the backend, connector, installer, and Angular dashboard. Connector platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CapturePipeline
participant RuntimeState
participant AdminDashboard
CapturePipeline->>RuntimeState: Publish camera frame and status
AdminDashboard->>RuntimeState: Request live camera status or frame
RuntimeState-->>AdminDashboard: Return status or MJPEG frame
sequenceDiagram
participant TrayApplication
participant UpdateManifest
participant Installer
TrayApplication->>UpdateManifest: Fetch latest release metadata
UpdateManifest-->>TrayApplication: Return version, URL, size, and SHA-256
TrayApplication->>TrayApplication: Verify downloaded installer
TrayApplication->>Installer: Launch update mode
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
1. All-sources delete can't sync
|
PR Summary by QodoConnector 1.1.12: installer/tray updates, live preview, and camera disable flows
AI Description
Diagram
High-Level Assessment
Files changed (31)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+130
to
+143
| def _sync_sources(wizard: WizardConfig) -> bool: | ||
| connector_id = store.get_cred("connector_id") if store else None | ||
| api_key = store.get_cred("api_key") if store else None | ||
| if client is None or not (connector_id and api_key): | ||
| wizard.setup_complete = False | ||
| save_wizard_config(wizard) | ||
| return False | ||
| client.set_credentials(connector_id, api_key) | ||
| client.finalize_setup([ | ||
| source.source_key or source_key_for(source) for source in wizard.sources | ||
| ]) | ||
| wizard.setup_complete = True | ||
| save_wizard_config(wizard) | ||
| return True |
There was a problem hiding this comment.
1. All-sources delete can't sync 🐞 Bug ≡ Correctness
The new local admin bulk-delete flow can remove every source, but then calls finalize_setup with an empty source-key list; the backend FinalizeSetup rejects empty lists, so the backend source set cannot be cleared and local vs backend state diverges.
Agent Prompt
### Issue description
The connector local admin UI can delete all configured sources, but `_sync_sources()` always calls `client.finalize_setup([...])` and will send an empty list after deleting the last source. The backend `FinalizeSetup` currently rejects empty `SourceKeys`, so “remove all sources” cannot be finalized and backend cameras remain attached.
### Issue Context
- This PR introduces bulk-delete in the connector admin UI and source-set syncing via `finalize_setup`.
- Backend `FinalizeSetup` currently enforces `SourceKeys.Count > 0`.
### Fix Focus Areas
- backend/Controllers/ConnectorsController.cs[254-294]
- connector/app/admin.py[130-143]
- connector/app/admin.py[191-212]
### Suggested fix
1. **Backend:** Update `FinalizeSetup` to accept an empty `SourceKeys` list as a valid “detach everything for this connector” operation (execute the detach update for all cameras with `ConnectorId == connector.Id`). Return `Ok` with `activeCameraCount = 0`.
2. **Connector admin:** In `_sync_sources`, if `wizard.sources` is empty, still call `finalize_setup([])` (after backend supports it) and then set `wizard.setup_complete = True`.
3. Add/adjust a test to cover deleting the last source and verifying backend detachment.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+295
to
+351
| def _check_updates(self, notify: bool = True) -> None: | ||
| try: | ||
| response = requests.get(UPDATE_MANIFEST_URL, timeout=15) | ||
| response.raise_for_status() | ||
| metadata = response.json() | ||
| latest = str(metadata.get("version") or "").strip() | ||
| available = bool(latest) and ( | ||
| self._version_tuple(latest) > self._version_tuple(CURRENT_VERSION) | ||
| ) | ||
| newly_available = available and latest != self.latest_version | ||
| self.update_metadata = metadata if available else {} | ||
| self.latest_version = latest if available else "" | ||
| self.update_available = available | ||
| self._last_update_check = time.monotonic() | ||
| if newly_available and notify and self.icon is not None: | ||
| self._prompt_for_update(latest) | ||
| except (requests.RequestException, ValueError, TypeError): | ||
| self._last_update_check = time.monotonic() | ||
| if self.icon is not None: | ||
| self.icon.update_menu() | ||
|
|
||
| def _install_update(self, _icon=None, _item=None) -> None: | ||
| if self.update_busy or not self.update_available: | ||
| return | ||
|
|
||
| def worker(): | ||
| self.update_busy = True | ||
| if self.icon is not None: | ||
| self.icon.update_menu() | ||
| try: | ||
| metadata = self.update_metadata | ||
| download_url = str(metadata.get("downloadUrl") or "").strip() | ||
| expected_hash = str(metadata.get("sha256") or "").strip().lower() | ||
| expected_size = int(metadata.get("sizeBytes") or 0) | ||
| if not download_url.lower().startswith("https://"): | ||
| raise RuntimeError("The update download URL is not secure.") | ||
| update_dir = self.state_dir.parent / "updates" | ||
| update_dir.mkdir(parents=True, exist_ok=True) | ||
| target = update_dir / f"ONEVO-Connector-Update-{self.latest_version}.exe" | ||
| partial = target.with_suffix(".exe.partial") | ||
| digest = hashlib.sha256() | ||
| received = 0 | ||
| self._notify(f"Downloading ONEVO Connector {self.latest_version}...") | ||
| with requests.get(download_url, stream=True, timeout=(15, 120)) as response: | ||
| response.raise_for_status() | ||
| with partial.open("wb") as output: | ||
| for chunk in response.iter_content(1024 * 1024): | ||
| if not chunk: | ||
| continue | ||
| output.write(chunk) | ||
| digest.update(chunk) | ||
| received += len(chunk) | ||
| if expected_size and received != expected_size: | ||
| raise RuntimeError("Downloaded update size does not match the release.") | ||
| if not expected_hash or digest.hexdigest().lower() != expected_hash: | ||
| raise RuntimeError("Downloaded update failed SHA-256 verification.") | ||
| partial.replace(target) |
There was a problem hiding this comment.
2. Updater path traversal 🐞 Bug ⛨ Security
TrayApplication builds the downloaded update filename from latest_version fetched from a remote JSON manifest without sanitizing it, allowing path separators/".." in the version to escape the updates directory and write to unintended paths.
Agent Prompt
### Issue description
`latest_version` is taken from the remote update manifest and interpolated directly into the local update filename. If the manifest contains `..`, `\\`, or `/`, the resulting `Path` can escape the intended `updates` directory, enabling arbitrary file write locations before any hash verification.
### Issue Context
- `latest_version` is set from `UPDATE_MANIFEST_URL` JSON.
- The code writes to `partial.open("wb")` using a path derived from that version.
### Fix Focus Areas
- connector/app/tray.py[295-351]
### Suggested fix
1. Validate the manifest version strictly (e.g., `re.fullmatch(r"\d+(?:\.\d+){0,3}", latest)`), and **reject** versions containing any path separators or dot-dot sequences.
2. Build the target filename from the **sanitized** version only, or avoid embedding it at all (e.g., always download to a fixed filename like `ONEVO-Connector-Update.exe` and keep the version only in metadata).
3. Add a containment check before writing:
- Resolve both paths and assert `target.resolve()` is under `update_dir.resolve()` (platform-appropriate).
4. Add a unit test with a malicious version like `"..\\..\\evil"` ensuring the update is rejected.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+603
to
+621
| @app.get("/live/cameras/{camera_id}/stream.mjpg") | ||
| def live_camera_stream(camera_id: str): | ||
| def frames(): | ||
| last_frame = None | ||
| while True: | ||
| frame = state.get_frame(camera_id) | ||
| if frame and frame is not last_frame: | ||
| last_frame = frame | ||
| yield ( | ||
| b"--frame\r\nContent-Type: image/jpeg\r\n" | ||
| + f"Content-Length: {len(frame)}\r\n\r\n".encode("ascii") | ||
| + frame | ||
| + b"\r\n" | ||
| ) | ||
| time.sleep(0.125) | ||
|
|
||
| return StreamingResponse( | ||
| frames(), media_type="multipart/x-mixed-replace; boundary=frame" | ||
| ) |
There was a problem hiding this comment.
3. Mjpeg stream cleanup missing 🐞 Bug ☼ Reliability
The new MJPEG streaming endpoint uses a synchronous infinite generator with time.sleep and no explicit disconnect/cancellation handling, relying entirely on framework behavior for cleanup and potentially increasing worker usage under frequent connect/disconnect.
Agent Prompt
### Issue description
`/live/cameras/{camera_id}/stream.mjpg` streams via an infinite sync generator and does not explicitly handle client disconnects or cancellation. While Starlette/Uvicorn may close/cancel iterators, adding explicit handling makes behavior deterministic and more robust.
### Issue Context
- Endpoint is synchronous (`def`) and uses `time.sleep(0.125)` in a `while True` loop.
### Fix Focus Areas
- connector/app/admin.py[603-621]
### Suggested fix
1. Accept a `Request` object and switch to an **async generator**:
- `async def frames(): ...; if await request.is_disconnected(): break; await asyncio.sleep(...)`
2. Wrap the generator loop with `try/finally` to ensure any per-stream cleanup is executed.
3. Optionally add a max stream duration / idle timeout to avoid forgotten tabs keeping streams open indefinitely.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
change
Summary by CodeRabbit