From 335cf09d762aecb9309fc4d852aed992acbf0461 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Fri, 4 Sep 2026 17:45:14 -0300 Subject: [PATCH 1/2] fix(salesforce): preserve the caller's OAuth state alongside the PKCE verifier getAuthorizationUri() overwrote the `state` query parameter with the encrypted PKCE verifier and never read `this.state`, the value Frigg passes through from the adopter. In OAuth 2.0 `state` belongs to the client, and adopters rely on it: Clockwork serves every firm from one bounce endpoint and resolves the firm hostname out of `state`, so the Salesforce callback landed on "unknown firm" for every connect attempt. HubSpot's module appends `this.state` and works; this module was the outlier. The verifier still has to survive the round trip, so it is now appended behind `~` rather than replacing the caller's value. `~` is unreserved in RFC 3986 and absent from base64url, so it cannot appear inside the verifier. restoreVerifierFromState() reads the trailing segment and falls back to the whole string, which keeps authorizations that were in flight before this change working and leaves callers who pass no state exactly where they were. Co-Authored-By: Claude Fable 5.1 --- packages/v1-ready/salesforce/api.js | 46 ++++++++++++---- packages/v1-ready/salesforce/test/api.test.js | 52 +++++++++++++++++++ 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/packages/v1-ready/salesforce/api.js b/packages/v1-ready/salesforce/api.js index 13c7317..0368048 100644 --- a/packages/v1-ready/salesforce/api.js +++ b/packages/v1-ready/salesforce/api.js @@ -3,6 +3,12 @@ const jsforce = require('jsforce'); const crypto = require('crypto'); class Api extends OAuth2Requester { + // Separates the caller's state from the appended PKCE verifier. `~` is + // unreserved per RFC 3986 (so it round-trips through the provider redirect + // unescaped) and is not in the base64url alphabet, so it cannot appear + // inside the encrypted verifier itself. + static STATE_VERIFIER_DELIMITER = '~'; + constructor(params) { super(params); this.jsforce = jsforce; @@ -53,16 +59,38 @@ class Api extends OAuth2Requester { this.conn.oauth2 = this.oauth2; const url = this.oauth2.getAuthorizationUrl({ scope: this.scope }); const verifier = this.oauth2.codeVerifier; - if (verifier) { - const urlObj = new URL(url); - urlObj.searchParams.set('state', this._encryptVerifier(verifier)); - return urlObj.toString(); + const callerState = this.state || null; + if (!verifier) { + return callerState ? this._withState(url, callerState) : url; } - return url; - } - - restoreVerifierFromState(encryptedState) { - const verifier = this._decryptVerifier(encryptedState); + // `state` is the caller's: Frigg hands the adopter's value to the + // constructor and adopters route the OAuth callback with it. We need the + // PKCE verifier back on the return leg too, so APPEND it behind a + // delimiter rather than replacing what the caller asked for. Replacing + // it left adopters unable to route their own callback. + const encoded = this._encryptVerifier(verifier); + return this._withState( + url, + callerState + ? `${callerState}${Api.STATE_VERIFIER_DELIMITER}${encoded}` + : encoded + ); + } + + _withState(url, state) { + const urlObj = new URL(url); + urlObj.searchParams.set('state', state); + return urlObj.toString(); + } + + restoreVerifierFromState(state) { + // The encrypted verifier is the trailing segment. A state with no + // delimiter is either an authorization that started before this change + // or one where the caller supplied no state of their own — both carry + // the verifier alone. + const at = String(state).lastIndexOf(Api.STATE_VERIFIER_DELIMITER); + const encrypted = at === -1 ? state : String(state).slice(at + 1); + const verifier = this._decryptVerifier(encrypted); this.oauth2.codeVerifier = verifier; this.conn.oauth2.codeVerifier = verifier; } diff --git a/packages/v1-ready/salesforce/test/api.test.js b/packages/v1-ready/salesforce/test/api.test.js index 322c9e7..050f227 100644 --- a/packages/v1-ready/salesforce/test/api.test.js +++ b/packages/v1-ready/salesforce/test/api.test.js @@ -14,6 +14,9 @@ jest.mock('jsforce', () => { return { OAuth2: jest.fn().mockImplementation((params) => ({ getAuthorizationUrl: mockGetAuthorizationUrl, + // Real jsforce populates this whenever useVerifier is set, which is + // what getAuthorizationUri does for the auth flow. + codeVerifier: params?.useVerifier ? 'test-code-verifier' : undefined, _params: params, })), Connection: jest.fn().mockImplementation(() => mockConnection), @@ -119,3 +122,52 @@ describe('Salesforce Api', () => { }); }); }); + +// `state` belongs to the caller: Frigg passes the adopter's value into the Api +// constructor and adopters route the OAuth callback with it — Clockwork resolves +// the firm from a "." state at its bounce endpoint. This +// module also needs its PKCE verifier back on the return leg, so the two have to +// coexist. Replacing the caller's state (the old behaviour) made the callback +// unroutable. +describe('getAuthorizationUri state handling', () => { + const stateOf = (url) => + new URL(url).searchParams.get('state'); + + it('preserves the caller state and appends the encrypted verifier', () => { + const api = new Api({ ...baseParams, state: 'testfirma.NONCE123' }); + const state = stateOf(api.getAuthorizationUri()); + expect(state.startsWith('testfirma.NONCE123')).toBe(true); + expect(state).not.toBe('testfirma.NONCE123'); + }); + + it('keeps the caller state parseable by a first-dot split', () => { + const api = new Api({ ...baseParams, state: 'testfirma.NONCE123' }); + const state = stateOf(api.getAuthorizationUri()); + // How Clockwork's bounce resolves the firm. + expect(state.split('.', 1)[0]).toBe('testfirma'); + }); + + it('round-trips the verifier out of the composed state', () => { + const api = new Api({ ...baseParams, state: 'testfirma.NONCE123' }); + const state = stateOf(api.getAuthorizationUri()); + api.restoreVerifierFromState(state); + expect(api.oauth2.codeVerifier).toBe('test-code-verifier'); + }); + + it('emits the bare encrypted verifier when the caller supplied no state', () => { + const api = new Api(baseParams); + const state = stateOf(api.getAuthorizationUri()); + expect(state).toBeTruthy(); + api.restoreVerifierFromState(state); + expect(api.oauth2.codeVerifier).toBe('test-code-verifier'); + }); + + it('still restores from a legacy state that carries only the verifier', () => { + // An authorization already in flight when this change ships comes back + // in the old format. + const api = new Api(baseParams); + const legacy = api._encryptVerifier('test-code-verifier'); + api.restoreVerifierFromState(legacy); + expect(api.oauth2.codeVerifier).toBe('test-code-verifier'); + }); +}); From ffcd0f5b7b4f21230bacebe34cb762d59d770cd3 Mon Sep 17 00:00:00 2001 From: d-klotz Date: Fri, 4 Sep 2026 18:39:48 -0300 Subject: [PATCH 2/2] refactor(salesforce): trim comments on the state fix Cut the comments added by the state fix down to the repo's style; the rationale stays in the PR description. Behaviour is unchanged. Co-Authored-By: Claude Fable 5.1 --- packages/v1-ready/salesforce/api.js | 14 +------------- packages/v1-ready/salesforce/test/api.test.js | 11 ----------- 2 files changed, 1 insertion(+), 24 deletions(-) diff --git a/packages/v1-ready/salesforce/api.js b/packages/v1-ready/salesforce/api.js index 0368048..0ba369f 100644 --- a/packages/v1-ready/salesforce/api.js +++ b/packages/v1-ready/salesforce/api.js @@ -3,10 +3,7 @@ const jsforce = require('jsforce'); const crypto = require('crypto'); class Api extends OAuth2Requester { - // Separates the caller's state from the appended PKCE verifier. `~` is - // unreserved per RFC 3986 (so it round-trips through the provider redirect - // unescaped) and is not in the base64url alphabet, so it cannot appear - // inside the encrypted verifier itself. + // URL-unreserved and outside the base64url alphabet. static STATE_VERIFIER_DELIMITER = '~'; constructor(params) { @@ -63,11 +60,6 @@ class Api extends OAuth2Requester { if (!verifier) { return callerState ? this._withState(url, callerState) : url; } - // `state` is the caller's: Frigg hands the adopter's value to the - // constructor and adopters route the OAuth callback with it. We need the - // PKCE verifier back on the return leg too, so APPEND it behind a - // delimiter rather than replacing what the caller asked for. Replacing - // it left adopters unable to route their own callback. const encoded = this._encryptVerifier(verifier); return this._withState( url, @@ -84,10 +76,6 @@ class Api extends OAuth2Requester { } restoreVerifierFromState(state) { - // The encrypted verifier is the trailing segment. A state with no - // delimiter is either an authorization that started before this change - // or one where the caller supplied no state of their own — both carry - // the verifier alone. const at = String(state).lastIndexOf(Api.STATE_VERIFIER_DELIMITER); const encrypted = at === -1 ? state : String(state).slice(at + 1); const verifier = this._decryptVerifier(encrypted); diff --git a/packages/v1-ready/salesforce/test/api.test.js b/packages/v1-ready/salesforce/test/api.test.js index 050f227..ddf75da 100644 --- a/packages/v1-ready/salesforce/test/api.test.js +++ b/packages/v1-ready/salesforce/test/api.test.js @@ -14,8 +14,6 @@ jest.mock('jsforce', () => { return { OAuth2: jest.fn().mockImplementation((params) => ({ getAuthorizationUrl: mockGetAuthorizationUrl, - // Real jsforce populates this whenever useVerifier is set, which is - // what getAuthorizationUri does for the auth flow. codeVerifier: params?.useVerifier ? 'test-code-verifier' : undefined, _params: params, })), @@ -123,12 +121,6 @@ describe('Salesforce Api', () => { }); }); -// `state` belongs to the caller: Frigg passes the adopter's value into the Api -// constructor and adopters route the OAuth callback with it — Clockwork resolves -// the firm from a "." state at its bounce endpoint. This -// module also needs its PKCE verifier back on the return leg, so the two have to -// coexist. Replacing the caller's state (the old behaviour) made the callback -// unroutable. describe('getAuthorizationUri state handling', () => { const stateOf = (url) => new URL(url).searchParams.get('state'); @@ -143,7 +135,6 @@ describe('getAuthorizationUri state handling', () => { it('keeps the caller state parseable by a first-dot split', () => { const api = new Api({ ...baseParams, state: 'testfirma.NONCE123' }); const state = stateOf(api.getAuthorizationUri()); - // How Clockwork's bounce resolves the firm. expect(state.split('.', 1)[0]).toBe('testfirma'); }); @@ -163,8 +154,6 @@ describe('getAuthorizationUri state handling', () => { }); it('still restores from a legacy state that carries only the verifier', () => { - // An authorization already in flight when this change ships comes back - // in the old format. const api = new Api(baseParams); const legacy = api._encryptVerifier('test-code-verifier'); api.restoreVerifierFromState(legacy);