Skip to content

fix(matrix-bridge): markdown link-scheme whitelist + null-byte hardening#19

Merged
replicas-connector[bot] merged 2 commits into
mainfrom
fix/matrix-markdown-link-scheme-xss
May 30, 2026
Merged

fix(matrix-bridge): markdown link-scheme whitelist + null-byte hardening#19
replicas-connector[bot] merged 2 commits into
mainfrom
fix/matrix-markdown-link-scheme-xss

Conversation

@replicas-connector

@replicas-connector replicas-connector Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Round-3 audit focused on the agent-output → Matrix HTML rendering layer (src/markdown.ts, src/render.ts, src/matrix.ts). Two findings shipped, both in src/markdown.ts.

Findings shipped

MED — Link scheme whitelist (src/markdown.ts)

  • [text](url) previously emitted <a href="${url}"> with only " escaped. An agent producing malicious markdown (prompt-injected web content, agent-as-attacker scenarios) could craft [click](javascript:alert(1)) and rely on whatever the downstream Matrix client did with non-http schemes.
  • Matrix spec says clients SHOULD filter to http/https/ftp/mailto/magnet, but actual behavior varies across Element/Beeper/native bridges/custom clients.
  • Now whitelisted: http(s), ftp, mailto, magnet + relative paths + fragment-only links. Everything else renders as bracketed plain text — user still sees the emitted content but it can't be clicked into an XSS.

LOW — NULL-byte placeholder hardening (src/markdown.ts)

  • markdownToTelegramHtml uses \u0000PH<n>\u0000 as the sentinel for code-block extraction. An agent emitting a literal NULL-bounded PH0 in its body could theoretically corrupt the splice-back step (substituting one of its own extracted code blocks).
  • Strip NULL bytes up front — never legitimately part of Markdown body.

Other files — clean

  • src/render.ts — escaping is consistent, every line-array element is HTML-ready before render time, parsePlan is opaque to adversarial input.
  • src/matrix.ts — call sites handle 429 correctly via MatrixError.retryAfterMs, stripHtml unescape order is correct (verified ampersand precedence), pin/unpin handles missing-power gracefully.

Validation

  • npm run typecheck
  • npm test ✅ 102/102 tests (was 97 — added 5: javascript:/data: deny, http/https/mailto/ftp allow, relative paths, NULL-byte strip)
  • Deployed (ae74f9ee-616a-4692-bed0-239bdcee1881)

Test plan

  • Typecheck clean
  • Existing + new tests pass (102/102)
  • Deploy succeeds
  • After merge: confirm normal http(s) markdown links still render correctly in Beeper
  • Confirm [click](javascript:...) renders as plain bracketed text

Created by Jaden (jadengarza@pm.me) with Replicas
Workspace  ·  Slack Thread

Open in Devin Review

@capy-ai

capy-ai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 4 potential issues.

View 7 additional findings in Devin Review.

Open in Devin Review

if (!shouldDispatch) continue;
await this.dispatchMessage(roomId, queued.event_id, decrypted.body);
}
await this.state.storage.delete(key);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 drainPendingForSession unconditionally deletes the queue, silently dropping events that still can't decrypt

When a Megolm key is captured and drainPendingForSession runs, events that still fail to decrypt (line 543-544) log "leaving in queue" but are actually discarded because this.state.storage.delete(key) at line 552 unconditionally deletes the entire queue after the loop. Any event in the pending-decrypt queue that fails re-decryption (e.g. encrypted with a different session key than the one just captured) is permanently lost — the user's message is silently eaten.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts, the drainPendingForSession method at line 552 unconditionally deletes the entire pending-decrypt queue after iterating through it. However, the loop at lines 542-545 continues past items that still fail to decrypt, logging 'leaving in queue'. The fix is to track which items were successfully processed and only remove those, keeping failed items in the queue for future retry.

Approach: Collect the event_ids of successfully dispatched (or skipped-for-non-text) items during the loop, then after the loop, filter the original list to keep only un-processed entries. If the filtered list is empty, delete the key; otherwise, write the filtered list back. This preserves the 'leave in queue' contract the log message promises.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 drainPendingForSession unconditionally deletes the queue, silently dropping events that still can't decrypt

When a Megolm key is captured and drainPendingForSession runs, events that still fail to decrypt (line 543-544) log "leaving in queue" but are actually discarded because this.state.storage.delete(key) at line 552 unconditionally deletes the entire queue after the loop. Any event in the pending-decrypt queue that fails re-decryption (e.g. encrypted with a different session key than the one just captured) is permanently lost — the user's message is silently eaten.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts, the drainPendingForSession method at line 552 unconditionally deletes the entire pending-decrypt queue after iterating through it. However, the loop at lines 542-545 continues past items that still fail to decrypt, logging 'leaving in queue'. The fix is to track which items were successfully processed and only remove those, keeping failed items in the queue for future retry.

Approach: Collect the event_ids of successfully dispatched (or skipped-for-non-text) items during the loop, then after the loop, filter the original list to keep only un-processed entries. If the filtered list is empty, delete the key; otherwise, write the filtered list back. This preserves the 'leave in queue' contract the log message promises.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration[bot] This repository is not connected to a Replicas organization.

To enable Replicas on this repository:

  1. Go to GitHub integration settings
  2. Add this repository to your organization

Note: Each repository can only be connected to one Replicas organization.

// Gate auto-dispatch on room size + mention. In a 2-person
// room (you + me), every message is for me. In a larger room
// the bot should stay quiet unless it's been mentioned.
const shouldDispatch = await this.shouldHandleMessage(roomId, ev);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 shouldHandleMessage receives encrypted wrapper content on main timeline path, breaking mention detection in encrypted multi-person rooms

At line 305, shouldHandleMessage(roomId, ev) is called with the raw Matrix event. For encrypted events (ev.type === "m.room.encrypted"), ev.content contains {algorithm, ciphertext, session_id, sender_key} — NOT the decrypted m.mentions or formatted_body. So shouldHandleMessage (listener.ts:560-572) will never find mentions in the encrypted content, and in rooms with >2 members the bot silently drops the message. This exact issue was already identified and fixed in the /recover-room handler (lines 124-132, "Audit finding #8") but the fix was not applied to this main timeline code path.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts at line 305, shouldHandleMessage is called with the original event `ev`, which for encrypted events has the ciphertext wrapper as its content (no m.mentions, no formatted_body). The same bug was already fixed in the /recover-room handler at lines 124-132 (Audit finding #8) by constructing a synthetic event with the DECRYPTED inner content.

To fix: when the event was decrypted (ev.type === 'm.room.encrypted' path at lines 280-285), the tryDecrypt method needs to also return the inner content object (not just msgtype/body). Then at line 305, pass a synthetic {content: innerContent} to shouldHandleMessage instead of the raw `ev`. This mirrors what the recover-room handler does at line 131.

The same fix needs to be applied in drainPendingForSession at line 548, where the same pattern occurs — shouldHandleMessage is called with the encrypted synthetic object instead of the decrypted inner content.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 shouldHandleMessage receives encrypted wrapper content on main timeline path, breaking mention detection in encrypted multi-person rooms

At line 305, shouldHandleMessage(roomId, ev) is called with the raw Matrix event. For encrypted events (ev.type === "m.room.encrypted"), ev.content contains {algorithm, ciphertext, session_id, sender_key} — NOT the decrypted m.mentions or formatted_body. So shouldHandleMessage (listener.ts:560-572) will never find mentions in the encrypted content, and in rooms with >2 members the bot silently drops the message. This exact issue was already identified and fixed in the /recover-room handler (lines 124-132, "Audit finding #8") but the fix was not applied to this main timeline code path.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts at line 305, shouldHandleMessage is called with the original event `ev`, which for encrypted events has the ciphertext wrapper as its content (no m.mentions, no formatted_body). The same bug was already fixed in the /recover-room handler at lines 124-132 (Audit finding #8) by constructing a synthetic event with the DECRYPTED inner content.

To fix: when the event was decrypted (ev.type === 'm.room.encrypted' path at lines 280-285), the tryDecrypt method needs to also return the inner content object (not just msgtype/body). Then at line 305, pass a synthetic {content: innerContent} to shouldHandleMessage instead of the raw `ev`. This mirrors what the recover-room handler does at line 131.

The same fix needs to be applied in drainPendingForSession at line 548, where the same pattern occurs — shouldHandleMessage is called with the encrypted synthetic object instead of the decrypted inner content.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration[bot] This repository is not connected to a Replicas organization.

To enable Replicas on this repository:

  1. Go to GitHub integration settings
  2. Add this repository to your organization

Note: Each repository can only be connected to one Replicas organization.

continue;
}
if (decrypted.msgtype !== "m.text" || !decrypted.body) continue;
const shouldDispatch = await this.shouldHandleMessage(roomId, synthetic);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 shouldHandleMessage in drainPendingForSession also receives encrypted wrapper, same mention-detection failure

At line 548, shouldHandleMessage(roomId, synthetic) is called with synthetic whose content field is {algorithm, session_id, ciphertext, sender_key} — the encrypted wrapper, not the decrypted inner content. In encrypted multi-person rooms (>2 members), mention detection fails and the drained message is silently skipped. This is the same class of bug as the main timeline path (listener.ts:305) and the /recover-room handler's "Audit finding #8" — but the fix from recover-room was not applied here.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts drainPendingForSession at line 548, shouldHandleMessage receives the encrypted synthetic object. The tryDecrypt call at line 542 returns {msgtype, body} but not the full decrypted inner content needed for mention detection.

To fix: modify tryDecrypt to also return the inner content object (the full inner.content from the decrypted plaintext), then at line 548 pass {content: decryptedInnerContent} to shouldHandleMessage. This matches the pattern already used in the /recover-room handler at line 131.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 shouldHandleMessage in drainPendingForSession also receives encrypted wrapper, same mention-detection failure

At line 548, shouldHandleMessage(roomId, synthetic) is called with synthetic whose content field is {algorithm, session_id, ciphertext, sender_key} — the encrypted wrapper, not the decrypted inner content. In encrypted multi-person rooms (>2 members), mention detection fails and the drained message is silently skipped. This is the same class of bug as the main timeline path (listener.ts:305) and the /recover-room handler's "Audit finding #8" — but the fix from recover-room was not applied here.

Prompt for agents
In replicas-matrix-bridge/src/listener.ts drainPendingForSession at line 548, shouldHandleMessage receives the encrypted synthetic object. The tryDecrypt call at line 542 returns {msgtype, body} but not the full decrypted inner content needed for mention detection.

To fix: modify tryDecrypt to also return the inner content object (the full inner.content from the decrypted plaintext), then at line 548 pass {content: decryptedInnerContent} to shouldHandleMessage. This matches the pattern already used in the /recover-room handler at line 131.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration[bot] This repository is not connected to a Replicas organization.

To enable Replicas on this repository:

  1. Go to GitHub integration settings
  2. Add this repository to your organization

Note: Each repository can only be connected to one Replicas organization.

Comment on lines +383 to +384
const next = pickles.filter((p) => p !== usedSessionPickle);
next.push(usedSessionPickle);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Olm session pickle filter never removes the pre-decrypt pickle, causing unbounded duplication

In decryptToDevice at line 383, after a successful decrypt the code filters pickles to remove the used session: pickles.filter((p) => p !== usedSessionPickle). But usedSessionPickle is the post-decrypt pickle (ratchet advanced, line 356), while pickles contains the pre-decrypt pickle. Since the strings differ (the ratchet state changed), the filter removes nothing — the old stale pickle stays AND the new pickle is appended. Over successive messages from the same sender, this accumulates duplicate entries (old ratchet states that will always fail to decrypt), wasting decrypt attempts on each message until the .slice(-8) cap evicts them and potentially evicting still-valid sessions for other conversations with the same sender key.

Prompt for agents
In replicas-matrix-bridge/src/olm-vault.ts decryptToDevice, the session-persistence logic at lines 382-387 tries to replace the old session pickle with the new (post-decrypt, ratchet-advanced) one. But it filters by comparing against usedSessionPickle (the NEW pickle) rather than the original pre-decrypt pickle, so nothing is ever removed.

Fix: track the original pickled value that was used for decryption. In the existing-session loop (lines 350-363), store the current `pickled` value (the one from the array) alongside usedSessionPickle. Then at line 383, filter against the original: `pickles.filter((p) => p !== originalPickle)`. For the new-session path (lines 367-374), there's no original to remove since the session is brand new, so the filter correctly does nothing.

Alternatively, since the code already iterates `pickles` and knows the index of the matching session, track the index and use splice instead of filter.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Olm session pickle filter never removes the pre-decrypt pickle, causing unbounded duplication

In decryptToDevice at line 383, after a successful decrypt the code filters pickles to remove the used session: pickles.filter((p) => p !== usedSessionPickle). But usedSessionPickle is the post-decrypt pickle (ratchet advanced, line 356), while pickles contains the pre-decrypt pickle. Since the strings differ (the ratchet state changed), the filter removes nothing — the old stale pickle stays AND the new pickle is appended. Over successive messages from the same sender, this accumulates duplicate entries (old ratchet states that will always fail to decrypt), wasting decrypt attempts on each message until the .slice(-8) cap evicts them and potentially evicting still-valid sessions for other conversations with the same sender key.

Prompt for agents
In replicas-matrix-bridge/src/olm-vault.ts decryptToDevice, the session-persistence logic at lines 382-387 tries to replace the old session pickle with the new (post-decrypt, ratchet-advanced) one. But it filters by comparing against usedSessionPickle (the NEW pickle) rather than the original pre-decrypt pickle, so nothing is ever removed.

Fix: track the original pickled value that was used for decryption. In the existing-session loop (lines 350-363), store the current `pickled` value (the one from the array) alongside usedSessionPickle. Then at line 383, filter against the original: `pickles.filter((p) => p !== originalPickle)`. For the new-session path (lines 367-374), there's no original to remove since the session is brand new, so the filter correctly does nothing.

Alternatively, since the code already iterates `pickles` and knows the index of the matching session, track the index and use splice instead of filter.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration[bot] This repository is not connected to a Replicas organization.

To enable Replicas on this repository:

  1. Go to GitHub integration settings
  2. Add this repository to your organization

Note: Each repository can only be connected to one Replicas organization.

@replicas-connector

Copy link
Copy Markdown
Contributor Author

Devin Review found 4 potential issues.

View 7 additional findings in Devin Review.

Open in Devin Review

@devin-ai-integration[bot] This repository is not connected to a Replicas organization.

To enable Replicas on this repository:

  1. Go to GitHub integration settings
  2. Add this repository to your organization

Note: Each repository can only be connected to one Replicas organization.

Round-3 audit focused on the agent-output → Matrix HTML rendering layer.
Two findings shipped in src/markdown.ts (the markdown→HTML converter
used for every Done frame's embedded reply body).

1. **Link scheme whitelist** — `[text](url)` previously emitted `<a
   href="${url}">` with only `"` escaped. An agent producing malicious
   markdown (prompt-injected web content, agent-as-attacker, etc.)
   could craft `[click](javascript:alert(1))` and rely on whatever the
   downstream Matrix client did with non-http schemes. Matrix spec says
   clients SHOULD filter but behavior varies across
   Element/Beeper/native bridges/custom clients. Now whitelisted to
   http(s), ftp, mailto, magnet, plus relative paths and fragment-only
   links; everything else renders as bracketed plain text so the user
   still sees the emitted content but it can't be clicked into an XSS.

2. **NULL-byte placeholder hardening** — `markdownToTelegramHtml` uses
   `\u0000PH<n>\u0000` as the sentinel for code-block extraction.
   Theoretically an agent could emit a literal NULL-bounded `PH0` and
   corrupt the splice-back step (substituting one of its own code
   blocks). Strip NULL bytes up front; they're never legitimately part
   of Markdown body text.

5 new tests cover the link-scheme allow/deny matrix and the NULL-byte
strip. Other files in this audit pass (src/render.ts, src/matrix.ts)
came back clean — escaping is consistent, line-array elements are HTML-
ready before render time, plan parsing is opaque to adversarial input.

Typecheck clean. 102/102 tests pass (was 97).

Co-Authored-By: itsablabla <itsablabla@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@replicas-connector replicas-connector Bot force-pushed the fix/matrix-markdown-link-scheme-xss branch from a9fd23b to 08ef971 Compare May 30, 2026 16:40
@replicas-connector replicas-connector Bot merged commit 203d965 into main May 30, 2026
1 check failed
replicas-connector Bot added a commit that referenced this pull request May 30, 2026
Closes out the deferred-with-rationale items from rounds 1–3 (commits
2313473, PR #18, PR #19). Every item now has a real code fix.

**Round 1 (correctness/lifecycle) — 4 items**

#1. pending-decrypt:* prune cron — listener alarm now runs a 30-min
periodic prune that drops queue entries older than 7 days and deletes
empty queues. Bounds long-term DO storage growth from sessions whose
forwarders went offline before responding.

#2. seen:* KV race redesign — dispatch dedupe was using KV's eventual
read-modify-write, letting two concurrent reads both see "absent" and
both proceed. New /dedup-claim endpoint on MatrixListener wraps the
check + write in blockConcurrencyWhile for genuine put-if-absent
semantics. Periodic prune handles cleanup (1h TTL); KV fallback
preserved for the case where the listener DO is unreachable.

#3. /admin/listener/reload-keys endpoint — clears the in-memory
cachedKeys + cachedCurve25519 so the operator can rotate Megolm
session keys (MATRIX_MEGOLM_KEYS_JSON) without a full redeploy.

#4. parsePlan hijack tightening — only accept Plan(d/t) headers
BEFORE any tool calls and before any prior plan has been parsed.
Prevents a confused/adversarial agent from overwriting in-progress
view by emitting `Plan (5/5)` mid-turn to fake completion.

**Round 2 (security/spec/integration) — 4 items**

#5. m.forwarded_room_key validation — vault now tags captured Megolm
keys as `forwarded` vs `live`; listener checks `key-request:*`
prefix to confirm we previously asked for the (room, session). If
not, calls new vault `/keystore-delete` to evict the unsolicited
key. Closes the "any Olm-paired sender can plant Megolm keys for any
room" hole.

#6. usage:org KV race — routed through OlmVault singleton DO via
new /usage-bump (write, atomic) and /usage-read (read) endpoints.
Concurrent Done frames across rooms can no longer drop entries via
read-modify-write race.

#7. Olm session cap bump — was .slice(-8), now .slice(-32). Verified
multi-device senders rotate fast enough that 8 was occasionally
dropping legitimate live sessions. Evictions are now logged so the
operator can spot pathological churn.

#8. /admin/recover-room missing-senderKey warning — surface bridges
that strip the outer sender_key field (m.room_key_request routing
degrades to `*` when missing, may not land on the originating device).

Typecheck clean. 102/102 tests pass.

Rolling tally tonight: 19 findings · 19 shipped · 0 deferred.

Co-Authored-By: itsablabla <itsablabla@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
replicas-connector Bot added a commit that referenced this pull request May 30, 2026
Closes out the deferred-with-rationale items from rounds 1–3 (commits
2313473, PR #18, PR #19). Every item now has a real code fix.

**Round 1 (correctness/lifecycle) — 4 items**

#1. pending-decrypt:* prune cron — listener alarm now runs a 30-min
periodic prune that drops queue entries older than 7 days and deletes
empty queues. Bounds long-term DO storage growth from sessions whose
forwarders went offline before responding.

#2. seen:* KV race redesign — dispatch dedupe was using KV's eventual
read-modify-write, letting two concurrent reads both see "absent" and
both proceed. New /dedup-claim endpoint on MatrixListener wraps the
check + write in blockConcurrencyWhile for genuine put-if-absent
semantics. Periodic prune handles cleanup (1h TTL); KV fallback
preserved for the case where the listener DO is unreachable.

#3. /admin/listener/reload-keys endpoint — clears the in-memory
cachedKeys + cachedCurve25519 so the operator can rotate Megolm
session keys (MATRIX_MEGOLM_KEYS_JSON) without a full redeploy.

#4. parsePlan hijack tightening — only accept Plan(d/t) headers
BEFORE any tool calls and before any prior plan has been parsed.
Prevents a confused/adversarial agent from overwriting in-progress
view by emitting `Plan (5/5)` mid-turn to fake completion.

**Round 2 (security/spec/integration) — 4 items**

#5. m.forwarded_room_key validation — vault now tags captured Megolm
keys as `forwarded` vs `live`; listener checks `key-request:*`
prefix to confirm we previously asked for the (room, session). If
not, calls new vault `/keystore-delete` to evict the unsolicited
key. Closes the "any Olm-paired sender can plant Megolm keys for any
room" hole.

#6. usage:org KV race — routed through OlmVault singleton DO via
new /usage-bump (write, atomic) and /usage-read (read) endpoints.
Concurrent Done frames across rooms can no longer drop entries via
read-modify-write race.

#7. Olm session cap bump — was .slice(-8), now .slice(-32). Verified
multi-device senders rotate fast enough that 8 was occasionally
dropping legitimate live sessions. Evictions are now logged so the
operator can spot pathological churn.

#8. /admin/recover-room missing-senderKey warning — surface bridges
that strip the outer sender_key field (m.room_key_request routing
degrades to `*` when missing, may not land on the originating device).

Typecheck clean. 102/102 tests pass.

Rolling tally tonight: 19 findings · 19 shipped · 0 deferred.

Co-authored-by: replicas-connector[bot] <replicas-connector[bot]@users.noreply.github.com>
Co-authored-by: itsablabla <itsablabla@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant