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

fix(MM-94): address code review feedback for POST /v1/handles

- Swap DNS/INSERT order: INSERT before create_record so a DB row without a
DNS record is recoverable; a DNS record without a row is an invisible orphan
- Drop SELECT EXISTS pre-check; detect is_unique_violation() on INSERT directly
so a concurrent UNIQUE conflict returns 409 HANDLE_TAKEN instead of 500
- Remove duplicate HandleAlreadyExists error code; use HandleTaken everywhere
- Strip scheme from public_url before passing hostname to DnsProvider
- Add tracing::debug! before base64 decode failure and session lookup miss in
require_session (silent auth errors were unobservable in operator logs)
- Add did field to DNS error tracing::error! for per-account correlation
- Enforce RFC 1035 63-char DNS label limit in validate_handle
- Add DnsError to status_code_mapping test
- Add AlwaysOkDns/AlwaysErrDns test doubles + DNS success/failure integration tests
- Add 4 require_session unit tests (missing header, bad base64, valid, expired)

authored by malpercio.dev and committed by

Tangled aa9ee61d 98933081

+313 -45
+1 -3
crates/common/src/error.rs
··· 44 44 PlcDirectoryError, 45 45 /// A configured DNS provider returned an error when creating a subdomain record. 46 46 DnsError, 47 - /// A handle submitted for registration is already claimed. 48 - HandleAlreadyExists, 49 47 // TODO: add remaining codes from Appendix A as endpoints are implemented: 50 48 // 400: INVALID_DOCUMENT, INVALID_PROOF, INVALID_ENDPOINT, INVALID_CONFIRMATION 51 49 // 401: INVALID_CREDENTIALS ··· 80 78 ErrorCode::DidAlreadyExists => 409, 81 79 ErrorCode::PlcDirectoryError => 502, 82 80 ErrorCode::DnsError => 502, 83 - ErrorCode::HandleAlreadyExists => 409, 84 81 } 85 82 } 86 83 } ··· 234 231 (ErrorCode::ClaimCodeRedeemed, 409), 235 232 (ErrorCode::DidAlreadyExists, 409), 236 233 (ErrorCode::PlcDirectoryError, 502), 234 + (ErrorCode::DnsError, 502), 237 235 ]; 238 236 for (code, expected) in cases { 239 237 assert_eq!(code.status_code(), expected, "wrong status for {code:?}");
+134 -1
crates/relay/src/routes/auth.rs
··· 176 176 177 177 let token_bytes = URL_SAFE_NO_PAD 178 178 .decode(token) 179 - .map_err(|_| ApiError::new(ErrorCode::Unauthorized, "invalid session token"))?; 179 + .map_err(|_| { 180 + tracing::debug!("session token is not valid base64url"); 181 + ApiError::new(ErrorCode::Unauthorized, "invalid session token") 182 + })?; 180 183 let token_hash: String = Sha256::digest(&token_bytes) 181 184 .iter() 182 185 .map(|b| format!("{b:02x}")) ··· 194 197 })?; 195 198 196 199 let (did,) = row.ok_or_else(|| { 200 + tracing::debug!("no unexpired session row found for token hash"); 197 201 ApiError::new(ErrorCode::Unauthorized, "invalid or expired session token") 198 202 })?; 199 203 ··· 479 483 HeaderValue::from_bytes(b"Bearer \xff\xfe").unwrap(), 480 484 ); 481 485 let err = require_pending_session(&headers, &state.db) 486 + .await 487 + .unwrap_err(); 488 + assert_eq!(err.status_code(), 401); 489 + } 490 + 491 + // ── require_session tests ───────────────────────────────────────────────── 492 + 493 + #[tokio::test] 494 + async fn session_missing_authorization_header_returns_401() { 495 + let state = test_state().await; 496 + let err = require_session(&HeaderMap::new(), &state.db) 497 + .await 498 + .unwrap_err(); 499 + assert_eq!(err.status_code(), 401); 500 + } 501 + 502 + #[tokio::test] 503 + async fn session_non_base64url_token_returns_401() { 504 + let mut headers = HeaderMap::new(); 505 + headers.insert( 506 + axum::http::header::AUTHORIZATION, 507 + "Bearer not-valid-base64url!!!".parse().unwrap(), 508 + ); 509 + let state = test_state().await; 510 + let err = require_session(&headers, &state.db) 511 + .await 512 + .unwrap_err(); 513 + assert_eq!(err.status_code(), 401); 514 + } 515 + 516 + #[tokio::test] 517 + async fn session_valid_unexpired_session_returns_ok() { 518 + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; 519 + use rand_core::{OsRng, RngCore}; 520 + use sha2::{Digest, Sha256}; 521 + use uuid::Uuid; 522 + 523 + let state = test_state().await; 524 + 525 + // Insert an account (required by sessions FK constraint). 526 + let did = format!("did:plc:{}", &Uuid::new_v4().to_string().replace('-', "")[..24]); 527 + sqlx::query( 528 + "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 529 + VALUES (?, ?, NULL, datetime('now'), datetime('now'))", 530 + ) 531 + .bind(&did) 532 + .bind(format!("test{}@example.com", &did[8..16])) 533 + .execute(&state.db) 534 + .await 535 + .expect("insert account"); 536 + 537 + let mut token_bytes = [0u8; 32]; 538 + OsRng.fill_bytes(&mut token_bytes); 539 + let session_token = URL_SAFE_NO_PAD.encode(token_bytes); 540 + let token_hash: String = Sha256::digest(token_bytes) 541 + .iter() 542 + .map(|b| format!("{b:02x}")) 543 + .collect(); 544 + 545 + sqlx::query( 546 + "INSERT INTO sessions (id, did, device_id, token_hash, created_at, expires_at) \ 547 + VALUES (?, ?, NULL, ?, datetime('now'), datetime('now', '+1 year'))", 548 + ) 549 + .bind(Uuid::new_v4().to_string()) 550 + .bind(&did) 551 + .bind(&token_hash) 552 + .execute(&state.db) 553 + .await 554 + .expect("insert session"); 555 + 556 + let mut headers = HeaderMap::new(); 557 + headers.insert( 558 + axum::http::header::AUTHORIZATION, 559 + format!("Bearer {session_token}").parse().unwrap(), 560 + ); 561 + 562 + let result = require_session(&headers, &state.db) 563 + .await 564 + .expect("valid session should succeed"); 565 + assert_eq!(result.did, did); 566 + } 567 + 568 + #[tokio::test] 569 + async fn session_expired_session_returns_401() { 570 + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; 571 + use rand_core::{OsRng, RngCore}; 572 + use sha2::{Digest, Sha256}; 573 + use uuid::Uuid; 574 + 575 + let state = test_state().await; 576 + 577 + // Insert an account (required by sessions FK constraint). 578 + let did = format!("did:plc:{}", &Uuid::new_v4().to_string().replace('-', "")[..24]); 579 + sqlx::query( 580 + "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 581 + VALUES (?, ?, NULL, datetime('now'), datetime('now'))", 582 + ) 583 + .bind(&did) 584 + .bind(format!("test{}@example.com", &did[8..16])) 585 + .execute(&state.db) 586 + .await 587 + .expect("insert account"); 588 + 589 + let mut token_bytes = [0u8; 32]; 590 + OsRng.fill_bytes(&mut token_bytes); 591 + let session_token = URL_SAFE_NO_PAD.encode(token_bytes); 592 + let token_hash: String = Sha256::digest(token_bytes) 593 + .iter() 594 + .map(|b| format!("{b:02x}")) 595 + .collect(); 596 + 597 + sqlx::query( 598 + "INSERT INTO sessions (id, did, device_id, token_hash, created_at, expires_at) \ 599 + VALUES (?, ?, NULL, ?, datetime('now'), datetime('now', '-1 hour'))", 600 + ) 601 + .bind(Uuid::new_v4().to_string()) 602 + .bind(&did) 603 + .bind(&token_hash) 604 + .execute(&state.db) 605 + .await 606 + .expect("insert expired session"); 607 + 608 + let mut headers = HeaderMap::new(); 609 + headers.insert( 610 + axum::http::header::AUTHORIZATION, 611 + format!("Bearer {session_token}").parse().unwrap(), 612 + ); 613 + 614 + let err = require_session(&headers, &state.db) 482 615 .await 483 616 .unwrap_err(); 484 617 assert_eq!(err.status_code(), 401);
+178 -41
crates/relay/src/routes/create_handle.rs
··· 13 13 // 1. require_session → SessionInfo { did } 14 14 // 2. Validate account_id matches session did (prevents acting on other accounts) 15 15 // 3. validate_handle(handle, available_user_domains) → 400 INVALID_HANDLE on failure 16 - // 4. SELECT EXISTS(SELECT 1 FROM handles WHERE handle = ?) → 409 HANDLE_ALREADY_EXISTS 17 - // 5. If state.dns_provider is Some: call create_record(name, target); dns_status = "propagating" 16 + // 4. INSERT INTO handles (handle, did, created_at) → 409 HANDLE_TAKEN on UNIQUE violation 17 + // 5. If state.dns_provider is Some: call create_record(name, hostname); dns_status = "propagating" 18 18 // If state.dns_provider is None: dns_status = "not_configured" 19 - // 6. INSERT INTO handles (handle, did, created_at) 20 - // 7. Return { "handle": "...", "dns_status": "...", "did": "..." } 19 + // 6. Return { "handle": "...", "dns_status": "...", "did": "..." } 20 + // 21 + // Note: INSERT precedes the DNS call (step 4 before step 5) so that a DB row 22 + // without a DNS record is a recoverable/operator-fixable state, whereas a DNS 23 + // record without a DB row would be an invisible orphan. 21 24 // 22 25 // Outputs (success): 200 { "handle": "...", "dns_status": "not_configured"|"propagating", "did": "..." } 23 - // Outputs (error): 400 INVALID_HANDLE, 401 UNAUTHORIZED, 409 HANDLE_ALREADY_EXISTS, 24 - // 500 INTERNAL_ERROR 26 + // Outputs (error): 400 INVALID_HANDLE, 401 UNAUTHORIZED, 409 HANDLE_TAKEN, 27 + // 502 DNS_ERROR, 500 INTERNAL_ERROR 25 28 26 29 use axum::{extract::State, http::HeaderMap, Json}; 27 30 use serde::{Deserialize, Serialize}; ··· 64 67 let name = validate_handle(&payload.handle, &state.config.available_user_domains) 65 68 .map_err(|msg| ApiError::new(ErrorCode::InvalidHandle, msg))?; 66 69 67 - // Step 4: Check handle uniqueness. 68 - let exists: bool = 69 - sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM handles WHERE handle = ?)") 70 - .bind(&payload.handle) 71 - .fetch_one(&state.db) 72 - .await 73 - .map_err(|e| { 74 - tracing::error!(error = %e, "failed to check handle uniqueness"); 75 - ApiError::new(ErrorCode::InternalError, "database error") 76 - })?; 77 - 78 - if exists { 79 - return Err(ApiError::new( 80 - ErrorCode::HandleAlreadyExists, 81 - "handle is already taken", 82 - )); 83 - } 70 + // Step 4: Insert the handle. A UNIQUE violation means the handle is already taken. 71 + sqlx::query("INSERT INTO handles (handle, did, created_at) VALUES (?, ?, datetime('now'))") 72 + .bind(&payload.handle) 73 + .bind(&session.did) 74 + .execute(&state.db) 75 + .await 76 + .map_err(|e| { 77 + if let sqlx::Error::Database(db_err) = &e { 78 + if db_err.is_unique_violation() { 79 + return ApiError::new(ErrorCode::HandleTaken, "handle is already taken"); 80 + } 81 + } 82 + tracing::error!(error = %e, "failed to insert handle"); 83 + ApiError::new(ErrorCode::InternalError, "failed to register handle") 84 + })?; 84 85 85 86 // Step 5: Create DNS record if a provider is configured. 87 + // INSERT precedes this call: a row with no DNS record is recoverable; a DNS record 88 + // with no row would be an invisible orphan. 89 + let public_url = &state.config.public_url; 90 + let hostname = public_url 91 + .strip_prefix("https://") 92 + .or_else(|| public_url.strip_prefix("http://")) 93 + .unwrap_or(public_url.as_str()); 94 + 86 95 let dns_status = if let Some(provider) = &state.dns_provider { 87 96 provider 88 - .create_record(name, &state.config.public_url) 97 + .create_record(name, hostname) 89 98 .await 90 99 .map_err(|e| { 91 - tracing::error!(error = %e, handle = %payload.handle, "DNS record creation failed"); 100 + tracing::error!( 101 + error = %e, 102 + handle = %payload.handle, 103 + did = %session.did, 104 + "DNS record creation failed" 105 + ); 92 106 ApiError::new(ErrorCode::DnsError, "failed to create DNS record") 93 107 })?; 94 108 "propagating" ··· 96 110 "not_configured" 97 111 }; 98 112 99 - // Step 6: Insert the handle. 100 - sqlx::query("INSERT INTO handles (handle, did, created_at) VALUES (?, ?, datetime('now'))") 101 - .bind(&payload.handle) 102 - .bind(&session.did) 103 - .execute(&state.db) 104 - .await 105 - .map_err(|e| { 106 - tracing::error!(error = %e, "failed to insert handle"); 107 - ApiError::new(ErrorCode::InternalError, "failed to register handle") 108 - })?; 109 - 110 - // Step 7: Return the result. 113 + // Step 6: Return the result. 111 114 Ok(Json(CreateHandleResponse { 112 115 handle: payload.handle, 113 116 dns_status, ··· 118 121 /// Validate a handle string against the server's available user domains. 119 122 /// 120 123 /// A valid handle is `<name>.<domain>` where: 121 - /// - `name` is non-empty, contains only ASCII alphanumerics and hyphens, 122 - /// and does not start or end with a hyphen. 124 + /// - `name` is non-empty, at most 63 characters (RFC 1035 label limit), contains only 125 + /// ASCII alphanumerics and hyphens, and does not start or end with a hyphen. 123 126 /// - `domain` is one of the server's `available_user_domains`. 124 127 /// 125 128 /// Returns the `name` portion on success so callers can use it for DNS record creation. ··· 140 143 if name.is_empty() { 141 144 return Err("handle name cannot be empty"); 142 145 } 146 + if name.len() > 63 { 147 + return Err("handle name exceeds maximum DNS label length of 63 characters"); 148 + } 143 149 if name.starts_with('-') || name.ends_with('-') { 144 150 return Err("handle name cannot start or end with a hyphen"); 145 151 } ··· 169 175 use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; 170 176 use rand_core::{OsRng, RngCore}; 171 177 use sha2::{Digest, Sha256}; 178 + use std::future::Future; 179 + use std::pin::Pin; 180 + use std::sync::Arc; 172 181 use tower::ServiceExt; 173 182 use uuid::Uuid; 174 183 ··· 227 236 assert_eq!(validate_handle("al-ice.example.com", &domains), Ok("al-ice")); 228 237 } 229 238 239 + #[test] 240 + fn validate_handle_rejects_name_exceeding_63_chars() { 241 + let domains = vec!["example.com".to_string()]; 242 + let long_name = "a".repeat(64); 243 + assert!(validate_handle(&format!("{long_name}.example.com"), &domains).is_err()); 244 + } 245 + 246 + #[test] 247 + fn validate_handle_accepts_name_exactly_63_chars() { 248 + let domains = vec!["example.com".to_string()]; 249 + let name = "a".repeat(63); 250 + assert!(validate_handle(&format!("{name}.example.com"), &domains).is_ok()); 251 + } 252 + 253 + // ── DNS provider test doubles ────────────────────────────────────────────── 254 + 255 + struct AlwaysOkDns; 256 + struct AlwaysErrDns; 257 + 258 + impl crate::dns::DnsProvider for AlwaysOkDns { 259 + fn create_record<'a>( 260 + &'a self, 261 + _name: &'a str, 262 + _target: &'a str, 263 + ) -> Pin<Box<dyn Future<Output = Result<(), crate::dns::DnsError>> + Send + 'a>> { 264 + Box::pin(async { Ok(()) }) 265 + } 266 + } 267 + 268 + impl crate::dns::DnsProvider for AlwaysErrDns { 269 + fn create_record<'a>( 270 + &'a self, 271 + _name: &'a str, 272 + _target: &'a str, 273 + ) -> Pin<Box<dyn Future<Output = Result<(), crate::dns::DnsError>> + Send + 'a>> { 274 + Box::pin(async { 275 + Err(crate::dns::DnsError("simulated provider error".to_string())) 276 + }) 277 + } 278 + } 279 + 280 + async fn state_with_ok_dns() -> crate::app::AppState { 281 + let base = test_state().await; 282 + crate::app::AppState { 283 + dns_provider: Some(Arc::new(AlwaysOkDns)), 284 + ..base 285 + } 286 + } 287 + 288 + async fn state_with_err_dns() -> crate::app::AppState { 289 + let base = test_state().await; 290 + crate::app::AppState { 291 + dns_provider: Some(Arc::new(AlwaysErrDns)), 292 + ..base 293 + } 294 + } 295 + 230 296 // ── Integration test helpers ─────────────────────────────────────────────── 231 297 232 298 struct TestSession { ··· 328 394 assert_eq!(stored_did, ts.did); 329 395 } 330 396 397 + // ── DNS provider tests ───────────────────────────────────────────────────── 398 + 399 + /// DNS provider succeeds: row is inserted, response has dns_status: "propagating". 400 + #[tokio::test] 401 + async fn dns_provider_success_returns_propagating_status() { 402 + let state = state_with_ok_dns().await; 403 + let db = state.db.clone(); 404 + let ts = insert_account_and_session(&db).await; 405 + let handle = format!("alice.{}", state.config.available_user_domains[0]); 406 + 407 + let app = crate::app::app(state); 408 + let response = app 409 + .oneshot(create_handle_request(&ts.session_token, &ts.did, &handle)) 410 + .await 411 + .unwrap(); 412 + 413 + assert_eq!(response.status(), StatusCode::OK); 414 + let body: serde_json::Value = serde_json::from_slice( 415 + &axum::body::to_bytes(response.into_body(), usize::MAX) 416 + .await 417 + .unwrap(), 418 + ) 419 + .unwrap(); 420 + assert_eq!(body["dns_status"].as_str(), Some("propagating")); 421 + 422 + let row: Option<(String,)> = 423 + sqlx::query_as("SELECT did FROM handles WHERE handle = ?") 424 + .bind(&handle) 425 + .fetch_optional(&db) 426 + .await 427 + .unwrap(); 428 + assert!(row.is_some(), "handles row must be inserted on DNS success"); 429 + } 430 + 431 + /// DNS provider fails: returns 502 DNS_ERROR; the handles row is inserted before DNS 432 + /// is attempted and persists (recoverable/retryable by an operator). 433 + #[tokio::test] 434 + async fn dns_provider_failure_returns_502_and_row_persists() { 435 + let state = state_with_err_dns().await; 436 + let db = state.db.clone(); 437 + let ts = insert_account_and_session(&db).await; 438 + let handle = format!("alice.{}", state.config.available_user_domains[0]); 439 + 440 + let app = crate::app::app(state); 441 + let response = app 442 + .oneshot(create_handle_request(&ts.session_token, &ts.did, &handle)) 443 + .await 444 + .unwrap(); 445 + 446 + assert_eq!(response.status(), StatusCode::BAD_GATEWAY); 447 + let body: serde_json::Value = serde_json::from_slice( 448 + &axum::body::to_bytes(response.into_body(), usize::MAX) 449 + .await 450 + .unwrap(), 451 + ) 452 + .unwrap(); 453 + assert_eq!(body["error"]["code"], "DNS_ERROR"); 454 + 455 + // INSERT precedes the DNS call: the row is durable even when DNS fails. 456 + let row: Option<(String,)> = 457 + sqlx::query_as("SELECT did FROM handles WHERE handle = ?") 458 + .bind(&handle) 459 + .fetch_optional(&db) 460 + .await 461 + .unwrap(); 462 + assert!( 463 + row.is_some(), 464 + "handles row is inserted before DNS and persists even when DNS fails" 465 + ); 466 + } 467 + 331 468 // ── Duplicate handle ─────────────────────────────────────────────────────── 332 469 333 - /// Creating the same handle twice returns 409 HANDLE_ALREADY_EXISTS. 470 + /// Creating the same handle twice returns 409 HANDLE_TAKEN. 334 471 #[tokio::test] 335 472 async fn duplicate_handle_returns_409() { 336 473 let state = test_state().await; ··· 359 496 .unwrap(), 360 497 ) 361 498 .unwrap(); 362 - assert_eq!(body["error"]["code"], "HANDLE_ALREADY_EXISTS"); 499 + assert_eq!(body["error"]["code"], "HANDLE_TAKEN"); 363 500 } 364 501 365 502 // ── Invalid handle format ──────────────────────────────────────────────────