diff --git a/src/Domain/Common/Providers/Http.php b/src/Domain/Common/Providers/Http.php index ce2435cdb..ca261c6be 100644 --- a/src/Domain/Common/Providers/Http.php +++ b/src/Domain/Common/Providers/Http.php @@ -34,24 +34,31 @@ final class Http { /** - * Check and force (if necessary) the HTTPS connection + * The HTTPS address this request should have been made to, or null if none is needed. + * + * This used to send the redirect itself, with a bare `header('Location: …')` — no status code, + * no exit, no check that headers had already gone out. A `Location` on a 200 is not a redirect: + * browsers ignore it, and execution carried on through the whole request, so the response the + * setting was meant to prevent — an account page, an API token — was still built and sent over + * the plaintext connection. "Force HTTPS" forced nothing. + * + * Answering with the address instead leaves the redirect to the caller, which is the only + * place that can also stop the request: HttpModuleBase::redirectToHttpsIfRequired() sends it + * through the router and throws, the way every other refusal in Init already does. */ - public static function checkHttps(ConfigDataInterface $configData, RequestService $request): void + public static function httpsUrlFor(ConfigDataInterface $configData, RequestService $request): ?string { - if ($configData->isHttpsEnabled() && !$request->isHttps()) { - $serverPort = $request->getServerPort(); + if (!$configData->isHttpsEnabled() || $request->isHttps()) { + return null; + } - $port = $serverPort !== 443 ? ':'.$serverPort : ''; - $host = str_replace('http', 'https', $request->getHttpHost()); + $serverPort = $request->getServerPort(); + $port = $serverPort !== 443 ? ':' . $serverPort : ''; - header( - sprintf( - 'Location: %s%s%s', - $host, - $port, - $_SERVER['REQUEST_URI'] ?? '' - ) - ); - } + // Only the scheme. str_replace('http', 'https', …) rewrote every occurrence, so a host + // with "http" in its name — http://httpbin.example — came back as https://httpsbin.example. + $host = preg_replace('#^http://#i', 'https://', $request->getHttpHost()) ?? ''; + + return sprintf('%s%s%s', $host, $port, $_SERVER['REQUEST_URI'] ?? ''); } } diff --git a/src/Infrastructure/Adapter/In/Api/Init.php b/src/Infrastructure/Adapter/In/Api/Init.php index 680608658..ac345824b 100644 --- a/src/Infrastructure/Adapter/In/Api/Init.php +++ b/src/Infrastructure/Adapter/In/Api/Init.php @@ -31,7 +31,6 @@ use SP\Infrastructure\HttpModuleBase; use SP\Infrastructure\Language; use SP\Infrastructure\ProvidersHelper; -use SP\Domain\Common\Providers\Http; use SP\Domain\Core\Exceptions\InitializationException; use SP\Domain\Core\Exceptions\SPException; use SP\Domain\Core\LanguageInterface; @@ -90,7 +89,7 @@ public function initialize(string $controller): void $this->language->setLanguage(); // Checks if it needs to switch the request over HTTPS - Http::checkHttps($this->configData, $this->request); + $this->redirectToHttpsIfRequired(); // Checks if sysPass is installed $this->checkInstalled(); diff --git a/src/Infrastructure/Adapter/In/Web/Init.php b/src/Infrastructure/Adapter/In/Web/Init.php index 2ff3cb398..12c81bde5 100644 --- a/src/Infrastructure/Adapter/In/Web/Init.php +++ b/src/Infrastructure/Adapter/In/Web/Init.php @@ -37,7 +37,6 @@ use SP\Domain\Crypt\Ports\SessionKeyService; use SP\Infrastructure\HttpModuleBase; use SP\Infrastructure\ProvidersHelper; -use SP\Domain\Common\Providers\Http; use SP\Domain\Core\Bootstrap\UriContextInterface; use SP\Domain\Core\Context\SessionContext; use SP\Domain\Core\Crypt\CsrfHandler; @@ -215,7 +214,7 @@ public function initialize(string $controller): void $this->language->setLanguage($isReload); // Check whether it is necessary to switch to HTTPS - Http::checkHttps($this->configData, $this->request); + $this->redirectToHttpsIfRequired(); $partialInit = in_array($controller, self::PARTIAL_INIT, true); diff --git a/src/Infrastructure/HttpModuleBase.php b/src/Infrastructure/HttpModuleBase.php index 9a610a35d..2c77adb89 100644 --- a/src/Infrastructure/HttpModuleBase.php +++ b/src/Infrastructure/HttpModuleBase.php @@ -28,6 +28,8 @@ use SP\Application\Application; use SP\Infrastructure\Bootstrap\Router; +use SP\Domain\Common\Providers\Http; +use SP\Domain\Core\Exceptions\InitializationException; use SP\Domain\Core\Exceptions\SPException; use SP\Domain\Core\Ports\AppLockHandler; use SP\Domain\Http\Ports\RequestService; @@ -35,6 +37,8 @@ /** * Base module for HTTP based modules */ +use function SP\logger; + abstract class HttpModuleBase extends ModuleBase { public function __construct( @@ -47,6 +51,35 @@ public function __construct( parent::__construct($application, $providersHelper); } + /** + * Send the request to HTTPS and stop, when the configuration says it must be. + * + * The sending and the stopping are one thing, which is why this is here rather than in the + * helper that works out the address. A redirect that does not halt is not a redirect: the + * response carries on being built and goes out over the connection the setting exists to + * refuse. + * + * Both entry points call this, and it mirrors what Init already does for a not-installed + * instance, a database it cannot reach and maintenance mode — redirect through the router, + * then throw. + * + * @throws InitializationException + */ + protected function redirectToHttpsIfRequired(): void + { + $httpsUrl = Http::httpsUrlFor($this->configData, $this->request); + + if ($httpsUrl === null) { + return; + } + + logger('Redirecting to HTTPS', 'INFO'); + + $this->router->response()->redirect($httpsUrl)->send(); + + throw new InitializationException('HTTPS required'); + } + /** * Check whether maintenance mode is enabled * This function checks whether maintenance mode is enabled. diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigSecurity/ConfigSecurityTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigSecurity/ConfigSecurityTest.php index eb12e560d..f49404471 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigSecurity/ConfigSecurityTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigSecurity/ConfigSecurityTest.php @@ -106,7 +106,9 @@ public function submittingWithoutTheFieldsTurnsBothFlagsOff(): void $this->expectOutputString('{"status":"OK","description":"Configuration updated","data":null}'); - $this->runController([], $configFileService, $eventDispatcher); + // Over HTTPS: the stored config already requires it, and Init now refuses a plaintext + // request on such an installation instead of merely mentioning where it should have gone. + $this->runController([], $configFileService, $eventDispatcher, ['HTTPS' => 'on']); self::assertSame($configData, $saved->value); self::assertFalse($configData->isHttpsEnabled()); @@ -229,13 +231,26 @@ public function getEvents(): ?string * @throws ContainerExceptionInterface * @throws NotFoundExceptionInterface */ + /** + * @param array $server Extra server parameters — an installation that already + * has "Force HTTPS" on refuses a plaintext request in + * Init, so a test starting from that state has to arrive + * over HTTPS to reach the controller at all. + */ private function runController( array $fields, ConfigFileService $configFileService, - EventDispatcherInterface $eventDispatcher + EventDispatcherInterface $eventDispatcher, + array $server = [] ): void { $container = $this->buildContainer( - IntegrationTestCase::buildRequest('post', 'index.php', ['r' => 'configSecurity/save'], $fields), + IntegrationTestCase::buildRequest( + 'post', + 'index.php', + ['r' => 'configSecurity/save'], + $fields, + server: $server + ), [ ConfigFileService::class => $configFileService, EventDispatcherInterface::class => $eventDispatcher, diff --git a/tests/Support/IntegrationTestCase.php b/tests/Support/IntegrationTestCase.php index bd39f11a9..900e74924 100644 --- a/tests/Support/IntegrationTestCase.php +++ b/tests/Support/IntegrationTestCase.php @@ -129,7 +129,8 @@ protected static function buildRequest( array $paramsGet = [], array $paramsPost = [], array $files = [], - ?string $csrfToken = self::CSRF_TOKEN + ?string $csrfToken = self::CSRF_TOKEN, + array $server = [] ): Request { $server = array_merge( $_SERVER, @@ -143,7 +144,11 @@ protected static function buildRequest( 'REMOTE_ADDR' => '127.0.0.1' //'QUERY_STRING' => $query ], - $csrfToken !== null ? ['HTTP_X_CSRF' => $csrfToken] : [] + $csrfToken !== null ? ['HTTP_X_CSRF' => $csrfToken] : [], + // Last, so a test can say something the defaults above do not — a request that arrived + // over HTTPS, say, which an installation with "Force HTTPS" on is the only kind that + // gets past Init. + $server ); return new Request( diff --git a/tests/Unit/Domain/Common/Providers/HttpTest.php b/tests/Unit/Domain/Common/Providers/HttpTest.php index ad7c1668b..0592765e5 100644 --- a/tests/Unit/Domain/Common/Providers/HttpTest.php +++ b/tests/Unit/Domain/Common/Providers/HttpTest.php @@ -40,78 +40,110 @@ class HttpTest extends TestCase { /** + * The address a plaintext request should have been made to. + * + * These used to assert only which mock methods had been called, which is why the defect they + * now pin survived: the method sent a bare `header('Location: …')` with no status code and no + * exit, so the caller carried on and the response went out over the plaintext connection + * anyway. A `Location` on a 200 is not a redirect. Asserting the answer, rather than the + * calls, is the difference. + * * @throws Exception */ - public function testCheckHttps() + public function testHttpsUrlForAPlaintextRequest() { $configData = $this->createMock(ConfigDataInterface::class); $request = $this->createMock(RequestService::class); - $configData->expects($this->once()) - ->method('isHttpsEnabled') - ->willReturn(true); + $configData->expects($this->once())->method('isHttpsEnabled')->willReturn(true); + $request->expects($this->once())->method('isHttps')->willReturn(false); + $request->expects($this->once())->method('getServerPort')->willReturn(8080); + $request->expects($this->once())->method('getHttpHost')->willReturn('http://localhost'); - $request->expects($this->once()) - ->method('isHttps') - ->willReturn(false); + $_SERVER['REQUEST_URI'] = '/index.php?r=account/index'; - $request->expects($this->once()) - ->method('getServerPort') - ->willReturn(8080); - - $request->expects($this->once()) - ->method('getHttpHost') - ->willReturn('localhost'); - - Http::checkHttps($configData, $request); + self::assertSame( + 'https://localhost:8080/index.php?r=account/index', + Http::httpsUrlFor($configData, $request) + ); } /** + * The standard port is not written out. + * * @throws Exception */ - public function testCheckHttpsWithNoHttpsEnabled() + public function testHttpsUrlForOmitsThePortWhenItIsTheDefault() { - $configData = $this->createMock(ConfigDataInterface::class); - $request = $this->createMock(RequestService::class); + $configData = $this->createStub(ConfigDataInterface::class); + $request = $this->createStub(RequestService::class); + + $configData->method('isHttpsEnabled')->willReturn(true); + $request->method('isHttps')->willReturn(false); + $request->method('getServerPort')->willReturn(443); + $request->method('getHttpHost')->willReturn('http://vault.example'); - $configData->expects($this->once()) - ->method('isHttpsEnabled') - ->willReturn(false); + $_SERVER['REQUEST_URI'] = '/'; - $request->expects($this->never()) - ->method('isHttps'); + self::assertSame('https://vault.example/', Http::httpsUrlFor($configData, $request)); + } + + /** + * Only the scheme is rewritten. + * + * It was a str_replace of 'http' for 'https' over the whole host, which rewrites every + * occurrence — so an installation at http://httpbin.example was redirected to + * https://httpsbin.example, a host that need not exist and need not be theirs. + * + * @throws Exception + */ + public function testHttpsUrlForRewritesOnlyTheScheme() + { + $configData = $this->createStub(ConfigDataInterface::class); + $request = $this->createStub(RequestService::class); - $request->expects($this->never()) - ->method('getServerPort'); + $configData->method('isHttpsEnabled')->willReturn(true); + $request->method('isHttps')->willReturn(false); + $request->method('getServerPort')->willReturn(443); + $request->method('getHttpHost')->willReturn('http://httpbin.example'); - $request->expects($this->never()) - ->method('getHttpHost'); + $_SERVER['REQUEST_URI'] = '/'; - Http::checkHttps($configData, $request); + self::assertSame('https://httpbin.example/', Http::httpsUrlFor($configData, $request)); } /** + * Nothing to do when the setting is off — and the request is not even examined. + * * @throws Exception */ - public function testCheckHttpsWithHttpsEnabledAndHttpsRequest() + public function testHttpsUrlForIsNullWhenNotEnabled() { $configData = $this->createMock(ConfigDataInterface::class); $request = $this->createMock(RequestService::class); - $configData->expects($this->once()) - ->method('isHttpsEnabled') - ->willReturn(true); + $configData->expects($this->once())->method('isHttpsEnabled')->willReturn(false); + $request->expects($this->never())->method('getServerPort'); + $request->expects($this->never())->method('getHttpHost'); - $request->expects($this->once()) - ->method('isHttps') - ->willReturn(true); + self::assertNull(Http::httpsUrlFor($configData, $request)); + } - $request->expects($this->never()) - ->method('getServerPort'); + /** + * Nor when the request already arrived over HTTPS. + * + * @throws Exception + */ + public function testHttpsUrlForIsNullWhenAlreadyHttps() + { + $configData = $this->createMock(ConfigDataInterface::class); + $request = $this->createMock(RequestService::class); - $request->expects($this->never()) - ->method('getHttpHost'); + $configData->expects($this->once())->method('isHttpsEnabled')->willReturn(true); + $request->expects($this->once())->method('isHttps')->willReturn(true); + $request->expects($this->never())->method('getServerPort'); + $request->expects($this->never())->method('getHttpHost'); - Http::checkHttps($configData, $request); + self::assertNull(Http::httpsUrlFor($configData, $request)); } } diff --git a/tests/Unit/Infrastructure/HttpModuleBaseTest.php b/tests/Unit/Infrastructure/HttpModuleBaseTest.php new file mode 100644 index 000000000..8001da3e4 --- /dev/null +++ b/tests/Unit/Infrastructure/HttpModuleBaseTest.php @@ -0,0 +1,179 @@ +. + */ + +declare(strict_types=1); + +namespace SP\Tests\Unit\Infrastructure; + +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\MockObject\Exception; +use SP\Domain\Core\Exceptions\InitializationException; +use SP\Domain\Core\Ports\AppLockHandler; +use SP\Domain\Http\Ports\RequestService; +use SP\Infrastructure\Bootstrap\Router; +use Symfony\Component\HttpFoundation\Request; +use SP\Infrastructure\Http\Ports\ResponseService; +use SP\Infrastructure\HttpModuleBase; +use Psr\Log\LoggerInterface; +use SP\Domain\Core\LanguageInterface; +use SP\Infrastructure\Log\Providers\LogHandler; +use SP\Infrastructure\ProvidersHelper; +use SP\Tests\Support\UnitaryTestCase; + +/** + * "Force HTTPS" has to stop the request, not merely mention where it should have gone. + * + * It used to send a bare `header('Location: …')` with no status code and no exit, and then carry + * on: the whole response — an account page on the web, a token on the API — was still built and + * sent over the plaintext connection the setting exists to refuse. Browsers ignore a `Location` on + * a 200, so nothing about it was visible either. + * + * The redirect and the halt are one thing, which is why they live together in the shared base both + * entry points extend, and why these assert the throw rather than the header. + */ +#[Group('unitary')] +class HttpModuleBaseTest extends UnitaryTestCase +{ + /** + * @throws Exception + */ + #[Test] + public function aPlaintextRequestIsRedirectedAndStopped(): void + { + $this->config->getConfigData()->setHttpsEnabled(true); + + $request = $this->createStub(RequestService::class); + $request->method('isHttps')->willReturn(false); + $request->method('getServerPort')->willReturn(443); + $request->method('getHttpHost')->willReturn('http://vault.example'); + + $_SERVER['REQUEST_URI'] = '/index.php?r=account/index'; + + $response = $this->createMock(ResponseService::class); + $response->expects(self::once()) + ->method('redirect') + ->with('https://vault.example/index.php?r=account/index') + ->willReturnSelf(); + $response->expects(self::once())->method('send')->willReturnSelf(); + + $this->expectException(InitializationException::class); + $this->expectExceptionMessage('HTTPS required'); + + $this->moduleFor($request, $response)->redirectToHttps(); + } + + /** + * A request that already arrived over HTTPS is left alone — nothing sent, nothing thrown. + * + * Without this the test above is satisfied by a base that refuses every request. + * + * @throws Exception + * @throws InitializationException + */ + #[Test] + public function anHttpsRequestIsLeftAlone(): void + { + $this->config->getConfigData()->setHttpsEnabled(true); + + $request = $this->createStub(RequestService::class); + $request->method('isHttps')->willReturn(true); + + // Asserted on the response rather than the router, which is final and cannot be doubled: + // a redirect that is never built is a redirect that never happened. + $response = $this->createMock(ResponseService::class); + $response->expects(self::never())->method('redirect'); + $response->expects(self::never())->method('send'); + + $this->moduleFor($request, $response)->redirectToHttps(); + } + + /** + * And so is one on an installation that has not turned the setting on. + * + * @throws Exception + * @throws InitializationException + */ + #[Test] + public function aPlaintextRequestIsLeftAloneWhenTheSettingIsOff(): void + { + $this->config->getConfigData()->setHttpsEnabled(false); + + $request = $this->createStub(RequestService::class); + $request->method('isHttps')->willReturn(false); + + // Asserted on the response rather than the router, which is final and cannot be doubled: + // a redirect that is never built is a redirect that never happened. + $response = $this->createMock(ResponseService::class); + $response->expects(self::never())->method('redirect'); + $response->expects(self::never())->method('send'); + + $this->moduleFor($request, $response)->redirectToHttps(); + } + + /** + * `HttpModuleBase` is abstract and its guard is protected — this is the smallest concrete thing + * that can reach it, standing in for Web\Init and Api\Init, which differ in nothing that + * matters here. + * + * @throws Exception + */ + private function moduleFor(RequestService $request, ResponseService $response): object + { + return new class ( + $this->application, + // ProvidersHelper and LogHandler are both final; neither is used by the guard under + // test, but the base requires one, so it gets a real one over stubbed collaborators. + new ProvidersHelper( + new LogHandler( + $this->application, + $this->createStub(LoggerInterface::class), + $this->createStub(LanguageInterface::class), + $request + ) + ), + $request, + new Router(new Request(), $response), + $this->createStub(AppLockHandler::class) + ) extends HttpModuleBase { + public function initialize(string $controller): void + { + } + + public function getName(): string + { + return 'test'; + } + + /** + * @throws InitializationException + */ + public function redirectToHttps(): void + { + $this->redirectToHttpsIfRequired(); + } + }; + } +}