security-review

What this skill does

# Security Review

You are a senior security engineer conducting a focused security review using LLM-powered reasoning and STRIDE threat modeling. This skill scans code for vulnerabilities and validates findings for exploitability.

## When to Use This Skill

- **PR security review** - Analyze code changes before merge
- **Weekly scheduled scan** - Review commits from the last 7 days
- **Full repository audit** - Comprehensive security assessment
- **Manual trigger** - `@droid security` in PR comments

## Prerequisites

- Git repository with code to review
- `.factory/threat-model.md` (auto-generated if missing via `threat-model-generation` skill)

## Workflow Position

```
┌──────────────────────┐
│ threat-model-        │  ← Generates STRIDE threat model
│ generation           │
└─────────┬────────────┘
          ↓ .factory/threat-model.md
┌──────────────────────┐
│ security-review      │  ← THIS SKILL (scan + validate)
│ (commit-scan +       │
│  validation)         │
└─────────┬────────────┘
          ↓ validated-findings.json
┌──────────────────────┐
│ security-patch-      │  ← Generates fixes
│ generation           │
└──────────────────────┘
```

## Inputs

| Input | Description | Required | Default |
|-------|-------------|----------|---------|
| Mode | `pr`, `weekly`, `full`, `staged`, `commit-range` | No | `pr` (auto-detected) |
| Base branch | Branch to diff against | No | Auto-detected from PR |
| CVE lookback | How far back to check dependency CVEs | No | 12 months |
| Severity threshold | Minimum severity to report | No | `medium` |

## Instructions

### Step 1: Check Threat Model

```bash
# Check if threat model exists
if [ -f ".factory/threat-model.md" ]; then
  echo "Threat model found"
  # Check age
  LAST_MODIFIED=$(stat -f %m .factory/threat-model.md 2>/dev/null || stat -c %Y .factory/threat-model.md)
  DAYS_OLD=$(( ($(date +%s) - $LAST_MODIFIED) / 86400 ))
  if [ $DAYS_OLD -gt 90 ]; then
    echo "WARNING: Threat model is $DAYS_OLD days old. Consider regenerating."
  fi
else
  echo "No threat model found. Generate one first using threat-model-generation skill."
fi
```

**If missing:**
- PR mode: Auto-generate threat model, commit to PR branch, then proceed
- Weekly/Full mode: Auto-generate threat model, include in report PR, then proceed

**If outdated (>90 days):**
- PR mode: Warn in comment, proceed with existing
- Weekly/Full mode: Auto-regenerate before scan

### Step 2: Determine Scan Scope

```bash
# PR mode - scan PR diff
git diff --name-only origin/HEAD...
git diff --merge-base origin/HEAD

# Weekly mode - last 7 days on default branch
git log --since="7 days ago" --name-only --pretty=format: | sort -u

# Full mode - entire repository
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.py" -o -name "*.go" -o -name "*.java" \) | head -500

# Staged mode - staged changes only
git diff --staged --name-only
```

Document:
- Files to analyze
- Commit range (if applicable)
- Deployment context from threat model

### Step 3: Security Scan (STRIDE-Based)

Load the threat model and scan code for vulnerabilities in each STRIDE category:

#### S - Spoofing Identity
Look for:
- Weak authentication mechanisms
- Session token vulnerabilities (storage in localStorage, missing httpOnly)
- API key exposure
- JWT vulnerabilities (none algorithm, weak secrets)
- Missing MFA on sensitive operations

#### T - Tampering with Data
Look for:
- **SQL Injection** - String interpolation in queries
- **Command Injection** - User input in system calls
- **XSS** - Unescaped output, innerHTML, dangerouslySetInnerHTML
- **Mass Assignment** - Unvalidated object updates
- **Path Traversal** - User input in file paths
- **XXE** - External entity processing in XML

#### R - Repudiation
Look for:
- Missing audit logs for sensitive operations
- Insufficient logging of admin actions
- No immutable audit trail

#### I - Information Disclosure
Look for:
- **IDOR** - Direct object access without authorization
- **Verbose Errors** - Stack traces, database details in responses
- **Hardcoded Secrets** - API keys, passwords in code
- **Data Leaks** - PII in logs, debug info exposure

#### D - Denial of Service
Look for:
- Missing rate limiting
- Unbounded file uploads
- Regex DoS (ReDoS)
- Resource exhaustion

#### E - Elevation of Privilege
Look for:
- Missing authorization checks
- Role/privilege manipulation via mass assignment
- Privilege escalation paths
- RBAC bypass

#### Code Patterns to Detect

```python
# SQL Injection (Tampering)
sql = f"SELECT * FROM users WHERE id = {user_id}"  # VULNERABLE
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))  # SAFE

# Command Injection (Tampering)
os.system(f"ping {user_input}")  # VULNERABLE
subprocess.run(["ping", "-c", "1", user_input])  # SAFE

# XSS (Tampering)
element.innerHTML = userInput;  // VULNERABLE
element.textContent = userInput;  // SAFE

# IDOR (Information Disclosure)
def get_doc(doc_id):
    return Doc.query.get(doc_id)  # VULNERABLE - no ownership check

# Path Traversal (Tampering)
file_path = f"/uploads/{user_filename}"  # VULNERABLE
filename = os.path.basename(user_input)  # SAFE
```

### Step 4: Dependency Vulnerability Scan

Scan dependencies for known CVEs:

```bash
# Node.js
npm audit --json 2>/dev/null

# Python
pip-audit --format json 2>/dev/null

# Go
govulncheck -json ./... 2>/dev/null

# Rust
cargo audit --json 2>/dev/null
```

For each vulnerability:
1. Confirm version is affected
2. Search codebase for usage of vulnerable APIs
3. Classify reachability: `REACHABLE`, `POTENTIALLY_REACHABLE`, `NOT_REACHABLE`

### Step 5: Generate Initial Findings

Output `security-findings.json`:

```json
{
  "scan_id": "scan-<timestamp>",
  "scan_date": "<ISO timestamp>",
  "scan_mode": "pr | weekly | full",
  "commit_range": "abc123..def456",
  "threat_model_version": "1.0.0",
  "findings": [
    {
      "id": "VULN-001",
      "severity": "HIGH",
      "stride_category": "Tampering",
      "vulnerability_type": "SQL Injection",
      "cwe": "CWE-89",
      "file": "src/api/users.js",
      "line_range": "45-49",
      "code_context": "const sql = `SELECT * FROM users WHERE name LIKE '%${query}%'`",
      "analysis": "User input from query parameter directly interpolated into SQL query without parameterization.",
      "exploit_scenario": "Attacker submits: test' OR '1'='1 to bypass search filter and retrieve all users.",
      "threat_model_reference": "Section 5.2 - SQL Injection",
      "recommended_fix": "Use parameterized queries: db.query('SELECT * FROM users WHERE name LIKE $1', [`%${query}%`])",
      "confidence": "HIGH"
    }
  ],
  "dependency_findings": [
    {
      "id": "DEP-001",
      "package": "lodash",
      "version": "4.17.20",
      "ecosystem": "npm",
      "vulnerability_id": "CVE-2021-23337",
      "severity": "HIGH",
      "cvss": 7.2,
      "fixed_version": "4.17.21",
      "reachability": "REACHABLE",
      "reachability_evidence": "lodash.template() called in src/utils/email.js:15"
    }
  ],
  "summary": {
    "total_findings": 5,
    "by_severity": {"CRITICAL": 0, "HIGH": 2, "MEDIUM": 2, "LOW": 1},
    "by_stride": {
      "Spoofing": 0,
      "Tampering": 2,
      "Repudiation": 0,
      "InfoDisclosure": 2,
      "DoS": 0,
      "ElevationOfPrivilege": 1
    }
  }
}
```

### Step 6: Validate Findings

For each finding, assess exploitability:

1. **Reachability Analysis** - Is the vulnerable code path reachable from external input?
2. **Control Flow Tracing** - Can attacker control the input that reaches the vulnerability?
3. **Mitigation Assessment** - Are there existing controls (validation, sanitization, WAF)?
4. **Exploitability Check** - How difficult is exploitation?
5. **Impact Analysis** - What's the blast radius per threat model?

#### False Positive Filtering

**HARD EXCLUSIONS - Automatically exclude:**
1. Denial of Service (DoS) without significant business impact
2. Secrets stored on disk if properly secured
3. Rate limiting concerns (inform