From 25d0c3fa7b4683f343181db8df5b98e70f9f2472 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:15:16 -0400 Subject: [PATCH 1/9] feat: add Java language support with 18 authz detection rules Add tree-sitter-java parser and 18 pattern rules covering Spring Security, Apache Shiro, Jakarta @RolesAllowed, Servlet API, and custom authorization patterns. Includes unsupported-language warning in scan command and cargo fmt formatting fixes. --- Cargo.lock | 11 + Cargo.toml | 1 + rules/java/access-decision-voter.toml | 31 +++ rules/java/authenticated-check.toml | 35 +++ rules/java/feature-gate-check.toml | 37 ++++ rules/java/has-role-call.toml | 46 ++++ rules/java/http-security-authorize.toml | 35 +++ rules/java/is-user-in-role.toml | 46 ++++ rules/java/ownership-check.toml | 45 ++++ rules/java/role-equals-check.toml | 51 +++++ rules/java/security-interface-impl.toml | 32 +++ rules/java/shiro-is-permitted.toml | 56 +++++ rules/java/shiro-requires-authentication.toml | 41 ++++ rules/java/shiro-requires-permissions.toml | 44 ++++ rules/java/shiro-requires-roles.toml | 44 ++++ rules/java/spring-permit-all.toml | 41 ++++ rules/java/spring-preauthorize.toml | 54 +++++ rules/java/spring-roles-allowed-array.toml | 35 +++ rules/java/spring-roles-allowed.toml | 44 ++++ rules/java/spring-secured.toml | 44 ++++ src/commands/scan.rs | 9 + src/rules/embedded.rs | 74 +++++++ src/scanner/discovery.rs | 9 + src/scanner/matcher.rs | 201 +++++++++++++++++- src/scanner/parser.rs | 20 ++ 25 files changed, 1085 insertions(+), 1 deletion(-) create mode 100644 rules/java/access-decision-voter.toml create mode 100644 rules/java/authenticated-check.toml create mode 100644 rules/java/feature-gate-check.toml create mode 100644 rules/java/has-role-call.toml create mode 100644 rules/java/http-security-authorize.toml create mode 100644 rules/java/is-user-in-role.toml create mode 100644 rules/java/ownership-check.toml create mode 100644 rules/java/role-equals-check.toml create mode 100644 rules/java/security-interface-impl.toml create mode 100644 rules/java/shiro-is-permitted.toml create mode 100644 rules/java/shiro-requires-authentication.toml create mode 100644 rules/java/shiro-requires-permissions.toml create mode 100644 rules/java/shiro-requires-roles.toml create mode 100644 rules/java/spring-permit-all.toml create mode 100644 rules/java/spring-preauthorize.toml create mode 100644 rules/java/spring-roles-allowed-array.toml create mode 100644 rules/java/spring-roles-allowed.toml create mode 100644 rules/java/spring-secured.toml 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..01c358b --- /dev/null +++ b/rules/java/access-decision-voter.toml @@ -0,0 +1,31 @@ +[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 +""" + +[rule.predicates.parent_class] +match = "^(AccessDecisionVoter|AbstractAccessDecisionManager|SecurityInterceptor|AbstractSecurityInterceptor|PermissionEvaluator)$" + +[[rule.tests]] +input = """ +public class CustomVoter extends 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..2971dc5 --- /dev/null +++ b/rules/java/feature-gate-check.toml @@ -0,0 +1,37 @@ +[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 (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..d812e7d --- /dev/null +++ b/rules/java/has-role-call.toml @@ -0,0 +1,46 @@ +[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 + +allow if { + input.user.role 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("/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..8dc9d4e --- /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..6c67a1d --- /dev/null +++ b/rules/java/role-equals-check.toml @@ -0,0 +1,51 @@ +[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|contains|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.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..04208cd --- /dev/null +++ b/rules/java/security-interface-impl.toml @@ -0,0 +1,32 @@ +[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 +""" + +[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 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..6d0d81c --- /dev/null +++ b/rules/java/shiro-requires-authentication.toml @@ -0,0 +1,41 @@ +[rule] +id = "java-shiro-requires-authentication" +languages = ["java"] +category = "middleware" +confidence = "high" +description = "Apache Shiro @RequiresAuthentication or @RequiresGuest 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 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.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..88badc6 --- /dev/null +++ b/rules/java/spring-roles-allowed-array.toml @@ -0,0 +1,35 @@ +[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.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..de31dfd 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,79 @@ 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-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..c0d9b1e 100644 --- a/src/scanner/discovery.rs +++ b/src/scanner/discovery.rs @@ -19,6 +19,7 @@ pub fn detect_language(path: &Path) -> Option<(Language, bool)> { "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)); } } From 5da4a33ba8eac5ee3b34075b1a3cdaa54eae0f00 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:29:09 -0400 Subject: [PATCH 2/9] fix: address PR review feedback - Add missing rego_template to spring-roles-allowed-array rule - Expand access-decision-voter to match both extends and implements - Anchor ownership-check getter regexes to prevent substring matches - Fix shiro-requires-authentication description to include @RequiresUser --- rules/java/access-decision-voter.toml | 16 +++++++++++++++- rules/java/ownership-check.toml | 4 ++-- rules/java/shiro-requires-authentication.toml | 2 +- rules/java/spring-roles-allowed-array.toml | 9 +++++++++ 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/rules/java/access-decision-voter.toml b/rules/java/access-decision-voter.toml index 01c358b..6654a27 100644 --- a/rules/java/access-decision-voter.toml +++ b/rules/java/access-decision-voter.toml @@ -9,6 +9,12 @@ query = """ (superclass (type_identifier) @parent_class) ) @match + +(class_declaration + (super_interfaces + (type_list + (type_identifier) @parent_class)) +) @match """ [rule.predicates.parent_class] @@ -16,7 +22,15 @@ match = "^(AccessDecisionVoter|AbstractAccessDecisionManager|SecurityInterceptor [[rule.tests]] input = """ -public class CustomVoter extends AccessDecisionVoter { +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; } } """ diff --git a/rules/java/ownership-check.toml b/rules/java/ownership-check.toml index 8dc9d4e..48d7c24 100644 --- a/rules/java/ownership-check.toml +++ b/rules/java/ownership-check.toml @@ -16,13 +16,13 @@ query = """ """ [rule.predicates.getter] -match = "(?i)(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)" +match = "(?i)^(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)$" [rule.predicates.method_name] eq = "equals" [rule.predicates.other_getter] -match = "(?i)(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)" +match = "(?i)^(getId|getUserId|getOwnerId|getCreatedBy|getAuthorId)$" [[rule.tests]] input = """ diff --git a/rules/java/shiro-requires-authentication.toml b/rules/java/shiro-requires-authentication.toml index 6d0d81c..f29e8ae 100644 --- a/rules/java/shiro-requires-authentication.toml +++ b/rules/java/shiro-requires-authentication.toml @@ -3,7 +3,7 @@ id = "java-shiro-requires-authentication" languages = ["java"] category = "middleware" confidence = "high" -description = "Apache Shiro @RequiresAuthentication or @RequiresGuest annotation" +description = "Apache Shiro @RequiresAuthentication, @RequiresGuest, or @RequiresUser annotation" query = """ (marker_annotation name: (identifier) @anno_name diff --git a/rules/java/spring-roles-allowed-array.toml b/rules/java/spring-roles-allowed-array.toml index 88badc6..07ed6fe 100644 --- a/rules/java/spring-roles-allowed-array.toml +++ b/rules/java/spring-roles-allowed-array.toml @@ -16,6 +16,15 @@ query = """ [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 { From 67fddc203b665a3872f18353b0fddb11f8157df9 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:46:34 -0400 Subject: [PATCH 3/9] chore: update address-pr-feedback command with bot prefixes and pagination Sync with ea/spicy versions: add /q prefix for Amazon Q replies, @coderabbitai prefix for CodeRabbit replies, pagination warnings, token budget awareness, and two-pass comment fetching. --- .claude/commands/address-pr-feedback.md | 170 +++++++++++++++++++++--- 1 file changed, 148 insertions(+), 22 deletions(-) diff --git a/.claude/commands/address-pr-feedback.md b/.claude/commands/address-pr-feedback.md index b070f47..570f517 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,180 @@ 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 +- **"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 and duplicates 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 +200,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 +210,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 From 11f7a050d928f108e2e71c0453ac6d9ffa7ec5e7 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:52:14 -0400 Subject: [PATCH 4/9] fix: address CodeRabbit nitpick feedback - Add hasAnyRole/hasAnyAuthority tests to has-role-call rule - Add isFeatureEnabled/hasPlan tests to feature-gate-check rule - Add @RequiresUser test to shiro-requires-authentication rule - Add multi-interface test to security-interface-impl rule - Add shiro-requires-roles-array rule for array form @RequiresRoles - Normalize file extension casing in discovery.rs - Add outside-diff-range handling to address-pr-feedback command --- .claude/commands/address-pr-feedback.md | 3 +- rules/java/feature-gate-check.toml | 20 +++++++++ rules/java/has-role-call.toml | 20 +++++++++ rules/java/security-interface-impl.toml | 8 ++++ rules/java/shiro-requires-authentication.toml | 9 ++++ rules/java/shiro-requires-roles-array.toml | 44 +++++++++++++++++++ src/rules/embedded.rs | 4 ++ src/scanner/discovery.rs | 4 +- 8 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 rules/java/shiro-requires-roles-array.toml diff --git a/.claude/commands/address-pr-feedback.md b/.claude/commands/address-pr-feedback.md index 570f517..32b1a3a 100644 --- a/.claude/commands/address-pr-feedback.md +++ b/.claude/commands/address-pr-feedback.md @@ -94,9 +94,10 @@ gh api "repos/{owner}/{repo}/pulls/{pr}/reviews/{review_id}" \ 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 -**The inline comments (3a) are only the Critical/Major items. Nitpicks and duplicates 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. +**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. #### 3c. Amazon Q general comments (if any) diff --git a/rules/java/feature-gate-check.toml b/rules/java/feature-gate-check.toml index 2971dc5..017ec39 100644 --- a/rules/java/feature-gate-check.toml +++ b/rules/java/feature-gate-check.toml @@ -26,6 +26,26 @@ public class Service { """ 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 { diff --git a/rules/java/has-role-call.toml b/rules/java/has-role-call.toml index d812e7d..114ce58 100644 --- a/rules/java/has-role-call.toml +++ b/rules/java/has-role-call.toml @@ -35,6 +35,26 @@ public class SecurityConfig { """ 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 { diff --git a/rules/java/security-interface-impl.toml b/rules/java/security-interface-impl.toml index 04208cd..7620722 100644 --- a/rules/java/security-interface-impl.toml +++ b/rules/java/security-interface-impl.toml @@ -23,6 +23,14 @@ public class MyUserService implements UserDetailsService { """ 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 MyService implements Serializable { diff --git a/rules/java/shiro-requires-authentication.toml b/rules/java/shiro-requires-authentication.toml index f29e8ae..be2ab66 100644 --- a/rules/java/shiro-requires-authentication.toml +++ b/rules/java/shiro-requires-authentication.toml @@ -31,6 +31,15 @@ public class SecureController { """ expect_match = true +[[rule.tests]] +input = """ +public class SecureController { + @RequiresUser + public void userAction() { } +} +""" +expect_match = true + [[rule.tests]] input = """ public class Controller { 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/src/rules/embedded.rs b/src/rules/embedded.rs index de31dfd..dc149b3 100644 --- a/src/rules/embedded.rs +++ b/src/rules/embedded.rs @@ -93,6 +93,10 @@ const EMBEDDED_RULES: &[(&str, &str)] = &[ "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"), diff --git a/src/scanner/discovery.rs b/src/scanner/discovery.rs index c0d9b1e..832324e 100644 --- a/src/scanner/discovery.rs +++ b/src/scanner/discovery.rs @@ -13,8 +13,8 @@ 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)), From c80e58542dfd683c7a2e62c2c3e7aad582aff7a8 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:54:43 -0400 Subject: [PATCH 5/9] chore: add .coderabbit.yaml to prevent trickle reviews --- .coderabbit.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .coderabbit.yaml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..e79ffad --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,9 @@ +# .coderabbit.yaml +reviews: + 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. From 0b776ed741cbd6c92dd84721fc5a988358a3a956 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 21:56:40 -0400 Subject: [PATCH 6/9] fix: remove contains from role-equals-check predicate Substring matching misaligns with the equality-focused rule and its Rego template. Added regression test to prevent re-broadening. --- rules/java/role-equals-check.toml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/rules/java/role-equals-check.toml b/rules/java/role-equals-check.toml index 6c67a1d..c9d3112 100644 --- a/rules/java/role-equals-check.toml +++ b/rules/java/role-equals-check.toml @@ -19,7 +19,7 @@ query = """ match = "^(getRole|getRoles|getAuthority|getAuthorities)$" [rule.predicates.method_name] -match = "^(equals|contains|equalsIgnoreCase)$" +match = "^(equals|equalsIgnoreCase)$" [rule.rego_template] template = """ @@ -40,6 +40,16 @@ public class Service { """ 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 { From 8244127b6db2666ae1f86ca0d3b4e2843398c6ba Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 22:09:46 -0400 Subject: [PATCH 7/9] fix: address PR review feedback round 3 - Fix has-role-call Rego template to use granted_authority instead of role - Add hasAuthority positive test case to has-role-call - Expand security-interface-impl to match generic type implementations --- rules/java/has-role-call.toml | 12 +++++++++++- rules/java/security-interface-impl.toml | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/rules/java/has-role-call.toml b/rules/java/has-role-call.toml index 114ce58..1280fcf 100644 --- a/rules/java/has-role-call.toml +++ b/rules/java/has-role-call.toml @@ -21,7 +21,7 @@ template = """ default allow := false allow if { - input.user.role in {"{{role_value}}"} + input.user.granted_authority in {"{{role_value}}"} } """ @@ -55,6 +55,16 @@ public class SecurityConfig { """ 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 { diff --git a/rules/java/security-interface-impl.toml b/rules/java/security-interface-impl.toml index 7620722..c5251b4 100644 --- a/rules/java/security-interface-impl.toml +++ b/rules/java/security-interface-impl.toml @@ -10,6 +10,13 @@ query = """ (type_list (type_identifier) @iface_name)) ) @match + +(class_declaration + (super_interfaces + (type_list + (generic_type + (type_identifier) @iface_name))) +) @match """ [rule.predicates.iface_name] @@ -31,6 +38,14 @@ public class MyUserService implements Serializable, UserDetailsService { """ 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 { From 52c74b1a7a5f0ee59577cde3d120d8e4a708a917 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 22:15:17 -0400 Subject: [PATCH 8/9] fix: add scoped_type_identifier pattern and document granted_authority - Handle fully-qualified interface implementations in security-interface-impl - Add inline comment documenting granted_authority field in Rego template --- rules/java/has-role-call.toml | 3 +++ rules/java/security-interface-impl.toml | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/rules/java/has-role-call.toml b/rules/java/has-role-call.toml index 1280fcf..28787f1 100644 --- a/rules/java/has-role-call.toml +++ b/rules/java/has-role-call.toml @@ -20,6 +20,9 @@ match = "^(hasRole|hasAnyRole|hasAuthority|hasAnyAuthority)$" 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}}"} } diff --git a/rules/java/security-interface-impl.toml b/rules/java/security-interface-impl.toml index c5251b4..97c85f2 100644 --- a/rules/java/security-interface-impl.toml +++ b/rules/java/security-interface-impl.toml @@ -17,6 +17,13 @@ query = """ (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] From b53b8cc16b5c0e5f786f9814eea21f955f885a9a Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Wed, 22 Apr 2026 22:33:19 -0400 Subject: [PATCH 9/9] chore: enable request_changes_workflow in CodeRabbit config --- .coderabbit.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index e79ffad..b157e7a 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,5 +1,6 @@ # .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.