fix(PasswordConfirmationMiddleware): Make more robust

lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php

@@ -26,8 +26,24 @@
 use Psr\Log\LoggerInterface;
 use ReflectionMethod;
 
+/**
+ * Middleware that enforces password reconfirmation for sensitive controller actions.
+ *
+ * When a controller method is annotated/attributed with PasswordConfirmationRequired this
+ * middleware ensures a recent password reconfirmation or performs a strict HTTP Basic
+ * re-check. Skip conditions include tokens with IToken::SCOPE_SKIP_PASSWORD_VALIDATION and
+ * backends that implement IPasswordConfirmationBackend and opt out of confirmation.
+ *
+ * Implementation notes:
+ *  - Malformed headers or failed checks raise NotConfirmedException.
+ *  - Credentials are never logged; token retrieval failures are logged and result in no enforcement.
+ */
 class PasswordConfirmationMiddleware extends Middleware {
-	private array $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true];
+	// TODO: confirm excludedUserBackEnds can go away and remove it
+	private array $excludedUserBackEnds = [
+		'user_saml' => true,
+		'user_globalsiteselector' => true
+	];
 
 	public function __construct(
 		private ControllerMethodReflector $reflector,
@@ -42,7 +58,19 @@ public function __construct(
 	}
 
 	/**
-	 * @throws NotConfirmedException
+	 * Enforce password confirmation before invoking the controller method.
+	 *
+	 * If confirmation is required (see class doc) this performs either:
+	 *  - Strict mode: validate "Authorization: Basic <base64(user:pass)>", verify via Manager::checkPassword(),
+	 *    and update session 'last-password-confirm' on success.
+	 *  - Non-strict mode: verify session 'last-password-confirm' falls within the allowed window.
+	 *
+	 * Does not log credential material. On malformed input or failed verification a NotConfirmedException is thrown.
+	 *
+	 * @param Controller $controller The controller instance to be executed.
+	 * @param string $methodName The controller method name to be invoked.
+	 * @throws NotConfirmedException When confirmation is required but not satisfied.
+	 * @internal
 	 */
 	public function beforeController(Controller $controller, string $methodName) {
 		$reflectionMethod = new ReflectionMethod($controller, $methodName);
@@ -69,37 +97,100 @@ public function beforeController(Controller $controller, string $methodName) {
 			$token = $this->tokenProvider->getToken($sessionId);
 		} catch (SessionNotAvailableException|InvalidTokenException|WipeTokenException|ExpiredTokenException) {
 			// States we do not deal with here.
+			// NOTE: returning here may skip enforcement in certain edge cases where token retrieval fails.
+			$this->logger->info('PasswordConfirmationMiddleware: could not retrieve sessionId or token; continuing anyway');
 			return;
 		}
 
 		$scope = $token->getScopeAsArray();
-		if (isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) && $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === true) {
+		if (isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION])
+			&& $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === true
+		) {
 			// Users logging in from SSO backends cannot confirm their password by design
 			return;
 		}
 
 		if ($this->isPasswordConfirmationStrict($reflectionMethod)) {
 			$authHeader = $this->request->getHeader('Authorization');
-			if (!str_starts_with(strtolower($authHeader), 'basic ')) {
+
+			// Validate header is present and a string
+			/** @psalm-suppress TypeDoesNotContainType */
+			if (!is_string($authHeader) || $authHeader === '') {
 				throw new NotConfirmedException('Required authorization header missing');
 			}
-			[, $password] = explode(':', base64_decode(substr($authHeader, 6)), 2);
-			$loginName = $this->session->get('loginname');
+
+			// Extract base64 token from "Basic <b64>" in a robust, case-insensitive way
+			if (!preg_match('/^\s*Basic\s+([A-Za-z0-9+\/=]+)\s*$/i', $authHeader, $matches)) {
+				// Accept only "Basic <token>" and nothing else (allow trailing whitespace)
+				throw new NotConfirmedException('Required authorization header missing or malformed');
+			}
+			$b64 = trim($matches[1]);
+
+			// Enforce reasonable max length to reduce risk of abuse/DoS
+			if (strlen($b64) > 4096) {
+				throw new NotConfirmedException('Authorization token too long');
+			}
+			// Strictly decode base64; false means invalid base64
+			$decoded = base64_decode($b64, true);
+			if ($decoded === false || $decoded === '') {
+				throw new NotConfirmedException('Invalid authorization header encoding');
+			}
+
+			// Detect non-UTF-8 payloads for monitoring/telemetry while preserving
+			// current behavior (we continue to use the raw bytes as before).
+			// Do NOT log the decoded payload or any credential material.
+			if (function_exists('mb_check_encoding')) {
+				$isUtf8 = mb_check_encoding($decoded, 'UTF-8');
+			} else {
+				// Fallback if mbstring isn't available: preg_match with /u will
+				// return 1 for valid UTF-8, 0 for invalid. Suppress warnings.
+				$isUtf8 = @preg_match('//u', $decoded) === 1;
+			}
+			if (!$isUtf8) {
+				$this->logger->info('Non-UTF-8 Authorization Basic payload detected; continuing with raw bytes for compatibility');
+			}
+
+			// Expect exactly "username:password" (password may contain colons; we split into max 2 parts)
+			$parts = explode(':', $decoded, 2);
+			if (!isset($parts[1])) { // password is missing
+				throw new NotConfirmedException('Authorization payload malformed');
+			}
+			$password = $parts[1];
+
+			$loginName = $this->session->get('loginname'); // Current behavior, but would $user->getUID() be more robust?
+			if (!is_string($loginName) || $loginName === '') {
+				$this->logger->warning('PasswordConfirmationMiddleware: session loginname missing for strict confirmation');
+				throw new NotConfirmedException('Unable to confirm password');
+			}
+
 			$loginResult = $this->userManager->checkPassword($loginName, $password);
 			if ($loginResult === false) {
-				throw new NotConfirmedException();
+				throw new NotConfirmedException('Unable to confirm password');
 			}
 
 			$this->session->set('last-password-confirm', $this->timeFactory->getTime());
 		} else {
 			$lastConfirm = (int)$this->session->get('last-password-confirm');
 			// TODO: confirm excludedUserBackEnds can go away and remove it
-			if (!isset($this->excludedUserBackEnds[$backendClassName]) && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay
+			if (!isset($this->excludedUserBackEnds[$backendClassName])
+				&& $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15)) // allow 15 seconds delay
+			) {
 				throw new NotConfirmedException();
 			}
 		}
 	}
 
+	/**
+	 * Determine whether the given controller method requires password reconfirmation.
+	 *
+	 * Checks for the PasswordConfirmationRequired attribute (preferred). If no attribute
+	 * is present the legacy @PasswordConfirmationRequired annotation is queried; when
+	 * the legacy annotation is used a debug log is emitted recommending the attribute.
+	 *
+	 * @param ReflectionMethod $reflectionMethod The controller method to inspect.
+	 * @return bool True when password confirmation is required, false otherwise.
+	 * @internal
+	 */
 	private function needsPasswordConfirmation(ReflectionMethod $reflectionMethod): bool {
 		$attributes = $reflectionMethod->getAttributes(PasswordConfirmationRequired::class);
 		if (!empty($attributes)) {
@@ -114,6 +205,17 @@ private function needsPasswordConfirmation(ReflectionMethod $reflectionMethod):
 		return false;
 	}
 
+	/**
+	 * Determine whether the PasswordConfirmationRequired attribute requests strict confirmation.
+	 *
+	 * Inspects the PasswordConfirmationRequired attribute on the provided controller method
+	 * and returns its `strict` flag. Only attributes are considered (legacy annotations are
+	 * not evaluated by this helper).
+	 *
+	 * @param ReflectionMethod $reflectionMethod The controller method to inspect.
+	 * @return bool True if strict (HTTP Basic re-check) is required, false otherwise.
+	 * @internal
+	 */
 	private function isPasswordConfirmationStrict(ReflectionMethod $reflectionMethod): bool {
 		$attributes = $reflectionMethod->getAttributes(PasswordConfirmationRequired::class);
 		return !empty($attributes) && ($attributes[0]->newInstance()->getStrict());

tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php

@@ -35,4 +35,9 @@ public function testAttribute() {
 	#[PasswordConfirmationRequired]
 	public function testSSO() {
 	}
+
+	#[PasswordConfirmationRequired(strict: true)]
+	public function testStrictAction(): void {
+		// intentionally empty; used by PasswordConfirmationMiddleware tests
+	}
 }

tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php

@@ -206,4 +206,131 @@ public function testSSO(): void {
 
 		$this->assertSame(false, $thrown);
 	}
+
+	//
+	// Additional strict-mode / edge-case tests integrated below.
+	// These reuse the existing test fixtures (reflector, session, request, tokenProvider, etc.)
+	//
+
+	public function testStrictMissingHeaderThrows(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// Request returns no header
+		$this->request->method('getHeader')->with('Authorization')->willReturn(null);
+
+		// token retrieval should succeed
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testStrictMalformedHeaderThrows(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// malformed forms like "Basic:" should be rejected by the parser
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic: abc');
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testStrictInvalidBase64Throws(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// token contains invalid base64 characters
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic !!not-base64!!');
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testStrictEmptyDecodedThrows(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// header with an "empty" base64 payload -> parser will treat as malformed or invalid
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic ' . base64_encode(''));
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testStrictTooLongTokenThrows(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		$long = str_repeat('A', 5000); // valid base64 chars but exceeds 4096 limit
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic ' . $long);
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testStrictUrlSafeBase64Rejected(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// create a standard base64 string and then convert to URL-safe variant
+		$normal = base64_encode('alice:secret');
+		$urlsafe = strtr($normal, '+/', '-_'); // produce '-' or '_' which the regex rejects
+
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic ' . $urlsafe);
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		$this->expectException(NotConfirmedException::class);
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
+
+	public function testNonUtf8DetectionLogsAndContinues(): void {
+		$this->reflector->reflect($this->controller, 'testStrictAction');
+
+		// Construct a decoded payload that contains invalid UTF-8 bytes,
+		// but still contains a colon separating username and password.
+		$decoded = "\x80" . 'alice:secret'; // leading 0x80 byte makes this invalid UTF-8
+		$b64 = base64_encode($decoded);
+		$this->request->method('getHeader')->with('Authorization')->willReturn('Basic ' . $b64);
+
+		$token = $this->createMock(IToken::class);
+		$token->method('getScopeAsArray')->willReturn([]);
+		$this->session->method('getId')->willReturn('sid');
+		$this->tokenProvider->method('getToken')->willReturn($token);
+
+		// Provide a loginname in session and accept the password check.
+		$this->session->method('get')->with('loginname')->willReturn('alice');
+		$this->userManager->method('checkPassword')->with('alice', 'secret')->willReturn(true);
+
+		// Expect logger->info to be called with the non-UTF-8 message.
+		$this->logger->expects($this->once())->method('info')->with($this->stringContains('Non-UTF-8 Authorization Basic payload detected'));
+
+		// Expect session->set called to record last-password-confirm (timeFactory returns known time)
+		$this->timeFactory->method('getTime')->willReturn(12345);
+		$this->session->expects($this->once())->method('set')->with('last-password-confirm', 12345);
+
+		$this->middleware->beforeController($this->controller, 'testStrictAction');
+	}
 }