diff --git a/.changeset/oauth-refresh-store-writability-gate.md b/.changeset/oauth-refresh-store-writability-gate.md new file mode 100644 index 0000000000..23d6994d30 --- /dev/null +++ b/.changeset/oauth-refresh-store-writability-gate.md @@ -0,0 +1,13 @@ +--- +"executor": patch +--- + +**A credential-store outage no longer costs an OAuth connection its grant** + +Refreshing an OAuth token spends the stored refresh token: the authorization server rotates it, so the copy we sent stops working the moment the grant succeeds and the rotated one is the only thing that can mint again. Persisting the rotated token first bounds what a partial write can lose, but it cannot help when the store is refusing writes outright — the grant has already run, there is nowhere to put the successor, and every later refresh replays a token the server has revoked. The connection then reports `invalid_grant` and demands a re-auth over what was only a storage blip. + +The refresh is now gated on a store that is proven writable. Before the grant runs, it writes a fixed value to an item of its own that holds no credential and sits in the same partition as the connection's tokens. A store that cannot take that write fails the resolve while the stored refresh token is still valid, so the connection recovers on its own once the store does. + +The probe deliberately does not test the store by rewriting the refresh token with the value it just read. That is a read-then-write with no compare-and-set, and two instances refreshing one connection would lose the newer token to it: one reads the stored token, the other spends that same token and stores its rotated replacement, and the first then writes the spent one back over the replacement. The connection would die exactly the way the gate is meant to prevent. + +The probe also removes a write rather than adding one in the common case. Authorization servers that do not rotate hand back the same refresh token, and that value is no longer re-persisted when it has not changed — a rotated token never matches, so the write that matters still happens. diff --git a/e2e/cloud/credential-write-durability.test.ts b/e2e/cloud/credential-write-durability.test.ts index 589b050db4..3353513cdc 100644 --- a/e2e/cloud/credential-write-durability.test.ts +++ b/e2e/cloud/credential-write-durability.test.ts @@ -1,6 +1,6 @@ // Cloud: a refreshed OAuth credential has to be PERSISTED, and persisting it is // a pair of version-checked writes into WorkOS Vault — the rotated refresh -// token and the new access token, one after the other, not atomically. Two +// token and the new access token, one after the other, not atomically. Three // production failures live in that gap. // // 1. Contention. Two concurrent probes of one connection each run a refresh and @@ -14,6 +14,14 @@ // token we sent, so nothing can mint again and every later use of the // connection comes back `invalid_grant`. The access token, by contrast, is // disposable — one more grant re-mints it. +// 3. Writability. Ordering bounds the damage but cannot remove it: when the +// store refuses writes outright, a grant that has already run has spent the +// stored refresh token and there is nowhere to put its successor. The +// refresh must therefore be gated on a store that is proven writable BEFORE +// the grant, so a storage outage costs the user nothing but the wait. The +// gate writes an object of its own that holds no credential — proving the +// store on the refresh token's own object would mean writing a value read +// moments earlier, which is how a peer's rotated token gets overwritten. // // Both are pinned here at the product surface, black box. Failures are armed on // the WorkOS emulator that the product's own WorkOS client talks to; no product @@ -568,3 +576,85 @@ scenario( }), ), ); + +scenario( + "Credential persistence · a store that cannot accept writes fails the refresh before the grant is spent", + {}, + Effect.scoped( + Effect.gen(function* () { + const { attempt, call, oauth, slug, upstream, workos } = yield* connectIntegration; + + yield* call("baseline"); + + // One healthy refresh first, so the object the writability gate uses + // exists and can be named. That object is the point of this scenario: + // the gate proves the store on an item of its OWN, never by rewriting + // the refresh token it is about to spend. + upstream.revokeSeenBearers(); + yield* call("pre-outage-refresh"); + + const objects = yield* vaultObjectsFor(workos, slug); + const refreshObject = objects.find((object) => object.name.endsWith("refresh")); + expect(refreshObject, "the connection stored a refresh token in the vault").toBeDefined(); + const probeObject = objects.find((object) => object.name.endsWith("store-probe")); + expect( + probeObject, + "the writability gate wrote an object of its own, not the refresh token's", + ).toBeDefined(); + expect(probeObject!.id, "and it is a distinct object").not.toBe(refreshObject!.id); + + const grantsBeforeOutage = (yield* oauth.requests).filter(isRefreshGrant).length; + + // Break that object, and only that one, with a status the write policy + // cannot treat as contention — 503 is the store being down, not a peer + // holding the row, so no amount of retrying can land it. This is a store + // that will not accept a write at all. + const duringOutage = yield* Effect.scoped( + Effect.gen(function* () { + const armed = yield* armFault(workos, { + match: { method: "PUT", pathPattern: `/vault/v1/kv/${probeObject!.id}` }, + response: { status: 503, body: { code: "unavailable", message: "vault unavailable" } }, + times: 3, + }); + + upstream.revokeSeenBearers(); + const result = yield* attempt(); + expect( + yield* faultsServed(workos, armed), + "the write the gate makes is the one that broke", + ).toBeGreaterThanOrEqual(1); + return result; + }), + ); + expect( + duringOutage.ok, + `the call reports the failure while the store is down (got: ${duringOutage.text.slice(0, 200)})`, + ).toBe(false); + + // The whole point: the failure landed on the writability check, not on + // the persist that follows a grant. No grant ran, so the stored refresh + // token was never spent and is still the one the authorization server + // will honour. + expect( + (yield* oauth.requests).filter(isRefreshGrant).length, + "no refresh grant is spent while the store cannot persist its rotated successor", + ).toBe(grantsBeforeOutage); + + // The store is back. A transient outage must cost nothing but the wait: + // the connection still holds a live grant and refreshes itself, with no + // human reconnecting anything. + const recovered = yield* attempt(); + expect( + recovered.ok, + `the connection refreshes itself once the store recovers (got: ${recovered.text.slice(0, 400)})`, + ).toBe(true); + expect((JSON.parse(recovered.text) as ToolEnvelope).ok, "the recovered call succeeded").toBe( + true, + ); + expect( + (yield* oauth.requests).filter(isRefreshGrant).length, + "the recovery was a real refresh grant on the token the outage preserved", + ).toBe(grantsBeforeOutage + 1); + }), + ), +); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index f5ad432e79..675ded62d0 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -105,6 +105,8 @@ import type { } from "./integration"; import { makeOAuthService, + STORE_WRITABILITY_PROBE_VALUE, + storeWritabilityProbeItemIdFor, type MintOAuthConnectionInput, type OAuthScopePolicy, } from "./oauth-service"; @@ -1956,11 +1958,21 @@ export const createExecutor = => Effect.gen(function* () { if (provider.set) { @@ -1969,7 +1981,11 @@ export const createExecutor = { ), ); + // Two product instances, one connection, one credential store. The in-flight + // refresh gate serialises refreshes WITHIN an instance and cannot see across + // them, so nothing but the store itself stands between two refreshers and + // the same rotated token. The provider below opens a seam exactly where the + // danger is — between the read of the stored refresh token and whatever the + // reader writes next — because a probe that "tests" the store by rewriting + // the value it just read would put the spent token back over the peer's + // rotated one, and kill the connection it was added to protect. + it.effect( + "a refresher paused after reading the stored token never writes it back over a peer's rotated one", + () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + + const store = new Map(); + // Every item id written, in order — the tape that says WHICH item a + // refresher touched, which is the whole question here. + const writes: string[] = []; + const pausedAtRead = yield* Deferred.make(); + const resumeFromRead = yield* Deferred.make(); + // One-shot: the FIRST read of a refresh token stops there; the + // peer's read, moments later, runs straight through. + let pauseNextRefreshRead = false; + + const sharedStore: CredentialProvider = { + key: ProviderKey.make("shared-memory"), + writable: true, + get: (id) => + Effect.gen(function* () { + const value = store.get(String(id)) ?? null; + if (pauseNextRefreshRead && String(id).endsWith(":refresh")) { + pauseNextRefreshRead = false; + yield* Deferred.succeed(pausedAtRead, undefined); + yield* Deferred.await(resumeFromRead); + } + return value; + }), + set: (id, value) => + Effect.sync(() => { + writes.push(String(id)); + store.set(String(id), value); + }), + delete: (id) => Effect.sync(() => void store.delete(String(id))), + }; + + // One database and one credential store, two executors over them — + // the deployment this race needs and the one a single harness + // cannot express. + const config = { + ...makeTestConfig({ plugins: [oauthPlugin] as const }), + providers: [sharedStore], + }; + const instanceA = yield* createExecutor(config); + const instanceB = yield* createExecutor(config); + yield* Effect.addFinalizer(() => + Effect.promise(() => config.testDb.close()).pipe(Effect.ignore), + ); + yield* Effect.addFinalizer(() => instanceA.close().pipe(Effect.ignore)); + yield* Effect.addFinalizer(() => instanceB.close().pipe(Effect.ignore)); + + yield* instanceA.acme.seed(); + yield* instanceA.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + const started = yield* instanceA.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* instanceA.oauth.complete({ state: started.state, code: callback.code }); + + const refreshItemId = [...store.keys()].find((key) => key.endsWith(":refresh")); + expect(refreshItemId, "the completed connection stored a refresh token").toBeDefined(); + const spentRefreshToken = store.get(refreshItemId!); + + // Expire the access token so both instances must refresh. + yield* Effect.promise(() => + config.db.updateMany("connection", { + where: (b) => b("name", "=", "main"), + set: { expires_at: Date.now() - 60_000 }, + }), + ); + + // A begins a refresh and stops the instant it has the stored refresh + // token in hand. This is the window. + pauseNextRefreshRead = true; + const refresherA = yield* Effect.forkChild( + Effect.exit(instanceA.execute(ToolAddress.make("tools.acme.org.main.whoami"), {})), + ); + yield* Deferred.await(pausedAtRead); + + // B refreshes on that same token, to completion. The authorization + // server rotates it, so what the store holds afterwards is the only + // value left that can ever mint again. + yield* instanceB.execute(ToolAddress.make("tools.acme.org.main.whoami"), {}); + const rotatedByB = store.get(refreshItemId!); + expect(rotatedByB, "the peer's refresh rotated the stored token").not.toBe( + spentRefreshToken, + ); + const writesBeforeAResumes = writes.length; + + // A resumes into a world where the token it is holding is already + // spent — and still has to prove the store is writable before it + // tries to spend it. + yield* Deferred.succeed(resumeFromRead, undefined); + yield* Fiber.join(refresherA); + + expect( + store.get(refreshItemId!), + "the peer's rotated refresh token is still what the store holds", + ).toBe(rotatedByB); + expect( + [...store.entries()] + .filter(([, value]) => value === spentRefreshToken) + .map(([key]) => key), + "the spent refresh token was not written back anywhere", + ).toEqual([]); + + // The gate did still run for A — on an item of its own, holding no + // credential. Its grant then failed on the spent token, so it + // persisted nothing: this one write is everything A wrote. + const writtenByA = writes.slice(writesBeforeAResumes); + expect(writtenByA, "the resumed refresher wrote exactly one item").toHaveLength(1); + expect(writtenByA[0], "and it was not the refresh token's own item").not.toBe( + refreshItemId, + ); + expect( + [spentRefreshToken, rotatedByB], + "the item it wrote carries no credential", + ).not.toContain(store.get(writtenByA[0]!)); + + // Both instances really did reach the authorization server, so the + // interleaving under test happened rather than being short-circuited. + expect( + (yield* server.requests).filter( + (request) => + request.path === "/token" && request.body.includes("grant_type=refresh_token"), + ), + "both instances sent a refresh grant", + ).toHaveLength(2); + }), + ), + ); + it.effect( "refreshes a Personal (user) connection minted through a Workspace (org) app — own→shared client resolution", () => diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index ad35a07aa1..b853a8eac0 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -270,6 +270,34 @@ const accessItemId = (owner: Owner, integration: IntegrationSlug, name: Connecti `oauth:${owner}:${integration}:${name}`; const refreshItemIdFor = (accessId: string): string => `${accessId}:refresh`; +/** The item a refresh writes to prove the credential store will ACCEPT a write, + * before the grant spends the single-use refresh token. It holds no credential + * and never has. + * + * It has to be its own item. The cheaper-looking probe — rewriting the refresh + * token with the value just read — is a read-then-write with no + * compare-and-set, and two refreshers of one connection on different instances + * lose the newer token to it: A reads R0, B consumes R0 and stores the rotated + * R1, then A's probe puts R0 back over R1 and the connection is dead the next + * time anything needs it. The in-memory single-flight gate spans one instance + * only, and a backing store whose own write path is read-latest-then-write + * cannot catch it either. + * + * The id is the refresh item's id plus a fixed suffix, rather than one + * rebuilt from the connection's parts, so it carries the same prefix and + * therefore the same embedded owner — the same store partition, the same + * encryption context, the same object-name head. A store that would refuse + * the refresh token's write refuses this one. Per connection rather than one + * per partition, so the only writers that can contend on it are the + * concurrent refreshers of a single connection, and they all write the same + * constant. */ +export const storeWritabilityProbeItemIdFor = (refreshItemId: string): string => + `${refreshItemId}:store-probe`; + +/** What the writability probe stores. A constant, because the item exists to + * prove a write lands and carries no information of its own. */ +export const STORE_WRITABILITY_PROBE_VALUE = "writable"; + /** Order-preserving de-duplication of a scope list. */ const dedupeScopes = (scopes: readonly string[]): readonly string[] => [...new Set(scopes)]; diff --git a/packages/core/sdk/src/provider-item-owner.ts b/packages/core/sdk/src/provider-item-owner.ts index 74e857fdb1..f4513faef5 100644 --- a/packages/core/sdk/src/provider-item-owner.ts +++ b/packages/core/sdk/src/provider-item-owner.ts @@ -4,9 +4,14 @@ // ids in executor.ts, the oauth-client secret ids): // // connection:::: -// oauth:::[:refresh] +// oauth:::[:refresh[:store-probe]] // oauth-client:::secret // +// The `:store-probe` tail is the one id here that holds no credential: a +// refresh writes it to prove the store accepts writes before a grant spends +// the refresh token. It hangs off the refresh id so it inherits the same +// owner, and therefore the same partition, as the credential it stands in for. +// // Credential providers file plugin-storage rows by THIS owner, not the acting // caller's binding — an org connection whose OAuth consent completes in one // member's browser session must produce rows every member can resolve