add js api key field & hashing for visitor recognition #20
Conversation
WalkthroughThe changes modify the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (6)
🪧 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: 1
🧹 Nitpick comments (3)
src/core/TawktoGenerator.php (3)
66-68: Conditionally setting$secure_mode_enabled.Assigning the secure mode value from
$display_optsis correct. Consider validating its type (boolean) to avoid unexpected behavior if the stored data is not a strict boolean.
119-137: NewgetKeyModule()method.Cleanly checks for the Key module and retrieves available keys. Consider renaming to something like
getKeyRepository()for clarity, but otherwise it’s a solid addition.
502-547: Security Options fieldset.Nicely introduces a secure mode checkbox and a dropdown for selecting the JS API key. Potential enhancements:
- Hide or disable the JS API key dropdown unless “Secure mode enabled” is checked.
- Alert users when no valid key is selected, if relevant.
Overall, a helpful addition to the UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/TawktoGenerator.php(6 hunks)
🔇 Additional comments (8)
src/core/TawktoGenerator.php (8)
57-58: Initialization looks good.Defining
$secure_mode_enabledand$js_api_keywith sane defaults is clear and aligns with the intended secure mode logic.
70-72: Extracting$js_api_keyfrom user input.Logic here is consistent. You might also verify that the provided key string is expected or sanitized to prevent misconfiguration.
80-80: Preparing$hashfor secure mode usage.Initializing
$hashto NULL is fine for clarity of intent.
82-83: Loading key repository.Retrieving the keys from the newly introduced
getKeyModule()is well-structured. No immediate issues found.
85-89: Hash generation for secure mode.The
hash_hmac('sha256', $usermail, $keys[$js_api_key]->getKeyValue())logic looks correct. Consider handling empty or invalid$usermailvalues explicitly to avoid unexpected hashes, and ensure$keys[$js_api_key]->getKeyValue()is non-null.
259-261: Retrieving key module state for the form.Straightforward usage of
getKeyModule()output. Properly sets up$keyModuleInstalledand$keys.
780-782: Storingjs_api_keyin$jsonOpts.Storing the trimmed value is correct; you might also ensure it exists in the actual key repository to prevent invalid references.
788-788: Boolean assignment for secure mode.Storing
secure_mode_enabledas a boolean is consistent with Drupal config patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/TawktoGenerator.php (1)
93-95:⚠️ Potential issuePotential XSS vulnerability in visitor data embedding.
The code directly embeds user data (
$username,$usermail, and now$hash) into JavaScript without proper escaping, which could lead to XSS vulnerabilities if these values contain malicious code.Apply sanitization to all user-provided data:
- name : "' . $username . '", - email : "' . $usermail . '",' . - (!is_null($hash) ? PHP_EOL . 'hash : "' . $hash . '",' : '') . + use Drupal\Component\Utility\Html; + name : "' . Html::escape($username) . '", + email : "' . Html::escape($usermail) . '",' . + (!is_null($hash) ? PHP_EOL . 'hash : "' . Html::escape($hash) . '",' : '') .
🧹 Nitpick comments (2)
src/core/TawktoGenerator.php (2)
520-546: Improve JavaScript API Key selection logic.There are two issues with the current implementation:
- The dropdown selection logic assumes
$display_opts->js_api_keyis always set, which may not be true on first load- The dropdown doesn't include a "None" or "-- Select --" option when no key is selected
Improve the key selection dropdown with these changes:
<select name="js_api_key" id="js_api_key"> + <option value="">-- Select a key --</option> <?php foreach (array_keys($keys) as $key) { - $selected = 'selected'; - if (!is_null($display_opts) && $display_opts->js_api_key !== $key) { - $selected = ''; - } + $selected = ''; + if (!is_null($display_opts) && isset($display_opts->js_api_key) && $display_opts->js_api_key === $key) { + $selected = 'selected'; + } ?> <option value="<?php echo $key; ?>" <?php echo $selected; ?>><?php echo $key; ?></option> <?php } ?> </select>
94-94: Potential newline rendering issue in JavaScript object.Adding a newline with
PHP_EOLinside a JavaScript object might cause syntax errors in some browsers if not properly handled.Consider using a cleaner approach without the conditional newline:
- email : "' . $usermail . '",' . - (!is_null($hash) ? PHP_EOL . 'hash : "' . $hash . '",' : '') . + email : "' . $usermail . '"' . + (!is_null($hash) ? ',' . PHP_EOL . 'hash : "' . $hash . '"' : '') .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/TawktoGenerator.php(7 hunks)
🔇 Additional comments (11)
src/core/TawktoGenerator.php (11)
57-58: Security enhancement with secure mode configuration.These new variables add support for the secure mode feature, which helps enhance visitor recognition security through hash verification.
66-72: LGTM: Proper retrieval of secure mode configuration.The code correctly extracts the secure mode settings from the display options, following the same pattern used for other settings.
80-89: Hash generation is correctly implemented for secure mode.The implementation follows Tawk.to's secure mode recommendations by:
- Only generating the hash when secure mode is enabled
- Verifying that a valid JavaScript API key exists
- Using a secure hashing algorithm (HMAC SHA256) with the visitor's email
This implementation helps prevent identity spoofing by ensuring visitors are who they claim to be.
119-137: Well-structured key module integration.The
getKeyModule()method is well-implemented:
- It properly checks if the key repository service is available
- It safely retrieves keys if the service exists
- It returns a clear structure with both installation status and available keys
- It's appropriately encapsulated as a private method
This supports the secure mode feature by providing access to API keys stored through Drupal's Key module.
259-262: LGTM: Consistent use of key module retrieval.The code reuses the
getKeyModule()method to maintain consistency and avoid code duplication when generating the form.
502-507: Good UX with clear security section header and instructions.The new "Security Options" section provides clear instructions about secure mode functionality, helping administrators understand the purpose and requirements of this feature.
508-519: LGTM: Secure mode toggle with proper default.The secure mode toggle is correctly implemented with a proper default value and maintains consistency with other option toggles in the form.
780-783: LGTM: Proper handling of API key in options.The code correctly handles the JavaScript API key as a string value and trims it to remove any whitespace.
788-788: LGTM: Added secure mode to boolean options.The secure mode setting is correctly added to the list of boolean options, ensuring it's properly processed as a boolean value.
564-564: LGTM: Updated iframe URL to include Drupal version.The code correctly uses the major Drupal version in the iframe URL, which helps with compatibility tracking.
85-89:❓ Verification inconclusive
Verify hash generation function works with different key types.
The hash generation relies on the key module, but it doesn't check what type of key is being used. Different key providers might store or return values differently.
🏁 Script executed:
#!/bin/bash # Description: Check what key types are available and supported in Drupal's Key module # Check if the Key module is installed and what key types are available drush pm-list | grep key drush eval "print_r(\Drupal::service('plugin.manager.key.key_type')->getDefinitions());" # Check for examples of key usage in the codebase rg -A 3 "getKeyValue" --type phpLength of output: 574
Action: Ensure hash generation correctly handles keys from various providers
The implementation in
src/core/TawktoGenerator.phpat lines 85–89 calls$keys[$js_api_key]->getKeyValue()directly for generating the hash. However, different key providers may return key values in formats other than a plain string. Please verify that the returned key value is in (or is converted to) the expected format before it’s used inhash_hmac(). Consider one or more of the following:
- Type Validation/Casting: Explicitly validate or cast the key value to a string to ensure compatibility with
hash_hmac().- Unit Tests: Add tests that simulate various key provider outputs to confirm that the hash is generated correctly regardless of the underlying key type.
eug-L
force-pushed
the
feature/secure-mode
branch
from
2323480 to
46500a0
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. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/core/TawktoGenerator.php (5)
531-541: Add a default "Select a key" option in the dropdown.Currently, if no keys are available, the dropdown won't have a default empty option. This could lead to the first key being selected by default, which might not be the intended behavior.
<select name="js_api_key" id="js_api_key"> + <option value="">Select a key</option> <?php foreach (array_keys($keys) as $key) { $selected = 'selected'; if (!is_null($display_opts) && $display_opts->js_api_key !== $key) { $selected = ''; } ?> <option value="<?php echo $key; ?>" <?php echo $selected; ?>><?php echo $key; ?></option> <?php } ?> </select>
259-262: Consider eliminating duplicate code.The same key module retrieval code appears in multiple places. Consider calling this once at the beginning of the method and reusing the values.
function getIframe() { $iframeUrl = $this->getIframeUrl(); $baseUrl = $this->getBaseUrl(); $widget = $this->getWidgetVars(); + $keyModule = $this->getKeyModule(); + $keyModuleInstalled = $keyModule['installed']; + $keys = $keyModule['keys']; // Remove duplicate code at lines 259-262 and 279-282Also applies to: 279-282
82-89: Add additional error handling for key value retrieval.When retrieving the key value for hash generation, consider adding checks to ensure the key value can be successfully retrieved.
if ($secure_mode_enabled) { if (!is_null($js_api_key) && isset($keys[$js_api_key])) { - $hash = hash_hmac('sha256', $usermail, $keys[$js_api_key]->getKeyValue()); + $keyValue = $keys[$js_api_key]->getKeyValue(); + if (!empty($keyValue)) { + $hash = hash_hmac('sha256', $usermail, $keyValue); + } } }
542-542: Specify the key type requirements in the UI.Add guidance about the key type needed (Authentication) directly in the UI, consistent with what's mentioned elsewhere in the code.
- <div>To create a new key, go to <a href="/admin/config/system/keys">/admin/config/system/keys</a></div> + <div>To create a new key, go to <a href="/admin/config/system/keys">/admin/config/system/keys</a>. Create an <b>Authentication</b> key type for the JavaScript API key.</div>
86-88: Consider sanitizing email before using it in hash generation.For added security, consider sanitizing the email before using it for hash generation to ensure consistent hashing.
if ($secure_mode_enabled) { if (!is_null($js_api_key) && isset($keys[$js_api_key])) { - $hash = hash_hmac('sha256', $usermail, $keys[$js_api_key]->getKeyValue()); + $sanitizedEmail = strtolower(trim($usermail)); + $hash = hash_hmac('sha256', $sanitizedEmail, $keys[$js_api_key]->getKeyValue()); } }
🛑 Comments failed to post (1)
src/core/TawktoGenerator.php (1)
513-514:
⚠️ Potential issueDefault option state should match initialization value.
The secure mode is initialized as
FALSEin the widget method (line 57), but the checkbox is checked by default in the form (using$checked = 'checked'). This inconsistency could lead to unexpected behavior.- $checked = 'checked'; - if (!is_null($display_opts) && !$display_opts->secure_mode_enabled) { - $checked = ''; - } + $checked = ''; + if (!is_null($display_opts) && $display_opts->secure_mode_enabled) { + $checked = 'checked'; + }📝 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.$checked = ''; if (!is_null($display_opts) && $display_opts->secure_mode_enabled) { $checked = 'checked'; }
Summary by CodeRabbit