Fix security issues and PHP 8.4 compatibility#139
Open
aaapl wants to merge 3 commits intosmsapi:masterfrom
Open
Fix security issues and PHP 8.4 compatibility#139aaapl wants to merge 3 commits intosmsapi:masterfrom
aaapl wants to merge 3 commits intosmsapi:masterfrom
Conversation
…rator - Mask Authorization and Proxy-Authorization headers in logs, showing only "xxxx... (len = N)" instead of the actual token value - Use StreamInterface type hint on readBody() for proper contracts - For seekable streams: read body via (string) cast, then rewind() so downstream code receives an intact stream - For non-seekable streams: log a placeholder instead of consuming or crashing on rewind() — prevents logging from breaking requests - Add LoggerDecoratorTest with 7 tests covering: header sanitization, seekable body preservation, and non-seekable stream safety
Set CURLOPT_SSL_VERIFYPEER=true and CURLOPT_SSL_VERIFYHOST=2 to ensure SSL verification is always active regardless of system-level php.ini or cURL configuration.
Replace `bool $param = null` with `$param = null` (untyped) to avoid E_DEPRECATED in PHP 8.4 while maintaining backward compatibility with PHP 7.0+ as declared in composer.json. Type information preserved in @param docblocks for IDE/static analysis.
Collaborator
|
It would be best to divide it into 3 independent parts, as divided in summary:
|
maciejlew
requested changes
Mar 30, 2026
Collaborator
maciejlew
left a comment
maciejlew
left a comment
There was a problem hiding this comment.
Apart from the login part, everything else is acceptable.
| $this->assertArrayHasKey('headers', $requestLog); | ||
|
|
||
| $authHeader = $requestLog['headers']['Authorization'][0]; | ||
| $this->assertStringNotContainsString('my-secret-token', $authHeader); |
Collaborator
There was a problem hiding this comment.
Merge/rebase master and run PHP 7.0 tests:
make test-suite-php-7.0 SUITE=unityou will get:
Error: Call to undefined method Smsapi\Client\Tests\Unit\Infrastructure\HttpClient\Decorator\LoggerDecoratorTest::assertStringNotContainsString()
/app/tests/Unit/Infrastructure/HttpClient/Decorator/LoggerDecoratorTest.php:40
phpvfscomposer:///app/vendor/phpunit/phpunit/phpunit:52| { | ||
| private $logs = []; | ||
|
|
||
| public function log($level, $message, array $context = []): void |
Collaborator
There was a problem hiding this comment.
Merge/rebase master and run PHP 7.0 tests:
make test-suite-php-7.0 SUITE=unityou will get
Smsapi\Client\Tests\Unit\Infrastructure\HttpClient\Decorator\LoggerDecoratorTest::it_sanitizes_authorization_header
TypeError: Return value of Smsapi\Client\Tests\Unit\Infrastructure\HttpClient\Decorator\SpyLogger::log() must be an instance of Smsapi\Client\Tests\Unit\Infrastructure\HttpClient\Decorator\void, none returned
/app/tests/Unit/Infrastructure/HttpClient/Decorator/LoggerDecoratorTest.php:174
/app/vendor/psr/log/Psr/Log/AbstractLogger.php:113
/app/src/Infrastructure/HttpClient/Decorator/LoggerDecorator.php:33
/app/tests/Unit/Infrastructure/HttpClient/Decorator/LoggerDecoratorTest.php:34
phpvfscomposer:///app/vendor/phpunit/phpunit/phpunit:52| 'uri' => $request->getUri(), | ||
| 'headers' => $request->getHeaders(), | ||
| 'body' => $request->getBody()->getContents(), | ||
| 'headers' => $this->sanitizeHeaders($request->getHeaders()), |
Collaborator
There was a problem hiding this comment.
The default logger is NullLogger. If you're injecting your own logger and have trouble with the logged data, your logger is the place to process it.
Collaborator
There was a problem hiding this comment.
Collaborator
There was a problem hiding this comment.
Detach all that logging related stuff from this PR. All the rest is acceptable.
| * @param bool|null $checkIdx | ||
| */ | ||
| public function setExternalId(string $idx, bool $checkIdx = null): self | ||
| public function setExternalId(string $idx, $checkIdx = null): self |
Collaborator
There was a problem hiding this comment.
This solution is not 100% backward compatible. If someone extended these classes and methods, they would receive a warning for PHP < 8.0:
Warning: Declaration of Smsapi\Client\Tests\Unit\Feature\Sms\Bag\ExtendedSendSmssBag::setExternalId(array $idx, ?bool $checkIdx = NULL): Smsapi\Client\Feature\Sms\Bag\SendSmssBag should be compatible with Smsapi\Client\Feature\Sms\Bag\SendSmssBag::setExternalId(array $idx, $checkIdx = NULL): Smsapi\Client\Feature\Sms\Bag\SendSmssBag in /app/tests/Unit/Feature/Sms/Bag/SendSmssBagTest.php on line 23and a fatal error for PHP >= 8.0.
Fatal error: Declaration of Smsapi\Client\Tests\Unit\Feature\Sms\Bag\ExtendedSendSmssBag::setExternalId(array $idx, ?bool $checkIdx = null): Smsapi\Client\Feature\Sms\Bag\SendSmssBag must be compatible with Smsapi\Client\Feature\Sms\Bag\SendSmssBag::setExternalId(array $idx, $checkIdx = null): Smsapi\Client\Feature\Sms\Bag\SendSmssBag in /app/tests/Unit/Feature/Sms/Bag/SendSmssBagTest.php on line 23
Collaborator
There was a problem hiding this comment.
Well, there is probably no better solution - I'd merge it and release it in the next version of the lib.
| { | ||
| curl_setopt($httpClient, CURLOPT_HEADER, true); | ||
| curl_setopt($httpClient, CURLOPT_RETURNTRANSFER, true); | ||
| curl_setopt($httpClient, CURLOPT_SSL_VERIFYPEER, true); |
Collaborator
There was a problem hiding this comment.
Extract a prepareConnectionOptions method for these options.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Authorization/Proxy-Authorizationheaders in logs — tokens are now masked asxxxx... (len = N). Fix stream handling: seekable streams are rewound after read, non-seekable streams are not touched (placeholder logged instead)CURLOPT_SSL_VERIFYPEER=true,CURLOPT_SSL_VERIFYHOST=2)bool $param = nullpattern (deprecated in PHP 8.4) across 11 occurrences in 8 files, replacing with untyped$param = nullfor backward compatibility with PHP 7.0+Motivation
getContents()consumed the stream without rewinding — broke request/response body for downstream coderewind()would crash on non-seekable PSR-7 streamsE_DEPRECATEDfor every call to methods with implicit nullable typesTests
Added
LoggerDecoratorTest(7 tests, 17 assertions) covering:Test plan
E_DEPRECATEDnotices on PHP 8.4