---
name: secure-code-review
id: secure-code-review
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 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) - 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 your organization's playbook / `SECURITY-CONTROLS.md` (federal teams: NIST SP 800-53, e.g. AC-6 least privilege).