Conversation
WalkthroughA new PHP service is introduced to prefill afform fields using encrypted tokens from URL parameters. The service decrypts and parses the token, updates form fields accordingly, and logs the process. Additionally, dispatch email URLs are now constructed with encrypted tokens instead of plain query parameters for enhanced security. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant AfformPrefillService
participant AngularApp
User->>WebApp: Access URL with encrypted token parameter
WebApp->>AfformPrefillService: Call preprocess with Angular manager
AfformPrefillService->>AfformPrefillService: Decrypt and parse token JSON
AfformPrefillService->>AngularApp: Inject ChangeSet to prefill and lock fields
AngularApp->>User: Render prefilled, read-only form fields
sequenceDiagram
participant DroppingCenterService
participant EmailSystem
participant User
DroppingCenterService->>DroppingCenterService: Prepare dispatch form parameters
DroppingCenterService->>DroppingCenterService: Encode and encrypt parameters as token
DroppingCenterService->>EmailSystem: Send email with URL containing encrypted token
User->>WebApp: Click URL, triggers afform prefill process
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php (1)
Learnt from: belwalshubham
PR: ColoredCow/goonj#284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php (2)
73-81: LGTM!The
getFieldDefinitionmethod properly validates the field definition format before parsing.
57-62: Sanitize values before injecting into HTMLThe values from the decrypted token are directly injected into HTML attributes without proper sanitization, which could lead to XSS vulnerabilities.
The code already uses
htmlspecialcharsfor the entire object, but individual values should be validated:// Inject default value and make read-only + // Validate the value type and sanitize if string + if (is_string($data[$fieldName])) { + $data[$fieldName] = htmlspecialchars($data[$fieldName], ENT_QUOTES); + } $fieldDefn['afform_default'] = $data[$fieldName]; $fieldDefn['readonly'] = true;Likely an incorrect or invalid review comment.
| // Step 1: Gather data | ||
| $dispatchData = [ | ||
| 'Camp_Vehicle_Dispatch.Dropping_Center' => $droppingCenterId, | ||
| 'Camp_Vehicle_Dispatch.Filled_by' => $contactId, | ||
| 'Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent' => $goonjOffice, | ||
| 'Camp_Vehicle_Dispatch.Goonj_Office_Name' => $goonjOfficeName, | ||
| 'Eck_Collection_Camp1' => $droppingCenterId, | ||
| 'id' => $droppingCenterId, | ||
| ]; | ||
|
|
||
| // Step 2: Convert to JSON and encrypt | ||
| $json = json_encode($dispatchData); | ||
| $token = \CRM_Utils_Crypt::encrypt($json); |
There was a problem hiding this comment.
Add error handling for encryption failures
The encryption operation could fail, but there's no error handling. If CRM_Utils_Crypt::encrypt() throws an exception or returns false, the email will be sent with a malformed URL.
Wrap the encryption in a try-catch block:
- // Step 2: Convert to JSON and encrypt
- $json = json_encode($dispatchData);
- $token = \CRM_Utils_Crypt::encrypt($json);
+ // Step 2: Convert to JSON and encrypt
+ $json = json_encode($dispatchData);
+ try {
+ $token = \CRM_Utils_Crypt::encrypt($json);
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to encrypt dispatch data: ' . $e->getMessage());
+ throw new \CRM_Core_Exception('Failed to generate dispatch email');
+ }🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php around
lines 548 to 560, the encryption call to CRM_Utils_Crypt::encrypt() lacks error
handling, risking malformed URLs if encryption fails. Wrap the encryption call
in a try-catch block to catch exceptions and check if the result is false;
handle these cases by logging the error or aborting the operation to prevent
sending invalid data.
| $dispatchData = [ | ||
| 'Camp_Vehicle_Dispatch.Dropping_Center' => $droppingCenterId, | ||
| 'Camp_Vehicle_Dispatch.Filled_by' => $contactId, | ||
| 'Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent' => $goonjOffice, | ||
| 'Camp_Vehicle_Dispatch.Goonj_Office_Name' => $goonjOfficeName, | ||
| 'Eck_Collection_Camp1' => $droppingCenterId, | ||
| 'id' => $droppingCenterId, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input parameters before encryption
The method assumes all parameters are valid without verification. This could lead to issues if any required data is missing or malformed.
Add validation before constructing the dispatch data:
+ // Validate required parameters
+ if (empty($droppingCenterId) || empty($contactId) || empty($goonjOffice)) {
+ \Civi::log()->error('Missing required parameters for dispatch email');
+ return;
+ }
+
// Step 1: Gather data
$dispatchData = [
'Camp_Vehicle_Dispatch.Dropping_Center' => $droppingCenterId,🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php around
lines 549 to 556, the code constructs the $dispatchData array without validating
input parameters, which risks errors if data is missing or invalid. Add
validation checks for each input parameter (like $droppingCenterId, $contactId,
$goonjOffice, and $goonjOfficeName) before using them to ensure they are set and
correctly formatted. If any required parameter is missing or invalid, handle the
error appropriately before proceeding to construct $dispatchData.
| $emailHtml = " | ||
| <html> | ||
| <body> | ||
| <p>Dear {$initiatorName},</p> | ||
| <p>Thank you so much for your invaluable efforts in running the Goonj Dropping Center. | ||
| Your dedication plays a crucial role in our work, and we deeply appreciate your continued support.</p> | ||
| <p>Please fill out this Dispatch Form – <a href='{$vehicleDispatchFormUrl}'>Link</a> once the vehicle is loaded and ready to head to Goonj’s processing center. | ||
| This will help us to verify and acknowledge the materials as soon as they arrive.</p> | ||
| <p>We truly appreciate your cooperation and continued commitment to our cause.</p> | ||
| <p>Warm Regards,<br>Team Goonj..</p> | ||
| </body> | ||
| </html> | ||
| "; | ||
| $from = HelperService::getDefaultFromEmail(); | ||
| $mailParams = [ | ||
| 'subject' => 'Kindly fill the Dispatch Form for Material Pickup', | ||
| 'from' => $from, | ||
| 'toEmail' => $email, | ||
| 'html' => $emailHtml, | ||
| ]; | ||
| </body> | ||
| </html> | ||
| "; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract email HTML to a template file
Inline HTML violates the separation of concerns principle and makes the code harder to maintain. Consider moving this to a Smarty template for better maintainability and localization support.
Create a new template file templates/CRM/Goonjcustom/Email/DispatchForm.tpl and use CiviCRM's template system:
- // Step 4: Compose HTML email
- $emailHtml = "
- <html>
- <body>
- <p>Dear {$initiatorName},</p>
- <p>Thank you so much for your invaluable efforts in running the Goonj Dropping Center.
- Your dedication plays a crucial role in our work, and we deeply appreciate your continued support.</p>
- <p>Please fill out this Dispatch Form – <a href='{$vehicleDispatchFormUrl}'>Link</a> once the vehicle is loaded and ready to head to Goonj's processing center.
- This will help us to verify and acknowledge the materials as soon as they arrive.</p>
- <p>We truly appreciate your cooperation and continued commitment to our cause.</p>
- <p>Warm Regards,<br>Team Goonj..</p>
- </body>
- </html>
- ";
+ // Step 4: Compose HTML email
+ $tplParams = [
+ 'initiatorName' => $initiatorName,
+ 'vehicleDispatchFormUrl' => $vehicleDispatchFormUrl,
+ ];
+ $emailHtml = CRM_Core_Smarty::singleton()->fetchWith('CRM/Goonjcustom/Email/DispatchForm.tpl', $tplParams);📝 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.
| $emailHtml = " | |
| <html> | |
| <body> | |
| <p>Dear {$initiatorName},</p> | |
| <p>Thank you so much for your invaluable efforts in running the Goonj Dropping Center. | |
| Your dedication plays a crucial role in our work, and we deeply appreciate your continued support.</p> | |
| <p>Please fill out this Dispatch Form – <a href='{$vehicleDispatchFormUrl}'>Link</a> once the vehicle is loaded and ready to head to Goonj’s processing center. | |
| This will help us to verify and acknowledge the materials as soon as they arrive.</p> | |
| <p>We truly appreciate your cooperation and continued commitment to our cause.</p> | |
| <p>Warm Regards,<br>Team Goonj..</p> | |
| </body> | |
| </html> | |
| "; | |
| $from = HelperService::getDefaultFromEmail(); | |
| $mailParams = [ | |
| 'subject' => 'Kindly fill the Dispatch Form for Material Pickup', | |
| 'from' => $from, | |
| 'toEmail' => $email, | |
| 'html' => $emailHtml, | |
| ]; | |
| </body> | |
| </html> | |
| "; | |
| // Step 4: Compose HTML email | |
| $tplParams = [ | |
| 'initiatorName' => $initiatorName, | |
| 'vehicleDispatchFormUrl'=> $vehicleDispatchFormUrl, | |
| ]; | |
| $emailHtml = CRM_Core_Smarty::singleton() | |
| ->fetchWith('CRM/Goonjcustom/Email/DispatchForm.tpl', $tplParams); |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php around
lines 566 to 578, the email HTML is hardcoded inline, which reduces
maintainability and localization support. To fix this, extract the HTML content
into a new Smarty template file named
templates/CRM/Goonjcustom/Email/DispatchForm.tpl. Then, update the PHP code to
load and render this template using CiviCRM's template system, passing necessary
variables like initiatorName and vehicleDispatchFormUrl to the template for
dynamic content rendering.
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | ||
|
|
||
| if (!$request) { | ||
| Civi::log()->warning('[AfformPrefill] No token found in URL.'); | ||
| return; | ||
| } | ||
|
|
||
| // Decrypt token | ||
| try { | ||
| $decrypted = CRM_Utils_Crypt::decrypt($request); | ||
| $data = json_decode($decrypted, true); | ||
| Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true)); | ||
| } catch (\Exception $e) { | ||
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| if (!is_array($data)) { | ||
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add security validations for the token
The token is accepted without any validation of its length, format, or structure. This could be exploited with malformed tokens.
Add validation for the token and decrypted data:
$request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET');
if (!$request) {
Civi::log()->warning('[AfformPrefill] No token found in URL.');
return;
}
+
+ // Validate token length to prevent DoS
+ if (strlen($request) > 10000) {
+ Civi::log()->error('[AfformPrefill] Token exceeds maximum allowed length.');
+ return;
+ }
// Decrypt token
try {
$decrypted = CRM_Utils_Crypt::decrypt($request);
$data = json_decode($decrypted, true);
+
+ // Validate expected structure
+ $expectedKeys = ['Camp_Vehicle_Dispatch.Dropping_Center', 'Camp_Vehicle_Dispatch.Filled_by'];
+ foreach ($expectedKeys as $key) {
+ if (!array_key_exists($key, $data)) {
+ throw new \Exception("Missing required key: {$key}");
+ }
+ }
+
Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true));📝 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.
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | |
| if (!$request) { | |
| Civi::log()->warning('[AfformPrefill] No token found in URL.'); | |
| return; | |
| } | |
| // Decrypt token | |
| try { | |
| $decrypted = CRM_Utils_Crypt::decrypt($request); | |
| $data = json_decode($decrypted, true); | |
| Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true)); | |
| } catch (\Exception $e) { | |
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | |
| return; | |
| } | |
| if (!is_array($data)) { | |
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | |
| return; | |
| } | |
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | |
| if (!$request) { | |
| Civi::log()->warning('[AfformPrefill] No token found in URL.'); | |
| return; | |
| } | |
| // Validate token length to prevent DoS | |
| if (strlen($request) > 10000) { | |
| Civi::log()->error('[AfformPrefill] Token exceeds maximum allowed length.'); | |
| return; | |
| } | |
| // Decrypt token | |
| try { | |
| $decrypted = CRM_Utils_Crypt::decrypt($request); | |
| $data = json_decode($decrypted, true); | |
| // Validate expected structure | |
| $expectedKeys = [ | |
| 'Camp_Vehicle_Dispatch.Dropping_Center', | |
| 'Camp_Vehicle_Dispatch.Filled_by', | |
| ]; | |
| foreach ($expectedKeys as $key) { | |
| if (!array_key_exists($key, $data)) { | |
| throw new \Exception("Missing required key: {$key}"); | |
| } | |
| } | |
| Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true)); | |
| } catch (\Exception $e) { | |
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | |
| return; | |
| } | |
| if (!is_array($data)) { | |
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
around lines 16 to 36, the token retrieved from the request lacks validation for
length, format, or structure, which poses a security risk. Add checks to
validate the token string before decryption, ensuring it meets expected length
and format criteria (e.g., alphanumeric, specific length). After decryption,
validate that the decoded data is an array and contains the required keys or
structure expected for further processing. Return early and log warnings or
errors if validations fail.
| Civi::log()->debug('[AfformPrefill] Preprocess started.'); | ||
|
|
||
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | ||
|
|
||
| if (!$request) { | ||
| Civi::log()->warning('[AfformPrefill] No token found in URL.'); | ||
| return; | ||
| } | ||
|
|
||
| // Decrypt token | ||
| try { | ||
| $decrypted = CRM_Utils_Crypt::decrypt($request); | ||
| $data = json_decode($decrypted, true); | ||
| Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true)); | ||
| } catch (\Exception $e) { | ||
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| if (!is_array($data)) { | ||
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | ||
| return; | ||
| } | ||
|
|
||
| $changeSet = \Civi\Angular\ChangeSet::create('injectPrefillValues') | ||
| ->alterHtml(';\\.aff\\.html$;', function($doc, $path) use ($data) { | ||
| Civi::log()->debug("[AfformPrefill] Altering HTML template: {$path}"); | ||
|
|
||
| foreach (pq('af-field', $doc) as $afField) { | ||
| $fieldName = $afField->getAttribute('name'); | ||
|
|
||
| if (!$fieldName) { | ||
| continue; | ||
| } | ||
|
|
||
| if (array_key_exists($fieldName, $data)) { | ||
| $fieldDefn = self::getFieldDefinition($afField); | ||
|
|
||
| if (!$fieldDefn) { | ||
| Civi::log()->warning("[AfformPrefill] No field definition for: {$fieldName}"); | ||
| continue; | ||
| } | ||
|
|
||
| // Inject default value and make read-only | ||
| $fieldDefn['afform_default'] = $data[$fieldName]; | ||
| $fieldDefn['readonly'] = true; | ||
|
|
||
| pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); | ||
| Civi::log()->info("[AfformPrefill] Prefilled field '{$fieldName}' with value: " . json_encode($data[$fieldName])); | ||
| } else { | ||
| Civi::log()->debug("[AfformPrefill] Field '{$fieldName}' not in token data, skipping."); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| $event->angular->add($changeSet); | ||
| Civi::log()->debug('[AfformPrefill] ChangeSet injected successfully.'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce logging verbosity and avoid logging sensitive data
The service has excessive debug logging that could impact performance and logs decrypted data (line 27) which may contain sensitive information.
Remove verbose debug logs and sensitive data logging:
- Civi::log()->debug('[AfformPrefill] Preprocess started.');
-
$request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET');
if (!$request) {
- Civi::log()->warning('[AfformPrefill] No token found in URL.');
+ Civi::log()->debug('[AfformPrefill] No token found in URL.');
return;
}
// Decrypt token
try {
$decrypted = CRM_Utils_Crypt::decrypt($request);
$data = json_decode($decrypted, true);
- Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true));
+ Civi::log()->info('[AfformPrefill] Token decrypted successfully');Also remove the debug logs on lines 40, 64, and 70 as they don't add value in production.
📝 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.
| Civi::log()->debug('[AfformPrefill] Preprocess started.'); | |
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | |
| if (!$request) { | |
| Civi::log()->warning('[AfformPrefill] No token found in URL.'); | |
| return; | |
| } | |
| // Decrypt token | |
| try { | |
| $decrypted = CRM_Utils_Crypt::decrypt($request); | |
| $data = json_decode($decrypted, true); | |
| Civi::log()->debug('[AfformPrefill] Token decrypted: ' . print_r($data, true)); | |
| } catch (\Exception $e) { | |
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | |
| return; | |
| } | |
| if (!is_array($data)) { | |
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | |
| return; | |
| } | |
| $changeSet = \Civi\Angular\ChangeSet::create('injectPrefillValues') | |
| ->alterHtml(';\\.aff\\.html$;', function($doc, $path) use ($data) { | |
| Civi::log()->debug("[AfformPrefill] Altering HTML template: {$path}"); | |
| foreach (pq('af-field', $doc) as $afField) { | |
| $fieldName = $afField->getAttribute('name'); | |
| if (!$fieldName) { | |
| continue; | |
| } | |
| if (array_key_exists($fieldName, $data)) { | |
| $fieldDefn = self::getFieldDefinition($afField); | |
| if (!$fieldDefn) { | |
| Civi::log()->warning("[AfformPrefill] No field definition for: {$fieldName}"); | |
| continue; | |
| } | |
| // Inject default value and make read-only | |
| $fieldDefn['afform_default'] = $data[$fieldName]; | |
| $fieldDefn['readonly'] = true; | |
| pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); | |
| Civi::log()->info("[AfformPrefill] Prefilled field '{$fieldName}' with value: " . json_encode($data[$fieldName])); | |
| } else { | |
| Civi::log()->debug("[AfformPrefill] Field '{$fieldName}' not in token data, skipping."); | |
| } | |
| } | |
| }); | |
| $event->angular->add($changeSet); | |
| Civi::log()->debug('[AfformPrefill] ChangeSet injected successfully.'); | |
| // (Removed overly-verbose startup debug) | |
| $request = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | |
| if (!$request) { | |
| Civi::log()->debug('[AfformPrefill] No token found in URL.'); | |
| return; | |
| } | |
| // Decrypt token | |
| try { | |
| $decrypted = CRM_Utils_Crypt::decrypt($request); | |
| $data = json_decode($decrypted, true); | |
| Civi::log()->info('[AfformPrefill] Token decrypted successfully'); | |
| } catch (\Exception $e) { | |
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | |
| return; | |
| } | |
| if (!is_array($data)) { | |
| Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); | |
| return; | |
| } | |
| $changeSet = \Civi\Angular\ChangeSet::create('injectPrefillValues') | |
| ->alterHtml(';\.aff\.html$;', function($doc, $path) use ($data) { | |
| Civi::log()->debug("[AfformPrefill] Altering HTML template: {$path}"); | |
| foreach (pq('af-field', $doc) as $afField) { | |
| $fieldName = $afField->getAttribute('name'); | |
| if (!$fieldName) { | |
| continue; | |
| } | |
| if (array_key_exists($fieldName, $data)) { | |
| $fieldDefn = self::getFieldDefinition($afField); | |
| if (!$fieldDefn) { | |
| Civi::log()->warning("[AfformPrefill] No field definition for: {$fieldName}"); | |
| continue; | |
| } | |
| // Inject default value and make read-only | |
| $fieldDefn['afform_default'] = $data[$fieldName]; | |
| $fieldDefn['readonly'] = true; | |
| pq($afField) | |
| ->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); | |
| Civi::log()->info("[AfformPrefill] Prefilled field '{$fieldName}' with value: " . json_encode($data[$fieldName])); | |
| } else { | |
| Civi::log()->debug("[AfformPrefill] Field '{$fieldName}' not in token data, skipping."); | |
| } | |
| } | |
| }); | |
| $event->angular->add($changeSet); | |
| Civi::log()->debug('[AfformPrefill] ChangeSet injected successfully.'); |
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
from lines 14 to 70, reduce logging verbosity by removing debug logs that output
decrypted token data and other non-essential debug messages. Specifically,
remove the debug log on line 27 that prints decrypted token contents, and also
remove debug logs on lines 40, 64, and 70. Keep only warning, error, and info
logs that are necessary for monitoring and troubleshooting, avoiding logging any
sensitive data.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php (2)
16-21: Add security validations for the tokenThe token is accepted without any validation of its length, format, or structure. This could be exploited with malformed tokens.
14-14: Reduce logging verbosity and avoid logging sensitive dataThe service has excessive debug logging that could impact performance and logs decrypted data (line 26) which may contain sensitive information.
Also applies to: 26-26, 39-39, 62-62, 68-68
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php (2)
38-38: Make the regex pattern more specificThe current regex
;\\.aff\\.html$;is too broad and could match unintended files. Consider making it more specific to the expected template structure.- ->alterHtml(';\\.aff\\.html$;', function ($doc, $path) use ($data) { + ->alterHtml(';/afform/[^/]+\\.aff\\.html$;', function ($doc, $path) use ($data) {
32-35: Improve error message specificityThe error message "not valid JSON" is misleading since the JSON was successfully decoded but the result wasn't an array. This could confuse debugging efforts.
if (!is_array($data)) { - Civi::log()->error('[AfformPrefill] Decrypted token is not valid JSON.'); + Civi::log()->error('[AfformPrefill] Decrypted token data is not an array.'); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php(1 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/civi-extensions/goonjcustom/goonjcustom.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pokhiii
PR: ColoredCow/goonj#349
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php:95-96
Timestamp: 2024-10-12T10:58:37.585Z
Learning: When constructing URLs for CiviCRM's Formbuilder, using `#?` is an intentional method to pass values.
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php (2)
Learnt from: belwalshubham
PR: ColoredCow/goonj#284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Learnt from: belwalshubham
PR: ColoredCow/goonj#338
File: wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php:69-70
Timestamp: 2024-10-13T01:00:56.008Z
Learning: In `DroppingCenterService.php` (PHP, CiviCRM framework), when the `is_active` property remains constant (`1`) for all tabs in the `droppingCenterTabset` method, it's preferred to hardcode `is_active` in the loop instead of including it in the `$tabConfigs` array to reduce redundancy.
| Civi::log()->debug("[AfformPrefill] Altering HTML template: {$path}"); | ||
|
|
||
| foreach (pq('af-field', $doc) as $afField) { | ||
| $fieldName = $afField->getAttribute('name'); |
There was a problem hiding this comment.
Sanitize field names to prevent potential attacks
Field names are used directly without validation, which could lead to security issues if malicious field names are processed.
$fieldName = $afField->getAttribute('name');
if (!$fieldName) {
continue;
}
+// Validate field name format
+if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_\.]*$/', $fieldName)) {
+ Civi::log()->warning("[AfformPrefill] Invalid field name format: {$fieldName}");
+ continue;
+}
if (array_key_exists($fieldName, $data)) {Also applies to: 48-48
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
at lines 42 and 48, the field names retrieved from getAttribute('name') are used
without sanitization, posing a security risk. To fix this, sanitize the field
names by validating and cleaning the input to allow only expected characters
(e.g., alphanumeric and underscores) before using them. This prevents potential
injection or other attacks from malicious field names.
| $changeSet = \Civi\Angular\ChangeSet::create('injectPrefillValues') | ||
| ->alterHtml(';\\.aff\\.html$;', function ($doc, $path) use ($data) { | ||
| Civi::log()->debug("[AfformPrefill] Altering HTML template: {$path}"); | ||
|
|
||
| foreach (pq('af-field', $doc) as $afField) { | ||
| $fieldName = $afField->getAttribute('name'); | ||
|
|
||
| if (!$fieldName) { | ||
| continue; | ||
| } | ||
|
|
||
| if (array_key_exists($fieldName, $data)) { | ||
| $fieldDefn = self::getFieldDefinition($afField); | ||
|
|
||
| if (!$fieldDefn) { | ||
| Civi::log()->warning("[AfformPrefill] No field definition for: {$fieldName}"); | ||
| continue; | ||
| } | ||
|
|
||
| $fieldDefn['afform_default'] = $data[$fieldName]; | ||
| $fieldDefn['readonly'] = true; | ||
|
|
||
| pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); | ||
| Civi::log()->info("[AfformPrefill] Prefilled field '{$fieldName}' with value: " . json_encode($data[$fieldName])); | ||
| } else { | ||
| Civi::log()->debug("[AfformPrefill] Field '{$fieldName}' not in token data, skipping."); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| $angular->add($changeSet); | ||
| Civi::log()->debug('[AfformPrefill] ChangeSet injected successfully.'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break down the monolithic preprocess method
This method violates the Single Responsibility Principle by handling token processing, template modification, and field updates all in one place. Consider extracting smaller, focused methods.
public function preprocess(\Civi\Angular\Manager $angular) {
- // All the current logic here
+ $data = $this->processToken();
+ if (!$data) {
+ return;
+ }
+
+ $changeSet = $this->createFieldPrefillChangeSet($data);
+ $angular->add($changeSet);
}
+private function processToken() {
+ // Token retrieval and decryption logic
+}
+private function createFieldPrefillChangeSet($data) {
+ // ChangeSet creation and field processing logic
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
between lines 37 and 69, the current method is handling multiple
responsibilities including token processing, HTML template modification, and
field updates, which violates the Single Responsibility Principle. Refactor by
extracting distinct smaller methods for each responsibility: one for processing
tokens and preparing data, another for modifying the HTML template, and a third
for updating individual fields. Then call these methods sequentially to keep the
main method clean and focused.
| */ | ||
| class CRM_Goonjcustom_AfformFieldPrefillService extends AutoService { | ||
|
|
||
| public static function preprocess(\Civi\Angular\Manager $angular) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid static methods for better testability
Static methods make unit testing difficult and violate dependency injection principles. Consider making this an instance method instead.
- public static function preprocess(\Civi\Angular\Manager $angular) {
+ public function preprocess(\Civi\Angular\Manager $angular) {📝 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.
| public static function preprocess(\Civi\Angular\Manager $angular) { | |
| public function preprocess(\Civi\Angular\Manager $angular) { |
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
at line 13, the preprocess method is declared static, which hinders unit testing
and dependency injection. Change the method from static to an instance method by
removing the static keyword and update all calls to this method accordingly to
use an instance of the class instead of calling it statically.
| $fieldDefn['afform_default'] = $data[$fieldName]; | ||
| $fieldDefn['readonly'] = true; | ||
|
|
||
| pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); |
There was a problem hiding this comment.
Potential XSS vulnerability in field definition handling
The field definition is serialized and inserted into HTML attributes without proper validation of the input data values, which could lead to XSS attacks.
+// Sanitize the field value before setting
+$sanitizedValue = is_string($data[$fieldName]) ?
+ htmlspecialchars($data[$fieldName], ENT_QUOTES, 'UTF-8') :
+ $data[$fieldName];
+
+$fieldDefn['afform_default'] = $sanitizedValue;
-$fieldDefn['afform_default'] = $data[$fieldName];
$fieldDefn['readonly'] = true;
pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT));📝 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.
| pq($afField)->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); | |
| // Sanitize the field value before setting | |
| $sanitizedValue = is_string($data[$fieldName]) | |
| ? htmlspecialchars($data[$fieldName], ENT_QUOTES, 'UTF-8') | |
| : $data[$fieldName]; | |
| $fieldDefn['afform_default'] = $sanitizedValue; | |
| $fieldDefn['readonly'] = true; | |
| pq($afField) | |
| ->attr('defn', htmlspecialchars(\CRM_Utils_JS::writeObject($fieldDefn), ENT_COMPAT)); |
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
at line 59, the field definition is serialized and inserted into an HTML
attribute without sufficient validation, risking XSS vulnerabilities. To fix
this, ensure that all data within $fieldDefn is properly sanitized or escaped
before serialization, and consider using a safer encoding method that prevents
injection of malicious scripts when outputting into HTML attributes.
| public static function getFieldDefinition($afField) { | ||
| $existingFieldDefn = trim(pq($afField)->attr('defn') ?: ''); | ||
|
|
||
| if ($existingFieldDefn && $existingFieldDefn[0] !== '{') { | ||
| return NULL; | ||
| } | ||
|
|
||
| return $existingFieldDefn ? \CRM_Utils_JS::getRawProps($existingFieldDefn) : []; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for field definition parsing
The CRM_Utils_JS::getRawProps() call could throw exceptions that aren't handled, potentially breaking the entire prefill process.
- return $existingFieldDefn ? \CRM_Utils_JS::getRawProps($existingFieldDefn) : [];
+ if (!$existingFieldDefn) {
+ return [];
+ }
+
+ try {
+ return \CRM_Utils_JS::getRawProps($existingFieldDefn);
+ } catch (\Exception $e) {
+ Civi::log()->error('[AfformPrefill] Failed to parse field definition: ' . $e->getMessage());
+ return [];
+ }📝 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.
| public static function getFieldDefinition($afField) { | |
| $existingFieldDefn = trim(pq($afField)->attr('defn') ?: ''); | |
| if ($existingFieldDefn && $existingFieldDefn[0] !== '{') { | |
| return NULL; | |
| } | |
| return $existingFieldDefn ? \CRM_Utils_JS::getRawProps($existingFieldDefn) : []; | |
| } | |
| public static function getFieldDefinition($afField) { | |
| $existingFieldDefn = trim(pq($afField)->attr('defn') ?: ''); | |
| if ($existingFieldDefn && $existingFieldDefn[0] !== '{') { | |
| return NULL; | |
| } | |
| if (!$existingFieldDefn) { | |
| return []; | |
| } | |
| try { | |
| return \CRM_Utils_JS::getRawProps($existingFieldDefn); | |
| } catch (\Exception $e) { | |
| Civi::log()->error('[AfformPrefill] Failed to parse field definition: ' . $e->getMessage()); | |
| return []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/AfformFieldPrefillService.php
around lines 71 to 79, the call to CRM_Utils_JS::getRawProps() may throw
exceptions that are currently unhandled. Wrap this call in a try-catch block to
catch any exceptions, and handle them gracefully by returning an empty array or
null to prevent the prefill process from breaking.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
812-812: Fix undefined variable$fromEmailThe variable
$fromEmailis not defined, which will cause a runtime error.Apply this diff to fix the undefined variable:
- 'replyTo' => $fromEmail['label'], + 'replyTo' => $from,
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (1)
67-67: Remove excessive blank linesMultiple consecutive blank lines affect code readability.
Remove the extra blank lines to maintain consistent spacing throughout the file.
Also applies to: 79-80, 90-94
wp-content/civi-extensions/goonjcustom/js/prefill.js (2)
28-28: Use optional chaining for cleaner codeStatic analysis correctly identifies an opportunity to use optional chaining.
Apply this diff to use optional chaining:
-if (!(window.angular && window.angular.module)) { +if (!window.angular?.module) {
158-158: Fix typo in console logMissing space in the console log message.
- console.log('[AfformPrefill]Autofill completed!'); + console.log('[AfformPrefill] Autofill completed!');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(2 hunks)wp-content/civi-extensions/goonjcustom/js/prefill.js(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: pokhiii
PR: ColoredCow/goonj#349
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php:95-96
Timestamp: 2024-10-12T10:58:37.585Z
Learning: When constructing URLs for CiviCRM's Formbuilder, using `#?` is an intentional method to pass values.
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php (2)
Learnt from: belwalshubham
PR: #284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php, it's acceptable that the reGenerateCollectionCampQr method calls generateQrCode with only the $collectionCampId parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Learnt from: pokhiii
PR: #349
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php:95-96
Timestamp: 2024-10-12T10:58:37.585Z
Learning: When constructing URLs for CiviCRM's Formbuilder, using #? is an intentional method to pass values.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
Learnt from: belwalshubham
PR: #284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php, it's acceptable that the reGenerateCollectionCampQr method calls generateQrCode with only the $collectionCampId parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
🪛 PHPMD (2.15.0)
wp-content/civi-extensions/goonjcustom/goonjcustom.php
39-39: Avoid unused parameters such as '$angular'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 Biome (1.9.4)
wp-content/civi-extensions/goonjcustom/js/prefill.js
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterService.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pokhiii
PR: ColoredCow/goonj#349
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php:95-96
Timestamp: 2024-10-12T10:58:37.585Z
Learning: When constructing URLs for CiviCRM's Formbuilder, using `#?` is an intentional method to pass values.
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php (2)
Learnt from: belwalshubham
PR: #284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php, it's acceptable that the reGenerateCollectionCampQr method calls generateQrCode with only the $collectionCampId parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Learnt from: pokhiii
PR: #349
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampVolunteerFeedbackService.php:95-96
Timestamp: 2024-10-12T10:58:37.585Z
Learning: When constructing URLs for CiviCRM's Formbuilder, using #? is an intentional method to pass values.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
Learnt from: belwalshubham
PR: #284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php, it's acceptable that the reGenerateCollectionCampQr method calls generateQrCode with only the $collectionCampId parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
🪛 PHPMD (2.15.0)
wp-content/civi-extensions/goonjcustom/goonjcustom.php
39-39: Avoid unused parameters such as '$angular'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 Biome (1.9.4)
wp-content/civi-extensions/goonjcustom/js/prefill.js
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| */ | ||
| public static function sendDispatchEmail($nameOfInstitution, $address, $phone, $email, $initiatorName, $droppingCenterId, $contactId, $goonjOffice, $goonjOfficeName) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| public static function sendDispatchEmail($nameOfInstitution, $address, $phone, $email, $initiatorName, $droppingCenterId, $contactId, $goonjOffice, $goonjOfficeName) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor method to reduce parameter count
The method has 9 parameters, which makes it difficult to maintain and call correctly.
Consider using a data transfer object (DTO) or array to encapsulate the dispatch email data:
-public static function sendDispatchEmail($nameOfInstitution, $address, $phone, $email, $initiatorName, $droppingCenterId, $contactId, $goonjOffice, $goonjOfficeName) {
+public static function sendDispatchEmail(array $dispatchEmailData) {
+ // Extract values from $dispatchEmailData arrayCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php
at line 673, the sendDispatchEmail method has 9 parameters, making it hard to
maintain and use. Refactor this method by creating a data transfer object (DTO)
or an associative array to encapsulate all these parameters into a single
argument. Update the method signature to accept this DTO or array and adjust the
method body to extract needed values from it, simplifying calls and improving
readability.
| // Step 2: Convert to JSON and encrypt | ||
| $json = json_encode($dispatchData); | ||
| $token = \CRM_Utils_Crypt::encrypt($json); | ||
|
|
There was a problem hiding this comment.
Add token expiration for enhanced security
The encrypted token doesn't include an expiration timestamp, which could allow indefinite reuse.
Consider adding an expiration timestamp to the token data:
// Step 2: Convert to JSON and encrypt
+ $dispatchData['exp'] = time() + (24 * 60 * 60); // 24 hour expiration
$json = json_encode($dispatchData);
$token = \CRM_Utils_Crypt::encrypt($json);🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php
around lines 690 to 693, the encrypted token is created without an expiration
timestamp, allowing indefinite reuse. To fix this, add an expiration timestamp
field to the $dispatchData array before encoding it to JSON and encrypting. Set
the expiration to a reasonable future time (e.g., current time plus a validity
period) so that the token can be validated and rejected after expiration.
| // $url = \CRM_Utils_System::url($path, $params, TRUE, NULL, FALSE, TRUE); | ||
| $config = CRM_Core_Config::singleton(); | ||
| $url = $config->userFrameworkBaseURL . $path . '#?' . $params; | ||
| $url = $config->userFrameworkBaseURL . $path . '?' . $params; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify URL construction change doesn't break CiviCRM Formbuilder
The URL construction was changed from hash fragment (#?) to standard query string (?). According to retrieved learnings, #? is an intentional method for passing values to CiviCRM's Formbuilder.
Please verify that this change doesn't break the form functionality. If the forms require the hash fragment format, consider reverting this change.
🏁 Script executed:
#!/bin/bash
# Search for other instances of URL construction for CiviCRM forms
rg -A 3 -B 3 "userFrameworkBaseURL.*\?" --type php
# Check for any documentation or comments about URL format requirements
rg -i "formbuilder.*url|url.*formbuilder|#\?" --type phpLength of output: 34790
Revert to using hash-fragment (#?) for CiviCRM Formbuilder links
The rest of our custom Form link generators consistently use #? to pass parameters into CiviCRM’s Formbuilder. Switching this one to a standard query (?) will almost certainly break the form loading logic in WordPress.
Please update:
• File:
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php
Line: 167
• Change back from:
$url = $config->userFrameworkBaseURL . $path . '?' . $params;to:
$url = $config->userFrameworkBaseURL . $path . '#?' . $params;This will align it with all other …Links.php classes (e.g. UrbanVisitLinks, EventsLinks, CollectionCampLinks, etc.) and preserve the intended Formbuilder behavior.
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Form/InstitutionDroppingCenterLinks.php
at line 167, revert the URL construction to use a hash-fragment (#?) instead of
a query (?). Change the line from concatenating '?' before $params to
concatenating '#?' to ensure consistency with other Form link generators and
maintain proper Formbuilder functionality in WordPress.
| /** | ||
| * Implements hook_civicrm_alterAngular(). | ||
| */ | ||
| function goonjcustom_civicrm_alterAngular($angular) { | ||
| Civi::log()->debug('[AfformPrefill] alterAngular hook called'); | ||
|
|
||
| $token = \CRM_Utils_Request::retrieve('token', 'String', CRM_Core_DAO::$_nullObject, FALSE, NULL, 'GET'); | ||
| Civi::log()->debug('[AfformPrefill] Token: ' . ($token ? $token : '(none)')); | ||
| if (!$token) { | ||
| Civi::log()->debug('[AfformPrefill] No token, skipping prefill.'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $decrypted = \CRM_Utils_Crypt::decrypt($token); | ||
| Civi::log()->debug('[AfformPrefill] Decrypted: ' . $decrypted); | ||
| $data = json_decode($decrypted, true); | ||
| Civi::log()->debug('[AfformPrefill] Parsed array: ' . print_r($data, 1)); | ||
| } catch (\Exception $e) { | ||
| Civi::log()->error('[AfformPrefill] Token decryption failed: ' . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| if (!is_array($data)) { | ||
| Civi::log()->error('[AfformPrefill] Decrypted data is not an array.'); | ||
| return; | ||
| } | ||
|
|
||
| $prefillData = json_encode($data, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT); | ||
| \Civi::resources()->addScriptFile('goonjcustom', 'js/prefill.js', 10, 'html-header'); | ||
|
|
||
|
|
||
| \Civi::log()->debug('[AfformPrefill] JavaScript prefill payload: ' . $prefillData); | ||
|
|
||
| \Civi::resources()->addScript(" | ||
| (function() { | ||
| if (window.CRM === undefined) window.CRM = {}; | ||
| window.CRM.afformPrefillData = $prefillData; | ||
| console.log('[AfformPrefill] Prefill data available on window.CRM.afformPrefillData:', window.CRM.afformPrefillData); | ||
| })(); | ||
| ", 10, 'html-header'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add token validation and consider splitting responsibilities
The function handles multiple responsibilities and lacks token validation:
- Missing token expiration/integrity checks before decryption
- No validation of expected JSON structure after parsing
- Function combines token processing with resource injection
Consider:
- Add token validation (expiration, signature) before decryption
- Extract token processing into a separate service class
- Validate the decrypted data structure matches expected schema
🧰 Tools
🪛 PHPMD (2.15.0)
39-39: Avoid unused parameters such as '$angular'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/goonjcustom.php around lines 36 to 77,
the goonjcustom_civicrm_alterAngular function lacks token validation, does not
verify the decrypted JSON structure, and mixes token processing with resource
injection. To fix this, first add token validation checks such as expiration and
signature verification before attempting decryption. Then, refactor by
extracting the token processing logic into a separate service class to separate
concerns. Finally, validate that the decrypted data matches the expected JSON
schema before encoding and injecting it into the page.
| /** | ||
| * Implements hook_civicrm_alterAngular(). | ||
| */ | ||
| function goonjcustom_civicrm_alterAngular($angular) { |
There was a problem hiding this comment.
Remove unused parameter $angular
The $angular parameter is not used in the function implementation.
Apply this diff to remove the unused parameter:
-function goonjcustom_civicrm_alterAngular($angular) {
+function goonjcustom_civicrm_alterAngular() {📝 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.
| function goonjcustom_civicrm_alterAngular($angular) { | |
| -function goonjcustom_civicrm_alterAngular($angular) { | |
| +function goonjcustom_civicrm_alterAngular() { |
🧰 Tools
🪛 PHPMD (2.15.0)
39-39: Avoid unused parameters such as '$angular'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/goonjcustom.php at line 39, the
function goonjcustom_civicrm_alterAngular declares a parameter $angular that is
not used anywhere in the function body. Remove the $angular parameter from the
function definition to clean up the code and avoid unused parameter warnings.
| const data = window.CRM?.afformPrefillData; | ||
| if (!data) return console.warn('[AfformPrefill] No afformPrefillData found ❗'); | ||
|
|
There was a problem hiding this comment.
Add validation for prefill data structure
The code doesn't validate the structure of afformPrefillData before using it, which could lead to runtime errors.
Add validation to ensure data integrity:
const data = window.CRM?.afformPrefillData;
if (!data) return console.warn('[AfformPrefill] No afformPrefillData found ❗');
+
+ // Validate data is an object
+ if (typeof data !== 'object' || Array.isArray(data)) {
+ return console.error('[AfformPrefill] Invalid data format');
+ }📝 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.
| const data = window.CRM?.afformPrefillData; | |
| if (!data) return console.warn('[AfformPrefill] No afformPrefillData found ❗'); | |
| const data = window.CRM?.afformPrefillData; | |
| if (!data) return console.warn('[AfformPrefill] No afformPrefillData found ❗'); | |
| // Validate data is an object | |
| if (typeof data !== 'object' || Array.isArray(data)) { | |
| return console.error('[AfformPrefill] Invalid data format'); | |
| } |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/js/prefill.js around lines 53 to 55,
the code checks for the existence of afformPrefillData but does not validate its
structure, risking runtime errors. Add validation to confirm that
afformPrefillData is an object and contains the expected properties before
proceeding. If the validation fails, log a warning and exit early to prevent
errors.
| }); | ||
|
|
||
| scope._afformPrefilled = true; | ||
| try { scope.$apply(); $rootScope.$apply(); } catch (_) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid empty catch blocks
Empty catch blocks hide errors and make debugging difficult.
Log errors instead of silently ignoring them:
- try { scope.$apply(); $rootScope.$apply(); } catch (_) {}
+ try {
+ scope.$apply();
+ $rootScope.$apply();
+ } catch (err) {
+ console.warn('[AfformPrefill] Digest cycle error:', err.message);
+ }📝 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.
| try { scope.$apply(); $rootScope.$apply(); } catch (_) {} | |
| try { | |
| scope.$apply(); | |
| $rootScope.$apply(); | |
| } catch (err) { | |
| console.warn('[AfformPrefill] Digest cycle error:', err.message); | |
| } |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/js/prefill.js at line 156, the catch
block is empty, which hides errors and complicates debugging. Modify the catch
block to log the caught error instead of ignoring it silently. Use console.error
or an appropriate logging method to output the error details for better
visibility during debugging.
| try { scope.$apply(); $rootScope.$apply(); } catch (_) {} | ||
|
|
||
| console.log('[AfformPrefill]Autofill completed!'); | ||
| }, 1000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce timeout delay for better user experience
The 1-second delay before prefilling might cause a noticeable lag for users.
Consider reducing the timeout or making it configurable:
- }, 1000);
+ }, 100); // Reduced from 1000ms to 100ms📝 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.
| }, 1000); | |
| }, 100); // Reduced from 1000ms to 100ms |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/js/prefill.js at line 159, the current
timeout delay of 1000 milliseconds before prefilling causes a noticeable lag.
Reduce this timeout to a shorter duration, such as 300-500 milliseconds, or
refactor the code to make the delay configurable via a parameter or setting to
improve user experience.
1 participant
Below is my requirement along with the research part:
https://github.com/coloredcow-admin/goonj-crm/issues/197
Summary by CodeRabbit
New Features
Refactor