security-review-mr

name	security-review-mr
description	Audit a merge request diff for common security concerns (input validation, sanitization, authentication and authorization, secrets management, unsafe dependencies, and related risks) and fold findings into the same review.json produced by the base MR review. Use as a supplement to `review-mr` whenever a code MR is being reviewed.

Security Review MR Skill

Audit the current merge request for security concerns and fold any findings into the same review.json produced by the base review-mr skill.

Goal

Provide a focused security pass on top of the general MR review. This is a supplement to review-mr, not a separate output. Findings must be merged into the single combined review.json so reviewers receive one cohesive review.

Context

• The working directory is the MR branch checkout.
• The CI job usually provides an annotated diff in mr_diff.txt.
• The CI job usually provides the MR description in mr_description.txt.
• Focus on the files and lines changed by this MR.
• Default behavior: do not post comments or reviews to GitLab directly.

When to apply this skill

• Apply on code MRs whenever review-mr is applied.
• Do not apply on spec-only MRs handled by review-spec.
• Skip the skill entirely when no changed file introduces code or configuration that touches the concerns below; it is better to stay silent than to manufacture findings.
• Do not duplicate findings the base review-mr pass will already raise. If the base review would naturally catch an issue, leave it there rather than re-reporting it from the security pass.

Security concerns to audit

Evaluate each changed hunk against the following concerns. Treat the list as a checklist, not a ceiling — flag other clearly security-relevant issues when they appear.

Input validation and untrusted data

• User-supplied or network-supplied input used without validation, length limits, or type checks.
• Deserialization of untrusted data (e.g. pickle, yaml.load, eval, Function, JSON.parse feeding into a command).
• Path traversal risk: user input concatenated into filesystem paths without normalization or an allowlist.
• SSRF risk: user-controlled URLs passed to outbound HTTP clients without scheme or host restrictions.
• Unbounded resource use driven by untrusted input (memory, loops, regex with catastrophic backtracking).

Output encoding and sanitization

• Untrusted data interpolated into SQL, shell commands, HTML, Markdown, log lines, or URLs without the right encoding or parameterization.
• Use of shell=True, string concatenation into exec/spawn, or raw SQL strings.
• Rendering user-supplied Markdown or HTML into a UI without sanitization.

Authentication and authorization

• Missing or weakened authentication checks on new endpoints, RPC handlers, or CLI commands.
• Authorization checks that trust client-supplied identifiers instead of the authenticated principal.
• Permission checks removed or made more permissive without clear justification.

Secrets management

• Hardcoded credentials, API keys, private keys, or tokens committed in source.
• Secrets read from insecure locations (world-readable files, logs, environment dumps printed to stdout).
• Secrets passed via command-line arguments where they would leak into process listings or shell history.
• Secrets written to logs, error messages, analytics events, or serialized payloads.
• New secrets added to .env, fixtures, or test data that are real rather than clearly synthetic placeholders.

Cryptography and randomness

• Use of weak or deprecated primitives (MD5, SHA1 for security purposes, ECB mode, RC4).
• Hand-rolled crypto when a vetted library is available.
• Non-cryptographic randomness (random.random, Math.random) used for tokens, IDs, or security decisions.
• Missing integrity or authenticity checks when decrypting or verifying tokens.

Dependencies and supply chain

• New dependencies from unknown registries or forks without a clear rationale.
• Pinning loosened in a way that allows untrusted upgrades (e.g. * or very broad ranges on a sensitive package).
• Fetching scripts over HTTP or piping curl to a shell inside build or CI steps.

Data handling and privacy

• Logging or echoing personally identifiable information, auth tokens, session IDs, or request bodies that may contain them.
• New telemetry or analytics events that capture sensitive data without redaction.
• Expanding the scope of stored data beyond what the feature requires.

Configuration and defaults

• New feature flags or configuration options that default to insecure values (e.g. TLS disabled, auth optional).
• CORS, cookies, or headers weakened in scope without an explicit justification.
• Permissive file modes on newly created files that contain sensitive material.

How to evaluate

1. Read mr_description.txt and mr_diff.txt to understand the scope and intent of the change.
2. For each changed hunk, consider which of the concerns above could plausibly apply given the surrounding code in the checkout.
3. Prefer evidence-based findings tied to specific changed lines. If a concern only applies to untouched code, describe it in the review summary instead of as an inline comment.
4. Do not flag purely stylistic or non-security issues here — those belong in the base review-mr pass.
5. Do not repeat findings already covered by the base review; if the base pass would naturally catch it, leave it there.

How to report findings

• Do not create a separate report file.
• Fold security findings into the same review.json produced by review-mr.
• Prefix every security finding's comment body with a [SECURITY] tag after the severity label so reviewers can tell the source of the concern. For example:
  • 🚨 [CRITICAL] [SECURITY] SQL injection: ...
  • ⚠️ [IMPORTANT] [SECURITY] Secret written to logs: ...
  • 💡 [SUGGESTION] [SECURITY] Prefer parameterized query: ...
• In the review summary, add a dedicated ## Security subsection when there are security findings. List the most important security concerns there in addition to any inline comments. If there are no security findings, do not add the subsection.
• Count security findings toward the existing Found: X critical, Y important, Z suggestions tally in the summary. Do not add a separate security counter.
• Upgrade the overall verdict if a security finding materially demands it. A critical security finding should generally result in Request changes.

Severity mapping

• 🚨 [CRITICAL] for issues that are likely exploitable in production (e.g. shell injection with attacker-controlled input, committed live secret, missing auth on a destructive endpoint).
• ⚠️ [IMPORTANT] for plausible security weaknesses that should be fixed before merge (e.g. missing input validation on an internal-only endpoint, weak hashing for non-password data, overly broad CORS).
• 💡 [SUGGESTION] for defense-in-depth improvements that are clearly worthwhile but not immediate risks.
• 🧹 [NIT] is rarely appropriate for security findings; use only when the comment includes a concrete suggestion block and the issue is genuinely cosmetic.

Inline comment requirements

• Follow the same diff-line rules as review-mr: inline comments must target lines that exist in this MR's diff.
• Keep comments concise, direct, and actionable. Explain the threat, then the fix.
• When proposing code changes, use the same suggestion block format as review-mr.

Boundaries

• Do not run dynamic scans, fetch remote advisories, or call external security APIs.
• Do not speculate about vulnerabilities that cannot be tied to the diff or the checked-out files.
• Do not gate the MR on theoretical risks; prefer 💡 [SUGGESTION] when the risk is low or the fix is optional.
• Do not post to GitLab directly. Your only output is the merged review.json from the base review pass.