differential-review（差别审查）

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:

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]

• // 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

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

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`](https://github.com/oimiragieo/agent-studio/blob/HEAD/.claude/skills/differential-review/../static-analysis/SKILL.md) - Full codebase static analysis
- [`variant-analysis`](https://github.com/oimiragieo/agent-studio/blob/HEAD/.claude/skills/differential-review/../variant-analysis/SKILL.md) - Pattern-based vulnerability discovery
- [`semgrep-rule-creator`](https://github.com/oimiragieo/agent-studio/blob/HEAD/.claude/skills/differential-review/../semgrep-rule-creator/SKILL.md) - Custom detection rules
- [`insecure-defaults`](https://github.com/oimiragieo/agent-studio/blob/HEAD/.claude/skills/differential-review/../insecure-defaults/SKILL.md) - Hardcoded credentials detection
- [`security-architect`](https://github.com/oimiragieo/agent-studio/blob/HEAD/.claude/skills/differential-review/../security-architect/SKILL.md) - 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.