ESC 399 - Enhance redirect URL handling and MentionParser functionality #639
Conversation
- Updated the `redirect_url` validation in `UserFormRequest` to accept strings instead of just max length. - Modified `MentionParser` to include a `urlFriendlyOutput` method, allowing for URL encoding of special characters in parsed values. - Adjusted the `PublicFormController` to utilize the new `urlFriendlyOutput` method for redirect URLs. - Created a migration to change the `redirect_url` field type in the database from string to text, accommodating longer URLs. - Added tests to ensure proper handling of long redirect URLs and the functionality of the new URL-friendly output feature in `MentionParser`. This update improves the flexibility and robustness of form handling and URL processing.
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the handling of the Changes
Possibly related PRs
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (5)
api/tests/Feature/Forms/RedirectUrlLengthTest.php (1)
3-17: Consider enhancing the long URL test case.While the test verifies basic functionality, consider these improvements:
- Remove
withoutExceptionHandling()to ensure proper error handling- Use a more realistic long URL with actual query parameters
- Add edge cases for special characters and URL encoding
Here's a suggested improvement:
- $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); + // Real-world long URL with special characters + $longUrl = 'https://example.com/callback?' . http_build_query([ + 'token' => str_repeat('t', 100), + 'data' => json_encode(['key' => str_repeat('value', 30)]), + 'special' => '[email protected]&other=value', + 'unicode' => '테스트' + ]);api/app/Open/MentionParser.php (1)
50-50: Consider URL parsing implications of the '+' separator.Using '+' as an array separator might cause issues as it's a special character in URLs (represents space). Consider using a more URL-safe separator like '%2C' (encoded comma).
- $textNode = $doc->createTextNode(is_array($value) ? implode($this->urlFriendly ? ',+' : ', ', $value) : $value); + $textNode = $doc->createTextNode(is_array($value) ? implode($this->urlFriendly ? '%2C' : ', ', $value) : $value);api/app/Http/Controllers/Forms/PublicFormController.php (2)
119-119: Consider adding error handling for parsing failures.While the addition of
urlFriendlyOutput()improves URL handling, consider adding error handling for potential parsing failures.- $redirectUrl = ($form->redirect_url) ? (new MentionParser($form->redirect_url, $formattedData))->urlFriendlyOutput()->parseAsText() : null; + try { + $redirectUrl = ($form->redirect_url) + ? (new MentionParser($form->redirect_url, $formattedData)) + ->urlFriendlyOutput() + ->parseAsText() + : null; + } catch (\Exception $e) { + \Log::warning('Failed to parse redirect URL', [ + 'form_id' => $form->id, + 'error' => $e->getMessage() + ]); + $redirectUrl = null; + }
Line range hint
121-123: Consider validating URL before parsing.The URL validation happens after parsing, which might make it harder to identify the root cause of invalid URLs. Consider validating the original URL before parsing.
+ if ($form->redirect_url && !filter_var($form->redirect_url, FILTER_VALIDATE_URL)) { + \Log::warning('Invalid redirect URL format before parsing', [ + 'form_id' => $form->id, + 'url' => $form->redirect_url + ]); + } + if ($redirectUrl && !filter_var($redirectUrl, FILTER_VALIDATE_URL)) { + \Log::warning('Invalid redirect URL format after parsing', [ + 'form_id' => $form->id, + 'url' => $redirectUrl + ]); $redirectUrl = null; }api/tests/Unit/Service/Forms/MentionParserTest.php (1)
177-217: Consider adding edge case tests.The test suite is comprehensive but could benefit from additional edge cases:
- Empty string values
- Null values
- Very long values
+ test('it handles empty values', function () { + $content = '<p>Empty: <span mention mention-field-id="123">Placeholder</span></p>'; + $data = [['id' => '123', 'value' => '']]; + + $parser = new MentionParser($content, $data); + $result = $parser->urlFriendlyOutput()->parse(); + + expect($result)->toBe('<p>Empty: </p>'); + }); + + test('it handles null values', function () { + $content = '<p>Null: <span mention mention-field-id="123">Placeholder</span></p>'; + $data = [['id' => '123', 'value' => null]]; + + $parser = new MentionParser($content, $data); + $result = $parser->urlFriendlyOutput()->parse(); + + expect($result)->toBe('<p>Null: </p>'); + }); + + test('it handles very long values', function () { + $content = '<p>Long: <span mention mention-field-id="123">Placeholder</span></p>'; + $data = [['id' => '123', 'value' => str_repeat('a ', 1024)]]; + + $parser = new MentionParser($content, $data); + $result = $parser->urlFriendlyOutput()->parse(); + + expect($result)->toContain(str_repeat('a+', 1024)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
api/app/Http/Controllers/Forms/PublicFormController.php(1 hunks)api/app/Http/Requests/UserFormRequest.php(1 hunks)api/app/Open/MentionParser.php(3 hunks)api/database/migrations/2024_12_05_121609_change_redirect_url_to_text_in_forms_table.php(1 hunks)api/tests/Feature/Forms/RedirectUrlLengthTest.php(1 hunks)api/tests/Unit/Service/Forms/MentionParserTest.php(1 hunks)
🔇 Additional comments (3)
api/database/migrations/2024_12_05_121609_change_redirect_url_to_text_in_forms_table.php (1)
13-15: LGTM! Verify existing data integrity.
The migration correctly changes the redirect_url column type from string to text while maintaining nullability. This change supports the requirement to handle longer URLs.
Let's verify the existing data in the forms table:
✅ Verification successful
Migration is safe and well-justified by test requirements
The change from string to text for redirect_url is well-justified and safe because:
- The original migration shows
redirect_urlwas created as a nullable string column - The test file demonstrates a clear requirement to support URLs longer than 255 characters
- The change maintains nullability, ensuring backward compatibility
- Text type can safely store all existing string data without truncation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing redirect_url values that might be affected
ast-grep --pattern 'Schema::table($_, function ($table) {
$table->string("redirect_url")$$$
})'
Length of output: 99
Script:
#!/bin/bash
# Let's search for the initial migration that created the redirect_url column
fd -e php -x grep -l "redirect_url" {} \;
# Also search for any string patterns related to redirect_url column creation
ast-grep --pattern 'Schema::create($_, function ($table) {
$$$
$table->string("redirect_url")$$$
})'
Length of output: 654
Script:
#!/bin/bash
# Let's check the content of the initial migration file to understand the original column definition
cat ./api/database/migrations/2021_05_19_140326_create_forms_table.php
# Also check the test file that might provide insights about the change requirement
cat ./api/tests/Feature/Forms/RedirectUrlLengthTest.php
Length of output: 4425
api/tests/Feature/Forms/RedirectUrlLengthTest.php (1)
19-29: LGTM! Test covers null handling.
The test properly verifies that null values are accepted for the redirect_url field.
api/app/Open/MentionParser.php (1)
20-24: LGTM! Method enables URL-friendly output.
The urlFriendlyOutput method provides a clean way to toggle URL encoding behavior.
| if ($this->urlFriendly && $value !== null) { | ||
| return is_array($value) | ||
| ? array_map('urlencode', $value) | ||
| : urlencode($value); |
There was a problem hiding this comment.
🛠️ 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.
| 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); |
| @@ -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', | |||
There was a problem hiding this comment.
Consider adding URL validation and reasonable length constraints.
While removing the 255-character limit is necessary for longer URLs, consider:
- Adding URL format validation at the request level
- 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.
| 'redirect_url' => 'nullable|string', | |
| 'redirect_url' => 'nullable|string|url|max:2048', |
…J#639) - Updated the `redirect_url` validation in `UserFormRequest` to accept strings instead of just max length. - Modified `MentionParser` to include a `urlFriendlyOutput` method, allowing for URL encoding of special characters in parsed values. - Adjusted the `PublicFormController` to utilize the new `urlFriendlyOutput` method for redirect URLs. - Created a migration to change the `redirect_url` field type in the database from string to text, accommodating longer URLs. - Added tests to ensure proper handling of long redirect URLs and the functionality of the new URL-friendly output feature in `MentionParser`. This update improves the flexibility and robustness of form handling and URL processing.
redirect_urlvalidation inUserFormRequestto accept strings instead of just max length.MentionParserto include aurlFriendlyOutputmethod, allowing for URL encoding of special characters in parsed values.PublicFormControllerto utilize the newurlFriendlyOutputmethod for redirect URLs.redirect_urlfield type in the database from string to text, accommodating longer URLs.MentionParser.This update improves the flexibility and robustness of form handling and URL processing.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests