Skip to content

fix(sql): Escape #4861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ssiyad wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix(sql): Escape #4861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions press/api/account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,13 @@ def get_permissions():
[*frappe.get_all("Press Permission Group User", {"user": user}, pluck="parent"), "1", "2"]
) # [1, 2] is for avoiding singleton tuples
docperms = frappe.db.sql(
f"""
"""
SELECT `document_name`, GROUP_CONCAT(`action`) as `actions`
FROM `tabPress User Permission`
WHERE user='{user}' or `group` in {groups}
WHERE user=%s or `group` in %s
GROUP BY `document_name`
""",
(frappe.db.escape(user), frappe.db.escape(groups)),
as_dict=True,
Comment on lines +551 to 555
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
)
return {perm.document_name: perm.actions.split(",") for perm in docperms if perm.actions}
Expand Down
19 changes: 10 additions & 9 deletions press/api/analytics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,15 +1524,16 @@ def plausible_analytics(name):
response.update(
{
"weekly_installs": frappe.db.sql(
f"""
SELECT DATE_FORMAT(sa.creation, '%Y-%m-%d') AS date, COUNT(*) AS value
FROM `tabSite Activity` as sa
WHERE sa.action = 'Install App'
AND sa.creation >= DATE_SUB(CURDATE(), INTERVAL 8 WEEK)
AND sa.reason = '{name}'
GROUP BY WEEK(sa.creation)
ORDER BY date
""",
"""
SELECT DATE_FORMAT(sa.creation, '%Y-%m-%d') AS date, COUNT(*) AS value
FROM `tabSite Activity` as sa
WHERE sa.action = 'Install App'
AND sa.creation >= DATE_SUB(CURDATE(), INTERVAL 8 WEEK)
AND sa.reason = %s
GROUP BY WEEK(sa.creation)
ORDER BY date
""",
(name,),
as_dict=True,
),
}
Expand Down
10 changes: 6 additions & 4 deletions press/api/partner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,18 +261,19 @@ def get_partner_mrr(partner_email):
def get_dashboard_stats():
team = get_current_team(get_doc=True)
data = frappe.db.sql(
f"""
"""
SELECT
site.plan as plan,
COUNT(site.name) as count
FROM
tabSite as site JOIN tabTeam as team ON site.team = team.name
WHERE
team.name = '{team.name}'
team.name = %s
AND site.status = 'Active'
GROUP BY
site.plan
""",
(team.name,),
as_dict=True,
)
return [d for d in data]
Expand All @@ -283,7 +284,7 @@ def get_dashboard_stats():
def get_lead_stats():
team = get_current_team(get_doc=True)
data = frappe.db.sql(
f"""
"""
SELECT
COUNT(name) as total,
SUM(CASE WHEN status in ('Open', 'In Process') THEN 1 ELSE 0 END) as open,
Expand All @@ -292,8 +293,9 @@ def get_lead_stats():
FROM
`tabPartner Lead`
WHERE
partner_team = '{team.name}'
partner_team = %s
""",
(team.name,),
as_dict=True,
)
return data[0] if data else {}
Expand Down
11 changes: 7 additions & 4 deletions press/press/doctype/invoice/invoice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@

return invoices

def get_doc(self, doc):

Check failure on line 182 in press/press/doctype/invoice/invoice.py

View workflow job for this annotation

GitHub Actions / Lint and Format

Ruff (C901) 

press/press/doctype/invoice/invoice.py:182:6: C901 `get_doc` is too complex (9 > 8)
doc.invoice_pdf = self.invoice_pdf or (self.currency == "USD" and self.get_pdf())
currency = frappe.get_value("Team", self.team, "currency")
price_field = "price_inr" if currency == "INR" else "price_usd"
Expand Down Expand Up @@ -520,13 +520,16 @@

if self.type == "Subscription" and self.period_start and self.period_end and self.is_new():
query = (
f"select `name` from `tabInvoice` where team = '{self.team}' and"
f" status = 'Draft' and ('{self.period_start}' between `period_start` and"
f" `period_end` or '{self.period_end}' between `period_start` and"
"select `name` from `tabInvoice` where team = %s and"
" status = 'Draft' and (%s between `period_start` and"
" `period_end` or %s between `period_start` and"
" `period_end`)"
)

intersecting_invoices = [x[0] for x in frappe.db.sql(query, as_list=True)]
intersecting_invoices = [
x[0]
for x in frappe.db.sql(query, (self.team, self.period_start, self.period_end), as_list=True)
]

if intersecting_invoices:
frappe.throw(
Expand Down
4 changes: 3 additions & 1 deletion press/press/doctype/site/erpnext_site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ def get_erpnext_bench():
server.use_for_new_sites DESC, bench.creation DESC
LIMIT 1
"""
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
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
query, (frappe.db.escape(proxy_servers), frappe.db.escape(release_group)), as_dict=True
query, (tuple(proxy_servers), release_group), as_dict=True

Copilot uses AI. Check for mistakes.
)[0].name


def get_erpnext_domain():
Expand Down
6 changes: 3 additions & 3 deletions press/press/doctype/site_update/site_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,12 +787,12 @@ def benches_with_available_update(site=None, server=None):
if server:
values["server"] = server
source_benches_info = frappe.db.sql(
f"""
"""
SELECT sb.name AS source_bench, sb.candidate AS source_candidate, sb.server AS server, dcd.destination AS destination_candidate
FROM `tabBench` sb, `tabDeploy Candidate Difference` dcd
WHERE sb.status IN ('Active', 'Broken') AND sb.candidate = dcd.source
{"AND sb.name = %(site_bench)s" if site else ""}
{"AND sb.server = %(server)s" if server else ""}
AND (%(site_bench)s IS NULL OR sb.name = %(site_bench)s)
AND (%(server)s IS NULL OR sb.server = %(server)s)
Comment on lines +794 to +795
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%(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.

Copilot uses AI. Check for mistakes.
""",
values=values,
as_dict=True,
Expand Down
Loading