🛡️ Sentinel: [CRITICAL] Fix SQL injection in db.py kwargs unpacking#66
🛡️ Sentinel: [CRITICAL] Fix SQL injection in db.py kwargs unpacking#66davidjuarezdev wants to merge 1 commit intomainfrom
Conversation
Co-authored-by: davidjuarezdev <[email protected]>
|
👋 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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces insecure assert-based validation in the database wrapper with explicit runtime checks and extends key validation to the remove() method to prevent SQL injection via kwargs, and documents the incident in the Sentinel journal. Sequence diagram for validated database remove operationsequenceDiagram
actor Caller
participant DBTable
participant SQLiteDB
Caller->>DBTable: remove(items_kwargs)
DBTable->>DBTable: allowed_keys = structure.keys()
DBTable->>DBTable: validate all(item in allowed_keys)
alt Invalid keys
DBTable-->>Caller: raise ValueError
else Valid keys
DBTable->>DBTable: build conditions: key=? AND ...
DBTable->>SQLiteDB: execute DELETE FROM name WHERE conditions
SQLiteDB-->>DBTable: rows_affected
DBTable-->>Caller: None
end
Sequence diagram for contains and add with explicit validationsequenceDiagram
actor Caller
participant DBTable
participant SQLiteDB
Caller->>DBTable: contains(items_kwargs)
DBTable->>DBTable: allowed_keys = structure.keys()
DBTable->>DBTable: validate all(item in allowed_keys)
alt Invalid contains keys
DBTable-->>Caller: raise ValueError
else Valid contains keys
DBTable->>DBTable: cast values to str
DBTable->>SQLiteDB: execute SELECT ... WHERE key=? AND ...
SQLiteDB-->>DBTable: result_exists
DBTable-->>Caller: bool
end
Caller->>DBTable: add(items_tuple)
DBTable->>DBTable: validate len(items) == len(structure)
alt Wrong number of items
DBTable-->>Caller: raise ValueError
else Correct number of items
DBTable->>DBTable: params = join(structure.keys())
DBTable->>DBTable: placeholders = ?, ?, ...
DBTable->>SQLiteDB: execute INSERT INTO name (params) VALUES(placeholders)
SQLiteDB-->>DBTable: success
DBTable-->>Caller: None
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🚨 Severity: CRITICAL
💡 Vulnerability: The
streamrip/db.pymodule unpacks**itemsdictionaries to construct dynamic SQL queries for methods likeremoveandcontains. Theremovemethod had no validation, and thecontainsandaddmethods usedassertstatements which are removed when Python runs with optimization flags (e.g.python -O), making the queries vulnerable to SQL injection through unsanitized keys.🎯 Impact: Attackers could inject arbitrary SQL commands by passing malicious keys inside kwargs dictionary arguments to database operations.
🔧 Fix: Replaced weak
assertstatements with explicitif not inchecks raisingValueErrorincontains,add, andremovemethods to strictly validate query column keys against the table schema.✅ Verification: Ran
PYTHONPATH=. poetry run pytest testssuite, verified all tests pass, and ensured manual validations correctly execute. Added journal entry to.jules/sentinel.md.PR created automatically by Jules for task 2931766349854045394 started by @davidjuarezdev
Summary by Sourcery
Harden database operations against unsafe kwargs by enforcing runtime key validation and documenting the resolved SQL injection risk.
Bug Fixes:
Documentation:
Summary by cubic
Fixes a critical SQL injection in
streamrip/db.pyby validating kwargs keys and replacing asserts with runtime checks. Blocks malicious keys from reaching dynamic SQL, including when Python runs with -O.containsandremoveagainst the table schema; raise ValueError on invalid keys.assertwith explicit runtime checks incontainsandaddto enforce valid keys and correct item count.Written for commit a7d3922. Summary will update on new commits.