mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
fix(security): address review feedback on cross-channel approval checks
1. thread_ops.rs: Remove .or(Some(&*message.channel)) fallback in maybe_hydrate_thread() so that when source_channel is NULL in the DB, it stays None rather than being stamped with the requesting channel. This preserves the fail-closed behavior of is_approval_authorized(). 2. libsql_migrations.rs: Remove source_channel from base SCHEMA to eliminate duplicate column definition. The column is now added solely by V14 migration, preventing fresh databases from failing on startup. 3. wasm/setup.rs: Expand RESERVED_CHANNEL_NAMES to cover all built-in channels (http, signal, slack-relay, secret_save) and add a dynamic collision check against already-registered channel names passed from the startup sequence. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
@@ -146,7 +146,7 @@ impl Agent {
|
|||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
};
|
};
|
||||||
let effective_source_channel = db_source_channel.as_deref().or(Some(&*message.channel));
|
let effective_source_channel = db_source_channel.as_deref();
|
||||||
|
|
||||||
let session_id = {
|
let session_id = {
|
||||||
let sess = session.lock().await;
|
let sess = session.lock().await;
|
||||||
|
|||||||
@@ -34,6 +34,7 @@ pub async fn setup_wasm_channels(
|
|||||||
secrets_store: &Option<Arc<dyn SecretsStore + Send + Sync>>,
|
secrets_store: &Option<Arc<dyn SecretsStore + Send + Sync>>,
|
||||||
extension_manager: Option<&Arc<ExtensionManager>>,
|
extension_manager: Option<&Arc<ExtensionManager>>,
|
||||||
database: Option<&Arc<dyn Database>>,
|
database: Option<&Arc<dyn Database>>,
|
||||||
|
registered_channel_names: &[String],
|
||||||
) -> Option<WasmChannelSetup> {
|
) -> Option<WasmChannelSetup> {
|
||||||
let runtime = match WasmChannelRuntime::new(WasmChannelRuntimeConfig::default()) {
|
let runtime = match WasmChannelRuntime::new(WasmChannelRuntimeConfig::default()) {
|
||||||
Ok(r) => Arc::new(r),
|
Ok(r) => Arc::new(r),
|
||||||
@@ -74,7 +75,19 @@ pub async fn setup_wasm_channels(
|
|||||||
// Reserved channel names that WASM modules must not claim.
|
// Reserved channel names that WASM modules must not claim.
|
||||||
// A malicious module could otherwise register as a trusted built-in
|
// A malicious module could otherwise register as a trusted built-in
|
||||||
// channel and bypass cross-channel authorization checks.
|
// channel and bypass cross-channel authorization checks.
|
||||||
const RESERVED_CHANNEL_NAMES: &[&str] = &["web", "gateway", "cli", "repl"];
|
// This list must cover every built-in channel name to prevent a WASM
|
||||||
|
// module from impersonating a built-in and satisfying same-channel
|
||||||
|
// approval checks.
|
||||||
|
const RESERVED_CHANNEL_NAMES: &[&str] = &[
|
||||||
|
"web",
|
||||||
|
"gateway",
|
||||||
|
"cli",
|
||||||
|
"repl",
|
||||||
|
"http",
|
||||||
|
"signal",
|
||||||
|
"slack-relay",
|
||||||
|
"secret_save",
|
||||||
|
];
|
||||||
|
|
||||||
for loaded in results.loaded {
|
for loaded in results.loaded {
|
||||||
let name_lower = loaded.name().to_ascii_lowercase();
|
let name_lower = loaded.name().to_ascii_lowercase();
|
||||||
@@ -85,6 +98,19 @@ pub async fn setup_wasm_channels(
|
|||||||
);
|
);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
// Also reject any name that collides with an already-registered
|
||||||
|
// channel to prevent a WASM module from shadowing a channel that
|
||||||
|
// was registered earlier in the startup sequence.
|
||||||
|
if registered_channel_names
|
||||||
|
.iter()
|
||||||
|
.any(|n| n.to_ascii_lowercase() == name_lower)
|
||||||
|
{
|
||||||
|
tracing::warn!(
|
||||||
|
channel = %loaded.name(),
|
||||||
|
"Rejected WASM channel that collides with already-registered channel"
|
||||||
|
);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
let (name, channel) = register_channel(
|
let (name, channel) = register_channel(
|
||||||
loaded,
|
loaded,
|
||||||
|
|||||||
@@ -38,8 +38,7 @@ CREATE TABLE IF NOT EXISTS conversations (
|
|||||||
thread_id TEXT,
|
thread_id TEXT,
|
||||||
started_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
|
started_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
|
||||||
last_activity TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
|
last_activity TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
|
||||||
metadata TEXT NOT NULL DEFAULT '{}',
|
metadata TEXT NOT NULL DEFAULT '{}'
|
||||||
source_channel TEXT
|
|
||||||
);
|
);
|
||||||
|
|
||||||
CREATE INDEX IF NOT EXISTS idx_conversations_channel ON conversations(channel);
|
CREATE INDEX IF NOT EXISTS idx_conversations_channel ON conversations(channel);
|
||||||
|
|||||||
@@ -449,6 +449,7 @@ async fn async_main() -> anyhow::Result<()> {
|
|||||||
&components.secrets_store,
|
&components.secrets_store,
|
||||||
components.extension_manager.as_ref(),
|
components.extension_manager.as_ref(),
|
||||||
components.db.as_ref(),
|
components.db.as_ref(),
|
||||||
|
&channel_names,
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user