security-review

Files (4)

• SKILL.md
• DETECTION-PLAYBOOK.md · references/
• QUALITY-FILTER.md · references/
• REVIEW-VOICE.md · references/

SKILL.md (first lines)

# Security Review

A specialised lens on [`pr-review`](https://github.com/priyank766/OpenSource-SKILL/blob/main/skills/pr-review/SKILL.md): the same diff, read only for the
question *what can an attacker make this code do?*

**Read first:** [`DETECTION-PLAYBOOK.md`](references/DETECTION-PLAYBOOK.md) ·
[`QUALITY-FILTER.md`](references/QUALITY-FILTER.md) ·
[`REVIEW-VOICE.md`](references/REVIEW-VOICE.md)

> **Where the time goes.** Finding the flaw is the review. The playbook above is the
> general flaw-hunting procedure; this file is the domain-specific lens on top of it.
> Scoring and writing the comment are the last ten minutes, not the work.

**Severity floor: 7.5** — lower than the suite default of 8.0. The cost of a miss is
asymmetric. A false negative ships a CVE that outlives the release and lands on the
maintainer as an unplanned patch cycle. A false positive costs the author five minutes to
read and dismiss. When the two errors are that unequally priced, you lower the floor.

The floor does not license volume. Everything under 9.0 is still batched into one comment.

---

## Why this exists

Security findings are where generic AI review is at its worst. It flags `eval` in a comment
string, misses the `os.path.join` two lines below that accepts a user-supplied filename, and
then posts both with identical confidence. Maintainers learn to skim security feedback,
which is exactly the feedback they cannot afford to skim.

This skill trades breadth for a traceable argument: source → sink → why the sanitiser in
between does not hold.

### What each role gets out of it

| Role | Time this gives back |
| :--- | :--- |
| **Maintainer** | The security pass you keep deferring, done in one read. Each finding arrives with the taint path already traced, so you spend your time on the fix, not on reproducing the reviewer's reasoning. Non-exploitable hardening arrives as one batched comment instead of eleven. |
| **Contributor** | A pre-submit check on the classes maintainers actually reject for — shell interpolation, path joins, deserialization — so review is one round instead of three. |
| **Project owner** | Exploitable findings never land in a public thread by accident. The skill checks for `SECURITY.md` and private reporting *before* drafting, so disclosure policy is enforced by procedure rather than by memory. |

---

## Procedure

```mermaid
flowchart TD
    A[Start] --> B[1. Scope the diff]
    B --> C[2. Establish the trust boundary]
    C --> D[3. Check the disclosure policy]
    D --> E[4. Secret scan: diff and history]
    E --> F[5. Sink sweep + taint trace]
    F --> G[6. Vulnerability class checklist]
    G --> H[7. Read existing comments]
    H --> I[8. Score against the 7.5 floor]
    I -->|"score below 7.5"| J[Discard silently]
    I -->|"score 7.5 to 9.0"| K[Batch into one hardening comment]
    I -->|"score above 9.0"| L{Exploitable in a release?}
    L -->|"yes"| M[Private disclosure channel]
    L -->|"no, PR branch only"| N[Inline comment, minimal detail]
    M --> O[9. Approval, then send]
    N --> O
    K --> O
```

### 1. Scope the diff

```bash
gh pr checkout <PR>
git diff upstream/main...HEAD                      # three dots, always
git diff --stat upstream/main...HEAD               # sets the comment budget
git diff --name-only upstream/main...HEAD | grep -Ei \
  'auth|login|session|token|crypt|password|perm|acl|upload|parse|exec|shell|path|url'
```

Three dots. Two dots pulls in everything merged to `main` since the branch point, and you
will spend an hour auditing code the contributor never touched.

### 2. Establish the trust boundary

Before reading a single line for bugs, write down in one sentence: *where does data from
outside this process enter the changed code?* HTTP handler, CLI arg, env var, config file,
message queue, webhook, uploaded archive, database row written by another tenant. If you
cannot name the entry point, you cannot classify the finding, and an unclassified security
comment is noise.

### 3. Check the disclosure policy — before you write anything

```bash
ls SECURITY.md .github/SECURITY.md docs/SECURITY.md 2>/dev/null
gh api /repos/<OWNER>/<REPO>/private-vulnerability-reporting --jq '.enabled'
gh api /repos/<OWNER>/<REPO> --jq '.security_and_analysis'   # null unless you have admin
```

`.enabled == true` from `/private-vulnerability-reporting` means private vulnerability
reporting is on and you have a channel. Note that a `200` from `/security-advisories` proves
nothing — that endpoint answers for any repo you can read, empty list included. `false` (or
an error) plus no `SECURITY.md` means you must ask the maintainer where to send it — not
post it. See
[Responsible disclosure](#responsible-disclosure) below; that section overrides everything
else in this file.

### 4. Secret scan — diff and history

A secret removed in a later commit is still in the objects and still compromised.

```bash
# added lines only
git diff upstream/main...HEAD -U0 | grep -E '^\+' | grep -Ei \
  '(api[_-]?key|secret|passwd|password|token|private[_-]key|BEGIN [A-Z ]*PRIVATE KEY|AKIA[0-9A-Z]{16}|gh[pousr]_[A-Za-z0-9]{36}|xox[baprs]-)'

# was it ever committed, even if deleted since?
git log -p -S 'AKIA' --oneline -- .
git log -p -S 'BEGIN RSA PRIVATE KEY' --oneline -- .

# fixtures and .env are the usual leak sites
git diff --name-only upstream/main...HEAD | grep -Ei '\.env|secret|credential|\.pem|\.p12'
```

If a live credential is in history, that is a disclosure-channel finding and a rotation

From priyank766/OpenSource-SKILL, Apache-2.0.

More from priyank766/OpenSource-SKILL