Differential Review

Security Notice

AUTHORIZED USE ONLY: These skills are for DEFENSIVE security analysis and authorized research:

• Pull request security review for owned repositories
• Pre-merge security validation in CI/CD pipelines
• Security regression detection in code changes
• Compliance validation of code modifications
• Educational purposes in controlled environments

NEVER use for:

• Reviewing code you are not authorized to access
• Exploiting discovered vulnerabilities without disclosure
• Circumventing code review processes
• Any illegal activities

Step 1: Obtain the Diff

Git Diff Methods

# Review staged changes
git diff --cached

# Review specific commit
git diff HEAD~1..HEAD

# Review pull request (GitHub)
gh pr diff <PR-NUMBER>

# Review specific files
git diff --cached -- src/auth/ src/api/

# Review with context (10 lines)
git diff -U10 HEAD~1..HEAD

# Show only changed file names
git diff --name-only HEAD~1..HEAD

# Show stats (insertions/deletions per file)
git diff --stat HEAD~1..HEAD

Classify Changed Files

Prioritize review by security sensitivity:

Priority	File Patterns	Reason
P0	**/auth/**, **/security/**, **/crypto/**	Direct security code
P0	*.env*, **/config/**, **/secrets/**	Configuration and secrets
P0	**/middleware/**, **/guards/**, **/validators/**	Security controls
P1	**/api/**, **/routes/**, **/controllers/**	Attack surface
P1	package.json, requirements.txt, go.mod	Dependency changes
P1	Dockerfile, docker-compose.yml, *.yaml	Infrastructure config
P2	**/models/**, **/db/**, **/queries/**	Data access layer
P2	**/utils/**, **/helpers/**	Shared utility code
P3	**/tests/**, **/docs/**	Tests and documentation

Step 2: Security-Focused Diff Analysis

Analysis Framework

For each changed file, evaluate these security dimensions:

2.1 Input Validation Changes

CHECK: Did the change modify input validation?
- Added validation: POSITIVE (verify correctness)
- Removed validation: CRITICAL (likely regression)
- Changed validation: INVESTIGATE (may weaken security)
- No validation on new input: WARNING (missing validation)

Red Flags:

• Removing or weakening regex patterns
• Commenting out validation middleware
• Changing strict mode to loose
• Adding any type or disabling type checks
• Removing length limits or range checks

2.2 Authentication/Authorization Changes

CHECK: Did the change affect auth?
- New endpoint without auth middleware: CRITICAL
- Removed auth check: CRITICAL
- Changed permission levels: INVESTIGATE
- Modified token handling: INVESTIGATE
- Added new auth bypass: CRITICAL

Red Flags:

• Routes added without authentication middleware
• isAdmin checks removed or weakened
• Token expiry extended significantly
• Session management changes
• CORS policy relaxation

2.3 Data Flow Changes

CHECK: Did the change introduce new data flows?
- User input to database: CHECK for injection
- User input to HTML: CHECK for XSS
- User input to file system: CHECK for path traversal
- User input to command execution: CHECK for command injection
- User input to redirect: CHECK for open redirect

2.4 Cryptographic Changes

CHECK: Did the change affect cryptography?
- Algorithm downgrade: CRITICAL (e.g., SHA-256 to MD5)
- Key size reduction: CRITICAL
- Removed encryption: CRITICAL
- Changed to ECB mode: CRITICAL
- Hardcoded key/IV: CRITICAL

2.5 Error Handling Changes

CHECK: Did the change affect error handling?
- Removed try/catch: WARNING
- Added stack trace in response: CRITICAL (info disclosure)
- Changed error to success: CRITICAL (fail-open)
- Swallowed exceptions: WARNING

2.6 Dependency Changes

CHECK: Did dependencies change?
- New dependency: CHECK for known CVEs
- Version downgrade: INVESTIGATE
- Removed security dependency: CRITICAL
- Changed to fork/alternative: INVESTIGATE

# Check new dependencies for known vulnerabilities
npm audit
pip audit
go list -m -json all | nancy sleuth

Step 3: Inline Security Comments

Comment Format

For each finding, provide a structured inline comment:

**SECURITY [SEVERITY]**: [Brief description]

**Location**: `file.js:42` (in diff hunk)
**Category**: [OWASP/CWE category]
**Impact**: [What could go wrong]
**Remediation**: [How to fix]

```diff
- // Current (vulnerable)
- db.query("SELECT * FROM users WHERE id = " + userId);
+ // Suggested (safe)
+ db.query("SELECT * FROM users WHERE id = $1", [userId]);
```

### Severity Levels for Diff Findings

| Severity | Criteria | Action |
|----------|----------|--------|
| **CRITICAL** | Exploitable vulnerability introduced | Block merge |
| **HIGH** | Security regression or missing control | Block merge |
| **MEDIUM** | Weak pattern that could lead to vulnerability | Request changes |
| **LOW** | Style issue with security implications | Suggest improvement |
| **INFO** | Security observation, no immediate risk | Note for awareness |

## Step 4: Differential Security Report

### Report Template

```markdown
## Differential Security Review

**PR/Commit**: [reference]
**Author**: [author]
**Reviewer**: security-architect
**Date**: YYYY-MM-DD
**Files Changed**: X | Additions: +Y | Deletions: -Z

### Security Impact Summary

| Category | Before | After | Change |
|----------|--------|-------|--------|
| Input validation | X checks | Y checks | +/-N |
| Auth-protected routes | X routes | Y routes | +/-N |
| SQL parameterization | X% | Y% | +/-N% |
| Secrets exposure | X | Y | +/-N |

### Findings

#### CRITICAL
1. [Finding with full details and remediation]

#### HIGH
1. [Finding with full details and remediation]

#### MEDIUM
1. [Finding with full details and remediation]

### Verdict

- [ ] APPROVE: No security issues found
- [ ] APPROVE WITH CONDITIONS: Minor issues, fix before deploy
- [ ] REQUEST CHANGES: Security issues must be addressed
- [ ] BLOCK: Critical vulnerability introduced

Step 5: Automated Diff Scanning

Semgrep Diff Mode

# Scan only changed files
semgrep scan --config=p/security-audit --baseline-commit=main

# Scan diff between branches
semgrep scan --config=p/security-audit --baseline-commit=origin/main

# Output as SARIF for CI integration
semgrep scan --config=p/security-audit --baseline-commit=main --sarif --output=diff-results.sarif

Custom Diff Security Checks

# Check for secrets in diff
git diff --cached | grep -iE "(password|secret|api.?key|token|credential)\s*[=:]"

# Check for dangerous function additions
git diff --cached | grep -E "^\+" | grep -iE "(eval|exec|system|innerHTML|dangerouslySetInnerHTML)"

# Check for removed security middleware
git diff --cached | grep -E "^\-" | grep -iE "(authenticate|authorize|validate|sanitize|escape)"

# Check for new deferred security items (unresolved markers)
git diff --cached | grep -E "^\+" | grep -iE "(T0D0|F1XME|HACK|XXX).*(security|auth|vuln)"

GitHub Actions Integration

name: Security Diff Review
on: [pull_request]
jobs:
  security-diff:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - name: Semgrep diff scan
        uses: returntocorp/semgrep-action@v1
        with:
          config: p/security-audit
      - name: Check for secrets
        run: |
          git diff origin/main..HEAD | grep -iE "(password|secret|api.?key|token)\s*[=:]" && exit 1 || exit 0

Common Security Regressions in Diffs

Pattern	What Changed	Risk
Removed helmet() middleware	Security headers removed	Header injection, clickjacking
Changed sameSite: 'strict' to 'none'	Cookie policy weakened	CSRF attacks
Removed rate limiting middleware	Rate limit removed	Brute force, DoS
Added cors({ origin: '*' })	CORS wildcard	Cross-origin attacks
Removed csrf() middleware	CSRF protection removed	CSRF attacks
Changed httpOnly: true to false	Cookie accessible to JS	XSS token theft

Related Skills

• static-analysis - Full codebase static analysis
• variant-analysis - Pattern-based vulnerability discovery
• semgrep-rule-creator - Custom detection rules
• insecure-defaults - Hardcoded credentials detection
• security-architect - STRIDE threat modeling

Agent Integration

• code-reviewer (primary): Security-augmented code review
• security-architect (primary): Security assessment of changes
• penetration-tester (secondary): Verify exploitability of findings
• developer (secondary): Security-aware development guidance

Iron Laws

1. ALWAYS classify changed files by security sensitivity (P0–P3) before reviewing — never dive into code without a triage map; you will miss the highest-risk changes.
2. NEVER treat removal of security middleware (auth, CSRF, rate-limit, helmet) as a routine refactor — always flag as CRITICAL and require explicit justification in the PR description.
3. ALWAYS use git diff -U10 for context-extended diffs — the default 3-line context is insufficient to detect security regressions from function reordering or middleware removal.
4. NEVER approve a diff that adds a new public endpoint without verifying authentication middleware is applied — unauthenticated routes in diffs are high-frequency security regressions.
5. ALWAYS check deleted lines as carefully as added lines — removed security controls (validation, logging, auth checks) are as dangerous as new vulnerable code.

Anti-Patterns

Anti-Pattern	Why It Fails	Correct Approach
Reviewing only changed lines without reading surrounding context	Security regressions appear as refactors when surrounding auth/middleware is removed	Use git diff -U10; read full function scope before and after the change
Treating security dependency removal as a dependency update	Removing a security package (helmet, csurf) eliminates its protections silently	Classify all dependency changes; flag security-package removals as CRITICAL
Skipping deleted-line review	Removed input validation, auth checks, or logging are invisible in addition-only review	Review deletions first; build the "what protections were removed" list
Approving new routes without auth check verification	New endpoints skip existing middleware when not explicitly added	Verify middleware chain for every new route/controller in the diff
Using informal severity like "looks fine" without CWE/OWASP reference	Severity ambiguity makes remediation prioritization inconsistent	Use the structured format: SECURITY [SEVERITY], CWE, OWASP category, remediation

Memory Protocol (MANDATORY)

Before starting: Read .claude/context/memory/learnings.md

After completing:

• New pattern -> .claude/context/memory/learnings.md
• Issue found -> .claude/context/memory/issues.md
• Decision made -> .claude/context/memory/decisions.md

ASSUME INTERRUPTION: If it's not in memory, it didn't happen.