Skip to content
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

Technical review issues #2

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

Technical review issues #2

wants to merge 11 commits into from

Conversation

eug-L
Copy link
Collaborator

@eug-L eug-L commented Oct 30, 2024

  • Removed obsolete InstallSchema.php, UpgradeSchema.php and UpgradeData.php
  • Added db_schema.xml and data patch script
  • Fixed htmlspecialchars and parse_url warnings

Tested patch data script for 1.5.2->1.6.1

Summary by CodeRabbit

Release Notes

  • New Features

    • Updated to PHP version 8.2.20 for improved performance and security.
    • Introduced a new database schema for the tawk_widget table, enhancing widget and store connectivity.
  • Improvements

    • Enhanced URL handling and escaping in templates for better security against XSS vulnerabilities.
    • Updated coding standards and dependencies for improved code quality.
  • Bug Fixes

    • Removed deprecated methods and classes to streamline functionality.
  • Chores

    • Updated development dependencies in composer.json for better compatibility and performance.
    • Added exclusion patterns in coding standards configuration for more efficient analysis.

Copy link

coderabbitai bot commented Oct 30, 2024

Walkthrough

The changes in this pull request include updates to various files related to the Tawk Widget module in a Magento environment. Key modifications involve the introduction of a new PHP version specification, updates to coding standards exclusions, and enhancements to the SelectWidgetBlock and Embed classes by adding an Escaper dependency. Additionally, several files related to database schema management have been removed or restructured, including the introduction of a new database schema definition in etc/db_schema.xml. The composer.json file has also been updated to reflect changes in dependency versions and configurations.

Changes

File Change Summary
.php-version Version updated to 8.2.20.
.phpcs.xml Exclude pattern added: <exclude-pattern>tmp</exclude-pattern>; obsolete rules removed.
Block/Adminhtml/SelectWidgetBlock.php Property protected $escaper added; constructor signature updated to include Escaper.
Block/Embed.php Property protected $escaper added; constructor signature updated to include Escaper.
Setup/InstallSchema.php Class and method removed; functionality for setting up the database schema eliminated.
Setup/Patch/Data/UpdateExcludeIncludeUrl.php Class renamed from UpgradeData to UpdateExcludeIncludeUrl; implements DataPatchInterface; properties visibility updated; new methods added for patching.
Setup/UpgradeSchema.php Class and method removed; functionality for managing database schema upgrades eliminated.
composer.json Dependency versions updated for squizlabs/php_codesniffer and magento/magento-coding-standard; script paths modified; new plugin allowed.
etc/db_schema.xml New table tawk_widget defined with various columns for store and widget connections.
view/adminhtml/templates/selectwidget.phtml URL escaping updated to use $escaper->escapeUrl() and $escaper->escapeJs().
view/frontend/templates/embed.phtml URL and customer detail escaping updated to use escaper->escapeUrl() and escaper->escapeJs().

Poem

🐇 In the garden where we play,
New changes hop along the way.
With escapers by our side,
Safe and sound, we take our stride.
PHP's version shines so bright,
Tawk's widgets now take flight! 🌼


🪧 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: 6

🧹 Outside diff range and nitpick comments (7)
.phpcs.xml (1)

Line range hint 19-20: Remove the TODO comment as it's being addressed in this PR.

Since this PR is implementing the migration to Declarative Schema (removing InstallSchema.php, UpgradeSchema.php, and adding db_schema.xml), this TODO comment is no longer needed.

-		<!-- TODO: Upgrade obsolete migration scripts to Declarative Schema and Data Patches -->
		<exclude name="Magento2.Legacy.InstallUpgrade" />
etc/db_schema.xml (2)

1-3: Add charset and improve table description.

While the table definition is correct, consider adding charset/collation specifications and a more descriptive comment:

-<table name="tawk_widget" resource="default" engine="innodb" comment="Tawk Widget table that makes connection between stores and widgets">
+<table name="tawk_widget" resource="default" engine="innodb" charset="utf8mb4" collation="utf8mb4_unicode_ci" 
+    comment="Stores the configuration and mapping between store views and Tawk.to chat widgets">

15-15: Optimize visitor recognition flag.

Consider using a more appropriate data type for the boolean flag:

-<column name="enable_visitor_recognition" xsi:type="int" nullable="false" default="1" 
-    comment="Enable visitor recognition feature"/>
+<column name="enable_visitor_recognition" xsi:type="smallint" unsigned="true" nullable="false" default="1" 
+    comment="Flag to enable automatic visitor recognition through browser cookies"/>
view/frontend/templates/embed.phtml (1)

Line range hint 19-42: Consider adding error handling for the embed URL

While the current implementation works, it might benefit from additional error handling to gracefully handle cases where:

  1. The embed URL is invalid or empty
  2. The customer details are malformed

Consider adding a validation layer:

<?php if ($block->getEmbedUrl()): ?>
    <script type="text/x-magento-init">
    // ... existing code ...
    </script>
<?php else: ?>
    <!-- Add appropriate fallback or error message -->
<?php endif; ?>
Block/Adminhtml/SelectWidgetBlock.php (1)

Line range hint 83-93: Add escaping to prevent potential XSS vulnerabilities.

Several methods construct URLs and HTML without proper escaping:

  1. mainurl(): Unescaped URL concatenation
  2. getWebSiteoptions(): Direct HTML construction
  3. getIframeUrl(): URL parameter concatenation

Apply these security improvements:

 public function mainurl()
 {
     $protocol = 'http';
     if ($this->request->isSecure()) {
         $protocol = 'https';
     }
-    return $protocol . "://" . $this->request->getServer('HTTP_HOST');
+    return $protocol . "://" . $this->escaper->escapeUrl($this->request->getServer('HTTP_HOST'));
 }

 public function getWebSiteoptions()
 {
     $sdstr = '';
     $websites = $this->_storeManager->getWebsites();
     foreach ($websites as $website) {
-        $sdstr .= '<option value="'.$website->getId().'">'.$website->getName().'</option>';
+        $sdstr .= '<option value="' . $this->escaper->escapeHtmlAttr($website->getId()) . '">' 
+            . $this->escaper->escapeHtml($website->getName()) . '</option>';
     }
     return $sdstr;
 }

 public function getIframeUrl()
 {
-    return $this->getBaseUrl().'/generic/widgets'
-        .'?currentWidgetId=&currentPageId=&transparentBackground=1'
-        .'&pltf=magento&pltfv=2&parentDomain='.$this->mainurl();
+    return $this->escaper->escapeUrl(
+        $this->getBaseUrl() . '/generic/widgets'
+        . '?currentWidgetId=&currentPageId=&transparentBackground=1'
+        . '&pltf=magento&pltfv=2&parentDomain=' . $this->mainurl()
+    );
 }

Also applies to: 97-109, 110-120

Block/Embed.php (1)

Line range hint 174-187: Security: Improve URL handling in _toHtml method.

The current URL construction using raw server variables (HTTP_HOST, REQUEST_URI) is vulnerable to HTTP Host header injection. Consider:

  1. Using Magento's UrlInterface for URL handling
  2. Validating and sanitizing URL components before use

Example refactor:

use Magento\Framework\UrlInterface;

protected $urlBuilder;

public function __construct(
    // ... other params ...
    UrlInterface $urlBuilder
) {
    $this->urlBuilder = $urlBuilder;
}

// In _toHtml:
$current_url = $this->urlBuilder->getCurrentUrl();

Also applies to: 205-218

Setup/Patch/Data/UpdateExcludeIncludeUrl.php (1)

Line range hint 98-149: Optimize the addWildcardToPatternList Method

The addWildcardToPatternList method contains nested conditionals and string manipulations that might affect readability and maintainability. Consider refactoring the method to simplify the logic, possibly by:

  • Breaking down complex conditionals into smaller, well-named methods.
  • Utilizing array functions like array_filter and array_map to process the pattern list.

Refactoring the method could enhance clarity and reduce potential bugs in pattern processing.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6431bbf and bdc341d.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • .php-version (1 hunks)
  • .phpcs.xml (1 hunks)
  • Block/Adminhtml/SelectWidgetBlock.php (2 hunks)
  • Block/Embed.php (4 hunks)
  • Setup/InstallSchema.php (0 hunks)
  • Setup/Patch/Data/UpdateExcludeIncludeUrl.php (2 hunks)
  • Setup/UpgradeSchema.php (0 hunks)
  • composer.json (2 hunks)
  • etc/db_schema.xml (1 hunks)
  • view/adminhtml/templates/selectwidget.phtml (2 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
💤 Files with no reviewable changes (2)
  • Setup/InstallSchema.php
  • Setup/UpgradeSchema.php
✅ Files skipped from review due to trivial changes (1)
  • .php-version
🔇 Additional comments (20)
.phpcs.xml (1)

8-8: LGTM!

The addition of the tmp directory to the exclusion patterns is appropriate and follows the existing pattern of excluding non-source directories.

etc/db_schema.xml (1)

4-7: 🛠️ Refactor suggestion

Add index for store lookups.

Consider adding an index on for_store_id to improve query performance when filtering widgets by store:

 <constraint xsi:type="primary" referenceId="PRIMARY">
     <column name="id"/>
 </constraint>
+<index referenceId="TAWK_WIDGET_STORE_ID" indexType="btree">
+    <column name="for_store_id"/>
+</index>

Likely invalid or redundant comment.

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

36-37: Verify PII handling requirements

The customer's PII (name and email) is properly escaped for JavaScript context. However, please ensure that:

  1. The customer has consented to share this information with Tawk.to
  2. This data sharing complies with your privacy policy and GDPR requirements
  3. No PII is being logged in server logs

Let's check for any potential PII logging:

✅ Verification successful

PII handling appears to be properly implemented with appropriate controls

The verification shows that:

  1. Customer details are only shared when enableVisitorRecognition is explicitly enabled (opt-in)
  2. Data is only collected for logged-in customers, not anonymous visitors
  3. No logging of PII was found in the codebase
  4. The implementation uses proper escaping and secure data handling
  5. The data sharing is transparent, as it's part of the tawk.to chat widget functionality that's visible to users

Recommendations for compliance:

  • Ensure your privacy policy mentions the data sharing with tawk.to
  • Document in your GDPR records that customer name and email are shared with tawk.to when chat widget is enabled
  • Consider adding a chat widget privacy notice to inform users about data handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for logging statements that might contain customer details
rg -g '!{.git,vendor}' -i '(log|debug|info|error|warning).*customer.*detail'

# Check if there are any other files accessing these customer details
ast-grep --pattern 'getCurrentCustomerDetails'

Length of output: 125


Script:

#!/bin/bash
# Let's try a broader search for logging patterns with customer data
rg -g '!{.git,vendor}' -i '(log|debug|info|error|warning).*customer'

# Search for files that handle customer data
rg -g '!{.git,vendor}' -i 'customer.*details'

# Look for any privacy or GDPR related configurations
fd -g '*privacy*' -g '*gdpr*' -g '*consent*'

# Check for tawk.to integration files to understand data handling
fd -g '*tawk*'

Length of output: 1012


Script:

#!/bin/bash
# Let's examine the Block/Embed.php to see how customer details are handled
cat Block/Embed.php

# Check the tawk embed JS file for any data handling
cat view/frontend/web/js/tawk-embed.js

# Look for any privacy policy or terms in the README
cat README.md

Length of output: 10970

composer.json (3)

46-49: LGTM! Appropriate use of Magento's PHP compatibility rules.

The switch to magento/php-compatibility-fork is a good practice for Magento 2 modules, as it provides more accurate PHP compatibility checks specific to the Magento ecosystem.


66-67: LGTM! Required plugin configuration for PHP_CodeSniffer.

The addition of dealerdirect/phpcodesniffer-composer-installer to allowed plugins is necessary for proper integration of the updated coding standards tools.


38-39: LGTM! Verify compatibility with the project's PHP version.

The updates to newer versions of coding standards tools are good improvements. However, let's verify the compatibility requirements.

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

Line range hint 35-37: Verify that getWebSiteoptions() properly escapes its output.

The template uses /* @noEscape */ for the website options output. While this is a valid Magento pattern for pre-escaped content, we should verify that getWebSiteoptions() implements proper escaping internally.

Let's check the implementation:

#!/bin/bash
# Description: Verify escaping in getWebSiteoptions method
ast-grep --pattern 'function getWebSiteoptions() {
  $$$
}'

23-29: LGTM! Proper context-aware escaping implemented.

The changes correctly implement Magento's recommended escaping practices by using the dedicated escaper service with appropriate context-specific methods:

  • escapeUrl() for URLs
  • escapeJs() for JavaScript values

Let's verify that the escaper dependency is properly injected in the block class:

✅ Verification successful

Escaper dependency properly implemented and used correctly

The verification confirms that:

  • The block class correctly declares the Escaper dependency in its constructor
  • The escaper instance is properly injected and stored in the public $escaper property
  • The class uses the correct namespace import use Magento\Framework\Escaper

The template's usage of $block->escaper->escapeUrl() and $block->escaper->escapeJs() is therefore correct and secure.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify escaper dependency in SelectWidgetBlock
ast-grep --pattern 'class SelectWidgetBlock {
  $$$
  public function __construct($_, $_, \Magento\Framework\Escaper $escaper, $_) {
    $$$
  }
  $$$
}'

Length of output: 274


Script:

#!/bin/bash
# Let's try a different approach to find the block class file first
fd -t f "SelectWidget.*php"

# Then we can search for escaper usage patterns
rg -l "Magento\\Framework\\Escaper" 

# Also check for class definition and constructor
rg "class\s+SelectWidget.*extends" -A 10

Length of output: 866


Script:

#!/bin/bash
# Let's check the content of the block class file
cat Block/Adminhtml/SelectWidgetBlock.php

# Also search for escaper usage with proper escaping
rg "\\\\Magento\\\\Framework\\\\Escaper" 

# And check Template class inheritance which might provide escaper
rg "class Template"

Length of output: 6988

Block/Adminhtml/SelectWidgetBlock.php (2)

67-80: LGTM! Constructor changes follow best practices.

The constructor properly implements dependency injection for the Escaper service and maintains correct initialization order.


21-21: Verify template usage and similar classes.

Since we're adding escaping functionality, we should verify:

  1. Template files using this block are updated to use proper escaping
  2. Similar admin blocks have consistent security measures
✅ Verification successful

Let me check the layout file and analyze the block usage further.


Let me check the template file that's using this block to verify the escaping usage.


The escaping implementation is properly used and no issues found

The template file correctly uses the escaper instance for all dynamic content:

  • URLs are properly escaped using escapeUrl()
  • JavaScript content is escaped using escapeJs()
  • HTML content is escaped using escapeHtml()
  • The only unescaped content is explicitly marked with @noEscape annotation and contains pre-generated HTML options

Since this is the only admin block in the module and the escaping is properly implemented in both the block class and template, no further verification is needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check template files using this block
echo "Checking template usage..."
rg -l "SelectWidgetBlock" view/

# Check other admin blocks that might need similar security improvements
echo "Checking similar admin blocks..."
fd -e php -t f . Block/Adminhtml/

Length of output: 295


Script:

#!/bin/bash
# Check the layout file content to understand block usage
echo "Checking layout file content..."
cat view/adminhtml/layout/widget_selectwidget_index.xml

# Check if the block is used in any template files
echo -e "\nChecking template references..."
rg -l "SelectWidgetBlock" view/adminhtml/templates/

# Check the block class implementation for context
echo -e "\nChecking block implementation..."
cat Block/Adminhtml/SelectWidgetBlock.php

Length of output: 8504


Script:

#!/bin/bash
# Check the template file content
echo "Checking template content..."
cat view/adminhtml/templates/selectwidget.phtml

# Check if there are any other similar admin blocks that might need escaping
echo -e "\nChecking other admin blocks for potential escaping needs..."
rg -l "extends Template" Block/Adminhtml/

Length of output: 7062

Block/Embed.php (2)

Line range hint 84-102: LGTM! Constructor changes follow Magento best practices.

The Escaper dependency is properly injected and initialized, following Magento's dependency injection pattern.


113-114: Consider additional URL validation.

While the pageId and widgetId are properly escaped, consider adding validation for:

  1. The base URL protocol (currently hardcoded as 'https')
  2. The format of pageId and widgetId before escaping
Setup/Patch/Data/UpdateExcludeIncludeUrl.php (8)

15-15: Proper Transition to Data Patch Mechanism

Updating the class to implement DataPatchInterface and renaming it to UpdateExcludeIncludeUrl aligns with Magento's recommended practices for module data patches in newer versions. This enhances the maintainability and future compatibility of the code.


31-36: Addition of ModuleDataSetup Property and Dependency

Introducing the $_moduleDataSetup property and injecting ModuleDataSetupInterface is essential for the patch to interact with the database setup. This is a necessary change for the data patch mechanism.


43-52: Correctly Injected Dependencies in Constructor

The constructor now includes all necessary dependencies, including the new ModuleDataSetupInterface. This ensures that the class has all required resources to function properly.


56-74: Implementation of Required DataPatchInterface Methods

Adding the getAliases() and getDependencies() methods fulfills the contract of the DataPatchInterface. Returning empty arrays is appropriate if there are no aliases or dependencies for this patch.


78-96: Migration of Upgrade Logic to apply() Method

Moving the upgrade logic into the apply() method conforms to the data patch pattern. Starting and ending the setup process with $this->_moduleDataSetup->getConnection()->startSetup(); and endSetup(); ensures that the database changes are correctly managed within a transaction.


110-117: Use of Laminas URI for Robust URL Parsing

Replacing parse_url with UriFactory::factory enhances the robustness of URL parsing and avoids deprecated or less secure functions. This change improves the reliability of extracting host and port information from URLs.


88-93: Ensure Data Integrity When Saving Updated Patterns

After updating the excludeUrl and includeUrl with wildcards, the modified data is saved back to the item. Ensure that this process correctly handles all possible data scenarios, such as empty or malformed URLs, to prevent potential data corruption.

Consider adding validation or exception handling to manage unexpected data formats.


22-29: ⚠️ Potential issue

Improved Encapsulation by Changing Property Visibility

Changing the visibility of $_modelWidgetFactory and $_modelStoreManager from protected to private enhances encapsulation and restricts access to these properties within the class. Ensure that no child classes depend on these properties; otherwise, this change might introduce issues.

Please verify if any subclasses extend this class and rely on these properties. Run the following script to check for subclasses:

✅ Verification successful

Property Visibility Change is Safe

The change from protected to private visibility for $_modelWidgetFactory and $_modelStoreManager properties is safe. The search results confirm:

  • No classes extend UpdateExcludeIncludeUrl
  • The properties are only used within the same class
  • All property accesses are through $this within the class methods
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find subclasses that might be affected by the visibility change.

# Test: Search for classes extending UpdateExcludeIncludeUrl.
# Expected Result: No subclasses found.

rg -t php 'class\s+\w+\s+extends\s+\\?Tawk\\Widget\\Setup\\Patch\\Data\\UpdateExcludeIncludeUrl'

Length of output: 98


Script:

#!/bin/bash
# Let's try a more comprehensive search approach

# First, let's verify the exact namespace and class name
rg -t php "namespace.*Setup.Patch.Data.*UpdateExcludeIncludeUrl" -B1 -A1

# Then search for any class that extends UpdateExcludeIncludeUrl without namespace restrictions
rg -t php "class.*extends.*UpdateExcludeIncludeUrl"

# Also search for potential usage of these properties in any PHP files
rg -t php "(_modelWidgetFactory|_modelStoreManager)"

Length of output: 983

etc/db_schema.xml Outdated Show resolved Hide resolved
etc/db_schema.xml Outdated Show resolved Hide resolved
view/frontend/templates/embed.phtml Outdated Show resolved Hide resolved
view/adminhtml/templates/selectwidget.phtml Outdated Show resolved Hide resolved
Block/Adminhtml/SelectWidgetBlock.php Outdated Show resolved Hide resolved
Block/Embed.php Show resolved Hide resolved
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

🧹 Outside diff range and nitpick comments (1)
etc/db_schema.xml (1)

13-14: Clarify the URL pattern separator in comments.

The comments mention "Comma-separated list" but in practice, URL patterns in text fields are often stored as newline-separated lists for better readability. Please verify the expected separator and update the comments accordingly.

-			comment="Comma-separated list of url patterns where widget should not be displayed"/>
+			comment="Newline-separated list of URL patterns where widget should not be displayed"/>
-			comment="Comma-separated list of url patterns where widget should be displayed"/>
+			comment="Newline-separated list of URL patterns where widget should be displayed"/>

Also applies to: 17-18

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bdc341d and c4c5b2a.

📒 Files selected for processing (4)
  • .phpcs.xml (1 hunks)
  • etc/db_schema.xml (1 hunks)
  • view/adminhtml/templates/selectwidget.phtml (2 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .phpcs.xml
  • view/adminhtml/templates/selectwidget.phtml
  • view/frontend/templates/embed.phtml
🔇 Additional comments (5)
etc/db_schema.xml (5)

1-2: LGTM! Schema declaration follows Magento standards.


3-3: LGTM! Table definition is well-structured.


4-7: LGTM! Primary key is properly defined.


8-10: Consider making page_id and widget_id required fields.

These columns appear to be essential for the widget's functionality. If a widget record without these IDs would be invalid, consider making them nullable="false".

Let's check if these fields are always populated in the existing implementation:

#!/bin/bash
# Search for places where these fields are set to understand if they can be null
rg -l "page_id|widget_id" --type php

The previous review comment about adding a foreign key constraint for for_store_id is still valid.


19-20: LGTM! Feature flag is properly defined.

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

🧹 Outside diff range and nitpick comments (1)
Block/Embed.php (1)

241-260: Consider simplifying method names.

While the methods are well-documented, their names are redundant with the "escaper" prefix. Since they're already in a block class, we could simplify them to just escapeUrl and escapeJs.

Consider this improvement:

-    public function escaperEscapeUrl($url)
+    public function escapeUrl($url)
     {
         return $this->escaper->escapeUrl($url);
     }

-    public function escaperEscapeJs($js)
+    public function escapeJs($js)
     {
         return $this->escaper->escapeJs($js);
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c4c5b2a and 2455124.

📒 Files selected for processing (4)
  • Block/Adminhtml/SelectWidgetBlock.php (3 hunks)
  • Block/Embed.php (5 hunks)
  • view/adminhtml/templates/selectwidget.phtml (2 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • Block/Adminhtml/SelectWidgetBlock.php
  • view/adminhtml/templates/selectwidget.phtml
  • view/frontend/templates/embed.phtml
🔇 Additional comments (2)
Block/Embed.php (2)

22-22: LGTM! Well-documented Escaper dependency.

The Escaper dependency is properly imported and the property is correctly declared as protected with comprehensive PHPDoc.

Also applies to: 72-77


Line range hint 84-102: LGTM! Constructor properly handles Escaper dependency.

The constructor follows Magento's dependency injection best practices with proper type hinting and initialization.

Block/Embed.php Outdated Show resolved Hide resolved
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: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2455124 and e55aee6.

📒 Files selected for processing (1)
  • Block/Embed.php (5 hunks)
🔇 Additional comments (1)
Block/Embed.php (1)

22-22: LGTM: Clean dependency injection implementation.

The Escaper dependency is properly injected and follows Magento's best practices with appropriate type hints and documentation.

Also applies to: 72-77, 85-85, 92-92, 102-102

Block/Embed.php Show resolved Hide resolved
Block/Embed.php Outdated Show resolved Hide resolved
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

🧹 Outside diff range and nitpick comments (2)
Block/Adminhtml/SelectWidgetBlock.php (2)

Line range hint 77-86: Security: Add proper escaping to URL and HTML generation.

Several methods are generating URLs and HTML without proper escaping:

  1. mainurl(): Concatenates HTTP host without escaping
  2. getWebSiteoptions(): Builds HTML options without escaping values
  3. getIframeUrl(): Concatenates URLs without escaping

Apply these security improvements:

     public function mainurl()
     {
         $protocol = 'http';
         if ($this->request->isSecure()) {
             $protocol = 'https';
         }
-        return $protocol . "://" . $this->request->getServer('HTTP_HOST');
+        return $protocol . "://" . $this->escaper->escapeUrl($this->request->getServer('HTTP_HOST'));
     }

     public function getWebSiteoptions()
     {
         $sdstr = '';
 
         $websites = $this->_storeManager->getWebsites();
         foreach ($websites as $website) {
-            $sdstr .= '<option value="'.$website->getId().'">'.$website->getName().'</option>';
+            $sdstr .= '<option value="' . $this->escaper->escapeHtmlAttr($website->getId()) . '">'
+                   . $this->escaper->escapeHtml($website->getName()) . '</option>';
         }
         return $sdstr;
     }

     public function getIframeUrl()
     {
-        return $this->getBaseUrl().'/generic/widgets'
+        return $this->escaper->escapeUrl($this->getBaseUrl().'/generic/widgets'
             .'?currentWidgetId=&currentPageId=&transparentBackground=1'
-            .'&pltf=magento&pltfv=2&parentDomain='.$this->mainurl();
+            .'&pltf=magento&pltfv=2&parentDomain='.$this->mainurl());
     }

Also applies to: 90-101, 105-113


Line range hint 1-248: Enhance type safety with return type declarations.

The code is well-documented and organized, but could benefit from explicit return type declarations for better type safety and IDE support.

Consider adding return type declarations to methods:

-    public function mainurl()
+    public function mainurl(): string

-    public function getWebSiteoptions()
+    public function getWebSiteoptions(): string

-    public function getIframeUrl()
+    public function getIframeUrl(): string

-    public function getBaseUrl()
+    public function getBaseUrl(): string

-    public function getHierarchy()
+    public function getHierarchy(): array

-    public function getCollection()
+    public function getCollection(): \Magento\Framework\Data\Collection

-    public function getFormAction()
+    public function getFormAction(): string

-    public function getRemoveUrl()
+    public function getRemoveUrl(): string

-    private function parseGroups($groups)
+    private function parseGroups($groups): array

-    private function parseStores($stores)
+    private function parseStores($stores): array

-    private function getCurrentValuesFor($id)
+    private function getCurrentValuesFor(string $id): array
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c5a01c2 and 5adea92.

📒 Files selected for processing (4)
  • Block/Adminhtml/SelectWidgetBlock.php (1 hunks)
  • Block/Embed.php (4 hunks)
  • view/adminhtml/templates/selectwidget.phtml (2 hunks)
  • view/frontend/templates/embed.phtml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • Block/Embed.php
  • view/adminhtml/templates/selectwidget.phtml
  • view/frontend/templates/embed.phtml

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.

1 participant