Skip to content

Commit 6a5ca6f

Browse files
ibhagwanibhagwan
committed
fix(signal): align validation rules and add toggle_platform support
Frontend validation (ChannelSettingCard.tsx): - Update E.164 regex to /^\+[1-9]\d{5,13}$/ for true E.164 compliance - Enforce 7-15 digits total (6-14 after '+'), first digit 1-9 - Update error message and help text to reflect correct range Backend validation (target.rs): - Change is_valid_e164 to require 6-14 digits after '+' (7-15 total) - normalize_signal_target: Reuse is_valid_e164() for bare digit validation - parse_signal_target_parts: Add empty segment checks to all match arms - Reject empty uuid, group_id, phone, instance, or single segments - Prevents invalid targets like 'uuid:' or 'signal:work:' API state (state.rs): - Add signal_permissions field to ApiState - Add set_signal_permissions() method for hot-reload support toggle_platform (messaging.rs): - Add Signal case to toggle_platform for runtime adapter start/stop - Support both default and named Signal instances - Update existing ArcSwap pointee instead of creating new one when permissions exist send_message_to_another_channel: - parse_implicit_signal_shorthand: Use is_valid_e164() for strict validation - Add specific error messages for each validation failure case - Add early error for ambiguous Signal adapter selection when multiple named instances exist without a default, preventing broadcast() failure - Fix adapter resolution to distinguish default 'signal' from named adapters Tests (messaging.rs): - Update E.164 tests for 6-14 digit range (7-15 total)
1 parent 7c7d5b1 commit 6a5ca6f

5 files changed

Lines changed: 206 additions & 75 deletions

File tree

‎interface/src/components/ChannelSettingCard.tsx‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -648,12 +648,12 @@ export function AddInstanceCard({platform, isDefault, onCancel, onCreated}: AddI
648648
setMessage({text: "Account phone number is required", type: "error"});
649649
return;
650650
}
651-
// Basic E.164 validation (frontend)
651+
// Basic E.164 validation (frontend) - match backend rules
652652
const account = credentialInputs.signal_account.trim();
653-
const e164Regex = /^\+\d{7,}$/;
653+
const e164Regex = /^\+[1-9]\d{5,13}$/;
654654
if (!e164Regex.test(account)) {
655655
setMessage({
656-
text: "Account must be in E.164 format: + followed by 7+ digits (e.g., +1234567890)",
656+
text: "Account must be in E.164 format: + followed by 6-14 digits, first digit 1-9 (e.g., +1234567890)",
657657
type: "error"
658658
});
659659
return;
@@ -973,7 +973,7 @@ export function AddInstanceCard({platform, isDefault, onCancel, onCreated}: AddI
973973
onKeyDown={(e) => { if (e.key === "Enter") handleSave(); }}
974974
/>
975975
<p className="mt-1 text-xs text-ink-faint">
976-
Your Signal phone number in E.164 format (+ followed by 7+ digits)
976+
Your Signal phone number in E.164 format (+ followed by 6-14 digits)
977977
</p>
978978
</div>
979979
</>

‎src/api/messaging.rs‎

Lines changed: 87 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ pub(super) struct MessagingStatusResponse {
3434
email: PlatformStatus,
3535
webhook: PlatformStatus,
3636
twitch: PlatformStatus,
37+
signal: PlatformStatus,
3738
instances: Vec<AdapterInstanceStatus>,
3839
}
3940

@@ -194,7 +195,9 @@ pub(super) async fn messaging_status(
194195
) -> Result<Json<MessagingStatusResponse>, StatusCode> {
195196
let config_path = state.config_path.read().await.clone();
196197

197-
let (discord, slack, telegram, email, webhook, twitch, instances) = if config_path.exists() {
198+
let (discord, slack, telegram, email, webhook, twitch, signal, instances) = if config_path
199+
.exists()
200+
{
198201
let content = tokio::fs::read_to_string(&config_path)
199202
.await
200203
.map_err(|error| {
@@ -519,7 +522,7 @@ pub(super) async fn messaging_status(
519522
});
520523

521524
// Signal status and instances
522-
let _signal_status = doc
525+
let signal_status = doc
523526
.get("messaging")
524527
.and_then(|m| m.get("signal"))
525528
.map(|s| {
@@ -592,6 +595,7 @@ pub(super) async fn messaging_status(
592595
email_status,
593596
webhook_status,
594597
twitch_status,
598+
signal_status,
595599
instances,
596600
)
597601
} else {
@@ -605,7 +609,8 @@ pub(super) async fn messaging_status(
605609
default.clone(),
606610
default.clone(),
607611
default.clone(),
608-
default,
612+
default.clone(),
613+
default.clone(),
609614
Vec::new(),
610615
)
611616
};
@@ -617,6 +622,7 @@ pub(super) async fn messaging_status(
617622
email,
618623
webhook,
619624
twitch,
625+
signal,
620626
instances,
621627
}))
622628
}
@@ -1161,6 +1167,76 @@ pub(super) async fn toggle_platform(
11611167
}
11621168
}
11631169
}
1170+
"signal" => {
1171+
if let Some(signal_config) = &new_config.messaging.signal {
1172+
if !signal_config.http_url.is_empty() && !signal_config.account.is_empty() {
1173+
let permissions = {
1174+
let perms_guard = state.signal_permissions.read().await;
1175+
match perms_guard.as_ref() {
1176+
Some(existing) => {
1177+
// Update existing ArcSwap pointee
1178+
let perms = crate::config::SignalPermissions::from_config(
1179+
signal_config,
1180+
);
1181+
existing.store(std::sync::Arc::new(perms));
1182+
existing.clone()
1183+
}
1184+
None => {
1185+
drop(perms_guard);
1186+
let perms = crate::config::SignalPermissions::from_config(
1187+
signal_config,
1188+
);
1189+
let arc_swap = std::sync::Arc::new(
1190+
arc_swap::ArcSwap::from_pointee(perms),
1191+
);
1192+
state.set_signal_permissions(arc_swap.clone()).await;
1193+
arc_swap
1194+
}
1195+
}
1196+
};
1197+
let instance_dir = state.instance_dir.load();
1198+
let tmp_dir = instance_dir.join("tmp");
1199+
let adapter = crate::messaging::signal::SignalAdapter::new(
1200+
"signal",
1201+
&signal_config.http_url,
1202+
&signal_config.account,
1203+
signal_config.ignore_stories,
1204+
permissions,
1205+
tmp_dir,
1206+
);
1207+
if let Err(error) = manager.register_and_start(adapter).await {
1208+
tracing::error!(%error, "failed to start signal adapter on toggle");
1209+
}
1210+
}
1211+
1212+
for instance in signal_config
1213+
.instances
1214+
.iter()
1215+
.filter(|instance| instance.enabled)
1216+
{
1217+
let runtime_key = crate::config::binding_runtime_adapter_key(
1218+
"signal",
1219+
Some(instance.name.as_str()),
1220+
);
1221+
let permissions = std::sync::Arc::new(arc_swap::ArcSwap::from_pointee(
1222+
crate::config::SignalPermissions::from_instance_config(instance),
1223+
));
1224+
let instance_dir = state.instance_dir.load();
1225+
let tmp_dir = instance_dir.join("tmp");
1226+
let adapter = crate::messaging::signal::SignalAdapter::new(
1227+
runtime_key,
1228+
&instance.http_url,
1229+
&instance.account,
1230+
instance.ignore_stories,
1231+
permissions,
1232+
tmp_dir,
1233+
);
1234+
if let Err(error) = manager.register_and_start(adapter).await {
1235+
tracing::error!(%error, adapter = %instance.name, "failed to start named signal adapter on toggle");
1236+
}
1237+
}
1238+
}
1239+
}
11641240
_ => {}
11651241
}
11661242
}
@@ -1877,20 +1953,20 @@ mod tests {
18771953

18781954
#[test]
18791955
fn test_is_valid_e164_valid_numbers() {
1880-
// Valid E.164 numbers (minimum 7 digits after +)
1956+
// Valid E.164 numbers (6-14 digits after +, 7-15 total)
18811957
assert!(is_valid_e164("+1234567890"));
1882-
assert!(is_valid_e164("+1234567")); // Exactly 7 digits
1958+
assert!(is_valid_e164("+123456")); // Minimum: 6 digits after +
18831959
assert!(is_valid_e164("+14155552671"));
1884-
assert!(is_valid_e164("+123456789012345")); // Many digits
1960+
assert!(is_valid_e164("+12345678901234")); // Maximum: 14 digits after +
18851961
}
18861962

18871963
#[test]
18881964
fn test_is_valid_e164_invalid_numbers() {
18891965
// Missing +
18901966
assert!(!is_valid_e164("1234567890"));
18911967

1892-
// Too short (less than 7 digits)
1893-
assert!(!is_valid_e164("+123456"));
1968+
// Too short (less than 6 digits after +)
1969+
assert!(!is_valid_e164("+12345"));
18941970
assert!(!is_valid_e164("+123"));
18951971

18961972
// Empty or just +
@@ -1907,11 +1983,11 @@ mod tests {
19071983
assert!(!is_valid_e164("+1234567890 "));
19081984

19091985
// First digit is 0 (E.164 requires 1-9)
1910-
assert!(!is_valid_e164("+01234567"));
1986+
assert!(!is_valid_e164("+0123456"));
19111987
assert!(!is_valid_e164("+01234567890"));
19121988

1913-
// Too long (more than 15 digits)
1989+
// Too long (more than 14 digits after +)
1990+
assert!(!is_valid_e164("+123456789012345"));
19141991
assert!(!is_valid_e164("+1234567890123456"));
1915-
assert!(!is_valid_e164("+12345678901234567"));
19161992
}
19171993
}

‎src/api/state.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
use crate::agent::channel::ChannelState;
44
use crate::agent::cortex_chat::CortexChatSession;
55
use crate::agent::status::StatusBlock;
6-
use crate::config::{Binding, DefaultsConfig, DiscordPermissions, RuntimeConfig, SlackPermissions};
6+
use crate::config::{
7+
Binding, DefaultsConfig, DiscordPermissions, RuntimeConfig, SignalPermissions, SlackPermissions,
8+
};
79
use crate::conversation::worker_transcript::{ActionContent, TranscriptStep};
810
use crate::cron::{CronStore, Scheduler};
911
use crate::llm::LlmManager;
@@ -96,6 +98,8 @@ pub struct ApiState {
9698
pub discord_permissions: RwLock<Option<Arc<ArcSwap<DiscordPermissions>>>>,
9799
/// Shared reference to the Slack permissions ArcSwap (same instance used by the adapter and file watcher).
98100
pub slack_permissions: RwLock<Option<Arc<ArcSwap<SlackPermissions>>>>,
101+
/// Shared reference to the Signal permissions ArcSwap (same instance used by the adapter and file watcher).
102+
pub signal_permissions: RwLock<Option<Arc<ArcSwap<SignalPermissions>>>>,
99103
/// Shared reference to the bindings ArcSwap (same instance used by the main loop and file watcher).
100104
pub bindings: RwLock<Option<Arc<ArcSwap<Vec<Binding>>>>>,
101105
/// Shared messaging manager for runtime adapter addition.
@@ -316,6 +320,7 @@ impl ApiState {
316320
secrets_store: ArcSwap::from_pointee(None),
317321
discord_permissions: RwLock::new(None),
318322
slack_permissions: RwLock::new(None),
323+
signal_permissions: RwLock::new(None),
319324
bindings: RwLock::new(None),
320325
messaging_manager: RwLock::new(None),
321326
provider_setup_tx,
@@ -786,6 +791,11 @@ impl ApiState {
786791
*self.slack_permissions.write().await = Some(permissions);
787792
}
788793

794+
/// Share the Signal permissions ArcSwap with the API so reads get hot-reloaded values.
795+
pub async fn set_signal_permissions(&self, permissions: Arc<ArcSwap<SignalPermissions>>) {
796+
*self.signal_permissions.write().await = Some(permissions);
797+
}
798+
789799
/// Share the bindings ArcSwap with the API so reads get hot-reloaded values.
790800
pub async fn set_bindings(&self, bindings: Arc<ArcSwap<Vec<Binding>>>) {
791801
*self.bindings.write().await = Some(bindings);

‎src/messaging/target.rs‎

Lines changed: 44 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -275,12 +275,12 @@ fn normalize_email_target(raw_target: &str) -> Option<String> {
275275
/// Requirements:
276276
/// - Must start with '+'
277277
/// - First digit after '+' must be 1-9 (not 0)
278-
/// - Minimum 7 digits total
279-
/// - Maximum 15 digits total (E.164 standard)
278+
/// - Minimum 7 digits total (6 after '+')
279+
/// - Maximum 15 digits total (14 after '+', E.164 standard)
280280
/// - All characters after '+' must be ASCII digits
281281
pub fn is_valid_e164(phone: &str) -> bool {
282282
if let Some(digits) = phone.strip_prefix('+') {
283-
if digits.len() < 7 || digits.len() > 15 {
283+
if digits.len() < 6 || digits.len() > 14 {
284284
return false;
285285
}
286286
// First digit must be 1-9 (not 0)
@@ -334,9 +334,12 @@ fn normalize_signal_target(raw_target: &str) -> Option<String> {
334334
return Some(format!("uuid:{target}"));
335335
}
336336

337-
// Check if it's a bare phone number (7+ digits required for E.164)
338-
if target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 {
339-
return Some(format!("+{target}"));
337+
// Check if it's a bare phone number (E.164 format)
338+
if target.chars().all(|c| c.is_ascii_digit()) {
339+
let with_plus = format!("+{target}");
340+
if is_valid_e164(&with_plus) {
341+
return Some(with_plus);
342+
}
340343
}
341344

342345
None
@@ -400,60 +403,68 @@ fn extract_signal_adapter_from_channel_id(channel_id: &str) -> String {
400403
pub fn parse_signal_target_parts(parts: &[&str]) -> Option<BroadcastTarget> {
401404
match parts {
402405
// Default adapter: signal:uuid:xxx, signal:group:xxx, signal:e164:+xxx, signal:+xxx
403-
["uuid", uuid] => Some(BroadcastTarget {
406+
["uuid", uuid] if !uuid.is_empty() => Some(BroadcastTarget {
404407
adapter: "signal".to_string(),
405408
target: format!("uuid:{uuid}"),
406409
}),
407-
["group", group_id] => Some(BroadcastTarget {
410+
["group", group_id] if !group_id.is_empty() => Some(BroadcastTarget {
408411
adapter: "signal".to_string(),
409412
target: format!("group:{group_id}"),
410413
}),
411414
// Use normalize_signal_target for phone/e164 to ensure consistent parsing
412-
["e164", phone] => {
413-
normalize_signal_target(&format!("e164:{phone}")).map(|target| BroadcastTarget {
415+
["e164", phone] if !phone.is_empty() => normalize_signal_target(&format!("e164:{phone}"))
416+
.map(|target| BroadcastTarget {
414417
adapter: "signal".to_string(),
415418
target,
416-
})
417-
}
418-
[phone] if phone.starts_with('+') => {
419-
normalize_signal_target(phone).map(|target| BroadcastTarget {
419+
}),
420+
[phone] if phone.starts_with('+') && !phone.is_empty() => normalize_signal_target(phone)
421+
.map(|target| BroadcastTarget {
422+
adapter: "signal".to_string(),
423+
target,
424+
}),
425+
// Single-part targets: delegate to normalize_signal_target for bare UUIDs/phones
426+
[single] if !single.is_empty() => {
427+
normalize_signal_target(single).map(|target| BroadcastTarget {
420428
adapter: "signal".to_string(),
421429
target,
422430
})
423431
}
424-
// Single-part targets: delegate to normalize_signal_target for bare UUIDs/phones
425-
[single] => normalize_signal_target(single).map(|target| BroadcastTarget {
426-
adapter: "signal".to_string(),
427-
target,
428-
}),
429432
// Named adapter: signal:instance:uuid:xxx, signal:instance:group:xxx
430-
[instance, "uuid", uuid] => Some(BroadcastTarget {
431-
adapter: format!("signal:{instance}"),
432-
target: format!("uuid:{uuid}"),
433-
}),
434-
[instance, "group", group_id] => Some(BroadcastTarget {
435-
adapter: format!("signal:{instance}"),
436-
target: format!("group:{group_id}"),
437-
}),
433+
[instance, "uuid", uuid] if !instance.is_empty() && !uuid.is_empty() => {
434+
Some(BroadcastTarget {
435+
adapter: format!("signal:{instance}"),
436+
target: format!("uuid:{uuid}"),
437+
})
438+
}
439+
[instance, "group", group_id] if !instance.is_empty() && !group_id.is_empty() => {
440+
Some(BroadcastTarget {
441+
adapter: format!("signal:{instance}"),
442+
target: format!("group:{group_id}"),
443+
})
444+
}
438445
// Named adapter: signal:instance:e164:+xxx - use normalize_signal_target
439-
[instance, "e164", phone] => {
446+
[instance, "e164", phone] if !instance.is_empty() && !phone.is_empty() => {
440447
normalize_signal_target(&format!("e164:{phone}")).map(|target| BroadcastTarget {
441448
adapter: format!("signal:{instance}"),
442449
target,
443450
})
444451
}
445452
// Named adapter: signal:instance:+xxx - use normalize_signal_target
446-
[instance, phone] if phone.starts_with('+') => {
453+
[instance, phone]
454+
if !instance.is_empty() && phone.starts_with('+') && !phone.is_empty() =>
455+
{
447456
normalize_signal_target(phone).map(|target| BroadcastTarget {
448457
adapter: format!("signal:{instance}"),
449458
target,
450459
})
451460
}
452461
// Named adapter with single-part target: delegate to normalize_signal_target
453-
[instance, single] => normalize_signal_target(single).map(|target| BroadcastTarget {
454-
adapter: format!("signal:{instance}"),
455-
target,
456-
}),
462+
[instance, single] if !instance.is_empty() && !single.is_empty() => {
463+
normalize_signal_target(single).map(|target| BroadcastTarget {
464+
adapter: format!("signal:{instance}"),
465+
target,
466+
})
467+
}
457468
_ => None,
458469
}
459470
}

0 commit comments

Comments
 (0)