Open
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 139 files out of 297 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe PR adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Page Request
participant Nav as NavigationPermissionService
participant API as CRM API4
participant S3 as S3 Storage
participant JS as JavaScript Engine
Page->>Nav: pageRun event (testing handler)
alt CRM_Core_Page_File
Nav->>Nav: Extract file URL parameter
Nav->>S3: Construct & fetch file
S3-->>Nav: File content
Nav->>Page: Output with headers + exit
else CRM_Contact_Page_View_Summary
Nav->>API: Request contact QR code file
API-->>Nav: QR code file reference
Nav->>S3: Construct S3 URL
S3-->>Nav: S3 URL confirmation
Nav->>Nav: Assign to custom token & fileUrl variable
Nav->>JS: Inject inline rendering script
JS->>Page: Render QR code image
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
![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
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/NavigationPermissionService.php (2)
22-23: Event handler nametestingis unclear and the handler mixes concernsFor a long‑lived
pageRunsubscriber,testingis not self‑descriptive and the method currently handles two unrelated responsibilities (file download override and contact QR rendering). This hurts readability and violates single‑responsibility.Consider:
- Renaming the handler to something intention‑revealing (e.g.
routeFilesToS3/handleS3Overrides), and- Splitting the logic into two private methods (e.g.
handleFileDownloadFromS3()andinjectContactQrFromS3()) that the event handler calls.
52-55: Hard‑coded and duplicated S3 URL construction reduces maintainabilityThe S3 base URL and URL‑building logic are duplicated:
- Lines 52‑55 and 99‑100 both hard‑code
https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/and concatenateuri.- If the bucket, region, or path prefix changes, you’ll need to hunt down multiple occurrences.
For maintainability and readability, consider:
- Defining a class constant or config‑driven setting for the S3 base (e.g.
const S3_BASE = 'https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/';), and- A small helper like
buildS3Url(string $uri): stringthat encapsulates any required path normalization.This keeps the logic DRY without over‑engineering and makes future S3 moves or environment changes safer. Based on learnings, this is more important here than in one‑time CLI scripts because this handler runs in regular UI traffic.
Also applies to: 99-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/NavigationPermissionService.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tarunnjoshi
Repo: ColoredCow/goonj PR: 1230
File: wp-content/civi-extensions/civirazorpay/cli/update-recurring-contact-info.php:115-117
Timestamp: 2025-05-27T07:31:44.131Z
Learning: For one-time scripts in the Goonj project, tarunnjoshi prefers to keep hardcoded values rather than creating shared constants or configuration files, prioritizing simplicity over strict adherence to DRY principle to avoid over-engineering temporary code.
Comment on lines
+30
to
+67
| public function testing(&$page) { | ||
| error_log('Page class: ' . get_class($page)); | ||
|
|
||
| /* | ||
| * ---------------------------------------------------- | ||
| * 1️⃣ SEARCHKIT + FILE FIELDS (File Handler Override) | ||
| * ---------------------------------------------------- | ||
| */ | ||
| if (get_class($page) === 'CRM_Core_Page_File') { | ||
| error_log("File handler override triggered"); | ||
|
|
||
| $fileId = \CRM_Utils_Request::retrieve('id', 'Integer'); | ||
| error_log("Retrieved fileId from URL: " . print_r($fileId, true)); | ||
| if (!$fileId) return; | ||
|
|
||
| try { | ||
| $file = civicrm_api3('File', 'getsingle', ['id' => $fileId]); | ||
| } catch (\Exception $e) { | ||
| error_log("Error fetching civicrm_file: " . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| $filename = $file['uri']; | ||
| $mime = $file['mime_type']; | ||
| $s3Url = "https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/" . $filename; | ||
| error_log("S3 URL built: " . $s3Url); | ||
|
|
||
| $image = @file_get_contents($s3Url); | ||
| if ($image === FALSE) { | ||
| error_log("S3 file not found at URL: " . $s3Url); | ||
| return; | ||
| } | ||
|
|
||
| header("Content-Type: {$mime}"); | ||
| header("Content-Length: " . strlen($image)); | ||
| echo $image; | ||
| \CRM_Utils_System::civiExit(); | ||
| } |
There was a problem hiding this comment.
S3 file download override lacks explicit permission checks and uses brittle patterns
In the CRM_Core_Page_File branch you pull a file by ID and stream it from S3 directly:
- There is no explicit ACL/permission check before serving the file. Anyone who can hit
CRM_Core_Page_Filewith a valididwill get the file contents from S3, regardless of entity‑level access rules. For attachments or sensitive files this is a security risk. - Using
get_class($page) === 'CRM_Core_Page_File'is brittle if Civi ever subclasses this page;instanceofor$page->getVar('_name')(as used elsewhere in this class) is more robust. - Suppressing errors with
@file_get_contentshides useful diagnostics and can make production issues harder to debug.
Suggestions:
- Before streaming from S3, enforce the same permission model Civi uses for that file (e.g. via the related entity/ACLs or by delegating to existing helpers) so that this override does not weaken access control.
- Prefer
instanceof CRM_Core_Page_Fileor$page->getVar('_name')for page detection. - Drop the error suppression and rely on your explicit
FALSEcheck and logging.
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/NavigationPermissionService.php
around lines 30–67, replace the brittle page check and silent file fetch with a
secure, robust flow: use "if ($page instanceof CRM_Core_Page_File)" (or
$page->getVar('_name')) instead of get_class(...), remove the "@" error
suppression on file_get_contents and log any failure, and before streaming the
S3 object enforce Civi's access model by resolving the File's related entity
(via the File API/BAO) and verifying the current user has permission to view
that entity/file (use existing Civi permission helpers / ACL checks or delegate
to the standard file download helper); only stream the S3 contents after the
permission check passes and return/log and exit on denial or fetch errors.
Comment on lines
+69
to
+131
| if (get_class($page) === 'CRM_Contact_Page_View_Summary') { | ||
| error_log("Contact Summary page detected"); | ||
|
|
||
| $contactId = $page->getVar('_contactId'); | ||
| error_log("Contact ID: " . print_r($contactId, true)); | ||
| if (!$contactId) return; | ||
|
|
||
| // Get custom QR code file ID | ||
| try { | ||
| $contacts = \Civi\Api4\Contact::get(TRUE) | ||
| ->addSelect('Contact_QR_Code.QR_Code') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute() | ||
| ->first(); | ||
| } catch (\Exception $e) { | ||
| error_log("Error fetching contact QR code: " . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| $fileId = $contacts['Contact_QR_Code.QR_Code']; | ||
| if (!$fileId) return; | ||
|
|
||
| // Load civicrm_file record | ||
| try { | ||
| $file = civicrm_api3('File', 'getsingle', ['id' => $fileId]); | ||
| } catch (\Exception $e) { | ||
| error_log("Error fetching civicrm_file: " . $e->getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| $s3Url = "https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/" . $file['uri']; | ||
| error_log("S3 URL for contact QR code: " . $s3Url); | ||
|
|
||
| // Assign S3 URL to custom field token and fileUrl | ||
| $customFields = \Civi\Api4\CustomField::get(TRUE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Contact_QR_Code') | ||
| ->addWhere('name', '=', 'QR_Code') | ||
| ->setLimit(1) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| $tokenName = 'custom_' . $customFields['id']; | ||
| $page->assign($tokenName, $s3Url); | ||
| $page->assign("fileUrl_{$fileId}", $s3Url); | ||
| error_log("Custom field image assigned successfully to token and fileUrl"); | ||
|
|
||
| // ---------------------------- | ||
| // Inject JS safely via CRM_Core_Resources | ||
| // ---------------------------- | ||
| $js = <<<JS | ||
| document.addEventListener('DOMContentLoaded', function() { | ||
| var qrDiv = document.querySelector('#custom-set-content-3 .crm-content.crm-custom-data'); | ||
| if(qrDiv && !qrDiv.querySelector('img')) { | ||
| qrDiv.innerHTML = '<img src="{$s3Url}" alt="QR Code" style="max-width:150px;height:auto;">'; | ||
| console.log('QR code injected for contact {$contactId}'); | ||
| } | ||
| }); | ||
| JS; | ||
|
|
||
| \CRM_Core_Resources::singleton()->addScript($js, 0, 'inline'); | ||
| error_log("JS injected via CRM_Core_Resources for inline rendering"); | ||
| } |
There was a problem hiding this comment.
Contact summary S3 QR rendering needs stronger error handling and is DOM‑fragile
In the CRM_Contact_Page_View_Summary branch:
$contacts = ...->first();is assumed to return an array withContact_QR_Code.QR_Code. If the custom group/field is disabled, renamed, or the API returns no rows,$contactsmay beNULLor missing that key, leading to notices or errors.\Civi\Api4\CustomField::get(TRUE)...->first();is not wrapped in atry/catch, so any API failure will bubble up as a fatal and break the contact summary page.- The JS uses a hard‑coded selector
'#custom-set-content-3 .crm-content.crm-custom-data'and replacesinnerHTML. This is brittle to layout/order changes and will wipe any other content in that container, which can surprise future maintainers.
Consider:
- $contacts = \Civi\Api4\Contact::get(TRUE)
+ $contacts = \Civi\Api4\Contact::get(TRUE)
->addSelect('Contact_QR_Code.QR_Code')
->addWhere('id', '=', $contactId)
->execute()
->first();
- $fileId = $contacts['Contact_QR_Code.QR_Code'];
- if (!$fileId) return;
+ if (empty($contacts) || empty($contacts['Contact_QR_Code.QR_Code'])) {
+ return;
+ }
+ $fileId = $contacts['Contact_QR_Code.QR_Code'];and wrapping the CustomField::get chain in a try/catch similar to the other API calls.
For the JS, appending an <img> element instead of replacing innerHTML, and targeting a more stable hook (e.g. a dedicated wrapper id/class for the QR field) would make this less fragile.
📝 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 (get_class($page) === 'CRM_Contact_Page_View_Summary') { | |
| error_log("Contact Summary page detected"); | |
| $contactId = $page->getVar('_contactId'); | |
| error_log("Contact ID: " . print_r($contactId, true)); | |
| if (!$contactId) return; | |
| // Get custom QR code file ID | |
| try { | |
| $contacts = \Civi\Api4\Contact::get(TRUE) | |
| ->addSelect('Contact_QR_Code.QR_Code') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute() | |
| ->first(); | |
| } catch (\Exception $e) { | |
| error_log("Error fetching contact QR code: " . $e->getMessage()); | |
| return; | |
| } | |
| $fileId = $contacts['Contact_QR_Code.QR_Code']; | |
| if (!$fileId) return; | |
| // Load civicrm_file record | |
| try { | |
| $file = civicrm_api3('File', 'getsingle', ['id' => $fileId]); | |
| } catch (\Exception $e) { | |
| error_log("Error fetching civicrm_file: " . $e->getMessage()); | |
| return; | |
| } | |
| $s3Url = "https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/" . $file['uri']; | |
| error_log("S3 URL for contact QR code: " . $s3Url); | |
| // Assign S3 URL to custom field token and fileUrl | |
| $customFields = \Civi\Api4\CustomField::get(TRUE) | |
| ->addSelect('id') | |
| ->addWhere('custom_group_id:name', '=', 'Contact_QR_Code') | |
| ->addWhere('name', '=', 'QR_Code') | |
| ->setLimit(1) | |
| ->execute() | |
| ->first(); | |
| $tokenName = 'custom_' . $customFields['id']; | |
| $page->assign($tokenName, $s3Url); | |
| $page->assign("fileUrl_{$fileId}", $s3Url); | |
| error_log("Custom field image assigned successfully to token and fileUrl"); | |
| // ---------------------------- | |
| // Inject JS safely via CRM_Core_Resources | |
| // ---------------------------- | |
| $js = <<<JS | |
| document.addEventListener('DOMContentLoaded', function() { | |
| var qrDiv = document.querySelector('#custom-set-content-3 .crm-content.crm-custom-data'); | |
| if(qrDiv && !qrDiv.querySelector('img')) { | |
| qrDiv.innerHTML = '<img src="{$s3Url}" alt="QR Code" style="max-width:150px;height:auto;">'; | |
| console.log('QR code injected for contact {$contactId}'); | |
| } | |
| }); | |
| JS; | |
| \CRM_Core_Resources::singleton()->addScript($js, 0, 'inline'); | |
| error_log("JS injected via CRM_Core_Resources for inline rendering"); | |
| } | |
| if (get_class($page) === 'CRM_Contact_Page_View_Summary') { | |
| error_log("Contact Summary page detected"); | |
| $contactId = $page->getVar('_contactId'); | |
| error_log("Contact ID: " . print_r($contactId, true)); | |
| if (!$contactId) return; | |
| // Get custom QR code file ID | |
| try { | |
| $contacts = \Civi\Api4\Contact::get(TRUE) | |
| ->addSelect('Contact_QR_Code.QR_Code') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute() | |
| ->first(); | |
| } catch (\Exception $e) { | |
| error_log("Error fetching contact QR code: " . $e->getMessage()); | |
| return; | |
| } | |
| if (empty($contacts) || empty($contacts['Contact_QR_Code.QR_Code'])) { | |
| return; | |
| } | |
| $fileId = $contacts['Contact_QR_Code.QR_Code']; | |
| // Load civicrm_file record | |
| try { | |
| $file = civicrm_api3('File', 'getsingle', ['id' => $fileId]); | |
| } catch (\Exception $e) { | |
| error_log("Error fetching civicrm_file: " . $e->getMessage()); | |
| return; | |
| } | |
| $s3Url = "https://goonj-uploads-items.s3.ap-south-1.amazonaws.com/custom/" . $file['uri']; | |
| error_log("S3 URL for contact QR code: " . $s3Url); | |
| // Assign S3 URL to custom field token and fileUrl | |
| $customFields = \Civi\Api4\CustomField::get(TRUE) | |
| ->addSelect('id') | |
| ->addWhere('custom_group_id:name', '=', 'Contact_QR_Code') | |
| ->addWhere('name', '=', 'QR_Code') | |
| ->setLimit(1) | |
| ->execute() | |
| ->first(); | |
| $tokenName = 'custom_' . $customFields['id']; | |
| $page->assign($tokenName, $s3Url); | |
| $page->assign("fileUrl_{$fileId}", $s3Url); | |
| error_log("Custom field image assigned successfully to token and fileUrl"); | |
| // ---------------------------- | |
| // Inject JS safely via CRM_Core_Resources | |
| // ---------------------------- | |
| $js = <<<JS | |
| document.addEventListener('DOMContentLoaded', function() { | |
| var qrDiv = document.querySelector('#custom-set-content-3 .crm-content.crm-custom-data'); | |
| if(qrDiv && !qrDiv.querySelector('img')) { | |
| qrDiv.innerHTML = '<img src="{$s3Url}" alt="QR Code" style="max-width:150px;height:auto;">'; | |
| console.log('QR code injected for contact {$contactId}'); | |
| } | |
| }); | |
| JS; | |
| \CRM_Core_Resources::singleton()->addScript($js, 0, 'inline'); | |
| error_log("JS injected via CRM_Core_Resources for inline rendering"); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move items to s3
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.