2 Commits

Author SHA1 Message Date
AochenShen99
658c39ccf7
feat(skills): Add native SkillScan phase 1 for skills (#3033)
* Add phase 1 skill static scanning

* Rework SkillScan phase 1 as native scanner

* refactor(skillscan): align phase 1 with trimmed RFC contract

- SecurityFinding: 7 fields (rule_id, severity, file, line, message,
  remediation, evidence); category/analyzer derive from the rule_id
  prefix, confidence/column/fingerprint/metadata removed
- scan_archive_preflight()/scan_skill_dir() are pure functions: no
  ScanContext, no policy schema; CRITICAL-blocks is a code constant and
  skill_scan.enabled is applied by enforce_static_scan()/callers
- secret-* evidence is redacted before findings leave the scanner
- de-dup keys on (rule_id, file, line) so repeated occurrences keep
  distinct locations for agent self-correction
- cloud-metadata detection consolidated into network-cloud-metadata
- nested zip members get a one-level stdlib magic-byte peek; an
  executable member escalates package-nested-archive to CRITICAL
- install metadata sidecar removed (Phase 7 decides if it is needed)
- rule specs moved next to their analyzers; skillscan/rules/ removed
- tests updated + new anchors: redaction, dedup lines, nested-zip
  escalation, single cloud-metadata rule, bundled-skill zero-CRITICAL

* fix(skillscan): tighten reverse-shell/secret/archive scan rules from review

Address PR #3033 review feedback on the native SkillScan analyzers:

- Reverse-shell false positives: split shell detection by signal strength
  (/dev/tcp/, nc -e stay CRITICAL; bash -i, mkfifo -> new HIGH
  shell-reverse-shell-heuristic, warn->LLM). The Python check is now
  AST-anchored on real socket.socket/os.dup2/subprocess call sites instead
  of raw-text substring matching, so prose/docstrings no longer hard-block.
- Secret evidence: _redact_secret_evidence returns [redacted] with no secret
  bytes (was value[:6], which leaked 2 real token bytes past the prefix).
- Archive DoS: cap outer archive member count (_MAX_ARCHIVE_MEMBERS=4096);
  scan_archive_preflight early-aborts with a package-too-many-members CRITICAL
  finding (routes through the existing blocked->400 fail-closed path).
- shell-destructive-command: broaden the rm -rf matcher to sensitive system
  roots (/home, /usr, /*, --no-preserve-root /) while leaving safe subpaths
  unflagged.
- Dead code: collapse _decode_text_for_analysis to a single decode path and
  drop the unused _TEXT_SUFFIXES set and _has_text_shebang helper.
- local_skill_storage: document why the host_path branch keeps app_config
  possibly-None (lazy kill-switch resolution; avoids eager get_app_config in
  config-free environments such as CI).

Tests: new negative/positive coverage in test_skillscan_native.py. Full
backend suite 6616 passed, 26 skipped.
2026-07-07 21:44:28 +08:00
AochenShen99
b8f5ed360f
fix(skills): keep skill archive installation off the event loop (#3505)
* fix(skills): keep skill archive installation off the event loop

ainstall_skill_from_archive — the async entry point awaited by the gateway
POST /skills/install route — ran its entire filesystem pipeline inline on
the event loop: zip extraction, frontmatter validation, rglob enumeration,
per-file read_text, shutil.copytree staging, and tempdir cleanup.

Restructure into offloaded phases: prepare (extract + validate) and commit
(stage + move) run via asyncio.to_thread, the tempdir lifecycle is
offloaded, and the security scanner's file enumeration and reads move off
the loop — only the per-file LLM scan (genuinely async) stays awaited.
Security decision logic and exception contract are unchanged.

Anchor: tests/blocking_io/test_skills_install.py drives the real install
pipeline (real .skill archive, real FS; only scan_skill_content stubbed)
under the strict Blockbuster gate. Verified red on pre-fix code
(BlockingError: os.stat), green with the fix.

* fix(skills): log temp-dir cleanup failures instead of swallowing them

Review follow-up on the install offload: rmtree(ignore_errors=True) kept
the primary install exception but silently leaked the extraction dir on
cleanup failure. Keep the never-mask behaviour, add a warning log.

* fix(skills): bound install tmp cleanup and pass skill_dir explicitly (review)

- Wrap the best-effort temp-dir cleanup in asyncio.wait_for (5s) so a
  hung filesystem in the finally block cannot stall or mask the install
  outcome; timeout is logged like the existing OSError path.
- Hoist _collect_scannable_files to module level with skill_dir as an
  explicit argument instead of a closure capture.
2026-06-12 15:17:40 +08:00