str4d.xyz / atproto-devtool
CLI app for developers prototyping atproto functionality
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

fix(oauth-client): correct RP crypto, clean up sign_dpop errors, tighten happy-path test

Three groups of changes that only make sense together, because tightening
the happy-path assertions exposes crypto bugs that must be fixed in the
same commit for bisect-ability:

**Crypto correctness (relying_party.rs):**

1. new_pkce was computing the PKCE S256 code_challenge as
BASE64URL(SHA256(raw_random_bytes)). RFC 7636 §4.2 requires
BASE64URL(SHA256(ASCII(code_verifier))) — the hash is over the ASCII
of the base64url-encoded verifier string, not over the raw preimage.
Fix: hasher.update(verifier.as_bytes()). The fake AS already matched
the RFC, so this was a pure RP-side bug.

2. sign_dpop was base64url-encoding the DPoP signature a second time.
jsonwebtoken::crypto::sign returns a base64url-encoded String, not
raw bytes. The code treated it as Vec<u8> and re-encoded it, producing
a nonsense signature that verification rejected with
invalid_dpop_proof. Fix: take the returned String directly into the
JWS.

**Interactive stage (pipeline/interactive.rs):**

3. The DPoP header presence check compared against the literal "DPoP",
but axum stores header names lowercase in HeaderMap::as_str. Fix:
eq_ignore_ascii_case.

**Cleanup (relying_party.rs, addresses cycle-3 I3 and M1):**

4. sign_dpop's two serde_json::to_vec call sites now use
.expect("serde_json::to_vec over Value is infallible") instead of
mapping to RpError::MetadataMalformed. serde_json::to_vec over a
serde_json::Value is infallible in practice, and MetadataMalformed
was semantically wrong for a serialization failure.

5. The dead signing_key: Arc<P256SigningKey> field and its
#[expect(dead_code)] are removed. encoding_key + signing_jwk_public
are both derived at construction, so signing_key is now a local in
new() that's dropped after use.

**Test tightening (tests/oauth_client_interactive.rs, addresses cycle-3 C2):**

6. interactive_happy_path_gates_all_pass now asserts CheckStatus::Pass
by name on each of ClientReachedPar, ClientUsedPkceS256,
ClientIncludedDpop, ClientCompletedToken. The previous assertion
set (len == 6 + ServerBound + ClientRefreshed) was satisfied even
when every path-variable check was SpecViolation. The tight
assertions exercise the real RP↔fake-AS round-trip and caught the
crypto bugs above.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Jack Grigg 40c51bf2 0b021084

+64 -15
+5 -2
src/commands/test/oauth/client/pipeline/interactive.rs
··· 361 361 results.push(Check::ClientUsedPkceS256.spec_violation()); 362 362 } 363 363 364 - // Check for DPoP. 364 + // Check for DPoP. HTTP header names are stored lowercase. 365 365 let has_dpop = log_snapshot.iter().any(|req| { 366 366 req.method == "POST" 367 367 && req.path == "/oauth/par" 368 - && req.headers.iter().any(|(k, _)| k == "DPoP") 368 + && req 369 + .headers 370 + .iter() 371 + .any(|(k, _)| k.eq_ignore_ascii_case("DPoP")) 369 372 }); 370 373 371 374 if has_dpop {
+11 -13
src/common/oauth/relying_party.rs
··· 118 118 pub struct RelyingParty { 119 119 client_id: Url, 120 120 kind: ClientKind, 121 - #[expect(dead_code)] 122 - signing_key: Arc<P256SigningKey>, 123 121 encoding_key: Arc<EncodingKey>, 124 122 signing_jwk_public: Value, 125 123 clock: Arc<dyn Clock>, ··· 179 177 Self { 180 178 client_id, 181 179 kind, 182 - signing_key: Arc::new(signing_key), 183 180 encoding_key: Arc::new(encoding_key), 184 181 signing_jwk_public, 185 182 clock, ··· 506 503 } 507 504 508 505 /// Generate a PKCE code verifier and challenge (S256). 506 + /// 507 + /// Per RFC 7636, `code_challenge = BASE64URL(SHA256(ASCII(code_verifier)))`. 508 + /// The verifier is the base64url-nopad string; the hash is computed over 509 + /// that ASCII string, not over the raw random bytes. 509 510 fn new_pkce(&self) -> (String, String) { 510 511 let mut verifier_bytes = [0u8; 32]; 511 512 let mut rng = self.rng.lock().unwrap(); ··· 514 515 let b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD; 515 516 let verifier = b64.encode(verifier_bytes); 516 517 517 - // SHA-256 hash of the verifier bytes (not the base64 string). 518 518 let mut hasher = Sha256::new(); 519 - hasher.update(verifier_bytes); 519 + hasher.update(verifier.as_bytes()); 520 520 let hash = hasher.finalize(); 521 521 let challenge = b64.encode(hash); 522 522 ··· 559 559 // Manually build the JWS by encoding header and claims as base64url. 560 560 let b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD; 561 561 let header_bytes = 562 - serde_json::to_vec(&header_obj).map_err(|_| RpError::MetadataMalformed { 563 - reason: "failed to serialize DPoP header".to_string(), 564 - })?; 565 - let claims_bytes = serde_json::to_vec(&claims).map_err(|_| RpError::MetadataMalformed { 566 - reason: "failed to serialize DPoP claims".to_string(), 567 - })?; 562 + serde_json::to_vec(&header_obj).expect("serde_json::to_vec over Value is infallible"); 563 + let claims_bytes = 564 + serde_json::to_vec(&claims).expect("serde_json::to_vec over Value is infallible"); 568 565 569 566 let header_b64 = b64.encode(&header_bytes); 570 567 let claims_b64 = b64.encode(&claims_bytes); 571 568 let signing_input = format!("{header_b64}.{claims_b64}"); 572 569 573 570 // Sign the input using the jsonwebtoken crate's low-level API. 574 - let signature = jsonwebtoken::crypto::sign( 571 + // `jsonwebtoken::crypto::sign` returns a base64url-encoded signature string; 572 + // do not re-encode. 573 + let signature_b64 = jsonwebtoken::crypto::sign( 575 574 signing_input.as_bytes(), 576 575 &self.encoding_key, 577 576 Algorithm::ES256, 578 577 ) 579 578 .map_err(|e| RpError::JwsFailure(jws::JwsError::SignFailed(e)))?; 580 579 581 - let signature_b64 = b64.encode(&signature); 582 580 Ok(format!("{signing_input}.{signature_b64}")) 583 581 } 584 582
+48
tests/oauth_client_interactive.rs
··· 285 285 "ServerBound should pass when gates allow" 286 286 ); 287 287 288 + // ClientReachedPar should pass after RP drives PAR. 289 + let client_reached_par = output 290 + .results 291 + .iter() 292 + .find(|r| r.id == "oauth_client::interactive::client_reached_par") 293 + .expect("ClientReachedPar check present"); 294 + assert_eq!( 295 + client_reached_par.status, 296 + CheckStatus::Pass, 297 + "ClientReachedPar should pass after RP drives PAR" 298 + ); 299 + 300 + // ClientUsedPkceS256 should pass for S256-compliant RP. 301 + let client_used_pkce_s256 = output 302 + .results 303 + .iter() 304 + .find(|r| r.id == "oauth_client::interactive::client_used_pkce_s256") 305 + .expect("ClientUsedPkceS256 check present"); 306 + assert_eq!( 307 + client_used_pkce_s256.status, 308 + CheckStatus::Pass, 309 + "ClientUsedPkceS256 should pass after RP drives PAR with S256" 310 + ); 311 + 312 + // ClientIncludedDpop should pass for DPoP-aware RP. 313 + let client_included_dpop = output 314 + .results 315 + .iter() 316 + .find(|r| r.id == "oauth_client::interactive::client_included_dpop") 317 + .expect("ClientIncludedDpop check present"); 318 + assert_eq!( 319 + client_included_dpop.status, 320 + CheckStatus::Pass, 321 + "ClientIncludedDpop should pass after RP drives PAR with DPoP" 322 + ); 323 + 324 + // ClientCompletedToken should pass for token exchange. 325 + let client_completed_token = output 326 + .results 327 + .iter() 328 + .find(|r| r.id == "oauth_client::interactive::client_completed_token") 329 + .expect("ClientCompletedToken check present"); 330 + assert_eq!( 331 + client_completed_token.status, 332 + CheckStatus::Pass, 333 + "ClientCompletedToken should pass after successful token exchange" 334 + ); 335 + 288 336 // ClientRefreshed should always be skipped in Phase 7. 289 337 let client_refreshed = output 290 338 .results