diff --git a/crates/synapse-module/src/lib.rs b/crates/synapse-module/src/lib.rs index 3dc35254..0fb025f8 100644 --- a/crates/synapse-module/src/lib.rs +++ b/crates/synapse-module/src/lib.rs @@ -80,8 +80,8 @@ use store::{ JOB_STATE_PAUSED_NEEDS_REAUTH, JOB_STATE_QUEUED, JOB_STATE_RUNNING, }; use subc_client_rs::{ - async_trait, BindDecision, HandlerOutcome, HealthReport, ModuleHandler, RequestCtx, - RouteBindRequest, RouteHandle, SubcModuleError, + async_trait, build_provenance, BindDecision, HandlerOutcome, HealthReport, ModuleHandler, + RequestCtx, RouteBindRequest, RouteHandle, SubcModuleError, }; use subc_protocol::{ manifest::{ @@ -13612,10 +13612,18 @@ fn manifest(module_id: &str) -> ModuleManifest { // outside its own store and models directory, and observation-anchored // signals would claim watch points we do not maintain. self_signals: Some(Vec::new()), - // Honest-until-injected: release scripts do not stamp CK_BUILD_* facts - // yet, and fabricating build provenance would defeat the field's - // purpose. The daemon overlays process-identity evidence regardless. - provenance: None, + // Declare what is known rather than blanket-None: the SDK helper stamps + // `wire_crate_version` from the linked subc-protocol crate, and the + // newest migration this binary carries is a fact a daemon can compare + // against a store's actual version to spot a stale binary directly. + // Build facts stay absent because release scripts do not stamp + // CK_BUILD_* yet, and the helper maps an absent input to field omission + // rather than minting a sentinel string that would read as a fact. + provenance: Some(build_provenance( + None, + None, + Some(&store::newest_schema_version().to_string()), + )), } } @@ -14002,6 +14010,48 @@ mod tests { ); } + #[test] + fn manifest_provenance_declares_only_facts_this_binary_knows() { + let provenance = manifest("synapse") + .provenance + .expect("an SDK module always has at least one honest provenance fact"); + + // The referent is the linked subc-protocol crate -- the fleet's shared + // wire vocabulary -- never synapse's own version, which would be a + // real number from the wrong numbering space and would read as correct + // to any check that inspects shape rather than meaning. + assert_eq!( + provenance.wire_crate_version.as_deref(), + Some(subc_client_rs::SUBC_PROTOCOL_CRATE_VERSION), + "wire_crate_version must name the linked SDK crate" + ); + + // Release scripts do not stamp these yet. Absent is the honest shape; + // a sentinel string like "unknown" would be a well-formed lie. + assert_eq!(provenance.build_git_sha, None); + assert_eq!(provenance.build_lock_digest, None); + + // Deliberately NOT asserting the number itself: restating a derived + // value here would make this test agree with the code by construction + // and pass whatever the migration list said. Shape and presence are + // what this can check honestly; the derivation is what keeps the value + // true. + let schema_version = provenance + .store_schema_version + .as_deref() + .expect("a module with a migration list can state its newest migration"); + assert!( + schema_version + .parse::() + .is_ok_and(|version| version > 0), + "store_schema_version must be a real migration number, got {schema_version:?}" + ); + + provenance + .validate() + .expect("declared provenance must satisfy the wire contract"); + } + #[test] fn sidecar_config_is_default_off() { assert!(!ModuleConfig::default().sidecar_spec.enabled); diff --git a/crates/synapse-module/src/remote/vault.rs b/crates/synapse-module/src/remote/vault.rs index f4197912..61229ab1 100644 --- a/crates/synapse-module/src/remote/vault.rs +++ b/crates/synapse-module/src/remote/vault.rs @@ -1,6 +1,7 @@ use std::{future::Future, path::PathBuf, sync::Arc, time::Duration}; use serde::{Deserialize, Serialize}; +use serde_json::Value; use subc_client_rs::{async_trait, CallError, CallOptions, ConsumerOptions, SubcConsumer}; use subc_protocol::{BindIdentity, Priority, RouteTarget}; use tokio::sync::Mutex; @@ -8,6 +9,8 @@ use tokio::sync::Mutex; use super::runtime::{CredentialToken, VaultCredentialClient, VaultError}; const CREDENTIALS_MODULE_ID: &str = "claustrum"; +/// Envelope key claustrum wraps every `credential.get` reply in. +const CREDENTIAL_RESULT_KEY: &str = "result"; const VAULT_MIN_TTL_MS: u64 = 600_000; const VAULT_CALL_TIMEOUT: Duration = Duration::from_secs(10); const VAULT_ROUTE_READY_TIMEOUT: Duration = Duration::from_secs(1); @@ -92,21 +95,6 @@ struct CredentialReportParams<'a> { record_version: u64, } -#[derive(Deserialize)] -#[serde(untagged)] -enum CredentialGetResponse { - Direct(CredentialGetOutcome), - Wrapped { result: CredentialGetOutcome }, -} - -impl CredentialGetResponse { - fn into_outcome(self) -> CredentialGetOutcome { - match self { - Self::Direct(outcome) | Self::Wrapped { result: outcome } => outcome, - } - } -} - #[derive(Deserialize)] #[serde(untagged)] enum CredentialGetOutcome { @@ -127,8 +115,26 @@ struct CredentialReadError { code: String, } -fn decode_credential_get_response(response: &[u8]) -> Result { - serde_json::from_slice(response).map_err(|_| VaultError::MalformedHandlesFile) +/// Selects the credential envelope explicitly instead of by `#[serde(untagged)]` +/// variant order. +/// +/// Untagged enums are tried in declaration order, so a frame carrying both a +/// wrapped `result.error` and top-level success fields resolved as a SUCCESS and +/// discarded the error — returning a token built from the stray bytes. Claustrum +/// cannot emit that frame today (their outcome is an enum whose variants cannot +/// coexist), but the envelope choice was resting on declaration order rather than +/// on a decision, and it failed toward serving a credential. +/// +/// `result` therefore wins whenever it is present. The unwrapped form stays +/// accepted, but as a documented fallback rather than a coincidence of ordering. +fn decode_credential_get_response(response: &[u8]) -> Result { + let envelope: Value = + serde_json::from_slice(response).map_err(|_| VaultError::MalformedHandlesFile)?; + let outcome = match envelope.get(CREDENTIAL_RESULT_KEY) { + Some(result) => result.clone(), + None => envelope, + }; + serde_json::from_value(outcome).map_err(|_| VaultError::MalformedHandlesFile) } fn map_credential_read_error(error: CredentialReadError) -> VaultError { @@ -154,8 +160,7 @@ impl VaultCredentialClient for SubcVaultCredentialClient { }) .map_err(|_| VaultError::MalformedHandle)?; let response = self.call(body).await?; - let response = decode_credential_get_response(&response)?; - match response.into_outcome() { + match decode_credential_get_response(&response)? { CredentialGetOutcome::Success { payload, expires_at_ms, @@ -264,23 +269,68 @@ mod tests { #[test] fn credential_wire_decoder_never_exposes_payload_in_errors() { - let response: CredentialGetResponse = serde_json::from_value(serde_json::json!({ - "result": {"payload": [115, 101, 99, 114, 101, 116], "expires_at_ms": null, "record_version": 3} - })) - .unwrap(); - let CredentialGetOutcome::Success { payload, .. } = response.into_outcome() else { + let response = decode_credential_get_response( + br#"{"result":{"payload":[115,101,99,114,101,116],"expires_at_ms":null,"record_version":3}}"#, + ) + .expect("a well-formed credential success must deserialize"); + let CredentialGetOutcome::Success { payload, .. } = response else { panic!("expected credential success"); }; assert_eq!(payload.len(), 6); } + #[test] + fn wrapped_error_wins_over_stray_top_level_success_fields() { + // Envelope selection must not depend on untagged variant order: the + // wrapped error is the reply, and the stray top-level fields are not a + // credential. Decoding this as a success would hand a provider call a + // token built from bytes that accompanied a failure. + let response = decode_credential_get_response( + br#"{"result":{"error":{"class":"transient","code":"x"}},"payload":[1],"record_version":9}"#, + ) + .expect("the wrapped error is a well-formed frame"); + let CredentialGetOutcome::AppError { error } = response else { + panic!("stray top-level success fields must not outrank a wrapped error"); + }; + assert_eq!(error.class, "transient"); + } + + #[test] + fn unwrapped_outcome_is_still_accepted() { + // The fallback is deliberate, not incidental — pin it so removing it is a + // decision rather than a side effect. + let response = decode_credential_get_response( + br#"{"error":{"class":"permanent","code":"not_found"}}"#, + ) + .expect("an unwrapped outcome remains decodable"); + let CredentialGetOutcome::AppError { error } = response else { + panic!("expected credential error outcome"); + }; + assert_eq!(error.code, "not_found"); + } + + #[test] + fn missing_error_code_is_a_malformed_credential_frame() { + // `code` is non-Option, so this is currently rejected by the type rather + // than by intent. Pinning it turns an accident into a guarantee: adding + // #[serde(default)] later goes red here instead of silently widening + // what synapse accepts from the vault. + let Err(error) = + decode_credential_get_response(br#"{"result":{"error":{"class":"permanent"}}}"#) + else { + panic!("credential errors without the required code are malformed"); + }; + + assert_eq!(error, VaultError::MalformedHandlesFile); + } + #[test] fn unknown_code_in_known_class_deserializes_without_becoming_malformed() { let response = decode_credential_get_response( br#"{"result":{"error":{"class":"transient","code":"future_refresh_path"}}}"#, ) .expect("a well-formed credential error must deserialize"); - let CredentialGetOutcome::AppError { error } = response.into_outcome() else { + let CredentialGetOutcome::AppError { error } = response else { panic!("expected credential error outcome"); }; diff --git a/crates/synapse-module/src/store.rs b/crates/synapse-module/src/store.rs index a8ce8e78..d6fbd3ad 100644 --- a/crates/synapse-module/src/store.rs +++ b/crates/synapse-module/src/store.rs @@ -27,6 +27,21 @@ pub const JOB_STATE_DONE: &str = "done"; pub const JOB_STATE_FAILED_TRANSIENT: &str = "failed_transient"; pub const JOB_STATE_FAILED_PERMANENT: &str = "failed_permanent"; +/// Newest schema version this binary can migrate a store to. +/// +/// Derived from the migration list rather than restated as a literal: a literal +/// keeps reporting the old number the first time a migration is appended, and a +/// daemon comparing this against a store's actual version would then read a +/// stale binary as current. `max` rather than `last` so it does not depend on +/// the list staying sorted. +pub fn newest_schema_version() -> u32 { + MIGRATIONS + .iter() + .map(|migration| migration.version) + .max() + .unwrap_or(0) +} + const MIGRATIONS: &[Migration] = &[ Migration { version: 1,