Skip to content

Commit 29d6979

Browse files
branoverclaude
andauthored
fix: make reveal's Ghidra enrichment opt-in, not automatic (#277)
* fix: make reveal's Ghidra enrichment opt-in, not automatic Follow-up to #275 (the target_reveal_dir hang fix): revealing a target has always ALSO auto-triggered optional Ghidra enrichment whenever features.ghidra.enrich_recon is on — a coupling that predates #275 and that #275 only made non-blocking, not optional. On review with the operator: "reveal" reads as a pure visibility toggle, but silently queuing a potentially long-running background analysis job for every revealed target is surprising — this coupling is what actually caused the incident (revealing an Erlang erts/bin directory queued a dozen-plus background Ghidra jobs the operator never explicitly asked for on that call). set_visible/reveal_dir gain an explicit `enrich: bool = False` parameter. Enrichment now requires BOTH the global features.ghidra.enrich_recon setting AND an explicit per-call enrich=True — reveal alone never queues analysis. The capability isn't removed (an agent doing deliberate VR work can still opt in per call), just no longer implicit. Threaded through the whole stack: engine (reveal.py), MCP tools + catalog schemas, REST (VisibleUpdate/RevealDir gain enrich: bool = False), and frontend API types. FilesystemBrowser.tsx's reveal actions don't pass enrich=true (reveal-to-browse shouldn't trigger analysis); the now-dead "Ghidra enrichment running" UI message for a directory reveal was removed since it can no longer fire from that path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: fix stale _maybe_enrich_ghidra docstring after the enrich opt-in change Flagged in #277's review: the docstring still said "reveal runs this enrichment then" — no longer true now that reveal requires an explicit per-call enrich=True, not just the global feature flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3f96c96 commit 29d6979

9 files changed

Lines changed: 139 additions & 65 deletions

File tree

frontend/src/api.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -306,12 +306,15 @@ export const api = {
306306
filesystem: (tid: string) => getJSON<{ unpacked: boolean; method?: string; files: FsEntry[] }>(`/api/targets/${tid}/filesystem`),
307307
promoteFile: (pid: string, fwId: string, rel: string) => postJSON<any>(`/api/projects/${pid}/targets/${fwId}/promote-file`, { rel }),
308308
// Reveal/re-hide a target in the curated graph. Firmware ELF children are hidden by default
309-
// (unpack registers them all); revealing materializes the target's recon nodes from stored facts.
310-
setTargetVisible: (pid: string, tid: string, visible: boolean) => postJSON<{ target_id: string; name: string; visible: boolean; materialized: boolean; enrichment_queued: boolean }>(`/api/projects/${pid}/targets/${tid}/visible`, { visible }),
309+
// (unpack registers them all); revealing materializes the target's recon nodes from stored
310+
// facts. Ghidra enrichment does NOT run automatically — pass enrich:true to also queue it,
311+
// detached (default false: revealing to browse shouldn't silently kick off analysis jobs).
312+
setTargetVisible: (pid: string, tid: string, visible: boolean, enrich = false) => postJSON<{ target_id: string; name: string; visible: boolean; materialized: boolean; enrichment_queued: boolean }>(`/api/projects/${pid}/targets/${tid}/visible`, { visible, enrich }),
311313
// Bulk-reveal every hidden firmware child whose rootfs path is under `prefix` (e.g. "usr/sbin").
312-
// `enrichment_queued` = how many revealed targets got a background Ghidra enrichment task
313-
// (revealing/materializing itself is synchronous and fast; enrichment is not).
314-
revealDir: (pid: string, fwId: string, prefix: string) => postJSON<{ firmware_target_id: string; prefix: string; revealed: number; target_ids: string[]; enrichment_queued: number }>(`/api/projects/${pid}/targets/${fwId}/reveal-dir`, { prefix }),
314+
// `enrichment_queued` = how many revealed targets got queued into ONE background batch
315+
// Ghidra-enrichment task (revealing/materializing itself is always synchronous and fast;
316+
// enrichment only happens at all when enrich:true is passed — default false).
317+
revealDir: (pid: string, fwId: string, prefix: string, enrich = false) => postJSON<{ firmware_target_id: string; prefix: string; revealed: number; target_ids: string[]; enrichment_queued: number }>(`/api/projects/${pid}/targets/${fwId}/reveal-dir`, { prefix, enrich }),
315318
// Target-level follow-up suggestions from enriched recon metadata (e.g. risky-sink → static-analyze).
316319
targetSuggestions: (tid: string) => getJSON<any[]>(`/api/targets/${tid}/suggestions`),
317320
// Source trees (Phase 1 — read-only IDE browse)

frontend/src/components/FilesystemBrowser.tsx

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,18 +68,16 @@ export default function FilesystemBrowser({ projectId, targetId, onChanged }: {
6868
finally { setBusy(""); }
6969
};
7070

71-
// Reveal every hidden child under a directory prefix in one call.
71+
// Reveal every hidden child under a directory prefix in one call. Ghidra enrichment is
72+
// NOT requested here (api.revealDir's enrich param defaults to false) — reveal is a fast,
73+
// predictable visibility toggle; deliberately opting a whole directory into a dozen-plus
74+
// background Ghidra analyses is a bigger decision than "let me see what's in here",
75+
// and doing it implicitly on every reveal is what caused a real 2+ hour incident.
7276
const revealDir = async (prefix: string) => {
7377
setBusy("dir:" + prefix);
7478
try {
7579
const r = await api.revealDir(projectId, targetId, prefix);
7680
if (r.revealed === 0) alert("No hidden binaries to reveal under " + (prefix || "/"));
77-
// Ghidra enrichment per binary (if enabled) runs in the background — a directory with
78-
// many binaries would otherwise block this call for a long time (each is a cold headless
79-
// Ghidra pass). The binaries are already revealed/searchable; enrichment just deepens them.
80-
else if (r.enrichment_queued > 0)
81-
alert(`Revealed ${r.revealed} binaries. Ghidra enrichment for ${r.enrichment_queued} of `
82-
+ `them is running in the background and may take a while.`);
8381
await load(); onChanged?.();
8482
} catch (e: any) { alert(String(e.message || e)); }
8583
finally { setBusy(""); }

src/hexgraph/agent/mcp_catalog.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,10 @@
125125
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string"}}, "required": ["project_id", "target_id"]}),
126126
("write", "target_restore", _t.restore_target, "Un-archive a previously soft-removed target subtree (its nodes/findings reappear).",
127127
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string"}}, "required": ["project_id", "target_id"]}),
128-
("write", "target_set_visible", _t.set_visible, "REVEAL (visible=true) or re-HIDE (visible=false) ONE target in the curated graph. Firmware ELF children are hidden by default — unpack registers each (searchable via target_list(include_hidden=true), addressable, recon-enriched) but they'd flood the graph, so they contribute NOTHING to it until revealed. Revealing materializes the target's recon symbol/string nodes from the already-stored facts (no re-run) so it joins the graph + Targets pane immediately; optional Ghidra enrichment (if enabled) runs DETACHED in the background a cold headless pass can take minutes, this call does not wait for it. Use after target_list(include_hidden=true)/fs_list surfaces a child worth analyzing. Returns {target_id, name, visible, materialized, enrichment_queued}.",
129-
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string"}, "visible": {"type": "boolean", "description": "true to reveal (default), false to re-hide"}}, "required": ["project_id", "target_id"]}),
130-
("write", "target_reveal_dir", _t.reveal_dir, "REVEAL every HIDDEN child of a firmware whose rootfs path is under `prefix` (e.g. prefix='usr/sbin' reveals all ELFs in /usr/sbin) — the bulk counterpart to target_set_visible for bringing a whole directory of binaries into the curated graph at once. An empty prefix reveals ALL hidden children. Materializes each revealed child's recon nodes from stored facts (no re-run) — fast, even for a directory with many binaries; optional per-binary Ghidra enrichment (if enabled) runs DETACHED in the background instead of blocking this call (a dozen+ binaries analyzed sequentially can take hours). `target_id` is the firmware. Returns {firmware_target_id, prefix, revealed, target_ids, enrichment_queued}.",
131-
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string", "description": "the firmware target whose children to reveal"}, "prefix": {"type": "string", "description": "rootfs directory prefix (e.g. 'usr/sbin'); empty reveals all hidden children"}}, "required": ["project_id", "target_id"]}),
128+
("write", "target_set_visible", _t.set_visible, "REVEAL (visible=true) or re-HIDE (visible=false) ONE target in the curated graph. Firmware ELF children are hidden by default — unpack registers each (searchable via target_list(include_hidden=true), addressable, recon-enriched) but they'd flood the graph, so they contribute NOTHING to it until revealed. Revealing materializes the target's recon symbol/string nodes from the already-stored facts (no re-run) so it joins the graph + Targets pane immediately. Ghidra enrichment does NOT run automatically — pass `enrich=true` to also queue it DETACHED in the background (a cold headless pass can take minutes; still needs features.ghidra.enrich_recon on — enrich=true is an ADDITIONAL opt-in per call, not a replacement). Use after target_list(include_hidden=true)/fs_list surfaces a child worth analyzing. Returns {target_id, name, visible, materialized, enrichment_queued}.",
129+
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string"}, "visible": {"type": "boolean", "description": "true to reveal (default), false to re-hide"}, "enrich": {"type": "boolean", "description": "also queue detached Ghidra enrichment for this target (default false — reveal alone does not trigger analysis)"}}, "required": ["project_id", "target_id"]}),
130+
("write", "target_reveal_dir", _t.reveal_dir, "REVEAL every HIDDEN child of a firmware whose rootfs path is under `prefix` (e.g. prefix='usr/sbin' reveals all ELFs in /usr/sbin) — the bulk counterpart to target_set_visible for bringing a whole directory of binaries into the curated graph at once. An empty prefix reveals ALL hidden children. Materializes each revealed child's recon nodes from stored facts (no re-run) — fast, even for a directory with many binaries. Ghidra enrichment does NOT run automatically — pass `enrich=true` to also queue it for the WHOLE batch as one detached background process working through them sequentially (a dozen+ binaries can take hours; still needs features.ghidra.enrich_recon on). Don't pass enrich=true reflexively; it's for when you deliberately want deep analysis on everything in that directory, not the default way to browse. `target_id` is the firmware. Returns {firmware_target_id, prefix, revealed, target_ids, enrichment_queued}.",
131+
{"type": "object", "properties": {"project_id": {"type": "string"}, "target_id": {"type": "string", "description": "the firmware target whose children to reveal"}, "prefix": {"type": "string", "description": "rootfs directory prefix (e.g. 'usr/sbin'); empty reveals all hidden children"}, "enrich": {"type": "boolean", "description": "also queue detached Ghidra enrichment for every revealed binary, as one batched background process (default false)"}}, "required": ["project_id", "target_id"]}),
132132

133133
# ---- re: static reverse engineering ---------------------------------------------
134134
("read", "re_list_functions", _t.list_functions, "List/GREP the functions discovered in a target (name + the discovered set) for a substring `pattern` (or a regex with regex=true) — the fast function-name search. Server-side filtered + PAGINATED exactly like re_list_strings: pass `pattern` to filter by name, `offset`/`limit` to page (default 200, max 1000); the result reports the total match count + the next offset. With no `pattern` it pages the whole list. QUERY: records a function_list Observation; adds no graph nodes. Requires a saved analysis when Ghidra is the active backend (run re_analyze first on a warm miss).", {"type": "object", "properties": {"target_id": {"type": "string"}, "pattern": {"type": "string", "description": "substring to grep the function names for"}, "regex": {"type": "boolean", "description": "treat pattern as a regex (falls back to substring if it doesn't compile)"}, "offset": {"type": "integer", "description": "page start index into the matches (default 0)"}, "limit": {"type": "integer", "description": "max names to return (default 200, clamped 1-1000)"}}, "required": ["target_id"]}),

src/hexgraph/agent/mcp_tools.py

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1543,38 +1543,43 @@ def restore_target(project_id: str, target_id: str) -> dict:
15431543
return {"error": str(exc)}
15441544

15451545

1546-
def set_visible(project_id: str, target_id: str, visible: bool = True) -> dict:
1546+
def set_visible(project_id: str, target_id: str, visible: bool = True, enrich: bool = False) -> dict:
15471547
"""REVEAL (visible=true) or re-HIDE (visible=false) one target in the curated graph.
15481548
Firmware ELF children are HIDDEN by default (unpack registers each so it's searchable
15491549
and addressable, but a 765-ELF firmware would otherwise flood the graph/Targets pane);
15501550
recon already enriched them. Revealing materializes the target's recon nodes from the
1551-
already-stored facts (no re-run) so it joins the graph immediately; optional Ghidra
1552-
enrichment (if enabled) runs DETACHED in the background — `enrichment_queued` says
1553-
whether a task was kicked off (a cold headless Ghidra pass can take minutes; this call
1554-
does not wait for it). Returns {target_id, name, visible, materialized, enrichment_queued}."""
1551+
already-stored facts (no re-run) so it joins the graph immediately. Ghidra enrichment does
1552+
NOT happen automatically — pass `enrich=true` to also queue it, DETACHED in the background
1553+
(a cold headless Ghidra pass can take minutes; still requires features.ghidra.enrich_recon
1554+
to be on, `enrich=true` is an additional opt-in per call, not a replacement for it).
1555+
`enrichment_queued` says whether a task was actually kicked off. Returns {target_id, name,
1556+
visible, materialized, enrichment_queued}."""
15551557
from hexgraph.engine.targets.reveal import set_visible as _set
15561558

15571559
with session_scope() as s:
15581560
if s.get(Project, project_id) is None:
15591561
return {"error": "project not found"}
15601562
try:
1561-
return _set(s, project_id, target_id, visible)
1563+
return _set(s, project_id, target_id, visible, enrich=enrich)
15621564
except ValueError as exc:
15631565
return {"error": str(exc)}
15641566

15651567

1566-
def reveal_dir(project_id: str, target_id: str, prefix: str = "") -> dict:
1568+
def reveal_dir(project_id: str, target_id: str, prefix: str = "", enrich: bool = False) -> dict:
15671569
"""REVEAL every HIDDEN child of a firmware whose rootfs path is under `prefix`
15681570
(e.g. prefix='usr/sbin' reveals all ELFs in /usr/sbin) — the bulk counterpart to
15691571
target_set_visible for bringing a whole directory of binaries into the curated graph
15701572
at once. An empty prefix reveals ALL hidden children. Materializes each revealed
15711573
child's recon nodes from stored facts (no re-run) — fast, even for a directory with many
1572-
binaries. Optional Ghidra enrichment per binary (if enabled) runs DETACHED in the
1573-
background rather than blocking this call (a cold headless Ghidra pass on a dozen+
1574-
binaries sequentially can take hours; this call returns as soon as they're revealed).
1574+
binaries. Ghidra enrichment does NOT happen automatically — pass `enrich=true` to also
1575+
queue it for the whole batch as ONE detached background process working through them
1576+
sequentially (a cold headless Ghidra pass on a dozen+ binaries can take hours; still
1577+
requires features.ghidra.enrich_recon to be on). Don't pass enrich=true reflexively —
1578+
it's meant for when you deliberately want deep analysis on everything in that directory,
1579+
not as the default way to browse a firmware's contents.
15751580
`target_id` is the firmware. Returns {firmware_target_id, prefix, revealed, target_ids,
1576-
enrichment_queued} (enrichment_queued = how many revealed targets got a background
1577-
enrichment task)."""
1581+
enrichment_queued} (enrichment_queued = how many revealed targets got queued into that
1582+
background batch; always 0 when enrich=false)."""
15781583
# NB: the catalog advertises this arg as `target_id` (the firmware), and the MCP
15791584
# server dispatches by KEYWORD (`fn(**arguments)`), so this param name MUST match
15801585
# the catalog schema — otherwise every MCP call raises TypeError.
@@ -1584,7 +1589,7 @@ def reveal_dir(project_id: str, target_id: str, prefix: str = "") -> dict:
15841589
if s.get(Project, project_id) is None:
15851590
return {"error": "project not found"}
15861591
try:
1587-
return _reveal(s, project_id, target_id, prefix)
1592+
return _reveal(s, project_id, target_id, prefix, enrich=enrich)
15881593
except ValueError as exc:
15891594
return {"error": str(exc)}
15901595

src/hexgraph/agent/vr_skill.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -417,10 +417,12 @@
417417
`fs_list` entry that's `added` but not `revealed` already IS a target — reveal it (recon
418418
already enriched it; reveal materializes its nodes, no re-run) rather than re-promoting.
419419
Reveal the binaries worth analyzing, then decompile / `task_run` / fuzz them like any other.
420-
If Ghidra enrichment is enabled, it runs DETACHED per revealed binary (`enrichment_queued`
421-
in the response) — a directory can have a dozen-plus binaries, each a cold headless Ghidra
422-
pass; don't expect it done by the time this call returns, and don't loop calling
423-
`target_reveal_dir` back-to-back on overlapping prefixes waiting for it to "finish".
420+
Ghidra enrichment does NOT run automatically on reveal — pass `enrich=true` only when you
421+
deliberately want deep Ghidra analysis on everything you're revealing (still needs
422+
features.ghidra.enrich_recon on). When you do, it runs DETACHED as one batched background
423+
process, not one per binary (`enrichment_queued` in the response) — a directory can have a
424+
dozen-plus binaries; don't expect it done by the time this call returns, and don't loop
425+
calling `target_reveal_dir` back-to-back on overlapping prefixes waiting for it to "finish".
424426
- **target_promote_file(target_id, path)** — promote a NON-ELF file or one unpack didn't register
425427
(a CGI script, a helper) into its OWN child target (created visible), then analyze it. Use
426428
`target_set_visible`/`target_reveal_dir` for the ELF children that already exist hidden, and

src/hexgraph/api/routers/targets.py

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -177,41 +177,49 @@ def api_remove_target(project_id: str, target_id: str):
177177

178178
class VisibleUpdate(BaseModel):
179179
visible: bool = True
180+
# Off by default: revealing a target and running a (detached, but still real) Ghidra
181+
# analysis on it are separate decisions — see engine.targets.reveal for why auto-enrich
182+
# on reveal was a real incident (an operator paged 2+ hours revealing a directory whose
183+
# dozen-plus binaries each silently queued a background analysis).
184+
enrich: bool = False
180185

181186

182187
@router.post("/api/projects/{project_id}/targets/{target_id}/visible")
183188
def api_set_target_visible(project_id: str, target_id: str, body: VisibleUpdate):
184189
"""Reveal (visible=true) or re-hide (visible=false) one target. Firmware children
185190
are HIDDEN by default (unpack registers them so they're searchable/addressable, but
186191
they add nothing to the curated graph); revealing materializes the target's recon
187-
nodes from the already-stored facts (no re-run)."""
192+
nodes from the already-stored facts (no re-run). Ghidra enrichment does not run
193+
automatically — pass enrich=true to also queue it, detached."""
188194
from hexgraph.engine.targets.reveal import set_visible
189195

190196
with session_scope() as s:
191197
if s.get(Project, project_id) is None:
192198
raise HTTPException(404, "project not found")
193199
try:
194-
return set_visible(s, project_id, target_id, body.visible)
200+
return set_visible(s, project_id, target_id, body.visible, enrich=body.enrich)
195201
except ValueError as exc:
196202
raise HTTPException(404, str(exc))
197203

198204

199205
class RevealDir(BaseModel):
200206
prefix: str = ""
207+
enrich: bool = False
201208

202209

203210
@router.post("/api/projects/{project_id}/targets/{target_id}/reveal-dir")
204211
def api_reveal_dir(project_id: str, target_id: str, body: RevealDir):
205212
"""Reveal every HIDDEN child of a firmware whose rootfs path is under `prefix`
206213
(e.g. "usr/sbin" reveals all ELFs under that directory). Materializes each revealed
207-
child's recon nodes. `target_id` is the firmware target."""
214+
child's recon nodes. `target_id` is the firmware target. Ghidra enrichment does not run
215+
automatically — pass enrich=true to also queue it for the whole batch, detached."""
208216
from hexgraph.engine.targets.reveal import reveal_dir
209217

210218
with session_scope() as s:
211219
if s.get(Project, project_id) is None:
212220
raise HTTPException(404, "project not found")
213221
try:
214-
return reveal_dir(s, project_id, target_id, body.prefix)
222+
return reveal_dir(s, project_id, target_id, body.prefix, enrich=body.enrich)
215223
except ValueError as exc:
216224
raise HTTPException(404, str(exc))
217225

src/hexgraph/engine/pipeline.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,9 @@ def _record_progress(session: Session, target: Target, stage: str, **extra) -> N
5353
def _maybe_enrich_ghidra(session: Session, project: Project, target: Target, facts: dict) -> None:
5454
"""Optionally fold Ghidra's function/call-graph/struct inventory into the graph
5555
(Settings → features.ghidra.enrich_recon). Best-effort: never breaks recon.
56-
Skipped for HIDDEN targets — a hidden target adds nothing to the curated graph
57-
until revealed (reveal runs this enrichment then)."""
56+
Skipped for HIDDEN targets — a hidden target adds nothing to the curated graph until
57+
revealed, and revealing no longer auto-enriches either (engine.targets.reveal requires
58+
an explicit per-call `enrich=True`, not just the global setting)."""
5859
if not target.visible:
5960
return
6061
if facts.get("kind") not in _ENRICHABLE_KINDS:

0 commit comments

Comments
 (0)