fix(security): close the run_kql guard and the redaction boundary, and catch the docs up to nine servers - #101
Conversation
run_kql's control-command guard checked only the whole-query prefix, so a
Kusto control command (.drop, .create, .set-or-append, .ingest) on a second
line ("print 1\n.show diagnostics") reached client.query() unrejected. The
server's charter is reject-before-dispatch; "the query-only endpoint
probably won't execute it" is weaker than "we never sent it".
Refactor the existing non-printable-prefix hardening into a per-line
classifier (_line_control_command_reason) and apply it to every line of the
query, not just the first -- closing both the plain dot-command form and the
invisible-character-hidden form on any line.
Deliberately does not reject ";" -- the same-line form
(Heartbeat | take 1; .drop table X) stays open, documented in a comment, since
rejecting ";" would break legitimate `let` statements.
Fixes #99.
All nine servers' _render called redact_obj, which only ever sees the literal keys "key"/"value" on a flat Evidence entry -- never the semantic evidence key name (e.g. client_secret). redact_finding's extra evidence-key pass exists exactly to close that gap, but its only caller was the generated-report path (core/reports/emit.py), leaving the live tool-output path -- where a secret would surface first -- less protected than the report path. Backwards. Switch every server's _render to redact_finding(f).model_dump(), keeping each _render's list[dict] return type unchanged (redact_finding itself returns a Finding). Correct redact_finding's docstring, which claimed this parity already held. Add one parametrised contract test (core/tests/test_redaction.py) over all nine server _render modules instead of nine near-identical copies. Fixes #100.
…gal KQL Review follow-up on #99: "no legitimate KQL line begins with a dot" was true only for the first line of a statement, not for interior lines of a multi-line construct. The per-line guard added for #99 wrongly rejected two legal shapes: 1. A decimal literal opening a continuation line -- KQL is whitespace- insensitive across an unterminated expression, so `| where Ratio >` followed by ` .5` on the next line is valid. 2. A Kusto verbatim string literal (```...```) whose body spans multiple lines and happens to contain a line starting with "." (e.g. an embedded sample log line). Both failed safe (refused rather than dispatched), but were still false positives an operator would hit on ordinary queries. Fix, two narrow changes: - A line only counts as a control-command prefix when the dot is immediately followed by a letter (.drop, .show, .set-or-append, ...) -- the only shape a real Kusto control command takes. A dot followed by a digit, another dot, or end of line is never a command. - Lines inside an open ``` ... ``` block are skipped entirely by a simple open/close toggle in run_kql (not a full lexer). The toggle cannot be used as an evasion: a control command placed on a line AFTER a block has already closed is still classified and still rejected -- verified by a dedicated test. Updated the guard's comments to state what is actually true instead of the "no legitimate line begins with a dot" over-claim. Tests: both false-positive cases now assert DISPATCH, plus the evasion case (command after a closed block still BLOCKs) and a classifier boundary case (a bare "." or ".." line, followed by non-letters, still dispatches). Every existing rejection test, including the invisible-prefix ones, still passes unchanged.
Minor follow-up from the #100 review: all nine live_smoke_*.py scripts and demo_mock_findings.py still called bare redact_obj(f.model_dump()) on Finding objects before printing to a terminal -- the one place a secret-hinting evidence key would actually be visible, and after #100 they were the only remaining call sites weaker than the servers they exercise. Switch every Finding-shaped call to redact_finding(f).model_dump(). Left scripts/live_smoke_projectachilles.py's other redact_obj call untouched: it redacts a raw wire dict (an API response row), not a Finding, so redact_finding does not apply there.
…exemption
The previous commit's fix for the multi-line dot-command guard added a
triple-backtick exemption for Kusto verbatim string literals. That
implementation was exploitable: it skipped a line WHOLESALE whenever the
line merely contained a ``` fence ("if '```' in line: continue"), so
appending a stray ``` to a control command exempted it entirely --
".drop table X ```" dispatched unrejected. This was a net regression: the
pre-#99 guard (whole-query startswith(".")) would have caught that exact
single-line query outright. Caught by an automated security review before
merge, confirmed exploitable by direct execution.
Fix: never skip a line because it contains a fence. Split each line on
```, track open/closed block state across segments, keep only the text
OUTSIDE any open block, and classify that joined text -- so a command
placed after (or appended to) a fence on the same line is still seen.
The exemption itself only activates when the total ``` count across the
WHOLE query is even -- i.e. every opened block is provably closed
somewhere in the query. An odd total (a block opened but never closed by
the end of the query) disables the exemption for the entire query and
every line is classified at face value, fences included: we cannot prove
the backend would treat an unclosed block's remainder as inert string
content, so strictness wins over cleverness there.
Corrected the code comment, which previously claimed the toggle "cannot be
used to smuggle a command past a closed block" -- true only for genuinely
closed blocks, false for the wholesale-skip bug this commit removes.
Tests: locked in all 8 must-block and 5 must-dispatch cases from the
review, including the exact exploit strings
(".drop table X ```", "print 1\n.drop table X ```",
"print 1\n.drop table X ```y```"), the unclosed-block case
("``` \n.drop table X"), the balanced-inline-block case
("```x``` \n.drop table X"), the already-covered evasion case (command
after a genuinely closed block still blocks), and a `let`-statement
dispatch case. Verified all 13 by direct execution against the fixed code
before formalizing as pytest cases.
Refs #99.
Re-derive every number against docs/reference/ (58 tools, 9 servers, 30 skills) and the code (6 gated-write tools: Defender isolate_host/release_host + pa-actions run_test/schedule_test/set_schedule_status/cancel_tasks) rather than trusting the old prose. Also fills in the purview/sentinel credential and server-list gaps in the Hermes setup guide, which were missing even before the count went stale. Left untouched: docs/reference/ (generated), docs/proposals/2026-07-22-* (records state as of its date, by design), and opencode.md's "7 MCP servers" line + opencode.json (pending user decision).
…ion fix Follows the purview entry's established convention under [0.2.1] for introducing a new server, plus the two security-boundary fixes landed on this branch: run_kql's per-line dot-command guard and the switch of every server's _render to redact_finding (evidence-key-aware redaction).
… all nine servers Both prompts/f0-sectools-system-prompt.md and docs/running-with-local-models.md still described a two-server (Defender + Entra) repo. Rewrite for all nine: - The system prompt stays short by design (it's pasted as-is into small local models in LM Studio / Open WebUI, where prompt length competes with tool- selection accuracy) -- one line per server on what it covers, plus the routing rules that actually cause misroutes in practice (Sentinel vs. Defender for incidents and KQL, Sentinel vs. Purview for M365 audit, domain/URL vs. IP/port for Sentinel's hunt tools, and "no data" vs. "no findings"), verified against each tool's routing docstring rather than invented. Persona modes and read-only/never-fabricate principles preserved. - The local-models guide gets a redrawn architecture diagram grouping the nine servers by credential family (Microsoft-platform / other-platform) instead of nine literal boxes, plus updated credential and MCP-client-config steps. - examples/mcp/mcp.json, which the guide points readers at, gains the seven servers it was missing.
servers/README.md still listed sentinel-mcp under Planned while it sits built, live-validated, and merged in that very directory -- move it to the built list (9, was 8) with a one-line description matching the others. docs/user-guide/runtimes/opencode.md said "7 MCP servers ... six read servers connected" -- opencode.json actually wires 9 (verified via the file's own mcp block: 9 entries, 8 enabled + f0-pa-actions disabled), not just the two sentinel/purview were missing from. Recounted rather than adding 2 to the old numbers. Both were wrongly filed as "out of scope" in the previous pass; the opencode.json pending-decision note applies only to the config file itself (a local model key + skip-worktree flag), not to prose describing it.
using-skills-and-personas said 25 skills (now 30). The README's 'pending the next scorecard pass' list named every server added since the 2026-07-13 sweep except Sentinel -- the one most in need of a scorecard run, since it takes the registry from 51 to 58 tools. The dated historical counts (34 tools/six servers at the 2026-07-13 sweep, '(then-)22 tools') are correct as written and deliberately left alone.
… in run_kql
The ``` fence exemption in the run_kql control-command guard was computed
from the caller's own query (query.count("```") % 2) and activated over
caller-controlled lines, so a fence hidden inside a `//` comment or a
quoted string literal -- inert to the Kusto engine but still counted by
this check -- let a real control command (.drop, .ingest, ...) dispatch
unrejected. This is the third time the guard has been broken by adding
machinery to repair it; a partial lexer can only ever subtract text from
the check, never make it stricter.
Delete the fence/in_string_block state machine entirely. Every line is now
classified at face value by _line_control_command_reason, whose rule is
tightened to close the remaining gaps: a dot is a control-command prefix
when the first subsequent non-digit character, skipping whitespace and
other invisible characters, is a letter (". drop", ".\tdrop", and a
zero-width-space-hidden variant all now match, matching how Kusto's own
parser tolerates them).
The one accepted cost is a narrow false rejection: a multi-line verbatim
string literal whose interior line opens with a dot-letter sequence (e.g.
an embedded sample log) is now rejected as a control command even though
it is legal KQL -- documented and locked in by
test_run_kql_dot_line_inside_verbatim_string_now_rejected. Mutation-tested
by temporarily restoring the removed exemption and confirming the new
regression test fails.
Refs #99
The last docs sweep left several operator-facing pages saying "eight" or "six" servers, and the FAQ still listed Sentinel under Planned (it contradicts CHANGELOG.md on this same branch). Fix docs/user-guide/faq.md, docs/user-guide/README.md, CONTRIBUTING.md, CLAUDE.md's platform-integrations intro, docs/demo.md, and the pi/opencode integration READMEs to the current count (9 servers, 58 tools, 30 skills, 6 gated-write tools), verified against the code rather than assumed. docs/explanation/architecture.md's two mermaid diagrams were missing sentinel-mcp entirely -- add it to both the system-context diagram and the servers/core detail diagram (now correctly captioned "9 thin adapters"). Add the missing examples/findings/sentinel.json sample (synthetic incident finding, no real tenant/workspace/user data), and tighten scripts/tests/test_examples_valid.py and core/tests/test_redaction.py's hand-maintained server lists to assert equality against the discovered server count under servers/, not just a floor, so a tenth server can't silently ship without a sample or a redaction regression test. CHANGELOG.md: add a Changed entry documenting that the evidence-key redaction pass also blanks hunt-result columns whose names hint at a secret (TokenIssuerType, IncomingTokenType, CookieCount, ApiKeyId, PasswordExpiry, CredentialsUsed confirmed; AuthenticationDetails and ordinary columns survive), and rewrite the run_kql Fixed entry to describe the guard's current, exemption-free behavior. prompts/f0-sectools-system-prompt.md: note that list_sentinel_incidents' severity_min (informational|low|medium|high) and f0-defender.list_incidents' severity_min (info|low|medium|high|critical) are different enums and not interchangeable, now that the prompt encourages calling both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
search_audit_log is an asynchronous Graph search that takes 5-15 minutes; search_office_activity (f0-sentinel) answers the same SharePoint/OneDrive/ Exchange/Teams question from Log Analytics in under a second. The tool docstrings on both sides already cross-referenced each other, but the audit-investigation skill an agent actually follows during an investigation did not, so it would sit through the slow path unnecessarily. Point the skill at the fast path first, falling back to search_audit_log only when there's no Sentinel workspace or the records predate its retention. Add the same reciprocal pointer to the purview-mcp README, which was missing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
…ing docs Sentinel's get_detection_coverage answers an executive risk question no other server covers -- how much of the ATT&CK matrix do our own analytics rules actually cover, as distinct from Microsoft-managed ones. Add it as a seventh pillar to the ciso-risk-rollup skill, with the custom-vs-overall distinction stated as a pitfall, and update every place that names the pillar count (workflows.md, the pi and opencode CISO prompts). The report generator's CISO persona (report_gather.py) is unchanged at six pillars -- Sentinel isn't wired into it -- so the report-table row and the opencode prompt's report-generation note are worded to say so explicitly rather than imply seven. Also update workflows.md's threat-hunt and detection-coverage-check sections to point at Sentinel, and add three Sentinel-specific workflow sections (network investigation, detection coverage review, data-source coverage) mirroring the file's existing per-platform style, including the domain-vs-IP routing rule for hunt_dns_web vs hunt_firewall. Add the missing `sentinel` slug to findings-schema.md's source enum, and fix examples/mcp/README.md's stale "Defender + Entra" description of mcp.json (now nine servers) plus a caveat that it ships the gated-write f0-pa-actions server with no enabled:false equivalent, unlike every other wiring template. Regenerated docs/reference/tools/sentinel.md via scripts/gen_docs.py -- the skill edits above added get_detection_coverage and search_office_activity cross-references that the generator picks up automatically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
…rvers The four opencode persona agents and pi's AGENTS.md all introduce the assistant with a hardcoded platform list that stopped at Tenable -- missing Purview (server #8) and Sentinel (#9). These are the identity line every persona session starts from, so an agent was being told two of its nine platforms do not exist. The pi persona prompts under integrations/pi/prompts/ are lens-only and correctly enumerate no servers; left alone.
The prompt itself was rewritten for all nine servers earlier on this branch; its own index entry was the last thing still describing the two-server version.
|
Claude finished @ubercylon8's task in 2m 37s —— View job Security review of PR #101
SummaryThis is a clean, well-executed security fix. I traced both boundary fixes against #99 —
#100 — redaction boundary (
Gated-write path — Docs/wiring — No issues found with the findings-schema conformance, small-model argument shape ( I wasn't able to execute |
What & why
Closes the two security-boundary findings from the review of #98, and catches the
documentation up with a repo that has grown from two servers to nine.
Closes #99. Closes #100.
#99 —
run_kql's control-command guardrun_kqlaccepts caller-supplied KQL on a read-only-by-charter server, so Kustocontrol commands (
.create,.drop,.ingest, …) must be rejected beforedispatch. The guard checked only the whole-query prefix, so a dot-command on a
second line went through.
This took four attempts, and the middle two made it worse. Recording that here
because the shape of the failure matters more than the fix:
.drop table X ```through on line 1 — something the original code caught. A net regression// ```/.drop table X/// ```dispatchedRound 2 was caught by an automated commit review; round 3 by a review subagent
attacking the guard structurally. Both survived the implementer's tests and a
hand-written ten-case probe, because every case anyone thought to write put the
fence on a different line from the command.
The structural lesson:
exempt_verbatim_stringswas computed from the caller's ownstring, so the caller chose both whether the exemption applied and which lines it
hid. A partial lexer bolted onto a prefix check can only ever make a guard blinder,
never stricter. Completing it correctly would need a real KQL lexer.
Measured before deleting it: with face-value per-line classification, decimal
continuations,
//comments andletstatements all still pass on the.-followed-by-a-letter rule alone. The exemption rescued exactly one case — amulti-line sample log whose interior line starts with
.+letter — and that failureis a false rejection with a clear posture finding, not a security event. The guard
is now simpler than any previous version and strictly stronger. A comment records
that the exemption was tried and removed, so it does not come back as an
"improvement".
The same-line
;vector (Heartbeat | take 1; .drop) stays knowingly open anddocumented: rejecting
;would break legitimateletstatements.#100 — redaction boundary across all nine servers
core/redactionshipsredact_objandredact_finding. Only the latter blanks anevidence value whose key hints at a secret — necessary because
Evidenceserialises flat as
{"key": …, "value": …}, soredact_obj's key check only eversees the literal strings
"key"/"value".Every server's
_rendercalledredact_obj; onlyreports/emit.pycalledredact_finding. So the generated-report path was better protected than the livetool path — backwards, since the live path is where a secret surfaces first.
redact_finding's own docstring claimed parity with_render, which was false.All nine servers switched (import + one line each, no other change to the eight
shipped servers), plus the smoke/demo scripts. Two
redact_objcalls deliberatelyremain where the payload is a raw platform dict rather than a
Finding.Changed behaviour worth knowing about: the key-hint pass cannot distinguish a
credential name from a column name, so hunt-result columns like
TokenIssuerType,ApiKeyId,CookieCount,PasswordExpiryare now blanked in tool output.TokenIssuerTypein particular is an analytically useful sign-in column. This is thefail-safe direction, and the CHANGELOG carries a Changed note so an operator
expects it.
Documentation
The counts were stale (51 tools / eight servers / 22–25 skills), but the substantive
gaps were about capability:
skills/purview/audit-investigationstill taught the slow path — walking anagent through the 5–15 minute async
search_audit_logwith no mention thatsearch_office_activityanswers the same question in under a second. The tooldocstrings cross-referenced each other since feat(sentinel): sentinel-mcp server #9 — KQL telemetry, SOC incident queue, detection coverage #98; the skill an agent actually
follows did not.
skills/cross-platform/ciso-risk-rolluppulled a posture pillar from sixservers. Sentinel adds one nothing else answers — how much of ATT&CK our own
analytics rules cover — so it is now seven, and the skill directs the reader to the
custom-rule figure rather than the overall number that Microsoft-managed rules
inflate.
Tenable, missing Purview and Sentinel. That is the identity line every persona
session starts from.
prompts/f0-sectools-system-prompt.mdanddocs/running-with-local-models.mdstill described a two-server repo. Both rewritten for nine. The prompt is pasted as a
system prompt for small local models, so it stays orientation-and-routing at ~1050
words rather than becoming a 58-tool catalogue — the MCP client already advertises
the tools; the prompt's job is deciding which server answers the question.
Two test gates were also unfailable and are now tied to the discovered server count:
test_examples_valid.pyassertedlen(SAMPLES) >= 8, and the redactionparametrisation was a hand-maintained list. Server #10 can no longer silently skip
either.
examples/findings/sentinel.jsonwas missing and has been added.Known gaps, stated rather than papered over
scripts/report_gather.pystill gathers six pillars — Sentinel is not wired intoautomated report generation. The agent-driven rollup covers seven.
workflows.mdsaysso explicitly. Adding
_pillar_detection_coveragefollows an obvious existing pattern.examples/mcp/mcp.jsonis the only wiring template with no drift guard, and shipsf0-pa-actionswithout theenabled: falseequivalent every other template carries.The README now carries a caveat.
Checklist (mirrors the Critical Rules in CLAUDE.md)
run_kql's guard strengthened.core/— the redaction fix routes all nine serversthrough one existing
core/entry point rather than adding per-server logic.examples/findings/sentinel.jsonuses synthetic values.uv run pytest(1006 passed, 1 skipped),uv run ruff check .,uv run mypy .all clean;gen_docs.pyproduces no diff.🤖 Generated with Claude Code
https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ