Skip to content

🛡️ Sentinel: [CRITICAL] Fix SQL injection risk in dynamic database queries#47

Draft
davidjuarezdev wants to merge 1 commit intomainfrom
jules-11980032661180759317-4cef851c
Draft

🛡️ Sentinel: [CRITICAL] Fix SQL injection risk in dynamic database queries#47
davidjuarezdev wants to merge 1 commit intomainfrom
jules-11980032661180759317-4cef851c

Conversation

@davidjuarezdev
Copy link
Copy Markdown
Owner

🚨 Severity: CRITICAL
💡 Vulnerability: SQL injection vulnerability via Python's assert and **kwargs dictionary unpacking bypass in dynamic database queries (streamrip/db.py).
🎯 Impact: If the Python interpreter runs with the optimization flag -O, assert statements are stripped out entirely. An attacker (or corrupted input) could pass malicious column names via **kwargs dictionary unpacking (which allows any identifier), completely bypassing the key validation logic in DatabaseBase.contains. The keys were then concatenated directly into a query string, leading to SQL injection. Additionally, DatabaseBase.remove had no key validation at all.
🔧 Fix:

  • Modified DatabaseBase.contains to use raise ValueError instead of assert to ensure the key validation runs regardless of optimization flags.
  • Added equivalent explicit key validation to DatabaseBase.remove.
  • Added a Sentinel learning journal entry in .jules/sentinel.md documenting this edge case.
    Verification:
  • Validated via automated test suites (all 60 pytest suites passing).
  • Run formatting and linting.

PR created automatically by Jules for task 11980032661180759317 started by @davidjuarezdev

…eries

Replaced `assert` in `streamrip/db.py` with `ValueError` for validating keys during query construction, as `assert` can be optimized away in production, potentially leaving the code vulnerable to SQL injection through dictionary unpacking bypass. Also added key validation to the `remove` method which had no checks. Added a journal entry explaining this finding.

Co-authored-by: davidjuarezdev <230496599+davidjuarezdev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant