diff --git a/Classes/Service/ResponseCacheService.php b/Classes/Service/ResponseCacheService.php index 9d5ecd6c..03adfdda 100644 --- a/Classes/Service/ResponseCacheService.php +++ b/Classes/Service/ResponseCacheService.php @@ -67,8 +67,6 @@ public function buildEntryIdentifier(OperationInterface $operation, array $route http_build_query($allowedQueryParams), ]; - // An empty `identifierExpressions` array (the default) leaves the loop below a no-op, so - // entries built without this setting hash identically to before it existed. foreach ($responseCacheSettings->getIdentifierExpressions() as $identifierExpression) { $identifierExpressionValue = $this->evaluateIdentifierExpression($identifierExpression, $operation, $route, $request); if ($identifierExpressionValue === null) { @@ -77,11 +75,13 @@ public function buildEntryIdentifier(OperationInterface $operation, array $route $keyParts[] = $identifierExpressionValue; } + // serialize() keeps the parts apart - with a separator, a value containing it could produce + // the same key as different values (`a|b` + `c` and `a` + `b|c`). // md5() here is a non-cryptographic cache key derivation, not a security control - there is // no secret to protect and no adversarial value in collision resistance, since `security` is // still fully re-evaluated on every cache hit (see `OperationResponseCache`), so a collision // could not bypass access control. - return md5(implode('|', $keyParts)); // NOSONAR php:S4790 - weak hash algorithm, not used in a sensitive context here + return md5(serialize($keyParts)); // NOSONAR php:S4790 - weak hash algorithm, not used in a sensitive context here } /** @@ -189,14 +189,11 @@ public function evaluateStoreTagExpressions( ): ?array { $tags = []; foreach ($tagExpressions as $tagExpression) { - $tag = $this->evaluateStoreTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); - if ($tag === null) { + $expressionTags = $this->evaluateStoreTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); + if ($expressionTags === null) { return null; } - - if ($tag !== '') { - $tags[] = $tag; - } + array_push($tags, ...$expressionTags); } return $tags; @@ -225,10 +222,7 @@ public function evaluateFlushTagExpressions( ): array { $tags = []; foreach ($tagExpressions as $tagExpression) { - $tag = $this->evaluateFlushTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); - if ($tag !== null && $tag !== '') { - $tags[] = $tag; - } + array_push($tags, ...$this->evaluateFlushTagExpression($tagExpression, $operation, $route, $request, $additionalVariables)); } return array_values(array_unique($tags)); @@ -236,10 +230,11 @@ public function evaluateFlushTagExpressions( /** * Store-side single-expression evaluation: logs at error level and signals "do not store" - * (`null`) both when the expression itself fails and when it produces a non-empty tag the - * cache frontend rejects. + * (`null`) both when the expression itself fails and when it produces a tag the cache + * frontend rejects. * * @param array $additionalVariables + * @return string[]|null */ private function evaluateStoreTagExpression( string $tagExpression, @@ -247,9 +242,9 @@ private function evaluateStoreTagExpression( array $route, Request $request, array $additionalVariables = [] - ): ?string { + ): ?array { try { - $tag = $this->evaluateTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); + $tags = $this->evaluateTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); } catch (\Throwable $throwable) { $this->logger?->error('t3api response cache tag expression evaluation failed - entry not stored', [ 'tagExpression' => $tagExpression, @@ -259,24 +254,26 @@ private function evaluateStoreTagExpression( return null; } - if ($tag !== '' && !$this->isValidTag($tag)) { - $this->logger?->error('t3api response cache tag expression produced an invalid tag - entry not stored', [ - 'tagExpression' => $tagExpression, - 'tag' => $tag, - ]); + foreach ($tags as $tag) { + if (!$this->isValidTag($tag)) { + $this->logger?->error('t3api response cache tag expression produced an invalid tag - entry not stored', [ + 'tagExpression' => $tagExpression, + 'tag' => $tag, + ]); - return null; + return null; + } } - return $tag; + return $tags; } /** - * Flush-side single-expression evaluation: logs at warning level and signals "skip this tag" - * (`null`) both when the expression itself fails and when it produces a non-empty tag the - * cache frontend rejects - the other declared tags still flush. + * Flush-side single-expression evaluation: logs at warning level and skips the whole expression + * when it fails, or a single tag the cache frontend rejects - the other declared tags still flush. * * @param array $additionalVariables + * @return string[] */ private function evaluateFlushTagExpression( string $tagExpression, @@ -284,28 +281,29 @@ private function evaluateFlushTagExpression( array $route, Request $request, array $additionalVariables = [] - ): ?string { + ): array { try { - $tag = $this->evaluateTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); + $tags = $this->evaluateTagExpression($tagExpression, $operation, $route, $request, $additionalVariables); } catch (\Throwable $throwable) { $this->logger?->warning('t3api response cache tag expression evaluation failed - skipped', [ 'tagExpression' => $tagExpression, 'exception' => $throwable, ]); - return null; + return []; } - if ($tag !== '' && !$this->isValidTag($tag)) { + return array_values(array_filter($tags, function (string $tag) use ($tagExpression): bool { + if ($this->isValidTag($tag)) { + return true; + } $this->logger?->warning('t3api response cache tag expression produced an invalid tag - skipped', [ 'tagExpression' => $tagExpression, 'tag' => $tag, ]); - return null; - } - - return $tag; + return false; + })); } /** @@ -315,7 +313,11 @@ private function evaluateFlushTagExpression( * evaluation failure - the store and flush sides apply their own failure policy (see * `evaluateStoreTagExpression()` and `evaluateFlushTagExpression()`). * + * An array result gives one tag per item (e.g. UIDs of related records). Empty strings give no tag. + * * @param array $additionalVariables + * @return string[] + * @throws \UnexpectedValueException when the result or an array item can not be converted to a string */ private function evaluateTagExpression( string $tagExpression, @@ -323,11 +325,26 @@ private function evaluateTagExpression( array $route, Request $request, array $additionalVariables = [] - ): string { - return (string)$this->getConditionResolver()->evaluate( + ): array { + $result = $this->getConditionResolver()->evaluate( $tagExpression, $this->getConditionVariables($operation, $route, $request, $additionalVariables) ); + + $tags = []; + foreach (is_array($result) ? $result : [$result] as $value) { + if (!($value === null || is_scalar($value) || $value instanceof \Stringable)) { + throw new \UnexpectedValueException( + sprintf('Value of type `%s` can not be converted to a cache tag', get_debug_type($value)), + 1793650001 + ); + } + if ((string)$value !== '') { + $tags[] = (string)$value; + } + } + + return $tags; } /** @@ -401,18 +418,21 @@ protected function getAllowedParameterNames(OperationInterface $operation): arra * shared with requests it was meant to be kept separate from, so `null` here means "do not * cache this request" rather than "treat as empty". A null/false expression result, on the * other hand, casts to `''` and is a perfectly valid - if unhelpful - identifier contribution. + * A result which can not be converted to a string (see `normalizeIdentifierValue()`) fails + * closed the same way. */ protected function evaluateIdentifierExpression( string $identifierExpression, OperationInterface $operation, array $route, Request $request - ): ?string { + ): string|array|null { try { - return (string)$this->getConditionResolver()->evaluate( + $value = $this->getConditionResolver()->evaluate( $identifierExpression, $this->getConditionVariables($operation, $route, $request) ); + $identifierValue = $this->normalizeIdentifierValue($value); } catch (\Throwable $throwable) { $this->logger?->error('t3api response cache identifier expression evaluation failed', [ 'identifierExpression' => $identifierExpression, @@ -421,6 +441,55 @@ protected function evaluateIdentifierExpression( return null; } + + if ($identifierValue === null) { + $this->logger?->error('t3api response cache identifier expression result can not be converted to a string', [ + 'identifierExpression' => $identifierExpression, + 'type' => get_debug_type($value), + ]); + } + + return $identifierValue; + } + + /** + * Scalars and `Stringable` objects are converted to strings. Arrays (e.g. `frontend.user.userGroupIds`) + * stay arrays - so they never produce the same identifier as a string - with normalized and sorted + * items, so the order of items does not change the identifier. Returns null for values which can not + * be converted to a string (objects without `__toString()`, also inside arrays). + * + * @return string|array|null + */ + protected function normalizeIdentifierValue(mixed $value): string|array|null + { + if (is_array($value)) { + $items = []; + foreach ($value as $key => $item) { + $normalizedItem = $this->normalizeIdentifierValue($item); + if ($normalizedItem === null) { + return null; + } + $items[$key] = $normalizedItem; + } + // Sorted by keys first, so a list is detected regardless of the insertion order of its keys + // (`[1 => 'a', 0 => 'b']`). Default flags: `SORT_STRING` would put key 10 before key 2. + ksort($items); + if (array_is_list($items)) { + // Compares the unambiguous representation - a numeric comparison treats e.g. `01` and `1` + // as equal, which would keep their original order. + usort($items, static fn(string|array $a, string|array $b): int => strcmp(serialize($a), serialize($b))); + } else { + ksort($items, SORT_STRING); + } + + return $items; + } + + if ($value === null || is_scalar($value) || $value instanceof \Stringable) { + return (string)$value; + } + + return null; } /** diff --git a/Documentation/Miscellaneous/Changelog/Index.rst b/Documentation/Miscellaneous/Changelog/Index.rst index 09a05f28..6530b750 100644 --- a/Documentation/Miscellaneous/Changelog/Index.rst +++ b/Documentation/Miscellaneous/Changelog/Index.rst @@ -6,6 +6,7 @@ Changelog 5.0.0 ===== +- Fix response cache identifier for ``identifierExpressions`` returning an array (e.g. ``frontend.user.userGroupIds``) - array items are part of the identifier now. Tag expressions returning an array add a tag per item. See :ref:`response-cache`. - [!!!] Validate origin and content type of requests changing data: requests from another origin not allowed by CORS ``allowOrigin`` return ``403 Forbidden``, JSON body without JSON ``Content-Type`` returns ``415 Unsupported Media Type``. Add origins of frontend applications to ``allowOrigin`` and send ``Content-Type: application/json``. See :ref:`cors_write_requests`. - Fix response cache invalidation when the page cache is cleared from the backend (cache tags passed as a list were ignored, numeric tags caused errors). - Log unexpected exceptions with an event ID. Outside of ``Development`` context HTTP 500 response contains a generic message with this event ID instead of the exception message. See :ref:`ThrowableSubscriber `. diff --git a/Documentation/ResponseCache/Index.rst b/Documentation/ResponseCache/Index.rst index 49383bbb..6a4932af 100644 --- a/Documentation/ResponseCache/Index.rst +++ b/Documentation/ResponseCache/Index.rst @@ -51,7 +51,9 @@ configured separately, via the sibling request context beyond URL and declared filters) you **must** declare a suitable ``identifierExpressions`` entry (typically reading a TYPO3 Context aspect via ``context``, see "Cache key" below) - otherwise the - first user's response is served to everyone. + first user's response is served to everyone. If the response contains + records restricted by ``fe_group`` (also in relations), add + ``frontend.user.userGroupIds`` to ``identifierExpressions``. Configuration reference ======================== @@ -115,6 +117,31 @@ the error is logged, regardless of whether the other expressions would have evaluated fine. Leaving the array empty (the default) leaves the identifier exactly as it would be without this setting. +An expression may also return an array, e.g. ``groupIds`` above - the items +are sorted, so the same items in a different order produce the same +identifier. A result which can not be converted to a string (an object +without ``__toString()``) is treated like a failed expression. + +For records restricted by ``fe_group`` add the group IDs of the current +frontend user: + +.. code-block:: php + + /** + * @ApiResource( + * attributes={ + * "cache"={ + * "identifierExpressions"={"frontend.user.userGroupIds"} + * } + * } + * ) + */ + +Each combination of frontend user groups gets separate cache entries; +anonymous visitors share one entry. Add it only to resources whose output +depends on user groups - every additional combination of groups multiplies +the number of stored entries. + .. note:: A non-empty ``cache`` block enables caching automatically - there is no need to add ``"enabled"=true`` yourself. An empty (or absent) ``cache`` @@ -559,6 +586,10 @@ All three are evaluated by the same expression engine and variable set as "Availability of ``object``" below for the full picture across every response cache expression on this page. +An expression returning an array adds one tag per non-empty item - e.g. a +result ``['category_1', 'category_2']`` adds both tags. A result (or array item) +which can not be converted to a string is treated like a failed expression. + The canonical route-driven case is a write handler that bypasses DataHandler/Extbase persistence and must flush a tag built from a matched route parameter (see :ref:`response-cache-invalidation` below for the full diff --git a/Tests/Unit/Service/ResponseCacheServiceTest.php b/Tests/Unit/Service/ResponseCacheServiceTest.php index ca19718d..3395419a 100644 --- a/Tests/Unit/Service/ResponseCacheServiceTest.php +++ b/Tests/Unit/Service/ResponseCacheServiceTest.php @@ -777,6 +777,227 @@ public function evaluateFlushTagExpressionsSkipsInvalidTagAndLogsWarning(): void ); } + #[Test] + public function identifierExpressionReturningArrayVariesByItemsButNotByTheirOrder(): void + { + $operation = $this->createOperation([ + 'identifierExpressions' => ["context.getPropertyFromAspect('frontend.user', 'groupIds', '')"], + ]); + + self::assertNotSame( + $this->buildIdentifierForGroups($operation, [0, -2, 1]), + $this->buildIdentifierForGroups($operation, [0, -2, 3]) + ); + self::assertSame( + $this->buildIdentifierForGroups($operation, [0, -2, 1, 3]), + $this->buildIdentifierForGroups($operation, [3, 1, -2, 0]) + ); + } + + #[Test] + public function identifierExpressionReturningAssociativeArrayDoesNotDependOnKeyOrder(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $service = new ResponseCacheService($this->cache, new Context()); + $request = Request::create('https://example.com/_api/books', 'GET'); + + self::assertSame( + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["{'a': 1, 'b': [2, 1]}"]]), [], $request), + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["{'b': [1, 2], 'a': 1}"]]), [], $request) + ); + } + + #[Test] + public function identifierExpressionReturningArrayDoesNotCollideWithStringOfSameRepresentation(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $service = new ResponseCacheService($this->cache, new Context()); + $request = Request::create('https://example.com/_api/books', 'GET'); + $build = fn(string $expression): ?string => $service->buildEntryIdentifier( + $this->createOperation(['identifierExpressions' => [$expression]]), + [], + $request + ); + + self::assertNotSame($build("['admin']"), $build("'" . serialize(['admin']) . "'")); + self::assertNotSame($build("[['admin']]"), $build("['" . serialize(['admin']) . "']")); + } + + #[Test] + public function identifierExpressionReturningArrayDoesNotDependOnOrderOfNumericStrings(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $service = new ResponseCacheService($this->cache, new Context()); + $request = Request::create('https://example.com/_api/books', 'GET'); + + self::assertSame( + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["['01', '1']"]]), [], $request), + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["['1', '01']"]]), [], $request) + ); + } + + #[Test] + public function identifierExpressionReturningArrayDoesNotDependOnInsertionOrderOfNumericKeys(): void + { + $operation = $this->createOperation(['identifierExpressions' => ['value']]); + $request = Request::create('https://example.com/_api/books', 'GET'); + $build = function (array $value) use ($operation, $request): ?string { + $this->registerPlainSymfonyExpressionResolver(['value' => $value]); + + return (new ResponseCacheService($this->cache, new Context()))->buildEntryIdentifier($operation, [], $request); + }; + + self::assertSame($build([0 => 'b', 1 => 'a']), $build([1 => 'a', 0 => 'b'])); + self::assertSame($build(range(0, 10)), $build(array_reverse(range(0, 10), true))); + } + + #[Test] + public function identifierExpressionResultFailingStringConversionFailsClosedAndLogsError(): void + { + $this->registerPlainSymfonyExpressionResolver(['value' => new class () implements \Stringable { + public function __toString(): string + { + throw new \RuntimeException('Conversion failed', 1793650002); + } + }]); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('error'); + + $service = new ResponseCacheService($this->cache, new Context()); + $service->setLogger($logger); + + self::assertNull($service->buildEntryIdentifier( + $this->createOperation(['identifierExpressions' => ['[value]']]), + [], + Request::create('https://example.com/_api/books', 'GET') + )); + } + + #[Test] + public function identifierExpressionReturningObjectWithoutStringRepresentationFailsClosedAndLogsError(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('error'); + + $service = new ResponseCacheService($this->cache, new Context()); + $service->setLogger($logger); + + self::assertNull($service->buildEntryIdentifier( + $this->createOperation(['identifierExpressions' => ['[t3apiOperation]']]), + [], + Request::create('https://example.com/_api/books', 'GET') + )); + } + + #[Test] + public function identifierExpressionValuesContainingSeparatorDoNotProduceSameIdentifier(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $service = new ResponseCacheService($this->cache, new Context()); + $request = Request::create('https://example.com/_api/books', 'GET'); + + self::assertNotSame( + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["'a|b'", "'c'"]]), [], $request), + $service->buildEntryIdentifier($this->createOperation(['identifierExpressions' => ["'a'", "'b|c'"]]), [], $request) + ); + } + + #[Test] + public function frontendUserGroupIdsIdentifierExpressionVariesByGroupsButNotByTheirOrder(): void + { + $operation = $this->createOperation(['identifierExpressions' => ['frontend.user.userGroupIds']]); + + self::assertNotSame( + $this->buildIdentifierForGroups($operation, [0, -1]), + $this->buildIdentifierForGroups($operation, [0, -2]) + ); + self::assertNotSame( + $this->buildIdentifierForGroups($operation, [0, -2, 1]), + $this->buildIdentifierForGroups($operation, [0, -2, 3]) + ); + self::assertSame( + $this->buildIdentifierForGroups($operation, [0, -2, 1, 3]), + $this->buildIdentifierForGroups($operation, [3, 1, -2, 0]) + ); + } + + #[Test] + public function tagExpressionReturningArrayGivesTagPerItem(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $this->cache->method('isValidTag')->willReturn(true); + $service = new ResponseCacheService($this->cache, new Context()); + $operation = $this->createOperation([]); + $request = Request::create('https://example.com/_api/books', 'GET'); + $tagExpressions = ["['category_1', '', 'category_2']", "'book'"]; + + self::assertSame( + ['category_1', 'category_2', 'book'], + $service->evaluateStoreTagExpressions($tagExpressions, $operation, [], $request) + ); + self::assertSame( + ['category_1', 'category_2', 'book'], + $service->evaluateFlushTagExpressions($tagExpressions, $operation, [], $request) + ); + } + + #[Test] + public function storeTagExpressionReturningObjectWithoutStringRepresentationFailsClosedAndLogsError(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $this->cache->method('isValidTag')->willReturn(true); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('error'); + + $service = new ResponseCacheService($this->cache, new Context()); + $service->setLogger($logger); + + self::assertNull($service->evaluateStoreTagExpressions( + ["'book'", 't3apiOperation'], + $this->createOperation([]), + [], + Request::create('https://example.com/_api/books', 'GET') + )); + } + + #[Test] + public function flushTagExpressionSkipsInvalidItemsAndExpressionsAndLogsWarning(): void + { + $this->registerPlainSymfonyExpressionResolver(); + $this->cache->method('isValidTag')->willReturnCallback(static fn(string $tag): bool => !str_contains($tag, ' ')); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::exactly(2))->method('warning'); + + $service = new ResponseCacheService($this->cache, new Context()); + $service->setLogger($logger); + + self::assertSame( + ['category_1', 'book'], + $service->evaluateFlushTagExpressions( + ["['category_1', 'invalid tag']", 't3apiOperation', "'book'"], + $this->createOperation([]), + [], + Request::create('https://example.com/_api/books', 'GET') + ) + ); + } + + /** + * @param int[] $groupIds + */ + private function buildIdentifierForGroups(CollectionOperation $operation, array $groupIds): ?string + { + $context = new Context(); + $context->setAspect('frontend.user', new UserAspect(null, $groupIds)); + if ($operation->getResponseCacheSettings()->getIdentifierExpressions() !== []) { + $this->registerPlainSymfonyExpressionResolver(['context' => $context]); + } + + return (new ResponseCacheService($this->cache, $context)) + ->buildEntryIdentifier($operation, [], Request::create('https://example.com/_api/books', 'GET')); + } + /** * Registers a `Resolver` double backed by a plain Symfony `ExpressionLanguage` (without * TYPO3's provider loading, which needs a booted TYPO3 instance) so the conditions are