refactor(channels): introduce ExternalThreadId newtype at channel boundary (#2685)

* refactor(channels): introduce ExternalThreadId newtype at channel boundary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from #2349, #2444, #2517 where thread-id confusion crossed a layer silently. * fix(bridge): adapt test thread_id to ExternalThreadId newtype Post-merge fix: a test added in staging (insert_and_notify_pending_gate_uses_extension_manager_for_auth_display_name) assigned a raw String to message.thread_id, but the field type became ExternalThreadId on this branch. Wrap with ExternalThreadId::from_trusted to match the other tests in the same module. * refactor(types): address review feedback — byte units, shared validate, try_-variants, dedup pending-gate * refactor(types): validate scope_thread_id + relay respond prefers typed msg.thread_id - router.rs: scope_thread_id written to PendingGate was wrapped via ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted WASM/metadata-sourced strings. Now validates via ExternalThreadId::new; invalid values log at debug and store None. Applied at both call sites (authentication-fallback path and generic gate-insertion path). - relay/channel.rs: respond() derived thread_id only from response or metadata — now also consults the validated msg.thread_id as the second fallback (before raw metadata) and filters empty strings so we never emit thread_ts: "" to Slack.
2026-09-02 23:56:24 +08:00 · 2026-04-20 15:29:04 +09:00
parent 0476a3d8e9
commit 833cb4844f
24 changed files with 785 additions and 99 deletions
--- a/crates/ironclaw_common/src/identity.rs
+++ b/crates/ironclaw_common/src/identity.rs
@@ -248,6 +248,302 @@ impl ExtensionName {
    }
 }

+/// Maximum length for an [`ExternalThreadId`], measured in bytes.
+///
+/// Chosen to accommodate Slack's compound `thread_ts` identifiers, web-UI
+/// generated UUID strings, Telegram chat IDs, and comparable channel-specific
+/// thread tokens, while still bounding what we'll accept from an external
+/// system.
+pub const MAX_EXTERNAL_THREAD_ID_LEN: usize = 512;
+
+/// Why a candidate string is not a valid external thread id.
+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
+pub enum ExternalThreadIdError {
+    #[error("external thread id must not be empty")]
+    Empty,
+    #[error("external thread id exceeds {MAX_EXTERNAL_THREAD_ID_LEN} bytes")]
+    TooLong,
+    #[error("external thread id must not contain NUL bytes")]
+    ContainsNul,
+}
+
+/// External (channel-supplied) thread identifier — e.g. a Telegram chat id,
+/// a Slack `thread_ts`, a web-UI-generated UUID string.
+///
+/// **Not** the internal engine `ThreadId(Uuid)`. Channels supply whatever
+/// shape their platform uses; [`crate::identity::ExternalThreadId`] is the
+/// typed boundary representation that carries that raw string safely across
+/// internal module boundaries. Conversion to an internal UUID happens inside
+/// `SessionManager::resolve_thread` and equivalents.
+///
+/// See `.claude/rules/types.md` for why this is a newtype.
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct ExternalThreadId(String);
+
+impl ExternalThreadId {
+    /// Construct from any string-like value, validating length and
+    /// disallowing NUL bytes. Returns [`ExternalThreadIdError`] on failure.
+    ///
+    /// Length is measured in bytes via `str::len`.
+    pub fn new(raw: impl AsRef<str>) -> Result<Self, ExternalThreadIdError> {
+        Self::validate(raw.as_ref())?;
+        Ok(Self(raw.as_ref().to_string()))
+    }
+
+    /// Validate a candidate string without constructing.
+    ///
+    /// Shared by `new` (which allocates) and `TryFrom<String>` (which
+    /// consumes the owned String without reallocating). Length is
+    /// measured in bytes via `str::len`.
+    fn validate(s: &str) -> Result<(), ExternalThreadIdError> {
+        if s.is_empty() {
+            return Err(ExternalThreadIdError::Empty);
+        }
+        if s.len() > MAX_EXTERNAL_THREAD_ID_LEN {
+            return Err(ExternalThreadIdError::TooLong);
+        }
+        if s.contains('\0') {
+            return Err(ExternalThreadIdError::ContainsNul);
+        }
+        Ok(())
+    }
+
+    /// Construct without validation.
+    ///
+    /// Use for values sourced from a typed upstream that the caller already
+    /// trusts — a DB row, a persisted pending-gate payload, or a
+    /// `#[serde(transparent)]` deserialization whose wire contract predates
+    /// the newtype. Prefer [`Self::new`] for anything touching external input.
+    pub fn from_trusted(raw: String) -> Self {
+        Self(raw)
+    }
+
+    /// Borrow the inner string.
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+
+    /// Consume and return the inner `String`.
+    pub fn into_inner(self) -> String {
+        self.0
+    }
+}
+
+impl fmt::Display for ExternalThreadId {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+// Intentionally no `Deref<Target = str>`, no `From<String>`, no
+// `From<&str>`: the whole point of this newtype is to force callers to
+// make the boundary crossing explicit via `new` (validating) or
+// `from_trusted` (documented opt-out).
+impl AsRef<str> for ExternalThreadId {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl TryFrom<&str> for ExternalThreadId {
+    type Error = ExternalThreadIdError;
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        Self::new(value)
+    }
+}
+
+impl TryFrom<String> for ExternalThreadId {
+    type Error = ExternalThreadIdError;
+    fn try_from(value: String) -> Result<Self, Self::Error> {
+        Self::validate(&value)?;
+        Ok(Self(value))
+    }
+}
+
+impl FromStr for ExternalThreadId {
+    type Err = ExternalThreadIdError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Self::new(s)
+    }
+}
+
+impl From<ExternalThreadId> for String {
+    fn from(value: ExternalThreadId) -> String {
+        value.0
+    }
+}
+
+impl PartialEq<str> for ExternalThreadId {
+    fn eq(&self, other: &str) -> bool {
+        self.0 == other
+    }
+}
+
+impl PartialEq<&str> for ExternalThreadId {
+    fn eq(&self, other: &&str) -> bool {
+        self.0 == *other
+    }
+}
+
+/// Maximum length for an [`McpServerName`], measured in bytes.
+///
+/// MCP server names are used as tool-name prefixes in LLM providers (which
+/// typically require `^[a-zA-Z0-9_-]+$`), as components of secret-store keys
+/// (e.g. `mcp_<name>_access_token`), and as filesystem-adjacent identifiers.
+/// 64 bytes matches the shared `MAX_NAME_LEN` used for other identity names.
+pub const MAX_MCP_SERVER_NAME_LEN: usize = 64;
+
+/// Why a candidate string is not a valid MCP server name.
+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
+pub enum McpServerNameError {
+    #[error("MCP server name must not be empty")]
+    Empty,
+    #[error("MCP server name exceeds {MAX_MCP_SERVER_NAME_LEN} bytes")]
+    TooLong,
+    #[error(
+        "MCP server name '{0}' contains invalid characters \
+         (only alphanumeric, dash, underscore are allowed)"
+    )]
+    InvalidChar(String),
+}
+
+/// MCP server identifier — e.g. `notion`, `github`, `my-server`.
+///
+/// The allowlist rules mirror the pre-newtype check that landed in #2400:
+/// alphanumeric, dash, and underscore only. These rules are intentionally
+/// more permissive than [`CredentialName`] / [`ExtensionName`] because MCP
+/// server names were historically free-form — we reject shell metacharacters
+/// and path separators but still accept uppercase letters and dashes. The
+/// character set is a superset of what LLM providers accept for tool-name
+/// prefixes (`^[a-zA-Z0-9_-]+$`).
+///
+/// Callers must go through [`Self::new`] (validating) or
+/// [`Self::from_trusted`] (documented opt-out, e.g. for values already
+/// validated at load time). Deliberately no `From<String>` / `From<&str>`.
+///
+/// `#[serde(transparent)]` preserves on-wire compatibility — legacy config
+/// rows continue to deserialize cleanly, and invalid values are only
+/// surfaced when re-validated through [`Self::new`]. See
+/// `.claude/rules/types.md`.
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct McpServerName(String);
+
+impl McpServerName {
+    /// Construct from any string-like value, validating the allowlist.
+    ///
+    /// Rejects empty strings, strings longer than
+    /// [`MAX_MCP_SERVER_NAME_LEN`] bytes (length is measured in bytes
+    /// via `str::len`), and strings containing any character outside the
+    /// allowlist (alphanumeric, `-`, `_`). Path separators, shell
+    /// metacharacters, NUL bytes, and whitespace all fall into the
+    /// invalid-character bucket.
+    pub fn new(raw: impl AsRef<str>) -> Result<Self, McpServerNameError> {
+        Self::validate(raw.as_ref())?;
+        Ok(Self(raw.as_ref().to_string()))
+    }
+
+    /// Validate a candidate string without constructing.
+    ///
+    /// Shared by `new` (which allocates) and `TryFrom<String>` (which
+    /// consumes the owned String without reallocating). Length is
+    /// measured in bytes via `str::len`.
+    fn validate(s: &str) -> Result<(), McpServerNameError> {
+        if s.is_empty() {
+            return Err(McpServerNameError::Empty);
+        }
+        if s.len() > MAX_MCP_SERVER_NAME_LEN {
+            return Err(McpServerNameError::TooLong);
+        }
+        if !s
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
+        {
+            return Err(McpServerNameError::InvalidChar(s.to_string()));
+        }
+        Ok(())
+    }
+
+    /// Construct without validation.
+    ///
+    /// Use for values sourced from a typed upstream that the caller already
+    /// trusts — an already-validated config row, a canonicalised name after
+    /// hyphen-to-underscore folding in the factory, or a
+    /// `#[serde(transparent)]` deserialization whose wire contract predates
+    /// the newtype. Prefer [`Self::new`] for anything touching external
+    /// input.
+    pub fn from_trusted(raw: String) -> Self {
+        Self(raw)
+    }
+
+    /// Borrow the inner string.
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+
+    /// Consume and return the inner `String`.
+    pub fn into_inner(self) -> String {
+        self.0
+    }
+}
+
+impl fmt::Display for McpServerName {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+// Intentionally no `Deref<Target = str>`, no `From<String>`, no
+// `From<&str>`: the whole point of this newtype is to force callers to
+// make the boundary crossing explicit via `new` (validating) or
+// `from_trusted` (documented opt-out).
+impl AsRef<str> for McpServerName {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl TryFrom<&str> for McpServerName {
+    type Error = McpServerNameError;
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        Self::new(value)
+    }
+}
+
+impl TryFrom<String> for McpServerName {
+    type Error = McpServerNameError;
+    fn try_from(value: String) -> Result<Self, Self::Error> {
+        Self::validate(&value)?;
+        Ok(Self(value))
+    }
+}
+
+impl FromStr for McpServerName {
+    type Err = McpServerNameError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Self::new(s)
+    }
+}
+
+impl From<McpServerName> for String {
+    fn from(value: McpServerName) -> String {
+        value.0
+    }
+}
+
+impl PartialEq<str> for McpServerName {
+    fn eq(&self, other: &str) -> bool {
+        self.0 == other
+    }
+}
+
+impl PartialEq<&str> for McpServerName {
+    fn eq(&self, other: &&str) -> bool {
+        self.0 == *other
+    }
+}
+
 #[cfg(test)]
 mod tests {
    use super::*;
@@ -425,6 +721,90 @@ mod tests {
        assert_eq!(via_as_ref, "gmail");
    }

+    // ---- ExternalThreadId tests ----
+
+    #[test]
+    fn external_thread_id_accepts_common_channel_shapes() {
+        // Telegram-style numeric chat id
+        assert_eq!(
+            ExternalThreadId::new("123456789").unwrap().as_str(),
+            "123456789"
+        );
+        // Web UI UUID
+        assert_eq!(
+            ExternalThreadId::new("550e8400-e29b-41d4-a716-446655440000")
+                .unwrap()
+                .as_str(),
+            "550e8400-e29b-41d4-a716-446655440000"
+        );
+        // Slack thread_ts
+        assert_eq!(
+            ExternalThreadId::new("1234567890.123456").unwrap().as_str(),
+            "1234567890.123456"
+        );
+        // Generic text with mixed punctuation — channels define shape
+        assert!(ExternalThreadId::new("room:general").is_ok());
+    }
+
+    #[test]
+    fn external_thread_id_rejects_empty() {
+        assert_eq!(ExternalThreadId::new(""), Err(ExternalThreadIdError::Empty));
+    }
+
+    #[test]
+    fn external_thread_id_rejects_too_long() {
+        let long = "a".repeat(MAX_EXTERNAL_THREAD_ID_LEN + 1);
+        assert_eq!(
+            ExternalThreadId::new(&long),
+            Err(ExternalThreadIdError::TooLong)
+        );
+    }
+
+    #[test]
+    fn external_thread_id_rejects_nul() {
+        assert_eq!(
+            ExternalThreadId::new("abc\0def"),
+            Err(ExternalThreadIdError::ContainsNul)
+        );
+    }
+
+    #[test]
+    fn external_thread_id_serde_is_transparent() {
+        let tid = ExternalThreadId::new("thread-xyz").unwrap();
+        let json = serde_json::to_string(&tid).unwrap();
+        assert_eq!(json, "\"thread-xyz\"");
+
+        let round: ExternalThreadId = serde_json::from_str("\"thread-xyz\"").unwrap();
+        assert_eq!(round.as_str(), "thread-xyz");
+    }
+
+    /// Like the other identity newtypes, `#[serde(transparent)]` means we
+    /// do not re-validate at deserialize time — legacy persisted rows must
+    /// keep loading. Validation happens at construction sites.
+    #[test]
+    fn external_thread_id_serde_does_not_revalidate() {
+        // Even an empty string deserializes — we only reject via `new`.
+        let legacy: ExternalThreadId = serde_json::from_str("\"\"").unwrap();
+        assert_eq!(legacy.as_str(), "");
+    }
+
+    #[test]
+    fn external_thread_id_from_trusted_preserves_raw() {
+        let raw = "unvalidated::value".to_string();
+        let tid = ExternalThreadId::from_trusted(raw.clone());
+        assert_eq!(tid.as_str(), raw);
+    }
+
+    #[test]
+    fn external_thread_id_distinct_from_extension_name() {
+        let ext = ExtensionName::new("telegram").unwrap();
+        let tid = ExternalThreadId::new("telegram").unwrap();
+        // Compile-time distinction — both have the same inner shape but
+        // are different types, so a function signature requiring one will
+        // reject the other at the call site.
+        assert_eq!(ext.as_str(), tid.as_str());
+    }
+
    #[test]
    fn preserves_existing_credential_shape() {
        // Every credential name used in the codebase today (as of the
@@ -441,4 +821,113 @@ mod tests {
            assert!(CredentialName::new(ok).is_ok(), "expected {ok} to validate",);
        }
    }
+
+    // ---- McpServerName tests ----
+
+    #[test]
+    fn mcp_server_name_accepts_allowlist_characters() {
+        // Alphanumeric, dashes, underscores, mixed case are all accepted —
+        // this mirrors the pre-newtype `McpServerConfig::validate` coverage
+        // (`test_server_name_valid_characters_accepted`).
+        for ok in ["notion", "my-server", "my_server", "MCP-1", "server123"] {
+            let name = McpServerName::new(ok).expect("should accept allowlist chars");
+            assert_eq!(name.as_str(), ok);
+        }
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_shell_metacharacters() {
+        // Regression: the allowlist originated in #2400 as defence against
+        // shell-metacharacter injection when the name is interpolated into
+        // secret keys or tool-name prefixes.
+        for bad in [
+            "server; rm -rf /",
+            "server$(whoami)",
+            "server`id`",
+            "server|cat /etc/passwd",
+            "server&bg",
+            "server>out",
+            "server<in",
+            "name with spaces",
+        ] {
+            assert!(
+                matches!(
+                    McpServerName::new(bad),
+                    Err(McpServerNameError::InvalidChar(_))
+                ),
+                "expected {bad:?} to be rejected"
+            );
+        }
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_path_separators() {
+        for bad in ["../etc/passwd", "server/name", "server\\name"] {
+            assert!(
+                matches!(
+                    McpServerName::new(bad),
+                    Err(McpServerNameError::InvalidChar(_))
+                ),
+                "expected {bad:?} to be rejected"
+            );
+        }
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_dots() {
+        // Dots are rejected because server names are used as tool-name
+        // prefixes and LLM providers require `^[a-zA-Z0-9_-]+$`.
+        assert!(matches!(
+            McpServerName::new("my.server"),
+            Err(McpServerNameError::InvalidChar(_))
+        ));
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_null_byte() {
+        assert!(matches!(
+            McpServerName::new("server\0name"),
+            Err(McpServerNameError::InvalidChar(_))
+        ));
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_empty() {
+        assert_eq!(McpServerName::new(""), Err(McpServerNameError::Empty));
+    }
+
+    #[test]
+    fn mcp_server_name_rejects_too_long() {
+        let long = "a".repeat(MAX_MCP_SERVER_NAME_LEN + 1);
+        assert_eq!(McpServerName::new(&long), Err(McpServerNameError::TooLong));
+    }
+
+    #[test]
+    fn mcp_server_name_serde_is_transparent() {
+        let name = McpServerName::new("notion").unwrap();
+        let json = serde_json::to_string(&name).unwrap();
+        assert_eq!(json, "\"notion\"");
+
+        let round: McpServerName = serde_json::from_str("\"notion\"").unwrap();
+        assert_eq!(round.as_str(), "notion");
+    }
+
+    /// Like the other identity newtypes, `#[serde(transparent)]` means we
+    /// do not re-validate at deserialize time — legacy persisted
+    /// `mcp-servers.json` rows must keep loading. Validation happens at
+    /// construction sites (e.g. `McpServerConfig::validate`), not on the
+    /// wire.
+    #[test]
+    fn mcp_server_name_serde_does_not_revalidate() {
+        let legacy: McpServerName = serde_json::from_str("\"bad;server\"").unwrap();
+        assert_eq!(legacy.as_str(), "bad;server");
+    }
+
+    #[test]
+    fn mcp_server_name_from_trusted_preserves_raw() {
+        // `from_trusted` is the documented escape hatch for canonicalised
+        // post-validation values (e.g. after the factory folds hyphens).
+        let name = McpServerName::from_trusted("my_server".to_string());
+        assert_eq!(name.as_str(), "my_server");
+    }
 }
--- a/crates/ironclaw_common/src/lib.rs
+++ b/crates/ironclaw_common/src/lib.rs
@@ -9,7 +9,10 @@ pub use event::{
    AppEvent, JobResultStatus, JobResultStatusParseError, OnboardingStateDto, PlanStepDto,
    ToolDecisionDto,
 };
-pub use identity::{CredentialName, ExtensionName, IdentityError, MAX_NAME_LEN};
+pub use identity::{
+    CredentialName, ExtensionName, ExternalThreadId, ExternalThreadIdError, IdentityError,
+    MAX_EXTERNAL_THREAD_ID_LEN, MAX_NAME_LEN,
+};
 pub use timezone::{ValidTimezone, deserialize_option_lenient};
 pub use util::truncate_preview;