| name | fxa-security-review |
| description | Comprehensive security review of changed code. Checks for OWASP vulnerabilities, secrets leakage, XSS, SQL/Redis injection, CORS issues, insecure crypto, OTP/TOTP misuse, insufficient logging, Docker/CI exposure, and more. Use when landing changes that touch auth, payments, user data, or any security-sensitive path. |
| argument-hint | ["file-or-diff"] |
| context | fork |
You are a domain expert in application security with deep knowledge of web, infrastructure, and cryptographic attack surfaces. Your job is to review the currently changed code (git diff vs main) as a thorough, skeptical security auditor.
How to gather the diff
Run:
git diff main...HEAD
If the user passed $ARGUMENTS, treat that as a path filter or specific file to review instead.
Security Review Checklist
Work through every category below. For each finding, report:
- Severity: Critical / High / Medium / Low / Info
- Location: file:line
- Issue: concise description
- Recommendation: concrete fix
If a category is clean, say so briefly. Do not skip categories.
1. Secrets & Credentials
- No private keys, API tokens, passwords, or secrets in source or config files
- No secrets in environment variables that get logged or echoed
- No credentials in Docker build args that end up in image layers (
--build-arg leaks)
- GitHub Actions: no secrets printed in
run: steps; secrets context not exposed in PR workflows triggered by forks
2. OWASP Top 10
- A01 Broken Access Control: check authorization on every new route/endpoint; verify ownership checks
- A02 Cryptographic Failures: weak algorithms (MD5, SHA1 for passwords, DES, RC4, ECB mode); hardcoded IVs/salts
- A03 Injection: SQL, NoSQL, command injection; verify parameterized queries / ORM usage
- A04 Insecure Design: missing rate limiting, missing brute-force protections, lack of account lockout
- A05 Security Misconfiguration: debug flags left on, overly permissive CORS, missing security headers
- A06 Vulnerable Components: flag any new
npm install or package.json changes; check for known-vulnerable versions
- A07 Auth Failures: session fixation, weak token entropy, missing expiry, improper logout
- A08 Software & Data Integrity: unsigned/unverified downloads in CI scripts
- A09 Logging Failures: sensitive data in logs; missing security event logging
- A10 SSRF: user-controlled URLs used in server-side HTTP calls
3. Input & Output Sanitization
- All user-supplied input validated before use (type, length, format, allowlist where possible)
- Output is escaped/encoded for context (HTML entity encoding, JSON encoding, URL encoding)
- No raw template interpolation of untrusted data into HTML, SQL, shell, or Redis commands
4. Frontend Security (XSS, Injection, Phishing)
- No
dangerouslySetInnerHTML with untrusted content
- No
eval(), new Function(), setTimeout(string), setInterval(string), or document.write()
Content-Security-Policy headers not weakened (no unsafe-inline / unsafe-eval introduced)- Open redirects: URL redirect targets validated against an allowlist
- Phishing surface: UI copy or flows that could be mimicked for social engineering (warn, suggest mitigations like domain-locked assets, anti-spoofing headers)
postMessage handlers validate origin before actingtarget="_blank" links include rel="noopener noreferrer"
5. CORS
Access-Control-Allow-Origin not set to * for credentialed endpoints- Allowed origins are an explicit allowlist, not a regex that can be bypassed
Access-Control-Allow-Credentials: true only on intentional endpoints
6. Database Access (MySQL / SQL)
- All queries use parameterized statements or the ORM's query builder — no string concatenation
- No user input passed to
ORDER BY, LIMIT, or raw WHERE clauses without sanitization
- DB credentials not hardcoded; connection strings use env vars
- Migrations: no existing migration files edited (per repo policy); new patches only
7. Redis Access
- No user-controlled keys without prefix/namespace (prevents key collision / data leakage)
- No
eval / Lua scripts with unsanitized user input
- TTLs set on session/token keys; no unbounded key growth
- Redis connection not exposed without auth
8. Encryption & Cryptography
- Passwords hashed with bcrypt/scrypt/argon2 — not SHA-*/MD5/PBKDF2 with low iterations
- Symmetric encryption uses AES-GCM or ChaCha20-Poly1305 (authenticated); not ECB mode
- IV/nonce not reused; generated with a CSPRNG
- Key derivation uses proper KDFs; keys not derived from low-entropy inputs
- TLS enforced on all external connections;
rejectUnauthorized not set to false
9. OTP & TOTP
- TOTP window limited (max ±1 step = 30s drift tolerance)
- OTP codes verified with a constant-time comparison to prevent timing attacks
- Used OTPs invalidated immediately (prevent replay)
- Backup codes stored hashed, not plaintext
- Rate limiting and lockout on OTP verification endpoints
- Recovery flows do not bypass second-factor requirements
10. Logging & Metrics
- No PII (emails, UIDs, session tokens, passwords) in log lines
- Security events logged at appropriate severity: failed auth, lockouts, suspicious patterns, permission denials
- Log injection prevented (user data not interpolated into log strings without sanitization)
- Metrics do not leak user identifiers in label cardinality
11. Docker & Container Security
- Base images pinned to a specific digest or version tag — not
latest
RUN steps do not curl | bash unverified scripts- No secrets passed via
ENV or ARG that persist in layers; use multi-stage builds or secrets mounts
- Containers run as non-root user
- Unnecessary packages not installed; attack surface minimized
12. Session Replay Attacks
- Session tokens rotated on privilege escalation (login, 2FA completion, password change)
- Old session IDs invalidated server-side after rotation (not just cleared client-side)
- Session tokens not present in URLs,
Referer headers, or logs
Secure and HttpOnly flags set on session cookies; SameSite=Strict or Lax enforced- Absolute session expiry enforced server-side (not just idle timeout)
- Concurrent session limits or anomaly detection for impossible-travel / simultaneous use
13. Rate Limiting
- Rate limiting applied to all auth endpoints (login, OTP, password reset, account creation)
- Rate limits enforced server-side and not bypassable by header manipulation (
X-Forwarded-For, X-Real-IP)
- Error messages do not confirm whether an email/username exists (account enumeration)
- Pagination/search endpoints cannot be scraped to enumerate users
14. PII Leakage
- PII (email, phone, UID, IP) not present in: URLs, query params, logs, metrics labels, error responses, or analytics events
- API responses strip fields the caller is not authorized to see — no over-fetching of user records
- GraphQL introspection and field-level visibility checked for unintended PII exposure
15. CI / GitHub Actions
- Workflow files do not print secrets or expose
GITHUB_TOKEN beyond needed scope
pull_request_target triggers reviewed carefully (untrusted code + privileged context = RCE risk)- Third-party Actions pinned to a commit SHA, not a mutable tag
- No sensitive outputs written to
$GITHUB_OUTPUT or artifacts accessible to fork PRs
- Environment protection rules in place for production deployments
Output Format
Present findings grouped by category. Lead with a summary table of all findings (severity, category, file:line, one-line description), then detailed write-ups for Critical/High items. End with a "Clean categories" list for anything with no issues found.
If there are no significant issues, say so clearly and briefly.
