Implement regex validation conditions in form logic #645
Conversation
- Added support for 'matches_regex' and 'does_not_match_regex' conditions in FormPropertyLogicRule and FormLogicConditionChecker. - Updated validation logic to handle regex patterns, including error handling for invalid patterns. - Enhanced tests to cover scenarios for successful and failed regex validation, ensuring proper feedback for form submissions. - Updated JSON schema to include new regex condition types. These changes improve the flexibility of form validation by allowing regex-based conditions, enhancing user experience through more robust validation mechanisms.
|
Warning Rate limit exceeded@chiragchhatrala has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 40 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 (1)
WalkthroughThe pull request introduces enhancements to the form logic validation system by adding two new regex-based comparators, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
api/app/Service/Forms/FormLogicConditionChecker.php (1)
311-311: Remove debug statements.The
ray()debugging statements should not be present in production code.- ray('matches_regex', $propertyCondition['value'], $value); - ray('matches_regex_error', $e);Also applies to: 314-314
api/tests/Feature/Forms/FormLogicTest.php (1)
500-546: Consider using data provider for comprehensive regex pattern testing.The test could be enhanced by testing multiple invalid regex patterns to ensure robust error handling.
Consider using a data provider to test various invalid patterns:
- Unmatched parentheses:
(- Unmatched brackets:
[- Invalid quantifiers:
?*- Invalid character classes:
[a-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/app/Rules/FormPropertyLogicRule.php(4 hunks)api/app/Service/Forms/FormLogicConditionChecker.php(1 hunks)api/tests/Feature/Forms/FormLogicTest.php(2 hunks)client/data/open_filters.json(1 hunks)client/lib/forms/FormLogicConditionChecker.js(1 hunks)
🔇 Additional comments (4)
client/lib/forms/FormLogicConditionChecker.js (1)
Line range hint 1-1: Request review of client-side implementation.
The JavaScript implementation of regex validation in client/lib/forms/FormLogicConditionChecker.js is missing from the review. This file is critical for ensuring consistent regex validation behavior between client and server sides.
client/data/open_filters.json (1)
232-242: Consider adding regex pattern validation constraints.
While the schema correctly defines the new regex comparators, consider adding additional constraints to prevent invalid or malicious regex patterns:
- Maximum pattern length to prevent ReDoS attacks
- Allowed/disallowed regex features (e.g., lookaheads, backreferences)
- Pattern validation rules (e.g., must be a valid regex syntax)
api/app/Rules/FormPropertyLogicRule.php (2)
74-85: LGTM! Regex comparators are well-defined.
The new comparators are correctly configured with appropriate types and formats.
687-688: LGTM! Property is well-defined.
The private property is correctly defined and serves a clear purpose.
| case 'matches_regex': | ||
| try { | ||
| ray('matches_regex', $propertyCondition['value'], $value); | ||
| return (bool) preg_match('/' . $propertyCondition['value'] . '/', $value); |
There was a problem hiding this comment.
Fix potential regex injection vulnerability.
Direct concatenation of user input into regex patterns is unsafe. The pattern should be properly escaped and validated:
- Escape regex delimiters in the pattern
- Validate the pattern before use
- Consider using a whitelist of allowed regex features
- return (bool) preg_match('/' . $propertyCondition['value'] . '/', $value);
+ $pattern = $propertyCondition['value'];
+ if (!$this->isValidRegexPattern($pattern)) {
+ throw new \InvalidArgumentException('Invalid regex pattern');
+ }
+ $escaped_pattern = '/' . str_replace('/', '\/', $pattern) . '/';
+ return (bool) preg_match($escaped_pattern, $value);Add this helper method:
private function isValidRegexPattern(string $pattern): bool
{
try {
// Test compile the pattern
return @preg_match('/' . str_replace('/', '\/', $pattern) . '/', '') !== false;
} catch (\Exception $e) {
return false;
}
}Also applies to: 319-319
| } catch (\Exception $e) { | ||
| ray('matches_regex_error', $e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Standardize error handling behavior.
The error handling differs between matches_regex (returns false) and does_not_match_regex (returns true). This inconsistency could lead to unexpected behavior. Consider standardizing the error handling to always return false on invalid patterns.
} catch (\Exception $e) {
- ray('matches_regex_error', $e);
- return false;
+ throw new \InvalidArgumentException('Invalid regex pattern: ' . $e->getMessage());
}
case 'does_not_match_regex':
try {
return !(bool) preg_match('/' . $propertyCondition['value'] . '/', $value);
} catch (\Exception $e) {
- return true;
+ throw new \InvalidArgumentException('Invalid regex pattern: ' . $e->getMessage());
}Also applies to: 320-322
| case 'matches_regex': | ||
| try { | ||
| ray('matches_regex', $propertyCondition['value'], $value); | ||
| return (bool) preg_match('/' . $propertyCondition['value'] . '/', $value); | ||
| } catch (\Exception $e) { | ||
| ray('matches_regex_error', $e); | ||
| return false; | ||
| } | ||
| case 'does_not_match_regex': | ||
| try { | ||
| return !(bool) preg_match('/' . $propertyCondition['value'] . '/', $value); | ||
| } catch (\Exception $e) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add performance safeguards for regex evaluation.
Consider adding safeguards against catastrophic backtracking:
- Set a timeout limit for regex evaluation
- Limit pattern complexity
- Consider using
preg_match_all()with a limit for potentially problematic patterns
private function evaluateRegexPattern(string $pattern, string $value, int $timeout_ms = 100): bool
{
// Set timeout
ini_set('pcre.backtrack_limit', '1000000');
ini_set('pcre.jit', '0');
// Use custom error handler to catch timeouts
set_error_handler(function($errno, $errstr) {
throw new \RuntimeException('Regex evaluation timeout');
}, E_WARNING);
try {
$escaped_pattern = '/' . str_replace('/', '\/', $pattern) . '/';
return (bool) preg_match($escaped_pattern, $value);
} finally {
restore_error_handler();
}
}| if (in_array($property['name'], ['Name'])) { | ||
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | ||
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should pass | ||
| } |
There was a problem hiding this comment.
Fix field mapping in validation setup.
The validation is being applied to the 'Name' field but using the 'Email' field's value.
Apply this diff to fix the field mapping:
-if (in_array($property['name'], ['Name'])) {
+if (in_array($property['name'], ['Email'])) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in_array($property['name'], ['Name'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should pass | |
| } | |
| if (in_array($property['name'], ['Email'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should pass | |
| } |
| if (in_array($property['name'], ['Name'])) { | ||
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | ||
| $submissionData[$targetField['id']] = '[email protected]'; | ||
| } |
There was a problem hiding this comment.
Fix field mapping in validation setup.
The validation is being applied to the 'Name' field but using the 'Email' field's value, which is confusing and could lead to maintenance issues.
Apply this diff to fix the field mapping:
-if (in_array($property['name'], ['Name'])) {
+if (in_array($property['name'], ['Email'])) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in_array($property['name'], ['Name'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; | |
| } | |
| if (in_array($property['name'], ['Email'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; | |
| } |
| if (in_array($property['name'], ['Name'])) { | ||
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | ||
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should fail | ||
| } |
There was a problem hiding this comment.
Fix field mapping in validation setup.
The validation is being applied to the 'Name' field but using the 'Email' field's value.
Apply this diff to fix the field mapping:
-if (in_array($property['name'], ['Name'])) {
+if (in_array($property['name'], ['Email'])) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in_array($property['name'], ['Name'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should fail | |
| } | |
| if (in_array($property['name'], ['Email'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should fail | |
| } |
| if (in_array($property['name'], ['Name'])) { | ||
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | ||
| $submissionData[$targetField['id']] = '[email protected]'; | ||
| } |
There was a problem hiding this comment.
Fix field mapping in validation setup.
The validation is being applied to the 'Name' field but using the 'Email' field's value.
Apply this diff to fix the field mapping:
-if (in_array($property['name'], ['Name'])) {
+if (in_array($property['name'], ['Email'])) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in_array($property['name'], ['Name'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; | |
| } | |
| if (in_array($property['name'], ['Email'])) { | |
| $property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage]; | |
| $submissionData[$targetField['id']] = '[email protected]'; | |
| } |
| if ($type === 'string' && isset(self::CONDITION_MAPPING[$this->field['type']]['comparators'][$this->operator]['format'])) { | ||
| $format = self::CONDITION_MAPPING[$this->field['type']]['comparators'][$this->operator]['format']; | ||
| if ($format['type'] === 'regex') { | ||
| try { | ||
| preg_match('/' . $value . '/', ''); | ||
| return true; | ||
| } catch (\Exception $e) { | ||
| $this->conditionErrors[] = 'invalid regex pattern'; | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve regex pattern validation for better security and reliability.
The current implementation has potential security issues and might not catch all invalid patterns.
Consider these improvements:
- Use a proper regex delimiter and escape pattern:
- preg_match('/' . $value . '/', '');
+ $pattern = '/' . preg_quote($value, '/') . '/';
+ preg_match($pattern, 'test');- Add additional validation:
if (strlen($value) > 1000) {
$this->conditionErrors[] = 'regex pattern too long';
return false;
}- Consider using
preg_match_allwith a limit to prevent catastrophic backtracking:
@preg_match_all($pattern, 'test', $matches, 0, -1, PREG_OFFSET_CAPTURE);
if (preg_last_error() !== PREG_NO_ERROR) {
$this->conditionErrors[] = 'invalid regex pattern: ' . preg_last_error_msg();
return false;
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
api/resources/data/open_filters.json (2)
232-243: Consider adding regex pattern constraints for securityWhile the regex comparators are well-structured, consider adding pattern constraints or validation rules to prevent potential security issues like ReDoS (Regular Expression Denial of Service) attacks.
Suggestions:
- Add a maximum pattern length limit
- Consider adding a
pattern_constraintsfield to specify allowed/disallowed regex features- Document safe regex patterns in the schema
Example enhancement:
"matches_regex": { "expected_type": "string", "format": { "type": "regex" + "constraints": { + "max_length": 1000, + "disallowed_features": ["lookaheads", "backreferences"] + } } }
257-257: Fix inconsistent JSON formattingThere are spacing inconsistencies in the "type" field declarations.
Apply this formatting fix:
- "type":"object" + "type": "object"Also applies to: 263-263, 269-269
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/resources/data/open_filters.json(1 hunks)
🔇 Additional comments (1)
api/resources/data/open_filters.json (1)
246-270: Verify scope of matrix comparator changes
The PR objectives specifically mention implementing regex validation conditions, but the changes also include new matrix comparators. Please clarify if these matrix-related changes should be part of this PR or moved to a separate one for better change isolation.
| "matrix": { | ||
| "comparators": { | ||
| "equals": { | ||
| "expected_type": "object", | ||
| "format": { | ||
| "type": "object" | ||
| } | ||
| }, | ||
| "does_not_equal": { | ||
| "expected_type": "object", | ||
| "format": { | ||
| "type":"object" | ||
| } | ||
| }, | ||
| "contains": { | ||
| "expected_type": "object", | ||
| "format": { | ||
| "type":"object" | ||
| } | ||
| }, | ||
| "does_not_contain": { | ||
| "expected_type": "object", | ||
| "format": { | ||
| "type":"object" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define schema for matrix object structure
The matrix comparators lack a schema definition for the expected object structure, which could lead to validation inconsistencies.
Consider adding a schema definition:
"matrix": {
"comparators": {
+ "schema": {
+ "type": "object",
+ "properties": {
+ "rows": {
+ "type": "array",
+ "items": {
+ "type": "string"
+ }
+ },
+ "columns": {
+ "type": "array",
+ "items": {
+ "type": "string"
+ }
+ }
+ },
+ "required": ["rows", "columns"]
+ },
"equals": {
"expected_type": "object",
"format": {
"type": "object"
+ "ref": "#/comparators/schema"
}
},
// Apply similar changes to other comparatorsCommittable suggestion skipped: line range outside the PR's diff.
These changes improve the flexibility of form validation by allowing regex-based conditions, enhancing user experience through more robust validation mechanisms.
Summary by CodeRabbit
New Features
matches_regexanddoes_not_match_regex.Bug Fixes
Tests