+5 -4

src/commands/test/oauth/client/pipeline.rs

reviewed

··· 49 49 pub has_key_for_signing_alg: CheckStatus, 50 50 /// Status of the grant_types check. 51 51 pub grant_types_includes_authorization_code: CheckStatus, 52 52 - /// Status of the refresh_token grant type check. 53 53 - /// Phase 7 hard-codes ClientRefreshed.skipped; refresh token support is wired in Phase 8. 52 52 + /// Status of the refresh_token grant type check. Consulted by the 53 53 + /// `scope_variations` and `dpop_edges` sub-stages, which exercise 54 54 + /// refresh flows; the happy-path flow always skips `ClientRefreshed`. 54 55 pub grant_types_includes_refresh_token: CheckStatus, 55 55 - /// Status of the response_types check. 56 56 - /// Used by dpop_edges sub-stage gating in Phase 8. 56 56 + /// Status of the response_types check. Consulted by the `dpop_edges` 57 57 + /// sub-stage's gate. 57 58 pub response_types_is_code: CheckStatus, 58 59 } 59 60

+33 -17

src/commands/test/oauth/client/pipeline/interactive.rs

reviewed

··· 45 45 pub struct InteractiveStageOutput { 46 46 /// All check results from this stage. 47 47 pub results: Vec<CheckResult>, 48 48 - /// Facts produced by this stage (unused by Phase 7; reserved for Phase 8). 48 48 + /// Facts produced by this stage. Currently unused; retained as a 49 49 + /// hook for future consumers. 49 50 pub facts: Option<InteractiveFacts>, 50 51 } 51 52 ··· 244 245 /// single live flow and cannot exercise this check. 245 246 const IS_SUB_EXTERNAL_DRIVER_REASON: &str = "broken-AS sub-stage requires in-process RP driver; external-client mode only observes a single flow"; 246 247 248 248 + /// Reason emitted on `ClientRefreshed` from the happy-path flow. The 249 249 + /// happy-path deliberately does not exercise refresh; the 250 250 + /// `scope_variations` and `dpop_edges` sub-stages drive refresh flows 251 251 + /// with downscoping, nonce rotation, and replay scenarios. 252 252 + const REFRESH_COVERED_BY_SUBSTAGES_REASON: &str = 253 253 + "covered by the scope_variations and dpop_edges sub-stages"; 254 254 + 247 255 /// Returns true if the form-encoded `par_body` contains a `key=` pair. 248 256 /// Used to detect `state=` / `nonce=` without requiring a full 249 257 /// form-decode (we only care about key presence). Handles both ··· 311 319 "oauth_client::metadata::scope_present", 312 320 )); 313 321 } 314 314 - // ClientRefreshed is always skipped in Phase 7. 315 315 - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 322 322 + // ClientRefreshed is never emitted by the happy-path flow; the 323 323 + // scope_variations and dpop_edges sub-stages exercise refresh. 324 324 + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 316 325 // Emit ServerBound.skipped to maintain consistent 6-check inventory. 317 326 results.insert( 318 327 0, ··· 342 351 "oauth_client::metadata::dpop_bound_required", 343 352 )); 344 353 } 345 345 - // ClientRefreshed is always skipped in Phase 7. 346 346 - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 354 354 + // ClientRefreshed is never emitted by the happy-path flow; the 355 355 + // scope_variations and dpop_edges sub-stages exercise refresh. 356 356 + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 347 357 // Emit ServerBound.skipped to maintain consistent 6-check inventory. 348 358 results.insert( 349 359 0, ··· 365 375 Check::ClientCompletedToken.summary(), 366 376 "oauth_client::jws::has_key_for_signing_alg", 367 377 )); 368 368 - // ClientRefreshed is always skipped in Phase 7. 369 369 - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 378 378 + // ClientRefreshed is never emitted by the happy-path flow; the 379 379 + // scope_variations and dpop_edges sub-stages exercise refresh. 380 380 + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 370 381 // Emit ServerBound.skipped to maintain consistent 6-check inventory. 371 382 results.insert( 372 383 0, ··· 410 421 InteractiveDriveMode::WaitForExternalClient => { 411 422 // Wait for Ctrl-C. 412 423 let _ = tokio::signal::ctrl_c().await; 413 413 - // TODO(Phase 8): Inspect request log and emit checks based on what was captured. 424 424 + // TODO: Inspect the request log and emit checks based on what 425 425 + // was captured from the external client. Until that lands, we 426 426 + // can't tell whether the client behaved, so every check that 427 427 + // requires observation is pessimistically reported as a 428 428 + // SpecViolation. 414 429 results.push(Check::ClientReachedPar.spec_violation()); 415 430 results.push(Check::ClientUsedPkceS256.spec_violation()); 416 431 results.push(Check::ClientIncludedDpop.spec_violation()); ··· 419 434 results.push(Check::ClientVerifiedIss.skipped(IS_SUB_EXTERNAL_DRIVER_REASON)); 420 435 results.push(Check::ClientVerifiedSub.skipped(IS_SUB_EXTERNAL_DRIVER_REASON)); 421 436 results.push(Check::ClientCompletedToken.spec_violation()); 422 422 - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 437 437 + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 423 438 } 424 439 InteractiveDriveMode::DriveRpInProcess { rp_factory } => { 425 440 // Drive the RP through the happy path. ··· 444 459 results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); 445 460 results.push(Check::ClientCompletedToken.spec_violation()); 446 461 results 447 447 - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 462 462 + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 448 463 server.shutdown().await; 449 464 return InteractiveStageOutput { 450 465 results, ··· 475 490 results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); 476 491 results.push(Check::ClientCompletedToken.spec_violation()); 477 492 results 478 478 - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 493 493 + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 479 494 server.shutdown().await; 480 495 return InteractiveStageOutput { 481 496 results, ··· 565 580 results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); 566 581 results.push(Check::ClientCompletedToken.spec_violation()); 567 582 results 568 568 - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 583 583 + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 569 584 server.shutdown().await; 570 585 return InteractiveStageOutput { 571 586 results, ··· 582 597 results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); 583 598 results.push(Check::ClientCompletedToken.spec_violation()); 584 599 results 585 585 - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 600 600 + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 586 601 server.shutdown().await; 587 602 return InteractiveStageOutput { 588 603 results, ··· 610 625 } 611 626 } 612 627 613 613 - // Phase 7 doesn't test refresh; skip with reason. 614 614 - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 628 628 + // The happy-path flow doesn't test refresh; the 629 629 + // scope_variations and dpop_edges sub-stages cover it. 630 630 + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); 615 631 616 632 // Broken-AS sub-stage: exercise the client's `iss` and 617 633 // `sub` verification. Drives two additional flows with ··· 623 639 let iss_sub_results = iss_sub_verification::run(&server, &rp).await; 624 640 results.extend(iss_sub_results); 625 641 626 626 - // Phase 8: Gate and run scope_variations sub-stage. 642 642 + // Gate and run scope_variations sub-stage. 627 643 let scope_gates_pass = static_gating.scope_present == CheckStatus::Pass 628 644 && static_gating.grant_types_includes_authorization_code == CheckStatus::Pass 629 645 && static_gating.dpop_bound_required == CheckStatus::Pass; ··· 663 679 results.extend(scope_results); 664 680 } 665 681 666 666 - // Phase 8: Gate and run dpop_edges sub-stage. 682 682 + // Gate and run dpop_edges sub-stage. 667 683 let dpop_gates_pass = 668 684 scope_gates_pass && static_gating.response_types_is_code == CheckStatus::Pass; 669 685

+1 -2

src/commands/test/oauth/client/pipeline/metadata.rs

reviewed

··· 29 29 pub dpop_bound_access_tokens: Option<bool>, 30 30 pub token_endpoint_auth_method: Option<String>, 31 31 pub token_endpoint_auth_signing_alg: Option<String>, 32 32 - /// Stored as raw JSON; Phase 5 parses it into JWK format. 32 32 + /// Stored as raw JSON; the JWKS stage parses it into JWK format. 33 33 pub jwks: Option<serde_json::Value>, 34 34 pub jwks_uri: Option<String>, 35 35 pub client_uri: Option<String>, ··· 208 208 for param in query_str.split('&') { 209 209 match param.split_once('=') { 210 210 Some((key, val)) => { 211 211 - // Optionally percent-decode key and value (for Phase 4, we keep it simple). 212 211 let key = percent_decode(key).map_err(|_| ScopeParseError { 213 212 token: format!("?{param}"), 214 213 byte_offset,

+5 -4

src/common/identity.rs

reviewed

··· 365 365 async fn get_bytes(&self, url: &Url) -> Result<(u16, Vec<u8>), IdentityError>; 366 366 367 367 /// Like `get_bytes` but additionally returns the response's `Content-Type` 368 368 - /// header value (if any). Added in Phase 3 of the oauth_client rollout so 369 369 - /// discovery can include content-type in AC1.6 diagnostics. Default impl 370 370 - /// calls `get_bytes` and returns `None` for `content_type`, so existing 371 371 - /// implementers (labeler stages) don't need updates. 368 368 + /// header value (if any). The oauth_client discovery stage needs the 369 369 + /// content-type to emit a targeted diagnostic when a metadata endpoint 370 370 + /// returns the wrong MIME. Default impl calls `get_bytes` and returns 371 371 + /// `None` for `content_type`, so existing implementers (labeler stages) 372 372 + /// don't need updates. 372 373 async fn get_bytes_with_content_type( 373 374 &self, 374 375 url: &Url,

+13 -13

src/common/oauth/jws.rs

reviewed

··· 75 75 76 76 /// A parsed JWK. Holds whatever structural metadata the JWK declares — 77 77 /// but deliberately does NOT validate that the JWK is complete or 78 78 - /// acceptable. Callers (the Phase 5 JWKS stage) decide whether a 79 79 - /// missing `alg` or an unrecognised algorithm is a spec violation, 80 80 - /// because those decisions map to specific stage-level check IDs. 78 78 + /// acceptable. Callers (the JWKS stage) decide whether a missing `alg` 79 79 + /// or an unrecognised algorithm is a spec violation, because those 80 80 + /// decisions map to specific stage-level check IDs. 81 81 /// 82 82 /// Design note: the JWS layer is deliberately permissive on `alg` 83 83 /// presence and value. Per RFC 7517 `alg` is OPTIONAL on a JWK, and 84 84 /// the atproto OAuth profile does not override that, so absence is 85 85 /// not a parse-time failure here. This keeps the per-key error → 86 86 - /// stage-level check mapping in exactly one place (Phase 5's 86 86 + /// stage-level check mapping in exactly one place (the JWKS stage's 87 87 /// `jwks::run`), which compares each key to the client metadata's 88 88 /// `token_endpoint_auth_signing_alg` via `has_key_for_signing_alg`. 89 89 /// 90 90 /// The case `alg = None` combined with `crv = secp256k1` is valid at 91 91 - /// the JWS layer (it parses successfully). Phase 5 decides whether 92 92 - /// this is a downstream concern (e.g., structurally incompatible with 93 93 - /// a declared `token_endpoint_auth_signing_alg`), not a hard 94 94 - /// structural failure. 91 91 + /// the JWS layer (it parses successfully). The JWKS stage decides 92 92 + /// whether this is a downstream concern (e.g., structurally 93 93 + /// incompatible with a declared `token_endpoint_auth_signing_alg`), 94 94 + /// not a hard structural failure. 95 95 #[derive(Debug, Clone)] 96 96 pub struct ParsedJwk { 97 97 pub kid: Option<Arc<str>>, 98 98 - /// Absent if the JWK omits `alg`. Phase 5 falls back to matching 99 99 - /// `crv` against `token_endpoint_auth_signing_alg` when `alg` is 100 100 - /// None. 98 98 + /// Absent if the JWK omits `alg`. The JWKS stage falls back to 99 99 + /// matching `crv` against `token_endpoint_auth_signing_alg` when 100 100 + /// `alg` is None. 101 101 pub alg: Option<JwsAlg>, 102 102 /// Raw alg string preserved verbatim when `alg` is present but not 103 103 - /// in {ES256, ES256K} — Phase 5 uses it to emit a pointed 103 103 + /// in {ES256, ES256K} — the JWKS stage uses it to emit a pointed 104 104 /// `algs_are_modern_ec` diagnostic citing the offending value. 105 105 pub alg_raw: Option<Arc<str>>, 106 106 /// The curve declared on the JWK. Retained so the JWKS stage can ··· 204 204 /// Returns a `ParsedJwk` if the object is structurally a JSON object with 205 205 /// at least a recognised `kty`. Deliberately permissive on `alg` (absent 206 206 /// or unrecognised) and `use` (any string accepted into `JwkUse::Other`); 207 207 - /// callers (Phase 5 JWKS stage) decide whether those are violations. 207 207 + /// callers (the JWKS stage) decide whether those are violations. 208 208 /// 209 209 /// Returns `Err(JwsError)` only for hard structural failures: 210 210 /// - Input is not a JSON object.

+4 -1

src/common/oauth/relying_party.rs

reviewed

··· 649 649 params.push(("client_assertion", jwt)); 650 650 } 651 651 652 652 - // Build DPoP proof (ath claim omitted per Phase 7 plan). 652 652 + // Build DPoP proof. The `ath` claim is omitted because this is 653 653 + // the authorization-code → token exchange: there is no access 654 654 + // token to bind yet (RFC 9449 §4.1 requires `ath` only when the 655 655 + // proof is presented alongside an access token). 653 656 let dpop = self.sign_dpop("POST", &as_descriptor.token_endpoint, None)?; 654 657 655 658 let body = serde_urlencoded::to_string(&params)?;

+5 -5

tests/oauth_client_ac_coverage.rs

reviewed

··· 199 199 let output = interactive::run(static_gating, None, None, &interactive_opts, clock).await; 200 200 server.shutdown().await; 201 201 202 202 - // Every Phase 7 + Phase 8 check ID should appear among emitted results. 202 202 + // Every happy-path and sub-stage check ID should appear among emitted results. 203 203 use atproto_devtool::commands::test::oauth::client::pipeline::interactive::{ 204 204 CHECK_ALL as INTERACTIVE_ALL, dpop_edges, scope_variations, 205 205 }; ··· 238 238 use atproto_devtool::common::oauth::relying_party::{DeterministicRpFactory, RpFactory}; 239 239 use atproto_devtool::common::report::CheckStatus; 240 240 241 241 - // Fail `grant_types_includes_refresh_token` — only ClientRefreshed (Phase 7) 242 242 - // is conditionally-refreshed; AC5.4 says non-gating failures leave every 241 241 + // Fail `grant_types_includes_refresh_token` — only ClientRefreshed is 242 242 + // conditionally-refreshed; AC5.4 says non-gating failures leave every 243 243 // other interactive check running. The gate table in interactive::run 244 244 // doesn't actually consult refresh_token presence (ClientRefreshed is 245 245 - // always Skipped in Phase 7), so failing it must not affect any other 246 246 - // check's execution. 245 245 + // always Skipped by the happy-path flow), so failing it must not 246 246 + // affect any other check's execution. 247 247 let (server, clock) = spawn_fake_as().await; 248 248 let static_gating = StaticGating { 249 249 scope_present: CheckStatus::Pass,

+4 -4

tests/oauth_client_check_id_coverage.rs

reviewed

··· 120 120 fn all_expected_diagnostic_codes() -> Vec<&'static str> { 121 121 // Only codes currently exercised by a failing-path snapshot are listed. 122 122 // Codes that live in source but have no triggering fixture today 123 123 - // (`public_forbids_jwks`, the three Phase-7 interactive codes that only 124 124 - // fire on WaitForExternalClient, etc.) are intentionally omitted rather 125 125 - // than failing this test vacuously — add them when a corresponding 126 126 - // snapshot lands. 123 123 + // (`public_forbids_jwks`, the happy-path interactive codes that only 124 124 + // fire on WaitForExternalClient, etc.) are intentionally omitted 125 125 + // rather than failing this test vacuously — add them when a 126 126 + // corresponding snapshot lands. 127 127 vec![ 128 128 // Discovery stage diagnostic codes. 129 129 "oauth_client::discovery::metadata_document_fetchable",

+9 -8

tests/oauth_client_interactive.rs

reviewed

··· 276 276 let output = interactive::run(static_gating, None, None, &interactive_opts, clock).await; 277 277 278 278 // Verify that all expected checks are present. 279 279 - // Phase 7: 10 checks (ServerBound, ClientReachedPar, ClientUsedPkceS256, 280 280 - // ClientIncludedDpop, ClientIncludedState, ClientOmittedNonce, 281 281 - // ClientVerifiedIss, ClientVerifiedSub, ClientCompletedToken, 282 282 - // ClientRefreshed). 283 283 - // Phase 8: 6 scope_variations checks + 6 dpop_edges checks. 279 279 + // Happy-path: 10 checks (ServerBound, ClientReachedPar, 280 280 + // ClientUsedPkceS256, ClientIncludedDpop, ClientIncludedState, 281 281 + // ClientOmittedNonce, ClientVerifiedIss, ClientVerifiedSub, 282 282 + // ClientCompletedToken, ClientRefreshed). 283 283 + // Sub-stages: 6 scope_variations checks + 6 dpop_edges checks. 284 284 // Total: 22 checks. 285 285 assert_eq!( 286 286 output.results.len(), 287 287 22, 288 288 - "should have 22 checks (10 phase 7 + 6 scope_variations + 6 dpop_edges)" 288 288 + "should have 22 checks (10 happy-path + 6 scope_variations + 6 dpop_edges)" 289 289 ); 290 290 291 291 // Check that ServerBound passed (server bind was successful). ··· 348 348 "ClientCompletedToken should pass after successful token exchange" 349 349 ); 350 350 351 351 - // ClientRefreshed should always be skipped in Phase 7. 351 351 + // ClientRefreshed is always skipped by the happy-path flow; refresh 352 352 + // behavior is exercised by the scope_variations / dpop_edges sub-stages. 352 353 let client_refreshed = output 353 354 .results 354 355 .iter() ··· 357 358 assert_eq!( 358 359 client_refreshed.status, 359 360 CheckStatus::Skipped, 360 360 - "ClientRefreshed should be skipped in Phase 7" 361 361 + "ClientRefreshed should be skipped by the happy-path flow" 361 362 ); 362 363 363 364 // AC4.10: the broken-AS sub-stage drives two extra flows and

+8 -7

tests/oauth_client_substage_snapshots.rs

reviewed

··· 203 203 204 204 #[tokio::test] 205 205 async fn interactive_stage_blocked_by_static_failures_snapshot() { 206 206 - // Exercises the blocked_by branch of interactive::run: every Phase 7 207 207 - // interactive check emits with its stable ID visible as "blocked by <id>" 208 208 - // or as the diagnostic-code header on a SpecViolation row. The point is 209 209 - // to put `oauth_client::interactive::*` IDs into at least one snapshot. 206 206 + // Exercises the blocked_by branch of interactive::run: every 207 207 + // happy-path interactive check emits with its stable ID visible as 208 208 + // "blocked by <id>" or as the diagnostic-code header on a 209 209 + // SpecViolation row. The point is to put 210 210 + // `oauth_client::interactive::*` IDs into at least one snapshot. 210 211 use atproto_devtool::commands::test::oauth::client::pipeline::interactive; 211 212 use atproto_devtool::commands::test::oauth::client::pipeline::{ 212 213 InteractiveDriveMode, InteractiveOptions, StaticGating, ··· 215 216 216 217 let (server, clock) = spawn_fake_as().await; 217 218 218 218 - // Simulate a failed scope_present prerequisite — all 5 interactive 219 219 - // Phase 7 checks emit blocked_by referencing their own IDs in the 220 220 - // rendered output, plus ServerBound as Skipped. 219 219 + // Simulate a failed scope_present prerequisite — all 5 happy-path 220 220 + // interactive checks emit blocked_by referencing their own IDs in 221 221 + // the rendered output, plus ServerBound as Skipped. 221 222 let static_gating = StaticGating { 222 223 scope_present: CheckStatus::SpecViolation, 223 224 dpop_bound_required: CheckStatus::Pass,

+1 -1

tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap

reviewed

··· 15 15 [SKIP] Client verified AS `iss` on authorization redirect — blocked by oauth_client::metadata::scope_present 16 16 [SKIP] Client verified token response `sub` matches expected DID — blocked by oauth_client::metadata::scope_present 17 17 [SKIP] Client completed token exchange — blocked by oauth_client::metadata::scope_present 18 18 - [SKIP] Client refreshed access token — covered in Phase 8 flow variants 18 18 + [SKIP] Client refreshed access token — covered by the scope_variations and dpop_edges sub-stages 19 19 20 20 Summary: 0 passed, 0 failed (spec), 0 network errors, 0 advisories, 10 skipped. Exit code: 0