fix(federation): remove plaintext auth token from error log in getSharedSecret

apps/federation/lib/BackgroundJob/GetSharedSecret.php

@@ -159,11 +159,21 @@ protected function run($argument) {
 			if ($status === Http::STATUS_FORBIDDEN) {
 				$this->logger->info($target . ' refused to exchange a shared secret with you.', ['app' => 'federation']);
 			} else {
-				$this->logger->logException($e, ['app' => 'federation']);
+				// NB: do not log the exception verbatim - the token is sent as a
+				// GET query parameter, so the Guzzle message embeds the full
+				// request URI including "?token=...", leaking the plaintext token.
+				$this->logger->error(
+					'Could not exchange a shared secret with ' . $target . ' (HTTP ' . $status . ')',
+					['app' => 'federation']
+				);
 			}
 		} catch (\Exception $e) {
 			$status = Http::STATUS_INTERNAL_SERVER_ERROR;
-			$this->logger->logException($e, ['app' => 'federation']);
+			// NB: see above - the token must not reach the log via the request URI.
+			$this->logger->error(
+				'Could not exchange a shared secret with ' . $target,
+				['app' => 'federation']
+			);
 		}
 
 		// if we received a unexpected response we try again later

apps/federation/lib/Controller/OCSAuthAPIController.php

@@ -159,9 +159,8 @@ public function getSharedSecret($url, $token) {
 		}
 
 		if ($this->isValidToken($url, $token) === false) {
-			$expectedToken = $this->dbHandler->getToken($url);
 			$this->logger->error(
-				'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret',
+				'remote server (' . $url . ') didn\'t send a valid token while getting shared secret',
 				['app' => 'federation']
 			);
 			return ['statuscode' => Http::STATUS_FORBIDDEN];

apps/federation/tests/API/OCSAuthAPITest.php

@@ -235,6 +235,40 @@ public function dataTestGetSharedSecret() {
 		];
 	}
 
+	/**
+	 * Verify that when an invalid token is supplied, the error log message does NOT
+	 * contain either the attacker-supplied token or the expected stored token.
+	 * Leaking either value allows an attacker with log-read access to replay the
+	 * valid token and obtain the shared secret (CVE-class: information disclosure).
+	 */
+	public function testGetSharedSecretDoesNotLogTokenValues() {
+		$url = 'https://trusted.example.com';
+		$attackerToken = 'attacker_supplied_token_XYZ';
+		$storedToken = 'super_secret_expected_token_ABC';
+
+		$this->trustedServers
+			->method('isTrustedServer')->with($url)->willReturn(true);
+
+		// dbHandler->getToken is used by isValidToken; return the stored token
+		$this->dbHandler
+			->method('getToken')->with($url)->willReturn($storedToken);
+
+		// Capture every call to logger->error and assert no token values appear
+		$this->logger
+			->expects($this->once())
+			->method('error')
+			->with(
+				$this->logicalAnd(
+					$this->logicalNot($this->stringContains($attackerToken)),
+					$this->logicalNot($this->stringContains($storedToken))
+				),
+				$this->anything()
+			);
+
+		$result = $this->ocsAuthApi->getSharedSecret($url, $attackerToken);
+		$this->assertSame(Http::STATUS_FORBIDDEN, $result['statuscode']);
+	}
+
 	/**
 	 * @dataProvider dataTestIsTokenValid
 	 */

apps/federation/tests/BackgroundJob/GetSharedSecretTest.php

@@ -23,6 +23,9 @@
 
 namespace OCA\Federation\Tests\BackgroundJob;
 
+use GuzzleHttp\Exception\ClientException;
+use GuzzleHttp\Psr7\Request;
+use GuzzleHttp\Psr7\Response;
 use OCA\Federation\BackgroundJob\GetSharedSecret;
 use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
@@ -203,4 +206,51 @@ public function dataTestRun() {
 			[Http::STATUS_CONFLICT],
 		];
 	}
+
+	/**
+	 * On a non-403 HTTP failure the shared secret is sent as a GET query
+	 * parameter, so the Guzzle exception message embeds the full request URI
+	 * including "?token=...". Logging that exception verbatim would leak the
+	 * plaintext token, so the token value must never reach the logger.
+	 */
+	public function testRunDoesNotLogTokenOnClientException() {
+		$target = 'https://remote.example.com';
+		$source = 'https://local.example.com';
+		$token = 'super-secret-federation-token';
+
+		$argument = ['url' => $target, 'token' => $token];
+
+		$this->urlGenerator->expects($this->once())->method('getAbsoluteURL')->with('/')
+			->willReturn($source);
+
+		// Build a real Guzzle ClientException whose request URI carries the
+		// token in the query string, exactly as the live client would produce.
+		$request = new Request(
+			'GET',
+			$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret?url=' . $source . '&token=' . $token . '&format=json'
+		);
+		$exception = new ClientException(
+			'Client error: `' . $request->getUri() . '` resulted in a `409 Conflict` response',
+			$request,
+			new Response(Http::STATUS_CONFLICT)
+		);
+
+		$this->httpClient->expects($this->once())->method('get')
+			->willThrowException($exception);
+
+		// The token must not appear in any exception logged verbatim...
+		$this->logger->expects($this->never())->method('logException');
+		// ...nor in any error/info message string.
+		$assertNoToken = function ($message) use ($token) {
+			$this->assertStringNotContainsString($token, $message);
+			return true;
+		};
+		$this->logger->method('error')->with($this->callback($assertNoToken));
+		$this->logger->method('info')->with($this->callback($assertNoToken));
+
+		self::invokePrivate($this->getSharedSecret, 'run', [$argument]);
+
+		// A 409 is an unexpected response, so the job is retained for retry.
+		$this->assertTrue(self::invokePrivate($this->getSharedSecret, 'retainJob'));
+	}
 }

changelog/unreleased/41578

@@ -0,0 +1,15 @@
+Security: Remove plaintext federation auth token from error log
+
+When getSharedSecret received an invalid token it logged both the
+submitted value and the expected valid token in plaintext. Since the
+endpoint is public, any unauthenticated caller could trigger this log
+entry at will for any trusted server URL, exposing the valid token to
+anyone with log-read access.
+
+A second leak in the same code path has also been closed: the
+GetSharedSecret background job sends the token as a GET query
+parameter, so on an unexpected HTTP response the Guzzle exception -
+whose message embeds the full request URI including "?token=..." -
+was logged verbatim. Both log sites no longer emit the token value.
+
+https://github.com/owncloud/core/pull/41578