diff --git a/Cargo.lock b/Cargo.lock index 427b18f..a0284ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -176,6 +176,7 @@ dependencies = [ "multibase", "p256", "percent-encoding", + "proptest", "rand 0.8.6", "rand_chacha 0.3.1", "rand_core 0.6.4", @@ -386,6 +387,21 @@ version = "1.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2af50177e190e07a26ab74f8b1efbfe2ef87da2116221318cb1c2e82baf7de06" +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitflags" version = "2.11.0" @@ -1033,6 +1049,12 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "fnv" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" + [[package]] name = "foldhash" version = "0.1.5" @@ -2209,6 +2231,31 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags", + "num-traits", + "rand 0.9.4", + "rand_chacha 0.9.0", + "rand_xorshift", + "regex-syntax", + "rusty-fork", + "tempfile", + "unarray", +] + +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quinn" version = "0.11.9" @@ -2345,6 +2392,15 @@ dependencies = [ "getrandom 0.3.4", ] +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core 0.9.5", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -2617,6 +2673,18 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "rusty-fork" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc6bf79ff24e648f6da1f8d1f011e9cac26491b619e6b9280f2b47f1774e6ee2" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.23" @@ -3304,6 +3372,12 @@ version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "562d481066bde0658276a35467c4af00bdc6ee726305698a55b86e61d7ad82bb" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicode-ident" version = "1.0.24" diff --git a/Cargo.toml b/Cargo.toml index 4ed5bd4..fc68630 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,12 @@ url = "2.5" [dev-dependencies] assert_cmd = "2.0" insta = "1.47" +# Property-based tests pin roundtrip / idempotence properties for the +# cryptographic primitives (multikey encode/decode, JWT encode/verify, +# JWS sign/verify, signature byte-length invariants). 64 cases per +# property is enough to exercise every byte-pattern edge case without +# making the test suite slow. +proptest = "1.5" rand = "0.8" rand_chacha = "0.3" tokio = { version = "1.51", features = ["rt", "macros", "test-util", "time"] } diff --git a/src/commands/test/oauth/client/pipeline/discovery.rs b/src/commands/test/oauth/client/pipeline/discovery.rs index b23d5f1..eb35ca4 100644 --- a/src/commands/test/oauth/client/pipeline/discovery.rs +++ b/src/commands/test/oauth/client/pipeline/discovery.rs @@ -190,174 +190,188 @@ pub struct DiscoveryStageOutput { pub results: Vec, } +/// Outcome of an HTTPS metadata fetch, in a shape suitable for the pure +/// `evaluate_https_metadata_response` evaluator below. +#[derive(Debug)] +pub(super) enum HttpsFetchOutcome { + /// The fetch failed at the transport layer. + NetworkFailure(crate::common::identity::IdentityError), + /// The fetch returned an HTTP response (2xx or otherwise). + Response { + status: u16, + body: Vec, + content_type: Option, + }, +} + /// Run the discovery stage for the given target. +/// +/// This is the imperative shell: it does the network fetch and dispatches +/// to the pure `evaluate_*` helpers below to compute every check result. pub async fn run(target: &OauthClientTarget, http: &dyn HttpClient) -> DiscoveryStageOutput { - let mut results = Vec::new(); - match target { OauthClientTarget::HttpsUrl(url) => { - // Check that the client_id is well-formed (it's already validated by parse_target, - // but we still emit the check). - results.push(Check::ClientIdWellFormed.pass()); - - // Fetch the metadata document. - match http.get_bytes_with_content_type(url).await { - Err(e) => { - // Network error on fetch. - let diagnostic: Box = - Box::new(FetchError::from_identity_error(e, url)); - results.push(Check::MetadataDocumentFetchable.network_error(Some(diagnostic))); - for check in [Check::MetadataContentTypeIsJson, Check::MetadataIsJson] { - results.push(blocked_by( - check.id(), - Stage::OAUTH_CLIENT_DISCOVERY, - check.pass_summary(), - Check::MetadataDocumentFetchable.id(), - )); - } - DiscoveryStageOutput { - facts: None, - results, - } - } - Ok((status, _, _)) if !(200..=299).contains(&status) => { - // Non-2xx status: transport-layer failure. - let diagnostic: Box = Box::new(HttpStatusError { + let outcome = match http.get_bytes_with_content_type(url).await { + Err(e) => HttpsFetchOutcome::NetworkFailure(e), + Ok((status, body, content_type)) => HttpsFetchOutcome::Response { + status, + body, + content_type, + }, + }; + evaluate_https_metadata_response(url, outcome) + } + OauthClientTarget::Loopback(_) => evaluate_loopback_metadata(), + } +} + +/// Pure: evaluate an HTTPS metadata fetch outcome and emit all four +/// discovery-stage check results plus the discovery facts (when the +/// fetch produced parseable JSON). +pub(super) fn evaluate_https_metadata_response( + url: &Url, + fetch_outcome: HttpsFetchOutcome, +) -> DiscoveryStageOutput { + let mut results = Vec::new(); + // `client_id` validity is already enforced by `parse_target`; emit the + // pass row so the report still shows the check. + results.push(Check::ClientIdWellFormed.pass()); + + match fetch_outcome { + HttpsFetchOutcome::NetworkFailure(e) => { + let diagnostic: Box = + Box::new(FetchError::from_identity_error(e, url)); + results.push(Check::MetadataDocumentFetchable.network_error(Some(diagnostic))); + push_blocked_by_fetchable(&mut results); + DiscoveryStageOutput { + facts: None, + results, + } + } + HttpsFetchOutcome::Response { + status, + body, + content_type, + } => { + if !(200..=299).contains(&status) { + // Non-2xx status: transport-layer failure. + let diagnostic: Box = Box::new(HttpStatusError { + url: url.clone(), + status, + }); + results.push(Check::MetadataDocumentFetchable.network_error(Some(diagnostic))); + push_blocked_by_fetchable(&mut results); + return DiscoveryStageOutput { + facts: None, + results, + }; + } + if status != 200 { + // 2xx but not 200: spec violation per + // + // ("must be 200 (not another 2xx or a redirect)"). + let diagnostic: Box = Box::new(NonOkStatusError { + url: url.clone(), + status, + }); + results.push(Check::MetadataDocumentFetchable.spec_violation(Some(diagnostic))); + push_blocked_by_fetchable(&mut results); + return DiscoveryStageOutput { + facts: None, + results, + }; + } + results.push(Check::MetadataDocumentFetchable.pass()); + + // Content-Type check: the atproto OAuth profile requires the + // metadata response carry the correct JSON content type. + // Accept `application/json` with optional parameters like + // `charset=utf-8`. + if content_type_is_json(content_type.as_deref()) { + results.push(Check::MetadataContentTypeIsJson.pass()); + } else { + let diagnostic: Box = + Box::new(NonJsonContentTypeError { url: url.clone(), - status, + content_type: content_type.clone(), }); - results.push(Check::MetadataDocumentFetchable.network_error(Some(diagnostic))); - for check in [Check::MetadataContentTypeIsJson, Check::MetadataIsJson] { - results.push(blocked_by( - check.id(), - Stage::OAUTH_CLIENT_DISCOVERY, - check.pass_summary(), - Check::MetadataDocumentFetchable.id(), - )); - } + results.push(Check::MetadataContentTypeIsJson.spec_violation(Some(diagnostic))); + } + + // Try to parse as JSON. + match serde_json::from_slice::(&body) { + Err(json_err) => { + let pretty_body = crate::common::diagnostics::pretty_json_for_display(&body); + let span = + span_at_line_column(&pretty_body, json_err.line(), json_err.column()); + let ct = content_type.as_deref().unwrap_or(""); + let diagnostic: Box = Box::new(JsonParseError { + source: named_source_from_bytes( + format!("metadata document (content-type: {ct})"), + pretty_body, + ), + span, + message: format!("response body is not valid JSON (content-type: {ct})"), + }); + results.push(Check::MetadataIsJson.spec_violation(Some(diagnostic))); DiscoveryStageOutput { facts: None, results, } } - Ok((status, _, _)) if status != 200 => { - // 2xx but not 200: spec violation per - // - // ("must be 200 (not another 2xx or a redirect)"). - let diagnostic: Box = - Box::new(NonOkStatusError { - url: url.clone(), - status, - }); - results.push(Check::MetadataDocumentFetchable.spec_violation(Some(diagnostic))); - for check in [Check::MetadataContentTypeIsJson, Check::MetadataIsJson] { - results.push(blocked_by( - check.id(), - Stage::OAUTH_CLIENT_DISCOVERY, - check.pass_summary(), - Check::MetadataDocumentFetchable.id(), - )); - } + Ok(_) => { + results.push(Check::MetadataIsJson.pass()); DiscoveryStageOutput { - facts: None, + facts: Some(DiscoveryFacts { + client_id: url.clone(), + kind: ClientIdKind::HttpsUrl, + raw_metadata: RawMetadata::Document { + bytes: Arc::from(body), + content_type, + }, + }), results, } } - Ok((_status, body, content_type)) => { - results.push(Check::MetadataDocumentFetchable.pass()); - - // Content-Type check: the atproto OAuth profile - // requires the metadata response carry the correct - // JSON content type. Accept `application/json` - // with optional parameters like `charset=utf-8`. - if content_type_is_json(content_type.as_deref()) { - results.push(Check::MetadataContentTypeIsJson.pass()); - } else { - let diagnostic: Box = - Box::new(NonJsonContentTypeError { - url: url.clone(), - content_type: content_type.clone(), - }); - results.push( - Check::MetadataContentTypeIsJson.spec_violation(Some(diagnostic)), - ); - } - - // Try to parse as JSON. - match serde_json::from_slice::(&body) { - Err(json_err) => { - // Parse error. - let pretty_body = - crate::common::diagnostics::pretty_json_for_display(&body); - let span = span_at_line_column( - &pretty_body, - json_err.line(), - json_err.column(), - ); - let ct = content_type.as_deref().unwrap_or(""); - let diagnostic: Box = - Box::new(JsonParseError { - source: named_source_from_bytes( - format!("metadata document (content-type: {ct})"), - pretty_body, - ), - span, - message: format!( - "response body is not valid JSON (content-type: {ct})" - ), - }); - results.push(Check::MetadataIsJson.spec_violation(Some(diagnostic))); - DiscoveryStageOutput { - facts: None, - results, - } - } - Ok(_) => { - // Valid JSON. - results.push(Check::MetadataIsJson.pass()); - DiscoveryStageOutput { - facts: Some(DiscoveryFacts { - client_id: url.clone(), - kind: ClientIdKind::HttpsUrl, - raw_metadata: RawMetadata::Document { - bytes: Arc::from(body), - content_type, - }, - }), - results, - } - } - } - } } } - OauthClientTarget::Loopback(_) => { - // Loopback targets have well-formed client_id by definition. - results.push(Check::ClientIdWellFormed.pass()); - - // Metadata is implicit for loopback clients. - for check in [ - Check::MetadataDocumentFetchable, - Check::MetadataContentTypeIsJson, - Check::MetadataIsJson, - ] { - results.push(check.skipped("metadata is implicit for loopback clients")); - } + } +} - // The atproto loopback client_id is fixed at - // `http://localhost/`. - let client_id = Url::parse("http://localhost/") - .expect("`http://localhost/` is a statically known-good URL"); +/// Pure: emit the discovery-stage results for a loopback client. Loopback +/// targets have no metadata document; the well-formedness check passes +/// by definition and the three document-related checks are skipped. +pub(super) fn evaluate_loopback_metadata() -> DiscoveryStageOutput { + let mut results = vec![Check::ClientIdWellFormed.pass()]; + for check in [ + Check::MetadataDocumentFetchable, + Check::MetadataContentTypeIsJson, + Check::MetadataIsJson, + ] { + results.push(check.skipped("metadata is implicit for loopback clients")); + } + let client_id = Url::parse("http://localhost/") + .expect("`http://localhost/` is a statically known-good URL"); + DiscoveryStageOutput { + facts: Some(DiscoveryFacts { + client_id: client_id.clone(), + kind: ClientIdKind::Loopback, + raw_metadata: RawMetadata::Implicit { client_id }, + }), + results, + } +} - DiscoveryStageOutput { - facts: Some(DiscoveryFacts { - client_id: client_id.clone(), - kind: ClientIdKind::Loopback, - raw_metadata: RawMetadata::Implicit { client_id }, - }), - results, - } - } +/// Append the two `blocked_by` skip rows that follow a failed +/// `MetadataDocumentFetchable` check. +fn push_blocked_by_fetchable(results: &mut Vec) { + for check in [Check::MetadataContentTypeIsJson, Check::MetadataIsJson] { + results.push(blocked_by( + check.id(), + Stage::OAUTH_CLIENT_DISCOVERY, + check.pass_summary(), + Check::MetadataDocumentFetchable.id(), + )); } } @@ -508,3 +522,179 @@ fn content_type_is_json(content_type: Option<&str>) -> bool { let media_type = ct.split(';').next().unwrap_or("").trim(); media_type.eq_ignore_ascii_case("application/json") } + +#[cfg(test)] +mod tests { + use super::*; + use crate::common::identity::IdentityError; + use crate::common::report::CheckStatus; + + fn https_url() -> Url { + Url::parse("https://example.com/client-metadata.json").unwrap() + } + + fn check_ids(output: &DiscoveryStageOutput) -> Vec<&'static str> { + output.results.iter().map(|r| r.id).collect() + } + + fn statuses(output: &DiscoveryStageOutput) -> Vec { + output.results.iter().map(|r| r.status).collect() + } + + #[test] + fn loopback_emits_one_pass_and_three_skips_with_facts() { + let output = evaluate_loopback_metadata(); + assert_eq!( + check_ids(&output), + vec![ + Check::ClientIdWellFormed.id(), + Check::MetadataDocumentFetchable.id(), + Check::MetadataContentTypeIsJson.id(), + Check::MetadataIsJson.id(), + ] + ); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::Skipped, + CheckStatus::Skipped, + CheckStatus::Skipped, + ] + ); + let facts = output.facts.expect("loopback always produces facts"); + assert_eq!(facts.kind, ClientIdKind::Loopback); + assert!(matches!(facts.raw_metadata, RawMetadata::Implicit { .. })); + } + + #[test] + fn network_failure_blocks_downstream_checks() { + let outcome = HttpsFetchOutcome::NetworkFailure(IdentityError::InvalidHandle); + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert!(output.facts.is_none()); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::NetworkError, + CheckStatus::Skipped, + CheckStatus::Skipped, + ] + ); + } + + #[test] + fn non_2xx_is_network_error_not_spec_violation() { + let outcome = HttpsFetchOutcome::Response { + status: 500, + body: b"{}".to_vec(), + content_type: Some("application/json".to_string()), + }; + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert!(output.facts.is_none()); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::NetworkError, + CheckStatus::Skipped, + CheckStatus::Skipped, + ] + ); + } + + #[test] + fn non_200_2xx_is_spec_violation() { + // 201 / 204 / 3xx (followed) all violate the + // "must be 200" + // requirement. + let outcome = HttpsFetchOutcome::Response { + status: 201, + body: b"{}".to_vec(), + content_type: Some("application/json".to_string()), + }; + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert!(output.facts.is_none()); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::SpecViolation, + CheckStatus::Skipped, + CheckStatus::Skipped, + ] + ); + } + + #[test] + fn ok_with_html_content_type_passes_json_parse_but_flags_content_type() { + let outcome = HttpsFetchOutcome::Response { + status: 200, + body: br#"{"client_id":"https://example.com/x"}"#.to_vec(), + content_type: Some("text/html".to_string()), + }; + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, // ClientIdWellFormed + CheckStatus::Pass, // MetadataDocumentFetchable + CheckStatus::SpecViolation, // MetadataContentTypeIsJson + CheckStatus::Pass, // MetadataIsJson + ] + ); + // Facts are still produced — downstream metadata stage can run. + assert!(output.facts.is_some()); + } + + #[test] + fn ok_with_invalid_json_emits_spec_violation_and_no_facts() { + let outcome = HttpsFetchOutcome::Response { + status: 200, + body: b"not json".to_vec(), + content_type: Some("application/json".to_string()), + }; + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert!(output.facts.is_none()); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::Pass, + CheckStatus::Pass, + CheckStatus::SpecViolation, + ] + ); + } + + #[test] + fn ok_with_charset_param_is_accepted_as_json_content_type() { + let outcome = HttpsFetchOutcome::Response { + status: 200, + body: b"{}".to_vec(), + content_type: Some("application/json; charset=utf-8".to_string()), + }; + let output = evaluate_https_metadata_response(&https_url(), outcome); + assert!(output.facts.is_some()); + assert_eq!( + statuses(&output), + vec![ + CheckStatus::Pass, + CheckStatus::Pass, + CheckStatus::Pass, + CheckStatus::Pass, + ] + ); + } + + #[test] + fn content_type_predicate_handles_edge_cases() { + assert!(content_type_is_json(Some("application/json"))); + assert!(content_type_is_json(Some("application/JSON"))); + assert!(content_type_is_json(Some("application/json;charset=utf-8"))); + assert!(content_type_is_json(Some(" application/json "))); + assert!(!content_type_is_json(None)); + assert!(!content_type_is_json(Some("text/html"))); + assert!(!content_type_is_json(Some("application/ld+json"))); + } +} diff --git a/src/common/identity.rs b/src/common/identity.rs index 83696b4..2ef6ab5 100644 --- a/src/common/identity.rs +++ b/src/common/identity.rs @@ -1739,4 +1739,110 @@ mod tests { _ => panic!("Expected P256 keys"), } } + + // Property-based tests pinning the roundtrip and length invariants + // documented in `src/common/CLAUDE.md`: + // - `encode_multikey(parse_multikey(s).verifying_key) == s` for + // every well-formed atproto multikey, + // - `AnySignature::to_jws_bytes()` is always exactly 64 bytes for + // both curves. + // Keys are generated deterministically from the proptest-supplied + // 32-byte seed via `ChaCha20Rng`, so each shrunk failure is + // reproducible from the seed alone. + mod pbt { + use super::*; + use proptest::prelude::*; + use rand_chacha::ChaCha20Rng; + use rand_core::SeedableRng; + + proptest! { + #![proptest_config(ProptestConfig::with_cases(64))] + + #[test] + fn multikey_roundtrip_k256(seed in any::<[u8; 32]>()) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = k256::ecdsa::SigningKey::random(&mut rng); + let original = AnyVerifyingKey::K256(*signing.verifying_key()); + + let encoded = encode_multikey(&original); + prop_assert!(encoded.starts_with('z')); + + let parsed = parse_multikey(&encoded) + .expect("a freshly encoded multikey must parse"); + let re_encoded = encode_multikey(&parsed.verifying_key); + prop_assert_eq!( + encoded, + re_encoded, + "encode_multikey must round-trip through parse_multikey" + ); + } + + #[test] + fn multikey_roundtrip_p256(seed in any::<[u8; 32]>()) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = p256::ecdsa::SigningKey::random(&mut rng); + let original = AnyVerifyingKey::P256(*signing.verifying_key()); + + let encoded = encode_multikey(&original); + prop_assert!(encoded.starts_with('z')); + + let parsed = parse_multikey(&encoded) + .expect("a freshly encoded multikey must parse"); + let re_encoded = encode_multikey(&parsed.verifying_key); + prop_assert_eq!( + encoded, + re_encoded, + "encode_multikey must round-trip through parse_multikey" + ); + } + + #[test] + fn signature_jws_bytes_is_always_64_k256( + seed in any::<[u8; 32]>(), + msg_seed in any::<[u8; 32]>(), + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::K256(k256::ecdsa::SigningKey::random(&mut rng)); + let signature = signing.sign_prehash(&msg_seed); + prop_assert_eq!(signature.to_jws_bytes().len(), 64); + } + + #[test] + fn signature_jws_bytes_is_always_64_p256( + seed in any::<[u8; 32]>(), + msg_seed in any::<[u8; 32]>(), + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::P256(p256::ecdsa::SigningKey::random(&mut rng)); + let signature = signing.sign_prehash(&msg_seed); + prop_assert_eq!(signature.to_jws_bytes().len(), 64); + } + + #[test] + fn sign_verify_prehash_roundtrip_k256( + seed in any::<[u8; 32]>(), + msg in any::<[u8; 32]>(), + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::K256(k256::ecdsa::SigningKey::random(&mut rng)); + let signature = signing.sign_prehash(&msg); + let verifying = signing.verifying_key(); + verifying.verify_prehash(&msg, &signature) + .expect("verify_prehash must accept a freshly produced signature"); + } + + #[test] + fn sign_verify_prehash_roundtrip_p256( + seed in any::<[u8; 32]>(), + msg in any::<[u8; 32]>(), + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::P256(p256::ecdsa::SigningKey::random(&mut rng)); + let signature = signing.sign_prehash(&msg); + let verifying = signing.verifying_key(); + verifying.verify_prehash(&msg, &signature) + .expect("verify_prehash must accept a freshly produced signature"); + } + } + } } diff --git a/src/common/jwt.rs b/src/common/jwt.rs index 81a11e6..1b162e4 100644 --- a/src/common/jwt.rs +++ b/src/common/jwt.rs @@ -459,4 +459,93 @@ mod tests { let result = verify_compact(&tampered, &vkey); assert!(matches!(result, Err(JwtError::InvalidSignatureScalar))); } + + // Property-based roundtrip tests pinning the invariant that + // `verify_compact(encode_compact(claims, key), key.verifying_key())` + // recovers the same claim payload, for every well-formed claim set + // and every signing key generated from a 32-byte seed. + mod pbt { + use super::*; + use proptest::prelude::*; + use rand_chacha::ChaCha20Rng; + use rand_core::SeedableRng; + + // 16 hex chars matches the format atproto labelers expect for + // `jti` and is what `RelyingParty::new_jti` produces. + const JTI_REGEX: &str = "[0-9a-f]{16}"; + + proptest! { + #![proptest_config(ProptestConfig::with_cases(32))] + + #[test] + fn encode_verify_compact_roundtrip_k256( + seed in any::<[u8; 32]>(), + jti in JTI_REGEX, + iat in 1_500_000_000i64..2_500_000_000i64, + exp_offset in 1i64..86_400i64, + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::K256(k256::ecdsa::SigningKey::random(&mut rng)); + let vkey = signing.verifying_key(); + let header = JwtHeader::for_signing_key(&signing); + let claims = JwtClaims { + iss: "did:web:test".to_string(), + aud: "did:plc:test".to_string(), + exp: iat + exp_offset, + iat, + lxm: "com.atproto.moderation.createReport".to_string(), + jti, + }; + + let token = encode_compact(&header, &claims, &signing) + .expect("encode_compact must succeed"); + let (decoded_header, decoded_claims) = verify_compact(&token, &vkey) + .expect("verify_compact must accept a freshly produced token"); + + prop_assert_eq!(decoded_header.alg, "ES256K"); + prop_assert_eq!(decoded_header.typ, header.typ); + prop_assert_eq!(decoded_claims.iss, claims.iss); + prop_assert_eq!(decoded_claims.aud, claims.aud); + prop_assert_eq!(decoded_claims.exp, claims.exp); + prop_assert_eq!(decoded_claims.iat, claims.iat); + prop_assert_eq!(decoded_claims.lxm, claims.lxm); + prop_assert_eq!(decoded_claims.jti, claims.jti); + } + + #[test] + fn encode_verify_compact_roundtrip_p256( + seed in any::<[u8; 32]>(), + jti in JTI_REGEX, + iat in 1_500_000_000i64..2_500_000_000i64, + exp_offset in 1i64..86_400i64, + ) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing = AnySigningKey::P256(p256::ecdsa::SigningKey::random(&mut rng)); + let vkey = signing.verifying_key(); + let header = JwtHeader::for_signing_key(&signing); + let claims = JwtClaims { + iss: "did:web:example.com".to_string(), + aud: "did:plc:test".to_string(), + exp: iat + exp_offset, + iat, + lxm: "com.atproto.moderation.createReport".to_string(), + jti, + }; + + let token = encode_compact(&header, &claims, &signing) + .expect("encode_compact must succeed"); + let (decoded_header, decoded_claims) = verify_compact(&token, &vkey) + .expect("verify_compact must accept a freshly produced token"); + + prop_assert_eq!(decoded_header.alg, "ES256"); + prop_assert_eq!(decoded_header.typ, header.typ); + prop_assert_eq!(decoded_claims.iss, claims.iss); + prop_assert_eq!(decoded_claims.aud, claims.aud); + prop_assert_eq!(decoded_claims.exp, claims.exp); + prop_assert_eq!(decoded_claims.iat, claims.iat); + prop_assert_eq!(decoded_claims.lxm, claims.lxm); + prop_assert_eq!(decoded_claims.jti, claims.jti); + } + } + } } diff --git a/src/common/oauth/jws.rs b/src/common/oauth/jws.rs index e5b1e7e..7c47069 100644 --- a/src/common/oauth/jws.rs +++ b/src/common/oauth/jws.rs @@ -750,4 +750,75 @@ mod tests { assert_eq!(code_str, Some("oauth_client::jws::not_json".to_string())); } } + + // Property-based test for the ES256 sign/verify roundtrip: + // `verify_jws(sign_es256_jws(claims, key), parsed_jwk(key)) == claims` + // for every signing key generated from a 32-byte seed and arbitrary + // string claim payload. + mod pbt { + use super::*; + use p256::ecdsa::SigningKey; + use p256::pkcs8::EncodePrivateKey; + use proptest::prelude::*; + use rand_chacha::ChaCha20Rng; + use rand_core::SeedableRng; + + /// Construct the JWK + encoding key pair backing a P-256 signing + /// key, in the same shape `oauth/jws::parse_jwk` accepts. + fn build_es256_material(seed: [u8; 32]) -> (ParsedJwk, EncodingKey) { + let mut rng = ChaCha20Rng::from_seed(seed); + let signing_key = SigningKey::random(&mut rng); + + let public_key = signing_key.verifying_key(); + let sec1 = public_key.to_sec1_bytes(); + let x_b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(&sec1[1..33]); + let y_b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(&sec1[33..65]); + + let jwk_json = serde_json::json!({ + "kty": "EC", + "crv": "P-256", + "x": x_b64, + "y": y_b64, + "kid": "k1", + "alg": "ES256", + "use": "sig", + }); + let source = Arc::<[u8]>::from(b"test".as_ref()); + let parsed = parse_jwk(&jwk_json, "test", source).expect("freshly built JWK parses"); + + let der = signing_key + .to_pkcs8_der() + .expect("p256 keys must export to PKCS8"); + let encoding_key = EncodingKey::from_ec_der(der.as_bytes()); + + (parsed, encoding_key) + } + + proptest! { + // Each iteration builds a key + does a sign/verify pair; cap + // at 16 to keep the suite fast. + #![proptest_config(ProptestConfig::with_cases(16))] + + #[test] + fn es256_sign_verify_roundtrip( + seed in any::<[u8; 32]>(), + foo in any::(), + count in 0u32..1000u32, + ) { + let (jwk, encoding_key) = build_es256_material(seed); + + let claims = serde_json::json!({"foo": foo, "count": count}); + let mut header = jsonwebtoken::Header::new(Algorithm::ES256); + header.kid = Some("k1".to_string()); + + let token = sign_es256_jws(&header, &claims, &encoding_key) + .expect("sign_es256_jws must succeed"); + + let decoded: jsonwebtoken::TokenData = + verify_jws(&token, &jwk, JwsAlg::Es256) + .expect("verify_jws must accept a freshly produced token"); + prop_assert_eq!(decoded.claims, claims); + } + } + } } diff --git a/src/common/oauth/relying_party.rs b/src/common/oauth/relying_party.rs index 60bcccb..82ea7d5 100644 --- a/src/common/oauth/relying_party.rs +++ b/src/common/oauth/relying_party.rs @@ -4,6 +4,27 @@ //! (Pushed Authorization Request), PKCE S256, DPoP proof, and private_key_jwt //! flows. The RelyingParty is constructed with a deterministic seeded RNG to //! enable reproducible testing. +//! +//! # HTTP-client seam exception +//! +//! Unlike every other network-touching module in the crate (which routes +//! through the `HttpClient` trait from `common::identity` so tests can +//! intercept traffic with `FakeHttpClient`), the RelyingParty uses +//! `reqwest::Client` directly. This is deliberate: +//! +//! 1. The RP only ever talks to an authorization server over genuine HTTP. +//! The interactive tests stand up a real axum fake-AS on `127.0.0.1`, +//! so determinism comes from a fixed `Clock` + seeded `ChaCha20Rng`, +//! not from intercepting bytes on the wire. +//! 2. The `do_authorize` flow needs a redirect-following policy distinct +//! from the rest of the RP's calls, which is awkward to express +//! through a two-method `HttpClient` trait without leaking +//! reqwest-specific concepts back through the seam. +//! +//! The RP owns two `reqwest::Client` instances built once at construction +//! time: `http` (default redirect policy) for PAR / token / refresh / +//! discover_as, and `http_no_redirect` for `do_authorize_inner`, which +//! manually inspects the first 3xx response. use std::collections::HashMap; use std::sync::{Arc, Mutex}; @@ -147,7 +168,13 @@ pub struct RelyingParty { signing_jwk_public: Value, clock: Arc, rng: Mutex, + /// Default-redirect-policy client used for PAR, token, refresh, + /// and `discover_as`. http: ReqwestClient, + /// Redirect-disabled client used by `do_authorize_inner`, which + /// inspects the first 3xx response from the authorization endpoint + /// and resolves the `redirect_uri` itself. + http_no_redirect: ReqwestClient, /// DPoP nonces keyed by endpoint URL, for use_dpop_nonce retry. dpop_nonces: Mutex>, } @@ -246,13 +273,26 @@ impl RelyingParty { "y": y, }); - // Build HTTP client with rustls, user-agent, and timeout. + // Build HTTP clients with rustls, user-agent, and timeout. The + // no-redirect variant is used by `do_authorize_inner` to inspect + // the first 3xx response from the authorization endpoint + // directly. Both clients share the same TLS pool only within + // their own builder; reqwest does not let us share a connection + // pool across two redirect policies, so we accept two pools per + // RP. let http = ReqwestClient::builder() .use_rustls_tls() .user_agent(APP_USER_AGENT) .timeout(Duration::from_secs(30)) .build() .unwrap_or_else(|_| ReqwestClient::new()); + let http_no_redirect = ReqwestClient::builder() + .use_rustls_tls() + .user_agent(APP_USER_AGENT) + .timeout(Duration::from_secs(30)) + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap_or_else(|_| ReqwestClient::new()); Self { client_id, @@ -262,6 +302,7 @@ impl RelyingParty { clock, rng: Mutex::new(rng), http, + http_no_redirect, dpop_nonces: Mutex::new(HashMap::new()), } } @@ -278,57 +319,8 @@ impl RelyingParty { }); } - let metadata: serde_json::Value = response.json().await?; - - let issuer = metadata - .get("issuer") - .and_then(|v| v.as_str()) - .and_then(|s| Url::parse(s).ok()) - .ok_or_else(|| RpError::MetadataMalformed { - reason: "missing or invalid issuer".to_string(), - })?; - - let pushed_authorization_request_endpoint = metadata - .get("pushed_authorization_request_endpoint") - .and_then(|v| v.as_str()) - .and_then(|s| Url::parse(s).ok()) - .ok_or_else(|| RpError::MetadataMalformed { - reason: "missing or invalid pushed_authorization_request_endpoint".to_string(), - })?; - - let authorization_endpoint = metadata - .get("authorization_endpoint") - .and_then(|v| v.as_str()) - .and_then(|s| Url::parse(s).ok()) - .ok_or_else(|| RpError::MetadataMalformed { - reason: "missing or invalid authorization_endpoint".to_string(), - })?; - - let token_endpoint = metadata - .get("token_endpoint") - .and_then(|v| v.as_str()) - .and_then(|s| Url::parse(s).ok()) - .ok_or_else(|| RpError::MetadataMalformed { - reason: "missing or invalid token_endpoint".to_string(), - })?; - - let require_pushed_authorization_requests = metadata - .get("require_pushed_authorization_requests") - .and_then(|v| v.as_bool()) - .unwrap_or(false); - - if !require_pushed_authorization_requests { - return Err(RpError::MetadataMalformed { - reason: "require_pushed_authorization_requests is not true".to_string(), - }); - } - - Ok(AsDescriptor { - issuer, - pushed_authorization_request_endpoint, - authorization_endpoint, - token_endpoint, - }) + let metadata: Value = response.json().await?; + parse_as_descriptor(&metadata) } /// Perform Pushed Authorization Request (PAR). @@ -478,16 +470,10 @@ impl RelyingParty { .append_pair("request_uri", request_uri) .append_pair("client_id", self.client_id.as_str()); - // Build a client with redirect policy disabled to manually follow redirects. - let client = ReqwestClient::builder() - .use_rustls_tls() - .user_agent(APP_USER_AGENT) - .timeout(Duration::from_secs(30)) - .redirect(reqwest::redirect::Policy::none()) - .build() - .unwrap_or_else(|_| ReqwestClient::new()); - - let response = client.get(url).send().await?; + // Reuse the redirect-disabled client built once in + // `RelyingParty::new` so successive `do_authorize` calls share + // its connection pool. + let response = self.http_no_redirect.get(url).send().await?; // Follow redirects manually: stop on the first redirect whose // Location header matches the redirect_uri scheme/origin. @@ -965,6 +951,64 @@ impl RelyingParty { } } +/// Pure: parse an authorization-server metadata document into an +/// `AsDescriptor`. Returns `RpError::MetadataMalformed` if any required +/// field (`issuer`, `pushed_authorization_request_endpoint`, +/// `authorization_endpoint`, `token_endpoint`) is missing or unparseable, +/// or if `require_pushed_authorization_requests` is not advertised as +/// `true` (atproto requires PAR). +fn parse_as_descriptor(metadata: &Value) -> Result { + let issuer = metadata + .get("issuer") + .and_then(|v| v.as_str()) + .and_then(|s| Url::parse(s).ok()) + .ok_or_else(|| RpError::MetadataMalformed { + reason: "missing or invalid issuer".to_string(), + })?; + + let pushed_authorization_request_endpoint = metadata + .get("pushed_authorization_request_endpoint") + .and_then(|v| v.as_str()) + .and_then(|s| Url::parse(s).ok()) + .ok_or_else(|| RpError::MetadataMalformed { + reason: "missing or invalid pushed_authorization_request_endpoint".to_string(), + })?; + + let authorization_endpoint = metadata + .get("authorization_endpoint") + .and_then(|v| v.as_str()) + .and_then(|s| Url::parse(s).ok()) + .ok_or_else(|| RpError::MetadataMalformed { + reason: "missing or invalid authorization_endpoint".to_string(), + })?; + + let token_endpoint = metadata + .get("token_endpoint") + .and_then(|v| v.as_str()) + .and_then(|s| Url::parse(s).ok()) + .ok_or_else(|| RpError::MetadataMalformed { + reason: "missing or invalid token_endpoint".to_string(), + })?; + + let require_pushed_authorization_requests = metadata + .get("require_pushed_authorization_requests") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + + if !require_pushed_authorization_requests { + return Err(RpError::MetadataMalformed { + reason: "require_pushed_authorization_requests is not true".to_string(), + }); + } + + Ok(AsDescriptor { + issuer, + pushed_authorization_request_endpoint, + authorization_endpoint, + token_endpoint, + }) +} + /// Verify that the `iss` query parameter on an authorization /// redirect matches the expected AS issuer. The comparison strips a /// single trailing slash from both sides because atproto's @@ -1110,4 +1154,107 @@ mod tests { assert!(payload.get("jti").is_some(), "JTI should be present"); assert!(payload.get("iat").is_some(), "iat should be present"); } + + fn good_metadata() -> serde_json::Value { + json!({ + "issuer": "https://auth.example.com", + "pushed_authorization_request_endpoint": "https://auth.example.com/oauth/par", + "authorization_endpoint": "https://auth.example.com/oauth/authorize", + "token_endpoint": "https://auth.example.com/oauth/token", + "require_pushed_authorization_requests": true, + }) + } + + #[test] + fn parse_as_descriptor_accepts_well_formed_metadata() { + let metadata = good_metadata(); + let descriptor = parse_as_descriptor(&metadata).expect("well-formed metadata parses"); + assert_eq!(descriptor.issuer.as_str(), "https://auth.example.com/"); + assert_eq!( + descriptor.token_endpoint.as_str(), + "https://auth.example.com/oauth/token" + ); + } + + #[test] + fn parse_as_descriptor_requires_par_advertisement() { + // atproto requires `require_pushed_authorization_requests = true`; + // any other value (including missing) must error. + let mut metadata = good_metadata(); + metadata + .as_object_mut() + .unwrap() + .remove("require_pushed_authorization_requests"); + let err = parse_as_descriptor(&metadata).expect_err("missing PAR flag must error"); + assert!(matches!(err, RpError::MetadataMalformed { .. })); + + let metadata = json!({ + "issuer": "https://auth.example.com", + "pushed_authorization_request_endpoint": "https://auth.example.com/oauth/par", + "authorization_endpoint": "https://auth.example.com/oauth/authorize", + "token_endpoint": "https://auth.example.com/oauth/token", + "require_pushed_authorization_requests": false, + }); + let err = parse_as_descriptor(&metadata).expect_err("PAR=false must error"); + assert!(matches!(err, RpError::MetadataMalformed { .. })); + } + + #[test] + fn parse_as_descriptor_rejects_each_missing_required_field() { + for field in [ + "issuer", + "pushed_authorization_request_endpoint", + "authorization_endpoint", + "token_endpoint", + ] { + let mut metadata = good_metadata(); + metadata.as_object_mut().unwrap().remove(field); + let err = parse_as_descriptor(&metadata) + .unwrap_err_with(|_| format!("missing {field} must error")); + match err { + RpError::MetadataMalformed { reason } => { + assert!( + reason.contains(field), + "error message should mention {field}, got {reason:?}" + ); + } + other => panic!("expected MetadataMalformed, got {other:?}"), + } + } + } + + #[test] + fn parse_as_descriptor_rejects_non_url_endpoint() { + let mut metadata = good_metadata(); + metadata.as_object_mut().unwrap().insert( + "token_endpoint".to_string(), + serde_json::Value::String("not a url".to_string()), + ); + let err = parse_as_descriptor(&metadata).expect_err("non-URL endpoint must error"); + match err { + RpError::MetadataMalformed { reason } => { + assert!( + reason.contains("token_endpoint"), + "error message should mention token_endpoint, got {reason:?}" + ); + } + other => panic!("expected MetadataMalformed, got {other:?}"), + } + } + + /// Variant of `Result::expect_err` whose panic message is computed + /// from the surprising `Ok` value, so loop iterations can attribute + /// the failure back to the field under test. + trait UnwrapErrWith { + fn unwrap_err_with String>(self, msg: F) -> E; + } + + impl UnwrapErrWith for Result { + fn unwrap_err_with String>(self, msg: F) -> E { + match self { + Ok(v) => panic!("{}: got Ok({v:?})", msg(&v)), + Err(e) => e, + } + } + } } -- 2.51.2 From 0e0fcdd711cd8c47906aea2b971267401d9a477c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 26 Apr 2026 00:33:32 +0000 Subject: [PATCH 2/2] Classify every src file under FCIS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a `// pattern: ` comment to every src file with runtime behaviour, per the ed3d-house-style FCIS skill. Distribution: 10 Functional Core, 8 Imperative Shell, 20 Mixed (each with a one-line reason). The three barrel/re-export files (lib.rs, common.rs, common/oauth.rs) are exempt under the skill's rules. These are descriptive comments only — no behaviour change. They make the existing architecture (narrow trait seams, pure helpers, stage orchestration) legible to future readers without having to infer the boundary from imports. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/cli.rs | 2 ++ src/commands.rs | 2 ++ src/commands/test.rs | 2 ++ src/commands/test/labeler.rs | 2 ++ src/commands/test/labeler/pipeline.rs | 8 ++++++++ src/commands/test/labeler/pipeline/create_report.rs | 2 ++ .../labeler/pipeline/create_report/did_doc_server.rs | 5 +++++ .../test/labeler/pipeline/create_report/pollution.rs | 2 ++ .../test/labeler/pipeline/create_report/self_mint.rs | 6 ++++++ .../test/labeler/pipeline/create_report/sentinel.rs | 5 +++++ src/commands/test/labeler/pipeline/crypto.rs | 2 ++ src/commands/test/labeler/pipeline/http.rs | 2 ++ src/commands/test/labeler/pipeline/identity.rs | 2 ++ src/commands/test/labeler/pipeline/subscription.rs | 2 ++ src/commands/test/labeler/target.rs | 2 ++ src/commands/test/oauth.rs | 2 ++ src/commands/test/oauth/client.rs | 2 ++ src/commands/test/oauth/client/fake_as.rs | 7 +++++++ src/commands/test/oauth/client/fake_as/endpoints.rs | 7 +++++++ src/commands/test/oauth/client/fake_as/identity.rs | 2 ++ src/commands/test/oauth/client/fake_as/request_log.rs | 6 ++++++ src/commands/test/oauth/client/pipeline.rs | 2 ++ src/commands/test/oauth/client/pipeline/discovery.rs | 7 +++++++ .../test/oauth/client/pipeline/interactive.rs | 2 ++ .../oauth/client/pipeline/interactive/dpop_edges.rs | 2 ++ .../pipeline/interactive/iss_sub_verification.rs | 2 ++ .../client/pipeline/interactive/scope_variations.rs | 2 ++ src/commands/test/oauth/client/pipeline/jwks.rs | 2 ++ src/commands/test/oauth/client/pipeline/metadata.rs | 6 ++++++ src/commands/test/oauth/client/target.rs | 2 ++ src/common/diagnostics.rs | 9 +++++++++ src/common/identity.rs | 11 +++++++++++ src/common/jwt.rs | 2 ++ src/common/oauth/clock.rs | 6 ++++++ src/common/oauth/jws.rs | 2 ++ src/common/oauth/relying_party.rs | 9 +++++++++ src/common/report.rs | 2 ++ src/main.rs | 2 ++ 38 files changed, 142 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index 4cef87a..24d2272 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,5 +1,7 @@ //! Root clap parser and dispatch entry point. +// pattern: Imperative Shell + use clap::Parser; use miette::Result; use std::process::ExitCode; diff --git a/src/commands.rs b/src/commands.rs index c5bf487..5092e2c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1,5 +1,7 @@ //! Top-level subcommand dispatch. +// pattern: Imperative Shell + use clap::Subcommand; use miette::Report; use std::process::ExitCode; diff --git a/src/commands/test.rs b/src/commands/test.rs index 9d9061e..c193ae7 100644 --- a/src/commands/test.rs +++ b/src/commands/test.rs @@ -1,5 +1,7 @@ //! `atproto-devtool test ...` subcommand tree. +// pattern: Imperative Shell + use clap::Subcommand; use miette::Report; use std::process::ExitCode; diff --git a/src/commands/test/labeler.rs b/src/commands/test/labeler.rs index 6139503..d73b6ee 100644 --- a/src/commands/test/labeler.rs +++ b/src/commands/test/labeler.rs @@ -1,5 +1,7 @@ //! `atproto-devtool test labeler ` command. +// pattern: Imperative Shell + pub mod pipeline; pub mod target; diff --git a/src/commands/test/labeler/pipeline.rs b/src/commands/test/labeler/pipeline.rs index 716f412..f743639 100644 --- a/src/commands/test/labeler/pipeline.rs +++ b/src/commands/test/labeler/pipeline.rs @@ -1,5 +1,13 @@ //! Target parsing and pipeline driver skeleton for labeler conformance checks. +// pattern: Mixed (unavoidable) +// +// Stage orchestration: drives every stage's `run` (which itself does I/O +// through trait seams) and threads facts forward. Splitting orchestration +// from per-stage gating logic would not improve testability — the +// pipeline is already exercised end-to-end by `tests/labeler_*` with +// fakes wired in. + use std::time::Duration; use url::Url; diff --git a/src/commands/test/labeler/pipeline/create_report.rs b/src/commands/test/labeler/pipeline/create_report.rs index 79c0ced..662a241 100644 --- a/src/commands/test/labeler/pipeline/create_report.rs +++ b/src/commands/test/labeler/pipeline/create_report.rs @@ -4,6 +4,8 @@ //! The `sentinel` submodule builds the pollution-avoidance reason string //! that every committed report body carries. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::sync::Arc; use std::time::{Duration, SystemTime, UNIX_EPOCH}; diff --git a/src/commands/test/labeler/pipeline/create_report/did_doc_server.rs b/src/commands/test/labeler/pipeline/create_report/did_doc_server.rs index d964ed2..ba25f4f 100644 --- a/src/commands/test/labeler/pipeline/create_report/did_doc_server.rs +++ b/src/commands/test/labeler/pipeline/create_report/did_doc_server.rs @@ -9,6 +9,11 @@ //! Shuts down on drop: the RAII handle aborts the background task and //! closes the listener. +// pattern: Imperative Shell +// +// Pure-listener loop: binds a TCP socket and serves a fixed JSON +// payload. No business logic; the DID document is built upstream. + use std::net::SocketAddr; use std::sync::Arc; diff --git a/src/commands/test/labeler/pipeline/create_report/pollution.rs b/src/commands/test/labeler/pipeline/create_report/pollution.rs index fead3b8..bc3e994 100644 --- a/src/commands/test/labeler/pipeline/create_report/pollution.rs +++ b/src/commands/test/labeler/pipeline/create_report/pollution.rs @@ -23,6 +23,8 @@ //! (pointing at the reporter's own DID) to exercise the simplest working //! shape. This makes the test deterministic for round-trip debugging. +// pattern: Functional Core + use serde_json::{Value, json}; use crate::common::identity::Did; diff --git a/src/commands/test/labeler/pipeline/create_report/self_mint.rs b/src/commands/test/labeler/pipeline/create_report/self_mint.rs index 37b7ca3..3d0ef22 100644 --- a/src/commands/test/labeler/pipeline/create_report/self_mint.rs +++ b/src/commands/test/labeler/pipeline/create_report/self_mint.rs @@ -2,6 +2,12 @@ //! server, and a reference curve. Exposes a single method for signing //! atproto service-auth JWTs with that identity. +// pattern: Mixed (unavoidable) +// +// Owns a signing key (pure crypto), a `DidDocServer` (imperative TCP), +// and a JWT minting helper (pure). The mix mirrors the lifecycle: the +// signer must keep the server alive while it's signing tokens. + use std::net::SocketAddr; use std::time::Duration; diff --git a/src/commands/test/labeler/pipeline/create_report/sentinel.rs b/src/commands/test/labeler/pipeline/create_report/sentinel.rs index cb3467e..d616a46 100644 --- a/src/commands/test/labeler/pipeline/create_report/sentinel.rs +++ b/src/commands/test/labeler/pipeline/create_report/sentinel.rs @@ -14,6 +14,11 @@ //! within a single `test labeler` invocation so operators can trace a group //! of test reports back to one run. +// pattern: Functional Core +// +// `build` and `format_rfc3339_utc` take time as a parameter; the +// `SystemTime` import is for the type only. + use std::time::{SystemTime, UNIX_EPOCH}; /// Prefix used so operators can grep their moderation queue for diff --git a/src/commands/test/labeler/pipeline/crypto.rs b/src/commands/test/labeler/pipeline/crypto.rs index a7ffdde..cd54f85 100644 --- a/src/commands/test/labeler/pipeline/crypto.rs +++ b/src/commands/test/labeler/pipeline/crypto.rs @@ -5,6 +5,8 @@ //! (deterministic canonical encoding per RFC 8949) and supports key rotation via //! the did:plc audit log. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::collections::BTreeMap; diff --git a/src/commands/test/labeler/pipeline/http.rs b/src/commands/test/labeler/pipeline/http.rs index 5557265..8f9ddcd 100644 --- a/src/commands/test/labeler/pipeline/http.rs +++ b/src/commands/test/labeler/pipeline/http.rs @@ -3,6 +3,8 @@ //! Performs `com.atproto.label.queryLabels` requests against the labeler endpoint, //! verifies schema conformance, and exercises pagination. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::sync::Arc; diff --git a/src/commands/test/labeler/pipeline/identity.rs b/src/commands/test/labeler/pipeline/identity.rs index cfa8a05..1870057 100644 --- a/src/commands/test/labeler/pipeline/identity.rs +++ b/src/commands/test/labeler/pipeline/identity.rs @@ -3,6 +3,8 @@ //! Performs DID document resolution and labeler record validation, //! emitting a series of named checks for each identity-layer requirement. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::sync::Arc; diff --git a/src/commands/test/labeler/pipeline/subscription.rs b/src/commands/test/labeler/pipeline/subscription.rs index b569289..46717a7 100644 --- a/src/commands/test/labeler/pipeline/subscription.rs +++ b/src/commands/test/labeler/pipeline/subscription.rs @@ -4,6 +4,8 @@ //! using a two-connection strategy: backfill with cursor=0, and live-tail if backfill //! did not complete within the budget. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::sync::Arc; use std::time::Duration; diff --git a/src/commands/test/labeler/target.rs b/src/commands/test/labeler/target.rs index 3954a19..0f3a20a 100644 --- a/src/commands/test/labeler/target.rs +++ b/src/commands/test/labeler/target.rs @@ -1,3 +1,5 @@ +// pattern: Functional Core + use std::fmt; use miette::Diagnostic; diff --git a/src/commands/test/oauth.rs b/src/commands/test/oauth.rs index 7e9629b..404aea3 100644 --- a/src/commands/test/oauth.rs +++ b/src/commands/test/oauth.rs @@ -1,5 +1,7 @@ //! OAuth conformance tests. +// pattern: Imperative Shell + use std::process::ExitCode; use clap::Subcommand; diff --git a/src/commands/test/oauth/client.rs b/src/commands/test/oauth/client.rs index 5191702..5b4924d 100644 --- a/src/commands/test/oauth/client.rs +++ b/src/commands/test/oauth/client.rs @@ -6,6 +6,8 @@ //! spins up an in-process fake authorization server and observes the //! client driving end-to-end OAuth flows. +// pattern: Imperative Shell + pub mod fake_as; pub mod pipeline; pub mod target; diff --git a/src/commands/test/oauth/client/fake_as.rs b/src/commands/test/oauth/client/fake_as.rs index 47051f4..e960b1f 100644 --- a/src/commands/test/oauth/client/fake_as.rs +++ b/src/commands/test/oauth/client/fake_as.rs @@ -1,5 +1,12 @@ //! In-process fake atproto OAuth authorization server for interactive mode. +// pattern: Imperative Shell +// +// Owns the axum server lifecycle (bind, spawn, shutdown) and exposes +// a `ServerHandle` through which tests address the AS by URL. Routing +// and per-flow logic live in `endpoints` and `identity` (Mixed and +// Functional Core respectively). + pub mod endpoints; pub mod identity; pub mod request_log; diff --git a/src/commands/test/oauth/client/fake_as/endpoints.rs b/src/commands/test/oauth/client/fake_as/endpoints.rs index de53c4e..a323a09 100644 --- a/src/commands/test/oauth/client/fake_as/endpoints.rs +++ b/src/commands/test/oauth/client/fake_as/endpoints.rs @@ -1,3 +1,10 @@ +// pattern: Mixed (unavoidable) +// +// Axum handlers (Imperative Shell — accept the request, log it, return +// the response) wrap the per-`FlowScript` decision logic (pure: which +// status, which body, which header). The mix is deliberate: each handler +// is short enough that splitting would obscure the request/response shape. + use std::collections::{HashMap, HashSet, VecDeque}; use std::sync::{Arc, Mutex}; diff --git a/src/commands/test/oauth/client/fake_as/identity.rs b/src/commands/test/oauth/client/fake_as/identity.rs index 9ca77c8..f3c4af2 100644 --- a/src/commands/test/oauth/client/fake_as/identity.rs +++ b/src/commands/test/oauth/client/fake_as/identity.rs @@ -1,3 +1,5 @@ +// pattern: Functional Core + use k256::ecdsa::SigningKey as K256SigningKey; use serde_json::json; use url::Url; diff --git a/src/commands/test/oauth/client/fake_as/request_log.rs b/src/commands/test/oauth/client/fake_as/request_log.rs index 20d38df..b70cc3e 100644 --- a/src/commands/test/oauth/client/fake_as/request_log.rs +++ b/src/commands/test/oauth/client/fake_as/request_log.rs @@ -1,3 +1,9 @@ +// pattern: Functional Core +// +// In-memory append-only log container. The `Mutex` provides thread-safe +// state, not external I/O — timestamps are supplied by the caller, not +// observed from `SystemTime` here. + use std::sync::Mutex; /// Append-only log of inbound requests. Cloneable `Arc` handle; locks diff --git a/src/commands/test/oauth/client/pipeline.rs b/src/commands/test/oauth/client/pipeline.rs index 7bd7e39..c1cd228 100644 --- a/src/commands/test/oauth/client/pipeline.rs +++ b/src/commands/test/oauth/client/pipeline.rs @@ -1,5 +1,7 @@ //! OAuth client conformance test pipeline and target parsing. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use super::target::OauthClientTarget; pub mod discovery; diff --git a/src/commands/test/oauth/client/pipeline/discovery.rs b/src/commands/test/oauth/client/pipeline/discovery.rs index eb35ca4..c9f46c2 100644 --- a/src/commands/test/oauth/client/pipeline/discovery.rs +++ b/src/commands/test/oauth/client/pipeline/discovery.rs @@ -3,6 +3,13 @@ //! This stage resolves the client metadata document for HTTPS targets or //! synthesizes implicit metadata for loopback development clients. +// pattern: Mixed (unavoidable) +// +// `run` is the imperative shell (does the HTTP fetch); the bulk of the +// logic lives in pure helpers `evaluate_https_metadata_response` and +// `evaluate_loopback_metadata`, which is why this stage has unit tests +// at the bottom of the file alongside its integration coverage. + use std::borrow::Cow; use std::sync::Arc; diff --git a/src/commands/test/oauth/client/pipeline/interactive.rs b/src/commands/test/oauth/client/pipeline/interactive.rs index b2bf79c..9cab96c 100644 --- a/src/commands/test/oauth/client/pipeline/interactive.rs +++ b/src/commands/test/oauth/client/pipeline/interactive.rs @@ -1,5 +1,7 @@ //! OAuth client interactive stage — fake AS server, RP-driven flow, conformance checks. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + pub mod dpop_edges; pub mod iss_sub_verification; pub mod scope_variations; diff --git a/src/commands/test/oauth/client/pipeline/interactive/dpop_edges.rs b/src/commands/test/oauth/client/pipeline/interactive/dpop_edges.rs index c8a3f4c..674c92b 100644 --- a/src/commands/test/oauth/client/pipeline/interactive/dpop_edges.rs +++ b/src/commands/test/oauth/client/pipeline/interactive/dpop_edges.rs @@ -2,6 +2,8 @@ //! //! Verifies AC7.1–AC7.6: nonce rotation, refresh rotation, replay rejection, and compliance violations. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::fmt; use std::sync::Arc; diff --git a/src/commands/test/oauth/client/pipeline/interactive/iss_sub_verification.rs b/src/commands/test/oauth/client/pipeline/interactive/iss_sub_verification.rs index e142ca5..1d4d3e2 100644 --- a/src/commands/test/oauth/client/pipeline/interactive/iss_sub_verification.rs +++ b/src/commands/test/oauth/client/pipeline/interactive/iss_sub_verification.rs @@ -18,6 +18,8 @@ //! behaviour on all other endpoints, a cooperating RP can reach the //! final verification step before the broken-AS injection kicks in. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use crate::commands::test::oauth::client::fake_as::{ServerHandle, endpoints::FlowScript}; use crate::common::oauth::relying_party::{AuthorizeOutcome, ParRequest, RelyingParty, RpError}; use crate::common::report::CheckResult; diff --git a/src/commands/test/oauth/client/pipeline/interactive/scope_variations.rs b/src/commands/test/oauth/client/pipeline/interactive/scope_variations.rs index 67123b0..30f0d6c 100644 --- a/src/commands/test/oauth/client/pipeline/interactive/scope_variations.rs +++ b/src/commands/test/oauth/client/pipeline/interactive/scope_variations.rs @@ -2,6 +2,8 @@ //! //! Verifies AC6.1–AC6.6: scope grant behaviors, user denial, refresh scoping, and mandatory fields. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use std::borrow::Cow; use std::fmt; use std::sync::Arc; diff --git a/src/commands/test/oauth/client/pipeline/jwks.rs b/src/commands/test/oauth/client/pipeline/jwks.rs index 882368e..2c98166 100644 --- a/src/commands/test/oauth/client/pipeline/jwks.rs +++ b/src/commands/test/oauth/client/pipeline/jwks.rs @@ -3,6 +3,8 @@ //! Fetches and validates the client's JWKS (JSON Web Key Set) for confidential //! clients using either an inline document or an external URI. +// pattern: Mixed (unavoidable: stage orchestration interleaves trait-object I/O with check-result construction) + use async_trait::async_trait; use miette::{Diagnostic, LabeledSpan, NamedSource}; use reqwest::Client as ReqwestClient; diff --git a/src/commands/test/oauth/client/pipeline/metadata.rs b/src/commands/test/oauth/client/pipeline/metadata.rs index 12e28f8..0873fb4 100644 --- a/src/commands/test/oauth/client/pipeline/metadata.rs +++ b/src/commands/test/oauth/client/pipeline/metadata.rs @@ -4,6 +4,12 @@ //! every spec-derived static property of an atproto OAuth client metadata //! document. +// pattern: Functional Core +// +// `metadata::run` is synchronous — it consumes `DiscoveryFacts` (already +// gathered by the discovery shell) and produces check results plus +// downstream facts. No I/O. + use miette::{Diagnostic, NamedSource, SourceSpan}; use serde::Deserialize; use std::borrow::Cow; diff --git a/src/commands/test/oauth/client/target.rs b/src/commands/test/oauth/client/target.rs index 9772f64..e722bc8 100644 --- a/src/commands/test/oauth/client/target.rs +++ b/src/commands/test/oauth/client/target.rs @@ -1,3 +1,5 @@ +// pattern: Functional Core + use std::fmt; use miette::{Diagnostic, NamedSource, SourceSpan}; diff --git a/src/common/diagnostics.rs b/src/common/diagnostics.rs index 7c2953b..d1aa524 100644 --- a/src/common/diagnostics.rs +++ b/src/common/diagnostics.rs @@ -1,5 +1,14 @@ //! Shared miette configuration and `NamedSource` helpers. +// pattern: Mixed (unavoidable) +// +// `install_miette_handler` mutates global process state via +// `miette::set_hook` (a side effect, called once from `cli::run`); the +// other helpers (`named_source_from_bytes`, `pretty_json_for_display`, +// `span_at_line_column`, `span_for_quoted_literal`) are pure and could +// live in a sibling file, but the module is small enough that splitting +// would obscure the shared diagnostic concern. + use std::sync::Arc; use miette::{GraphicalTheme, MietteHandlerOpts, NamedSource, SourceSpan}; diff --git a/src/common/identity.rs b/src/common/identity.rs index 2ef6ab5..f705aa0 100644 --- a/src/common/identity.rs +++ b/src/common/identity.rs @@ -3,6 +3,17 @@ //! This module provides a narrow interface over HTTP and DNS resolution, //! allowing callers to swap real network I/O with recorded fixtures in tests. +// pattern: Mixed (unavoidable) +// +// Pure: `parse_multikey`, `encode_multikey`, the `AnyVerifyingKey` / +// `AnySigningKey` / `AnySignature` newtypes and their crypto methods, +// `is_local_labeler_hostname`, all `IdentityError` construction. +// Imperative shell: `RealHttpClient`, `RealDnsResolver`, `resolve_handle`, +// `resolve_did`, `plc_history_for_fragment`. Splitting would force every +// downstream stage to import from two modules and would not improve +// testability — the I/O surface is already behind narrow trait seams +// (`HttpClient`, `DnsResolver`). + use async_trait::async_trait; use k256::ecdsa::signature::hazmat::PrehashVerifier; use serde::{Deserialize, Serialize}; diff --git a/src/common/jwt.rs b/src/common/jwt.rs index 1b162e4..05b0f24 100644 --- a/src/common/jwt.rs +++ b/src/common/jwt.rs @@ -7,6 +7,8 @@ //! Only ES256 and ES256K are supported (RFC 7518 §3.4); raw r||s signature //! encoding, unpadded base64url segments, UTF-8 JSON payloads. +// pattern: Functional Core + use base64::Engine; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use serde::{Deserialize, Serialize}; diff --git a/src/common/oauth/clock.rs b/src/common/oauth/clock.rs index f8e4d6d..a301aa5 100644 --- a/src/common/oauth/clock.rs +++ b/src/common/oauth/clock.rs @@ -1,3 +1,9 @@ +// pattern: Imperative Shell +// +// `RealClock::now_unix_seconds` reads `SystemTime::now()`, the canonical +// non-deterministic side effect. The trait itself is the seam tests use +// to inject `FakeClock`. + use std::time::{Duration, SystemTime, UNIX_EPOCH}; /// A clock source. `RealClock` uses `SystemTime::now()`; tests inject diff --git a/src/common/oauth/jws.rs b/src/common/oauth/jws.rs index 7c47069..abe7081 100644 --- a/src/common/oauth/jws.rs +++ b/src/common/oauth/jws.rs @@ -4,6 +4,8 @@ //! test-friendly JWS signing and JWKS parsing. Diagnostic codes in the //! `oauth_client::jws::*` namespace enable stable error reporting downstream. +// pattern: Functional Core + use std::sync::Arc; use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey}; diff --git a/src/common/oauth/relying_party.rs b/src/common/oauth/relying_party.rs index 82ea7d5..8fce96b 100644 --- a/src/common/oauth/relying_party.rs +++ b/src/common/oauth/relying_party.rs @@ -26,6 +26,15 @@ //! discover_as, and `http_no_redirect` for `do_authorize_inner`, which //! manually inspects the first 3xx response. +// pattern: Mixed (unavoidable) +// +// Pure: `parse_as_descriptor`, `verify_redirect_iss`, `extract_auth_response`, +// `new_pkce`, `new_jti`, `sign_dpop`, `sign_private_key_jwt`. Imperative +// shell: every `do_*` flow method (PAR / authorize / token / refresh) +// drives reqwest. The mix is deliberate — see the module-level docs above +// for why the RP holds its own reqwest clients rather than going through +// the project's `HttpClient` seam. + use std::collections::HashMap; use std::sync::{Arc, Mutex}; use std::time::Duration; diff --git a/src/common/report.rs b/src/common/report.rs index 4b688d2..4bcbf8c 100644 --- a/src/common/report.rs +++ b/src/common/report.rs @@ -1,5 +1,7 @@ //! Report aggregation and rendering for the labeler conformance suite. +// pattern: Functional Core + use std::borrow::Cow; use std::fmt; use std::io; diff --git a/src/main.rs b/src/main.rs index 3c2a453..a2b5258 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,7 @@ //! `atproto-devtool` binary entry point. +// pattern: Imperative Shell + use miette::Result; use std::process::ExitCode;