malpercio.dev / ezpds
An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fork atom

fix: address code review feedback for MM-90

Critical fixes:
- C1: Remove crypto error detail from client response (opaques security oracle)
Changed: 'invalid signed genesis op: {e}' → 'signed genesis op is invalid'
Server-side logging still captures full error detail

Important fixes:
- I1: Replace unwrap_or_default() on service_endpoint with proper error handling
Prevents silent DID document with empty serviceEndpoint
Returns 500 if service endpoint is missing in verified op

- I2: Handle UNIQUE constraint violation on INSERT accounts as 409 not 500
Added is_unique_violation() helper to detect constraint violations
Returns 409 DID_ALREADY_EXISTS instead of 500 INTERNAL_ERROR

- I3: Check rows_affected() on UPDATE pending_accounts SET pending_did
Detects if pending_accounts row vanished during pre-store phase
Returns error if zero rows affected (race condition detection)

- I4: Add explicit emptiness checks for rotation_keys and also_known_as arrays
Checks array is non-empty BEFORE calling first()
Returns specific error for empty arrays vs. element mismatch

Test coverage:
- G2: Add test for retry with mismatched pending_did (tampered retry)
Verifies that DID mismatch returns 500 INTERNAL_ERROR

- G3: Add device row deletion assertion to happy_path test
Verifies devices table cleanup during account promotion

- G4: Add test for malformed rotationKeyPublic format
Verifies format validation (must start with 'did:key:z')
Returns 400 INVALID_CLAIM with valid session token

Note: G5 (expired session coverage) already exists in auth.rs
(pending_session_expired_session_returns_401 test at line 321)

All tests pass: 274 total tests
No clippy warnings, cargo fmt clean

authored by malpercio.dev and committed by

Tangled e2456f18 911ca8e5

+132 -8
+132 -8
crates/relay/src/routes/create_did.rs
··· 44 44 use crate::routes::auth::require_pending_session; 45 45 use common::{ApiError, ErrorCode}; 46 46 47 + /// Check if a sqlx::Error is a UNIQUE constraint violation. 48 + fn is_unique_violation(e: &sqlx::Error) -> bool { 49 + matches!( 50 + e, 51 + sqlx::Error::Database(db_err) 52 + if db_err.kind() == sqlx::error::ErrorKind::UniqueViolation 53 + ) 54 + } 55 + 47 56 #[derive(Deserialize)] 48 57 #[serde(rename_all = "camelCase")] 49 58 pub struct CreateDidRequest { ··· 96 105 // Step 5: Verify the ECDSA signature and derive the DID. 97 106 let verified = crypto::verify_genesis_op(&signed_op_str, &rotation_key).map_err(|e| { 98 107 tracing::warn!(error = %e, "genesis op verification failed"); 99 - ApiError::new( 100 - ErrorCode::InvalidClaim, 101 - format!("invalid signed genesis op: {e}"), 102 - ) 108 + ApiError::new(ErrorCode::InvalidClaim, "signed genesis op is invalid") 103 109 })?; 104 110 105 111 // Step 6: Semantic validation — ensure op fields match account and server config. 112 + if verified.rotation_keys.is_empty() { 113 + return Err(ApiError::new( 114 + ErrorCode::InvalidClaim, 115 + "op rotationKeys is empty", 116 + )); 117 + } 106 118 if verified.rotation_keys.first().map(String::as_str) != Some(&payload.rotation_key_public) { 107 119 return Err(ApiError::new( 108 120 ErrorCode::InvalidClaim, 109 121 "rotationKeys[0] in op does not match rotationKeyPublic", 122 + )); 123 + } 124 + if verified.also_known_as.is_empty() { 125 + return Err(ApiError::new( 126 + ErrorCode::InvalidClaim, 127 + "op alsoKnownAs is empty", 110 128 )); 111 129 } 112 130 if verified.also_known_as.first().map(String::as_str) != Some(&format!("at://{handle}")) { ··· 140 158 tracing::info!(did = %pre_stored_did, "retry detected: pending_did already set, skipping plc.directory"); 141 159 true 142 160 } else { 143 - sqlx::query("UPDATE pending_accounts SET pending_did = ? WHERE id = ?") 161 + let result = sqlx::query("UPDATE pending_accounts SET pending_did = ? WHERE id = ?") 144 162 .bind(did) 145 163 .bind(&session.account_id) 146 164 .execute(&state.db) ··· 149 167 tracing::error!(error = %e, "failed to pre-store pending_did"); 150 168 ApiError::new(ErrorCode::InternalError, "failed to store pending DID") 151 169 })?; 170 + if result.rows_affected() == 0 { 171 + tracing::error!(account_id = %session.account_id, "pending account row vanished during DID pre-store"); 172 + return Err(ApiError::new( 173 + ErrorCode::InternalError, 174 + "account no longer exists", 175 + )); 176 + } 152 177 false 153 178 }; 154 179 ··· 229 254 .bind(&email) 230 255 .execute(&mut *tx) 231 256 .await 232 - .inspect_err(|e| tracing::error!(error = %e, "failed to insert account")) 233 - .map_err(|_| ApiError::new(ErrorCode::InternalError, "failed to create account"))?; 257 + .map_err(|e| { 258 + tracing::error!(error = %e, "failed to insert account"); 259 + if is_unique_violation(&e) { 260 + ApiError::new(ErrorCode::DidAlreadyExists, "DID is already fully promoted") 261 + } else { 262 + ApiError::new(ErrorCode::InternalError, "failed to create account") 263 + } 264 + })?; 234 265 235 266 sqlx::query( 236 267 "INSERT INTO did_documents (did, document, created_at, updated_at) \ ··· 312 343 ) 313 344 })?; 314 345 315 - let service_endpoint = verified.atproto_pds_endpoint.as_deref().unwrap_or_default(); 346 + let service_endpoint = verified.atproto_pds_endpoint.as_deref().ok_or_else(|| { 347 + ApiError::new( 348 + ErrorCode::InternalError, 349 + "missing service endpoint in verified op", 350 + ) 351 + })?; 316 352 317 353 Ok(serde_json::json!({ 318 354 "@context": [ ··· 609 645 .await 610 646 .unwrap(); 611 647 assert_eq!(session_count, 0, "pending_sessions rows should be deleted"); 648 + 649 + // AC2.5: devices deleted 650 + let device_count: i64 = 651 + sqlx::query_scalar("SELECT COUNT(*) FROM devices WHERE account_id = ?") 652 + .bind(&setup.account_id) 653 + .fetch_one(&db) 654 + .await 655 + .unwrap(); 656 + assert_eq!(device_count, 0, "devices rows should be deleted"); 612 657 } 613 658 614 659 // ── AC2.6: Retry path skips plc.directory ───────────────────────────────── ··· 670 715 // wiremock verifies expect(0) on mock_server drop 671 716 } 672 717 718 + // ── Test Gap G2: Retry with mismatched pending_did ──────────────────────── 719 + 720 + /// Retry path with a DIFFERENT signedCreationOp (tampered retry) should 721 + /// derive a different DID and return 500 INTERNAL_ERROR because the 722 + /// pre-stored pending_did doesn't match. 723 + #[tokio::test] 724 + async fn retry_with_mismatched_pending_did_returns_500() { 725 + let state = test_state_for_did("https://plc.directory".to_string()).await; 726 + let db = state.db.clone(); 727 + let setup = insert_test_data(&db).await; 728 + let (rotation_key_public, signed_op) = 729 + make_signed_op(&setup.handle, &state.config.public_url); 730 + 731 + // Pre-set pending_did to a DIFFERENT value (tampered/corrupted retry). 732 + let tampered_did = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaa".to_string(); 733 + sqlx::query("UPDATE pending_accounts SET pending_did = ? WHERE id = ?") 734 + .bind(&tampered_did) 735 + .bind(&setup.account_id) 736 + .execute(&db) 737 + .await 738 + .expect("pre-store tampered pending_did"); 739 + 740 + let app = crate::app::app(state); 741 + let response = app 742 + .oneshot(create_did_request( 743 + &setup.session_token, 744 + &rotation_key_public, 745 + &signed_op, 746 + )) 747 + .await 748 + .unwrap(); 749 + 750 + // Derived DID != tampered pending_did → 500 INTERNAL_ERROR 751 + assert_eq!( 752 + response.status(), 753 + StatusCode::INTERNAL_SERVER_ERROR, 754 + "expected 500" 755 + ); 756 + let body_bytes = axum::body::to_bytes(response.into_body(), usize::MAX) 757 + .await 758 + .unwrap(); 759 + let body: serde_json::Value = serde_json::from_slice(&body_bytes).unwrap(); 760 + assert_eq!(body["error"]["code"], "INTERNAL_ERROR"); 761 + } 762 + 673 763 // ── AC3.1: Invalid signature ─────────────────────────────────────────────── 674 764 675 765 /// MM-90.AC3.1: Corrupted signature returns 400 INVALID_CLAIM. ··· 808 898 )) 809 899 .await 810 900 .unwrap(); 901 + 902 + assert_eq!(response.status(), StatusCode::BAD_REQUEST, "expected 400"); 903 + let body_bytes = axum::body::to_bytes(response.into_body(), usize::MAX) 904 + .await 905 + .unwrap(); 906 + let body: serde_json::Value = serde_json::from_slice(&body_bytes).unwrap(); 907 + assert_eq!(body["error"]["code"], "INVALID_CLAIM"); 908 + } 909 + 910 + // ── Test Gap G4: Malformed rotationKeyPublic format ──────────────────────── 911 + 912 + /// rotationKeyPublic that doesn't start with "did:key:z" returns 400 INVALID_CLAIM, 913 + /// even with a valid session token. 914 + #[tokio::test] 915 + async fn invalid_rotation_key_format_returns_400() { 916 + let state = test_state_for_did("https://plc.directory".to_string()).await; 917 + let db = state.db.clone(); 918 + let setup = insert_test_data(&db).await; 919 + 920 + let request_body = serde_json::json!({ 921 + "rotationKeyPublic": "not-a-did-key", 922 + "signedCreationOp": serde_json::json!({}) 923 + }); 924 + 925 + let request = Request::builder() 926 + .method("POST") 927 + .uri("/v1/dids") 928 + .header("Authorization", format!("Bearer {}", setup.session_token)) 929 + .header("Content-Type", "application/json") 930 + .body(Body::from(request_body.to_string())) 931 + .unwrap(); 932 + 933 + let app = crate::app::app(state); 934 + let response = app.oneshot(request).await.unwrap(); 811 935 812 936 assert_eq!(response.status(), StatusCode::BAD_REQUEST, "expected 400"); 813 937 let body_bytes = axum::body::to_bytes(response.into_body(), usize::MAX)