[Aikido] AI Fix for Using blacklisted XML parsing function is dangerous - #237
Open
aikido-autofix[bot] wants to merge 9 commits into
Open
aikido-autofix[bot] wants to merge 9 commits into
aikido-autofix[bot] wants to merge 9 commits into
Conversation
…y scans (#215) * Add no_tools_found Sentry event to diagnose empty discovery scans A scan that finds zero tools was previously silent — indistinguishable from an errored scan, a missed detection, or a genuinely empty device. Emit one fire-and-forget Sentry warning (phase=no_tools_found) carrying the discriminators needed to tell them apart: - homes_enumerated: pre-fallback enumerated-user count (0 = enumeration miss; the existing post-fallback user_count masks 0 as 1) - users_scanned, used_fallback_user, is_root, os, duration_ms Counts/booleans only (no PII); reuses the existing curl-based report_to_sentry; wrapped so telemetry can never raise into or slow the scan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Let no_tools_found bypass the per-run Sentry cap (Greptile P2) A noisy empty scan (many earlier detector errors) could exhaust the shared 30-event per-run cap before the terminal no_tools_found summary runs, dropping exactly the diagnostic you most need on a likely missed-detection. Add a `priority` flag to report_to_sentry that bypasses the COUNT cap only — it still honors the circuit breaker (a dead transport can't be helped) and dedup (no spam) — and pass priority=True for the no_tools_found event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Let priority Sentry events bypass the circuit breaker too (Greptile P1) The priority flag bypassed the count cap but the circuit breaker — tripped by 3 consecutive, possibly transient, send failures — could still skip the terminal no_tools_found event without even an attempt. Allow a priority event ONE bounded attempt past the breaker (at most one ~4s curl at end of run) so a transient mid-scan outage doesn't drop the terminal diagnostic. Dedup is still honored, so it can never spam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…216) The discovery CI suite runs the real script against a loopback mock gateway on clean GitHub-hosted runners, so every scan finds zero tools and the previously log-only Sentry paths self-report to the production project: no_tools_found / send-failure / timeout / signal events with zero customer impact (DISCOVERY-TOOL-SCRIPT-17 / -13 / -12 / -D; 346 events observed in 24h). Add a before_send-style guard at the top of report_to_sentry() that drops an event carrying a CI fingerprint, using three signals, each ~0-false-positive on customer machines: - a CI environment marker (GITHUB_ACTIONS / CI) -- ground truth, and the only one that also covers the bare-context extract/detect emits; - the report target (domain) is a loopback host (127.0.0.0/8 dotted quad / localhost / ::1 / 0.0.0.0) -- a real install always points at the customer gateway URL, never loopback; - the OS account (system_user) is a GitHub-hosted-runner account (runner / runneradmin). The guard never raises and defaults to keeping the event, so a malformed context can never suppress a genuine customer error. Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
release-29-06-29-v1
Promote is_root, used_fallback_user, homes_enumerated and users_scanned
from the event's unindexed `extra` into _SENTRY_TAG_KEYS so the
DISCOVERY-TOOL-SCRIPT-17 ("Discovery found no tools") population becomes
queryable/aggregatable in Sentry: root vs non-root scans, and
enumeration-miss (homes_enumerated==0) vs genuinely-empty devices.
All four are low-cardinality (2 bools + small ints). duration_ms is
deliberately left in `extra` (high cardinality). The tag-builder's
`if k in ctx` guard keeps this safe where a key is absent (is_root is
POSIX-only). No behavior change; observability-only; forward-only tagging.
Co-authored-by: audit <audit@local>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* WEB-4774: Windows lock liveness + auto-resume for interrupted discovery (#223) * WEB-4774: Windows lock liveness + auto-resume for interrupted discovery B1-Win: cross-platform process-liveness for the discovery lock. _pid_alive_windows (OpenProcess) + an _owner_alive dispatcher so a lock left by a SIGKILLed Windows scan is stolen immediately instead of waiting out the 15-min stale window (posix liveness was already handled). Resume: a lightweight run checkpoint (run_id/status/updated_at/done) in the discovery cache lets a re-run within RESUME_WINDOW_SECONDS (150s) of an interrupted run skip re-PROCESSING already-reported tools. Fresh run_id stays data-safe because the backend replaces installations per (device, tool, home_user), never per scan; detection still runs so newly-installed tools/skills are never missed. Tests: 8 Windows-probe + 9 resume-checkpoint unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4774: fold cache writes, add resume integration + Windows smoke tests - record_report(): single atomic cache write that persists the payload hash (on an actual upload) and appends the run-checkpoint done entry, replacing the update_tool + mark_run_uploaded pair on main()'s hot per-report path. Shared mutators _set_tool_hash / _record_run_done; update_tool + mark_run_uploaded retained as thin wrappers. - TestResumeSkipsReprocessingInMain: drives main() in-process (detector/backend mocked, real temp cache) and asserts an already-reported tool is not re-processed, a pending tool is, and the checkpoint flips to completed. - test_windows_probe_real_process_smoke: exercises the real OpenProcess path on Windows (skipUnless nt); +2 record_report unit tests. Full flow suite: 90 passed, 1 skipped (Windows smoke, skipped off-Windows). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4774: trap SIGHUP and the other catchable termination signals main() now installs the abort handler (release lock + report failed) for every catchable signal whose default action would terminate discovery on an external request: SIGTERM, SIGINT, SIGHUP, SIGQUIT, SIGBREAK (Windows), SIGXCPU, SIGXFSZ, SIGPWR (Linux) — each getattr-guarded so platform-absent names are skipped. Deliberately not trapped (documented inline): SIGKILL/SIGSTOP (uncatchable); SIGPIPE (Python -> BrokenPipeError, not a kill); SIGTSTP/SIGTTIN/SIGTTOU (suspend, not terminate); and the program-error/core signals (SIGSEGV/SIGBUS/ SIGABRT/SIGFPE/SIGILL/SIGSYS/SIGTRAP) — cleanup from a corrupted process is unsafe and genuine crashes are already reported via the except-Exception path. Broadened the signal test to assert the abort handler is installed for the whole set (incl. SIGHUP) and restored in finally. Full flow suite: 90 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4774: address bot review findings - HIGH (Greptile P1 / Cursor): _pid_alive_windows read GetLastError from a WinDLL without use_last_error, so ctypes/GC bookkeeping between OpenProcess and the read could clobber the code and misclassify a PID (dead->alive, or worse live->dead -> steal a running scan's lock). Switch to ctypes.WinDLL('kernel32', use_last_error=True) + ctypes.get_last_error(); regression test asserts the verdict uses the saved error, not a late kernel32.GetLastError(). - P2 (Greptile): resume now logs the interrupted run_id alongside the new one (cross-run correlation), and logs each per-user resume skip + records resumed users in the per-tool summary (no more silently-reduced user counts). - TRIAGE (consensus) + Medium (Cursor): cache written explicitly 0600 (cross-user forge blocked, incl. root MDM scans); resumable_done() fail-safes to a full scan on any malformed checkpoint incl. a non-UUID run_id. Same-user forging stays out of scope (bounded to one 150s window; next non-resumed scan re-reports; the pre-existing hash cache is equally forgeable) — documented inline. Full suite: 92 passed, 1 skipped. E2E kill/resume re-verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4774: add resume + lock-steal observability to discovery metrics Follow-up from review: the kill/rerun behavior had no production metrics, so a misfiring checkpoint (stale done-set, wrong skips) or the lock-steal path would be invisible in dashboards. Extend sentry_metrics_payload.metadata with: - resumed : did this run resume an interrupted one - resume_pairs_skipped : (tool,user) pairs skipped - resume_tools_skipped : tools whose processing was fully skipped - lock_outcome : acquired | stolen_dead_pid | stolen_stale (new cache.last_lock_outcome, so the POSIX/Windows dead-PID steal is visible) Tests: last_lock_outcome records dead-PID steal vs clean acquire; the resume integration test now asserts the metrics payload carries the resume fields. Full suite: 94 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Sync staging and main (#228) * [WEB-4956] Add no_tools_found Sentry event to diagnose empty discovery scans (#215) * Add no_tools_found Sentry event to diagnose empty discovery scans A scan that finds zero tools was previously silent — indistinguishable from an errored scan, a missed detection, or a genuinely empty device. Emit one fire-and-forget Sentry warning (phase=no_tools_found) carrying the discriminators needed to tell them apart: - homes_enumerated: pre-fallback enumerated-user count (0 = enumeration miss; the existing post-fallback user_count masks 0 as 1) - users_scanned, used_fallback_user, is_root, os, duration_ms Counts/booleans only (no PII); reuses the existing curl-based report_to_sentry; wrapped so telemetry can never raise into or slow the scan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Let no_tools_found bypass the per-run Sentry cap (Greptile P2) A noisy empty scan (many earlier detector errors) could exhaust the shared 30-event per-run cap before the terminal no_tools_found summary runs, dropping exactly the diagnostic you most need on a likely missed-detection. Add a `priority` flag to report_to_sentry that bypasses the COUNT cap only — it still honors the circuit breaker (a dead transport can't be helped) and dedup (no spam) — and pass priority=True for the no_tools_found event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Let priority Sentry events bypass the circuit breaker too (Greptile P1) The priority flag bypassed the count cap but the circuit breaker — tripped by 3 consecutive, possibly transient, send failures — could still skip the terminal no_tools_found event without even an attempt. Allow a priority event ONE bounded attempt past the breaker (at most one ~4s curl at end of run) so a transient mid-scan outage doesn't drop the terminal diagnostic. Dedup is still honored, so it can never spam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop CI/local-run noise from production Sentry in report_to_sentry() (#216) The discovery CI suite runs the real script against a loopback mock gateway on clean GitHub-hosted runners, so every scan finds zero tools and the previously log-only Sentry paths self-report to the production project: no_tools_found / send-failure / timeout / signal events with zero customer impact (DISCOVERY-TOOL-SCRIPT-17 / -13 / -12 / -D; 346 events observed in 24h). Add a before_send-style guard at the top of report_to_sentry() that drops an event carrying a CI fingerprint, using three signals, each ~0-false-positive on customer machines: - a CI environment marker (GITHUB_ACTIONS / CI) -- ground truth, and the only one that also covers the bare-context extract/detect emits; - the report target (domain) is a loopback host (127.0.0.0/8 dotted quad / localhost / ::1 / 0.0.0.0) -- a real install always points at the customer gateway URL, never loopback; - the OS account (system_user) is a GitHub-hosted-runner account (runner / runneradmin). The guard never raises and defaults to keeping the event, so a malformed context can never suppress a genuine customer error. Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * observability: index no_tools_found discriminators as Sentry tags (#222) Promote is_root, used_fallback_user, homes_enumerated and users_scanned from the event's unindexed `extra` into _SENTRY_TAG_KEYS so the DISCOVERY-TOOL-SCRIPT-17 ("Discovery found no tools") population becomes queryable/aggregatable in Sentry: root vs non-root scans, and enumeration-miss (homes_enumerated==0) vs genuinely-empty devices. All four are low-cardinality (2 bools + small ints). duration_ms is deliberately left in `extra` (high cardinality). The tag-builder's `if k in ctx` guard keeps this safe where a key is absent (is_root is POSIX-only). No behavior change; observability-only; forward-only tagging. Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: pugazhendhi-m <132246623+pugazhendhi-m@users.noreply.github.com> Co-authored-by: audit <audit@local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Vishnu <79318686+zeus-12@users.noreply.github.com> * WEB-4575: Add Agent Skills extraction for Codex, Gemini CLI, Junie, Kilo Code, OpenCode, Roo Code, Windsurf, Replit (#224) * WEB-4575: Agent Skills extraction — foundation + Codex + 8-tool extractors (WIP) Shared: _extract_and_merge_tool_skills + skills args on _process_tool_with_rules_and_mcp; 8 Base<Tool>SkillsExtractor ABCs. Codex fully wired + tested (16 tests). Helpers + macOS/Windows/Linux extractors for all 8 tools (Codex, Gemini CLI, OpenCode, Junie, Kilo, Replit, Roo mode-dirs, Windsurf nested-user). Central wiring for the other 7 tools + per-tool tests still pending. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: wire 7 remaining tools + per-tool skills tests Central wiring (factory + linux package exports + AIToolsDetector) for Gemini CLI, Junie, Kilo Code, OpenCode, Roo Code, Windsurf, Replit. Replit gets a new skills-only branch (no rules/MCP). Per-tool test files (105 skills tests total, all passing); special-case coverage: OpenCode/Windsurf no-double-count, Roo skills-{mode} discovery, Replit project-scope-only no-op user extraction. Full suite: 1539 passed, 1 skipped; the 13 failures are pre-existing/environmental (real /opt/homebrew/bin/copilot breaks Copilot-CLI binary-gate tests; real ~/.unbound breaks main()/lock tests) and reproduce without this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: correct Windsurf skill dirs to match official docs Re-verified all 8 tools' skill paths against vendor docs. 7/8 were exact; Windsurf over-collected .github/.cursor/.codex (an earlier-research error — docs.devin.ai lists ONLY .windsurf/.agents/.claude as Cascade skill dirs) and missed ~/.claude at user scope. Now: project .windsurf/.agents/.claude; user ~/.codeium/windsurf + ~/.agents + ~/.claude. Docstring + tests updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: guard skills walks against other-tool config dirs Adds traverses_other_tool_config_dir(item, allow=SHARED_SKILL_DIRS | own parents) to all 24 skills extractor walks, matching the Copilot CLI extractor. Prevents a tool's project walk from descending into ANOTHER tool's ~/.<tool>/ config/extension dir and over-collecting its bundled .agents/.claude skills, while never skipping the tool's OWN parent dirs (the per-tool allow set). Adds test_skills_overcollection.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: complete OTHER_TOOL_CONFIG_DIRS (e2e fix) On-machine e2e surfaced a real over-collection: a plugin skill at ~/.codex/.tmp/plugins/.agents/skills/plugin-creator was attributed to 7 tools, because OTHER_TOOL_CONFIG_DIRS (the walk's other-tool skip list) was missing the newer tools' config dirs. Added .codex/.kilo/.opencode/.copilot; each tool still collects its own dir via the per-tool allow set. .augment intentionally omitted (its extractor doesn't allow its own dir yet — would self-skip). Adds a regression test reproducing the exact e2e case. Full suite 1542 passed; 13 failures pre-existing/environmental (real copilot binary, real ~/.unbound). Verified on-machine: the ~/.codex leak is gone; legit ~/.claude user + project skills still collected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: remove WEB-4774 checkpoint-resume code that leaked into main() Greptile flagged that main() called discovery_cache.resumable_done()/start_run()/ mark_run_uploaded()/mark_run_completed() — none of which exist on staging (they're uncommitted WEB-4774 WIP). They rode in via 'git add -A' during the initial branch split, while the matching cache.py methods were correctly excluded — so every scan would AttributeError at startup before any tool is processed. Restores main() to origin/staging verbatim (the checkpoint code is 100% confined to main(); all WEB-4575 skills code lives in the class above it, untouched). This also fixes 4 test_discovery_flow main tests that were failing due to the contamination (previously mis-reported as environmental). Full suite: 1547 passed; the 9 remaining failures are all test_copilot_cli binary-gate/discovery (real /opt/homebrew/bin/copilot on the dev box). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: don't follow symlinked dirs/files in skills walks (security) Addresses the HIGH consensus/Cursor security finding: skills walks followed symlinked directories before (macOS/Linux) or without (Windows) a guard, letting a project-planted symlink (e.g. .agents -> /Users/victim) redirect a privileged/ all-user scan into another user's tree and mis-attribute their skills. - All 24 walkers: hoist 'if item.is_symlink(): continue' above the parent-dir branch (was after it on macOS/Linux; absent on Windows), + guard the type_dir ('and not type_dir.is_symlink()'). - Generic engine (claude_code_skills_helpers): skip symlinked skill-name dirs and symlinked SKILL.md marker files in both project- and user-level extraction — closes the residual file-symlink vector for ALL tools at the root. (Size is already bounded by read_file_content/MAX_CONFIG_FILE_SIZE, so the huge-file DoS is a non-issue.) - Roo helper iter_roo_skill_type_dirs also rejects symlinked mode-dirs. - New test_skills_symlink_security.py covers all four vectors (parent dir, type dir, skill-name dir, SKILL.md file) + legit-still-collected. The WEB-4774 checkpoint findings from earlier review rounds were already resolved by ce4548d. Full suite 1551 passed; 9 failures are pre-existing environmental (real /opt/homebrew/bin/copilot). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: address Greptile review (docstring + Replit user-enum) - coding_tool_base: fix stale BaseWindsurfSkillsExtractor docstring — it still listed .github/.cursor/.codex, which the impl correctly dropped (docs.devin.ai lists only .windsurf/.agents/.claude). - Replit macOS/Windows/Linux extractors: Replit is project-scope only (REPLIT_USER_DIR_NAMES == ()), so _extract_user_level_skills enumerated every user just to run a no-op. Make it an explicit no-op and drop the now-unused user-enumeration imports (is_running_as_root/scan_user_directories/ scan_windows_user_directories/extract_replit_user_level_items). Per-flow skills metrics (also suggested) deferred: emitting a new field in the discovery-metrics payload requires a coordinated backend change in discovery_metrics_service to ingest it; a client-only key would be ignored or, under strict validation, break the best-effort metrics send. Tracking separately. Full suite 1551 passed; 9 failures pre-existing/environmental (real copilot binary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: fix stale Windsurf module docstrings (cosmetic) macos/windows windsurf skills_extractor module docstrings still listed .github/.cursor/.codex as project compat dirs; the implementation only consults .windsurf/.agents/.claude (docs.devin.ai). Docstring-only; no runtime change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: detect Windows junctions + add TOCTOU containment (security) Addresses the two findings in the consensus review at head 2baaa21. HIGH — Windows junction/reparse points bypassed the is_symlink() guards. Path.is_symlink() returns False for an NTFS directory junction (IO_REPARSE_TAG_MOUNT_POINT), which any user can create via 'mklink /J' with no admin rights, and Path.is_junction() is 3.12+ while this project supports 3.9+ (CI matrix 3.9/3.11/3.12). So a planted junction sailed through the 1e200e1 hardening. Adds constants.is_symlink_or_junction(): a single lstat (not is_symlink()+lstat — this runs per directory entry of a whole-disk walk) that checks S_ISLNK and the Windows reparse tag. Only MOUNT_POINT/SYMLINK tags count as redirects, so OneDrive cloud placeholders stay traversable (otherwise a redirected Documents folder would silently stop being scanned). Conservative on lstat error. Wired into all 24 walkers (item + type_dir), the shared engine, and the Roo mode-dir helper; no raw .is_symlink() remains in those paths. TRIAGE — TOCTOU between the link check and the read. The engine now resolves the skills dir once and requires every marker file to resolve inside it, rejecting anything that escaped the boundary. This shrinks the race window and is independent of the link guard (proved by a test that disables the guard). Fully closing it would need fd-based no-follow traversal (openat/O_NOFOLLOW), which pathlib does not expose portably — documented in the helper. Tests: test_symlink_junction_guard.py (9, incl. junction-vs-cloud-placeholder and conservative-on-error) + containment test in test_skills_symlink_security.py. Full suite 1561 passed; the 9 failures are pre-existing/environmental (real /opt/homebrew/bin/copilot). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: add per-flow skills metrics (addresses Greptile observability gap) Each of the 8 SKILL.md extraction flows now records a counter, forwarded once at end of run in the discovery-metrics payload as a new 'skills' section: {tool, status, user_skills, project_skills, projects} Why: a silently-failing extractor (vendor path change, permission regression) raises no exception and fires no Sentry event — it only logs 'No skills found', which is indistinguishable from a machine that genuinely has none. ZERO counts are therefore recorded deliberately, so the backend can emit a per-tool discovery.skills.<tool> counter and a fleet-wide drop becomes alertable. 'status' distinguishes ok / unsupported_os / error. Safety (why this is a safe client-only change, revising the earlier deferral): send_discovery_metrics is a SEPARATE fire-and-forget POST carrying tools=[], with a short timeout and no retries, and the whole block is wrapped in try/except — so a backend that ignores or rejects the new key cannot affect tool reports or the scan. Backend ingestion of the new section remains a follow-up; until then the field is simply ignored. Detail: tool names are sanitised to Sentry's metric-key pattern [a-zA-Z_][a-zA-Z0-9_.\-]* ('Gemini CLI' -> 'gemini_cli'), since spaces would otherwise produce invalid metric names. _record_skills_metric never raises — telemetry bookkeeping must not break a scan. Tests: test_skills_metrics.py (10) covering zero-recording, counts, unsupported_os, error status, name sanitisation, and never-raises. Full suite 1571 passed; the 9 failures are pre-existing/environmental (real /opt/homebrew/bin/copilot). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: fix Codex user skills path (real e2e found we missed 100% of them) Found by running a real e2e against an installed Codex (0.139.0), using the CLI itself as the oracle: 'codex app-server' exposes skills/list, which reports every discovered skill with a scope and absolute path. THE BUG: Codex user skills live at $CODEX_HOME/skills (default ~/.codex/skills), not ~/.agents/skills. Codex's own bundled skill-creator says it writes there 'so Codex can discover it automatically', skill-installer installs to $CODEX_HOME/skills/<name>, and the binary resolves os.path.join(_codex_home(), 'skills'). The docs page only documents .agents/skills. We read only ~/.agents/skills AND had .codex in the walk's skip list, so EVERY user-installed Codex skill was silently missed. Proven: a probe skill that skills/list reported as scope=user was extracted as nothing by us. THE FIX: two-parent-set (same technique as OpenCode/Windsurf) so both properties hold at once: - CODEX_USER_DIR_NAMES = ('.codex', '.agents') -> user scope reads ~/.codex/skills directly (user extraction bypasses the walk), resolving back to the home via CODEX_USER_PARENT_DIR_NAMES. - CODEX_PARENT_DIR_NAMES stays ('.agents',) -> the project walk still never descends into ~/.codex, so vendor content is NOT over-collected: OpenAI built-ins (~/.codex/skills/.system/) and marketplace plugin skills (~/.codex/plugins/cache/) remain excluded. Verified on the live machine after the fix: our extractor now matches skills/list exactly for both the user-scope and repo-scope probe skills, with .system built-ins correctly excluded. Probes removed afterwards. Tests: regression for ~/.codex/skills user extraction + .system exclusion + project-walk-excludes-.codex. Full suite 1575 passed; the 9 failures are pre-existing/environmental (real /opt/homebrew/bin/copilot). NOTE: the same docs-vs-reality risk applies to the other 7 tools, whose paths were derived from vendor docs. Only Codex is installed on this box; oracle-validating Gemini CLI / OpenCode requires installing them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: fix Kilo/Windsurf/Junie skill paths (real e2e; docs were wrong for 3 more tools) Installed each remaining tool and used it as its OWN oracle (its skill-list CLI, shipped extension source, app predicate, or product-bundled docs) — not vendor docs. Found and fixed 3 more under-collection bugs (after Codex): KILO CODE (oracle: 'kilo debug skill' — Kilo's runtime is an OpenCode fork): discovers far more than its docs claim. Added user dirs .kilocode (legacy) and the nested ~/.config/kilo (OpenCode-style, two-parent-set so it resolves to home), plus project .kilocode, and a SINGULAR 'skill/' dir variant alongside 'skills/'. Was: .kilo + .agents only -> missed .kilocode, .config/kilo, and skill/. WINDSURF (oracle: Devin.app's own SKILL.md predicate): after the Windsurf->Devin rebrand the data folder is '.devin' (dataFolderName:'.devin', oldDataFolderName:'.windsurf'); the app reads BOTH. Added .devin to project dirs and to OTHER_TOOL_CONFIG_DIRS. User dirs (~/.codeium/windsurf, ~/.agents, ~/.claude-gated) already matched the app's global loader. JUNIE (oracle: junie-cli-user-disk-storage.md, shipped inside junie-release-*.jar): the CLI reads .junie/agent-skills while the IDE plugin uses .junie/skills. We read only 'skills' -> missed 100% of Junie CLI skills. Now reads BOTH dir names at user and project scope. Also confirmed CORRECT via oracle (no change): Gemini CLI (~/.gemini/skills + .agents, via 'gemini skills'), OpenCode (5/5 dirs via 'opencode debug skill'), Roo Code (.roo/.agents + skills-{mode}, no .claude — from shipped ext source), Replit (cloud-only; project .agents/skills is the sole local path). Tests: test_skills_paths_e2e_verified.py pins each corrected path; stale constant-assertions updated to the verified values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: guard user-level + top-level dir traversal against symlinks/junctions Addresses the 2 findings in the consensus review at head 854dee9. HIGH — user-level extraction followed symlinked/junctioned skills dirs. extract_user_level_items opens ~/<tool>/skills DIRECTLY (not via the guarded project walk), so a user could plant ~/.agents/skills, ~/.codex/skills, ~/.roo, etc. as a symlink/junction into another user's tree; _is_contained didn't help because its root was the resolved link TARGET. Now each user-level skills dir is rejected if it is itself a symlink/junction OR if it (or any ancestor, e.g. a symlinked ~/.config) resolves outside the scanned user's home. Same guard added to extract_roo_user_level_items' base dir. TRIAGE — top-level dir enumeration skipped the junction guard. Added 'if is_symlink_or_junction(current_dir): return' at the top of all 24 _walk_for_skills (covers the walk entry on every OS: macOS '/', Windows drive root, Linux home), plus 'and not is_symlink_or_junction(item)' to the Windows top_level_dirs comprehension so a drive-root junction is never even enqueued. Tests: user-level symlinked-dir + symlinked-ancestor, and top-level symlinked-dir, added to test_skills_symlink_security.py (20 security tests total). Full suite 1583 passed; the failures are all pre-existing/environmental (real copilot + junie binaries installed on the dev box for the e2e validation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: metrics record error on broken extractor; guard hard-linked SKILL.md Two review findings (heads cd7fe2b / cd7fe2b): 1) CORRECTNESS — a broken extractor was recorded as status=ok/0, not error. The extract_all_<tool>_skills wrappers catch their own exceptions and return None (they do NOT re-raise), so a crash never reached _extract_and_merge_tool_skills's except block — it saw a falsy result and logged status=ok with zero counts, defeating the fleet failure alert. Since the extractor is already confirmed present by the guard above, a None result now unambiguously means the extractor raised, so it's recorded as status=error. Fixed test_skills_metrics to exercise the REAL production path (through the wrapper) instead of an unreachable direct-raise path, and removed the now-wrong 'None -> zero' assertion. 2) SECURITY (triage) — hard-linked SKILL.md bypassed the link/containment guards. A hard link to another user's file passes is_file()/is_symlink_or_junction()/ _is_contained(), so a root/all-user scan could read + upload (<=50 KB of) another user's content. Added _is_foreign_hardlink(): rejects a multiply-linked file whose owner differs from its parent dir's owner (cross-user), while still collecting a user's own hard-linked skill; on Windows (st_uid unreliable) any nlink>1 is rejected. Wired into all 4 file-read guards in the shared engine (project + user scope, nested + flat). Added hard-link regression tests. Full suite 1586 passed; the failures are pre-existing/environmental (real copilot + junie binaries installed on the dev box). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: never attribute user skills to the scanner's home (drop + count) Review finding (head b47f02c): a user skill missing project_path fell back to Path.home() — which is the SCANNING PROCESS's home (e.g. /root) under a privileged/all-user scan — so another user's SKILL.md content could be mis-filed into the wrong user's report/filter context. project_path is the owning home derived from the skill file at extraction time and must always be present. If it is somehow absent, the skill is now DROPPED (and the count surfaced via a new dropped_no_home metric field) rather than attributed to Path.home(). No user skill is ever bucketed under the scanner's home. Test added: an orphan user skill (no project_path) is dropped, not placed under Path.home(), and dropped_no_home is recorded. Full suite 1587 passed; the failures are pre-existing/environmental (real copilot + junie binaries on the dev box). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: wire legacy skills tools into per-flow metrics (close blind spot) Greptile 5/5 non-blocking observation: the pre-existing skills tools (Claude Code, Cursor, Cline, Augment, Copilot CLI, Copilot VS Code, Cowork) merge skills inline and were not recorded in the per-flow skills metrics, so once the backend consumes the payload those tools would be a blind spot for fleet-wide drop alerting. Added _record_skills_result_metric(tool_name, skills_result) and called it at all 7 legacy inline skills sites, emitting the same {user_skills, project_skills, projects} shape as the 8 newer tools. Inline extractors return None on failure -> recorded as zero (a crash still surfaces as a fleet-wide drop; only the richer status=error signal, available for the newer tools, is absent here). Purely additive: the recorder is a no-op on the scan result and never raises (same guarantee as _record_skills_metric); no existing extraction/merge behavior changed. Full suite 1589 passed; the failures are pre-existing/environmental (real copilot + junie binaries on the dev box). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4575: complete Linux Windsurf extractor docstring (parity w/ macOS/Windows) Non-blocking 5/5-review nit: the Linux Windsurf skills_extractor carried a terse one-line module docstring while its macOS/Windows siblings document the full skill path map. Brought it to parity. Docstring-only; no runtime change. * WEB-4575: sync with staging (WEB-4774) + stop skills block from aborting metrics send CI was red on every matrix job with a single failure: test_discovery_flow.TestResumeSkipsReprocessingInMain .test_done_tool_is_skipped_pending_tool_is_processed AssertionError: discovery metrics were not sent Two causes: 1. Stale branch. WEB-4774 (auto-resume, #223) has since merged to staging, bringing the cache resume APIs, the resume logic in main(), and this test. GitHub runs PR checks against head-merged-with-base, so CI saw the new test while the branch did not. Merged origin/staging in (clean, no conflicts); the merged metrics payload correctly carries both #223's resume metadata (resumed / resume_pairs_skipped / resume_tools_skipped / lock_outcome) and this PR's per-tool skills array. 2. A real fragility bug in this PR, which the merge exposed. The skills block ran `if detector.skills_metrics:` then `sorted(detector.skills_metrics.items())` inside the best-effort metrics try/except. The resume test uses a bare Mock() detector, so skills_metrics is a truthy Mock, sorted() raises TypeError, the except swallows it -- and send_discovery_metrics is never called at all. A real detector always holds a dict so production never hit this, but a non-dict must not be able to abort the ENTIRE metrics send. Guarded with isinstance, fixing the root cause for every mock-detector main() test rather than point-patching the one test. Full suite: 1613 passed. The 10 failures are the known environmental ones (real copilot + junie binaries installed on this dev box during the e2e sweep break the "not detected when nothing present" assertions); CI's clean runners show none of them -- CI's only failure was the resume test above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WEB-4575: mark the cross-user hardlink tests POSIX-only (fixes Windows CI) With macOS green, the Windows jobs ran to completion for the first time (the matrix is fail-fast, so earlier macOS failures had been cancelling Windows mid-run and masking this) and surfaced 2 real failures, both in this PR's tests: FAIL TestForeignHardlinkGuard.test_hardlink_owned_by_other_user_rejected AssertionError: Lists differ: [] != ['legit'] FAIL TestForeignHardlinkGuard.test_own_hardlink_allowed AssertionError: True is not false Both are POSIX-semantics tests, not product bugs: - test_own_hardlink_allowed asserts a user's OWN hard link is still collected. That is uid-comparison behaviour. On Windows st_uid is always 0, so _is_foreign_hardlink deliberately (and per its docstring) rejects ANY st_nlink > 1 rather than compare owners -- so "allowed" is false there by design. - test_hardlink_owned_by_other_user_rejected force-patches os.name='posix' to exercise the uid path, and fakes os.lstat with a hand-rolled stat object carrying only st_nlink/st_uid/st_mode. On Windows is_symlink_or_junction also reads st_file_attributes/st_reparse_tag, which that fake lacks, so the walk collected nothing -- hence [] instead of ['legit']. Production is correct on Windows either way: a normal SKILL.md has st_nlink == 1 -> not foreign -> collected; only nlink > 1 is conservatively rejected. So the guard is not over-rejecting real files. Skipped the class on nt with an explicit reason, matching existing precedent in this suite (Windows already skips ~51 POSIX-semantics tests). On POSIX all 10 tests in the file still run and pass -- the coverage is unchanged where it means something. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WEB-4575: legacy skills flows record status=error on failure (close last review gap) Greptile 4/5's one live finding: the retroactively-instrumented legacy flows (Claude Code, Cursor, Cline, Augment, Copilot CLI, Copilot VS Code, Cowork) recorded status=ok even when the extractor failed, weakening per-tool failure alerting for exactly those flows. The earlier justification ("None is indistinguishable from a genuine zero for the inline extractors") does not hold up. Every legacy call site is inside `if self._<tool>_skills_extractor:`, so unsupported-OS is already excluded, and extract_all_<tool>_skills returns None ONLY when extraction raised -- a genuine "no skills" returns a dict with empty lists. So None there means the extractor FAILED, and recording it as a legitimate zero hides a broken extractor behind a plausible-looking count. _record_skills_result_metric now records status=error on None, exactly as the 8 newer tools do via _extract_and_merge_tool_skills; a real empty result still records status=ok with zero counts, so an empty machine never looks broken. Tests cover both directions. Full suite: 1614 passed (the 10 failures are the known environmental ones -- real copilot + junie binaries on this dev box; CI's clean runners show none). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WEB-4575: drop dead extract_<tool>_item_info wrappers from the 7 new helpers Greptile 5/5 non-blocking observation (dead code wrappers in replit_skills_helpers.py). It is not just Replit: extract_<tool>_item_info is defined and never called in ALL 7 new helper modules (codex, gemini_cli, junie, kilocode, opencode, windsurf, replit). The shape was copied from cline_skills_helpers, where extract_cline_item_info IS used (10 refs) -- but the new OS extractors reach the generic engine via extract_<tool>_items_from_directory instead, so the item_info delegation never had a caller. Removed all 7 (root cause, not just the flagged instance) plus the now-orphaned extract_item_info import in each. The pre-existing cursor/augment/copilot_cli modules keep theirs -- those are genuinely used and are untouched. Pure dead-code removal: no call sites, no test references, zero runtime impact. All 8 helper modules + the factory still import cleanly. Full suite: 1614 passed (unchanged; the 10 failures are the known environmental real-copilot/junie ones). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4679: declare a scan manifest so the backend can prune uninstalled tools (#187) * WEB-4679: declare a scan manifest so the backend can prune uninstalled tools The discovery agent never told the backend which tools were still installed, so an uninstalled tool's row persisted on the dashboard forever. The completed scan event now carries a manifest of the (home_user, tool_name) pairs successfully scanned this run plus covered_home_users, letting the backend reconcile by set-difference and soft-delete what's gone. - ai_tools_discovery.py: accumulate scanned_manifest on the send-success and dedup hash-match paths only (a tool that errored on read is left out so it is never mistaken for uninstalled); pass manifest + covered_home_users (= all enumerated OS users, so a user who removed their last tool is still in scope) to the completed send_scan_event. - utils.py: send_scan_event gains optional manifest / covered_home_users, inserted into the payload only when present (backward compatible). Stdlib-only; the accumulation is pure in-memory and cannot raise. Pairs with the ai-gateway-data WEB-4679 reconcile change (forward/backward compatible: an old backend simply ignores the new fields). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4679: dedupe scan manifest via a set of (home_user, tool_name) Addresses Greptile P2: accumulate the manifest as a set of tuples (a pair can never be double-recorded) and serialize to a sorted list of dicts at the send site. Functionally equivalent today (the success and hash-match branches are mutually exclusive, one entry per (tool, user)), but removes the latent duplicate risk and makes the output deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4679: keep read-success tools in the manifest on upload failure; tighten comments Greptile flag: a tool whose READ succeeded but whose upload failed transiently was dropped from the manifest, so the backend could mistake a network blip for an uninstall and prune a live tool (enforce mode). Record it in the send-failure branch too — the manifest tracks what was SEEN, not what uploaded. Adds a regression test (read-success + upload-failure stays in the manifest). Also condense the WEB-4679 comments to concise one-liners. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4679: fail closed — manifest=None when a read error may omit a live tool A tool whose read errors without a scan_event=failed (the generic per-user except, the per-tool except, or a PermissionError whose failed-event send itself fails) leaves the manifest possibly missing an installed tool — the backend would then set-diff it as uninstalled and wrongly prune it. Track scanned_manifest_complete and send manifest=None (backend treats as legacy = no prune) when the run wasn't fully read, deferring cleanup to a clean run. Adds a test forcing a generic (non-Permission) read error -> manifest=None. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WEB-4679: build scan manifest from detection/presence, not extraction success A read/extraction error no longer drops a live tool from the manifest (presence is recorded before extraction) and never fail-closes the whole manifest to None — so one tool's failure can't block pruning every other tool on the device. A detector that errors is kept in the manifest (presence unknown, not an uninstall). Removes the global scanned_manifest_complete fail-close. Tests rewritten to the new contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web-4679): per-user manifest + emission gate; mark scan unclean on detector error Addresses two Greptile P1s on the scan-manifest: - Phantom ownership: the manifest was added for every (user x globally-deduped tool), so a user-scoped tool one user has was attributed to co-resident users who never detected it -> backend could never prune their stale rows. Build the manifest from per-user detection, and only emit a report for a (user, tool) the user actually detected (gate on manifest membership). Add the missing Copilot CLI ownership discard (only Augment had it). - Umbrella names: a detector failure recorded detector.tool_name (e.g. 'GitHub Copilot'), not the concrete row names ('GitHub Copilot (VS Code)'), so the set-diff would prune the real surface rows. Instead mark the scan unclean (a 'failed' event before 'completed') so the backend skips pruning. Also add manifest_size + scan_incomplete to discovery metrics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(web-4679): lock JetBrains prune-key naming determinism Regression guard for the discovery prune key: the JetBrains tool name must stay the bare display_name (version + license/plan kept in separate fields), so a version bump or Free<->Licensed change never orphans the install row against the manifest. Fails CI if a future change re-embeds version/plan into the name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web-4679): per-user extension scoping + atomic incomplete-scan signal Addresses the re-review on the prior commit: - Phantom ownership (Greptile P1): Roo/Cline/Kilo detect() now scope to the per-user home set by detect_tool_for_user, so a root multi-user scan no longer self-enumerates /Users and attributes every user's extension to the outer-loop user. - Incomplete-scan false-delete (Cursor + Greptile P1): an incomplete scan now sends NO manifest on the completed event itself (atomic), instead of a separate 'failed' event whose loss could leave the backend pruning a partial inventory. Removed the fragile separate event. - Trimmed verbose comments; folded the standalone JetBrains determinism test into test_scan_completed_manifest.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(web-4679): trim verbose docstrings/comments in manifest tests Comment-only: concise module/class/test docstrings; fix stale references; no test logic change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web-4679): scope Linux/Windows extension detectors per-user; omit covered scope on incomplete scan Greptile re-review residuals (both P1, on the latest head): - Phantom ownership: the per-user scoping fix was macOS-only. Linux + Windows Cline/Kilo/Roo detect() now also scope to the per-user home set by detect_tool_for_user, so an elevated scan can't attribute one user's extension to another. - Incomplete scope: on a detector failure the completed event now omits covered_home_users too (not just manifest), so the backend never sees a prune scope without an inventory. Tests: extension-detector-scopes-to-user-home + detector-error-omits-covered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: trim comments, drop ticket references from code comments Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: raise CLI e2e subprocess timeout 600->1800s (real-machine scans exceed 10min on loaded dev machines) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: condense multi-line comments to single lines Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: audit <audit@local> --------- Co-authored-by: Aakash Velusamy <aakashvpsgtech@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: pugazhendhi-m <132246623+pugazhendhi-m@users.noreply.github.com> Co-authored-by: audit <audit@local> Co-authored-by: Vishnu <79318686+zeus-12@users.noreply.github.com>
pugazhendhi-m
approved these changes
Aug 6, 2026
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import logging | ||
| import re | ||
| import xml.etree.ElementTree as ET | ||
| import defusedxml.ElementTree as DefusedET |
Contributor
There was a problem hiding this comment.
When discovery runs from the repository's supported dependency installation, the eager defusedxml import executes even though requirements.txt declares no external dependencies, causing ModuleNotFoundError before the scanner can initialize. Add defusedxml to every supported installation and packaging path.
Knowledge Base Used:
| servers = [] | ||
| try: | ||
| tree = ET.parse(xml_path) | ||
| tree = DefusedET.parse(xml_path) |
Contributor
There was a problem hiding this comment.
When discovery encounters crafted JetBrains XML, macOS recent-project parsing at line 145 and Windows IDE-level MCP parsing at line 178 still use ET.parse, leaving those active paths exposed to malicious XML resource exhaustion despite this mitigation. Convert both sibling parsing paths to the hardened parser as well.
How this was verified: The normal extraction flows pass existing recent-project and IDE MCP XML files directly to the remaining ET.parse calls.
Knowledge Base Used:
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
Contributor
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
2 findings — 2 high-confidence, 0 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 Incomplete XXE mitigation — remaining ET.parse call sites
scripts/coding_discovery_tools/macos/jetbrains/mcp_config_extractor.py:145 · scripts/coding_discovery_tools/windows/jetbrains/mcp_config_extractor.py:178
- Impact: macOS recent-project XML and Windows IDE-level MCP XML still parse through vulnerable
xml.etree.ElementTree, leaving reachable XXE / XML-bomb exposure on the paths this PR is meant to harden. - Fix: Replace both remaining
ET.parsecalls withDefusedET.parse(same pattern as the hardened siblings in this diff). - Flagged by: Semgrep, Greptile, Cursor
🔴 Undeclared defusedxml runtime dependency
scripts/coding_discovery_tools/macos/jetbrains/jetbrains.py:9 · scripts/coding_discovery_tools/macos/jetbrains/mcp_config_extractor.py:9 · scripts/coding_discovery_tools/windows/jetbrains/jetbrains.py:9 · scripts/coding_discovery_tools/windows/jetbrains/mcp_config_extractor.py:9
- Impact: Eager
import defusedxml.ElementTreeruns at module load; withoutdefusedxmlin supported install/packaging paths (requirements.txtcurrently has no external deps), discovery can fail withModuleNotFoundErrorbefore any XML is parsed. - Fix: Add
defusedxmlto every supported dependency and deployment path (e.g.requirements.txt, packaging manifests, install docs). - Flagged by: Greptile, Cursor
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head d6f398d6 · 2026-08-06T05:40Z
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch mitigates XML external entity (XXE) and other XML-based attacks in JetBrains discovery and configuration extraction scripts across macOS and Windows platforms by replacing vulnerable xml.etree.ElementTree parsing functions with their secure defusedxml.ElementTree equivalents at lines 250 and 201 in macOS scripts (jetbrains.py and mcp_config_extractor.py respectively) and lines 327 and 287 in their Windows counterparts.
Aikido used AI to generate this PR.
Medium confidence: Aikido has validated similar fixes and observed positive outcomes. Validation is required.
Note
Low Risk
Behavioral change is limited to XML parsing libraries on discovery paths; ensure defusedxml is declared as a dependency and note partial migration leaves some ET.parse paths unchanged.
Overview
Replaces selected
xml.etree.ElementTreeparse entry points in the macOS and Windows JetBrains coding-discovery scripts withdefusedxml.ElementTree(DefusedET.fromstring/DefusedET.parse) to reduce XXE and related XML abuse when reading untrusted or user-local XML.Plugin metadata:
_parse_plugin_xmlon both platforms now parses cleanedplugin.xmlstrings viaDefusedET.fromstring(from disk or inside plugin JARs).MCP / IDE config: macOS
_parse_mcp_xmland Windows_extract_project_paths_from_xmlnow useDefusedET.parseinstead ofET.parse.ETis still imported where unchanged (e.g.ET.ParseError, macOS recent-project XML, Windows_parse_mcp_xml). Linux JetBrains modules and otherET.parsecall sites in these extractors were not updated in this diff—worth confirming dependency ondefusedxmland whether follow-up hardening is intended.Reviewed by Cursor Bugbot for commit d6f398d. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR switches selected JetBrains XML parsing operations on macOS and Windows to defusedxml to reduce exposure to malicious XML.
Confidence Score: 2/5
This PR should not merge until defusedxml is installed through the supported deployment paths and all reachable JetBrains XML parsers covered by the mitigation are hardened.
The eager imports can prevent discovery from starting in normally installed environments, and two active XML parsing paths remain exposed to the weakness this patch intends to address.
Files Needing Attention: scripts/coding_discovery_tools/macos/jetbrains/jetbrains.py, scripts/coding_discovery_tools/macos/jetbrains/mcp_config_extractor.py, scripts/coding_discovery_tools/windows/jetbrains/jetbrains.py, scripts/coding_discovery_tools/windows/jetbrains/mcp_config_extractor.py
Security Review
One reachable XML parsing path remains unhardened in each MCP extractor: macOS recent-project parsing and Windows IDE-level MCP parsing still use
xml.etree.ElementTree.Important Files Changed
Reviews (1): Last reviewed commit: "fix(security): autofix Using blacklisted..." | Re-trigger Greptile
Context used: