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.

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

Apply Mentions everywhere #595

Merged
merged 18 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
108 changes: 108 additions & 0 deletions api/app/Console/Commands/EmailNotificationMigration.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
<?php

namespace App\Console\Commands;

use App\Models\Forms\Form;
use App\Models\Integration\FormIntegration;
use Illuminate\Console\Command;

class EmailNotificationMigration extends Command
{
/**
* The name and signature of the console command.
*
* @var string
*/
protected $signature = 'forms:email-notification-migration';

/**
* The console command description.
*
* @var string
*/
protected $description = 'One Time Only -- Migrate Email & Submission Notifications to new Email Integration';

/**
* Execute the console command.
*
* @return int
*/
public function handle()
{
if (app()->environment('production')) {
if (!$this->confirm('Are you sure you want to run this migration in production?')) {
$this->info('Migration aborted.');
return 0;
}
}
$query = FormIntegration::whereIn('integration_id', ['email', 'submission_confirmation'])
->whereHas('form');
$totalCount = $query->count();
$progressBar = $this->output->createProgressBar($totalCount);
$progressBar->start();

$query->with('form')->chunk(100, function ($integrations) use ($progressBar) {
foreach ($integrations as $integration) {
try {
$this->updateIntegration($integration);
} catch (\Exception $e) {
$this->error('Error updating integration ' . $integration->id . '. Error: ' . $e->getMessage());
ray($e);
}
$progressBar->advance();
}
});

$progressBar->finish();
$this->newLine();

$this->line('Migration Done');
}

public function updateIntegration(FormIntegration $integration)
{
if (!$integration->form) {
return;
}
$existingData = $integration->data;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent Usage of $integration->data Detected

The analysis reveals that $integration->data is treated both as an object and as an array in different parts of the codebase. To prevent unexpected behavior, ensure that $integration->data maintains a consistent type throughout its usage.

  • Locations Identified:
    • api/tests/Feature/Zapier/IntegrationsTest.php: Accessed as an object.
    • api/app/Console/Commands/EmailNotificationMigration.php: Assigned as an array.
🔗 Analysis chain

Ensure $integration->data is an array before accessing

When assigning $existingData = $integration->data;, confirm that $integration->data is consistently an array to prevent unexpected behavior.

Run the following script to check the type of $integration->data:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that $integration->data is always an array.

# Test: Search for all instances where FormIntegration's data is set or retrieved.
# Expectation: $integration->data should be an array in all cases.

rg --type=php '\$integration->data' -A 3

Length of output: 1487

if ($integration->integration_id === 'email') {
$integration->data = [
'send_to' => $existingData->notification_emails ?? null,
'sender_name' => 'OpnForm',
'subject' => 'New form submission',
'email_content' => 'Hello there 👋 <br>New form submission received.',
'include_submission_data' => true,
'include_hidden_fields_submission_data' => false,
'reply_to' => $existingData->notification_reply_to ?? null
];
} elseif ($integration->integration_id === 'submission_confirmation') {
$integration->integration_id = 'email';
$integration->data = [
'send_to' => $this->getMentionHtml($integration->form),
'sender_name' => $existingData->notification_sender,
'subject' => $existingData->notification_subject,
'email_content' => $existingData->notification_body,
'include_submission_data' => $existingData->notifications_include_submission,
'include_hidden_fields_submission_data' => false,
'reply_to' => $existingData->confirmation_reply_to ?? null
];
}
return $integration->save();
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

Check the result of $integration->save() and handle failures

Currently, the return value of $integration->save() is not acted upon. If the save operation fails, the migration will proceed without notice.

Consider adding error handling to capture and log save failures:

if (!$integration->save()) {
    $this->error('Failed to save integration ID: ' . $integration->id);
}

}

private function getMentionHtml(Form $form)
{
$emailField = $this->getRespondentEmail($form);
return $emailField ? '<span mention-field-id="' . $emailField['id'] . '" mention-field-name="' . $emailField['name'] . '" mention-fallback="" contenteditable="false" mention="true">' . $emailField['name'] . '</span>' : '';
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

Sanitize $emailField['name'] to prevent XSS vulnerabilities

In the getMentionHtml method, directly outputting $emailField['name'] into HTML attributes and content without sanitization could lead to Cross-Site Scripting (XSS) vulnerabilities if the field name contains malicious content.

Apply this diff to sanitize the field name:

return $emailField ? '<span mention-field-id="' . $emailField['id'] . '" mention-field-name="'
- . $emailField['name'] . '" mention-fallback="" contenteditable="false" mention="true">' . $emailField['name'] . '</span>' : '';
+ . htmlspecialchars($emailField['name'], ENT_QUOTES, 'UTF-8') . '" mention-fallback="" contenteditable="false" mention="true">'
+ . htmlspecialchars($emailField['name'], ENT_QUOTES, 'UTF-8') . '</span>' : '';
📝 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
return $emailField ? '<span mention-field-id="' . $emailField['id'] . '" mention-field-name="' . $emailField['name'] . '" mention-fallback="" contenteditable="false" mention="true">' . $emailField['name'] . '</span>' : '';
return $emailField ? '<span mention-field-id="' . $emailField['id'] . '" mention-field-name="'
. htmlspecialchars($emailField['name'], ENT_QUOTES, 'UTF-8') . '" mention-fallback="" contenteditable="false" mention="true">'
. htmlspecialchars($emailField['name'], ENT_QUOTES, 'UTF-8') . '</span>' : '';

}

private function getRespondentEmail(Form $form)
{
$emailFields = collect($form->properties)->filter(function ($field) {
$hidden = $field['hidden'] ?? false;
return !$hidden && $field['type'] == 'email';
});
Comment on lines +101 to +104
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

Handle cases where $form->properties is null or not iterable

There might be scenarios where $form->properties is null or not an array/object, which would cause an error when calling collect($form->properties).

Consider checking if $form->properties is iterable before proceeding:

- $emailFields = collect($form->properties)->filter(function ($field) {
+ $properties = is_iterable($form->properties) ? $form->properties : [];
+ $emailFields = collect($properties)->filter(function ($field) {
    $hidden = $field['hidden'] ?? false;
    return !$hidden && $field['type'] == '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
$emailFields = collect($form->properties)->filter(function ($field) {
$hidden = $field['hidden'] ?? false;
return !$hidden && $field['type'] == 'email';
});
$properties = is_iterable($form->properties) ? $form->properties : [];
$emailFields = collect($properties)->filter(function ($field) {
$hidden = $field['hidden'] ?? false;
return !$hidden && $field['type'] == 'email';
});


return $emailFields->count() > 0 ? $emailFields->first() : null;
}
}
24 changes: 20 additions & 4 deletions api/app/Http/Controllers/Forms/PublicFormController.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use App\Jobs\Form\StoreFormSubmissionJob;
use App\Models\Forms\Form;
use App\Models\Forms\FormSubmission;
use App\Open\MentionParser;
use App\Service\Forms\FormCleaner;
use App\Service\WorkspaceHelper;
use Illuminate\Http\Request;
Expand Down Expand Up @@ -105,13 +106,28 @@ public function answer(AnswerFormRequest $request)
return $this->success(array_merge([
'message' => 'Form submission saved.',
'submission_id' => $submissionId,
'is_first_submission' => $isFirstSubmission
], $request->form->is_pro && $request->form->redirect_url ? [
'is_first_submission' => $isFirstSubmission,
], $this->getRedirectData($request->form, $submissionData)));
}

private function getRedirectData($form, $submissionData)
{
$formattedData = collect($submissionData)->map(function ($value, $key) {
return ['id' => $key, 'value' => $value];
})->values()->all();

$redirectUrl = ($form->redirect_url) ? (new MentionParser($form->redirect_url, $formattedData))->parse() : null;

if ($redirectUrl && !filter_var($redirectUrl, FILTER_VALIDATE_URL)) {
$redirectUrl = null;
}

return $form->is_pro && $redirectUrl ? [
'redirect' => true,
'redirect_url' => $request->form->redirect_url,
'redirect_url' => $redirectUrl,
] : [
'redirect' => false,
]));
];
}

public function fetchSubmission(Request $request, string $slug, string $submissionId)
Expand Down
2 changes: 1 addition & 1 deletion api/app/Http/Requests/UserFormRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public function rules()
're_fillable' => 'boolean',
're_fill_button_text' => 'string|min:1|max:50',
'submitted_text' => 'string|max:2000',
'redirect_url' => 'nullable|active_url|max:255',
'redirect_url' => 'nullable|max:255',
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

Consider adding URL format validation for 'redirect_url'

The validation rule for 'redirect_url' has been changed from 'active_url' to 'nullable|max:255'. This change allows for more flexibility by accepting nullable values and setting a maximum length, which is good. However, removing the 'active_url' validation without replacing it with a basic URL format check might lead to accepting malformed URLs.

Consider adding the 'url' validation rule to ensure basic URL format correctness:

-            'redirect_url' => 'nullable|max:255',
+            'redirect_url' => 'nullable|url|max:255',

This change will maintain the flexibility of allowing nullable values and the 255 character limit while ensuring that when a value is provided, it adheres to a valid URL format.

For a more robust solution, consider implementing a custom validation rule that checks for allowed URL schemes (e.g., http, https) and performs any application-specific URL validations without checking for active accessibility. This approach would provide better control over accepted URL formats while avoiding potential performance issues associated with the 'active_url' rule.

📝 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
'redirect_url' => 'nullable|max:255',
'redirect_url' => 'nullable|url|max:255',

'database_fields_update' => 'nullable|array',
'max_submissions_count' => 'integer|nullable|min:1',
'max_submissions_reached_text' => 'string|nullable',
Expand Down
10 changes: 7 additions & 3 deletions api/app/Integrations/Handlers/DiscordIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Integrations\Handlers;

use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Support\Arr;
use Vinkla\Hashids\Facades\Hashids;
Expand Down Expand Up @@ -32,6 +33,9 @@ protected function shouldRun(): bool

protected function getWebhookData(): array
{
$formatter = (new FormSubmissionFormatter($this->form, $this->submissionData))->outputStringsOnly();
$formattedData = $formatter->getFieldsWithValue();

$settings = (array) $this->integrationData ?? [];
$externalLinks = [];
if (Arr::get($settings, 'link_open_form', true)) {
Expand All @@ -50,8 +54,7 @@ protected function getWebhookData(): array
$blocks = [];
if (Arr::get($settings, 'include_submission_data', true)) {
$submissionString = '';
$formatter = (new FormSubmissionFormatter($this->form, $this->submissionData))->outputStringsOnly();
foreach ($formatter->getFieldsWithValue() as $field) {
foreach ($formattedData as $field) {
$tmpVal = is_array($field['value']) ? implode(',', $field['value']) : $field['value'];
$submissionString .= '**' . ucfirst($field['name']) . '**: ' . $tmpVal . "\n";
}
Expand Down Expand Up @@ -80,8 +83,9 @@ protected function getWebhookData(): array
];
}

$message = Arr::get($settings, 'message', 'New form submission');
return [
'content' => 'New submission for your form **' . $this->form->title . '**',
'content' => (new MentionParser($message, $formattedData))->parse(),
'tts' => false,
'username' => config('app.name'),
'avatar_url' => asset('img/logo.png'),
Expand Down
53 changes: 44 additions & 9 deletions api/app/Integrations/Handlers/EmailIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,51 @@

namespace App\Integrations\Handlers;

use App\Rules\OneEmailPerLine;
use App\Notifications\Forms\FormEmailNotification;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Notification;
use App\Notifications\Forms\FormSubmissionNotification;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;

class EmailIntegration extends AbstractEmailIntegrationHandler
{
public const RISKY_USERS_LIMIT = 120;
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

Consider making RISKY_USERS_LIMIT configurable

Defining RISKY_USERS_LIMIT as a hard-coded constant may limit flexibility. Consider retrieving this value from a configuration file or environment variable to allow for easier adjustments without code changes.


public static function getValidationRules(): array
{
return [
'notification_emails' => ['required', new OneEmailPerLine()],
'notification_reply_to' => 'email|nullable',
'send_to' => 'required',
'sender_name' => 'required',
'sender_email' => 'email|nullable',
'subject' => 'required',
'email_content' => 'required',
'include_submission_data' => 'boolean',
'include_hidden_fields_submission_data' => ['nullable', 'boolean'],
'reply_to' => 'nullable',
Comment on lines +18 to +25
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

Consider enhancing validation for the 'send_to' field

The validation rules have been updated comprehensively. However, the 'send_to' field might benefit from additional validation to ensure it contains valid email addresses or mentions. Consider implementing a custom validation rule to check for the expected format.

];
}

protected function shouldRun(): bool
{
return $this->integrationData->notification_emails && parent::shouldRun();
return $this->integrationData->send_to && parent::shouldRun() && !$this->riskLimitReached();
}

// To avoid phishing abuse we limit this feature for risky users
private function riskLimitReached(): bool
{
// This is a per-workspace limit for risky workspaces
if ($this->form->workspace->is_risky) {
if ($this->form->workspace->submissions_count >= self::RISKY_USERS_LIMIT) {
Log::error('!!!DANGER!!! Dangerous user detected! Attempting many email sending.', [
'form_id' => $this->form->id,
'workspace_id' => $this->form->workspace->id,
]);
Comment on lines +40 to +43
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 the log message for clarity and professionalism

The current log message uses multiple exclamation marks and the word "DANGEROUS," which may be less professional. Consider rephrasing the message to maintain a professional tone while clearly conveying the issue.

Apply this diff to adjust the log message:

 Log::error('!!!DANGER!!! Dangerous user detected! Attempting many email sending.', [
-    'form_id' => $this->form->id,
-    'workspace_id' => $this->form->workspace->id,
+    'message' => 'Risk limit reached for email sending.',
+    'form_id' => $this->form->id,
+    'workspace_id' => $this->form->workspace->id,
 ]);
📝 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
Log::error('!!!DANGER!!! Dangerous user detected! Attempting many email sending.', [
'form_id' => $this->form->id,
'workspace_id' => $this->form->workspace->id,
]);
Log::error('Risk limit reached for email sending.', [
'message' => 'Risk limit reached for email sending.',
'form_id' => $this->form->id,
'workspace_id' => $this->form->workspace->id,
]);


return true;
}
}

return false;
}

public function handle(): void
Expand All @@ -28,19 +55,27 @@ public function handle(): void
return;
}

$subscribers = collect(preg_split("/\r\n|\n|\r/", $this->integrationData->notification_emails))
if ($this->form->is_pro) { // For Send to field Mentions are Pro feature
$formatter = (new FormSubmissionFormatter($this->form, $this->submissionData))->outputStringsOnly();
$parser = new MentionParser($this->integrationData->send_to, $formatter->getFieldsWithValue());
$sendTo = $parser->parse();
} else {
$sendTo = $this->integrationData->send_to;
}

$recipients = collect(preg_split("/\r\n|\n|\r/", $sendTo))
->filter(function ($email) {
return filter_var($email, FILTER_VALIDATE_EMAIL);
});
Log::debug('Sending email notification', [
'recipients' => $subscribers->toArray(),
'recipients' => $recipients->toArray(),
'form_id' => $this->form->id,
'form_slug' => $this->form->slug,
'mailer' => $this->mailer
]);
$subscribers->each(function ($subscriber) {
$recipients->each(function ($subscriber) {
Notification::route('mail', $subscriber)->notify(
new FormSubmissionNotification($this->event, $this->integrationData, $this->mailer)
new FormEmailNotification($this->event, $this->integrationData, $this->mailer)
Comment on lines +76 to +78
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

⚠️ Potential issue

Add error handling when sending notifications

Exceptions may occur when sending notifications (e.g., mail server issues). To enhance resilience, consider adding try-catch blocks to handle potential exceptions and log errors accordingly.

Apply this diff to include error handling:

 $recipients->each(function ($subscriber) {
+    try {
         Notification::route('mail', $subscriber)->notify(
             new FormEmailNotification($this->event, $this->integrationData, $this->mailer)
         );
+    } catch (\Exception $e) {
+        Log::error('Failed to send email notification', [
+            'subscriber' => $subscriber,
+            'error' => $e->getMessage(),
+        ]);
+    }
 });
📝 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
$recipients->each(function ($subscriber) {
Notification::route('mail', $subscriber)->notify(
new FormSubmissionNotification($this->event, $this->integrationData, $this->mailer)
new FormEmailNotification($this->event, $this->integrationData, $this->mailer)
$recipients->each(function ($subscriber) {
try {
Notification::route('mail', $subscriber)->notify(
new FormEmailNotification($this->event, $this->integrationData, $this->mailer)
);
} catch (\Exception $e) {
Log::error('Failed to send email notification', [
'subscriber' => $subscriber,
'error' => $e->getMessage(),
]);
}
});

);
});
}
Expand Down
10 changes: 7 additions & 3 deletions api/app/Integrations/Handlers/SlackIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Integrations\Handlers;

use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Support\Arr;
use Vinkla\Hashids\Facades\Hashids;
Expand Down Expand Up @@ -32,6 +33,9 @@ protected function shouldRun(): bool

protected function getWebhookData(): array
{
$formatter = (new FormSubmissionFormatter($this->form, $this->submissionData))->outputStringsOnly();
$formattedData = $formatter->getFieldsWithValue();

$settings = (array) $this->integrationData ?? [];
$externalLinks = [];
if (Arr::get($settings, 'link_open_form', true)) {
Expand All @@ -46,20 +50,20 @@ protected function getWebhookData(): array
$externalLinks[] = '*<' . $this->form->share_url . '?submission_id=' . $submissionId . '|✍️ ' . $this->form->editable_submissions_button_text . '>*';
}

$message = Arr::get($settings, 'message', 'New form submission');
$blocks = [
[
'type' => 'section',
'text' => [
'type' => 'mrkdwn',
'text' => 'New submission for your form *' . $this->form->title . '*',
'text' => (new MentionParser($message, $formattedData))->parse(),
],
],
];

if (Arr::get($settings, 'include_submission_data', true)) {
$submissionString = '';
$formatter = (new FormSubmissionFormatter($this->form, $this->submissionData))->outputStringsOnly();
foreach ($formatter->getFieldsWithValue() as $field) {
foreach ($formattedData as $field) {
$tmpVal = is_array($field['value']) ? implode(',', $field['value']) : $field['value'];
$submissionString .= '>*' . ucfirst($field['name']) . '*: ' . $tmpVal . " \n";
}
Comment on lines +66 to 69
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

Sanitize submission data to protect sensitive information

Including raw submission data in the Slack message may expose sensitive or personally identifiable information (PII). Consider implementing data sanitization or filtering to exclude or mask sensitive fields before sending the data.

Apply this diff to filter out sensitive fields:

 foreach ($formattedData as $field) {
+    // Skip sensitive fields
+    if (in_array(strtolower($field['name']), ['password', 'ssn', 'credit_card_number'])) {
+        continue;
+    }
     $tmpVal = is_array($field['value']) ? implode(',', $field['value']) : $field['value'];
     $submissionString .= '>*' . ucfirst($field['name']) . '*: ' . $tmpVal . " \n";
 }
📝 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
foreach ($formattedData as $field) {
$tmpVal = is_array($field['value']) ? implode(',', $field['value']) : $field['value'];
$submissionString .= '>*' . ucfirst($field['name']) . '*: ' . $tmpVal . " \n";
}
foreach ($formattedData as $field) {
// Skip sensitive fields
if (in_array(strtolower($field['name']), ['password', 'ssn', 'credit_card_number'])) {
continue;
}
$tmpVal = is_array($field['value']) ? implode(',', $field['value']) : $field['value'];
$submissionString .= '>*' . ucfirst($field['name']) . '*: ' . $tmpVal . " \n";
}

Expand Down
Loading
Loading