Repository navigation
security(chat): scan/redact PII and flag injection risk on every raw chat message (#16529, #16530) - #16545
Conversation
…chat message (#16529, #16530) Internal audit of the LLM request pipeline found api/chat.py forwarded the raw user message to the LLM (and to persistent storage) without ever calling the existing PII pipeline (a2a/pii_pipeline.py) or the existing prompt-injection detector (security/prompt_injection_detector.py) -- both were wired to narrower features but never to the literal chat message on any turn. Adds security/chat_message_safety.py as the one choke point every raw- message entry point in chat.py now calls first: /chat, /chat/stream (both branches), the frontend's primary /chats/{id}/message path, /chat/direct, and /ai-stack + /stream-ai-stack. PII reuses the existing BLOCK/REDACT/HASH policy unchanged via scrub_outbound(); SSN/API-key/card/JWT-shaped content is rejected before the LLM call, lower-severity types are redacted in place. Injection detection flags and logs HIGH/CRITICAL hits rather than blocking outright -- measured directly against a benign-message sample while wiring this in, the shared detector's pattern set false-positives on ordinary chat phrasing ("you are a great assistant"), so a flat block would reject legitimate turns. The existing AUTOBOT_INJECTION_HARDBLOCK_ENABLED opt-in still rejects a message when an operator has turned on stricter enforcement. chat.py is a grandfathered file frozen at its current line count (#14236); the five call sites land as in-place line edits (tuple-assignment / inline-call) rather than new lines, so the file's size does not change.
…uction args (#16529, #16530) get_prompt_injection_detector() is a lazy_singleton(PromptInjectionDetector): whichever caller constructs it first fixes its args for the process, and every later caller passing different args gets an error instead of the detector. Every existing call site (prompt_manager.py, screen_analysis_ prompt.py, chat_knowledge_prompt.py, intent_analyzer.py, ...) already uses strict_mode=True; the new chat safety module called it bare, so whichever of the two constructed first could poison the other depending on import/ call order. Caught by running the new tests alongside tests/test_injection_ detection.py, where TestPromptManagerInjectionDetection started failing with a "lazy_singleton: called with different args" log line.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
AutoBot Phase Validation ResultsSystem Maturity: 96.5% Phase Status:PASS Phase 1: Core Infrastructure: 100.0% Recommendations:
|
…generic SSE error (#16529, #16530) Code-review follow-up on the /chat/stream path: scan_chat_message() ran inside _generate_llm_stream's try block, so a PIIBlocked/hard-block HTTPException(400) was caught by that function's own broad except Exception and downgraded to a generic {"type":"error"} SSE event at HTTP 200 -- the LLM was still never called (already covered by a test), but the client could not distinguish "blocked by policy" from "the LLM failed", unlike the other four call sites, which all return a real 400. Moves the scan into stream_message() itself, before stream_chat_response() ever constructs a StreamingResponse -- a StreamingResponse commits to its status code the moment it is returned, before its body generator is entered, so scanning there is the only way to get a genuine pre-stream 400. _generate_llm_stream no longer scans; by the time it runs, message.content is already clean. Also: standardised two more #16529-only comment tags to #16529/#16530 for consistency (a third stays #16529-only -- the full tag would exceed this frozen file's 120-char line limit), and added an injection-flagged-but-not- blocked behavioral test at each of the five call sites -- the existing tests only covered the PII-block path per site, not that the #16530 false-positive guard (flag-and-log, not block) actually holds through the real wiring and not just the primitive.
AutoBot Phase Validation ResultsSystem Maturity: 96.5% Phase Status:PASS Phase 1: Core Infrastructure: 100.0% Recommendations:
|
|
Post-merge review ( |
Review at eefaafb (HIGH): self._prompt_compressor.compress(content) in LLMService._compress_messages() had no exception handling, and runs by default on every message of 100+ characters. Any exception in compress() (a regex edge case, a future phrase-list edit, non-string multimodal content) propagated and failed the whole chat/stream request -- a pure cost optimisation failing closed, unlike its siblings quota_headroom and provider_degradation, which fail open. Now wrapped in try/except, logged, falling back to the message's original content. Added a regression test where compress() raises and the request still succeeds uncompressed, and a second pinning that a system-role message carrying a #16545 PII-redaction marker ([REDACTED:{type}]) survives compression byte-for-byte -- true today only by inspection, not by any assertion, per the review's own note. llm_service.py grows 1194 -> 1206 lines from the try/except plus its docstring note; ceiling bumped in both ratchet-tracking files to match.
Review at a6dd6f0: the _index.md row for this doc cited no issue, unlike every sibling row, and the doc body never mapped a gap to the issue filed for it. - docs/research/_index.md: cite (#16524 (children #16526-#16533)) on the llm-gateway-signal-driven-routing row, matching sibling rows. - docs/research/llm-gateway-signal-driven-routing.md: add a Filed section with a gap -> issue table (matching community-prompt-and-skill-registry.md's pattern), and note gaps 1-2 (PII scanning, prompt-injection guard) as delivered by #16545 (cc35144) with #16561 as the post-merge review follow-up -- both findings predated that merge and would otherwise read as still open.
1 participant
Closes #16529, Closes #16530
Thinking Path
An internal audit of the LLM request pipeline found two related gaps on the
main chat path: the only wired PII detector in the codebase (
a2a/pii_pipeline.py)ran on outbound A2A task payloads only, and the prompt-injection detector
(
security/prompt_injection_detector.py) was wired to narrow features (screenanalysis, KB transcript, advanced-workflow intent) but never to the literal
chat message. Re-verified both claims directly against
api/chat.pybeforewriting any code: no
pii/injectioncall existed anywhere in that file.Audited every place in
api/chat.pywhere a raw user-typed message isforwarded toward an LLM, not just the one call site the issues named, since
a partial fix would leave the same audit finding other entry points later:
/chat,/chat/stream(both its direct-stream branch and its non-streamingfallback), the frontend's actual primary send path
/chats/{chat_id}/message,/chat/direct, and/ai-stack+/stream-ai-stack(which share one pipelinefunction).
/chats/{chat_id}/resumewas excluded: its body carries astructured approve/deny decision, not freeform chat text.
Policy decision (the ambiguous part these issues flagged for review):
PII — reuse the existing BLOCK/REDACT/HASH table unchanged, via the
same
scrub_outbound()entry point the A2A path already uses. SSN/API-key/card/JWT-shaped content raises
PIIBlocked, surfaced here as anHTTP 400 before anything reaches the LLM or storage; lower-severity types
(email, phone, IP, ...) are redacted in the text that gets used from then on.
This needed no new policy — it is the repo's existing, already-proven
convention.
Injection — flag-and-log by default, not block. This is the one I
measured rather than guessed: running the shared detector's default
HIGH/CRITICAL risk mapping against a set of ordinary benign chat sentences
showed real false positives — "You are a huge help, thank you so much.",
"I am writing a new instructions manual for my team, any tips?", and
"How do I forget all my stress after work?" all came back CRITICAL /
blocked=Trueunderdetect_injection()'s own logic, because its patternbank (
you are a,new instructions,forget all, ...) was built fornarrower structured inputs, not freeform conversation. A flat HIGH/CRITICAL
→ HTTP block would fail this task's own "no false-positive regression on
normal chat" requirement on real, unremarkable turns. So every chat entry
point now logs a structured security warning on a HIGH/CRITICAL hit (for
audit/trend visibility) without rejecting the turn, and only rejects it when
result.hard_blockedis set — the detector's own existing confidence-gatedsignal that stays off unless an operator opts in via
AUTOBOT_INJECTION_HARDBLOCK_ENABLED. No new flag was introduced; this justwires the chat path to respect the knob that already exists.
api/chat.pyis a grandfathered, line-count-frozen file (#14236 ceiling,currently exactly at it). All new logic lives in a new module,
security/chat_message_safety.py; the five call sites inchat.pyland asin-place edits to existing lines (a tuple-assignment or an inlined call)
rather than new lines, so the file's size does not change.
What Changed
autobot-backend/security/chat_message_safety.py(new):scan_chat_message()— the one choke point for the PII scrub + injection flag, with the policy
rationale above in its module docstring.
autobot-backend/api/chat.py: wiredscan_chat_message()into_store_and_log_user_message(/chat,/chat/message, and/chat/stream'snon-streaming fallback),
_generate_llm_stream(/chat/stream's directbranch),
send_chat_message_by_id(/chats/{chat_id}/message),send_direct_chat_response(/chat/direct), and_store_ai_stack_user_message(
/ai-stack+/stream-ai-stack). No lines added to the file overall.autobot-backend/tests/api/test_chat_message_safety.py(new): unittests on the primitive (PII block vs redact, injection flag vs hard-block,
benign-message false-positive check) plus behavioral tests on each of the
five touched call sites, asserting the downstream storage/LLM call never
happens when a message is PII-blocked — one of these caught a real
ordering bug in
send_direct_chat_responseduring review (see commits).get_prompt_injection_detector()is a
lazy_singletonwhose first caller locks in its construction args;every existing call site passes
strict_mode=Trueand the new module'sfirst draft didn't, which would have broken
prompt_manager.py's injectionscan depending on which caller constructed the singleton first.
Verification
Manually verified against obviously-fake fixtures (never real-looking
secrets): a fake SSN / API-key-shaped string / the well-known Visa test
card number each raise the 400 block before any LLM/storage call; a fake
email+phone get redacted in place; "Ignore previous instructions ... rm -rf /"
and a role-switch attempt are flagged and logged on every entry point's
default path.
Model Used
Claude Sonnet 5
🤖 Generated with Claude Code