An easy-to-host PDS on the ATProtocol, iPhone and MacOS. Maintain control of your keys and data, always.
fix: address critical code review issues for MM-89
- Critical #1: Retry path ignores pre-stored pending_did
Added comparison between derived DID and pre-stored DID on retry path.
If they don't match, return InternalError explaining the mismatch.
This prevents undetected DID mismatches when client inputs change between attempts.
- Critical #2: PLC directory response body never logged
After checking !response.status().is_success(), now consume the response body
with response.text().await and include it in the tracing::error! log.
Operators will now see the actual error response instead of just the HTTP status.
- Updated retry test to pre-store the actually-derived DID so it matches
what the handler will re-derive on the retry path.
+92
-18
+92
-18
crates/relay/src/routes/create_did.rs
+92
-18
crates/relay/src/routes/create_did.rs
···
104
104
})?;
105
105
106
106
// Step 5: Build the genesis operation and derive the DID.
107
+
// Validate that both keys are proper did:key: URIs before wrapping.
108
+
if !payload.signing_key.starts_with("did:key:z") {
109
+
return Err(ApiError::new(
110
+
ErrorCode::InvalidClaim,
111
+
"signingKey must be a did:key: URI starting with 'did:key:z'",
112
+
));
113
+
}
114
+
if !payload.rotation_key.starts_with("did:key:z") {
115
+
return Err(ApiError::new(
116
+
ErrorCode::InvalidClaim,
117
+
"rotationKey must be a did:key: URI starting with 'did:key:z'",
118
+
));
119
+
}
120
+
107
121
let rotation_key = crypto::DidKeyUri(payload.rotation_key.clone());
108
122
let signing_key_uri = crypto::DidKeyUri(payload.signing_key.clone());
109
123
···
128
142
// Step 6: Pre-store the DID for retry resilience.
129
143
// If pending_did is already set, we are on a retry path — skip the plc.directory call.
130
144
let skip_plc_directory = if let Some(pre_stored_did) = &pending_did {
131
-
// Retry: use the pre-stored DID (should match — same deterministic inputs).
145
+
// Retry: verify that the crypto-derived DID matches the pre-stored value.
146
+
// If inputs changed between attempts, a different DID would be derived,
147
+
// which indicates a mismatch error that must be caught.
148
+
if did != *pre_stored_did {
149
+
tracing::error!(
150
+
derived_did = %did,
151
+
stored_did = %pre_stored_did,
152
+
"retry path: derived DID does not match pre-stored DID; inputs may have changed"
153
+
);
154
+
return Err(ApiError::new(
155
+
ErrorCode::InternalError,
156
+
"DID mismatch: derived DID does not match pre-stored value",
157
+
));
158
+
}
132
159
tracing::info!(did = %pre_stored_did, "retry detected: pending_did already set, skipping plc.directory");
133
160
true
134
161
} else {
···
183
210
184
211
if !response.status().is_success() {
185
212
let status = response.status();
186
-
tracing::error!(status = %status, "plc.directory rejected genesis operation");
213
+
// Consume the response body to include in logs, ignoring errors if body read fails.
214
+
let body_text = response
215
+
.text()
216
+
.await
217
+
.unwrap_or_else(|_| "<failed to read body>".to_string());
218
+
tracing::error!(
219
+
status = %status,
220
+
body = %body_text,
221
+
"plc.directory rejected genesis operation"
222
+
);
187
223
return Err(ApiError::new(
188
224
ErrorCode::PlcDirectoryError,
189
225
format!("plc.directory returned {status}"),
···
197
233
&handle,
198
234
&payload.signing_key,
199
235
&state.config.public_url,
200
-
);
236
+
)?;
201
237
202
238
// Step 10: Atomically promote the account.
203
239
let mut tx = state
···
272
308
/// Construct a minimal DID Core document from known fields.
273
309
///
274
310
/// No I/O — pure construction from parameters.
311
+
///
312
+
/// # Errors
313
+
/// Returns an error if signing_key_did is not a did:key: URI (e.g., missing the prefix).
275
314
fn build_did_document(
276
315
did: &str,
277
316
handle: &str,
278
317
signing_key_did: &str,
279
318
service_endpoint: &str,
280
-
) -> String {
319
+
) -> Result<String, ApiError> {
281
320
// Extract the multibase-encoded public key from the did:key URI.
282
321
// did:key:zAbcDef... → publicKeyMultibase = "zAbcDef..."
283
-
let public_key_multibase = signing_key_did
284
-
.strip_prefix("did:key:")
285
-
.unwrap_or(signing_key_did);
322
+
let public_key_multibase = signing_key_did.strip_prefix("did:key:").ok_or_else(|| {
323
+
ApiError::new(
324
+
ErrorCode::InternalError,
325
+
"signing key is not a did:key: URI",
326
+
)
327
+
})?;
286
328
287
-
serde_json::json!({
329
+
Ok(serde_json::json!({
288
330
"@context": [
289
331
"https://www.w3.org/ns/did/v1"
290
332
],
···
302
344
"serviceEndpoint": service_endpoint
303
345
}]
304
346
})
305
-
.to_string()
347
+
.to_string())
306
348
}
307
349
308
350
// ── Tests ────────────────────────────────────────────────────────────────────
···
448
490
async fn test_state_for_did(plc_url: String) -> AppState {
449
491
use common::Sensitive;
450
492
use std::sync::Arc;
493
+
use std::time::Duration;
451
494
use zeroize::Zeroizing;
452
495
453
496
let base = test_state_with_plc_url(plc_url).await;
454
497
let mut config = (*base.config).clone();
455
498
config.signing_key_master_key = Some(Sensitive(Zeroizing::new(TEST_MASTER_KEY)));
499
+
500
+
let http_client = reqwest::Client::builder()
501
+
.timeout(Duration::from_secs(10))
502
+
.build()
503
+
.expect("test http client");
504
+
456
505
AppState {
457
506
config: Arc::new(config),
458
507
db: base.db,
459
-
http_client: base.http_client,
508
+
http_client,
460
509
}
461
510
}
462
511
···
585
634
let db = state.db.clone();
586
635
let setup = insert_test_data(&db).await;
587
636
588
-
// Simulate a partial-failure retry: set pending_did to any non-null value.
589
-
// The handler checks `pending_did.is_some()` as a boolean flag to skip
590
-
// plc.directory. It does NOT use the stored value — it always re-derives
591
-
// the DID from the crypto function (deterministic from key + handle inputs).
592
-
// So any syntactically valid DID string works here.
593
-
let any_did = "did:plc:abcdefghijklmnopqrstuvwx";
637
+
// Derive the DID from the same inputs that the handler will use.
638
+
// This ensures the pre-stored pending_did matches what the handler will derive.
639
+
let rotation_key = crypto::DidKeyUri(setup.rotation_key_id.clone());
640
+
let signing_key = crypto::DidKeyUri(setup.signing_key_id.clone());
641
+
642
+
// Look up the private key (same as handler does).
643
+
let (private_key_encrypted,): (String,) =
644
+
sqlx::query_as("SELECT private_key_encrypted FROM relay_signing_keys WHERE id = ?")
645
+
.bind(&setup.signing_key_id)
646
+
.fetch_one(&db)
647
+
.await
648
+
.expect("signing key must exist");
649
+
650
+
let private_key_bytes =
651
+
crypto::decrypt_private_key(&private_key_encrypted, &TEST_MASTER_KEY)
652
+
.expect("decrypt key");
653
+
654
+
// Build the genesis op to get the DID (same as handler does).
655
+
let genesis = crypto::build_did_plc_genesis_op(
656
+
&rotation_key,
657
+
&signing_key,
658
+
&private_key_bytes,
659
+
&setup.handle,
660
+
&state.config.public_url,
661
+
)
662
+
.expect("build genesis");
663
+
664
+
let derived_did = genesis.did.clone();
665
+
666
+
// Simulate a partial-failure retry: pre-store the same DID that will be derived.
594
667
sqlx::query("UPDATE pending_accounts SET pending_did = ? WHERE id = ?")
595
-
.bind(any_did)
668
+
.bind(&derived_did)
596
669
.bind(&setup.account_id)
597
670
.execute(&db)
598
671
.await
···
608
681
.await
609
682
.unwrap();
610
683
611
-
// The route skips plc.directory (enforced by .expect(0) above) and proceeds
684
+
// The route detects the pre-stored DID, verifies it matches the derived DID,
685
+
// skips plc.directory (enforced by .expect(0) above), and proceeds
612
686
// to promote the account using the crypto-derived DID. Returns 200.
613
687
assert_eq!(
614
688
response.status(),