diff --git a/.changeset/28326-oauth2-connection-door-hardening.md b/.changeset/28326-oauth2-connection-door-hardening.md new file mode 100644 index 00000000000..89bd54d6d2f --- /dev/null +++ b/.changeset/28326-oauth2-connection-door-hardening.md @@ -0,0 +1,17 @@ +--- +'@objectstack/service-automation': patch +'@objectstack/runtime': patch +--- + +fix(service-automation): harden the OAuth2 connector connection door (ADR-0143 S1 follow-up, #28326) + +Clause-②: no +Metadata-protocol: none + +- **`@objectstack/service-automation`**: + - **The connect that is waiting for its callback is held sealed.** The pending authorization (the PKCE verifier, the user, the organization and the redirect URI) is held in the cluster KV, or in process on a single node, as an AES-256-GCM envelope under a key derived from the raw `state`. The server keeps only the state's digest. A party that can read the KV learns none of those values. A value that is edited, or moved to another state's slot, opens as nothing, and its code is never exchanged. A connect begun before the upgrade and completed after it is refused as expired; begin it again. + - **The connection row is written only over the revision that was read.** A refresh, the "reconnect required" mark and a reconnect are each one conditional write (`where: { id, revision }`), so a stale writer on any replica never overwrites a newer grant. A reconnect that loses such a race is refused `503` and keeps nothing; begin it again. + - **A connection store that cannot be read refuses a connect or a revoke with `503`**, the declared answer, instead of an undeclared `500`. + - **An `oauth2` token endpoint's redirect is refused, never followed.** The token request carries the client secret and the refresh token, and a `307` or `308` would replay them to the `Location` it names. + - **Plain `http://` is accepted only to a loopback host:** `localhost`, a `127.0.0.0/8` address or `[::1]`. A `*.localhost` name is no longer accepted, because the system resolver sends it to DNS like any other name. Every other `tokenUrl` or `authorizationUrl` must be `https://`, or the instance is held degraded with the field named. +- **`@objectstack/runtime`**: the route ledger row for `DELETE /api/v1/automation/connectors/:name/connection` now says that a revoke is local only. No RFC 7009 revocation is sent to the vendor yet, because no revocation endpoint is known for the instance until ADR-0143 S2 discovery. The refresh token stays valid at the vendor until it expires or is revoked there. diff --git a/content/docs/automation/connectors.mdx b/content/docs/automation/connectors.mdx index 49f31da03d5..3f44bb67234 100644 --- a/content/docs/automation/connectors.mdx +++ b/content/docs/automation/connectors.mdx @@ -495,7 +495,15 @@ a rotating refresh token is never spent twice. (`not_connected`, `connected`, `reconnect_required`), never a token. The `state` and the PKCE verifier are bound to the user and organization that began the connect and are used once; a callback from anyone else is refused - before its code is exchanged. The redirect URI is the deployment's + before its code is exchanged. While the connect waits (ten minutes at most) + it is held in the cluster KV, or in process on a single node, **sealed** + under a key derived from the `state` itself, which the server never stores: + the KV holds only the state's digest and an AES-256-GCM envelope, so a party + that can read the KV learns no verifier, user or organization, and a value + edited or moved there opens as nothing and its code is never exchanged. + Write access to the cluster KV stays inside the platform's trust boundary, + as it is for the cluster lock: such a writer can still delete a waiting + connect. The redirect URI is the deployment's (`OS_BASE_URL` or `OS_AUTH_URL` plus `/api/v1/automation/connectors/oauth/callback`, or the automation plugin's `connectorOAuthRedirectUri`), never the request's; register that URI with the vendor. The refresh token is kept @@ -505,9 +513,15 @@ a rotating refresh token is never spent twice. instance's only connection, and is refused when there are several. When the vendor refuses the grant (`invalid_grant`), the connection is marked **reconnect required** and the instance degraded until an administrator - connects it again. A revoke deletes the stored connection only: the server - does not yet revoke the refresh token at the vendor (RFC 7009), so it stays - valid there until it expires or is revoked in the vendor's console. + connects it again. A refresh, and the mark "reconnect required", are each + written only over the revision they read, in one conditional write, so a + stale one never overwrites a newer connect on any replica. A revoke deletes + the stored connection only: the server does not revoke the refresh token at + the vendor (RFC 7009), because no revocation endpoint is known for the + instance yet — the auth declares none, and the discovery that would supply + one is ADR-0143 S2. The refresh token stays valid at the vendor until it + expires or is revoked in the vendor's console; revoke it there too when the + disconnect is a response to a leak. What is not served yet is held **degraded** with a reason that names it, and never called without its credential: `app-credential` (its vendor exchanges @@ -515,8 +529,11 @@ are not run yet), an `oauth2` instance on a provider that does not yet ask the credential handle (the built-in `rest`, `openapi` and `mcp` providers do not yet), an `mcp` instance that left its endpoints to discovery, and an `oauth2` instance whose `tokenUrl` or `authorizationUrl` is not `https://` (plain -`http://` is accepted only to a loopback host, so the client secret and the -refresh token never cross a network in the clear). A datasource +`http://` is accepted only to `localhost`, a `127.0.0.0/8` address or +`[::1]` — not a `*.localhost` name, which the system resolver sends to DNS — +so the client secret and the refresh token never cross a network in the +clear; a token endpoint that answers with a redirect is refused rather than +followed). A datasource that borrows from a grant-bearing instance (`external.connector`) is refused with `422`. Until then, declare a static kind (`bearer`, `api-key` or `basic`) to dispatch now; don't emulate a grant by putting a short-lived access token diff --git a/content/docs/permissions/tenant-audit-census.mdx b/content/docs/permissions/tenant-audit-census.mdx index ec46df82faa..df56c813730 100644 --- a/content/docs/permissions/tenant-audit-census.mdx +++ b/content/docs/permissions/tenant-audit-census.mdx @@ -93,7 +93,7 @@ what moved this page's population from 225 to 227; nothing about the two sites changed, only whether this instrument could see them. **The expensive failure direction is a keyword.** Sites whose receiver the author -typed `any` have no type to read, and there are 63 of them — just over a fifth +typed `any` have no type to read, and there are 62 of them — just over a fifth of the population, concentrated in exactly the seed and bootstrap paths this control exists for. Scoring an unreadable receiver as "not an engine" would have dropped every one of them silently, with a clean exit and a smaller number that @@ -132,7 +132,7 @@ are reported as `undecidable` rather than assumed either way. The same holds twice over for the context. An options argument spelled as a literal can be read; one spelled `options`, `{ ...opts }`, or handed through a -forwarding shim cannot, and **51 of the 257 sites are spelled that way**. A +forwarding shim cannot, and **51 of the 255 sites are spelled that way**. A context resolved from an inline literal or a local `const` can be tested for `isSystem`; one arriving from a helper call cannot. @@ -197,10 +197,10 @@ reproduce them. Where it disagrees, it disagrees on the page: | carried figure | where it survives | this census | | :--- | :--- | ---: | -| 175 write call sites | quoted in the merged changeset | **257** | +| 175 write call sites | quoted in the merged changeset | **255** | | 24 carrying no tenant context | quoted in the merged changeset | **2** provable and tenancy-enabled; **29** more whose options argument is unreadable | -| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **179 of 257** decidable, **78** undecidable | -| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 147 decidably elevated, 0 decidably not, 102 undecidable | +| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **177 of 255** decidable, **78** undecidable | +| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 145 decidably elevated, 0 decidably not, 102 undecidable | | 141 and 132, two independent re-derivations | the card that filed this work | — | **The differences are not reconciled, and deliberately so.** The old census's @@ -210,18 +210,18 @@ be stated is what this instrument counts, which is written above and re-runnable at any commit. Two structural facts do plausibly widen this reading against any hand or regex -one, and both are counted in the generated tables below: the 63 sites reached -through an erased (`any`) receiver, and the 63 that name their object through a +one, and both are counted in the generated tables below: the 62 sites reached +through an erased (`any`) receiver, and the 61 that name their object through a `const` rather than inline. An instrument that read either the way a person does would report a smaller number and would not say so. The fourth row is the one worth flagging to anyone citing it. **The 135 / 77% figure has no surviving corroboration anywhere in the tree.** This census reads -147 of 257 (57%) as decidably elevated, with 102 more whose elevation is a +145 of 255 (57%) as decidably elevated, with 102 more whose elevation is a run-time fact — so the claim is neither confirmed nor refuted, and the honest answer is that a static reading cannot settle it. -⇒ **Cite `2 / 257`, and say what it is**: the sites whose options argument was +⇒ **Cite `2 / 255`, and say what it is**: the sites whose options argument was READ and holds no tenant context, against a decidably tenancy-enabled object. That is the control's provable yield surface. ⛔ Do not cite it as "the sites without tenant context" — **29 further sites** have an options argument this @@ -233,29 +233,29 @@ cannot read, and they are neither in nor out. | what | count | | :--- | ---: | -| write call sites on the application surface | **257** | -| …whose object name is statically decidable | 179 | +| write call sites on the application surface | **255** | +| …whose object name is statically decidable | 177 | | …whose object name is chosen at run time | 78 | -| …against an object with tenancy ENABLED | 178 | +| …against an object with tenancy ENABLED | 176 | | …against an object that declares tenancy off | 1 | -| threading a tenant context | 198 | +| threading a tenant context | 196 | | PROVABLY carrying none (options read, no context key) | **8** | | …of those, against a decidably tenancy-enabled object | **2** | | options argument UNREADABLE — may or may not carry one | 51 | | …of those, against a decidably tenancy-enabled object | 29 | -| threading a decidably ELEVATED (`isSystem`) context | 147 | +| threading a decidably ELEVATED (`isSystem`) context | 145 | | threading a context that is decidably NOT elevated | 0 | | threading a context whose elevation is a run-time fact | 102 | | how the instrument reached the site | count | | :--- | ---: | -| receiver carried a readable engine type | 194 | -| receiver erased, placed by the object NAME | 53 | +| receiver carried a readable engine type | 193 | +| receiver erased, placed by the object NAME | 52 | | receiver erased, placed by an `object: string` PARAMETER | 5 | | receiver erased, placed by an `UNTYPED_RECEIVERS` row | 5 | | object name spelled inline | 116 | -| object name spelled through a `const` | 63 | +| object name spelled through a `const` | 61 | | object name is an `object: string` parameter | 7 | | object name is some other run-time expression | 71 | @@ -307,13 +307,13 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-10-31 at `30dc6587c`. +Measured on 2026-10-31 at `d06a02507`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 829 | +| tracked non-test sources scanned | 830 | | engine-shaped types recognised | 96 | | declared objects in the registry | 124 | -| same-named calls subtracted as non-engine | 212 | +| same-named calls subtracted as non-engine | 214 | {/* END GENERATED: tenant-audit-census */} diff --git a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md index 50380aab887..20c7b3dcb8d 100644 --- a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md +++ b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md @@ -33,17 +33,17 @@ silent, and `node scripts/tenant-audit-census.mjs --write` is the resolution. | Measure | Value | |---|---:| -| Write call sites | 257 | -| Object name statically decidable | 179 | +| Write call sites | 255 | +| Object name statically decidable | 177 | | Object name chosen at run time | 78 | -| Against a tenancy-enabled object | 178 | +| Against a tenancy-enabled object | 176 | | Against an object declaring tenancy off | 1 | -| Threading a tenant context | 198 | +| Threading a tenant context | 196 | | Provably carrying none | 8 | | …and decidably tenancy-enabled | 2 | | Options argument unreadable | 51 | | …and decidably tenancy-enabled | 29 | -| Threading a decidably elevated context | 147 | +| Threading a decidably elevated context | 145 | | Threading a decidably non-elevated context | 0 | | Threading a context of undecidable elevation | 102 | @@ -90,14 +90,14 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-10-31 at `30dc6587c`. +Measured on 2026-10-31 at `d06a02507`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 829 | +| tracked non-test sources scanned | 830 | | engine-shaped types recognised | 96 | | declared objects in the registry | 124 | -| same-named calls subtracted as non-engine | 212 | +| same-named calls subtracted as non-engine | 214 | ## Every site @@ -205,7 +205,6 @@ Measured on 2026-10-31 at `30dc6587c`. | `packages/services/service-automation/src/connector-connections.ts` | `delete` | `sys_connector_connection` | enabled | context, elevation undecidable | 1 | | `packages/services/service-automation/src/connector-connections.ts` | `insert` | `sys_connector_connection` | enabled | context, elevation undecidable | 1 | | `packages/services/service-automation/src/connector-connections.ts` | `update` | `sys_connector_connection` | enabled | context, elevation undecidable | 1 | -| `packages/services/service-automation/src/connector-connections.ts` | `update` | `sys_connector_connection` | enabled | elevated | 2 | | `packages/services/service-automation/src/connector-credentials.ts` | `delete` | `sys_connector_credential` | enabled | context, elevation undecidable | 1 | | `packages/services/service-automation/src/connector-credentials.ts` | `insert` | `sys_connector_credential` | enabled | context, elevation undecidable | 1 | | `packages/services/service-automation/src/connector-credentials.ts` | `update` | `sys_connector_credential` | enabled | context, elevation undecidable | 1 | diff --git a/packages/runtime/src/route-ledger.ts b/packages/runtime/src/route-ledger.ts index 7d4773ceb9c..9dc54997b06 100644 --- a/packages/runtime/src/route-ledger.ts +++ b/packages/runtime/src/route-ledger.ts @@ -548,9 +548,9 @@ export const ROUTE_LEDGER: readonly RouteLedgerEntry[] = [ { route: 'GET /automation/connectors/:name/connection', domain: '/automation', disposition: 'server-only', note: "The caller's organization's connection of one declared `oauth2` `authorization_code` instance: `status` (`not_connected` / `connected` / `reconnect_required`), its reason, the granted scopes, the access token's expiry and who connected it. ⛔ Never a token. Refused: an undeclared instance 404 `RESOURCE_NOT_FOUND`; one whose auth is not that grant, or a caller with no organization, 400 `VALIDATION_ERROR`. ⚑ `manage_metadata`, fail-closed, only `isSystem` bypassing; no walled refusal, since a connection belongs to its organization. Closed query set: none. NOT JS-SDK surface on this leg: the driver is the Console's Connect / Reconnect flow (ADR-0143 S4). Pinned in `domains/automation-connector-connection-door.test.ts`" }, { route: 'POST /automation/connectors/:name/connection', domain: '/automation', disposition: 'server-only', - note: "Begin connecting (or reconnecting) the instance for the caller's organization: answers `{ authorizationUrl, expiresAt }`, the vendor URL carrying a fresh `state` and an `S256` PKCE challenge. Nothing is written to the connection record until the callback completes; the pending authorization (verifier included) waits ten minutes in the cluster KV, or in process on a single node. The redirect URI is the deployment's (`OS_AUTH_URL` / `OS_BASE_URL`, or the plugin option), never the request's — 503 `SERVICE_UNAVAILABLE` when there is none. No body. Same refusals, gates and SDK posture as the GET row above. Pinned in `domains/automation-connector-connection-door.test.ts`" }, + note: "Begin connecting (or reconnecting) the instance for the caller's organization: answers `{ authorizationUrl, expiresAt }`, the vendor URL carrying a fresh `state` and an `S256` PKCE challenge. Nothing is written to the connection record until the callback completes; the pending authorization (verifier included) waits ten minutes in the cluster KV, or in process on a single node, sealed (AES-256-GCM) under a key derived from the raw `state`, which the server keeps only as a digest. The redirect URI is the deployment's (`OS_AUTH_URL` / `OS_BASE_URL`, or the plugin option), never the request's — 503 `SERVICE_UNAVAILABLE` when there is none. No body. Same refusals, gates and SDK posture as the GET row above. Pinned in `domains/automation-connector-connection-door.test.ts`" }, { route: 'DELETE /automation/connectors/:name/connection', domain: '/automation', disposition: 'server-only', - note: "Revoke the caller's organization's connection: the row is removed, the cached access token dropped, and the instance re-judged — its next dispatch there is refused 'not connected'. 404 `RESOURCE_NOT_FOUND` when it was not connected. Same gates and SDK posture as the GET row above. Pinned in `domains/automation-connector-connection-door.test.ts`" }, + note: "Revoke the caller's organization's connection: the row is removed, the cached access token dropped, and the instance re-judged — its next dispatch there is refused 'not connected'. ⚑ Local only: no RFC 7009 revocation is sent to the vendor, because no revocation endpoint is known for the instance (the auth declares none; ADR-0143 S2 discovery would supply one), so the refresh token stays valid at the vendor until it expires or is revoked there. 404 `RESOURCE_NOT_FOUND` when it was not connected; 503 `SERVICE_UNAVAILABLE` when the connection store cannot be read. Same gates and SDK posture as the GET row above. Pinned in `domains/automation-connector-connection-door.test.ts`" }, { route: 'GET /automation/connectors/oauth/callback', domain: '/automation', disposition: 'server-only', note: "The authorization server's redirect (ADR-0143 D5). Closed query set: `code`, `state`, `error`, `error_description`, `error_uri`, `iss`; an unknown or repeated one is 400 `VALIDATION_ERROR`. The `state` is taken once and must have been begun by the same user in the same organization, else 400 before any code is exchanged; an `error` answer is 400 naming its registered code only. The code is exchanged with the PKCE verifier and the connection stored; answers the connection's status. 503 when the token endpoint cannot be reached. Same capability gate as the rows above: the browser landing here is the administrator's. Pinned in `domains/automation-connector-connection-door.test.ts`" }, { route: 'GET /automation/_status', domain: '/automation', disposition: 'sdk', client: 'automation.getRuntimeStatus' }, diff --git a/packages/services/service-automation/src/connector-connections.test.ts b/packages/services/service-automation/src/connector-connections.test.ts new file mode 100644 index 00000000000..e15818d3eb3 --- /dev/null +++ b/packages/services/service-automation/src/connector-connections.test.ts @@ -0,0 +1,207 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ADR-0143 D3 / D4 — the `sys_connector_connection` store over a real ObjectQL +// engine (SQLite, a test crypto provider): a refresh and the "reconnect +// required" mark are written only over the revision they read, by the write +// itself; a store that cannot be read refuses a connect and a revoke with the +// declared 503. The values below are obvious dummies. + +import { afterEach, describe, expect, it } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysSecret } from '@objectstack/platform-objects'; +import type { CryptoContext, CryptoHandle, ICryptoProvider } from '@objectstack/spec/contracts'; + +import { + CONNECTOR_CONNECTION_OBJECT, + ConnectorConnectionStore, + type ConnectorConnectionRecord, +} from './connector-connections.js'; +import type { ConnectorCredentialEngine } from './connector-credentials.js'; +import { SysConnectorConnection } from './sys-connector-connection.object.js'; + +const SYSTEM = { isSystem: true } as const; + +function fakeCrypto(): ICryptoProvider { + let n = 0; + return { + async encrypt(plain: string, _ctx: CryptoContext): Promise { + n += 1; + return { id: `sec_${n}`, kmsKeyId: 'test', alg: 'test-rev', version: 1, ciphertext: [...plain].reverse().join('') }; + }, + async decrypt(handle: CryptoHandle): Promise { + return [...handle.ciphertext].reverse().join(''); + }, + async rotateKey(handle: CryptoHandle): Promise { + return { ...handle, version: handle.version + 1 }; + }, + digest: (plain: string) => `d:${plain.length}`, + keyedDigest: async (plain: string) => `k:${plain.length}`, + }; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + for (const e of engines.splice(0)) { + try { await (e as any).destroy?.(); } catch { /* noop */ } + } +}); + +async function dataEngine(): Promise { + const ql = new ObjectQL(); + engines.push(ql); + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await driver.connect(); + ql.registerDriver(driver, true); + await ql.init(); + for (const object of [SysSecret, SysConnectorConnection]) (ql as any).registry.registerObject(object as never, 'test'); + await ql.syncSchemas(); + ql.setCryptoProvider(fakeCrypto()); + return ql; +} + +/** + * The engine as the store sees it, with `beforeUpdate` run between the store's + * call and the real write — another replica's write landing in that gap. + */ +function interleaved(ql: ObjectQL, beforeUpdate: () => Promise): ConnectorCredentialEngine { + let armed = true; + return { + find: (o, q) => ql.find(o, q as never), + insert: (o, d, opts) => ql.insert(o, d, opts as never), + delete: (o, opts) => { + assertEngineDeleteDispatch(opts as never); + return ql.delete(o, opts as never); + }, + resolveSecretField: (o, id, f) => (ql as any).resolveSecretField(o, id, f), + async update(o, d, opts) { + assertEngineUpdateDispatch(d as never, opts as never); + if (armed) { + armed = false; + await beforeUpdate(); + } + return ql.update(o, d, opts as never); + }, + }; +} + +async function connected(ql: ObjectQL): Promise<{ store: ConnectorConnectionStore; record: ConnectorConnectionRecord }> { + const store = new ConnectorConnectionStore(() => ql as unknown as ConnectorCredentialEngine); + const record = await store.connect({ organizationId: 'org_a', connectorName: 'crm', userId: 'usr_admin', refreshToken: 'dummy-rt-1' }); + return { store, record }; +} + +/** Another replica's reconnect: a fresh grant over the same row, revision raised. */ +async function reconnectElsewhere(ql: ObjectQL, record: ConnectorConnectionRecord): Promise { + await ql.update( + CONNECTOR_CONNECTION_OBJECT, + { id: record.id, refresh_token: 'dummy-rt-elsewhere', status: 'connected', status_reason: null, revision: record.revision + 1 }, + { context: SYSTEM } as never, + ); +} + +describe('ADR-0143 D4 — a refresh is written only over the revision it read', () => { + it('writes a rotated token and raises the revision when the row has not moved', async () => { + const ql = await dataEngine(); + const { store, record } = await connected(ql); + const expiresAt = new Date('2030-01-01T00:00:00.000Z'); + expect(await store.recordRefresh(record, { refreshToken: 'dummy-rt-2', scopes: 'crm.read', expiresAt })).toBe(true); + const now = (await store.read('org_a', 'crm'))!; + expect(now).toMatchObject({ revision: record.revision + 1, scopes: 'crm.read', expiresAt: expiresAt.toISOString() }); + expect(await store.resolveRefreshToken(now)).toBe('dummy-rt-2'); + }); + + it('a refresh that rotates nothing keeps the token and the revision, and still writes the expiry', async () => { + const ql = await dataEngine(); + const { store, record } = await connected(ql); + expect(await store.recordRefresh(record, { expiresAt: new Date('2030-01-01T00:00:00.000Z') })).toBe(true); + const now = (await store.read('org_a', 'crm'))!; + expect(now.revision).toBe(record.revision); + expect(now.expiresAt).toBe('2030-01-01T00:00:00.000Z'); + expect(await store.resolveRefreshToken(now)).toBe('dummy-rt-1'); + }); + + it('⛔ a reconnect landing between the refresh and its write is never overwritten by the stale rotation', async () => { + const ql = await dataEngine(); + const { record } = await connected(ql); + const store = new ConnectorConnectionStore(() => interleaved(ql, () => reconnectElsewhere(ql, record))); + expect(await store.recordRefresh(record, { refreshToken: 'dummy-rt-stale' })).toBe(false); + const now = (await store.read('org_a', 'crm'))!; + expect(now.revision).toBe(record.revision + 1); + expect(await store.resolveRefreshToken(now)).toBe('dummy-rt-elsewhere'); + }); + + it('⛔ a row revoked since the refresh read it is not written', async () => { + const ql = await dataEngine(); + const { store, record } = await connected(ql); + expect(await store.revoke('org_a', 'crm')).toBe(true); + expect(await store.recordRefresh(record, { refreshToken: 'dummy-rt-stale' })).toBe(false); + expect(await store.read('org_a', 'crm')).toBeUndefined(); + }); +}); + +describe('ADR-0143 D4 — "reconnect required" is written only over the revision that was refused', () => { + it('marks the refused grant and drops its token when the row has not moved', async () => { + const ql = await dataEngine(); + const { store, record } = await connected(ql); + await store.markReconnectRequired(record, 'the vendor refused the grant'); + const now = (await store.read('org_a', 'crm'))!; + expect(now).toMatchObject({ status: 'reconnect_required', statusReason: 'the vendor refused the grant', hasRefreshToken: false }); + }); + + it('⛔ a reconnect landing between the refusal and its write keeps its fresh grant', async () => { + const ql = await dataEngine(); + const { record } = await connected(ql); + const store = new ConnectorConnectionStore(() => interleaved(ql, () => reconnectElsewhere(ql, record))); + await store.markReconnectRequired(record, 'the vendor refused the grant'); + const now = (await store.read('org_a', 'crm'))!; + expect(now).toMatchObject({ status: 'connected', revision: record.revision + 1, hasRefreshToken: true }); + expect(await store.resolveRefreshToken(now)).toBe('dummy-rt-elsewhere'); + }); +}); + +describe('ADR-0143 D5 — a reconnect is written only over the revision it read', () => { + it('replaces the grant and raises the revision', async () => { + const ql = await dataEngine(); + const { store, record } = await connected(ql); + const again = await store.connect({ organizationId: 'org_a', connectorName: 'crm', refreshToken: 'dummy-rt-again' }); + expect(again.id).toBe(record.id); + expect(again.revision).toBe(record.revision + 1); + expect(await store.resolveRefreshToken(again)).toBe('dummy-rt-again'); + }); + + it('⛔ a reconnect racing another one refuses 503 and keeps nothing of its own', async () => { + const ql = await dataEngine(); + const { record } = await connected(ql); + const store = new ConnectorConnectionStore(() => interleaved(ql, () => reconnectElsewhere(ql, record))); + await expect(store.connect({ organizationId: 'org_a', connectorName: 'crm', refreshToken: 'dummy-rt-loser' })) + .rejects.toMatchObject({ name: 'ConnectorCredentialUnavailableError', status: 503 }); + const now = (await store.read('org_a', 'crm'))!; + expect(await store.resolveRefreshToken(now)).toBe('dummy-rt-elsewhere'); + }); +}); + +describe('ADR-0143 D5 — a connection store that cannot be read refuses 503, never 500', () => { + const unreadable = (): ConnectorCredentialEngine => ({ + find: async () => { throw new Error('dummy driver failure naming row 42'); }, + insert: async () => { throw new Error('not reached'); }, + update: async (_o, d, opts) => { + assertEngineUpdateDispatch(d as never, opts as never); + throw new Error('not reached'); + }, + delete: async (_o, opts) => { + assertEngineDeleteDispatch(opts as never); + throw new Error('not reached'); + }, + }); + + it.each([ + ['connect', (s: ConnectorConnectionStore) => s.connect({ organizationId: 'org_a', connectorName: 'crm', refreshToken: 'dummy-rt' })], + ['revoke', (s: ConnectorConnectionStore) => s.revoke('org_a', 'crm')], + ] as const)('⛔ %s', async (_verb, act) => { + const err = await act(new ConnectorConnectionStore(unreadable)).catch((e: unknown) => e); + expect(err).toMatchObject({ name: 'ConnectorCredentialUnavailableError', status: 503 }); + expect(String((err as Error).message)).not.toContain('row 42'); + }); +}); diff --git a/packages/services/service-automation/src/connector-connections.ts b/packages/services/service-automation/src/connector-connections.ts index 70a97903571..06c4978a08f 100644 --- a/packages/services/service-automation/src/connector-connections.ts +++ b/packages/services/service-automation/src/connector-connections.ts @@ -89,6 +89,16 @@ export interface ConnectorGrantWrite { expiresAt?: Date; } +/** + * How many rows a predicate write reports it changed. The engine answers a + * `multi` update with the affected-row count; anything else (a driver that + * returns no count) is read as "none" — the revision guard fails closed. + */ +function affectedRows(result: unknown): number { + const count = typeof result === 'number' ? result : Number((result as { count?: unknown } | null)?.count); + return Number.isFinite(count) ? count : 0; +} + /** * The `sys_connector_connection` store. One per automation plugin. It never * caches a token: {@link resolveRefreshToken} reads the row at the moment of use. @@ -139,6 +149,28 @@ export class ConnectorConnectionStore { return undefined; } + /** + * Write `fields` over the row `id` only while it still carries `revision`: + * one conditional statement, `where: { id, revision }`, so the revision + * check and the write cannot be split by another writer on any replica. + * `true` when the row was written; `false`, with nothing written, when the + * row moved (or is gone) since `revision` was read. + */ + private async updateAtRevision( + engine: ConnectorCredentialEngine, + id: string, + revision: number, + fields: Record, + context: Record, + ): Promise { + const result = await engine.update(CONNECTOR_CONNECTION_OBJECT, fields, { + where: { id, revision }, + multi: true, + context, + }); + return affectedRows(result) > 0; + } + /** Every organization's row for one instance. */ async listFor(connectorName: string): Promise { const engine = this.resolveEngine(); @@ -186,7 +218,7 @@ export class ConnectorConnectionStore { */ async connect(args: { organizationId: string; connectorName: string; userId?: string } & ConnectorGrantWrite): Promise { const engine = this.engineOrThrow(`Connector instance '${args.connectorName}' was not connected`); - const existing = await this.read(args.organizationId, args.connectorName); + const existing = await this.readOrRefuse(args.organizationId, args.connectorName); const context = { ...SYSTEM_CONTEXT, ...(args.userId ? { userId: args.userId } : {}) }; const row = { grant: 'authorization_code', @@ -199,9 +231,10 @@ export class ConnectorConnectionStore { connected_at: new Date().toISOString(), connected_by: args.userId ?? null, }; + let moved = false; try { if (existing) { - await engine.update(CONNECTOR_CONNECTION_OBJECT, { id: existing.id, ...row }, { context }); + moved = !(await this.updateAtRevision(engine, existing.id, existing.revision, row, context)); } else { await engine.insert( CONNECTOR_CONNECTION_OBJECT, @@ -221,7 +254,13 @@ export class ConnectorConnectionStore { `Connector instance '${args.connectorName}' was not connected: the connection store refused the write. Nothing was kept.`, ); } - const stored = await this.read(args.organizationId, args.connectorName); + if (moved) { + throw new ConnectorCredentialUnavailableError( + `Connector instance '${args.connectorName}' was not connected: its connection changed while this connect was ` + + 'completing (another connect, a refresh or a revoke landed first). Nothing was kept; begin the connect again.', + ); + } + const stored = await this.readOrRefuse(args.organizationId, args.connectorName); if (!stored) { throw new ConnectorCredentialUnavailableError( `Connector instance '${args.connectorName}' was not connected: the connection did not read back after its write.`, @@ -235,47 +274,49 @@ export class ConnectorConnectionStore { * (D2.1, atomically), the new expiry and scopes. Written only over the * revision `record` carries — `false`, with nothing written, when another * writer moved the row since (a reconnect, a revoke, a refresh on another - * replica). The caller runs this inside the connection's refresh lock. + * replica). The revision is checked by the write itself + * ({@link updateAtRevision}), not by a read before it; the caller's + * refresh lock narrows the race, it does not carry the invariant. */ async recordRefresh(record: ConnectorConnectionRecord, write: ConnectorGrantWrite): Promise { const engine = this.engineOrThrow(`The connection of connector instance '${record.connectorName}' was not renewed`); - const current = await this.read(record.organizationId, record.connectorName); - if (!current || current.revision !== record.revision) return false; const rotated = write.refreshToken !== undefined; - await engine.update( - CONNECTOR_CONNECTION_OBJECT, + return this.updateAtRevision( + engine, + record.id, + record.revision, { - id: current.id, - ...(rotated ? { [CONNECTOR_CONNECTION_REFRESH_TOKEN_FIELD]: write.refreshToken, revision: current.revision + 1 } : {}), + ...(rotated ? { [CONNECTOR_CONNECTION_REFRESH_TOKEN_FIELD]: write.refreshToken, revision: record.revision + 1 } : {}), ...(write.scopes !== undefined ? { scopes: write.scopes } : {}), expires_at: write.expiresAt ? write.expiresAt.toISOString() : null, }, - { context: SYSTEM_CONTEXT }, + SYSTEM_CONTEXT, ); - return true; } /** * Record that the authorization server refused the grant `record` held * (revoked, or expired with no usable refresh). `reason` is the platform's - * sentence, never the server's text. Left alone when the row moved since. + * sentence, never the server's text. Left alone when the row moved since + * — checked by the write itself ({@link updateAtRevision}): a reconnect + * that landed between the refusal and this write keeps its fresh grant. */ async markReconnectRequired(record: ConnectorConnectionRecord, reason: string): Promise { const engine = this.resolveEngine(); if (!engine) return; - const current = await this.read(record.organizationId, record.connectorName); - if (!current || current.revision !== record.revision) return; - await engine.update( - CONNECTOR_CONNECTION_OBJECT, - { id: current.id, status: 'reconnect_required', status_reason: reason, [CONNECTOR_CONNECTION_REFRESH_TOKEN_FIELD]: null }, - { context: SYSTEM_CONTEXT }, + await this.updateAtRevision( + engine, + record.id, + record.revision, + { status: 'reconnect_required', status_reason: reason, [CONNECTOR_CONNECTION_REFRESH_TOKEN_FIELD]: null }, + SYSTEM_CONTEXT, ); } /** Remove the connection of one instance in one organization. `false` when it was not connected there. */ async revoke(organizationId: string, connectorName: string, userId?: string): Promise { const engine = this.engineOrThrow(`The connection of connector instance '${connectorName}' was not revoked`); - const existing = await this.read(organizationId, connectorName); + const existing = await this.readOrRefuse(organizationId, connectorName); if (!existing) return false; const context = { ...SYSTEM_CONTEXT, ...(userId ? { userId } : {}) }; await engine.delete(CONNECTOR_CONNECTION_OBJECT, { where: { id: existing.id }, context }); diff --git a/packages/services/service-automation/src/oauth2-grant/authorization.test.ts b/packages/services/service-automation/src/oauth2-grant/authorization.test.ts index 3b213d35304..60c4a2ec04f 100644 --- a/packages/services/service-automation/src/oauth2-grant/authorization.test.ts +++ b/packages/services/service-automation/src/oauth2-grant/authorization.test.ts @@ -13,6 +13,8 @@ import { createPkcePair, kvPendingAuthorizationStore, memoryPendingAuthorizationStore, + openPendingAuthorization, + sealPendingAuthorization, type PendingAuthorization, } from './authorization.js'; @@ -122,3 +124,58 @@ describe('ADR-0143 D5 — a begun connect is taken once', () => { expect(await store.take('st')).toBeUndefined(); }); }); + +describe('ADR-0143 D5 — a begun connect is held sealed (the KV trust boundary)', () => { + const begun = pending({ verifier: 'dummy-verifier-7Qx', redirectUri: 'https://app.example.com/cb-sealed' }); + const keyOf = (state: string) => `connector-oauth2:pending:${createHash('sha256').update(state).digest('hex')}`; + + it('the KV holds no verifier, user, organization, instance or redirect URI — only an envelope', async () => { + const { kv, held } = fakeKv(); + await kvPendingAuthorizationStore(kv).put('st-sealed', begun); + const [, entry] = [...held][0]; + const stored = JSON.stringify(entry.value); + for (const plain of [begun.verifier, begun.userId, begun.organizationId, begun.connector, begun.redirectUri, 'st-sealed']) { + expect(stored).not.toContain(plain); + } + expect(Object.keys(entry.value as object).sort()).toEqual(['ct', 'iv', 'tag', 'v']); + }); + + it('an envelope edited in the KV opens as nothing, so its code is never exchanged', async () => { + const { kv, held } = fakeKv(); + const store = kvPendingAuthorizationStore(kv); + await store.put('st-edit', begun); + const [key, entry] = [...held][0]; + const env = entry.value as { ct: string }; + const ct = Buffer.from(env.ct, 'base64url'); + ct[0] ^= 0x01; + held.set(key, { ...entry, value: { ...env, ct: ct.toString('base64url') } }); + expect(await store.take('st-edit')).toBeUndefined(); + }); + + it('an envelope moved to another state\'s slot opens as nothing there', async () => { + const { kv, held } = fakeKv(); + const store = kvPendingAuthorizationStore(kv); + await store.put('st-victim', begun); + const [, entry] = [...held][0]; + await kv.set(keyOf('st-other'), entry.value); + expect(await store.take('st-other')).toBeUndefined(); + // The slot the envelope was sealed for still opens. + expect(await store.take('st-victim')).toMatchObject({ verifier: begun.verifier }); + }); + + it('a plain value written into the KV is not read as a begun connect', async () => { + const { kv } = fakeKv(); + await kv.set(keyOf('st-plain'), begun); + expect(await kvPendingAuthorizationStore(kv).take('st-plain')).toBeUndefined(); + }); + + it('the seal opens only under the state it was sealed with; the memory store holds the same envelope', async () => { + const sealed = sealPendingAuthorization('st-a', begun); + expect(openPendingAuthorization('st-a', sealed)).toEqual(begun); + expect(openPendingAuthorization('st-b', sealed)).toBeUndefined(); + expect(openPendingAuthorization('st-a', { ...sealed, v: 2 })).toBeUndefined(); + const memory = memoryPendingAuthorizationStore(); + await memory.put('st-m', begun); + expect(await memory.take('st-m')).toEqual(begun); + }); +}); diff --git a/packages/services/service-automation/src/oauth2-grant/authorization.ts b/packages/services/service-automation/src/oauth2-grant/authorization.ts index 3bd6fd9553e..80c6eda22dc 100644 --- a/packages/services/service-automation/src/oauth2-grant/authorization.ts +++ b/packages/services/service-automation/src/oauth2-grant/authorization.ts @@ -21,6 +21,32 @@ * otherwise (a single-node deployment). The KV key is the SHA-256 of the * state, so the raw state is never a key anyone can list. * + * **What is held there is sealed** (AES-256-GCM), under a key derived from the + * raw state by HKDF-SHA-256 — and the raw state is held nowhere on the server: + * it lives in the administrator's browser, round-trip through the vendor, and + * the server keeps only its digest. So the KV trust boundary is: + * + * - a party that can READ the KV (a shared Redis, its backups, a replica's + * memory dump) learns no verifier, user, organization or redirect URI — + * only an opaque envelope under a digest it cannot invert; + * - a party that can WRITE the KV cannot alter or re-home an entry it did + * not seal: the envelope authenticates under its own state's key, and its + * AAD names the slot it was written to, so an edited, swapped or moved + * value opens as nothing ("no connect waiting") and its code is never + * exchanged; + * - what a KV writer CAN still do is delete an entry (a connect that has to + * be begun again) or plant one under a state it chose itself. Write access + * to the cluster KV is inside the platform's trust boundary — the same KV + * carries the cluster lock the refresh single-flight relies on — and the + * callback still answers only the signed-in user the planted entry names. + * + * Why not the host `ICryptoProvider`: its contexts are scoped by the closed + * `CRYPTO_CONTEXT_SCOPES` vocabulary (ADR-0128 D1), which has no member for + * this producer, and its handle is persisted as a `sys_secret` row — a + * durable record for a value that lives ten minutes. The state-derived key + * needs no shared server secret, so it also holds on a cluster whose replicas + * share nothing but the KV. + * * **What it is bound to.** The organization and the user that began it, and * the instance. A callback completed by anyone else — another user's browser * carrying a code an attacker obtained for their own vendor account — is @@ -28,7 +54,7 @@ * deployment by the caller and never taken from the request. */ -import { createHash, randomBytes } from 'node:crypto'; +import { createCipheriv, createDecipheriv, createHash, hkdfSync, randomBytes } from 'node:crypto'; import type { IKV } from '@objectstack/spec/contracts'; /** How long a begun connect may wait for its callback. */ @@ -71,37 +97,90 @@ export function createAuthorizationState(): string { const stateKey = (state: string): string => `connector-oauth2:pending:${createHash('sha256').update(state, 'utf8').digest('hex')}`; -/** The store over the cluster's ephemeral KV. */ +/** + * A pending authorization as it is held: an AES-256-GCM envelope, never the + * value. `v` names the sealing scheme, so a later one is told apart rather + * than guessed at. + */ +export interface SealedPendingAuthorization { + readonly v: 1; + /** 96-bit nonce, base64url. */ + readonly iv: string; + /** Ciphertext of the JSON-encoded {@link PendingAuthorization}, base64url. */ + readonly ct: string; + /** GCM authentication tag, base64url. */ + readonly tag: string; +} + +/** HKDF `info` for the sealing key — distinct from the digest that names the slot. */ +const SEAL_KEY_INFO = 'objectstack:connector-oauth2:pending-authorization:seal:v1'; + +/** The sealing key of one state: HKDF-SHA-256 over the raw state, which the server never holds. */ +function sealKey(state: string): Buffer { + return Buffer.from(hkdfSync('sha256', Buffer.from(state, 'utf8'), Buffer.alloc(0), SEAL_KEY_INFO, 32)); +} + +/** Seal `pending` under `state`'s key, bound (AAD) to the slot `key` it is written to. */ +export function sealPendingAuthorization(state: string, pending: PendingAuthorization): SealedPendingAuthorization { + const iv = randomBytes(12); + const cipher = createCipheriv('aes-256-gcm', sealKey(state), iv); + cipher.setAAD(Buffer.from(stateKey(state), 'utf8')); + const ct = Buffer.concat([cipher.update(JSON.stringify(pending), 'utf8'), cipher.final()]); + return { v: 1, iv: base64url(iv), ct: base64url(ct), tag: base64url(cipher.getAuthTag()) }; +} + +/** + * Open what {@link sealPendingAuthorization} sealed under `state`, or + * `undefined` — never a throw — when the value is not an envelope of this + * scheme or does not authenticate under this state and slot. A callback reads + * that the same as "no connect waiting": fail closed, nothing exchanged. + */ +export function openPendingAuthorization(state: string, sealed: unknown): PendingAuthorization | undefined { + const envelope = sealed as Partial | null | undefined; + if (!envelope || envelope.v !== 1) return undefined; + if (typeof envelope.iv !== 'string' || typeof envelope.ct !== 'string' || typeof envelope.tag !== 'string') return undefined; + try { + const decipher = createDecipheriv('aes-256-gcm', sealKey(state), Buffer.from(envelope.iv, 'base64url')); + decipher.setAAD(Buffer.from(stateKey(state), 'utf8')); + decipher.setAuthTag(Buffer.from(envelope.tag, 'base64url')); + const plain = Buffer.concat([decipher.update(Buffer.from(envelope.ct, 'base64url')), decipher.final()]).toString('utf8'); + return JSON.parse(plain) as PendingAuthorization; + } catch { + return undefined; + } +} + +/** The store over the cluster's ephemeral KV. It holds only sealed envelopes. */ export function kvPendingAuthorizationStore(kv: IKV): PendingAuthorizationStore { return { async put(state, pending) { - await kv.set(stateKey(state), pending, { ttl: PENDING_AUTHORIZATION_TTL_SECONDS }); + await kv.set(stateKey(state), sealPendingAuthorization(state, pending), { ttl: PENDING_AUTHORIZATION_TTL_SECONDS }); }, async take(state) { const key = stateKey(state); - const entry = await kv.get(key); + const entry = await kv.get(key); if (!entry) return undefined; // Only the caller whose delete lands holds it: a replayed callback, // or two racing on two replicas, finds nothing. const deleted = await kv.delete(key, { ifVersion: entry.version }); - return deleted ? entry.value : undefined; + return deleted ? openPendingAuthorization(state, entry.value) : undefined; }, }; } -/** The in-process store, for a deployment with no cluster service. */ +/** The in-process store, for a deployment with no cluster service. It holds the same sealed envelopes. */ export function memoryPendingAuthorizationStore(now: () => number = Date.now): PendingAuthorizationStore { - const held = new Map(); + const held = new Map(); return { async put(state, pending) { for (const [key, value] of held) if (value.expiresAt <= now()) held.delete(key); - held.set(stateKey(state), pending); + held.set(stateKey(state), { expiresAt: pending.expiresAt, sealed: sealPendingAuthorization(state, pending) }); }, async take(state) { const key = stateKey(state); - const pending = held.get(key); + const entry = held.get(key); held.delete(key); - return pending; + return entry ? openPendingAuthorization(state, entry.sealed) : undefined; }, }; } diff --git a/packages/services/service-automation/src/oauth2-grant/endpoint-transport.test.ts b/packages/services/service-automation/src/oauth2-grant/endpoint-transport.test.ts index 0ec981b39e5..acc46b647ee 100644 --- a/packages/services/service-automation/src/oauth2-grant/endpoint-transport.test.ts +++ b/packages/services/service-automation/src/oauth2-grant/endpoint-transport.test.ts @@ -11,7 +11,6 @@ describe('ADR-0143 D4 — which oauth2 endpoint a secret may be sent to', () => 'https://idp.example.com/token', 'HTTPS://IDP.EXAMPLE.COM/token', 'http://localhost:8080/token', - 'http://idp.localhost/token', 'http://127.0.0.1:9000/token', 'http://127.255.0.1/token', 'http://[::1]:9000/token', @@ -25,6 +24,13 @@ describe('ADR-0143 D4 — which oauth2 endpoint a secret may be sent to', () => 'http://128.0.0.1/token', 'http://127.0.0.1.example.com/token', 'http://localhost.example.com/token', + // A `*.localhost` name resolves through DNS like any other name. + 'http://idp.localhost/token', + 'http://localhost.localhost/token', + // The userinfo is not the host: this is a request to evil.com. + 'http://localhost@evil.com/token', + // An IPv4-mapped IPv6 address is not one of the two loopback spellings. + 'http://[::ffff:127.0.0.1]/token', 'ftp://idp.example.com/token', 'javascript:alert(1)', 'not a url', diff --git a/packages/services/service-automation/src/oauth2-grant/endpoint-transport.ts b/packages/services/service-automation/src/oauth2-grant/endpoint-transport.ts index 80eec8885ed..e05bbbf95c8 100644 --- a/packages/services/service-automation/src/oauth2-grant/endpoint-transport.ts +++ b/packages/services/service-automation/src/oauth2-grant/endpoint-transport.ts @@ -12,10 +12,16 @@ /** The `oauth2` endpoint fields this check covers, in the order a reason names them. */ export const OAUTH2_ENDPOINT_FIELDS = ['tokenUrl', 'authorizationUrl'] as const; -/** True for `localhost`, a `*.localhost` name, any `127.0.0.0/8` address and `[::1]`. */ +/** + * True for `localhost`, any `127.0.0.0/8` address and `[::1]`, judged on the + * host the WHATWG parser extracts (so `http://localhost@evil.example` is + * `evil.example`). ⛔ Not a `*.localhost` name: RFC 6761 reserves it, but the + * system resolver (glibc's included) does not special-case it, so such a name + * resolves through DNS like any other and may leave the machine. + */ function isLoopbackHost(hostname: string): boolean { const host = hostname.toLowerCase(); - if (host === 'localhost' || host.endsWith('.localhost')) return true; + if (host === 'localhost') return true; if (host === '[::1]') return true; return /^127(?:\.(?:25[0-5]|2[0-4]\d|1?\d?\d)){3}$/.test(host); } diff --git a/packages/services/service-automation/src/oauth2-grant/token-endpoint.test.ts b/packages/services/service-automation/src/oauth2-grant/token-endpoint.test.ts index be7e2aa0ea5..a6b334ee4b8 100644 --- a/packages/services/service-automation/src/oauth2-grant/token-endpoint.test.ts +++ b/packages/services/service-automation/src/oauth2-grant/token-endpoint.test.ts @@ -4,7 +4,9 @@ // authentication, the four ways it fails, and the server text it never carries. // The values below are obvious dummies. -import { describe, expect, it } from 'vitest'; +import { createServer, type Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { afterEach, describe, expect, it } from 'vitest'; import { ConnectorUpstreamUnavailableError } from '@objectstack/spec/integration'; import { OAuth2ClientRefusedError, @@ -126,3 +128,43 @@ describe('ADR-0143 D4 — how a token request fails', () => { expect((upstream as Error).message).not.toContain('dummy'); }); }); + +describe('ADR-0143 D4 — a token endpoint\'s redirect is never followed', () => { + const servers: Server[] = []; + afterEach(async () => { + await Promise.all(servers.splice(0).map((srv) => new Promise((done) => srv.close(done)))); + }); + const listen = async (handler: Parameters[1]): Promise => { + const srv = createServer(handler); + servers.push(srv); + await new Promise((done) => srv.listen(0, '127.0.0.1', done)); + return `http://127.0.0.1:${(srv.address() as AddressInfo).port}`; + }; + + it.each([307, 308, 302])('⛔ a %i is refused, and its Location receives nothing', async (status) => { + const reached: string[] = []; + const elsewhere = await listen((req, res) => { + let body = ''; + req.on('data', (chunk) => { body += chunk; }); + req.on('end', () => { + reached.push(body); + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ access_token: 'dummy-at', token_type: 'Bearer' })); + }); + }); + const tokenUrl = await listen((_req, res) => { + res.writeHead(status, { location: `${elsewhere}/token` }); + res.end(); + }); + // The real transport: a stub that never follows would prove nothing. + const err = await requestOAuth2Token({ + connector: 'crm', + tokenUrl: `${tokenUrl}/token`, + client: { clientId: 'client-1', clientSecret: 'dummy-secret' }, + params: { grant_type: 'refresh_token', refresh_token: 'dummy-rt' }, + }).catch((e: unknown) => e); + expect(err).toBeInstanceOf(OAuth2TokenResponseError); + expect(err).toMatchObject({ code: 'SERVICE_UNAVAILABLE', status: 503 }); + expect(reached).toEqual([]); + }); +}); diff --git a/packages/services/service-automation/src/oauth2-grant/token-endpoint.ts b/packages/services/service-automation/src/oauth2-grant/token-endpoint.ts index 612e8043310..46f9978f4a8 100644 --- a/packages/services/service-automation/src/oauth2-grant/token-endpoint.ts +++ b/packages/services/service-automation/src/oauth2-grant/token-endpoint.ts @@ -22,8 +22,15 @@ * refusal ("reconnect required", "the code was refused"); * - `invalid_client` / `unauthorized_client` → {@link OAuth2ClientRefusedError}: * the client id or secret is wrong, which the instance's configuration fixes; - * - anything else, or a `2xx` without a bearer `access_token` → - * {@link OAuth2TokenResponseError}. + * - a redirect (`3xx`), anything else, or a `2xx` without a bearer + * `access_token` → {@link OAuth2TokenResponseError}. + * + * **A redirect is never followed.** The request carries the client secret + * and, on a refresh, the refresh token; a `307`/`308` would replay that body + * to wherever `Location` names — another host, or plain `http:` past the + * https-only check the instance's `tokenUrl` passed. RFC 6749 §3.2 gives a + * token endpoint no redirect to answer, so one is refused as a wrong answer + * (the `tokenUrl` is not the endpoint itself), not retried as an outage. * * The last two carry the connector seam's `503 SERVICE_UNAVAILABLE` (the * shape `connectorCredentialRefusal` stamps), a standard code: no code of @@ -159,6 +166,8 @@ export async function requestOAuth2Token(request: OAuth2TokenRequest): Promise= 300 && response.status < 400)) { + throw new OAuth2TokenResponseError( + `the token endpoint of connector instance '${connector}' answered a redirect, which is not followed: the client ` + + 'secret and refresh token are sent only to the declared tokenUrl. Declare the endpoint the redirect names', + ); + } + let parsed: unknown; try { parsed = await response.json(); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 8e2e038462f..793772e4e78 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -4761,6 +4761,16 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/services/service-automation/src/connector-connections.test.ts", + "verb": "delete", + "pinned": 2 + }, + { + "file": "packages/services/service-automation/src/connector-connections.test.ts", + "verb": "update", + "pinned": 2 + }, { "file": "packages/services/service-automation/src/flow-activation-ledger.test.ts", "verb": "update",