You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
@devin-ai-integration[bot] This repository is not connected to a Replicas organization.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
@devin-ai-integration[bot] This repository is not connected to a Replicas organization.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
@devin-ai-integration[bot] This repository is not connected to a Replicas organization.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Was this helpful? React with 👍 or 👎 to provide feedback.
@devin-ai-integration[bot] This repository is not connected to a 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>
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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 insrc/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.LOW — NULL-byte placeholder hardening (
src/markdown.ts)markdownToTelegramHtmluses\u0000PH<n>\u0000as the sentinel for code-block extraction. An agent emitting a literal NULL-boundedPH0in its body could theoretically corrupt the splice-back step (substituting one of its own extracted code blocks).Other files — clean
src/render.ts— escaping is consistent, every line-array element is HTML-ready before render time,parsePlanis opaque to adversarial input.src/matrix.ts— call sites handle 429 correctly viaMatrixError.retryAfterMs,stripHtmlunescape 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)ae74f9ee-616a-4692-bed0-239bdcee1881)Test plan
[click](javascript:...)renders as plain bracketed textWorkspace · Slack Thread