Conversation
New `moltis-matrix` crate using matrix-sdk 0.16 (official Rust Matrix SDK). Supports DM and room messaging with allowlist/OTP gating, streaming via m.replace edit-in-place, reactions, typing indicators, and invite auto-join. - `MatrixPlugin` implementing `ChannelPlugin` with session restore and persistent sync loop via `CancellationToken` - `ChannelOutbound`: send_text (markdown), send_html, send_media, send_typing, send_interactive (text fallback), add_reaction - `ChannelStreamOutbound`: edit-in-place streaming with 500ms throttle - Access control: DM/room policies, user/room allowlists, OTP challenge - `ChannelType::Matrix` variant with descriptor (GatewayLoop, streaming, threads, reactions, OTP) - Feature-gated in gateway and CLI (`matrix` feature, default-enabled) - matrix-sdk default-features disabled to avoid libsqlite3-sys conflict with sqlx (same pattern as WhatsApp crate) - 11 tests: config round-trip, access control, descriptor coherence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR adds a new
Confidence Score: 4/5Not safe to merge as-is — plaintext token serialization and silent E2EE misconfiguration are real security defects that should be resolved first. Four P1 findings block merge: (1) Matrix access token written in plaintext into account_config_json output; (2) e2ee defaults to true but the SDK feature is absent, providing no encryption silently; (3) DM detection by member count can misclassify rooms, enabling an access-control bypass; (4) unwrap() on user_id can panic the gateway process. The crate structure, sync-loop cancellation, OTP flow, and streaming are solid — fixing these four points would make this ready to merge. crates/matrix/src/config.rs (token serialization), crates/matrix/Cargo.toml (e2ee feature), crates/matrix/src/access.rs (DM detection), crates/matrix/src/plugin.rs (unwrap on user_id) Important Files Changed
Sequence DiagramsequenceDiagram
participant User as Matrix User
participant SDK as matrix-sdk (sync loop)
participant Handler as handler.rs
participant Access as access.rs
participant OTP as OtpState
participant Sink as ChannelEventSink
participant Outbound as MatrixOutbound
User->>SDK: sends room message
SDK->>Handler: handle_room_message(ev, room)
Handler->>Handler: skip if sender == bot_user_id
Handler->>Access: check_access(config, chat_type, sender, room)
alt Access denied + OTP eligible
Handler->>OTP: verify(sender, code) or initiate(sender)
OTP-->>Handler: OtpVerifyResult / OtpInitResult
Handler->>User: send OTP prompt or result via send_text()
Handler->>Sink: emit OtpChallenge / OtpResolved
else Access granted
Handler->>Sink: emit InboundMessage
Handler->>Sink: dispatch_to_chat(body, reply_to, meta)
Sink->>Outbound: send_stream() or send_text()
alt Streaming
Outbound->>User: initial message (>=30 chars buffered)
loop every 500ms while tokens arrive
Outbound->>User: m.replace edit-in-place
end
Outbound->>User: final edit (StreamEvent::Done)
else Non-streaming
Outbound->>User: RoomMessageEventContent::text_markdown
end
end
User->>SDK: room invite received
SDK->>Handler: handle_invite(ev, room)
alt auto_join = true
Handler->>SDK: room.join()
end
Reviews (1): Last reviewed commit: "feat(channels): add Matrix channel integ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn serialize_secret<S: serde::Serializer>( | ||
| secret: &Secret<String>, | ||
| serializer: S, | ||
| ) -> Result<S::Ok, S::Error> { | ||
| serializer.serialize_str(secret.expose_secret()) | ||
| } |
There was a problem hiding this comment.
The custom serializer on lines 139–144 calls expose_secret() and writes the raw access token into the JSON output. Because account_config_json() in plugin.rs (line 252–256) calls serde_json::to_value(&s.config), any call to that method — from an admin API, config dump, or log statement — will expose the plaintext credential.
The serializer should redact the value instead of exposing it:
serializer.serialize_str("[REDACTED]")Config persistence that genuinely needs the raw token should use a separate, explicitly gated serialization path rather than the general-purpose account_config_json method.
| version.workspace = true | ||
|
|
||
| [dependencies] | ||
| matrix-sdk = { default-features = false, features = ["markdown", "native-tls"], version = "0.16" } |
There was a problem hiding this comment.
e2ee config field is silently non-functional
The MatrixAccountConfig exposes an e2ee field that defaults to true (config.rs line 85), but matrix-sdk is compiled without the e2ee feature here. As a result, the SDK will never perform end-to-end encryption regardless of the config value — users who rely on e2ee = true for privacy will get plaintext traffic without any warning.
Either add the e2ee feature to the dependency:
matrix-sdk = { default-features = false, features = ["markdown", "native-tls", "e2ee"], version = "0.16" }Or remove the e2ee field from MatrixAccountConfig and document that E2EE is not yet supported.
| pub fn is_dm_room(joined_member_count: u64) -> ChatType { | ||
| if joined_member_count <= 2 { | ||
| ChatType::Dm | ||
| } else { | ||
| ChatType::Group | ||
| } | ||
| } |
There was a problem hiding this comment.
is_dm_room classifies any room with ≤ 2 members as a DM. This creates two correctness problems:
- False DM (security bypass): A 2-person group chat (the bot + one other user) is classified as a DM. If
dm_policyisOpenbutroom_policyisAllowlist, a user in an unlisted group chat gains access because the room is wrongly treated as a DM. - False group: A bridged or federated DM can easily have 3 participants, causing it to be treated as a group room and rejected under a restrictive
room_policy.
Matrix exposes a proper is_direct() method on Room (backed by m.direct account data). The handler should pass room.is_direct() to the access layer rather than using member count.
// In handler.rs, replace:
let member_count = room.joined_members_count();
let chat_type = is_dm_room(member_count);
// With:
let chat_type = if room.is_direct().await.unwrap_or(false) {
ChatType::Dm
} else {
ChatType::Group
};And remove the is_dm_room helper from access.rs.
| .await | ||
| .map_err(|e| ChannelError::external("matrix session restore", e))?; | ||
|
|
||
| let bot_user_id = client.user_id().unwrap().to_owned(); |
There was a problem hiding this comment.
client.user_id() returns Option<&UserId>. Calling .unwrap() here panics the entire gateway process if the SDK doesn't populate the user_id — which can happen if restore_session silently fails to set session metadata in an edge case.
Since the error is recoverable, prefer propagating it:
let bot_user_id = client
.user_id()
.ok_or_else(|| ChannelError::external("matrix session restore", "user_id not set after restore_session"))?
.to_owned();|
|
||
| match result { | ||
| OtpInitResult::Created(code) => { | ||
| let expires_at = unix_now() + 300; |
There was a problem hiding this comment.
otp_cooldown_secs
expires_at is always unix_now() + 300 (5 minutes), even though MatrixAccountConfig has an otp_cooldown_secs field. The OTP challenge event emitted to the sink will have an expiry that diverges from what OtpState actually enforces, potentially confusing admin UIs that display when a code expires.
let expires_at = unix_now() + config.otp_cooldown_secs as i64;| async fn remove_reaction( | ||
| &self, | ||
| account_id: &str, | ||
| _channel_id: &str, | ||
| message_id: &str, | ||
| emoji: &str, | ||
| ) -> ChannelResult<()> { | ||
| debug!( | ||
| account_id, | ||
| message_id, emoji, "remove_reaction not yet implemented for Matrix" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
remove_reaction silently succeeds without doing anything
The method returns Ok(()) after a debug! log, so callers get no indication that the reaction was not removed. This can cause stale reactions to remain visible in the client. Consider at minimum a warn! log:
warn!(
account_id,
message_id, emoji, "remove_reaction not yet implemented for Matrix — reaction will remain visible"
);| fn html_to_plain(html: &str) -> String { | ||
| html.replace("<br>", "\n") | ||
| .replace("<br/>", "\n") | ||
| .replace("<br />", "\n") | ||
| .replace("<p>", "") | ||
| .replace("</p>", "\n") | ||
| .replace("<b>", "**") | ||
| .replace("</b>", "**") | ||
| .replace("<strong>", "**") | ||
| .replace("</strong>", "**") | ||
| .replace("<i>", "_") | ||
| .replace("</i>", "_") | ||
| .replace("<em>", "_") | ||
| .replace("</em>", "_") | ||
| .replace("<code>", "`") | ||
| .replace("</code>", "`") | ||
| } |
There was a problem hiding this comment.
html_to_plain only handles a fixed set of known tags. Any other HTML element — <div>, <span>, <ul>, <li>, <a href="...">, <script>, etc. — will be passed through verbatim to the Matrix plain-text body. The Matrix spec requires the plain-text body to be human-readable.
Consider using a lightweight HTML-stripping crate (e.g. ammonia or htmd), or add a residual tag-stripping pass after the existing replacements.
Merging this PR will not alter performance
Comparing 
Footnotes
|
2 participants
Summary
Cherry-picked from #331 (which contained multiple unrelated features).
moltis-matrixcrate using matrix-sdk 0.16 (official Rust Matrix SDK, powers Element X)matrixfeature, default-enabled)moltis-discordWhat's included
crates/channels/src/plugin.rsChannelType::Matrixvariant + descriptorcrates/matrix/src/plugin.rsChannelPluginimpl — session restore, sync loop, cancellationcrates/matrix/src/outbound.rsChannelOutbound+ChannelStreamOutbound— text, html, media, reactions, streamingcrates/matrix/src/handler.rscrates/matrix/src/access.rscrates/matrix/src/config.rsMatrixAccountConfigwithChannelConfigViewcrates/matrix/src/state.rsAccountStatewith Client, CancellationToken, OTPcrates/matrix/src/error.rsValidation
Completed
cargo check -p moltis-matrixcargo test -p moltis-matrixRemaining
./scripts/local-validate.shManual QA
moltis.tomlwith a test homeserverSupersedes the Matrix portion of #331.