From 25f8ee7a6147daef0ae0c0ce4b1e7d32a67b4a0f Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 08:56:40 +0200 Subject: [PATCH 1/2] fix: Force HTTPS actually stops the plaintext response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Http::checkHttps()` sent a bare `header('Location: …')` — no status code, no exit, no check that headers had already gone — and then returned. Both entry points called it and carried straight on: `Web\Init` through the install and database checks to controller dispatch, `Api\Init` likewise. The response was built and sent over the plaintext connection the setting exists to refuse, and since a `Location` on a 200 is not a redirect, no browser acted on the header either. Turning "Force HTTPS" on changed nothing except adding an inert header. The sibling refusals in the same method have always done it properly — not installed, database unreachable, maintenance mode each redirect through the router and *then throw*. That throw is the part that was missing, and it is why this now lives beside them: `Http::httpsUrlFor()` answers with the address, and `HttpModuleBase::redirectToHttpsIfRequired()` sends it and stops the request, in the base both entry points already extend. Also: the host was rewritten with `str_replace('http', 'https', …)`, which replaces every occurrence — an installation at http://httpbin.example was redirected to https://httpsbin.example, a host that need not exist and need not be theirs. Only the scheme is rewritten now. The three existing tests asserted which mock methods had been called and nothing else, which is exactly why this survived: a method that calls isHttpsEnabled(), isHttps(), getServerPort() and getHttpHost() satisfies all three whether or not it does anything useful with them. They assert the address now, and three more assert what the base does with it — that a plaintext request is redirected *and* stopped, and that an HTTPS request and an installation with the setting off are both left alone, so a base that refused everything would not pass. Writing that test found a fatal in the first version of this change: `logger()` is a global function in namespace `SP`, and `HttpModuleBase` is in `SP\Infrastructure`, so the bare call resolved to nothing and every redirect would have died on it. It is imported. --- src/Domain/Common/Providers/Http.php | 37 ++-- src/Infrastructure/Adapter/In/Api/Init.php | 3 +- src/Infrastructure/Adapter/In/Web/Init.php | 3 +- src/Infrastructure/HttpModuleBase.php | 33 ++++ .../Unit/Domain/Common/Providers/HttpTest.php | 114 +++++++---- .../Infrastructure/HttpModuleBaseTest.php | 179 ++++++++++++++++++ 6 files changed, 309 insertions(+), 60 deletions(-) create mode 100644 tests/Unit/Infrastructure/HttpModuleBaseTest.php 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/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(); + } + }; + } +} From 02650167fd49d18eb3433e761284f9dc92f4a062 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 09:06:08 +0200 Subject: [PATCH 2/2] fix: an installation that requires HTTPS refuses the plaintext request in its tests too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration suite caught the consequence of making the redirect actually stop the request: ConfigSecurityTest starts from an installation that already has "Force HTTPS" on and then dispatches over plain HTTP, which is exactly the case Init now refuses — so the controller never ran and nothing was saved. The test's premise is what changed, not its assertion. An installation requiring HTTPS is reached over HTTPS, so the request says so now. `buildRequest()` grows an optional server array for it, merged last so a test can state something the defaults do not. Only this one test needed it; nothing else in the suite turns the setting on. --- .../ConfigSecurity/ConfigSecurityTest.php | 21 ++++++++++++++++--- tests/Support/IntegrationTestCase.php | 9 ++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) 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(