Stats
Actions
Tags
From prodsec-skills
Performs security-focused differential review of PRs, commits, and diffs. Analyzes code for security regressions, calculates blast radius, checks test coverage, and adapts depth to codebase size.
How this skill is triggered — by the user, by Claude, or both
Slash command
/prodsec-skills:differential-reviewThe summary Claude sees in its skill listing — used to decide when to auto-load this skill
<!-- skillsaw-disable context-budget -->
Security-focused code review for PRs, commits, and diffs.
| Rationalization | Why It's Wrong | Required Action |
|---|---|---|
| "Small PR, quick review" | Heartbleed was 2 lines | Classify by RISK, not size |
| "I know this codebase" | Familiarity breeds blind spots | Build explicit baseline context |
| "Git history takes too long" | History reveals regressions | Never skip Phase 1 |
| "Blast radius is obvious" | You'll miss transitive callers | Calculate quantitatively |
| "No tests = not my problem" | Missing tests = elevated risk rating | Flag in report, elevate severity |
| "Just a refactor, no security impact" | Refactors break invariants | Analyze as HIGH until proven LOW |
| "I'll explain verbally" | No artifact = findings lost | Always write report |
| Codebase Size | Strategy | Approach |
|---|---|---|
| SMALL (<20 files) | DEEP | Read all deps, full git blame |
| MEDIUM (20-200) | FOCUSED | 1-hop deps, priority files |
| LARGE (200+) | SURGICAL | Critical paths only |
| Risk Level | Triggers |
|---|---|
| HIGH | Auth, crypto, external calls, value transfer, validation removal |
| MEDIUM | Business logic, state changes, new public APIs |
| LOW | Comments, tests, UI, logging |
Pre-Analysis → Phase 0: Triage → Phase 1: Code Analysis → Phase 2: Test Coverage
↓ ↓ ↓ ↓
Phase 3: Blast Radius → Phase 4: Deep Context → Phase 5: Adversarial → Phase 6: Report
Starting a review?
├─ Need detailed phase-by-phase methodology?
│ └─ Jump to: [Differential Review Methodology](#differential-review-methodology)
│ (Pre-Analysis + Phases 0-4: triage, code analysis, test coverage, blast radius)
│
├─ Analyzing HIGH RISK change?
│ └─ Jump to: [Adversarial Vulnerability Analysis (Phase 5)](#adversarial-vulnerability-analysis-phase-5)
│
├─ Writing the final report?
│ └─ Jump to: [Report Generation (Phase 6)](#report-generation-phase-6)
│
├─ Looking for specific vulnerability patterns?
│ └─ Jump to: [Common Vulnerability Patterns](#common-vulnerability-patterns)
│
└─ Quick triage only?
└─ Use Quick Reference above, skip detailed docs
Before delivering:
audit-context-building (audit-context-building.md in this folder):
Formal reporting: Transform findings into formal audit reports using your team's workflow.
(See upstream Trail of Bits prodsec-skills for the issue-writer companion skill if you use that distribution.)
Input: 5 file PR, 2 HIGH RISK files
Strategy: Use Quick Reference
1. Classify risk level per file (2 HIGH, 3 LOW)
2. Focus on 2 HIGH files only
3. Git blame removed code
4. Generate minimal report
Time: ~30 minutes
Input: 80 files, 12 HIGH RISK changes
Strategy: FOCUSED (see [Differential Review Methodology](#differential-review-methodology))
1. Full workflow on HIGH RISK files
2. Surface scan on MEDIUM
3. Skip LOW risk files
4. Complete report with all sections
Time: ~3-4 hours
Input: 450 files, auth system rewrite
Strategy: SURGICAL + audit-context-building
1. Baseline context with audit-context-building
2. Deep analysis on auth changes only
3. Blast radius analysis
4. Adversarial modeling
5. Comprehensive report
Time: ~6-8 hours
For these cases, use standard code review instead.
Immediate escalation triggers:
These patterns require adversarial analysis even in quick triage.
Do:
Don't:
For first-time users: Start with Differential Review Methodology.
For experienced users: Use this page's Quick Reference and Decision Tree to jump to the section you need.
Detailed phase-by-phase workflow for security-focused code review.
FIRST ACTION - Build complete baseline understanding:
If the audit-context-building workflow is available (see audit-context-building.md in this folder):
git checkout <baseline_commit>packages/contracts/contracts/ for Solidity, src/ for Rust, or repository root).(See upstream Trail of Bits prodsec-skills for companion automation or agent wiring.)
Capture from baseline analysis:
Why this matters:
Store baseline context for reference during differential analysis.
After baseline analysis, checkout back to head commit to analyze changes.
Extract changes:
# For commit range
git diff <base>..<head> --stat
git log <base>..<head> --oneline
# For PR
gh pr view <number> --json files,additions,deletions
# Get all changed files
git diff <base>..<head> --name-only
Assess codebase size:
find . -name "*.sol" -o -name "*.rs" -o -name "*.go" -o -name "*.ts" | wc -l
Classify complexity:
Risk score each file:
For each changed file:
Read both versions (baseline and changed)
Analyze each diff region:
BEFORE: [exact code]
AFTER: [exact code]
CHANGE: [behavioral impact]
SECURITY: [implications]
Git blame removed code:
# When was it added? Why?
git log -S "removed_code" --all --oneline
git blame <baseline> -- file.sol | grep "pattern"
Red flags:
Check for regressions (re-added code):
git log -S "added_code" --all -p
Pattern: Code added → removed for security → re-added now = REGRESSION
Micro-adversarial analysis for each change:
Generate concrete attack scenarios:
SCENARIO: [attack goal]
PRECONDITIONS: [required state]
STEPS:
1. [specific action]
2. [expected outcome]
3. [exploitation]
WHY IT WORKS: [reference code change]
IMPACT: [severity + scope]
Identify coverage gaps:
# Production code changes (exclude tests)
git diff <range> --name-only | grep -v "test"
# Test changes
git diff <range> --name-only | grep "test"
# For each changed function, search for tests
grep -r "test.*functionName" test/ --include="*.sol" --include="*.js"
Risk elevation rules:
Calculate impact:
# Count callers for each modified function
grep -r "functionName(" --include="*.sol" . | wc -l
Classify blast radius:
Priority matrix:
| Change Risk | Blast Radius | Priority | Analysis Depth |
|---|---|---|---|
| HIGH | CRITICAL | P0 | Deep + all deps |
| HIGH | HIGH/MEDIUM | P1 | Deep |
| HIGH | LOW | P2 | Standard |
| MEDIUM | CRITICAL/HIGH | P1 | Standard + callers |
If the audit-context-building workflow is available, use it on the changed function and its dependencies to help answer the questions below. Scope the file(s) containing the changed function; focus on flow analysis, call graphs, invariants, and root cause.
(See upstream Trail of Bits prodsec-skills for companion automation.)
That workflow helps you answer:
Map complete function flow:
Trace internal calls:
Trace external calls:
Identify invariants:
Five Whys root cause:
If that workflow is not used, manually perform the line-by-line analysis above using your editor, search, and code tracing.
Cross-cutting pattern detection:
# Find repeated validation patterns
grep -r "require.*amount > 0" --include="*.sol" .
grep -r "onlyOwner" --include="*.sol" .
# Check if any removed in diff
git diff <range> | grep "^-.*require.*amount > 0"
Flag if removal breaks defense-in-depth.
Next steps:
Structured methodology for finding vulnerabilities through attacker modeling.
When to use: After completing deep context analysis (Phase 4), apply this to all HIGH RISK changes.
WHO is the attacker?
WHAT access/privileges do they have?
WHERE do they interact with the system?
ENTRY POINT: [Exact function/endpoint attacker can access]
ATTACK SEQUENCE:
1. [Specific API call/transaction with parameters]
2. [How this reaches the vulnerable code]
3. [What happens in the vulnerable code]
4. [Impact achieved]
PROOF OF ACCESSIBILITY:
- Show the function is public/external
- Demonstrate attacker has required permissions
- Prove attack path exists through actual interfaces
EASY: Exploitable via public APIs with no special privileges
MEDIUM: Requires specific conditions or elevated privileges
HARD: Requires privileged access or rare conditions
ATTACKER STARTING POSITION:
[What the attacker has at the beginning]
STEP-BY-STEP EXPLOITATION:
Step 1: [Concrete action through accessible interface]
- Command: [Exact call/request]
- Parameters: [Specific values]
- Expected result: [What happens]
Step 2: [Next action]
- Command: [Exact call/request]
- Why this works: [Reference to code change]
- System state change: [What changed]
Step 3: [Final impact]
- Result: [Concrete harm achieved]
- Evidence: [How to verify impact]
CONCRETE IMPACT:
[Specific, measurable impact - not "could cause issues"]
- Exact amount of funds drained
- Specific privileges escalated
- Particular data exposed
From baseline analysis (see Differential Review Methodology), check:
Generate this for each finding:
## [SEVERITY] Vulnerability Title
**Attacker Model:**
- WHO: [Specific attacker type]
- ACCESS: [Exact privileges]
- INTERFACE: [Specific entry point]
**Attack Vector:**
[Step-by-step exploit through accessible interfaces]
**Exploitability:** EASY/MEDIUM/HARD
**Justification:** [Why this rating]
**Concrete Impact:**
[Specific, measurable harm - not theoretical]
**Proof of Concept:**
```code
// Exact code to reproduce
Root Cause: [Reference specific code change at file.sol:L123]
Blast Radius: [N callers affected] Baseline Violation: [Which invariant/pattern broken]
---
## Example: Complete Adversarial Analysis
**Change:** Removed `require(amount > 0)` check from `withdraw()` function
### 1. Attacker Model
- **WHO:** Unauthenticated external user
- **ACCESS:** Can call public contract functions
- **INTERFACE:** `withdraw(uint256 amount)` at 0x1234...
### 2. Attack Vector
**ENTRY POINT:** `withdraw(0)`
**ATTACK SEQUENCE:**
1. Call `withdraw(0)` from attacker address
2. Code bypasses amount check (removed)
3. Withdraw event emitted with 0 amount
4. Accounting updated incorrectly
**PROOF:** Function is `external`, no auth required
### 3. Exploitability
**RATING:** EASY
- Single transaction
- Public function
- No special state required
### 4. Exploit Scenario
**ATTACKER POSITION:** Has user account with 0 balance
**EXPLOITATION:**
```solidity
Step 1: attacker.withdraw(0)
- Passes removed validation
- Emits Withdraw(user, 0)
- Updates withdrawnAmount[user] += 0
Step 2: Off-chain indexer sees Withdraw event
- Credits attacker for 0 withdrawal
- But accounting thinks withdrawal happened
Step 3: Accounting mismatch exploited
- Total supply decremented
- User balance not changed
- System invariants broken
IMPACT:
Next: Document all findings in final report (see Report Generation (Phase 6))
Comprehensive markdown report structure and formatting guidelines.
Generate markdown report with these mandatory sections:
Template:
# Executive Summary
| Severity | Count |
|----------|-------|
| 🔴 CRITICAL | X |
| 🟠 HIGH | Y |
| 🟡 MEDIUM | Z |
| 🟢 LOW | W |
**Overall Risk:** CRITICAL/HIGH/MEDIUM/LOW
**Recommendation:** APPROVE/REJECT/CONDITIONAL
**Key Metrics:**
- Files analyzed: X/Y (Z%)
- Test coverage gaps: N functions
- High blast radius changes: M functions
- Security regressions detected: P
Template:
## What Changed
**Commit Range:** `base..head`
**Commits:** X
**Timeline:** YYYY-MM-DD to YYYY-MM-DD
| File | +Lines | -Lines | Risk | Blast Radius |
|------|--------|--------|------|--------------|
| file1.sol | +50 | -20 | HIGH | CRITICAL |
| file2.sol | +10 | -5 | MEDIUM | LOW |
**Total:** +N, -M lines across K files
For each HIGH/CRITICAL issue:
### [SEVERITY] Title
**File**: path/to/file.ext:lineNumber
**Commit**: hash
**Blast Radius**: N callers (HIGH/MEDIUM/LOW)
**Test Coverage**: YES/NO/PARTIAL
**Description**: [clear explanation]
**Historical Context**:
- Git blame: Added in commit X (date)
- Message: "[original commit message]"
- [Why this code existed]
**Attack Scenario**:
[Concrete exploitation steps from adversarial.md]
**Proof of Concept**:
```code demonstrating issue```
**Recommendation**:
[Specific fix with code]
Example:
### 🔴 CRITICAL: Authorization Bypass in Withdraw
**File**: TokenVault.sol:156
**Commit**: abc123def
**Blast Radius**: 23 callers (HIGH)
**Test Coverage**: NO
**Description**:
Removed `require(msg.sender == owner)` check allows any user to withdraw funds.
**Historical Context**:
- Git blame: Added 2024-06-15 (commit def456)
- Message: "Add owner check per audit finding #45"
- Code existed to prevent unauthorized withdrawals
**Attack Scenario**:
1. Attacker calls `withdraw(1000 ether)`
2. No authorization check (removed)
3. 1000 ETH transferred to attacker
4. Protocol funds drained
**Proof of Concept**:
```solidity
// As any address
vault.withdraw(vault.balance());
// Success - funds stolen
Recommendation:
function withdraw(uint256 amount) external {
+ require(msg.sender == owner, "Unauthorized");
// ... rest of function
}
---
### 4. Test Coverage Analysis
- Coverage statistics
- Untested changes list
- Risk assessment
**Template:**
```markdown
## Test Coverage Analysis
**Coverage:** X% of changed code
**Untested Changes:**
| Function | Risk | Impact |
|----------|------|--------|
| functionA() | HIGH | No validation tests |
| functionB() | MEDIUM | Logic untested |
**Risk Assessment:**
N HIGH-risk functions without tests → Recommend blocking merge
Template:
## Blast Radius Analysis
**High-Impact Changes:**
| Function | Callers | Risk | Priority |
|----------|---------|------|----------|
| transfer() | 89 | HIGH | P0 |
| validate() | 45 | MEDIUM | P1 |
Template:
## Historical Context
**Security-Related Removals:**
- Line 45: `require` removed (added 2024-03 for CVE-2024-1234)
- Line 78: Validation removed (added 2023-12 "security hardening")
**Regression Risks:**
- Code pattern removed in commit X, re-added in commit Y
Template:
## Recommendations
### Immediate (Blocking)
- [ ] Fix CRITICAL issue in TokenVault.sol:156
- [ ] Add tests for withdraw() function
### Before Production
- [ ] Security audit of auth changes
- [ ] Load test blast radius functions
### Technical Debt
- [ ] Refactor validation pattern consistency
Template:
## Analysis Methodology
**Strategy:** FOCUSED (80 files, medium codebase)
**Analysis Scope:**
- Files reviewed: 45/80 (56%)
- HIGH RISK: 100% coverage
- MEDIUM RISK: 60% coverage
- LOW RISK: Excluded
**Techniques:**
- Git blame on all removals
- Blast radius calculation
- Test coverage analysis
- Adversarial modeling for HIGH RISK
**Limitations:**
- Did not analyze external dependencies
- Limited to 1-hop caller analysis
**Confidence:** HIGH for analyzed scope, MEDIUM overall
Tables: Use markdown tables for structured data
Code blocks: Always include syntax highlighting
// Solidity code
// Rust code
Status indicators:
Severity:
Before/After comparisons:
**BEFORE:**
```code
old code
AFTER:
new code
**Line number references:** Always include
- Format: `file.sol:L123`
- Link to commit: `file.sol:L123 (commit abc123)`
---
## File Naming and Location
**Priority order for output:**
1. Current working directory (if project repo)
2. User's Desktop or another agreed location
3. A dedicated `security-reviews/` or `docs/` subdirectory in the project (team convention)
**Filename format:**
DIFFERENTIAL_REVIEW.md
Example: VeChain_Stargate_DIFFERENTIAL_REVIEW_2025-12-26.md
---
## User Notification Template
After generating report:
```markdown
Report generated successfully!
📄 File: [filename]
📁 Location: [path]
📏 Size: XX KB
⏱️ Review Time: ~X hours
Summary:
- X findings (Y critical, Z high)
- Final recommendation: APPROVE/REJECT/CONDITIONAL
- Confidence: HIGH/MEDIUM/LOW
Next steps:
- Review findings in detail
- Address CRITICAL/HIGH issues before merge
- Consider chaining with your team's formal report workflow for stakeholders
After generating differential review, transform into audit report:
Use your team's issue or formal report workflow to transform the markdown into stakeholder-facing format if needed.
(See upstream Trail of Bits prodsec-skills for the issue-writer companion skill if you use that distribution.)
This creates polished documentation for non-technical stakeholders.
If file write fails:
Always prioritize persistent artifact generation over ephemeral chat output.
Quick reference for detecting common security issues in code changes. Examples use Python and Go to illustrate patterns, but the underlying vulnerabilities apply across a full stack — system software, OpenShift operators, Ansible modules, C/C++ daemons, Rust tooling, and beyond. Adapt examples to the language and framework of the code under review.
Domain-specific auditing skills in this repository complement the patterns below.
Pattern: Code previously removed for a security fix is re-added in a later change.
Detection:
# Search for code that was removed in a security-related commit
git log -S "pattern" --all --grep="security\|fix\|CVE"
git diff <range> | grep "^+"
Red flags:
# VULNERABLE: handler removed in commit abc123 "Fix SQL injection CVE-2024-5678"
# is re-added in current PR without the fix
@app.get("/users")
def list_users(name: str):
query = f"SELECT * FROM users WHERE name = '{name}'" # REGRESSION: raw f-string SQL
return db.execute(query).fetchall()
# SAFE: the parameterized version that the fix introduced
@app.get("/users")
def list_users(name: str):
return db.execute("SELECT * FROM users WHERE name = :n", {"n": name}).fetchall()
// VULNERABLE: endpoint removed in commit abc123 "Fix path traversal CVE-2024-9012"
// is re-added without sanitization
func serveFile(w http.ResponseWriter, r *http.Request) {
path := r.URL.Query().Get("file")
http.ServeFile(w, r, filepath.Join("/data", path)) // REGRESSION: unsanitized path
}
// SAFE: resolve the full path and verify it stays under the allowed root
func serveFile(w http.ResponseWriter, r *http.Request) {
name := filepath.Clean(r.URL.Query().Get("file"))
full := filepath.Join("/data", name)
if !strings.HasPrefix(full, "/data/") {
http.Error(w, "forbidden", http.StatusForbidden)
return
}
http.ServeFile(w, r, full)
}
Risk: A previously patched vulnerability is silently reintroduced.
Pattern: The same accounting operation (balance update, counter increment, quota deduction) is applied twice for a single logical event.
Detection:
# Look for duplicate state mutations in related functions
grep -rn "balance\|quota\|count\|credit\|debit" --include="*.py" --include="*.go"
git diff <range> | grep -E "^\+.*(balance|quota|credit|debit)"
# VULNERABLE: balance deducted in both request and processing stages
def request_withdrawal(user_id: str, amount: Decimal):
user = db.query(User).get(user_id)
user.balance -= amount # first deduction
db.commit()
withdrawal_queue.enqueue(user_id, amount)
def process_withdrawal(user_id: str, amount: Decimal):
user = db.query(User).get(user_id)
user.balance -= amount # second deduction -- BUG
db.commit()
transfer_funds(user_id, amount)
# SAFE: deduct once, mark the withdrawal as processed
def request_withdrawal(user_id: str, amount: Decimal):
user = db.query(User).get(user_id)
user.balance -= amount
db.commit()
withdrawal_queue.enqueue(user_id, amount)
def process_withdrawal(user_id: str, amount: Decimal):
# Balance already deducted during request; only execute transfer
transfer_funds(user_id, amount)
mark_complete(user_id, amount)
// VULNERABLE: inventory decremented in both reservation and fulfillment
func ReserveItem(db *sql.DB, itemID int, qty int) error {
_, err := db.Exec("UPDATE inventory SET stock = stock - $1 WHERE id = $2", qty, itemID)
return err // first decrement
}
func FulfillOrder(db *sql.DB, itemID int, qty int) error {
_, err := db.Exec("UPDATE inventory SET stock = stock - $1 WHERE id = $2", qty, itemID) // BUG: second decrement
return err
}
// SAFE: decrement once during reservation; fulfillment only ships
func FulfillOrder(db *sql.DB, itemID int, qty int) error {
_, err := db.Exec("UPDATE orders SET status = 'shipped' WHERE item_id = $1 AND qty = $2", itemID, qty)
return err
}
Risk: Users lose funds or resources twice, or the system drifts into an inconsistent state.
Pattern: Input validation or error checks are removed or absent without an equivalent replacement.
Detection:
# Removed validation keywords across languages
git diff <range> | grep "^-" | grep -E "raise|assert|validate|ValueError|HTTPException|if err|return err|http\.Error|errors\."
Questions to ask:
# VULNERABLE: validation removed from FastAPI handler
@router.post("/transfer")
def transfer(body: dict):
# No validation -- amount could be negative, zero, or absurdly large
execute_transfer(body["from_account"], body["to_account"], body["amount"])
# SAFE: Pydantic model enforces constraints
from pydantic import BaseModel, Field
class TransferRequest(BaseModel):
from_account: str = Field(min_length=1)
to_account: str = Field(min_length=1)
amount: Decimal = Field(gt=0, le=1_000_000)
@router.post("/transfer")
def transfer(body: TransferRequest):
execute_transfer(body.from_account, body.to_account, body.amount)
// VULNERABLE: no input validation in HTTP handler
func transferHandler(w http.ResponseWriter, r *http.Request) {
var req TransferRequest
json.NewDecoder(r.Body).Decode(&req) // error ignored, fields unchecked
executeTransfer(req.From, req.To, req.Amount)
}
// SAFE: validate all fields before use
func transferHandler(w http.ResponseWriter, r *http.Request) {
var req TransferRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
http.Error(w, "invalid request body", http.StatusBadRequest)
return
}
if req.Amount <= 0 || req.Amount > 1_000_000 {
http.Error(w, "amount out of range", http.StatusBadRequest)
return
}
if req.From == "" || req.To == "" {
http.Error(w, "missing account", http.StatusBadRequest)
return
}
executeTransfer(req.From, req.To, req.Amount)
}
Risk: Attackers supply malformed or malicious input that bypasses business rules.
Pattern: Arithmetic on integers that can silently wrap around or lose precision during type conversion. In languages with fixed-width integers (Go, C, Rust) values wrap or truncate; in Python, implicit int-to-float conversion loses precision for large values.
Detection:
# Narrowing conversions and casts
grep -rn "int8\|int16\|uint8\|uint16\|int32\|uint32" --include="*.go"
grep -rn "as u8\|as u16\|as i16\|as i32" --include="*.rs"
grep -rn "(int16)\|(int32)\|(uint8)" --include="*.go"
# Python: implicit float conversion or unchecked large int
grep -rn "int(.*\.\|float(\|struct\.pack\|ctypes" --include="*.py"
# VULNERABLE: large user-supplied integer silently loses precision as float
def allocate_buffer(size_str: str) -> bytearray:
size = int(size_str)
factor = 1.5
adjusted = int(size * factor) # float multiplication -- loses precision for large ints
return bytearray(adjusted)
# SAFE: keep arithmetic in integer domain and bound the input
def allocate_buffer(size_str: str) -> bytearray:
size = int(size_str)
if size <= 0 or size > 10 * 1024 * 1024:
raise ValueError("size out of allowed range")
adjusted = size + size // 2 # integer-only arithmetic
return bytearray(adjusted)
// VULNERABLE: user-controlled value converted to int16, silently wraps
func setPort(input string) (int16, error) {
v, err := strconv.Atoi(input)
if err != nil {
return 0, err
}
return int16(v), nil // 70000 wraps to 4464
}
// SAFE: validate range before narrowing conversion
func setPort(input string) (int16, error) {
v, err := strconv.Atoi(input)
if err != nil {
return 0, err
}
if v < 0 || v > math.MaxInt16 {
return 0, fmt.Errorf("port %d out of int16 range", v)
}
return int16(v), nil
}
Risk: Silent wrap-around or precision loss leads to under-allocated buffers, incorrect accounting, or bypassed limits.
Pattern: A condition is checked and then acted upon, but the underlying state can change between the check and the action. This applies to database rows, file system state, and in-memory data accessed across async or concurrent boundaries.
Detection:
# Check-then-insert patterns in ORM or SQL code
grep -rn "if.*query.*first\|if.*exists\|if not.*get" --include="*.py"
# Check-then-act on shared state without atomicity
git diff <range> | grep "^+" | grep -E "\w+\[.*\]|\bLoad\b" | grep -v "sync\.\|mu\.\|atomic\."
# File-system TOCTOU (any language)
grep -rn "os\.path\.exists\|os\.access\|os\.Stat" --include="*.py" --include="*.go"
# VULNERABLE: check-then-insert without atomicity (SQLAlchemy)
def create_user(db: Session, email: str):
existing = db.query(User).filter_by(email=email).first()
if existing is None:
# Another request can insert the same email between check and insert
db.add(User(email=email))
db.commit()
# SAFE: use a unique constraint and handle the conflict
def create_user(db: Session, email: str):
try:
db.add(User(email=email))
db.commit()
except IntegrityError:
db.rollback()
raise ValueError("email already registered")
// VULNERABLE: read-then-write on shared map without lock
var cache = make(map[string]int)
func Increment(key string) {
val := cache[key] // read
cache[key] = val + 1 // write -- race if called from multiple goroutines
}
// SAFE: protect with a mutex
var (
cache = make(map[string]int)
mu sync.Mutex
)
func Increment(key string) {
mu.Lock()
defer mu.Unlock()
cache[key]++
}
Risk: Duplicate records, corrupted state, or privilege escalation when two operations interleave.
Pattern: Authentication or authorization checks are removed, relaxed, or skipped for certain routes or operations.
Detection:
# Removed auth decorators, middleware, or guard functions
git diff <range> | grep "^-" | grep -E "Depends\(|@login_required|@requires_auth|get_current_user|Permission|middleware|AuthRequired|RequireRole|checkAuth"
Questions:
# VULNERABLE: auth dependency removed from FastAPI route
@router.put("/admin/config")
def update_config(body: ConfigUpdate):
# Anyone can call this -- Depends(get_current_admin) was removed
apply_config(body)
# SAFE: auth dependency enforced
@router.put("/admin/config")
def update_config(body: ConfigUpdate, admin: User = Depends(get_current_admin)):
apply_config(body)
// VULNERABLE: auth middleware removed from route registration
mux.HandleFunc("/admin/config", updateConfigHandler)
// SAFE: auth middleware wraps the handler
mux.Handle("/admin/config", AuthRequired(http.HandlerFunc(updateConfigHandler)))
Risk: Unauthenticated or low-privilege users access admin functionality.
Pattern: Shared mutable state accessed from multiple threads, goroutines, or async tasks without synchronization (locks, channels, atomics, or similar primitives).
Detection:
# Goroutines or threads launched near shared state
grep -rn "go func\|go .*(" --include="*.go"
grep -rn "threading\.\|asyncio\.\|concurrent\.futures" --include="*.py"
# Shared mutable globals
grep -rn "var.*=.*make\(map" --include="*.go"
# VULNERABLE: shared dict mutated from multiple async tasks
user_sessions: dict[str, int] = {}
async def track_login(user_id: str):
count = user_sessions.get(user_id, 0)
await asyncio.sleep(0) # yields control -- another task can interleave
user_sessions[user_id] = count + 1
# SAFE: use an asyncio.Lock to serialize access
session_lock = asyncio.Lock()
async def track_login(user_id: str):
async with session_lock:
user_sessions[user_id] = user_sessions.get(user_id, 0) + 1
// VULNERABLE: goroutines write to shared slice without synchronization
var results []string
func collect(items []string) {
for _, item := range items {
go func(s string) {
results = append(results, s) // data race
}(item)
}
}
// SAFE: use a mutex or channel
var (
results []string
mu sync.Mutex
)
func collect(items []string) {
var wg sync.WaitGroup
for _, item := range items {
wg.Add(1)
go func(s string) {
defer wg.Done()
mu.Lock()
results = append(results, s)
mu.Unlock()
}(item)
}
wg.Wait()
}
Risk: Data corruption, lost updates, panics, or non-deterministic behavior that is difficult to reproduce.
Pattern: Using wall-clock time for security-sensitive decisions such as token expiry, replay prevention, or rate limiting without accounting for clock skew, NTP jumps, or attacker-controlled clocks.
Detection:
# Wall-clock usage in security paths
grep -rn "time\.time()\|datetime\.now()\|datetime\.utcnow()" --include="*.py"
grep -rn "time\.Now()\|time\.Since\|time\.Until" --include="*.go"
grep -rn "System\.currentTimeMillis\|Instant\.now" --include="*.java"
# VULNERABLE: in-process rate limiter uses wall clock -- NTP jump backward
# resets the timer and allows a burst of requests
import time
last_request: dict[str, float] = {}
def is_rate_limited(user_id: str, min_interval: float = 1.0) -> bool:
now = time.time()
if now - last_request.get(user_id, 0.0) < min_interval:
return True
last_request[user_id] = now
return False
# SAFE: monotonic clock is unaffected by NTP or system clock changes
last_request: dict[str, float] = {}
def is_rate_limited(user_id: str, min_interval: float = 1.0) -> bool:
now = time.monotonic()
if now - last_request.get(user_id, 0.0) < min_interval:
return True
last_request[user_id] = now
return False
// VULNERABLE: replay window based on wall clock
func isRequestFresh(ts time.Time) bool {
return time.Since(ts) < 30*time.Second // clock skew can make stale requests appear fresh
}
// SAFE: nonce prevents replay regardless of clock accuracy;
// time.Since on a parsed timestamp is still wall-clock-based, so the
// window check is best-effort -- the nonce is the real protection.
var seen sync.Map // NOTE: in production, prune entries older than the replay window to prevent unbounded growth
func isRequestFresh(ts time.Time, nonce string) bool {
if time.Since(ts) > 30*time.Second {
return false
}
if _, loaded := seen.LoadOrStore(nonce, struct{}{}); loaded {
return false // replay detected
}
return true
}
Risk: Attackers replay expired tokens, bypass rate limits, or exploit clock skew to extend time-gated access.
Pattern: Error return values are silently discarded or exceptions are caught with a bare handler and silently suppressed, hiding failures that should abort or alter control flow.
Detection:
# Ignored error return values (Go)
grep -rn "_, _ =" --include="*.go"
grep -rn ", _ :=" --include="*.go"
# Bare except or pass-only handlers (Python)
grep -rn "except:" --include="*.py"
grep -rn "except .*:" --include="*.py" -A1 | grep "pass"
# Ignored Result (Rust)
grep -rn "let _ =" --include="*.rs"
# VULNERABLE: bare except swallows all errors including auth failures
def authenticate(token: str) -> User:
try:
return verify_and_decode(token)
except:
pass # silently returns None -- caller may treat None as "no auth required"
# SAFE: catch specific exceptions, let unexpected errors propagate
def authenticate(token: str) -> User:
try:
return verify_and_decode(token)
except (jwt.ExpiredSignatureError, jwt.InvalidTokenError) as exc:
raise HTTPException(status_code=401, detail=str(exc))
// VULNERABLE: Write and Close errors silently discarded -- partial or
// unflushed data is treated as success with no indication to the caller
func writeConfig(path string, data []byte) {
f, err := os.Create(path)
if err != nil {
return // caller receives no error; config silently not written
}
f.Write(data) // error ignored -- partial write undetected
f.Close() // error ignored -- OS buffer may not be flushed
}
// SAFE: check every error; named return allows the deferred Close error to propagate
func writeConfig(path string, data []byte) (err error) {
f, err := os.Create(path)
if err != nil {
return fmt.Errorf("create %s: %w", path, err)
}
defer func() {
if cerr := f.Close(); cerr != nil && err == nil {
err = cerr
}
}()
_, err = f.Write(data)
return
}
Risk: Silent failures lead to partial writes, data loss, authentication bypass, or inconsistent system state.
Pattern: Operations that grow without bound based on user-controlled input: unbounded queries, thread/goroutine leaks, infinite loops, or unlimited memory allocation.
Detection:
# Unbounded queries or missing LIMIT
grep -rn "\.all()\|fetchall()\|SELECT \*" --include="*.py"
grep -rn "QueryRow\|Query(" --include="*.go" | grep -v "LIMIT"
# Goroutines or threads launched without lifecycle management
grep -rn "go func\|go .*(" --include="*.go"
# Missing pagination or size limits in new code
git diff <range> | grep -E "^\+.*(\.all\(\)|fetchall|SELECT \*|go func)"
# VULNERABLE: returns entire table to the client
@router.get("/events")
def get_events(db: Session):
return db.query(Event).all() # unbounded -- millions of rows crash the service
# SAFE: enforce pagination limits
@router.get("/events")
def get_events(db: Session, offset: int = 0, limit: int = Query(default=50, le=200)):
return db.query(Event).offset(offset).limit(limit).all()
// VULNERABLE: goroutine launched per request with no cancellation
func streamHandler(w http.ResponseWriter, r *http.Request) {
go func() {
for {
data := fetchData() // runs forever if client disconnects
fmt.Fprintln(w, data)
}
}()
}
// SAFE: use a ticker so the goroutine yields between iterations instead
// of spinning at full CPU speed when fetchData() returns quickly
func streamHandler(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
ticker := time.NewTicker(1 * time.Second)
defer ticker.Stop()
go func() {
for {
select {
case <-ctx.Done():
return // client disconnected -- stop the goroutine
case <-ticker.C:
data := fetchData()
fmt.Fprintln(w, data)
}
}
}()
}
Risk: Resource exhaustion (CPU, memory, database connections, goroutines/threads) causes service outages.
Pattern: Passwords, API keys, tokens, PII, or other secrets written to application logs.
Detection:
# Logging calls that may include sensitive fields
grep -rn "logging\.\|logger\.\|log\.\|slog\.\|zap\." --include="*.py" --include="*.go" | grep -iE "password|token|secret|key|ssn|credit"
# VULNERABLE: password and token logged in plaintext
import logging
logger = logging.getLogger(__name__)
def login(username: str, password: str):
logger.info(f"Login attempt: user={username} password={password}") # password in logs
token = authenticate(username, password)
logger.debug(f"Generated token: {token}") # token in logs
return token
# SAFE: redact sensitive fields before logging
def login(username: str, password: str):
logger.info("Login attempt: user=%s", username) # no password
token = authenticate(username, password)
logger.debug("Token generated for user=%s", username) # no token value
return token
// VULNERABLE: secret logged via structured logger
func Login(username, password string) (string, error) {
slog.Info("login attempt", "user", username, "password", password) // password in logs
token, err := authenticate(username, password)
if err != nil {
return "", err
}
slog.Info("token issued", "token", token) // token in logs
return token, nil
}
// SAFE: omit sensitive fields from log entries
func Login(username, password string) (string, error) {
slog.Info("login attempt", "user", username) // no password
token, err := authenticate(username, password)
if err != nil {
return "", err
}
slog.Info("token issued", "user", username) // no token value
return token, nil
}
Risk: Secrets exposed in log aggregation systems, crash dumps, or stdout are accessible to anyone with log access and may violate compliance requirements (GDPR, PCI-DSS).
Find removed validation or error checks:
git diff <range> | grep "^-" | grep -E "raise|assert|validate|ValueError|HTTPException|if err|return err|http\.Error|errors\."
Find new external calls or HTTP clients:
git diff <range> | grep "^+" | grep -E "requests\.|httpx\.|urllib|aiohttp|http\.Get|http\.Post|http\.NewRequest|net\.Dial"
Find changed auth decorators or middleware:
git diff <range> | grep -E "^[-+].*(Depends\(|@login_required|@requires_auth|get_current_user|middleware|AuthRequired|RequireRole|checkAuth)"
Find arithmetic on user-controlled values:
git diff <range> | grep "^+" | grep -E "int\(.*\.(query|form|json|body)|float\(|strconv\.(Atoi|Parse)|int16\(|int32\("
Find logging of sensitive fields:
git diff <range> | grep "^+" | grep -iE "(log|logger|slog|zap)\b.*\b(password|token|secret|key|ssn|credit)"
For detailed analysis workflow, see Differential Review Methodology For building exploit scenarios, see Adversarial Vulnerability Analysis (Phase 5)
npx claudepluginhub redhatproductsecurity/prodsec-skills --plugin prodsec-skillsPerforms security-focused differential review of PRs, commits, and diffs with git history context, blast radius calculation, test coverage checks, and markdown report generation.
Performs security reviews of PRs, commits, and diffs using git history for context, blast radius calculation, test coverage checks, and markdown reports.
Security-focused code review for PRs, commits, and diffs. Targets auth, crypto, external calls, and value transfer with evidence-based findings and a report artifact.