From dce498c7e6adaa5ec1ee8515a52bda54693d178d Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:24:50 +0200 Subject: [PATCH] tests: remove the network capability rather than relying on not using it Six test rigs across main.rs and admin_surface.rs built a real ReqwestTransport. None of them ever sent a request through it: five construct the engine with an empty adapter list, so no refresh can dispatch, and the sixth passes TtlFixtureAdapter, which ignores the transport it is handed. So the suite made no outbound calls -- but that was a property of the ARGUMENTS at each call site, never of the type. Adding one real adapter to any rig, or a fixture that forwards rather than ignores, would silently turn a unit test into a live token exchange against a provider. Nothing would have flagged it; the test would still pass, and it would pass for a reason supplied by a remote service. Two sibling repos found exactly that today. One suite had been issuing thirty real token-exchange requests per run against a vendor endpoint with fabricated credentials, and one of its tests passed only because the endpoint rejected them -- the assertion had been reading the provider's live response instead of the code under test, green for months. The traffic is not the defect. A remote service supplying a test's precondition is. I verified this suite empirically first, running it in a network namespace with egress dropped, and it passed. That proves today's arguments and expires the moment someone edits a rig. NoHttp makes it structural: no adapter added later can reach outward, because the transport it would be handed has no outward. Returning RefreshError::Transport also names the cause at the point of use rather than leaving someone to read a timeout. Verified as a capability change rather than a behaviour change: patching both NoHttp arms to panic leaves the suite green (80 passed), so nothing exercises the transport at all. The replacement asserted its five call sites were present before editing, and the production wiring at main.rs:437 is untouched. Gate: exit 0, nine real-daemon e2e arms executing. --- .../credentials-module/src/admin_surface.rs | 53 ++++++++++++++- crates/credentials-module/src/main.rs | 65 +++++++++++++++++-- 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/crates/credentials-module/src/admin_surface.rs b/crates/credentials-module/src/admin_surface.rs index 0163e12..37e41b1 100644 --- a/crates/credentials-module/src/admin_surface.rs +++ b/crates/credentials-module/src/admin_surface.rs @@ -356,12 +356,61 @@ mod tests { use super::*; use cortexkit_store::{open_sqlite, Isolation, StorageBackend, StorageDescriptor}; use credentials_core::audit::{AuditCtx, AuditOp}; - use credentials_core::http::ReqwestTransport; use credentials_core::key::{MasterKey, MASTER_KEY_LEN}; use credentials_core::record::{CredentialKind, VaultRecord}; use credentials_core::store::{mint_handle, EncryptedStore}; use credentials_core::vault_id_for; + /// A transport that cannot reach the network, used everywhere in this module. + /// + /// These rigs previously built a real `ReqwestTransport`. Nothing here ever sent a + /// request through it -- the engine is constructed with an EMPTY adapter list, so no + /// refresh can dispatch -- but that is a property of the arguments at each call site, + /// not of the type. Adding one adapter to any of these rigs would silently turn an + /// admin test into a live token exchange against a provider. + /// + /// Two sibling repos discovered exactly that on 2026-08-29: one suite was issuing 30 + /// real token-exchange requests per run with fabricated credentials, and its + /// assertions had been reading the provider's live rejection rather than their own + /// code. The defect is not the traffic, it is that a remote service was supplying a + /// test's precondition. + /// + /// This makes the guarantee structural: no adapter added later can reach outward, + /// because the transport it would be handed has no outward. `RefreshError::Transport` + /// on use also names the cause at the failure rather than producing a timeout. + struct NoHttp; + + #[async_trait::async_trait] + impl credentials_core::refresh_adapters::HttpTransport for NoHttp { + async fn post( + &self, + _url: &str, + _headers: &[(&str, &str)], + _content_type: &str, + _body: Vec, + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "admin_surface tests do not make network calls".into(), + )) + } + + async fn get( + &self, + _url: &str, + _headers: &[(&str, &str)], + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "admin_surface tests do not make network calls".into(), + )) + } + } + /// A test rig: the AdminSurface plus everything a caller-side signer needs /// (the same MAC key derivation the CLI would perform from the keychain key). struct Rig { @@ -397,7 +446,7 @@ mod tests { let key_id = key.key_id(); let vault_id = vault_id_for(&root).expect("vault id"); let store = Arc::new(EncryptedStore::open(store, key).expect("open vault")); - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); Rig { admin: AdminSurface::new(engine, mac_key, vault_id, key_id), diff --git a/crates/credentials-module/src/main.rs b/crates/credentials-module/src/main.rs index e0ee6b7..cb94656 100644 --- a/crates/credentials-module/src/main.rs +++ b/crates/credentials-module/src/main.rs @@ -1470,6 +1470,61 @@ mod tests { use credentials_core::store::GrantOperation; use read_surface::ReadSurface; + /// A transport that cannot reach the network, used by every rig in this module. + /// + /// These rigs previously built a real `ReqwestTransport`. None of them ever sent a + /// request through it: four construct the engine with an EMPTY adapter list, and the + /// fifth passes `TtlFixtureAdapter`, which ignores the transport it is handed. So the + /// suite made no outbound calls -- but that was a property of the ARGUMENTS at each + /// call site, never of the type. Adding a real adapter to any rig, or a fixture that + /// forwards, would silently turn a unit test into a live token exchange. + /// + /// Two sibling repos found exactly that on 2026-08-29. One suite had been issuing 30 + /// real token-exchange requests per run against a vendor endpoint with fabricated + /// credentials, and one of its tests passed only because the remote service rejected + /// them -- the assertion was reading the provider's live response instead of the code + /// under test. The traffic is not the defect. A remote service supplying a test's + /// precondition is. + /// + /// Verified here by running the suite inside a network namespace with egress dropped, + /// which passed -- but that proves TODAY's arguments, and has to be re-run to keep + /// meaning anything. This makes it structural instead: no adapter added later can + /// reach outward, because the transport it would be handed has no outward. Returning + /// `RefreshError::Transport` also names the cause at the point of use rather than + /// leaving someone to read a timeout. + struct NoHttp; + + #[async_trait::async_trait] + impl credentials_core::refresh_adapters::HttpTransport for NoHttp { + async fn post( + &self, + _url: &str, + _headers: &[(&str, &str)], + _content_type: &str, + _body: Vec, + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "main tests do not make network calls".into(), + )) + } + + async fn get( + &self, + _url: &str, + _headers: &[(&str, &str)], + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "main tests do not make network calls".into(), + )) + } + } + fn tmp_surface(seed: u8) -> Arc { tmp_surface_with_store(seed).0 } @@ -1507,7 +1562,7 @@ mod tests { .open_intent("apikey:crashed", 1, &hash) .expect("open intent"); - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); // The daemon's own boot-gate sequence: reconcile, then record. Calls the same @@ -1533,7 +1588,7 @@ mod tests { /// known master key (seed) so tests can derive the same MAC key caller-side. fn tmp_admin(seed: u8) -> (Arc, Arc) { let (_, store, db_path) = tmp_surface_with_store(seed); - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let key = MasterKey::from_bytes([seed; MASTER_KEY_LEN]); let mac_key = credentials_core::admin_auth::AdminMacKey::derive(&key); @@ -1588,7 +1643,7 @@ mod tests { store.invalidate("apikey:dead").expect("invalidate"); let store = Arc::new(store); - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let surface = Arc::new(ReadSurface::new(engine, FetchLimiter::new(Caps::default()))); (surface, store, db_path) @@ -1646,7 +1701,7 @@ mod tests { calls: Arc::clone(&calls), fresh_ttl_ms, }; - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new( Arc::clone(&store), vec![Arc::new(adapter)], @@ -1897,7 +1952,7 @@ mod tests { Arc, ) { let (surface, store, db_path) = tmp_surface_with_store(seed); - let http = Arc::new(ReqwestTransport::new().expect("http")); + let http = Arc::new(NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let key = MasterKey::from_bytes([seed; MASTER_KEY_LEN]); let mac_key = credentials_core::admin_auth::AdminMacKey::derive(&key);