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

Email spam security #641

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
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
18 changes: 15 additions & 3 deletions api/app/Http/Controllers/Auth/RegisterController.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Validator;
use Illuminate\Validation\Rule;
use App\Rules\ValidHCaptcha;

class RegisterController extends Controller
{
Expand All @@ -27,6 +28,10 @@ class RegisterController extends Controller
public function __construct()
{
$this->middleware('guest');
if (app()->environment() !== 'testing') {
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why this is in this if?

$this->middleware('throttle:5,1')->only('register'); // 5 attempts per minute
$this->middleware('throttle:30,60')->only('register'); // 30 attempts per hour
}
}

/**
Expand Down Expand Up @@ -56,16 +61,22 @@ protected function registered(Request $request, User $user)
*/
protected function validator(array $data)
{
return Validator::make($data, [
$rules = [
'name' => 'required|max:255',
'email' => 'required|email:filter|max:255|unique:users|indisposable',
'password' => 'required|min:6|confirmed',
'hear_about_us' => 'required|string',
'agree_terms' => ['required', Rule::in([true])],
'appsumo_license' => ['nullable'],
'invite_token' => ['nullable', 'string'],
'utm_data' => ['nullable', 'array']
], [
'utm_data' => ['nullable', 'array'],
];

if (config('services.h_captcha.secret_key')) {
$rules['h-captcha-response'] = [new ValidHCaptcha()];
}

return Validator::make($data, $rules, [
'agree_terms' => 'Please agree with the terms and conditions.',
]);
}
Expand All @@ -84,6 +95,7 @@ protected function create(array $data)
'password' => bcrypt($data['password']),
'hear_about_us' => $data['hear_about_us'],
'utm_data' => array_key_exists('utm_data', $data) ? $data['utm_data'] : null,
'meta' => ['registration_ip' => request()->ip()],
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

Review IP address storage for privacy compliance

Storing IP addresses has privacy implications and might be subject to data protection regulations (GDPR, CCPA):

  1. Consider hashing the IP address
  2. Implement a retention policy
  3. Document the purpose of collection
-            'meta' => ['registration_ip' => request()->ip()],
+            'meta' => ['registration_ip' => hash('sha256', request()->ip())],

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

]);

// Add relation with user
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Http\Requests\Integration;

use App\Models\Forms\Form;
use App\Models\Integration\FormIntegration;
use App\Rules\IntegrationLogicRule;
use Illuminate\Foundation\Http\FormRequest;
Expand All @@ -14,9 +15,11 @@ class FormIntegrationsRequest extends FormRequest
public array $integrationRules = [];

private ?string $integrationClassName = null;
private ?Form $form = null;
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

Ensure $integrationClassName is initialized to prevent null reference errors

Since methods like attributes() and isOAuthRequired() rely on $this->integrationClassName, ensure it is initialized even when $request->integration_id is not provided to prevent potential null reference errors.

Consider initializing $integrationClassName or adding checks before its usage.


public function __construct(Request $request)
{
$this->form = Form::findOrFail(request()->route('id'));
if ($request->integration_id) {
// Load integration class, and get rules
$integration = FormIntegration::getIntegration($request->integration_id);
Expand Down Expand Up @@ -77,7 +80,7 @@ protected function isOAuthRequired(): bool

private function loadIntegrationRules()
{
foreach ($this->integrationClassName::getValidationRules() as $key => $value) {
foreach ($this->integrationClassName::getValidationRules($this->form) as $key => $value) {
$this->integrationRules['settings.' . $key] = $value;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public function handle(): void
Http::throw()->post($this->getWebhookUrl(), $this->getWebhookData());
}

abstract public static function getValidationRules(): array;
abstract public static function getValidationRules(?Form $form): array;

public static function isOAuthRequired(): bool
{
Expand Down
3 changes: 2 additions & 1 deletion api/app/Integrations/Handlers/DiscordIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

namespace App\Integrations\Handlers;

use App\Models\Forms\Form;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Support\Arr;
use Vinkla\Hashids\Facades\Hashids;

class DiscordIntegration extends AbstractIntegrationHandler
{
public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [
'discord_webhook_url' => 'required|url|starts_with:https://discord.com/api/webhooks',
Expand Down
34 changes: 31 additions & 3 deletions api/app/Integrations/Handlers/EmailIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@

namespace App\Integrations\Handlers;

use App\Models\Forms\Form;
use App\Models\Integration\FormIntegration;
use App\Notifications\Forms\FormEmailNotification;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Notification;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Validation\ValidationException;

class EmailIntegration extends AbstractEmailIntegrationHandler
{
public const RISKY_USERS_LIMIT = 120;

public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [
'send_to' => 'required',
$rules = [
'send_to' => ['required'],
'sender_name' => 'required',
'sender_email' => 'email|nullable',
'subject' => 'required',
Expand All @@ -24,6 +27,31 @@ public static function getValidationRules(): array
'include_hidden_fields_submission_data' => ['nullable', 'boolean'],
'reply_to' => 'nullable',
];

if ($form->is_pro || config('app.self_hosted')) {
return $rules;
}

// Free plan users can only send to a single email address (avoid spam)
$rules['send_to'][] = function ($attribute, $value, $fail) use ($form) {
if (count(explode("\n", trim($value))) > 1 || count(explode(',', $value)) > 1) {
$fail('You can only send to a single email address on the free plan. Please upgrade to the Pro plan to create a new integration.');
}
};

// Free plan users can only have a single email integration per form (avoid spam)
if (!request()->route('integrationid')) {
$existingEmailIntegrations = FormIntegration::where('form_id', $form->id)
->where('integration_id', 'email')
->count();
if ($existingEmailIntegrations > 0) {
throw ValidationException::withMessages([
'settings.send_to' => ['Free users are limited to 1 email integration per form.']
]);
}
}
Comment on lines +42 to +52
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 edge cases in integration validation.

The validation for existing integrations should:

  1. Consider soft-deleted integrations
  2. Use consistent error messages
 if (!request()->route('integrationid')) {
     $existingEmailIntegrations = FormIntegration::where('form_id', $form->id)
         ->where('integration_id', 'email')
+        ->whereNull('deleted_at')  // Exclude soft-deleted integrations
         ->count();
     if ($existingEmailIntegrations > 0) {
         throw ValidationException::withMessages([
-            'settings.send_to' => ['Free users are limited to 1 email integration per form.']
+            'settings.send_to' => ['You can only have a single email integration on the free plan. Please upgrade to the Pro plan to create a new integration.']
         ]);
     }
 }
📝 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
// Free plan users can only have a single email integration per form (avoid spam)
if (!request()->route('integrationid')) {
$existingEmailIntegrations = FormIntegration::where('form_id', $form->id)
->where('integration_id', 'email')
->count();
if ($existingEmailIntegrations > 0) {
throw ValidationException::withMessages([
'settings.send_to' => ['Free users are limited to 1 email integration per form.']
]);
}
}
// Free plan users can only have a single email integration per form (avoid spam)
if (!request()->route('integrationid')) {
$existingEmailIntegrations = FormIntegration::where('form_id', $form->id)
->where('integration_id', 'email')
->whereNull('deleted_at') // Exclude soft-deleted integrations
->count();
if ($existingEmailIntegrations > 0) {
throw ValidationException::withMessages([
'settings.send_to' => ['You can only have a single email integration on the free plan. Please upgrade to the Pro plan to create a new integration.']
]);
}
}


return $rules;
}

protected function shouldRun(): bool
Expand Down
7 changes: 3 additions & 4 deletions api/app/Integrations/Handlers/GoogleSheetsIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use App\Events\Forms\FormSubmitted;
use App\Integrations\Google\Google;
use App\Models\Forms\Form;
use App\Models\Integration\FormIntegration;
use Exception;
use Illuminate\Support\Facades\Log;
Expand All @@ -22,11 +23,9 @@ public function __construct(
$this->client = new Google($formIntegration);
}

public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [

];
return [];
}

public static function isOAuthRequired(): bool
Expand Down
3 changes: 2 additions & 1 deletion api/app/Integrations/Handlers/SlackIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

namespace App\Integrations\Handlers;

use App\Models\Forms\Form;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Support\Arr;
use Vinkla\Hashids\Facades\Hashids;

class SlackIntegration extends AbstractIntegrationHandler
{
public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [
'slack_webhook_url' => 'required|url|starts_with:https://hooks.slack.com/',
Expand Down
4 changes: 3 additions & 1 deletion api/app/Integrations/Handlers/WebhookIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

namespace App\Integrations\Handlers;

use App\Models\Forms\Form;

class WebhookIntegration extends AbstractIntegrationHandler
{
public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [
'webhook_url' => 'required|url'
Expand Down
3 changes: 2 additions & 1 deletion api/app/Integrations/Handlers/ZapierIntegration.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace App\Integrations\Handlers;

use App\Events\Forms\FormSubmitted;
use App\Models\Forms\Form;
use App\Models\Integration\FormIntegration;
use Exception;

Expand All @@ -16,7 +17,7 @@ public function __construct(
parent::__construct($event, $formIntegration, $integration);
}

public static function getValidationRules(): array
public static function getValidationRules(?Form $form): array
{
return [];
}
Expand Down
3 changes: 3 additions & 0 deletions api/app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class User extends Authenticatable implements JWTSubject
'password',
'hear_about_us',
'utm_data',
'meta'
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

Enhance security for the meta attribute

While it's good that the meta field is hidden, being fillable without validation poses risks:

  1. Mass assignment could allow injection of unexpected data
  2. No schema validation for meta contents

Consider:

  1. Remove from fillable and use explicit setters
  2. Add validation for meta contents
  3. Implement strict typing or schema validation
-    protected $fillable = [
-        'name',
-        'email',
-        'password',
-        'hear_about_us',
-        'utm_data',
-        'meta'
-    ];
+    protected $fillable = [
+        'name',
+        'email',
+        'password',
+        'hear_about_us',
+        'utm_data',
+    ];

+    public function setMeta(array $meta): void
+    {
+        $allowedKeys = ['registration_ip'];
+        $filteredMeta = array_intersect_key($meta, array_flip($allowedKeys));
+        $this->attributes['meta'] = json_encode($filteredMeta);
+    }

Also applies to: 48-48, 61-61

];

/**
Expand All @@ -44,6 +45,7 @@ class User extends Authenticatable implements JWTSubject
'password',
'remember_token',
'hear_about_us',
'meta'
];

/**
Expand All @@ -56,6 +58,7 @@ protected function casts()
return [
'email_verified_at' => 'datetime',
'utm_data' => 'array',
'meta' => 'array',
];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Query\Expression;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;

return new class () extends Migration {
/**
* Run the migrations.
*/
public function up(): void
{
$driver = DB::getDriverName();

Schema::table('users', function (Blueprint $table) use ($driver) {
if ($driver === 'mysql') {
$table->json('meta')->default(new Expression('(JSON_OBJECT())'));
} else {
$table->json('meta')->default('{}');
}
});
}

/**
* Reverse the migrations.
*/
public function down(): void
{
Schema::table('users', function (Blueprint $table) {
$table->dropColumn('meta');
});
}
};
Loading
Loading