diff --git a/.claude/commands/address-pr-feedback.md b/.claude/commands/address-pr-feedback.md index b070f47..32b1a3a 100644 --- a/.claude/commands/address-pr-feedback.md +++ b/.claude/commands/address-pr-feedback.md @@ -1,6 +1,11 @@ # Address PR Feedback -Fetch and address all review feedback on the current PR. +Fetch and address all review bot feedback on the current PR. + +## Token Budget Awareness + +This skill fetches external data (GitHub API). Every API response counts against the context window. +**Goal: stay under ~30K tokens for the fetch phase.** Use truncated previews first, full bodies only when needed. ## Instructions @@ -12,61 +17,181 @@ gh pr view --json number,url,state --jq '{number, url, state}' If no PR exists for the current branch, abort with a message. -### 2. Fetch review comments +### 2. Check that review bots are done + +Both CodeRabbit and Amazon Q review asynchronously. Before addressing feedback, verify they have finished. ```bash -# Get all reviews on the PR +# Get all reviews on the PR — compact output gh pr view --json reviews --jq '.reviews[] | {author: .author.login, state: .state}' -# Get inline review comments (truncated to save tokens) +# Check PR comments for bot activity +gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate \ + --jq '[.[] | .user.login] | unique' +``` + +**CodeRabbit**: Look for a PR comment containing "Walkthrough" or a review with `coderabbitai` as author. If not present, inform the user: +> "CodeRabbit hasn't reviewed this PR yet. Wait for its review or run `@coderabbitai review` as a PR comment, then re-run this command." + +**Amazon Q**: Look for review comments from `amazon-q-developer[bot]`. If not present, inform the user: +> "Amazon Q hasn't reviewed this PR yet. Wait for its review, then re-run this command." + +**If either bot hasn't finished, stop here.** Do not proceed to fixing issues with incomplete feedback. + +### 3. Fetch review comments (token-efficient two-pass approach) + +**CRITICAL: Always use `--paginate` with `gh api` for review comments.** The default page size is 30, which is easily exceeded when bots post 16+ inline comments plus replies. Without `--paginate`, you will miss comments from later review passes. + +**CRITICAL: Use truncated previews first.** Full comment bodies contain analysis chains, shell scripts, and committable suggestions that can be 2-5KB each. Fetch summaries first, then only fetch full bodies for items you actually need to fix. + +#### 3a. Pass 1 — Scan inline comments (truncated) + +Fetch all bot inline comments with bodies truncated to 300 chars. This is enough to understand the issue and triage it. Run these two queries in parallel: + +```bash +# Get bot inline comments — TRUNCATED bodies (saves ~80% tokens) +# MUST use --paginate to get all comments across pages gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate \ --jq '.[] | select(.in_reply_to_id == null) + | select(.user.login == "coderabbitai[bot]" or .user.login == "amazon-q-developer[bot]") | {id, author: .user.login, path, line, created_at, body: (.body[:300])}' -# Get IDs already replied to +# Get IDs already replied to by the PR author gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate \ - --jq '[.[] | select(.in_reply_to_id != null) | .in_reply_to_id] | unique' + --jq '[.[] | select(.in_reply_to_id != null) | select(.user.login != "coderabbitai[bot]" and .user.login != "amazon-q-developer[bot]") | .in_reply_to_id] | unique' +``` + +To identify **new unaddressed root comments**, filter by: +- `in_reply_to_id == null` (root comment, not a reply) +- `user.login` is a bot (`coderabbitai[bot]` or `amazon-q-developer[bot]`) +- No reply from the PR author exists with matching `in_reply_to_id` + +Cross-reference to find **unreplied** bot comments only. + +#### 3b. Pass 1 — Scan CodeRabbit review bodies (extract counts only) + +CodeRabbit review bodies are the largest token consumers (3-8KB each). First extract just the actionable metadata: + +```bash +# Get review metadata — NOT full bodies +gh api "repos/{owner}/{repo}/pulls/{pr}/reviews" --paginate \ + --jq '.[] | select(.user.login == "coderabbitai[bot]") | select(.body | length > 0) + | {id, + actionable: ((.body | try capture("Actionable comments posted: (?[0-9]+)") catch null | .n) // "0"), + has_nitpicks: (.body | test("Nitpick comments")), + has_duplicates: (.body | test("Duplicate comments")), + has_agent_prompt: (.body | test("Prompt for AI Agents"))}' +``` + +**Only fetch the full review body** if `has_nitpicks`, `has_duplicates`, or `has_agent_prompt` is true AND the review is from the latest round (i.e., after your last push). For earlier rounds where inline comments were already replied to, skip the full body fetch. + +```bash +# Fetch full body ONLY for reviews that need it (one at a time) +gh api "repos/{owner}/{repo}/pulls/{pr}/reviews/{review_id}" \ + --jq '{id, body}' ``` -Cross-reference to find **unreplied** comments only. +Parse each fetched review body for: +- **"Nitpick comments (N)"** — valid code quality items; fix them +- **"Duplicate comments (N)"** — re-raised from prior reviews; fix them +- **"Outside diff range comments (N)"** — comments on code outside the changed lines; triage these the same as inline comments +- **"Prompt for AI Agents"** — structured fix instructions with file paths and line numbers -### 3. Identify actionable feedback +**The inline comments (3a) are only the Critical/Major items. Nitpicks, duplicates, and outside-diff-range items stay in the review body (3b).** If the user says "5 comments and 3 comments", those numbers come from "Actionable comments posted: N" in separate review bodies. -For each unreplied comment, verify the finding against current code and decide: +#### 3c. Amazon Q general comments (if any) + +```bash +gh pr view --json comments \ + --jq '.comments[] | select(.author.login == "amazon-q-developer") | {bodyPreview: (.body[:300])}' +``` + +#### 3d. CodeRabbit summary comment — SKIP unless user asks + +The walkthrough/summary comment is almost never actionable for code fixes. **Do not fetch it** unless the user specifically asks about it. This saves 10-20KB. + +### 4. Pass 2 — Fetch full bodies for items to fix + +For each unreplied comment you plan to fix (not dismiss), fetch the full body: + +```bash +# Fetch full body for a single comment +gh api "repos/{owner}/{repo}/pulls/comments/{comment_id}" --jq '.body' +``` + +**Only do this for comments where the 300-char preview wasn't enough to understand the required fix.** Many fixes are obvious from the preview alone. + +### 5. Identify actionable feedback + +Collect ALL feedback from inline comments (3a), review body items (3b), and Amazon Q comments. **Do not skip nitpicks or duplicate items from the review body**; they get the same triage treatment as inline review comments: + +- **CodeRabbit** (`coderabbitai[bot]`): Inline review comments (3a) + review body nitpicks/duplicates/actionable items (3b) +- **Amazon Q** (`amazon-q-developer[bot]`): Inline review comments (3a) + +**Before applying any fix**, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. + +For each comment, determine: 1. **Valid concern** — fix it -2. **False positive** — reply explaining why +2. **False positive** — reply explaining why (e.g., Drizzle parameterization handles SQL injection) 3. **Stale** — code was already changed/removed since the comment was posted 4. **Ambiguous** — ask the user which direction to take -### 4. Present decisions for approval +### 6. Present decisions for approval **STOP and present a table** before making any changes. The user must approve the plan first. | # | Source | File | Line | Comment Summary | Decision | Rationale | |---|--------|------|------|-----------------|----------|-----------| -| 1 | reviewer | `path/to/file.rs` | 42 | Brief summary | Fix / Dismiss / Stale | Why | +| 1 | CR inline | `path/to/file.rs` | 42 | Brief summary | Fix / Dismiss / Stale | Why | +| 2 | CR review body | `path/to/file.rs` | 10 | Brief summary | Fix / Dismiss / Stale | Why | +| 3 | AQ inline | `path/to/file.rs` | 55 | Brief summary | Fix / Dismiss / Stale | Why | + +Wait for the user to: +- **Approve all** — proceed with all decisions as proposed +- **Override specific rows** — change the decision for individual items (e.g., "dismiss #3 instead of fixing") +- **Ask questions** — clarify any items before approving + +**Do not proceed to step 7 until the user approves.** + +### 7. Address each item + +**CRITICAL — Reply rules:** -Wait for the user to approve before proceeding. +1. **ALWAYS reply on the conversation thread** — use `gh api .../comments/{comment_id}/replies`. NEVER use `gh pr comment` to post a top-level PR comment. Bots track conversations by thread, not by scanning all PR comments. -### 5. Address each item +2. **Bot reply prefixes** — replies MUST start with the correct prefix: + - **Amazon Q**: `/q` (e.g., `/q Fixed — `) + - **CodeRabbit**: `@coderabbitai` (e.g., `@coderabbitai Fixed — `) + +Without thread replies and correct prefixes, the bots will NOT see your reply and the comment won't be resolved. For valid concerns: 1. Read the file and understand the context around the flagged line 2. Apply the fix 3. Reply to the review comment thread explaining what was fixed: ```bash + # For Amazon Q comments — MUST start with /q + gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \ + -X POST -f body="/q Fixed — " + + # For CodeRabbit comments — MUST start with @coderabbitai gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \ - -X POST -f body="Fixed — " + -X POST -f body="@coderabbitai Fixed — " ``` For false positives: -1. Reply to the review comment thread explaining why: +1. Reply to the review comment thread explaining why it's not an issue: ```bash + # For Amazon Q comments — MUST start with /q gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \ - -X POST -f body="" + -X POST -f body="/q " + + # For CodeRabbit comments — MUST start with @coderabbitai + gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \ + -X POST -f body="@coderabbitai " ``` -### 6. Run checks +### 8. Run checks After all fixes are applied: @@ -76,7 +201,7 @@ cargo check && cargo clippy -- -D warnings && cargo test All must pass before committing. -### 7. Commit and push +### 9. Commit and push If any code changes were made: @@ -86,10 +211,12 @@ git commit -m "fix: address PR review feedback" git push ``` -### 8. Report summary +### 10. Report summary + +Present a summary to the user: -- **Fixed**: List of issues fixed with brief descriptions +- **Fixed**: List of issues that were fixed with brief descriptions - **Dismissed**: List of false positives with reasoning -- **Stale**: Comments on code already changed/removed +- **Stale**: Comments on code that was already changed/removed - **Needs input**: Any ambiguous items requiring user decision - **Checks**: Pass/fail status diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..b157e7a --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,10 @@ +# .coderabbit.yaml +reviews: + request_changes_workflow: true + instructions: | + - Surface ALL findings in round 1. Never trickle comments across multiple rounds. + If uncertain whether something is worth flagging, include it in round 1 or skip it entirely. + - Do NOT generate new review comments on incremental pushes to a PR once the maintainer + has addressed or declined a suggestion. Respect final decisions. + - Files under `.claude/` are planning/agentic documents, not production code. + A single high-level pass only — no iterative feedback. diff --git a/Cargo.lock b/Cargo.lock index f517a2e..7b53887 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -828,6 +828,16 @@ dependencies = [ "tree-sitter-language", ] +[[package]] +name = "tree-sitter-java" +version = "0.23.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0aa6cbcdc8c679b214e616fd3300da67da0e492e066df01bcf5a5921a71e90d6" +dependencies = [ + "cc", + "tree-sitter-language", +] + [[package]] name = "tree-sitter-javascript" version = "0.23.1" @@ -1097,6 +1107,7 @@ dependencies = [ "tracing", "tracing-subscriber", "tree-sitter", + "tree-sitter-java", "tree-sitter-javascript", "tree-sitter-typescript", ] diff --git a/Cargo.toml b/Cargo.toml index 73736b1..459e3fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } tree-sitter = "0.24" tree-sitter-typescript = "0.23" +tree-sitter-java = "0.23" tree-sitter-javascript = "0.23" ignore = "0.4" sha2 = "0.10" diff --git a/rules/java/access-decision-voter.toml b/rules/java/access-decision-voter.toml new file mode 100644 index 0000000..6654a27 --- /dev/null +++ b/rules/java/access-decision-voter.toml @@ -0,0 +1,45 @@ +[rule] +id = "java-access-decision-voter" +languages = ["java"] +category = "custom" +confidence = "medium" +description = "Custom AccessDecisionVoter or security interceptor implementation" +query = """ +(class_declaration + (superclass + (type_identifier) @parent_class) +) @match + +(class_declaration + (super_interfaces + (type_list + (type_identifier) @parent_class)) +) @match +""" + +[rule.predicates.parent_class] +match = "^(AccessDecisionVoter|AbstractAccessDecisionManager|SecurityInterceptor|AbstractSecurityInterceptor|PermissionEvaluator)$" + +[[rule.tests]] +input = """ +public class CustomVoter extends AbstractAccessDecisionManager { + public int vote() { return 0; } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class CustomVoter implements AccessDecisionVoter { + public int vote() { return 0; } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class MyService extends BaseService { + public void doWork() { } +} +""" +expect_match = false diff --git a/rules/java/authenticated-check.toml b/rules/java/authenticated-check.toml new file mode 100644 index 0000000..a55f14e --- /dev/null +++ b/rules/java/authenticated-check.toml @@ -0,0 +1,35 @@ +[rule] +id = "java-authenticated-check" +languages = ["java"] +category = "middleware" +confidence = "medium" +description = "Authentication status check (isAuthenticated, isAnonymous, etc.)" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list) +) @match +""" + +[rule.predicates.method_name] +match = "^(isAuthenticated|isAnonymous|isFullyAuthenticated|isRememberMe)$" + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (authentication.isAuthenticated()) { allow(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (request.isSecure()) { allow(); } + } +} +""" +expect_match = false diff --git a/rules/java/feature-gate-check.toml b/rules/java/feature-gate-check.toml new file mode 100644 index 0000000..017ec39 --- /dev/null +++ b/rules/java/feature-gate-check.toml @@ -0,0 +1,57 @@ +[rule] +id = "java-feature-gate-check" +languages = ["java"] +category = "feature_gate" +confidence = "medium" +description = "Feature flag or plan-based gating in Java" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list + (string_literal + (string_fragment) @feature)) +) @match +""" + +[rule.predicates.method_name] +match = "^(hasFeature|isFeatureEnabled|checkFeature|hasPlan|isPlanActive)$" + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (featureFlags.hasFeature("advanced-analytics")) { enable(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (featureFlags.isFeatureEnabled("beta-dashboard")) { enable(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (subscription.hasPlan("pro")) { enable(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (validator.isEnabled("field")) { validate(); } + } +} +""" +expect_match = false diff --git a/rules/java/has-role-call.toml b/rules/java/has-role-call.toml new file mode 100644 index 0000000..28787f1 --- /dev/null +++ b/rules/java/has-role-call.toml @@ -0,0 +1,79 @@ +[rule] +id = "java-has-role-call" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Spring Security hasRole/hasAuthority method call" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.method_name] +match = "^(hasRole|hasAnyRole|hasAuthority|hasAnyAuthority)$" + +[rule.rego_template] +template = """ +default allow := false + +# granted_authority covers both roles (e.g. "ROLE_ADMIN") and +# authorities/permissions (e.g. "SCOPE_read") per Spring Security's +# GrantedAuthority abstraction. +allow if { + input.user.granted_authority in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure() { + http.authorizeRequests().antMatchers("/admin/**").hasRole("ADMIN"); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure() { + http.authorizeRequests().antMatchers("/ops/**").hasAnyRole("ADMIN", "OPS"); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure() { + http.authorizeRequests().antMatchers("/api/**").hasAnyAuthority("SCOPE_read", "SCOPE_write"); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure() { + http.authorizeRequests().antMatchers("/reports/**").hasAuthority("SCOPE_reports.read"); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure() { + http.authorizeRequests().antMatchers("/public/**").permitAll(); + } +} +""" +expect_match = false diff --git a/rules/java/http-security-authorize.toml b/rules/java/http-security-authorize.toml new file mode 100644 index 0000000..c232320 --- /dev/null +++ b/rules/java/http-security-authorize.toml @@ -0,0 +1,35 @@ +[rule] +id = "java-http-security-authorize" +languages = ["java"] +category = "middleware" +confidence = "high" +description = "Spring Security HttpSecurity authorization configuration" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list) +) @match +""" + +[rule.predicates.method_name] +match = "^(authorizeRequests|authorizeHttpRequests|formLogin|httpBasic|oauth2Login|oauth2ResourceServer)$" + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure(HttpSecurity http) { + http.authorizeRequests().anyRequest().authenticated(); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecurityConfig { + public void configure(HttpSecurity http) { + http.csrf().disable(); + } +} +""" +expect_match = false diff --git a/rules/java/is-user-in-role.toml b/rules/java/is-user-in-role.toml new file mode 100644 index 0000000..d1b5ce9 --- /dev/null +++ b/rules/java/is-user-in-role.toml @@ -0,0 +1,46 @@ +[rule] +id = "java-is-user-in-role" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Servlet API isUserInRole() call" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.method_name] +eq = "isUserInRole" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class MyServlet { + public void doGet() { + if (request.isUserInRole("admin")) { doSomething(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class MyServlet { + public void doGet() { + if (request.isAuthenticated()) { doSomething(); } + } +} +""" +expect_match = false diff --git a/rules/java/ownership-check.toml b/rules/java/ownership-check.toml new file mode 100644 index 0000000..48d7c24 --- /dev/null +++ b/rules/java/ownership-check.toml @@ -0,0 +1,45 @@ +[rule] +id = "java-ownership-check" +languages = ["java"] +category = "ownership" +confidence = "medium" +description = "Resource ownership comparison (e.g., getUserId().equals(resource.getOwnerId()))" +query = """ +(method_invocation + object: (method_invocation + name: (identifier) @getter) + name: (identifier) @method_name + arguments: (argument_list + (method_invocation + name: (identifier) @other_getter)) +) @match +""" + +[rule.predicates.getter] +match = "(?i)^(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)$" + +[rule.predicates.method_name] +eq = "equals" + +[rule.predicates.other_getter] +match = "(?i)^(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)$" + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (user.getUserId().equals(resource.getOwnerId())) { allow(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (user.getName().equals(other.getName())) { compare(); } + } +} +""" +expect_match = false diff --git a/rules/java/role-equals-check.toml b/rules/java/role-equals-check.toml new file mode 100644 index 0000000..c9d3112 --- /dev/null +++ b/rules/java/role-equals-check.toml @@ -0,0 +1,61 @@ +[rule] +id = "java-role-equals-check" +languages = ["java"] +category = "rbac" +confidence = "medium" +description = "Role comparison via .equals() or string equality" +query = """ +(method_invocation + object: (method_invocation + name: (identifier) @getter) + name: (identifier) @method_name + arguments: (argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.getter] +match = "^(getRole|getRoles|getAuthority|getAuthorities)$" + +[rule.predicates.method_name] +match = "^(equals|equalsIgnoreCase)$" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (user.getRole().equals("admin")) { doSomething(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (user.getRole().contains("ad")) { doSomething(); } + } +} +""" +expect_match = false + +[[rule.tests]] +input = """ +public class Service { + public void check() { + if (user.getName().equals("admin")) { doSomething(); } + } +} +""" +expect_match = false diff --git a/rules/java/security-interface-impl.toml b/rules/java/security-interface-impl.toml new file mode 100644 index 0000000..97c85f2 --- /dev/null +++ b/rules/java/security-interface-impl.toml @@ -0,0 +1,62 @@ +[rule] +id = "java-security-interface-impl" +languages = ["java"] +category = "custom" +confidence = "medium" +description = "Implementation of Spring Security interfaces (UserDetailsService, etc.)" +query = """ +(class_declaration + (super_interfaces + (type_list + (type_identifier) @iface_name)) +) @match + +(class_declaration + (super_interfaces + (type_list + (generic_type + (type_identifier) @iface_name))) +) @match + +(class_declaration + (super_interfaces + (type_list + (scoped_type_identifier + name: (type_identifier) @iface_name))) +) @match +""" + +[rule.predicates.iface_name] +match = "^(UserDetailsService|AuthenticationProvider|AuthorizationManager|GrantedAuthority|SecurityConfigurer|WebSecurityConfigurer)$" + +[[rule.tests]] +input = """ +public class MyUserService implements UserDetailsService { + public UserDetails loadUserByUsername(String username) { return null; } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class MyUserService implements Serializable, UserDetailsService { + public UserDetails loadUserByUsername(String username) { return null; } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class MyAuthManager implements AuthorizationManager { + public AuthorizationDecision check() { return null; } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class MyService implements Serializable { + public void doWork() { } +} +""" +expect_match = false diff --git a/rules/java/shiro-is-permitted.toml b/rules/java/shiro-is-permitted.toml new file mode 100644 index 0000000..2f224fc --- /dev/null +++ b/rules/java/shiro-is-permitted.toml @@ -0,0 +1,56 @@ +[rule] +id = "java-shiro-is-permitted" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Apache Shiro Subject.isPermitted() call" +query = """ +(method_invocation + name: (identifier) @method_name + arguments: (argument_list + (string_literal + (string_fragment) @perm_value)) +) @match +""" + +[rule.predicates.method_name] +match = "^(isPermitted|isPermittedAll|checkPermission|checkPermissions)$" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + "{{perm_value}}" in input.user.permissions +} +""" + +[[rule.tests]] +input = """ +public class Service { + public void delete() { + if (subject.isPermitted("user:delete")) { doIt(); } + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void delete() { + subject.checkPermission("user:delete"); + } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Service { + public void delete() { + subject.doAction("user:delete"); + } +} +""" +expect_match = false diff --git a/rules/java/shiro-requires-authentication.toml b/rules/java/shiro-requires-authentication.toml new file mode 100644 index 0000000..be2ab66 --- /dev/null +++ b/rules/java/shiro-requires-authentication.toml @@ -0,0 +1,50 @@ +[rule] +id = "java-shiro-requires-authentication" +languages = ["java"] +category = "middleware" +confidence = "high" +description = "Apache Shiro @RequiresAuthentication, @RequiresGuest, or @RequiresUser annotation" +query = """ +(marker_annotation + name: (identifier) @anno_name +) @match +""" + +[rule.predicates.anno_name] +match = "^(RequiresAuthentication|RequiresGuest|RequiresUser)$" + +[[rule.tests]] +input = """ +public class SecureController { + @RequiresAuthentication + public void secureAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecureController { + @RequiresGuest + public void guestAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecureController { + @RequiresUser + public void userAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class Controller { + @Override + public void action() { } +} +""" +expect_match = false diff --git a/rules/java/shiro-requires-permissions.toml b/rules/java/shiro-requires-permissions.toml new file mode 100644 index 0000000..f144047 --- /dev/null +++ b/rules/java/shiro-requires-permissions.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-shiro-requires-permissions" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Apache Shiro @RequiresPermissions annotation" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (string_literal + (string_fragment) @perm_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "RequiresPermissions" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + "{{perm_value}}" in input.user.permissions +} +""" + +[[rule.tests]] +input = """ +public class UserController { + @RequiresPermissions("user:delete") + public void deleteUser(Long id) { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class UserController { + @Transactional + public void deleteUser(Long id) { } +} +""" +expect_match = false diff --git a/rules/java/shiro-requires-roles-array.toml b/rules/java/shiro-requires-roles-array.toml new file mode 100644 index 0000000..3e287d5 --- /dev/null +++ b/rules/java/shiro-requires-roles-array.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-shiro-requires-roles-array" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Apache Shiro @RequiresRoles annotation with array of roles" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (element_value_array_initializer + (string_literal) @role_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "RequiresRoles" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {{{role_value}}} +} +""" + +[[rule.tests]] +input = """ +public class AdminController { + @RequiresRoles({"admin", "manager"}) + public void adminAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class AdminController { + @Transactional + public void adminAction() { } +} +""" +expect_match = false diff --git a/rules/java/shiro-requires-roles.toml b/rules/java/shiro-requires-roles.toml new file mode 100644 index 0000000..5b07968 --- /dev/null +++ b/rules/java/shiro-requires-roles.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-shiro-requires-roles" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Apache Shiro @RequiresRoles annotation" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "RequiresRoles" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class AdminController { + @RequiresRoles("admin") + public void adminAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class AdminController { + @Transactional + public void adminAction() { } +} +""" +expect_match = false diff --git a/rules/java/spring-permit-all.toml b/rules/java/spring-permit-all.toml new file mode 100644 index 0000000..7848c53 --- /dev/null +++ b/rules/java/spring-permit-all.toml @@ -0,0 +1,41 @@ +[rule] +id = "java-spring-permit-all" +languages = ["java"] +category = "middleware" +confidence = "medium" +description = "Spring Security @PermitAll or @DenyAll annotation" +query = """ +(marker_annotation + name: (identifier) @anno_name +) @match +""" + +[rule.predicates.anno_name] +match = "^(PermitAll|DenyAll)$" + +[[rule.tests]] +input = """ +public class PublicController { + @PermitAll + public void publicEndpoint() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class AdminController { + @DenyAll + public void lockedEndpoint() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class UserController { + @Override + public void doSomething() { } +} +""" +expect_match = false diff --git a/rules/java/spring-preauthorize.toml b/rules/java/spring-preauthorize.toml new file mode 100644 index 0000000..d35f5fe --- /dev/null +++ b/rules/java/spring-preauthorize.toml @@ -0,0 +1,54 @@ +[rule] +id = "java-spring-preauthorize" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Spring Security @PreAuthorize annotation" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (string_literal + (string_fragment) @expr)) +) @match +""" + +[rule.predicates.anno_name] +eq = "PreAuthorize" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + # TODO: translate SpEL expression: {{expr}} + input.user.role in {"TODO"} +} +""" + +[[rule.tests]] +input = """ +public class AdminController { + @PreAuthorize("hasRole('ADMIN')") + public void deleteUser(Long id) { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class AdminController { + @PreAuthorize("hasAuthority('SCOPE_read')") + public void readData() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class AdminController { + @Override + public void deleteUser(Long id) { } +} +""" +expect_match = false diff --git a/rules/java/spring-roles-allowed-array.toml b/rules/java/spring-roles-allowed-array.toml new file mode 100644 index 0000000..07ed6fe --- /dev/null +++ b/rules/java/spring-roles-allowed-array.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-roles-allowed-array" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Jakarta/JSR-250 @RolesAllowed annotation with array of roles" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (element_value_array_initializer + (string_literal) @role_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "RolesAllowed" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {{{role_value}}} +} +""" + +[[rule.tests]] +input = """ +public class UserController { + @RolesAllowed({"admin", "manager"}) + public void deleteUser(Long id) { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class UserController { + @Transactional + public void deleteUser(Long id) { } +} +""" +expect_match = false diff --git a/rules/java/spring-roles-allowed.toml b/rules/java/spring-roles-allowed.toml new file mode 100644 index 0000000..6089305 --- /dev/null +++ b/rules/java/spring-roles-allowed.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-roles-allowed" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Jakarta/JSR-250 @RolesAllowed annotation" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "RolesAllowed" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class UserController { + @RolesAllowed("admin") + public void deleteUser(Long id) { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class UserController { + @Transactional + public void deleteUser(Long id) { } +} +""" +expect_match = false diff --git a/rules/java/spring-secured.toml b/rules/java/spring-secured.toml new file mode 100644 index 0000000..5769b28 --- /dev/null +++ b/rules/java/spring-secured.toml @@ -0,0 +1,44 @@ +[rule] +id = "java-spring-secured" +languages = ["java"] +category = "rbac" +confidence = "high" +description = "Spring Security @Secured annotation" +query = """ +(annotation + name: (identifier) @anno_name + arguments: (annotation_argument_list + (string_literal + (string_fragment) @role_value)) +) @match +""" + +[rule.predicates.anno_name] +eq = "Secured" + +[rule.rego_template] +template = """ +default allow := false + +allow if { + input.user.role in {"{{role_value}}"} +} +""" + +[[rule.tests]] +input = """ +public class UserController { + @Secured("ROLE_ADMIN") + public void deleteUser(Long id) { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class UserController { + @Override + public void deleteUser(Long id) { } +} +""" +expect_match = false diff --git a/src/commands/scan.rs b/src/commands/scan.rs index d0939c7..546e46b 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -26,6 +26,15 @@ pub fn execute(args: ScanArgs, config: ZiftConfig) -> Result<()> { ); } + // Warn about explicitly requested languages that lack parser support + for lang in &args.language { + if !scanner::parser::is_language_supported(*lang) { + eprintln!( + "warning: {lang} scanning is not yet supported — {lang} files will be skipped" + ); + } + } + let loaded_rules = rules::load_rules(args.rules_dir.as_deref(), &config)?; tracing::info!("loaded {} pattern rules", loaded_rules.len()); diff --git a/src/rules/embedded.rs b/src/rules/embedded.rs index 1693464..dc149b3 100644 --- a/src/rules/embedded.rs +++ b/src/rules/embedded.rs @@ -3,6 +3,7 @@ use crate::error::Result; use super::{PatternRule, parse_rule}; const EMBEDDED_RULES: &[(&str, &str)] = &[ + // -- TypeScript / JavaScript -- ( "role-check-conditional", include_str!("../../rules/typescript/role-check-conditional.toml"), @@ -55,6 +56,83 @@ const EMBEDDED_RULES: &[(&str, &str)] = &[ "feature-gate-check", include_str!("../../rules/typescript/feature-gate-check.toml"), ), + // -- Java -- + ( + "java-spring-preauthorize", + include_str!("../../rules/java/spring-preauthorize.toml"), + ), + ( + "java-spring-secured", + include_str!("../../rules/java/spring-secured.toml"), + ), + ( + "java-roles-allowed", + include_str!("../../rules/java/spring-roles-allowed.toml"), + ), + ( + "java-roles-allowed-array", + include_str!("../../rules/java/spring-roles-allowed-array.toml"), + ), + ( + "java-spring-permit-all", + include_str!("../../rules/java/spring-permit-all.toml"), + ), + ( + "java-is-user-in-role", + include_str!("../../rules/java/is-user-in-role.toml"), + ), + ( + "java-has-role-call", + include_str!("../../rules/java/has-role-call.toml"), + ), + ( + "java-shiro-requires-permissions", + include_str!("../../rules/java/shiro-requires-permissions.toml"), + ), + ( + "java-shiro-requires-roles", + include_str!("../../rules/java/shiro-requires-roles.toml"), + ), + ( + "java-shiro-requires-roles-array", + include_str!("../../rules/java/shiro-requires-roles-array.toml"), + ), + ( + "java-shiro-requires-authentication", + include_str!("../../rules/java/shiro-requires-authentication.toml"), + ), + ( + "java-shiro-is-permitted", + include_str!("../../rules/java/shiro-is-permitted.toml"), + ), + ( + "java-role-equals-check", + include_str!("../../rules/java/role-equals-check.toml"), + ), + ( + "java-ownership-check", + include_str!("../../rules/java/ownership-check.toml"), + ), + ( + "java-http-security-authorize", + include_str!("../../rules/java/http-security-authorize.toml"), + ), + ( + "java-authenticated-check", + include_str!("../../rules/java/authenticated-check.toml"), + ), + ( + "java-access-decision-voter", + include_str!("../../rules/java/access-decision-voter.toml"), + ), + ( + "java-security-interface-impl", + include_str!("../../rules/java/security-interface-impl.toml"), + ), + ( + "java-feature-gate-check", + include_str!("../../rules/java/feature-gate-check.toml"), + ), ]; pub fn load_embedded_rules() -> Result> { diff --git a/src/scanner/discovery.rs b/src/scanner/discovery.rs index 7d1076c..832324e 100644 --- a/src/scanner/discovery.rs +++ b/src/scanner/discovery.rs @@ -13,12 +13,13 @@ pub struct DiscoveredFile { } pub fn detect_language(path: &Path) -> Option<(Language, bool)> { - let ext = path.extension()?.to_str()?; - match ext { + let ext = path.extension()?.to_str()?.to_ascii_lowercase(); + match ext.as_str() { "ts" => Some((Language::TypeScript, false)), "tsx" => Some((Language::TypeScript, true)), "js" | "mjs" | "cjs" => Some((Language::JavaScript, false)), "jsx" => Some((Language::JavaScript, true)), + "java" => Some((Language::Java, false)), _ => None, } } @@ -103,6 +104,14 @@ mod tests { ); } + #[test] + fn detect_java_extension() { + assert_eq!( + detect_language(Path::new("Foo.java")), + Some((Language::Java, false)) + ); + } + #[test] fn detect_unknown_extension() { assert_eq!(detect_language(Path::new("foo.rs")), None); diff --git a/src/scanner/matcher.rs b/src/scanner/matcher.rs index 504c307..a5dc0ea 100644 --- a/src/scanner/matcher.rs +++ b/src/scanner/matcher.rs @@ -72,7 +72,7 @@ pub fn execute_query( "rule '{}': capture index {} out of range (max {})", compiled.rule.id, capture.index, - compiled.capture_names.len() - 1, + compiled.capture_names.len(), )) })?; let text = capture @@ -287,6 +287,205 @@ mod tests { assert!(!findings.is_empty()); } + // -- Java rule tests -- + + fn parse_and_match_java(source: &str, rule_toml: &str) -> Vec { + let rule = rules::parse_rule_for_test(rule_toml); + let mut ts_parser = tree_sitter::Parser::new(); + let lang = Language::Java; + let ts_lang = parser::get_language(lang, false).unwrap(); + // Wrap in a class+method if not already a class declaration + let wrapped = if source.contains("class ") { + source.to_string() + } else { + format!("public class Test {{ public void test() {{ {source} }} }}") + }; + let tree = parser::parse_source(&mut ts_parser, wrapped.as_bytes(), lang, false).unwrap(); + let compiled = compile_rule(&rule, &ts_lang).unwrap(); + execute_query( + &compiled, + &tree, + wrapped.as_bytes(), + Path::new("Test.java"), + lang, + ) + .unwrap() + } + + #[test] + fn java_preauthorize_matches() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @PreAuthorize("hasRole('ADMIN')") + public void delete() { } +} +"#, + include_str!("../../rules/java/spring-preauthorize.toml"), + ); + assert!(!findings.is_empty(), "should match @PreAuthorize"); + assert_eq!(findings[0].category, crate::types::AuthCategory::Rbac); + } + + #[test] + fn java_preauthorize_no_false_positive() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @Override + public void delete() { } +} +"#, + include_str!("../../rules/java/spring-preauthorize.toml"), + ); + assert!(findings.is_empty()); + } + + #[test] + fn java_secured_matches() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @Secured("ROLE_ADMIN") + public void delete() { } +} +"#, + include_str!("../../rules/java/spring-secured.toml"), + ); + assert!(!findings.is_empty(), "should match @Secured"); + } + + #[test] + fn java_roles_allowed_matches() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @RolesAllowed("admin") + public void delete() { } +} +"#, + include_str!("../../rules/java/spring-roles-allowed.toml"), + ); + assert!(!findings.is_empty(), "should match @RolesAllowed"); + } + + #[test] + fn java_is_user_in_role_matches() { + let findings = parse_and_match_java( + r#"request.isUserInRole("admin");"#, + include_str!("../../rules/java/is-user-in-role.toml"), + ); + assert!(!findings.is_empty(), "should match isUserInRole"); + } + + #[test] + fn java_has_role_call_matches() { + let findings = parse_and_match_java( + r#"http.authorizeRequests().antMatchers("/admin/**").hasRole("ADMIN");"#, + include_str!("../../rules/java/has-role-call.toml"), + ); + assert!(!findings.is_empty(), "should match hasRole"); + } + + #[test] + fn java_shiro_requires_permissions_matches() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @RequiresPermissions("user:delete") + public void delete() { } +} +"#, + include_str!("../../rules/java/shiro-requires-permissions.toml"), + ); + assert!(!findings.is_empty(), "should match @RequiresPermissions"); + } + + #[test] + fn java_shiro_is_permitted_matches() { + let findings = parse_and_match_java( + r#"subject.isPermitted("user:delete");"#, + include_str!("../../rules/java/shiro-is-permitted.toml"), + ); + assert!(!findings.is_empty(), "should match isPermitted"); + } + + #[test] + fn java_marker_annotation_matches() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @RequiresAuthentication + public void secure() { } +} +"#, + include_str!("../../rules/java/shiro-requires-authentication.toml"), + ); + assert!( + !findings.is_empty(), + "should match @RequiresAuthentication marker annotation" + ); + } + + #[test] + fn java_marker_annotation_no_false_positive() { + let findings = parse_and_match_java( + r#" +public class Ctrl { + @Override + public void toString() { } +} +"#, + include_str!("../../rules/java/shiro-requires-authentication.toml"), + ); + assert!(findings.is_empty(), "should not match @Override"); + } + + #[test] + fn java_role_equals_check_matches() { + let findings = parse_and_match_java( + r#"user.getRole().equals("admin");"#, + include_str!("../../rules/java/role-equals-check.toml"), + ); + assert!(!findings.is_empty(), "should match getRole().equals()"); + } + + #[test] + fn java_role_equals_check_no_false_positive() { + let findings = parse_and_match_java( + r#"user.getName().equals("admin");"#, + include_str!("../../rules/java/role-equals-check.toml"), + ); + assert!(findings.is_empty(), "should not match getName().equals()"); + } + + #[test] + fn java_ownership_check_matches() { + let findings = parse_and_match_java( + r#"user.getUserId().equals(resource.getOwnerId());"#, + include_str!("../../rules/java/ownership-check.toml"), + ); + assert!(!findings.is_empty(), "should match ownership check"); + } + + #[test] + fn java_authenticated_check_matches() { + let findings = parse_and_match_java( + r#"authentication.isAuthenticated();"#, + include_str!("../../rules/java/authenticated-check.toml"), + ); + assert!(!findings.is_empty(), "should match isAuthenticated()"); + } + + #[test] + fn java_feature_gate_matches() { + let findings = parse_and_match_java( + r#"featureFlags.hasFeature("advanced");"#, + include_str!("../../rules/java/feature-gate-check.toml"), + ); + assert!(!findings.is_empty(), "should match hasFeature()"); + } + #[test] fn dedup_removes_duplicates() { let f = Finding { diff --git a/src/scanner/parser.rs b/src/scanner/parser.rs index c70a0d8..3a5fc69 100644 --- a/src/scanner/parser.rs +++ b/src/scanner/parser.rs @@ -3,11 +3,17 @@ use tree_sitter::Tree; use crate::error::{Result, ZiftError}; use crate::types::Language; +/// Returns true if the language has tree-sitter parser support. +pub fn is_language_supported(lang: Language) -> bool { + get_language(lang, false).is_ok() +} + pub fn get_language(lang: Language, is_tsx_jsx: bool) -> Result { match (lang, is_tsx_jsx) { (Language::TypeScript, false) => Ok(tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into()), (Language::TypeScript, true) => Ok(tree_sitter_typescript::LANGUAGE_TSX.into()), (Language::JavaScript, _) => Ok(tree_sitter_javascript::LANGUAGE.into()), + (Language::Java, _) => Ok(tree_sitter_java::LANGUAGE.into()), _ => Err(ZiftError::General(format!( "language {lang:?} not yet supported" ))), @@ -58,8 +64,22 @@ mod tests { assert!(!tree.root_node().has_error()); } + #[test] + fn parse_java() { + let mut parser = tree_sitter::Parser::new(); + let source = b"public class Foo { public void bar() {} }"; + let tree = parse_source(&mut parser, source, Language::Java, false).unwrap(); + assert!(!tree.root_node().has_error()); + } + + #[test] + fn java_is_supported() { + assert!(is_language_supported(Language::Java)); + } + #[test] fn unsupported_language_returns_error() { assert!(get_language(Language::Python, false).is_err()); + assert!(!is_language_supported(Language::Python)); } }