From 8c88cecc1f0d355c50c06aa996e63b53a362d34a Mon Sep 17 00:00:00 2001 From: Rohit Waghchaure Date: Sun, 14 Jun 2026 23:18:32 +0530 Subject: [PATCH 1/2] fix: regression issues related to security fixes --- erpnext/accounts/party.py | 6 +- .../purchase_order/services/subcontracting.py | 2 +- .../subcontracting_inward_controller.py | 4 +- .../sales_order/services/subcontracting.py | 2 +- .../stock/doctype/stock_entry/stock_entry.py | 6 +- .../subcontracting_inward_order.py | 10 +++- .../subcontracting_order.py | 10 +++- .../test_subcontracting_order.py | 56 +++++++++++++++++++ 8 files changed, 85 insertions(+), 11 deletions(-) diff --git a/erpnext/accounts/party.py b/erpnext/accounts/party.py index d38a396e983..64f212415d7 100644 --- a/erpnext/accounts/party.py +++ b/erpnext/accounts/party.py @@ -509,10 +509,10 @@ def get_party_advance_account(party_type, party, company): return account -@frappe.whitelist() def get_party_bank_account(party_type: str, party: str): - frappe.has_permission("Bank Account", "read", throw=True) - return frappe.db.get_value("Bank Account", {"party_type": party_type, "party": party, "is_default": 1}) + return frappe.db.get_value( + "Bank Account", {"party_type": party_type, "party": party, "is_default": 1, "disabled": 0}, "name" + ) def get_party_account_currency(party_type, party, company): diff --git a/erpnext/buying/doctype/purchase_order/services/subcontracting.py b/erpnext/buying/doctype/purchase_order/services/subcontracting.py index 6488f627ef7..13408596239 100644 --- a/erpnext/buying/doctype/purchase_order/services/subcontracting.py +++ b/erpnext/buying/doctype/purchase_order/services/subcontracting.py @@ -86,7 +86,7 @@ class SubcontractingService: def update_subcontracting_order_status(self) -> None: from erpnext.subcontracting.doctype.subcontracting_order.subcontracting_order import ( - update_subcontracting_order_status as update_sco_status, + set_subcontracting_order_status as update_sco_status, ) doc = self.doc diff --git a/erpnext/controllers/subcontracting_inward_controller.py b/erpnext/controllers/subcontracting_inward_controller.py index d1c36b61d32..8bffb6bc6d0 100644 --- a/erpnext/controllers/subcontracting_inward_controller.py +++ b/erpnext/controllers/subcontracting_inward_controller.py @@ -1124,10 +1124,10 @@ class SubcontractingInwardController: def update_inward_order_status(self): if self.subcontracting_inward_order: from erpnext.subcontracting.doctype.subcontracting_inward_order.subcontracting_inward_order import ( - update_subcontracting_inward_order_status, + set_subcontracting_inward_order_status, ) - update_subcontracting_inward_order_status(self.subcontracting_inward_order) + set_subcontracting_inward_order_status(self.subcontracting_inward_order) @frappe.whitelist() diff --git a/erpnext/selling/doctype/sales_order/services/subcontracting.py b/erpnext/selling/doctype/sales_order/services/subcontracting.py index 8c67f1e0990..78b3f377e35 100644 --- a/erpnext/selling/doctype/sales_order/services/subcontracting.py +++ b/erpnext/selling/doctype/sales_order/services/subcontracting.py @@ -56,7 +56,7 @@ class SubcontractingService: def update_subcontracting_order_status(self) -> None: from erpnext.subcontracting.doctype.subcontracting_inward_order.subcontracting_inward_order import ( - update_subcontracting_inward_order_status as update_scio_status, + set_subcontracting_inward_order_status as update_scio_status, ) doc = self.doc diff --git a/erpnext/stock/doctype/stock_entry/stock_entry.py b/erpnext/stock/doctype/stock_entry/stock_entry.py index 80552b7c25f..21f366c6239 100644 --- a/erpnext/stock/doctype/stock_entry/stock_entry.py +++ b/erpnext/stock/doctype/stock_entry/stock_entry.py @@ -1465,10 +1465,12 @@ class StockEntry(StockController, SubcontractingInwardController): def update_subcontracting_order_status(self): if self.subcontracting_order and self.purpose in ["Send to Subcontractor", "Material Transfer"]: from erpnext.subcontracting.doctype.subcontracting_order.subcontracting_order import ( - update_subcontracting_order_status, + set_subcontracting_order_status, ) - update_subcontracting_order_status(self.subcontracting_order) + # Trusted submit/cancel flow — a Stock operation must not require Subcontracting Order + # write permission, so use the no-check internal helper (not the whitelisted boundary). + set_subcontracting_order_status(self.subcontracting_order) def update_pick_list_status(self): from erpnext.stock.doctype.pick_list.pick_list import update_pick_list_status diff --git a/erpnext/subcontracting/doctype/subcontracting_inward_order/subcontracting_inward_order.py b/erpnext/subcontracting/doctype/subcontracting_inward_order/subcontracting_inward_order.py index d15ae8beaa6..c591d28ed47 100644 --- a/erpnext/subcontracting/doctype/subcontracting_inward_order/subcontracting_inward_order.py +++ b/erpnext/subcontracting/doctype/subcontracting_inward_order/subcontracting_inward_order.py @@ -557,10 +557,18 @@ class SubcontractingInwardOrder(SubcontractingController): return stock_entry.as_dict() +def set_subcontracting_inward_order_status(scio: str | Document, status: str | None = None): + if isinstance(scio, str): + scio = frappe.get_doc("Subcontracting Inward Order", scio) + + scio.update_status(status) + + @frappe.whitelist() def update_subcontracting_inward_order_status(scio: str | Document, status: str | None = None): + """Whitelisted boundary for direct API/UI calls — enforces write permission, then delegates.""" if isinstance(scio, str): scio = frappe.get_doc("Subcontracting Inward Order", scio) scio.check_permission("write") - scio.update_status(status) + set_subcontracting_inward_order_status(scio, status) diff --git a/erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py b/erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py index ff08793e15a..9b092ad5c0a 100644 --- a/erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py +++ b/erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py @@ -479,10 +479,18 @@ def get_mapped_subcontracting_receipt(source_name, target_doc=None, items=None): return target_doc +def set_subcontracting_order_status(sco: str | Document, status: str | None = None): + if isinstance(sco, str): + sco = frappe.get_doc("Subcontracting Order", sco) + + sco.update_status(status) + + @frappe.whitelist() def update_subcontracting_order_status(sco: str | Document, status: str | None = None): + """Whitelisted boundary for direct API/UI calls — enforces write permission, then delegates.""" if isinstance(sco, str): sco = frappe.get_doc("Subcontracting Order", sco) sco.check_permission("write") - sco.update_status(status) + set_subcontracting_order_status(sco, status) diff --git a/erpnext/subcontracting/doctype/subcontracting_order/test_subcontracting_order.py b/erpnext/subcontracting/doctype/subcontracting_order/test_subcontracting_order.py index 9b6185d6f14..4e60d37d356 100644 --- a/erpnext/subcontracting/doctype/subcontracting_order/test_subcontracting_order.py +++ b/erpnext/subcontracting/doctype/subcontracting_order/test_subcontracting_order.py @@ -336,6 +336,62 @@ class TestSubcontractingOrder(ERPNextTestSuite): bin_after_cancel_sco.reserved_qty_for_sub_contract, bin_before_sco.reserved_qty_for_sub_contract ) + def test_send_to_subcontractor_ste_submit_without_sco_write_permission(self): + """A Stock-only user (can submit Stock Entries but has no Subcontracting Order write) must be + able to submit and cancel a 'Send to Subcontractor' Stock Entry. The SCO status update on the + on_submit/on_cancel path goes through the no-permission-check internal helper, not the + whitelisted API boundary. + + Regression: the permission hardening put check_permission('write') on the shared status + function, so a Stock Manager (no SCO write) hit PermissionError submitting/cancelling the + Stock Entry. The suite otherwise runs as Administrator and never caught it.""" + from frappe.core.doctype.user_permission.test_user_permission import create_user + + make_stock_entry(target="_Test Warehouse - _TC", item_code="_Test Item", qty=10, basic_rate=100) + + service_items = [ + { + "warehouse": "_Test Warehouse - _TC", + "item_code": "Subcontracted Service Item 1", + "qty": 10, + "rate": 100, + "fg_item": "_Test FG Item", + "fg_item_qty": 10, + }, + ] + sco = get_subcontracting_order(service_items=service_items) + + rm_items = [ + { + "item_code": "_Test FG Item", + "rm_item_code": "_Test Item", + "item_name": "_Test Item", + "qty": 10, + "warehouse": "_Test Warehouse - _TC", + "rate": 100, + "amount": 1000, + "stock_uom": "Nos", + }, + ] + ste = frappe.get_doc(make_rm_stock_entry(sco.name, rm_items)) + ste.to_warehouse = "_Test Warehouse 1 - _TC" + ste.save() + + stock_user = create_user("test_sco_stock_only@example.com", "Stock Manager") + self.assertFalse( + frappe.has_permission("Subcontracting Order", "write", user=stock_user.name), + "Precondition: the Stock-only user must not have Subcontracting Order write permission.", + ) + + frappe.set_user(stock_user.name) + try: + ste.reload() + ste.submit() # must not raise PermissionError on the SCO status update + ste.reload() + ste.cancel() # same on the cancel path + finally: + frappe.set_user("Administrator") + def test_exploded_items(self): item_code = "_Test Subcontracted FG Item 11" make_subcontracted_item(item_code=item_code) From e1d8d06966d068f3bf1d4b4dd64742c0078dde24 Mon Sep 17 00:00:00 2001 From: Rohit Waghchaure Date: Sun, 14 Jun 2026 23:50:23 +0530 Subject: [PATCH 2/2] refactor: consolidate duplicate get_party_bank_account into bank_account.py --- erpnext/accounts/doctype/payment_request/payment_request.py | 3 ++- erpnext/accounts/party.py | 6 ------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/erpnext/accounts/doctype/payment_request/payment_request.py b/erpnext/accounts/doctype/payment_request/payment_request.py index 537ef4c7644..69dada00561 100644 --- a/erpnext/accounts/doctype/payment_request/payment_request.py +++ b/erpnext/accounts/doctype/payment_request/payment_request.py @@ -11,11 +11,12 @@ from erpnext import get_company_currency from erpnext.accounts.doctype.accounting_dimension.accounting_dimension import ( get_accounting_dimensions, ) +from erpnext.accounts.doctype.bank_account.bank_account import get_party_bank_account from erpnext.accounts.doctype.payment_entry.payment_entry import ( get_payment_entry, ) from erpnext.accounts.doctype.subscription_plan.subscription_plan import get_plan_rate -from erpnext.accounts.party import get_party_account, get_party_bank_account +from erpnext.accounts.party import get_party_account from erpnext.accounts.utils import get_account_currency, get_advance_payment_doctypes, get_currency_precision from erpnext.utilities import payment_app_import_guard diff --git a/erpnext/accounts/party.py b/erpnext/accounts/party.py index 64f212415d7..06511d770d2 100644 --- a/erpnext/accounts/party.py +++ b/erpnext/accounts/party.py @@ -509,12 +509,6 @@ def get_party_advance_account(party_type, party, company): return account -def get_party_bank_account(party_type: str, party: str): - return frappe.db.get_value( - "Bank Account", {"party_type": party_type, "party": party, "is_default": 1, "disabled": 0}, "name" - ) - - def get_party_account_currency(party_type, party, company): def generator(): party_account = get_party_account(party_type, party, company)