-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix WASM channel startup restore semantics #2563
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,6 +65,7 @@ pub async fn setup_wasm_channels( | |||||||||
| extension_manager: Option<&Arc<ExtensionManager>>, | ||||||||||
| database: Option<&Arc<dyn Database>>, | ||||||||||
| registered_channel_names: &[String], | ||||||||||
| startup_active_channel_names: &HashSet<String>, | ||||||||||
| ownership_cache: Arc<crate::ownership::OwnershipCache>, | ||||||||||
| ) -> Option<WasmChannelSetup> { | ||||||||||
| let runtime = match WasmChannelRuntime::new(WasmChannelRuntimeConfig::default()) { | ||||||||||
|
|
@@ -105,6 +106,54 @@ pub async fn setup_wasm_channels( | |||||||||
| }; | ||||||||||
|
|
||||||||||
| let wasm_router = Arc::new(WasmChannelRouter::new()); | ||||||||||
| let (channels, channel_names) = register_startup_channels( | ||||||||||
| results.loaded, | ||||||||||
| config, | ||||||||||
| secrets_store, | ||||||||||
| settings_store.as_ref(), | ||||||||||
| registered_channel_names, | ||||||||||
| startup_active_channel_names, | ||||||||||
| &pairing_store, | ||||||||||
| &wasm_router, | ||||||||||
| ) | ||||||||||
| .await; | ||||||||||
|
|
||||||||||
| for (path, err) in &results.errors { | ||||||||||
| tracing::warn!("Failed to load WASM channel {}: {}", path.display(), err); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Always create webhook routes (even with no channels loaded) so that | ||||||||||
| // channels hot-added at runtime can receive webhooks without a restart. | ||||||||||
| let webhook_routes = { | ||||||||||
| Some(create_wasm_channel_router( | ||||||||||
| Arc::clone(&wasm_router), | ||||||||||
| extension_manager.map(Arc::clone), | ||||||||||
| )) | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| Some(WasmChannelSetup { | ||||||||||
| channels, | ||||||||||
| channel_names, | ||||||||||
| webhook_routes, | ||||||||||
| wasm_channel_runtime: runtime, | ||||||||||
| pairing_store, | ||||||||||
| wasm_channel_router: wasm_router, | ||||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn register_startup_channels( | ||||||||||
| loaded_channels: Vec<LoadedChannel>, | ||||||||||
| config: &Config, | ||||||||||
| secrets_store: &Option<Arc<dyn SecretsStore + Send + Sync>>, | ||||||||||
| settings_store: Option<&Arc<dyn crate::db::SettingsStore>>, | ||||||||||
| registered_channel_names: &[String], | ||||||||||
| startup_active_channel_names: &HashSet<String>, | ||||||||||
| pairing_store: &Arc<PairingStore>, | ||||||||||
| wasm_router: &Arc<WasmChannelRouter>, | ||||||||||
| ) -> ( | ||||||||||
| Vec<(String, Box<dyn crate::channels::Channel>)>, | ||||||||||
| Vec<String>, | ||||||||||
| ) { | ||||||||||
| let mut channels: Vec<(String, Box<dyn crate::channels::Channel>)> = Vec::new(); | ||||||||||
| let mut channel_names: Vec<String> = Vec::new(); | ||||||||||
|
|
||||||||||
|
|
@@ -116,11 +165,20 @@ pub async fn setup_wasm_channels( | |||||||||
| // - All native/built-in channel names (prevent impersonation) | ||||||||||
| // - Trusted approval channels from session::TRUSTED_APPROVAL_CHANNELS | ||||||||||
| // - The bootstrap sentinel (universal approval wildcard) | ||||||||||
| for loaded in results.loaded { | ||||||||||
| let name_lower = loaded.name().to_ascii_lowercase(); | ||||||||||
| for loaded in loaded_channels { | ||||||||||
| let channel_name = loaded.name().to_string(); | ||||||||||
| if !startup_active_channel_names.contains(&channel_name) { | ||||||||||
| tracing::debug!( | ||||||||||
| channel = %channel_name, | ||||||||||
| "Skipping installed but inactive WASM channel during startup restore" | ||||||||||
| ); | ||||||||||
| continue; | ||||||||||
| } | ||||||||||
|
Comment on lines
+168
to
+176
|
||||||||||
|
|
||||||||||
| let name_lower = channel_name.to_ascii_lowercase(); | ||||||||||
| if is_reserved_wasm_channel_name(&name_lower) { | ||||||||||
| tracing::warn!( | ||||||||||
| channel = %loaded.name(), | ||||||||||
| channel = %channel_name, | ||||||||||
| "Rejected WASM channel with reserved name" | ||||||||||
| ); | ||||||||||
| continue; | ||||||||||
|
|
@@ -133,7 +191,7 @@ pub async fn setup_wasm_channels( | |||||||||
| .any(|n| n.to_ascii_lowercase() == name_lower) | ||||||||||
| { | ||||||||||
| tracing::warn!( | ||||||||||
| channel = %loaded.name(), | ||||||||||
| channel = %channel_name, | ||||||||||
| "Rejected WASM channel that collides with already-registered channel" | ||||||||||
| ); | ||||||||||
| continue; | ||||||||||
|
|
@@ -143,36 +201,16 @@ pub async fn setup_wasm_channels( | |||||||||
| loaded, | ||||||||||
| config, | ||||||||||
| secrets_store, | ||||||||||
| settings_store.as_ref(), | ||||||||||
| &pairing_store, | ||||||||||
| &wasm_router, | ||||||||||
| settings_store, | ||||||||||
| pairing_store, | ||||||||||
| wasm_router, | ||||||||||
| ) | ||||||||||
| .await; | ||||||||||
| channel_names.push(name.clone()); | ||||||||||
| channels.push((name, channel)); | ||||||||||
|
Comment on lines
209
to
210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure consistency with the activation tracking logic in
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| for (path, err) in &results.errors { | ||||||||||
| tracing::warn!("Failed to load WASM channel {}: {}", path.display(), err); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Always create webhook routes (even with no channels loaded) so that | ||||||||||
| // channels hot-added at runtime can receive webhooks without a restart. | ||||||||||
| let webhook_routes = { | ||||||||||
| Some(create_wasm_channel_router( | ||||||||||
| Arc::clone(&wasm_router), | ||||||||||
| extension_manager.map(Arc::clone), | ||||||||||
| )) | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| Some(WasmChannelSetup { | ||||||||||
| channels, | ||||||||||
| channel_names, | ||||||||||
| webhook_routes, | ||||||||||
| wasm_channel_runtime: runtime, | ||||||||||
| pairing_store, | ||||||||||
| wasm_channel_router: wasm_router, | ||||||||||
| }) | ||||||||||
| (channels, channel_names) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// Process a single loaded WASM channel: retrieve secrets, inject config, | ||||||||||
|
|
@@ -593,7 +631,7 @@ async fn inject_channel_secrets_into_config( | |||||||||
|
|
||||||||||
| #[cfg(test)] | ||||||||||
| mod tests { | ||||||||||
| use std::collections::HashMap; | ||||||||||
| use std::collections::{HashMap, HashSet}; | ||||||||||
| use std::sync::Arc; | ||||||||||
|
|
||||||||||
| use super::reserved_wasm_channel_names; | ||||||||||
|
|
@@ -875,6 +913,52 @@ mod tests { | |||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[tokio::test] | ||||||||||
| async fn register_startup_channels_only_restores_persisted_active_channels() { | ||||||||||
| let (config, _temp_dir) = test_config(); | ||||||||||
| let wasm_router = Arc::new(WasmChannelRouter::new()); | ||||||||||
| let pairing_store = Arc::new(PairingStore::new_noop()); | ||||||||||
| let loaded_channels = vec![ | ||||||||||
| test_loaded_channel("telegram", serde_json::json!({ "owner_id": 12345 })), | ||||||||||
| test_loaded_channel("discord", serde_json::json!({ "owner_id": 67890 })), | ||||||||||
| ]; | ||||||||||
| let startup_active_channel_names = | ||||||||||
| HashSet::from([String::from("telegram"), String::from("missing_channel")]); | ||||||||||
|
|
||||||||||
| let (channels, channel_names) = super::register_startup_channels( | ||||||||||
| loaded_channels, | ||||||||||
| &config, | ||||||||||
| &None, | ||||||||||
| None, | ||||||||||
| &[], | ||||||||||
| &startup_active_channel_names, | ||||||||||
| &pairing_store, | ||||||||||
| &wasm_router, | ||||||||||
| ) | ||||||||||
| .await; | ||||||||||
|
|
||||||||||
| assert_eq!( | ||||||||||
| channels.len(), | ||||||||||
| 1, | ||||||||||
| "only the persisted active channel should restore" | ||||||||||
| ); | ||||||||||
| assert_eq!(channel_names, vec!["telegram"]); | ||||||||||
| assert!( | ||||||||||
| wasm_router | ||||||||||
| .get_channel_for_path("/webhook/telegram") | ||||||||||
| .await | ||||||||||
| .is_some(), | ||||||||||
| "persisted active channel should be registered on the webhook router" | ||||||||||
| ); | ||||||||||
| assert!( | ||||||||||
| wasm_router | ||||||||||
| .get_channel_for_path("/webhook/discord") | ||||||||||
| .await | ||||||||||
| .is_none(), | ||||||||||
| "installed but inactive channel should not be registered on the webhook router" | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[tokio::test] | ||||||||||
| async fn inject_channel_secrets_uses_owner_scope() { | ||||||||||
| let crypto = | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check against
startup_active_channel_namesis case-sensitive and does not account for extension name canonicalization (e.g., converting dashes to underscores). Sincestartup_active_channel_namesis populated usingnormalize_extension_names, it contains canonicalized names. Ifloaded.name()returns a non-canonical form (likemy-channelorTelegram), it will be incorrectly skipped during startup restore.You should canonicalize the loaded channel's name before checking the set, handling potential errors gracefully as per project standards.
References