* Add e2e repro for vault write failure during token refresh * Gate OAuth token refresh on credential-store writability A vault write failure while persisting a refreshed OAuth token lost the rotated refresh token: the grant had already consumed the single-use stored token at the authorization server, so once the vault recovered the next refresh replayed the revoked token, got invalid_grant, and the connection demanded a re-auth over what was only a storage blip. Before consuming the refresh token, rewrite the stored value in place as a writability probe. A store outage now fails the resolve before the grant, leaving the stored token valid so the connection recovers with the store. * Fold the store-writability gate into the credential-persistence suite Main now persists the rotated refresh token before the access token, so the ordering half of this failure is fixed. The writability half is not: when the store refuses writes outright, the grant has already spent the stored token and there is nowhere to put its successor. Keep the pre-flight rewrite that proves the store is writable before the grant, move its scenario into the existing credential-write-durability suite rather than standing up a second file, and drop the prose that still claimed the persist ordering was wrong. Skip the persist's own refresh-token write when the authorization server hands back an unrotated token, so the probe removes a vault write on the common path instead of adding one. * Probe store writability on a dedicated item, not the refresh token The writability gate proved the store by rewriting the refresh token with the value it had just read. That is a read-then-write with no compare-and-set, so two instances refreshing one connection lose the newer token: one reads the stored token, the other spends it and stores the rotated replacement, and the first then writes the spent one back over it. The in-flight refresh gate is per instance and cannot see the peer. The gate now writes a fixed value to an item of its own, derived from the refresh item's id so it lands in the same store partition and proves the same thing. It holds no credential, so nothing is at risk when two refreshers race on it.
Unified · Split
| @@ -0,0 +1,13 @@ | ||
| 1 | +--- | |
| 2 | +"executor": patch | |
| 3 | +--- | |
| 4 | + | |
| 5 | +**A credential-store outage no longer costs an OAuth connection its grant** | |
| 6 | + | |
| 7 | +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. | |
| 8 | + | |
| 9 | +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. | |
| 10 | + | |
| 11 | +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. | |
| 12 | + | |
| 13 | +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. | |
| @@ -1,6 +1,6 @@ | ||
| 1 | 1 | // Cloud: a refreshed OAuth credential has to be PERSISTED, and persisting it is |
| 2 | 2 | // a pair of version-checked writes into WorkOS Vault — the rotated refresh |
| 3 | −// token and the new access token, one after the other, not atomically. Two | |
| 3 | +// token and the new access token, one after the other, not atomically. Three | |
| 4 | 4 | // production failures live in that gap. |
| 5 | 5 | // |
| 6 | 6 | // 1. Contention. Two concurrent probes of one connection each run a refresh and |
| @@ -14,6 +14,14 @@ · 7 unmodified lines | ||
| 14 | 14 | // token we sent, so nothing can mint again and every later use of the |
| 15 | 15 | // connection comes back `invalid_grant`. The access token, by contrast, is |
| 16 | 16 | // disposable — one more grant re-mints it. |
| 17 | +// 3. Writability. Ordering bounds the damage but cannot remove it: when the | |
| 18 | +// store refuses writes outright, a grant that has already run has spent the | |
| 19 | +// stored refresh token and there is nowhere to put its successor. The | |
| 20 | +// refresh must therefore be gated on a store that is proven writable BEFORE | |
| 21 | +// the grant, so a storage outage costs the user nothing but the wait. The | |
| 22 | +// gate writes an object of its own that holds no credential — proving the | |
| 23 | +// store on the refresh token's own object would mean writing a value read | |
| 24 | +// moments earlier, which is how a peer's rotated token gets overwritten. | |
| 17 | 25 | // |
| 18 | 26 | // Both are pinned here at the product surface, black box. Failures are armed on |
| 19 | 27 | // the WorkOS emulator that the product's own WorkOS client talks to; no product |
| @@ -568,3 +576,85 @@ scenario( · 548 unmodified lines | ||
| 568 | 576 | }), |
| 569 | 577 | ), |
| 570 | 578 | ); |
| 579 | + | |
| 580 | +scenario( | |
| 581 | + "Credential persistence · a store that cannot accept writes fails the refresh before the grant is spent", | |
| 582 | + {}, | |
| 583 | + Effect.scoped( | |
| 584 | + Effect.gen(function* () { | |
| 585 | + const { attempt, call, oauth, slug, upstream, workos } = yield* connectIntegration; | |
| 586 | + | |
| 587 | + yield* call("baseline"); | |
| 588 | + | |
| 589 | + // One healthy refresh first, so the object the writability gate uses | |
| 590 | + // exists and can be named. That object is the point of this scenario: | |
| 591 | + // the gate proves the store on an item of its OWN, never by rewriting | |
| 592 | + // the refresh token it is about to spend. | |
| 593 | + upstream.revokeSeenBearers(); | |
| 594 | + yield* call("pre-outage-refresh"); | |
| 595 | + | |
| 596 | + const objects = yield* vaultObjectsFor(workos, slug); | |
| 597 | + const refreshObject = objects.find((object) => object.name.endsWith("refresh")); | |
| 598 | + expect(refreshObject, "the connection stored a refresh token in the vault").toBeDefined(); | |
| 599 | + const probeObject = objects.find((object) => object.name.endsWith("store-probe")); | |
| 600 | + expect( | |
| 601 | + probeObject, | |
| 602 | + "the writability gate wrote an object of its own, not the refresh token's", | |
| 603 | + ).toBeDefined(); | |
| 604 | + expect(probeObject!.id, "and it is a distinct object").not.toBe(refreshObject!.id); | |
| 605 | + | |
| 606 | + const grantsBeforeOutage = (yield* oauth.requests).filter(isRefreshGrant).length; | |
| 607 | + | |
| 608 | + // Break that object, and only that one, with a status the write policy | |
| 609 | + // cannot treat as contention — 503 is the store being down, not a peer | |
| 610 | + // holding the row, so no amount of retrying can land it. This is a store | |
| 611 | + // that will not accept a write at all. | |
| 612 | + const duringOutage = yield* Effect.scoped( | |
| 613 | + Effect.gen(function* () { | |
| 614 | + const armed = yield* armFault(workos, { | |
| 615 | + match: { method: "PUT", pathPattern: `/vault/v1/kv/${probeObject!.id}` }, | |
| 616 | + response: { status: 503, body: { code: "unavailable", message: "vault unavailable" } }, | |
| 617 | + times: 3, | |
| 618 | + }); | |
| 619 | + | |
| 620 | + upstream.revokeSeenBearers(); | |
| 621 | + const result = yield* attempt(); | |
| 622 | + expect( | |
| 623 | + yield* faultsServed(workos, armed), | |
| 624 | + "the write the gate makes is the one that broke", | |
| 625 | + ).toBeGreaterThanOrEqual(1); | |
| 626 | + return result; | |
| 627 | + }), | |
| 628 | + ); | |
| 629 | + expect( | |
| 630 | + duringOutage.ok, | |
| 631 | + `the call reports the failure while the store is down (got: ${duringOutage.text.slice(0, 200)})`, | |
| 632 | + ).toBe(false); | |
| 633 | + | |
| 634 | + // The whole point: the failure landed on the writability check, not on | |
| 635 | + // the persist that follows a grant. No grant ran, so the stored refresh | |
| 636 | + // token was never spent and is still the one the authorization server | |
| 637 | + // will honour. | |
| 638 | + expect( | |
| 639 | + (yield* oauth.requests).filter(isRefreshGrant).length, | |
| 640 | + "no refresh grant is spent while the store cannot persist its rotated successor", | |
| 641 | + ).toBe(grantsBeforeOutage); | |
| 642 | + | |
| 643 | + // The store is back. A transient outage must cost nothing but the wait: | |
| 644 | + // the connection still holds a live grant and refreshes itself, with no | |
| 645 | + // human reconnecting anything. | |
| 646 | + const recovered = yield* attempt(); | |
| 647 | + expect( | |
| 648 | + recovered.ok, | |
| 649 | + `the connection refreshes itself once the store recovers (got: ${recovered.text.slice(0, 400)})`, | |
| 650 | + ).toBe(true); | |
| 651 | + expect((JSON.parse(recovered.text) as ToolEnvelope).ok, "the recovered call succeeded").toBe( | |
| 652 | + true, | |
| 653 | + ); | |
| 654 | + expect( | |
| 655 | + (yield* oauth.requests).filter(isRefreshGrant).length, | |
| 656 | + "the recovery was a real refresh grant on the token the outage preserved", | |
| 657 | + ).toBe(grantsBeforeOutage + 1); | |
| 658 | + }), | |
| 659 | + ), | |
| 660 | +); | |
| @@ -105,6 +105,8 @@ import type { · 104 unmodified lines | ||
| 105 | 105 | } from "./integration"; |
| 106 | 106 | import { |
| 107 | 107 | makeOAuthService, |
| 108 | + STORE_WRITABILITY_PROBE_VALUE, | |
| 109 | + storeWritabilityProbeItemIdFor, | |
| 108 | 110 | type MintOAuthConnectionInput, |
| 109 | 111 | type OAuthScopePolicy, |
| 110 | 112 | } from "./oauth-service"; |
| @@ -1956,11 +1958,21 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 1845 unmodified lines | ||
| 1956 | 1958 | * re-mints it. Persisting the access token first means a failure in |
| 1957 | 1959 | * between drops a single-use credential the authorization server has |
| 1958 | 1960 | * already consumed, and every later refresh comes back `invalid_grant` — |
| 1959 | − * a connection that silently disconnects itself. */ | |
| 1961 | + * a connection that silently disconnects itself. | |
| 1962 | + * | |
| 1963 | + * `storedRefreshToken` is the value the store already held when the | |
| 1964 | + * caller read it on the way in. Many authorization servers do NOT rotate | |
| 1965 | + * on refresh and hand back the very same refresh token, so writing it | |
| 1966 | + * again is a round trip that can only re-persist what is already there — | |
| 1967 | + * and every write bumps the stored object's version, which is the | |
| 1968 | + * contention this path spends retries fighting. Skip it when the value | |
| 1969 | + * has not changed; a rotated token never matches, so the write that | |
| 1970 | + * actually matters is never skipped. */ | |
| 1960 | 1971 | const persistRefreshedToken = ( |
| 1961 | 1972 | row: ConnectionRow, |
| 1962 | 1973 | provider: CredentialProvider, |
| 1963 | 1974 | token: OAuth2TokenResponse, |
| 1975 | + storedRefreshToken?: string | undefined, | |
| 1964 | 1976 | ): Effect.Effect<void, StorageFailure> => |
| 1965 | 1977 | Effect.gen(function* () { |
| 1966 | 1978 | if (provider.set) { |
| @@ -1969,7 +1981,11 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 2 unmodified lines | ||
| 1969 | 1981 | const tokenItemId = |
| 1970 | 1982 | connectionItemIds(row)[PRIMARY_INPUT_VARIABLE] ?? |
| 1971 | 1983 | `connection:${row.owner}:${row.integration}:${row.name}:${PRIMARY_INPUT_VARIABLE}`; |
| 1972 | − if (token.refresh_token && row.refresh_item_id) { | |
| 1984 | + if ( | |
| 1985 | + token.refresh_token && | |
| 1986 | + row.refresh_item_id && | |
| 1987 | + token.refresh_token !== storedRefreshToken | |
| 1988 | + ) { | |
| 1973 | 1989 | yield* provider.set(ProviderItemId.make(row.refresh_item_id), token.refresh_token); |
| 1974 | 1990 | } |
| 1975 | 1991 | yield* provider.set(ProviderItemId.make(tokenItemId), token.access_token); |
| @@ -2243,6 +2259,12 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 267 unmodified lines | ||
| 2243 | 2259 | // path below needs a stored refresh token. Branching on grant here is |
| 2244 | 2260 | // what keeps a client_credentials connection (e.g. DealCloud) from |
| 2245 | 2261 | // demanding a re-auth on a credential that has no human to re-auth. |
| 2262 | + // What the credential store held for this connection when the grant | |
| 2263 | + // below was prepared, so the persist can tell a ROTATED refresh token | |
| 2264 | + // from one the authorization server simply handed back unchanged. | |
| 2265 | + // Only the authorization_code path has one; client_credentials and | |
| 2266 | + // id_jag carry no refresh token at all. | |
| 2267 | + let storedRefreshToken: string | undefined; | |
| 2246 | 2268 | const token = |
| 2247 | 2269 | clientRow.grant === "client_credentials" |
| 2248 | 2270 | ? yield* exchangeClientCredentials({ |
| @@ -2275,6 +2297,36 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 26 unmodified lines | ||
| 2275 | 2297 | if (!refreshToken) { |
| 2276 | 2298 | return yield* reauth("Stored refresh token could not be resolved."); |
| 2277 | 2299 | } |
| 2300 | + // Prove the credential store is WRITABLE before consuming the | |
| 2301 | + // single-use refresh token. Ordering the persist correctly | |
| 2302 | + // (refresh token first) bounds the damage once a grant has | |
| 2303 | + // run, but it cannot help when the store is refusing writes | |
| 2304 | + // outright: the grant spends the stored token at the | |
| 2305 | + // authorization server, so a store that cannot accept the | |
| 2306 | + // rotated successor leaves the connection holding a token the | |
| 2307 | + // server has already revoked. Every later refresh then replays | |
| 2308 | + // it, gets invalid_grant, and a storage outage that healed in | |
| 2309 | + // minutes has cost the user a re-auth. Failing here instead | |
| 2310 | + // leaves the stored token valid, so the connection recovers on | |
| 2311 | + // its own when the store does. | |
| 2312 | + // | |
| 2313 | + // The probe writes its OWN item and never the refresh token's. | |
| 2314 | + // Rewriting the value just read would be one round trip | |
| 2315 | + // cheaper and is the trap: it is a read-then-write with no | |
| 2316 | + // compare-and-set, so a peer refresher on another instance | |
| 2317 | + // that spent this same token and stored its rotated successor | |
| 2318 | + // in between would have that successor overwritten by the | |
| 2319 | + // stale value — the exact dead connection this gate exists to | |
| 2320 | + // prevent, now caused by the gate. The probe item sits in the | |
| 2321 | + // same partition and holds a constant, so it proves what the | |
| 2322 | + // store will accept while no credential is ever at risk. | |
| 2323 | + if (provider.set) { | |
| 2324 | + yield* provider.set( | |
| 2325 | + ProviderItemId.make(storeWritabilityProbeItemIdFor(row.refresh_item_id)), | |
| 2326 | + STORE_WRITABILITY_PROBE_VALUE, | |
| 2327 | + ); | |
| 2328 | + } | |
| 2329 | + storedRefreshToken = refreshToken; | |
| 2278 | 2330 | return yield* refreshAccessToken({ |
| 2279 | 2331 | tokenUrl, |
| 2280 | 2332 | clientId: clientRow.clientId, |
| @@ -2354,7 +2406,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 73 unmodified lines | ||
| 2354 | 2406 | ); |
| 2355 | 2407 | }); |
| 2356 | 2408 | |
| 2357 | − yield* persistRefreshedToken(row, provider, token); | |
| 2409 | + yield* persistRefreshedToken(row, provider, token, storedRefreshToken); | |
| 2358 | 2410 | return token.access_token; |
| 2359 | 2411 | }).pipe( |
| 2360 | 2412 | // The refresh path was previously invisible to telemetry: no span, no |
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import { describe, expect, it } from "@effect/vitest"; |
| 2 | −import { Effect, Predicate } from "effect"; | |
| 2 | +import { Deferred, Effect, Fiber, Predicate } from "effect"; | |
| 3 | 3 | |
| 4 | 4 | import { |
| 5 | 5 | AuthTemplateSlug, |
| @@ -7,15 +7,18 @@ import { · 1 unmodified line | ||
| 7 | 7 | IntegrationSlug, |
| 8 | 8 | OAuthClientSlug, |
| 9 | 9 | OAuthState, |
| 10 | + ProviderKey, | |
| 10 | 11 | ToolAddress, |
| 11 | 12 | ToolName, |
| 12 | 13 | } from "./ids"; |
| 13 | 14 | import { authToolFailure } from "./auth-tool-failure"; |
| 15 | +import { createExecutor } from "./executor"; | |
| 14 | 16 | import { decodeOAuthCallbackState } from "./oauth"; |
| 15 | 17 | import { OAuthStartError } from "./oauth-client"; |
| 16 | 18 | import { missingGrantedOAuthScopes } from "./oauth-service"; |
| 17 | 19 | import { definePlugin } from "./plugin"; |
| 18 | −import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; | |
| 20 | +import type { CredentialProvider } from "./provider"; | |
| 21 | +import { makeTestConfig, makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; | |
| 19 | 22 | import { ToolResult } from "./tool-result"; |
| 20 | 23 | import { serveOAuthTestServer } from "./testing/oauth-test-server"; |
| 21 | 24 | |
| @@ -884,6 +887,165 @@ describe("oauth token refresh in resolveConnectionValue", () => { · 862 unmodified lines | ||
| 884 | 887 | ), |
| 885 | 888 | ); |
| 886 | 889 | |
| 890 | + // Two product instances, one connection, one credential store. The in-flight | |
| 891 | + // refresh gate serialises refreshes WITHIN an instance and cannot see across | |
| 892 | + // them, so nothing but the store itself stands between two refreshers and | |
| 893 | + // the same rotated token. The provider below opens a seam exactly where the | |
| 894 | + // danger is — between the read of the stored refresh token and whatever the | |
| 895 | + // reader writes next — because a probe that "tests" the store by rewriting | |
| 896 | + // the value it just read would put the spent token back over the peer's | |
| 897 | + // rotated one, and kill the connection it was added to protect. | |
| 898 | + it.effect( | |
| 899 | + "a refresher paused after reading the stored token never writes it back over a peer's rotated one", | |
| 900 | + () => | |
| 901 | + Effect.scoped( | |
| 902 | + Effect.gen(function* () { | |
| 903 | + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); | |
| 904 | + | |
| 905 | + const store = new Map<string, string>(); | |
| 906 | + // Every item id written, in order — the tape that says WHICH item a | |
| 907 | + // refresher touched, which is the whole question here. | |
| 908 | + const writes: string[] = []; | |
| 909 | + const pausedAtRead = yield* Deferred.make<void>(); | |
| 910 | + const resumeFromRead = yield* Deferred.make<void>(); | |
| 911 | + // One-shot: the FIRST read of a refresh token stops there; the | |
| 912 | + // peer's read, moments later, runs straight through. | |
| 913 | + let pauseNextRefreshRead = false; | |
| 914 | + | |
| 915 | + const sharedStore: CredentialProvider = { | |
| 916 | + key: ProviderKey.make("shared-memory"), | |
| 917 | + writable: true, | |
| 918 | + get: (id) => | |
| 919 | + Effect.gen(function* () { | |
| 920 | + const value = store.get(String(id)) ?? null; | |
| 921 | + if (pauseNextRefreshRead && String(id).endsWith(":refresh")) { | |
| 922 | + pauseNextRefreshRead = false; | |
| 923 | + yield* Deferred.succeed(pausedAtRead, undefined); | |
| 924 | + yield* Deferred.await(resumeFromRead); | |
| 925 | + } | |
| 926 | + return value; | |
| 927 | + }), | |
| 928 | + set: (id, value) => | |
| 929 | + Effect.sync(() => { | |
| 930 | + writes.push(String(id)); | |
| 931 | + store.set(String(id), value); | |
| 932 | + }), | |
| 933 | + delete: (id) => Effect.sync(() => void store.delete(String(id))), | |
| 934 | + }; | |
| 935 | + | |
| 936 | + // One database and one credential store, two executors over them — | |
| 937 | + // the deployment this race needs and the one a single harness | |
| 938 | + // cannot express. | |
| 939 | + const config = { | |
| 940 | + ...makeTestConfig({ plugins: [oauthPlugin] as const }), | |
| 941 | + providers: [sharedStore], | |
| 942 | + }; | |
| 943 | + const instanceA = yield* createExecutor(config); | |
| 944 | + const instanceB = yield* createExecutor(config); | |
| 945 | + yield* Effect.addFinalizer(() => | |
| 946 | + Effect.promise(() => config.testDb.close()).pipe(Effect.ignore), | |
| 947 | + ); | |
| 948 | + yield* Effect.addFinalizer(() => instanceA.close().pipe(Effect.ignore)); | |
| 949 | + yield* Effect.addFinalizer(() => instanceB.close().pipe(Effect.ignore)); | |
| 950 | + | |
| 951 | + yield* instanceA.acme.seed(); | |
| 952 | + yield* instanceA.oauth.createClient({ | |
| 953 | + owner: "org", | |
| 954 | + slug: CLIENT, | |
| 955 | + authorizationUrl: server.authorizationEndpoint, | |
| 956 | + tokenUrl: server.tokenEndpoint, | |
| 957 | + grant: "authorization_code", | |
| 958 | + clientId: "test-client", | |
| 959 | + clientSecret: "test-secret", | |
| 960 | + }); | |
| 961 | + const started = yield* instanceA.oauth.start({ | |
| 962 | + owner: "org", | |
| 963 | + client: CLIENT, | |
| 964 | + clientOwner: "org", | |
| 965 | + name: ConnectionName.make("main"), | |
| 966 | + integration: INTEG, | |
| 967 | + template: TEMPLATE, | |
| 968 | + }); | |
| 969 | + expect(started.status).toBe("redirect"); | |
| 970 | + if (started.status !== "redirect") return; | |
| 971 | + const callback = yield* server.completeAuthorizationCodeFlow({ | |
| 972 | + authorizationUrl: started.authorizationUrl, | |
| 973 | + }); | |
| 974 | + yield* instanceA.oauth.complete({ state: started.state, code: callback.code }); | |
| 975 | + | |
| 976 | + const refreshItemId = [...store.keys()].find((key) => key.endsWith(":refresh")); | |
| 977 | + expect(refreshItemId, "the completed connection stored a refresh token").toBeDefined(); | |
| 978 | + const spentRefreshToken = store.get(refreshItemId!); | |
| 979 | + | |
| 980 | + // Expire the access token so both instances must refresh. | |
| 981 | + yield* Effect.promise(() => | |
| 982 | + config.db.updateMany("connection", { | |
| 983 | + where: (b) => b("name", "=", "main"), | |
| 984 | + set: { expires_at: Date.now() - 60_000 }, | |
| 985 | + }), | |
| 986 | + ); | |
| 987 | + | |
| 988 | + // A begins a refresh and stops the instant it has the stored refresh | |
| 989 | + // token in hand. This is the window. | |
| 990 | + pauseNextRefreshRead = true; | |
| 991 | + const refresherA = yield* Effect.forkChild( | |
| 992 | + Effect.exit(instanceA.execute(ToolAddress.make("tools.acme.org.main.whoami"), {})), | |
| 993 | + ); | |
| 994 | + yield* Deferred.await(pausedAtRead); | |
| 995 | + | |
| 996 | + // B refreshes on that same token, to completion. The authorization | |
| 997 | + // server rotates it, so what the store holds afterwards is the only | |
| 998 | + // value left that can ever mint again. | |
| 999 | + yield* instanceB.execute(ToolAddress.make("tools.acme.org.main.whoami"), {}); | |
| 1000 | + const rotatedByB = store.get(refreshItemId!); | |
| 1001 | + expect(rotatedByB, "the peer's refresh rotated the stored token").not.toBe( | |
| 1002 | + spentRefreshToken, | |
| 1003 | + ); | |
| 1004 | + const writesBeforeAResumes = writes.length; | |
| 1005 | + | |
| 1006 | + // A resumes into a world where the token it is holding is already | |
| 1007 | + // spent — and still has to prove the store is writable before it | |
| 1008 | + // tries to spend it. | |
| 1009 | + yield* Deferred.succeed(resumeFromRead, undefined); | |
| 1010 | + yield* Fiber.join(refresherA); | |
| 1011 | + | |
| 1012 | + expect( | |
| 1013 | + store.get(refreshItemId!), | |
| 1014 | + "the peer's rotated refresh token is still what the store holds", | |
| 1015 | + ).toBe(rotatedByB); | |
| 1016 | + expect( | |
| 1017 | + [...store.entries()] | |
| 1018 | + .filter(([, value]) => value === spentRefreshToken) | |
| 1019 | + .map(([key]) => key), | |
| 1020 | + "the spent refresh token was not written back anywhere", | |
| 1021 | + ).toEqual([]); | |
| 1022 | + | |
| 1023 | + // The gate did still run for A — on an item of its own, holding no | |
| 1024 | + // credential. Its grant then failed on the spent token, so it | |
| 1025 | + // persisted nothing: this one write is everything A wrote. | |
| 1026 | + const writtenByA = writes.slice(writesBeforeAResumes); | |
| 1027 | + expect(writtenByA, "the resumed refresher wrote exactly one item").toHaveLength(1); | |
| 1028 | + expect(writtenByA[0], "and it was not the refresh token's own item").not.toBe( | |
| 1029 | + refreshItemId, | |
| 1030 | + ); | |
| 1031 | + expect( | |
| 1032 | + [spentRefreshToken, rotatedByB], | |
| 1033 | + "the item it wrote carries no credential", | |
| 1034 | + ).not.toContain(store.get(writtenByA[0]!)); | |
| 1035 | + | |
| 1036 | + // Both instances really did reach the authorization server, so the | |
| 1037 | + // interleaving under test happened rather than being short-circuited. | |
| 1038 | + expect( | |
| 1039 | + (yield* server.requests).filter( | |
| 1040 | + (request) => | |
| 1041 | + request.path === "/token" && request.body.includes("grant_type=refresh_token"), | |
| 1042 | + ), | |
| 1043 | + "both instances sent a refresh grant", | |
| 1044 | + ).toHaveLength(2); | |
| 1045 | + }), | |
| 1046 | + ), | |
| 1047 | + ); | |
| 1048 | + | |
| 887 | 1049 | it.effect( |
| 888 | 1050 | "refreshes a Personal (user) connection minted through a Workspace (org) app — own→shared client resolution", |
| 889 | 1051 | () => |
| @@ -270,6 +270,34 @@ const accessItemId = (owner: Owner, integration: IntegrationSlug, name: Connecti · 269 unmodified lines | ||
| 270 | 270 | `oauth:${owner}:${integration}:${name}`; |
| 271 | 271 | const refreshItemIdFor = (accessId: string): string => `${accessId}:refresh`; |
| 272 | 272 | |
| 273 | +/** The item a refresh writes to prove the credential store will ACCEPT a write, | |
| 274 | + * before the grant spends the single-use refresh token. It holds no credential | |
| 275 | + * and never has. | |
| 276 | + * | |
| 277 | + * It has to be its own item. The cheaper-looking probe — rewriting the refresh | |
| 278 | + * token with the value just read — is a read-then-write with no | |
| 279 | + * compare-and-set, and two refreshers of one connection on different instances | |
| 280 | + * lose the newer token to it: A reads R0, B consumes R0 and stores the rotated | |
| 281 | + * R1, then A's probe puts R0 back over R1 and the connection is dead the next | |
| 282 | + * time anything needs it. The in-memory single-flight gate spans one instance | |
| 283 | + * only, and a backing store whose own write path is read-latest-then-write | |
| 284 | + * cannot catch it either. | |
| 285 | + * | |
| 286 | + * The id is the refresh item's id plus a fixed suffix, rather than one | |
| 287 | + * rebuilt from the connection's parts, so it carries the same prefix and | |
| 288 | + * therefore the same embedded owner — the same store partition, the same | |
| 289 | + * encryption context, the same object-name head. A store that would refuse | |
| 290 | + * the refresh token's write refuses this one. Per connection rather than one | |
| 291 | + * per partition, so the only writers that can contend on it are the | |
| 292 | + * concurrent refreshers of a single connection, and they all write the same | |
| 293 | + * constant. */ | |
| 294 | +export const storeWritabilityProbeItemIdFor = (refreshItemId: string): string => | |
| 295 | + `${refreshItemId}:store-probe`; | |
| 296 | + | |
| 297 | +/** What the writability probe stores. A constant, because the item exists to | |
| 298 | + * prove a write lands and carries no information of its own. */ | |
| 299 | +export const STORE_WRITABILITY_PROBE_VALUE = "writable"; | |
| 300 | + | |
| 273 | 301 | /** Order-preserving de-duplication of a scope list. */ |
| 274 | 302 | const dedupeScopes = (scopes: readonly string[]): readonly string[] => [...new Set(scopes)]; |
| 275 | 303 | |
| @@ -4,9 +4,14 @@ · 3 unmodified lines | ||
| 4 | 4 | // ids in executor.ts, the oauth-client secret ids): |
| 5 | 5 | // |
| 6 | 6 | // connection:<owner>:<integration>:<name>:<variable> |
| 7 | −// oauth:<owner>:<integration>:<name>[:refresh] | |
| 7 | +// oauth:<owner>:<integration>:<name>[:refresh[:store-probe]] | |
| 8 | 8 | // oauth-client:<owner>:<slug>:secret |
| 9 | 9 | // |
| 10 | +// The `:store-probe` tail is the one id here that holds no credential: a | |
| 11 | +// refresh writes it to prove the store accepts writes before a grant spends | |
| 12 | +// the refresh token. It hangs off the refresh id so it inherits the same | |
| 13 | +// owner, and therefore the same partition, as the credential it stands in for. | |
| 14 | +// | |
| 10 | 15 | // Credential providers file plugin-storage rows by THIS owner, not the acting |
| 11 | 16 | // caller's binding — an org connection whose OAuth consent completes in one |
| 12 | 17 | // member's browser session must produce rows every member can resolve |