feat: add functionality for plugin checksums #344
Conversation
WalkthroughThis update introduces a comprehensive plugin file integrity checker for Shopware, spanning backend services, Symfony console commands, an API endpoint, and administration UI enhancements. It enables checksum generation and verification for plugins, exposes results via a new API, and displays plugin file status in the admin interface with multilingual support. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUser
participant AdminUI
participant FroshToolsAPI
participant PluginFilesController
participant PluginFileHashService
participant PluginRepository
AdminUser->>AdminUI: Open Files Tab
AdminUI->>FroshToolsAPI: GET /plugin-files
FroshToolsAPI->>PluginFilesController: listPluginFiles()
PluginFilesController->>PluginRepository: fetch all plugins
loop For each plugin
PluginFilesController->>PluginFileHashService: checkPluginForChanges(plugin)
end
PluginFilesController-->>FroshToolsAPI: JSON (success, pluginResults)
FroshToolsAPI-->>AdminUI: Results
AdminUI-->>AdminUser: Display plugin file status (new/changed/missing)
sequenceDiagram
participant AdminUser
participant Console
participant PluginChecksumCheckCommand
participant PluginRepository
participant PluginFileHashService
AdminUser->>Console: Run "frosh:plugin:checksum:check [pluginName]"
Console->>PluginChecksumCheckCommand: execute()
PluginChecksumCheckCommand->>PluginRepository: fetch plugin(s)
loop For each plugin
PluginChecksumCheckCommand->>PluginFileHashService: checkPluginForChanges(plugin)
PluginFileHashService-->>PluginChecksumCheckCommand: PluginChecksumCheckResult
end
PluginChecksumCheckCommand-->>Console: Output status and exit code
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Components/PluginChecksum/PluginFileHashService.php (2)
35-43: Consider adding algorithm customization parameterThe
getChecksumDatamethod doesn't allow customizing the hashing algorithm, while the underlyinggetHashesmethod does. Consider adding an optional parameter for consistency and flexibility.- public function getChecksumData(PluginEntity $plugin, array $fileExtensions): PluginChecksumStruct + public function getChecksumData(PluginEntity $plugin, array $fileExtensions, ?string $algorithm = null): PluginChecksumStruct { return PluginChecksumStruct::fromArray([ - 'algorithm' => self::HASH_ALGORITHM, + 'algorithm' => $algorithm ?? self::HASH_ALGORITHM, 'fileExtensions' => $fileExtensions, - 'hashes' => $this->getHashes($plugin, $fileExtensions), + 'hashes' => $this->getHashes($plugin, $fileExtensions, $algorithm), 'pluginVersion' => $plugin->getVersion(), ]); }
45-92: Consider consistent error handling strategyThe
checkPluginForChangesmethod throws exceptions for some error cases (unreadable or invalid checksum file) but returns a specialized result object for others (missing file, wrong version). Consider a more consistent approach for error handling.Either handle all errors via exceptions:
public function checkPluginForChanges(PluginEntity $plugin): PluginChecksumCheckResult { $checksumFilePath = $this->getChecksumFilePathForPlugin($plugin); if (!is_file($checksumFilePath)) { - return new PluginChecksumCheckResult(fileMissing: true); + throw new \RuntimeException(sprintf('Checksum file "%s" does not exist', $checksumFilePath)); } // ... if ($plugin->getVersion() !== $checksumPluginVersion) { - return new PluginChecksumCheckResult(wrongVersion: true); + throw new \RuntimeException(sprintf('Plugin version mismatch: expected "%s", got "%s"', $checksumPluginVersion, $plugin->getVersion())); } // ... }Or handle all errors via the result object:
public function checkPluginForChanges(PluginEntity $plugin): PluginChecksumCheckResult { $checksumFilePath = $this->getChecksumFilePathForPlugin($plugin); if (!is_file($checksumFilePath)) { return new PluginChecksumCheckResult(fileMissing: true); } if (!is_readable($checksumFilePath)) { - throw new \RuntimeException(\sprintf('Checksum file "%s" exists but is not readable', $checksumFilePath)); + return new PluginChecksumCheckResult(error: sprintf('Checksum file "%s" exists but is not readable', $checksumFilePath)); } try { // ... } catch (\JsonException $exception) { - throw new \RuntimeException(\sprintf('Checksum file "%s" is not valid JSON', $checksumFilePath), 0, $exception); + return new PluginChecksumCheckResult(error: sprintf('Checksum file "%s" is not valid JSON', $checksumFilePath)); } // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
src/Command/PluginChecksumCheckCommand.php(1 hunks)src/Command/PluginChecksumCreateCommand.php(1 hunks)src/Components/PluginChecksum/PluginFileHashService.php(1 hunks)src/Components/PluginChecksum/Struct/PluginChecksumCheckResult.php(1 hunks)src/Components/PluginChecksum/Struct/PluginChecksumStruct.php(1 hunks)src/Controller/PluginFilesController.php(1 hunks)src/Resources/app/administration/src/api/frosh-tools.js(1 hunks)src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/index.js(2 hunks)src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/template.twig(1 hunks)src/Resources/app/administration/src/module/frosh-tools/snippet/de-DE.json(1 hunks)src/Resources/app/administration/src/module/frosh-tools/snippet/en-GB.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Resources/app/administration/src/module/frosh-tools/snippet/en-GB.json
- src/Resources/app/administration/src/module/frosh-tools/snippet/de-DE.json
- src/Resources/app/administration/src/api/frosh-tools.js
- src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/template.twig
- src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/index.js
- src/Components/PluginChecksum/Struct/PluginChecksumCheckResult.php
- src/Command/PluginChecksumCheckCommand.php
- src/Components/PluginChecksum/Struct/PluginChecksumStruct.php
- src/Controller/PluginFilesController.php
- src/Command/PluginChecksumCreateCommand.php
🔇 Additional comments (7)
src/Components/PluginChecksum/PluginFileHashService.php (7)
17-17: Good choice of hashing algorithmUsing xxh128 is an excellent choice for file integrity checks due to its speed and collision resistance, as noted in your comments.
52-54: Good implementation of file readability checkProperly checking if the checksum file is readable before attempting to read it prevents cryptic errors later. This is a good practice.
81-85: Correct comparison algorithm for detecting file changesThe explicit key/value comparison to detect changed files is implemented correctly, ensuring accurate identification of modified files.
111-114: Proper extension normalizationThe code correctly normalizes file extensions to glob patterns, ensuring Symfony's Finder will properly match files regardless of how extensions are provided.
126-133: Robust hash generation with proper error handlingThe code checks if hash generation was successful and throws an appropriate exception with a descriptive message when it fails. This prevents silent failures.
136-136: Cross-platform path handlingGood implementation of path normalization that handles both Windows and Unix paths, ensuring consistent relative paths regardless of the operating system.
153-157: Good fallback for plugins without autoload informationThe code properly handles the case where a plugin doesn't have autoload information by falling back to the plugin root directory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Command/PluginChecksumCreateCommand.php (2)
41-89: Add a force option to ensure conscientious overwriting of existing checksum files.The command currently writes to the checksum file without checking if it already exists, which could lead to unexpected overwrites. Consider adding a
--forceoption to make this behavior explicit.protected function configure(): void { $this->addArgument('plugin', InputArgument::REQUIRED, 'Plugin name'); + $this->addOption( + 'force', + 'f', + InputInterface::VALUE_NONE, + 'Force overwrite of existing checksum file' + ); } protected function execute(InputInterface $input, OutputInterface $output): int { $io = new ShopwareStyle($input, $output); // @phpstan-ignore-next-line $context = method_exists(Context::class, 'createCLIContext') ? Context::createCLIContext() : Context::createDefaultContext(); $pluginName = (string) $input->getArgument('plugin'); $plugin = $this->getPlugin($pluginName, $context); // ... $checksumFilePath = $this->getChecksumFilePathForPlugin($plugin); + + if (file_exists($checksumFilePath) && !$input->getOption('force')) { + $io->warning(sprintf( + 'Checksum file "%s" already exists. Use --force to overwrite.', + $checksumFilePath + )); + + return self::FAILURE; + } // rest of the method...
66-68: Add verbose output option to show the checksummed filesThe command could benefit from an option to display all files being checksummed for better visibility and debugging.
protected function configure(): void { $this->addArgument('plugin', InputArgument::REQUIRED, 'Plugin name'); + $this->addOption( + 'verbose', + 'v', + InputInterface::VALUE_NONE, + 'Display all files being checksummed' + ); }Then in the execute method:
$checksumStruct = $this->pluginFileHashService->getChecksumData($plugin); $io->info(\sprintf('Writing %s checksums for plugin "%s" to file %s', \count($checksumStruct->getHashes()), $plugin->getName(), $checksumFilePath)); +if ($input->getOption('verbose')) { + $io->section('Files included in checksum:'); + foreach (array_keys($checksumStruct->getHashes()) as $file) { + $io->writeln(' - ' . $file); + } +} + $directory = \dirname($checksumFilePath);src/Components/PluginChecksum/PluginFileHashService.php (5)
73-80: Consider adding file path normalization for cross-platform compatibilityWhile you're handling Windows vs. Unix paths in the
getHashesmethod, ensure consistent normalization when comparing hashes from previously stored checksums that might have been generated on a different platform.$newFiles = array_diff_key($currentHashes, $previouslyHashedFiles); $missingFiles = array_diff_key($previouslyHashedFiles, $currentHashes); $changedFiles = []; + +// Normalize keys for cross-platform compatibility +$normalizedCurrentHashes = []; +foreach ($currentHashes as $path => $hash) { + $normalizedPath = str_replace('\\', '/', $path); + $normalizedCurrentHashes[$normalizedPath] = $hash; +} + +$normalizedPreviousHashes = []; +foreach ($previouslyHashedFiles as $path => $hash) { + $normalizedPath = str_replace('\\', '/', $path); + $normalizedPreviousHashes[$normalizedPath] = $hash; +} + foreach ($previouslyHashedFiles as $file => $oldHash) { - if (isset($currentHashes[$file]) && $currentHashes[$file] !== $oldHash) { + $normalizedFile = str_replace('\\', '/', $file); + if (isset($normalizedCurrentHashes[$normalizedFile]) && $normalizedCurrentHashes[$normalizedFile] !== $oldHash) { $changedFiles[] = $file; } }
98-108: Add filtering by file extension patterns for more focused checksummingThe service currently scans all files in the specified directories except those in vendor and node_modules. Consider adding an optional parameter to filter by specific file extensions for more focused checksumming.
- private function getHashes(PluginEntity $plugin, ?string $algorithm = null): array + /** + * @param string[]|null $filePatterns Optional array of glob patterns for files to include (e.g. ['*.php', '*.js']) + * @return array<string, string> + */ + private function getHashes(PluginEntity $plugin, ?string $algorithm = null, ?array $filePatterns = null): array { $algorithm = $algorithm ?? self::HASH_ALGORITHM; $rootPluginPath = $this->getPluginRootPath($plugin); $directories = $this->getDirectories($plugin); if ($directories === []) { return []; } $finder = new Finder(); - $finder->in($directories) - ->files() - ->notPath('/vendor/') - ->notPath('/node_modules/'); + $finder->in($directories)->files()->notPath('/vendor/')->notPath('/node_modules/'); + + if ($filePatterns !== null && count($filePatterns) > 0) { + $finder->name($filePatterns); + }Then update the public methods to accept and pass this parameter.
116-124: Add safeguard against hashing very large filesReading and hashing extremely large files could lead to memory or performance issues. Consider adding a size check before processing.
+ // Skip files larger than 100MB to prevent performance issues + if ($file->getSize() > 100 * 1024 * 1024) { + continue; + } + $hash = \hash_file($algorithm, $absoluteFilePath); if ($hash === false) { throw new \RuntimeException(\sprintf( 'Could not generate %s hash for "%s"', $algorithm, $absoluteFilePath )); }
126-127: Ensure correct path normalization for both Windows and Unix pathsThe path normalization is good, but the order of operations could be improved to ensure consistent handling across all platforms.
- // Make sure the replacement handles Windows and Unix paths - $relativePath = \ltrim(str_replace([$rootPluginPath, '\\'], ['', '/'], $absoluteFilePath), '/'); + // Normalize path separators first, then remove the plugin root path + $normalizedPath = str_replace('\\', '/', $absoluteFilePath); + $normalizedRootPath = str_replace('\\', '/', $rootPluginPath); + $relativePath = \ltrim(str_replace($normalizedRootPath, '', $normalizedPath), '/');
96-97: Consider caching the plugin root path computationThe
getPluginRootPathmethod is called in multiple places. Consider caching the result to avoid redundant concatenation and trimming operations.private function getHashes(PluginEntity $plugin, ?string $algorithm = null): array { $algorithm = $algorithm ?? self::HASH_ALGORITHM; - $rootPluginPath = $this->getPluginRootPath($plugin); + static $pathCache = []; + $pluginId = $plugin->getId(); + $rootPluginPath = $pathCache[$pluginId] ?? ($pathCache[$pluginId] = $this->getPluginRootPath($plugin));Similar caching could be applied in other methods that use
getPluginRootPath.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Command/PluginChecksumCreateCommand.php(1 hunks)src/Components/PluginChecksum/PluginFileHashService.php(1 hunks)src/Components/PluginChecksum/Struct/PluginChecksumStruct.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Components/PluginChecksum/Struct/PluginChecksumStruct.php
🧰 Additional context used
🧠 Learnings (1)
src/Command/PluginChecksumCreateCommand.php (3)
Learnt from: M-arcus
PR: FriendsOfShopware/FroshTools#344
File: src/Command/PluginChecksumCheckCommand.php:94-98
Timestamp: 2025-05-08T09:54:13.855Z
Learning: In Shopware, `Context::createDefaultContext()` should be used in CLI commands when backward compatibility with older Shopware 6 versions is required, as not all Shopware 6 versions implement the `Context::createCLIContext()` method.
Learnt from: M-arcus
PR: FriendsOfShopware/FroshTools#344
File: src/Command/PluginChecksumCheckCommand.php:94-94
Timestamp: 2025-05-07T21:41:20.968Z
Learning: In Shopware, `Context::createDefaultContext()` is the appropriate method to use for creating context in CLI commands. There is no `Context::createSystemContext()` method.
Learnt from: M-arcus
PR: FriendsOfShopware/FroshTools#344
File: src/Command/PluginChecksumCheckCommand.php:94-94
Timestamp: 2025-05-07T21:41:20.968Z
Learning: In Shopware, the available context creation methods include `Context::createDefaultContext()` and `Context::createCLIContext()` for CLI commands. There is no `Context::createSystemContext()` method.
🔇 Additional comments (5)
src/Command/PluginChecksumCreateCommand.php (1)
27-30:Details
❌ Incorrect review comment
Missing nullable type for getChecksumFilePathForPlugin method return
The
PluginFileHashServicemethod is called on line 59 with conditionif (!$checksumFilePath)which suggests it might returnnullin some cases, but the method declaration doesn't reflect this possibility.Let's verify this by checking the PluginFileHashService implementation:
🏁 Script executed:
#!/bin/bash # Check the return type of the getChecksumFilePathForPlugin method rg -A1 -B1 "getChecksumFilePathForPlugin.*\): " src/Components/PluginChecksum/PluginFileHashService.phpLength of output: 191
No nullable return type needed for getChecksumFilePathForPlugin
The
PluginFileHashService::getChecksumFilePathForPluginmethod is explicitly declared to return a non-nullablestringand does not returnnull. Theif (!$checksumFilePath)check simply guards against an empty string, not a null value. No change to the return type is required.Likely an incorrect or invalid review comment.
src/Components/PluginChecksum/PluginFileHashService.php (4)
11-26: Good choice of hash algorithm with clear documentationThe xxh128 algorithm is a good choice for this use case as it provides excellent speed and collision resistance. The clearly documented rationale helps future maintainers understand this design decision.
27-30: Consistent file path handling across the serviceThe path construction is now consistent with the fixes made in previous reviews, correctly handling path separators.
41-87: Robust implementation of checksum verification with explicit error handlingThe checksum verification logic includes proper error handling for file existence, readability, JSON parsing, and version checks. The previous issues with hash comparison have been fixed using a key-by-key comparison approach.
146-149: Good fallback implementation for plugins without autoload informationThe fallback to the plugin root directory when autoload information is not available is a good defensive programming pattern that ensures the service works even with minimal plugin configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Components/PluginChecksum/Struct/PluginChecksumStruct.php (1)
22-28: Consider adding validation for required fields in the factory method.While the parent's
assign()method handles property assignment, consider validating that required fields (algorithm, hashes, pluginVersion) are present in the input array to fail fast with clear error messages.public static function fromArray(array $data): self { + $requiredFields = ['algorithm', 'hashes', 'pluginVersion']; + foreach ($requiredFields as $field) { + if (!isset($data[$field])) { + throw new \InvalidArgumentException(sprintf('Required field "%s" is missing', $field)); + } + } + return (new self())->assign($data); }src/Components/PluginChecksum/PluginFileHashService.php (1)
137-140: Consider using DIRECTORY_SEPARATOR for better cross-platform compatibility.While the current implementation works, using
DIRECTORY_SEPARATORwould make the intent clearer and ensure consistent behavior across platforms.private function getExtensionRootPath(PluginEntity $plugin): string { - return \rtrim($this->rootDir . '/' . $plugin->getPath(), '/\\'); + return \rtrim($this->rootDir . DIRECTORY_SEPARATOR . $plugin->getPath(), '/\\'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Command/PluginChecksumCheckCommand.php(1 hunks)src/Components/PluginChecksum/PluginFileHashService.php(1 hunks)src/Components/PluginChecksum/Struct/PluginChecksumCheckResult.php(1 hunks)src/Components/PluginChecksum/Struct/PluginChecksumStruct.php(1 hunks)src/Controller/PluginFilesController.php(1 hunks)src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/template.twig(1 hunks)src/Resources/app/administration/src/module/frosh-tools/snippet/de-DE.json(1 hunks)src/Resources/app/administration/src/module/frosh-tools/snippet/en-GB.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Resources/app/administration/src/module/frosh-tools/snippet/de-DE.json
- src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-files/template.twig
- src/Resources/app/administration/src/module/frosh-tools/snippet/en-GB.json
- src/Command/PluginChecksumCheckCommand.php
- src/Controller/PluginFilesController.php
- src/Components/PluginChecksum/Struct/PluginChecksumCheckResult.php
🔇 Additional comments (7)
src/Components/PluginChecksum/Struct/PluginChecksumStruct.php (2)
7-21: Well-structured data class with proper type annotations.The class structure follows best practices with protected properties, appropriate type hints, and clear PHPDoc annotations for array types. The nullable
versionproperty provides good flexibility for future versioning.
30-51: Clean implementation of getter methods.All getter methods are properly typed and follow naming conventions. The PHPDoc annotation for
getHashes()maintains type information for the array elements.src/Components/PluginChecksum/PluginFileHashService.php (5)
11-25: Well-structured service class with appropriate algorithm choice.The xxh128 algorithm is an excellent choice for file integrity verification due to its speed and collision resistance. The constructor properly uses dependency injection with the readonly modifier for immutability.
27-30: Correctly implements checksum file path generation.The method properly constructs the file path with the necessary separator, addressing previous path concatenation concerns.
32-40: Clean implementation of checksum data generation.The method properly assembles all required data for the checksum structure, using appropriate constants and retrieving the plugin version from the entity.
42-90: Robust implementation with comprehensive error handling.The method properly handles all edge cases including:
- File existence and readability checks
- JSON parsing with proper error messages
- Version mismatch detection
- Correct hash comparison logic using key-value pairs
The comment about future version handling (line 66-67) is helpful for maintainability.
92-135: Well-implemented file hashing with proper exclusions.The method correctly:
- Excludes appropriate directories (vendor, node_modules, admin resources)
- Handles file path resolution safely
- Provides clear error messages on hash generation failure
- Normalizes paths for cross-platform compatibility
- Sorts results for consistent output
2 participants
Why is this change necessary?
Plugin developers have no way to see if their plugin code has been modified other than checking each file. This functionality was included in Shopware 5.
What does this change do, exactly?
It adds 2 commands for the CLI
frosh:extension:checksum:checkCheck the integrity of plugin filesfrosh:extension:checksum:createCreates a list of files and their checksums for a pluginIt adds a list in the administration of FroshTools (
/admin#/frosh/tools/index/files)Where is this from?
Ported from shopware/shopware#5362 because it was recommended to move it here.
Summary by CodeRabbit