Skip to content

Feature/secure mode #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Feature/secure mode #3

wants to merge 10 commits into from

Conversation

eug-L
Copy link
Collaborator

@eug-L eug-L commented Mar 3, 2025

Secure mode support

  • add js api key input on widget settings
  • add hashing of customer email for visitor recognition

Misc

  • add proper alert for saving widget settings
  • add build script

Summary by CodeRabbit

  • New Features

    • Added a new "Security Options" section in the admin widget settings, allowing users to enter and manage a JavaScript API key with real-time success and error notifications.
    • Enhanced the live chat widget by including additional visitor details for improved security integration.
  • Enhancements

    • Improved validation and feedback during widget configuration updates, including encryption and error handling for the JavaScript API key.
    • Refined interface styling and messaging for a clearer, more user-friendly experience, including new alert notifications.
    • Introduced a new constant for managing visitor sessions securely.
    • Added a new exception class for handling errors related to saving widgets.

Copy link

coderabbitai bot commented Mar 3, 2025

Walkthrough

This update introduces a new interface and additional constants for handling JS API keys. Multiple components are enhanced with encryption and validation logic, including administrative controllers and the Embed block. The changes add new database columns for storing JS API keys and update frontend templates to include a visitor hash. Packaging is streamlined with a new shell script, and various view files now include improved UI elements for security options and user feedback.

Changes

File(s) Change Summary
Api/ConfigInterface.php Added new interface (ConfigInterface) with constant JS_API_KEY_NO_CHANGE.
Block/Embed.php Introduced constant TAWKTO_VISITOR_SESSION, injected an EncryptorInterface, added getVisitorHash method, and updated getCurrentCustomerDetails() to include a customer hash.
Controller/Adminhtml/SaveWidget/Index.php, Controller/Adminhtml/StoreWidget/Index.php Added an $encryptor property, introduced private setJsApiKey method for validation/encryption, and updated response data to include jsApiKey.
build/build-package.sh, composer.json Added a new packaging script and updated the composer "package" script to delegate packaging logic to the shell script.
etc/db_schema.xml Added new columns config_version and js_api_key to the tawk_widget table.
view/adminhtml/templates/selectwidget.phtml, view/adminhtml/web/css/tawk-widget-select.css, view/adminhtml/web/js/tawk-widget-select.js Added a "Security Options" section with input fields, success/failure messages, reorganized CSS rules, and enhanced AJAX response handling.
view/frontend/templates/embed.phtml, view/frontend/web/js/tawk-embed.js Updated the embed template and JS to include a visitor "hash" in the JSON details and assign it to Tawk_API if present.
Exception/SaveWidgetException.php Added new exception class SaveWidgetException for handling widget save errors.

Sequence Diagram(s)

sequenceDiagram
    participant Admin as Admin Controller
    participant Encryptor as EncryptorInterface
    participant Widget as Widget Model
    Admin->>+Admin: Receive SaveWidget request with JS API key
    Admin->>+Admin: Call setJsApiKey(model, jsApiKey)
    alt API key valid and not "no_change"
        Admin->>Encryptor: Encrypt API key
        Encryptor-->>Admin: Return encrypted key
        Admin->>Widget: Update widget with encrypted API key
    else API key is "no_change" or invalid
        Admin->>Admin: Set API key to null / handle error
    end
    Admin-->>-Admin: Return response (success or error)
Loading
sequenceDiagram
    participant Browser as Client Browser
    participant Embed as Embed Block
    participant Encryptor as EncryptorInterface
    Browser->>Embed: Request customer details
    Embed->>Encryptor: Decrypt JS API key (if provided)
    Encryptor-->>Embed: Return decrypted API key or error
    Embed->>Embed: Generate hash from email & API key
    Embed-->>Browser: Return details (name, email, hash)
    Browser->>Browser: Assign visitor.hash to Tawk_API
Loading

Poem

I'm a hopping rabbit, code so bright,
Encrypting keys from morning to night.
With each new line, our widget sings,
Secure and swift with encrypted things.
Packaged neatly, our code takes flight –
A joyful leap in development light!


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (8)
build/build-package.sh (3)

1-2: Shebang and Robust Execution
The script begins correctly with a shebang. Consider adding a set -e immediately after the shebang (e.g., on line 2) to ensure the script exits on any error. This makes the build process more robust.


5-16: Explicit Copy Commands for Build Packaging
The series of cp commands clearly copy the necessary directories and files (such as view, etc, Setup, Model, etc.) to the temporary directory. While this approach is clear, consider refactoring these repetitive commands into a loop or helper function if you plan to extend the list in the future to improve maintainability.


18-18: Zip Packaging Command
The use of the zip command with maximum compression is straightforward and works well for packaging. For enhanced reliability, consider adding error checking or logging to confirm that the zip archive (tawkmagento2.zip) is successfully created.

Controller/Adminhtml/StoreWidget/Index.php (1)

132-136: Security-first approach for jsApiKey.

Returning JS_API_KEY_NO_CHANGE if the key is set helps avoid leaking the real key value. Consider using a trim() check in case of accidental whitespace, but otherwise this is a good practice.

-if (!empty($jsApiKey)) {
+if (!empty(trim($jsApiKey))) {
     $jsApiKey = ConfigInterface::JS_API_KEY_NO_CHANGE;
 }
view/frontend/web/js/tawk-embed.js (1)

19-21: Potential error if visitor is undefined.

Accessing visitor.hash without verifying visitor might cause a TypeError. Consider nesting this inside the existing if (visitor) check.

-if (visitor.hash) {
+if (visitor && visitor.hash) {
     window.Tawk_API.visitor.hash = visitor.hash;
 }
view/adminhtml/web/css/tawk-widget-select.css (1)

170-177: New .options-alert class provides flexible alert styling.

The properties are well-defined for a future notification element. Optionally, consider reusing Magento admin alert classes if you want to align with built-in admin styling.

Block/Embed.php (2)

179-185: Ensure consistent exception handling when decrypting the API key.

The try-catch handles decryption failures gracefully by setting an empty hash. Consider logging the exception or providing user feedback if needed.


193-215: Potential security implications of storing the decrypted key in session.

While this approach promotes better performance by avoiding repeated decryption, it can pose a risk if the session is compromised. Consider storing only transient hashes or using a more secure medium for the decrypted key.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c9a265 and 8d3db1e.

📒 Files selected for processing (12)
  • Api/ConfigInterface.php (1 hunks)
  • Block/Embed.php (5 hunks)
  • Controller/Adminhtml/SaveWidget/Index.php (5 hunks)
  • Controller/Adminhtml/StoreWidget/Index.php (4 hunks)
  • build/build-package.sh (1 hunks)
  • composer.json (1 hunks)
  • etc/db_schema.xml (1 hunks)
  • view/adminhtml/templates/selectwidget.phtml (1 hunks)
  • view/adminhtml/web/css/tawk-widget-select.css (2 hunks)
  • view/adminhtml/web/js/tawk-widget-select.js (5 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
  • view/frontend/web/js/tawk-embed.js (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • Api/ConfigInterface.php
🔇 Additional comments (36)
build/build-package.sh (1)

3-3: Composer Clean Command Verification
The command composer run clean is executed to clean the environment. Please verify that your Composer configuration supports this usage (e.g., that the "clean" script is properly defined) and works as expected across environments.

composer.json (1)

55-55: Simplified Packaging Script Command
Replacing the long inline packaging command with "package": "./build/build-package.sh" greatly simplifies the Composer scripts section and delegates the packaging logic to the dedicated build script. Ensure that ./build/build-package.sh has execute permissions (e.g., via chmod +x ./build/build-package.sh) so that it runs smoothly in all environments.

etc/db_schema.xml (1)

21-21: Appropriate schema addition for JS API key.

Adding this column to store the JavaScript API key is a good addition that supports the secure mode feature. The column is appropriately defined as nullable since it would only be required when secure mode is enabled.

view/adminhtml/templates/selectwidget.phtml (2)

150-160: Clear and informative security options section.

The addition of the Security Options section with a clear explanatory notice about when the JavaScript API key is required is well structured. The password input type is appropriate for sensitive credentials.


164-174:

✅ Verification successful

Good UI feedback for user actions.

Adding success and error notifications provides important feedback to users when saving settings. This enhances user experience significantly.

Consider adding a brief timeout to automatically hide these messages after a few seconds for better UX.


🏁 Script executed:

#!/bin/bash
# Check if JavaScript already handles the timeout for alert messages

grep -A 10 "optionsSuccessMessage\|optionsFailureMessage" view/adminhtml/web/js/tawk-widget-select.js

Length of output: 526


UX Review: Auto-Hide Functionality Confirmed!

Great work on adding clear feedback messages for user actions. The JavaScript in view/adminhtml/web/js/tawk-widget-select.js already implements an auto-hide behavior (using a 3000ms delay and fade-out effect) for both success and failure messages. This means the timeout functionality is already in place, and no further changes are required.

view/adminhtml/web/js/tawk-widget-select.js (5)

46-46: Good implementation of API key value retrieval.

Correctly sets the API key field value from the server response, ensuring the form is properly populated with existing configuration.


66-66: Secure transmission of API key.

Properly includes the JavaScript API key in the form submission data when setting the widget.


68-72: Important success verification added.

Added response validation before executing the callback action. This prevents sending incorrect messages back to the source when the operation fails.


98-98: Consistent API key handling.

Correctly includes the JavaScript API key in the form submission when saving visibility options, mirroring the implementation in the setWidget function.


100-106: Enhanced error handling with user feedback.

The improved response handling now provides appropriate visual feedback based on operation success or failure. The implementation correctly shows success messages and displays error messages with details when operations fail.

I see that the messages are already configured to automatically fade out after 3 seconds (.delay(3000).fadeOut()), which is a good user experience practice.

view/frontend/templates/embed.phtml (1)

37-38:

❓ Verification inconclusive

Important security enhancement with visitor hash.

Adding the hash property to the visitor object is critical for the secure mode implementation. This allows for secure visitor recognition without exposing sensitive information.

The implementation correctly escapes the hash value using escapeJs to prevent XSS vulnerabilities.

Let's confirm that the hash generation is implemented properly on the backend:


🏁 Script executed:

#!/bin/bash
# Check how the hash is generated in the Block/Embed.php file

grep -r -n "getCurrentCustomerDetails" --include="*.php" .
grep -A 30 -B 5 "hash" --include="Block/Embed.php" .

Length of output: 203


Security Verification for Visitor Hash Integration

The template code correctly escapes the visitor hash using escapeJs. However, the earlier backend check using grep ran into issues because the command was targeting a directory rather than the specific file. Please manually verify the hash generation logic in Block/Embed.php by re-running a corrected command such as:

grep -A 30 -B 5 "hash" Block/Embed.php

This will confirm that the hash is generated as expected to securely recognize visitors.

Controller/Adminhtml/StoreWidget/Index.php (4)

23-23: Import statement looks correct.

The new import aligns with usage of the JsonFactory. No issues detected.


27-27: New ConfigInterface import is consistent with usage.

This import is necessary for referencing JS_API_KEY_NO_CHANGE. Looks good.


101-102: Docblock update is clear and accurate.

Adding enableVisitorRecognition: int and jsApiKey: string clarifies return structure.


145-146: Confirmed correct exposure of new fields.

Returning enableVisitorRecognition and jsApiKey is aligned with the docblock.

view/frontend/web/js/tawk-embed.js (1)

6-8: Reformatting of variable declarations is valid.

Splitting variables onto separate lines can improve readability. No functional concerns.

view/adminhtml/web/css/tawk-widget-select.css (4)

1-3: Unified styling for form elements is clear.

Grouping input, textarea, select for consistent styling is good.

Also applies to: 13-13


15-18: Height specification on input, select.

Ensuring a 34px height for consistency. No issues.


20-24: Focus styles are correct.

Highlighting .websiteids with a border color and shadow is user-friendly.


26-28: .websiteids styling.

Setting width to 350px and adding transitions. Looks consistent.

Controller/Adminhtml/SaveWidget/Index.php (8)

23-24: Added imports are aligned with new requirements.

EncryptorInterface and LocalizedException are properly introduced for secure key handling and validation.


29-29: Introduced ConfigInterface import.

This is needed for the JS_API_KEY_NO_CHANGE constant.


68-74: Docblock for $encryptor is clear.

Documenting the encryptor property helps maintain clarity in code.


83-83: Constructor parameter docblock.

The EncryptorInterface parameter is clearly described.


90-91: Successful injection of $encryptor into constructor.

Dependency injection is well-handled.


130-130: $jsApiKey sanitization is appropriate.

Stripping tags and quotes mitigates risks.


153-156: Appropriate error-handling.

Catching exceptions around setJsApiKey() ensures that errors are reported gracefully.


163-185: Secure and validated JS API Key handling.

  • Returning early if JS_API_KEY_NO_CHANGE or empty is a good approach.
  • Enforcing length 40 ensures key integrity, though consider documenting or externalizing this requirement.
  • Encryption with $this->encryptor->encrypt() is a solid practice.
Block/Embed.php (8)

24-25: Good addition of necessary classes for encryption & exception handling.

These imports are essential to enable the new encryption feature and provide descriptive error messages to the user.


32-32: Clear naming convention for new constant.

Defining TAWKTO_JS_API_KEY as a constant enhances readability by providing a clear identifier for the key used in session storage.


83-89: Appropriately documented encryptor property.

The added property and its docblock clearly convey the new Encryptor interface’s purpose. This is good for maintaining clarity in the codebase.


97-97: Docblock improvement for constructor parameter.

Including @param EncryptorInterface $encryptor ensures clarity and consistency in documentation.


105-105: Constructor signature updated to accommodate EncryptorInterface.

This addition is a logical step for enabling secure decryption. It’s consistent with the new encryption feature.


113-116: Successful assignment of encryptor dependency.

Storing $encryptor in a protected property allows reuse across the class without cluttering the constructor.


163-164: Docblock expanded to include 'email' and 'hash' fields.

Documenting additional return values helps maintain clarity for future maintainers. Good job updating it.


188-189: Return array includes the newly introduced hash.

Providing the hashed email in the response upholds security requirements. Ensure the front-end logic properly validates and handles empty hashes.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
Controller/Adminhtml/SaveWidget/Index.php (1)

165-187: Thorough validation and security for JS API key.

The method implements several security best practices:

  • Special case handling for "no change" scenario
  • Proper null handling for empty values
  • Length validation (40 characters)
  • Encryption of sensitive data before storage

One suggestion to consider:

-        if (strlen(trim($jsApiKey)) !== 40) {
+        $trimmedKey = trim($jsApiKey);
+        if (strlen($trimmedKey) !== 40) {
             throw new LocalizedException(__('Invalid API key. Please provide value with 40 characters'));
         }
 
-        return $model->setJsApiKey($this->encryptor->encrypt($jsApiKey));
+        return $model->setJsApiKey($this->encryptor->encrypt($trimmedKey));

This avoids calling trim() twice and ensures we store the trimmed value.

Block/Embed.php (1)

225-241: Secure handling of JS API key decryption.

The method properly:

  1. Validates input before processing
  2. Uses the platform's encryption service
  3. Provides clear error messages

One suggestion:

 private function decryptJsApiKey(string $js_api_key)
 {
     if (empty($js_api_key)) {
         throw new LocalizedException(__('JS API key is empty'));
     }

     $key = $this->encryptor->decrypt($js_api_key);
-
     return $key;
 }

The blank line before the return statement can be removed for consistency.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 8d3db1e and 63dd82e.

📒 Files selected for processing (4)
  • Block/Embed.php (5 hunks)
  • Controller/Adminhtml/SaveWidget/Index.php (5 hunks)
  • etc/db_schema.xml (2 hunks)
  • view/frontend/web/js/tawk-embed.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • view/frontend/web/js/tawk-embed.js
🔇 Additional comments (10)
Controller/Adminhtml/SaveWidget/Index.php (4)

23-29: Good use of encryption and exception handling for API key.

The inclusion of EncryptorInterface and LocalizedException is appropriate for managing the secure JS API key feature. The ConfigInterface import will be used for constant reference in the validation logic.


68-74: Well-documented encryptor property.

The property documentation follows Magento's documentation standards with proper formatting and type hints.


130-130: Proper sanitization of API key input.

The JS API key is properly sanitized using the helper method, consistent with how other parameters are handled in this controller.


152-159: Good error handling and version tracking.

The code correctly:

  1. Wraps the API key handling in a try-catch block
  2. Returns meaningful error responses
  3. Increments the configuration version to ensure caching consistency

This approach ensures robust error handling when dealing with API key operations.

etc/db_schema.xml (2)

8-8: Good addition of config_version column.

The config_version column is properly defined as an unsigned integer with a default value of 0. This is important for tracking configuration changes and will help with caching invalidation for the visitor hash implementation.


22-22: Appropriate definition for the JS API key column.

The js_api_key column is correctly defined as:

  • Text type (appropriate for storing encrypted data)
  • Nullable (since not all installations will use secure mode)
  • With a clear comment describing its purpose

This definition aligns well with the new secure mode feature.

Block/Embed.php (4)

32-32: Good use of constants for session keys.

Using a class constant for the session key name follows best practices by centralizing the definition and avoiding string literals throughout the code.


83-116: Proper dependency injection for the encryptor service.

The code correctly:

  1. Declares the encryptor property with proper documentation
  2. Updates the constructor signature to accept the dependency
  3. Sets the property in the constructor

This follows Magento 2's dependency injection best practices.


162-185: Good implementation of visitor hash in customer details.

The method now includes the visitor hash in the returned data structure while maintaining backward compatibility with the existing interface.


188-223: Well-implemented visitor hash generation with caching.

The getVisitorHash method:

  1. Checks for an existing valid hash in the session first (optimization)
  2. Validates using both email and configuration version to ensure freshness
  3. Properly handles exceptions during key decryption
  4. Uses a secure hashing algorithm (HMAC-SHA256)
  5. Stores results in session for performance

This implementation follows security best practices for generating visitor identification hashes.

Block/Embed.php Outdated
}

try {
$jsApiKey = $this->decryptJsApiKey($this->model->getJsApiKey());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if you even have a key before attempting to de-crypt

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
build/build-package.sh (2)

5-17: Robustness of Copy Commands

The series of cp -r commands are used to copy directories and files into the temporary packaging directory. It might be worthwhile to add checks (or rely on the aforementioned set -e) to ensure that all source directories and files exist before attempting to copy them. This can help prevent the script from failing unexpectedly if any of these paths are missing.


19-19: Packaging Command Enhancements

The packaging command (cd ./tmp && zip -9 -rq ./tawkmagento2.zip ./tawkmagento2) is implemented efficiently using maximum compression in quiet mode. For additional robustness, consider verifying that the zip archive was created successfully. Additionally, you might want to clean up the temporary directory once the packaging is complete.

Controller/Adminhtml/SaveWidget/Index.php (1)

170-194: Thorough implementation of the JS API key setting logic.

The setJsApiKey method has:

  1. Appropriate handling of unchanged or empty key values
  2. Proper validation of key length
  3. Key trimming before validation and encryption
  4. Appropriate error handling

Consider improving the error message for invalid API key - it currently doesn't tell users what's expected.

-            throw new SaveWidgetException(__('Invalid API key'));
+            throw new SaveWidgetException(__('Invalid API key: must be exactly 40 characters long'));
Block/Embed.php (1)

193-230: Well-implemented hash generation with caching.

The hash generation logic:

  1. Uses session caching to avoid repeated decryption operations
  2. Validates the session data against the current email and config version
  3. Checks for the presence of an encrypted key before attempting to decrypt
  4. Uses a secure hashing algorithm (HMAC SHA256)
  5. Stores the computed hash in the session for reuse

Consider adding a check to verify the decryption was successful before using the key for hashing.

        $jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);

+       if (empty($jsApiKey)) {
+           return null;
+       }
        
        $hash = hash_hmac('sha256', $email, $jsApiKey);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 63dd82e and e6e3ea2.

📒 Files selected for processing (6)
  • Block/Embed.php (5 hunks)
  • Controller/Adminhtml/SaveWidget/Index.php (5 hunks)
  • Exception/SaveWidgetException.php (1 hunks)
  • build/build-package.sh (1 hunks)
  • view/adminhtml/web/js/tawk-widget-select.js (5 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • view/frontend/templates/embed.phtml
  • view/adminhtml/web/js/tawk-widget-select.js
🧰 Additional context used
🧬 Code Definitions (2)
Controller/Adminhtml/SaveWidget/Index.php (3)
Model/Widget.php (1) (1)
  • Widget (24-46)
Helper/StringUtil.php (2) (2)
  • StringUtil (21-33)
  • stripTagsandQuotes (29-32)
Exception/SaveWidgetException.php (1) (1)
  • SaveWidgetException (23-25)
Block/Embed.php (2)
Model/Widget.php (1) (1)
  • Widget (24-46)
Controller/Adminhtml/SaveWidget/Index.php (1) (1)
  • __construct (86-101)
🔇 Additional comments (10)
build/build-package.sh (1)

3-3: Composer Clean Command Verification

The command composer run clean assumes that a composer script named clean is defined in your composer.json. Please verify that this script is configured correctly to avoid runtime errors.

Exception/SaveWidgetException.php (1)

19-25: Well-structured custom exception class.

Creating a specific exception type for widget saving errors is good practice. This allows for more precise error handling in the controller.

Controller/Adminhtml/SaveWidget/Index.php (5)

23-30: Good addition of required imports.

The new imports support the encryption functionality and error handling for the secure mode feature.


69-100: Property and constructor parameter documentation is complete.

The encryptor property is well-documented with proper PHPDoc.


131-132: Appropriate parameter sanitization.

The JS API key is properly sanitized using the stripTagsandQuotes method.


153-161: Good error handling structure.

The try-catch block provides appropriate error handling with specific responses for SaveWidgetException vs. generic LocalizedExceptions.


163-163: Good practice: incrementing config version.

Incrementing the configuration version when changes are made ensures that cached data will be refreshed appropriately.

Block/Embed.php (3)

32-32: Clear constant declaration for session key.

Using a constant for the session key name is good practice and improves code maintainability.


83-116: Well-structured encryptor integration.

The encryptor dependency is properly added to the class with complete documentation and constructor injection.


167-191: Good implementation of visitor hash support.

The method correctly:

  1. Respects the feature toggle (enableVisitorRecognition)
  2. Checks for logged-in status
  3. Tries to generate a hash with proper error handling
  4. Returns the hash alongside the user details

Comment on lines +1 to +2
#!/bin/sh

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Proper Shebang & Error Handling Consideration

The script begins with a proper shebang (#!/bin/sh). Consider adding error handling options (e.g., set -e, set -u, and/or set -o pipefail) immediately after the shebang to ensure the script exits on failure, which can help avoid silent failures in the build process.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
Block/Embed.php (1)

180-184: Error handling could be improved.

The current error handling catches exceptions but only logs them using error_log(). Consider using the logger service that's already injected into this class.

- try {
-     $hash = $this->getVisitorHash($customerSession->getEmail());
- } catch (LocalizedException $e) {
-     error_log($e->getMessage());
- }
+ try {
+     $hash = $this->getVisitorHash($customerSession->getEmail());
+ } catch (LocalizedException $e) {
+     $this->logger->error('Failed to generate visitor hash: ' . $e->getMessage());
+ }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between e6e3ea2 and 7bb2744.

📒 Files selected for processing (1)
  • Block/Embed.php (5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Block/Embed.php (2)
Model/Widget.php (1) (1)
  • Widget (24-46)
Controller/Adminhtml/SaveWidget/Index.php (1) (1)
  • __construct (86-101)
🔇 Additional comments (5)
Block/Embed.php (5)

209-217: Good implementation of session caching for the visitor hash.

The code correctly checks if the hash already exists in the session and validates that the email and config version match before reusing it. This is an efficient approach that prevents unnecessary recalculations.


203-205: Proper validation before decryption.

Good practice to check if the encryptedJsApiKey is empty before attempting to decrypt it, addressing a previous review comment.


221-227: Secure implementation of visitor hash.

The implementation uses hash_hmac with SHA-256, which is a secure approach for generating the visitor hash. The session storage also correctly includes the email and config version to invalidate the hash if these values change.


223-227: Avoid storing sensitive data in session.

Based on a previous review comment, consider storing only the hash in the session rather than including the email. The email could be compared directly from the customer session when needed.

- $this->modelSessionFactory->setData(self::TAWKTO_VISITOR_SESSION, [
-     'hash' => $hash,
-     'email' => $email,
-     'config_version' => $configVersion,
- ]);
+ $this->modelSessionFactory->setData(self::TAWKTO_VISITOR_SESSION, [
+     'hash' => $hash,
+     'config_version' => $configVersion,
+ ]);

Then update the session check to:

- if (isset($currentSession['hash']) &&
-     $currentSession['email'] === $email &&
-     $currentSession['config_version'] === $configVersion) {
+ if (isset($currentSession['hash']) &&
+     $currentSession['config_version'] === $configVersion) {

187-190: Good implementation of the return structure.

The return structure now includes the hash alongside the customer's name and email, which aligns with the secure mode requirements mentioned in the PR objectives.

Comment on lines +219 to +221
$jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);

$hash = hash_hmac('sha256', $email, $jsApiKey);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Additional error handling needed for decryption.

The decrypt operation could fail and throw an exception. Consider adding specific error handling for the decryption operation.

- $jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);
+ try {
+     $jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);
+ } catch (\Exception $e) {
+     $this->logger->error('Failed to decrypt JS API key: ' . $e->getMessage());
+     return null;
+ }
📝 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
$jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);
$hash = hash_hmac('sha256', $email, $jsApiKey);
try {
$jsApiKey = $this->encryptor->decrypt($encryptedJsApiKey);
} catch (\Exception $e) {
$this->logger->error('Failed to decrypt JS API key: ' . $e->getMessage());
return null;
}
$hash = hash_hmac('sha256', $email, $jsApiKey);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants