fix(scanner/imports): use code-side alias for named ES imports#14
Merged
Conversation
`import { authorize as auth } from "../policy"` previously captured
`authorize` (the original name) instead of `auth` (the binding actually
used in code). The word-boundary regex in `is_enforcement_point` then
failed to match calls like `auth(...)`, misclassifying enforcement
points as inline auth findings.
Update `TS_NAMED_IMPORT_QUERY` to alternate on the `import_specifier`:
capture `alias` when present, otherwise capture `name` (using the
`!alias` negative-field operator). Add tests for purely-aliased,
mixed, and enforcement-point-via-alias cases.
Closes #8
Collapse the four `java_security_interface_impl_*` tests (plain, fully-qualified, generic, no-false-positive) into a single table-driven test. Failure messages include the case name so per-case context is preserved. Addresses CodeRabbit nitpick on #11.
The rule's inclusive `is|has|can` matcher and exclusive `hasAny*` not_match weren't directly fixture-tested. Add positive fixtures for `canEditRole` and `canManagePermission`, and explicit negative fixtures for `hasAnyRole` and `hasAnyAuthority` so neither side of the predicate can silently regress. Addresses CodeRabbit nitpick on #13.
…l name `detects_aliased_named_policy_import` asserted that the alias from a non-policy path (`Router as R`) is not captured, but didn't explicitly assert that the original name (`Router`) is also excluded. Add that assertion so both halves of the path-filtering contract on aliased imports are pinned, and split the inline comment into its two intents (policy-path alias capture vs. non-policy path filtering).
…call The rule's non-greedy inclusive regex `^(is|has|can)\w*?(Admin|...|Role|...)` is exercised for `is*` with keyword-in-middle (via `isAdminForAccount`) but not for `can*`. Add a `canAssignAdminRole` fixture so the `can`-prefixed keyword-in-middle case can't silently regress.
The `access-decision-voter` rule only matched `(type_list (type_identifier))`, so generic Spring Security interfaces like `AccessDecisionVoter<Object>` (which parse as `generic_type`) and fully-qualified names like `org.springframework.security.access. PermissionEvaluator` (which parse as `scoped_type_identifier`) were missed. The existing `AccessDecisionVoter<Object>` fixture in the rule's own test block had been failing on main as a result. Add `generic_type` and `scoped_type_identifier` query branches, mirroring `security-interface-impl.toml`, and pin the fully-qualified case with a new fixture.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR expands Java security rule coverage for interface matching (supporting generic and fully-qualified types), fixes TypeScript aliased import capture in the scanner, enhances authorization predicate test cases with Spring API exclusions, and refactors Java interface tests into a table-driven structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully fixes the aliased ES import bug (#8) and adds important missing functionality for Java rules. The changes are well-tested with comprehensive fixtures covering edge cases. All modifications are working correctly and ready to merge.
Key improvements:
- Aliased named imports now correctly capture the code-side binding (e.g.,
import { authorize as auth }captures "auth") - Java
access-decision-voterrule now matches generic types (AccessDecisionVoter<Object>) and fully-qualified interfaces - Enhanced test coverage with additional fixtures for
can*predicates and framework API exclusions - Test refactoring consolidates four separate tests into one table-driven test
The implementation correctly handles the alternation pattern in Tree-sitter queries and all tests pass (105/105 unit tests, 122/122 rule tests).
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
import { authorize as auth } from "../policy") so the regex inis_enforcement_pointmatches against the alias actually used in code, not the original name.access-decision-voterrule to match generic and fully-qualified Spring Security interfaces (e.g.AccessDecisionVoter<Object>,org.springframework.security.access.PermissionEvaluator). The existing fixture for the generic case had been failing onmain.java_security_interface_impl_*tests into one table-driven test, and pincan*/hasAny*predicates with explicit fixtures incustom-authz-call.Changes
src/scanner/imports.rs—TS_NAMED_IMPORT_QUERYnow uses an alternation[(import_specifier alias: ...) (import_specifier !alias name: ...)], capturing the alias when present and the original name otherwise. New tests cover aliased-only imports, mixed aliased+plain in one statement, and enforcement-point regex matching via alias. Existing aliased-import test also asserts the original non-policy name is excluded.src/scanner/matcher.rs— Fourjava_security_interface_impl_*tests collapsed into a single table-drivenjava_security_interface_impl_caseswith per-case context preserved in assertion messages. (CodeRabbit nitpick, fix(rules/java): repair invalid query in security-interface-impl #11)rules/java/custom-authz-call.toml— Added positive fixtures forcan*predicates (canEditRole,canManagePermission,canAssignAdminRolefor keyword-in-middle), and explicit negative fixtures forhasAnyRole/hasAnyAuthorityframework APIs. (CodeRabbit nitpick, feat(rules/java): heuristic detection of custom authz service calls #13)rules/java/access-decision-voter.toml— Addedgeneric_typeandscoped_type_identifierquery branches mirroringsecurity-interface-impl.toml. Pinned the fully-qualified case with a new fixture.Test plan
cargo checkcargo clippy --all-targets -- -D warningscargo fmt --checkcargo test— 105 / 105 passcargo run -- rules test— 122 / 122 pass (the 1 pre-existing failure onmainis now also fixed)Summary by CodeRabbit
Bug Fixes
Tests