[AI] fix(enablebanking): fix HypoVereinsbank auth and data access#8355
Open
cakallie wants to merge 10 commits into
Open
[AI] fix(enablebanking): fix HypoVereinsbank auth and data access#8355cakallie wants to merge 10 commits into
cakallie wants to merge 10 commits into
Conversation
added 2 commits
June 28, 2026 11:17
HypoVereinsbank falls back to corporate auth portal when psu-ip-address header is missing, because the first auth_method is "business". Passing the PSU headers from the incoming request selects the personal banking method instead.
HypoVereinsbank requires Psu-Ip-Address during authorization to select the personal auth method, but rejects it on balance/transaction API calls with ASPSP_ERROR. Restrict PSU headers to startAuth only. Also add balances/transactions consent scopes to the auth request as required by the Enable Banking API.
actual-github-bot
Bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
A quick note on volume: please get one PR reviewed and merged before opening several more. A stack of simultaneous, similar PRs from one author is reviewed slowly, and low-effort, untested, or undisclosed-AI PRs may be closed without a detailed review. We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnable Banking auth and data-access requests now forward PSU headers conditionally, include ASPSP names in transaction downloads, and add a release note entry for the fix. ChangesEnable Banking request routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sync-server/src/app-enablebanking/app-enablebanking.ts`:
- Line 118: Scope the PSU-header stripping workaround to HypoVereinsbank only;
right now the `buildSessionResult()` path applies it for every ASPSP and can
affect downstream balance fetches and rate-limit handling. Update the logic
around `buildSessionResult` and the shared request helper so PSU headers are
removed only when the current bank is HypoVereinsbank, and leave all other ASPSP
requests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f80f686-8739-4eca-8c56-91671e4f6b0f
📒 Files selected for processing (2)
packages/sync-server/src/app-enablebanking/app-enablebanking.tspackages/sync-server/src/app-enablebanking/services/enablebanking-service.ts
Instead of stripping PSU headers globally from balance/transaction fetches, introduce `ASPSPS_WITHOUT_DATA_ACCESS_PSU_HEADERS` and `dataAccessPsuHeaders()` so only ASPSPs known to reject these headers are affected. Other ASPSPs continue to receive PSU headers on data access calls. Also restore PSU headers being passed through `buildSessionResult()` at both the auth_callback and poll-auth call sites, and propagate `aspspName` from the client via the `/transactions` endpoint so the scoping helper has the context it needs to decide. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/sync-server/src/app-enablebanking/app-enablebanking.ts (1)
52-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't drop PSU headers when the ASPSP name is missing.
Line 56 currently treats an unknown
aspspNamethe same as the HypoVereinsbank allowlist case. That means bothbuildSessionResult(session.aspsp?.name, ...)and/transactionswill strip PSU headers whenever the bank name is absent, even though this workaround is supposed to be bank-specific.Suggested fix
function dataAccessPsuHeaders( aspspName: string | undefined, psuHeaders: PsuHeaders | undefined, ): PsuHeaders | undefined { - if (!aspspName || ASPSPS_WITHOUT_DATA_ACCESS_PSU_HEADERS.has(aspspName)) { + if ( + aspspName && + ASPSPS_WITHOUT_DATA_ACCESS_PSU_HEADERS.has(aspspName) + ) { return undefined; } return psuHeaders; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sync-server/src/app-enablebanking/app-enablebanking.ts` around lines 52 - 59, The dataAccessPsuHeaders helper is incorrectly dropping PSU headers whenever aspspName is missing, which makes unknown banks behave like the ASPSPS_WITHOUT_DATA_ACCESS_PSU_HEADERS allowlist. Update dataAccessPsuHeaders so it only returns undefined for explicit allowlisted ASPSP names and preserves psuHeaders when aspspName is absent; this will keep buildSessionResult(session.aspsp?.name, ...) and /transactions from stripping headers unless the bank is specifically covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/sync-server/src/app-enablebanking/app-enablebanking.ts`:
- Around line 52-59: The dataAccessPsuHeaders helper is incorrectly dropping PSU
headers whenever aspspName is missing, which makes unknown banks behave like the
ASPSPS_WITHOUT_DATA_ACCESS_PSU_HEADERS allowlist. Update dataAccessPsuHeaders so
it only returns undefined for explicit allowlisted ASPSP names and preserves
psuHeaders when aspspName is absent; this will keep
buildSessionResult(session.aspsp?.name, ...) and /transactions from stripping
headers unless the bank is specifically covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ac627d3-3c78-450d-875e-6a35d23a8ea8
📒 Files selected for processing (2)
packages/loot-core/src/server/accounts/sync.tspackages/sync-server/src/app-enablebanking/app-enablebanking.ts
…known The previous condition `!aspspName || set.has(aspspName)` stripped PSU headers whenever aspspName was undefined, breaking all ASPSPs that don't send it. The correct default is to pass headers through for unknown ASPSPs and only strip them when the ASPSP is explicitly listed in the exclusion set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cakallie
changed the title
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
1 participant
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.
Description
HypoVereinsbank auths via Enable Banking were routed to the corporate portal because the Psu-Ip-Address header was missing. With it present, the personal auth method is selected correctly. Additionally, PSU headers must not be sent on balance/transaction API calls — HypoVereinsbank returns an ASPSP_ERROR when they are present.
AI disclosure
This fix was developed with Claude Code (Anthropic).
The bug was identified and verified by a human; Claude Code assisted with
root cause analysis and implementation. Disclosed per the
AI Usage Policy.
🤖 Maintainer: please add the
AI generatedlabel per the AI usage policy.Related issue(s)
Testing
Tested locally with a custom Docker build of the sync-server against a HypoVereinsbank personal account via Enable Banking. Authentication now correctly routes to the personal banking portal, and balance/transaction sync completes successfully.
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/accounts/sync.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/accounts/sync.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
crdt
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged