fix(security): remove pre-existing bandit HIGH findings and enable the HIGH gate

[kovtcharov-amd]

Bandit ran warning-only, so 18 pre-existing HIGH-severity findings sat unblocked in core paths (CLI, subprocess helpers, MCP stdio transport, Lemonade launch). Before: a new shell=True injection site or weak-hash regression would pass lint and ship. After: all 18 are fixed (16 by removing the risk, 2 kept as justified suppressions) and any new HIGH finding now fails util/lint.py --bandit.

How each finding was handled

Fixed by removing the risk (16): shell=True string commands converted to shell=False args-lists.

• util.py / cli.py port-kill helpers: run netstat/lsof bare and filter output in Python instead of piping through the shell; taskkill/kill take args-lists. Port inputs are int()-validated first.
• lemonade_client.py: os.system(taskkill …) → subprocess.run([...], shell=False) (B605).
• logger.py: chcp (a cmd.exe builtin) → cmd /c chcp.
• ui/build.py: npm calls (.cmd shims) → cmd /c npm … on Windows, bare on other platforms.
• cli.py mailto opener: start "" <url> → os.startfile() (ShellExecute, no shell parsing) — this one mattered because the URL is user-built.
• context7_cache.py: MD5 is a cache key, fixed with usedforsecurity=False (B324).

Kept as justified suppressions (2) — inline # nosec B602 + a matching entry in .security-suppressions.json:

• agents/tools/shell_tools.py — sandboxed shell executor; every command (and pipeline segment) is whitelist-validated before running, and shell=True is Windows-only so cmd.exe can resolve built-ins/pipes the tool exists to run.
• mcp/client/transports/stdio.py — legacy from_command() accepts a full shell command string (documented contract), needs shell parsing; command is trusted SDK config, and modern from_config() already runs shell=False.

The gate

util/check_security_gates.py adds new_bandit_highs(results, baseline), keyed by (normalized path, test_id) — never line number, so cosmetic line shifts don't trip it. Wired into util/lint.py's check_bandit(); .bandit-baseline.json ships as an empty allowlist so any HIGH fails.

Test plan

• python -m bandit -r src/gaia -ll -f json → 0 HIGH findings
• python util/lint.py --bandit → PASS
• Negative check: adding a subprocess.run(f"echo {x}", shell=True) probe makes the gate exit 1; passes again once removed
• pytest tests/unit/test_check_security_gates.py → 18 passed
• pytest tests/unit/test_webui_build.py tests/unit/mcp/client/ tests/unit/test_shell_guardrails.py → pass
• Positive-path functional test: started a real listener, confirmed the converted netstat detection + taskkill args-list finds and kills it and frees the port
• black/isort clean on all changed files

[github-actions]

Verdict: Approve with suggestions — safe to merge; one easy efficiency fix worth applying.

This PR closes 18 pre-existing bandit HIGH findings (16 by converting shell=True string commands to shell=False args-lists, 2 kept as justified inline suppressions) and adds a blocking gate so any new HIGH fails lint. The conversions are faithful — port inputs are int()-validated before use, PIDs are passed as strings, and the two retained shell=True sites are genuinely constrained (whitelist-validated sandbox executor; trusted SDK config). Test coverage for the gate logic is thorough.

The one thing worth fixing before merge: the lint step runs bandit over src/gaia twice per invocation, which noticeably slows lint.py. Reuse the report you already fetched. Details below.

🔍 Technical details

🟢 Minor

Bandit scans src/gaia twice per lint run (util/lint.py:299,304)
check_bandit() calls run_bandit(SRC_DIR) to get report, then calls check_bandit_high_gate(SRC_DIR), which internally calls run_bandit(SRC_DIR) a second time. Every lint.py --bandit / --all pays for two full scans. Reuse the report you already have:

    from check_security_gates import (
        format_finding,
        load_baseline,
        new_bandit_highs,
        run_bandit,
    )

    try:
        report = run_bandit(SRC_DIR)
    except (RuntimeError, ValueError) as exc:
        print(f"[ERROR] Could not run Bandit: {exc}")
        return CheckResult("Security Check (Bandit)", False, False, 1, str(exc))

    new_highs = new_bandit_highs(
        report.get("results", []), load_baseline(".bandit-baseline.json")
    )
    passed = not new_highs

(This drops the now-unused check_bandit_high_gate import; the standalone main() path still uses it.)

Note (not blocking): run_bandit hardcodes DEFAULT_EXCLUDE rather than reading lint.py's EXCLUDE_DIRS. They appear equivalent today, but two exclude lists that must stay in sync will drift. Consider having check_bandit() pass EXCLUDE_DIRS through.

Strengths

• Faithful conversions: int()-validation on ports precedes every subprocess call, PIDs are stringified (str(pid) / already-string parts[-1]) so the args-lists don't hit TypeError, and the Python-side f":{port}" in line filter preserves the old findstr/grep semantics.
• The two retained shell=True sites are the defensible ones, each with an inline # nosec and a matching justification in .security-suppressions.json — no blanket suppression.
• Gate keyed by (normalized path, test_id) rather than line number is the right call; cosmetic line shifts won't re-trip it, and test_check_security_gates.py covers that plus Windows-path normalization, MEDIUM/LOW exclusion, and the "baseline must stay empty" invariant.
• os.startfile() for the user-built mailto URL is the correct fix — ShellExecute with no shell parsing.

[github-actions]

🟡 check_bandit() runs bandit twice on every python util/lint.py invocation, doubling the most expensive lint step.

run_bandit(SRC_DIR) is called explicitly (line 303) to populate report for the MEDIUM/LOW count, then check_bandit_high_gate(SRC_DIR) is called (line 308) which calls run_bandit again internally. Bandit scans the whole source tree — two invocations is a significant, silent regression to lint time.

Fix: drop check_bandit_high_gate and drive everything off the single report already in hand:

🔍 Technical details

util/lint.py:308 calls check_bandit_high_gate(SRC_DIR), and util/check_security_gates.py:635 shows check_bandit_high_gate unconditionally calls run_bandit(src_dir) itself.

Replace the two-call pattern with:

from check_security_gates import (
    format_finding,
    load_baseline,
    new_bandit_highs,
    run_bandit,
)

try:
    report = run_bandit(SRC_DIR)
except (RuntimeError, ValueError) as exc:
    print(f"[ERROR] Could not run Bandit: {exc}")
    return CheckResult("Security Check (Bandit)", False, False, 1, str(exc))

baseline = load_baseline(".bandit-baseline.json")
new_highs = new_bandit_highs(report.get("results", []), baseline)
passed = not new_highs

This removes the duplicate scan while keeping both the HIGH gate and the MEDIUM/LOW informational count.

[kovtcharov-amd]

Good catch on the double scan — fixed in 1f1b8825. check_bandit() now runs bandit once and drives both the HIGH gate and the MEDIUM/LOW informational count off that single report.

[github-actions]

🟡 lint.py crashes when bandit isn't installed — previously it degraded gracefully with a warning.

The new check_bandit() catches (RuntimeError, ValueError) from run_bandit(), but subprocess.run raises FileNotFoundError when the bandit binary isn't in PATH. That exception type isn't caught, so it propagates out of check_bandit() and crashes lint.py. Before this PR, check_bandit() called run_command() which explicitly handles FileNotFoundError and returns (1, "Command not found: bandit").

In practice this only bites contributors who haven't installed dev dependencies, so CI is unaffected — but it's a meaningful UX regression.

🔍 Technical details

util/lint.py:50-64 — the old run_command() catches FileNotFoundError:

except FileNotFoundError:
    return 1, f"Command not found: {cmd[0]}"

util/check_security_gates.py:597 — the new run_bandit() uses bare subprocess.run with no FileNotFoundError guard.

util/lint.py:704-706 — the new check_bandit() only catches:

except (RuntimeError, ValueError) as exc:

Minimal fix — add FileNotFoundError to the except tuple in check_bandit():

except (RuntimeError, ValueError, FileNotFoundError) as exc:
    print(f"[ERROR] Could not run Bandit: {exc}")
    return CheckResult("Security Check (Bandit)", False, False, 1, str(exc))

Or handle it in run_bandit() itself and re-raise as RuntimeError for consistency.

[kovtcharov-amd]

Fixed in d74deaa7. run_bandit() now catches FileNotFoundError and re-raises an actionable RuntimeError (with an install hint), so lint.py degrades gracefully when bandit isn't installed instead of crashing — callers still handle a single exception type. Added a unit test for it, and threaded lint.py's EXCLUDE_DIRS through run_bandit() per your non-blocking note so the two exclude lists can't drift.