Add security review persona for injection vulnerability audits#206
Merged
DiamondDagger590 merged 3 commits intoMar 15, 2026
Merged
Conversation
Adds an automated security review persona that triggers on every PR touching src/main/ Java files. The persona checks for: - MiniMessage/Adventure API injection via user-controlled strings - Command injection through player.performCommand() concatenation - Permission bypass (missing hasPermission checks, isOp misuse) - SQL/data injection in DAO methods Review comments use a CodeRabbit-style format: findings are organized by file, each with a diff block showing the fix and an "AI Agent Prompt" section containing step-by-step implementation instructions. Also wires needs_security into pr-review.yml domain detection and review-meta.json so the artifact-based workflow_run path passes the flag through correctly. https://claude.ai/code/session_01A7DVvhYkmHxX9bAdxGmjSB
Replaces inconsistent pipe-separated output (CONCERN | WHY | WHERE) with an explicit line-break format that matches the preferred style shown in testing reviews: **CONCERN:** [issue] **WHY:** [impact] **WHERE:** [location] --- Applied to all command files (.claude/commands/review-*.md) and all persona files (.cursor/rules/persona-*.mdc) in both repos. The explicit newline format prevents the model from collapsing fields onto a single line with | separators. https://claude.ai/code/session_01A7DVvhYkmHxX9bAdxGmjSB
WalkthroughThis pull request standardizes finding-report formatting across review docs, adds a Security Engineer persona for injection-focused audits, and wires security-change detection and conditional security reviews into the GitHub Actions PR review workflows. Changes
Sequence DiagramsequenceDiagram
actor PR as Pull Request
participant Detect as detect-changes
participant Prepare as prepare-review
participant ReviewBot as ai-persona-review
participant Comment as PR Commenter
PR->>Detect: run detect-changes
Detect->>Detect: scan changed paths (e.g., src/main/**/*.java)
Detect->>Detect: set needs_security = true/false
Detect->>Prepare: emit needs_security
Prepare->>Prepare: load review-meta.json, evaluate proceed && needs_security
alt Security review required
Prepare->>ReviewBot: invoke security persona review
ReviewBot->>ReviewBot: run Security Engineer prompt + checks
ReviewBot->>Comment: post security findings to PR
else Security review not required
Prepare->>Comment: skip security persona step
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/review-security.md:
- Line 47: Line 47 uses "4." in an ordered list which triggers markdownlint
MD029; change that list item prefix to "1." (i.e., update the line that reads
`4. If nothing to flag: "No security concerns found."` to `1. If nothing to
flag: "No security concerns found."`) so the ordered-list numbering style starts
at 1 and matches markdownlint expectations; ensure the surrounding ordered-list
items use a consistent numbering scheme.
- Line 1: Add a top-level H1 header as the very first line of the document to
satisfy markdownlint MD041; edit the file's first line (the document header) and
insert a descriptive H1 such as "# Security Engineer persona" (or similar) so
the file begins with an H1 before any other content.
In @.cursor/rules/persona-security.mdc:
- Line 63: Update the maintenance note (the line under "**Maintenance:** If a
new injection vector or unsafe pattern is found during review...") to require
that any new injection vector or unsafe pattern added to this file and
`.claude/commands/review-security.md` must also be added to
`.cursor/rules/core.mdc` and `CLAUDE.md` in the same PR; edit the sentence to
explicitly list `.cursor/rules/core.mdc` and `CLAUDE.md` alongside
`.claude/commands/review-security.md` so reviewers are instructed to sync core
docs (`core.mdc`) and the top-level `CLAUDE.md` whenever a new anti-pattern or
convention is introduced.
- Around line 21-22: The rule in persona-security.mdc is too broad and flags
legitimate internal normalization/comparison calls (e.g.,
getMiniMessage().deserialize(...)) — narrow the rule so it only targets
deserialize usage on player-controllable input and add an explicit exception for
internal comparison/normalization flows and known safe types; update the rule to
check call sites like getMiniMessage().deserialize(...) and only flag when the
argument can be tainted by player input (not when used in functions/methods
dedicated to internal normalization or comparison), and add an allowlist for
safe server-controlled values (Material, EntityType, UUID.toString(),
NamespacedKey.getKey()/getNamespace(), numeric types) and for calls originating
from McRPGLocalizationManager or other internal normalization utilities.
- Around line 56-57: The guidance incorrectly suggests using
PlainTextComponentSerializer.plainText().serialize(Component.text(userInput))
which does not prevent MiniMessage tag parsing; update the remediation for the
vulnerable deserialize(userInput) call to use safe literal insertion instead —
replace embedding userInput into MiniMessage templates with
Placeholder.unparsed(key, userInput) or Placeholder.component(key,
Component.text(userInput)) so the input is treated as literal content, or if you
must pass a string template use MiniMessage.escapeTags(userInput) before
inserting; update any examples and text that reference
PlainTextComponentSerializer.plainText().serialize(Component.text(userInput)) to
one of these safe alternatives and mention the deserialize(...) and
MiniMessage.escapeTags/Placeholder.* symbols so readers can find the affected
snippets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaada6aa-36bf-4f36-a25c-1fa81f647ba6
📒 Files selected for processing (12)
.claude/commands/review-extensibility.md.claude/commands/review-gui-ux.md.claude/commands/review-security.md.claude/commands/review-server-owner.md.claude/commands/review-testing.md.cursor/rules/persona-extensibility.mdc.cursor/rules/persona-gui-ux.mdc.cursor/rules/persona-security.mdc.cursor/rules/persona-server-owner.mdc.cursor/rules/persona-testing.mdc.github/workflows/ai-persona-review.yml.github/workflows/pr-review.yml
- Replace PlainTextComponentSerializer guidance with Placeholder.unparsed() and MiniMessage.escapeTags() — the serializer roundtrip is not safe - Narrow MiniMessage injection rule to exclude internal normalization calls inside McRPGLocalizationManager and similar utilities - Restrict workflow_dispatch to default branch only (branch security fix) - Add ref: default_branch to checkout step to prevent persona file injection - Fix MD029 (ordered list numbering) and MD041 (missing H1) in command file - Update maintenance note to include core.mdc and CLAUDE.md https://claude.ai/code/session_01A7DVvhYkmHxX9bAdxGmjSB
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/review-security.md:
- Around line 8-11: Add a short remediation paragraph to the MiniMessage
checklist explaining safe escaping alternatives: mention using
Placeholder.unparsed(...) and MiniMessage.escapeTags(...) (or equivalent safe
APIs) when inserting user-controlled strings, recommend routing player-facing
strings through McRPGLocalizationManager instead of direct deserialize(), and
warn about sanitizing stored user data before calling MiniMessage.deserialize();
place this guidance immediately after the existing checklist so reviewers see
both the risk and concrete safe options.
In @.github/workflows/ai-persona-review.yml:
- Around line 129-130: The two workflow pattern checks both match the same glob
`src/main/.*\.java` so `tests_changed` and `security` are being set identically;
add a short inline comment next to the duplicated checks (the two lines
containing the `[[ "$f" =~ src/main/.*\.java ]]` patterns) stating this overlap
is intentional and kept for future divergence (or clarify why both flags must be
set), so reviewers understand the duplication and its purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c07df993-5168-43b3-bcd2-86236ac365f3
📒 Files selected for processing (3)
.claude/commands/review-security.md.cursor/rules/persona-security.mdc.github/workflows/ai-persona-review.yml
Comment on lines
+8
to
+11
| - Does any code pass a user-controlled string (player chat, player-set name, sign text, book content, anvil rename) to `MiniMessage.deserialize()` or `getMiniMessage().deserialize()`? Injection allows `<click:run_command:>`, `<click:open_url:>`, hover events. | ||
| - Is user-controlled data stored (DB, NBT) and later passed to `MiniMessage.deserialize()` without sanitization? Watch for: player input → storage → `deserialize()`. | ||
| - Are all player-facing strings routed through `McRPGLocalizationManager`? Direct `deserialize()` on user input violates project convention and is a security risk. | ||
| - **Safe to concatenate (skip):** `Material`, `EntityType`, other Bukkit enums; `UUID.toString()`; integers; `NamespacedKey` fragments. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add safe escaping guidance to match the persona file.
The MiniMessage injection checklist here omits the remediation guidance present in the persona file. Consider adding a brief note about safe alternatives (e.g., Placeholder.unparsed(), MiniMessage.escapeTags()) so reviewers using this command have the same context.
Suggested addition after line 11
- **Safe to concatenate (skip):** `Material`, `EntityType`, other Bukkit enums; `UUID.toString()`; integers; `NamespacedKey` fragments.
+- **Remediation:** Route through localization manager, or use `Placeholder.unparsed("key", userInput)` / `MiniMessage.escapeTags(userInput)`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Does any code pass a user-controlled string (player chat, player-set name, sign text, book content, anvil rename) to `MiniMessage.deserialize()` or `getMiniMessage().deserialize()`? Injection allows `<click:run_command:>`, `<click:open_url:>`, hover events. | |
| - Is user-controlled data stored (DB, NBT) and later passed to `MiniMessage.deserialize()` without sanitization? Watch for: player input → storage → `deserialize()`. | |
| - Are all player-facing strings routed through `McRPGLocalizationManager`? Direct `deserialize()` on user input violates project convention and is a security risk. | |
| - **Safe to concatenate (skip):** `Material`, `EntityType`, other Bukkit enums; `UUID.toString()`; integers; `NamespacedKey` fragments. | |
| - Does any code pass a user-controlled string (player chat, player-set name, sign text, book content, anvil rename) to `MiniMessage.deserialize()` or `getMiniMessage().deserialize()`? Injection allows `<click:run_command:>`, `<click:open_url:>`, hover events. | |
| - Is user-controlled data stored (DB, NBT) and later passed to `MiniMessage.deserialize()` without sanitization? Watch for: player input → storage → `deserialize()`. | |
| - Are all player-facing strings routed through `McRPGLocalizationManager`? Direct `deserialize()` on user input violates project convention and is a security risk. | |
| - **Safe to concatenate (skip):** `Material`, `EntityType`, other Bukkit enums; `UUID.toString()`; integers; `NamespacedKey` fragments. | |
| - **Remediation:** Route through localization manager, or use `Placeholder.unparsed("key", userInput)` / `MiniMessage.escapeTags(userInput)`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/review-security.md around lines 8 - 11, Add a short
remediation paragraph to the MiniMessage checklist explaining safe escaping
alternatives: mention using Placeholder.unparsed(...) and
MiniMessage.escapeTags(...) (or equivalent safe APIs) when inserting
user-controlled strings, recommend routing player-facing strings through
McRPGLocalizationManager instead of direct deserialize(), and warn about
sanitizing stored user data before calling MiniMessage.deserialize(); place this
guidance immediately after the existing checklist so reviewers see both the risk
and concrete safe options.
Comment on lines
129
to
+130
| [[ "$f" =~ src/main/.*\.java ]] && tests_changed=true | ||
| [[ "$f" =~ src/main/.*\.java ]] && security=true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the intentional pattern overlap.
Lines 129-130 both match src/main/.*\.java, making tests_changed and security flags identical. While this allows future divergence, a brief comment would clarify the intent.
Optional: Add clarifying comment
[[ "$f" =~ src/main/.*\.java ]] && tests_changed=true
+ # Security review also triggers on main Java changes (may diverge later)
[[ "$f" =~ src/main/.*\.java ]] && security=true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [[ "$f" =~ src/main/.*\.java ]] && tests_changed=true | |
| [[ "$f" =~ src/main/.*\.java ]] && security=true | |
| [[ "$f" =~ src/main/.*\.java ]] && tests_changed=true | |
| # Security review also triggers on main Java changes (may diverge later) | |
| [[ "$f" =~ src/main/.*\.java ]] && security=true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ai-persona-review.yml around lines 129 - 130, The two
workflow pattern checks both match the same glob `src/main/.*\.java` so
`tests_changed` and `security` are being set identically; add a short inline
comment next to the duplicated checks (the two lines containing the `[[ "$f" =~
src/main/.*\.java ]]` patterns) stating this overlap is intentional and kept for
future divergence (or clarify why both flags must be set), so reviewers
understand the duplication and its purpose.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a new security engineer persona for automated code review that audits McRPG for player-exploitable injection vulnerabilities. The persona is invoked manually via
/review-securityand focuses on MiniMessage/Adventure API injection, command injection, permission bypass, and SQL/data injection vectors.Key Changes
New security review persona (
.cursor/rules/persona-security.mdc): Defines threat model (players with normal server access), safe patterns (Bukkit enums, UUIDs, integers), and dangerous sinks (MiniMessage deserialization,performCommand(),dispatchCommand(), unguarded privileged operations, string-concatenated SQL queries).Security review command (
.claude/commands/review-security.md): Provides checklist-driven instructions for the AI agent to systematically audit code changes, with standardized output format (file-organized findings, severity levels, diff blocks, and corrective prompts).CI/CD integration (
.github/workflows/pr-review.ymland.ai-persona-review.yml):needs_securitydetection flag that triggers when anysrc/main/Java file changesImplementation Details
Threat model scope: Focuses on player-controlled inputs (chat, command arguments, GUI interactions, sign/book text, anvil renames) flowing into dangerous sinks without sanitization. Server admin config and Bukkit enums are explicitly out of scope to reduce false positives.
Output format: Findings organized by file with severity levels (HIGH/MEDIUM/LOW), one-sentence vulnerability descriptions, before/after code diffs, and AI agent prompts explaining the fix and why it preserves legitimate behavior.
Manual invocation: Unlike other personas, security review is
alwaysApply: falseand must be explicitly requested via/review-securityto avoid noise in routine coding sessions.https://claude.ai/code/session_01A7DVvhYkmHxX9bAdxGmjSB
Summary by CodeRabbit
New Features
Documentation