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

ESC 399 - Enhance redirect URL handling and MentionParser functionality #639

Merged
merged 1 commit into from
Dec 10, 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
2 changes: 1 addition & 1 deletion api/app/Http/Controllers/Forms/PublicFormController.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ private function getRedirectData($form, $submissionData)
return ['id' => $key, 'value' => $value];
})->values()->all();

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

if ($redirectUrl && !filter_var($redirectUrl, FILTER_VALIDATE_URL)) {
$redirectUrl = null;
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 @@ -55,7 +55,7 @@ public function rules()
're_fillable' => 'boolean',
're_fill_button_text' => 'string|min:1|max:50',
'submitted_text' => 'string|max:2000',
'redirect_url' => 'nullable|max:255',
'redirect_url' => 'nullable|string',
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

Consider adding URL validation and reasonable length constraints.

While removing the 255-character limit is necessary for longer URLs, consider:

  1. Adding URL format validation at the request level
  2. Setting a reasonable maximum length to prevent potential DoS attacks
-            'redirect_url' => 'nullable|string',
+            'redirect_url' => 'nullable|string|url|max:2048',
📝 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|string',
'redirect_url' => 'nullable|string|url|max:2048',

'database_fields_update' => 'nullable|array',
'max_submissions_count' => 'integer|nullable|min:1',
'max_submissions_reached_text' => 'string|nullable',
Expand Down
17 changes: 15 additions & 2 deletions api/app/Open/MentionParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ class MentionParser
{
private $content;
private $data;
private $urlFriendly = false;

public function __construct($content, $data)
{
$this->content = $content;
$this->data = $data;
}

public function urlFriendlyOutput(bool $enable = true): self
{
$this->urlFriendly = $enable;
return $this;
}

public function parse()
{
$doc = new DOMDocument();
Expand All @@ -40,7 +47,7 @@ public function parse()
$value = $this->getData($fieldId);

if ($value !== null) {
$textNode = $doc->createTextNode(is_array($value) ? implode(', ', $value) : $value);
$textNode = $doc->createTextNode(is_array($value) ? implode($this->urlFriendly ? ',+' : ', ', $value) : $value);
$element->parentNode->replaceChild($textNode, $element);
} elseif ($fallback) {
$textNode = $doc->createTextNode($fallback);
Expand Down Expand Up @@ -127,7 +134,13 @@ private function getData($fieldId)
$value = collect($this->data)->firstWhere('id', $fieldId)['value'] ?? null;

if (is_object($value)) {
return (array) $value;
$value = (array) $value;
}

if ($this->urlFriendly && $value !== null) {
return is_array($value)
? array_map('urlencode', $value)
: urlencode($value);
Comment on lines +140 to +143
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 safeguards against double URL encoding.

The current implementation might double-encode values if they're already URL-encoded. Consider adding a check to prevent this.

     if ($this->urlFriendly && $value !== null) {
+        $encode = function($str) {
+            return rawurlencode(rawurldecode($str));
+        };
         return is_array($value)
-            ? array_map('urlencode', $value)
-            : urlencode($value);
+            ? array_map($encode, $value)
+            : $encode($value);
     }
📝 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 ($this->urlFriendly && $value !== null) {
return is_array($value)
? array_map('urlencode', $value)
: urlencode($value);
if ($this->urlFriendly && $value !== null) {
$encode = function($str) {
return rawurlencode(rawurldecode($str));
};
return is_array($value)
? array_map($encode, $value)
: $encode($value);

}

return $value;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class () extends Migration {
/**
* Run the migrations.
*/
public function up(): void
{
Schema::table('forms', function (Blueprint $table) {
$table->text('redirect_url')->nullable()->change();
});
}

/**
* Reverse the migrations.
*/
public function down(): void
{
Schema::table('forms', function (Blueprint $table) {
$table->string('redirect_url')->nullable()->change();
});
}
};
29 changes: 29 additions & 0 deletions api/tests/Feature/Forms/RedirectUrlLengthTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php

test('form accepts long redirect urls', function () {
$this->withoutExceptionHandling();
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);

// Create a very long URL (more than 255 characters)
$longUrl = 'https://example.com/?' . str_repeat('very-long-parameter=value&', 50);

$this->putJson(route('open.forms.update', $form->id), array_merge($form->toArray(), [
'redirect_url' => $longUrl
]))->assertStatus(200);

expect($form->fresh()->redirect_url)->toBe($longUrl);
});

test('form accepts null redirect url', function () {
$user = $this->actingAsUser();
$workspace = $this->createUserWorkspace($user);
$form = $this->createForm($user, $workspace);

$this->putJson(route('open.forms.update', $form->id), array_merge($form->toArray(), [
'redirect_url' => null
]))->assertStatus(200);

expect($form->fresh()->redirect_url)->toBeNull();
});
42 changes: 42 additions & 0 deletions api/tests/Unit/Service/Forms/MentionParserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,46 @@

expect($result)->toBe('some text replaced text dewde');
});

describe('urlFriendlyOutput', function () {
test('it encodes special characters in values', function () {
$content = '<p>Test: <span mention mention-field-id="123">Placeholder</span></p>';
$data = [['id' => '123', 'value' => 'Hello & World']];

$parser = new MentionParser($content, $data);
$result = $parser->urlFriendlyOutput()->parse();

expect($result)->toBe('<p>Test: Hello+%26+World</p>');
});

test('it encodes spaces in values', function () {
$content = '<p>Name: <span mention mention-field-id="123">Placeholder</span></p>';
$data = [['id' => '123', 'value' => 'John Doe']];

$parser = new MentionParser($content, $data);
$result = $parser->urlFriendlyOutput()->parse();

expect($result)->toBe('<p>Name: John+Doe</p>');
});

test('it encodes array values', function () {
$content = '<p>Tags: <span mention mention-field-id="123">Placeholder</span></p>';
$data = [['id' => '123', 'value' => ['Web & Mobile', 'PHP/Laravel']]];

$parser = new MentionParser($content, $data);
$result = $parser->urlFriendlyOutput()->parse();

expect($result)->toBe('<p>Tags: Web+%26+Mobile,+PHP%2FLaravel</p>');
});

test('it can be disabled explicitly', function () {
$content = '<p>Test: <span mention mention-field-id="123">Placeholder</span></p>';
$data = [['id' => '123', 'value' => 'Hello & World']];

$parser = new MentionParser($content, $data);
$result = $parser->urlFriendlyOutput(false)->parse();

expect($result)->toBe('<p>Test: Hello & World</p>');
});
});
});
Loading