Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the desktop application, including a unified AI chat bridge (ACP), local-first task management with background synchronization, and enhanced drag-and-drop capabilities. It also addresses several stability issues, such as the cursor jump bug in click-through views and improved OOM detection for the AI subprocess. However, there are a few high-severity concerns regarding security and performance that should be addressed: a potential SQL injection vulnerability in the cloud agent's sync logic, the use of blocking thread sleep in the proactive assistants plugin, and the aggressive use of killall on a system daemon.
I am having trouble creating individual review comments. Click here to see my feedback.
desktop/agent-cloud/agent.mjs (824-832)
The dynamic schema migration logic is vulnerable to SQL injection. The table and col variables are interpolated directly into SQL strings used in db.prepare(). Since identifiers like table and column names cannot be parameterized in SQLite, they must be strictly validated against an allowlist or a strict regex pattern (e.g., /^[a-zA-Z0-9_]+$/) before being used in a query. An attacker could potentially manipulate these values to execute arbitrary SQL or corrupt the database schema.
desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift (272)
Using Thread.sleep(forTimeInterval:) is discouraged in modern Swift, especially within the context of structured concurrency. It blocks the current thread, which, if running on the cooperative thread pool, can degrade application performance and potentially lead to deadlocks. Use await Task.sleep(nanoseconds:) instead to yield the thread while waiting.
desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift (262-268)
Restarting the system-wide usernoted daemon via killall is an extremely intrusive operation. While it may force the system to pick up fresh notification registrations, it affects all running applications and can lead to a poor user experience (e.g., missing notifications from other apps, UI glitches). This should be a last resort, and ideally, a more standard API for refreshing notification settings should be used if available.
desktop/Desktop/Sources/AgentSyncService.swift (234)
The PRAGMA table_info query uses string interpolation for the table name. While spec.name is likely derived from internal configuration, it is best practice to avoid interpolation in SQL strings to prevent potential injection or syntax errors if a table name contains special characters (like single quotes).
|
Hey @m13v 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
Automated sync from omi-desktop at 474e1e1f.
This PR was created by the sync pipeline.