From 432466088824e87d374dfd6bcc910f4f18fc3dd9 Mon Sep 17 00:00:00 2001 From: Lars Nolden Date: Thu, 10 Sep 2026 17:47:44 +0200 Subject: [PATCH] Pace provider traffic and identify genuine foreground bank requests --- OPERATIONS.txt | 40 ++++- README.md | 6 +- internal/app/import.go | 4 + internal/app/sync_test.go | 10 +- internal/banking/enablebanking.go | 131 +++++++++++++- internal/banking/enablebanking_test.go | 180 +++++++++++++++++++ internal/classification/client.go | 3 +- internal/classification/client_test.go | 5 +- internal/classification/rate_limit_test.go | 55 ++++++ internal/ratelimit/controller.go | 89 ++++++++-- internal/ratelimit/controller_test.go | 190 +++++++++++++++++++++ internal/server/server.go | 42 ++++- internal/server/server_test.go | 94 ++++++++++ 13 files changed, 817 insertions(+), 32 deletions(-) create mode 100644 internal/ratelimit/controller_test.go diff --git a/OPERATIONS.txt b/OPERATIONS.txt index f91fcc3..33c2c39 100644 --- a/OPERATIONS.txt +++ b/OPERATIONS.txt @@ -157,12 +157,17 @@ imports and previews, including previews choosing another model, share the AI gate. HTTP 429 retries retain the original request and privacy/authentication controls. Other HTTP failures are not automatically retried. -Each operation makes at most four attempts. Without a usable Retry-After hint, -retry delays are 1, 2 and 4 seconds; an exhausted fourth attempt retains an -8-second cooldown. Retry-After delta-seconds and HTTP dates can extend these -delays, never shorten them. Automatic waiting totals at most two minutes, -separate from each network attempt's timeout (bank: at most 30 seconds; AI: -45 seconds by default). Waiting and queued requests honor context cancellation. +Every outbound attempt is paced, including successful calls: AI request starts +are at least 3 seconds apart; bank starts at least 1 second apart. These are +conservative application defaults, not universal provider quota guarantees. +On 429, exponential fallback begins at 15 seconds for AI and 30 seconds for +banking. Consecutive failures escalate across operations, capped at 15 minutes; +success resets failure escalation but retains learned spacing up to 30 seconds. +Retry-After seconds and HTTP dates can extend delays, never shorten them. +Each operation makes at most four attempts and spends at most two minutes in +automatic retry waits. Normal pacing waits and queueing honor cancellation. +Network attempt timeouts remain separate (bank: at most 30 seconds; AI: +45 seconds by default). Long hints, exhausted attempts, and canceled retries retain the provider's cooldown. New calls fail without contacting that provider before its retry @@ -180,6 +185,29 @@ An unavailable session reports its original safe rate error once, without additional unavailable-account errors or a false reconnection requirement. Failed-account cursors and the last successful complete sync remain unchanged. +Manual sync, history import and balance HTTP requests forward actual PSU +metadata to account-data GET requests, consistently across retries/pages. +Only IP, User-Agent and available browser Accept headers are forwarded; never +cookies, authorization headers, referrer URLs, or arbitrary supplied Psu-* fields. +Scheduled sync contexts have no PSU metadata. User metadata is request-local, +never saved on a shared bank client or copied into classification requests. + +With a public origin configured, a loopback reverse proxy must append its +observed client IP to X-Forwarded-For. Only its rightmost appended address is +trusted; direct/non-loopback callers cannot override their peer IP. Missing or +malformed proxy addresses remain unknown, not invented. Tailscale Serve uses +this deployment arrangement; other proxies must preserve this trust contract. + +For a confirmed background ASPSP_RATE_LIMIT_EXCEEDED, defer that account and +endpoint at least six hours (longer provider hints take precedence), rather +than replaying every few seconds. This background-only quota does not suppress +an eligible genuine user request or another account/endpoint. Short generic +platform limits still use the shared bank controller for both request modes. +Only the bounded, exact error code is inspected; private provider messages are +not returned or logged. Quota state is client-local, finite entries expire, +and restart does not lift the upstream bank's actual quota. +Source: https://enablebanking.com/docs/faq/ + Enable Banking -------------- Register your application and public certificate with Enable Banking. For diff --git a/README.md b/README.md index 985f4dc..5062bcf 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,8 @@ Initial synchronization requests the selected number of **calendar months of boo **HTTP 429 is a provider rate limit, not evidence that bank consent has expired.** Bank reads honor `Retry-After` and use bounded exponential retries. A longer or exhausted limit pauses further requests until the reported retry time; failed accounts keep their previous sync cursors and imported data. Session checks use the saved account metadata rather than fetching every account's details again. A failed session is reported once instead of also marking each of its accounts unavailable. After the cooldown, **Sync now** can retry; the warning clears after a successful sync. One-time authorization and code-exchange requests are never automatically replayed. +Manual **Sync now**, **Import older history**, and balance requests forward the requesting user's IP, browser User-Agent, and available Accept headers to the bank as PSU metadata. Scheduled syncs never claim a user is present. This distinction matters: [Enable Banking documents background limits of roughly four fetches per day at many banks](https://enablebanking.com/docs/faq/#why-am-i-getting-429-response-code-are-there-rate-limits-for-the-api). A confirmed background `ASPSP_RATE_LIMIT_EXCEEDED` defers that account's affected endpoint for at least six hours, preserving longer provider hints; it does not block an eligible user-initiated fetch. General provider limits still apply to both. Bank requests are spaced by at least one second, with longer learned spacing after throttling. + ### Import older history for a connected account Open **Accounts**, find the account, and click **Import older history**. Choose **Months back** (default **12**, whole numbers from **1 to 120**) and confirm. This requests that account's booked transactions from the selected number of calendar months ago through today; the bank may provide less history. @@ -351,6 +353,8 @@ A direct bind to a VPN interface is also supported with `-listen :8080` Open **Settings → OpenRouter credentials**, paste your API key, and click **Save key**. Then choose an exact OpenRouter `provider/model` identifier under **Classification preferences** and save those preferences. No SSH, Nix configuration changes, or service restart is needed. +Use the complete identifier, for example **`deepseek/deepseek-v4.1-flash`**, not just `deepseek-v4.1-flash`. Verify identifiers in OpenRouter's model catalog rather than relying on a model's display name. + Use **Replace key** to rotate the credential or **Remove key** to disable AI. Changes apply to future classifications immediately and survive restart; an already-running classification keeps the key it started with. “Configured” means a key is present, not that OpenRouter has accepted it. A successful **AI classification → Analyse** request checks the key, model, and private routing together. The key is stored separately from preferences and journals in `state/openrouter.json` beneath your data directory (`/var/lib/finance-duck/state/openrouter.json` on the deployed server), as a private `0600` plaintext file. The API never returns it, and the UI clears the password field after saving rather than storing it in browser storage. Treat data-directory backups as containing credentials. Anyone able to access this no-login application can replace or remove the key: keep access restricted to trusted tailnet users. @@ -361,7 +365,7 @@ Bank imports do **not** require this key. Without AI, explicit merchant-default Every AI classification requests `provider.data_collection = "deny"`, `provider.zdr = true`, and `provider.require_parameters = true`. Unsupported private routing fails rather than falling back to a less restrictive provider. Amount sharing is off by default. Keep OpenRouter account prompt logging disabled as well. Automatic redaction minimizes data; it is not a guarantee that arbitrary transaction prose is anonymous. -Classification honors HTTP 429 `Retry-After` delays (seconds or HTTP dates), with exponential backoff when no usable delay is supplied. It makes at most **four attempts**, with at most **two minutes of automatic waiting**, preserving the same model, sanitized prompt, and privacy controls on every attempt. Imports and classification previews share a cooldown, so remaining records cannot immediately hammer a rate-limited provider. Long or exhausted limits leave records unclassified with a retry-time error; local merchant rules still work. After the cooldown, run **AI classification → Analyse** again for previously failed records—repeating a bank import does not reclassify existing transactions. +Classification spaces request starts by at least **three seconds**, including successful requests, rather than sending a burst between 429s. This is a conservative application policy, not a published quota for every model. On HTTP 429, backoff starts at **15 seconds** and increases across consecutive failures; `Retry-After` seconds or HTTP dates can extend the wait. Successful retries retain the learned spacing (up to **30 seconds**) instead of immediately bursting again. Each operation makes at most **four attempts**, with at most **two minutes of automatic retry waiting**, preserving the same model, sanitized prompt, and privacy controls. Imports and previews share this pacing and cooldown. Long or exhausted limits leave records unclassified with a retry-time error; local merchant rules still work. After the cooldown, run **AI classification → Analyse** again for previously failed records—repeating a bank import does not reclassify existing transactions. ## Data, backups, and recovery diff --git a/internal/app/import.go b/internal/app/import.go index d0fdc79..489deed 100644 --- a/internal/app/import.go +++ b/internal/app/import.go @@ -299,6 +299,10 @@ func (a *App) Balances(ctx context.Context, id string) ([]banking.Balance, error // Only typed, locally generated errors are safe to expose; provider errors may // wrap private response data even when their underlying cause is recognizable. func bankFailure(err error, fallback string) error { + var background *banking.BackgroundQuotaError + if errors.As(err, &background) { + return fmt.Errorf("Enable Banking: %w", background) + } var limited *ratelimit.RateLimitError if errors.As(err, &limited) { return fmt.Errorf("Enable Banking: %w", limited) diff --git a/internal/app/sync_test.go b/internal/app/sync_test.go index c0f8321..6c42500 100644 --- a/internal/app/sync_test.go +++ b/internal/app/sync_test.go @@ -270,7 +270,7 @@ func TestSyncMissingMembershipStillRejectsAccount(t *testing.T) { } func TestSyncTransactionFailuresPreserveProgressAndSafeErrors(t *testing.T) { - for _, scenario := range []string{"rate limit", "reconnect", "private response"} { + for _, scenario := range []string{"rate limit", "background rate limit", "reconnect", "private response"} { t.Run(scenario, func(t *testing.T) { a, s, b := backfillApp(t) s = seed(t, a, s) @@ -282,6 +282,9 @@ func TestSyncTransactionFailuresPreserveProgressAndSafeErrors(t *testing.T) { switch scenario { case "rate limit": b.fetchErr = bankRateError(t) + case "background rate limit": + quota := &banking.BackgroundQuotaError{RateLimitError: ratelimit.NewError(time.Now().Add(6 * time.Hour))} + b.fetchErr = fmt.Errorf("private provider response: %w", quota) case "reconnect": b.fetchErr = fmt.Errorf("private provider response: %w", banking.ErrReconnect) case "private response": @@ -295,9 +298,12 @@ func TestSyncTransactionFailuresPreserveProgressAndSafeErrors(t *testing.T) { if failed.Status.SyncError == "" || meta.Error == "" || strings.Contains(failed.Status.SyncError+meta.Error, "private provider response") { t.Fatal("transaction failure was lost or exposed provider data") } - if scenario == "rate limit" && (!strings.Contains(failed.Status.SyncError, "429") || !strings.Contains(meta.Error, "429") || !strings.Contains(meta.Error, "retry")) { + if strings.Contains(scenario, "rate limit") && (!strings.Contains(failed.Status.SyncError, "429") || !strings.Contains(meta.Error, "429") || !strings.Contains(meta.Error, "retry")) { t.Fatal("transaction rate error was obscured") } + if scenario == "background rate limit" && !strings.Contains(meta.Error, "background") { + t.Fatal("daily bank quota was confused with a short request throttle") + } if meta.NeedsReconnect != (scenario == "reconnect") { t.Fatal("transaction failure classified consent incorrectly") } diff --git a/internal/banking/enablebanking.go b/internal/banking/enablebanking.go index 8d90f67..08338e2 100644 --- a/internal/banking/enablebanking.go +++ b/internal/banking/enablebanking.go @@ -17,8 +17,10 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" + "strconv" "strings" "time" @@ -62,6 +64,8 @@ type EnableBanking struct { key *rsa.PrivateKey redirectURL string requests ratelimit.Controller + // Accessed only while requests is acquired. Keys exclude pagination/query data. + backgroundQuotas map[string]time.Time } var _ Provider = (*EnableBanking)(nil) @@ -116,7 +120,7 @@ func NewEnableBanking(appID string, keyPEM []byte, redirectURL string) (*EnableB if err = key.Validate(); err != nil { return nil, fmt.Errorf("invalid Enable Banking RSA private key") } - return &EnableBanking{HTTPClient: &http.Client{Timeout: 30 * time.Second}, BaseURL: "https://api.enablebanking.com", appID: appID, key: key, redirectURL: redirectURL}, nil + return &EnableBanking{HTTPClient: &http.Client{Timeout: 30 * time.Second}, BaseURL: "https://api.enablebanking.com", appID: appID, key: key, redirectURL: redirectURL, requests: ratelimit.Controller{MinimumInterval: time.Second, InitialBackoff: 30 * time.Second}}, nil } func (p *EnableBanking) jwt() (string, error) { if p.key == nil || p.appID == "" { @@ -133,11 +137,114 @@ func (p *EnableBanking) jwt() (string, error) { } return unsigned + "." + base64.RawURLEncoding.EncodeToString(signature), nil } + +// PSU contains only the documented, nonsecret browser metadata for a person +// actively requesting account data. It must never be stored on a shared client. +type PSU struct { + IPAddress string + UserAgent string + Accept string + AcceptCharset string + AcceptEncoding string + AcceptLanguage string +} + +type psuContextKey struct{} + +// WithPSU marks this request context as user initiated. Call only at a manual +// HTTP boundary, never for scheduled retrieval. Unknown metadata stays empty. +func WithPSU(ctx context.Context, psu PSU) context.Context { + return context.WithValue(ctx, psuContextKey{}, psu) +} + +func (psu PSU) setHeaders(header http.Header) { + if ip := net.ParseIP(psu.IPAddress); ip != nil { + header.Set("Psu-Ip-Address", ip.String()) + } + for _, field := range []struct{ name, value string }{ + {"Psu-User-Agent", psu.UserAgent}, + {"Psu-Accept", psu.Accept}, + {"Psu-Accept-Charset", psu.AcceptCharset}, + {"Psu-Accept-Encoding", psu.AcceptEncoding}, + {"Psu-Accept-Language", psu.AcceptLanguage}, + } { + if field.value != "" { + header.Set(field.name, field.value) + } + } +} + +// accountDataPath admits only the internally generated account GET endpoints. +// The escaped account segment is retained; dates and continuation keys are not. +func accountDataPath(method, path string) string { + if method != http.MethodGet { + return "" + } + path, _, _ = strings.Cut(path, "?") + parts := strings.Split(path, "/") + if len(parts) != 4 || parts[0] != "" || parts[1] != "accounts" || (parts[3] != "balances" && parts[3] != "transactions") { + return "" + } + account, err := url.PathUnescape(parts[2]) + if err != nil || account == "" || account == "." || account == ".." || strings.ContainsAny(account, "/\\") || url.PathEscape(account) != parts[2] { + return "" + } + return path +} + +// BackgroundQuotaError identifies the bank's background-only account quota. +// It preserves RateLimitError identity and exposes no provider response text. +type BackgroundQuotaError struct { + *ratelimit.RateLimitError +} + +func (e *BackgroundQuotaError) Error() string { + return "background bank retrieval quota (normally six hours): " + e.RateLimitError.Error() +} + +func (e *BackgroundQuotaError) Unwrap() error { + return e.RateLimitError +} + +// backgroundRetryAt follows the bank's six-hour guidance, never shortening a +// longer provider deadline. Zero retains the limiter's unbounded-delay meaning. +func backgroundRetryAt(header string, now time.Time) time.Time { + next := now.Add(6 * time.Hour) + header = strings.TrimSpace(header) + if header == "" { + return next + } + if strings.Trim(header, "0123456789") == "" { + seconds, err := strconv.ParseUint(header, 10, 64) + if err != nil || seconds > uint64((1<<63-1)/int64(time.Second)) { + return time.Time{} + } + if delay := time.Duration(seconds) * time.Second; delay > 6*time.Hour { + return now.Add(delay) + } + } else if date, err := http.ParseTime(header); err == nil && date.After(next) { + return date + } + return next +} func (p *EnableBanking) request(ctx context.Context, method, path string, input, output any) error { if err := p.requests.Acquire(ctx); err != nil { return fmt.Errorf("Enable Banking: %w", err) } defer p.requests.Release() + scope := accountDataPath(method, path) + psu, foreground := ctx.Value(psuContextKey{}).(PSU) + now := time.Now() + for key, next := range p.backgroundQuotas { + if !next.IsZero() && !next.After(now) { + delete(p.backgroundQuotas, key) + } + } + if scope != "" && !foreground { + if next, limited := p.backgroundQuotas[scope]; limited { + return fmt.Errorf("Enable Banking: %w", &BackgroundQuotaError{ratelimit.NewError(next)}) + } + } token, err := p.jwt() if err != nil { return err @@ -160,6 +267,9 @@ func (p *EnableBanking) request(ctx context.Context, method, path string, input, } req.Header.Set("Authorization", "Bearer "+token) req.Header.Set("Accept", "application/json") + if scope != "" && foreground { + psu.setHeaders(req.Header) + } if input != nil { req.Header.Set("Content-Type", "application/json") } @@ -184,6 +294,25 @@ func (p *EnableBanking) request(ctx context.Context, method, path string, input, } return nil, errors.New("connection failed") } + if response.StatusCode == http.StatusTooManyRequests && scope != "" && !foreground { + // Inspect only a small structured error envelope, never exposing its + // detail/message or retaining response bytes. Other 429s remain the + // common controller's responsibility (including body closure). + const errorLimit = 16 << 10 + body, readErr := io.ReadAll(io.LimitReader(response.Body, errorLimit+1)) + var envelope struct { + Error string `json:"error"` + } + if readErr == nil && len(body) <= errorLimit && json.Unmarshal(body, &envelope) == nil && envelope.Error == "ASPSP_RATE_LIMIT_EXCEEDED" { + next := backgroundRetryAt(response.Header.Get("Retry-After"), time.Now()) + if p.backgroundQuotas == nil { + p.backgroundQuotas = make(map[string]time.Time) + } + p.backgroundQuotas[scope] = next + response.Body.Close() + return nil, &BackgroundQuotaError{ratelimit.NewError(next)} + } + } return response, nil }, method == http.MethodGet) if err != nil { diff --git a/internal/banking/enablebanking_test.go b/internal/banking/enablebanking_test.go index 400715a..66ef3dd 100644 --- a/internal/banking/enablebanking_test.go +++ b/internal/banking/enablebanking_test.go @@ -39,6 +39,7 @@ func testProvider(t *testing.T, handler http.HandlerFunc) (*EnableBanking, *rsa. t.Cleanup(server.Close) p.BaseURL = server.URL p.HTTPClient = server.Client() + p.requests = ratelimit.Controller{} return p, key } func assertJWT(t *testing.T, r *http.Request, key *rsa.PrivateKey) { @@ -462,6 +463,185 @@ func TestEnableBankingCanceledRetryRetainsCooldown(t *testing.T) { } } +func TestEnableBankingPSUSurvivesRetryAndPaginationWithoutLeakingToBackground(t *testing.T) { + var manualCalls, backgroundCalls atomic.Int32 + psu := PSU{IPAddress: "203.0.113.42", UserAgent: "test-browser", Accept: "application/json", AcceptCharset: "utf-8", AcceptEncoding: "gzip", AcceptLanguage: "de"} + p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/accounts/uid/transactions" { + for name, want := range map[string]string{ + "Psu-Ip-Address": psu.IPAddress, "Psu-User-Agent": psu.UserAgent, + "Psu-Accept": psu.Accept, "Psu-Accept-Charset": psu.AcceptCharset, + "Psu-Accept-Encoding": psu.AcceptEncoding, "Psu-Accept-Language": psu.AcceptLanguage, + } { + if got := r.Header.Get(name); got != want { + t.Errorf("%s = %q, want %q", name, got, want) + } + } + switch manualCalls.Add(1) { + case 1: + w.WriteHeader(http.StatusTooManyRequests) + fmt.Fprint(w, `{"error":"ASPSP_RATE_LIMIT_EXCEEDED","detail":"private"}`) + case 2: + fmt.Fprint(w, `{"transactions":[],"continuation_key":"next-private-page"}`) + case 3: + if r.URL.Query().Get("continuation_key") != "next-private-page" || r.URL.Query().Get("date_from") != "2026-01-01" { + t.Error("pagination lost original filters or continuation") + } + fmt.Fprint(w, `{"transactions":[{"entry_reference":"entry","transaction_amount":{"amount":"1.00","currency":"EUR"},"credit_debit_indicator":"CRDT","status":"BOOK","booking_date":"2026-09-01"}]}`) + default: + t.Error("unexpected manual replay") + } + return + } + backgroundCalls.Add(1) + for name := range r.Header { + if strings.HasPrefix(strings.ToLower(name), "psu-") { + t.Errorf("PSU metadata escaped manual account request: %s", name) + } + } + if r.URL.Path == "/sessions/session" { + fmt.Fprint(w, `{"status":"AUTHORIZED","accounts":["uid"],"access":{"valid_until":"2099-01-01T00:00:00Z"}}`) + } else { + fmt.Fprint(w, `{"balances":[]}`) + } + }) + p.requests = ratelimit.Controller{InitialBackoff: time.Millisecond} + ctx := WithPSU(context.Background(), psu) + account := fixtureDataset().Accounts[0] + account.ExternalAccountID = "uid" + rows, err := p.Transactions(ctx, account, "2026-01-01", "") + if err != nil || len(rows) != 1 || rows[0].ExternalID != "entry" { + t.Fatalf("manual history failed: %+v %v", rows, err) + } + if _, err := p.Balances(context.Background(), "uid"); err != nil { + t.Fatal(err) + } + if _, err := p.Status(ctx, "session"); err != nil { + t.Fatal(err) + } + if manualCalls.Load() != 3 || backgroundCalls.Load() != 2 { + t.Fatalf("unexpected request counts: manual=%d other=%d", manualCalls.Load(), backgroundCalls.Load()) + } +} + +func TestEnableBankingBackgroundQuotaIsScopedAndExpires(t *testing.T) { + var calls atomic.Int32 + p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) == 1 { + w.WriteHeader(http.StatusTooManyRequests) + fmt.Fprint(w, `{"error":"ASPSP_RATE_LIMIT_EXCEEDED","message":"private-bank-message"}`) + return + } + if strings.HasSuffix(r.URL.Path, "/transactions") { + fmt.Fprint(w, `{"transactions":[]}`) + } else { + fmt.Fprint(w, `{"balances":[]}`) + } + }) + before := time.Now() + _, err := p.Balances(context.Background(), "uid") + var first *ratelimit.RateLimitError + if !errors.As(err, &first) || first.RetryAt().Before(before.Add(6*time.Hour)) || first.RetryAt().After(time.Now().Add(6*time.Hour)) || strings.Contains(err.Error(), "private") || errors.Is(err, ErrReconnect) { + t.Fatalf("missing safe six-hour bank quota: %v", err) + } + var quota *BackgroundQuotaError + if !errors.As(err, "a) || !quota.RetryAt().Equal(first.RetryAt()) { + t.Fatalf("background quota identity was lost: %v", err) + } + _, err = p.Balances(context.Background(), "uid") + var retained *ratelimit.RateLimitError + if !errors.As(err, &retained) || !retained.RetryAt().Equal(first.RetryAt()) || calls.Load() != 1 { + t.Fatalf("background quota was replayed: calls=%d err=%v", calls.Load(), err) + } + if _, err := p.Balances(WithPSU(context.Background(), PSU{IPAddress: "203.0.113.42", UserAgent: "test-browser"}), "uid"); err != nil { + t.Fatalf("background quota blocked real user: %v", err) + } + _, err = p.Balances(context.Background(), "uid") + if !errors.As(err, "a) || !quota.RetryAt().Equal(first.RetryAt()) || calls.Load() != 2 { + t.Fatalf("foreground success erased background quota: calls=%d err=%v", calls.Load(), err) + } + if _, err := p.Balances(context.Background(), "other-uid"); err != nil { + t.Fatalf("background quota blocked another account: %v", err) + } + account := fixtureDataset().Accounts[0] + account.ExternalAccountID = "uid" + if _, err := p.Transactions(context.Background(), account, "", ""); err != nil { + t.Fatalf("balance quota blocked transaction endpoint: %v", err) + } + // Simulate the stored deadline passing without a six-hour wall-clock wait. + p.backgroundQuotas["/accounts/uid/balances"] = time.Now().Add(-time.Second) + if _, err := p.Balances(context.Background(), "uid"); err != nil { + t.Fatalf("expired quota blocked retrieval: %v", err) + } + if calls.Load() != 5 { + t.Fatalf("unexpected bank replays: %d", calls.Load()) + } +} + +func TestEnableBankingBackgroundQuotaPreservesLongerHints(t *testing.T) { + for _, hint := range []string{"43200", time.Now().Add(24 * time.Hour).UTC().Format(http.TimeFormat), "999999999999999999999999999999999"} { + t.Run(hint, func(t *testing.T) { + var calls atomic.Int32 + p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("Retry-After", hint) + w.WriteHeader(http.StatusTooManyRequests) + fmt.Fprint(w, `{"error":"ASPSP_RATE_LIMIT_EXCEEDED"}`) + }) + before := time.Now() + _, err := p.Balances(context.Background(), "uid") + var limit *ratelimit.RateLimitError + if !errors.As(err, &limit) || calls.Load() != 1 { + t.Fatalf("quota replayed or lost: calls=%d err=%v", calls.Load(), err) + } + switch hint { + case "43200": + if limit.RetryAt().Before(before.Add(12 * time.Hour)) { + t.Fatal("shortened numeric provider hint") + } + case "999999999999999999999999999999999": + if !limit.RetryAt().IsZero() { + t.Fatal("overflow became a finite short retry") + } + default: + date, _ := http.ParseTime(hint) + if !limit.RetryAt().Equal(date) { + t.Fatal("shortened absolute provider hint") + } + } + }) + } +} + +func TestEnableBankingOnlyExactBoundedBankErrorUsesBackgroundQuota(t *testing.T) { + for name, payload := range map[string]string{ + "message only": `{"error":"OTHER","message":"ASPSP_RATE_LIMIT_EXCEEDED"}`, + "prefix": `{"error":"ASPSP_RATE_LIMIT_EXCEEDED_OTHER"}`, + "nested": `{"error":{"code":"ASPSP_RATE_LIMIT_EXCEEDED"}}`, + "oversized": `{"error":"ASPSP_RATE_LIMIT_EXCEEDED","detail":"` + strings.Repeat("x", 16<<10) + `"}`, + } { + t.Run(name, func(t *testing.T) { + var calls atomic.Int32 + p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("Retry-After", "300") + w.WriteHeader(http.StatusTooManyRequests) + fmt.Fprint(w, payload) + }) + _, err := p.Balances(context.Background(), "uid") + var first *ratelimit.RateLimitError + if !errors.As(err, &first) || first.RetryAt().After(time.Now().Add(6*time.Minute)) { + t.Fatalf("unrecognized error used bank quota: %v", err) + } + _, err = p.Balances(WithPSU(context.Background(), PSU{IPAddress: "203.0.113.42"}), "other") + var common *ratelimit.RateLimitError + if !errors.As(err, &common) || !common.RetryAt().Equal(first.RetryAt()) || calls.Load() != 1 { + t.Fatalf("generic platform cooldown not shared: calls=%d err=%v", calls.Load(), err) + } + }) + } +} + func TestEnableBankingValidatesUploadedCredentials(t *testing.T) { _, key := testProvider(t, func(w http.ResponseWriter, r *http.Request) { t.Error("credential validation must not call provider") diff --git a/internal/classification/client.go b/internal/classification/client.go index ae45f45..a48d51a 100644 --- a/internal/classification/client.go +++ b/internal/classification/client.go @@ -49,7 +49,8 @@ func (c *Client) rateControl() *ratelimit.Controller { if gate := c.rate.Load(); gate != nil { return gate } - gate := &ratelimit.Controller{} + // Conservative 20-RPM ceiling, independent of model/provider quota claims. + gate := &ratelimit.Controller{MinimumInterval: 3 * time.Second, InitialBackoff: 15 * time.Second} if c.rate.CompareAndSwap(nil, gate) { return gate } diff --git a/internal/classification/client_test.go b/internal/classification/client_test.go index 927b712..595f2b9 100644 --- a/internal/classification/client_test.go +++ b/internal/classification/client_test.go @@ -13,6 +13,7 @@ import ( "testing" "finance-duck/internal/domain" + "finance-duck/internal/ratelimit" ) func fixture() (domain.Facts, domain.Dataset) { @@ -37,7 +38,9 @@ func mockClient(t *testing.T, handler http.HandlerFunc) *Client { t.Helper() server := httptest.NewServer(handler) t.Cleanup(server.Close) - return &Client{APIKey: "test-secret", Model: "test/strict-model", BaseURL: server.URL, HTTPClient: server.Client()} + client := &Client{APIKey: "test-secret", Model: "test/strict-model", BaseURL: server.URL, HTTPClient: server.Client()} + client.rate.Store(&ratelimit.Controller{}) + return client } func TestExplicitDefaultsAreOptInAndBypassAI(t *testing.T) { diff --git a/internal/classification/rate_limit_test.go b/internal/classification/rate_limit_test.go index 92575da..7ee0917 100644 --- a/internal/classification/rate_limit_test.go +++ b/internal/classification/rate_limit_test.go @@ -12,8 +12,63 @@ import ( "sync/atomic" "testing" "time" + + "finance-duck/internal/ratelimit" ) +func TestProductionRatePolicyAcrossModelSnapshots(t *testing.T) { + t.Run("successful classifications stay paced", func(t *testing.T) { + f, d := fixture() + arrivals := make(chan time.Time, 2) + c := mockClient(t, func(w http.ResponseWriter, r *http.Request) { + arrivals <- time.Now() + reply(w, validAnswer) + }) + // Remove only the fixture's fast policy, exercising real lazy production + // initialization and the same shared controller used by previews. + c.rate.Store(nil) + snapshot := c.WithModel("test/preview-model") + ctx, cancel := context.WithTimeout(context.Background(), 6*time.Second) + defer cancel() + for _, client := range []*Client{c, snapshot} { + p, err := client.Classify(ctx, f, d, true) + if err != nil || p.Enrichment.Classification.Source != "openrouter" { + t.Fatalf("production-paced classification failed: %+v, %v", p, err) + } + } + first, second := <-arrivals, <-arrivals + if gap := second.Sub(first); gap < 3*time.Second-25*time.Millisecond { + t.Fatalf("model snapshots burst after success: %v", gap) + } + }) + t.Run("first failure retains conservative retry deadline", func(t *testing.T) { + f, d := fixture() + var calls atomic.Int32 + c := mockClient(t, func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusTooManyRequests) + }) + c.rate.Store(nil) + snapshot := c.WithModel("test/preview-model") + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + _, err := c.Classify(ctx, f, d, true) + var limit *ratelimit.RateLimitError + if !errors.Is(err, context.DeadlineExceeded) || !errors.As(err, &limit) { + t.Fatalf("production retry wait lost cancellation or quota identity: %v", err) + } + if delay := time.Until(limit.RetryAt()); delay < 14*time.Second { + t.Fatalf("production retry used a short per-record fallback: %v", delay) + } + probeCtx, probeCancel := context.WithTimeout(context.Background(), time.Second) + defer probeCancel() + _, err = snapshot.Classify(probeCtx, f, d, true) + if !errors.As(err, &limit) || errors.Is(err, context.DeadlineExceeded) || calls.Load() != 1 { + t.Fatalf("preview discarded production cooldown: %v, calls=%d", err, calls.Load()) + } + }) +} + func TestRateLimitRetryPreservesPrivateRequest(t *testing.T) { f, d := fixture() f.Counterparty = "Alice Privateperson" diff --git a/internal/ratelimit/controller.go b/internal/ratelimit/controller.go index 9e50c0d..698e9c6 100644 --- a/internal/ratelimit/controller.go +++ b/internal/ratelimit/controller.go @@ -13,18 +13,30 @@ import ( ) const ( - maxRateAttempts = 4 - maxRateWait = 2 * time.Minute + maxRateAttempts = 4 + maxRateWait = 2 * time.Minute + maxLearnedInterval = 30 * time.Second + maxBackoff = 15 * time.Minute ) // Controller serializes provider calls and retains their HTTP 429 cooldown. // Its zero value is ready for use. A Controller must not be copied after use. // The active caller owns retries; other callers fail fast during known cooldowns. type Controller struct { + // Configure before first use; these fields must remain immutable afterward. + // Zero values preserve unpaced requests and a one-second initial backoff. + MinimumInterval time.Duration + InitialBackoff time.Duration + once sync.Once active chan struct{} mu sync.Mutex limit *RateLimitError + + // Only the acquired caller accesses pacing and consecutive-failure state. + lastAttempt time.Time + learnedInterval time.Duration + backoff time.Duration } // RateLimitError is a safe provider HTTP 429 error. Its message contains only @@ -35,6 +47,12 @@ type RateLimitError struct { unbounded bool } +// NewError records a trusted provider-specific retry deadline without exposing +// provider response text. A zero deadline disables automatic retries. +func NewError(retryAt time.Time) *RateLimitError { + return &RateLimitError{next: retryAt, unbounded: retryAt.IsZero()} +} + func (r *RateLimitError) Error() string { if r.unbounded { return "provider rate limit (HTTP 429): retry time exceeds the supported range; automatic retry disabled" @@ -128,7 +146,7 @@ func retryLimit(header string, now time.Time, fallback time.Duration) *RateLimit // Its per-attempt timeout must not include this controller's retry waiting. func (g *Controller) Do(ctx context.Context, attempt func(context.Context) (*http.Response, error), retry bool) (*http.Response, error) { remainingWait := maxRateWait - var lastLimit error + var lastLimit *RateLimitError canceled := func(err error) error { if lastLimit != nil { return fmt.Errorf("%w: %w", lastLimit, err) @@ -139,6 +157,31 @@ func (g *Controller) Do(ctx context.Context, attempt func(context.Context) (*htt if err := ctx.Err(); err != nil { return nil, canceled(err) } + // Pace all attempts, not just retries, including after successful calls. + next := g.lastAttempt.Add(max(g.MinimumInterval, g.learnedInterval)) + if lastLimit != nil && lastLimit.next.After(next) { + next = lastLimit.next + } + delay := time.Until(next) + if lastLimit != nil && (lastLimit.unbounded || delay > remainingWait) { + return nil, lastLimit + } + if delay > 0 { + if lastLimit != nil { + remainingWait -= delay + } + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + timer.Stop() + return nil, canceled(ctx.Err()) + case <-timer.C: + } + } + if err := ctx.Err(); err != nil { + return nil, canceled(err) + } + g.lastAttempt = time.Now() resp, err := attempt(ctx) if err != nil { if ctx.Err() != nil { @@ -150,12 +193,36 @@ func (g *Controller) Do(ctx context.Context, attempt func(context.Context) (*htt return nil, err } if resp.StatusCode != http.StatusTooManyRequests { + if resp.StatusCode < http.StatusBadRequest { + // Recovery resets consecutive-failure escalation, but never + // erases learned spacing: a single success is not a quota reset. + g.backoff = 0 + } return resp, nil } - limit := retryLimit(resp.Header.Get("Retry-After"), time.Now(), time.Second<= maxBackoff/2 { + g.backoff = max(g.backoff, maxBackoff) + } else { + g.backoff *= 2 + } + fallback := max(g.backoff, g.MinimumInterval, g.learnedInterval) + limit := retryLimit(resp.Header.Get("Retry-After"), time.Now(), fallback) g.mu.Lock() g.limit = limit g.mu.Unlock() + // Keep the most conservative learned cadence for this controller's + // lifetime, capped at 30 seconds. The actual provider deadline is never + // capped; persistent failures separately escalate up to 15 minutes. + learned := maxLearnedInterval + if !limit.unbounded { + learned = min(learned, time.Until(limit.next)) + } + g.learnedInterval = max(g.learnedInterval, learned) // Never read or expose provider errors, and release each response before // any sleep or retry. Other responses are processed by the caller. resp.Body.Close() @@ -163,21 +230,9 @@ func (g *Controller) Do(ctx context.Context, attempt func(context.Context) (*htt if err := ctx.Err(); err != nil { return nil, canceled(err) } - delay := time.Until(limit.next) - if !retry || number == maxRateAttempts-1 || limit.unbounded || delay > remainingWait { + if !retry || number == maxRateAttempts-1 { return nil, limit } - if delay <= 0 { - continue - } - remainingWait -= delay - timer := time.NewTimer(delay) - select { - case <-ctx.Done(): - timer.Stop() - return nil, canceled(ctx.Err()) - case <-timer.C: - } } return nil, lastLimit } diff --git a/internal/ratelimit/controller_test.go b/internal/ratelimit/controller_test.go new file mode 100644 index 0000000..9239516 --- /dev/null +++ b/internal/ratelimit/controller_test.go @@ -0,0 +1,190 @@ +package ratelimit_test + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "strings" + "sync" + "testing" + "time" + + "finance-duck/internal/ratelimit" +) + +func execute(ctx context.Context, gate *ratelimit.Controller, attempt func(context.Context) (*http.Response, error), retry bool) error { + if err := gate.Acquire(ctx); err != nil { + return err + } + defer gate.Release() + response, err := gate.Do(ctx, attempt, retry) + if response != nil { + response.Body.Close() + } + return err +} + +func response(status int) *http.Response { + return &http.Response{StatusCode: status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(""))} +} + +func TestSuccessfulConcurrentCallsArePaced(t *testing.T) { + const interval = 30 * time.Millisecond + gate := &ratelimit.Controller{MinimumInterval: interval} + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + var mu sync.Mutex + var arrivals []time.Time + attempt := func(context.Context) (*http.Response, error) { + mu.Lock() + arrivals = append(arrivals, time.Now()) + mu.Unlock() + return response(http.StatusOK), nil + } + done := make(chan error, 4) + for range cap(done) { + go func() { done <- execute(ctx, gate, attempt, true) }() + } + for range cap(done) { + if err := <-done; err != nil { + t.Fatal(err) + } + } + mu.Lock() + defer mu.Unlock() + if len(arrivals) != 4 { + t.Fatalf("got %d outbound calls, want four", len(arrivals)) + } + for i := 1; i < len(arrivals); i++ { + if gap := arrivals[i].Sub(arrivals[i-1]); gap < interval-time.Millisecond { + t.Fatalf("successful calls burst: gap %v, minimum %v", gap, interval) + } + } +} + +func TestRecoveryRetainsLearnedPacing(t *testing.T) { + const initial = 20 * time.Millisecond + gate := &ratelimit.Controller{InitialBackoff: initial} + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + var arrivals []time.Time + attempt := func(context.Context) (*http.Response, error) { + arrivals = append(arrivals, time.Now()) + if len(arrivals) <= 2 { + return response(http.StatusTooManyRequests), nil + } + return response(http.StatusOK), nil + } + for range 3 { + if err := execute(ctx, gate, attempt, true); err != nil { + t.Fatal(err) + } + } + if len(arrivals) != 5 { + t.Fatalf("got %d attempts, want two retries and three successes", len(arrivals)) + } + for i := 2; i < len(arrivals); i++ { + if gap := arrivals[i].Sub(arrivals[i-1]); gap < 2*initial-time.Millisecond { + t.Fatalf("attempt %d discarded learned cadence after recovery: %v", i+1, gap) + } + } +} + +func TestExhaustionEscalatesAcrossCalls(t *testing.T) { + const initial = 10 * time.Millisecond + gate := &ratelimit.Controller{InitialBackoff: initial} + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + var arrivals []time.Time + attempt := func(context.Context) (*http.Response, error) { + arrivals = append(arrivals, time.Now()) + return response(http.StatusTooManyRequests), nil + } + err := execute(ctx, gate, attempt, true) + var exhausted *ratelimit.RateLimitError + if !errors.As(err, &exhausted) || len(arrivals) != 4 { + t.Fatalf("expected exhaustion after four attempts, got %v and %d", err, len(arrivals)) + } + for i := 1; i < len(arrivals); i++ { + if gap := arrivals[i].Sub(arrivals[i-1]); gap < (initial<<(i-1))-time.Millisecond { + t.Fatalf("retry %d did not escalate: %v", i, gap) + } + } + // A new transaction is eligible only after the fourth attempt's cooldown. + time.Sleep(time.Until(exhausted.RetryAt())) + err = execute(ctx, gate, attempt, false) + var next *ratelimit.RateLimitError + if !errors.As(err, &next) || len(arrivals) != 5 { + t.Fatalf("eligible successor did not make exactly one attempt: %v, %d", err, len(arrivals)) + } + if delay := next.RetryAt().Sub(arrivals[4]); delay < 16*initial { + t.Fatalf("new call reset consecutive-failure backoff: %v, need at least %v", delay, 16*initial) + } + if err := execute(ctx, gate, attempt, true); !errors.As(err, &next) || len(arrivals) != 5 { + t.Fatalf("retained cooldown allowed an early attempt: %v, %d", err, len(arrivals)) + } +} + +func TestCanceledPacingWaitMakesNoOutboundCall(t *testing.T) { + gate := &ratelimit.Controller{MinimumInterval: 100 * time.Millisecond} + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + calls := 0 + attempt := func(context.Context) (*http.Response, error) { + calls++ + return response(http.StatusOK), nil + } + if err := execute(ctx, gate, attempt, true); err != nil { + t.Fatal(err) + } + pacedCtx, pacedCancel := context.WithTimeout(ctx, 10*time.Millisecond) + err := execute(pacedCtx, gate, attempt, true) + pacedCancel() + if !errors.Is(err, context.DeadlineExceeded) || calls != 1 { + t.Fatalf("paced cancellation sent a request or lost context identity: %v, calls=%d", err, calls) + } + if err := execute(ctx, gate, attempt, true); err != nil || calls != 2 { + t.Fatalf("cancelled waiter leaked an attempt or blocked its successor: %v, calls=%d", err, calls) + } +} + +func TestAutomaticWaitBudgetRetainsLongCooldown(t *testing.T) { + gate := &ratelimit.Controller{InitialBackoff: 3 * time.Minute} + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + calls := 0 + attempt := func(context.Context) (*http.Response, error) { + calls++ + return response(http.StatusTooManyRequests), nil + } + for range 2 { + err := execute(ctx, gate, attempt, true) + var limit *ratelimit.RateLimitError + if !errors.As(err, &limit) || errors.Is(err, context.DeadlineExceeded) || calls != 1 { + t.Fatalf("over-budget cooldown waited or replayed: %v, calls=%d", err, calls) + } + } +} + +func TestProviderSpecificErrorDoesNotPauseUnrelatedCalls(t *testing.T) { + for _, deadline := range []time.Time{time.Now().Add(6 * time.Hour), {}} { + gate := &ratelimit.Controller{} + err := execute(context.Background(), gate, func(context.Context) (*http.Response, error) { + return nil, fmt.Errorf("account quota: %w", ratelimit.NewError(deadline)) + }, true) + var limit *ratelimit.RateLimitError + if !errors.As(err, &limit) || !limit.RetryAt().Equal(deadline) { + t.Fatalf("provider quota lost its typed retry deadline: %v", err) + } + calls := 0 + err = execute(context.Background(), gate, func(context.Context) (*http.Response, error) { + calls++ + return response(http.StatusOK), nil + }, true) + if err != nil || calls != 1 { + t.Fatalf("provider-specific quota poisoned common controller: %v, calls=%d", err, calls) + } + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 70f1500..1d0843d 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1,6 +1,7 @@ package server import ( + "context" "encoding/json" "errors" "io" @@ -14,6 +15,7 @@ import ( "finance-duck/internal/analytics" "finance-duck/internal/app" + "finance-duck/internal/banking" "finance-duck/internal/domain" ) @@ -43,14 +45,14 @@ func New(a *app.App, assets fs.FS, publicURL string) (http.Handler, error) { s.mux.HandleFunc("POST /api/import", s.importCSV) s.mux.HandleFunc("POST /api/backfill", s.backfill) s.mux.HandleFunc("POST /api/rebuild", func(w http.ResponseWriter, r *http.Request) { v, e := a.Rebuild(r.Context()); respond(w, v, e) }) - s.mux.HandleFunc("POST /api/sync", func(w http.ResponseWriter, r *http.Request) { v, e := a.Sync(r.Context()); respond(w, v, e) }) + s.mux.HandleFunc("POST /api/sync", func(w http.ResponseWriter, r *http.Request) { v, e := a.Sync(s.manualBankContext(r)); respond(w, v, e) }) s.mux.HandleFunc("POST /api/settings", s.settings) s.mux.HandleFunc("POST /api/settings/openrouter", s.openRouterKey) s.mux.HandleFunc("POST /api/settings/enablebanking", s.bankingSettings) s.mux.HandleFunc("POST /api/banking/authorize", s.authorize) s.mux.HandleFunc("GET /api/banking/callback", s.callback) s.mux.HandleFunc("GET /api/balances", func(w http.ResponseWriter, r *http.Request) { - v, e := a.Balances(r.Context(), r.URL.Query().Get("account_id")) + v, e := a.Balances(s.manualBankContext(r), r.URL.Query().Get("account_id")) respond(w, v, e) }) s.mux.HandleFunc("POST /api/reclassify/preview", s.preview) @@ -139,6 +141,40 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { r.Body = http.MaxBytesReader(w, r.Body, 32<<20) s.mux.ServeHTTP(w, r) } + +// manualBankContext is used only at manual account-data boundaries, after the +// Host/origin guards. The configured public origin trusts the local reverse +// proxy to append its observed client IP to X-Forwarded-For. Never trust a +// client-supplied leading hop, or forwarding headers from a non-loopback peer. +func (s *Server) manualBankContext(r *http.Request) context.Context { + peer, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + peer = r.RemoteAddr + } + ip := net.ParseIP(peer) + if s.origin != nil && ip != nil && ip.IsLoopback() { + // A local proxy address is not the end user's address. + ip = nil + forwarded := r.Header.Values("X-Forwarded-For") + if len(forwarded) > 0 { + last := forwarded[len(forwarded)-1] + if index := strings.LastIndexByte(last, ','); index >= 0 { + last = last[index+1:] + } + // A malformed proxy hop is unknown, not the proxy's own PSU IP. + ip = net.ParseIP(strings.TrimSpace(last)) + } + } + address := "" + if ip != nil { + address = ip.String() + } + return banking.WithPSU(r.Context(), banking.PSU{ + IPAddress: address, UserAgent: r.UserAgent(), + Accept: r.Header.Get("Accept"), AcceptCharset: r.Header.Get("Accept-Charset"), + AcceptEncoding: r.Header.Get("Accept-Encoding"), AcceptLanguage: r.Header.Get("Accept-Language"), + }) +} func decode(w http.ResponseWriter, r *http.Request, v any) bool { d := json.NewDecoder(io.LimitReader(r.Body, 1<<20)) d.DisallowUnknownFields() @@ -291,7 +327,7 @@ func (s *Server) backfill(w http.ResponseWriter, r *http.Request) { if !decode(w, r, &b) { return } - v, e := s.app.Backfill(r.Context(), b.Revision, b.AccountID, b.HistoryMonths) + v, e := s.app.Backfill(s.manualBankContext(r), b.Revision, b.AccountID, b.HistoryMonths) respond(w, v, e) } func (s *Server) settings(w http.ResponseWriter, r *http.Request) { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index a495f37..2604688 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -9,11 +9,13 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "testing/fstest" "finance-duck/internal/app" + "finance-duck/internal/banking" ) func TestOriginAndHostGuardProtectNoLoginService(t *testing.T) { @@ -213,3 +215,95 @@ func TestBankingConfigurationProtectsPrivateKeyAndCallbackOrigin(t *testing.T) { configured(check("POST", endpoint, `{"remove":true}`, origin, http.StatusOK), false) configured(check("GET", "/api/state", "", origin, http.StatusOK), false) } + +type psuRoundTripFunc func(*http.Request) (*http.Response, error) + +func (f psuRoundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} + +func TestManualBankContextUsesOnlyTrustedPeerAndBrowserMetadata(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) + origin, err := url.Parse("https://finance.internal") + if err != nil { + t.Fatal(err) + } + for _, tt := range []struct { + name string + public bool + peer string + forwarded []string + wantIP string + userAgent string + }{ + {"direct rejects forwarding", false, "198.51.100.8:1234", []string{"192.0.2.99"}, "198.51.100.8", "real-browser"}, + {"untrusted remote proxy", true, "198.51.100.8:1234", []string{"192.0.2.99"}, "198.51.100.8", "real-browser"}, + {"unconfigured loopback", false, "127.0.0.1:1234", []string{"192.0.2.99"}, "127.0.0.1", "real-browser"}, + {"trusted appended hop", true, "127.0.0.1:1234", []string{"192.0.2.99, 203.0.113.42"}, "203.0.113.42", "real-browser"}, + {"last header appended hop", true, "[::1]:1234", []string{"192.0.2.99", "203.0.113.42"}, "203.0.113.42", "real-browser"}, + {"IPv6 client", true, "[::1]:1234", []string{"192.0.2.99, 2001:db8::42"}, "2001:db8::42", "real-browser"}, + {"missing trusted hop", true, "127.0.0.1:1234", nil, "", "real-browser"}, + {"invalid appended hop not leading spoof", true, "127.0.0.1:1234", []string{"192.0.2.99, invalid"}, "", "real-browser"}, + {"unknown peer and absent user agent", false, "invalid", []string{"192.0.2.99"}, "", ""}, + } { + t.Run(tt.name, func(t *testing.T) { + s := &Server{} + if tt.public { + s.origin = origin + } + r := httptest.NewRequest(http.MethodPost, "https://finance.internal/api/backfill?secret=private", strings.NewReader(`{}`)) + r.RemoteAddr = tt.peer + r.Header["X-Forwarded-For"] = tt.forwarded + r.Header.Set("User-Agent", tt.userAgent) + r.Header.Set("Accept", "application/json") + r.Header.Set("Accept-Charset", "utf-8") + r.Header.Set("Accept-Encoding", "gzip, br") + r.Header.Set("Accept-Language", "de-DE") + for _, name := range []string{"Cookie", "Authorization", "Referer", "Psu-Ip-Address", "Psu-User-Agent", "Psu-Referer", "Psu-Geo-Location", "Psu-Cookie"} { + r.Header.Set(name, "private-spoofed-value") + } + p, err := banking.NewEnableBanking("test-app", keyPEM, "https://finance.internal/api/banking/callback") + if err != nil { + t.Fatal(err) + } + called := false + p.HTTPClient = &http.Client{Transport: psuRoundTripFunc(func(out *http.Request) (*http.Response, error) { + called = true + want := map[string]string{ + "Psu-Ip-Address": tt.wantIP, "Psu-User-Agent": tt.userAgent, + "Psu-Accept": "application/json", "Psu-Accept-Charset": "utf-8", + "Psu-Accept-Encoding": "gzip, br", "Psu-Accept-Language": "de-DE", + } + for name, value := range want { + if got := out.Header.Get(name); got != value { + t.Errorf("%s = %q, want %q", name, got, value) + } + } + for name, values := range out.Header { + if strings.HasPrefix(strings.ToLower(name), "psu-") { + if _, allowed := want[name]; !allowed { + t.Errorf("unexpected PSU header %s", name) + } + } + if strings.Contains(strings.Join(values, ","), "private") { + t.Errorf("secret request metadata leaked in %s", name) + } + } + if out.URL.RawQuery != "" || out.Header.Get("Cookie") != "" || out.Header.Get("Referer") != "" { + t.Error("request URL or secret headers copied to bank") + } + return &http.Response{StatusCode: http.StatusOK, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(`{"balances":[]}`))}, nil + })} + if _, err := p.Balances(s.manualBankContext(r), "uid"); err != nil { + t.Fatal(err) + } + if !called { + t.Fatal("manual retrieval never reached bank transport") + } + }) + } +}