harden: make every whitelisted write POST-only

[vibhavkatre]

Found while auditing sharing for #422. It is not a sharing problem, which is why it is not in that PR.

The gap

A whitelisted method with no methods accepts GET. That puts it one cross-site request away from being fired by any page the victim happens to visit — no form, no fetch, an <img src> is enough. Eleven of Draw's write endpoints were in that state while the rest of the app was already POST-only:

Module	Endpoints
share.py	share_diagram, unshare_diagram, set_general_access, set_public
comment.py	add_comment, reply_comment, edit_comment, resolve_comment, delete_comment
drive_integration.py	add_to_drive, move_to_drive_folder

Sharing is the worst of them. A forged request there grants the attacker standing access to the document rather than damaging it once, and it is the one write whose effect outlives the session that made it. share_diagram(name, user, level) takes only values an attacker already knows or chooses.

Why it did not bite

Frappe rolls a GET back at the end of the request, so nothing persisted. Worth being precise that this is a property of the transaction handler, not of these modules — and that diagram.py has already had a manual frappe.db.commit() turn exactly this shape into a live CSRF write vector. The comment above save_diagram still records it:

the removed manual commit previously made a GET (which the framework rolls back) durable anyway — a CSRF write vector.

One commit() added to any of these paths, for any reason, and the rollback stops covering us. The rule is worth holding whether or not today's default saves us.

No client changes

frappe-ui's call() sets method: 'POST' (utils/call.js:21), which is how all eleven are reached. Verified against the call sites in useComments.js, useShare.js, DriveMenu.vue and data/drive.js.

The guard

A signature test sits beside the existing annotation one and for the same reason: an in-process call cannot see which verbs an endpoint would accept over HTTP, so this whole class is invisible to every other test in the file. It walks the app's AST, takes every whitelisted function whose name starts with a mutating verb, and fails any that accepts GET.

That heuristic is deliberately name-based rather than a hand-written list — a new delete_collection or set_whatever is caught the day it is written, without anyone remembering this PR.

What I did NOT change

The rest of the sharing audit came out clean and is left alone: every mutation checks has_permission(..., "share"), share targets are validated against real enabled users, search_users escapes LIKE wildcards and refuses queries under two characters, and both the access level and the general-access tier are checked against allowlists. No raw SQL anywhere in the module.

set_public is dead — no caller outside a test that documents it as a shim for older clients. It is POST-restricted here rather than removed, since deleting a whitelisted endpoint is an API breaking change and worth deciding separately.

Test plan

• bench --site test.localhost run-tests --app draw --module draw.draw.doctype.draw_diagram.test_draw_diagram — 76 tests, 1 new
• Confirmed no mutating endpoint is left GET-reachable
• The one failure on this bench (test_diagram_drive_path_degrades_without_raising_on_a_broken_drive) is pre-existing and environment-specific — a dangling suite entry in this bench's apps.txt. It passes in CI.
• E2E exercises the comment and sharing flows end to end, so the POST paths are covered there.

🤖 Generated with Claude Code