From a0766497092d4d6042397f80b6c716fbc3d1425c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 10:44:50 +0000 Subject: [PATCH 1/9] fix(stack-auth,stack-profile): clamp server expires_in and wipe secret buffers A server-supplied `expires_in` was added to the clock unchecked. On the token paths `now + expires_in` panics in debug and wraps in release (no overflow-checks), giving an already-expired token and a refresh loop; the device-code deadline `Instant::now() + Duration` panics in every profile. Token lifetimes now go through one helper, `expires_at_after`, which clamps to 30 days and saturates, the rule `is_expired_at` already followed. Device codes are clamped to one hour when received, so the deadline cannot overflow and `PendingDeviceCode::expires_in()` reports what is enforced. The JSON that carries `auth.json` and `secretkey.json` to and from disk was held in plain Strings and freed unwiped, undercutting `ZeroizeOnDrop` on the secrets inside. It is now serialised into a writer that wipes on drop and on every growth (a Zeroizing alone frees its old allocation unwiped when it reallocates), and read through `fs::read`, which sizes the buffer once, into a Zeroizing>. The access key from `CS_CLIENT_ACCESS_KEY` or `with_access_key` is held as Zeroizing until it is parsed into an `AccessKey`. Fixes CIP-5159. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- .changeset/auth-expiry-and-secret-wiping.md | 10 ++ Cargo.lock | 1 + packages/stack-auth/src/auto_strategy.rs | 53 ++++---- packages/stack-auth/src/device_code/mod.rs | 20 +++- packages/stack-auth/src/device_code/tests.rs | 97 +++++++++++++++ packages/stack-auth/src/token.rs | 89 +++++++++++++- packages/stack-profile/Cargo.toml | 3 + packages/stack-profile/src/lib.rs | 1 + packages/stack-profile/src/profile_store.rs | 21 +++- packages/stack-profile/src/secret_buf.rs | 120 +++++++++++++++++++ 10 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 .changeset/auth-expiry-and-secret-wiping.md create mode 100644 packages/stack-profile/src/secret_buf.rs diff --git a/.changeset/auth-expiry-and-secret-wiping.md b/.changeset/auth-expiry-and-secret-wiping.md new file mode 100644 index 0000000000..5cfedddf7e --- /dev/null +++ b/.changeset/auth-expiry-and-secret-wiping.md @@ -0,0 +1,10 @@ +--- +"@cipherstash/auth": patch +--- + +Sign-in and token refresh no longer trust an unbounded `expires_in` from the +auth server. A token lifetime longer than 30 days is treated as 30 days, and a +device code is polled for at most one hour, so an oversized value can no +longer panic the process or wrap to an already-expired token. The JSON that +carries saved tokens and keys to and from `~/.cipherstash`, and an access key +read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. diff --git a/Cargo.lock b/Cargo.lock index 6964dbbf09..5548762390 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3410,6 +3410,7 @@ dependencies = [ "tempfile", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-auth/src/auto_strategy.rs b/packages/stack-auth/src/auto_strategy.rs index 8a28f618f7..4a23b38689 100644 --- a/packages/stack-auth/src/auto_strategy.rs +++ b/packages/stack-auth/src/auto_strategy.rs @@ -10,6 +10,7 @@ use crate::HttpTransport; #[cfg(not(target_arch = "wasm32"))] use crate::Token; use crate::{AuthError, AuthStrategy, ServiceToken}; +use zeroize::Zeroizing; /// An [`AuthStrategy`] that automatically detects available credentials /// and delegates to the appropriate inner strategy. @@ -106,7 +107,7 @@ impl AutoStrategy { /// unreachable (no filesystem) — callers must supply an access key. #[cfg(not(target_arch = "wasm32"))] fn detect_inner( - access_key: Option, + access_key: Option<&str>, crn: Option, store: Option, transport: Option, @@ -143,7 +144,7 @@ impl AutoStrategy { #[cfg(target_arch = "wasm32")] fn detect_inner( - access_key: Option, + access_key: Option<&str>, crn: Option, transport: Option, ) -> Result { @@ -180,7 +181,8 @@ impl AutoStrategy { /// # } /// ``` pub struct AutoStrategyBuilder { - access_key: Option, + /// Wiped on drop: the access key is a long-lived credential. + access_key: Option>, crn: Option, transport: Option, } @@ -195,7 +197,7 @@ impl AutoStrategyBuilder { /// Provide an explicit access key. Takes precedence over env vars. pub fn with_access_key(mut self, access_key: impl Into) -> Self { - self.access_key = Some(access_key.into()); + self.access_key = Some(Zeroizing::new(access_key.into())); self } @@ -214,9 +216,13 @@ impl AutoStrategyBuilder { /// 4. [`AuthError::NotAuthenticated`] pub fn detect(self) -> Result { // Merge explicit values with env vars (explicit wins) - let access_key = self - .access_key - .or_else(|| std::env::var("CS_CLIENT_ACCESS_KEY").ok()); + // Wrapped as soon as it is read, so this copy is wiped on drop. The + // process environment keeps its own copy, which is beyond our reach. + let access_key = self.access_key.or_else(|| { + std::env::var("CS_CLIENT_ACCESS_KEY") + .ok() + .map(Zeroizing::new) + }); let crn = match self.crn { Some(crn) => Some(crn), @@ -238,11 +244,20 @@ impl AutoStrategyBuilder { None } }; - AutoStrategy::detect_inner(access_key, crn, store, self.transport) + AutoStrategy::detect_inner( + access_key.as_ref().map(|key| key.as_str()), + crn, + store, + self.transport, + ) } #[cfg(target_arch = "wasm32")] { - AutoStrategy::detect_inner(access_key, crn, self.transport) + AutoStrategy::detect_inner( + access_key.as_ref().map(|key| key.as_str()), + crn, + self.transport, + ) } } } @@ -319,7 +334,7 @@ mod tests { #[cfg(feature = "http")] // builds a strategy with no transport of its own fn access_key_with_valid_crn() { let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), + Some("CSAKtestKeyId.testKeySecret"), Some(valid_crn()), None, None, @@ -331,24 +346,16 @@ mod tests { #[test] fn access_key_without_crn_returns_missing_workspace_crn() { - let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), - None, - None, - None, - ); + let result = + AutoStrategy::detect_inner(Some("CSAKtestKeyId.testKeySecret"), None, None, None); assert!(matches!(result, Err(AuthError::MissingWorkspaceCrn(_)))); } #[test] fn invalid_access_key_format_returns_invalid_access_key() { - let result = AutoStrategy::detect_inner( - Some("not-a-valid-key".into()), - Some(valid_crn()), - None, - None, - ); + let result = + AutoStrategy::detect_inner(Some("not-a-valid-key"), Some(valid_crn()), None, None); assert!(matches!(result, Err(AuthError::InvalidAccessKey(_)))); } @@ -389,7 +396,7 @@ mod tests { let store = write_token_store(dir.path()); let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), + Some("CSAKtestKeyId.testKeySecret"), Some(valid_crn()), Some(store), None, diff --git a/packages/stack-auth/src/device_code/mod.rs b/packages/stack-auth/src/device_code/mod.rs index eb8f659d41..16fabca9ee 100644 --- a/packages/stack-auth/src/device_code/mod.rs +++ b/packages/stack-auth/src/device_code/mod.rs @@ -18,6 +18,16 @@ use protocol::{ #[cfg(test)] mod tests; +/// The longest device-code lifetime this crate polls for, in seconds (one +/// hour). +/// +/// `expires_in` is server-supplied, and [`PendingDeviceCode::poll_for_token`] +/// adds it to an [`Instant`](tokio::time::Instant), which panics on overflow +/// in every build profile. RFC 8628 device codes live for minutes, so a +/// larger value is clamped rather than trusted: at worst the user restarts +/// sign-in after an hour. +const MAX_DEVICE_CODE_LIFETIME_SECS: u64 = 60 * 60; + // Keep the browser boundary visible to callers of `open_in_browser`. fn launch_browser(uri: &str) -> std::io::Result<()> { #[cfg(test)] @@ -156,7 +166,7 @@ impl DeviceCodeStrategy { user_code: code.user_code, verification_uri: code.verification_uri, verification_uri_complete: code.verification_uri_complete, - expires_in: code.expires_in, + expires_in: code.expires_in.min(MAX_DEVICE_CODE_LIFETIME_SECS), profile_dir: self.profile_dir.clone(), device_identity: self.device_identity.clone(), }) @@ -265,7 +275,8 @@ pub struct PendingDeviceCode { verification_uri: String, /// The full verification URI with the user code pre-filled. verification_uri_complete: String, - /// How many seconds the device code remains valid. + /// How many seconds the device code remains valid, clamped to + /// [`MAX_DEVICE_CODE_LIFETIME_SECS`]. expires_in: u64, /// Profile directory override. Falls back to `~/.cipherstash`. profile_dir: Option, @@ -290,6 +301,9 @@ impl PendingDeviceCode { } /// How many seconds the device code remains valid. + /// + /// This is the server's `expires_in`, capped at one hour: polling stops + /// at that point even if the server claimed a longer lifetime. pub fn expires_in(&self) -> u64 { self.expires_in } @@ -352,7 +366,7 @@ impl PendingDeviceCode { let mut token = Token { access_token: token_resp.access_token, token_type: token_resp.token_type, - expires_at: now + token_resp.expires_in, + expires_at: crate::token::expires_at_after(now, token_resp.expires_in), refresh_token: token_resp.refresh_token, region: None, client_id: None, diff --git a/packages/stack-auth/src/device_code/tests.rs b/packages/stack-auth/src/device_code/tests.rs index c277b0bacd..941b60419e 100644 --- a/packages/stack-auth/src/device_code/tests.rs +++ b/packages/stack-auth/src/device_code/tests.rs @@ -514,6 +514,103 @@ async fn test_poll_for_token_slow_down_increases_interval() { assert!(matches!(err, AuthError::TokenExpired(_))); } +fn device_code_json_expiring_in(expires_in: u64) -> serde_json::Value { + let mut json = device_code_json(); + json["expires_in"] = expires_in.into(); + json +} + +/// A server-supplied device-code lifetime past the cap is clamped, so the +/// poll deadline (`Instant::now() + expires_in`) cannot overflow and panic. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_clamps_an_oversized_device_code_lifetime() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(u64::MAX)); + }); + mocks.mock(|when, then| { + when.post().path("/oauth/device/token"); + then.json(token_json()); + }); + let server = start_server(mocks).await; + + let pending = begin_pending(&server, &dir).await; + assert_eq!( + pending.expires_in(), + 3_600, + "an oversized device-code lifetime must be clamped to one hour" + ); + + let token = pending + .poll_for_token() + .await + .expect("polling must not panic on an oversized device-code lifetime"); + assert!(!token.is_expired()); +} + +/// The clamped deadline is still enforced: once it passes, polling stops +/// with `TokenExpired` rather than running for as long as the server said. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_expires_at_the_clamped_deadline() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(u64::MAX)); + }); + mocks.mock(|when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("authorization_pending")); + }); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); + let elapsed = started.elapsed().as_secs(); + assert!( + (MAX_DEVICE_CODE_LIFETIME_SECS..MAX_DEVICE_CODE_LIFETIME_SECS + 10).contains(&elapsed), + "polling should stop at the one-hour cap; stopped after {elapsed}s" + ); +} + +/// A server-supplied token lifetime past the cap is clamped on the +/// device-code path too, rather than wrapping `now + expires_in` to a +/// timestamp in the past. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_clamps_an_oversized_token_lifetime() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mock_code_endpoint(&mut mocks); + mocks.mock(|when, then| { + when.post().path("/oauth/device/token"); + then.json(serde_json::json!({ + "access_token": test_access_token(), + "token_type": "Bearer", + "expires_in": u64::MAX + })); + }); + let server = start_server(mocks).await; + + let token = begin_pending(&server, &dir) + .await + .poll_for_token() + .await + .unwrap(); + + let max = crate::token::MAX_TOKEN_LIFETIME_SECS; + assert!(!token.is_expired(), "the token must not wrap to expired"); + assert!( + (max - 2..=max).contains(&token.expires_in()), + "token lifetime should be clamped; got {}", + token.expires_in() + ); +} + // ---- ensure_trailing_slash / URL join tests ---- #[test] diff --git a/packages/stack-auth/src/token.rs b/packages/stack-auth/src/token.rs index a66d10499c..02f98b5f8a 100644 --- a/packages/stack-auth/src/token.rs +++ b/packages/stack-auth/src/token.rs @@ -17,6 +17,29 @@ impl stack_profile::ProfileData for Token { /// callers can still use the current token. const EXPIRY_LEEWAY_SECS: u64 = 90; +/// The longest token lifetime this crate accepts from a server, in seconds +/// (30 days). +/// +/// `expires_in` is server-supplied. A larger value is clamped to this rather +/// than trusted: a token that claims to outlive it is refreshed early, which +/// is safe, whereas an unbounded value would overflow `now + expires_in` +/// (see [`expires_at_after`]). +pub(crate) const MAX_TOKEN_LIFETIME_SECS: u64 = 30 * 24 * 60 * 60; + +/// The absolute expiry (seconds since the Unix epoch) of a token issued at +/// `now` with a server-supplied lifetime of `expires_in` seconds. +/// +/// Every place that turns an OAuth `expires_in` into a [`Token::expires_at`] +/// goes through here. The lifetime is clamped to [`MAX_TOKEN_LIFETIME_SECS`] +/// and the addition saturates, so a buggy or hostile server cannot make it +/// panic (debug builds) or wrap to an already-expired timestamp and send the +/// caller into a refresh loop (release builds, which carry no +/// `overflow-checks`). This is the same rule [`Token::is_expired_at`] +/// follows for its leeway. +pub(crate) fn expires_at_after(now: u64, expires_in: u64) -> u64 { + now.saturating_add(expires_in.min(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 @@ -329,7 +352,7 @@ impl Token { Ok(Token { access_token: token_resp.access_token, token_type: token_resp.token_type, - expires_at: now_unix_secs() + token_resp.expires_in, + expires_at: expires_at_after(now_unix_secs(), token_resp.expires_in), refresh_token: token_resp.refresh_token, region: None, client_id: None, @@ -477,6 +500,39 @@ mod tests { ); } + // ---- expires_at_after ---- + + #[test] + fn expires_at_after_passes_an_ordinary_lifetime_through() { + assert_eq!(expires_at_after(1_000, 3_600), 4_600); + } + + #[test] + fn the_token_lifetime_cap_is_thirty_days() { + assert_eq!(MAX_TOKEN_LIFETIME_SECS, 2_592_000); + } + + #[test] + fn expires_at_after_clamps_an_oversized_lifetime() { + assert_eq!( + expires_at_after(1_000, u64::MAX), + 1_000 + MAX_TOKEN_LIFETIME_SECS, + "a server-supplied lifetime past the cap must be clamped, not trusted" + ); + assert_eq!( + expires_at_after(1_000, MAX_TOKEN_LIFETIME_SECS), + 1_000 + MAX_TOKEN_LIFETIME_SECS, + "the cap itself is accepted as-is" + ); + } + + #[test] + fn expires_at_after_saturates_instead_of_wrapping() { + // A wrapped sum would be a timestamp in the past: an already-expired + // token, and so a refresh loop. + assert_eq!(expires_at_after(u64::MAX - 10, 3_600), u64::MAX); + } + // ---- refresh() tests ---- // // Grouped under one feature gate rather than one per item: every helper @@ -536,6 +592,37 @@ mod tests { assert!((3598..=3600).contains(&refreshed.expires_in())); } + #[tokio::test] + async fn test_refresh_clamps_an_oversized_expires_in() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/token"); + then.json(serde_json::json!({ + "access_token": "new-access-token", + "token_type": "Bearer", + "expires_in": u64::MAX, + })); + }); + let server = start_server(mocks).await; + let base_url = server.url(""); + + let refresh_token = SecretToken::new("test-refresh-token"); + let refreshed = Token::refresh(&refresh_token, &base_url, "cli", None) + .await + .expect("an oversized expires_in must not fail or panic the refresh"); + + assert!( + !refreshed.is_expired(), + "the token must not wrap to an already-expired timestamp" + ); + assert!( + (MAX_TOKEN_LIFETIME_SECS - 2..=MAX_TOKEN_LIFETIME_SECS) + .contains(&refreshed.expires_in()), + "lifetime should be clamped to MAX_TOKEN_LIFETIME_SECS; got {}", + refreshed.expires_in() + ); + } + #[tokio::test] async fn test_refresh_invalid_grant() { let mut mocks = MockSet::new(); diff --git a/packages/stack-profile/Cargo.toml b/packages/stack-profile/Cargo.toml index b2b57354e2..fecede5af5 100644 --- a/packages/stack-profile/Cargo.toml +++ b/packages/stack-profile/Cargo.toml @@ -17,6 +17,9 @@ serde = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } uuid = { workspace = true } +# auth.json and secretkey.json hold bearer tokens and keys: the JSON buffers +# that carry them to and from disk are wiped (see `secret_buf`). +zeroize = { workspace = true } # The default device name is the hostname, which only the creating half of # `DeviceIdentity` needs — and that half is native-only: the crate builds for diff --git a/packages/stack-profile/src/lib.rs b/packages/stack-profile/src/lib.rs index 1455102f45..bbf6c98d5a 100644 --- a/packages/stack-profile/src/lib.rs +++ b/packages/stack-profile/src/lib.rs @@ -66,6 +66,7 @@ mod device_identity; pub mod diagnostic; mod error; mod profile_store; +mod secret_buf; pub use device_identity::DeviceIdentity; pub use diagnostic::ErrorPayload; diff --git a/packages/stack-profile/src/profile_store.rs b/packages/stack-profile/src/profile_store.rs index f32b47be9f..097e88940b 100644 --- a/packages/stack-profile/src/profile_store.rs +++ b/packages/stack-profile/src/profile_store.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use serde::de::DeserializeOwned; use serde::Serialize; +use crate::secret_buf::ZeroizingWriter; use crate::{ProfileData, ProfileError}; const CS_CONFIG_PATH_ENV: &str = "CS_CONFIG_PATH"; @@ -291,8 +292,12 @@ impl ProfileStore { Self::validate_filename(filename)?; std::fs::create_dir_all(&self.dir)?; let path = self.dir.join(filename); - let json = serde_json::to_string_pretty(value)?; - Self::write_to_path(&path, &json, _mode) + // The serialised form of a profile can carry secrets (`auth.json`'s + // tokens, `secretkey.json`'s key), so it is built in a buffer that is + // wiped on drop and on every reallocation, never in a plain `String`. + let mut json = ZeroizingWriter::new(); + serde_json::to_writer_pretty(&mut json, value)?; + Self::write_to_path(&path, json.as_bytes(), _mode) } /// Atomically write JSON content to an absolute path, optionally setting @@ -324,7 +329,7 @@ impl ProfileStore { /// processes won't trip this — but to defend against third-party /// readers we retry the rename a handful of times with brief backoff /// before giving up. - fn write_to_path(path: &Path, json: &str, _mode: Option) -> Result<(), ProfileError> { + fn write_to_path(path: &Path, json: &[u8], _mode: Option) -> Result<(), ProfileError> { use std::io::Write; let parent = path.parent().ok_or_else(|| { @@ -363,7 +368,7 @@ impl ProfileStore { } opts.open(&tmp_path)? }; - file.write_all(json.as_bytes())?; + file.write_all(json)?; file.sync_all()?; drop(file); @@ -451,9 +456,13 @@ impl ProfileStore { pub fn load(&self, filename: &str) -> Result { Self::validate_filename(filename)?; let path = self.dir.join(filename); - match std::fs::read_to_string(&path) { + // Read into a buffer wiped on drop: the file may hold secrets (see + // `write`). `fs::read` sizes the buffer from the file's metadata, so + // it is allocated once and never reallocated, which would otherwise + // free an un-wiped copy of the contents. + match std::fs::read(&path).map(zeroize::Zeroizing::new) { Ok(contents) => { - let value: T = serde_json::from_str(&contents)?; + let value: T = serde_json::from_slice(&contents)?; Ok(value) } Err(e) if e.kind() == std::io::ErrorKind::NotFound => { diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs new file mode 100644 index 0000000000..0408dba011 --- /dev/null +++ b/packages/stack-profile/src/secret_buf.rs @@ -0,0 +1,120 @@ +//! A byte buffer for serialising secrets, wiped on drop and on growth. + +use zeroize::Zeroizing; + +/// The capacity a [`ZeroizingWriter`] starts with. Profile files are small +/// (a token file is well under 4 KiB), so most writes never grow the buffer. +const INITIAL_CAPACITY: usize = 4096; + +/// An [`io::Write`](std::io::Write) sink that keeps everything written to it +/// in a buffer that is zeroized when dropped. +/// +/// Wrapping a `Vec` in [`Zeroizing`] alone is not enough for a buffer +/// that grows: when a `Vec` reallocates, it frees the old allocation without +/// wiping it, leaving a copy of the bytes written so far on the heap. This +/// writer never lets the `Vec` reallocate itself. When more room is needed it +/// allocates a larger buffer, copies the contents across, and drops the old +/// one, which [`Zeroizing`] wipes. +pub(crate) struct ZeroizingWriter(Zeroizing>); + +impl ZeroizingWriter { + pub(crate) fn new() -> Self { + Self::with_capacity(INITIAL_CAPACITY) + } + + fn with_capacity(capacity: usize) -> Self { + Self(Zeroizing::new(Vec::with_capacity(capacity))) + } + + /// The bytes written so far. + pub(crate) fn as_bytes(&self) -> &[u8] { + &self.0 + } + + /// Make room for `additional` more bytes without letting the `Vec` + /// reallocate in place. + fn reserve(&mut self, additional: usize) -> std::io::Result<()> { + let needed = self + .0 + .len() + .checked_add(additional) + .ok_or(std::io::ErrorKind::OutOfMemory)?; + if needed <= self.0.capacity() { + return Ok(()); + } + let capacity = needed.max(self.0.capacity().saturating_mul(2)); + let mut grown = Zeroizing::new(Vec::with_capacity(capacity)); + grown.extend_from_slice(&self.0); + // The old buffer is dropped here, and `Zeroizing` wipes it. + self.0 = grown; + Ok(()) + } +} + +impl std::io::Write for ZeroizingWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.reserve(buf.len())?; + self.0.extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use std::io::Write; + + use super::*; + + #[test] + fn it_holds_exactly_what_was_written() { + let mut writer = ZeroizingWriter::new(); + writer.write_all(b"hello, ").unwrap(); + writer.write_all(b"world").unwrap(); + assert_eq!(writer.as_bytes(), b"hello, world"); + } + + #[test] + fn it_grows_past_its_capacity_without_losing_bytes() { + let mut writer = ZeroizingWriter::with_capacity(4); + let payload: Vec = (0..=255).cycle().take(10_000).collect(); + for chunk in payload.chunks(7) { + writer.write_all(chunk).unwrap(); + } + assert_eq!(writer.as_bytes(), payload.as_slice()); + } + + #[test] + fn it_never_lets_the_vec_reallocate_itself() { + // Growth must go through `reserve`, which replaces the buffer, so the + // `Vec` is always written within its existing capacity. If a write + // reallocated in place, the capacity would change between `reserve` + // and the write; asserting that `reserve` always leaves enough room + // pins the invariant. + let mut writer = ZeroizingWriter::with_capacity(1); + for len in [1, 2, 3, 100, 1, 5_000] { + writer.reserve(len).unwrap(); + let capacity = writer.0.capacity(); + writer.write_all(&vec![0xAB; len]).unwrap(); + assert_eq!( + writer.0.capacity(), + capacity, + "a write after reserve must not reallocate" + ); + } + } + + #[test] + fn it_matches_serde_json_pretty_output() { + let value = serde_json::json!({ "access_token": "secret", "expires_at": 1 }); + let mut writer = ZeroizingWriter::with_capacity(1); + serde_json::to_writer_pretty(&mut writer, &value).unwrap(); + assert_eq!( + writer.as_bytes(), + serde_json::to_string_pretty(&value).unwrap().as_bytes() + ); + } +} From 8cc9c40596db14483d4b0981c426b713ac40e9f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 10:47:53 +0000 Subject: [PATCH 2/9] fix(stack-auth,stack-profile): close the gaps review found in secret wiping - Read profile files through `SecretBuf::read_from` rather than `fs::read`. `fs::read` only avoids reallocating when the file's metadata length is right; on a filesystem that under-reports it, the Vec grows and frees unwiped copies, and the final probe read goes through an unwiped stack buffer. `SecretBuf` (renamed from `ZeroizingWriter`) reads straight into its own wiped buffer. - Hold the builder's access key as a `SecretToken`, the crate's existing secret type, rather than a bare `Zeroizing`, so it is also masked in `Debug`. - Log the enforced device-code lifetime next to the server's, so the trace matches what polling does. - Drop a test that called `reserve` itself and so could not catch `write` skipping it; add `read_from` tests for short, interrupted and failed reads. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- .changeset/auth-expiry-and-secret-wiping.md | 4 +- packages/stack-auth/src/auto_strategy.rs | 15 +- packages/stack-auth/src/device_code/mod.rs | 6 +- packages/stack-profile/src/profile_store.rs | 21 ++- packages/stack-profile/src/secret_buf.rs | 168 ++++++++++++++------ 5 files changed, 149 insertions(+), 65 deletions(-) diff --git a/.changeset/auth-expiry-and-secret-wiping.md b/.changeset/auth-expiry-and-secret-wiping.md index 5cfedddf7e..e47df25d79 100644 --- a/.changeset/auth-expiry-and-secret-wiping.md +++ b/.changeset/auth-expiry-and-secret-wiping.md @@ -5,6 +5,6 @@ Sign-in and token refresh no longer trust an unbounded `expires_in` from the auth server. A token lifetime longer than 30 days is treated as 30 days, and a device code is polled for at most one hour, so an oversized value can no -longer panic the process or wrap to an already-expired token. The JSON that -carries saved tokens and keys to and from `~/.cipherstash`, and an access key +longer panic the process or wrap to an already-expired token. The buffers that +carry saved tokens and keys to and from `~/.cipherstash`, and an access key read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. diff --git a/packages/stack-auth/src/auto_strategy.rs b/packages/stack-auth/src/auto_strategy.rs index 4a23b38689..5ce2d3d6a3 100644 --- a/packages/stack-auth/src/auto_strategy.rs +++ b/packages/stack-auth/src/auto_strategy.rs @@ -9,8 +9,7 @@ use crate::transport::{self, SharedTransport}; use crate::HttpTransport; #[cfg(not(target_arch = "wasm32"))] use crate::Token; -use crate::{AuthError, AuthStrategy, ServiceToken}; -use zeroize::Zeroizing; +use crate::{AuthError, AuthStrategy, SecretToken, ServiceToken}; /// An [`AuthStrategy`] that automatically detects available credentials /// and delegates to the appropriate inner strategy. @@ -181,8 +180,8 @@ impl AutoStrategy { /// # } /// ``` pub struct AutoStrategyBuilder { - /// Wiped on drop: the access key is a long-lived credential. - access_key: Option>, + /// A [`SecretToken`] so the key is wiped on drop and masked in `Debug`. + access_key: Option, crn: Option, transport: Option, } @@ -197,7 +196,7 @@ impl AutoStrategyBuilder { /// Provide an explicit access key. Takes precedence over env vars. pub fn with_access_key(mut self, access_key: impl Into) -> Self { - self.access_key = Some(Zeroizing::new(access_key.into())); + self.access_key = Some(SecretToken::new(access_key)); self } @@ -221,7 +220,7 @@ impl AutoStrategyBuilder { let access_key = self.access_key.or_else(|| { std::env::var("CS_CLIENT_ACCESS_KEY") .ok() - .map(Zeroizing::new) + .map(SecretToken::new) }); let crn = match self.crn { @@ -245,7 +244,7 @@ impl AutoStrategyBuilder { } }; AutoStrategy::detect_inner( - access_key.as_ref().map(|key| key.as_str()), + access_key.as_ref().map(SecretToken::as_str), crn, store, self.transport, @@ -254,7 +253,7 @@ impl AutoStrategyBuilder { #[cfg(target_arch = "wasm32")] { AutoStrategy::detect_inner( - access_key.as_ref().map(|key| key.as_str()), + access_key.as_ref().map(SecretToken::as_str), crn, self.transport, ) diff --git a/packages/stack-auth/src/device_code/mod.rs b/packages/stack-auth/src/device_code/mod.rs index 16fabca9ee..1d16b77c25 100644 --- a/packages/stack-auth/src/device_code/mod.rs +++ b/packages/stack-auth/src/device_code/mod.rs @@ -152,9 +152,11 @@ impl DeviceCodeStrategy { let token_url = self.base_url.join("oauth/device/token")?; + let expires_in = code.expires_in.min(MAX_DEVICE_CODE_LIFETIME_SECS); tracing::debug!( user_code = %code.user_code, - expires_in = code.expires_in, + expires_in, + server_expires_in = code.expires_in, "device code received" ); @@ -166,7 +168,7 @@ impl DeviceCodeStrategy { user_code: code.user_code, verification_uri: code.verification_uri, verification_uri_complete: code.verification_uri_complete, - expires_in: code.expires_in.min(MAX_DEVICE_CODE_LIFETIME_SECS), + expires_in, profile_dir: self.profile_dir.clone(), device_identity: self.device_identity.clone(), }) diff --git a/packages/stack-profile/src/profile_store.rs b/packages/stack-profile/src/profile_store.rs index 097e88940b..a5fb5882ff 100644 --- a/packages/stack-profile/src/profile_store.rs +++ b/packages/stack-profile/src/profile_store.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use serde::de::DeserializeOwned; use serde::Serialize; -use crate::secret_buf::ZeroizingWriter; +use crate::secret_buf::SecretBuf; use crate::{ProfileData, ProfileError}; const CS_CONFIG_PATH_ENV: &str = "CS_CONFIG_PATH"; @@ -295,7 +295,7 @@ impl ProfileStore { // The serialised form of a profile can carry secrets (`auth.json`'s // tokens, `secretkey.json`'s key), so it is built in a buffer that is // wiped on drop and on every reallocation, never in a plain `String`. - let mut json = ZeroizingWriter::new(); + let mut json = SecretBuf::new(); serde_json::to_writer_pretty(&mut json, value)?; Self::write_to_path(&path, json.as_bytes(), _mode) } @@ -456,13 +456,18 @@ impl ProfileStore { pub fn load(&self, filename: &str) -> Result { Self::validate_filename(filename)?; let path = self.dir.join(filename); - // Read into a buffer wiped on drop: the file may hold secrets (see - // `write`). `fs::read` sizes the buffer from the file's metadata, so - // it is allocated once and never reallocated, which would otherwise - // free an un-wiped copy of the contents. - match std::fs::read(&path).map(zeroize::Zeroizing::new) { + // Read into a buffer wiped on drop and on growth: the file may hold + // secrets (see `write`). `fs::read` would not do: it reallocates, and + // frees an unwiped copy, whenever the file's metadata under-reports + // its length. + // + // serde_json still decodes a string containing escape sequences + // through a scratch buffer of its own, which it does not wipe. Tokens + // and keys are base64 or JWTs, which need no escaping, so that path + // is not taken for them. + match std::fs::File::open(&path).and_then(SecretBuf::read_from) { Ok(contents) => { - let value: T = serde_json::from_slice(&contents)?; + let value: T = serde_json::from_slice(contents.as_bytes())?; Ok(value) } Err(e) if e.kind() == std::io::ErrorKind::NotFound => { diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index 0408dba011..5c18c08f28 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -1,23 +1,33 @@ -//! A byte buffer for serialising secrets, wiped on drop and on growth. +//! A byte buffer for secrets that is wiped on drop and whenever it grows. + +use std::io::Read; use zeroize::Zeroizing; -/// The capacity a [`ZeroizingWriter`] starts with. Profile files are small -/// (a token file is well under 4 KiB), so most writes never grow the buffer. +/// The capacity a [`SecretBuf`] starts with. Profile files are small (a token +/// file is well under 4 KiB), so most buffers never grow. const INITIAL_CAPACITY: usize = 4096; -/// An [`io::Write`](std::io::Write) sink that keeps everything written to it -/// in a buffer that is zeroized when dropped. +/// How much room [`SecretBuf::read_from`] makes for each read. +const READ_CHUNK: usize = 4096; + +/// A byte buffer that is zeroized when dropped, including every allocation it +/// outgrows. /// /// Wrapping a `Vec` in [`Zeroizing`] alone is not enough for a buffer /// that grows: when a `Vec` reallocates, it frees the old allocation without -/// wiping it, leaving a copy of the bytes written so far on the heap. This -/// writer never lets the `Vec` reallocate itself. When more room is needed it +/// wiping it, leaving a copy of the bytes so far on the heap. `SecretBuf` +/// never lets its `Vec` reallocate itself. When it needs more room it /// allocates a larger buffer, copies the contents across, and drops the old -/// one, which [`Zeroizing`] wipes. -pub(crate) struct ZeroizingWriter(Zeroizing>); +/// 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>); -impl ZeroizingWriter { +impl SecretBuf { pub(crate) fn new() -> Self { Self::with_capacity(INITIAL_CAPACITY) } @@ -26,7 +36,31 @@ impl ZeroizingWriter { Self(Zeroizing::new(Vec::with_capacity(capacity))) } - /// The bytes written so far. + /// Read `reader` to its end. + /// + /// Unlike [`Read::read_to_end`], this does not depend on a correct size + /// 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. + pub(crate) fn read_from(mut reader: impl Read) -> std::io::Result { + let mut buf = Self::new(); + loop { + buf.reserve(READ_CHUNK)?; + let filled = buf.0.len(); + // Within capacity, so this does not reallocate. + buf.0.resize(filled + READ_CHUNK, 0); + let result = reader.read(&mut buf.0[filled..]); + buf.0.truncate(filled + *result.as_ref().unwrap_or(&0)); + match result { + Ok(0) => return Ok(buf), + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::Interrupted => {} + Err(e) => return Err(e), + } + } + } + + /// The bytes held so far. pub(crate) fn as_bytes(&self) -> &[u8] { &self.0 } @@ -51,7 +85,7 @@ impl ZeroizingWriter { } } -impl std::io::Write for ZeroizingWriter { +impl std::io::Write for SecretBuf { fn write(&mut self, buf: &[u8]) -> std::io::Result { self.reserve(buf.len())?; self.0.extend_from_slice(buf); @@ -69,52 +103,96 @@ mod tests { use super::*; - #[test] - fn it_holds_exactly_what_was_written() { - let mut writer = ZeroizingWriter::new(); - writer.write_all(b"hello, ").unwrap(); - writer.write_all(b"world").unwrap(); - assert_eq!(writer.as_bytes(), b"hello, world"); + /// Yields its bytes a few at a time, with an `Interrupted` in between, + /// the way a slow or signalled file read can. + struct Trickle { + bytes: Vec, + pos: usize, + interrupt_next: bool, } - #[test] - fn it_grows_past_its_capacity_without_losing_bytes() { - let mut writer = ZeroizingWriter::with_capacity(4); - let payload: Vec = (0..=255).cycle().take(10_000).collect(); - for chunk in payload.chunks(7) { - writer.write_all(chunk).unwrap(); + impl Read for Trickle { + fn read(&mut self, out: &mut [u8]) -> std::io::Result { + self.interrupt_next = !self.interrupt_next; + if self.interrupt_next { + return Err(std::io::ErrorKind::Interrupted.into()); + } + let n = out.len().min(3).min(self.bytes.len() - self.pos); + out[..n].copy_from_slice(&self.bytes[self.pos..self.pos + n]); + self.pos += n; + Ok(n) + } + } + + struct Failing; + + impl Read for Failing { + fn read(&mut self, _: &mut [u8]) -> std::io::Result { + Err(std::io::ErrorKind::PermissionDenied.into()) } - assert_eq!(writer.as_bytes(), payload.as_slice()); + } + + fn payload(len: usize) -> Vec { + (0..=255).cycle().take(len).collect() } #[test] - fn it_never_lets_the_vec_reallocate_itself() { - // Growth must go through `reserve`, which replaces the buffer, so the - // `Vec` is always written within its existing capacity. If a write - // reallocated in place, the capacity would change between `reserve` - // and the write; asserting that `reserve` always leaves enough room - // pins the invariant. - let mut writer = ZeroizingWriter::with_capacity(1); - for len in [1, 2, 3, 100, 1, 5_000] { - writer.reserve(len).unwrap(); - let capacity = writer.0.capacity(); - writer.write_all(&vec![0xAB; len]).unwrap(); - assert_eq!( - writer.0.capacity(), - capacity, - "a write after reserve must not reallocate" - ); + fn writes_hold_exactly_what_was_written() { + let mut buf = SecretBuf::new(); + buf.write_all(b"hello, ").unwrap(); + buf.write_all(b"world").unwrap(); + assert_eq!(buf.as_bytes(), b"hello, world"); + } + + #[test] + fn writes_grow_past_the_capacity_without_losing_bytes() { + let mut buf = SecretBuf::with_capacity(4); + let payload = payload(10_000); + for chunk in payload.chunks(7) { + buf.write_all(chunk).unwrap(); } + assert_eq!(buf.as_bytes(), payload.as_slice()); } #[test] - fn it_matches_serde_json_pretty_output() { + fn writes_match_serde_json_pretty_output() { let value = serde_json::json!({ "access_token": "secret", "expires_at": 1 }); - let mut writer = ZeroizingWriter::with_capacity(1); - serde_json::to_writer_pretty(&mut writer, &value).unwrap(); + let mut buf = SecretBuf::with_capacity(1); + serde_json::to_writer_pretty(&mut buf, &value).unwrap(); assert_eq!( - writer.as_bytes(), + buf.as_bytes(), serde_json::to_string_pretty(&value).unwrap().as_bytes() ); } + + #[test] + fn read_from_reads_everything_across_many_growths() { + let payload = payload(3 * INITIAL_CAPACITY + 17); + let buf = SecretBuf::read_from(payload.as_slice()).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + } + + #[test] + fn read_from_reads_an_empty_reader() { + let buf = SecretBuf::read_from(&[][..]).unwrap(); + assert!(buf.as_bytes().is_empty()); + } + + #[test] + fn read_from_retries_interrupted_and_short_reads() { + let payload = payload(50); + let reader = Trickle { + bytes: payload.clone(), + pos: 0, + interrupt_next: false, + }; + let buf = SecretBuf::read_from(reader).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + } + + #[test] + fn read_from_returns_a_read_error() { + let err = SecretBuf::read_from(Failing).err().unwrap(); + assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); + } } From dd043af4b8f8e93ad83e2dffb0cfb1486a21f760 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 10:58:04 +0000 Subject: [PATCH 3/9] chore(deps): record stack-profile's zeroize dependency in detached lockfiles stack-profile gained a `zeroize` dependency, and six detached Cargo workspaces resolve it by path: both Go guests, EQL and the three fuzz crates. The Go guest job checks its lockfiles with --locked and failed on the stale entry. Each lockfile gains only the one dependency line. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- languages/golang/auth/guest/Cargo.lock | 1 + languages/golang/encrypt/guest/Cargo.lock | 1 + packages/eql/Cargo.lock | 1 + packages/stack-auth/fuzz/Cargo.lock | 1 + packages/stack-encrypt/fuzz/Cargo.lock | 1 + packages/stack-kms/fuzz/Cargo.lock | 1 + 6 files changed, 6 insertions(+) diff --git a/languages/golang/auth/guest/Cargo.lock b/languages/golang/auth/guest/Cargo.lock index 970b1cd538..ba6588228c 100644 --- a/languages/golang/auth/guest/Cargo.lock +++ b/languages/golang/auth/guest/Cargo.lock @@ -1847,6 +1847,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/languages/golang/encrypt/guest/Cargo.lock b/languages/golang/encrypt/guest/Cargo.lock index 7f542b6ded..46b22fa357 100644 --- a/languages/golang/encrypt/guest/Cargo.lock +++ b/languages/golang/encrypt/guest/Cargo.lock @@ -2126,6 +2126,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/eql/Cargo.lock b/packages/eql/Cargo.lock index 5c90c00475..2eda5f8a01 100644 --- a/packages/eql/Cargo.lock +++ b/packages/eql/Cargo.lock @@ -4205,6 +4205,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-auth/fuzz/Cargo.lock b/packages/stack-auth/fuzz/Cargo.lock index 99bbb9c36a..699f876fac 100644 --- a/packages/stack-auth/fuzz/Cargo.lock +++ b/packages/stack-auth/fuzz/Cargo.lock @@ -2643,6 +2643,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-encrypt/fuzz/Cargo.lock b/packages/stack-encrypt/fuzz/Cargo.lock index 1663a68989..e2f5a569cc 100644 --- a/packages/stack-encrypt/fuzz/Cargo.lock +++ b/packages/stack-encrypt/fuzz/Cargo.lock @@ -2058,6 +2058,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-kms/fuzz/Cargo.lock b/packages/stack-kms/fuzz/Cargo.lock index a18c2eba9f..8c5b3705a6 100644 --- a/packages/stack-kms/fuzz/Cargo.lock +++ b/packages/stack-kms/fuzz/Cargo.lock @@ -2009,6 +2009,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] From 5c1a11b87bbc1fb4f6d0f4d411269a891483ebdf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 01:31:54 +0000 Subject: [PATCH 4/9] fix(stack-profile): pin the non-UTF-8 error and skip growth on small reads Review follow-ups on #1553. - Reading through `SecretBuf` + `from_slice` leaves UTF-8 validation to serde_json, so a non-UTF-8 profile file is now `ProfileError::Json` rather than `ProfileError::Io` with `InvalidData`. Pin that with a test and say so in the changeset. - `read_from` grew the buffer for every read, so even a 2 KB profile grew to 8 KiB for the end-of-file probe. It now grows only when less than 512 bytes are spare, and reads into all the spare capacity it has. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- .changeset/auth-expiry-and-secret-wiping.md | 4 ++- packages/stack-profile/src/profile_store.rs | 16 ++++++++++++ packages/stack-profile/src/secret_buf.rs | 28 ++++++++++++++++++--- 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/.changeset/auth-expiry-and-secret-wiping.md b/.changeset/auth-expiry-and-secret-wiping.md index e47df25d79..7b32ef7e8f 100644 --- a/.changeset/auth-expiry-and-secret-wiping.md +++ b/.changeset/auth-expiry-and-secret-wiping.md @@ -7,4 +7,6 @@ auth server. A token lifetime longer than 30 days is treated as 30 days, and a device code is polled for at most one hour, so an oversized value can no longer panic the process or wrap to an already-expired token. The buffers that carry saved tokens and keys to and from `~/.cipherstash`, and an access key -read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. +read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. A +saved profile file that is not valid UTF-8 is now reported as malformed JSON, +like any other unreadable profile, rather than as an I/O error. diff --git a/packages/stack-profile/src/profile_store.rs b/packages/stack-profile/src/profile_store.rs index a5fb5882ff..9dac887373 100644 --- a/packages/stack-profile/src/profile_store.rs +++ b/packages/stack-profile/src/profile_store.rs @@ -635,6 +635,22 @@ mod tests { assert!(matches!(err, ProfileError::NotFound { .. })); } + #[test] + fn load_reports_a_non_utf8_file_as_a_json_error() { + // Reading through `SecretBuf` leaves UTF-8 validation to serde_json, + // so a file of invalid bytes is malformed JSON, like any other + // unparseable file, rather than an I/O error. + let dir = tempfile::tempdir().unwrap(); + let store = ProfileStore::new(dir.path()); + std::fs::write(dir.path().join("data.json"), b"{\"name\": \"\xff\"}").unwrap(); + + let err = store.load::("data.json").unwrap_err(); + assert!( + matches!(err, ProfileError::Json(_)), + "a non-UTF-8 file should be a JSON error; got {err:?}" + ); + } + #[test] fn clear_removes_existing_file() { let dir = tempfile::tempdir().unwrap(); diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index 5c18c08f28..a3096aa09f 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -8,9 +8,14 @@ use zeroize::Zeroizing; /// file is well under 4 KiB), so most buffers never grow. const INITIAL_CAPACITY: usize = 4096; -/// How much room [`SecretBuf::read_from`] makes for each read. +/// How much room [`SecretBuf::read_from`] makes when it grows the buffer. const READ_CHUNK: usize = 4096; +/// The least spare room [`SecretBuf::read_from`] reads into before it grows +/// the buffer. A file that fits in the initial capacity with this much to +/// spare is read, end-of-file probe included, without growing. +const MIN_READ_SPARE: usize = 512; + /// A byte buffer that is zeroized when dropped, including every allocation it /// outgrows. /// @@ -45,10 +50,13 @@ impl SecretBuf { pub(crate) fn read_from(mut reader: impl Read) -> std::io::Result { let mut buf = Self::new(); loop { - buf.reserve(READ_CHUNK)?; + if buf.0.capacity() - buf.0.len() < MIN_READ_SPARE { + buf.reserve(READ_CHUNK)?; + } let filled = buf.0.len(); - // Within capacity, so this does not reallocate. - buf.0.resize(filled + READ_CHUNK, 0); + // Exactly up to capacity, so this does not reallocate. + let capacity = buf.0.capacity(); + buf.0.resize(capacity, 0); let result = reader.read(&mut buf.0[filled..]); buf.0.truncate(filled + *result.as_ref().unwrap_or(&0)); match result { @@ -172,6 +180,18 @@ mod tests { assert_eq!(buf.as_bytes(), payload.as_slice()); } + #[test] + fn read_from_does_not_grow_for_a_file_that_fits() { + let payload = payload(2_000); + let buf = SecretBuf::read_from(payload.as_slice()).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + assert_eq!( + buf.0.capacity(), + INITIAL_CAPACITY, + "a profile-sized file should be read without growing the buffer" + ); + } + #[test] fn read_from_reads_an_empty_reader() { let buf = SecretBuf::read_from(&[][..]).unwrap(); From 0565c5eb439e6ec45777403f8e3c73e9f3b6bdce Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 02:18:24 +0000 Subject: [PATCH 5/9] fix(stack-auth,stack-profile): enforce the device-code deadline and validate UTF-8 Review follow-ups on #1553. - The device-code deadline is now fixed when `begin` receives the code and stored on `PendingDeviceCode`. Before, it was computed when polling started, so a caller that waited before polling got a fresh window. The deadline is enforced across sleeps (`sleep_until` the earlier of the next poll and the deadline) and across in-flight requests (`timeout_at`), so polling stops at the deadline and a success that arrives after it is not saved. - `load` validates the whole file as UTF-8 before parsing. `from_slice` skips the bytes of an ignored field, so it accepted invalid UTF-8 in an unknown field. An invalid file is again `ProfileError::Io` with `InvalidData`, as it was under `read_to_string`, which reverts the `Json` mapping the previous commit introduced. - `SecretBuf::reserve` uses `try_reserve_exact` rather than `Vec::with_capacity`, so an oversized input returns an error instead of aborting the process. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- .changeset/auth-expiry-and-secret-wiping.md | 6 +-- packages/stack-auth/src/device_code/mod.rs | 46 +++++++++++------ packages/stack-auth/src/device_code/tests.rs | 54 ++++++++++++++++++++ packages/stack-profile/src/profile_store.rs | 38 ++++++++++---- packages/stack-profile/src/secret_buf.rs | 16 +++++- 5 files changed, 132 insertions(+), 28 deletions(-) diff --git a/.changeset/auth-expiry-and-secret-wiping.md b/.changeset/auth-expiry-and-secret-wiping.md index 7b32ef7e8f..7146e61144 100644 --- a/.changeset/auth-expiry-and-secret-wiping.md +++ b/.changeset/auth-expiry-and-secret-wiping.md @@ -7,6 +7,6 @@ auth server. A token lifetime longer than 30 days is treated as 30 days, and a device code is polled for at most one hour, so an oversized value can no longer panic the process or wrap to an already-expired token. The buffers that carry saved tokens and keys to and from `~/.cipherstash`, and an access key -read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. A -saved profile file that is not valid UTF-8 is now reported as malformed JSON, -like any other unreadable profile, rather than as an I/O error. +read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. +A device code's lifetime now runs from when it was issued, and polling stops +at that deadline rather than at the next poll after it. diff --git a/packages/stack-auth/src/device_code/mod.rs b/packages/stack-auth/src/device_code/mod.rs index 1d16b77c25..d343cb1022 100644 --- a/packages/stack-auth/src/device_code/mod.rs +++ b/packages/stack-auth/src/device_code/mod.rs @@ -169,6 +169,10 @@ impl DeviceCodeStrategy { verification_uri: code.verification_uri, verification_uri_complete: code.verification_uri_complete, expires_in, + // Fixed now, when the code is issued, so a caller that waits + // before polling does not get a fresh window. `expires_in` is + // clamped above, so this cannot overflow. + deadline: tokio::time::Instant::now() + tokio::time::Duration::from_secs(expires_in), profile_dir: self.profile_dir.clone(), device_identity: self.device_identity.clone(), }) @@ -280,6 +284,9 @@ pub struct PendingDeviceCode { /// How many seconds the device code remains valid, clamped to /// [`MAX_DEVICE_CODE_LIFETIME_SECS`]. expires_in: u64, + /// When the device code expires: when it was received plus `expires_in`. + /// Polling stops here, including mid-request and mid-sleep. + deadline: tokio::time::Instant, /// Profile directory override. Falls back to `~/.cipherstash`. profile_dir: Option, /// Device identity to associate with the token. @@ -302,10 +309,12 @@ impl PendingDeviceCode { &self.verification_uri_complete } - /// How many seconds the device code remains valid. + /// How many seconds the device code was valid for when it was issued. /// - /// This is the server's `expires_in`, capped at one hour: polling stops - /// at that point even if the server claimed a longer lifetime. + /// This is the server's `expires_in`, capped at one hour. The lifetime + /// runs from when [`begin`](DeviceCodeStrategy::begin) received the + /// code, not from when polling starts: polling stops at that point even + /// if the server claimed a longer lifetime. pub fn expires_in(&self) -> u64 { self.expires_in } @@ -332,8 +341,7 @@ impl PendingDeviceCode { pub async fn poll_for_token(self) -> Result { let transport = bundled_transport(); let mut interval = tokio::time::Duration::from_secs(5); - let deadline = - tokio::time::Instant::now() + tokio::time::Duration::from_secs(self.expires_in); + let deadline = self.deadline; tracing::debug!( url = %self.token_url, @@ -347,16 +355,22 @@ impl PendingDeviceCode { return Err(AuthError::TokenExpired(crate::error::TokenExpired)); } - let resp = transport::post_form( - &transport, - self.token_url.clone(), - &TokenRequest { - client_id: &self.client_id, - device_code: &self.device_code, - grant_type: "urn:ietf:params:oauth:grant-type:device_code", - }, + // A response that arrives after the deadline is dropped rather + // than saved: the code has expired, whatever the server says. + let resp = tokio::time::timeout_at( + deadline, + transport::post_form( + &transport, + self.token_url.clone(), + &TokenRequest { + client_id: &self.client_id, + device_code: &self.device_code, + grant_type: "urn:ietf:params:oauth:grant-type:device_code", + }, + ), ) - .await?; + .await + .map_err(|_| AuthError::TokenExpired(crate::error::TokenExpired))??; if resp.is_success() { tracing::debug!("token received"); @@ -433,7 +447,9 @@ impl PendingDeviceCode { } } - tokio::time::sleep(interval).await; + // Wake at the deadline rather than sleeping past it; the check at + // the top of the loop then reports the expiry. + tokio::time::sleep_until(deadline.min(tokio::time::Instant::now() + interval)).await; } } } diff --git a/packages/stack-auth/src/device_code/tests.rs b/packages/stack-auth/src/device_code/tests.rs index 941b60419e..bd6e41008f 100644 --- a/packages/stack-auth/src/device_code/tests.rs +++ b/packages/stack-auth/src/device_code/tests.rs @@ -578,6 +578,60 @@ async fn test_poll_for_token_expires_at_the_clamped_deadline() { ); } +fn mock_pending_forever(mocks: &mut MockSet, expires_in: u64) { + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(expires_in)); + }); + mocks.mock(|when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("authorization_pending")); + }); +} + +/// The lifetime runs from when the code was received, not from when +/// polling starts: a caller that waits past it gets no fresh window. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_counts_the_lifetime_from_begin() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mock_pending_forever(&mut mocks, 900); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + + tokio::time::advance(tokio::time::Duration::from_secs(901)).await; + let started = tokio::time::Instant::now(); + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); + assert_eq!( + started.elapsed().as_secs(), + 0, + "an already-expired code must not be polled for another window" + ); +} + +/// Polling stops at the deadline itself, not at the first poll after it. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_does_not_sleep_past_the_deadline() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + // Polls at T=0 and T=5; the next would be T=10, past the 7s deadline. + mock_pending_forever(&mut mocks, 7); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); + assert_eq!( + started.elapsed().as_secs(), + 7, + "polling should stop at the deadline, not sleep past it" + ); +} + /// A server-supplied token lifetime past the cap is clamped on the /// device-code path too, rather than wrapping `now + expires_in` to a /// timestamp in the past. diff --git a/packages/stack-profile/src/profile_store.rs b/packages/stack-profile/src/profile_store.rs index 9dac887373..9ea23583df 100644 --- a/packages/stack-profile/src/profile_store.rs +++ b/packages/stack-profile/src/profile_store.rs @@ -467,7 +467,17 @@ impl ProfileStore { // is not taken for them. match std::fs::File::open(&path).and_then(SecretBuf::read_from) { Ok(contents) => { - let value: T = serde_json::from_slice(contents.as_bytes())?; + // Validate the whole file as UTF-8 first, as `read_to_string` + // did: `from_slice` skips the bytes of fields it ignores, so + // it would accept invalid UTF-8 in an unknown field. This + // borrows the buffer, so it makes no copy. + let text = std::str::from_utf8(contents.as_bytes()).map_err(|_| { + ProfileError::Io(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "profile file is not valid UTF-8", + )) + })?; + let value: T = serde_json::from_str(text)?; Ok(value) } Err(e) if e.kind() == std::io::ErrorKind::NotFound => { @@ -635,19 +645,29 @@ mod tests { assert!(matches!(err, ProfileError::NotFound { .. })); } - #[test] - fn load_reports_a_non_utf8_file_as_a_json_error() { - // Reading through `SecretBuf` leaves UTF-8 validation to serde_json, - // so a file of invalid bytes is malformed JSON, like any other - // unparseable file, rather than an I/O error. + fn assert_invalid_utf8_is_an_io_error(contents: &[u8]) { let dir = tempfile::tempdir().unwrap(); let store = ProfileStore::new(dir.path()); - std::fs::write(dir.path().join("data.json"), b"{\"name\": \"\xff\"}").unwrap(); + std::fs::write(dir.path().join("data.json"), contents).unwrap(); let err = store.load::("data.json").unwrap_err(); assert!( - matches!(err, ProfileError::Json(_)), - "a non-UTF-8 file should be a JSON error; got {err:?}" + matches!(&err, ProfileError::Io(e) if e.kind() == std::io::ErrorKind::InvalidData), + "a non-UTF-8 file should be an InvalidData I/O error, as before; got {err:?}" + ); + } + + #[test] + fn load_rejects_invalid_utf8_in_a_known_field() { + assert_invalid_utf8_is_an_io_error(b"{\"name\": \"\xff\", \"value\": 1}"); + } + + #[test] + fn load_rejects_invalid_utf8_in_an_unknown_field() { + // serde_json skips an ignored field's bytes without validating them, + // so this file would load if UTF-8 were left to the parser. + assert_invalid_utf8_is_an_io_error( + b"{\"name\": \"ok\", \"value\": 1, \"extra\": \"\xff\"}", ); } diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index a3096aa09f..0adc35fc48 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -85,7 +85,12 @@ impl SecretBuf { return Ok(()); } let capacity = needed.max(self.0.capacity().saturating_mul(2)); - let mut grown = Zeroizing::new(Vec::with_capacity(capacity)); + // `try_reserve_exact`, not `Vec::with_capacity`: an oversized input + // must come back as an error, not abort the process. + let mut grown = Zeroizing::new(Vec::new()); + grown + .try_reserve_exact(capacity) + .map_err(|_| std::io::ErrorKind::OutOfMemory)?; grown.extend_from_slice(&self.0); // The old buffer is dropped here, and `Zeroizing` wipes it. self.0 = grown; @@ -192,6 +197,15 @@ mod tests { ); } + #[test] + fn reserve_returns_an_error_instead_of_aborting() { + let mut buf = SecretBuf::with_capacity(1); + buf.write_all(b"x").unwrap(); + let err = buf.reserve(isize::MAX as usize).unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::OutOfMemory); + assert_eq!(buf.as_bytes(), b"x", "a failed reserve keeps the contents"); + } + #[test] fn read_from_reads_an_empty_reader() { let buf = SecretBuf::read_from(&[][..]).unwrap(); From da0cbf69ee5b9c9f49912ecdff62a36002d763d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 02:19:36 +0000 Subject: [PATCH 6/9] fix(stack-auth): wipe a non-UTF-8 CS_CLIENT_ACCESS_KEY before discarding it Review follow-up on #1553. On Unix, `std::env::var` returns `VarError::NotUnicode` holding the whole value when it is not UTF-8, and `.ok()` dropped that copy of the key without wiping it. `access_key_from_env` now wipes it first. The value is still treated as unset, as before, since it cannot be a valid access key. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- packages/stack-auth/src/auto_strategy.rs | 49 +++++++++++++++++++++--- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/stack-auth/src/auto_strategy.rs b/packages/stack-auth/src/auto_strategy.rs index 5ce2d3d6a3..457bcf3ea3 100644 --- a/packages/stack-auth/src/auto_strategy.rs +++ b/packages/stack-auth/src/auto_strategy.rs @@ -217,11 +217,9 @@ impl AutoStrategyBuilder { // Merge explicit values with env vars (explicit wins) // Wrapped as soon as it is read, so this copy is wiped on drop. The // process environment keeps its own copy, which is beyond our reach. - let access_key = self.access_key.or_else(|| { - std::env::var("CS_CLIENT_ACCESS_KEY") - .ok() - .map(SecretToken::new) - }); + let access_key = self + .access_key + .or_else(|| access_key_from_env(std::env::var("CS_CLIENT_ACCESS_KEY"))); let crn = match self.crn { Some(crn) => Some(crn), @@ -270,6 +268,25 @@ impl AuthStrategy for &AutoStrategy { } } +/// Wrap an access key read from the environment, wiping it if it is unusable. +/// +/// A value that is not valid UTF-8 comes back as [`VarError::NotUnicode`], +/// which owns the whole key. It cannot be a valid access key, so it is +/// treated as unset, as before; but it is wiped first rather than dropped, +/// since it is still a credential. +/// +/// [`VarError::NotUnicode`]: std::env::VarError::NotUnicode +fn access_key_from_env(value: Result) -> Option { + match value { + Ok(key) => Some(SecretToken::new(key)), + Err(std::env::VarError::NotUnicode(key)) => { + drop(zeroize::Zeroizing::new(key.into_encoded_bytes())); + None + } + Err(std::env::VarError::NotPresent) => None, + } +} + #[cfg(test)] mod tests { use super::*; @@ -456,4 +473,26 @@ mod tests { assert!(matches!(result, Err(AuthError::InvalidAccessKey(_)))); } } + + mod access_key_from_env { + use super::super::access_key_from_env; + use std::env::VarError; + + #[test] + fn wraps_a_set_value() { + let key = access_key_from_env(Ok("CSAKid.secret".to_string())).unwrap(); + assert_eq!(key.as_str(), "CSAKid.secret"); + } + + #[test] + fn treats_an_unset_value_as_absent() { + assert!(access_key_from_env(Err(VarError::NotPresent)).is_none()); + } + + #[test] + fn treats_a_non_utf8_value_as_absent() { + let value = std::ffi::OsString::from("CSAKid.secret"); + assert!(access_key_from_env(Err(VarError::NotUnicode(value))).is_none()); + } + } } From a5fa05fc5cb0da449f786dd418d2d281a945053b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 02:26:21 +0000 Subject: [PATCH 7/9] test(stack-auth): pin the wait between device-code polls The mutants gate found that turning the poll sleep's deadline from now + interval into now - interval left every test passing: the loop then polls without waiting, and under paused time the run spins until the gate's timeout. The new test lets the pending answer through for exactly the polls that fit before the deadline, then denies access, so a loop that skips the wait fails at once rather than hanging. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- packages/stack-auth/src/device_code/tests.rs | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/stack-auth/src/device_code/tests.rs b/packages/stack-auth/src/device_code/tests.rs index bd6e41008f..e3fc69bc83 100644 --- a/packages/stack-auth/src/device_code/tests.rs +++ b/packages/stack-auth/src/device_code/tests.rs @@ -632,6 +632,40 @@ async fn test_poll_for_token_does_not_sleep_past_the_deadline() { ); } +/// Polling waits the interval between requests. The server answers +/// `authorization_pending` for exactly as many polls as fit before the +/// deadline and `access_denied` after that, so a loop that polls without +/// waiting fails fast instead of spinning. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_waits_the_interval_between_polls() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(12)); + }); + // Polls at T=0, 5 and 10; the next would be T=15, past the deadline. + mocks.mock_with_options(1, Some(3), |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("authorization_pending")); + }); + mocks.mock_with_options(2, None, |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("access_denied")); + }); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!( + matches!(err, AuthError::TokenExpired(_)), + "expected the deadline to end polling after three polls, got {err:?}" + ); + assert_eq!(started.elapsed().as_secs(), 12); +} + /// A server-supplied token lifetime past the cap is clamped on the /// device-code path too, rather than wrapping `now + expires_in` to a /// timestamp in the past. From 6162ddb9a2baafc62f1b25070dde7a219f0ac6fa Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 02:32:25 +0000 Subject: [PATCH 8/9] test(stack-auth): cap the polls every device-code test allows The mutants gate still timed out on the poll-wait mutant. With the wait gone, every test whose server keeps answering authorization_pending or slow_down spins: paused time only advances when the runtime is idle, so the deadline never arrives and the run hangs past the gate's timeout before the new test's failure can count. Each of those token mocks now allows a set number of polls and answers access_denied after that, so under the mutant all six tests fail in under a second instead of hanging. The polls each test allows are at least what the real loop makes, so the tests pass unchanged on the current code. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- packages/stack-auth/src/device_code/tests.rs | 64 +++++++++----------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/packages/stack-auth/src/device_code/tests.rs b/packages/stack-auth/src/device_code/tests.rs index e3fc69bc83..d77a7f6227 100644 --- a/packages/stack-auth/src/device_code/tests.rs +++ b/packages/stack-auth/src/device_code/tests.rs @@ -423,16 +423,13 @@ async fn test_poll_for_token_authorization_pending_then_success() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); mock_code_endpoint(&mut mocks); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("authorization_pending")); - }); + mock_token_errors(&mut mocks, "authorization_pending", 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; // Use tokio::join! so the swap future can borrow server.mocks() directly // (the shared RwLock) rather than cloning the MockSet. - // First poll at T=5s returns "authorization_pending". + // Polls at T=0s and T=5s return "authorization_pending". // At T=6s the mock is swapped. Second poll at T=10s returns success. let (result, _) = tokio::join!(pending.poll_for_token(), async { tokio::time::sleep(tokio::time::Duration::from_secs(6)).await; @@ -456,10 +453,7 @@ async fn test_poll_for_token_slow_down_then_success() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); mock_code_endpoint(&mut mocks); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("slow_down")); - }); + mock_token_errors(&mut mocks, "slow_down", 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; @@ -502,10 +496,7 @@ async fn test_poll_for_token_slow_down_increases_interval() { "expires_in": 12 })); }); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("slow_down")); - }); + mock_token_errors(&mut mocks, "slow_down", 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; @@ -514,6 +505,22 @@ async fn test_poll_for_token_slow_down_increases_interval() { assert!(matches!(err, AuthError::TokenExpired(_))); } +/// Answer the token endpoint with `error` for at most `polls` requests, +/// then with `access_denied`. A test whose server never stops answering +/// `authorization_pending` or `slow_down` would spin forever if polling +/// stopped waiting between requests (paused time only advances when the +/// runtime is idle); the cap turns that into a fast failure instead. +fn mock_token_errors(mocks: &mut MockSet, error: &'static str, polls: usize) { + mocks.mock_with_options(1, Some(polls), move |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json(error)); + }); + mocks.mock_with_options(2, None, |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("access_denied")); + }); +} + fn device_code_json_expiring_in(expires_in: u64) -> serde_json::Value { let mut json = device_code_json(); json["expires_in"] = expires_in.into(); @@ -560,10 +567,7 @@ async fn test_poll_for_token_expires_at_the_clamped_deadline() { when.post().path("/oauth/device/code"); then.json(device_code_json_expiring_in(u64::MAX)); }); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("authorization_pending")); - }); + mock_token_errors(&mut mocks, "authorization_pending", 721); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; let started = tokio::time::Instant::now(); @@ -578,15 +582,12 @@ async fn test_poll_for_token_expires_at_the_clamped_deadline() { ); } -fn mock_pending_forever(mocks: &mut MockSet, expires_in: u64) { +fn mock_pending_for(mocks: &mut MockSet, expires_in: u64, polls: usize) { mocks.mock(|when, then| { when.post().path("/oauth/device/code"); then.json(device_code_json_expiring_in(expires_in)); }); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("authorization_pending")); - }); + mock_token_errors(mocks, "authorization_pending", polls); } /// The lifetime runs from when the code was received, not from when @@ -595,7 +596,7 @@ fn mock_pending_forever(mocks: &mut MockSet, expires_in: u64) { async fn test_poll_for_token_counts_the_lifetime_from_begin() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); - mock_pending_forever(&mut mocks, 900); + mock_pending_for(&mut mocks, 900, 1); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; @@ -617,7 +618,7 @@ async fn test_poll_for_token_does_not_sleep_past_the_deadline() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); // Polls at T=0 and T=5; the next would be T=10, past the 7s deadline. - mock_pending_forever(&mut mocks, 7); + mock_pending_for(&mut mocks, 7, 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; let started = tokio::time::Instant::now(); @@ -632,10 +633,8 @@ async fn test_poll_for_token_does_not_sleep_past_the_deadline() { ); } -/// Polling waits the interval between requests. The server answers -/// `authorization_pending` for exactly as many polls as fit before the -/// deadline and `access_denied` after that, so a loop that polls without -/// waiting fails fast instead of spinning. +/// Polling waits the interval between requests: the server allows exactly +/// the polls that fit before the deadline, so one more fails the test. #[tokio::test(start_paused = true)] async fn test_poll_for_token_waits_the_interval_between_polls() { let dir = TempDir::new().unwrap(); @@ -645,14 +644,7 @@ async fn test_poll_for_token_waits_the_interval_between_polls() { then.json(device_code_json_expiring_in(12)); }); // Polls at T=0, 5 and 10; the next would be T=15, past the deadline. - mocks.mock_with_options(1, Some(3), |when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("authorization_pending")); - }); - mocks.mock_with_options(2, None, |when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("access_denied")); - }); + mock_token_errors(&mut mocks, "authorization_pending", 3); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; let started = tokio::time::Instant::now(); From 027cb1f2ee13b95e97e38c29d3c15c5d0bece26c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 03:30:23 +0000 Subject: [PATCH 9/9] fix(stack-profile): zero-fill at most one chunk per read in SecretBuf read_from resized the buffer to its whole capacity before every read, zero-filling all of the spare tail, then truncated back to the bytes read. A reader that returns short reads into a large buffer had the same tail zeroed again on every call, so the cost grew with the square of the file size. Each read now gets a window of at most one chunk. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Jmn7qCTmGiT6hfLv8rxDP --- packages/stack-profile/src/secret_buf.rs | 46 ++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index 0adc35fc48..cce52a53f5 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -54,9 +54,11 @@ impl SecretBuf { buf.reserve(READ_CHUNK)?; } let filled = buf.0.len(); - // Exactly up to capacity, so this does not reallocate. - let capacity = buf.0.capacity(); - buf.0.resize(capacity, 0); + // At most one chunk, so each read zero-fills a bounded window + // rather than the whole spare tail; and never past the + // capacity, so this does not reallocate. + let window_end = buf.0.capacity().min(filled + READ_CHUNK); + buf.0.resize(window_end, 0); let result = reader.read(&mut buf.0[filled..]); buf.0.truncate(filled + *result.as_ref().unwrap_or(&0)); match result { @@ -206,6 +208,44 @@ mod tests { assert_eq!(buf.as_bytes(), b"x", "a failed reserve keeps the contents"); } + /// Records the largest slice it is asked to fill, and fills at most + /// 1 KiB of it, the way a pipe or network filesystem can. + struct ShortReads { + bytes: Vec, + pos: usize, + largest_window: usize, + } + + impl Read for &mut ShortReads { + fn read(&mut self, out: &mut [u8]) -> std::io::Result { + self.largest_window = self.largest_window.max(out.len()); + let n = out.len().min(1024).min(self.bytes.len() - self.pos); + out[..n].copy_from_slice(&self.bytes[self.pos..self.pos + n]); + self.pos += n; + Ok(n) + } + } + + #[test] + fn read_from_zero_fills_at_most_one_chunk_per_read() { + // Each read zero-fills the window it reads into. If the window were + // all the spare capacity, short reads into a large buffer would + // re-zero its whole tail every time: quadratic in the file size. + let payload = payload(64 * 1024); + let mut reader = ShortReads { + bytes: payload.clone(), + pos: 0, + largest_window: 0, + }; + let buf = SecretBuf::read_from(&mut reader).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + assert!( + reader.largest_window <= READ_CHUNK, + "each read should fill at most {READ_CHUNK} bytes; one was offered {}", + reader.largest_window + ); + } + #[test] fn read_from_reads_an_empty_reader() { let buf = SecretBuf::read_from(&[][..]).unwrap();