feat(MPDZBS-877): zmscitizenapi into next #674
Closed
Conversation
This file contains 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
#574) * endpoint working * fix unit tests * one working test for post-reserve * some clean up * feat(ZMS-2517): fix up response for the reserve post request * feat(ZMS-2517): add more unit tests for validations * feat(ZMS-2517): add more unit tests for validations * feat(ZMS-2517): Zmscitizenapi Refactor Part 1 of 3 move controllers back to root * feat(ZMS-2517): Zmscitizenapi Refactor Part 2 of 3 create ZmsApiFacadeService ZmsApiClientService ValidationService MapperService UtilityHelper and delete fragmented services * feat(ZMS-2517): Zmscitizenapi Refactor Part 3 of 3 clean up validations and unit tests --------- Co-authored-by: Thomas Fink <[email protected]>
…by github commit history
* (feat MPDZBS-877) readd citizenapi to htaccess * feat(MPDZBS-877 ZMS-2517): Working Appointment update with too many emails exception * feat(MPDZBS-877 ZMS-2517): Test rendering working for update appointment test * feat(MPDZBS-877 ZMS-2517): Add 63 validation unit tests for appointment update * feat(MPDZBS-877 ZMS-2517): Refactor exception appointmentNotFound * feat(MPDZBS-877 ZMS-2517): Add two new edge case exception tests * feat(MPDZBS-877 ZMS-2517): Remove error_logs --------- Co-authored-by: Thomas Fink <[email protected]>
ThomasAFink
force-pushed
the
feature-MPDZBS-877-zmscitizenapi
branch
from
b57e687 to
1a34881
Compare
ThomasAFink
force-pushed
the
feature-MPDZBS-877-zmscitizenapi
branch
from
1a34881 to
c15dea6
Compare
tobiasholler
requested changes
Nov 20, 2024
… in rate limiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (22)
zmscitizenapi/src/Zmscitizenapi/Localization/ErrorMessages.php (3)
37-329: Standardize error code casing convention.Some error codes use UPPER_CASE while others use camelCase. For consistency, standardize the casing across all error codes.
Apply this diff to maintain consistency:
- 'errorCode' => 'EMPTY_SERVICE_ARRAYS', + 'errorCode' => 'emptyServiceArrays', - 'errorCode' => 'MISMATCHED_ARRAYS', + 'errorCode' => 'mismatchedArrays', - 'errorCode' => 'IP_BLACKLISTED', + 'errorCode' => 'ipBlacklisted',
299-327: Remove redundant translation comments.The middleware error messages contain redundant German translations in comments that match the actual translations in the DE array.
Remove the redundant
// DE: ...comments as they duplicate the translations already present in the DE constant.
443-447: Fix grammatical error in German translation.The German translation for 'appointmentCanNotBeCanceled' contains a grammatical error.
Apply this diff to fix the grammar:
- 'errorMessage' => 'Der von Ihnen gewählte Termin ist leider nicht mehr gelöscht werden.', + 'errorMessage' => 'Der von Ihnen gewählte Termin kann leider nicht mehr gelöscht werden.',zmscitizenapi/tests/Zmscitizenapi/ControllerTestCase.php (1)
7-7: Add type hint for the namespace property.For better type safety and code clarity, add a type hint to the namespace property.
- protected $namespace = '\\BO\\Zmscitizenapi\\'; + protected string $namespace = '\\BO\\Zmscitizenapi\\';zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php (4)
25-30: Consider adding cache cleanup in tearDown.While the setUp method clears the cache, it's good practice to also clear it in tearDown to ensure a clean state for subsequent tests.
protected function tearDown(): void { putenv('IP_BLACKLIST'); $_SERVER = []; // Reset server variables + if (\App::$cache) { + \App::$cache->clear(); + } parent::tearDown(); }
32-83: Consider using data providers for IP test cases.The first three test methods (
testAllowsRequestWithEmptyBlacklist,testAllowsNonBlacklistedIPv4,testAllowsNonBlacklistedIPv6) have similar structures and could benefit from using a data provider to reduce code duplication.Here's how you could refactor this:
+ /** + * @return array<string, array{string, string|null, bool}> + */ + public function ipTestCasesProvider(): array + { + return [ + 'empty_blacklist' => ['192.168.1.1', null, true], + 'non_blacklisted_ipv4' => ['192.168.2.1', '192.168.1.1,10.0.0.0/8', true], + 'non_blacklisted_ipv6' => ['2001:db9::1', '2001:db8::/32,::1', true], + ]; + } + + /** + * @dataProvider ipTestCasesProvider + */ + public function testAllowsRequest(string $ip, ?string $blacklist, bool $shouldAllow): void + { + if ($blacklist !== null) { + putenv("IP_BLACKLIST=$blacklist"); + } + $_SERVER['REMOTE_ADDR'] = $ip; + $middleware = new IpFilterMiddleware($this->logger); + + $request = $this->createRequest(['REMOTE_ADDR' => $ip]); + $response = new Response(); + $handler = $this->createHandler($response); + + $this->logger->expectLogInfo('Request processed successfully', [ + 'uri' => 'http://localhost/test' + ]); + + $result = $middleware->process($request, $handler); + $this->assertSame($response, $result); + }
85-133: Consider using data providers for blocked IP test cases.Similar to the previous suggestion, the blocked IP test cases (
testBlocksBlacklistedIPv4,testBlocksBlacklistedIPv6) could be combined using a data provider.Here's how you could refactor this:
+ /** + * @return array<string, array{string, string}> + */ + public function blockedIpTestCasesProvider(): array + { + return [ + 'blocked_ipv4' => ['192.168.1.1', '192.168.1.0/24'], + 'blocked_ipv6' => ['2001:db8:1::1', '2001:db8::/32'], + ]; + } + + /** + * @dataProvider blockedIpTestCasesProvider + */ + public function testBlocksBlacklistedIP(string $ip, string $blacklist): void + { + putenv("IP_BLACKLIST=$blacklist"); + $_SERVER['REMOTE_ADDR'] = $ip; + $middleware = new IpFilterMiddleware($this->logger); + + $request = $this->createRequest(['REMOTE_ADDR' => $ip]); + $response = new Response(); + $handler = $this->createHandler($response); + + $this->logger->expectLogInfo('Access denied - IP blacklisted', [ + 'ip' => $ip, + 'uri' => 'http://localhost/test' + ]); + + $result = $middleware->process($request, $handler); + $responseData = json_decode((string)$result->getBody(), true); + + $this->assertEquals(ErrorMessages::get('ipBlacklisted')['statusCode'], $result->getStatusCode()); + $this->assertEquals( + ['errors' => [ErrorMessages::get('ipBlacklisted')]], + $responseData + ); + }
135-170: Consider using constants for test data and error messages.The invalid IP test cases could benefit from using constants for test data and improving the assertions for error messages.
+ private const INVALID_IP = 'invalid-ip'; + private const VALID_IP = '192.168.1.2'; + public function testHandlesInvalidIP(): void { putenv('IP_BLACKLIST=192.168.1.1'); - $_SERVER['REMOTE_ADDR'] = 'invalid-ip'; + $_SERVER['REMOTE_ADDR'] = self::INVALID_IP; $middleware = new IpFilterMiddleware($this->logger); - $request = $this->createRequest(['REMOTE_ADDR' => 'invalid-ip']); + $request = $this->createRequest(['REMOTE_ADDR' => self::INVALID_IP]); $response = new Response(); $handler = $this->createHandler($response); $this->logger->expectLogInfo('Invalid IP address detected', [ - 'ip' => 'invalid-ip', + 'ip' => self::INVALID_IP, 'uri' => 'http://localhost/test' ]);zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (2)
20-27: Add validation for CORS environment variable.Consider validating the CORS environment variable to ensure it contains valid URLs and is not malformed.
public function __construct(LoggerService $logger) { $this->logger = $logger; $corsEnv = getenv('CORS'); if ($corsEnv) { + // Validate CORS environment variable format + if (!preg_match('/^[\w\-\.]+(:\d+)?(,[\w\-\.]+(:\d+)?)*$/', $corsEnv)) { + throw new \InvalidArgumentException('Invalid CORS configuration format'); + } $this->whitelist = array_map('trim', explode(',', $corsEnv)); + // Validate each origin is a valid URL + foreach ($this->whitelist as $origin) { + if (!filter_var($origin, FILTER_VALIDATE_URL)) { + throw new \InvalidArgumentException("Invalid origin URL: {$origin}"); + } + } } }
29-85: Reduce duplication of CORS headers.Consider extracting the common CORS headers into a private method to avoid duplication between OPTIONS and regular responses.
+ private function addCorsHeaders(ResponseInterface $response, string $origin): ResponseInterface + { + return $response + ->withHeader('Access-Control-Allow-Origin', $origin) + ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS') + ->withHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-CSRF-Token') + ->withHeader('Access-Control-Allow-Credentials', 'true') + ->withHeader('Access-Control-Max-Age', '86400'); + } public function process( ServerRequestInterface $request, RequestHandlerInterface $handler ): ResponseInterface { try { // ... existing code ... if ($request->getMethod() === 'OPTIONS') { $response = \App::$slim->getResponseFactory()->createResponse(200); - return $response - ->withHeader('Access-Control-Allow-Origin', $origin) - ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS') - ->withHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-CSRF-Token') - ->withHeader('Access-Control-Allow-Credentials', 'true') - ->withHeader('Access-Control-Max-Age', '86400'); + return $this->addCorsHeaders($response, $origin); } $response = $handler->handle($request); - return $response - ->withHeader('Access-Control-Allow-Origin', $origin) - ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS') - ->withHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-CSRF-Token') - ->withHeader('Access-Control-Allow-Credentials', 'true') - ->withHeader('Access-Control-Max-Age', '86400'); + return $this->addCorsHeaders($response, $origin); } catch (\Throwable $e) { // ... existing code ... } }zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php (2)
16-32: Remove unnecessary cache clearing.The cache clearing in setUp doesn't seem necessary for CORS middleware tests as it doesn't use caching.
protected function setUp(): void { parent::setUp(); \App::$source_name = 'unittest'; - - if (\App::$cache) { - \App::$cache->clear(); - } putenv('CORS=http://localhost:8080'); $this->middleware = new CorsMiddleware($this->logger); }
70-81: Add more thorough CORS header validation.Consider validating all CORS headers in the response, not just Origin and Methods.
public function testAllowsWhitelistedOrigin(): void { $request = $this->createRequest(['Origin' => 'http://localhost:8080']); $response = new Response(); $handler = $this->createHandler($response); $result = $this->middleware->process($request, $handler); $this->assertEquals('http://localhost:8080', $result->getHeaderLine('Access-Control-Allow-Origin')); $this->assertNotEmpty($result->getHeaderLine('Access-Control-Allow-Methods')); + $this->assertNotEmpty($result->getHeaderLine('Access-Control-Allow-Headers')); + $this->assertEquals('true', $result->getHeaderLine('Access-Control-Allow-Credentials')); + $this->assertEquals('86400', $result->getHeaderLine('Access-Control-Max-Age')); }zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (4)
22-23: Consider defining backoff constants in microsecondsThe backoff constants are defined in milliseconds but used with
usleep()which expects microseconds. This requires conversion at runtime.- private const BACKOFF_MIN = 10; // milliseconds - private const BACKOFF_MAX = 50; // milliseconds + private const BACKOFF_MIN_MICROS = 10000; // microseconds (10ms) + private const BACKOFF_MAX_MICROS = 50000; // microseconds (50ms)
24-24: Consider increasing lock timeoutA 1-second lock timeout might be too short in high-concurrency scenarios with network latency.
- private const LOCK_TIMEOUT = 1; // seconds + private const LOCK_TIMEOUT = 3; // seconds
180-184: Consider using @internal annotation for test-only methodsThe
isLockedmethod is public only for testing purposes. Consider using the@internalannotation to indicate this./** * For testing purposes - allows checking if a lock exists + * @internal */ public function isLocked(string $ip): bool
134-139: Enhance defensive programming in getCurrentRequestCountThe method should validate the timestamp to ensure the rate limit window hasn't expired.
private function getCurrentRequestCount(string $key): int { $requestData = $this->cache->get($key); - if (!is_array($requestData)) { + if (!is_array($requestData) || + !isset($requestData['count'], $requestData['timestamp']) || + $requestData['timestamp'] + self::TIME_WINDOW < time()) { return 0; } return (int)($requestData['count'] ?? 0); }zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (2)
22-38: Enhance error logging context.Consider adding more context to error logging to help with debugging.
- $this->logger->logError($e, $request); + $this->logger->logError($e, $request, null, [ + 'middleware' => self::class, + 'stage' => 'request_sanitization' + ]);
40-58: Add explicit handling for null parsed body.While the current implementation implicitly handles null parsed body by skipping it, it would be more maintainable to handle it explicitly.
$parsedBody = $request->getParsedBody(); - if (is_array($parsedBody)) { + if ($parsedBody === null) { + // Null is valid per PSR-7, no sanitization needed + } elseif (is_array($parsedBody)) { $sanitizedParsedBody = $this->sanitizeData($parsedBody); $request = $request->withParsedBody($sanitizedParsedBody); } elseif (is_object($parsedBody)) {zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (4)
391-392: Use ISO 8601 format for timestampsIn the
getTimestampmethod, the timestamp is formatted as'Y-m-d H:i:s'. Consider using the ISO 8601 format'c'to include timezone information and conform to international standards.Apply this change:
- self::$cachedTimestamp = date('Y-m-d H:i:s'); + self::$cachedTimestamp = date('c');
336-339: Ensure multibyte string handling consistencyIn the
writeLogmethod, you're usingmb_substrto truncate the message butstrlento get the message length. For consistency with multibyte encoding, consider usingmb_strleninstead ofstrlen.Apply this change:
- if (strlen($message) > self::MAX_MESSAGE_SIZE) { - $message = mb_substr($message, 0, self::MAX_MESSAGE_SIZE - 64) - . ' ... [truncated, full length: ' . strlen($message) . ']'; + if (mb_strlen($message) > self::MAX_MESSAGE_SIZE) { + $message = mb_substr($message, 0, self::MAX_MESSAGE_SIZE - 64) + . ' ... [truncated, full length: ' . mb_strlen($message) . ']'; }
174-179: Handle empty 'errors' array in responseIn the
logRequestmethod, when checking if the decoded response body contains an'errors'array, consider verifying that the'errors'array is not empty to ensure meaningful error logging.Modify the condition as follows:
- isset($decodedBody['errors']) && - is_array($decodedBody['errors']) + isset($decodedBody['errors']) && + is_array($decodedBody['errors']) && + !empty($decodedBody['errors'])
314-320: Clarify header casing in filtered headersIn the
filterSensitiveHeadersmethod, headers are stored with their original casing, but comparison is done using lowercase. This could lead to inconsistencies in log outputs if headers with different casing are treated differently. To maintain consistency, consider normalizing the header names.You could adjust the code like this:
foreach ($headers as $name => $values) { - $lower = strtolower($name); - if (in_array($lower, self::SENSITIVE_HEADERS, true)) { - $filtered[$name] = ['[REDACTED]']; - } elseif (in_array($lower, self::IMPORTANT_HEADERS, true)) { - $filtered[$name] = $values; + $lowerName = strtolower($name); + if (in_array($lowerName, self::SENSITIVE_HEADERS, true)) { + $filtered[$lowerName] = ['[REDACTED]']; + } elseif (in_array($lowerName, self::IMPORTANT_HEADERS, true)) { + $filtered[$lowerName] = $values; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
zmscitizenapi/src/Zmscitizenapi/Helper/ClientIpHelper.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Localization/ErrorMessages.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/CsrfMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSizeLimitMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/SecurityHeadersMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/ControllerTestCase.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/CsrfMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/RateLimitingMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/MiddlewareTestCase.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/TestLogger.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- zmscitizenapi/src/Zmscitizenapi/Helper/ClientIpHelper.php
- zmscitizenapi/tests/Zmscitizenapi/MiddlewareTestCase.php
- zmscitizenapi/src/Zmscitizenapi/Middleware/SecurityHeadersMiddleware.php
- zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSizeLimitMiddleware.php
- zmscitizenapi/tests/Zmscitizenapi/TestLogger.php
- zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/CsrfMiddlewareTest.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/RateLimitingMiddlewareTest.php
- zmscitizenapi/src/Zmscitizenapi/Middleware/CsrfMiddleware.php
🧰 Additional context used
📓 Learnings (6)
zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php (10)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should use Slim\Psr7\Response objects, follow the project's logging patterns with URI in context arrays, and use assertSame for response comparisons.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:37:28.068Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should create the middleware instance after setting environment variables in each test method, not in setUp(), to ensure correct blacklist configuration.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:42:27.947Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should create a new middleware instance in each test method after setting environment variables, not store it as a class property, to ensure correct blacklist configuration.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:04:28.814Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should use data providers for test cases and constants for test data to improve maintainability and readability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:00:46.107Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should clear environment variables in setUp before creating the middleware instance, and recreate the middleware after setting test-specific environment values.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:42:36.484Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should re-instantiate the middleware after setting environment variables to ensure they take effect, and clean up environment variables in tearDown().
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware in zmscitizenapi should follow the project's logging patterns by using logInfo with context arrays and proper logging expectations in tests.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:19:16.660Z
Learning: The IpFilterMiddleware in zmscitizenapi should use exact log message matches between middleware and tests, with consistent message format and context arrays.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware in zmscitizenapi should validate blacklist entries during parsing and handle both IPv4 and IPv6 addresses with proper CIDR notation support, using binary operations for network portion comparison.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T18:32:19.677Z
Learning: The IpFilterMiddleware in zmscitizenapi should only log success after both IP validation and blacklist checks pass, to ensure proper log message order and avoid false success logs.
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php:29-73
Timestamp: 2025-01-09T16:10:56.007Z
Learning: The CORS middleware in zmscitizenapi should handle preflight OPTIONS requests separately by returning a 200 response with CORS headers without processing the request further.
zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php (4)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php:70-81
Timestamp: 2025-01-09T15:52:26.749Z
Learning: When testing HTTP middleware that requires method manipulation (like CORS preflight requests), use real PSR-7 request objects instead of mocks to ensure full method support.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php:70-81
Timestamp: 2025-01-09T16:17:33.344Z
Learning: When creating Slim PSR-7 Request objects for testing, wrap headers in a Slim\Psr7\Headers object instead of passing a raw array (e.g., new \Slim\Psr7\Headers(['Origin' => 'value'])).
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php:70-81
Timestamp: 2025-01-09T16:43:50.977Z
Learning: When creating Slim PSR-7 Request objects for testing, use Slim\Psr7\Headers class to create a proper headers object instead of passing a plain array (e.g., new \Slim\Psr7\Headers(['Origin' => 'value'])).
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php:70-81
Timestamp: 2025-01-09T16:04:54.038Z
Learning: When creating Slim PSR-7 URI objects for testing, provide both the scheme and URI string to the constructor (e.g., new \Slim\Psr7\Uri('http', 'localhost/test')).
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (9)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:22:02.546Z
Learning: Rate limiting implementations should use retry mechanisms with random backoff when incrementing counters to handle race conditions in high-concurrency environments, while maintaining proper error handling and logging.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:21:02.931Z
Learning: Rate limiting implementations should use a get-and-set pattern with retries and random backoff when atomic operations are not available, while including proper error handling, logging, and rate limit headers in the response.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:34:12.274Z
Learning: Rate limiting implementations using PSR SimpleCache should rely only on standard interface methods (get/set) with retries and random backoff, as atomic operations like increment() are not part of the interface.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:30:58.599Z
Learning: Rate limiting implementations should use atomic increment operations when available, falling back to a get-and-set pattern with retries and random backoff to handle race conditions in high-concurrency environments.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T18:02:10.507Z
Learning: Rate limiting implementations should use the strategy pattern to separate retry mechanisms from core logic, allowing tests to use a simple implementation while production uses a robust retry-enabled implementation. This maintains clean separation of concerns and testability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:55:16.490Z
Learning: Rate limiting implementations should differentiate between test and production environments, using simplified logic for tests while maintaining robust concurrency handling (like retries with random backoff) in production to handle race conditions.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:59:16.545Z
Learning: When implementing retry mechanisms for race condition safety, consider using environment detection to disable retries in test environments while maintaining them in production. This balances the need for robust production code with maintainable tests.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:44:22.403Z
Learning: When implementing retry mechanisms in code that needs unit testing, consider providing a way to disable retries in test environments or ensure the test mocks are properly configured to handle retry scenarios. This helps balance real-world robustness with test maintainability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:39:11.369Z
Learning: When implementing rate limiting, the reset time calculation should be encapsulated within the getResetTime() method rather than passing it as an argument, to maintain a clean separation of concerns and prevent redundant calculations.
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (7)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:338-355
Timestamp: 2025-01-08T09:24:55.688Z
Learning: When using Symfony Cache (PSR-16), cache keys must not contain reserved characters {}()/\@:. Use dots (.) for separating parts of cache keys instead.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:338-355
Timestamp: 2025-01-08T09:17:37.328Z
Learning: Rate limiting in PHP applications should leverage existing cache infrastructure (like Symfony Cache) instead of introducing new dependencies, ensuring consistency with the application's caching strategy.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:309-326
Timestamp: 2025-01-07T14:14:44.222Z
Learning: Rate limiting in multi-process PHP applications should use a shared storage like Redis instead of static properties to ensure consistent rate limiting across all processes.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:229-230
Timestamp: 2025-01-07T14:16:22.061Z
Learning: In the LoggerService, response bodies should only be logged when the response has a 4xx/5xx status code AND contains a JSON response with an "errors" array. Example error format: {"errors":[{"errorCode":"string","errorMessage":"string","statusCode":number}]}
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:0-0
Timestamp: 2025-01-07T13:00:55.929Z
Learning: The LoggerService in zmscitizenapi should redact sensitive information from headers (authorization, cookie, x-api-key, auth-key, authkey) and limit stack trace size to prevent huge log entries.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:0-0
Timestamp: 2025-01-07T13:39:45.641Z
Learning: The LoggerService in zmscitizenapi should extract client IP using ClientIpHelper and log it as a separate field along with User-Agent, rather than including it in headers, to better track potential misuse.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:168-174
Timestamp: 2025-01-07T13:45:46.077Z
Learning: The LoggerService should implement rate limiting, persistent syslog connections, and error handling for syslog operations to ensure robust logging in production environments. It should also include safeguards like message size limits, timestamp caching, and proper handling of sensitive data.
zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php:49-52
Timestamp: 2025-01-09T16:15:52.095Z
Learning: According to PSR-7 ServerRequestInterface, the parsed body must be of type array|object|null. String values are not allowed to be passed to withParsedBody().
🔇 Additional comments (13)
zmscitizenapi/src/Zmscitizenapi/Localization/ErrorMessages.php (3)
1-36: LGTM! Well-structured class with proper constant definitions.The class follows good practices with:
- Strict typing enabled
- Private constants for internal use
- Standard HTTP status codes
- Clear language configuration
625-671: LGTM! Robust error message retrieval with proper fallback handling.The implementation is well-structured with:
- Clean language selection using match expressions
- Proper fallback logic
- Generic error handling for unknown keys
673-695: LGTM! Implementation follows previous review recommendations.The method correctly implements the suggested validation logic from past reviews:
- Handles empty arrays
- Validates error structure
- Returns appropriate status codes
zmscitizenapi/tests/Zmscitizenapi/ControllerTestCase.php (1)
9-22: Implementation looks good!The
readFixturemethod is well-implemented with proper type hints, input validation, and security measures against directory traversal attacks.zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php (1)
1-11: LGTM! File structure follows best practices.The namespace and imports are properly organized, with strict types declared.
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (2)
87-90: LGTM!The method correctly uses strict comparison for origin validation.
81-84: LGTM!The error handling appropriately logs and re-throws exceptions, maintaining the middleware chain's error handling capabilities.
zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php (2)
34-68: LGTM!The tests thoroughly verify both successful and error cases, including proper logging and response validation.
82-107: LGTM!The test correctly uses real PSR-7 request objects and properly validates the CORS preflight response headers.
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (3)
1-185: Overall implementation looks solid!The rate limiting implementation is well-structured with proper error handling, distributed locking, and exponential backoff. The suggested improvements will make it more robust and maintainable.
113-117: 🛠️ Refactor suggestionEnhance cache data validation
The cache data validation should check for required array keys to prevent undefined index errors.
if (!is_array($requestData)) { // Handle corrupted data $this->cache->delete($key); return false; } + + if (!isset($requestData['count'], $requestData['timestamp'])) { + // Handle incomplete data + $this->cache->delete($key); + return false; + }Likely invalid or redundant comment.
87-87:⚠️ Potential issueFix off-by-one error in remaining requests calculation
The remaining requests calculation subtracts one extra request unnecessarily since
getCurrentRequestCount()already includes the current request.- $remaining = max(0, self::MAX_REQUESTS - $this->getCurrentRequestCount($key) - 1); + $remaining = max(0, self::MAX_REQUESTS - $this->getCurrentRequestCount($key));Likely invalid or redundant comment.
zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (1)
1-21: Well-structured class implementation!The class follows PSR-15 middleware interface requirements and implements proper dependency injection.
zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php
Outdated
Show resolved
Hide resolved
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php
Outdated
Show resolved
Hide resolved
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php
Outdated
Show resolved
Hide resolved
zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php
Outdated
Show resolved
Hide resolved
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (5)
14-19: Consider using a more specific type hint for the whitelist array.The whitelist array will only contain strings, so we can make this more type-safe.
- private array $whitelist = []; + /** @var string[] */ + private array $whitelist = [];
20-27: Add validation for malformed CORS environment variable.The current implementation assumes the CORS environment variable is properly formatted. Consider adding validation to handle malformed input.
public function __construct(LoggerService $logger) { $this->logger = $logger; $corsEnv = getenv('CORS'); if ($corsEnv) { + if (!is_string($corsEnv)) { + $this->logger->logError('CORS environment variable must be a string'); + return; + } $this->whitelist = array_map('trim', explode(',', $corsEnv)); + $this->whitelist = array_filter($this->whitelist); // Remove empty entries } }
38-41: Remove or uncomment logging code.Commented-out code should either be removed or uncommented if the logging is necessary.
66-71: Extract duplicated CORS headers into a private method.The CORS headers are duplicated between OPTIONS and regular responses. Consider extracting them into a private method.
+ private function addCorsHeaders(ResponseInterface $response, string $origin): ResponseInterface + { + return $response + ->withHeader('Access-Control-Allow-Origin', $origin) + ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS') + ->withHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-CSRF-Token') + ->withHeader('Access-Control-Allow-Credentials', 'true') + ->withHeader('Access-Control-Max-Age', '86400'); + } // In process method: - return $response - ->withHeader('Access-Control-Allow-Origin', $origin) - ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS') - ->withHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-CSRF-Token') - ->withHeader('Access-Control-Allow-Credentials', 'true') - ->withHeader('Access-Control-Max-Age', '86400'); + return $this->addCorsHeaders($response, $origin);Also applies to: 75-80
87-90: Consider normalizing URLs before comparison.URLs can vary in format (e.g., trailing slashes, protocol differences). Consider normalizing URLs before comparison.
private function isOriginAllowed(string $origin): bool { - return in_array($origin, $this->whitelist, true); + $normalizedOrigin = rtrim(strtolower($origin), '/'); + $normalizedWhitelist = array_map( + fn($url) => rtrim(strtolower($url), '/'), + $this->whitelist + ); + return in_array($normalizedOrigin, $normalizedWhitelist, true); }zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php (2)
121-127: Consider using error suppression alternativesThe code uses @ to suppress errors from inet_pton. Consider using null coalescing or try-catch instead.
- $ipBin = @inet_pton($ip); - $subnetBin = @inet_pton($subnet); - - if ($ipBin === false || $subnetBin === false || - strlen($ipBin) !== strlen($subnetBin)) { - return false; - } + try { + $ipBin = inet_pton($ip); + $subnetBin = inet_pton($subnet); + + if ($ipBin === false || $subnetBin === false || + strlen($ipBin) !== strlen($subnetBin)) { + return false; + } + } catch (\ErrorException $e) { + return false; + }
136-144: Add comments explaining the bit manipulationThe bit manipulation for CIDR mask creation is complex. Consider adding comments to explain the logic.
$bytes = strlen($ipBin); + // Create full bytes of 1s for the first n/8 bytes $mask = str_repeat("\xFF", (int)($bits / 8)); if ($bits % 8) { + // Create partial byte for remaining bits + // e.g., for /27, create 0b11100000 $mask .= chr(0xFF << (8 - ($bits % 8))); } + // Pad remaining bytes with 0s $mask = str_pad($mask, $bytes, "\x00");zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (2)
16-33: Add PHPDoc for class and constructor.The class and constructor lack documentation explaining the purpose and parameters.
+/** + * Middleware for implementing rate limiting based on client IP. + * Limits requests to MAX_REQUESTS per TIME_WINDOW seconds. + */ class RateLimitingMiddleware implements MiddlewareInterface { private const ERROR_RATE_LIMIT = 'rateLimitExceeded'; private const MAX_REQUESTS = 60; private const TIME_WINDOW = 60; private const MAX_RETRIES = 3; private const BACKOFF_MIN = 10; // milliseconds private const BACKOFF_MAX = 50; // milliseconds private const LOCK_TIMEOUT = 1; // seconds private CacheInterface $cache; private LoggerService $logger; + /** + * @param CacheInterface $cache Cache implementation for storing rate limit data + * @param LoggerService $logger Logger for recording rate limit events + */ public function __construct(CacheInterface $cache, LoggerService $logger)
181-185: Reconsider exposing lock status for testing.The
isLockedmethod exposes internal implementation details. Consider using a more abstract test interface or moving this to a test-specific class.- /** - * For testing purposes - allows checking if a lock exists - */ - public function isLocked(string $ip): bool - { - $lockKey = 'rate_limit_' . md5($ip) . '_lock'; - return $this->cache->has($lockKey); - } + /** + * @internal For testing purposes only + */ + public function getRequestState(string $ip): array + { + $key = 'rate_limit_' . md5($ip); + return [ + 'count' => $this->getCurrentRequestCount($key), + 'resetTime' => $this->getResetTime($key), + 'isLocked' => $this->cache->has($key . '_lock') + ]; + }zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (3)
43-61: Consider logging sanitization failures.While the method handles different input types correctly, it would be beneficial to log when sanitization fails or when malformed input is detected.
private function sanitizeRequest(ServerRequestInterface $request): ServerRequestInterface { // Sanitize query parameters - $queryParams = $request->getQueryParams(); - $sanitizedQueryParams = $this->sanitizeData($queryParams); - $request = $request->withQueryParams($sanitizedQueryParams); + try { + $queryParams = $request->getQueryParams(); + $sanitizedQueryParams = $this->sanitizeData($queryParams); + $request = $request->withQueryParams($sanitizedQueryParams); + } catch (\RuntimeException $e) { + $this->logger->logWarning('Query parameters sanitization failed', [ + 'error' => $e->getMessage(), + 'uri' => (string) $request->getUri() + ]); + throw $e; + } // Sanitize parsed body $parsedBody = $request->getParsedBody(); if (is_array($parsedBody)) { - $sanitizedParsedBody = $this->sanitizeData($parsedBody); - $request = $request->withParsedBody($sanitizedParsedBody); + try { + $sanitizedParsedBody = $this->sanitizeData($parsedBody); + $request = $request->withParsedBody($sanitizedParsedBody); + } catch (\RuntimeException $e) { + $this->logger->logWarning('Request body sanitization failed', [ + 'error' => $e->getMessage(), + 'uri' => (string) $request->getUri() + ]); + throw $e; + } } elseif (is_object($parsedBody)) { - $sanitizedParsedBody = $this->sanitizeObject($parsedBody); - $request = $request->withParsedBody($sanitizedParsedBody); + try { + $sanitizedParsedBody = $this->sanitizeObject($parsedBody); + $request = $request->withParsedBody($sanitizedParsedBody); + } catch (\RuntimeException $e) { + $this->logger->logWarning('Request body sanitization failed', [ + 'error' => $e->getMessage(), + 'uri' => (string) $request->getUri() + ]); + throw $e; + } } return $request; }
63-108: Consider using a custom exception class.Using a custom exception class would make it easier to distinguish sanitization-related errors from other runtime exceptions.
Add this class to handle sanitization-specific exceptions:
namespace BO\Zmscitizenapi\Exception; class SanitizationException extends \RuntimeException { public static function maxRecursionDepthExceeded(): self { return new self('Maximum recursion depth exceeded'); } public static function maxStringLengthExceeded(int $length): self { return new self("String exceeds maximum length of {$length} bytes"); } }Then update the exception throwing:
- throw new \RuntimeException('Maximum recursion depth exceeded'); + throw SanitizationException::maxRecursionDepthExceeded();
110-124: Enhance string validation and error messages.The string sanitization could be improved with more specific error messages and additional character set validation.
private function sanitizeString(string $value): string { if (strlen($value) > self::MAX_STRING_LENGTH) { - throw new \RuntimeException('String exceeds maximum length'); + throw new \RuntimeException(sprintf( + 'String exceeds maximum length of %d bytes (actual: %d)', + self::MAX_STRING_LENGTH, + strlen($value) + )); } - $value = preg_replace('/[\x00-\x1F\x7F]/u', '', $value); + // Remove control characters while preserving allowed whitespace + $value = preg_replace('/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/u', '', $value); $value = trim($value); if (!mb_check_encoding($value, 'UTF-8')) { $this->logger->logWarning('Invalid string encoding detected.', ['value' => $value]); - $value = mb_convert_encoding($value, 'UTF-8', 'auto'); + // Convert to UTF-8 and remove any invalid sequences + $value = mb_convert_encoding($value, 'UTF-8', 'UTF-8//IGNORE'); } + + // Ensure only printable characters remain + if (preg_match('/[^\P{C}\t\r\n]/u', $value)) { + $this->logger->logWarning('Non-printable characters detected', ['value' => $value]); + } + return $value; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Middleware/SecurityHeadersMiddleware.php(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/ControllerTestCase.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/RequestSanitizerMiddlewareTest.php(1 hunks)zmscitizenapi/tests/Zmscitizenapi/Middleware/SecurityHeadersMiddlewareTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- zmscitizenapi/src/Zmscitizenapi/Middleware/SecurityHeadersMiddleware.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/SecurityHeadersMiddlewareTest.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.php
- zmscitizenapi/tests/Zmscitizenapi/ControllerTestCase.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/RequestSanitizerMiddlewareTest.php
- zmscitizenapi/tests/Zmscitizenapi/Middleware/CorsMiddlewareTest.php
🧰 Additional context used
📓 Learnings (5)
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php:29-73
Timestamp: 2025-01-09T16:10:56.007Z
Learning: The CORS middleware in zmscitizenapi should handle preflight OPTIONS requests separately by returning a 200 response with CORS headers without processing the request further.
zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php:60-99
Timestamp: 2025-01-09T19:05:38.451Z
Learning: The RequestSanitizerMiddleware in zmscitizenapi should include protection against DoS attacks by limiting recursion depth to 10 levels and string length to 32KB.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php:49-52
Timestamp: 2025-01-09T16:15:52.095Z
Learning: According to PSR-7 ServerRequestInterface, the parsed body must be of type array|object|null. String values are not allowed to be passed to withParsedBody().
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (9)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:22:02.546Z
Learning: Rate limiting implementations should use retry mechanisms with random backoff when incrementing counters to handle race conditions in high-concurrency environments, while maintaining proper error handling and logging.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:21:02.931Z
Learning: Rate limiting implementations should use a get-and-set pattern with retries and random backoff when atomic operations are not available, while including proper error handling, logging, and rate limit headers in the response.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:34:12.274Z
Learning: Rate limiting implementations using PSR SimpleCache should rely only on standard interface methods (get/set) with retries and random backoff, as atomic operations like increment() are not part of the interface.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:30:58.599Z
Learning: Rate limiting implementations should use atomic increment operations when available, falling back to a get-and-set pattern with retries and random backoff to handle race conditions in high-concurrency environments.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T18:02:10.507Z
Learning: Rate limiting implementations should use the strategy pattern to separate retry mechanisms from core logic, allowing tests to use a simple implementation while production uses a robust retry-enabled implementation. This maintains clean separation of concerns and testability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:55:16.490Z
Learning: Rate limiting implementations should differentiate between test and production environments, using simplified logic for tests while maintaining robust concurrency handling (like retries with random backoff) in production to handle race conditions.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:59:16.545Z
Learning: When implementing retry mechanisms for race condition safety, consider using environment detection to disable retries in test environments while maintaining them in production. This balances the need for robust production code with maintainable tests.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T17:44:22.403Z
Learning: When implementing retry mechanisms in code that needs unit testing, consider providing a way to disable retries in test environments or ensure the test mocks are properly configured to handle retry scenarios. This helps balance real-world robustness with test maintainability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php:71-79
Timestamp: 2025-01-09T16:39:11.369Z
Learning: When implementing rate limiting, the reset time calculation should be encapsulated within the getResetTime() method rather than passing it as an argument, to maintain a clean separation of concerns and prevent redundant calculations.
zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php (13)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware in zmscitizenapi should validate blacklist entries during parsing and handle both IPv4 and IPv6 addresses with proper CIDR notation support, using binary operations for network portion comparison.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T18:43:16.123Z
Learning: The IpFilterMiddleware in zmscitizenapi should validate raw IP from request server params before using ClientIpHelper, since ClientIpHelper returns '127.0.0.1' as fallback for invalid IPs.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T18:32:19.677Z
Learning: The IpFilterMiddleware in zmscitizenapi should only log success after both IP validation and blacklist checks pass, to ensure proper log message order and avoid false success logs.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware in zmscitizenapi should handle both IPv4 and IPv6 addresses using inet_pton() for proper IP comparison and CIDR range checking.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should use Slim\Psr7\Response objects, follow the project's logging patterns with URI in context arrays, and use assertSame for response comparisons.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:52:09.022Z
Learning: The IpFilterMiddleware in zmscitizenapi should use context arrays for logging with 'uri' and 'ip' keys, and log successful requests with 'Request processed successfully' message.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:32:44.243Z
Learning: The IpFilterMiddleware in zmscitizenapi should follow the project's logging patterns by using logInfo with context arrays and proper logging expectations in tests.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:42:27.947Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should create a new middleware instance in each test method after setting environment variables, not store it as a class property, to ensure correct blacklist configuration.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:37:28.068Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should create the middleware instance after setting environment variables in each test method, not in setUp(), to ensure correct blacklist configuration.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T16:42:36.484Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should re-instantiate the middleware after setting environment variables to ensure they take effect, and clean up environment variables in tearDown().
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:04:28.814Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should use data providers for test cases and constants for test data to improve maintainability and readability.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:00:46.107Z
Learning: The IpFilterMiddleware tests in zmscitizenapi should clear environment variables in setUp before creating the middleware instance, and recreate the middleware after setting test-specific environment values.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php:93-107
Timestamp: 2025-01-09T17:19:16.660Z
Learning: The IpFilterMiddleware in zmscitizenapi should use exact log message matches between middleware and tests, with consistent message format and context arrays.
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (7)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:338-355
Timestamp: 2025-01-08T09:24:55.688Z
Learning: When using Symfony Cache (PSR-16), cache keys must not contain reserved characters {}()/\@:. Use dots (.) for separating parts of cache keys instead.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:338-355
Timestamp: 2025-01-08T09:17:37.328Z
Learning: Rate limiting in PHP applications should leverage existing cache infrastructure (like Symfony Cache) instead of introducing new dependencies, ensuring consistency with the application's caching strategy.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:229-230
Timestamp: 2025-01-07T14:16:22.061Z
Learning: In the LoggerService, response bodies should only be logged when the response has a 4xx/5xx status code AND contains a JSON response with an "errors" array. Example error format: {"errors":[{"errorCode":"string","errorMessage":"string","statusCode":number}]}
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:309-326
Timestamp: 2025-01-07T14:14:44.222Z
Learning: Rate limiting in multi-process PHP applications should use a shared storage like Redis instead of static properties to ensure consistent rate limiting across all processes.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:0-0
Timestamp: 2025-01-07T13:00:55.929Z
Learning: The LoggerService in zmscitizenapi should redact sensitive information from headers (authorization, cookie, x-api-key, auth-key, authkey) and limit stack trace size to prevent huge log entries.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:0-0
Timestamp: 2025-01-07T13:39:45.641Z
Learning: The LoggerService in zmscitizenapi should extract client IP using ClientIpHelper and log it as a separate field along with User-Agent, rather than including it in headers, to better track potential misuse.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#674
File: zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php:168-174
Timestamp: 2025-01-07T13:45:46.077Z
Learning: The LoggerService should implement rate limiting, persistent syslog connections, and error handling for syslog operations to ensure robust logging in production environments. It should also include safeguards like message size limits, timestamp caching, and proper handling of sensitive data.
🔇 Additional comments (12)
zmscitizenapi/src/Zmscitizenapi/Middleware/CorsMiddleware.php (1)
1-13: LGTM! Well-structured file with proper imports.The file follows PSR standards with strict typing enabled and includes all necessary imports for PSR interfaces and internal services.
zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.php (1)
1-26: LGTM! Well-structured class with proper dependencies.The class is well-organized with:
- Strict typing enabled
- Proper namespace and imports
- Clear constants for error codes and IP bits
- Type-hinted constructor
zmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.php (2)
65-68:⚠️ Potential issuePrevent integer overflow in exponential backoff calculation.
The exponential backoff calculation could overflow for large attempt numbers. The current check only prevents overflow in multiplication with BACKOFF_MIN.
- $backoffMs = min( - self::BACKOFF_MAX, - (int)(self::BACKOFF_MIN * min(pow(2, $attempt), PHP_INT_MAX / self::BACKOFF_MIN)) - ); + // Cap the exponential value first to prevent overflow + $exponential = min(pow(2, $attempt), PHP_INT_MAX / self::BACKOFF_MIN); + $backoffMs = min( + self::BACKOFF_MAX, + (int)(self::BACKOFF_MIN * $exponential) + );Likely invalid or redundant comment.
125-129: 🛠️ Refactor suggestionImprove atomicity of counter updates.
The current implementation using get-then-set is not truly atomic. While we can't use atomic operations with PSR SimpleCache, we can improve the robustness by using optimistic locking.
// Update the counter atomically + $version = $requestData['version'] ?? 0; + $requestData['version'] = $version + 1; $requestData['count'] = $count + 1; $requestData['timestamp'] = time(); - $this->cache->set($key, $requestData, self::TIME_WINDOW); + // Only update if version matches + if (!$this->cache->get($key . '_version') === $version) { + return false; // Retry needed + } + $success = $this->cache->set($key, $requestData, self::TIME_WINDOW); + if ($success) { + $this->cache->set($key . '_version', $version + 1, self::TIME_WINDOW); + } + return $success;Likely invalid or redundant comment.
zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.php (4)
1-19: Well-structured class with security measures!The class follows PSR standards and includes proper security measures against DoS attacks with defined limits for recursion depth and string length.
20-24: LGTM! Clean dependency injection.The constructor properly injects the LoggerService dependency.
32-34: Uncomment or remove the logging code.The commented logging code should either be uncommented if it's needed for debugging/monitoring purposes or removed if it's no longer required.
Do you want me to help determine if this logging is needed by checking the logging patterns in other middleware classes?
✅ Verification successful
Remove the commented logging code
The commented logging code should be removed as it appears to be part of a systematic change where request logging was intentionally disabled across middleware components. IpFilterMiddleware shows the same pattern of commented-out logging code, indicating this was a deliberate decision rather than an oversight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other middleware classes log similar information rg -l "logInfo.*Request" --type phpLength of output: 177
Script:
#!/bin/bash # Check the logging context in IpFilterMiddleware rg "logInfo.*Request" -A 3 -B 3 zmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.phpLength of output: 328
1-125: Verify security measures against request smuggling.While the middleware implements good sanitization practices, consider adding protection against HTTP request smuggling attacks.
Add these security headers in the middleware to prevent request smuggling:
public function process( ServerRequestInterface $request, RequestHandlerInterface $handler ): ResponseInterface { try { $request = $this->sanitizeRequest($request); + // Prevent request smuggling + if ($request->hasHeader('Transfer-Encoding')) { + throw new \RuntimeException('Transfer-Encoding header not allowed'); + } + if ($request->hasHeader('Content-Length') && $request->hasHeader('Content-Encoding')) { + throw new \RuntimeException('Content-Length and Content-Encoding headers cannot be used together'); + } return $handler->handle($request); } catch (\Throwable $e) { $this->logger->logError($e, $request); throw $e; } }✅ Verification successful
The review comment is accurate and the suggested security measures are necessary.
The codebase currently lacks protection against HTTP request smuggling attacks. While
RequestSizeLimitMiddlewarehandles payload size restrictions, it doesn't validate potentially dangerous header combinations. The suggested security measures would complement the existing checks without causing conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for middleware classes and their security-related header checks ast-grep --pattern 'class $_ implements MiddlewareInterface { $$$ }' # Search for header validation patterns rg -l -i "transfer-encoding|content-length|content-encoding" --type php # Search for security middleware configurations rg -l -i "security|middleware|header" "*.php"Length of output: 513
Script:
#!/bin/bash # Check the implementation of RequestSizeLimitMiddleware cat zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSizeLimitMiddleware.php # Search for header validation patterns in the middleware rg -A 5 -B 5 "getHeader|hasHeader|withHeader" zmscitizenapi/src/Zmscitizenapi/Middleware/RequestSizeLimitMiddleware.phpLength of output: 3107
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php (4)
398-413: Verify PHP version compatibility forJSON_INVALID_UTF8_SUBSTITUTEThe
json_encode()optionJSON_INVALID_UTF8_SUBSTITUTEused inencodeJson()requires PHP 7.2 or higher. If your application supports older PHP versions, this could lead to compatibility issues.Please confirm that the minimum PHP version requirement for the application is PHP 7.2 or higher. If not, consider removing this option or implementing an alternative method to handle invalid UTF-8 characters.
310-322: Confirm the omission of non-important headersIn
filterSensitiveHeaders(), headers not listed in eitherSENSITIVE_HEADERSorIMPORTANT_HEADERSare omitted from the logs. While this reduces log verbosity, it may exclude headers that could be useful for debugging.Please verify if excluding these headers is intentional. If additional headers are needed for troubleshooting, consider adding them to the
IMPORTANT_HEADERSarray.
337-339: Ensure thembstringextension is availableThe use of
mb_substr()inwriteLog()requires thembstringPHP extension. If the extension is not enabled in the runtime environment, this will cause a fatal error.Verify that the
mbstringextension is installed and enabled on all environments where the application runs. If not, you can replacemb_substr()withsubstr(), assuming the log messages contain only ASCII characters, or include a check and fallback mechanism.
342-345: 🛠️ Refactor suggestionAvoid using the error suppression operator
@Using the
@operator suppresses errors and warnings, which can make debugging difficult. InwriteLog(), the@syslogcall may hide potential issues when writing to the syslog.Remove the
@operator to allow errors to surface, and handle them appropriately. This ensures that any issues with logging are not silently ignored.Apply this diff to update the code:
- if (@syslog($priority, $message) === false) { + if (syslog($priority, $message) === false) { error_log('Failed to write to syslog'); error_log($message); }Also, review other instances where the
@operator is used, such as in theinit()method on line 52.Likely invalid or redundant comment.
zmscitizenapi/src/Zmscitizenapi/Services/Core/LoggerService.php
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Short description or comments
Reference
Issues #XXX
Summary by CodeRabbit
Release Notes for zmscitizenapi
New Features
Improvements
Security Enhancements
Documentation
Performance