Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Symfony console command to scan Etherpad instances across all discovered versions and derive file-hash-to-version ranges, supporting maintenance of the existing file-hash lookup mapping.
Changes:
- Add
ether:generate-file-hashes-all-versionscommand to fetch instances, scan a given file, and summarize hashes per version. - Introduce
InstanceResult/InstanceResultsto collect and group scan results. - Update
FileHashLookupServicehash/version range mappings (and type the class constant).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/Service/FileHashLookupService.php |
Updates the file-hash → version-range dataset and types the constant. |
src/Console/InstanceResults.php |
Adds a small collection helper to group instance scan results by version. |
src/Console/InstanceResult.php |
Adds a DTO for per-instance scan results. |
src/Console/GenerateFileHashesAllVersionsCommand.php |
New command that fetches instance inventory, scans versions, and prints summary tables. |
bin/console.php |
Registers the new console command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,15 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This new file is missing declare(strict_types=1); after the opening <?php. The rest of src/ consistently enables strict types, so this should match to avoid accidental scalar coercions.
| <?php | |
| <?php | |
| declare(strict_types=1); |
| $this | ||
| ->addOption('matches-count', null, InputArgument::OPTIONAL, 'Minimum count of matches for version to be considered valid', 3) | ||
| ->addArgument('file', InputArgument::REQUIRED, 'File path to check'); |
There was a problem hiding this comment.
The option is registered using InputArgument::OPTIONAL, but addOption() expects InputOption mode constants (e.g., VALUE_OPTIONAL / VALUE_REQUIRED). With the current code the option may not be parsed as intended on different Symfony versions; switch to Symfony\Component\Console\Input\InputOption and the appropriate VALUE_* constant.
| $filePath = $input->getArgument('file'); | ||
| $countVersionsMatch = $input->getOption('matches-count'); | ||
|
|
||
| $allInstances = $this->getInstances(); | ||
| $instanceResults = new InstanceResults(); | ||
|
|
||
| foreach ($this->getAllVersions($allInstances) as $version) { | ||
| $this->scanVersionInstances($allInstances, $version, $filePath, $instanceResults, $output, $countVersionsMatch); | ||
| } |
There was a problem hiding this comment.
Because strict_types is enabled, $input->getOption('matches-count') will be a string when provided via CLI, but scanVersionInstances() requires an int. This will throw a TypeError when the option is used; cast/validate the option value to int (and consider rejecting non-numeric input).
| $instanceResult = new InstanceResult($instance['name'], $version, $fileHash, $fileContent); | ||
| $instanceResults->add($instanceResult); | ||
|
|
||
| if ($fileHash === null) { | ||
| $output->writeln('<error>Could not get hash for instance ' . $instance['name'] . '</error>', OutputInterface::VERBOSITY_VERY_VERBOSE); | ||
|
|
||
| if ($scannedInstances > 4 && count($foundMatchesForHash) === 0) { | ||
| break; | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if ($this->matches($instanceResults, $instanceResult, $version)) { | ||
| $output->writeln('Match found for version ' . $version . ' and hash ' . $instanceResult->fileHash); | ||
|
|
There was a problem hiding this comment.
matches() will always return true because the current InstanceResult is added to InstanceResults before calling matches(), and matches() compares against all instances for the version (including itself). This makes the match logic ineffective; either check for matches before adding, or have matches() ignore the current instance (e.g., by name).
| $client = new Client(); | ||
| $response = $client->get('https://ether-scan.stefans-entwicklerecke.de/api/instances'); | ||
|
|
||
| $body = (string)$response->getBody(); | ||
| $data = json_decode($body, true); | ||
| return $data['instances'] ?? []; |
There was a problem hiding this comment.
getInstances() does not handle request failures and assumes json_decode() returns an array. If the API is unreachable or returns invalid JSON, $data will be null and $data['instances'] will throw ("Cannot access offset on null"). Add exception handling/timeouts and validate that decoded data is an array before indexing.
| $client = new Client(); | |
| $response = $client->get('https://ether-scan.stefans-entwicklerecke.de/api/instances'); | |
| $body = (string)$response->getBody(); | |
| $data = json_decode($body, true); | |
| return $data['instances'] ?? []; | |
| try { | |
| $client = new Client([ | |
| RequestOptions::TIMEOUT => 3.0, | |
| RequestOptions::CONNECT_TIMEOUT => 1.0, | |
| ]); | |
| $response = $client->get('https://ether-scan.stefans-entwicklerecke.de/api/instances'); | |
| $body = (string) $response->getBody(); | |
| $data = json_decode($body, true); | |
| if (!is_array($data)) { | |
| return []; | |
| } | |
| $instances = $data['instances'] ?? []; | |
| return is_array($instances) ? $instances : []; | |
| } catch (GuzzleException) { | |
| return []; | |
| } |
| @@ -0,0 +1,46 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This new file is missing declare(strict_types=1); after the opening <?php. The rest of src/ consistently enables strict types, so this should match to avoid accidental scalar coercions.
| <?php | |
| <?php | |
| declare(strict_types=1); |
| 'base_uri' => $url, | ||
| RequestOptions::TIMEOUT => 3.0, | ||
| RequestOptions::CONNECT_TIMEOUT => 1.0, | ||
| 'verify' => false, |
There was a problem hiding this comment.
The GuzzleHttp\Client in getFile() is configured with 'verify' => false, which disables TLS certificate validation for HTTPS requests to instance URLs. An attacker who can intercept or spoof network traffic (e.g., via DNS poisoning or a malicious proxy) could serve tampered file contents, leading to incorrect hash generation and undermining the integrity of the scan results. Remove the verify override and rely on default certificate verification (or a properly configured CA bundle) so that HTTPS connections are authenticated and protected against man-in-the-middle attacks.
| 'verify' => false, |
No description provided.