Merge pull request #50099 from nextcloud/jtr/fix-appframework-server-proto

Author: skjnldsv

lib/composer/composer/InstalledVersions.php

@@ -26,6 +26,12 @@
  */
 class InstalledVersions
 {
+    /**
+     * @var string|null if set (by reflection by Composer), this should be set to the path where this class is being copied to
+     * @internal
+     */
+    private static $selfDir = null;
+
     /**
      * @var mixed[]|null
      * @psalm-var array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: string[], dev: bool}, versions: array<string, array{pretty_version?: string, version?: string, reference?: string|null, type?: string, install_path?: string, aliases?: string[], dev_requirement: bool, replaced?: string[], provided?: string[]}>}|array{}|null
@@ -322,6 +328,18 @@ public static function reload($data)
         self::$installedIsLocalDir = false;
     }
 
+    /**
+     * @return string
+     */
+    private static function getSelfDir()
+    {
+        if (self::$selfDir === null) {
+            self::$selfDir = strtr(__DIR__, '\\', '/');
+        }
+
+        return self::$selfDir;
+    }
+
     /**
      * @return array[]
      * @psalm-return list<array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: string[], dev: bool}, versions: array<string, array{pretty_version?: string, version?: string, reference?: string|null, type?: string, install_path?: string, aliases?: string[], dev_requirement: bool, replaced?: string[], provided?: string[]}>}>
@@ -336,7 +354,7 @@ private static function getInstalled()
         $copiedLocalDir = false;
 
         if (self::$canGetVendors) {
-            $selfDir = strtr(__DIR__, '\\', '/');
+            $selfDir = self::getSelfDir();
             foreach (ClassLoader::getRegisteredLoaders() as $vendorDir => $loader) {
                 $vendorDir = strtr($vendorDir, '\\', '/');
                 if (isset(self::$installedByVendor[$vendorDir])) {

lib/composer/composer/installed.php

@@ -3,7 +3,7 @@
         'name' => '__root__',
         'pretty_version' => 'dev-master',
         'version' => 'dev-master',
-        'reference' => 'b7422ba97b7b42a9955a52031a32457ca521d740',
+        'reference' => '3fce359f4c606737b21b1b4213efd5bc5536e867',
         'type' => 'library',
         'install_path' => __DIR__ . '/../../../',
         'aliases' => array(),
@@ -13,7 +13,7 @@
         '__root__' => array(
             'pretty_version' => 'dev-master',
             'version' => 'dev-master',
-            'reference' => 'b7422ba97b7b42a9955a52031a32457ca521d740',
+            'reference' => '3fce359f4c606737b21b1b4213efd5bc5536e867',
             'type' => 'library',
             'install_path' => __DIR__ . '/../../../',
             'aliases' => array(),

lib/private/AppFramework/Http/Request.php

@@ -14,6 +14,7 @@
 use OCP\IConfig;
 use OCP\IRequest;
 use OCP\IRequestId;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpFoundation\IpUtils;
 
 /**
@@ -627,36 +628,46 @@ private function isOverwriteCondition(): bool {
 
 	/**
 	 * Returns the server protocol. It respects one or more reverse proxies servers
-	 * and load balancers
+	 * and load balancers. Precedence:
+	 *   1. `overwriteprotocol` config value
+	 *   2. `X-Forwarded-Proto` header value
+	 *   3. $_SERVER['HTTPS'] value
+	 * If an invalid protocol is provided, defaults to http, continues, but logs as an error.
+	 *
 	 * @return string Server protocol (http or https)
 	 */
 	public function getServerProtocol(): string {
-		if ($this->config->getSystemValueString('overwriteprotocol') !== ''
-			&& $this->isOverwriteCondition()) {
-			return $this->config->getSystemValueString('overwriteprotocol');
-		}
+		$proto = 'http';
 
-		if ($this->fromTrustedProxy() && isset($this->server['HTTP_X_FORWARDED_PROTO'])) {
+		if ($this->config->getSystemValueString('overwriteprotocol') !== ''
+			&& $this->isOverwriteCondition()
+		) {
+			$proto = strtolower($this->config->getSystemValueString('overwriteprotocol'));
+		} elseif ($this->fromTrustedProxy()
+			&& isset($this->server['HTTP_X_FORWARDED_PROTO'])
+		) {
 			if (str_contains($this->server['HTTP_X_FORWARDED_PROTO'], ',')) {
 				$parts = explode(',', $this->server['HTTP_X_FORWARDED_PROTO']);
 				$proto = strtolower(trim($parts[0]));
 			} else {
 				$proto = strtolower($this->server['HTTP_X_FORWARDED_PROTO']);
 			}
-
-			// Verify that the protocol is always HTTP or HTTPS
-			// default to http if an invalid value is provided
-			return $proto === 'https' ? 'https' : 'http';
+		} elseif (!empty($this->server['HTTPS'])
+			&& $this->server['HTTPS'] !== 'off'
+		) {
+			$proto = 'https';
 		}
 
-		if (isset($this->server['HTTPS'])
-			&& $this->server['HTTPS'] !== null
-			&& $this->server['HTTPS'] !== 'off'
-			&& $this->server['HTTPS'] !== '') {
-			return 'https';
+		if ($proto !== 'https' && $proto !== 'http') {
+			// log unrecognized value so admin has a chance to fix it
+			\OCP\Server::get(LoggerInterface::class)->critical(
+				'Server protocol is malformed [falling back to http] (check overwriteprotocol and/or X-Forwarded-Proto to remedy): ' . $proto,
+				['app' => 'core']
+			);
 		}
 
-		return 'http';
+		// default to http if provided an invalid value
+		return $proto === 'https' ? 'https' : 'http';
 	}
 
 	/**
@@ -743,11 +754,11 @@ public function getRawPathInfo(): string {
 	}
 
 	/**
-	 * Get PathInfo from request
+	 * Get PathInfo from request (rawurldecoded)
 	 * @throws \Exception
 	 * @return string|false Path info or false when not found
 	 */
-	public function getPathInfo() {
+	public function getPathInfo(): string|false {
 		$pathInfo = $this->getRawPathInfo();
 		return \Sabre\HTTP\decodePath($pathInfo);
 	}

tests/lib/AppFramework/Http/RequestTest.php

@@ -777,12 +777,12 @@ public function testGetHttpProtocol($input, $expected): void {
 		$this->assertSame($expected, $request->getHttpProtocol());
 	}
 
-	public function testGetServerProtocolWithOverride(): void {
+	public function testGetServerProtocolWithOverrideValid(): void {
 		$this->config
 			->expects($this->exactly(3))
 			->method('getSystemValueString')
 			->willReturnMap([
-				['overwriteprotocol', '', 'customProtocol'],
+				['overwriteprotocol', '', 'HTTPS'], // should be automatically lowercased
 				['overwritecondaddr', '', ''],
 			]);
 
@@ -794,7 +794,27 @@ public function testGetServerProtocolWithOverride(): void {
 			$this->stream
 		);
 
-		$this->assertSame('customProtocol', $request->getServerProtocol());
+		$this->assertSame('https', $request->getServerProtocol());
+	}
+
+	public function testGetServerProtocolWithOverrideInValid(): void {
+		$this->config
+			->expects($this->exactly(3))
+			->method('getSystemValueString')
+			->willReturnMap([
+				['overwriteprotocol', '', 'bogusProtocol'], // should trigger fallback to http
+				['overwritecondaddr', '', ''],
+			]);
+
+		$request = new Request(
+			[],
+			$this->requestId,
+			$this->config,
+			$this->csrfTokenManager,
+			$this->stream
+		);
+
+		$this->assertSame('http', $request->getServerProtocol());
 	}
 
 	public function testGetServerProtocolWithProtoValid(): void {