fix(security): close channel media RBAC bypass and audit findings#6141
Conversation
A multi-crate scan surfaced one high-severity auth bypass plus several correctness and hardening defects; each was adversarially re-verified against source before fixing. channels/bridge.rs — the multimodal (image/voice/video) dispatch path authorized `message.sender.platform_id`, which for group messages is the group/chat JID, not the individual sender. An unauthorized group member could bypass the RBAC allowlist by sending media instead of text. The media path also skipped the group_policy / dm_policy / rate-limit gating the text path enforces, letting media bypass a `mention_only`/`ignore` group or DM `dm_policy=ignore` and evade per-user throttling. Both paths now key on `sender_user_id(message)` and the debounced media branch runs the same gating helper as the text path (gated at the single ungated entry point so rate-limit tokens are not double-counted). skills/verify.rs, runtime/prompt_builder.rs, kernel/prompt_context.rs — the prompt-injection scanner ran Aho-Corasick over raw text, so a zero-width or bidi code point spliced into a literal (`ignore<ZWSP>previous instructions`) evaded every Critical pattern while invisible chars were only a non-blocking Warning and were never stripped before reaching the prompt. The scanner now also scans an invisible-stripped and a separator-restored variant (dedup shared across passes), `sanitize_for_prompt` drops invisible/format code points, and the previously-unsanitized `collect_prompt_context` content slot strips them too. api/routes/skills/clawhub.rs — `clawhub_install` and `clawhub_cn_install` joined `req.hand` into a filesystem path with no validation, unlike the sibling `install_skill`. A `../`-bearing hand id escaped the hands directory. Both now run `validate_skill_identifier` before the join. llm-drivers/gemini.rs — the streaming usage path dropped `cache_read_input_tokens`, over-charging cached Gemini prompt tokens at the full input rate. It now mirrors the non-streaming paths and captures `cached_content_token_count`. cli/mcp.rs — the MCP stdio server collapsed a malformed JSON body into EOF and returned `Err` on oversized frames, killing the whole session on one bad message. `read_message` now returns a `Frame` enum distinguishing true EOF from recoverable protocol errors; the run loop replies with JSON-RPC -32700 and continues. Verification: cargo check + clippy (-D warnings) green across all seven touched crates with --all-targets; new regression tests added for each fix (channels principal/gate parity, skills both bypass shapes, prompt_builder strip, gemini streaming cache tokens, mcp malformed-then-valid frame recovery) and the affected crates' unit + integration suites pass.
houko
left a comment
There was a problem hiding this comment.
The security fixes are correct and the test coverage is solid — no logic issues found.
One project-wide style rule is violated throughout: CLAUDE.md mandates "never write multi-line comment blocks — one short line max." Every changed file in this PR introduces multi-line // or /// blocks. Each should be collapsed to a single explanatory line (keeping only the WHY) or removed if the identifier already makes it obvious.
Specific locations:
crates/librefang-api/src/routes/skills/clawhub.rs — two identical 8-line // blocks (one in clawhub_install, one in clawhub_cn_install) should each become one line, e.g. // Validate hand_id before Path::join to prevent path traversal, mirroring install_skill.
crates/librefang-channels/src/bridge.rs
- The 14-line
// --- Gate the debounced media path ---block before themedia_dispatch_allowedcall inflush_debounced→ one line. - The ~30-line
///doc-comment onmedia_dispatch_allowed→ one line explaining the purpose at the call site is enough; the function name itself is descriptive.
crates/librefang-cli/src/mcp.rs
- The 7-line doc-comment on
read_message→ one line. - The 3-line
///comment onFrame::ProtocolError→ one line. - The 2-line
// A single malformed or oversized frame…comment in therun_mcp_serverloop → one line.
crates/librefang-kernel/src/kernel/prompt_context.rs — the 17-line comment block before the invisible-chars .filter() → one line (the const name INVISIBLE_PROMPT_CHARS already communicates what it is; a single WHY sentence suffices).
crates/librefang-runtime/src/prompt_builder.rs
- The 4-line
///comment beforeINVISIBLE_PROMPT_CHARSconst → one line. - The 3-line
// Drop invisible / format code points…block insidesanitize_for_prompt→ one line.
crates/librefang-llm-drivers/src/drivers/gemini.rs — the 2-line // Cached prompt tokens (subset of input_tokens) for / cache-read pricing. comment → one line.
crates/librefang-skills/src/verify.rs — several multi-line blocks explaining the three-variant scan strategy (the // Invisible/format code points… block, the // Scan one text variant… block) → collapse each to one line at the head of its section.
The prose-wrapping rule (one sentence per line, no column limit) applies only at sentence boundaries inside prose paragraphs — it does not override the "one short line max" rule for code comment blocks.
Generated by Claude Code
Addresses findings from a high-effort review of the prior commit, all in code that commit introduced. channels/bridge.rs: - The media gate resolved channel-level overrides only; the text path resolves agent-level overrides (agent.toml) with channel-level as fallback. Media now resolves the target agent and merges per-agent overrides identically, so per-agent group/DM policy and rate limits apply to media. output_format and threading now also follow the merged overrides. - The gate omitted the reply-intent precheck the text path runs for group_policy=all, so a stray captioned media message got a reply where the same text would have stayed silent. The precheck is now replicated. - Record-on-skip used text_content(), which returns None for Image/Voice/Video, so captions were never recorded into group_history despite the comment. A new extracted_user_text() helper returns the real caption (never the [Photo: url] placeholder) and is used for both record-on-skip and precheck classification. skills/verify.rs (+ runtime/prompt_builder.rs, runtime/injection_guard.rs, kernel/prompt_context.rs): - INVISIBLE_CHARS omitted several commonly-abused code points (soft hyphen, combining grapheme joiner, arabic letter mark, hangul/khmer fillers, mongolian vowel separator, invisible math operators, variation selectors, …). Added them and synced all four duplicated copies. - A combined obfuscation payload (one zero-width char splitting a word AND another replacing a separator) defeated the stripped and spaced passes. Added a whitespace-invariant compact pass that matches multi-word threat phrases against the invisible-and-whitespace-stripped text. The compact pass runs ONLY when invisible chars are present, so benign line-broken prose (which has none) cannot be falsely flagged by whitespace gluing. cli/mcp.rs: - A malformed Content-Length value (or headers with no Content-Length) collapsed to length 0 and was reported as Frame::Eof, killing the session — the exact malformed-frame-kills-session class the prior commit set out to fix. read_message now tracks whether any header was seen and whether Content-Length parsed; a malformed/missing length on a framed message returns Frame::ProtocolError (reply -32700, continue) while only a genuine header-less EOF returns Frame::Eof. Verification: cargo clippy --all-targets -D warnings green across channels/skills/runtime/kernel/cli; lib suites pass (channels 496, skills 212, runtime prompt_builder/injection_guard, kernel prompt_context); new regression tests added for extracted_user_text, the combined/expanded invisible-char attacks with a benign-prose false-positive guard, and the malformed/missing Content-Length frame recovery.
…eck on captionless media (#6426) Post-merge review follow-ups to #6141. - Consolidate the four duplicated invisible/format code-point lists (skills verify, runtime injection_guard, runtime prompt_builder, kernel prompt_context) into one source of truth `librefang_types::text::INVISIBLE_FORMAT_CHARS`; the three char-only copies alias it directly and the skills labeled `(char, &str)` table is guarded by an equality test, so the set cannot silently drift between crates and reopen the scanner bypass in the un-updated copy. - Skip the billed reply-precheck LLM call for captionless media in the channel media gate: `group_policy=all` media ran `classify_reply_intent` on an empty string, so the precheck now runs only when there is text to classify (captioned media keeps parity with the text path) and captionless media proceeds without the call. Also reconciles #6141's PR notes.
Summary
A multi-agent scan across the workspace (per-crate + cross-cutting security/correctness lenses) surfaced a set of defects; each was adversarially re-verified against source before being fixed. This PR lands the seven confirmed findings as one tight security/correctness cluster.
Findings fixed
1. Channel media RBAC bypass (high) —
crates/librefang-channels/src/bridge.rsThe multimodal (image/voice/video) dispatch path authorized
message.sender.platform_id, which for group messages is the group/chat JID, not the individual sender. With RBAC enabled, an unauthorized group member could bypass the allowlist by sending media instead of text — exactly the voice-note vector flagged in the cross-chat-leak work (2d5a445). The text path already keys onsender_user_id(message); the media path now does too. Behavior-preserving for DMs (wheresender_user_idfalls back toplatform_id).2. Channel media skips policy/rate-limit gating (medium) — same file
The debounced media branch reached
dispatch_with_blocksdirectly, skipping theshould_process_group_message/dm_policy/ per-channel + per-user rate-limit gating the text path enforces. Media could bypass amention_only/ignoregroup or a DMdm_policy=ignore, and per-user rate limits never throttled media. Amedia_dispatch_allowedhelper now runs the identical gating, placed at the single ungated entry point (flush_debounced) so the five already-gateddispatch_with_blockscall sites insidedispatch_messagedo not double-count rate-limit tokens.3. Prompt-injection scanner zero-width/bidi bypass (medium) —
librefang-skills/src/verify.rs,librefang-runtime/src/prompt_builder.rs,librefang-kernel/src/kernel/prompt_context.rsThe Aho-Corasick scan ran over raw text, so a zero-width or bidi code point spliced into a literal (
ignore<ZWSP>previous instructions) evaded every Critical pattern, while invisible chars were only a non-blocking Warning and were never stripped before reaching the LLM prompt. Now: the scanner also scans an invisible-stripped variant (in-word insertion) and a separator-restored variant (zero-width-for-space), deduplicated across passes;sanitize_for_promptdrops invisible/format code points; and the previously-unsanitizedcollect_prompt_contextcontent slot strips them too. The benign-i18n Warning behavior (U+200E/U+200F, bidi isolates in RTL descriptions) is left intact rather than hard-blocked.4. ClawHub install path traversal (medium) —
crates/librefang-api/src/routes/skills/clawhub.rsclawhub_installandclawhub_cn_installjoinedreq.handintoworkspaces/hands/<hand>with no validation, unlike the siblinginstall_skill. A../-bearing id escaped the hands directory. Both now runvalidate_skill_identifier(hand_id, "hand")before the join.5. Gemini streaming over-charges cached tokens (medium) —
crates/librefang-llm-drivers/src/drivers/gemini.rsThe streaming usage path set
input/outputtokens but droppedcache_read_input_tokens, so cached Gemini prompt tokens were billed at the full input rate. It now capturescached_content_token_count, mirroring the two non-streaming paths.6. MCP stdio server dies on one bad frame (low) —
crates/librefang-cli/src/mcp.rsA malformed JSON body collapsed to EOF and an oversized frame returned
Err, killing the whole session on a single bad message.read_messagenow returns aFrameenum distinguishing true EOF from recoverable protocol errors; the run loop replies with JSON-RPC-32700and continues.Verification
cargo checkandcargo clippy --all-targets -- -D warningsare green across all seven touched crates (librefang-channels,-llm-drivers,-skills,-runtime,-kernel,-api,-cli).sanitize_for_promptdrops U+200B / U+202E.cachedContentTokenCountpopulatescache_read_input_tokens.Err.librefang-skills211/211) and the gemini integration test passes.Notes
reply_precheck(an LLM-cost optimization gated ongroup_policy=all) is deliberately not replicated into the media gate — it is a silence optimization, not a security gate, and adding it would introduce a billed LLM call on the media path.