fix(oauth): record revoked refresh tokens durably and surface them as reconnect by waleedlatif1 · Pull Request #8868 · simstudioai/sim

waleedlatif1 · 2026-10-10T01:25:34Z

Summary

Root cause: when a provider revokes a refresh token (invalid_grant and kin), the refresh path only set a 1-hour Redis flag. Each scheduled run then failed with a generic error, which was logged at ERROR three times: OAuthTokenResolution "Failed to refresh access token", ExecutorCredentialToken "Credential token resolution failed", and Tools "Error fetching access token for ". Every time the flag lapsed, the provider was asked again, roughly once an hour, with no end.
Durable record: account gets refresh_revoked_at / refresh_revoked_code / refresh_revoked_token_hash.
- The record holds only while its HMAC still matches the stored refresh token, so a writer that stores a new chain supersedes it.
- Every reconnect path also clears it explicitly. A provider can reauthorize without issuing a new refresh token, and the hash alone can't detect that. The reconnect paths are the credential draft hooks, the organization draft, and Better Auth's relink (account.update.after on callback paths).
No false revocations: the record is written only on rows that still hold the rejected token and whose updated_at hasn't moved since the leader reread the row under the refresh lock, after the existing chain-moved checks. A refresh that lost a race to a newer chain, or to a reconnect that keeps the same refresh token, records nothing. Revocations no longer use the Redis dead flag; it now holds only app-registration faults (invalid_client etc.), which stay ERROR.
Daily re-probe: a recorded revocation answers without calling the provider. Once a day a single refresh re-probes it, because a few rejections clear without a reconnect (e.g. an admin lifting a Conditional Access block).
- A successful refresh clears the record.
- A re-probe that fails for a transient reason keeps the record and restarts its window.
Typed error: refreshTokenIfNeeded throws CredentialRevokedError. Token resolution returns code: OAUTH_CREDENTIAL_REVOKED (POST and GET /api/auth/oauth/token) with a "reconnect" message, logged once at WARN with credential id, provider and error code.
- The executor and tool access-token sites no longer re-log it.
- The connector sync classifies the revocation first and leaves its single log line to executeSync, which already logs when it unschedules the connector.
- oauth.ts logs revocation-class provider rejections at WARN. Its pure classifiers moved from terminal-errors.ts, which is Redis-backed, to refresh-error-codes.ts, so this client-reachable module can import them.
- getCredentialTerminalRefreshError reads the durable record, so connectors keep unscheduling revoked credentials after any Redis TTL.
Migration 0405_oauth_refresh_revoked: three nullable ADD COLUMNs with no default. Expand-only and metadata-only; the deployed app neither reads nor writes these columns.

What to watch after deploy

For each revoked credential, the ERROR triple per scheduled run (ExecutorCredentialToken / OAuthTokenResolution / Tools) becomes a single OAuthTokenResolution WARN, "OAuth credential revoked by provider; reconnect required", carrying credentialId, providerId and errorCode.
"Skipping refresh: credential recently failed" WARNs should disappear for revocations. Provider re-hits drop from hourly to daily, each logging one OAuthCredentialService WARN, "Provider revoked the refresh token; recorded until reconnect".
Any remaining ERROR on these loggers is a non-revocation failure worth investigating.

Type of Change

Bug fix

Testing

New lib/oauth/credential-revocation.integration.ts: 13 tests against real Postgres and Redis with a fixture token endpoint. It covers:
- the typed code;
- no provider call once revoked;
- outages never recorded;
- race losers never recorded (OAuth and Slack);
- reconnect with a new token, reconnect keeping the old token, and a reconnect landing while a rejected refresh is in flight;
- day-old re-probe, with success clearing the record and an outage keeping it;
- concurrent runs, plus a follower in another process;
- a Slack installation-wide record cleared by fan-out.
Red before fix: 5 of the original tests fail on the pre-branch code. The 3 regression tests added after review (same-token reconnect, Slack race loser, re-probe outage) fail on the previous revision of this branch. The race and outage guards turn red under mutation.
CI runs the suite under both migrate and push provisioning (13/13), along with check:migrations, lint, type-check, audits and the unit suites. Locally, before the review rounds: bun run lint, bun run type-check, bun run check:audits, bun run check:migrations, drizzle-kit generate (no changes); the affected unit suites (lib/oauth, lib/credentials, executor/utils, lib/knowledge/connectors, app/api/auth/oauth, lib/auth, tools/index.test.ts); and bun run test:integration for the new suite plus shopify.integration.ts.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing (new tests pass the test-audit authoring gate)
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

vercel · 2026-10-10T01:25:41Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
docs	Ready	Preview	Oct 10, 2026 2:27am UTC

cubic-dev-ai

All reported issues were addressed across 22 files

_{Turn on auto-fix | Re-trigger cubic}

greptile-apps · 2026-10-10T01:31:25Z

Confidence Score: 5/5

[Critical impact] The PR appears safe to merge; no new actionable defect was established.

Summary

Records revoked OAuth refresh tokens in Postgres and returns a stable reconnect error.

Revoked refresh tokens stay recorded until they can be checked again.
Revoked credentials return a reconnect error instead of a generic failure.
Reconnects clear the old revocation even when the provider keeps the same token.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Access token needs refresh] --> B{Matching revocation recorded?}
  B -->|Yes, less than one day old| C[Return reconnect error]
  B -->|No, or daily retry due| D[Acquire refresh lock]
  D --> E[Reread stored record]
  E -->|Recent revocation| C
  E -->|Refresh allowed| F[Ask provider]
  F -->|Success| G[Store tokens and clear record]
  F -->|Revoked| H[Record only if token and account version still match]
  H --> C
  F -->|Temporary failure during daily retry| I[Keep matching record and restart retry window]
  I --> C
  J[Owner reconnects] --> K[Clear recorded refresh failure]

_{Reviews (5) · Last reviewed commit: "fix(oauth): clear only the recorded revo..." · Reviewed by Greptile}

waleedlatif1 · 2026-10-10T01:39:09Z

@greptile

waleedlatif1 · 2026-10-10T01:39:10Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-10T01:39:15Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 28 files

_{Reply with feedback, questions, or to request a fix.

Turn on auto-fix | Re-trigger cubic}

waleedlatif1 · 2026-10-10T01:51:25Z

@greptile

waleedlatif1 · 2026-10-10T01:51:26Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-10T01:51:33Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 28 files

_{Reply with feedback, questions, or to request a fix.

Turn on auto-fix | Re-trigger cubic}

waleedlatif1 · 2026-10-10T01:59:16Z

@greptile

waleedlatif1 · 2026-10-10T01:59:17Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-10T01:59:21Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 28 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Turn on auto-fix | Re-trigger cubic}

… reconnect A refresh token the provider revoked (invalid_grant and kin) was only flagged in Redis for an hour, so every scheduled run kept failing with a generic error logged at ERROR three times, and the provider was asked again each hour, indefinitely. - account gains refresh_revoked_at/_code/_token_hash (expand-only, nullable). The record holds only while its hash fingerprints the stored refresh token, so a writer storing a new chain supersedes it; every reconnect path also clears it explicitly, since a provider can reauthorize without issuing a new refresh token. - The record is written only on rows still holding the rejected token, so a refresh that lost a race to a newer chain cannot mark the live credential revoked. Revocations no longer use the Redis dead flag, which now holds only app-registration faults. - A recorded revocation answers without the provider; one re-probe a day lets rejections that clear without a reconnect recover. A successful refresh clears it; a re-probe that fails for a passing reason keeps it. - refreshTokenIfNeeded throws CredentialRevokedError; token resolution returns code OAUTH_CREDENTIAL_REVOKED (POST and GET) and logs it once at WARN; the executor, tool and connector sites no longer log it at ERROR.

- Record a revocation only when the row's updated_at has not moved since the leader read it, so a reconnect that keeps the same refresh token while a rejected refresh is in flight is not undone. - Reread the row under the refresh lock, so a revocation another process just recorded is honored instead of calling the provider again. - A re-probe whose transient failure meets a moved chain reports no revocation. - Slack fan-out clears the record only for a chain the provider just issued, not when re-spreading a stored one. - A reconnect whose cleanup fails now fails the callback. - Revocation rejections log at WARN in oauth.ts too; the pure refresh error classifiers move to refresh-error-codes.ts so that client-reachable module can use them. - The connector path leaves the revocation log to executeSync.

…rocess check without Redis

…uite

…ith no Redis on the sign-in path

waleedlatif1 · 2026-10-10T02:21:59Z

@greptile

waleedlatif1 · 2026-10-10T02:22:00Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-10T02:22:04Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 28 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.

Turn on auto-fix | Re-trigger cubic}