feat: php (Laravel + Symfony) structural support#97
Conversation
Closes #86. Wires tree-sitter-php into the structural scanner and ships 11 rules covering Laravel Gates/Policies/route-middleware, Symfony Voters and #[IsGranted], plus three idiomatic role-comparison shapes. Policy-import propagation gains a PHP path: `use` declarations (simple, aliased, function/const, grouped), `assignment_expression`, constructor DI via `simple_parameter` typed args, and `property_element` defaults — matching how PHP DI threads a policy service through a controller field and out to call sites. Calibration: Monica (Laravel) at e08e917 surfaces 41 findings across the expected authz surface (AuthServiceProvider Gate::define vocabulary, VaultPolicy CRUD methods, can:* route middleware); symfony/demo at 83d4ac1 surfaces 4 findings hitting every Symfony rule. Docs in docs/corpus/php.md.
`->can()` and `->cannot()` are common method names well outside Laravel
authz — network clients, render gates, feature toggles, JWT tokens.
The rule previously fired on any receiver, producing high-confidence
false positives like `$networkClient->cannot('disconnect')` and
`$widget->can('render')`.
Constrain the receiver to principal-shaped names: `$this`, user-named
variables (`$user`, `$currentUser`, `$me`, …), or call chains ending in
`->user()` / `::user()` (the canonical `Auth::user()` / `auth()->user()`
/ `$request->user()` Laravel idioms). Also add `nullsafe_member_call_expression`
to the query alternation so PHP 8's `$user?->can(...)` matches the same
way as the bare `->` form.
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds PHP structural scanning: tree-sitter-php integration, ~10 PHP authorization rules (Laravel + Symfony + idiomatic role checks), import/propagation analysis for PHP DI patterns, embedded rule registration, extensive matcher and E2E tests, and documentation/corpus updates. ChangesPHP Structural Scanning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
This PR successfully adds PHP language support to the scanner with comprehensive coverage of Laravel and Symfony authorization patterns. The implementation follows the established architecture and includes thorough test coverage across parser, imports, matcher, and enforcement point tracking. No blocking issues identified.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/scanner/matcher.rs (1)
2159-2174: ⚡ Quick winAdd a direct
$this->isGranted(...)positive case for Symfony parity.This suite asserts
denyAccessUnlessGranted(...), but not a direct$this->isGranted(...)call. Adding one dedicated positive test would better lock the rule behavior you claim in PR scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scanner/matcher.rs` around lines 2159 - 2174, Add a new positive test case to assert that a direct $this->isGranted(...) call is matched: create or extend the existing test function (e.g., php_symfony_is_granted_matches) to include a snippet using $this->isGranted('EDIT', $post) and call parse_and_match_php with the same rules file (php/symfony-is-granted.toml), then assert !findings.is_empty() to ensure the rule matches the direct isGranted usage; keep the test structure consistent with the existing denyAccessUnlessGranted case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rules/php/laravel-authorize-helper.toml`:
- Around line 32-42: The receiver predicate in the rule for
nullsafe_member_call_expression only matches non-nullsafe chains like .+->user()
and should be widened to also accept nullsafe chains ?->user(); update the
predicate that targets the `@receiver/`@method_name pattern to allow both -> and
?-> (e.g., change the regex or pattern fragment to accept an optional ? before
->), ensure the rule continues to bind the same captures
(nullsafe_member_call_expression, `@receiver`, `@method_name`, `@ability`), and add a
test fixture demonstrating a nullsafe chain such as
$request?->user()?->can('view', $post) to validate the change.
In `@rules/php/symfony-is-granted-attribute.toml`:
- Around line 14-27: The attribute query misses fully-qualified names because
tree-sitter-php uses qualified_name for namespaced attributes; update the
pattern that captures the attribute name to accept both bare and qualified forms
(e.g., match either (name) `@attr_name` or (qualified_name (name) `@attr_name`) so
#[...\\IsGranted] is captured) and adjust the predicate for
rule.predicates.attr_name (currently eq = "IsGranted") to compare the captured
`@attr_name` text (the short name) rather than the whole qualified string so the
rule still matches when the attribute is namespaced.
---
Nitpick comments:
In `@src/scanner/matcher.rs`:
- Around line 2159-2174: Add a new positive test case to assert that a direct
$this->isGranted(...) call is matched: create or extend the existing test
function (e.g., php_symfony_is_granted_matches) to include a snippet using
$this->isGranted('EDIT', $post) and call parse_and_match_php with the same rules
file (php/symfony-is-granted.toml), then assert !findings.is_empty() to ensure
the rule matches the direct isGranted usage; keep the test structure consistent
with the existing denyAccessUnlessGranted case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb75cf79-6ac7-431e-b9ff-24a7abaa33a2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlREADME.mddocs/corpus/README.mddocs/corpus/php.mdrules/php/has-role-call.tomlrules/php/in-array-role-check.tomlrules/php/laravel-authorize-helper.tomlrules/php/laravel-gate-allows-denies.tomlrules/php/laravel-gate-define.tomlrules/php/laravel-policy-class.tomlrules/php/laravel-route-middleware.tomlrules/php/role-equals-check.tomlrules/php/symfony-is-granted-attribute.tomlrules/php/symfony-is-granted.tomlrules/php/symfony-voter-class.tomlsrc/rules/embedded.rssrc/scanner/discovery.rssrc/scanner/imports.rssrc/scanner/matcher.rssrc/scanner/parser.rstests/scanner_enforcement_points.rs
…hapes - laravel-authorize-helper: receiver regex now accepts nullsafe chains like $request?->user()?->can(...) in addition to ->user(). - symfony-is-granted-attribute: query now matches qualified attribute names like #[\Symfony\...\IsGranted(...)] alongside bare #[IsGranted]. - matcher: add direct $this->isGranted(...) positive test.
|
@coderabbitai Addressed nitpick in b341e14: added |
|
Tip For best results, initiate chat on the files or code changes.
|
Summary
Adds first-class structural detection for PHP, covering Laravel (Gates, Policies, route middleware,
$this->authorize/$user->canhelpers) and Symfony (Voters,$this->isGranted/denyAccessUnlessGranted,#[IsGranted]attributes), plus idiomatic role / permission shapes that show up in hand-rolled authz.Tree-sitter-php is wired through the existing discovery → parser → matcher pipeline, with PHP-specific import tracking and propagation edges (parameters via type, fields via member-access name, property defaults) — mirroring the patterns already established for Kotlin, Ruby, and C#.
Changes
tree-sitter-php 0.24throughparser.rsanddiscovery.rs(.php/.phtmlextensions, plus aPhpentry in the deep-pass language map).rules/php/with both Rego and Cedar templates:gate-allows-denies,gate-define,authorize-helper,policy-class,route-middlewarevoter-class,is-granted,is-granted-attributerole-equals-check,in-array-role-check,has-role-callfind_php_policy_importscovering bothuse Foo\Bar [as B];(simple) anduse Foo\Bar\{Baz, Qux as Q};(group) forms, includinguse function …/use const ….simple_parameter(type → variable),assignment_expression($this->field = $x),property_elementdefault values, and the nullsafe member-call analogue.tests/scanner_enforcement_points.rs::enforcement_points_increments_for_php_policy_dipins constructor-DI → field propagation end-to-end.docs/corpus/php.mdcalibration report against Monica (Laravel) and the official Symfony demo: 41 findings on Monica, 4 on Symfony demo, 0 false positives across both runs.php-laravel-authorize-helperto require a principal-shaped receiver ($this, user-named variable, or->user()/::user()chain) so$widget->can('render')and$client->cannot('disconnect')no longer fire as high-confidence RBAC. Includes nullsafe ($user?->can(...)) coverage.docs/corpus/README.mdupdated: PHP moves from "planned" to "yes".Test plan
cargo buildcargo fmt --checkcargo clippy --all-features --all-targets -- -D warningscargo test --all-features(matcher + imports + integration tests all green)cargo run -- rules test(498 inline rule tests passing)$widget->can(...)/$client->cannot(...)correctly suppressed.Closes #86
Summary by CodeRabbit
New Features
Documentation
Tests