From 068a925ae03906ace135b34c2ec58b5db60ddea6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 12 Nov 2026 01:35:00 +0000 Subject: [PATCH 1/4] feat(stack-profile): make SecretBuf public with JSON helpers stack-auth serialises access keys, JWTs and tokens with serde_json's to_vec and to_string. Those grow a plain buffer as they write, and each outgrown copy is freed without being wiped. SecretBuf already avoids this for profile files. Make it public with a small surface (new, Default, io::Write, into_zeroizing, json and json_string) so stack-auth and the bindings can serialise secrets through it too. The raw accessors stay crate-private. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7rT5sXddtm3TRZtNXa --- packages/stack-profile/src/lib.rs | 1 + packages/stack-profile/src/secret_buf.rs | 126 ++++++++++++++++++++++- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/packages/stack-profile/src/lib.rs b/packages/stack-profile/src/lib.rs index bbf6c98d5..fb719a8e3 100644 --- a/packages/stack-profile/src/lib.rs +++ b/packages/stack-profile/src/lib.rs @@ -72,6 +72,7 @@ pub use device_identity::DeviceIdentity; pub use diagnostic::ErrorPayload; pub use error::ProfileError; pub use profile_store::{FileLockGuard, ProfileStore}; +pub use secret_buf::SecretBuf; /// A type that can be stored in a profile directory. pub trait ProfileData: Serialize + DeserializeOwned { diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index cce52a53f..b5a8362eb 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -27,13 +27,32 @@ const MIN_READ_SPARE: usize = 512; /// one, which [`Zeroizing`] wipes. Spare capacity is wiped too, since /// `Vec`'s `Zeroize` covers the whole allocation. /// -/// Writing to it ([`io::Write`](std::io::Write)) and reading into it -/// ([`read_from`](Self::read_from)) both write straight into the buffer, so -/// no intermediate copy of the bytes is left behind. -pub(crate) struct SecretBuf(Zeroizing>); +/// Writing to it ([`io::Write`](std::io::Write)) writes straight into the +/// buffer, so no intermediate copy of the bytes is left behind. +/// +/// Use it wherever a secret is serialised: `serde_json::to_string` and +/// `to_vec` grow a plain buffer as they go, freeing a partial copy of the +/// secret at each step. +/// +/// ``` +/// use stack_profile::SecretBuf; +/// +/// let body = SecretBuf::json(&serde_json::json!({ "token": "s3cret" }))?; +/// // `body` is wiped when it is dropped. +/// assert_eq!(body.as_slice(), br#"{"token":"s3cret"}"#); +/// # Ok::<(), serde_json::Error>(()) +/// ``` +pub struct SecretBuf(Zeroizing>); + +impl Default for SecretBuf { + fn default() -> Self { + Self::new() + } +} impl SecretBuf { - pub(crate) fn new() -> Self { + /// An empty buffer with room for a typical profile file or token. + pub fn new() -> Self { Self::with_capacity(INITIAL_CAPACITY) } @@ -47,6 +66,13 @@ impl SecretBuf { /// hint to avoid reallocating, and reads through no unwiped scratch /// buffer: a file whose metadata under-reports its length (as some /// network and FUSE filesystems do) still leaves no copy behind. + /// + /// # Errors + /// + /// Returns the reader's error, other than + /// [`Interrupted`](std::io::ErrorKind::Interrupted), which is retried, or + /// [`OutOfMemory`](std::io::ErrorKind::OutOfMemory) when the buffer cannot + /// grow. The bytes read so far are wiped either way. pub(crate) fn read_from(mut reader: impl Read) -> std::io::Result { let mut buf = Self::new(); loop { @@ -75,6 +101,53 @@ impl SecretBuf { &self.0 } + /// The buffer itself, still wiped on drop. + /// + /// The allocation is handed over, not copied. Do not grow the returned + /// `Vec`: it is an ordinary `Vec` again, and growing it leaves an unwiped + /// copy behind. + pub fn into_zeroizing(self) -> Zeroizing> { + self.0 + } + + /// `value` as JSON, serialised straight into a `SecretBuf`. + /// + /// Use this, not `serde_json::to_vec` or `to_string`, for any value that + /// holds a secret: those grow a plain buffer as they write and free each + /// outgrown copy unwiped. + /// + /// # Errors + /// + /// Returns the serialiser's error. The bytes written so far are wiped. + pub fn json(value: &T) -> serde_json::Result>> { + let mut buf = Self::new(); + serde_json::to_writer(&mut buf, value)?; + Ok(buf.into_zeroizing()) + } + + /// [`json`](Self::json) as a `String`, for an API that takes one. + /// + /// The bytes are moved into the `String`, not copied. Take it out with + /// `std::mem::take(&mut *json)` when it must be handed over by value. + /// + /// # Errors + /// + /// Returns the serialiser's error. The bytes written so far are wiped. + pub fn json_string( + value: &T, + ) -> serde_json::Result> { + let mut bytes = Self::json(value)?; + match String::from_utf8(std::mem::take(&mut *bytes)) { + Ok(json) => Ok(Zeroizing::new(json)), + // serde_json only writes UTF-8, so this is unreachable; the bytes + // are still wiped if it ever is not. + Err(e) => { + drop(Zeroizing::new(e.into_bytes())); + Err(serde::ser::Error::custom("serialised JSON was not UTF-8")) + } + } + } + /// Make room for `additional` more bytes without letting the `Vec` /// reallocate in place. fn reserve(&mut self, additional: usize) -> std::io::Result<()> { @@ -151,6 +224,21 @@ mod tests { (0..=255).cycle().take(len).collect() } + #[test] + fn json_serialises_into_the_buffer() { + let value = serde_json::json!({ "token": "s3cret" }); + assert_eq!( + SecretBuf::json(&value).unwrap().as_slice(), + br#"{"token":"s3cret"}"#, + "the bytes must be the JSON of the value", + ); + assert_eq!( + SecretBuf::json_string(&value).unwrap().as_str(), + r#"{"token":"s3cret"}"#, + "the string must be the JSON of the value", + ); + } + #[test] fn writes_hold_exactly_what_was_written() { let mut buf = SecretBuf::new(); @@ -269,4 +357,32 @@ mod tests { let err = SecretBuf::read_from(Failing).err().unwrap(); assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); } + + #[test] + fn serialised_json_comes_out_byte_for_byte() { + // Large enough that the buffer grows several times on the way. + let secret = "s".repeat(20_000); + let mut buf = SecretBuf::new(); + serde_json::to_writer(&mut buf, &serde_json::json!({ "token": secret })).unwrap(); + + let expected = serde_json::to_vec(&serde_json::json!({ "token": secret })).unwrap(); + assert_eq!(buf.as_bytes(), expected.as_slice()); + assert_eq!(buf.into_zeroizing().as_slice(), expected.as_slice()); + } + + /// `into_zeroizing` hands over the buffer itself. A copy would leave the + /// original allocation behind for `SecretBuf`'s own drop to wipe, but the copy + /// would be an exact-size `Vec` the caller might grow, which is what the type + /// exists to prevent. + #[test] + fn into_zeroizing_moves_the_allocation_rather_than_copying_it() { + let mut buf = SecretBuf::new(); + buf.write_all(b"bearer-token").unwrap(); + let before = buf.as_bytes().as_ptr(); + + let bytes = buf.into_zeroizing(); + + assert_eq!(bytes.as_ptr(), before); + assert_eq!(bytes.as_slice(), b"bearer-token"); + } } From d482498ef7375f28765007d94a7a854cf27c6ab5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 12 Nov 2026 01:35:48 +0000 Subject: [PATCH 2/4] fix(stack-auth): serialise saved tokens and request bodies via SecretBuf serde_json's to_string and to_vec, and serde_urlencoded's to_string, grow a plain buffer as they write. Each outgrown copy of the token, access key, OIDC JWT or refresh token is freed without being wiped, so partial copies are left on the heap. Serialise all of them through stack_profile::SecretBuf instead: InMemoryTokenStore, TokenStoreFn, the Node and WASM binding save callbacks, post_json and the device client. A form body is written into a String reserved at its worst-case size up front, so it never grows. The mutants exclusion for TokenStoreFn's wasm32 load and save moves to their new line numbers. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7rT5sXddtm3TRZtNXa --- .cargo/mutants.toml | 2 +- ...th-authorise-expiry-cap-and-body-wiping.md | 9 ++ Cargo.lock | 2 + languages/golang/auth/guest/Cargo.lock | 1 + languages/golang/encrypt/guest/Cargo.lock | 1 + languages/typescript/packages/auth/src/lib.rs | 8 +- .../packages/stack-auth-wasm/Cargo.toml | 2 + .../packages/stack-auth-wasm/src/lib.rs | 4 +- packages/stack-auth/Cargo.toml | 3 + packages/stack-auth/fuzz/Cargo.lock | 1 + packages/stack-auth/src/device_client.rs | 4 +- packages/stack-auth/src/token_store.rs | 26 +++-- packages/stack-auth/src/transport.rs | 101 ++++++++++++++++-- packages/stack-encrypt/fuzz/Cargo.lock | 1 + packages/stack-kms/fuzz/Cargo.lock | 1 + 15 files changed, 145 insertions(+), 21 deletions(-) create mode 100644 .changeset/auth-authorise-expiry-cap-and-body-wiping.md diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml index 6cbd58933..27b3cb59a 100644 --- a/.cargo/mutants.toml +++ b/.cargo/mutants.toml @@ -63,7 +63,7 @@ exclude_re = [ # `a_builder_without_a_transport_is_refused_when_there_is_no_bundled_one`. 'stack-auth/src/error\.rs:\d+:\d+: replace RequestError::is_no_transport -> bool with false$', 'stack-auth/src/auto_strategy\.rs:150:9: replace AutoStrategy::detect_inner -> Result with Ok\(Default::default\(\)\)$', - 'stack-auth/src/token_store\.rs:(261|269):9: replace >::(load|save)', + 'stack-auth/src/token_store\.rs:(273|281):9: replace >::(load|save)', # stack-auth — equivalent. 'stack-auth/src/(access_key|oidc)_refresher\.rs:\d+:\d+: replace )?>::try_credential -> Option with Some\(Default::default\(\)\)$', 'stack-auth/src/error\.rs:\d+:\d+: replace AuthErrorKind::payload -> serde_json::Map with Default::default\(\)$', diff --git a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md new file mode 100644 index 000000000..ca0e6f37d --- /dev/null +++ b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md @@ -0,0 +1,9 @@ +--- +'@cipherstash/auth': patch +--- + +Token JSON saved by `InMemoryTokenStore`, callback token stores (including the +`loadToken`/`saveToken` callbacks of the Node and WASM builds), and the request +bodies sent to CTS (access keys, OIDC JWTs, refresh tokens), are now +serialised into a buffer that wipes itself as it grows, so no partial copies +are left in memory. diff --git a/Cargo.lock b/Cargo.lock index 554876239..d95f26510 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3244,6 +3244,7 @@ dependencies = [ "base64 0.22.1", "bytes", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "mocktail", @@ -3305,6 +3306,7 @@ dependencies = [ "serde-wasm-bindgen", "serde_json", "stack-auth", + "stack-profile", "wasm-bindgen", "wasm-bindgen-futures", "wasm-bindgen-test", diff --git a/languages/golang/auth/guest/Cargo.lock b/languages/golang/auth/guest/Cargo.lock index ba6588228..6e631a8cf 100644 --- a/languages/golang/auth/guest/Cargo.lock +++ b/languages/golang/auth/guest/Cargo.lock @@ -1785,6 +1785,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/languages/golang/encrypt/guest/Cargo.lock b/languages/golang/encrypt/guest/Cargo.lock index 46b22fa35..7670c5dfa 100644 --- a/languages/golang/encrypt/guest/Cargo.lock +++ b/languages/golang/encrypt/guest/Cargo.lock @@ -2002,6 +2002,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/languages/typescript/packages/auth/src/lib.rs b/languages/typescript/packages/auth/src/lib.rs index 3e5dff27b..c34fca1f1 100644 --- a/languages/typescript/packages/auth/src/lib.rs +++ b/languages/typescript/packages/auth/src/lib.rs @@ -296,12 +296,16 @@ impl TokenStore for NapiTokenStore { } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token).map(Zeroizing::new) else { + // Serialise into a `SecretBuf`, which wipes every buffer it + // outgrows, then move the bytes into the `String` the JS callback + // takes. + let Ok(mut json) = stack_profile::SecretBuf::json_string(token) else { return; }; + let json = std::mem::take(&mut *json); let (tx, rx) = oneshot::channel::>(); let status = self.save.call_with_return_value( - json.to_string(), + json, ThreadsafeFunctionCallMode::NonBlocking, move |promise: Promise<()>| { let _ = tx.send(promise); diff --git a/languages/typescript/packages/stack-auth-wasm/Cargo.toml b/languages/typescript/packages/stack-auth-wasm/Cargo.toml index edeed0d6d..329808f1a 100644 --- a/languages/typescript/packages/stack-auth-wasm/Cargo.toml +++ b/languages/typescript/packages/stack-auth-wasm/Cargo.toml @@ -14,6 +14,8 @@ crate-type = ["cdylib", "rlib"] [dependencies] stack-auth = { workspace = true, features = ["http"] } +# `SecretBuf`, to serialise saved tokens without leaving unwiped copies. +stack-profile = { workspace = true } cts-common = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/languages/typescript/packages/stack-auth-wasm/src/lib.rs b/languages/typescript/packages/stack-auth-wasm/src/lib.rs index 68e4d8272..5153b05f9 100644 --- a/languages/typescript/packages/stack-auth-wasm/src/lib.rs +++ b/languages/typescript/packages/stack-auth-wasm/src/lib.rs @@ -154,7 +154,9 @@ impl TokenStore for JsTokenStore { } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token).map(Zeroizing::new) else { + // Serialise into a `SecretBuf`, which wipes every buffer it + // outgrows, and the result when it is dropped. + let Ok(json) = stack_profile::SecretBuf::json_string(token) else { return; }; let promise = match self.save.call1(&JsValue::NULL, &JsValue::from_str(&json)) { diff --git a/packages/stack-auth/Cargo.toml b/packages/stack-auth/Cargo.toml index 3fae08b09..01078bccc 100644 --- a/packages/stack-auth/Cargo.toml +++ b/packages/stack-auth/Cargo.toml @@ -28,6 +28,9 @@ serde_json = { workspace = true } # `.form()` does (it uses this crate), so the bundled transport and a host's # own transport send byte-identical requests. serde_urlencoded = "0.7" +# `serde_urlencoded` only writes into a `form_urlencoded::Serializer`, which +# `transport::form_body` builds itself so it can size the target up front. +form_urlencoded = "1.2" # `OidcFederationStrategy` keys its per-user token cache on the SHA-256 of # the provider JWT, and stamps that digest on the tokens it persists. sha2 = "0.10" diff --git a/packages/stack-auth/fuzz/Cargo.lock b/packages/stack-auth/fuzz/Cargo.lock index 699f876fa..15e6b787f 100644 --- a/packages/stack-auth/fuzz/Cargo.lock +++ b/packages/stack-auth/fuzz/Cargo.lock @@ -2600,6 +2600,7 @@ dependencies = [ "base64", "bytes", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/packages/stack-auth/src/device_client.rs b/packages/stack-auth/src/device_client.rs index 7490c1cf6..e131b7e73 100644 --- a/packages/stack-auth/src/device_client.rs +++ b/packages/stack-auth/src/device_client.rs @@ -120,9 +120,7 @@ pub async fn bind_client_device(store: &ProfileStore) -> Result<(), DeviceClient let url = zerokms_url.join(CreateClientRequest::ENDPOINT)?; // Provisioning is native-only, so it always uses the bundled transport. - let body = zeroize::Zeroizing::new( - serde_json::to_vec(&request).map_err(|e| RequestError(Box::new(e)))?, - ); + let body = transport::json_body(&request)?; let response = transport::post( &transport::share(ReqwestTransport::default()), url, diff --git a/packages/stack-auth/src/token_store.rs b/packages/stack-auth/src/token_store.rs index 99c2547df..ad9933bec 100644 --- a/packages/stack-auth/src/token_store.rs +++ b/packages/stack-auth/src/token_store.rs @@ -55,6 +55,7 @@ let strategy = AccessKeyStrategy::builder(crn, key) use std::future::Future; use std::sync::Arc; +use stack_profile::SecretBuf; use tokio::sync::Mutex; use zeroize::Zeroizing; @@ -137,12 +138,15 @@ impl TokenStore for NoStore { /// /// Internally stores the JSON-serialised form of the token wrapped in /// [`Zeroizing`] so the buffer is wiped on overwrite and on store drop. The -/// [`SecretToken`](crate::SecretToken) wrapped inside [`Token`] is +/// JSON is written into a [`SecretBuf`], so no partial copy is left behind +/// while it is serialised either. +/// +/// The [`SecretToken`](crate::SecretToken) wrapped inside [`Token`] is /// [`ZeroizeOnDrop`](zeroize::ZeroizeOnDrop), so we deliberately don't clone /// the in-memory `Token` value — round-tripping through serde gives us a /// fresh `SecretToken` on each `load` without violating that invariant. pub struct InMemoryTokenStore { - state: Mutex>>, + state: Mutex>>>, } impl InMemoryTokenStore { @@ -164,19 +168,27 @@ impl TokenStore for InMemoryTokenStore { async fn load(&self) -> Option { let guard = self.state.lock().await; let json = guard.as_ref()?; - serde_json::from_str(json).ok() + serde_json::from_slice(json).ok() } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Ok(json) = SecretBuf::json(token) else { tracing::warn!("InMemoryTokenStore: failed to serialise token"); return; }; let mut guard = self.state.lock().await; - *guard = Some(Zeroizing::new(json)); + *guard = Some(json); } } +/// `token` as an owned JSON `String` for a `save` closure, moved rather than +/// copied out of the buffer it was serialised into. +fn token_json_string(token: &Token) -> Option { + SecretBuf::json_string(token) + .ok() + .map(|mut json| std::mem::take(&mut *json)) +} + /// [`TokenStore`] backed by user-supplied `load` and `save` async closures. /// /// This is the *persistence layer* primitive — it plugs into an existing @@ -241,7 +253,7 @@ where } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Some(json) = token_json_string(token) else { tracing::warn!("TokenStoreFn: failed to serialise token"); return; }; @@ -266,7 +278,7 @@ where } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Some(json) = token_json_string(token) else { tracing::warn!("TokenStoreFn: failed to serialise token"); return; }; diff --git a/packages/stack-auth/src/transport.rs b/packages/stack-auth/src/transport.rs index d0dd7fead..5e05b9f18 100644 --- a/packages/stack-auth/src/transport.rs +++ b/packages/stack-auth/src/transport.rs @@ -25,6 +25,7 @@ use std::fmt; use std::future::Future; use std::sync::{Arc, OnceLock}; +use stack_profile::SecretBuf; use url::Url; use zeroize::Zeroizing; @@ -343,27 +344,78 @@ pub(crate) async fn post_json( url: Url, body: &B, ) -> Result { - let body = Zeroizing::new(serde_json::to_vec(body).map_err(|e| RequestError(Box::new(e)))?); + let body = json_body(body)?; post(transport, url, "application/json", Vec::new(), body).await } +/// `body` as JSON in a [`SecretBuf`]. +/// +/// Request bodies carry access keys, OIDC JWTs and refresh tokens, and a +/// `SecretBuf` leaves no unwiped partial copy of them on the heap. +pub(crate) fn json_body(body: &B) -> Result>, RequestError> { + SecretBuf::json(body).map_err(|e| RequestError(Box::new(e))) +} + +/// An [`io::Write`](std::io::Write) that counts the bytes written to it and +/// keeps none of them. +struct ByteCount(usize); + +impl std::io::Write for ByteCount { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0 = self.0.saturating_add(buf.len()); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +/// `body` form-encoded, into a buffer that never grows. +/// +/// `serde_urlencoded` can only write into a `String`, which would free each +/// outgrown buffer unwiped, the same leak [`json_body`] avoids. So the +/// `String` is given its worst-case size up front. The JSON form of the body +/// holds every key and value character at least once, and form-encoding turns +/// a byte into at most three (`%XX`), with one separator per key and value +/// that JSON's own quotes more than pay for. Three times the JSON length is +/// therefore always enough. The JSON is only measured, never stored, so it +/// leaves no copy either. +fn form_body(body: &B) -> Result>, RequestError> { + let mut json_len = ByteCount(0); + serde_json::to_writer(&mut json_len, body).map_err(|e| RequestError(Box::new(e)))?; + let bound = json_len.0.saturating_mul(3); + let mut form = Zeroizing::new(String::new()); + form.try_reserve_exact(bound) + .map_err(|e| RequestError(Box::new(e)))?; + let capacity = form.capacity(); + { + let mut encoder = form_urlencoded::Serializer::new(&mut *form); + let _ = body + .serialize(serde_urlencoded::Serializer::new(&mut encoder)) + .map_err(|e| RequestError(Box::new(e)))?; + } + debug_assert_eq!( + form.capacity(), + capacity, + "the form body outgrew its reserved size, leaving an unwiped copy" + ); + // `take` moves the bytes out, leaving an empty `String` behind to drop. + Ok(Zeroizing::new(std::mem::take(&mut *form).into_bytes())) +} + /// `POST` a form (`application/x-www-form-urlencoded`) body. pub(crate) async fn post_form( transport: &SharedTransport, url: Url, body: &B, ) -> Result { - let body = Zeroizing::new( - serde_urlencoded::to_string(body) - .map_err(|e| RequestError(Box::new(e)))? - .into_bytes(), - ); post( transport, url, "application/x-www-form-urlencoded", Vec::new(), - body, + form_body(body)?, ) .await } @@ -741,6 +793,41 @@ mod tests { } } + /// A form body whose every byte needs percent-encoding is the worst case + /// for the buffer [`form_body`] reserves up front: each byte becomes three. + /// The bytes must match `serde_urlencoded` exactly, and (checked by the + /// `debug_assert` inside) the buffer must never have had to grow, since a + /// grown `String` frees its old allocation unwiped. + #[test] + fn form_body_encodes_like_serde_urlencoded_without_growing() { + let secret: String = "é&=+ /\u{1F600}%".repeat(500); + let body = [ + ("grant_type", "refresh_token"), + ("refresh_token", secret.as_str()), + ("", ""), + ]; + + let encoded = form_body(&body).unwrap(); + + assert_eq!( + encoded.as_slice(), + serde_urlencoded::to_string(body).unwrap().as_bytes() + ); + } + + /// `post_json` sends exactly what `serde_json` writes, however long. + #[tokio::test] + async fn post_json_sends_the_serialised_body() { + let stub = Arc::new(Stub::replying(200, "{}")); + let transport: SharedTransport = stub.clone(); + let body = serde_json::json!({ "accessKey": "k".repeat(20_000) }); + + post_json(&transport, base_url(), &body).await.unwrap(); + + let (_, _, _, sent) = seen(&stub); + assert_eq!(sent, serde_json::to_vec(&body).unwrap()); + } + #[test] fn the_user_agent_names_the_crate_version_and_platform() { assert_eq!(user_agent(), expected_user_agent()); diff --git a/packages/stack-encrypt/fuzz/Cargo.lock b/packages/stack-encrypt/fuzz/Cargo.lock index e2f5a569c..ae0c7f178 100644 --- a/packages/stack-encrypt/fuzz/Cargo.lock +++ b/packages/stack-encrypt/fuzz/Cargo.lock @@ -1957,6 +1957,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/packages/stack-kms/fuzz/Cargo.lock b/packages/stack-kms/fuzz/Cargo.lock index 8c5b3705a..aada3bdff 100644 --- a/packages/stack-kms/fuzz/Cargo.lock +++ b/packages/stack-kms/fuzz/Cargo.lock @@ -1942,6 +1942,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", From 37291a69d9a121b35d25c07b4fd13b9e9655c5ad Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 12 Nov 2026 01:36:05 +0000 Subject: [PATCH 3/4] fix(stack-auth): move the response body out of reqwest's buffer The bundled transport copied the response body with Bytes::to_vec. A response from /api/authorise carries the new access token, and the copy left reqwest's own buffer to be freed without being wiped. Vec::from(Bytes) takes over the allocation when nothing else shares it, so the token is wiped with the HttpResponse instead. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7rT5sXddtm3TRZtNXa --- ...th-authorise-expiry-cap-and-body-wiping.md | 4 +++ packages/stack-auth/src/transport.rs | 25 ++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md index ca0e6f37d..6dd50f3f3 100644 --- a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md +++ b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md @@ -7,3 +7,7 @@ Token JSON saved by `InMemoryTokenStore`, callback token stores (including the bodies sent to CTS (access keys, OIDC JWTs, refresh tokens), are now serialised into a buffer that wipes itself as it grows, so no partial copies are left in memory. + +A response body from CTS, which carries the new access token, is now taken over +from the HTTP client's buffer where that buffer is not shared, rather than always +copied, so it is wiped with the response instead of being freed unwiped. diff --git a/packages/stack-auth/src/transport.rs b/packages/stack-auth/src/transport.rs index 5e05b9f18..fd51ba816 100644 --- a/packages/stack-auth/src/transport.rs +++ b/packages/stack-auth/src/transport.rs @@ -516,11 +516,20 @@ impl HttpTransport for ReqwestTransport { ) }) .collect(); - let body = response.bytes().await?.to_vec(); + let body = response_body(response.bytes().await?); Ok(HttpResponse::new(status, headers, body)) } } +/// The bytes of a response body, moved out of reqwest's buffer rather than +/// copied when nothing else shares it. A minted token arrives in this body, +/// and [`HttpResponse`] wipes its own copy on drop; a copy would free +/// reqwest's buffer, token and all, unwiped. +#[cfg(feature = "http")] +fn response_body(bytes: bytes::Bytes) -> Vec { + Vec::from(bytes) +} + /// The bundled transport, boxed for the strategies. #[cfg(feature = "http")] pub(crate) fn default_transport() -> SharedTransport { @@ -828,6 +837,20 @@ mod tests { assert_eq!(sent, serde_json::to_vec(&body).unwrap()); } + /// The response body takes over reqwest's allocation instead of copying + /// it, so the token in it is wiped with [`HttpResponse`]. + #[test] + #[cfg(feature = "http")] + fn response_body_takes_over_the_buffer_it_is_given() { + let bytes = bytes::Bytes::from(br#"{"accessToken":"secret"}"#.to_vec()); + let ptr = bytes.as_ptr(); + + let body = response_body(bytes); + + assert_eq!(body, br#"{"accessToken":"secret"}"#); + assert_eq!(body.as_ptr(), ptr, "the buffer must be moved, not copied"); + } + #[test] fn the_user_agent_names_the_crate_version_and_platform() { assert_eq!(user_agent(), expected_user_agent()); From 59368db35ce83e81840b3f49833440561e1fdfca Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 12 Nov 2026 01:36:38 +0000 Subject: [PATCH 4/4] fix(stack-auth): cap authorise expiry and distrust over-long tokens CTS /api/authorise returns an absolute expiry, which was used as is. A bad value such as u64::MAX made the token look valid for ever, so AutoRefresh kept sending a dead token until the process restarted. A relative expires_in was already capped at 30 days; the absolute one was not. AuthoriseResponse::into_token(now) now caps the expiry at 30 days from now, through token::cap_expires_at. AutoRefresh also caps every token it refreshes. A token it is built with, or loads from a store, is refreshed before use if it claims more than 30 days: its issue time is unknown, and re-capping it on each load would let a process that restarts often use it for ever. raw_token in the test support now expires in an hour rather than at u64::MAX, which the engine now distrusts. Tests that wait on the engine are bounded with timeouts so a mutant that breaks it fails instead of hanging cargo-mutants. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7rT5sXddtm3TRZtNXa --- ...th-authorise-expiry-cap-and-body-wiping.md | 7 + .../stack-auth/src/access_key_refresher.rs | 34 ++- packages/stack-auth/src/authorize_dto.rs | 59 ++++-- packages/stack-auth/src/auto_refresh.rs | 197 +++++++++++++++++- .../src/auto_refresh/tests/cancellation.rs | 11 + .../src/oidc_federation_strategy.rs | 8 +- packages/stack-auth/src/oidc_refresher.rs | 29 ++- packages/stack-auth/src/test_support.rs | 9 +- packages/stack-auth/src/token.rs | 16 +- 9 files changed, 346 insertions(+), 24 deletions(-) diff --git a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md index 6dd50f3f3..d899f3237 100644 --- a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md +++ b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md @@ -2,6 +2,13 @@ '@cipherstash/auth': patch --- +Cap the access-token expiry from CTS's `/api/authorise` at 30 days from now, +as a relative `expires_in` already is. A token loaded from a token store, or +passed in when the client is built, that claims to last longer than that is +refreshed before it is used. A bad `expiry` value can no longer make a token look +valid for ever, which kept the client sending a dead token until the process +restarted. + Token JSON saved by `InMemoryTokenStore`, callback token stores (including the `loadToken`/`saveToken` callbacks of the Node and WASM builds), and the request bodies sent to CTS (access keys, OIDC JWTs, refresh tokens), are now diff --git a/packages/stack-auth/src/access_key_refresher.rs b/packages/stack-auth/src/access_key_refresher.rs index 5327ceefc..f33880515 100644 --- a/packages/stack-auth/src/access_key_refresher.rs +++ b/packages/stack-auth/src/access_key_refresher.rs @@ -78,8 +78,9 @@ impl Refresher for AccessKeyRefresher { let auth_resp: AuthoriseResponse = resp.json()?; // The response → Token mapping (including the absolute-epoch `expiry` - // handling that CIP-3233 fixed) lives on `From`. - Ok(auth_resp.into()) + // handling that CIP-3233 fixed, and the 30-day cap) lives on + // `AuthoriseResponse::into_token`. + Ok(auth_resp.into_token(crate::token::now_unix_secs())) } } @@ -209,6 +210,35 @@ mod tests { ); } + /// A CTS that answers with an absurd absolute `expiry` (buggy or hostile) + /// must not mint a token that never expires: `AutoRefresh` would then keep + /// sending a dead token until the process restarted. The expiry is capped + /// at 30 days from now, the same cap a relative `expires_in` gets. + #[tokio::test] + async fn access_key_expiry_is_capped_at_thirty_days() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/api/authorise"); + then.json(serde_json::json!({ "accessToken": "tok", "expiry": u64::MAX })); + }); + let server = start_server(mocks).await; + + let refresher = AccessKeyRefresher::new( + SecretToken::new("CSAKid.secret"), + server.url(""), + None, + default_transport(), + ); + let token = refresher.refresh(&()).await.unwrap(); + + assert!( + token.expires_in() <= crate::token::MAX_TOKEN_LIFETIME_SECS, + "expiry must be capped at 30 days; got expires_in = {}", + token.expires_in() + ); + assert!(!token.is_expired(), "a capped token is still a live token"); + } + // ---- Initial auth tests ---- #[tokio::test] diff --git a/packages/stack-auth/src/authorize_dto.rs b/packages/stack-auth/src/authorize_dto.rs index 96719a04e..e40799e2f 100644 --- a/packages/stack-auth/src/authorize_dto.rs +++ b/packages/stack-auth/src/authorize_dto.rs @@ -7,6 +7,7 @@ //! the wire contract lives here in one place. The request bodies differ //! (different credential fields) and stay private to each exchange. +use crate::token::cap_expires_at; use crate::{SecretToken, Token}; /// Success response from `POST /api/authorise`. @@ -17,24 +18,27 @@ pub(crate) struct AuthoriseResponse { pub(crate) expiry: u64, } -/// A `/api/authorise` success response *is* a complete CTS service token. Both -/// the access-key and OIDC federation flows hit the same endpoint and mint the -/// same kind of token, so the response → [`Token`] mapping lives here once -/// rather than being duplicated in each refresher — duplication is exactly how -/// the CIP-3233 expiry bug survived in the OIDC refresher after being fixed for -/// access keys. -impl From for Token { - fn from(resp: AuthoriseResponse) -> Self { +impl AuthoriseResponse { + /// A `/api/authorise` success response *is* a complete CTS service token. + /// Both the access-key and OIDC federation flows hit the same endpoint and + /// mint the same kind of token, so the response → [`Token`] mapping lives + /// here once rather than being duplicated in each refresher — duplication + /// is exactly how the CIP-3233 expiry bug survived in the OIDC refresher + /// after being fixed for access keys. + /// + /// `now` is the current Unix time, used only to cap the server's expiry. + pub(crate) fn into_token(self, now: u64) -> Token { Token { - access_token: resp.access_token, + access_token: self.access_token, token_type: "Bearer".to_string(), // CTS `/api/authorise` returns `expiry` as an ABSOLUTE Unix epoch (it is // the JWT `exp` claim), NOT a relative duration. Adding `now` here would // push the local expiry decades into the future, so `AutoRefresh` would // never consider the token expired and never refresh it — the token would // then silently die at its real (~15 min) `exp` and every request would - // fail until the process restarted. Use the value as-is. See CIP-3233. - expires_at: resp.expiry, + // fail until the process restarted. Use the value as-is (see CIP-3233), + // capped at 30 days from now so an absurd value cannot do the same. + expires_at: cap_expires_at(now, self.expiry), // `/api/authorise` issues no refresh token and carries no region / client // / device metadata; those are populated only by the OAuth device flow. refresh_token: None, @@ -49,6 +53,7 @@ impl From for Token { #[cfg(test)] mod tests { use super::*; + use crate::token::MAX_TOKEN_LIFETIME_SECS; /// The single source of truth for the response → `Token` mapping: CTS /// `expiry` is an ABSOLUTE Unix epoch and must be used verbatim. Pre-fix the @@ -61,7 +66,7 @@ mod tests { expiry: 1_900_000_000, // an absolute Unix epoch, not a duration }; - let token = Token::from(resp); + let token = resp.into_token(1_899_999_100); assert_eq!( token.expires_at(), @@ -75,4 +80,34 @@ mod tests { "/api/authorise issues no refresh token" ); } + + /// An `expiry` more than 30 days after `now` is cut back to `now` plus 30 + /// days, however large it is. + #[test] + fn authorise_response_caps_expiry_at_thirty_days_from_now() { + let now = 1_900_000_000; + for expiry in [now + MAX_TOKEN_LIFETIME_SECS + 1, u64::MAX] { + let resp = AuthoriseResponse { + access_token: SecretToken::new("cts-token"), + expiry, + }; + assert_eq!( + resp.into_token(now).expires_at(), + now + MAX_TOKEN_LIFETIME_SECS + ); + } + } + + /// An `expiry` exactly 30 days out is kept, and so is one already past. + #[test] + fn authorise_response_keeps_an_expiry_within_the_cap() { + let now = 1_900_000_000; + for expiry in [now + MAX_TOKEN_LIFETIME_SECS, now - 1] { + let resp = AuthoriseResponse { + access_token: SecretToken::new("cts-token"), + expiry, + }; + assert_eq!(resp.into_token(now).expires_at(), expiry); + } + } } diff --git a/packages/stack-auth/src/auto_refresh.rs b/packages/stack-auth/src/auto_refresh.rs index a459eb3ef..a5bd97d37 100644 --- a/packages/stack-auth/src/auto_refresh.rs +++ b/packages/stack-auth/src/auto_refresh.rs @@ -256,8 +256,37 @@ impl AutoRefresh { } } +/// A freshly refreshed `token` with its expiry capped at 30 days after `now`. +/// +/// A refreshed token was issued just now, so 30 days from now is the most it +/// can legitimately claim. Tokens minted since `/api/authorise` and +/// `expires_in` were capped satisfy this already; the cap here keeps the +/// engine safe from a [`Refresher`] that does not. +fn capped(mut token: Token, now: u64) -> Token { + token.expires_at = crate::token::cap_expires_at(now, token.expires_at); + token +} + +/// A `token` whose issue time is unknown (the one the engine is built with, +/// or one loaded from a [`TokenStore`]), treated as already expired if it +/// claims to last more than 30 days from `now`. +/// +/// A store can hand back a token saved before the `/api/authorise` cap +/// existed, or one written by whoever can write to the store (a cookie, a KV +/// entry). Capping it at `now + 30 days` would not do: every new engine would +/// cap it afresh, so a process restarting more often than every 30 days would +/// use an `expires_at` of `u64::MAX` for ever. No legitimate token claims more +/// than the cap, so one that does is refreshed before use. +fn distrust_overlong(mut token: Token, now: u64) -> Token { + if crate::token::cap_expires_at(now, token.expires_at) < token.expires_at { + token.expires_at = now; + } + token +} + impl AutoRefresh { fn new(refresher: R, store: S, token: Option, clock: SharedClock) -> Self { + let token = token.map(|t| distrust_overlong(t, clock.now_unix_secs())); Self { shared: Arc::new(RefreshState { refresher, @@ -276,6 +305,12 @@ impl AutoRefresh { Self::new(refresher, store, None, system_clock()) } + // Its only callers are in `expiry_crossing_regression`, which needs `http`. + #[cfg(all(test, feature = "http"))] + pub(crate) fn with_store_and_clock(refresher: R, store: S, clock: SharedClock) -> Self { + Self::new(refresher, store, None, clock) + } + pub(crate) fn refresher(&self) -> &R { &self.shared.refresher } @@ -308,7 +343,7 @@ impl RefreshState { let loaded = self.store.load().await; state = self.state.clone().lock_owned().await; if state.token.is_none() { - state.token = loaded; + state.token = loaded.map(|t| distrust_overlong(t, self.clock.now_unix_secs())); } } @@ -411,7 +446,7 @@ impl RefreshState { /// [`save_refreshed_token`](Self::save_refreshed_token). fn install_refreshed_token(&self, state: &mut State, new_token: Token) -> ServiceToken { let service_token = ServiceToken::new(new_token.access_token().clone()); - state.token = Some(new_token); + state.token = Some(capped(new_token, self.clock.now_unix_secs())); // A success proves whatever previously refused us has changed its mind. state.denial = None; state.last_refresh_error = None; @@ -1935,6 +1970,144 @@ mod expiry_crossing_regression { } } + /// A refresher whose `refresh` returns at once, counting its calls. + fn ready_refresher(clock: &TestClock, calls: &Arc) -> GatedRefresher { + let gate = Arc::new(Notify::new()); + // A stored permit: the first `refresh` passes the gate without waiting. + gate.notify_one(); + GatedRefresher { + started: Arc::new(Notify::new()), + gate, + calls: Arc::clone(calls), + refreshed_expires_at: clock.now() + 3600, + } + } + + use crate::token::MAX_TOKEN_LIFETIME_SECS; + + /// A token that reaches the engine claiming to expire at `u64::MAX` (one + /// saved before the `/api/authorise` cap existed, or written into a + /// caller-writable store) is refreshed before its first use, not used for + /// ever. + #[tokio::test] + async fn an_initial_token_claiming_to_never_expire_is_refreshed_before_use() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let strategy = AutoRefresh::with_token_and_clock( + ready_refresher(&clock, &calls), + make_token("forever-token", u64::MAX), + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// The same applies to a token loaded from a `TokenStore`. + #[tokio::test] + async fn a_stored_token_claiming_to_never_expire_is_refreshed_before_use() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = crate::InMemoryTokenStore::new(); + store.save(&make_token("forever-token", u64::MAX)).await; + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + store, + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// A stored token within the cap is used as is. + #[tokio::test] + async fn a_stored_token_within_thirty_days_is_used_without_refreshing() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = crate::InMemoryTokenStore::new(); + store + .save(&make_token( + "stored-token", + clock.now() + MAX_TOKEN_LIFETIME_SECS, + )) + .await; + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + store, + clock.shared(), + ); + + assert_eq!(strategy.get_token().await.unwrap().as_str(), "stored-token"); + assert_eq!(calls.load(Ordering::SeqCst), 0); + } + + /// A freshly refreshed token is capped at 30 days rather than distrusted: + /// it was issued just now. + #[tokio::test] + async fn a_refreshed_token_claiming_to_never_expire_is_capped_at_thirty_days() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let mut refresher = ready_refresher(&clock, &calls); + refresher.refreshed_expires_at = u64::MAX; + let strategy = AutoRefresh::with_token_and_clock( + refresher, + make_token("expired-token", clock.now()), + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + clock.advance(MAX_TOKEN_LIFETIME_SECS - 1_000); + // Bounded: a second refresh would wait on the gate for ever, and a + // cap that is too short must fail here, not hang the suite. + let token = tokio::time::timeout(std::time::Duration::from_secs(5), strategy.get_token()) + .await + .expect("the refreshed token stays usable within the cap, with no second refresh") + .unwrap(); + assert_eq!(token.as_str(), "refreshed-token"); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// A stored token that claims to outlive the 30-day cap is not trusted + /// again on every load. Re-capping it from each load's "now" would let a + /// process that restarts more often than every 30 days use it for ever, + /// because each fresh engine would see another 30 days left. + #[tokio::test] + async fn restarts_do_not_renew_a_stored_token_that_claims_to_never_expire() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = Arc::new(crate::InMemoryTokenStore::new()); + store.save(&make_token("forever-token", u64::MAX)).await; + + // Three short-lived processes, 20 days apart, sharing the store. + for restart in 0..3 { + let calls_before = calls.load(Ordering::SeqCst); + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + Arc::clone(&store), + clock.shared(), + ); + assert_ne!( + strategy.get_token().await.unwrap().as_str(), + "forever-token", + "restart {restart}: a token claiming to outlive the cap must be refreshed" + ); + assert_eq!(calls.load(Ordering::SeqCst), calls_before + 1); + // Put the over-long token back, as a stale or hostile writer would. + store.save(&make_token("forever-token", u64::MAX)).await; + clock.advance(20 * 24 * 60 * 60); + } + } + #[tokio::test(start_paused = true)] async fn initiating_caller_receives_the_refreshed_token() { for elapsed in [0, 9, 10, 11] { @@ -2113,6 +2286,16 @@ mod expiry_crossing_regression { /// `notified().await`) exists to make correct. #[tokio::test] async fn waiters_get_expired_when_in_flight_refresh_fails() { + // Bounded, so a broken engine fails here instead of hanging the suite. + tokio::time::timeout( + std::time::Duration::from_secs(10), + waiters_get_expired_when_in_flight_refresh_fails_unbounded(), + ) + .await + .expect("the test must finish"); + } + + async fn waiters_get_expired_when_in_flight_refresh_fails_unbounded() { let clock = TestClock::new(1_000_000); let started = Arc::new(Notify::new()); let gate = Arc::new(Notify::new()); @@ -2309,6 +2492,16 @@ mod expiry_crossing_regression { /// every caller except the one that happened to perform it. #[tokio::test] async fn waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal() { + // Bounded, so a broken engine fails here instead of hanging the suite. + tokio::time::timeout( + std::time::Duration::from_secs(10), + waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal_unbounded(), + ) + .await + .expect("the test must finish"); + } + + async fn waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal_unbounded() { let clock = TestClock::new(1_000_000); let started = Arc::new(Notify::new()); let gate = Arc::new(Notify::new()); diff --git a/packages/stack-auth/src/auto_refresh/tests/cancellation.rs b/packages/stack-auth/src/auto_refresh/tests/cancellation.rs index 821a034d1..61fae4451 100644 --- a/packages/stack-auth/src/auto_refresh/tests/cancellation.rs +++ b/packages/stack-auth/src/auto_refresh/tests/cancellation.rs @@ -120,7 +120,18 @@ enum Phase { Install, } +/// [`cancelled_at_unbounded`], bounded (virtual time) so a broken engine fails +/// instead of hanging. async fn cancelled_at(phase: Phase, usable: bool, initial: bool) { + tokio::time::timeout( + Duration::from_secs(60), + cancelled_at_unbounded(phase, usable, initial), + ) + .await + .expect("each phase must finish"); +} + +async fn cancelled_at_unbounded(phase: Phase, usable: bool, initial: bool) { let clock = TestClock::new(1_000); let issuer = Arc::new(Issuer::new(initial)); let store = Arc::new(GatedStore { diff --git a/packages/stack-auth/src/oidc_federation_strategy.rs b/packages/stack-auth/src/oidc_federation_strategy.rs index 177789e53..fba8131c0 100644 --- a/packages/stack-auth/src/oidc_federation_strategy.rs +++ b/packages/stack-auth/src/oidc_federation_strategy.rs @@ -1707,7 +1707,13 @@ mod tests { ); gate.add_permits(1); - let token = second.await.expect("task").expect("the survivor completes"); + // Bounded, so a broken exchange (one that never takes the permit) + // fails here instead of hanging the suite. + let token = tokio::time::timeout(std::time::Duration::from_secs(10), second) + .await + .expect("the survivor finishes") + .expect("task") + .expect("the survivor completes"); assert_eq!(subject(&token), "CS|header.payload.signature"); assert_eq!( strategy.cached_jwts(), diff --git a/packages/stack-auth/src/oidc_refresher.rs b/packages/stack-auth/src/oidc_refresher.rs index 779e89d48..340f1b39c 100644 --- a/packages/stack-auth/src/oidc_refresher.rs +++ b/packages/stack-auth/src/oidc_refresher.rs @@ -197,8 +197,9 @@ impl OidcFederation { let auth_resp: AuthoriseResponse = resp.json()?; // The response → Token mapping (including the absolute-epoch `expiry` - // handling that CIP-3233 fixed) lives on `From`. - Ok(auth_resp.into()) + // handling that CIP-3233 fixed, and the 30-day cap) lives on + // `AuthoriseResponse::into_token`. + Ok(auth_resp.into_token(crate::token::now_unix_secs())) } } @@ -401,6 +402,30 @@ mod tests { ); } + /// The federation step caps an absurd absolute `expiry` at 30 days from + /// now, exactly as the access-key path does: both go through one mapping. + #[tokio::test] + async fn oidc_expiry_is_capped_at_thirty_days() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/api/authorise"); + then.json(serde_json::json!({ "accessToken": "tok", "expiry": u64::MAX })); + }); + let server = start_server(mocks).await; + + let token = federation(&server) + .federate(&SecretToken::new("jwt-0")) + .await + .unwrap(); + + assert!( + token.expires_in() <= crate::token::MAX_TOKEN_LIFETIME_SECS, + "expiry must be capped at 30 days; got expires_in = {}", + token.expires_in() + ); + assert!(!token.is_expired(), "a capped token is still a live token"); + } + /// The federation step sends exactly the JWT it was given, and nothing it /// mints is stamped — stamping is the refresher's job, so a token that /// came straight from the endpoint is not mistaken for a cached one. diff --git a/packages/stack-auth/src/test_support.rs b/packages/stack-auth/src/test_support.rs index ec6b92ba3..80303b24c 100644 --- a/packages/stack-auth/src/test_support.rs +++ b/packages/stack-auth/src/test_support.rs @@ -10,14 +10,15 @@ use cts_common::Crn; use crate::{SecretToken, Token}; -/// A [`Token`] with the given raw access-token string and a far-future expiry, -/// so it reads as valid. The access token need not be a JWT — pass any string to -/// exercise the not-a-JWT paths. +/// A [`Token`] with the given raw access-token string, expiring an hour from +/// now, so it reads as valid. (Not `u64::MAX`: the engine treats a token that +/// claims to outlive the 30-day cap as already expired.) The access token +/// need not be a JWT — pass any string to exercise the not-a-JWT paths. pub(crate) fn raw_token(access_token: &str) -> Token { Token { access_token: SecretToken::new(access_token), token_type: "Bearer".to_string(), - expires_at: u64::MAX, + expires_at: crate::token::now_unix_secs() + 3600, refresh_token: Some(SecretToken::new("refresh-token")), region: None, client_id: None, diff --git a/packages/stack-auth/src/token.rs b/packages/stack-auth/src/token.rs index 02f98b5f8..d6d128d64 100644 --- a/packages/stack-auth/src/token.rs +++ b/packages/stack-auth/src/token.rs @@ -40,12 +40,26 @@ pub(crate) fn expires_at_after(now: u64, expires_in: u64) -> u64 { now.saturating_add(expires_in.min(MAX_TOKEN_LIFETIME_SECS)) } +/// A server-supplied ABSOLUTE expiry (seconds since the Unix epoch), capped +/// at [`MAX_TOKEN_LIFETIME_SECS`] after `now`. +/// +/// The counterpart of [`expires_at_after`] for servers that send an absolute +/// `exp` rather than a lifetime, such as CTS `/api/authorise`. No arithmetic +/// on the server's value is involved, so it cannot overflow, but it is still +/// unbounded: an `expiry` of `u64::MAX` would make the token look valid for +/// ever, so [`AutoRefresh`](crate::auto_refresh::AutoRefresh) would keep +/// sending it after the server stopped accepting it. Capping it means the +/// worst a bad value can do is make the token refresh within 30 days. +pub(crate) fn cap_expires_at(now: u64, expires_at: u64) -> u64 { + expires_at.min(now.saturating_add(MAX_TOKEN_LIFETIME_SECS)) +} + /// The current Unix time in whole seconds, from the system wall clock. /// /// Delegates to [`SystemClock`](crate::clock::SystemClock) so the crate has a /// single definition of "now"; the `*_at` methods take an explicit `now` for /// tests that drive a [`Clock`](crate::clock::Clock). -fn now_unix_secs() -> u64 { +pub(crate) fn now_unix_secs() -> u64 { use crate::clock::{Clock, SystemClock}; SystemClock.now_unix_secs() }