From e0e385dd8b2fc60e5a72b5355b692859bd8889fe Mon Sep 17 00:00:00 2001 From: fabcocco Date: Fri, 12 Jun 2026 14:26:24 +0200 Subject: [PATCH] Fix cache-key asymmetry, cache TOCTOU, response decode robustness, encoding leak Four robustness fixes in BaseApi, found during an audit of a consuming application (data-hub ERPlus gateway): 1. Cache-key asymmetry: getCacheKey() built the key from the raw endpoint (with ":param" placeholders) plus the request data. getEndpoint() consumes ":param" entries from the request data while resolving the URL, so the lookup key (computed before resolution) and the store key (computed after) never matched for keyed endpoints - their cache could never hit. The key is now built from the resolved endpoint. 2. Cache TOCTOU: loadResponseFromCache() used Cache::has() followed by Cache::get(); an entry expiring between the two calls assigned null to the typed array|object $response property and crashed with a TypeError. Now a single get() with a sentinel default. 3. Response decode robustness: setResponse() assigned json_decode() output directly to the typed property. Invalid JSON or empty bodies (null) and JSON scalar bodies (e.g. a bare string from an RPC-style action) crashed with an opaque TypeError. Invalid JSON now throws a descriptive exception, empty bodies become an empty object, and scalars are wrapped as (object)['value' => ...]. 4. Temporary request-encoding leak: setTemporaryRequestEncoding() was only restored inside setResponse(), so a transport failure (connection timeout) before a response leaked the temporary encoding into the next call. executeCall() now restores it on any throw. Also casts $requestEncoding for strtolower() to avoid the PHP 8.4 "passing null to non-nullable" deprecation (the property is nullable). Co-Authored-By: Claude Fable 5 --- src/BaseApi.php | 72 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/src/BaseApi.php b/src/BaseApi.php index a1f2c98..1ec8773 100644 --- a/src/BaseApi.php +++ b/src/BaseApi.php @@ -86,7 +86,7 @@ protected function authenticateRequest(): self protected function applyRequestEncoding(PendingRequest $request): PendingRequest { - switch (strtolower($this->requestEncoding)) { + switch (strtolower((string) $this->requestEncoding)) { case 'asform': case 'form': case 'x-www-form-urlencoded': @@ -128,12 +128,20 @@ protected function executeCall(): self return $this; } - return $this - ->makeRequest() - ->authenticateRequest() - ->setResponse( - $this->request->{$this->requestMethod}($this->getEndpoint(), $this->getRequestData()) - ); + try { + return $this + ->makeRequest() + ->authenticateRequest() + ->setResponse( + $this->request->{$this->requestMethod}($this->getEndpoint(), $this->getRequestData()) + ); + } catch (\Throwable $e) { + // a temporary request encoding must not leak into the next call + // when the request itself fails (e.g. a connection timeout) + $this->restoreRequestEncoding(); + + throw $e; + } } protected function fetchBearerToken(array $requestData = [], string $requestMethod = 'post'): self @@ -175,7 +183,11 @@ protected function getCacheKey(): string throw new Exception('no endpoint was specified'); } - return static::class.':'.$this->baseUrl.'/'.$this->endpoint.':'.md5(serialize($this->requestData)); + // use the resolved endpoint: getEndpoint() consumes ":param" entries from + // the request data, so building the key from the raw endpoint before and + // after resolution would produce two different keys for the same call + // (cache lookups for keyed endpoints could then never hit) + return static::class.':'.$this->baseUrl.'/'.$this->getEndpoint().':'.md5(serialize($this->requestData)); } protected function getHeaders(): array @@ -223,11 +235,22 @@ protected function getTokenCacheKey(): string protected function loadResponseFromCache(): bool { - if ($existsInCache = $this->useCache && Cache::has($this->getCacheKey())) { - $this->response = Cache::get($this->getCacheKey()); + if (!$this->useCache) { + return false; } - return $existsInCache; + // single read instead of has()+get(): an entry expiring between the two + // calls would assign null to the typed $response property (TypeError) + $miss = new \stdClass; + $cached = Cache::get($this->getCacheKey(), $miss); + + if ($cached === $miss || (!is_array($cached) && !is_object($cached))) { + return false; + } + + $this->response = $cached; + + return true; } protected function loadTokenFromCache(): bool @@ -306,11 +329,36 @@ protected function setResponse(Response $response): self $this ->restoreRequestEncoding() ->checkResponse($response) - ->response = json_decode($response->body()); + ->response = $this->decodeResponseBody($response); return $this->cacheResponse(); } + protected function decodeResponseBody(Response $response): array|object + { + $body = $response->body(); + + if (trim($body) === '') { + return new \stdClass; + } + + $decoded = json_decode($body); + + if (json_last_error() !== JSON_ERROR_NONE) { + // previously this assigned the failed decode result (null) to the + // typed $response property and crashed with an opaque TypeError + throw new Exception('response body is not valid JSON: '.json_last_error_msg()); + } + + if (!is_array($decoded) && !is_object($decoded)) { + // JSON scalar bodies (e.g. a bare string from an RPC-style action) + // are wrapped so the array|object contract keeps holding + return (object) ['value' => $decoded]; + } + + return $decoded; + } + protected function setTemporaryRequestEncoding(?string $encoding): self { $this->requestEncodingCache = $this->requestEncoding;