fix(matrix-bridge): 3 audit-found bugs — R2 secret leak, E2EE/voice mention gate, reverse-map backfill#31
Merged
Conversation
…ention gate, reverse-map backfill Round-4 audit of the new code from tonight's 12 PRs (#21-#30). Three real bugs found and fixed in this PR. **#1 (HIGH) — R2 archive basename can leak inline env-var values** `archiveBasename("Bash", { command: "OPENAI_API_KEY=sk-xxx node srv.js" })` used the first whitespace-delimited token (`OPENAI_API_KEY=sk-xxx`) as the R2 object's basename, which lands in the *public* r2.dev URL the bridge emits as `📎 View full output`. Any inline env-var assignment in front of a Bash command would publish the value to anyone who reads the chat. Fix: walk tokens skipping `KEY=value` env vars + `nohup`/`time`/`sudo`/ `exec`/`env` prefixes; defensively strip any trailing `=…` even if the regex missed something; cap the final basename at 40 chars. New test exercises the exact leak vector. **#2 (MED) — mention gate skipped over E2EE and voice content** `shouldHandleMessage(roomId, ev)` was reading body/m.mentions from the OUTER ev.content. For E2EE rooms that's only the megolm ciphertext (no body/mentions). For voice messages it's `"Voice message.ogg"` (not the transcript). Result: in E2EE groups the bot stayed silent on explicit mentions; in any group with voice messages the bot couldn't trigger on the "Jada" wake-word even when the transcript started with it. Fix: introduce `mentionContent` alongside `body`/`msgtype`. For plain events it's `ev.content`; for E2EE it's `tryDecrypt`'s inner content; for voice it's a synthetic `{ msgtype: "m.text", body: transcript }`. Then call `shouldHandleMessage(roomId, { content: mentionContent })`. **#3 (MED) — cross-room guard backward-compat hole** PR #29's guard only checks the `replica:{id}` reverse mapping; the 47 existing room mappings that pre-date the guard have no reverse, so the check is a no-op for them. Until each room respawns its replica (could be days), the contamination guard does nothing. Fix: opportunistic backfill on every successful follow-up — if the reverse mapping is absent, write it (this room owns this replica). Race-safe: a colliding peer's NEXT follow-up sees mismatch and respawns. Same TTL as the forward mapping. **Validation** - 173/173 tests pass (+8 new — archive basename security suite) - Typecheck clean - Deployed `43e13059` Co-Authored-By: itsablabla <itsablabla@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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-4 audit of the new code from tonight's 12 PRs (#21-#30). Three real bugs found and fixed.
#1 (HIGH) — R2 archive basename leaks inline env-var values
archiveBasename("Bash", { command: "OPENAI_API_KEY=sk-xxx node srv.js" })used the first whitespace-delimited token (OPENAI_API_KEY=sk-xxx) as the R2 object's basename, which lands in the public r2.dev URL the bridge emits as📎 View full output. Any inline env-var assignment in front of a Bash command would publish the value to anyone who reads the chat.Fix: walk tokens skipping
KEY=valueenv vars +nohup/time/sudo/exec/envprefixes; defensively strip any trailing=…; cap final basename at 40 chars. New test exercises the exact leak vector.#2 (MED) — Mention gate skipped E2EE + voice content
shouldHandleMessage(roomId, ev)was reading body/m.mentions from the OUTERev.content. For E2EE rooms that's only the megolm ciphertext (no body/mentions). For voice messages it's"Voice message.ogg"not the transcript. Result: in E2EE groups the bot stayed silent on explicit mentions; in any group with voice messages the wake-word check missed.Fix: introduce
mentionContentalongsidebody/msgtype. For plain events it'sev.content; for E2EE it's the decrypted inner; for voice it's a synthetic{ msgtype: "m.text", body: transcript }. Then callshouldHandleMessagewith the synthetic.#3 (MED) — Cross-room guard backward-compat hole
PR #29's guard only checks
replica:{id}reverse mapping; the 47 existing room mappings that pre-date the guard have no reverse, so the check is a no-op for them until each room respawns.Fix: opportunistic backfill on every successful follow-up — if reverse is absent, write it. Race-safe: a colliding peer's NEXT follow-up sees mismatch and respawns.
Validation
43e13059Test plan
📎 View full outputlink's path component contains NO secretJada hiin an E2EE group (Element/Beeper), confirm bot now respondsWorkspace · Slack Thread