Skip to content

Commit c35f0dc

Browse files
Migrate rand 0.8 to 0.10, and test the two values it generates
**It removes six crates and adds none.** The lock already carried rand 0.10.2, because `quinn-proto` needs it through reqwest — so `rand = "0.8"` was forcing a second, older copy alongside it. Migrating deduplicates onto the version already in the graph and drops 0.8's exclusive subtree: `rand@0.8.8`, `rand_chacha@0.3.1`, `rand_core@0.6.4`, `ppv-lite86@0.2.21`, `zerocopy@0.8.57`, `zerocopy-derive@0.8.57`. 207 packages to 201. The code change is two call sites in `auth.rs`, both renames: `thread_rng()` is now `rng()`, and `Rng::gen` is `random()`. `rand::random` is a free function, so the `use rand::Rng` import goes away rather than becoming `RngExt`. **The randomness guarantee is unchanged and now stated.** Both values — the PKCE verifier and the OAuth `state` — draw from `ThreadRng`, which rand 0.10 declares `TryCryptoRng`, the same CSPRNG guarantee 0.8's `thread_rng` gave. Checked in rand's source rather than assumed, because it is the property these two strings live or die by. **Nothing tested either of them before.** `generate_pkce` had no test at all. Four now: the pair satisfies RFC 7636 §4.1 and §4.2 (43 characters, and the challenge really is `BASE64URL(SHA256(ASCII(verifier)))` rather than a hash of the raw bytes), both strings use only the unreserved alphabet because they go into a query string unescaped, successive pairs differ, and the `state` parameter is covered too. That last one is the one that matters, and it fails for the right reason: replacing the generator with `[7u8; 32]` leaves the length and alphabet tests passing — a constant is perfectly well-formed — and only `every_pkce_pair_is_different` goes red. Ported from mapbox/mapbox-cli-private#139, which cannot merge there now that `oss/` is a submodule (mapbox/mapbox-cli-private#132). It exists because Dependabot tried this by moving the lockfile alone, which fails every `--locked` build while the manifest still says `rand = "0.8"`.
1 parent a3f5e16 commit c35f0dc

4 files changed

Lines changed: 104 additions & 69 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ the Mapbox APIs' own response bodies are not.
5151
`unsupported scheme socks5` in the message is the part that distinguishes
5252
it from the network being down.
5353

54+
- `rand` moved from 0.8 to 0.10. No behaviour changes: the two places it is
55+
used — the PKCE verifier and the OAuth `state` in `mapbox auth login` —
56+
draw from `ThreadRng` before and after, which `rand` declares a CSPRNG, and
57+
`thread_rng().gen()` becoming `random()` is a rename. Recorded because it
58+
is the crate that generates those two values, so a login problem around
59+
this release should be able to find it. Both are now covered by tests
60+
against RFC 7636, which they were not before.
61+
5462
## 0.1.8 - 2026-09-14
5563

5664
Initial beta release. The next release is `0.2.0`.

‎Cargo.lock‎

Lines changed: 6 additions & 65 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ flate2 = { version = "1", default-features = false, features = ["rust_backend"]
8888
tar = { version = "0.4", default-features = false }
8989
sha2 = "0.10"
9090
base64 = "0.22"
91-
rand = "0.8"
91+
rand = "0.10"
9292
open = "5"
9393
dirs = "5"
9494

‎src/auth.rs‎

Lines changed: 89 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
use anyhow::{anyhow, Context, Result};
22
use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _};
3-
use rand::Rng;
43
use serde_json::{json, Value};
54
use sha2::{Digest, Sha256};
65
use std::io::{IsTerminal, Read, Write};
@@ -1373,7 +1372,11 @@ fn describe(wait: Duration) -> String {
13731372
}
13741373

13751374
fn generate_pkce() -> (String, String) {
1376-
let verifier_bytes: [u8; 32] = rand::thread_rng().gen();
1375+
// `rand::random` draws from `ThreadRng`, which `rand` declares
1376+
// `TryCryptoRng` — a CSPRNG, which is the only kind a PKCE verifier may
1377+
// come from. `rand 0.8`'s `thread_rng().gen()` gave the same guarantee;
1378+
// the rename is all that changed.
1379+
let verifier_bytes: [u8; 32] = rand::random();
13771380
let verifier = URL_SAFE_NO_PAD.encode(verifier_bytes);
13781381
let challenge = URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes()));
13791382
(verifier, challenge)
@@ -1763,7 +1766,10 @@ pub fn login(debug: bool, profile: Option<&str>, mode: Mode) -> Result<()> {
17631766
let registration = register_client(&redirect_uri, debug, &scopes)?;
17641767

17651768
let (code_verifier, code_challenge) = generate_pkce();
1766-
let state: String = URL_SAFE_NO_PAD.encode(rand::thread_rng().gen::<[u8; 16]>());
1769+
// Same generator as the verifier above, and for the same reason: `state`
1770+
// is what ties the redirect back to this run, so a guessable one is a
1771+
// CSRF hole rather than a cosmetic flaw.
1772+
let state: String = URL_SAFE_NO_PAD.encode(rand::random::<[u8; 16]>());
17671773

17681774
let auth_url = format!(
17691775
"{}?client_id={}&redirect_uri={}&response_type=code&scope={}&state={}&code_challenge={}&code_challenge_method=S256",
@@ -2884,4 +2890,84 @@ mod tests {
28842890

28852891
std::fs::remove_dir_all(&dir).unwrap();
28862892
}
2893+
2894+
/// RFC 7636 §4.1: the verifier is 43–128 characters from the unreserved
2895+
/// set, and §4.2: the challenge is `BASE64URL(SHA256(ASCII(verifier)))`.
2896+
///
2897+
/// Untested until the `rand 0.8 → 0.10` migration, which is how it came to
2898+
/// be written: the two calls that produce these values changed, the suite
2899+
/// had nothing to say about it, and "it compiles" is not the standard this
2900+
/// particular pair of strings should be held to.
2901+
#[test]
2902+
fn the_pkce_pair_satisfies_rfc_7636() {
2903+
let (verifier, challenge) = generate_pkce();
2904+
2905+
// 32 bytes, base64url without padding.
2906+
assert_eq!(verifier.len(), 43, "verifier: {verifier}");
2907+
assert!(
2908+
(43..=128).contains(&verifier.len()),
2909+
"RFC 7636 allows 43 to 128 characters"
2910+
);
2911+
assert_eq!(challenge.len(), 43, "challenge: {challenge}");
2912+
2913+
// The challenge has to be the hash *of the encoded verifier's ASCII*,
2914+
// not of the raw bytes behind it — getting that wrong yields a pair
2915+
// the authorization server rejects with nothing to say why.
2916+
let expected = URL_SAFE_NO_PAD.encode(Sha256::digest(verifier.as_bytes()));
2917+
assert_eq!(challenge, expected);
2918+
}
2919+
2920+
/// Both strings go into a URL's query without further escaping, so the
2921+
/// alphabet is part of the contract: a `+`, `/` or `=` would arrive
2922+
/// meaning something else.
2923+
#[test]
2924+
fn the_pkce_pair_is_url_safe() {
2925+
let (verifier, challenge) = generate_pkce();
2926+
let unreserved = |c: char| c.is_ascii_alphanumeric() || c == '-' || c == '_';
2927+
2928+
assert!(verifier.chars().all(unreserved), "verifier: {verifier}");
2929+
assert!(challenge.chars().all(unreserved), "challenge: {challenge}");
2930+
}
2931+
2932+
/// **The property a botched migration would break.** A generator swapped
2933+
/// for a fixed seed, or a constant, still compiles and still produces a
2934+
/// well-formed pair of the right length — and every login would share one
2935+
/// verifier. Distinctness across calls is the cheapest thing that notices.
2936+
#[test]
2937+
fn every_pkce_pair_is_different() {
2938+
let pairs: Vec<(String, String)> = (0..16).map(|_| generate_pkce()).collect();
2939+
2940+
let verifiers: std::collections::BTreeSet<&str> =
2941+
pairs.iter().map(|(v, _)| v.as_str()).collect();
2942+
assert_eq!(verifiers.len(), pairs.len(), "a verifier repeated");
2943+
2944+
let challenges: std::collections::BTreeSet<&str> =
2945+
pairs.iter().map(|(_, c)| c.as_str()).collect();
2946+
assert_eq!(challenges.len(), pairs.len(), "a challenge repeated");
2947+
}
2948+
2949+
/// The `state` parameter is generated the same way and for a stronger
2950+
/// reason — it is what ties a redirect back to this run, so a predictable
2951+
/// one is a CSRF hole. Generated inline rather than in a function, so this
2952+
/// asserts the generator rather than the call site.
2953+
#[test]
2954+
fn the_oauth_state_is_random_and_url_safe() {
2955+
let states: Vec<String> = (0..16)
2956+
.map(|_| URL_SAFE_NO_PAD.encode(rand::random::<[u8; 16]>()))
2957+
.collect();
2958+
2959+
for state in &states {
2960+
assert_eq!(state.len(), 22, "16 bytes, base64url unpadded: {state}");
2961+
assert!(
2962+
state
2963+
.chars()
2964+
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_'),
2965+
"{state}"
2966+
);
2967+
}
2968+
2969+
let distinct: std::collections::BTreeSet<&str> =
2970+
states.iter().map(String::as_str).collect();
2971+
assert_eq!(distinct.len(), states.len(), "a state repeated");
2972+
}
28872973
}

0 commit comments

Comments
 (0)