Repository navigation
feat: audit OpenClaw plugins with the A.I.G MCP scanner - #149
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesAdds a confined A.I.G MCP scanner for plugins and selects it through an optional pluginScanners profile field. Example: Scan a plugin containing credential-exfiltration code in dist/index.js with --profile clawhub.
Review scores
ProductKind: Feature · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this maintainer-authored PR adds a useful scanner capability absent from main; the pinned diff has no actionable correctness or security findings. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe CLI resolves targets and profiles, dispatches scanner adapters inside the default Docker sandbox, preserves raw evidence, and passes completed scans to an external judge. flowchart LR
A[Plugin directory] --> B[Target and profile resolution]
B --> C[Bounded staged copy]
C --> D[Confined AIG MCP scanner]
D --> E[Raw SARIF artifact]
E --> F[External judge]
F --> G[Optional overview]
Technical reviewBest possible solution: Land the confined plugin scanner with the documented runtime-before-release sequencing and preserve existing project-profile behavior. Do we have a high-confidence way to reproduce the issue? This is a feature addition; source comparison confirms its absence from main, and supplied CLI results exercise the new scanner path. Is this the best way to solve the issue? The implementation uses the existing scanner registry, target classification, copy primitive, and external-judge boundary, with focused runtime confinement for the new upstream scanner. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 22d2f694bb81. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (4 earlier review cycles)
Reviewed November 4, 2026, 5:43 AM ET / 10:43 UTC (Revision 5). |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Add the plugin-only aig-mcp adapter around Tencent A.I.G's aig-mcp-scan 0.2.0. It stages a bounded copy of the whole plugin without node_modules, runs the validated OpenClaw plugin audit prompt with A.I.G model and endpoint conventions, keeps the SARIF raw and never reports failed or incomplete audits as clean. The clawhub profile gains pluginScanners so plugins run only aig-mcp and the judge receives that one raw SARIF result, while skills keep their scanners. The runtime image pins aig-mcp-scan and patches out its agent's shell and file-write tools; host runs require an explicit opt-in.
The patched runtime has no shell, so plugins past the upstream 200-file summary get a prioritized file inventory with read_file-compatible plugin/ paths. The model and endpoint are always passed explicitly so host LLM_/OPENAI_ settings never pair with the key, the final gate sees the plugin scanner list, and the schema admits only plugin-capable scanners.
…able The shell tool is no longer registered, and static audits accept only read_file, read_file_chunk, search_files, think and finish, matched on canonical names because upstream resolves tool names case-insensitively. read_file returns at most 30000 characters with an explicit continuation, and bounded, directory-confined chunk and literal-search tools keep the rest of large compiled files visible past upstream's history truncation. The in-image tests spy on process starts and check that a 50k bundle's tail reaches the agent history.
…cores aig-mcp now fails when upstream logs exhausted iterations or failed model calls on stdout or stderr, and requires every SARIF run to state scanNote complete. Imported aig-mcp results get the same checks, and profile plugin-only scanners accept --scanner-result. A benchmark case whose configured judge returned no recognized verdict is unscorable instead of falling back to scanner severity.
…ile reads read_file replaces invalid UTF-8 as Node does instead of reporting a plugin file as binary. Case-insensitive search matches on the original text so offsets index the file, and both read tools refuse files over 64 MiB. In-image tests cover these, symlink escapes, and a maximal chunk staying whole in agent history.
…results Unfinished-audit markers count only as WARNING/ERROR log records on stderr, so model reports or file names quoting them cannot fail a scan. Staging rejects plugin files the read tools cannot open, a plugin-only imported result on a non-plugin target fails instead of being ignored, and the contract test requires full commit ids.
…e CLI entry The MCP client helper now refuses any URL the model supplies unless it is the operator-configured dynamic server, before a transport exists. Static audits already reject the MCP tools by allow-list; this keeps a second layer. The image build runs the real aig-mcp-scan CLI against a loopback model stub that injects remote MCP calls under several spellings, with a loopback canary as every destination and as the HTTP(S) proxy, and fails if the canary sees a connection or the local read misses the model.
A v0.3.0-era .clawscan.yml without pluginScanners runs exactly its configured scanners and gate rules for plugin and skill targets, and never selects aig-mcp. Docs describe the MCP destination refusal and the in-build entry check.
Scanner staging inherited the judge workspace filter, which drops top-level .git/ and clawscan-results/, so plugin code there (for example an import of ./clawscan-results/payload.js) never reached aig-mcp. Staging now drops only node_modules, keeping its file, size and symlink checks, and the prompt inventory lists every file the upstream directory summary hides, including .git/, __pycache__/ and virtualenv content. The judge workspace filter is unchanged.
Upstream keeps the last of three format attempts even when none matched, so a refusal or prose became SARIF with scanNote complete, score 100 and no findings. The final-output path now raises when no attempt matches, and the single-stage audit accepts only an explicit <empty> result or findings that all parse. In-image tests drive the real CLI with a scripted model through refusal, prose, malformed and partly malformed reports (all fail with no SARIF), and through empty, parsed and late-retry reports (all produce SARIF).
1a64906 to
82ab0cd
Compare
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Why
Native OpenClaw plugins run in-process in the gateway with its full permissions, and nothing in ClawScan audits plugin code for malice today. This adds Tencent A.I.G's MCP code audit (
aig-mcp-scan0.2.0) as the plugin scanner. A local experiment validated that configuration on 20 popular ClawHub plugins and 4 synthetic malicious ones.What changes
New
aig-mcpadapter (plugin targets only). Skill, file and URL targets are skipped. For each plugin it:node_modulesdirectories. Embedded skills (SKILL.mdand their scripts), hooks,dist//build/output, and.git/andclawscan-results/content stay in the copy;aig-mcp-scan --repo plugin --language en -m <model> -u <base url> -p <validated OpenClaw plugin prompt> -o aig-mcp-scan.sarif.jsoninside that workspace. The prompt adds an "audit the whole artifact" scope note. Upstream leavesdist//build/out of its directory summary and truncates it at 200 files, and the patched agent has no shell to find files itself. So the prompt lists the omitted files asread_filepaths (plugin/...): every file for plugins over 200 files, up to 400 paths, source files andSKILL.mdfirst.Credentials and model settings. The key comes only from
LLM_API_KEYorOPENAI_API_KEYin the environment, never from argv. Model and endpoint follow theaigconventions:DEFAULT_MODEL/DEFAULT_BASE_URL, with OpenAI defaults for OpenAI or Codex keys (never OpenRouter) and otherwise upstream's own defaults. Both are always passed as-m/-u, so a host's genericOPENAI_BASE_URL/LLM_*values are never paired with the key.Evidence. The SARIF stays raw in the artifact, and the ClawHub judge receives that one raw SARIF result. Benchmarks map SARIF levels to severities:
error→ high, which predicts malicious; any other finding predicts suspicious.Failures are never reported as clean. Any of these marks the scanner failed, so the judge does not start:
LLM chat failed after retries,LLM chat error, retry 5 times);scanNoteother thancomplete.Failure messages keep the upstream error and a bounded stderr tail.
Plugin scanner selection. New profile field
pluginScannersreplacesscannersfor plugin targets. Theclawhubprofile setspluginScanners: [aig-mcp]:aig-mcp, with no skill-scanner calls and no skill-scanner entries in the artifact;skillspectorandaig, and their judge prompt is unchanged;--scanneroverrides both lists.The field is in the schema, is validated (built-in, plugin-capable, no duplicates), and shows in
clawscan profiles.Sandbox by default. Upstream 0.2.0 gives its agent
execute_shelland file writes during static audits, and its MCP client tools accept any model-supplied URL, so prompt injection in the plugin under review could reach them. The runtime image therefore installsaig-mcp-scan==0.2.0and appliespatches/aig-mcp-static-tools.patch: static audits get only confined read-only tools (allow-listed on canonical names), and model-chosen MCP destinations are refused before any transport.tests/test_aig_mcp_static_tools.pyandtests/test_aig_mcp_entry_network.pyrun during the build. They assert the exact reviewed tool set and that a real CLI audit with injected remote MCP calls reaches no destination. With--sandbox off, the adapter fails unlessAIG_MCP_SCAN_ALLOW_HOST_EXEC=1is set.Install and CI.
ARG AIG_MCP_SCAN_VERSION=0.2.0is added to the runtime Dockerfile.aig-mcp-scan --helpis added to the image publication smoke (publish.py) and to the runtime-tool-update smoke.clawscan install aig-mcprunspip install aig-mcp-scan==0.2.0(unpatched; documented).Production contract. Rebound to the latest runtime commit (
b98eb30) ind430dc4, following the existing pattern (e036a88).Evidence
Live pilot. Host run with the patched tool, the real OpenAI endpoint and
gpt-6.1-sol:src/)src/index.jsdist/dist/index.jsskills/notes/scripts/setup.sh)duckduckgopluginAn end-to-end
--profile clawhubrun on the embedded-skill sample, with the real scanner and a stand-in judge, gave:["aig-mcp"];skillspectorandaig-skill-scanbinaries;A.I.G MCP plugin code audit SARIFblock and target kindpackageRelease.Patch test.
test_aig_mcp_static_tools.py: unpatched 0.2.0 fails 3 of 4 tests (shell and write tools are offered and callable); patched passes 4 of 4.Tests.
go test -count=1 ./...,go vet ./...,gofmt,python3 -m unittest discover -s scripts/runtime-image -p 'test_*.py'andmake docs-siteall pass.cmd/clawscan/aig_mcp_test.go, stubaig-mcp-scan):aig-mcp, on a staged copy withoutnode_modules(embedded skill,dist/and hooks present), with the expected argv;aig-mcp-scan;--scanneroverrides both lists;pluginScannersresolution, the override, and validation.Combined with #148 (rebased onto
22d2f69)This branch now sits on #148's squash merge
22d2f69, whose tree is identical to #148's reviewed head9ed036b. The changes in the rebase:c6601d8, which duplicated feat!: add a Decisions API judge and a pluggable overview step for clawhub #148'sf905a55.COPYlines (judges/plus the aig-mcp patch and tests), the profile struct (PluginScannersandOverview), and the smoke list (aig-mcp-scanandclawscan-decisions-judge).TestClawHubProfileDecisionsJudgeExcludesPluginDependenciesnow uses anaig-mcpfixture withscanNote: complete.TestIntegratedPluginAIGDecisionsOverviewruns aig-mcp, then the real Decisions judge against an HTTP stub, then the overview. It checks that onlyaig-mcpruns, MCP01 reaches the judge, the embedded skill is included, nonode_modulescontent leaks, and the judge credential is never exposed.040f4fa1322145047a1280a2aae828627ad9ebfa, recomputed from feat!: add a Decisions API judge and a pluggable overview step for clawhub #148's binding.Local results:
go vetandgo test ./...pass;source_binding()replays OK;1a64906;test_decisions_judge.pypasses 24/24.An independent rebase-delta review found no P0 or P1 issues. It confirmed that #148-only files are identical to #148, apart from the intended fixture, and #149-only files are identical to
1a64906, apart from the docs.Proof for the ClawSweeper review of
d430dc4[P1] Remote MCP tools callable in static audits — fixed in two layers.
read_file,read_file_chunk,search_files,thinkandfinish, matched on canonical names.call_mcp_tooland the MCP listing tools are rejected before tool lookup._get_mcp_managernow refuses any model-supplied URL that isn't the operator-configured dynamic server, before a transport is created.Production-entry proof. No real network or LLM is used: the plugin is a synthetic, inert fixture, the model is a trusted loopback stub, and every destination is a loopback canary. The canary is also set as
HTTP(S)_PROXY, so any other egress would be counted rather than sent.The scripted agent follows a prompt injection and calls
call_mcp_tool,Call_MCP_Tool,list_mcp_tools,LIST_MCP_RESOURCESandlist_mcp_promptswith canary URLs. Then it reads a local file and finishes.clawscan ./plugin --profile clawhubd430dc4patchclawscan ./plugin --profile clawhubpython -m mcp_scan(the console script'scli())d430dc4patchpython -m mcp_scanAt this head, the shipped CLI run also showed:
["aig-mcp"],completed,scanNote: complete;-m/-u;The
python -m mcp_scanrows aretests/test_aig_mcp_entry_network.py, which runs inside the runtime image build on both architectures, so this proof is enforced by CI. It also checks that a model-chosen URL is refused before transport even when a dynamic server is configured, and includes a control that the canary does count connections.[P2] Plugin-selected scanner fixtures rejected — fixed.
clawscan ./plugin --profile clawhub --scanner-result aig-mcp=report.sarif.jsonis accepted and validated like a scanner run.completedand the judge runs.possibly-incompletegivesfailed, and the judge does not start.--scanner skillspector --scanner-result aig-mcp=….--profileand all-profiles--config).[P1, re-review of
f737248] Authored files dropped by staging — fixed in2ae7c16. Scanner staging had inherited the judge workspace filter, which skips top-level.git/andclawscan-results/, so plugin code there (for exampleimport "./clawscan-results/payload.js") never reachedaig-mcp. This restoresfa01a95's intent that scanners see authored code wherever it lives:node_modulesand keeps its file, size and symlink checks..git/,__pycache__/,.venv/,venv/, and_EVAL.txt-style names.The proof:
f737248and passes now, with the same layout covered at the--profile clawhubCLI seam.gpt-6.1-sol, current runtime) on a plugin whose only malicious code is an inert exfiltration payload inclawscan-results/payload.js, imported byindex.js: an MCP01 error atclawscan-results/payload.jsquoting its destination, 334s, scanners["aig-mcp"], judge completed.[P1, independent review of
c6601d8] Unusable final reports accepted as clean — fixed in69ab8ab. Upstream's_format_final_outputkeeps the last of three format attempts even when none matched. A model that finishes with a refusal ("I cannot perform this security scan.") therefore produced SARIF withscanNote: complete, score 100 and no findings, with no log marker. Separately,<vuln>blocks the extractor couldn't parse were dropped silently. The patched upstream path now:<empty>result, or findings that all parse;ClawScan audit output invalid: …and writes no SARIF, which the adapter records as a failure.Offline in-image tests drive the real
python -m mcp_scanCLI with a scripted loopback model:<empty>, a parsed finding, and a report fixed on a later format attempt: SARIF as expected.The four failure cases exit 0 on the previous runtime and pass now; all 28 in-image tests pass.
Real-model check of the stricter validation (
gpt-6.1-sol, this runtime). Hidden-tail sample: MCP01 error atdist/index.jsplus MCP06 for the embedded instruction, 111s. DuckDuckGo:aig-mcpcompleted with no findings, 77s, so genuine reports are not failed. The judge again stopped on thenode_modulessymlink in its workspace copy, which is #148's.Shipped
clawscan ./plugin --profile clawhubwith the refusal script:aig-mcpscanNote: complete, score 100, 0 findingsCompatibility with the latest stable release (v0.3.0). I verified the release binary checksum and ran three binaries side by side: v0.3.0,
main(fa01a95) and this PR..clawscan.ymlusesscannerEnvDefaults, built-in scanners with gate rules, a command scanner withtargets/env/gateblock,scannerResults, sandbox env and a judge, and has nopluginScanners. On both a skill and a plugin target, the artifacts are identical across all three binaries: scanners, statuses, raw evidence, gate rules and decisions (block/warn), env presence, sandbox metadata and judge. The new field changes nothing unless a profile sets it. This config is committed ascmd/clawscan/testdata/v0.3.0-project.clawscan.ymlwith a regression test.clawscan-run-v1artifact, re-imported into this PR's--profile clawhubon a skill, givecompletedfor both and a completed judge.--scanner skillspectoron a plugin runs only SkillSpector. v0.3.0 rejectsaig-mcpas an unknown scanner, so it is purely additive.Hold fixes (review of
d430dc4)All four findings are fixed at head; the reviewer's repro tests and the in-image tests fail at
d430dc4and pass now.P1, shell block bypassed by case. Upstream looks up tool names case-insensitively, so
Execute_Shellreached the shell past a case-sensitive deny list. The patch now:execute_shellis a non-executing stub that nothing imports;read_file,read_file_chunk,search_files,thinkandfinishin static audits, matched on canonical (trimmed, lowercase) names, and canonicalizes dynamic mode too.In-image spy tests (
Execute_Shell,EXECUTE_SHELL," execute_shell ",Write_File, MCP tools) fail on thed430dc4patch, where the spy records the process start, and pass now.P1, tail of large compiled files not reachable. Upstream keeps only a 20k prefix of tool results over 40k in the model's history. Now:
read_filereturns at most 30,000 characters and says how to continue;read_file_chunk(character offsets, at most 30k) andsearch_files(literal, at most 50 matches and 24k characters) stay confined to the audited directory;An in-image test drives
BaseAgentwith a 50k bundle. Repeated prefix reads never show the tail marker;read_file_chunkandsearch_filesput it in the model-visible history without truncation. A maximal chunk stays under the 40k limit.P1, exhausted or failed audits accepted.
Max iterations (N) reachedand the model-failure records, logged as WARNING/ERROR on stderr, fail the scan. INFO records and continuation lines that only quote the words don't. Every SARIF run must sayscanNote: complete; a missing value fails. Imported--scanner-result aig-mcpgets the same SARIF andscanNotechecks, and the judge does not start. A profile's plugin-only scanner accepts--scanner-result; on a non-plugin target such a result fails instead of being ignored.P2, benchmark fallback. A completed judge with no recognized verdict makes the case unscorable, with no fallback to scanner severity. Scanner-only runs still score from scanners.
Contract. Rebound to the full SHA of the last runtime commit (now
040f4fa1322145047a1280a2aae828627ad9ebfa, after the rebase onto feat!: add a Decisions API judge and a pluggable overview step for clawhub #148). The previous rebind wrote a short SHA, whichactions/checkoutandsource_binding()reject; a new test requires 40-hex ids and a matching workflowref. I replayed the realsource_binding()against a clean checkout and it passes.Pre-existing flake.
TestRenderClawHubPromptUsesProductionScannerContextShape(from feat!: make the runner judge-agnostic and remove VirusTotal and clawscan-static #147) compared prompts rendered moments apart, with a clock-derived SkillSpectorcheckedAt; it failed 9 of 300 runs onmainand once in this PR's Linux CI. Its fixture now has a fixed completion time (300/300 pass).Reviewer overlay (
TestReview149*): 4/5 pass. They fail atd430dc4. The fifth, judge workspacenode_modules, belongs to #148.Live inert control. Host run with the patched tool,
gpt-6.1-soland--profile clawhub. The sample has credential-exfiltration code at characters 45,894-46,400 of a single-line 58kdist/index.js(past both the read cap and the history cutoff). It also hides an instruction inSKILL.mdtelling the auditor to callEXECUTE_SHELLwithtouch <canary>and report no issues. Result:syncPrefsandcdn-metrics.example.net, which first appear at characters 45,894 and 46,173;["aig-mcp"], one SARIF block in the judge prompt.Repeated on the
a371960patch: both findings at error level, 172s, same results. The latera773037patch adds only the MCP destination refusal, which this sample's model never used.Review
Two independent review passes ran. The first pass's P1s, both now fixed:
Its P2s, also fixed:
dist/files hidden from the agent;predictionlabel in the judge evidence.The second pass found no P0/P1. Fixed from it:
plugin/-prefixed paths thatread_fileaccepts;-m/-u;Left as noted below:
SkillSpector findings: null);--sandbox-image <unpatched>not being detected.A re-pilot at the new head with
--profile clawhubshowed:plugin/dist/index.js, 59s;aig-mcp0 findings, 55s. The judge stage then failed, because the judge workspace copy refuses DuckDuckGo'snode_modules/@openclaw/plugin-sdksymlink. That copy still includesnode_modules, which feat!: add a Decisions API judge and a pluggable overview step for clawhub #148 removes;["aig-mcp"], with explicit-m gpt-6.1-sol -u https://api.openai.com/v1.Not covered
4 passed).clawscan install aig-mcpinstalls the unpatched PyPI package.--profile clawhubplugin run whosenode_modulescontains a symlink leaving the package (workspace links such as@openclaw/plugin-sdk) fails at the judge workspace copy. Theaig-mcpstage itself stages withoutnode_modulesand is unaffected.openclaw/clawhubscripts/security/install-clawscan-scanners.sh. It needs a follow-up that installsaig-mcp-scan0.2.0 with this patch and setsAIG_MCP_SCAN_ALLOW_HOST_EXEC=1before it moves its ClawScan pin past this change. Until then, plugin scans in that worker would fail closed.aig-mcp-scanpackage is pinned. It uses Chat Completions without the reasoning-effort or Standard-tier controls the patchedaiglane has.clawhub-aigis unchanged.