secure-code-review · diff
v1.0.0 to v1.1.0
172 added, 4 removed. Audit B to A.
---
+ name: secure-code-review
id: secure-code-review
- version: "1.0.0"
+ version: "1.1.0"
title: "Secure Code Review"
type: skill
description: "Security-focused code review pattern covering OWASP Top 10 vulnerabilities and common security issues"
status: experimental
owners:
- "@GSA-TTS/agentic-coding-team"
primary_personas:
- developers
- security
requires:
anchors: []
output:
format: markdown
contract:
required_sections:
- "Summary"
- "Findings"
prohibited_content:
+ - "Real Secrets"
- "Secrets"
- "Real PII"
- "Real CUI"
- "Internal URLs"
- "Actual Exploit Code"
quality_gates:
readability_max_grade: 10
citations_required: false
triggers:
- "code review"
- "security"
- "vulnerability"
- "OWASP"
tags:
- "security"
- "review"
- "owasp"
- "vulnerabilities"
categories:
- "security"
- "review"
+ compliance:
+ frameworks:
+ - "NIST SP 800-53"
+ - "OWASP Top 10 LLM 2025"
+ nist_controls:
+ - "AC-6"
+
+ changelog:
+ - version: "1.1.0"
+ date: "2026-07-02"
+ change_type: minor
+ summary: "Add diff modality + IaC and changed-workflow blast-radius sub-sections with flag-and-defer handoffs to sibling skills; extend secrets coverage to non-code surfaces (CI logs, agent reports, git history, base64/binary), placeholder-vs-real discrimination, and rotation. Absorbs proposed security-diff-review and secrets-handling-review."
+ - version: "1.0.0"
+ date: "2026-05-20"
+ change_type: major
+ summary: "Initial release."
+
risk_tier: moderate
human_review_required: true
allowed_tools: []
network_policy: deny
write_policy: deny
script_policy: deny
portability:
opencode: true
cursor: true
claude_projects: true
chatgpt: true
generic_llm: true
scope:
intended_use:
- "Review code changes for security vulnerabilities"
- "Identify OWASP Top 10 risks"
- "Provide remediation guidance"
+ - "Review a whole file or a PR diff hunk (diff is a modality)"
+ - "Review Infrastructure-as-Code (IaC) for insecure configuration"
+ - "Assess blast radius of a changed CI workflow file in a diff"
+ - "Detect exposed secrets in code and non-code surfaces"
exclusions:
- "Not for compliance auditing"
- "Not for penetration testing"
- "Does not replace automated security scanners"
+ - "Agent-invoking or injection-reachable CI triggers: defer to agentic-actions-auditor"
+ - "Scope-minimality judgments (GITHUB_TOKEN / IAM scope): defer to least-privilege-review"
+ - "Supply-chain / dependency intake: defer to dependency-analysis"
---
# Skill: Secure Code Review
Perform security-focused code review to identify common vulnerabilities and security issues before they reach production.
+ > **Diff is a modality, not a mode.** You can review either a whole file or a
+ > single PR diff hunk. Every check below applies to both. When you review a
+ > diff, focus on what the change *introduces* or *removes* — you do not need to
+ > re-audit unchanged lines, but you should flag when a change weakens an
+ > existing control.
+
+ ## Boundaries
+
+ This skill is the general **code-review hygiene** lane. Three neighbors own
+ adjacent lanes — flag and defer, do not re-implement them:
+
+ | If you see... | Defer to |
+ |---------------|----------|
+ | An agent-invoking or injection-reachable CI trigger (`pull_request_target`, `issue_comment`, `workflow_run` with untrusted checkout, a prompt built from PR/issue text) | **agentic-actions-auditor** |
+ | A judgment about whether a granted scope is *minimal* (GITHUB_TOKEN permissions, IAM scope) | **least-privilege-review** |
+ | Supply-chain or dependency intake (new packages, CVEs, licenses) | **dependency-analysis** |
+
+ secure-code-review's lane for CI is **general workflow-file code-review hygiene
+ plus the blast radius of the changed workflow in the diff** — not a full agent
+ or least-privilege audit.
+
## When to Use
- Reviewing pull requests or code changes
- Pre-deployment security checks
- Identifying injection vulnerabilities
- Validating authentication and authorization logic
- Checking for insecure configurations
- User asks "is this code secure?" or "check for vulnerabilities"
## Prerequisites
- Access to codebase
- Understanding of OWASP Top 10
- Familiarity with the programming language being reviewed
- Knowledge of secure coding practices
## Procedure
### Step 1: Input Validation Review
Check all external inputs (user input, API calls, file reads, environment variables):
**What to look for:**
- [ ] All inputs are validated against allowlists (not denylists)
- [ ] Input length limits enforced
- [ ] Special characters handled safely
- [ ] Type checking performed
**Common issues:**
```python
# ❌ Vulnerable - no validation
user_input = request.GET['id']
query = f"SELECT * FROM users WHERE id = {user_input}"
# ✅ Secure - parameterized query
user_id = int(request.GET['id']) # Type check
query = "SELECT * FROM users WHERE id = ?"
cursor.execute(query, (user_id,))
```
### Step 2: Injection Vulnerability Check
**SQL Injection:**
- [ ] No string concatenation in SQL queries
- [ ] Parameterized queries or ORM used
- [ ] Input sanitized before database operations
**Command Injection:**
- [ ] No shell execution with user input
- [ ] If shell needed, use safe APIs (e.g., `subprocess` with list args, not shell=True)
**XSS (Cross-Site Scripting):**
- [ ] Output encoding based on context (HTML, JavaScript, CSS, URL)
- [ ] Content Security Policy headers set
- [ ] User input not rendered as HTML without sanitization
### Step 3: Authentication and Authorization
**Authentication:**
- [ ] Passwords never stored in plaintext
- [ ] Strong password hashing (bcrypt, Argon2, scrypt)
- [ ] Multi-factor authentication supported
- [ ] Session tokens cryptographically random
- [ ] Tokens expire appropriately
**Authorization:**
- [ ] Access controls enforced server-side
- [ ] Principle of least privilege applied
- [ ] Authorization checked on every request
- [ ] No client-side-only authorization
```python
# ❌ Client-side only
if user.is_admin: # Trust client data
return admin_panel()
# ✅ Server-side enforcement
@require_role('admin')
def admin_panel(request):
# Check on every request
pass
```
### Step 4: Sensitive Data Handling
+ Secrets and sensitive data leak far beyond source code. Check every surface a
+ value can land on.
+
+ **In code:**
+
- [ ] No secrets in code or comments
- [ ] PII encrypted at rest and in transit
- [ ] Sensitive data not logged
- [ ] Secrets loaded from environment or secret manager
- [ ] API keys not in URLs (query parameters)
+ **Non-code surfaces (often missed):**
+
+ - [ ] CI logs — no token, key, or connection string echoed into build output
+ - [ ] LLM prompts — no secret pasted into a prompt sent to a model
+ - [ ] Agent summaries / reports (markdown) — no secret copied into a generated
+ report, review, or PR comment
+ - [ ] Shell history — no secret passed as a command-line argument
+ - [ ] Git history — a secret removed in the latest commit may still live in an
+ earlier commit
+
+ **Placeholder vs. real (do not be fooled by a label):**
+
+ Judge a value by its **shape, entropy, and context**, not by its name.
+
+ > **Anti-bypass note.** An attacker can *name* a real secret `EXAMPLE_TOKEN`,
+ > `TEST_KEY`, or `dummy_password` to slip it past a naive filter. A "test" or
+ > "example" label is **not** proof the value is fake. A short, low-entropy
+ > value like `xxxxx` or `changeme` is a placeholder; a long high-entropy value
+ > that matches a known key format is a real secret regardless of its name.
+
+ **Encoded and generated forms:**
+
+ - [ ] Base64 / hex / binary blobs decoded and checked (a secret may hide inside
+ an encoded string)
+ - [ ] Agent-generated markdown reports scanned the same way as code
+
+ **If a real secret was exposed — rotation, not deletion:**
+
+ Removing a leaked secret from a file does **not** un-leak it. Assume any secret
+ that reached a shared surface (a commit, a CI log, a report) is **compromised**.
+
+ - [ ] Rotate the credential at the issuer (revoke old, issue new)
+ - [ ] Remove it from the current file *and* from history if feasible
+ - [ ] Note in the finding that removal alone is insufficient
+
+ ### Step 4a: Infrastructure-as-Code (IaC) Review
+
+ Applies to Terraform, CloudFormation, and Kubernetes manifests. Review them the
+ same way you review code — the diff modality applies here too.
+
+ **What to look for:**
+
+ - [ ] No hardcoded secrets (passwords, keys, tokens) in `.tf`, templates, or
+ manifests — use a secret manager reference instead
+ - [ ] No over-permissive IAM / RBAC (wildcard `Action: "*"`,
+ `Resource: "*"`, cluster-admin bindings) — see least-privilege-review for
+ the *minimality* judgment
+ - [ ] No unintended public exposure (`0.0.0.0/0` ingress, public S3 buckets,
+ `LoadBalancer` on an internal service)
+ - [ ] Encryption not disabled (at rest and in transit)
+ - [ ] Destructive / blast-radius changes flagged (deleting a database, replacing
+ a resource that forces recreation, widening a security group)
+
+ ```hcl
+ # ❌ Over-permissive + hardcoded secret (synthetic)
+ resource "aws_iam_policy" "app" {
+ policy = jsonencode({ Statement = [{ Effect = "Allow", Action = "*", Resource = "*" }] })
+ }
+ resource "aws_db_instance" "app" {
+ password = "…" # FLAG: inline literal secret (redacted); use a secret ref
+ publicly_accessible = true # public exposure
+ }
+ ```
+
+ > **Future-split note.** IaC review may graduate into its own skill later. For
+ > now it lives here as a sub-section; keep IaC findings clearly labeled so the
+ > split is clean.
+
+ ### Step 4b: Changed-Workflow Blast-Radius Review
+
+ Applies to **CI workflow files that appear in a diff** (e.g. GitHub Actions
+ `.github/workflows/*.yml`). Your job is narrow: **what does this change
+ introduce, and what is the blast radius?**
+
+ Ask of the *changed* workflow:
+
+ - [ ] What new **permissions** does it grant?
+ - [ ] What new **secrets** does it read or expose?
+ - [ ] What new **triggers** does it add, and what can reach them?
+ - [ ] Does it echo a secret into logs, or pass one to an untrusted step?
+
+ **Critical boundary — flag and defer, do not fully audit here:**
+
+ - If the change adds an **agent-invoking or injection-reachable trigger**
+ (`pull_request_target`, `issue_comment`, `workflow_run` with an untrusted
+ checkout, or a prompt built from PR/issue text) → **FLAG it and DEFER to
+ agentic-actions-auditor.** Do not attempt to fully audit the trigger here.
+ - If the question is whether the granted **scope is minimal** (which
+ `GITHUB_TOKEN` permissions, which IAM actions) → **DEFER to
+ least-privilege-review.**
+
+ You still report the *blast radius* of the changed workflow (what it can now
+ touch), you just do not own the agent-injection or minimality verdicts.
+
+ ```yaml
+ # Changed workflow (synthetic diff) — FLAG + DEFER, do not audit the trigger here
+ on:
+ pull_request_target: # injection-reachable → defer to agentic-actions-auditor
+ types: [opened]
+ permissions:
+ contents: write # new write scope → defer minimality to least-privilege-review
+ ```
+
+ Example finding phrasing:
+
+ > This change adds a `pull_request_target` trigger and `contents: write`. The
+ > blast radius is any push to the default branch. This trigger is
+ > injection-reachable — **flagging and deferring to agentic-actions-auditor**
+ > for the full agent-trigger audit, and to least-privilege-review for whether
+ > `contents: write` is minimal. This skill does not audit the trigger itself.
+
### Step 5: Cryptography Review
- [ ] No custom crypto implementations
- [ ] Approved algorithms only (AES-256, RSA-2048+, SHA-256+)
- [ ] TLS 1.2+ for network communication
- [ ] Certificate validation not disabled
- [ ] Random values use cryptographically secure RNG
### Step 6: Error Handling and Logging
- [ ] Errors handled explicitly (no empty catch blocks)
- [ ] Stack traces not exposed to users
- [ ] Sensitive data not logged
- [ ] Error messages don't leak system details
- [ ] Security events logged for audit
### Step 7: Configuration and Deployment
- [ ] Security headers set (CSP, HSTS, X-Frame-Options, etc.)
- [ ] Debug mode disabled in production
- [ ] Default credentials changed
- [ ] Unnecessary features/endpoints disabled
- [ ] CORS configured restrictively
## Verification
After review, confirm:
- [ ] All OWASP Top 10 risks checked
- [ ] Injection vulnerabilities identified or ruled out
- [ ] Authentication/authorization logic verified
- - [ ] Sensitive data handling reviewed
+ - [ ] Sensitive data handling reviewed across code and non-code surfaces
+ - [ ] IaC config reviewed if manifests are in scope
+ - [ ] Changed CI workflows: blast radius reported; agent triggers and scope-minimality deferred to siblings
- [ ] Findings documented with severity and remediation
- [ ] No false positives that would block legitimate code
## Examples
### Example 1: SQL Injection Detection
**Code:**
```python
def get_user(username):
query = "SELECT * FROM users WHERE name = '" + username + "'"
return db.execute(query)
```
**Finding:**
- **Issue:** SQL injection vulnerability
- **Severity:** High
- **Impact:** Attacker can read/modify database
- **Remediation:** Use parameterized query:
```python
def get_user(username):
query = "SELECT * FROM users WHERE name = ?"
return db.execute(query, (username,))
```
### Example 2: XSS Prevention
**Code:**
```javascript
document.getElementById('message').innerHTML = userInput;
```
**Finding:**
- **Issue:** XSS vulnerability
- **Severity:** High
- **Impact:** Script injection, session hijacking
- **Remediation:** Use textContent or sanitize:
```javascript
document.getElementById('message').textContent = userInput;
// Or use DOMPurify if HTML needed
```
## Troubleshooting
| Issue | Cause | Solution |
|-------|-------|----------|
| Too many false positives | Overly aggressive checks | Focus on high/critical severity issues first |
| Unclear remediation | Generic security advice | Provide specific code examples |
| Missed vulnerabilities | Limited scope | Use automated scanners as complement |
## Related Patterns
- - [dependency-analysis](../dependency-analysis/SKILL.md) - Check for vulnerable dependencies
+ - [dependency-analysis](../dependency-analysis/SKILL.md) - Supply-chain and vulnerable-dependency intake
+ - [agentic-actions-auditor](../agentic-actions-auditor/SKILL.md) - Agent-invoking / injection-reachable CI triggers
+ - [least-privilege-review](../least-privilege-review/SKILL.md) - Whether a granted scope (token/IAM) is minimal
- [test-generation](../test-generation/SKILL.md) - Generate security test cases
## References
- [OWASP Top 10](https://owasp.org/www-project-top-ten/)
+ - [OWASP Top 10 for LLM Applications](https://genai.owasp.org/)
- [OWASP Cheat Sheet Series](https://cheatsheetseries.owasp.org/)
- - For compliance controls, see [agentic-coding-playbook SECURITY-CONTROLS.md](https://github.com/GSA-TTS/agentic-coding-playbook/blob/main/docs/SECURITY-CONTROLS.md)
+ - For compliance controls, see your organization's playbook / `SECURITY-CONTROLS.md` (federal teams: NIST SP 800-53, e.g. AC-6 least privilege).