diff --git a/Classes/Exception/OriginNotAllowedException.php b/Classes/Exception/OriginNotAllowedException.php new file mode 100644 index 00000000..9b501dfb --- /dev/null +++ b/Classes/Exception/OriginNotAllowedException.php @@ -0,0 +1,33 @@ +statusCode(SymfonyResponse::HTTP_FORBIDDEN) + ->description(self::translate('exception.origin_not_allowed.title')); + } + + public function __construct(string $origin, string $method, int $code) + { + $this->title = self::translate('exception.origin_not_allowed.title'); + parent::__construct(self::translate('exception.origin_not_allowed.description', [$origin, $method]), $code); + } + + public function getStatusCode(): int + { + return Response::HTTP_FORBIDDEN; + } +} diff --git a/Classes/Exception/UnsupportedMediaTypeException.php b/Classes/Exception/UnsupportedMediaTypeException.php new file mode 100644 index 00000000..8388a5e1 --- /dev/null +++ b/Classes/Exception/UnsupportedMediaTypeException.php @@ -0,0 +1,38 @@ +statusCode(SymfonyResponse::HTTP_UNSUPPORTED_MEDIA_TYPE) + ->description(self::translate('exception.unsupported_media_type.title')); + } + + /** + * @param string $contentType Content type of the request, empty string when the header is missing + */ + public function __construct(string $contentType, int $code) + { + $this->title = self::translate('exception.unsupported_media_type.title'); + parent::__construct( + $contentType === '' + ? self::translate('exception.unsupported_media_type.missing') + : self::translate('exception.unsupported_media_type.description', [$contentType]), + $code + ); + } + + public function getStatusCode(): int + { + return Response::HTTP_UNSUPPORTED_MEDIA_TYPE; + } +} diff --git a/Classes/OperationHandler/AbstractOperationHandler.php b/Classes/OperationHandler/AbstractOperationHandler.php index a73a5c95..bdd27abb 100644 --- a/Classes/OperationHandler/AbstractOperationHandler.php +++ b/Classes/OperationHandler/AbstractOperationHandler.php @@ -10,6 +10,7 @@ use SourceBroker\T3api\Event\AfterDeserializeOperationEvent; use SourceBroker\T3api\Exception\BadRequestException; use SourceBroker\T3api\Exception\OperationNotAllowedException; +use SourceBroker\T3api\Exception\UnsupportedMediaTypeException; use SourceBroker\T3api\Security\OperationAccessChecker; use SourceBroker\T3api\Serializer\ContextBuilder\DeserializationContextBuilder; use SourceBroker\T3api\Service\CommonDateFieldsService; @@ -59,12 +60,14 @@ protected function getRepositoryForOperation(OperationInterface $operation): Com /** * @throws BadRequestException * @throws OperationNotAllowedException + * @throws UnsupportedMediaTypeException */ protected function deserializeOperation( OperationInterface $operation, Request $request, ?AbstractDomainObject $targetObject = null ): AbstractDomainObject { + $this->assertJsonContentType($request); $content = $request->getContent(); $this->assertJsonObject($content); @@ -87,6 +90,21 @@ protected function deserializeOperation( return $afterDeserializeOperationEvent->getObject(); } + /** + * Accepts `application/json`, `application/ld+json` and other `application/*+json` types. + * + * @throws UnsupportedMediaTypeException + */ + protected function assertJsonContentType(Request $request): void + { + $contentType = trim((string)$request->headers->get('Content-Type')); + $mediaType = strtolower(trim(explode(';', $contentType)[0])); + + if ($mediaType !== 'application/json' && preg_match('#^application/[a-z0-9.+-]+\+json$#', $mediaType) !== 1) { + throw new UnsupportedMediaTypeException($contentType, 1793646772); + } + } + /** * Checked before deserialization, as the serializer reports malformed JSON with a generic runtime exception. * diff --git a/Classes/Processor/OriginValidationProcessor.php b/Classes/Processor/OriginValidationProcessor.php new file mode 100644 index 00000000..5ad50720 --- /dev/null +++ b/Classes/Processor/OriginValidationProcessor.php @@ -0,0 +1,37 @@ +isMethodSafe() || !$this->corsService->isCorsRequest($request)) { + return; + } + + $origin = (string)$request->headers->get('Origin'); + if (!$this->corsService->isAllowedOrigin($origin, $this->corsService->getOptions())) { + throw new OriginNotAllowedException($origin, $request->getMethod(), 1793646771); + } + } +} diff --git a/Classes/Service/CorsService.php b/Classes/Service/CorsService.php index afea4d5b..b641216a 100644 --- a/Classes/Service/CorsService.php +++ b/Classes/Service/CorsService.php @@ -19,9 +19,24 @@ public function getOptions(): CorsOptions public function isCorsRequest(Request $request): bool { - $origin = $request->headers->get('Origin'); + $origin = (string)$request->headers->get('Origin'); + + return $origin !== '' && !$this->isSameOrigin($request, $origin); + } + + /** + * Besides the `Host` header, the origin is compared with the request URL normalized by TYPO3 (respecting + * reverse proxy configuration), which is the source of `SERVER_NAME`, `SERVER_PORT` and `HTTPS` in requests + * converted from PSR-7. Behind a reverse proxy the `Host` header may contain an internal host name. + */ + public function isSameOrigin(Request $request, string $origin): bool + { + $scheme = $request->isSecure() ? 'https' : 'http'; + $port = (int)$request->server->get('SERVER_PORT'); + $normalizedOrigin = $scheme . '://' . $request->server->get('SERVER_NAME') + . ($port === 0 || $port === ($scheme === 'https' ? 443 : 80) ? '' : ':' . $port); - return $origin !== null && $origin !== '' && $origin !== $request->getSchemeAndHttpHost(); + return in_array(strtolower($origin), [strtolower($request->getSchemeAndHttpHost()), strtolower($normalizedOrigin)], true); } public function isPreflightRequest(Request $request): bool diff --git a/Configuration/Services.yaml b/Configuration/Services.yaml index 87c898b9..0ea4a2eb 100644 --- a/Configuration/Services.yaml +++ b/Configuration/Services.yaml @@ -57,6 +57,9 @@ services: SourceBroker\T3api\Processor\CorsProcessor: public: true + SourceBroker\T3api\Processor\OriginValidationProcessor: + public: true + SourceBroker\T3api\Processor\LanguageProcessor: public: true diff --git a/Documentation/Cors/Index.rst b/Documentation/Cors/Index.rst index fbf9b70d..11498c24 100644 --- a/Documentation/Cors/Index.rst +++ b/Documentation/Cors/Index.rst @@ -70,8 +70,32 @@ Existing ``Vary`` values are preserved. These headers tell HTTP caches which req as described in the `Fetch specification `__. t3api's response cache stores the serialized content; CORS headers are evaluated for each request. -CORS controls whether a browser can read a response. It does not authorize an operation or provide CSRF -protection for writes authenticated with cookies. Configure operation access rules and CSRF protection separately. +.. _cors_write_requests: + +Write requests from other origins +================================= + +CORS controls whether a browser can read a response. Additionally t3api validates requests changing data +(all methods except ``GET``, ``HEAD``, ``OPTIONS`` and ``TRACE``) and rejects requests which: + +- are sent from another origin (``Origin`` header differs from the API origin) not allowed by ``allowOrigin`` - + response ``403 Forbidden``. Requests without ``Origin`` header (e.g. server-to-server requests) are not affected, + browsers send this header with every such request, +- contain a JSON body without JSON content type (``application/json``, ``application/ld+json`` or other + ``application/*+json``) - response ``415 Unsupported Media Type``. + +To allow write requests from another origin (e.g. frontend application on a different domain), add it to +``allowOrigin``. Setting ``allowOrigin`` to ``*`` allows write requests from every origin. + +The origin check is done by ``SourceBroker\T3api\Processor\OriginValidationProcessor``. If you need to accept +write requests from any origin without allowing them to read responses, remove the processor in ``ext_localconf.php`` +of your extension: + +.. code-block:: php + + unset($GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['t3api']['processors'][\SourceBroker\T3api\Processor\OriginValidationProcessor::class]); + +CORS does not authorize an operation - configure operation access rules separately, see :ref:`security`. Customizing CORS handling ========================= diff --git a/Documentation/Miscellaneous/Changelog/Index.rst b/Documentation/Miscellaneous/Changelog/Index.rst index 26ad9e9b..09a05f28 100644 --- a/Documentation/Miscellaneous/Changelog/Index.rst +++ b/Documentation/Miscellaneous/Changelog/Index.rst @@ -6,6 +6,7 @@ Changelog 5.0.0 ===== +- [!!!] 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 `. - Return HTTP 500 for PHP errors (e.g. ``TypeError``) as API response with CORS headers instead of rethrowing them to TYPO3. [`issue `__] diff --git a/Documentation/Operations/Index.rst b/Documentation/Operations/Index.rst index 4c71c9b6..a6e994f6 100644 --- a/Documentation/Operations/Index.rst +++ b/Documentation/Operations/Index.rst @@ -102,6 +102,12 @@ T3api supports all REST methods. Method of the operation is set with ``method`` .. important:: Notice that ``POST`` operation is a type of collection operations +``POST``, ``PUT`` and ``PATCH`` operations expect a JSON object in the request body sent with header +``Content-Type: application/json`` (``application/ld+json`` and other ``application/*+json`` types are accepted too). +A missing or other content type returns ``415 Unsupported Media Type``, malformed JSON returns ``400 Bad Request``. +File upload operations use ``multipart/form-data`` instead. Write requests sent by a browser from another origin +have to be allowed by the CORS configuration, see :ref:`cors_write_requests`. + .. _operations_slugs: Slug fields diff --git a/Resources/Private/Language/locallang.xlf b/Resources/Private/Language/locallang.xlf index 6e5718be..3f94b3b6 100644 --- a/Resources/Private/Language/locallang.xlf +++ b/Resources/Private/Language/locallang.xlf @@ -69,6 +69,21 @@ Header `%s` has to contain the ID of an available language, `%s` given + + Origin not allowed + + + Origin `%s` is not allowed to send `%s` requests + + + Unsupported media type + + + Content type `%s` is not supported, use `application/json` + + + Header `Content-Type` is required, use `application/json` + Internal Server Error diff --git a/Tests/Functional/Dispatcher/WriteRequestValidationDispatcherTest.php b/Tests/Functional/Dispatcher/WriteRequestValidationDispatcherTest.php new file mode 100644 index 00000000..f7e9c475 --- /dev/null +++ b/Tests/Functional/Dispatcher/WriteRequestValidationDispatcherTest.php @@ -0,0 +1,194 @@ +dispatch('POST', '/_api/articles', '{"title": "Rejected"}', ['Origin' => $origin]); + + $json = $this->assertError($response, 403, 'Origin not allowed'); + self::assertSame(sprintf('Origin `%s` is not allowed to send `POST` requests', $origin), $json['hydra:description']); + self::assertFalse($response->hasHeader('Access-Control-Allow-Origin')); + self::assertSame(0, $this->countArticles()); + } + + public static function rejectedOriginProvider(): array + { + return [ + 'other origin' => ['https://untrusted.example'], + 'other scheme of own host' => ['http://example.com'], + 'opaque origin' => ['null'], + ]; + } + + #[Test] + #[DataProvider('acceptedOriginProvider')] + public function writeRequestFromAllowedOriginOrWithoutOriginIsExecuted(array $headers): void + { + $response = $this->dispatch('POST', '/_api/articles', '{"title": "Created"}', $headers); + + self::assertSame(201, $response->getStatusCode(), (string)$response->getBody()); + self::assertSame(1, $this->countArticles()); + } + + public static function acceptedOriginProvider(): array + { + return [ + 'without origin' => [[]], + 'same origin' => [['Origin' => 'https://example.com']], + 'same origin behind reverse proxy' => [['Origin' => 'https://example.com', 'Host' => 'backend.internal:8080']], + 'allowed origin' => [['Origin' => 'https://client.example']], + ]; + } + + #[Test] + public function wildcardOriginAllowsWriteRequestFromAnyOrigin(): void + { + $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['t3api']['cors']['allowOrigin'] = '*'; + + $response = $this->dispatch('POST', '/_api/articles', '{"title": "Created"}', ['Origin' => 'https://untrusted.example']); + + self::assertSame(201, $response->getStatusCode(), (string)$response->getBody()); + } + + #[Test] + public function patchRequestFromNotAllowedOriginIsRejected(): void + { + $this->importCSVDataSet(__DIR__ . '/../Fixtures/articles.csv'); + + $response = $this->dispatch('PATCH', '/_api/articles/1', '{"title": "Rejected"}', ['Origin' => 'https://untrusted.example']); + + $this->assertError($response, 403, 'Origin not allowed'); + self::assertSame('Article without creation date', $this->getConnectionPool()->getConnectionForTable(self::ARTICLE_TABLE) + ->select(['title'], self::ARTICLE_TABLE, ['uid' => 1])->fetchOne()); + } + + #[Test] + public function readRequestFromNotAllowedOriginIsNotRejected(): void + { + $this->importCSVDataSet(__DIR__ . '/../Fixtures/articles.csv'); + + $response = $this->dispatch('GET', '/_api/articles/1', '', ['Origin' => 'https://untrusted.example'], null); + + self::assertSame(200, $response->getStatusCode(), (string)$response->getBody()); + self::assertFalse($response->hasHeader('Access-Control-Allow-Origin')); + } + + #[Test] + #[DataProvider('unsupportedContentTypeProvider')] + public function jsonBodyWithUnsupportedContentTypeIsRejected(?string $contentType, string $description): void + { + $response = $this->dispatch('POST', '/_api/articles', '{"title": "Rejected"}', [], $contentType); + + $json = $this->assertError($response, 415, 'Unsupported media type'); + self::assertSame($description, $json['hydra:description']); + self::assertSame(0, $this->countArticles()); + } + + public static function unsupportedContentTypeProvider(): array + { + return [ + 'text/plain' => ['text/plain', 'Content type `text/plain` is not supported, use `application/json`'], + 'form' => ['application/x-www-form-urlencoded', 'Content type `application/x-www-form-urlencoded` is not supported, use `application/json`'], + 'missing' => [null, 'Header `Content-Type` is required, use `application/json`'], + ]; + } + + #[Test] + #[DataProvider('supportedContentTypeProvider')] + public function jsonBodyWithJsonContentTypeIsAccepted(string $contentType): void + { + $response = $this->dispatch('POST', '/_api/articles', '{"title": "Created"}', [], $contentType); + + self::assertSame(201, $response->getStatusCode(), (string)$response->getBody()); + } + + public static function supportedContentTypeProvider(): array + { + return [ + 'application/json' => ['application/json'], + 'with charset' => ['application/json; charset=utf-8'], + 'uppercase' => ['Application/JSON'], + 'JSON-LD' => ['application/ld+json'], + 'JSON merge patch' => ['application/merge-patch+json'], + ]; + } + + private function assertError(ResponseInterface $response, int $status, string $title): array + { + $body = (string)$response->getBody(); + $json = json_decode($body, true, 512, JSON_THROW_ON_ERROR); + + self::assertSame($status, $response->getStatusCode(), $body); + self::assertSame($title, $json['hydra:title']); + + return $json; + } + + private function countArticles(): int + { + return $this->getConnectionPool()->getConnectionForTable(self::ARTICLE_TABLE)->count('*', self::ARTICLE_TABLE, []); + } + + private function dispatch( + string $method, + string $path, + string $body, + array $headers, + ?string $contentType = 'application/json' + ): ResponseInterface { + $stream = new Stream('php://temp', 'rw'); + $stream->write($body); + $stream->rewind(); + + $request = new ServerRequest('https://example.com' . $path, $method, $stream); + if ($contentType !== null) { + $request = $request->withHeader('Content-Type', $contentType); + } + foreach ($headers as $name => $value) { + $request = $request->withHeader($name, $value); + } + + $dispatcher = new Bootstrap( + $this->get(SerializerService::class), + $this->get(ApiResourceRepository::class), + $this->get(SerializationContextBuilder::class), + $this->get(DeserializationContextBuilder::class), + $this->get(EventDispatcherInterface::class), + $this->get(OperationResponseCache::class) + ); + + return $dispatcher->process($request); + } +} diff --git a/Tests/Unit/Service/CorsServiceTest.php b/Tests/Unit/Service/CorsServiceTest.php index 3aeab657..1362a9d0 100644 --- a/Tests/Unit/Service/CorsServiceTest.php +++ b/Tests/Unit/Service/CorsServiceTest.php @@ -26,6 +26,39 @@ protected function setUp(): void ]; } + #[Test] + #[DataProvider('sameOriginProvider')] + public function sameOriginIsDetectedAlsoBehindReverseProxy(string $origin, ?string $hostHeader, bool $sameOrigin): void + { + $request = Request::create('https://api.example/items', 'POST'); + if ($hostHeader !== null) { + $request->headers->set('Host', $hostHeader); + } + + self::assertSame($sameOrigin, (new CorsService())->isSameOrigin($request, $origin)); + self::assertSame(!$sameOrigin, (new CorsService())->isCorsRequest(self::withOrigin($request, $origin))); + } + + public static function sameOriginProvider(): array + { + return [ + 'same origin' => ['https://api.example', null, true], + 'case insensitive' => ['https://API.example', null, true], + 'internal host header' => ['https://api.example', 'backend.internal:8080', true], + 'other host' => ['https://client.example', null, false], + 'other scheme' => ['http://api.example', null, false], + 'other port' => ['https://api.example:8443', null, false], + ]; + } + + private static function withOrigin(Request $request, string $origin): Request + { + $request = clone $request; + $request->headers->set('Origin', $origin); + + return $request; + } + #[Test] #[DataProvider('actualRequestProvider')] public function actualResponsesFollowOriginPolicy(array $configuration, ?string $origin, bool $allowed): void diff --git a/ext_localconf.php b/ext_localconf.php index 3fa8e8e4..c470da89 100644 --- a/ext_localconf.php +++ b/ext_localconf.php @@ -26,6 +26,7 @@ static function () { $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['t3api']['processors'] = [ \SourceBroker\T3api\Processor\CorsProcessor::class => 100, + \SourceBroker\T3api\Processor\OriginValidationProcessor::class => 90, ]; $GLOBALS['TYPO3_CONF_VARS']['EXTCONF']['t3api']['apiResourcePathProviders'] = [