Conversation
|
Warning Rate limit exceeded@eldadfux has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds a new PHP Web Application Firewall (WAF) library and project scaffold. Introduces CI workflows (CodeQL, linter, multi-version PHP tests), composer and tooling configs (composer.json, phpunit.xml, pint.json, phpstan.neon), .gitignore updates, and source code: Condition (DSL and evaluator), Firewall, Rule base with concrete rules (Allow, Deny, Challenge, RateLimit, Redirect), and a dedicated Condition exception. Adds PHPUnit tests covering Condition, Firewall, and rule behaviors. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
src/Exception/Condition.php (1)
1-7: Dedicated Condition exception type looks goodHaving a namespaced
Conditionexception extending\Exceptionis clean and sufficient. If you want to lock the hierarchy further, you could mark itfinal, but that’s optional.-class Condition extends \Exception +final class Condition extends \Exceptioncomposer.json (1)
1-33: Consider adding package metadata for better discoverability.The composer.json is functionally complete, but adding optional metadata would improve the package's discoverability and professionalism. Consider adding:
authorsarray with maintainer informationkeywordsfor package discoveryhomepageURLsupportsection (issues, source, docs)tests/ConditionTest.php (1)
11-53: LGTM! Good test coverage for core Condition functionality.The tests cover the essential flows:
- Serialization round-trips (array and string formats)
- Logical condition composition (AND)
- Exception handling for invalid inputs
Consider adding tests for additional edge cases in future iterations:
- OR logical conditions
- Empty condition arrays
- Other condition methods (contains, endsWith, etc.)
- Missing attribute scenarios
src/Rule.php (1)
56-65: Document behavior when conditions array is empty.The
matches()method returnstruewhen$conditionsis empty (line 64), meaning a rule with no conditions matches all requests. While this might be intentional, it could be surprising behavior. Consider either:
- Adding a docblock comment documenting this behavior
- Throwing an exception for empty conditions if that's not intended
Example documentation:
/** * Evaluate rule conditions against provided attributes. + * Returns true if all conditions match (AND semantics). + * A rule with no conditions matches all requests. * * @param array<string, mixed> $attributes + * @return bool True if all conditions match or if there are no conditions */ public function matches(array $attributes): boolsrc/Firewall.php (2)
55-60: Consider validating rule types insetRules.The method directly assigns the input array without validating that all elements are
Ruleinstances. While the PHPDoc annotation documents the expectation, runtime validation would provide better safety.public function setRules(array $rules): self { + foreach ($rules as $rule) { + if (!$rule instanceof Rule) { + throw new \InvalidArgumentException('All rules must be instances of Rule'); + } + } $this->rules = $rules; return $this; }
116-131: Minor redundancy in alias generation.The
array_uniquecall on line 130 is defensive but redundant since thein_arraycheck on line 126 already prevents duplicate entries. Not a bug, just a minor inefficiency.src/Condition.php (2)
439-454: Unused$inclusiveparameter inmatchesRange.The
$inclusiveparameter is always passed astruefrom the callers (lines 349-350). The exclusive path (line 453) is dead code. Consider removing the parameter or exposing exclusive range conditions if intended for future use.
481-508: Redundant fallback inresolveValue.Line 507 (
return $attributes[$this->attribute] ?? null) is unreachable for non-dot attributes because they're already handled by thearray_key_existscheck on lines 487-488, which returns the value or would fall through to line 507 only if the key doesn't exist (returning null either way).private function resolveValue(array $attributes): mixed { if ($this->attribute === '') { return null; } - if (array_key_exists($this->attribute, $attributes)) { - return $attributes[$this->attribute]; - } - - if (str_contains($this->attribute, '.')) { + if (!str_contains($this->attribute, '.')) { + return $attributes[$this->attribute] ?? null; + } + + // Handle dot-notation for nested access $segments = explode('.', $this->attribute); $current = $attributes; foreach ($segments as $segment) { if (\is_array($current) && array_key_exists($segment, $current)) { $current = $current[$segment]; continue; } return null; } return $current; - } - - return $attributes[$this->attribute] ?? null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)composer.json(1 hunks)phpunit.xml(1 hunks)pint.json(1 hunks)src/Condition.php(1 hunks)src/Exception/Condition.php(1 hunks)src/Firewall.php(1 hunks)src/Rule.php(1 hunks)src/Rules/Allow.php(1 hunks)src/Rules/Challenge.php(1 hunks)src/Rules/Deny.php(1 hunks)src/Rules/RateLimit.php(1 hunks)tests/ConditionTest.php(1 hunks)tests/FirewallTest.php(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~8-~8: Use a hyphen to join words.
Context: ... helpers and logical operators. - Action specific rule classes (Allow, Deny, ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (17)
.gitignore (1)
1-2: Ignore rules look appropriate for a PHP libraryIgnoring
vendor/and PHPUnit’s cache directory is aligned with typical Composer/PHPUnit setups; no concerns here.pint.json (1)
1-7: Pint configuration is minimal and sensiblePSR-12 with
no_unused_importsandsingle_quoteis a good baseline for this codebase; no issues spotted..github/workflows/lint.yml (1)
14-17: Confirmcomposer lintscript and consider future‑proofingThe Docker invocation pattern looks fine and matches Composer’s documented examples. Just ensure
composer lintis defined incomposer.jsonand remains idempotent, since this workflow assumes it exists and only runs after a full install..github/workflows/test.yml (1)
11-32: PHP constraint supports the full test matrix — no concerns.The
composer.jsonexplicitly declaresphp: ">=8.2"with no upper bound, which fully supports all matrix versions (8.2, 8.3, 8.4). Development dependencies (phpunit/phpunit ^11.0,laravel/pint ^1.18) are modern and compatible across the range. No silent version skips or conflicts detected.phpunit.xml (1)
1-14: Cannot verify PHPUnit version compatibility due to repository access limitationsI was unable to access the
composer.jsonorphpunit.xmlfiles in the repository to verify the PHPUnit version constraint. The repository root only containsLICENSEandREADME.md, and the files referenced in this review are not discoverable via GitHub's API.The original review comment's suggestion to verify that
phpunit/phpunitincomposer.jsontargets a compatible 11.x version remains valid and should be checked manually during code review, but I cannot confirm the current state of the configuration.src/Rules/Deny.php (1)
7-13: LGTM!The Deny rule implementation is clean and correct. It follows the expected pattern by extending Rule and returning the appropriate action constant.
src/Rules/Allow.php (1)
7-13: LGTM!The Allow rule implementation is clean and correct, following the expected pattern consistently with other rule types.
tests/FirewallTest.php (2)
14-39: LGTM! Excellent test coverage for rule ordering.The test correctly validates that rule order matters and that the first matching rule determines the outcome. The test structure is clear with good assertions and descriptive messages.
41-61: The test correctly implements the intended behavior.After examining the codebase, the test at lines 41-61 is correct. The
Firewall::verify()method returnstrueforACTION_RATE_LIMITby design (line 108 ofsrc/Firewall.php). Rate limiting is a distinct action type alongside ALLOW and DENY:
ACTION_ALLOW→ returnstrue(permit request)ACTION_DENY→ returnsfalse(block request)ACTION_CHALLENGE→ returnsfalse(challenge request)ACTION_RATE_LIMIT→ returnstrue(apply rate limiting)The rate-limit rule matching returns
truebecause the Firewall class only evaluates whether rules match—actual rate-limiting enforcement (tracking request counts and enforcing limits) occurs in consuming code outside the Firewall. The test correctly verifies that the matched rule is retrievable and its metadata (limit and interval) is accessible.src/Rules/RateLimit.php (2)
26-29: LGTM!The getAction() implementation correctly returns the ACTION_RATE_LIMIT constant.
31-39: LGTM!The accessor methods are straightforward and correct.
src/Rules/Challenge.php (1)
23-31: LGTM!The action and type accessor methods are implemented correctly.
src/Rule.php (2)
20-32: LGTM! Good defensive programming with condition cloning.The constructor correctly normalizes conditions and clones existing Condition instances to prevent external mutation. This is a good defensive practice that maintains rule immutability.
34-49: LGTM!The abstract method declaration and accessor methods are clean and well-structured.
src/Firewall.php (1)
102-111:ACTION_RATE_LIMITreturnstruewithout enforcing limits.The rate limit action allows the request through without tracking request counts or enforcing limits. The caller must use
getLastMatchedRule()to inspect rate limit metadata and enforce externally. Consider documenting this expected usage pattern or providing enforcement hooks.src/Condition.php (2)
82-100: LGTM!The constructor validates the method and normalizes values appropriately. The
__cloneimplementation correctly handles deep cloning of nested Condition instances.
130-146: LGTM!JSON parsing with proper exception handling and validation. The error messages are descriptive and helpful for debugging.
…onTest with additional comparison, contains, range, and logical operators tests.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… variable support and safe directory configuration; add PHPDoc annotations to Condition methods; improve FirewallTest assertions for better type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/RulesTest.php (1)
14-51: Rule tests cover the happy paths well; consider a couple of negative casesThe assertions for actions and metadata on
Allow,Deny,Challenge, andRateLimitall align with the rule implementations. As a small enhancement, you could add tests that assert:
Challengethrows on an invalid$type.RateLimitthrows on invalid$limit/$interval(e.g.0).This would ensure the constructor validations stay enforced over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks).gitignore(1 hunks)composer.json(1 hunks)phpstan.neon(1 hunks)src/Condition.php(1 hunks)src/Rules/Challenge.php(1 hunks)src/Rules/RateLimit.php(1 hunks)tests/ConditionTest.php(1 hunks)tests/RulesTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- phpstan.neon
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Rules/RateLimit.php
- composer.json
🧰 Additional context used
🧬 Code graph analysis (2)
tests/RulesTest.php (2)
src/Rules/Challenge.php (2)
Challenge(7-35)getType(31-34)src/Rules/RateLimit.php (3)
RateLimit(7-43)getLimit(34-37)getInterval(39-42)
tests/ConditionTest.php (1)
src/Condition.php (23)
Condition(13-559)equal(231-234)notEqual(236-239)matches(332-359)lessThan(241-244)lessThanEqual(246-249)greaterThan(251-254)greaterThanEqual(256-259)contains(261-264)notContains(266-269)between(271-274)notBetween(276-279)startsWith(281-284)notStartsWith(286-289)endsWith(291-294)notEndsWith(296-299)isNull(301-304)isNotNull(306-309)and(314-317)or(322-325)toString(219-226)parse(133-146)fromArray(151-183)
🪛 GitHub Actions: CodeQL
src/Condition.php
[error] 194-194: PHPStan: Method Utopia\WAF\Condition::toArray() return type has no value type specified in iterable type array.
[error] 261-261: PHPStan: Method Utopia\WAF\Condition::contains() has parameter $values with no value type specified in iterable type array.
[error] 266-266: PHPStan: Method Utopia\WAF\Condition::notContains() has parameter $values with no value type specified in iterable type array.
🪛 PHPStan (2.1.32)
src/Condition.php
194-194: Method Utopia\WAF\Condition::toArray() return type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
261-261: Method Utopia\WAF\Condition::contains() has parameter $values with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
266-266: Method Utopia\WAF\Condition::notContains() has parameter $values with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
🔇 Additional comments (4)
.gitignore (1)
1-3: Ignore patterns are sensible for this projectIgnoring
vendor/, PHPUnit cache, andvar/will keep build/test artifacts out of VCS; no changes needed..github/workflows/test.yml (1)
11-32: Confirm matrix PHP versions are supported by the action and the libraryThe workflow itself looks solid. Please just double‑check that
shivammathur/setup-php@v2and the library actually support all matrix versions (8.2–8.5); otherwise runs for unsupported versions will fail.src/Rules/Challenge.php (1)
7-34: Challenge rule implementation and validation look correctType constants, strict
$typevalidation, andgetAction()/getType()behavior are consistent with usage in tests and the Rule base class; no changes needed.tests/ConditionTest.php (1)
11-189: Comprehensive Condition test coverageThese tests exercise all major operator categories (including nested logic, null handling, and parse/serialize round‑trip) and align with the
Conditionimplementation. This should give good confidence in the DSL behavior.
- Introduced a new `Redirect` action in the WAF rules management library. - Updated the `Firewall` class to handle the `Redirect` action. - Enhanced the README to document the new `Redirect` rule class and its usage. - Added unit tests for the `Redirect` rule to ensure functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Rules/Redirect.php (1)
16-21: Consider validating redirect parameters.While the defaults are reasonable (302 for temporary redirect, '/' for root), the constructor accepts any integer for
$statusCodeand any string for$locationwithout validation. Invalid values (e.g.,statusCode: 999orlocation: '') could lead to unexpected behavior.Consider adding validation to ensure
$statusCodeis a valid HTTP redirect code (3xx range) and$locationis non-empty.Apply this diff to add validation:
public function __construct(array $conditions = [], string $location = '/', int $statusCode = 302) { parent::__construct($conditions); + + if ($location === '') { + throw new \InvalidArgumentException('Redirect location cannot be empty'); + } + + if ($statusCode < 300 || $statusCode >= 400) { + throw new \InvalidArgumentException('Redirect status code must be in 3xx range'); + } + $this->location = $location; $this->statusCode = $statusCode; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/lint.yml(1 hunks)README.md(1 hunks)src/Condition.php(1 hunks)src/Firewall.php(1 hunks)src/Rule.php(1 hunks)src/Rules/Redirect.php(1 hunks)tests/FirewallTest.php(1 hunks)tests/RulesTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Firewall.php
- tests/FirewallTest.php
- tests/RulesTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Rule.php (2)
src/Rules/Redirect.php (2)
__construct(16-21)getAction(23-26)src/Condition.php (2)
Condition(13-572)fromArray(151-183)
src/Rules/Redirect.php (1)
src/Rule.php (3)
Rule(5-67)__construct(21-33)getAction(35-35)
🪛 LanguageTool
README.md
[grammar] ~8-~8: Use a hyphen to join words.
Context: ... helpers and logical operators. - Action specific rule classes (Allow, Deny, ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (10)
.github/workflows/lint.yml (1)
1-20: LGTM!The linter workflow is properly configured with standard GitHub Actions and Docker-based PHP tooling. The use of
--ignore-platform-reqsis appropriate for CI environments.src/Rule.php (3)
7-11: LGTM!The action constants are well-defined and provide clear semantics for the WAF rule system. The addition of
ACTION_REDIRECTcomplements the existing action types appropriately.
21-33: LGTM!The constructor properly normalizes conditions and defensively clones
Conditioninstances to prevent external mutation. This is a solid design that maintains encapsulation.
57-66: LGTM!The
matches()method implements efficient AND-logic evaluation with early exit. All conditions must match for the rule to apply, which is the correct semantic for WAF rules.src/Rules/Redirect.php (1)
23-36: LGTM!The getter methods are straightforward and correctly expose the redirect metadata. The action type properly references the parent class constant.
src/Condition.php (5)
82-91: LGTM!The constructor properly validates the method against the defined types and normalizes values. The use of a custom exception for invalid methods provides clear error reporting.
133-192: LGTM!The parsing methods are well-implemented with comprehensive error handling. The recursive handling of logical conditions (lines 169-180) properly validates and transforms nested structures.
201-233: LGTM!The serialization methods properly handle both simple and logical conditions. The type annotations have been addressed per previous reviews, and error handling is comprehensive.
238-338: LGTM!The factory methods provide a clean, type-safe DSL for building conditions. The distinction between
equal()accepting multiple values andnotEqual()accepting a single value is semantically appropriate.
345-572: LGTM!The matching logic is comprehensive and handles edge cases properly. The null handling in
matchesRelational()andcompare()correctly prevents invalid comparisons. The nested attribute resolution via dot notation (lines 511-525) is a valuable feature for complex attribute structures.
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.