Conversation
…bility - Replace match expression with switch statement in runVersionUpgradeTask() - Ensures compatibility with PHP 7.4 and earlier versions - Maintains same functionality
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a new version of the plugin, focusing on enhancing the ActiveCampaign integration with tag update capabilities and refining its contact management. It also includes critical bug fixes for several integrations, such as SendFox, WooCommerce, Salesforce, and Google Products, ensuring more robust and reliable data synchronization. Furthermore, the PR improves the handling of OAuth routes by adjusting sanitization practices and updates core dependencies to maintain broader compatibility. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a version bump to 2.7.12, accompanied by significant refactoring, bug fixes, and new features, including a more robust data sanitization method in the routing utility and performance enhancements. However, a critical security vulnerability was identified in the core routing logic: the AJAX handlers lack proper capability-based authorization checks, potentially allowing any logged-in user to perform administrative actions. It is strongly recommended to enforce access control using current_user_can() in the routing layer or within all sensitive controller methods. Additionally, there's a critical bug in the SendFox integration layout component that could cause a runtime error, which needs to be addressed.
| const setMainAction = (value) => { | ||
| setSendFoxConf(prev => create(prev, draft => { | ||
| draft[field] = value | ||
|
|
||
| if (value === '2') { | ||
| draft.field_map = generateMappedField(newConf) | ||
| } else if (value === '1') { | ||
| draft.field_map_list = generateListMappedField(newConf) | ||
| } else if (value === '3') { | ||
| draft.field_map_unsubscribe = generateunsubscribeMappedField(newConf) | ||
| } | ||
| })) | ||
| } |
There was a problem hiding this comment.
The variables field and newConf are not defined within the scope of the setMainAction function. It seems field should be the string 'mainAction', and newConf should be sendFoxConf. Additionally, the select element for choosing an action is missing its onChange handler, which should call this setMainAction function.
| const setMainAction = (value) => { | |
| setSendFoxConf(prev => create(prev, draft => { | |
| draft[field] = value | |
| if (value === '2') { | |
| draft.field_map = generateMappedField(newConf) | |
| } else if (value === '1') { | |
| draft.field_map_list = generateListMappedField(newConf) | |
| } else if (value === '3') { | |
| draft.field_map_unsubscribe = generateunsubscribeMappedField(newConf) | |
| } | |
| })) | |
| } | |
| const setMainAction = (value) => { | |
| setSendFoxConf(prev => create(prev, draft => { | |
| draft.mainAction = value | |
| if (value === '2') { | |
| draft.field_map = generateMappedField(sendFoxConf) | |
| } else if (value === '1') { | |
| draft.field_map_list = generateListMappedField(sendFoxConf) | |
| } else if (value === '3') { | |
| draft.field_map_unsubscribe = generateunsubscribeMappedField(sendFoxConf) | |
| } | |
| })) | |
| } |
| $data = $noSanitize | ||
| ? $decoded | ||
| : (\is_object($decoded) || \is_array($decoded) ? map_deep($decoded, 'sanitize_text_field') : $decoded); | ||
| : (\is_object($decoded) || \is_array($decoded) ? map_deep($decoded, [__CLASS__, 'sanitizeValue']) : $decoded); |
There was a problem hiding this comment.
The Route::action() method, which serves as the central handler for all plugin AJAX requests, lacks a proper authorization check (e.g., current_user_can()). While the method verifies a nonce, WordPress nonces are not intended for access control and can be generated by any authenticated user. This flaw allows low-privileged users (such as Subscribers) to execute administrative functions registered by the plugin, including generating OAuth tokens and fetching sensitive integration data.
Remediation:
Implement a capability check at the beginning of the action() method or within the individual controller methods to ensure that only users with the appropriate permissions (e.g., manage_options or the plugin's custom manage_integrations capability) can access these endpoints.
|
|
||
| private static function getResponseType() | ||
| { | ||
| return strpos(HttpHelper::$responseCode, 20) === 0 ? 'success' : 'error'; |
There was a problem hiding this comment.
The strpos function expects string arguments, but HttpHelper::$responseCode can be an integer, and 20 is an integer literal. This can lead to unexpected behavior or errors in different PHP versions. It's safer to cast the response code to a string and use a string for the needle.
return strpos((string) HttpHelper::$responseCode, '20') === 0 ? 'success' : 'error';There was a problem hiding this comment.
Pull request overview
This PR bumps the plugin version to v2.7.12 and updates both backend and frontend pieces related to the release, including changelog entries, ActiveCampaign enhancements, SendFox UI refactors, and request sanitization adjustments for OAuth/token flows.
Changes:
- Bump plugin version to 2.7.12 and add the 2.7.12 changelog entries.
- Add ActiveCampaign tag update option and refactor ActiveCampaign contact update handling.
- Refactor parts of SendFox field mapping UI and adjust backend request sanitization (type-aware sanitizer +
no_sanitizeon multiple token routes).
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| readme.txt | Updates stable tag and adds 2.7.12 changelog. |
| languages/generatedString.php | Adds new i18n strings for ActiveCampaign tag update + minor ordering shift. |
| frontend/src/resource/sass/app.scss | Comments out .bit-logo background styles. |
| frontend/src/pages/Welcome.jsx | Adds FlowBuilder preloading on create-integration intent events. |
| frontend/src/pages/ChangelogToggle.jsx | Updates release date and changelog items for 2.7.12. |
| frontend/src/pages/AllIntegrations.jsx | Adds FlowBuilder preloading on create-integration intent events. |
| frontend/src/components/AllIntegrations/SendFox/SendFoxUnsubscribeFieldMap.jsx | Refactors unsubscribe field list handling and exports default fields. |
| frontend/src/components/AllIntegrations/SendFox/SendFoxListFieldMap.jsx | Refactors list field list handling and exports default fields. |
| frontend/src/components/AllIntegrations/SendFox/SendFoxIntegLayout.jsx | Refactors action options and introduces mutative draft update helper. |
| frontend/src/components/AllIntegrations/SendFox/SendFoxFieldMap.jsx | Refactors contact field list handling and exports default fields. |
| frontend/src/components/AllIntegrations/SendFox/SendFoxCommonFunc.js | Moves required field generation to use exported field definitions. |
| frontend/src/components/AllIntegrations/SendFox/SendFox.jsx | Removes actions/fields constants from config state. |
| frontend/src/components/AllIntegrations/ActiveCampaign/ActiveCampaignActions.jsx | Adds tag-update toggle and generalizes action handler. |
| composer.lock | Updates locked dependency versions and composer plugin API version. |
| bitwpfi.php | Bumps plugin header version and BTCBI_VERSION. |
| backend/Triggers/WC/WCHelper.php | Filters empty values before merging order/ACF/checkout/extra data. |
| backend/Core/Util/Route.php | Uses type-aware sanitizer to preserve JSON booleans/numbers. |
| backend/Core/Util/HttpHelper.php | Adds delete() request wrapper. |
| backend/Core/Util/Activation.php | Replaces match with switch for upgrade task selection. |
| backend/Config.php | Bumps Config::VERSION to 2.7.12. |
| backend/Actions/ZoomWebinar/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/Zoom/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoSheet/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoRecruit/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoMarketingHub/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoDesk/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoCreator/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoCampaigns/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoCRM/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoBigin/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ZohoAnalytics/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/Salesforce/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/Salesforce/RecordApiHelper.php | Refactors response-type detection logic for lead/contact actions. |
| backend/Actions/PCloud/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/OneDrive/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/Notion/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/Mautic/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/Mailup/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/MailChimp/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/LionDesk/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/Keap/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/GoogleSheet/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/GoogleDrive/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/GoogleContacts/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/GoogleCalendar/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/GoogleCalendar/GoogleCalendarController.php | Minor formatting/spacing change. |
| backend/Actions/Dropbox/Routes.php | Marks authorization route as no_sanitize(). |
| backend/Actions/ConstantContact/Routes.php | Marks generate-token route as no_sanitize(). |
| backend/Actions/ActiveCampaign/RecordApiHelper.php | Refactors field mapping/update logic and adds tag update behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { contactFields } from './SendFoxFieldMap' | ||
| import { listFields } from './SendFoxListFieldMap' | ||
| import { unsubscribeFields } from './SendFoxUnsubscribeFieldMap' |
There was a problem hiding this comment.
SendFoxCommonFunc now imports contactFields/listFields/unsubscribeFields from the FieldMap components, but those components also import functions from SendFoxCommonFunc, creating a circular module dependency. This can lead to undefined exports at runtime depending on evaluation order. Move the shared field definitions into a separate constants module (or keep them in SendFoxCommonFunc and have components import from there) to break the cycle.
| const setMainAction = (value) => { | ||
| setSendFoxConf(prev => create(prev, draft => { | ||
| draft[field] = value | ||
|
|
||
| if (value === '2') { | ||
| draft.field_map = generateMappedField(newConf) | ||
| } else if (value === '1') { | ||
| draft.field_map_list = generateListMappedField(newConf) | ||
| } else if (value === '3') { | ||
| draft.field_map_unsubscribe = generateunsubscribeMappedField(newConf) | ||
| } |
There was a problem hiding this comment.
setMainAction references undeclared identifiers (field and newConf) and create from mutative is only used inside this broken helper. If this handler gets wired up later it will throw at runtime; if it’s not needed, it should be removed along with the unused import. If it is needed, update it to set draft.mainAction (or the intended property) and use the current config when regenerating the appropriate field_map*.
| private static function getResponseType() | ||
| { | ||
| return strpos(HttpHelper::$responseCode, 20) === 0 ? 'success' : 'error'; | ||
| } |
There was a problem hiding this comment.
getResponseType() is using strpos(HttpHelper::$responseCode, 20) which will misclassify 2xx responses like 204 (it doesn’t start with "20") and also relies on implicit int→string conversion. Cast the response code to string and check the first digit (or use an integer range check like 200–299) to reliably detect success.
| if (!empty($tags)) { | ||
| if ($integrationDetails->actions->tagUpdate) { | ||
| $result['tags_removed'] = HttpHelper::delete("{$this->_apiEndpoint}/contactTags/{$contactId}", null, $this->_defaultHeader); | ||
| } |
There was a problem hiding this comment.
$integrationDetails->actions->tagUpdate is accessed without guarding that actions exists, which can trigger warnings/notices (and in newer PHP versions, runtime warnings) when actions are missing. Use a safe check like !empty($integrationDetails->actions->tagUpdate) (or pass/use the $actions argument already available in execute()) before attempting tag removal.
| label: 'Brekadance', | ||
| desc: 'Fixed trigger listening data issue.', | ||
| isPro: true |
There was a problem hiding this comment.
The integration name is misspelled as "Brekadance"; elsewhere in the codebase it appears as "Breakdance". Please correct this label to avoid inconsistent naming in the UI/changelog.
| - Brekadance: Fixed trigger listening data issue (Pro). | ||
|
|
There was a problem hiding this comment.
Changelog entry misspells the integration as "Brekadance"; it should be "Breakdance" for consistency with the product/integration name used elsewhere.
No description provided.