--- name: security-review description: "Audits code for security vulnerabilities against OWASP Top 10 2025 with concrete fix recipes: injection, broken access control, secrets, crypto, supply chain. Use before merging auth/input/crypto/dependency changes, or when the user says security review, vulnerability, pentest, XSS, or secrets." license: MIT metadata: author: lammanhhoang version: "1.0.0" --- # Security Review Static code audit against OWASP Top 10:2025, run as a scan -> fix -> rescan loop that only terminates in one of two states: **clean** or **explicitly accepted risk**. ## When to use - Before merging changes that touch auth, sessions, input handling, crypto, file upload, deserialization, CI config, or dependencies. - When the user says: security review, audit, vulnerability, pentest, XSS, SQL injection, IDOR, CSRF, leaked key, secrets. - After any suspected secret leak (rule #29 applies immediately). When NOT to use: - Style/readability/architecture review with no security angle — do a normal code review. - Runtime penetration testing, network scanning, fuzzing, social engineering — out of scope. This skill reads code; it does not attack running systems. **Scope note:** this is code review, not a pentest. For auth-heavy, payment, healthcare, or otherwise regulated systems, state in the report that a professional security audit / pentest is required before launch. Do not present a clean code review as equivalent. ## Core principle Every finding lands in the findings table with severity + `file:line` + category + fix. Never report "probably fine", never silently skip a hit because it "looks intentional". A scan ends **clean** (zero open findings) or **accepted** (each residual finding risk-accepted by the user, recorded in the report). Nothing else. ## Workflow 1. **Scope.** Decide diff-only (`git diff main... --name-only`) vs full repo. Auth/crypto/dependency changes always widen to the files they touch plus their callers. 2. **Scan.** Run every detection grep from references/CHECKLIST.md (load it now — it is the full audit checklist with greps and fix recipes). Use `rg` (ripgrep); plain `grep -rnE` is the escape hatch. Also run: - SCA: `osv-scanner scan source -r .` (default, all ecosystems). Escape hatches: `npm audit --audit-level=high` (Node-only), `pip-audit` (Python-only). - Secrets: `gitleaks git -v .` for history and `gitleaks dir -v .` for the working tree (default). Escape hatch: `trufflehog git file://. --results=verified`. 3. **Record.** Every hit goes into the findings table (format: assets/findings-template.md). Triage each hit as finding or false positive — false positives are listed with a one-line justification, not deleted. 4. **Fix.** Apply recipes from references/CHECKLIST.md, criticals first. One fix per finding; do not batch unrelated refactors into the security pass. 5. **Rescan.** Re-run the exact same scans. New or surviving findings loop back to step 3. Repeat until clean or every residual is explicitly risk-accepted by the user. 6. **Report** using assets/findings-template.md. Include the rescan log so the loop is auditable. ## Severity rubric | Severity | Meaning | Examples | |---|---|---| | Critical | Remote compromise or live credential exposure | SQLi, RCE via deserialization/`eval`, auth bypass, leaked active secret, `alg:none` JWT | | High | Direct data exposure or account takeover path | Stored XSS, IDOR, missing authz on endpoint, MD5/SHA-only password hashing, disabled TLS verification | | Medium | Materially weakens defenses | Missing security headers, verbose errors leaking internals, unpinned deps (no known CVE), missing rate limit on login | | Low | Hardening / informational | Missing SRI on static asset, noisy logs without PII, outdated but unaffected dep | ## Rules (cite findings by number, e.g. "violates rule #13") ### A01:2025 Broken Access Control 1. Authorization MUST be enforced server-side, deny-by-default, on **every** request — a route without an auth/authz check is a finding even if "the frontend hides it". 2. Object access MUST be scoped to the requester (IDOR): any `findById(req.params.id)`-shaped lookup MUST carry an ownership/tenant filter (`WHERE owner_id = :current_user`). 3. Client-side role checks, hidden URLs, and unguessable IDs MUST NOT be the only gate. Obscurity is not authorization. Detect: `rg -n "app\.(get|post|put|patch|delete)\(" | rg -v "auth"` · `rg -n "req\.(params|query|body)\.(id|userId)"` near DB calls · `rg -n "X-Original-URL|X-Rewrite-URL"` · `rg -n "Access-Control-Allow-Origin.*\*"`. Fix: global auth middleware mounted before all routes; per-route permission check; ownership filter in the query itself, not after fetch. ### A02:2025 Security Misconfiguration 4. Production config MUST have debug off, default credentials changed, directory listing off, and security headers set (CSP, HSTS, `X-Content-Type-Options: nosniff`). 5. CORS MUST NOT combine wildcard origin with credentials; CSRF protection MUST NOT be disabled on state-changing routes. Detect: `rg -n "DEBUG\s*=\s*True|NODE_ENV.*development"` in deploy config · `rg -n "csrf.*(disable|false)" -i` · `rg -n "credentials:\s*true" -A3 | rg "\*"` · `rg -n "admin.*admin|root.*root" -i` in config. Fix: environment-split config, secrets from env, `helmet()` (Express) / `SecurityMiddleware` (Django) defaults, explicit CORS allowlist. ### A03:2025 Software Supply Chain Failures 6. A lockfile (`package-lock.json`, `poetry.lock`, `Cargo.lock`, `go.sum`) MUST be committed; floating versions (`latest`, bare `*`) are findings. 7. An SCA scan (`osv-scanner` default) MUST run in CI and fail the build on high/critical vulns. 8. CI steps MUST be pinned to immutable revisions: GitHub Actions by full commit SHA, never `@main`; no `curl | bash` installs of unverified scripts. Detect: lockfile missing from repo root · `rg -n '"latest"|\"\*\"' package.json` · `rg -n "uses:.*@(main|master)$" .github/workflows/` · `rg -n "curl.*\|\s*(bash|sh)"`. Fix: commit lockfile, pin `uses: owner/action@<40-char-sha>`, add osv-scanner CI step with non-zero exit on findings. ### A04:2025 Cryptographic Failures 9. Passwords MUST be hashed with **Argon2id** (default: m=19456 KiB, t=2, p=1) or **bcrypt** (work factor >= 10) — MD5/SHA-1/SHA-256 for passwords is a critical finding, salted or not. 10. Security tokens MUST come from a CSPRNG (`crypto.randomBytes`, Python `secrets`) — `Math.random()`/`random.random()` for anything security-relevant is a finding. 11. TLS certificate verification MUST NOT be disabled (`rejectUnauthorized: false`, `verify=False`, `InsecureSkipVerify: true`); sensitive endpoints MUST be HTTPS. 12. Symmetric encryption MUST use an AEAD mode (AES-256-GCM default); ECB mode, hardcoded keys, and static IVs are findings. Detect: `rg -n "md5|sha1|createHash\('sha256'\).*password" -i` · `rg -n "Math\.random|random\.random" ` near token/session code · `rg -n "rejectUnauthorized:\s*false|verify\s*=\s*False|InsecureSkipVerify"` · `rg -n "ECB|createCipher\("`. Fix: swap hash to Argon2id and rehash-on-login for legacy rows; `crypto.randomBytes(32).toString('hex')`; delete verification bypasses; AES-256-GCM with random 12-byte nonce per message. ### A05:2025 Injection 13. SQL MUST use parameterized queries ONLY. String-built SQL (`+`, template literal, f-string, `%` format into a query) is a finding — **no exceptions**, including "the value is a constant today". 14. HTML output MUST use context-aware encoding via the framework's auto-escaping; `innerHTML`/`dangerouslySetInnerHTML`/`v-html`/`| safe` with user-influenced data requires sanitization (DOMPurify default) or is a finding. 15. OS commands MUST use arg-array APIs (`execFile`/`spawn`, `subprocess.run([...], shell=False)`); `shell=True`/`child_process.exec` with any user-influenced input is a finding. Same for `eval`/`Function`/`pickle` on user data. 16. Input validation MUST be server-side allowlist (type, length, range, pattern) — client-side validation is UX, not security. Detect: `rg -n "(SELECT|INSERT|UPDATE|DELETE).*(\\$\{|\+ |f\"|% ?\()" -i` · `rg -n "innerHTML|dangerouslySetInnerHTML|v-html|\| ?safe"` · `rg -n "eval\(|new Function\(|child_process\.exec\(|os\.system|shell\s*=\s*True"` · `rg -n "\$where"`. Fix: recipes with before/after code in references/CHECKLIST.md (load when fixing any injection finding). ### A06:2025 Insecure Design 17. Login, OTP, password-reset, and signup endpoints MUST be rate-limited server-side (e.g. `express-rate-limit`, `django-ratelimit`) with lockout/backoff. 18. Business limits (quantity, price, quota, state transitions) MUST be enforced server-side; a price or total accepted from the client is a finding. Detect: `rg -ln "login|reset|otp" --iglob "*route*"` then check those files for limiter middleware · `rg -n "(price|total|amount).*req\.body" -i`. Fix: rate limiter on auth routes; recompute money/quota values server-side from canonical data. ### A07:2025 Authentication Failures 19. The session ID MUST be regenerated on login and privilege change (`req.session.regenerate` / Django does this via `login()`); session fixation otherwise. 20. Session cookies MUST set `Secure` + `HttpOnly` + `SameSite` (Lax minimum; Strict for high-value actions). 21. JWTs MUST be verified for signature **and** `exp`, with an explicit algorithm allowlist that rejects `none`; access tokens SHOULD live <= 15 min with rotating refresh tokens. `jwt.decode()` used as verification is a critical finding. Detect: `rg -n "jwt\.decode\(|verify:\s*false|algorithms?" ` · `rg -n "httpOnly|sameSite|secure" -i` in session config (absence is the finding) · `rg -n "session\.regenerate|login\(request" ` (absence near login handlers). Fix: exact JWT/cookie/session snippets in references/CHECKLIST.md. ### A08:2025 Software or Data Integrity Failures 22. Untrusted data MUST NOT reach native deserializers: `pickle.loads`, `yaml.load` (without `SafeLoader`), Java `ObjectInputStream.readObject`, PHP `unserialize` — each is a critical finding on external input. Use JSON + schema validation. 23. Third-party `