feat(security): encrypt account OAuth tokens at rest - #7232
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
c49745d to
0a09630
Compare
Greptile SummaryThe PR introduces feature-gated AES-256-GCM encryption for OAuth tokens stored in account records, with mixed plaintext/ciphertext reads and a manual backfill path.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the supplied follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/oauth/account-token-crypto.ts | Defines the versioned AES-256-GCM account-token envelope and strict prefix-based encryption/decryption behavior. |
| apps/sim/lib/oauth/account-tokens.ts | Adds feature-gated token encryption on writes and tolerant per-column decryption on reads. |
| apps/sim/lib/oauth/credential-service.ts | Centralizes account-token loading, persistence, refresh, ownership projection, and provider-account upserts. |
| apps/sim/lib/auth/auth.ts | Integrates account-token encryption and decryption into Better Auth database hooks and consolidates Salesforce token handling. |
| scripts/backfill-account-token-encryption.ts | Adds an idempotent, batched, compare-and-swap manual backfill for legacy plaintext account tokens. |
| scripts/check-account-token-access.ts | Adds static auditing for direct account-token reads, writes, and projection-less account queries. |
| apps/sim/lib/oauth/token-resolution.ts | Centralizes resolved credential-token validation so missing access and refresh tokens fail before provider calls. |
| apps/sim/app/workspace/[workspaceId]/home/components/mothership-chat/use-chat-find.ts | Adds virtualized transcript search, navigation, and CSS Custom Highlight API integration. |
Sequence Diagram
sequenceDiagram
participant P as OAuth Provider
participant A as Application
participant F as Feature Configuration
participant K as Encryption Key
participant D as Account Table
participant C as Credential Consumer
P->>A: Return OAuth token set
A->>F: Check write-encryption flag
A->>K: Validate encryption key
alt Encryption enabled and key usable
A->>A: Encrypt each token as simenc:v1 envelope
else Encryption unavailable
A->>A: Preserve plaintext token values
end
A->>D: Persist account token columns
C->>D: Read account token columns
D-->>C: Mixed plaintext or encrypted values
C->>C: Detect envelope per value
C->>K: Decrypt encrypted values
C->>P: Call provider with resolved access token
Reviews (6): Last reviewed commit: "test(security): pin the shrunken audit a..." | Re-trigger Greptile
3964adb to
c9c37cb
Compare
The Better Auth `account` table stored `access_token`, `refresh_token` and `id_token` in plaintext. It is the credential store for every user-connected integration, so a dump of it was a dump of our customers' third-party data — the last plaintext credential store in the repo, and the largest. Tokens are now stored under a versioned AES-256-GCM envelope, `simenc:v1:<iv>:<ciphertext>:<authTag>`, built on the existing `encryptSecret`/`decryptSecret` primitives. Rollout is safe in both directions. Reads detect the format per value and never consult the flag, so a mixed-format table reads correctly throughout; only writes are gated, behind the AppConfig flag `oauth-token-encryption`, which is off by default. The deploy is therefore inert on arrival and the flag is flipped once every pod carries the tolerant reader. Rolling back is a config change. Self-hosted stays on plaintext until an operator opts in with a valid 64-hex `ENCRYPTION_KEY`; a misconfigured key degrades to plaintext rather than failing a user's OAuth connect. Better Auth's own `account.encryptOAuthTokens` is deliberately not used: it keys off `BETTER_AUTH_SECRET` rather than `ENCRYPTION_KEY`, leaves `idToken` in plaintext despite its docs, decrypts only inside its own endpoints rather than on the direct database reads this app performs, and detects ciphertext by treating any even-length hex string as encrypted — the shape of a real Trello or Airtable token. The rationale is recorded next to the envelope so the two schemes are never confused. Consolidation, because the duplication is what made encryption risky: - Three divergent copies of the token-staleness rule collapse into `refresh-policy.ts`. `getOAuthToken`'s copy omitted the Microsoft proactive-refresh arm, so credentials reached only that way could pass Microsoft's 90-day inactivity deadline and die; unifying fixes that. - `refreshTokenIfNeeded`'s `credential: any` becomes a branded `LoadedOAuthCredential`, which caught three callers passing raw rows at compile time. Two collapse onto the new `resolveAccessTokenForAccount`. - Eleven projection-less `account` reads become `id`/`userId` projections or calls to the existing `getCredentialOwner`. - The Shopify, Instagram and Trello connect flows — three copies of find/update/insert/re-find — share `upsertProviderAccountTokens`, so a new provider cannot store a plaintext token by copying an old flow. `check:account-token-access` enforces the boundary in CI, flagging direct token column reads, projection-less selects, and direct writes to the table. Also fixed along the way: `create.before` and `create.after` both called `fetchSalesforceInstanceUrl` and both prepended the instance-URL marker, so every Salesforce connect made the same live API call twice and stored a double-prefixed `scope`. And Better Auth's `/get-access-token` and `/refresh-token` endpoints, reachable through the catch-all and bypassing `databaseHooks` entirely, are now blocked — nothing in this app calls them. No migration: no query filters or joins on a token value, and the columns are `text`.
c9c37cb to
5ba3257
Compare
There was a problem hiding this comment.
All reported issues were addressed across 30 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
- The access audit matched line by line, so any Drizzle chain the formatter wrapped — which is how `db.select().from(account)` is normally written — was invisible to it. Matching over the whole source and mapping offsets back to lines closes that; a projection-less multiline select now fails CI as intended. - A legacy token beginning `simenc:` was classified as an envelope, which left it unencrypted on write and threw `unknown-version` on read, making the credential unavailable. Detection now requires a full versioned header, so such a value stays plaintext. - Encryption failures were caught and the token stored in plaintext. The only expected cause is an unusable key, which the gate already handles, so a throw past it is a real fault; swallowing it would silently break the guarantee the flag reports as on.
Once the flag is on, tokens envelope themselves as they are written or refreshed — but only for rows that get rewritten. A provider that issues no refresh token, and a user who never signs in again, both leave a row in plaintext indefinitely. This closes that tail. Deliberately a manual script rather than a `script-migration`, so it never runs as part of `db:migrate` or a deploy: a self-hosted upgrade is unaffected by its existence and nothing happens until an operator runs it. - Dry run unless `--apply`. Enveloping is not reversible without the key, so the destructive direction is opt-in twice. - Refuses to start unless `ENCRYPTION_KEY` is usable and an AES-GCM round trip agrees with itself, so a misconfigured deployment touches no rows. - Keyset pagination that must advance, so a persistently racing row cannot loop forever. - Compare-and-swap on the exact values read. A token rotated by a concurrent refresh is reported and skipped, never reverted to the stale one. - Never writes `updated_at` — Slack's fan-out version guard, Instagram's minimum token age, connection ordering and "last connected" all read it. `fieldsNeedingEncryption` moves into the crypto module so the bulk job and the live write path share one definition of "already encrypted" rather than drifting. The SQL pre-filter matches `simenc:v%`, not `simenc:%`, to agree with it: a legacy value that merely begins `simenc:` is not ours and must still be enveloped rather than skipped.
…nt-oauth-tokens # Conflicts: # apps/sim/lib/oauth/credential-service.ts
…aging merge Swarm-verification round on the merged tree. The end-to-end trace found no decrypt/encrypt gap and the privacy-mode threading from the selector unification survives every hop; these are the items the sweep did surface. - The backfill's SQL pre-filter (`LIKE 'simenc:v%'`) disagreed with the app's envelope classifier for a value such as `simenc:vX:…` — excluded from selection yet counted as plaintext by the app, so it would never be enveloped while the run reported the table as done. The predicate is now the regex twin of `ENVELOPE_HEADER_RE`. Also: dry runs walk the whole table so the summary is the real total, batch size is clamped, `--sleep=0` works, and a run of 25 consecutive row failures aborts instead of erroring 92k times. - `/account-info` joins the blocked Better Auth endpoints: it shares `getValidAccessToken` with the two POST endpoints but is GET, so it gets the same treatment on the GET handler. Nothing in the monorepo calls it. - `resolveAccessTokenForAccount` now accepts `CredentialTokenResolutionOptions` and gates its identifier logs, so a future selector routed through it keeps the privacy guarantees instead of silently losing them. - The refresh path logs its decision reason again — the consumer of `RefreshDecision.reason` had been collapsed away in the merge. - `safeAccountInsert` folds into `upsertProviderAccountTokens` (its only caller), `findAccountIdByProviderAccount` goes module-private, the token field list is declared once, and the audit allowlist shrinks to the two modules that genuinely touch the table. - Dead weight removed: an unused Shopify logger, a redundant dynamic import in the backfill, a stale `safeAccountInsert` mock for a module that no longer exists, and three change-log-style comments rewritten to describe the code.
The allowlist dropped the accessor and crypto modules — neither holds a query — but the audit's own test still asserted their exemption. It now asserts the inverse: only credential-service and slack are exempt, and the other two are audited like any file. This test runs from the root test chain, which is why the apps/sim suite stayed green while CI went red.
Why
The Better Auth
accounttable storedaccess_token,refresh_tokenandid_tokenin plaintext (packages/db/schema.ts:94-96). It is the credential store for every user-connected integration — Google Drive, Slack, GitHub, Salesforce, ~60 connectors — plus OIDC sign-in tokens. A dump of that table was a dump of our customers' third-party data.Every newer credential path already encrypts (
credential.encryptedOauthTokenSet,encryptedServiceAccountKey, MCP OAuth, BYOK, env vars). This was the last plaintext credential store, and the largest.What
Tokens are stored under a versioned AES-256-GCM envelope,
simenc:v1:<iv>:<ciphertext>:<authTag>, built on the existingencryptSecret/decryptSecretprimitives.Reads detect the format per value and never consult the flag, so a mixed-format table reads correctly throughout. Only writes are gated, behind the AppConfig flag
oauth-token-encryption, which is off by default. The deploy is therefore inert on arrival; the flag is flipped once every pod carries the tolerant reader, and rolling back is a config change rather than a data problem.Self-hosted stays on plaintext until an operator opts in. A misconfigured
ENCRYPTION_KEYdegrades to plaintext rather than failing a user's OAuth connect — losing a connection is worse than staying plaintext.No migration. No query filters or joins on a token value (the only predicate anywhere is
isNotNull, which is unaffected), and the columns aretext.Why not Better Auth's
account.encryptOAuthTokensDeliberately rejected, and the rationale is recorded next to the envelope so the two are never confused. It keys off
BETTER_AUTH_SECRETrather thanENCRYPTION_KEY; leavesidTokenin plaintext despite its docs; decrypts only inside its own endpoints, not on the ~20 direct database reads this app performs; and detects ciphertext by treating any even-length hex string as encrypted — which is the shape of a real Trello or Airtable token. The two schemes are mutually exclusive: its detector does not recognise our prefix.Consolidation
The duplication is what made encryption risky, so it went first:
refresh-policy.ts.getOAuthToken's copy omitted the Microsoft proactive-refresh arm, so credentials reached only that way could pass Microsoft's 90-day inactivity deadline and die. Unifying fixes that (called out below as an intentional behaviour change).refreshTokenIfNeeded'scredential: anybecomes a brandedLoadedOAuthCredential, which caught three callers passing raw rows at compile time. Two collapse onto the newresolveAccessTokenForAccount.accountreads becomeid/userIdprojections or calls to the already-existinggetCredentialOwner.upsertProviderAccountTokens, so a new provider cannot store a plaintext token by copying an old flow.Net −21 lines across modified files despite adding encryption.
Guardrail
check:account-token-access(new, incheck:audits) flags direct token-column reads, projection-less selects, and direct writes to the table. The write rule matters most:db.insert(account).values({ accessToken })is exactly how the next connect flow would be written, and no read-side rule catches it.Bugs fixed along the way
create.beforeandcreate.afterboth calledfetchSalesforceInstanceUrland both prepended the instance-URL marker (withSalesforceInstanceScopeprepends unconditionally) — so every Salesforce connect made the same live API call twice and stored a double-prefixedscope.POST /api/auth/get-access-tokenand/refresh-tokenare reachable through the catch-all and readaccountthrough the adapter with nodatabaseHookspass. Nothing in this app calls them; they are now blocked alongside the existing organization/SSO blocks.refreshTokenIfNeededcould return{ accessToken: null }— the parameter wasany, so nothing caught it — and callers forwarded the null to a provider. It now fails as the 401 it always was.Intentional behaviour changes
getOAuthToken. A bug fix, but it means those credentials issue refresh writes that bumpupdated_at; checked against all fourupdated_atconsumers and safe.<→<=on access-token expiry. A token expiring exactly atnowrefreshes.generateId()rather thantrello_${userId}_${Date.now()}. Verified nothing depends on the prefix.create.after. Previously both hooks fetched it — two identical live API calls per connect, and the marker was prepended twice intoscope. The trade: a transient failure of the single fetch now leaves the marker unwritten for that row, where before a second fetch might have caught it. The 9 Salesforce tools'idTokenfallback covers exactly that case (rows without markers already exist in production and work through it).resolveCredentialTokeninstead of a 200 carryingaccessToken: null— which callers forwarded to providers as a null bearer token — and the failed attempt no longer records a "credential used" audit row.Each is pinned by a test; 4 and 5 were surfaced by a post-merge behavioral-equivalence audit against staging, site by site.
Out of scope
managed_oauthand service-account credentials (already encrypted, different format, same key — do not unify). Moving Shopify's shop domain out of the overloadedid_tokeninto a scope marker. The backfill for dormant rows, which is a separate manually-run script. Tighteningenv.ts'sENCRYPTION_KEYvalidation frommin(32)to 64-hex, which is a breaking change for existing self-hosters and needs its own release note.Verification
bun run check:audits— 40/40bun run lint:check— 26/26 packagestsc --noEmit— clean@aws-sdk/client-lambdaones (reproduced with this branch stashed) and one unrelated timeout flake that passes in isolation