Skip to content

🛡️ Sentinel: [CRITICAL] Fix SQL injection vulnerability in db.py#56

Draft
davidjuarezdev wants to merge 1 commit intomainfrom
sentinel/fix-sql-injection-db-10316247338205181248
Draft

🛡️ Sentinel: [CRITICAL] Fix SQL injection vulnerability in db.py#56
davidjuarezdev wants to merge 1 commit intomainfrom
sentinel/fix-sql-injection-db-10316247338205181248

Conversation

@davidjuarezdev
Copy link
Copy Markdown
Owner

🚨 Severity: CRITICAL
💡 Vulnerability: SQL injection vulnerability in streamrip/db.py's contains and remove methods. The previous implementation validated dynamic **items dictionary keys using assert, which is stripped when Python is run with optimizations (e.g., python -O). In the remove method, keys weren't validated at all. This allowed arbitrary, malicious strings to be directly interpolated into the SQLite query strings (f"{key}=?").
🎯 Impact: An attacker could craft arbitrary **kwargs that bypass the identifier restrictions of standard python variable names, subsequently injecting malicious SQL fragments into dynamic query execution.
🔧 Fix: Replaced the fragile assert statements with explicit if key not in allowed_keys: raise ValueError(...) loops, applying this robust schema validation to both contains and remove methods prior to SQL construction.
Verification: Formatting and linting checks run successfully (poetry run ruff check). The comprehensive test suite passes via poetry run pytest tests.


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

Explicitly validates `**items` dictionary keys against the database
structure using `raise ValueError` instead of `assert` in `contains` and
`remove` methods. `assert` can be stripped out entirely when Python is
run with the `-O` flag, creating a critical vulnerability where arbitrary
keys could be interpolated directly into the SQL query text.

Co-authored-by: davidjuarezdev <[email protected]>
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