fix: require host_peer_id to delete P2P draft backups#5226
Conversation
Require host_peer_id on DELETE /p2p-draft-backup/:code and verify it matches the stored backup owner, mirroring the POST overwrite guard. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The security fix is not tested through the public DELETE endpoint. Evidence: crates/phase-server/src/admin.rs:176 now requires Query<P2pBackupDeleteQuery> and checks the stored owner before delete_p2p_backup, but the PR only adds a helper-level blank-id test at crates/server-core/src/p2p_backup_guard.rs:249; there is no route/handler test proving missing or wrong host_peer_id returns 400/403 and leaves the row intact. Why it matters: the vulnerability is at the unauthenticated HTTP boundary, so helper coverage would not catch a future handler regression that deletes by draft code alone. Suggested fix: add a phase-server endpoint/handler test that creates a backup, attempts DELETE with no peer id and with a mismatched peer id, asserts the status, and verifies the stored backup still exists.
[LOW] The PR carries unrelated generated changelog state. Evidence: client/public/changelog.json:1, client/public/changelog-meta.json:2, and scripts/changelog/state.json:2 add release id 165 for “Morph face-down casting arrives,” which is unrelated to P2P backup delete authorization. Why it matters: merging this security PR would publish unrelated release metadata and advance changelog state outside its scope. Suggested fix: drop the changelog files from this branch.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
Thanks for the review — both points addressed in 5b639f5: [MED] HTTP boundary tests: Added
Each test seeds a backup, hits the public DELETE route over TCP (same pattern as [LOW] Changelog files: This branch never carried those files — CI re-running on the new commit; will watch for green. |
Add route-level tests proving missing/wrong host_peer_id is rejected and the stored backup survives, per review on phase-rs#5226. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off: current-head review clean; public DELETE boundary tests now cover missing, mismatched, and matching host_peer_id paths; scope and security pre-checks verified.
Summary
Root cause:
DELETE /p2p-draft-backup/{code}only validated the 6-character draft code. Anyone who learned or guessed a code could wipe another host'''s recovery snapshot. POST already enforcedhost_peer_idownership on overwrite viaguard_p2p_backup_overwrite, but DELETE had no equivalent check.Fix: Require a
host_peer_idquery parameter on DELETE, validate its shape withvalidate_p2p_backup_host_peer_id, load the stored row, and reject with 403 unless the peer id matches the backup owner. The P2P draft host client now passes its peer id on cleanup.Impact: Prevents grief-deletion of P2P draft recovery snapshots without the host peer id. Backward-incompatible for any non-client callers that DELETE without
host_peer_id(intentional — those callers were unauthenticated).Risk / tradeoffs
host_peer_idnow receive 400. Only the phase client uses this endpoint today.Test plan
validate_p2p_backup_host_peer_idunit test inserver-corecargo nextest+phase-servercompile