fix(oauth-client): Phase 4 cycle 3 — address remaining code review issues

+66 -66

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

··· 493 493 ); 494 494 let diagnostic: Box<dyn miette::Diagnostic + Send + Sync> = 495 495 Box::new(RawDocumentDeserializationError { 496 - source: crate::common::diagnostics::named_source_from_bytes( 496 + src: crate::common::diagnostics::named_source_from_bytes( 497 497 raw_source_name, 498 498 &pretty_body, 499 499 ), ··· 523 523 524 524 // Pretty-print JSON once for all diagnostics. 525 525 let pretty_body = crate::common::diagnostics::pretty_json_for_display(bytes); 526 + let source = crate::common::diagnostics::named_source_from_bytes( 527 + raw_source_name, 528 + &pretty_body, 529 + ); 526 530 527 531 // Parse client_id. 528 532 let parsed_client_id = match &doc.client_id { ··· 748 752 // RedirectUrisShape depends on successful kind determination. 749 753 if let Some(ref kind) = kind { 750 754 // Validate redirect URIs shape. 751 - let redirect_uris_shape_valid = 752 - if let Some(parsed_id) = &parsed_client_id { 753 - validate_redirect_uris(&doc.redirect_uris, kind, parsed_id) 754 - } else { 755 - false 756 - }; 757 - if redirect_uris_shape_valid { 758 - results.push(Check::RedirectUrisShape.pass()); 755 + let validation_result = if let Some(parsed_id) = &parsed_client_id { 756 + validate_redirect_uris(&doc.redirect_uris, kind, parsed_id) 759 757 } else { 760 - let diagnostic: Box<dyn miette::Diagnostic + Send + Sync> = 761 - Box::new(MetadataViolationDiagnostic { 762 - message: match kind { 763 - ClientKind::WebConfidential | ClientKind::WebPublic => { 764 - "web client redirect_uris must be HTTPS with origin matching client_id".to_string() 765 - } 766 - ClientKind::Native => { 767 - "native client redirect_uris must be HTTPS with origin matching, or use custom scheme matching reverse-domain of client_id host".to_string() 768 - } 769 - ClientKind::Loopback => unreachable!(), 770 - }, 771 - code: "oauth_client::metadata::redirect_uris_shape", 772 - }); 773 - results.push( 774 - Check::RedirectUrisShape.spec_violation(Some(diagnostic)), 775 - ); 758 + Err(RedirectValidationFailure::Other) 759 + }; 760 + match validation_result { 761 + Ok(()) => { 762 + results.push(Check::RedirectUrisShape.pass()); 763 + } 764 + Err(RedirectValidationFailure::ReverseDomainMismatch) => { 765 + let diagnostic: Box<dyn miette::Diagnostic + Send + Sync> = 766 + Box::new(RedirectSchemeReverseDomainMismatchDiagnostic { 767 + src: source.clone(), 768 + span: None, 769 + }); 770 + results.push( 771 + Check::RedirectUrisShape.spec_violation(Some(diagnostic)), 772 + ); 773 + } 774 + Err(RedirectValidationFailure::Other) => { 775 + let diagnostic: Box<dyn miette::Diagnostic + Send + Sync> = 776 + Box::new(MetadataViolationDiagnostic { 777 + message: match kind { 778 + ClientKind::WebConfidential | ClientKind::WebPublic => { 779 + "web client redirect_uris must be HTTPS with origin matching client_id".to_string() 780 + } 781 + ClientKind::Native => { 782 + "native client redirect_uris must be HTTPS with origin matching, or use custom scheme matching reverse-domain of client_id host".to_string() 783 + } 784 + ClientKind::Loopback => unreachable!(), 785 + }, 786 + code: "oauth_client::metadata::redirect_uris_shape", 787 + }); 788 + results.push( 789 + Check::RedirectUrisShape.spec_violation(Some(diagnostic)), 790 + ); 791 + } 776 792 } 777 793 } else { 778 794 let diagnostic: Box<dyn miette::Diagnostic + Send + Sync> = ··· 1007 1023 } 1008 1024 } 1009 1025 1026 + /// Outcome of redirect URI validation for a native client. 1027 + enum RedirectValidationFailure { 1028 + /// Reverse-domain mismatch for a custom-scheme redirect URI. 1029 + ReverseDomainMismatch, 1030 + /// Some other validation failure. 1031 + Other, 1032 + } 1033 + 1010 1034 /// Validate redirect URIs against client kind and client_id. 1035 + /// Returns Ok if all URIs are valid, or Err with the specific failure type. 1011 1036 fn validate_redirect_uris( 1012 1037 redirect_uris: &Option<Vec<String>>, 1013 1038 kind: &ClientKind, 1014 1039 client_id: &Url, 1015 - ) -> bool { 1040 + ) -> Result<(), RedirectValidationFailure> { 1016 1041 let Some(uris) = redirect_uris else { 1017 - return false; 1042 + return Err(RedirectValidationFailure::Other); 1018 1043 }; 1019 1044 1020 1045 for uri_str in uris { ··· 1024 1049 match Url::parse(uri_str) { 1025 1050 Ok(uri) => { 1026 1051 if uri.scheme() != "https" { 1027 - return false; 1052 + return Err(RedirectValidationFailure::Other); 1028 1053 } 1029 1054 // Check origin (scheme + host + port) matches. 1030 1055 if uri.origin() != client_id.origin() { 1031 - return false; 1056 + return Err(RedirectValidationFailure::Other); 1032 1057 } 1033 1058 } 1034 - Err(_) => return false, 1059 + Err(_) => return Err(RedirectValidationFailure::Other), 1035 1060 } 1036 1061 } 1037 1062 ClientKind::Native => { ··· 1042 1067 if uri.scheme() == "https" { 1043 1068 // HTTPS: origin must match. 1044 1069 if uri.origin() != client_id.origin() { 1045 - return false; 1070 + return Err(RedirectValidationFailure::Other); 1046 1071 } 1047 1072 } else if uri.scheme() == "http" { 1048 1073 // HTTP is not allowed for native clients. 1049 - return false; 1074 + return Err(RedirectValidationFailure::Other); 1050 1075 } else { 1051 1076 // Custom scheme: must match reverse-domain of client_id host. 1052 1077 let expected_scheme = reverse_domain_from_host(client_id.host_str()); 1053 1078 if uri.scheme() != expected_scheme { 1054 - return false; 1079 + return Err(RedirectValidationFailure::ReverseDomainMismatch); 1055 1080 } 1056 1081 } 1057 1082 } 1058 - Err(_) => return false, 1083 + Err(_) => return Err(RedirectValidationFailure::Other), 1059 1084 } 1060 1085 } 1061 1086 ClientKind::Loopback => { ··· 1064 1089 } 1065 1090 } 1066 1091 1067 - true 1092 + Ok(()) 1068 1093 } 1069 1094 1070 1095 /// Compute the reverse-domain form of a hostname. ··· 1085 1110 } 1086 1111 1087 1112 /// Error from metadata document deserialization. 1088 - #[derive(Debug)] 1113 + #[derive(Debug, Error, Diagnostic)] 1114 + #[error("{message}")] 1115 + #[diagnostic(code = "oauth_client::metadata::raw_document_deserializes")] 1089 1116 struct RawDocumentDeserializationError { 1090 - source: NamedSource<Arc<[u8]>>, 1117 + #[source_code] 1118 + src: NamedSource<Arc<[u8]>>, 1119 + #[label("invalid JSON")] 1091 1120 span: SourceSpan, 1092 1121 message: String, 1093 - } 1094 - 1095 - impl std::fmt::Display for RawDocumentDeserializationError { 1096 - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { 1097 - write!(f, "{}", self.message) 1098 - } 1099 - } 1100 - 1101 - impl std::error::Error for RawDocumentDeserializationError {} 1102 - 1103 - impl miette::Diagnostic for RawDocumentDeserializationError { 1104 - fn code<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> { 1105 - Some(Box::new( 1106 - "oauth_client::metadata::raw_document_deserializes", 1107 - )) 1108 - } 1109 - 1110 - fn source_code(&self) -> Option<&(dyn miette::SourceCode + 'static)> { 1111 - Some(&self.source) 1112 - } 1113 - 1114 - fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> { 1115 - Some(Box::new(std::iter::once(miette::LabeledSpan::new( 1116 - Some("invalid JSON".to_string()), 1117 - self.span.offset(), 1118 - self.span.len(), 1119 - )))) 1120 - } 1121 1122 } 1122 1123 1123 1124 /// Client ID is invalid (not a valid URL). ··· 1166 1167 #[derive(Debug, Error, Diagnostic)] 1167 1168 #[error("native redirect_uri scheme does not match reverse-domain of client_id host")] 1168 1169 #[diagnostic(code = "oauth_client::metadata::redirect_scheme_reverse_domain_mismatch")] 1169 - #[expect(dead_code)] 1170 1170 struct RedirectSchemeReverseDomainMismatchDiagnostic { 1171 1171 #[source_code] 1172 1172 src: NamedSource<Arc<[u8]>>,

+2 -1

tests/snapshots/oauth_client_discovery__https_confidential_happy_discovery.snap

··· 1 1 --- 2 2 source: tests/oauth_client_discovery.rs 3 + assertion_line: 42 3 4 expression: rendered 4 5 --- 5 6 Target: https://client.example.com/metadata.json ··· 22 23 [OK] `token_endpoint_auth_method` matches client kind 23 24 [OK] Confidential client provides exactly one of `jwks`/`jwks_uri` 24 25 [OK] `scope` field is present 25 - [OK] `scope` parses against the atproto permission grammar 26 26 [OK] `scope` includes the `atproto` token 27 + [OK] `scope` parses against the atproto permission grammar 27 28 28 29 Summary: 17 passed, 0 failed (spec), 0 network errors, 0 advisories, 0 skipped. Exit code: 0

+3 -2

tests/snapshots/oauth_client_metadata__native_redirect_scheme_mismatch.snap

··· 1 1 --- 2 2 source: tests/oauth_client_metadata.rs 3 + assertion_line: 214 3 4 expression: rendered 4 5 --- 5 6 Target: https://app.example.com/oauth-client-metadata.json ··· 19 20 [OK] `dpop_bound_access_tokens` is `true` 20 21 [OK] `redirect_uris` is non-empty 21 22 [FAIL] Every `redirect_uri` has the right shape for the client kind 22 - oauth_client::metadata::redirect_uris_shape 23 + oauth_client::metadata::redirect_scheme_reverse_domain_mismatch 23 24 24 - × native client redirect_uris must be HTTPS with origin matching, or use custom scheme matching reverse-domain of client_id host 25 + × native redirect_uri scheme does not match reverse-domain of client_id host 25 26 [OK] `token_endpoint_auth_method` matches client kind 26 27 [OK] Public/native client does not provide `jwks` or `jwks_uri` 27 28 [OK] `scope` field is present

Configure Feed

Configure Feed