diff --git a/CHANGELOG.md b/CHANGELOG.md index ecbb112..9620cf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,14 @@ the Mapbox APIs' own response bodies are not. `unsupported scheme socks5` in the message is the part that distinguishes it from the network being down. +- `rand` moved from 0.8 to 0.10. No behaviour changes: the two places it is + used — the PKCE verifier and the OAuth `state` in `mapbox auth login` — + draw from `ThreadRng` before and after, which `rand` declares a CSPRNG, and + `thread_rng().gen()` becoming `random()` is a rename. Recorded because it + is the crate that generates those two values, so a login problem around + this release should be able to find it. Both are now covered by tests + against RFC 7636, which they were not before. + ## 0.1.8 - 2026-09-14 Initial beta release. The next release is `0.2.0`. diff --git a/Cargo.lock b/Cargo.lock index 6ca418b..92a2d14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -133,7 +133,7 @@ checksum = "65c35e4b699c7e15ccbe7ee35c005e4fc0a278d22238a2857e6ce2dadeda1b06" dependencies = [ "cfg-if", "cpufeatures 0.3.1", - "rand_core 0.10.1", + "rand_core", ] [[package]] @@ -420,7 +420,7 @@ dependencies = [ "js-sys", "libc", "r-efi", - "rand_core 0.10.1", + "rand_core", "wasm-bindgen", ] @@ -760,7 +760,7 @@ dependencies = [ "dirs", "flate2", "open", - "rand 0.8.8", + "rand", "reqwest", "serde", "serde_json", @@ -888,15 +888,6 @@ dependencies = [ "zerovec", ] -[[package]] -name = "ppv-lite86" -version = "0.2.21" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85eae3c4ed2f50dcfe72643da4befc30deadb458a9b590d720cde2f2b1e97da9" -dependencies = [ - "zerocopy", -] - [[package]] name = "proc-macro2" version = "1.0.107" @@ -935,7 +926,7 @@ dependencies = [ "bytes", "getrandom 0.4.3", "lru-slab", - "rand 0.10.2", + "rand", "rand_pcg", "ring", "rustc-hash", @@ -977,17 +968,6 @@ version = "6.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" -[[package]] -name = "rand" -version = "0.8.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e058c7de0b26af77780c769414d6257830bb240f3c38477dbc2c16e5f54d6d4c" -dependencies = [ - "libc", - "rand_chacha", - "rand_core 0.6.4", -] - [[package]] name = "rand" version = "0.10.2" @@ -996,26 +976,7 @@ checksum = "c7f5fa3a058cd35567ef9bfa5e75732bee0f9e4c55fa90477bef2dfcdbc4be80" dependencies = [ "chacha20", "getrandom 0.4.3", - "rand_core 0.10.1", -] - -[[package]] -name = "rand_chacha" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" -dependencies = [ - "ppv-lite86", - "rand_core 0.6.4", -] - -[[package]] -name = "rand_core" -version = "0.6.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" -dependencies = [ - "getrandom 0.2.17", + "rand_core", ] [[package]] @@ -1030,7 +991,7 @@ version = "0.10.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "caa0f4137e1c0a72f4c651489402276c8e8e1cf081f3b0ba156d2cbeef09e86a" dependencies = [ - "rand_core 0.10.1", + "rand_core", ] [[package]] @@ -1856,26 +1817,6 @@ dependencies = [ "synstructure", ] -[[package]] -name = "zerocopy" -version = "0.8.57" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d35102a9f36d089ccae9e4c6802bc118be4487b80aaffc0ab4e0cf5ce92d2873" -dependencies = [ - "zerocopy-derive", -] - -[[package]] -name = "zerocopy-derive" -version = "0.8.57" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "146c01f5ab44258da43cf276c74a2763db2ff3969c9c652c3f2de07041d0b2bc" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.119", -] - [[package]] name = "zerofrom" version = "0.1.8" diff --git a/Cargo.toml b/Cargo.toml index 2732000..ca4de12 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -88,7 +88,7 @@ flate2 = { version = "1", default-features = false, features = ["rust_backend"] tar = { version = "0.4", default-features = false } sha2 = "0.10" base64 = "0.22" -rand = "0.8" +rand = "0.10" open = "5" dirs = "5" diff --git a/src/auth.rs b/src/auth.rs index 735e1f7..164dfe4 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1,6 +1,5 @@ use anyhow::{anyhow, Context, Result}; use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; -use rand::Rng; use serde_json::{json, Value}; use sha2::{Digest, Sha256}; use std::io::{IsTerminal, Read, Write}; @@ -1373,7 +1372,11 @@ fn describe(wait: Duration) -> String { } fn generate_pkce() -> (String, String) { - let verifier_bytes: [u8; 32] = rand::thread_rng().gen(); + // `rand::random` draws from `ThreadRng`, which `rand` declares + // `TryCryptoRng` — a CSPRNG, which is the only kind a PKCE verifier may + // come from. `rand 0.8`'s `thread_rng().gen()` gave the same guarantee; + // the rename is all that changed. + let verifier_bytes: [u8; 32] = rand::random(); let verifier = URL_SAFE_NO_PAD.encode(verifier_bytes); let challenge = URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes())); (verifier, challenge) @@ -1763,7 +1766,10 @@ pub fn login(debug: bool, profile: Option<&str>, mode: Mode) -> Result<()> { let registration = register_client(&redirect_uri, debug, &scopes)?; let (code_verifier, code_challenge) = generate_pkce(); - let state: String = URL_SAFE_NO_PAD.encode(rand::thread_rng().gen::<[u8; 16]>()); + // Same generator as the verifier above, and for the same reason: `state` + // is what ties the redirect back to this run, so a guessable one is a + // CSRF hole rather than a cosmetic flaw. + let state: String = URL_SAFE_NO_PAD.encode(rand::random::<[u8; 16]>()); let auth_url = format!( "{}?client_id={}&redirect_uri={}&response_type=code&scope={}&state={}&code_challenge={}&code_challenge_method=S256", @@ -2884,4 +2890,84 @@ mod tests { std::fs::remove_dir_all(&dir).unwrap(); } + + /// RFC 7636 §4.1: the verifier is 43–128 characters from the unreserved + /// set, and §4.2: the challenge is `BASE64URL(SHA256(ASCII(verifier)))`. + /// + /// Untested until the `rand 0.8 → 0.10` migration, which is how it came to + /// be written: the two calls that produce these values changed, the suite + /// had nothing to say about it, and "it compiles" is not the standard this + /// particular pair of strings should be held to. + #[test] + fn the_pkce_pair_satisfies_rfc_7636() { + let (verifier, challenge) = generate_pkce(); + + // 32 bytes, base64url without padding. + assert_eq!(verifier.len(), 43, "verifier: {verifier}"); + assert!( + (43..=128).contains(&verifier.len()), + "RFC 7636 allows 43 to 128 characters" + ); + assert_eq!(challenge.len(), 43, "challenge: {challenge}"); + + // The challenge has to be the hash *of the encoded verifier's ASCII*, + // not of the raw bytes behind it — getting that wrong yields a pair + // the authorization server rejects with nothing to say why. + let expected = URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes())); + assert_eq!(challenge, expected); + } + + /// Both strings go into a URL's query without further escaping, so the + /// alphabet is part of the contract: a `+`, `/` or `=` would arrive + /// meaning something else. + #[test] + fn the_pkce_pair_is_url_safe() { + let (verifier, challenge) = generate_pkce(); + let unreserved = |c: char| c.is_ascii_alphanumeric() || c == '-' || c == '_'; + + assert!(verifier.chars().all(unreserved), "verifier: {verifier}"); + assert!(challenge.chars().all(unreserved), "challenge: {challenge}"); + } + + /// **The property a botched migration would break.** A generator swapped + /// for a fixed seed, or a constant, still compiles and still produces a + /// well-formed pair of the right length — and every login would share one + /// verifier. Distinctness across calls is the cheapest thing that notices. + #[test] + fn every_pkce_pair_is_different() { + let pairs: Vec<(String, String)> = (0..16).map(|_| generate_pkce()).collect(); + + let verifiers: std::collections::BTreeSet<&str> = + pairs.iter().map(|(v, _)| v.as_str()).collect(); + assert_eq!(verifiers.len(), pairs.len(), "a verifier repeated"); + + let challenges: std::collections::BTreeSet<&str> = + pairs.iter().map(|(_, c)| c.as_str()).collect(); + assert_eq!(challenges.len(), pairs.len(), "a challenge repeated"); + } + + /// The `state` parameter is generated the same way and for a stronger + /// reason — it is what ties a redirect back to this run, so a predictable + /// one is a CSRF hole. Generated inline rather than in a function, so this + /// asserts the generator rather than the call site. + #[test] + fn the_oauth_state_is_random_and_url_safe() { + let states: Vec = (0..16) + .map(|_| URL_SAFE_NO_PAD.encode(rand::random::<[u8; 16]>())) + .collect(); + + for state in &states { + assert_eq!(state.len(), 22, "16 bytes, base64url unpadded: {state}"); + assert!( + state + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_'), + "{state}" + ); + } + + let distinct: std::collections::BTreeSet<&str> = + states.iter().map(String::as_str).collect(); + assert_eq!(distinct.len(), states.len(), "a state repeated"); + } }