fix: Force HTTPS actually stops the plaintext response

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'] ?? '');
     }
 }

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();

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);
 

src/Infrastructure/HttpModuleBase.php

@@ -28,13 +28,17 @@
 
 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;
 
 /**
  * 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.

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<string, string> $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,

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(

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));
     }
 }