merge dev -> main #27
Conversation
… handling - Changed documentation file paths from "Docs/PHPDocumentor/classes" to "Docs/classes". - Updated test folder path from "test" to "tests". - Improved error messages in the Images class and added exception handling. - Refactored Crypto class methods to include type hints and return types. - Enhanced SQL class to validate table and column identifiers to prevent SQL injection. - Updated README to reflect Composer usage and improved module documentation. - Adjusted bootstrap file for PHPUnit tests to prefer Composer autoloading.
…cumentation - Added `declare(strict_types=1);` to all PHP files for stricter type checking. - Improved error handling by replacing `die()` with exceptions in various classes. - Updated README and other documentation files to reflect recent changes and improvements. - Enhanced test summaries to include new test cases and requirements. - Removed `composer.lock` from `.gitignore` to allow tracking of dependencies.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | 9cf7350 | tests/CryptoTest.php | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Pull request overview
This pull request merges the dev branch into main, introducing significant improvements to code quality, type safety, error handling, and testing infrastructure. The changes focus on modernizing the codebase with strict type declarations, replacing die() calls with exceptions, fixing cryptography bugs, and establishing CI/CD with GitHub Actions.
Changes:
- Added
declare(strict_types=1);to all PHP files for stricter type checking - Replaced
die()with exceptions in Network, Random, Files, and Debugger classes for better error handling - Fixed critical bugs in Crypto class (hash argument order, timing-safe comparison) and attempted IV handling improvements
- Added comprehensive test coverage: CryptoTest, FilesTest, SQLiteTest, TimesTest, RandomTest
- Introduced GitHub Actions CI workflow for automated testing on PHP 8.0-8.4
- Updated documentation with Composer installation instructions and CHANGELOG.md
Reviewed changes
Copilot reviewed 39 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| composer.lock | Added with locked dependency versions (contains future dates) |
| .github/workflows/php.yml | New CI workflow for automated PHPUnit testing |
| CHANGELOG.md | New changelog documenting all improvements |
| PHPUtils/*.php | Added strict types, improved error handling, fixed bugs |
| tests/*.php | Added strict types to all test files |
| tests/*Test.php | New test classes for Crypto, Files, SQLite, Times, Random |
| tests/bootstrap.php | Updated to prefer Composer autoload |
| README.md | Updated with Composer instructions and new doc links |
| index.php | Fixed doc and test file path references |
| TEST_SUMMARY.md | Updated with new test information |
Comments suppressed due to low confidence (1)
PHPUtils/Images.php:97
- The textToImage method is missing namespace prefixes for Imagick classes. Since the file uses
declare(strict_types=1);and is in the PHPUtils namespace, references to Imagick classes should be prefixed with backslash:
- Line 90:
new ImagickDraw()should benew \ImagickDraw() - Line 94:
Imagick::GRAVITY_NORTHWESTshould be\Imagick::GRAVITY_NORTHWEST - Line 97:
new Imagick($imagePath)should benew \Imagick($imagePath)
Also, line 60 correctly uses new Imagick($imagePath) but this should also be new \Imagick($imagePath) for consistency.
$image = new Imagick($imagePath);
$image->blurImage($radius, $sigma, $channel);
$image->writeImage($tmpFullPath);
// Schedule the file deletion when the script execution is finished
register_shutdown_function(function() use ($tmpFullPath, $image) {
$image->clear();
$image->destroy();
unlink($tmpFullPath);
});
return $tmpRelativePath; // return the path of the temporary file
} catch (\ImagickException $e) {
throw new \RuntimeException("Error blurring image: " . $e->getMessage(), 0, $e);
}
}
/**
* Add text to an image.
*
* @param string $text The text to add to the image.
* @param string $imagePath The path to the image file.
* @param string $fontPath The path to the font file.
* @param int $fontSize The font size.
* @param int $x The x-coordinate of the text position.
* @param int $y The y-coordinate of the text position.
* @param string $color The color of the text.
* @throws ImagickException If an error occurs while adding the text to the image.
*/
public function textToImage(string $text, string $imagePath, string $fontPath, int $fontSize, int $x, int $y, string $color = 'black') {
$draw = new ImagickDraw();
$draw->setFont($fontPath);
$draw->setFontSize($fontSize);
$draw->setFillColor($color);
$draw->setGravity(Imagick::GRAVITY_NORTHWEST);
$draw->annotation($x, $y, $text);
$image = new Imagick($imagePath);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $encrypted = openssl_encrypt($str, $method, $password, iv: $ivRaw); | ||
| return base64_encode($ivRaw . $encrypted); |
There was a problem hiding this comment.
In the encryptwithpw method, when $iv is true, the encrypted string returned by openssl_encrypt is already base64-encoded by default (unless the OPENSSL_RAW_DATA option is used). Concatenating the raw IV bytes with a base64 string and then base64-encoding the result creates incorrect data. The decryptwithpw method expects raw bytes concatenated before base64 encoding.
This should use OPENSSL_RAW_DATA option to get raw encrypted bytes, then concatenate with the raw IV, then base64 encode:
$encrypted = openssl_encrypt($str, $method, $password, OPENSSL_RAW_DATA, $ivRaw);| public function decryptwithpw(string $str, string $password, string $method = 'aes-256-cbc', string $iv = ''): string|false { | ||
| $ivLen = openssl_cipher_iv_length($method); | ||
| if ($iv !== '') { | ||
| $ivRaw = hex2bin($iv); | ||
| $ciphertext = base64_decode($str, true) ?: $str; | ||
| return openssl_decrypt($ciphertext, $method, $password, iv: $ivRaw); | ||
| } | ||
| $decoded = base64_decode($str, true); | ||
| if ($decoded !== false && strlen($decoded) > $ivLen) { | ||
| $ivRaw = substr($decoded, 0, $ivLen); | ||
| $ciphertext = substr($decoded, $ivLen); | ||
| return openssl_decrypt($ciphertext, $method, $password, iv: $ivRaw); | ||
| } | ||
| return openssl_decrypt($str, $method, $password); |
There was a problem hiding this comment.
The decryptwithpw method needs to be consistent with the encryption. When $iv is provided as a parameter, the code should use OPENSSL_RAW_DATA for decryption to match the expected format. When auto-detecting embedded IV, it assumes raw bytes were concatenated before base64 encoding, but this won't work correctly if openssl_encrypt returns base64 by default.
| return $this->res($call["status"], $call["data"]); | ||
| $call_status = $call["status"]; | ||
| $call_data = $call["data"]; | ||
| if (is_string($call_data)) { | ||
| $call_data = clean($call_data); | ||
| } | ||
| if ($call_status == "ERROR") { | ||
| return res("ERROR", $call_data); | ||
| return $this->res("ERROR", $call_data); | ||
| } | ||
| if ($call_data["status"] == "ERROR") { | ||
| return res("ERROR", $call_data); | ||
| return $this->res("ERROR", $call_data); | ||
| } | ||
| return res($call_status, $call_data); | ||
| return $this->res($call_status, $call_data); |
There was a problem hiding this comment.
Lines 428-439 are unreachable code. Line 427 returns early with return $this->res($call["status"], $call["data"]);, so all the subsequent logic that processes $call_status and $call_data will never execute. This appears to be dead code that should either be removed or the early return on line 427 should be removed if the additional processing is intended.
| $call_status = $call["status"]; | ||
| $call_data = $call["data"]; | ||
| if (is_string($call_data)) { | ||
| $call_data = clean($call_data); |
There was a problem hiding this comment.
On line 431, the call to clean() should be $this->clean($call_data) to call the instance method. However, this code is currently unreachable due to the early return on line 427.
| $call_data = clean($call_data); | |
| $call_data = $this->clean($call_data); |
| $now = new \DateTime('now'); | ||
| $then = new \DateTime($time); | ||
| $diff = $now->diff($then); |
There was a problem hiding this comment.
In the relativeTime method, the variable names $now and $then have been swapped compared to their intended semantics. Line 45 creates $now from the current time, and line 46 creates $then from the input $time parameter. However, the original code had $then = new DateTime('now') and $now = new DateTime($time), suggesting that $time (the parameter) represents the "now" point and we're calculating relative to it.
The current implementation calculates the diff from "now" to the past time in $time, which means:
- If $time is in the past, the diff will be negative/inverted
- The logic expects $time to be a past timestamp to show "X days ago"
The corrected semantics should be: calculate how long ago $time was from now. The swap is actually correct for this use case, but the variable names are confusing. Consider renaming for clarity or verifying the intended behavior with tests.
| $now = new \DateTime('now'); | |
| $then = new \DateTime($time); | |
| $diff = $now->diff($then); | |
| $currentTime = new \DateTime('now'); | |
| $inputTime = new \DateTime($time); | |
| $diff = $currentTime->diff($inputTime); |
| function sqlite_create_table(SQLite3 $db, string $table_name, array $columns = ["column_name" => "column_type"]) : array { | ||
| public function sqlite_create_table(\SQLite3 $db, string $table_name, array $columns = ["column_name" => "column_type"]) : array { | ||
| try { | ||
| if (sqlite_table_exists($db, $table_name)) { |
There was a problem hiding this comment.
The call to sqlite_table_exists() on line 133 is missing the $this-> prefix. It should be $this->sqlite_table_exists($db, $table_name) to call the instance method. This will cause a fatal error when this function is invoked.
| if (sqlite_table_exists($db, $table_name)) { | |
| if ($this->sqlite_table_exists($db, $table_name)) { |
| function sqlite_select(SQLite3 $db, string $table, array $columns = ["*"], string $where = "", array $params = [], string $order = "", string $limit = "") : array { | ||
| public function sqlite_select(\SQLite3 $db, string $table, array $columns = ["*"], string $where = "", array $params = [], string $order = "", string $limit = "") : array { | ||
| try { | ||
| if (!sqlite_table_exists($db, $table)) { |
There was a problem hiding this comment.
The call to sqlite_table_exists() on line 299 is also missing the $this-> prefix. It should be $this->sqlite_table_exists($db, $table) to call the instance method.
| if (!sqlite_table_exists($db, $table)) { | |
| if (!$this->sqlite_table_exists($db, $table)) { |
| return res($call_status, $call_data); | ||
| return $this->res($call_status, $call_data); | ||
|
|
||
| } catch (Exception $e) { |
There was a problem hiding this comment.
All catch blocks in SQLite.php catch Exception but should catch \Exception since the file uses declare(strict_types=1); and is in the PHPUtils namespace. Without the leading backslash, PHP will look for PHPUtils\Exception which doesn't exist. This affects lines 116, 145, 161, 177, 197, 223, 247, 280, 327, 368, 398, and 441.
| } catch (Exception $e) { | |
| } catch (\Exception $e) { |
| uses: shivammathur/setup-php@v2 | ||
| with: | ||
| php-version: ${{ matrix.php }} | ||
| extensions: mbstring, openssl |
There was a problem hiding this comment.
The GitHub Actions workflow only installs the mbstring and openssl extensions. However, the Images class requires the imagick extension (checked in its constructor). Tests that use the Images class will fail in CI unless the imagick extension is added to the workflow, or tests for Images are skipped when the extension is not available.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@Darknetzz I've opened a new pull request, #28, to work on those changes. Once the pull request is ready, I'll request review from you. |
Merge pull request #27 from Darknetzz/dev
This pull request introduces several improvements across the codebase, focusing on stricter type safety, improved error handling, enhanced cryptography utilities, and better documentation and testing. Notably, it replaces most uses of
die()with exceptions for more robust error handling, adds strict type declarations to all PHP files, and updates the cryptography and image manipulation methods for clarity and correctness. Continuous integration is also introduced for automated testing.Error handling improvements:
die()with appropriate exceptions throughout the codebase, including inNetwork,Random,Files,Debugger, andImagesclasses. This ensures that fatal errors are handled in a more controlled and testable way. (PHPUtils/Network.php[1] [2];PHPUtils/Random.php[3];PHPUtils/Files.php[4];PHPUtils/Debugger.php[5];PHPUtils/Images.php[6] [7]Type safety and code modernization:
declare(strict_types=1);to all PHP library and test files to enforce strict type checking. (PHPUtils/API.php[1]PHPUtils/Auth.php[2]PHPUtils/Base.php[3]PHPUtils/Calendar.php[4]PHPUtils/Crypto.php[5]PHPUtils/Debugger.php[6]PHPUtils/Files.php[7]PHPUtils/Funcs.php[8]PHPUtils/Images.php[9]PHPUtils/Navigation.php[10]PHPUtils/Network.php[11]PHPUtils/Random.php[12]CryptoandImagesclasses. (PHPUtils/Crypto.php[1]PHPUtils/Images.php[2] [3] [4]Cryptography and utility enhancements:
Cryptoclass: clarified IV handling, fixed thehash()argument order, and madeverifyhash()use timing-safe comparison. (PHPUtils/Crypto.phpPHPUtils/Crypto.phpL18-R97)Network::ipInRange()to properly validate IPs and removed direct output on error. (PHPUtils/Network.phpPHPUtils/Network.phpL34-R36)Continuous Integration and Testing:
.github/workflows/php.yml) to run PHPUnit tests on PHP versions 8.0–8.4 formainanddevbranches, ensuring automated testing and compatibility.Documentation and changelog:
CHANGELOG.mdfollowing the Keep a Changelog format to document all notable changes and improvements.These changes collectively improve the safety, maintainability, and reliability of the codebase.