diff --git a/packages/sync-server/src/app-secrets.js b/packages/sync-server/src/app-secrets.js index dc9b45551c..0aa370c534 100644 --- a/packages/sync-server/src/app-secrets.js +++ b/packages/sync-server/src/app-secrets.js @@ -42,8 +42,16 @@ app.post('/', async (req, res) => { return; } + const simplefinTokenChanged = + name === SecretName.simplefin_token && + value !== secretsService.get(SecretName.simplefin_token); + secretsService.set(name, value); + if (simplefinTokenChanged) { + secretsService.set(SecretName.simplefin_accessKey, null); + } + res.status(200).send({ status: 'ok' }); }); diff --git a/packages/sync-server/src/app-simplefin/app-simplefin.js b/packages/sync-server/src/app-simplefin/app-simplefin.js index d100fdda17..b99d9b5e94 100644 --- a/packages/sync-server/src/app-simplefin/app-simplefin.js +++ b/packages/sync-server/src/app-simplefin/app-simplefin.js @@ -1,5 +1,3 @@ -import https from 'https'; - import express from 'express'; import { handleError } from '#app-gocardless/util/handle-error'; @@ -20,7 +18,7 @@ app.post( '/status', handleError(async (req, res) => { const token = secretsService.get(SecretName.simplefin_token); - const configured = token != null && token !== 'Forbidden'; + const configured = token != null && !isForbidden(token); res.send({ status: 'ok', @@ -37,16 +35,16 @@ app.post( let accessKey = secretsService.get(SecretName.simplefin_accessKey); try { - if (accessKey == null || accessKey === 'Forbidden') { + if (isInvalidAccessKey(accessKey)) { const token = secretsService.get(SecretName.simplefin_token); - if (token == null || token === 'Forbidden') { + if (token == null || isForbidden(token)) { throw new Error('No token'); } else { accessKey = await getAccessKey(token); - secretsService.set(SecretName.simplefin_accessKey, accessKey); - if (accessKey == null || accessKey === 'Forbidden') { + if (isInvalidAccessKey(accessKey)) { throw new Error('No access key'); } + secretsService.set(SecretName.simplefin_accessKey, accessKey); } } } catch { @@ -77,7 +75,7 @@ app.post( const accessKey = secretsService.get(SecretName.simplefin_accessKey); - if (accessKey == null || accessKey === 'Forbidden') { + if (isInvalidAccessKey(accessKey)) { invalidToken(res); return; } @@ -104,7 +102,7 @@ app.post( new Date(earliestStartDate), ); } catch (e) { - if (e.message === 'Forbidden') { + if (isForbidden(e.message)) { invalidToken(res); } else { serverDown(e, res); @@ -319,22 +317,22 @@ async function getAccessKey(base64Token) { // private addresses are allowed here; cloud metadata and other always-blocked // ranges are still rejected. await assertUrlAllowed(token, { allowPrivateNetwork: true }); - const options = { - method: 'POST', - port: 443, - headers: { 'Content-Length': 0 }, - }; - return new Promise((resolve, reject) => { - const req = https.request(new URL(token), options, res => { - res.on('data', d => { - resolve(d.toString()); - }); - }); - req.on('error', e => { - reject(e); - }); - req.end(); - }); + + // don't auto-follow redirects for SSRF safety + const response = await fetch(token, { method: 'POST', redirect: 'manual' }); + return (await response.text()).trim(); +} + +function isForbidden(value) { + return typeof value === 'string' && value.startsWith('Forbidden'); +} + +function isInvalidAccessKey(accessKey) { + return ( + typeof accessKey !== 'string' || + accessKey.trim() === '' || + isForbidden(accessKey) + ); } async function getTransactions(accessKey, accounts, startDate, endDate) { diff --git a/packages/sync-server/src/app-simplefin/app-simplefin.test.js b/packages/sync-server/src/app-simplefin/app-simplefin.test.js new file mode 100644 index 0000000000..d61ea35f48 --- /dev/null +++ b/packages/sync-server/src/app-simplefin/app-simplefin.test.js @@ -0,0 +1,145 @@ +import request from 'supertest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { SecretName, secretsService } from '#services/secrets-service'; + +import { handlers as app } from './app-simplefin'; + +vi.mock('#util/ssrf', () => ({ + assertUrlAllowed: vi.fn().mockResolvedValue(undefined), +})); + +const VALID_ACCESS_KEY = 'https://user:pass@bridge.example.com/simplefin'; +const SETUP_TOKEN = Buffer.from( + 'https://bridge.example.com/claim/abc', +).toString('base64'); + +const okResponse = body => ({ + status: 200, + headers: { get: () => null }, + text: () => Promise.resolve(body), +}); + +// The claim is a POST to the bridge; listing accounts is a GET. Route the mock +// by method so a test can stub one or both. +function mockFetch({ claim, accounts }) { + global.fetch = vi + .fn() + .mockImplementation((url, options) => + Promise.resolve(options?.method === 'POST' ? claim : accounts), + ); +} + +const post = path => + request(app).post(path).set('x-actual-token', 'valid-token'); + +describe('app-simplefin', () => { + beforeEach(() => { + secretsService.set(SecretName.simplefin_token, null); + secretsService.set(SecretName.simplefin_accessKey, null); + vi.spyOn(console, 'log').mockImplementation(vi.fn()); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('/status', () => { + it('reports configured when a real token is stored', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + + const res = await post('/status'); + + expect(res.body.data.configured).toBe(true); + }); + + it('reports not configured when the stored token is a Forbidden message', async () => { + secretsService.set( + SecretName.simplefin_token, + 'Forbidden (was it already claimed?)', + ); + + const res = await post('/status'); + + expect(res.body.data.configured).toBe(false); + }); + }); + + describe('/accounts', () => { + it('claims the token, trims the access key and returns accounts', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + mockFetch({ + claim: okResponse(`${VALID_ACCESS_KEY}\n`), + accounts: okResponse( + JSON.stringify({ accounts: [{ id: 'account-1' }] }), + ), + }); + + const res = await post('/accounts'); + + expect(res.body.data.accounts).toEqual([{ id: 'account-1' }]); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBe( + VALID_ACCESS_KEY, + ); + }); + + it('treats a "Forbidden (was it already claimed?)" claim response as invalid and does not persist it', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + mockFetch({ claim: okResponse('Forbidden (was it already claimed?)') }); + + const res = await post('/accounts'); + + expect(res.body.data.error_code).toBe('INVALID_ACCESS_TOKEN'); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBeNull(); + }); + + it('treats a blank claim response as invalid and does not persist it', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + mockFetch({ claim: okResponse(' \n') }); + + const res = await post('/accounts'); + + expect(res.body.data.error_code).toBe('INVALID_ACCESS_TOKEN'); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBeNull(); + }); + + it('re-claims when a stale Forbidden access key is cached', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + secretsService.set( + SecretName.simplefin_accessKey, + 'Forbidden (was it already claimed?)', + ); + mockFetch({ + claim: okResponse(VALID_ACCESS_KEY), + accounts: okResponse( + JSON.stringify({ accounts: [{ id: 'account-1' }] }), + ), + }); + + const res = await post('/accounts'); + + expect(res.body.data.accounts).toEqual([{ id: 'account-1' }]); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBe( + VALID_ACCESS_KEY, + ); + }); + + it('re-claims when a stale empty access key is cached', async () => { + secretsService.set(SecretName.simplefin_token, SETUP_TOKEN); + secretsService.set(SecretName.simplefin_accessKey, ''); + mockFetch({ + claim: okResponse(VALID_ACCESS_KEY), + accounts: okResponse( + JSON.stringify({ accounts: [{ id: 'account-1' }] }), + ), + }); + + const res = await post('/accounts'); + + expect(res.body.data.accounts).toEqual([{ id: 'account-1' }]); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBe( + VALID_ACCESS_KEY, + ); + }); + }); +}); diff --git a/packages/sync-server/src/secrets.test.js b/packages/sync-server/src/secrets.test.js index 4197381f90..1688ed60df 100644 --- a/packages/sync-server/src/secrets.test.js +++ b/packages/sync-server/src/secrets.test.js @@ -101,6 +101,35 @@ describe('secretsService', () => { }); }); + it('clears the cached SimpleFIN access key when the token changes', async () => { + secretsService.set(SecretName.simplefin_token, 'old-token'); + secretsService.set(SecretName.simplefin_accessKey, 'cached-access-key'); + + const res = await request(app) + .post(`/`) + .set('x-actual-token', 'valid-token') + .send({ name: SecretName.simplefin_token, value: 'new-token' }); + + expect(res.statusCode).toEqual(200); + expect(secretsService.get(SecretName.simplefin_token)).toBe('new-token'); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBeNull(); + }); + + it('keeps the SimpleFIN access key when the token is unchanged', async () => { + secretsService.set(SecretName.simplefin_token, 'same-token'); + secretsService.set(SecretName.simplefin_accessKey, 'cached-access-key'); + + const res = await request(app) + .post(`/`) + .set('x-actual-token', 'valid-token') + .send({ name: SecretName.simplefin_token, value: 'same-token' }); + + expect(res.statusCode).toEqual(200); + expect(secretsService.get(SecretName.simplefin_accessKey)).toBe( + 'cached-access-key', + ); + }); + it('POST returns 400 for unknown secret names', async () => { const res = await request(app) .post('/') diff --git a/upcoming-release-notes/fix-simplefin-token-claim.md b/upcoming-release-notes/fix-simplefin-token-claim.md new file mode 100644 index 0000000000..8d9c0159c5 --- /dev/null +++ b/upcoming-release-notes/fix-simplefin-token-claim.md @@ -0,0 +1,6 @@ +--- +category: Bugfixes +authors: [matt-fidd] +--- + +Fix some SimpleFIN setup tokens failing to claim and link accounts