Repository navigation
fix(salesforce): persist rotated refresh tokens and route jsforce refreshes through core - #101
Conversation
…reshes through core
With refresh-token rotation enabled on the connected app, Salesforce returns a
new refresh_token on every refresh and invalidates the old one. jsforce's default
oauthRefreshFn never updates conn.refreshToken (connection.js _establish keeps
the old value), and the module persisted this.conn.refreshToken — the consumed
token — over the good one. Presenting a rotated-out token later trips
Salesforce's reuse detection, which revokes the current token and every access
token in the family. Observed 2026-09-16: four worker Lambdas overwrote the
daemon's correct write, then 765 refreshes failed with "expired access/refresh
token" while the credential stayed authIsValid=true.
Hand jsforce a custom refreshFn that delegates to core's _refreshAuthOnce().
Core (frigg#636, @friggframework/core 2.0.0-next.110) supplies
adopt-before-refresh, definitive-vs-transport classification,
adopt-after-invalid_grant with backoff, INVALID_AUTH, telemetry and
single-flight. The module adds only:
- _buildConnection(): the one place that constructs the jsforce Connection,
with the refreshFn. resetToSandbox() previously built its connection without
any refresh listener, so sandbox connections never persisted a refresh.
- _adoptNewerCredential(): mirrors adopted tokens onto conn.
- refreshAccessToken(): accepts both core's { refresh_token } and a token
response the caller already obtained (the CDC daemon's call shape). Persists
res.refresh_token. Leaves instanceUrl alone on purpose: getCredentialDetails
keys the credential upsert on it, and moving it would fork the Credential row.
- _normalizeTokenError(): jsforce reports the OAuth code in err.name with no
HTTP status; core classifies on statusCode first. ERROR_HTTP_5xx keeps that
status (transport); invalid_grant, invalid_client, invalid_client_id,
invalid_app_access, inactive_user and inactive_org become 400 (definitive).
- _refreshRejected memo: after a definitive rejection, later 401s only re-read
the store. Without it every 401 on a warm instance repeats the full cycle
(4 reads, 1 POST, 3 s backoff, 3 writes) and re-fires CREDENTIAL_INVALIDATED.
The dead token is never replayed.
The refresh event handler is deleted: a custom refreshFn still emits 'refresh',
and the old handler was fire-and-forget and logged the live access token.
Bump @friggframework/core from the exact 2.0.0-next.79 pin to ^2.0.0-next.110,
matching the other v1-ready modules. Every primitive above is absent in next.79.
Test fixture: baseParams now carries scope, as definition.env does at runtime.
The two getAuthorizationUri scope assertions were already failing on next.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97581e860a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!refreshed) { | ||
| this._refreshRejected = true; | ||
| return callback(new Error('Salesforce rejected the refresh token')); |
There was a problem hiding this comment.
After a definitive rejection sets _refreshRejected, the same API instance can later receive valid credentials through getAccessToken() during reauthorization or through the public persist-only refreshAccessToken(response) path, but neither clears this flag. When that new access token eventually expires, this branch only reloads the credential; because the stored refresh token now equals the in-memory token, adoption returns false and the valid refresh token is never submitted to Salesforce, leaving the reauthorized connection unable to refresh until the API object is recreated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 46601ad: every successful setTokens() now clears _refreshRejected, so a re-authorization via getAccessToken() or the persist-only refreshAccessToken(response) path submits the fresh refresh token on the next 401. Covered by two tests (one per entry point).
| if (this._refreshRejected) { | ||
| if (!(await this._adoptNewerCredential())) { | ||
| return callback( |
There was a problem hiding this comment.
When _refreshRejected is set and the credential reload throws—for example, during a transient database failure—the exception escapes this async callback adapter before callback is invoked. jsforce's callback-based refresh remains unsettled, so the originating API request hangs until an outer timeout and may also produce an unhandled rejection; wrap this reload path in the same error-to-callback handling used for _refreshAuthOnce().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 46601ad: both branches of _jsforceRefreshFn now share one try/catch, so a throwing credential reload reaches jsforce's callback instead of leaving the refresh promise unsettled. Test drives a rejecting reload while the memo is set and asserts the callback receives the error.
…rsist Review follow-ups on the rotated-refresh-token change. - refreshAccessToken() with neither access_token nor refresh_token used to POST an empty refresh_token; Salesforce answers invalid_grant, which the normaliser classifies as definitive and core turns into INVALID_AUTH. A caller passing a partial response could brick a healthy credential. It now throws before the request. - When the token POST succeeds but the credential write fails, the rotated pair stays in memory on purpose (replaying the consumed token revokes the family), but the stored token is now consumed and nothing said so. Log it at error level before rethrowing. - Initialise _refreshRejected in the constructor. - Tests: a transport failure must not arm the rejection memo (the 503 test now refreshes a second time and expects a token POST); the tokenless call throws without a POST or an INVALID_AUTH; the persist-failure ordering and its log. Dropped the "does not subscribe to the jsforce refresh event" test: it pinned a mechanism, not a behaviour. One TOKEN_UPDATE per refresh and no tokens in the console already cover what it stood for. Left as-is, deliberately: invalid_client_id / inactive_* are classified definitive (extends core's existing invalid_client policy); a missing client_id now surfaces on the first 401 rather than at construction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed a second commit after an Opus review of the diff (97581e8 → HEAD):
|
Two Codex findings on the memo introduced in 97581e8. - After a definitive rejection armed _refreshRejected, a re-authorization on the same Api instance — getAccessToken(), or the persist-only refreshAccessToken(response) shape — installed fresh tokens but left the memo set. On the next 401 the instance only re-read the store; the stored refresh token equalled the in-memory one, adoption returned false, and the perfectly valid token was never submitted. The instance stayed read-only until recreated. Every successful setTokens() now clears the memo. - The memo branch called _adoptNewerCredential() outside the try/catch that guards _refreshAuthOnce(). A throwing reload escaped the async refreshFn adapter, leaving jsforce's refresh promise unsettled and the originating request hanging. Both branches now share one try/catch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Third commit (46601ad) addresses Codex's two inline findings — the rejection memo is cleared by any successful |
|
🚀 PR was released in |
2 participants
Problem
With refresh-token rotation enabled on the connected app, Salesforce returns a new
refresh_tokenon every refresh and invalidates the old one. jsforce's defaultoauthRefreshFnnever updatesconn.refreshToken(connection.js_establishkeeps the old value), soapi.jspersistedthis.conn.refreshToken— the consumed token — over the good one on every refresh.Presenting a rotated-out token later trips Salesforce's reuse detection, which revokes the current refresh token and every access token in the family.
Observed in production on an integration syncing into a Salesforce org with rotation enabled: four concurrent Lambda workers overwrote a long-running process's correct write, then 765 refreshes failed with
Unable to refresh session due to: expired access/refresh token. The credential stayedauthIsValid: trueand the integration stayedENABLED— no signal, no recovery.Fix
Hand jsforce a custom
refreshFnthat delegates to core's_refreshAuthOnce(). Core (frigg#636, now in@friggframework/core@2.0.0-next.110) supplies adopt-before-refresh, definitive-vs-transport classification, adopt-after-invalid_grantwith backoff,INVALID_AUTH, telemetry and single-flight. The module adds only:_buildConnection()— one place that constructs the jsforceConnection, with therefreshFn.resetToSandbox()previously built its connection without any refresh listener, so sandbox connections never persisted a refresh at all._adoptNewerCredential()— mirrors adopted tokens ontoconn.refreshAccessToken()— accepts both core's{ refresh_token }and a token response the caller already obtained (the call shape of a long-running consumer that refreshes outside jsforce). Persistsres.refresh_token. LeavesinstanceUrlalone on purpose:getCredentialDetailskeys the credential upsert on it, and moving it would create a second Credential row holding the rotated token._normalizeTokenError()— jsforce reports the OAuth code inerr.namewith no HTTP status; core classifies onstatusCodefirst.ERROR_HTTP_5xx→ that status (transport).invalid_grant,invalid_client,invalid_client_id,invalid_app_access,inactive_user,inactive_org→ 400 (definitive)._refreshRejectedmemo — after a definitive rejection, later 401s only re-read the store. Without it every 401 on a warm instance repeats the full cycle (4 DB reads, 1 token POST, up to 3 s backoff, 3 writes) and re-firesCREDENTIAL_INVALIDATED. The dead token is never replayed, and the instance recovers by itself the moment another process writes a fresh credential.The
refreshevent handler is deleted: a customrefreshFnstill emits'refresh', and the old handler was fire-and-forget and logged the live access token to stdout.Dependency
@friggframework/core: exact2.0.0-next.79→^2.0.0-next.110, matching the other v1-ready modules. Every primitive above is absent in next.79.Semver note:
^2.0.0-next.110does not match the2.0.0--canary.636.*builds (the leading-canaryidentifier sorts belownext). A consumer still on a canary gets a nested next.110 copy locally; the deploy bundle prunes nestednode_modulesand the files are byte-identical.Tests
npm run test:unit— 45 passing (13 existing + 20 new). New coverage, one behaviour perit:invalid_grantwhen another worker wrote a newer tokeninvalid_grantwith nothing newer → oneINVALID_AUTH;invalid_client_id→ definitive withstatusCode: 400;ERROR_HTTP_503→ transport, no invalidationresetToSandboxkeeps the refresh hookrefreshAuth()refreshes through jsforce;refreshAccessToken(res)persist-only path;instanceUrlnever moves on refreshrefreshAccessToken({})throws before any POST; a transport failure leaves the rejection memo disarmed; when the credential write fails the rotated pair stays in memory and an error is loggedgetAccessTokenandrefreshAccessToken(res)); a throwing credential reload while the memo is set reaches jsforce's callbackFixture:
baseParamsnow carriesscope, asdefinition.envdoes at runtime. The twogetAuthorizationUriscope assertions were already failing onnextbefore this branch.Prettier: only the file's pre-existing long lines remain non-compliant; nothing outside this change was rewrapped.
After review
Second commit addresses an Opus review of the diff:
refreshAccessToken()with neither token now throws before the request — previously it POSTed an emptyrefresh_token, gotinvalid_grant, and would have marked a healthy credential invalid.rotated refresh token was not persisted; the stored token is now consumed) before rethrowing. The in-memory ordering was already right; it was silent._refreshRejectedinitialised in the constructor.refresh-event subscription test as implementation-coupled; its intent is covered by the one-TOKEN_UPDATE-per-refresh and no-tokens-in-console tests.Kept deliberately:
invalid_client_id/inactive_*are treated as definitive (extends core'sinvalid_clientpolicy); a missingclient_idnow surfaces at the first 401 instead of at construction.Third commit addresses Codex's two inline findings on
97581e8:getAccessToken, or the persist-onlyrefreshAccessToken(res)) left the memo armed, so the fresh refresh token was never submitted and the instance stayed read-only until recreated. Every successfulsetTokens()now clears the memo._refreshAuthOnce's try/catch, so a throwing credential reload reaches jsforce's callback instead of leaving the refresh promise unsettled.Rollout
Credentials already holding a consumed token (there is at least one) will, on first refresh after deploy, go
invalid_grant→ backoff re-read →INVALID_AUTH→ integrationERROR. That is the intended change from today's silent forever-failure. Re-authorizing through Frigg'sPOST /api/authorizerestoresENABLEDviaProcessAuthorizationCallback.restoreIntegrationsForEntity.Follow-ups (not in this PR)
@friggframework/coreto^2.0.0-next.110.conn.oauth2.refreshTokendirectly) can switch toapi.refreshAuth()— one refresh implementation.Module.onTokenUpdate; extend_isDefinitiveAuthRejection's regex to the codes above.🤖 Generated with Claude Code
Version
Published prerelease version:
@friggframework/api-module-salesforce@2.0.0-next.9Changelog
🐛 Bug Fix
@friggframework/api-module-salesforce@friggframework/api-module-microsoft-teams,@friggframework/api-module-slack,@friggframework/api-module-42matters,@friggframework/api-module-asana,@friggframework/api-module-attio,@friggframework/api-module-clio,@friggframework/api-module-connectwise,@friggframework/api-module-contentful,@friggframework/api-module-contentstack,@friggframework/api-module-crossbeam,@friggframework/api-module-deel,@friggframework/api-module-fathom,@friggframework/api-module-fireflies,@friggframework/api-module-frigg-scale-test,@friggframework/api-module-frontify,@friggframework/api-module-gong,@friggframework/api-module-google-calendar,@friggframework/api-module-google-drive,@friggframework/api-module-helpscout,@friggframework/api-module-hubspot,@friggframework/api-module-ironclad,@friggframework/api-module-linear,@friggframework/api-module-otter,@friggframework/api-module-pipedrive,@friggframework/api-module-quo,@friggframework/api-module-reevo,@friggframework/api-module-salesforce,@friggframework/api-module-stripe,@friggframework/api-module-unbabel-projects,@friggframework/api-module-unbabel,@friggframework/api-module-zoho-crm,@friggframework/api-module-zoomAuthors: 1