🔒 Fix SQL execution for SQLite Vec Queue Database embeddings deletion#15
Conversation
This addresses an issue reported as SQL injection via string concatenation in `frigate/db/sqlitevecq.py`. While the parameters were already passed securely using `?` placeholders (preventing actual SQL injection), this commit converts the parameter list to a tuple explicitly for clarity and robustness, and prevents syntax errors (`id IN ()`) when the `event_ids` list is empty by introducing an early return. Co-authored-by: manupawickramasinghe <73810867+manupawickramasinghe@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
PR template validation failedThis PR was automatically closed because the description does not follow the pull request template. Issues found:
Please update your PR description to include all required sections from the template, then reopen this PR.
|
🎯 What: The issue reported an SQL injection in
⚠️ Risk: If an empty list of
frigate/db/sqlitevecq.pyvia string concatenation indelete_embeddings_thumbnail. The existing code parameterized the values using?properly to mitigate SQL injection. However, it was missing handling for empty lists, leading to potential syntax errorsWHERE id IN (), and parameters weren't explicitly passed as a tuple.event_idsis passed, it results in a SQLite syntax error, potentially crashing the application or the background cleanup process. Although the original code parameterized correctly for non-empty lists, SAST tools often flag missing explicit bounds or tuple structures in such dynamicINclauses as risks.🛡️ Solution: Added a check to immediately return if the
event_idslist is empty, preventing the generation of an invalid SQL statement. Also explicitly castevent_idsto atuplein theexecute_sqlcall to ensure the backend strictly interprets it as positional parameters instead of potentially malformed input types. The same fix was also correctly applied to the adjacentdelete_embeddings_descriptionfunction.PR created automatically by Jules for task 13088560508748280241 started by @manupawickramasinghe