feature/secure-mode-support #39
Conversation
WalkthroughThe changes in this pull request encompass updates across multiple files to enhance the functionality and security of the Tawk.to plugin. Key modifications include the addition of a root property in the ESLint configuration, new visitor recognition features in JavaScript, the introduction of a JavaScript API key in the configuration, and significant updates to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)
🪧 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: 5
🧹 Outside diff range and nitpick comments (6)
tawkto/includes/default_config.php (1)
19-19: Consider separating security configurations from visibility settingsThe
js_api_keyis placed within thevisibilityarray, but it's a security credential rather than a visibility setting. Consider creating a separatesecurityorauthenticationconfiguration section to better organize and maintain security-related settings.return array( 'visibility' => array( 'always_display' => 1, // ... other visibility settings ... - 'js_api_key' => '', ), + 'security' => array( + 'js_api_key' => '', + ), );tawkto/assets/js/tawk.admin.js (3)
76-82: Consider clearing the API key field when disabledWhen visitor recognition is disabled, consider clearing the API key field value in addition to disabling it. This prevents any stored API key from being visible in the DOM when the feature is disabled.
if ( jQuery( '#enable-visitor-recognition' ).prop( 'checked' ) ) { jQuery( '.tawk-selected-visitor' ).show(); jQuery( '#js-api-key' ).prop( 'disabled', false ); } else { jQuery( '.tawk-selected-visitor' ).hide(); jQuery( '#js-api-key' ).prop( 'disabled', true ); + jQuery( '#js-api-key' ).val(''); }
84-94: Enhance change handler implementationTwo suggestions for improvement:
- Clear the API key field when disabling visitor recognition
- Add a consistent animation duration for better UX alignment with other animations in the file
jQuery( '#enable-visitor-recognition' ).change( function() { if ( this.checked ) { - jQuery( '.tawk-selected-visitor' ).fadeIn(); + jQuery( '.tawk-selected-visitor' ).fadeIn(400); jQuery( '#js-api-key' ).prop( 'disabled', false ); } else { - jQuery( '.tawk-selected-visitor' ).fadeOut(); + jQuery( '.tawk-selected-visitor' ).fadeOut(400); jQuery( '#js-api-key' ).prop( 'disabled', true ); + jQuery( '#js-api-key' ).val(''); } } );
75-94: Consider additional security measures for API key handlingSince this code handles sensitive API key information, consider implementing these security measures:
- Add input validation for the API key format
- Consider implementing a secure way to test the API key validity
- Ensure the API key is properly sanitized before being saved
Would you like assistance in implementing any of these security measures?
tawkto/templates/settings.php (1)
416-423: Enhance the notice with additional context and documentationConsider adding a link to the Tawk.to documentation about Secure Mode and its benefits to help users make informed decisions.
<p class='tawk-notice'> <?php esc_html_e( 'Note: If ', 'tawk-to-live-chat' ); ?> <b><?php esc_html_e( 'Secure Mode', 'tawk-to-live-chat' ); ?></b> <?php esc_html_e( ' is enabled on your property, please enter your ', 'tawk-to-live-chat' ); ?> <b><?php esc_html_e( 'Javascript API Key', 'tawk-to-live-chat' ); ?></b> <?php esc_html_e( ' for visitor recognition to work correctly.', 'tawk-to-live-chat' ); ?> + <br/> + <a href="https://www.tawk.to/knowledgebase/secure-mode" target="_blank" rel="noopener noreferrer"> + <?php esc_html_e( 'Learn more about Secure Mode', 'tawk-to-live-chat' ); ?> + </a> </p>tawkto/tawkto.php (1)
30-31: Useopenssl_cipher_iv_length()to determine IV length instead of hardcodingTo ensure compatibility with the specified cipher and improve maintainability, consider using
openssl_cipher_iv_length(self::CIPHER)to dynamically determine the IV length instead of hardcodingCIPHER_IV_LENGTHas16. This approach ensures that the IV length always matches the requirements of the cipher algorithm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.eslintrc.js(1 hunks)tawkto/assets/js/tawk.admin.js(1 hunks)tawkto/includes/default_config.php(1 hunks)tawkto/tawkto.php(5 hunks)tawkto/templates/settings.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .eslintrc.js
🔇 Additional comments (2)
tawkto/includes/default_config.php (1)
19-19: Verify secure handling of the API key
Since this is a sensitive credential being introduced as part of secure mode support, let's verify the implementation of proper security measures in the related files.
✅ Verification successful
Let me verify the encryption functions to ensure complete security. I'll check the implementation of encryption/decryption methods.
Let me try a different approach to check the encryption methods and frontend usage.
Let me check one final aspect about the cipher configuration and usage.
API key implementation follows security best practices
The implementation shows proper security measures:
- The API key is encrypted using AES-256-CBC with a random IV before storage
- Encryption uses WordPress's SECURE_AUTH_KEY constant
- The key is properly escaped when output in forms
- No direct exposure of the key in JavaScript
- Key is only used server-side for HMAC generation
- Decryption is properly handled with base64 decode and IV separation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper encryption and secure handling of js_api_key
# Check if the key is properly encrypted when stored
rg -A 5 "js_api_key" --type php
# Look for encryption-related functions handling this key
ast-grep --pattern 'function $_($$js_api_key) {
$$$
}'
# Check for any direct/unencrypted usage of the key
rg -i "js_api_key.*=.*['\"]\w+['\"]" --type php
Length of output: 2304
Script:
#!/bin/bash
# Check the implementation of encryption/decryption methods
ast-grep --pattern 'function get_encrypted_data($_) {
$$$
}'
ast-grep --pattern 'function get_decrypted_data($_) {
$$$
}'
# Check how the key is used in frontend/output
rg -A 3 "wp_localize_script.*js_api" --type php
Length of output: 179
Script:
#!/bin/bash
# Search for encryption-related functions in the class
rg -A 10 "function get_(en|de)crypted_data" --type php
# Check how the API key is passed to frontend
rg -A 5 "wp_localize_script|wp_add_inline_script" --type php
# Look for any direct output of the key
rg "echo.*js_api" --type php
Length of output: 1757
Script:
#!/bin/bash
# Check cipher configuration
rg -A 2 "CIPHER.*=" --type php
# Check if the key is used in any JavaScript
rg -g "*.js" "js_api|api_key"
# Check for any WordPress hooks/filters related to the key
rg "add_filter|apply_filters.*js_api" --type php
Length of output: 394
tawkto/templates/settings.php (1)
415-438: Add API key validation and test functionality
Consider adding immediate validation feedback and a test connection feature to help users verify their configuration.
<div class="tawk-selected-visitor">
<!-- ... existing code ... -->
<table class="form-table">
<tr valign="top">
<th class="tawk-setting" scope="row">
<?php esc_html_e( 'Javascript API Key', 'tawk-to-live-chat' ); ?>
</th>
<td>
<input type="password"
id="js-api-key"
name="tawkto-visibility-options[js_api_key]"
value="<?php echo esc_attr( $visibility['js_api_key'] ); ?>" />
+ <button type="button"
+ class="button"
+ id="test-api-key"
+ onclick="testApiKey()"
+ <?php echo empty($visibility['js_api_key']) ? 'disabled' : ''; ?>>
+ <?php esc_html_e( 'Test Connection', 'tawk-to-live-chat' ); ?>
+ </button>
+ <p class="description" id="js-api-key-status"></p>
</td>
</tr>
</table>
</div>Add this JavaScript to handle the test functionality:
function testApiKey() {
const apiKey = document.getElementById('js-api-key').value;
const statusEl = document.getElementById('js-api-key-status');
// Show loading state
statusEl.textContent = 'Testing connection...';
// Make an AJAX call to test the API key
// This requires implementing an endpoint in the plugin's PHP code
jQuery.post(ajaxurl, {
action: 'test_tawkto_api_key',
api_key: apiKey,
nonce: tawktoSettings.nonce
})
.done(function(response) {
if (response.success) {
statusEl.style.color = '#00a32a';
statusEl.textContent = 'Connection successful!';
} else {
statusEl.style.color = '#d63638';
statusEl.textContent = response.data.message || 'Connection failed!';
}
})
.fail(function() {
statusEl.style.color = '#d63638';
statusEl.textContent = 'Connection test failed. Please try again.';
});
}
// Enable/disable test button based on API key input
document.getElementById('js-api-key').addEventListener('input', function() {
document.getElementById('test-api-key').disabled = !this.value;
});
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tawkto/tawkto.php (1)
376-403: Improve error handling in encryption method.While the method handles basic error cases, it could be enhanced:
- Add logging for encryption failures
- Consider throwing exceptions instead of returning empty strings
private static function get_encrypted_data( $data ) { try { $iv = random_bytes( self::CIPHER_IV_LENGTH ); } catch ( Exception $e ) { + error_log( 'Failed to generate IV: ' . $e->getMessage() ); return ''; } $encrypted_data = openssl_encrypt( $data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); if ( false === $encrypted_data ) { + error_log( 'Failed to encrypt data with OpenSSL' ); return ''; } // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode $encrypted_data = base64_encode( $iv . $encrypted_data ); if ( false === $encrypted_data ) { + error_log( 'Failed to base64 encode encrypted data' ); return ''; } return $encrypted_data; }tawkto/templates/settings.php (2)
418-422: Typographical correction: 'Javascript' should be 'JavaScript'In the notice text, "Javascript API Key" should be spelled "JavaScript API Key" to reflect the correct spelling of "JavaScript".
428-428: Typographical correction: 'Javascript' should be 'JavaScript'In the label, "Javascript API Key" should be updated to "JavaScript API Key" for accurate spelling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tawkto/tawkto.php(5 hunks)tawkto/templates/settings.php(1 hunks)
🔇 Additional comments (8)
tawkto/tawkto.php (5)
30-31: LGTM! Encryption configuration looks secure.
The chosen cipher (AES-256-CBC) and IV length (16 bytes) follow cryptographic best practices.
254-254: Consider using a more appropriate sanitization method for API keys.
The current implementation uses sanitize_text_field() which might alter special characters in the API key.
Also applies to: 335-339
436-444: LGTM! API key getter implementation is secure.
The method properly handles missing or invalid API keys by returning an empty string.
543-548: LGTM! Secure implementation of visitor recognition.
The code correctly:
- Checks for empty email and API key before computing hash
- Uses HMAC-SHA256 for secure hash generation
411-429: 🛠️ Refactor suggestion
Add input validation in decryption method.
The method should validate the input length before attempting decryption.
private static function get_decrypted_data( $data ) {
+ if ( empty( $data ) ) {
+ return '';
+ }
+
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode
$decoded_data = base64_decode( $data );
if ( false === $decoded_data ) {
+ error_log( 'Failed to base64 decode encrypted data' );
return '';
}
+ if ( strlen( $decoded_data ) <= self::CIPHER_IV_LENGTH ) {
+ error_log( 'Decoded data is too short' );
+ return '';
+ }
$iv = substr( $decoded_data, 0, self::CIPHER_IV_LENGTH );
$encrypted_data = substr( $decoded_data, self::CIPHER_IV_LENGTH );
$decrypted_data = openssl_decrypt( $encrypted_data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv );
if ( false === $decrypted_data ) {
+ error_log( 'Failed to decrypt data with OpenSSL' );
return '';
}
return $decrypted_data;
}Likely invalid or redundant comment.
tawkto/templates/settings.php (3)
435-435: Reconsider the input validation pattern for 'JavaScript API Key'
The current pattern ^[a-zA-Z0-9]+$ allows only letters and digits. If the API key may include other characters such as hyphens or underscores, consider updating the pattern to accommodate them.
You can adjust the pattern as follows:
- pattern="^[a-zA-Z0-9]+$"
+ pattern="^[a-zA-Z0-9-_]+$"434-436: Add a placeholder to the API key input field for better user guidance
Including a placeholder can improve user experience by indicating what the user should enter.
You can add the placeholder attribute:
value="<?php echo esc_attr( $visibility['js_api_key'] ); ?>"
+ placeholder="<?php esc_attr_e( 'Enter your JavaScript API Key', 'tawk-to-live-chat' ); ?>"
pattern="^[a-zA-Z0-9]+$"431-436: Enhance the API key input field with a show/hide toggle
Adding a toggle button to show or hide the API key can improve usability while maintaining security.
If you'd like assistance implementing this feature, I can help provide the necessary code or open a GitHub issue.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tawkto/tawkto.php (3)
405-430: 🛠️ Refactor suggestionUse
random_bytes()and improve error handling in encryption method.The encryption implementation can be enhanced with better error handling and modern cryptographic functions.
438-456: 🛠️ Refactor suggestionAdd error handling and input validation in decryption method.
The decryption implementation needs better error handling for potential failures.
581-586: 🛠️ Refactor suggestionEnsure email is not empty before computing hash.
Verify that the email is not empty before computing the hash with
hash_hmac().
🧹 Nitpick comments (3)
tests/Coverages/PrivacyOptionsTest.php (1)
88-88: Use a realistic API key format for testing.The current test uses a repeated character for the API key. Consider using a more realistic API key format that matches the expected pattern from the tawk.to service.
-self::$driver->find_element_and_input('#js-api-key', str_repeat('a', 40)); +self::$driver->find_element_and_input('#js-api-key', '1234567890abcdef1234567890abcdef12345678');tawkto/templates/settings.php (1)
415-419: Add more context to the notice message.The notice should provide more information about Secure Mode and why the API key is required.
- <?php esc_html_e( 'Note: If Secure Mode is enabled on your property, please enter your Javascript API Key to ensure visitor recognition works correctly.', 'tawk-to-live-chat' ); ?> + <?php esc_html_e( 'Note: If Secure Mode is enabled on your property, please enter your Javascript API Key. This key is required to securely identify visitors and ensure visitor recognition works correctly. You can find your API key in the tawk.to dashboard under Property Settings > Security.', 'tawk-to-live-chat' ); ?>tawkto/tawkto.php (1)
463-479: Cache duration might be too long for sensitive data.The API key is cached for 1 hour (3600 seconds). Consider reducing the cache duration for sensitive data.
- set_transient( self::TAWK_API_KEY, $key, 60 * 60 ); + set_transient( self::TAWK_API_KEY, $key, 15 * 60 ); // Cache for 15 minutes instead of 1 hour
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tawkto/tawkto.php(7 hunks)tawkto/templates/settings.php(1 hunks)tests/Coverages/PrivacyOptionsTest.php(3 hunks)
🔇 Additional comments (4)
tests/Coverages/PrivacyOptionsTest.php (1)
64-64: LGTM! The assertion verifies secure mode behavior.The added assertion correctly verifies that the visitor object does not contain a hash when secure mode is not enabled (no API key provided).
tawkto/templates/settings.php (1)
427-433: Enhance the API key input field with security and UX improvements.The current implementation can be improved with additional security features and better user experience.
tawkto/tawkto.php (2)
30-34: LGTM! Constants are well-defined and follow naming conventions.The constants are appropriately named and provide clear separation of concerns for encryption-related functionality.
557-559: LGTM! Proper cleanup of sensitive data.The deactivation method correctly cleans up both the API key and its encrypted version from transients.
| self::$driver->move_mouse_to( '#submit-header' )->click(); | ||
| self::$driver->wait_for_seconds( 1 ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace arbitrary wait with explicit wait condition.
Using a fixed wait time can make tests flaky. Instead, wait for a specific condition to be met.
-self::$driver->move_mouse_to('#submit-header')->click();
-self::$driver->wait_for_seconds(1);
+self::$driver->move_mouse_to('#submit-header')->click();
+self::$driver->wait_until_element_is_located('#setting-error-settings_updated');📝 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.
| self::$driver->move_mouse_to( '#submit-header' )->click(); | |
| self::$driver->wait_for_seconds( 1 ); | |
| self::$driver->move_mouse_to('#submit-header')->click(); | |
| self::$driver->wait_until_element_is_located('#setting-error-settings_updated'); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tawkto/tawkto.php (1)
340-349: Add format validation for the API key.While the length check is good, consider adding a format validation to ensure the API key contains only valid characters.
- $fields['js_api_key'] = 40 === strlen( $fields['js_api_key'] ) ? self::get_encrypted_data( $fields['js_api_key'] ) : ''; + $fields['js_api_key'] = (40 === strlen( $fields['js_api_key'] ) && preg_match('/^[a-zA-Z0-9]+$/', $fields['js_api_key'])) + ? self::get_encrypted_data( $fields['js_api_key'] ) + : '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tawkto/tawkto.php(7 hunks)
🔇 Additional comments (4)
tawkto/tawkto.php (4)
30-33: LGTM! Well-defined encryption constants.The encryption configuration uses industry-standard AES-256-CBC with appropriate IV length.
404-429: LGTM! Robust encryption implementation.The encryption and decryption methods:
- Use cryptographically secure random_bytes()
- Include comprehensive error handling
- Properly handle the IV
- Use base64 encoding for safe storage
Also applies to: 437-455
579-584: LGTM! Secure implementation of visitor recognition.The implementation follows security best practices:
- Uses HMAC-SHA256 for secure hashing
- Properly validates email and API key presence
557-557: LGTM! Proper cleanup of sensitive data.The deactivation handler correctly removes the cached API key.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tawkto/upgrades/version.090.php (1)
3-5: Consider using a namespace for better organization.Since this is a new file, consider organizing the upgrade classes under a namespace to prevent potential class name conflicts.
<?php +namespace TawkTo\Upgrades; -if ( ! class_exists( 'TawkToUpgradeBase' ) ) { - require_once dirname( __FILE__ ) . '/base.php'; -} +use TawkTo\Upgrades\Base as TawkToUpgradeBase; +require_once dirname( __FILE__ ) . '/base.php';tawkto/upgrade.manager.php (1)
42-45: Add validation for upgrade class existence.Consider validating that the upgrade classes exist before adding them to the upgrades array to prevent potential runtime errors.
public function __construct( $version, $version_var_name ) { + $this->validateUpgradeClasses([ + TawkToUpgradeVersion070::class, + TawkToUpgradeVersion090::class + ]); + $this->upgrades = array( TawkToUpgradeVersion070::get_version() => TawkToUpgradeVersion070::class, TawkToUpgradeVersion090::get_version() => TawkToUpgradeVersion090::class, );Add this helper method:
/** * Validates that all upgrade classes exist * * @param array $classes Array of class names to validate * @throws RuntimeException if any class doesn't exist */ private function validateUpgradeClasses(array $classes) { foreach ($classes as $class) { if (!class_exists($class)) { throw new RuntimeException( sprintf('Upgrade class %s not found', $class) ); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tawkto/includes/default_config.php(1 hunks)tawkto/tawkto.php(15 hunks)tawkto/templates/settings.php(1 hunks)tawkto/upgrade.manager.php(2 hunks)tawkto/upgrades/version.090.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tawkto/includes/default_config.php
- tawkto/tawkto.php
🔇 Additional comments (4)
tawkto/templates/settings.php (2)
415-437: Enhance the API key input field with security and UX improvements.The current implementation includes basic security measures, but could benefit from the previously suggested improvements for better user experience while maintaining security.
The previous review comment suggested:
- Adding a placeholder text
- Adding a toggle button to show/hide the API key
- Using WordPress dashicons for consistency
- Adding ARIA labels for accessibility
407-409: Verify data migration for the visitor recognition setting.The setting has been moved from the visibility options to privacy options. This change requires data migration for existing installations to preserve user preferences.
Run this script to check if any installations might be affected:
✅ Verification successful
Migration is properly implemented for the visitor recognition setting
The change is safely handled by the migration code in
upgrades/version.090.php, which moves the setting from visibility options to privacy options while preserving user preferences.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential data migration issues # Look for files that might reference the old setting path # Search for old setting references echo "Searching for old setting references..." rg -l "tawkto-visibility-options\[enable_visitor_recognition\]" # Search for potential upgrade/migration code echo "Checking for migration handling..." rg -l "enable_visitor_recognition" -g "*/update*.php" rg -l "enable_visitor_recognition" -g "*/upgrade*.php"Length of output: 491
Script:
#!/bin/bash # Broader search for visitor recognition and migration code echo "Searching for visitor recognition setting..." rg -l "enable_visitor_recognition" echo -e "\nSearching for update/migration code..." rg -l "update.*options|upgrade.*options|register_activation_hook" --type php echo -e "\nChecking file content around visitor recognition..." rg -C 3 "enable_visitor_recognition"Length of output: 3897
tawkto/upgrade.manager.php (2)
4-4: LGTM!The new upgrade file is properly included.
44-44: Verify upgrade version ordering.The upgrade versions in the array determine the execution order. Ensure that version 0.9.0 should indeed be executed after version 0.7.0.
Run this script to verify the version ordering:
✅ Verification successful
Version ordering is correct ✓
The version ordering in the upgrade manager is proper, with version 0.9.0 correctly positioned to execute after version 0.7.0. This is confirmed by:
- Semantic versioning order (0.7.0 < 0.9.0)
- Independent migration purposes with no cross-version dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify upgrade version ordering by checking all upgrade files # Find all version files and extract version numbers echo "Checking upgrade version files ordering:" fd "version\.\d+\.php$" tawkto/upgrades/ --exec grep -H "VERSION = '\d\.\d\.\d'" {} \;Length of output: 188
Script:
#!/bin/bash # First, let's find all PHP files with "Version" in their name echo "=== Finding version-related PHP files ===" fd -e php -g "*[Vv]ersion*" echo -e "\n=== Searching for version constants ===" rg -l "VERSION\s*=\s*['\"]" --type php echo -e "\n=== Looking for version class definitions ===" ast-grep --pattern 'class TawkToUpgradeVersion$_ { $$$ }'Length of output: 557
Script:
#!/bin/bash echo "=== Version 0.7.0 implementation ===" rg "VERSION\s*=|class\s+TawkToUpgradeVersion070" -A 5 tawkto/upgrades/version.070.php echo -e "\n=== Version 0.9.0 implementation ===" rg "VERSION\s*=|class\s+TawkToUpgradeVersion090" -A 5 tawkto/upgrades/version.090.php echo -e "\n=== Checking for version dependencies ===" rg "require|depend|before|after" --type php tawkto/upgrades/Length of output: 1176
| public static function upgrade() { | ||
| $visibility = get_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS ); | ||
|
|
||
| if ( isset( $visibility['enable_visitor_recognition'] ) ) { | ||
| update_option( TawkTo_Settings::TAWK_PRIVACY_OPTIONS, array( 'enable_visitor_recognition' => $visibility['enable_visitor_recognition'] ) ); | ||
|
|
||
| unset( $visibility['enable_visitor_recognition'] ); | ||
| update_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS, $visibility ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation to the upgrade process.
The upgrade method should include:
- Validation of retrieved options
- Error handling for failed updates
- Logging of migration status for debugging
Consider this implementation:
public static function upgrade() {
+ $success = true;
$visibility = get_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS );
+
+ if (false === $visibility) {
+ error_log('TawkTo Upgrade 0.9.0: Failed to retrieve visibility options');
+ return false;
+ }
if ( isset( $visibility['enable_visitor_recognition'] ) ) {
- update_option( TawkTo_Settings::TAWK_PRIVACY_OPTIONS, array( 'enable_visitor_recognition' => $visibility['enable_visitor_recognition'] ) );
+ $privacy_update = update_option(
+ TawkTo_Settings::TAWK_PRIVACY_OPTIONS,
+ array( 'enable_visitor_recognition' => $visibility['enable_visitor_recognition'] )
+ );
+
+ if (!$privacy_update) {
+ error_log('TawkTo Upgrade 0.9.0: Failed to update privacy options');
+ $success = false;
+ }
unset( $visibility['enable_visitor_recognition'] );
- update_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS, $visibility );
+ $visibility_update = update_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS, $visibility );
+
+ if (!$visibility_update) {
+ error_log('TawkTo Upgrade 0.9.0: Failed to update visibility options');
+ $success = false;
+ }
}
+
+ return $success;
}📝 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 upgrade() { | |
| $visibility = get_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS ); | |
| if ( isset( $visibility['enable_visitor_recognition'] ) ) { | |
| update_option( TawkTo_Settings::TAWK_PRIVACY_OPTIONS, array( 'enable_visitor_recognition' => $visibility['enable_visitor_recognition'] ) ); | |
| unset( $visibility['enable_visitor_recognition'] ); | |
| update_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS, $visibility ); | |
| } | |
| } | |
| public static function upgrade() { | |
| $success = true; | |
| $visibility = get_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS ); | |
| if (false === $visibility) { | |
| error_log('TawkTo Upgrade 0.9.0: Failed to retrieve visibility options'); | |
| return false; | |
| } | |
| if ( isset( $visibility['enable_visitor_recognition'] ) ) { | |
| $privacy_update = update_option( | |
| TawkTo_Settings::TAWK_PRIVACY_OPTIONS, | |
| array( 'enable_visitor_recognition' => $visibility['enable_visitor_recognition'] ) | |
| ); | |
| if (!$privacy_update) { | |
| error_log('TawkTo Upgrade 0.9.0: Failed to update privacy options'); | |
| $success = false; | |
| } | |
| unset( $visibility['enable_visitor_recognition'] ); | |
| $visibility_update = update_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS, $visibility ); | |
| if (!$visibility_update) { | |
| error_log('TawkTo Upgrade 0.9.0: Failed to update visibility options'); | |
| $success = false; | |
| } | |
| } | |
| return $success; | |
| } |
eug-L
force-pushed
the
feature/secure-mode
branch
from
4d9bc2a to
ce5e8a4
Compare
eug-L
force-pushed
the
feature/secure-mode
branch
from
ce5e8a4 to
128eb19
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tawkto/tawkto.php (3)
81-95: Add error handling for database operations.The
init_optionsmethod should handle potential database failures when callingupdate_option(). Consider wrapping the operations in try-catch blocks and logging any failures.public static function init_options() { $options = self::get_default_options(); + try { if ( ! get_option( self::TAWK_VISIBILITY_OPTIONS, false ) ) { - update_option( self::TAWK_VISIBILITY_OPTIONS, $options['visibility'] ); + if ( ! update_option( self::TAWK_VISIBILITY_OPTIONS, $options['visibility'] ) ) { + error_log( 'Failed to initialize Tawk.to visibility options' ); + } } // Similar changes for privacy and security options + } catch ( Exception $e ) { + error_log( 'Error initializing Tawk.to options: ' . $e->getMessage() ); + } }
416-439: Enhance API key validation.The current validation only checks the length of the API key. Consider adding:
- Character set validation (alphanumeric check)
- Format validation (if the API key follows a specific pattern)
- Rate limiting for failed validation attempts
private static function validate_js_api_key( &$fields, $default ) { if ( self::NO_CHANGE === $fields['js_api_key'] ) { unset( $fields['js_api_key'] ); return; } delete_transient( self::TAWK_API_KEY ); if ( '' === $fields['js_api_key'] ) { return; } try { if ( 40 !== strlen( $fields['js_api_key'] ) ) { throw new Exception( 'Invalid key. Please provide value with 40 characters' ); } + if ( ! preg_match('/^[a-zA-Z0-9]+$/', $fields['js_api_key']) ) { + throw new Exception( 'Invalid key. Only alphanumeric characters are allowed' ); + } + // Add rate limiting check here if needed $fields['js_api_key'] = self::get_encrypted_data( $fields['js_api_key'] ); } catch ( Exception $e ) { self::show_tawk_options_error( 'Javascript API Key: ' . $e->getMessage() ); $fields['js_api_key'] = $default; } }
658-664: Enhance plugin cleanup on deactivation.Consider adding cleanup for any remaining transients and user meta data that might have been created during the plugin's operation.
public static function deactivate() { delete_option( TawkTo_Settings::TAWK_PAGE_ID_VARIABLE ); delete_option( TawkTo_Settings::TAWK_WIDGET_ID_VARIABLE ); delete_option( TawkTo_Settings::TAWK_VISIBILITY_OPTIONS ); delete_option( TawkTo_Settings::TAWK_PRIVACY_OPTIONS ); delete_option( TawkTo_Settings::TAWK_SECURITY_OPTIONS ); delete_option( self::PLUGIN_VERSION_VARIABLE ); delete_transient( TawkTo_Settings::TAWK_API_KEY ); + // Clean up any user meta data + delete_metadata( 'user', 0, 'tawkto_user_settings', '', true ); + // Clean up any additional transients + delete_transient( 'tawkto_rate_limit' ); }
🛑 Comments failed to post (1)
tawkto/tawkto.php (1)
553-568: 🛠️ Refactor suggestion
Use constant-time comparison for API key validation.
When comparing API keys in your application logic, ensure you use hash_equals() for constant-time comparison to prevent timing attacks.
public static function get_js_api_key() { - if ( ! empty( get_transient( self::TAWK_API_KEY ) ) ) { + $cached_key = get_transient( self::TAWK_API_KEY ); + if ( ! empty( $cached_key ) ) { return get_transient( self::TAWK_API_KEY ); } $security = get_option( self::TAWK_SECURITY_OPTIONS ); if ( ! isset( $security['js_api_key'] ) ) { return ''; } $key = self::get_decrypted_data( $security['js_api_key'] ); + if ( empty( $key ) ) { + error_log( 'Failed to decrypt API key' ); + return ''; + } set_transient( self::TAWK_API_KEY, $key, 60 * 60 ); return $key; }📝 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 get_js_api_key() { $cached_key = get_transient( self::TAWK_API_KEY ); if ( ! empty( $cached_key ) ) { return get_transient( self::TAWK_API_KEY ); } $security = get_option( self::TAWK_SECURITY_OPTIONS ); if ( ! isset( $security['js_api_key'] ) ) { return ''; } $key = self::get_decrypted_data( $security['js_api_key'] ); if ( empty( $key ) ) { error_log( 'Failed to decrypt API key' ); return ''; } set_transient( self::TAWK_API_KEY, $key, 60 * 60 ); return $key;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tawkto/tawkto.php (3)
416-439: Add input validation for the JS API key format.The JS API key validation could be enhanced to ensure it matches the expected format before encryption.
private static function validate_js_api_key( &$fields, $default ) { if ( self::NO_CHANGE === $fields['js_api_key'] ) { unset( $fields['js_api_key'] ); return; } delete_transient( self::TAWK_API_KEY ); if ( '' === $fields['js_api_key'] ) { return; } try { if ( 40 !== strlen( $fields['js_api_key'] ) ) { throw new Exception( 'Invalid key. Please provide value with 40 characters' ); } + if ( ! preg_match('/^[a-f0-9]{40}$/i', $fields['js_api_key']) ) { + throw new Exception( 'Invalid key format. Key should contain only hexadecimal characters' ); + } $fields['js_api_key'] = self::get_encrypted_data( $fields['js_api_key'] ); } catch ( Exception $e ) { self::show_tawk_options_error( 'Javascript API Key: ' . $e->getMessage() ); $fields['js_api_key'] = $default; } }
553-568: Consider increasing cache duration for API key.The API key is cached for only 1 hour. Consider increasing this to reduce database calls.
-set_transient( self::TAWK_API_KEY, $key, 60 * 60 ); +// Cache for 24 hours since API keys rarely change +set_transient( self::TAWK_API_KEY, $key, 24 * 60 * 60 );
701-709: Add type casting for privacy options.The privacy options should be explicitly type cast to ensure consistent behavior.
-if ( isset( $privacy ) && isset( $privacy['enable_visitor_recognition'] ) ) { - $enable_visitor_recognition = 1 === $privacy['enable_visitor_recognition']; +if ( isset( $privacy['enable_visitor_recognition'] ) ) { + $enable_visitor_recognition = 1 === (int) $privacy['enable_visitor_recognition']; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tawkto/tawkto.php(15 hunks)tawkto/upgrade.manager.php(2 hunks)tawkto/upgrades/version.090.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tawkto/upgrade.manager.php
- tawkto/upgrades/version.090.php
🔇 Additional comments (5)
tawkto/tawkto.php (5)
28-34: LGTM! Well-structured constants for new features.The new constants are well-named and logically grouped for privacy and security features.
81-95: LGTM! Proper initialization of default options.The
init_optionsmethod correctly initializes visibility, privacy, and security options with default values if they don't exist.
686-691: LGTM! Secure implementation of visitor hash.The implementation correctly uses HMAC-SHA256 for visitor recognition and only generates the hash when both email and API key are present.
495-520: 🛠️ Refactor suggestionEnhance error handling in encryption method.
The encryption method should validate the encrypted data length and handle OpenSSL errors.
private static function get_encrypted_data( $data ) { if ( ! defined( 'SECURE_AUTH_KEY' ) ) { throw new Exception( 'SECURE_AUTH_KEY is not defined' ); } try { $iv = random_bytes( self::CIPHER_IV_LENGTH ); } catch ( Exception $e ) { throw new Exception( 'Error generating IV' ); } + if ( ! in_array( self::CIPHER, openssl_get_cipher_methods() ) ) { + throw new Exception( 'Cipher method not supported' ); + } $encrypted_data = openssl_encrypt( $data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); if ( false === $encrypted_data ) { + $error = openssl_error_string(); + throw new Exception( 'Error encrypting data: ' . ($error ? $error : 'Unknown error') ); } // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode $encrypted_data = base64_encode( $iv . $encrypted_data ); if ( false === $encrypted_data ) { throw new Exception( 'Error encoding data' ); } + if ( empty( $encrypted_data ) ) { + throw new Exception( 'Encrypted data is empty' ); + } return $encrypted_data; }Likely invalid or redundant comment.
528-546: 🛠️ Refactor suggestionEnhance error handling in decryption method.
The decryption method should validate input length and handle OpenSSL errors.
private static function get_decrypted_data( $data ) { + if ( empty( $data ) ) { + return ''; + } // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode $decoded_data = base64_decode( $data ); if ( false === $decoded_data ) { return ''; } + if ( strlen( $decoded_data ) <= self::CIPHER_IV_LENGTH ) { + return ''; + } $iv = substr( $decoded_data, 0, self::CIPHER_IV_LENGTH ); $encrypted_data = substr( $decoded_data, self::CIPHER_IV_LENGTH ); $decrypted_data = openssl_decrypt( $encrypted_data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); if ( false === $decrypted_data ) { + $error = openssl_error_string(); + error_log( 'Tawk.to decryption error: ' . ($error ? $error : 'Unknown error') ); return ''; } return $decrypted_data; }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Coverages/PrivacyOptionsTest.php (1)
88-88: Use a meaningful API key constant.Instead of using a hardcoded string of repeated characters, consider defining a constant or using a more meaningful test API key. This improves test maintainability and makes it clearer what constitutes a valid API key format.
-self::$driver->find_element_and_input('#js-api-key', str_repeat('a', 40)); +const VALID_TEST_API_KEY = '1234567890abcdef1234567890abcdef12345678'; +self::$driver->find_element_and_input('#js-api-key', self::VALID_TEST_API_KEY);tests/TestFiles/Modules/Web.php (1)
Line range hint
242-303: Good alignment with test improvements!The changes consistently use appropriate success indicators:
- '#successMessage' for widget operations
- '#setting-error-settings_updated' for WordPress settings
Consider adding code comments to document these element IDs and their significance.
// ensures widget is added. -self::$driver->wait_until_element_is_located( '#successMessage' ); +// Wait for tawk.to's success message element that appears after widget operations +self::$driver->wait_until_element_is_located( '#successMessage' ); // ensures widget is added. -self::$driver->wait_until_element_is_located( '#setting-error-settings_updated' ); +// Wait for WordPress's standard settings updated success message +self::$driver->wait_until_element_is_located( '#setting-error-settings_updated' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/Coverages/PrivacyOptionsTest.php(3 hunks)tests/Coverages/VisibilityOptionsTest.php(15 hunks)tests/Coverages/WoocommerceOptionsTest.php(12 hunks)tests/TestFiles/Modules/Web.php(4 hunks)
🔇 Additional comments (5)
tests/Coverages/PrivacyOptionsTest.php (3)
38-38: LGTM! Good setup enhancement.Adding navigation to privacy options in setup ensures a consistent starting state for all tests.
64-64: LGTM! Good defensive testing.The added assertion verifies that the hash is not present by default, which is important for security and complements the new visitor recognition test case.
90-91: Replace arbitrary wait with explicit wait condition.Using a fixed wait time can make tests flaky. Instead, wait for a specific condition to be met.
-self::$driver->move_mouse_to('#submit-header')->click(); -self::$driver->wait_until_element_is_located('#setting-error-settings_updated'); +self::$driver->move_mouse_to('#submit-header')->click(); +self::$driver->wait_until_element_is_located('#setting-error-settings_updated', 'Settings saved.');tests/Coverages/WoocommerceOptionsTest.php (1)
Line range hint
85-240: Great improvement to test reliability!Replacing fixed delays with dynamic waits for the WordPress settings updated message makes the tests more robust and less prone to timing issues. The change is consistently applied across all test methods.
tests/Coverages/VisibilityOptionsTest.php (1)
Line range hint
81-411: Consistent implementation across test files!The changes mirror those in WoocommerceOptionsTest.php, maintaining consistency in the test suite's approach to handling asynchronous operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tawkto/tawkto.php (2)
552-567: Consider increasing the transient expiration time.The API key is cached for only 1 hour. Consider increasing this to reduce database calls, as API keys typically have longer lifetimes.
- set_transient( self::TAWK_API_KEY, $key, 60 * 60 ); + set_transient( self::TAWK_API_KEY, $key, 24 * 60 * 60 ); // Cache for 24 hours
Line range hint
792-811: Consider caching the current URL.The
get_current_url()is called multiple times in the URL pattern matching logic. Consider caching the result.Apply this diff to improve performance:
if ( isset( $visibility['include_url'] ) && 1 === $visibility['include_url'] ) { - $current_url = $this->get_current_url(); + $current_url = $current_url ?? $this->get_current_url(); $included_url_list = $visibility['included_url_list']; $included_url_list = array_map( 'trim', preg_split( '/,/', $included_url_list ) ); if ( UrlPatternMatcher::match( $current_url, $included_url_list ) ) { $display = true; } } if ( isset( $visibility['exclude_url'] ) && ( 1 === $visibility['exclude_url'] ) ) { - $current_url = $this->get_current_url(); + $current_url = $current_url ?? $this->get_current_url(); $excluded_url_list = $visibility['excluded_url_list'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tawkto/tawkto.php(15 hunks)
🔇 Additional comments (4)
tawkto/tawkto.php (4)
28-34: LGTM! Well-structured constants for new features.The new constants are well-named and follow WordPress coding standards.
76-95: LGTM! Proper initialization of default options.The
init_options()method correctly initializes visibility, privacy, and security options with default values if they don't exist.
685-690: LGTM! Secure implementation of visitor hash.The HMAC hash implementation for visitor recognition is secure and follows best practices.
527-545:⚠️ Potential issueAdd length validation in decryption method.
The decryption method should validate the length of the decoded data before substring operations.
Apply this diff to add validation:
private static function get_decrypted_data( $data ) { // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode $decoded_data = base64_decode( $data ); if ( false === $decoded_data ) { return ''; } + if ( strlen($decoded_data) <= self::CIPHER_IV_LENGTH ) { + return ''; + } + $iv = substr( $decoded_data, 0, self::CIPHER_IV_LENGTH ); $encrypted_data = substr( $decoded_data, self::CIPHER_IV_LENGTH );Likely invalid or redundant comment.
| private static function get_encrypted_data( $data ) { | ||
| if ( ! defined( 'SECURE_AUTH_KEY' ) ) { | ||
| throw new Exception( 'SECURE_AUTH_KEY is not defined' ); | ||
| } | ||
|
|
||
| try { | ||
| $iv = random_bytes( self::CIPHER_IV_LENGTH ); | ||
| } catch ( Exception $e ) { | ||
| throw new Exception( 'Error generating IV' ); | ||
| } | ||
|
|
||
| $encrypted_data = openssl_encrypt( $data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); | ||
|
|
||
| if ( false === $encrypted_data ) { | ||
| throw new Exception( 'Error encrypting data' ); | ||
| } | ||
|
|
||
| // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | ||
| $encrypted_data = base64_encode( $iv . $encrypted_data ); | ||
|
|
||
| if ( false === $encrypted_data ) { | ||
| throw new Exception( 'Error encoding data' ); | ||
| } | ||
|
|
||
| return $encrypted_data; | ||
| } |
There was a problem hiding this comment.
Add error handling for openssl_encrypt failures.
The encryption method needs additional error handling:
- Check if the OpenSSL extension is loaded
- Verify the cipher method exists
Apply this diff to improve error handling:
private static function get_encrypted_data( $data ) {
+ if (!extension_loaded('openssl')) {
+ throw new Exception('OpenSSL extension is not loaded');
+ }
+
+ if (!in_array(self::CIPHER, openssl_get_cipher_methods())) {
+ throw new Exception('Cipher method not supported');
+ }
+
if ( ! defined( 'SECURE_AUTH_KEY' ) ) {
throw new Exception( 'SECURE_AUTH_KEY is not defined' );
}📝 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.
| private static function get_encrypted_data( $data ) { | |
| if ( ! defined( 'SECURE_AUTH_KEY' ) ) { | |
| throw new Exception( 'SECURE_AUTH_KEY is not defined' ); | |
| } | |
| try { | |
| $iv = random_bytes( self::CIPHER_IV_LENGTH ); | |
| } catch ( Exception $e ) { | |
| throw new Exception( 'Error generating IV' ); | |
| } | |
| $encrypted_data = openssl_encrypt( $data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); | |
| if ( false === $encrypted_data ) { | |
| throw new Exception( 'Error encrypting data' ); | |
| } | |
| // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | |
| $encrypted_data = base64_encode( $iv . $encrypted_data ); | |
| if ( false === $encrypted_data ) { | |
| throw new Exception( 'Error encoding data' ); | |
| } | |
| return $encrypted_data; | |
| } | |
| private static function get_encrypted_data( $data ) { | |
| if (!extension_loaded('openssl')) { | |
| throw new Exception('OpenSSL extension is not loaded'); | |
| } | |
| if (!in_array(self::CIPHER, openssl_get_cipher_methods())) { | |
| throw new Exception('Cipher method not supported'); | |
| } | |
| if ( ! defined( 'SECURE_AUTH_KEY' ) ) { | |
| throw new Exception( 'SECURE_AUTH_KEY is not defined' ); | |
| } | |
| try { | |
| $iv = random_bytes( self::CIPHER_IV_LENGTH ); | |
| } catch ( Exception $e ) { | |
| throw new Exception( 'Error generating IV' ); | |
| } | |
| $encrypted_data = openssl_encrypt( $data, self::CIPHER, SECURE_AUTH_KEY, 0, $iv ); | |
| if ( false === $encrypted_data ) { | |
| throw new Exception( 'Error encrypting data' ); | |
| } | |
| // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | |
| $encrypted_data = base64_encode( $iv . $encrypted_data ); | |
| if ( false === $encrypted_data ) { | |
| throw new Exception( 'Error encoding data' ); | |
| } | |
| return $encrypted_data; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes