Conversation
remove_assignments forwarded a client-supplied ignore_permissions boolean directly into frappe.desk.form.assign_to.set_status, which only checks the caller's permission on the target document when that flag is falsy. Since this whitelisted API never hardcoded the parameter, any authenticated user could pass ignore_permissions: true to remove another user's assignment from any document of any doctype, bypassing the permission check entirely. Neither of the two legitimate frontend call sites ever send this parameter. Removes it from the function signature and hardcodes ignore_permissions=False.
|
Tick the box to add this pull request to the merge queue (same as
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| @frappe.whitelist() | ||
| def remove_assignments(doctype: str, name: str, assignees: str | list, ignore_permissions: bool = False): | ||
| def remove_assignments(doctype: str, name: str, assignees: str | list): |
There was a problem hiding this comment.
If an existing caller still sends the formerly optional ignore_permissions argument, removing it from this whitelisted API can break that request. This violates the repository directive to preserve backward compatibility for public APIs; retain the argument as deprecated and ignored while always forwarding False. This requirement must be satisfied before merging.
| def remove_assignments(doctype: str, name: str, assignees: str | list): | |
| def remove_assignments( | |
| doctype: str, name: str, assignees: str | list, ignore_permissions: bool = False | |
| ): |
Context Used: Guidelines for reviewing Frappe Framework applications. (source)
Knowledge Base Used: API and event services
Prompt To Fix With AI
This is a comment left during a code review.
Path: crm/api/doc.py
Line: 601
Comment:
**Public API compatibility break**
If an existing caller still sends the formerly optional `ignore_permissions` argument, removing it from this whitelisted API can break that request. This violates the repository directive to preserve backward compatibility for public APIs; retain the argument as deprecated and ignored while always forwarding `False`. This requirement must be satisfied before merging.
```suggestion
def remove_assignments(
doctype: str, name: str, assignees: str | list, ignore_permissions: bool = False
):
```
**Context Used:** Guidelines for reviewing Frappe Framework applications. ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
**Knowledge Base Used:** [API and event services](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/crm/-/docs/api-and-event-services.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| assign_to=assign_to, | ||
| status="Cancelled", | ||
| ignore_permissions=ignore_permissions, | ||
| ignore_permissions=False, |
There was a problem hiding this comment.
No existing test exercises this endpoint or fails if caller-controlled permission bypass is restored. The repository requires regression coverage for bug fixes, so add a test proving an unauthorized request cannot remove assignments even when it submits ignore_permissions=true. This requirement must be satisfied before merging.
Context Used: Guidelines for reviewing Frappe Framework applications. (source)
Knowledge Base Used: API and event services
Prompt To Fix With AI
This is a comment left during a code review.
Path: crm/api/doc.py
Line: 614
Comment:
**Permission fix lacks coverage**
No existing test exercises this endpoint or fails if caller-controlled permission bypass is restored. The repository requires regression coverage for bug fixes, so add a test proving an unauthorized request cannot remove assignments even when it submits `ignore_permissions=true`. This requirement must be satisfied before merging.
**Context Used:** Guidelines for reviewing Frappe Framework applications. ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
**Knowledge Base Used:** [API and event services](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/crm/-/docs/api-and-event-services.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
1 participant
Fixes #2854
Removes the
ignore_permissionsparameter fromremove_assignments's signature entirely and hardcodesignore_permissions=Falsein theset_statuscall, so a caller can no longer influence it. Previously this whitelisted API forwarded a client-suppliedignore_permissionsboolean directly intofrappe.desk.form.assign_to.set_status, which skips its permission check on the target document when that flag is truthy, letting any authenticated user bypass the permission check by setting it in the request. Neither of the two legitimate frontend call sites ever send this parameter.