Skip to content

fix(security): close channel media RBAC bypass and audit findings#6141

Merged
houko merged 2 commits into
mainfrom
fix/bug-sweep
Jun 17, 2026
Merged

fix(security): close channel media RBAC bypass and audit findings#6141
houko merged 2 commits into
mainfrom
fix/bug-sweep

Conversation

@houko

@houko houko commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

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.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. 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 on sender_user_id(message); the media path now does too. Behavior-preserving for DMs (where sender_user_id falls back to platform_id).

2. Channel media skips policy/rate-limit gating (medium) — same file

The debounced media branch reached dispatch_with_blocks directly, skipping the should_process_group_message / dm_policy / per-channel + per-user rate-limit gating the text path enforces. Media could bypass a mention_only/ignore group or a DM dm_policy=ignore, and per-user rate limits never throttled media. A media_dispatch_allowed helper now runs the identical gating, placed at the single ungated entry point (flush_debounced) so the five already-gated dispatch_with_blocks call sites inside dispatch_message do 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.rs

The 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_prompt drops invisible/format code points; and the previously-unsanitized collect_prompt_context content 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.rs

clawhub_install and clawhub_cn_install joined req.hand into workspaces/hands/<hand> with no validation, unlike the sibling install_skill. A ../-bearing id escaped the hands directory. Both now run validate_skill_identifier(hand_id, "hand") before the join.

5. Gemini streaming over-charges cached tokens (medium) — crates/librefang-llm-drivers/src/drivers/gemini.rs

The streaming usage path set input/output tokens but dropped cache_read_input_tokens, so cached Gemini prompt tokens were billed at the full input rate. It now captures cached_content_token_count, mirroring the two non-streaming paths.

6. MCP stdio server dies on one bad frame (low) — crates/librefang-cli/src/mcp.rs

A malformed JSON body collapsed to EOF and an oversized frame returned Err, killing the whole session on a single 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 and cargo clippy --all-targets -- -D warnings are green across all seven touched crates (librefang-channels, -llm-drivers, -skills, -runtime, -kernel, -api, -cli).
  • New regression tests added per fix:
    • channels: group-media authorizes the individual sender and the group/DM gate predicate holds.
    • skills: both bypass shapes (separator substitution and in-word insertion, incl. U+202E) now flag Critical.
    • prompt_builder: sanitize_for_prompt drops U+200B / U+202E.
    • gemini: streamed chunk with cachedContentTokenCount populates cache_read_input_tokens.
    • mcp: malformed-then-valid frame sequence recovers (no session kill); oversized body returns a recoverable error, not Err.
  • Affected crates' unit suites pass (e.g. librefang-skills 211/211) and the gemini integration test passes.

Notes

  • Two scanner findings were rejected during adversarial verification and are intentionally not included.
  • reply_precheck (an LLM-cost optimization gated on group_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.

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.
@github-actions github-actions Bot added size/L 250-999 lines changed area/channels Messaging channel adapters area/skills Skill system and FangHub marketplace area/runtime Agent loop, LLM drivers, WASM sandbox area/kernel Core kernel (scheduling, RBAC, workflows) labels Jun 17, 2026

@houko houko left a comment

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.

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 the media_dispatch_allowed call in flush_debounced → one line.
  • The ~30-line /// doc-comment on media_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 on Frame::ProtocolError → one line.
  • The 2-line // A single malformed or oversized frame… comment in the run_mcp_server loop → 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 before INVISIBLE_PROMPT_CHARS const → one line.
  • The 3-line // Drop invisible / format code points… block inside sanitize_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

@github-actions github-actions Bot added size/XL 1000+ lines changed and removed size/L 250-999 lines changed labels Jun 17, 2026
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.
@houko
houko enabled auto-merge (squash) June 17, 2026 05:15
@houko
houko merged commit 60c8612 into main Jun 17, 2026
34 checks passed
@houko
houko deleted the fix/bug-sweep branch June 17, 2026 05:33
houko added a commit that referenced this pull request Jul 10, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/channels Messaging channel adapters area/kernel Core kernel (scheduling, RBAC, workflows) area/runtime Agent loop, LLM drivers, WASM sandbox area/skills Skill system and FangHub marketplace size/XL 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant