fix(bank-sync): treat provider data as untrusted and keep secrets out of logs

budget/CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Fixed
+- **Bank sync shows what the bank sends as text.** The balance and currency in **Account Mappings** went into the page as they came from the bank provider, so a provider sending markup could run script in Budget. They are now shown as plain text, and the server only keeps a currency that is a three-letter code and a balance that is a number.
+- **The bank authorization page opens safely.** The GoCardless authorization link now opens with no link back to the Budget window and no referrer, and the server refuses a link that isn't https before your browser sees it. The "click here to authorize" link is always shown, since the browser can no longer say whether the window opened.
+- **A factory reset ends your bank connections at the provider.** Factory reset, clearing the sample data and purging a deleted user removed the bank connections from Budget but left the consent at GoCardless running, as only **Disconnect** revoked it. They now revoke each connection once the data has gone. A provider that can't be reached is logged and never stops the reset.
+- **Bank sync errors no longer put credentials in the log.** A failed connect, sync or bank list logged the error with its stack trace, and on PHP 8.1 that trace can hold the decrypted bank credentials. These errors are now logged with their type and message only. The message you see is the same as before.
+
 ## [3.3.0] - 2026-11-01
 
 ### Added

budget/lib/BackgroundJob/Queued/BankSyncConnectionJob.php

@@ -40,9 +40,12 @@ protected function run($argument): void {
 			return;
 		}
 
+		// Throwable, and the message only: a TypeError reaching the job runner
+		// would be logged with its trace, whose provider frames hold the
+		// decrypted credentials on PHP 8.1
 		try {
 			$this->syncService->sync($userId, $connectionId);
-		} catch (\Exception $e) {
+		} catch (\Throwable $e) {
 			$this->logger->warning(
 				"Bank sync failed for connection {$connectionId}: " . $e->getMessage(),
 				['app' => 'budget', 'userId' => $userId, 'connectionId' => $connectionId]

budget/lib/Controller/BankSyncController.php

@@ -18,6 +18,13 @@
 use OCP\IRequest;
 use Psr\Log\LoggerInterface;
 
+/**
+ * Errors here are logged through handleErrorWithoutTrace(): the providers
+ * take the decrypted bank credentials as arguments, and an exception's trace
+ * would carry them into nextcloud.log on PHP 8.1. Throwable is caught, not
+ * just Exception, so a TypeError inside a provider doesn't escape to
+ * Nextcloud's own handler, which logs the full trace.
+ */
 class BankSyncController extends Controller {
 	use ApiErrorHandlerTrait;
 
@@ -102,8 +109,8 @@ public function institutions(string $provider): DataResponse {
 			$institutions = $p->getInstitutions($accessToken, $country);
 
 			return new DataResponse($institutions);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to fetch institutions'), Http::STATUS_BAD_REQUEST);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to fetch institutions'), Http::STATUS_BAD_REQUEST);
 		}
 	}
 
@@ -167,8 +174,8 @@ public function connect(): DataResponse {
 
 			$result = $this->syncService->connect($this->userId, $provider, $providerParams, $name);
 			return new DataResponse($result);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to connect bank'), Http::STATUS_BAD_REQUEST);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to connect bank'), Http::STATUS_BAD_REQUEST);
 		}
 	}
 
@@ -185,8 +192,8 @@ public function disconnect(int $id): DataResponse {
 		try {
 			$this->syncService->disconnect($this->userId, $id);
 			return new DataResponse(['status' => 'ok']);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to disconnect bank'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to disconnect bank'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 
@@ -216,8 +223,8 @@ public function updateConnection(int $id): DataResponse {
 			$this->syncService->updateConnectionEntity($connection);
 
 			return new DataResponse($connection);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to update connection'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to update connection'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 
@@ -234,8 +241,8 @@ public function sync(int $id, bool $force = false): DataResponse {
 		try {
 			$result = $this->syncService->sync($this->userId, $id, $force);
 			return new DataResponse($result);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to sync bank data'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to sync bank data'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 
@@ -266,8 +273,8 @@ public function mappings(int $id): DataResponse {
 			}
 
 			return new DataResponse($mappings);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to fetch mappings'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to fetch mappings'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 
@@ -290,8 +297,8 @@ public function updateMapping(int $id, int $mappingId): DataResponse {
 
 			$mapping = $this->syncService->updateMapping($this->userId, $id, $mappingId, $budgetAccountId, $clearBudgetAccount, $enabled);
 			return new DataResponse($mapping);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to update mapping'), Http::STATUS_BAD_REQUEST);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to update mapping'), Http::STATUS_BAD_REQUEST);
 		}
 	}
 
@@ -308,8 +315,8 @@ public function refreshAccounts(int $id): DataResponse {
 		try {
 			$mappings = $this->syncService->refreshAccounts($this->userId, $id);
 			return new DataResponse($mappings);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to refresh accounts'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to refresh accounts'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 
@@ -341,8 +348,8 @@ public function reauthorize(int $id): DataResponse {
 
 			$result = $this->syncService->reauthorize($this->userId, $id, $institutionId, $redirectUrl ?? '');
 			return new DataResponse($result);
-		} catch (\Exception $e) {
-			return $this->handleError($e, $this->l->t('Failed to re-authorize'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
+		} catch (\Throwable $e) {
+			return $this->handleErrorWithoutTrace($e, $this->l->t('Failed to re-authorize'), Http::STATUS_BAD_REQUEST, ['connectionId' => $id]);
 		}
 	}
 

budget/lib/Service/BankSync/BankConnectionRevoker.php

@@ -0,0 +1,93 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\Budget\Service\BankSync;
+
+use OCA\Budget\Db\BankConnectionMapper;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Ends a user's bank connections at the provider (a GoCardless requisition
+ * is deleted, so the bank consent stops), best-effort.
+ *
+ * Disconnect always did this. A factory reset, clearing the sample data and
+ * purging a deleted user delete the connection rows as a table, so the
+ * consent the user gave their bank stayed live at the provider with nothing
+ * left in Budget to end it. They now read the connections first and revoke
+ * them once the rows are gone.
+ *
+ * Nothing here throws, and a failure is logged with the exception's class
+ * and message only: the credentials are this class's arguments, and an
+ * exception object carries them in its trace on PHP 8.1.
+ */
+class BankConnectionRevoker {
+	public function __construct(
+		private BankConnectionMapper $connectionMapper,
+		private ProviderFactory $providerFactory,
+		private LoggerInterface $logger,
+	) {
+	}
+
+	/**
+	 * The user's connections with their decrypted credentials, read before
+	 * the rows are deleted. A connection whose secret can't be read is
+	 * skipped (there is nothing to revoke with), and a failed read gives an
+	 * empty list: this must never be what stops a reset.
+	 *
+	 * @return list<array{id: int, provider: string, credentials: string}>
+	 */
+	public function collect(string $userId): array {
+		try {
+			$connections = $this->connectionMapper->findAll($userId);
+		} catch (\Throwable $e) {
+			$this->logger->warning('Could not read the bank connections of {user} to revoke them: {class}: {message}', [
+				'app' => 'budget',
+				'user' => $userId,
+				'class' => get_class($e),
+				'message' => $e->getMessage(),
+			]);
+			return [];
+		}
+
+		$collected = [];
+		foreach ($connections as $connection) {
+			$credentials = $connection->getCredentials();
+			if (!is_string($credentials) || $credentials === '') {
+				continue;
+			}
+			$collected[] = [
+				'id' => (int)$connection->getId(),
+				'provider' => (string)$connection->getProvider(),
+				'credentials' => $credentials,
+			];
+		}
+		return $collected;
+	}
+
+	/**
+	 * @param list<array{id: int, provider: string, credentials: string}> $connections from collect()
+	 */
+	public function revokeAll(array $connections): void {
+		foreach ($connections as $connection) {
+			$this->revoke($connection['provider'], $connection['credentials'], $connection['id']);
+		}
+	}
+
+	public function revoke(string $providerName, ?string $credentials, ?int $connectionId = null): void {
+		if ($credentials === null || $credentials === '') {
+			return;
+		}
+		try {
+			$this->providerFactory->getProvider($providerName)->revokeConnection($credentials);
+		} catch (\Throwable $e) {
+			$this->logger->warning('Failed to revoke bank connection {connection} at {provider}: {class}: {message}', [
+				'app' => 'budget',
+				'connection' => $connectionId,
+				'provider' => $providerName,
+				'class' => get_class($e),
+				'message' => $e->getMessage(),
+			]);
+		}
+	}
+}