fix(sql): Escape #4861
fix(sql): Escape #4861
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SQL execution by replacing f-string SQL interpolation with parameterized queries (and adjusting some conditional SQL filters).
Changes:
- Convert multiple
frappe.db.sqlcalls from f-strings to parameterized%s/%(name)squeries. - Update optional filtering logic in
benches_with_available_updateto avoid string-built WHERE clauses.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| press/press/doctype/site_update/site_update.py | Reworks optional SQL filters for benches with updates (now via IS NULL OR ...). |
| press/press/doctype/site/erpnext_site.py | Changes bench-selection query parameter passing (currently uses frappe.db.escape with placeholders). |
| press/press/doctype/invoice/invoice.py | Parameterizes invoice overlap query to avoid string interpolation. |
| press/api/partner.py | Parameterizes dashboard/lead stats queries to avoid interpolated team name. |
| press/api/analytics.py | Parameterizes Plausible “weekly installs” query to avoid interpolated app name. |
| press/api/account.py | Parameterizes permissions query (currently uses frappe.db.escape with placeholders). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WHERE user=%s or `group` in %s | ||
| GROUP BY `document_name` | ||
| """, | ||
| (frappe.db.escape(user), frappe.db.escape(groups)), | ||
| as_dict=True, |
There was a problem hiding this comment.
This query now uses %s placeholders but still passes frappe.db.escape(user) / frappe.db.escape(groups) as parameters. escape returns SQL-quoted strings, so the driver will quote them again and the lookup will not match (and IN %s will likely break). Pass the raw user and groups tuple as parameters instead (or use named placeholders with a values dict).
| AND (%(site_bench)s IS NULL OR sb.name = %(site_bench)s) | ||
| AND (%(server)s IS NULL OR sb.server = %(server)s) |
There was a problem hiding this comment.
%(site_bench)s and %(server)s placeholders are always present in the SQL now, but values only includes these keys when site/server are passed. Calling benches_with_available_update() with no args (or only one arg) will raise a missing-parameter error. Ensure values always provides both keys (set to None when not filtering), or revert to conditionally appending the WHERE clauses.
| """ | ||
| return frappe.db.sql(query, [proxy_servers, release_group], as_dict=True)[0].name | ||
| return frappe.db.sql( | ||
| query, (frappe.db.escape(proxy_servers), frappe.db.escape(release_group)), as_dict=True |
There was a problem hiding this comment.
frappe.db.escape(...) should not be used when passing values via %s placeholders—escape returns a SQL-quoted literal, so the DB driver will quote it again, and IN %s may stop matching (or become invalid). Pass raw values instead (e.g., proxy_servers as a list/tuple and release_group as a string) and let frappe.db.sql/the driver handle escaping.
| query, (frappe.db.escape(proxy_servers), frappe.db.escape(release_group)), as_dict=True | |
| query, (tuple(proxy_servers), release_group), as_dict=True |
1 participant
No description provided.