Skip to content

feat(bridge): switch Plaid linking to link_token + public_token exchange#446

Merged
islandbitcoin merged 7 commits into
lnflash:mainfrom
heyolaniran:fix/plaid_linking_flow
Jul 17, 2026
Merged

feat(bridge): switch Plaid linking to link_token + public_token exchange#446
islandbitcoin merged 7 commits into
lnflash:mainfrom
heyolaniran:fix/plaid_linking_flow

Conversation

@heyolaniran

@heyolaniran heyolaniran commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replace the hosted Bridge Plaid linkUrl flow with Bridge’s documented Plaid Link SDK path: bridgeAddExternalAccount now returns a linkToken (+ expiresAt) from POST …/plaid_link_requests.
  • Add bridgeExchangePlaidPublicToken so the app sends linkToken + publicToken to Flash, which exchanges with Bridge server-side (Api-Key never leaves the backend). External accounts still land asynchronously via the existing external_account webhook.
  • Additive cutover: linkUrl remains as a nullable @deprecated field (best-effort hosted URL) so already-shipped mobile clients keep working until the Plaid SDK cutover ships.
  • Bind issued linkTokens to the Flash account in Redis (TTL = link_token_expires_at); exchange rejects unknown / cross-account / already-used tokens (BRIDGE_INVALID_PLAID_TOKEN).
  • Map real exchange failures (400/404/409/422 → BRIDGE_INVALID_PLAID_TOKEN, 401/403 → BRIDGE_PLAID_NOT_AVAILABLE), block the new mutation for API keys (BLOCKED), and update docs / e2e / error-map coverage.
  • Supergraph regen in this PR also picks up catch-up drift from feat(bank-account): update approved bank account — backend [ENG-509] #444 (BankAccountUpdateRequest was missing from the committed supergraph).

API note

  • Confirmed against Bridge Plaid docs: create response uses link_token_expires_at (mapped to GraphQL expiresAt).

Client impact

  • New clients: open Plaid Link with linkToken, then call bridgeExchangePlaidPublicToken before relying on bridgeExternalAccounts.
  • Existing clients: keep selecting linkUrl until the mobile SDK work lands; remove after fleet migrates.

Test plan

  • Unit: exchange-plaid-public-token.spec.ts — validation, error mapping, KYC gate, linkUrl compat, Redis binding (ownership / expiry / consume / fail-closed)
  • Call bridgeAddExternalAccount → receive non-empty linkToken / expiresAt (and optional linkUrl)
  • Empty/ tokens on exchange → BRIDGE_INVALID_PLAID_TOKEN
  • Token issued to account A cannot be exchanged by account B
  • Successful exchange → webhook persists account → bridgeExternalAccounts lists it; second exchange of same token fails
  • API key cannot call bridgeExchangePlaidPublicToken
  • Manual fallback bridgeCreateExternalAccount still works when Plaid is unavailable
  • Confirm mobile ships the linkToken cutover before removing deprecated linkUrl

Replace hosted linkUrl with Bridge plaid_link_requests and a server-side
bridgeExchangePlaidPublicToken mutation so Api-Key never leaves the backend.
…e failures

Review follow-ups on the Plaid link_token PR:

- Keep the deprecated linkUrl field on BridgeExternalAccountLink (now
  nullable, served best-effort from the hosted endpoint) so already-shipped
  mobile clients that select linkUrl don't fail GraphQL validation the
  moment this deploys. Remove after the fleet is on linkToken.
- Map Bridge 4xx rejections on the exchange (expired link_token,
  already-used public_token, mismatched pair) to
  BRIDGE_INVALID_PLAID_TOKEN with Bridge's response detail, and 401/403 to
  BRIDGE_PLAID_NOT_AVAILABLE — consistent with addExternalAccount. 5xx
  still passes through raw for the alerting path.
- Drop the dead idempotencyKey parameter on createPlaidLinkRequest;
  request() already mints a fresh key per call.
- Add unit coverage: token validation/trimming, all exchange error
  mappings, KYC gate, and the linkUrl compat behavior (9 tests).
- Regenerate public SDL + supergraph via yarn write-sdl.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@islandbitcoin islandbitcoin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid direction — server-side exchange with the Api-Key never leaving the backend is the right architecture, and the resolver/scope-map/docs work is consistent with the existing Bridge mutations. I reviewed against the shipped mobile client and pushed a follow-up commit (bba1dce9a) addressing the blocking findings directly. Summary:

Pushed in bba1dce9a

1. The linkUrllinkToken rename would have broken every installed app on deploy day. The shipped mobile client's generated.gql selects linkUrl (opened in BridgeExternalAccountWebView), and GraphQL validation rejects the whole query for an unknown field — so this deploy would have killed bank linking for the entire installed base, while the mobile Plaid SDK work hasn't started yet (no plaid dep in flash-mobile). Made the change additive: linkUrl is back as a nullable @deprecated field, served best-effort from the hosted endpoint, alongside the new linkToken. Remove it after the fleet migrates.

2. Real-world exchange failures mapped to a garbage error. The only handled case was empty/whitespace tokens — which GraphQL non-null mostly prevents anyway. The failures that will actually happen daily (expired link_token, already-exchanged public_token) came back as BRIDGE_API_ERROR: "Bridge API error: 400 Bad Request" with Bridge's actual detail unread in error.response — and mobile already treats BRIDGE_API_ERROR as "account already linked" in the manual flow. Now: 400/404/409/422 → BRIDGE_INVALID_PLAID_TOKEN (surfacing Bridge's detail), 401/403 → BRIDGE_PLAID_NOT_AVAILABLE (consistent with addExternalAccount three definitions up), 5xx passes through raw for alerting.

3. Tests. The new code path had zero coverage (one error-map table line ≠ coverage; every test-plan checkbox unchecked). Added test/flash/unit/services/bridge/exchange-plaid-public-token.spec.ts — 9 tests: validation/trimming, every error mapping, the KYC gate, and the linkUrl compat behavior. Also dropped the dead idempotencyKey param (no caller; request() already mints one per call) and regenerated SDL + supergraph via yarn write-sdl.

Verified locally: tsc clean on both configs, eslint clean, 9/9 unit tests green, SDL matches generator output.

Still open — needs your input

4. The backend never binds a linkToken to the customer it was issued for. createPlaidLinkRequest's response is persisted nowhere, and on exchange customerId is only used as a KYC null-check — then we exchange whatever token the client sent via the global /plaid_exchange_public_token/{linkToken} endpoint. Any level-1 user can exchange any token pair through Flash's Api-Key, with no audit trail. Recommend: persist issued link tokens per customer (small TTL'd collection — they expire anyway) and reject exchanges for tokens we didn't issue to that account; or confirm whether Bridge offers a customer-scoped exchange endpoint. Didn't implement this — it's a persistence-layer design decision that should be yours. Fine as a fast-follow, but please open a ticket before merge.

5. Confirm link_token_expires_at against Bridge's docs — I couldn't verify the response field name offline.

6. Heads-up: the supergraph regen in this PR also carries catch-up drift from #444 (BankAccountUpdateRequest was missing from the committed supergraph). Legitimate, but worth a line in the PR description.

Also: branch is one commit behind main — please rebase or merge main before landing.

@heyolaniran

heyolaniran commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review and for landing bba1dce — agree on 1–3 (additive linkUrl, real exchange error mapping, and the unit coverage).

4 — linkToken → account binding (implemented)
Agreed this needed to close before merge. Bridge’s exchange endpoint is global (/plaid_exchange_public_token/{linkToken}), so Flash now owns the customer binding:

  • On bridgeAddExternalAccount: after createPlaidLinkRequest, we persist { accountId, bridgeCustomerId, expiresAt } in Redis under plaid:link:{linkToken} with TTL from link_token_expires_at (fail closed if Redis write fails — no unbound token returned).
  • On bridgeExchangePlaidPublicToken: consume the binding, reject if missing / expired / wrong accountId (BRIDGE_INVALID_PLAID_TOKEN), then call Bridge. Consume-before-exchange so concurrent reuse can’t slip through.

Covered in exchange-plaid-public-token.spec.ts (ownership, expiry, consume, fail-closed persist). Docs updated in FLOWS.md / API.md.

5 — link_token_expires_at
Confirmed against Bridge’s Plaid Link docs — response field is link_token_expires_at (we map it to GraphQL expiresAt). No change needed.

6 — supergraph drift from #444
Called out in the PR description: regen also picks up BankAccountUpdateRequest from #444.

Will rebase onto main before merge.

Persist issued link tokens in Redis with TTL from link_token_expires_at,
and reject exchange for unknown, expired, or cross-account tokens so Flash
does not act as an unbound Bridge Api-Key proxy.
@heyolaniran
heyolaniran force-pushed the fix/plaid_linking_flow branch from 251c258 to 2bc37af Compare July 15, 2026 17:27
heyolaniran and others added 2 commits July 15, 2026 17:34
consumeForAccount used a non-atomic get-then-clear: two concurrent exchanges
of the same link token could both read the binding before either cleared it,
so both proceeded — the "concurrent reuse can't slip through" comment was
false. clear() also discards Redis's DEL count, so it cannot tell the winner
of the race from the loser.

Add consumeCacheKey() to the redis cache service: one DEL, returns true only
for the caller that actually removed the key (Redis serializes DELs, so among
concurrent callers exactly one sees a removed-count of 1). Gate single-use on
it, after the ownership check on the non-destructive get — so a stranger still
cannot burn the owner's token.

Tests: replace the stateless single-use stub with real coverage — a second
exchange of a consumed token is rejected, and two concurrent exchanges yield
exactly one winner with Bridge called once. Cross-account now also asserts the
atomic consume is never reached.

Verified: tsc --noEmit clean; exchange spec 15/15 (incl. 2 new concurrency
tests), bridge index 64/64, bridge-error-map 41/41.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@islandbitcoin

Copy link
Copy Markdown
Contributor

Pushed a commit to this branch (036b9eeac) — heads up since it touches your Plaid link-token store.

What it changes: consumeForAccount consumed the token with a get followed by a separate clear, which is a TOCTOU race: two concurrent bridgeExchangePlaidPublicToken calls with the same token can both read the binding before either clears it, so both proceed. clear() also cannot distinguish the winner from the loser because it discards Redis's DEL count.

The commit adds consumeCacheKey() to the redis cache service — one DEL, returning true only for the caller that actually removed the key (Redis serializes DELs, so exactly one concurrent caller sees a removed-count of 1) — and gates single-use on it. Ownership is still checked on the non-destructive get first, so the cross-account no-burn behavior you added is preserved: a stranger still cannot delete the owner's token.

It also swaps the single-use unit test off the stateless cache stub for real coverage — a second exchange of a consumed token is rejected, and two concurrent exchanges resolve to exactly one winner with Bridge called once.

Verified locally: tsc --noEmit clean; exchange spec 15/15 (incl. the 2 new concurrency tests), bridge index 64/64, bridge-error-map 41/41.

One item left as your call (not changed here): the consume happens before the Bridge call, so a transient Bridge 5xx/timeout burns the link token and the user must restart linking. Reasonable for replay-safety, but if you'd prefer to keep the binding on transient faults so a retry can succeed, that would be a good follow-up.

Collapse the multi-line filter arrow flagged by prettier/prettier in
Check Code CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@islandbitcoin
islandbitcoin merged commit bb0ca8d into lnflash:main Jul 17, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants