make SimpleFIN token 'forbidden' check more flexible (#8339)

* make SimpleFIN token 'forbidden' check more flexible * clear accessKey when token is updated * https -> fetch * [AI] Add tests for SimpleFIN token claim fix Cover the flexible 'Forbidden' matching, full claim-response handling, and access-key invalidation when the setup token changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * note * Update upcoming-release-notes/fix-simplefin-token-claim.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * coderabbit feedback * [AI] Test SimpleFIN blank access key handling Cover rejecting a blank claim response (not persisted) and re-claiming when an empty access key is cached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2026-07-21 00:27:43 -05:00 · 2026-06-29 15:11:57 +00:00
parent 686c203987
commit 676eaf2a17
5 changed files with 211 additions and 25 deletions
@@ -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' });
 });

@@ -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) {
@@ -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,
+      );
+    });
+  });
+});
@@ -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('/')
@@ -0,0 +1,6 @@
+---
+category: Bugfixes
+authors: [matt-fidd]
+---
+
+Fix some SimpleFIN setup tokens failing to claim and link accounts