Skip to content

fix: require owner verification for P2P backup reads#5365

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
RealDiligent:fix/critical-issue-p2p-backup-get-auth
Jul 8, 2026
Merged

fix: require owner verification for P2P backup reads#5365
matthewevans merged 2 commits into
phase-rs:mainfrom
RealDiligent:fix/critical-issue-p2p-backup-get-auth

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: GET /p2p-draft-backup/{code} only required the 6-char draft code, so anyone with/guessing the code could read a host's backup snapshot.
  • Fix: require host_peer_id on GET and enforce ownership with the existing guard_p2p_backup_overwrite contract.
  • Added route-level tests for GET auth boundary:
    • missing host_peer_id -> 400
    • mismatched host_peer_id -> 403
    • matching host_peer_id -> 200

Impact

This removes unauthenticated read access to P2P backup snapshots by draft code alone while preserving host recovery flows.

Risk / tradeoffs

  • Clients calling GET without host_peer_id now receive 400.
  • This mirrors the existing DELETE ownership model for consistency.

Test plan

  • Added phase-server HTTP boundary tests for GET ownership checks
  • CI run on PR

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the GET /p2p-draft-backup/:code endpoint to require and validate a host_peer_id query parameter, preventing unauthorized access to draft backups, and adds corresponding unit tests. The review feedback suggests consolidating the query parameter struct to avoid duplication and returning 404 Not Found instead of 403 Forbidden on mismatched peer IDs to mitigate a side-channel draft code enumeration vulnerability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +136 to 147
/// Query params for `GET /p2p-draft-backup/:code`.
#[derive(Deserialize)]
pub struct P2pBackupGetQuery {
pub host_peer_id: String,
}

/// GET /p2p-draft-backup/:code — Retrieve a P2P draft backup.
pub async fn p2p_backup_get(
State(app_state): State<AppState>,
Path(code): Path<String>,
Query(query): Query<P2pBackupGetQuery>,
) -> impl IntoResponse {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[MEDIUM] Duplicate query parameter structs. Evidence: crates/phase-server/src/admin.rs:136-147.\n> Why it matters: Defining separate identical structs P2pBackupGetQuery and P2pBackupDeleteQuery introduces unnecessary code duplication and reduces maintainability.\n> Suggested fix: Consolidate them into a single shared P2pBackupQuery struct.

Suggested change
/// Query params for `GET /p2p-draft-backup/:code`.
#[derive(Deserialize)]
pub struct P2pBackupGetQuery {
pub host_peer_id: String,
}
/// GET /p2p-draft-backup/:code — Retrieve a P2P draft backup.
pub async fn p2p_backup_get(
State(app_state): State<AppState>,
Path(code): Path<String>,
Query(query): Query<P2pBackupGetQuery>,
) -> impl IntoResponse {
/// Query params for P2P draft backup endpoints.
#[derive(Deserialize)]
pub struct P2pBackupQuery {
pub host_peer_id: String,
}
/// GET /p2p-draft-backup/:code — Retrieve a P2P draft backup.
pub async fn p2p_backup_get(
State(app_state): State<AppState>,
Path(code): Path<String>,
Query(query): Query<P2pBackupQuery>,
) -> impl IntoResponse {

Comment thread crates/phase-server/src/admin.rs Outdated
Comment on lines +156 to +158
if let Err(reason) = guard_p2p_backup_overwrite(&existing_peer, &query.host_peer_id) {
return (StatusCode::FORBIDDEN, reason).into_response();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

[MEDIUM] Draft code enumeration side-channel. Evidence: crates/phase-server/src/admin.rs:156.\n> Why it matters: Returning 403 Forbidden for mismatched peer IDs and 404 Not Found for non-existent backups allows attackers to easily enumerate active 6-character draft codes.\n> Suggested fix: Return StatusCode::NOT_FOUND instead of StatusCode::FORBIDDEN to make unauthorized requests indistinguishable from non-existent backups.

Suggested change
if let Err(reason) = guard_p2p_backup_overwrite(&existing_peer, &query.host_peer_id) {
return (StatusCode::FORBIDDEN, reason).into_response();
}
if let Err(_) = guard_p2p_backup_overwrite(&existing_peer, &query.host_peer_id) {
return (StatusCode::NOT_FOUND, "No backup found").into_response();
}

Require host_peer_id ownership checks on GET /p2p-draft-backup/:code so a guessed draft code alone cannot read backup snapshots.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matthewevans matthewevans self-assigned this Jul 8, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 8, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head: GET backup reads now require matching host_peer_id, mismatch is non-enumerating 404, and boundary tests cover missing, mismatch, and match cases.

@matthewevans matthewevans enabled auto-merge July 8, 2026 13:24
@matthewevans matthewevans removed their assignment Jul 8, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 8, 2026
Merged via the queue into phase-rs:main with commit a3adbc4 Jul 8, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants