fix: gracefully handle ephemeral document permissions during GST validation#4167
fix: gracefully handle ephemeral document permissions during GST validation#4167Aakash1o1 wants to merge 6 commits intoresilient-tech:developfrom
Conversation
…dation Replaces database existence check with secure string prefix evaluation to prevent information disclosure while resolving DoesNotExistErrors on unsaved UI documents. - get_gstin_list: strips ephemeral party name before permission check, returns [] - make_default_tax_templates: throws user-friendly error for unsaved company
Applies Permission-First pattern to both get_gstin_list and make_default_tax_templates:
1. Doctype-level permission check first (doc=None, no DB lookup)
2. startswith('new-') + db.exists() to confirm ephemeral without false positives
3. Full document-level permission check only for confirmed real records
Prevents DoesNotExistError on unsaved UI documents while ensuring real records
named 'new-*' are handled correctly.
Resolved race condition gap, added missing test for tax templates, and improved documentation.
- Replaced startswith('new-') prefix guard with db.exists() covering all missing
records (ephemeral UI docs, deleted records, stale names)
- Added test_make_default_tax_templates_with_ephemeral_company with try/finally isolation
- Added architectural comment explaining intentional [] return in get_gstin_list
|
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 11 minutes and 3 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds early doctype-level permission checks and explicit existence guards to GST India functions. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 18 |
TIP This summary will be updated as you push new changes. Give us feedback
Confidence Score: 3/5Safe to merge for the primary ephemeral-name use case, but the residual TOCTOU window at the document-level has_permission call means the original DoesNotExistError can still appear in rare race-condition scenarios. The fix correctly handles the common case (ephemeral document names) and is a clear improvement over the status quo. However, the document-level has_permission call in both functions still receives a plain string name and Frappe will attempt to fetch the document internally, meaning a deletion race can reproduce the exact error the PR is meant to prevent. This is a low-probability but real gap that warrants a follow-up fix before merging. india_compliance/gst_india/utils/init.py (line 116) and india_compliance/gst_india/overrides/company.py (line 78) — the document-level has_permission calls that retain the TOCTOU window.
|
| Filename | Overview |
|---|---|
| india_compliance/gst_india/utils/init.py | Adds a three-step permission/existence guard to get_gstin_list: doctype-level check, db.exists early-return [], then document-level check. Correct approach for the ephemeral-name bug; minor TOCTOU window remains at the document-level has_permission call. |
| india_compliance/gst_india/overrides/company.py | Same three-step pattern added to make_default_tax_templates; throws a user-friendly ValidationError for ephemeral/missing companies. Same residual TOCTOU concern at the document-level has_permission call. |
| india_compliance/gst_india/utils/test_utils.py | New test asserts [] is returned for an ephemeral supplier name using Administrator context; correctly restores session user in a finally block. Only covers the Administrator path. |
| india_compliance/gst_india/overrides/test_company.py | New test asserts ValidationError with a specific message for an ephemeral company name; uses assertRaisesRegex correctly and restores user in a finally block. |
Reviews (1): Last reviewed commit: "fix: address AI review feedback for PR #..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # 3. Record confirmed to exist — run full document-level permission check | ||
| frappe.has_permission(party_type, ptype="read", doc=party, throw=True) |
There was a problem hiding this comment.
The db.exists() guard in step 2 closes the ephemeral-name case, but the document-level has_permission call on line 116 still passes doc=party as a string. Frappe resolves a string doc by calling frappe.get_doc(doctype, name) internally, so if the record is deleted between the db.exists() check and this call (a race condition), the original DoesNotExistError can still surface — the very error this PR is intended to prevent.
For get_gstin_list the impact is low (the subsequent DB queries would simply return no rows anyway), but the error would still propagate as an unhandled 404. Consider wrapping the step-3 check in a guard, or catching frappe.DoesNotExistError:
# 3. Record confirmed to exist — run full document-level permission check
try:
frappe.has_permission(party_type, ptype="read", doc=party, throw=True)
except frappe.DoesNotExistError:
return []The same pattern applies to make_default_tax_templates in company.py (line 78).
| def test_get_gstin_list_with_ephemeral_document(self): | ||
| """get_gstin_list should return empty list for unsaved (ephemeral) documents.""" | ||
| from india_compliance.gst_india.utils import get_gstin_list | ||
|
|
||
| original_user = frappe.session.user | ||
| try: | ||
| frappe.set_user("Administrator") | ||
| result = get_gstin_list("new-supplier-1", "Supplier") | ||
| self.assertEqual(result, []) | ||
| finally: | ||
| frappe.set_user(original_user) |
There was a problem hiding this comment.
The test sets the user to "Administrator" before calling get_gstin_list. In Frappe, Administrator bypasses most row-level permission checks, so this test does not verify the behaviour for a regular user who has doctype-level read permission but no access to a specific Supplier document. It is worth adding a case with a guest or restricted user to confirm that the doctype-level check (step 1) still fires and raises PermissionError when the user has no access, ensuring the early-return of [] does not silently bypass authorization for non-admin users.
| # 1. Doctype-level permission check first (no doc lookup, safe from disclosure) | ||
| frappe.has_permission(party_type, ptype="read", doc=None, throw=True) | ||
|
|
||
| # 2. Gracefully handle unsaved or missing documents. | ||
| # Returning [] is an intentional architectural choice: the frontend calls this | ||
| # function in real-time as the user types, including before the document is saved. | ||
| # An empty list signals "no GSTINs yet" without blocking the UI or leaking existence info. | ||
| # Case A: ephemeral UI name (new-*) that isn't a real saved record | ||
| # Case B: record deleted by another user or a stale/invalid name (race condition guard) | ||
| if not frappe.db.exists(party_type, party): | ||
| return [] | ||
|
|
||
| # 3. Record confirmed to exist — run full document-level permission check | ||
| frappe.has_permission(party_type, ptype="read", doc=party, throw=True) |
There was a problem hiding this comment.
The numbered step headings and multi-line rationale blocks (e.g. "Returning [] is an intentional architectural choice: the frontend calls this function in real-time…", the Case A / Case B explanations) are thorough design documentation, but they are more appropriate in the PR description or a contributing guide than in production source code. Consider condensing to a brief single-line comment per step:
# Doctype-level permission check (no doc lookup)
frappe.has_permission(party_type, ptype="read", doc=None, throw=True)
# Return early for ephemeral/deleted documents instead of raising
if not frappe.db.exists(party_type, party):
return []
frappe.has_permission(party_type, ptype="read", doc=party, throw=True)Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adds test_get_gstin_list_raises_permission_error_for_restricted_user to verify that users without doctype-level access receive PermissionError, confirming the Permission-First pattern blocks unauthorized access at the doctype check before any database lookup occurs.
Wraps document-level has_permission() in try/except DoesNotExistError to handle the window between the db.exists() check and the internal get_doc() call inside has_permission, where a record could be deleted by a concurrent user.
- Fix TOCTOU error message in make_default_tax_templates to distinguish between unsaved documents and records deleted mid-request - Add tearDownClass to TestUtils to roll back global state after tests - Add positive test using upstream test fixture (_Test Registered Supplier) - Add multi-role permission boundary test for make_default_tax_templates - Remove site-specific Supplier creation from test setup (not portable to upstream CI)
2 participants
Description
When a user creates a new, unsaved Supplier/Customer/Company and interacts with GST-related fields (e.g., adding a GSTIN), a `DoesNotExistError` is thrown, surfaced to the user as a "Supplier/Customer Not Found" (HTTP 404) error.
Root Cause
`frappe.has_permission(doctype, doc=docname, throw=True)` internally calls `frappe.get_doc(doctype, docname)` when `doc` is a string. For unsaved records, the client passes a temporary ephemeral name (e.g., `new-supplier-1`) which does not exist in the database, causing `frappe.DoesNotExistError` to be raised.
Fix Description
Implements the Permission-First pattern across both affected functions:
Affected Functions
Steps to Reproduce (Before Fix)
Testing