RV-S5: upload overwrite and mode-switch bypass delete-permission gate #175

What happened

External repository review at commit c7bebd1 reproduced live that an uploader (not holding delete permission) can destroy existing datasets through two upload flows that contain internal DROP operations unprotected by the delete-permission gate.

Reproduction:

1. DELETE on an existing dataset by an uploader → HTTP 403 (correct; delete gate held).
2. Upload the same filename with overwrite: true → success, prior table dropped silently.
3. Upload the same filename switching from dynamic to T&E mode → success, prior table dropped without even needing overwrite: true.

Code paths:

• api/auth.py L82–107 — delete-permission gate (works for DELETE endpoint)
• api/services/dynamic_loader.py L159–184 — overwrite path, contains internal DROP, no gate check
• api/services/te_loader.py L87–112 — mode-switch cleanup, contains internal DROP, no gate check

Why this matters

Role-model hole in the auth system, not a bug in isolation. The role matrix (docs/ROLES.md, M2 scope) defines uploader as "upload yes, delete no." The implementation enforces that on the DELETE endpoint but not on internal destructive paths within upload.

Consequences:

• An uploader can destroy any dataset in their tenant by re-upload + overwrite or by mode switch.
• A compromised uploader account can wipe tenant data through upload flows that auth considers permitted.
• Audit trail misleads: an auditor investigating "who deleted X" finds no DELETE in logs, only an upload.
• Combined with G44 / RV-F1 (browser history across tenants) this becomes relevant to BR-36, though backend schema isolation prevents the actual cross-tenant DB write.

Decision taken 3 Nov 2026

Option (b) of 2 considered: centralise destructive-op authorisation in a new api/services/destructive_ops.py with a single authorized_drop_table(user, tenant, table) function. Refactor the DELETE endpoint, dynamic_loader.py and te_loader.py to all go through it. Future destructive paths added during M5 (dataset versioning) and M6 (staged imports) call the same function or they do not DROP at all.

Option (a) rejected: patching each site individually (~5-10 lines × 2 sites) fixes today's symptom but leaves the same mistake available to be reintroduced by the next session that adds a destructive path.

Bundled with #170 (RV-F3) into one PR because:

• Both touch api/services/te_loader.py L87–112
• Both are about "destructive paths must check conditions before destroying"
• The centralised function naturally hosts both the role check (this issue) and the all-failed-rollback guard (RV-F3 (BR-34): all-failed T&E import runs registry cleanup and removes prior dataset #170)
• Merging as separate PRs guarantees a conflict

Proposed

1. New api/services/destructive_ops.py:
   • authorized_drop_table(user, tenant, table) — raises if user lacks delete permission for the target
   • safe_drop_with_outcome(user, tenant, table, outcome) — combines the auth check and the all-failed-rollback guard from RV-F3 (BR-34): all-failed T&E import runs registry cleanup and removes prior dataset #170
2. Refactor:
   • api/auth.py DELETE endpoint → call authorized_drop_table
   • api/services/dynamic_loader.py overwrite path → call authorized_drop_table
   • api/services/te_loader.py cleanup → call safe_drop_with_outcome
3. New role unit test: uploader attempts each destructive flow (DELETE, upload+overwrite, mode-switch), all three return 403 and leave prior data intact.

Done when

• Reproduction test added: an uploader (not holding delete permission) runs each of the three destructive flows (DELETE, upload+overwrite, dynamic-to-T&E mode switch); all three return 403; the prior table and its rows persist.
• The test fails on main at c7bebd1 without the fix.
• The test passes with the fix.
• Admin / tenant_admin flows still work (regression test): a role that does hold delete permission can still DELETE, overwrite and mode-switch.
• The RV-F3 (BR-34): all-failed T&E import runs registry cleanup and removes prior dataset #170 all-failed-rollback case still works via the same central function (combined test).
• CI replica run quoted on the PR (counts in passed / failed / skipped form).

Scope

Bundled with #170. The combined PR title should name both findings. Both tracker rows (G38, G46) update to the same issue number if GitHub supports multiple issues per PR; otherwise this issue stays open and #170 closes on merge, or vice versa.

Reference

Tracker row G38 (main tab). Repo review finding RV-S5. Bundled with #170 (RV-F3). Related: role matrix in docs/ROLES.md, M2 scope. Blocks: nothing today; higher priority than #170 alone because this is an auth hole an attacker can trigger, while #170 is user-mistake-triggered.