Repository navigation
fix(bank-sync): treat provider data as untrusted and keep secrets out of logs - #474
Merged
Merged
Conversation
… of logs - Escape a mapping's last balance and currency in the Account Mappings list, and store a currency only when it is a three-letter code and a balance only when it is a number. - Open the bank authorization link with noopener,noreferrer (the fallback link carries the same rel), and refuse a link that isn't https on the server before the connection is stored or the browser sees it. - Factory reset, clearing sample data and purging a deleted user now revoke each bank connection at the provider once the delete commits, through the same path Disconnect uses. Best-effort: failures are logged and never stop the reset. - Bank sync controller errors are logged with the exception's class and message only, never the exception object, whose trace carries the decrypted credentials on PHP 8.1. Throwable is caught so a TypeError doesn't reach Nextcloud's own handler with its trace.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardening for the existing bank-sync providers (GoCardless, SimpleFIN), found while reviewing #454. None of it needs a hostile user; items 1-2 need a hostile or compromised upstream, so all low severity.
renderMappings()putlastCurrency/lastBalancefrom the provider into innerHTML unescaped. Now escaped, and the server keeps a currency only if it's a 3-letter code (uppercased) and a balance only if numeric, through oneapplyProviderAccountDetails()used by connect, sync and refresh. On a throwaway instance a seeded<img onerror>currency created live elements before (only CSP stopped the handler) and renders as text after.noopener,noreferrer, fallback link getsrel="noopener noreferrer", and the server refuses any link that isn'thttps://with a host, before a connection is stored or updated. With noopener a blocked popup can't be detected, so the "click here if it didn't open" line now always shows.FactoryResetService::reset(), which deleted connections without telling the provider. NewBankConnectionRevoker(shared with Disconnect) reads credentials before the delete and revokes best-effort after the commit, so a rolled-back reset keeps working connections and provider calls don't run inside the DB transaction. Failures are logged without credentials and never block the reset.#[\SensitiveParameter]is ignored on 8.1, andlogError()attached the exception, whose trace can hold credential arguments. BankSyncController now useshandleErrorWithoutTrace()(logs class + file:line, same response), and catches\Throwableso a provider TypeError doesn't reach Nextcloud's own full-trace logger (that turns such a 500 into the usual 400 with the generic message).Tests: new Vitest (escaping, https check, noopener) and PHPUnit (cleaning, https refusal on connect/reauthorize, revoker, reset ordering/rollback/failure, log context has no secret); the new tests fail on the old code. Full PHPUnit 7570 OK, Vitest 2242, psalm and cs clean. No new strings.