Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement regex validation conditions in form logic #645

Merged
merged 4 commits into from
Dec 16, 2024
Merged

Implement regex validation conditions in form logic #645

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bb6c11
Implement regex validation conditions in form logic
chiragchhatrala Dec 13, 2024
2dd69d1
update resource filters
chiragchhatrala Dec 13, 2024
59eb540
Remove ray
chiragchhatrala Dec 13, 2024
b918bf8
Merge branch 'main' into 14ca6-regex-validation-rule
JhumanJ Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions api/app/Rules/FormPropertyLogicRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ class FormPropertyLogicRule implements DataAwareRule, ValidationRule
'content_length_less_than_or_equal_to' => [
'expected_type' => 'number',
],
'matches_regex' => [
'expected_type' => 'string',
'format' => [
'type' => 'regex'
]
],
'does_not_match_regex' => [
'expected_type' => 'string',
'format' => [
'type' => 'regex'
]
],
],
],
'matrix' => [
Expand Down Expand Up @@ -672,6 +684,8 @@ class FormPropertyLogicRule implements DataAwareRule, ValidationRule

private $data = [];

private $operator = '';

private function checkBaseCondition($condition)
{

Expand Down Expand Up @@ -712,6 +726,7 @@ private function checkBaseCondition($condition)

$typeField = $condition['value']['property_meta']['type'];
$operator = $condition['value']['operator'];
$this->operator = $operator;
$value = $condition['value']['value'];

if (!isset(self::CONDITION_MAPPING[$typeField])) {
Expand Down Expand Up @@ -750,6 +765,19 @@ private function checkBaseCondition($condition)

private function valueHasCorrectType($type, $value)
{
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;
}
}
}

Comment on lines +768 to +780
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Use a proper regex delimiter and escape pattern:
-                    preg_match('/' . $value . '/', '');
+                    $pattern = '/' . preg_quote($value, '/') . '/';
+                    preg_match($pattern, 'test');
  1. Add additional validation:
if (strlen($value) > 1000) {
    $this->conditionErrors[] = 'regex pattern too long';
    return false;
}
  1. Consider using preg_match_all with 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;
}

if (
($type === 'string' && gettype($value) !== 'string') ||
($type === 'boolean' && !is_bool($value)) ||
Expand Down
14 changes: 14 additions & 0 deletions api/app/Service/Forms/FormLogicConditionChecker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,20 @@ private function textConditionMet(array $propertyCondition, $value): bool
return $this->checkLength($propertyCondition, $value, '<');
case 'content_length_less_than_or_equal_to':
return $this->checkLength($propertyCondition, $value, '<=');
case 'matches_regex':
try {
ray('matches_regex', $propertyCondition['value'], $value);
return (bool) preg_match('/' . $propertyCondition['value'] . '/', $value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix potential regex injection vulnerability.

Direct concatenation of user input into regex patterns is unsafe. The pattern should be properly escaped and validated:

  1. Escape regex delimiters in the pattern
  2. Validate the pattern before use
  3. 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;
}
Comment on lines +312 to +315
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 'does_not_match_regex':
try {
return !(bool) preg_match('/' . $propertyCondition['value'] . '/', $value);
} catch (\Exception $e) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add performance safeguards for regex evaluation.

Consider adding safeguards against catastrophic backtracking:

  1. Set a timeout limit for regex evaluation
  2. Limit pattern complexity
  3. 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();
    }
}

}

return false;
Expand Down
40 changes: 40 additions & 0 deletions api/resources/data/open_filters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,46 @@
},
"content_length_less_than_or_equal_to": {
"expected_type": "number"
},
"matches_regex": {
"expected_type": "string",
"format": {
"type": "regex"
}
},
"does_not_match_regex": {
"expected_type": "string",
"format": {
"type": "regex"
}
}
}
},
"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"
}
Comment on lines +246 to +270
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 comparators

Committable suggestion skipped: line range outside the PR's diff.

}
}
},
Expand Down
196 changes: 196 additions & 0 deletions api/tests/Feature/Forms/FormLogicTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php

use Illuminate\Testing\Fluent\AssertableJson;
use Tests\Helpers\FormSubmissionDataFactory;

it('create form with logic', function () {
$user = $this->actingAsUser();
Expand Down Expand Up @@ -348,3 +349,198 @@
]
]);
});


it('can submit form with passed regex validation condition', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);
$targetField = collect($form->properties)->where('name', 'Email')->first();

// Regex condition to check if email is from gmail.com domain
$condition = [
'actions' => [],
'conditions' => [
'operatorIdentifier' => 'and',
'children' => [
[
'identifier' => $targetField['id'],
'value' => [
'operator' => 'matches_regex',
'property_meta' => [
'id' => $targetField['id'],
'type' => 'text',
],
'value' => '^[a-zA-Z0-9._%+-]+@gmail\.com$',
],
],
],
],
];

$submissionData = [];
$validationMessage = 'Must be a Gmail address';

$form->properties = collect($form->properties)->map(function ($property) use (&$submissionData, &$condition, &$validationMessage, $targetField) {
if (in_array($property['name'], ['Name'])) {
$property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage];
$submissionData[$targetField['id']] = '[email protected]';
}
Comment on lines +385 to +388
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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]';
}

return $property;
})->toArray();

$form->update();
$formData = FormSubmissionDataFactory::generateSubmissionData($form, $submissionData);

$response = $this->postJson(route('forms.answer', $form->slug), $formData);
$response->assertSuccessful()
->assertJson([
'type' => 'success',
'message' => 'Form submission saved.',
]);
});

it('can not submit form with failed regex validation condition', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);
$targetField = collect($form->properties)->where('name', 'Email')->first();

// Regex condition to check if email is from gmail.com domain
$condition = [
'actions' => [],
'conditions' => [
'operatorIdentifier' => 'and',
'children' => [
[
'identifier' => $targetField['id'],
'value' => [
'operator' => 'matches_regex',
'property_meta' => [
'id' => $targetField['id'],
'type' => 'text',
],
'value' => '^[a-zA-Z0-9._%+-]+@gmail\.com$',
],
],
],
],
];

$submissionData = [];
$validationMessage = 'Must be a Gmail address';

$form->properties = collect($form->properties)->map(function ($property) use (&$submissionData, &$condition, &$validationMessage, $targetField) {
if (in_array($property['name'], ['Name'])) {
$property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage];
$submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should fail
}
Comment on lines +434 to +437
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
}

return $property;
})->toArray();

$form->update();
$formData = FormSubmissionDataFactory::generateSubmissionData($form, $submissionData);

$this->postJson(route('forms.answer', $form->slug), $formData)
->assertStatus(422)
->assertJson([
'message' => $validationMessage,
]);
});

it('can submit form with does not match regex validation condition', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);
$targetField = collect($form->properties)->where('name', 'Email')->first();

// Regex condition to check if email is NOT from gmail.com domain
$condition = [
'actions' => [],
'conditions' => [
'operatorIdentifier' => 'and',
'children' => [
[
'identifier' => $targetField['id'],
'value' => [
'operator' => 'does_not_match_regex',
'property_meta' => [
'id' => $targetField['id'],
'type' => 'text',
],
'value' => '^[a-zA-Z0-9._%+-]+@gmail\.com$',
],
],
],
],
];

$submissionData = [];
$validationMessage = 'Gmail addresses not allowed';

$form->properties = collect($form->properties)->map(function ($property) use (&$submissionData, &$condition, &$validationMessage, $targetField) {
if (in_array($property['name'], ['Name'])) {
$property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage];
$submissionData[$targetField['id']] = '[email protected]'; // Non-Gmail address should pass
}
Comment on lines +482 to +485
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
}

return $property;
})->toArray();

$form->update();
$formData = FormSubmissionDataFactory::generateSubmissionData($form, $submissionData);

$response = $this->postJson(route('forms.answer', $form->slug), $formData);
$response->assertSuccessful()
->assertJson([
'type' => 'success',
'message' => 'Form submission saved.',
]);
});

it('handles invalid regex patterns gracefully', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);
$targetField = collect($form->properties)->where('name', 'Email')->first();

// Invalid regex pattern
$condition = [
'actions' => [],
'conditions' => [
'operatorIdentifier' => 'and',
'children' => [
[
'identifier' => $targetField['id'],
'value' => [
'operator' => 'matches_regex',
'property_meta' => [
'id' => $targetField['id'],
'type' => 'text',
],
'value' => '[Invalid Regex)', // Invalid regex pattern
],
],
],
],
];

$submissionData = [];
$validationMessage = 'Invalid regex pattern';

$form->properties = collect($form->properties)->map(function ($property) use (&$submissionData, &$condition, &$validationMessage, $targetField) {
if (in_array($property['name'], ['Name'])) {
$property['validation'] = ['error_conditions' => $condition, 'error_message' => $validationMessage];
$submissionData[$targetField['id']] = '[email protected]';
}
Comment on lines +531 to +534
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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]';
}

return $property;
})->toArray();

$form->update();
$formData = FormSubmissionDataFactory::generateSubmissionData($form, $submissionData);

$this->postJson(route('forms.answer', $form->slug), $formData)
->assertStatus(422)
->assertJson([
'message' => $validationMessage,
]);
});
12 changes: 12 additions & 0 deletions client/data/open_filters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,18 @@
},
"content_length_less_than_or_equal_to": {
"expected_type": "number"
},
"matches_regex": {
"expected_type": "string",
"format": {
"type": "regex"
}
},
"does_not_match_regex": {
"expected_type": "string",
"format": {
"type": "regex"
}
}
}
},
Expand Down
14 changes: 14 additions & 0 deletions client/lib/forms/FormLogicConditionChecker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,20 @@ function textConditionMet(propertyCondition, value) {
return checkLength(propertyCondition, value, "<")
case "content_length_less_than_or_equal_to":
return checkLength(propertyCondition, value, "<=")
case 'matches_regex':
try {
const regex = new RegExp(propertyCondition.value)
return regex.test(value)
} catch (e) {
return false
}
case 'does_not_match_regex':
try {
const regex = new RegExp(propertyCondition.value)
return !regex.test(value)
} catch (e) {
return true
}
}
return false
}
Expand Down
Loading