fix: add session ownership check before deleteCase and copyCase (#48)#50
Conversation
|
Thanks for this PR, @tejassinghbhati. I want to integrate #50 and #55 together on a branch
Please retarget this PR to Behavior to keep from this PR:
|
9e6071f to
9be3d8a
Compare
|
Hi @SeaCelo, done — I've retargeted this PR to sec-case-guard and rebased cleanly on top of it. No conflicts. On the 403 behaviour you flagged: both routes return distinct JSON messages — "No active session." when there's no osycase in session, and "Unauthorised: case does not match active session." on a mismatch — so the frontend can distinguish between the two cases. Let me know if you'd prefer a different message format or a single unified 403 body. Ready for review whenever you are — happy to adjust anything before #55 lands on top. |
|
Also raised #75 — /setSession can bypass the ownership guards we added here. |
Merging into `sec-case-guard` as the first of three PRs in the security integration sequence: 1. **#76** (this PR) — validate `/setSession` so session can only hold existing case names 2. **#50** — ownership guards on `/deleteCase` and `/copyCase` (depends on trusted session from #76) 3. **#55** — graceful handling of missing case directories (resilience layer) ### Minor cleanup needed post-merge The `from pathlib import Path` import inside `setSession()` is redundant — `Path` is already imported at line 2 of `app.py`. I'll remove it in a follow-up commit on `sec-case-guard`.
Summary
/deleteCaseand/copyCasepreviously performed destructive filesystem operations (shutil.rmtree,shutil.copytree) using a case name taken directly from user-supplied JSON, with no verification that the requesting client owned that case. The session was only checked after deletion to decide which status code to return — not before to guard the operation.This PR adds an ownership guard to both routes before any filesystem action is taken.
Changes
API/Routes/Case/CaseRoute.pyBoth
/deleteCaseand/copyCasenow:403if nonecasenamematchessession['osycase']— return403if notAdditionally,
deleteCaseis simplified — the redundantif/elsechecking session after deletion is replaced with a singlesession['osycase'] = None, since ownership is now guaranteed by the guard above.Verification
Manually verified the guard logic:
Authorised requests behave identically to before — no regression in normal usage.
Related
Closes #48