diff --git a/CHANGELOG.md b/CHANGELOG.md index 29bdd3c..ebd729e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Dedicated security-review step in `itkdev-review-php` and + `itkdev-review-javascript`, mirroring the one `itkdev-review-python` already + had: ten language-specific categories, a per-finding format (severity, + location, vulnerability type, description, proof of concept, remediation), + and a closing prioritized remediation plan. Skill descriptions updated to + advertise it. +- Drupal deep checks in `itkdev-review-php` (`\Drupal::` static calls in + services, access checks on routes/entities, render-array escaping and `t()` + placeholders, database API placeholders, deprecated Drupal 10/11 APIs, + business logic in hooks), with a soft reference to the `itkdev-drupal` skill + for deeper checks. Framework list reordered to Symfony, Drupal, Laravel, + plain PHP to match how ITK Dev actually works. +- Python, YAML, and Shell rows in the `itkdev-review-comments` comment-syntax + table. +- Security parity in `itkdev-review-python` itself: XSS (`mark_safe`, Jinja2 + autoescape), CSRF (`@csrf_exempt`), template injection (SSTI), and + weak-randomness/crypto checks in the Critical checklist; HTTP-timeout and + `assert`-for-validation deep checks; quick-reference rows for weak tokens, + SSTI, and missing timeouts. + +### Changed + +- The `itkdev-code-review` agent now defines the explicit severity mapping from + the review skills' four tiers to the report's three (Critical → Critical, + High → Warning, Medium/Low → Suggestion), and only flags `error_log` / + `console.warn` as debug code when they are clearly leftover debugging rather + than deliberate logging. +- Tone Guidelines, severity headers, and the output-format template are now + word-for-word identical across the three language review skills, so future + drift shows up in diffs. + ### Changed - **BREAKING:** The single `itkdev-skills` plugin has been split into three diff --git a/plugins/itkdev-code-quality-and-review/.claude-plugin/plugin.json b/plugins/itkdev-code-quality-and-review/.claude-plugin/plugin.json index f3705f3..cff02dd 100644 --- a/plugins/itkdev-code-quality-and-review/.claude-plugin/plugin.json +++ b/plugins/itkdev-code-quality-and-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "itkdev-code-quality-and-review", "description": "ITK Dev code quality and review: code review agent and per-language review skills (PHP, Python, JavaScript), comment review, and standards validation", - "version": "0.1.1", + "version": "0.2.0", "author": { "name": "ITK Dev", "email": "itkdev@mkb.aarhus.dk" diff --git a/plugins/itkdev-code-quality-and-review/agents/itkdev-code-review.md b/plugins/itkdev-code-quality-and-review/agents/itkdev-code-review.md index 7ba44da..678b49a 100644 --- a/plugins/itkdev-code-quality-and-review/agents/itkdev-code-review.md +++ b/plugins/itkdev-code-quality-and-review/agents/itkdev-code-review.md @@ -90,9 +90,11 @@ Apply Drupal-specific checks only when a Drupal project is detected. ### General Checks (all projects) **Debug code** — Flag any leftover debugging statements: -- PHP: `var_dump`, `print_r`, `dd()`, `dump()`, `dpm()`, `dsm()`, `kint()`, `error_log` -- JavaScript/TypeScript: `console.log`, `console.debug`, `console.warn`, `debugger` +- PHP: `var_dump`, `print_r`, `dd()`, `dump()`, `dpm()`, `dsm()`, `kint()` +- JavaScript/TypeScript: `console.log`, `console.debug`, `debugger` - Twig: `{{ dump() }}`, `{{ kint() }}` +- `error_log` (PHP) and `console.warn` (JS) only when they are clearly leftover debugging (dumps + of local variables, "here" markers) — not deliberate logging **Hardcoded secrets** — Flag potential secrets or credentials: - Hardcoded passwords, API keys, tokens @@ -121,8 +123,9 @@ the matching `itkdev-review-` skill. Apply only the skills whose languages | `.js`, `.jsx`, `.ts`, `.tsx`, `.mjs`, `.cjs` | `itkdev-review-javascript` | Each skill provides the prioritized Critical/High/Medium/Low checklist, language-specific deep -checks, and a security pass for that language — map its findings into this report's Critical / -Warning / Suggestion severities. +checks, and a security pass for that language. Map its findings into this report's severities as: +skill **Critical → Critical**, **High → Warning**, **Medium / Low → Suggestion** (include Low +findings only when they are genuinely worth the author's time). **Twig:** - Unescaped output (`|raw` filter) without justification diff --git a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-comments/SKILL.md b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-comments/SKILL.md index 8e9cd5f..dddc9da 100644 --- a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-comments/SKILL.md +++ b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-comments/SKILL.md @@ -33,8 +33,11 @@ Use the idiomatic comment syntax for each language: |------------|-----------------|----------------------| | PHP | `//` | `/** */` | | JavaScript | `//` | `/** */` (JSDoc) | +| Python | `#` | `"""docstring"""` | | Twig | `{# #}` | `{# #}` (multiline) | | CSS / SCSS | `/* */` | `/* */` | +| YAML | `#` | `#` (no block form) | +| Shell | `#` | `#` (no block form) | For languages not listed, follow the language's standard convention. diff --git a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-javascript/SKILL.md b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-javascript/SKILL.md index 21e8c89..d678bc3 100644 --- a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-javascript/SKILL.md +++ b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-javascript/SKILL.md @@ -1,7 +1,7 @@ --- name: itkdev-review-javascript user-invocable: true -description: JavaScript/TypeScript code review for ITK Dev projects (browser, Node.js, frontend frameworks). Use when reviewing, auditing, critiquing, or giving feedback on JS/TS code — including snippets, files, or pull requests. Covers security (XSS, prototype pollution, eval, injection), correctness, async/promise error handling, type safety (any usage), and style (var vs const/let). Triggers for "review my JS/TS", "what's wrong with this", "security review", or when JavaScript/TypeScript code is pasted for feedback. +description: JavaScript/TypeScript code review for ITK Dev projects (browser, Node.js, frontend frameworks). Use when reviewing, auditing, critiquing, or giving feedback on JS/TS code — including snippets, files, or pull requests. Covers a dedicated security review (XSS, prototype pollution, eval, injection, secrets in bundles, SSRF, supply chain, misconfiguration) plus correctness, async/promise error handling, type safety (any usage), and style (var vs const/let). Triggers for "review my JS/TS", "security review", "what's wrong with this", or when JavaScript/TypeScript code is pasted for feedback. --- # JavaScript / TypeScript Code Review @@ -91,7 +91,42 @@ Work through each category. Skip irrelevant ones and say so briefly. - **`innerHTML` and friends**: justified and sanitized? - **Dependency risks**: known-vulnerable packages; flag any imports worth checking. -### Step 4 — Format the output +### Step 4 — Dedicated security review + +When the user asks for a **security review** (or a full audit), act as a senior application +security engineer and cover at minimum these categories: + +1. **XSS sinks** — `innerHTML`, `dangerouslySetInnerHTML`, `document.write`, unescaped + server-rendered output. +2. **Prototype pollution** — untrusted objects merged into `__proto__` / `constructor` / shared + objects. +3. **Dynamic code execution** — `eval()`, `new Function()`, string arguments to + `setTimeout`/`setInterval`. +4. **Injection** — shell commands, SQL, or templates built from user input (Node.js). +5. **Secrets & credentials** — keys/tokens hardcoded or shipped in client bundles, insecure + env-variable usage. +6. **Origin handling** — `postMessage` without origin checks, permissive CORS, unsafe + `window.open` targets. +7. **SSRF (Node.js)** — user-controlled URLs passed to `fetch`/`http` without allowlisting. +8. **Dependency & supply-chain risks** — known-vulnerable packages, typosquats, install scripts + worth checking. +9. **Cryptography** — `Math.random()` for tokens/IDs, homegrown crypto instead of Web Crypto / + `node:crypto`. +10. **Security misconfigurations** — missing CSP, cookies without `HttpOnly`/`Secure`/`SameSite`, + debug endpoints exposed. + +For each security finding, provide: + +- **Severity**: Critical / High / Medium / Low / Informational +- **Location**: file and line number(s) +- **Vulnerability type**: e.g. SQLi, XSS, SSRF, hardcoded secret +- **Description**: what the issue is and why it's dangerous +- **Proof of concept**: a brief example of how it could be exploited +- **Remediation**: a specific code fix or mitigation + +After the findings, provide a **prioritized remediation plan** — what to fix first and why. + +### Step 5 — Format the output ``` ## Code Review: [filename or description] @@ -115,11 +150,14 @@ One paragraph: what the code does, overall impression, most important theme. [2–5 genuine positives] ``` +For a security-focused review, use the per-finding shape from Step 4 and end with the prioritized +remediation plan. + ## Tone Guidelines - **Be direct, not harsh.** Explain the fix, don't just condemn the code. - **Always explain why**, not just what to change. -- **Distinguish opinion from fact.** "This *could* be memoized" vs "This *is* an XSS sink." +- **Distinguish opinion from fact.** "This *could* be extracted" vs "This *is* vulnerable." - **Prioritize ruthlessly.** Group issues that share a root cause. - **Acknowledge good work.** "What's Working Well" is not optional filler. diff --git a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-php/SKILL.md b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-php/SKILL.md index 2585a63..03872f5 100644 --- a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-php/SKILL.md +++ b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-php/SKILL.md @@ -1,14 +1,14 @@ --- name: itkdev-review-php user-invocable: true -description: PHP code review for ITK Dev projects (Laravel, Symfony, plain PHP). Use when reviewing, auditing, critiquing, or giving feedback on PHP code — including snippets, files, or pull requests. Covers security (SQL injection, XSS, mass assignment, file inclusion, command injection), correctness, performance (N+1 queries), type safety, and PSR-12 style. Triggers for "review my PHP", "what's wrong with this", "security review", or when PHP code is pasted for feedback. +description: PHP code review for ITK Dev projects (Symfony, Drupal, Laravel, plain PHP). Use when reviewing, auditing, critiquing, or giving feedback on PHP code — including snippets, files, or pull requests. Covers a dedicated security review (SQL injection, XSS, mass assignment, file inclusion, command injection, weak crypto, insecure deserialization, misconfiguration) plus correctness, performance (N+1 queries), type safety, and PSR-12 style. Triggers for "review my PHP", "security review", "what's wrong with this", or when PHP code is pasted for feedback. --- # PHP Code Review -Produce structured, prioritized code reviews for PHP codebases (Laravel, Symfony, plain PHP). -Reviews should be developer-friendly: specific, actionable, and ranked so the author knows what -to fix first. +Produce structured, prioritized code reviews for PHP codebases (Symfony, Drupal, Laravel, plain +PHP). Reviews should be developer-friendly: specific, actionable, and ranked so the author knows +what to fix first. ## Review Process @@ -16,7 +16,7 @@ to fix first. Before diving in, identify: -- **Framework**: Laravel, Symfony, or raw PHP. +- **Framework**: Symfony, Drupal, Laravel, or raw PHP. - **Purpose**: Web endpoint? CLI tool? Library? Queue worker? - **Scope**: Quick feedback, security focus, full audit, or pre-merge PR review? - **Standards**: Apply PSR-12 and any project conventions the user mentions; otherwise use @@ -87,19 +87,57 @@ Work through each category. Skip irrelevant ones and say so briefly. - **`eval()`**: almost always a red flag. - **`include`/`require` with variables**: potential file inclusion vulnerability. - **`isset()` vs `empty()` vs `??`**: correct null-handling idiom used? +- **Symfony specifics**: + - Services autowired correctly? + - Doctrine entities: fetch type appropriate (LAZY vs EAGER)? + - Forms: CSRF protection enabled? +- **Drupal specifics**: + - `\Drupal::` static calls inside services? Inject dependencies instead. + - Access checks on routes and entity operations (`_permission` requirements, `$entity->access()`)? + - Output escaped in render arrays? `t()` placeholders (`@`, `%`, `:`) used instead of string + concatenation? + - Database API with placeholders — no string-concatenated SQL? + - APIs deprecated in Drupal 10/11? + - Business logic in hooks (prefer services)? + - For deeper Drupal checks, use the `itkdev-drupal` skill (from the + `itkdev-scaffolding-and-templates` plugin) when it is installed. - **Laravel specifics**: - Mass assignment protection (`$fillable` / `$guarded` on models). - Policies and Gates used for authorization? - Jobs/Queues idempotent? Handle retries? - Eloquent N+1: using `with()` for relationships? - Config via `config()` in app code (not `env()` outside config files). -- **Symfony specifics**: - - Services autowired correctly? - - Doctrine entities: fetch type appropriate (LAZY vs EAGER)? - - Forms: CSRF protection enabled? - **PSR compliance**: PSR-4 autoloading, PSR-12 code style. -### Step 4 — Format the output +### Step 4 — Dedicated security review + +When the user asks for a **security review** (or a full audit), act as a senior application +security engineer and cover at minimum these categories: + +1. **Injection** — SQL, command, LDAP, template injection. +2. **Authentication & authorization** — broken auth, missing access controls, insecure sessions. +3. **Secrets & credentials** — hardcoded keys/passwords/tokens, insecure env-variable usage. +4. **Cryptography** — weak algorithms, hardcoded salts/IVs, improper key management. +5. **Input validation** — missing sanitization, type juggling, path traversal, file inclusion. +6. **Dependency risks** — known-vulnerable Composer packages (flag any worth checking). +7. **Insecure deserialization** — `unserialize()` on untrusted input, Phar deserialization. +8. **Error handling & logging** — sensitive data in logs, verbose errors exposed to users. +9. **Race conditions & TOCTOU** — filesystem or shared-state issues. +10. **Security misconfigurations** — `display_errors` in production, debug mode on, permissive + CORS, weak TLS settings. + +For each security finding, provide: + +- **Severity**: Critical / High / Medium / Low / Informational +- **Location**: file and line number(s) +- **Vulnerability type**: e.g. SQLi, XSS, SSRF, hardcoded secret +- **Description**: what the issue is and why it's dangerous +- **Proof of concept**: a brief example of how it could be exploited +- **Remediation**: a specific code fix or mitigation + +After the findings, provide a **prioritized remediation plan** — what to fix first and why. + +### Step 5 — Format the output ``` ## Code Review: [filename or description] @@ -123,13 +161,14 @@ One paragraph: what the code does, overall impression, most important theme. [2–5 genuine positives] ``` +For a security-focused review, use the per-finding shape from Step 4 and end with the prioritized +remediation plan. + ## Tone Guidelines -- **Be direct, not harsh.** "This query is vulnerable to SQL injection — use a prepared statement - instead", not "This is terrible code." +- **Be direct, not harsh.** Explain the fix, don't just condemn the code. - **Always explain why**, not just what to change. -- **Distinguish opinion from fact.** "This *could* be extracted into a helper" vs "This query *is* - vulnerable." +- **Distinguish opinion from fact.** "This *could* be extracted" vs "This *is* vulnerable." - **Prioritize ruthlessly.** Group issues that share a root cause. - **Acknowledge good work.** "What's Working Well" is not optional filler. diff --git a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-python/SKILL.md b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-python/SKILL.md index 3c20f49..fe1183c 100644 --- a/plugins/itkdev-code-quality-and-review/skills/itkdev-review-python/SKILL.md +++ b/plugins/itkdev-code-quality-and-review/skills/itkdev-review-python/SKILL.md @@ -1,7 +1,7 @@ --- name: itkdev-review-python user-invocable: true -description: Python code review for ITK Dev projects (Django, Flask, FastAPI, scripts, data pipelines). Use when reviewing, auditing, critiquing, or giving feedback on Python code — including snippets, files, or pull requests. Covers a dedicated security review (injection, broken auth, secrets, weak crypto, insecure deserialization, SSRF, path traversal, misconfiguration) plus correctness, performance, type safety, and PEP 8 style. Triggers for "review my Python", "security review", "what's wrong with this", or when Python code is pasted for feedback. +description: Python code review for ITK Dev projects (Django, Flask, FastAPI, scripts, data pipelines). Use when reviewing, auditing, critiquing, or giving feedback on Python code — including snippets, files, or pull requests. Covers a dedicated security review (injection, XSS, CSRF, broken auth, secrets, weak crypto, insecure deserialization, SSRF, path traversal, misconfiguration) plus correctness, performance (N+1 queries), type safety, and PEP 8 style. Triggers for "review my Python", "security review", "what's wrong with this", or when Python code is pasted for feedback. --- # Python Code Review @@ -42,6 +42,13 @@ Work through each category. Skip irrelevant ones and say so briefly. - [ ] Sensitive data exposure: hardcoded API keys, passwords, tokens; secrets in logs or errors. - [ ] Path traversal: user-controlled file paths without sanitization. - [ ] SSRF: user-controlled URLs passed to `requests`/`urllib` without allowlisting. +- [ ] XSS: `mark_safe()` / `|safe` (Django), autoescape disabled or `Markup()` (Jinja2), HTML + built by hand from user input. +- [ ] CSRF: `@csrf_exempt` without justification (Django); state-changing Flask routes without + CSRF protection. +- [ ] Template injection (SSTI): `render_template_string()` / `jinja2.Template()` on user input. +- [ ] Weak crypto/randomness: `random` for tokens or secrets (use `secrets`); `md5`/`sha1` for + passwords (use framework hashers / `bcrypt` / `scrypt`). **Correctness** @@ -86,9 +93,11 @@ Work through each category. Skip irrelevant ones and say so briefly. - **`global` / `nonlocal` overuse**: sign of poor state design. - **`__all__`**: defined if the module is a public API? - **Context managers**: `with` used for files, locks, DB sessions, HTTP clients? +- **HTTP timeouts**: `requests`/`httpx` calls without `timeout=` hang forever. +- **`assert` for validation**: stripped under `python -O` — use explicit checks that raise. - **Django specifics**: raw SQL via `raw()` / `cursor.execute()` vs ORM; `select_related()` / `prefetch_related()` for N+1; `@login_required` / permissions on views; no business logic in - signals. + signals; `mark_safe` / `|safe` and `@csrf_exempt` usages justified? - **Flask/FastAPI specifics**: input validation (Pydantic, WTForms, marshmallow); thin route handlers (business logic in services); proper HTTP status codes. - **Data pipelines / scripts**: large files streamed/chunked rather than loaded fully into memory; @@ -167,3 +176,6 @@ remediation plan. | Subprocess injection | `subprocess.run(f"ls {user_input}", shell=True)` | `subprocess.run(["ls", user_input])` | | Insecure deserialization | `pickle.loads(user_data)` | Use JSON, or validate the source | | Unsafe YAML | `yaml.load(data)` | `yaml.safe_load(data)` | +| Weak token | `random.random()` | `secrets.token_urlsafe()` | +| SSTI | `render_template_string(user_input)` | Render a fixed template, pass data as variables | +| Missing timeout | `requests.get(url)` | `requests.get(url, timeout=10)` |