Fake AS emits spec-required iss/sub; declare deferred verify checks

+2

docs/design-plans/2026-04-16-test-oauth-client.md

··· 115 115 - **test-oauth-client.AC4.6 Failure:** A `--port` value that is already bound, out of range, or otherwise unbindable produces a clear startup error and a non-zero exit before the synthetic identity is printed. 116 116 - **test-oauth-client.AC4.7 Success:** The PAR request body includes a `state` parameter, and the `ClientIncludedState` check passes. The atproto OAuth profile (<https://atproto.com/specs/oauth#authorization-request-fields>) states: *"The `state` parameter in client authorization requests is mandatory."* A PAR body without `state=…` fails this check. 117 117 - **test-oauth-client.AC4.8 Success:** The PAR request body does NOT include a `nonce` parameter, and the `ClientOmittedNonce` check passes. The atproto OAuth profile calls this out explicitly: *"The `nonce` value, used in many other OAuth profiles, should not be included."* A PAR body that carries `nonce=…` fails this check. 118 + - **test-oauth-client.AC4.9 Success (fake AS compliance):** The fake AS's authorization redirect always includes `iss=<active_base>` (atproto's `authorization_response_iss_parameter_supported = true` contract), and its token response always includes `sub=<account DID>`. A client library pointed at the fake AS can therefore perform the mandatory issuer and subject verifications against real values. 119 + - **test-oauth-client.AC4.10 Deferred (client-side `iss`/`sub` verification):** The `ClientVerifiedIss` and `ClientVerifiedSub` check variants are declared in `CHECK_ALL` and rendered in every report, but emit `Skipped` with reason `"broken-AS sub-stage for client-side iss/sub verification not yet implemented"`. Fully exercising these requires a broken-AS sub-stage (similar in shape to `dpop_edges`) that emits a bogus `iss` on a redirect or a wrong `sub` in a token response and observes whether the client terminates the flow (`iss`) or discards the token (`sub`). The atproto OAuth profile calls this verification *"critical (mandatory)"* for all clients (<https://atproto.com/specs/oauth#authentication-and-subject>); closing this gap is a future phase. 118 120 119 121 ### test-oauth-client.AC5: Cross-mode dependency gating 120 122

+15 -2

src/commands/test/oauth/client/fake_as/endpoints.rs

··· 595 595 codes.insert(code.clone(), code_binding); 596 596 drop(codes); 597 597 598 + // Atproto OAuth requires the authorization response to 599 + // include `iss` so the client can verify the AS matches the 600 + // one it resolved for the account. See 601 + // <https://atproto.com/specs/oauth#authorization-response>. 602 + let iss = s.active_base.as_str().trim_end_matches('/').to_string(); 598 603 redirect_url 599 604 .query_pairs_mut() 600 605 .append_pair("code", &code) 601 - .append_pair("state", &par_request.state); 606 + .append_pair("state", &par_request.state) 607 + .append_pair("iss", &iss); 602 608 } else { 609 + let iss = s.active_base.as_str().trim_end_matches('/').to_string(); 603 610 redirect_url 604 611 .query_pairs_mut() 605 612 .append_pair("error", "access_denied") 606 - .append_pair("state", &par_request.state); 613 + .append_pair("state", &par_request.state) 614 + .append_pair("iss", &iss); 607 615 } 608 616 609 617 Redirect::permanent(redirect_url.as_str()).into_response() ··· 771 779 } 772 780 773 781 let scope = code_binding.granted_scope; 782 + // Atproto OAuth requires the token response to include `sub` 783 + // so the client can verify the authenticated account DID. 784 + // See <https://atproto.com/specs/oauth#authentication-and-subject>. 774 785 let resp = json!({ 775 786 "access_token": access_token, 776 787 "token_type": "DPoP", 777 788 "expires_in": 3600, 778 789 "refresh_token": new_refresh_token, 779 790 "scope": scope, 791 + "sub": s.identity.did.to_string(), 780 792 }); 781 793 782 794 // Store refresh token binding. ··· 832 844 "expires_in": 3600, 833 845 "refresh_token": new_refresh_token, 834 846 "scope": scope, 847 + "sub": s.identity.did.to_string(), 835 848 }); 836 849 837 850 // Store new refresh token binding.

+62

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

··· 102 102 /// A client that carries an OIDC-style `nonce` into an atproto 103 103 /// authorization request is non-conformant. 104 104 ClientOmittedNonce, 105 + /// The client verifies the `iss` parameter on the authorization 106 + /// redirect against the expected Authorization Server issuer. 107 + /// The atproto OAuth profile mandates 108 + /// `authorization_response_iss_parameter_supported = true` on AS 109 + /// metadata for exactly this reason: clients must confirm the 110 + /// redirect they received came from the AS they initiated the 111 + /// flow with (a protection against AS-mixup attacks per 112 + /// <https://datatracker.ietf.org/doc/html/rfc9207>). 113 + /// 114 + /// Fully exercising this check requires a broken-AS sub-stage 115 + /// that emits a bogus `iss` and observes whether the client 116 + /// terminates the flow before hitting `/oauth/token`. That 117 + /// infrastructure is not yet in place; for now the check emits 118 + /// `Skipped` with a reason pointing to the deferred 119 + /// implementation. 120 + ClientVerifiedIss, 121 + /// The client verifies the `sub` field of the token response 122 + /// matches the account DID it initiated the flow for. The 123 + /// atproto OAuth profile states: *"At the end of the auth flow, 124 + /// when the client does an initial token fetch, the Authorization 125 + /// Server must return the account DID in the `sub` field … it is 126 + /// critical (mandatory) for all clients to verify that the 127 + /// account identified by the AS is the one they expected."* 128 + /// (<https://atproto.com/specs/oauth#authentication-and-subject>) 129 + /// 130 + /// Fully exercising this check requires a broken-AS sub-stage 131 + /// that emits a wrong `sub` and observes whether the client 132 + /// discards the token. That infrastructure is not yet in place; 133 + /// for now the check emits `Skipped` with a reason pointing to 134 + /// the deferred implementation. 135 + ClientVerifiedSub, 105 136 /// The client completed the authorization-code ↔ token exchange 106 137 /// against `/oauth/token` and received an access token. Mirrors 107 138 /// RFC 6749 §4.1.3 ··· 129 160 Check::ClientIncludedDpop => "oauth_client::interactive::client_included_dpop", 130 161 Check::ClientIncludedState => "oauth_client::interactive::client_included_state", 131 162 Check::ClientOmittedNonce => "oauth_client::interactive::client_omitted_nonce", 163 + Check::ClientVerifiedIss => "oauth_client::interactive::client_verified_iss", 164 + Check::ClientVerifiedSub => "oauth_client::interactive::client_verified_sub", 132 165 Check::ClientCompletedToken => "oauth_client::interactive::client_completed_token", 133 166 Check::ClientRefreshed => "oauth_client::interactive::client_refreshed", 134 167 } ··· 143 176 Check::ClientIncludedDpop => "Client included DPoP proof", 144 177 Check::ClientIncludedState => "Client included `state` parameter in PAR", 145 178 Check::ClientOmittedNonce => "Client did not include `nonce` parameter in PAR", 179 + Check::ClientVerifiedIss => "Client verified AS `iss` on authorization redirect", 180 + Check::ClientVerifiedSub => "Client verified token response `sub` matches expected DID", 146 181 Check::ClientCompletedToken => "Client completed token exchange", 147 182 Check::ClientRefreshed => "Client refreshed access token", 148 183 } ··· 189 224 } 190 225 } 191 226 227 + /// Reason emitted on `ClientVerifiedIss` / `ClientVerifiedSub`'s 228 + /// `Skipped` rows until broken-AS sub-stages land. 229 + const CLIENT_VERIFICATION_SKIP_REASON: &str = 230 + "broken-AS sub-stage for client-side `iss`/`sub` verification not yet implemented"; 231 + 192 232 /// Returns true if the form-encoded `par_body` contains a `key=` pair. 193 233 /// Used to detect `state=` / `nonce=` without requiring a full 194 234 /// form-decode (we only care about key presence). Handles both ··· 207 247 Check::ClientIncludedDpop, 208 248 Check::ClientIncludedState, 209 249 Check::ClientOmittedNonce, 250 + Check::ClientVerifiedIss, 251 + Check::ClientVerifiedSub, 210 252 Check::ClientCompletedToken, 211 253 Check::ClientRefreshed, 212 254 ]; ··· 243 285 Check::ClientIncludedDpop, 244 286 Check::ClientIncludedState, 245 287 Check::ClientOmittedNonce, 288 + Check::ClientVerifiedIss, 289 + Check::ClientVerifiedSub, 246 290 Check::ClientCompletedToken, 247 291 ] { 248 292 results.push(blocked_by( ··· 272 316 Check::ClientIncludedDpop, 273 317 Check::ClientIncludedState, 274 318 Check::ClientOmittedNonce, 319 + Check::ClientVerifiedIss, 320 + Check::ClientVerifiedSub, 275 321 Check::ClientCompletedToken, 276 322 ] { 277 323 results.push(blocked_by( ··· 355 401 results.push(Check::ClientIncludedDpop.spec_violation()); 356 402 results.push(Check::ClientIncludedState.spec_violation()); 357 403 results.push(Check::ClientOmittedNonce.spec_violation()); 404 + results.push(Check::ClientVerifiedIss.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 405 + results.push(Check::ClientVerifiedSub.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 358 406 results.push(Check::ClientCompletedToken.spec_violation()); 359 407 results.push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); 360 408 } ··· 377 425 results.push(Check::ClientIncludedDpop.spec_violation()); 378 426 results.push(Check::ClientIncludedState.spec_violation()); 379 427 results.push(Check::ClientOmittedNonce.spec_violation()); 428 + results.push(Check::ClientVerifiedIss.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 429 + results.push(Check::ClientVerifiedSub.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 380 430 results.push(Check::ClientCompletedToken.spec_violation()); 381 431 results 382 432 .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); ··· 406 456 results.push(Check::ClientIncludedDpop.spec_violation()); 407 457 results.push(Check::ClientIncludedState.spec_violation()); 408 458 results.push(Check::ClientOmittedNonce.spec_violation()); 459 + results.push(Check::ClientVerifiedIss.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 460 + results.push(Check::ClientVerifiedSub.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 409 461 results.push(Check::ClientCompletedToken.spec_violation()); 410 462 results 411 463 .push(Check::ClientRefreshed.skipped("covered in Phase 8 flow variants")); ··· 474 526 } else { 475 527 results.push(Check::ClientOmittedNonce.pass()); 476 528 } 529 + 530 + // ClientVerifiedIss / ClientVerifiedSub require a 531 + // broken-AS sub-stage that emits a bogus `iss` or 532 + // `sub` and observes whether the client aborts the 533 + // flow. That infrastructure isn't in place yet, so 534 + // emit `Skipped` with a reason for now. 535 + results.push(Check::ClientVerifiedIss.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 536 + results.push(Check::ClientVerifiedSub.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 477 537 } else { 478 538 results.push(Check::ClientReachedPar.spec_violation()); 479 539 results.push(Check::ClientUsedPkceS256.spec_violation()); 480 540 results.push(Check::ClientIncludedDpop.spec_violation()); 481 541 results.push(Check::ClientIncludedState.spec_violation()); 482 542 results.push(Check::ClientOmittedNonce.spec_violation()); 543 + results.push(Check::ClientVerifiedIss.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 544 + results.push(Check::ClientVerifiedSub.skipped(CLIENT_VERIFICATION_SKIP_REASON)); 483 545 } 484 546 485 547 // Perform authorize.

+7 -4

tests/oauth_client_interactive.rs

··· 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 - // Phase 7: 8 checks (ServerBound, ClientReachedPar, ClientUsedPkceS256, ClientIncludedDpop, ClientIncludedState, ClientOmittedNonce, ClientCompletedToken, ClientRefreshed). 279 + // Phase 7: 10 checks (ServerBound, ClientReachedPar, ClientUsedPkceS256, 280 + // ClientIncludedDpop, ClientIncludedState, ClientOmittedNonce, 281 + // ClientVerifiedIss, ClientVerifiedSub, ClientCompletedToken, 282 + // ClientRefreshed). 280 283 // Phase 8: 6 scope_variations checks + 6 dpop_edges checks. 281 - // Total: 20 checks. 284 + // Total: 22 checks. 282 285 assert_eq!( 283 286 output.results.len(), 284 - 20, 285 - "should have 20 checks (8 phase 7 + 6 scope_variations + 6 dpop_edges)" 287 + 22, 288 + "should have 22 checks (10 phase 7 + 6 scope_variations + 6 dpop_edges)" 286 289 ); 287 290 288 291 // Check that ServerBound passed (server bind was successful).

+3 -1

tests/snapshots/oauth_client_substage_snapshots__interactive_stage_blocked_by_static_failures_snapshot.snap

··· 12 12 [SKIP] Client included DPoP proof — blocked by oauth_client::metadata::scope_present 13 13 [SKIP] Client included `state` parameter in PAR — blocked by oauth_client::metadata::scope_present 14 14 [SKIP] Client did not include `nonce` parameter in PAR — blocked by oauth_client::metadata::scope_present 15 + [SKIP] Client verified AS `iss` on authorization redirect — blocked by oauth_client::metadata::scope_present 16 + [SKIP] Client verified token response `sub` matches expected DID — blocked by oauth_client::metadata::scope_present 15 17 [SKIP] Client completed token exchange — blocked by oauth_client::metadata::scope_present 16 18 [SKIP] Client refreshed access token — covered in Phase 8 flow variants 17 19 18 - Summary: 0 passed, 0 failed (spec), 0 network errors, 0 advisories, 8 skipped. Exit code: 0 20 + Summary: 0 passed, 0 failed (spec), 0 network errors, 0 advisories, 10 skipped. Exit code: 0

Configure Feed

Configure Feed