Skip to content

tests: remove the network capability rather than relying on not using it #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ualtinok merged 1 commit into from
Aug 29, 2026
Merged

tests: remove the network capability rather than relying on not using it #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions crates/credentials-module/src/admin_surface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@cubic-dev-ai cubic-dev-ai Bot Aug 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This adds a second independently maintained NoHttp transport implementation; future transport changes can make the module test rigs diverge. Move the stub into one shared test helper and use it from both test modules.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/admin_surface.rs, line 381:

<comment>This adds a second independently maintained `NoHttp` transport implementation; future transport changes can make the module test rigs diverge. Move the stub into one shared test helper and use it from both test modules.</comment>

<file context>
@@ -356,12 +356,61 @@ mod tests {
+    /// 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]
</file context>


#[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<u8>,
) -> 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 {
Expand Down Expand Up @@ -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),
Expand Down
65 changes: 60 additions & 5 deletions crates/credentials-module/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<u8>,
) -> 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<ReadSurface> {
tmp_surface_with_store(seed).0
}
Expand Down Expand Up @@ -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
Expand All @@ -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<admin_surface::AdminSurface>, Arc<EncryptedStore>) {
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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)],
Expand Down Expand Up @@ -1897,7 +1952,7 @@ mod tests {
Arc<EncryptedStore>,
) {
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);
Expand Down
Loading