diff --git a/src/commands/test/oauth/client/pipeline.rs b/src/commands/test/oauth/client/pipeline.rs index df135ce..7bd7e39 100644 --- a/src/commands/test/oauth/client/pipeline.rs +++ b/src/commands/test/oauth/client/pipeline.rs @@ -49,11 +49,12 @@ pub struct StaticGating { pub has_key_for_signing_alg: CheckStatus, /// Status of the grant_types check. pub grant_types_includes_authorization_code: CheckStatus, - /// Status of the refresh_token grant type check. - /// Phase 7 hard-codes ClientRefreshed.skipped; refresh token support is wired in Phase 8. + /// Status of the refresh_token grant type check. Consulted by the + /// `scope_variations` and `dpop_edges` sub-stages, which exercise + /// refresh flows; the happy-path flow always skips `ClientRefreshed`. pub grant_types_includes_refresh_token: CheckStatus, - /// Status of the response_types check. - /// Used by dpop_edges sub-stage gating in Phase 8. + /// Status of the response_types check. Consulted by the `dpop_edges` + /// sub-stage's gate. pub response_types_is_code: CheckStatus, } diff --git a/src/commands/test/oauth/client/pipeline/interactive.rs b/src/commands/test/oauth/client/pipeline/interactive.rs index 6afac09..5e1dd26 100644 --- a/src/commands/test/oauth/client/pipeline/interactive.rs +++ b/src/commands/test/oauth/client/pipeline/interactive.rs @@ -45,7 +45,8 @@ pub struct InteractiveFacts { pub struct InteractiveStageOutput { /// All check results from this stage. pub results: Vec, - /// Facts produced by this stage (unused by Phase 7; reserved for Phase 8). + /// Facts produced by this stage. Currently unused; retained as a + /// hook for future consumers. pub facts: Option, } @@ -244,6 +245,13 @@ const IS_SUB_SKIP_BLOCKED_REASON: &str = /// single live flow and cannot exercise this check. const IS_SUB_EXTERNAL_DRIVER_REASON: &str = "broken-AS sub-stage requires in-process RP driver; external-client mode only observes a single flow"; +/// Reason emitted on `ClientRefreshed` from the happy-path flow. The +/// happy-path deliberately does not exercise refresh; the +/// `scope_variations` and `dpop_edges` sub-stages drive refresh flows +/// with downscoping, nonce rotation, and replay scenarios. +const REFRESH_COVERED_BY_SUBSTAGES_REASON: &str = + "covered by the scope_variations and dpop_edges sub-stages"; + /// Returns true if the form-encoded `par_body` contains a `key=` pair. /// Used to detect `state=` / `nonce=` without requiring a full /// form-decode (we only care about key presence). Handles both @@ -311,8 +319,9 @@ pub async fn run( "oauth_client::metadata::scope_present", )); } - // ClientRefreshed is always skipped in Phase 7. - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + // ClientRefreshed is never emitted by the happy-path flow; the + // scope_variations and dpop_edges sub-stages exercise refresh. + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); // Emit ServerBound.skipped to maintain consistent 6-check inventory. results.insert( 0, @@ -342,8 +351,9 @@ pub async fn run( "oauth_client::metadata::dpop_bound_required", )); } - // ClientRefreshed is always skipped in Phase 7. - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + // ClientRefreshed is never emitted by the happy-path flow; the + // scope_variations and dpop_edges sub-stages exercise refresh. + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); // Emit ServerBound.skipped to maintain consistent 6-check inventory. results.insert( 0, @@ -365,8 +375,9 @@ pub async fn run( Check::ClientCompletedToken.summary(), "oauth_client::jws::has_key_for_signing_alg", )); - // ClientRefreshed is always skipped in Phase 7. - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + // ClientRefreshed is never emitted by the happy-path flow; the + // scope_variations and dpop_edges sub-stages exercise refresh. + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); // Emit ServerBound.skipped to maintain consistent 6-check inventory. results.insert( 0, @@ -410,7 +421,11 @@ pub async fn run( InteractiveDriveMode::WaitForExternalClient => { // Wait for Ctrl-C. let _ = tokio::signal::ctrl_c().await; - // TODO(Phase 8): Inspect request log and emit checks based on what was captured. + // TODO: Inspect the request log and emit checks based on what + // was captured from the external client. Until that lands, we + // can't tell whether the client behaved, so every check that + // requires observation is pessimistically reported as a + // SpecViolation. results.push(Check::ClientReachedPar.spec_violation()); results.push(Check::ClientUsedPkceS256.spec_violation()); results.push(Check::ClientIncludedDpop.spec_violation()); @@ -419,7 +434,7 @@ pub async fn run( results.push(Check::ClientVerifiedIss.skipped(IS_SUB_EXTERNAL_DRIVER_REASON)); results.push(Check::ClientVerifiedSub.skipped(IS_SUB_EXTERNAL_DRIVER_REASON)); results.push(Check::ClientCompletedToken.spec_violation()); - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); } InteractiveDriveMode::DriveRpInProcess { rp_factory } => { // Drive the RP through the happy path. @@ -444,7 +459,7 @@ pub async fn run( results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); results.push(Check::ClientCompletedToken.spec_violation()); results - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); server.shutdown().await; return InteractiveStageOutput { results, @@ -475,7 +490,7 @@ pub async fn run( results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); results.push(Check::ClientCompletedToken.spec_violation()); results - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); server.shutdown().await; return InteractiveStageOutput { results, @@ -565,7 +580,7 @@ pub async fn run( results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); results.push(Check::ClientCompletedToken.spec_violation()); results - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); server.shutdown().await; return InteractiveStageOutput { results, @@ -582,7 +597,7 @@ pub async fn run( results.push(Check::ClientVerifiedSub.skipped(IS_SUB_SKIP_BLOCKED_REASON)); results.push(Check::ClientCompletedToken.spec_violation()); results - .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + .push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); server.shutdown().await; return InteractiveStageOutput { results, @@ -610,8 +625,9 @@ pub async fn run( } } - // Phase 7 doesn't test refresh; skip with reason. - results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); + // The happy-path flow doesn't test refresh; the + // scope_variations and dpop_edges sub-stages cover it. + results.push(Check::ClientRefreshed.skipped(REFRESH_COVERED_BY_SUBSTAGES_REASON)); // Broken-AS sub-stage: exercise the client's `iss` and // `sub` verification. Drives two additional flows with @@ -623,7 +639,7 @@ pub async fn run( let iss_sub_results = iss_sub_verification::run(&server, &rp).await; results.extend(iss_sub_results); - // Phase 8: Gate and run scope_variations sub-stage. + // Gate and run scope_variations sub-stage. let scope_gates_pass = static_gating.scope_present == CheckStatus::Pass && static_gating.grant_types_includes_authorization_code == CheckStatus::Pass && static_gating.dpop_bound_required == CheckStatus::Pass; @@ -663,7 +679,7 @@ pub async fn run( results.extend(scope_results); } - // Phase 8: Gate and run dpop_edges sub-stage. + // Gate and run dpop_edges sub-stage. let dpop_gates_pass = scope_gates_pass && static_gating.response_types_is_code == CheckStatus::Pass; diff --git a/src/commands/test/oauth/client/pipeline/metadata.rs b/src/commands/test/oauth/client/pipeline/metadata.rs index 57df6ee..12e28f8 100644 --- a/src/commands/test/oauth/client/pipeline/metadata.rs +++ b/src/commands/test/oauth/client/pipeline/metadata.rs @@ -29,7 +29,7 @@ pub struct RawMetadataDocument { pub dpop_bound_access_tokens: Option, pub token_endpoint_auth_method: Option, pub token_endpoint_auth_signing_alg: Option, - /// Stored as raw JSON; Phase 5 parses it into JWK format. + /// Stored as raw JSON; the JWKS stage parses it into JWK format. pub jwks: Option, pub jwks_uri: Option, pub client_uri: Option, @@ -208,7 +208,6 @@ fn parse_query_string( for param in query_str.split('&') { match param.split_once('=') { Some((key, val)) => { - // Optionally percent-decode key and value (for Phase 4, we keep it simple). let key = percent_decode(key).map_err(|_| ScopeParseError { token: format!("?{param}"), byte_offset, diff --git a/src/common/identity.rs b/src/common/identity.rs index 4b266a6..83696b4 100644 --- a/src/common/identity.rs +++ b/src/common/identity.rs @@ -365,10 +365,11 @@ pub trait HttpClient: Send + Sync { async fn get_bytes(&self, url: &Url) -> Result<(u16, Vec), IdentityError>; /// Like `get_bytes` but additionally returns the response's `Content-Type` - /// header value (if any). Added in Phase 3 of the oauth_client rollout so - /// discovery can include content-type in AC1.6 diagnostics. Default impl - /// calls `get_bytes` and returns `None` for `content_type`, so existing - /// implementers (labeler stages) don't need updates. + /// header value (if any). The oauth_client discovery stage needs the + /// content-type to emit a targeted diagnostic when a metadata endpoint + /// returns the wrong MIME. Default impl calls `get_bytes` and returns + /// `None` for `content_type`, so existing implementers (labeler stages) + /// don't need updates. async fn get_bytes_with_content_type( &self, url: &Url, diff --git a/src/common/oauth/jws.rs b/src/common/oauth/jws.rs index d10740e..cbe9326 100644 --- a/src/common/oauth/jws.rs +++ b/src/common/oauth/jws.rs @@ -75,32 +75,32 @@ impl JwkCrv { /// A parsed JWK. Holds whatever structural metadata the JWK declares — /// but deliberately does NOT validate that the JWK is complete or -/// acceptable. Callers (the Phase 5 JWKS stage) decide whether a -/// missing `alg` or an unrecognised algorithm is a spec violation, -/// because those decisions map to specific stage-level check IDs. +/// acceptable. Callers (the JWKS stage) decide whether a missing `alg` +/// or an unrecognised algorithm is a spec violation, because those +/// decisions map to specific stage-level check IDs. /// /// Design note: the JWS layer is deliberately permissive on `alg` /// presence and value. Per RFC 7517 `alg` is OPTIONAL on a JWK, and /// the atproto OAuth profile does not override that, so absence is /// not a parse-time failure here. This keeps the per-key error → -/// stage-level check mapping in exactly one place (Phase 5's +/// stage-level check mapping in exactly one place (the JWKS stage's /// `jwks::run`), which compares each key to the client metadata's /// `token_endpoint_auth_signing_alg` via `has_key_for_signing_alg`. /// /// The case `alg = None` combined with `crv = secp256k1` is valid at -/// the JWS layer (it parses successfully). Phase 5 decides whether -/// this is a downstream concern (e.g., structurally incompatible with -/// a declared `token_endpoint_auth_signing_alg`), not a hard -/// structural failure. +/// the JWS layer (it parses successfully). The JWKS stage decides +/// whether this is a downstream concern (e.g., structurally +/// incompatible with a declared `token_endpoint_auth_signing_alg`), +/// not a hard structural failure. #[derive(Debug, Clone)] pub struct ParsedJwk { pub kid: Option>, - /// Absent if the JWK omits `alg`. Phase 5 falls back to matching - /// `crv` against `token_endpoint_auth_signing_alg` when `alg` is - /// None. + /// Absent if the JWK omits `alg`. The JWKS stage falls back to + /// matching `crv` against `token_endpoint_auth_signing_alg` when + /// `alg` is None. pub alg: Option, /// Raw alg string preserved verbatim when `alg` is present but not - /// in {ES256, ES256K} — Phase 5 uses it to emit a pointed + /// in {ES256, ES256K} — the JWKS stage uses it to emit a pointed /// `algs_are_modern_ec` diagnostic citing the offending value. pub alg_raw: Option>, /// The curve declared on the JWK. Retained so the JWKS stage can @@ -204,7 +204,7 @@ pub enum JwsError { /// Returns a `ParsedJwk` if the object is structurally a JSON object with /// at least a recognised `kty`. Deliberately permissive on `alg` (absent /// or unrecognised) and `use` (any string accepted into `JwkUse::Other`); -/// callers (Phase 5 JWKS stage) decide whether those are violations. +/// callers (the JWKS stage) decide whether those are violations. /// /// Returns `Err(JwsError)` only for hard structural failures: /// - Input is not a JSON object. diff --git a/src/common/oauth/relying_party.rs b/src/common/oauth/relying_party.rs index f83ab64..60bcccb 100644 --- a/src/common/oauth/relying_party.rs +++ b/src/common/oauth/relying_party.rs @@ -649,7 +649,10 @@ impl RelyingParty { params.push(("client_assertion", jwt)); } - // Build DPoP proof (ath claim omitted per Phase 7 plan). + // Build DPoP proof. The `ath` claim is omitted because this is + // the authorization-code → token exchange: there is no access + // token to bind yet (RFC 9449 §4.1 requires `ath` only when the + // proof is presented alongside an access token). let dpop = self.sign_dpop("POST", &as_descriptor.token_endpoint, None)?; let body = serde_urlencoded::to_string(¶ms)?; diff --git a/tests/oauth_client_ac_coverage.rs b/tests/oauth_client_ac_coverage.rs index 97f6d47..7adbebc 100644 --- a/tests/oauth_client_ac_coverage.rs +++ b/tests/oauth_client_ac_coverage.rs @@ -199,7 +199,7 @@ async fn ac5_2_all_static_pass_runs_full_interactive_inventory() { let output = interactive::run(static_gating, None, None, &interactive_opts, clock).await; server.shutdown().await; - // Every Phase 7 + Phase 8 check ID should appear among emitted results. + // Every happy-path and sub-stage check ID should appear among emitted results. use atproto_devtool::commands::test::oauth::client::pipeline::interactive::{ CHECK_ALL as INTERACTIVE_ALL, dpop_edges, scope_variations, }; @@ -238,12 +238,12 @@ async fn ac5_4_non_gating_static_failure_leaves_interactive_inventory_intact() { use atproto_devtool::common::oauth::relying_party::{DeterministicRpFactory, RpFactory}; use atproto_devtool::common::report::CheckStatus; - // Fail `grant_types_includes_refresh_token` — only ClientRefreshed (Phase 7) - // is conditionally-refreshed; AC5.4 says non-gating failures leave every + // Fail `grant_types_includes_refresh_token` — only ClientRefreshed is + // conditionally-refreshed; AC5.4 says non-gating failures leave every // other interactive check running. The gate table in interactive::run // doesn't actually consult refresh_token presence (ClientRefreshed is - // always Skipped in Phase 7), so failing it must not affect any other - // check's execution. + // always Skipped by the happy-path flow), so failing it must not + // affect any other check's execution. let (server, clock) = spawn_fake_as().await; let static_gating = StaticGating { scope_present: CheckStatus::Pass, diff --git a/tests/oauth_client_check_id_coverage.rs b/tests/oauth_client_check_id_coverage.rs index a5f9c2a..9d48d40 100644 --- a/tests/oauth_client_check_id_coverage.rs +++ b/tests/oauth_client_check_id_coverage.rs @@ -120,10 +120,10 @@ fn all_expected_check_ids() -> Vec<(&'static str, &'static str)> { fn all_expected_diagnostic_codes() -> Vec<&'static str> { // Only codes currently exercised by a failing-path snapshot are listed. // Codes that live in source but have no triggering fixture today - // (`public_forbids_jwks`, the three Phase-7 interactive codes that only - // fire on WaitForExternalClient, etc.) are intentionally omitted rather - // than failing this test vacuously — add them when a corresponding - // snapshot lands. + // (`public_forbids_jwks`, the happy-path interactive codes that only + // fire on WaitForExternalClient, etc.) are intentionally omitted + // rather than failing this test vacuously — add them when a + // corresponding snapshot lands. vec![ // Discovery stage diagnostic codes. "oauth_client::discovery::metadata_document_fetchable", diff --git a/tests/oauth_client_interactive.rs b/tests/oauth_client_interactive.rs index 433821c..3c077f5 100644 --- a/tests/oauth_client_interactive.rs +++ b/tests/oauth_client_interactive.rs @@ -276,16 +276,16 @@ async fn interactive_happy_path_gates_all_pass() { let output = interactive::run(static_gating, None, None, &interactive_opts, clock).await; // Verify that all expected checks are present. - // Phase 7: 10 checks (ServerBound, ClientReachedPar, ClientUsedPkceS256, - // ClientIncludedDpop, ClientIncludedState, ClientOmittedNonce, - // ClientVerifiedIss, ClientVerifiedSub, ClientCompletedToken, - // ClientRefreshed). - // Phase 8: 6 scope_variations checks + 6 dpop_edges checks. + // Happy-path: 10 checks (ServerBound, ClientReachedPar, + // ClientUsedPkceS256, ClientIncludedDpop, ClientIncludedState, + // ClientOmittedNonce, ClientVerifiedIss, ClientVerifiedSub, + // ClientCompletedToken, ClientRefreshed). + // Sub-stages: 6 scope_variations checks + 6 dpop_edges checks. // Total: 22 checks. assert_eq!( output.results.len(), 22, - "should have 22 checks (10 phase 7 + 6 scope_variations + 6 dpop_edges)" + "should have 22 checks (10 happy-path + 6 scope_variations + 6 dpop_edges)" ); // Check that ServerBound passed (server bind was successful). @@ -348,7 +348,8 @@ async fn interactive_happy_path_gates_all_pass() { "ClientCompletedToken should pass after successful token exchange" ); - // ClientRefreshed should always be skipped in Phase 7. + // ClientRefreshed is always skipped by the happy-path flow; refresh + // behavior is exercised by the scope_variations / dpop_edges sub-stages. let client_refreshed = output .results .iter() @@ -357,7 +358,7 @@ async fn interactive_happy_path_gates_all_pass() { assert_eq!( client_refreshed.status, CheckStatus::Skipped, - "ClientRefreshed should be skipped in Phase 7" + "ClientRefreshed should be skipped by the happy-path flow" ); // AC4.10: the broken-AS sub-stage drives two extra flows and diff --git a/tests/oauth_client_substage_snapshots.rs b/tests/oauth_client_substage_snapshots.rs index 34df1e0..43bd553 100644 --- a/tests/oauth_client_substage_snapshots.rs +++ b/tests/oauth_client_substage_snapshots.rs @@ -203,10 +203,11 @@ async fn dpop_edges_with_refresh_token_reuse_violation_snapshot() { #[tokio::test] async fn interactive_stage_blocked_by_static_failures_snapshot() { - // Exercises the blocked_by branch of interactive::run: every Phase 7 - // interactive check emits with its stable ID visible as "blocked by " - // or as the diagnostic-code header on a SpecViolation row. The point is - // to put `oauth_client::interactive::*` IDs into at least one snapshot. + // Exercises the blocked_by branch of interactive::run: every + // happy-path interactive check emits with its stable ID visible as + // "blocked by " or as the diagnostic-code header on a + // SpecViolation row. The point is to put + // `oauth_client::interactive::*` IDs into at least one snapshot. use atproto_devtool::commands::test::oauth::client::pipeline::interactive; use atproto_devtool::commands::test::oauth::client::pipeline::{ InteractiveDriveMode, InteractiveOptions, StaticGating, @@ -215,9 +216,9 @@ async fn interactive_stage_blocked_by_static_failures_snapshot() { let (server, clock) = spawn_fake_as().await; - // Simulate a failed scope_present prerequisite — all 5 interactive - // Phase 7 checks emit blocked_by referencing their own IDs in the - // rendered output, plus ServerBound as Skipped. + // Simulate a failed scope_present prerequisite — all 5 happy-path + // interactive checks emit blocked_by referencing their own IDs in + // the rendered output, plus ServerBound as Skipped. let static_gating = StaticGating { scope_present: CheckStatus::SpecViolation, dpop_bound_required: CheckStatus::Pass, diff --git a/tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap b/tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap index e2411bc..61d4f77 100644 --- a/tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap +++ b/tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap @@ -15,6 +15,6 @@ Target: interactive stage blocked-by snapshot [SKIP] Client verified AS `iss` on authorization redirect — blocked by oauth_client::metadata::scope_present [SKIP] Client verified token response `sub` matches expected DID — blocked by oauth_client::metadata::scope_present [SKIP] Client completed token exchange — blocked by oauth_client::metadata::scope_present -[SKIP] Client refreshed access token — covered in Phase 8 flow variants +[SKIP] Client refreshed access token — covered by the scope_variations and dpop_edges sub-stages Summary: 0 passed, 0 failed (spec), 0 network errors, 0 advisories, 10 skipped. Exit code: 0