feat(lfx-security-engineer): new skill for LFX security code review

[dealako]

Summary

• Adds a new lfx-security-engineer skill for security reviews of LFX repos
• Covers OWASP Top 10 checks, auth/authz patterns, secrets detection, PII exposure, Terraform/OpenTofu infrastructure, and database migration safety
• Supports Angular, Vue, Go, Rust, TypeScript BFF repo types with per-language detection and security checks
• Two-phase approach: Phase 1 automated scan (mechanical checks) + Phase 2 judgment-based review

What's included

Repo type detection

• Layered primary + secondary signal detection for Angular, Vue, Go, Rust (Cargo), TypeScript BFF, Terraform/OpenTofu, and DB migrations
• Rust support added via Cargo.toml primary signal with Rust-specific checks for unsafe blocks, weak crypto crates, command injection, and missing auth extractors

Phase 1 — Automated Scan (9 checks)

• Secrets & credentials (hardcoded API keys, tokens, passwords)
• OWASP A01 Broken Access Control (missing auth middleware, IDOR, FGA bypass)
• OWASP A02 Cryptographic Failures (weak hashes, insecure RNG, JWT alg enforcement)
• OWASP A03 Injection (SQL, command, path traversal, template injection)
• OWASP A07 Authentication Failures (JWT validation, cookie flags, brute force)
• OWASP A09 Security Logging Failures (silent auth failures, missing audit trails)
• Sensitive Data Exposure (PII in logs, stack traces in responses, over-returning)
• Terraform/OpenTofu infrastructure security (IAM, network rules, encryption, state)
• Database migration safety (plain-text columns, broad grants, hardcoded PII)

Phase 2 — Security Review (4 reviews)

• Authentication flow end-to-end
• Authorization patterns (FGA, least privilege, consistency)
• Input validation
• Security test coverage gaps

Operational features

• --scan-only, --file, --full-scan, --explain, --ci-mode, --format json, --watch modes
• False positive reduction guidelines (test fixtures, mock data, security-ignore: suppression)
• .secignore support for excluding paths
• Progressive disclosure (CRITICAL first, --all for full output)
• Scan caching with .security-cache/

Example Execution With Warnings

Click to expand

/lfx-security-engineer

⏺ Bash(git diff --name-only origin/main...HEAD)
  ⎿  src/dags/bevy/bevy_chapters.py
     src/dags/bevy/bevy_reimport_cncf.py

  Searched for 1 pattern (ctrl+o to expand)

⏺ Bash(git diff --name-only HEAD && echo "---staged---" && git diff --name-only --cached
  ⎿  src/dags/bevy/bevy_chapter_events.py
     src/dags/bevy/bevy_chapter_members.py
     src/dags/bevy/bevy_chapter_team.py
     … +18 lines (ctrl+o to expand)

⏺ Bash(ls requirements*.txt setup.py pyproject.toml 2>/dev/null; python3 --version
  ⎿  (eval):1: no matches found: requirements*.txt
     Python 3.13.9
     __init__.py
     … +18 lines (ctrl+o to expand)

⏺ Repo type: Python / Apache Airflow DAGs. Now running Phase 1 scans on all 10 changed
  files.
⏺ Bash(FILES=$(git diff --name-only HEAD)
      # Check 1: Hardcoded secrets/credentials…)
  ⎿  === CHECK 1: SECRETS ===
     NONE
     === CHECK 3: WEAK CRYPTO ===
     … +19 lines (ctrl+o to expand)

⏺ Bash(FILES=$(git diff --name-only HEAD)
      # Check 6: Security logging failures…)
  ⎿  === CHECK 6: LOGGING ===
     src/dags/bevy/bevy_chapter_events.py:435:    except requests.exceptions.HTTPError as err:
     src/dags/bevy/bevy_chapter_events.py:439:    except requests.exceptions.RequestException as err:
     … +25 lines (ctrl+o to expand)

⏺ Bash(FILES=$(git diff --name-only HEAD)
      echo "=== CONNECTION/TOKEN RETRIEVAL ==="…)
  ⎿  === CONNECTION/TOKEN RETRIEVAL ===
     src/dags/bevy/bevy_chapter_events.py:16:from bevy.bevy_connection_helper import get_connection_from_slug
     src/dags/bevy/bevy_chapter_events.py:180:    bevy_conn = get_connection_from_slug(bevy_conn_slug)
     … +45 lines (ctrl+o to expand)

⏺ Bash(echo "=== BEVY_CHAPTERS full_refresh param usage ==="
      grep -n 'full_refresh\|watermark\|last_run\|start_date\|since\|cursor\|incremental'
  ⎿  === BEVY_CHAPTERS full_refresh param usage ===
     196:    In incremental mode (full_refresh=False, the default), returns a no-op SELECT
     198:    In full_refresh mode (full_refresh=True), returns the DROP TABLE statement to allow
     … +37 lines (ctrl+o to expand)

  Read 1 file (ctrl+o to expand)

⏺ Bash(echo "=== CHECK: Table names in f-string SQL (injection risk) ==="
      grep -n 'cursor.execute.*f"' src/dags/bevy/bevy_summary.py…)
  ⎿  === CHECK: Table names in f-string SQL (injection risk) ===
     87:                    cursor.execute(f"SELECT COUNT(*) FROM {table}")
     === _TABLES hardcoded vs user-controlled ===
     … +27 lines (ctrl+o to expand)

⏺ Now I have everything needed for the full report.

  ---
  ═══════════════════════════════════════════
  🛡   LFX SECURITY REVIEW RESULTS
  ═══════════════════════════════════════════

  Repository: lfx-airstream
  Branch:     incremental-add
  Files scanned: 10 changed files (Python / Apache Airflow DAGs)
  Repo type:  Python ETL pipeline (no Terraform, no SQL migrations)

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  🔴 CRITICAL FINDINGS (0)

  None.

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  🟡 WARNINGS (2)

  ⚠   SQL identifier interpolation at src/dags/bevy/bevy_summary.py:87

      cursor.execute(f"SELECT COUNT(*) FROM {table}")

      The `table` variable is sourced exclusively from the hardcoded `_TABLES`
      dict — it is NOT user-controlled, so there is no real injection risk today.
      However, Snowflake's cursor cannot parameterize identifiers (only values),
      so the f-string pattern is the correct approach here. The risk is that a
      future change pulls table names from external config without this context.

      Recommendation: Add a one-line comment to document why the f-string is safe:

        # Table names are hardcoded constants — not user-controlled.
        cursor.execute(f"SELECT COUNT(*) FROM {table}")  # nosec

      This prevents future readers from misidentifying this as a vulnerability
      and suppresses linter / SAST false positives.

  ⚠   Silent Snowflake failure in src/dags/bevy/bevy_summary.py:90–94

      except Exception as exc:
          log.warning("Error fetching Bevy table counts: %s", exc)
      # execution continues — counts dict is empty {}

      If Snowflake is unreachable, the DAG proceeds with all counts at zero
      and pushes a "success" Slack message with zeros. Ops has no alert that
      the query actually failed — only a WARNING in the Airflow task log.

      Risk: Silent data-quality regression. A failed Snowflake connection looks
      identical in Slack to a run that imported zero records.

      Recommended fix: Re-raise after logging, or push an error flag to XCom
      so the branch operator routes to the failure notification:

        except Exception as exc:
            log.warning("Error fetching Bevy table counts: %s", exc)
            counts = {}  # ensure downstream gets empty dict
            # Optionally: raise  ← routes DAG to on_failure_callback

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✅ PASSED CHECKS (11)

  ✓ No hardcoded API keys, tokens, or passwords
  ✓ All Bevy API tokens retrieved via Airflow connection store (get_connection_from_slug)
  ✓ No weak cryptography (MD5, SHA1, Math.random)
  ✓ No SQL injection — MERGE/INSERT statements use %s parameterized values
  ✓ No command injection (no subprocess, os.system, eval)
  ✓ No stack traces exposed in responses or logs
  ✓ All HTTP exception handlers log with log.warning() before returning
  ✓ full_refresh parameter safely parsed via _resolve_full_refresh() with
    explicit bool/string handling and a safe default of False
  ✓ No PII in URLs or log messages (emails in default_args are Airflow
    notification addresses, not user PII from data)
  ✓ project_name in Slack message sourced from trusted internal conf
    (TriggerDagRunOperator), not external user input
  ✓ Copyright/SPDX header present on new file bevy_summary.py

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  📊 SUMMARY

    Status: ⚠   REVIEW NEEDED

    0 critical issues.
    2 warnings — recommend addressing before merge.

    Overall security posture: GOOD. The incremental refresh changes follow
    established patterns in this codebase. Credentials are handled correctly
    via Airflow's connection store. The full_refresh flag propagation through
    TriggerDagRunOperator conf is implemented consistently across all three
    reimport DAGs. The new bevy_summary DAG is clean; the two warnings are
    low-risk operational concerns rather than exploitable vulnerabilities.

    Next steps:
    1. Add # nosec comment to bevy_summary.py:87 to document intent
    2. Decide on failure-handling strategy for bevy_summary.py:90 —
       either raise to alert ops or document the silent-continue behavior
    3. Re-run: /lfx-security-engineer

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  Exit code: 1 (warnings present, no blockers)

✻ Cooked for 1m 39s

Example Execution With Warnings

Click to expand

/lfx-security-engineer

⏺ Bash(git diff --name-only HEAD)
  ⎿  src/dags/bevy/bevy_chapter_events.py
     src/dags/bevy/bevy_chapter_members.py
     src/dags/bevy/bevy_chapter_team.py
     … +7 lines (ctrl+o to expand)

⏺ Bash(FILES=$(git diff --name-only HEAD)…)
  ⎿  === CHECK 1: SECRETS ===
     NONE
     === CHECK 3: WEAK CRYPTO ===
     … +31 lines (ctrl+o to expand)

  Searched for 1 pattern (ctrl+o to expand)

⏺ Both fixes are confirmed in place. Full report:

  ---
  ═══════════════════════════════════════════
  🛡   LFX SECURITY REVIEW RESULTS
  ═══════════════════════════════════════════

  Repository: lfx-airstream
  Branch:     incremental-add
  Files scanned: 10 changed files (Python / Apache Airflow DAGs)

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  🔴 CRITICAL FINDINGS (0)

  None.

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  🟡 WARNINGS (0)

  🟡 WARNING [SUPPRESSED]

  ⚠   SQL identifier interpolation at src/dags/bevy/bevy_summary.py:87
      cursor.execute(f"SELECT COUNT(*) FROM {table}")  # nosec B608
      Suppression reason: "table is a hardcoded constant, not user-controlled"
      Reason is valid and specific. No action needed.

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✅ PASSED CHECKS (13)

  ✓ No hardcoded API keys, tokens, or passwords
  ✓ All Bevy API tokens retrieved via Airflow connection store
  ✓ No weak cryptography (MD5, SHA1, random)
  ✓ No SQL injection — MERGE/INSERT use %s parameterized values
  ✓ No command injection (no subprocess, os.system, eval)
  ✓ No stack traces exposed in logs or responses
  ✓ All HTTP exception handlers log + raise (all 9 DAG files)
  ✓ bevy_summary.py Snowflake failure now logs.error + re-raises (fixed)
  ✓ SQL identifier f-string suppressed with valid nosec B608 justification (fixed)
  ✓ full_refresh parameter safely parsed with bool/string handling
  ✓ No PII in URLs or log messages
  ✓ project_name in Slack messages sourced from trusted internal conf
  ✓ Copyright/SPDX header present on new file bevy_summary.py

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  📊 SUMMARY

    Status: ✅ APPROVED

    0 critical issues.
    0 active warnings (1 suppressed with valid justification).

    Both findings from the previous scan have been resolved:
    - Silent Snowflake failure → now log.error + raise
    - SQL identifier interpolation → suppressed with nosec B608 + justification

    Overall security posture: GOOD. Ready for PR.

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  Exit code: 0 (approved)

✻ Cooked for 34s

Test plan

• Run /lfx-security-engineer on an Angular repo with known auth issues and verify findings are reported
• Run /lfx-security-engineer on a Go repo with a Terraform module and verify both application and infrastructure checks fire
• Run /lfx-security-engineer --scan-only and confirm Phase 2 is skipped
• Run /lfx-security-engineer --file src/auth/ and confirm scope is limited to that path
• Verify Rust detection triggers on a repo with Cargo.toml and no package.json

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds a new /lfx-security-engineer skill intended to guide security-focused reviews across LFX repositories, combining a mechanical “Phase 1” scan checklist with a “Phase 2” judgment-based review checklist.

Changes:

• Introduces a new skill prompt describing repo-type detection (Angular/Vue/Go/Rust/TS BFF + Terraform + migrations).
• Documents Phase 1 automated scan checks (secrets, OWASP categories, Terraform, migrations) and Phase 2 review prompts (auth/authz/input validation/tests).
• Adds operational/documentation guidance (CI mode, formatting, suppressions, caching, reporting format).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jordanconway]

Hi David,
Based on some things I learned from David Wheeler at OpenSSF while I was working on skills with him, here are some (AI enhanced) suggestions

Suggestion: Extract Phase 1 checks into scripts to reduce token usage

Most of Phase 1 is deterministic bash (regex greps, repo detection) that the LLM reads (~490 lines), interprets, then re-types into a terminal. If it called a script instead, it would read ~20 lines of structured output rather than ~1,150 lines of instructions — roughly 4x token savings per invocation.

lfx-security-engineer/
  SKILL.md                       # ~150 lines: orchestration + Phase 2 (needs LLM judgment)
  lib/
    detect-repo-type.sh          # Shared across skills (see below)
    security-scan.sh             # Phase 1: runs all 9 checks, outputs structured findings
  references/
    false-positive-patterns.md   # Framework-aware safe patterns (LLM judgment context)
    report-template.md           # Output format
    usage-examples.md            # Loaded on demand

Script output would be something like:

FINDING|CRITICAL|secrets|src/config/db.ts:15|Hardcoded API key matching AKIA pattern
PASSED|injection|No SQL injection patterns

Bonus: detect-repo-type.sh eliminates duplicated detection logic in lfx-security-engineer, lfx-preflight, and lfx-coordinator.

I figured you can work through this easily by asking your AI assistant to try to reduce token usage by abstracting repeatable actions into scripts.

[jordanconway]

If you'd prefer I can also push up that change as I've got it locally

[dealako]

@jordanconway - feel free to close this out and push your version, or combine or consolidate. I'll defer to you on this.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 11 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

[jordanconway]

@jordane @dealako @asithade I think this is ready for review - my changed reduced the token usage fairly substantially by offloading repeatable, predictable tasks into scripts called by the skill rather than being purely markdown agent instructions.

[dealako]

@jordanconway, sorry, I didn't see that you updated this. Can you work to resolve the conflicts?