From cd5a8b2f1036a58ee654fbd6b2a62ce8f94f3c82 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 17:57:44 +0000 Subject: [PATCH 1/2] fix(plugin-auth): run the D5.1 /oauth2/authorize env-access gate for a signed bearer (#8102) ADR-0069 D5.1's cloud-as-IdP gate resolved its subject with an inline copy of resolveActor -- line for line the same logic, in a second place. #8101 fixed one bug in the shared resolver and left the copy untouched, so the two diverged. The shared resolver learned that a bearer credential must have its signature stripped before lookup: bearer() hands clients the signed form in set-auth-token (the documented API-lane credential) and accepts it back, while session.token stores the unsigned value. The copy guarding /oauth2/authorize kept looking the signed credential up verbatim and resolved nothing. The unresolved case here is deliberately fail-open, so that miss did not deny the request -- it skipped the check entirely. An authenticated caller on the documented API lane was read as unauthenticated, and against a skip_consent client was issued an authorization code the gate would have refused. Delete the copy and call resolveActor. Two resolution sites are what let them diverge, so a second corrected copy would not have fixed the class. The fail-open default for genuinely unauthenticated callers is preserved unchanged. resolveActor also returns activeOrgId; this gate deliberately does not consume it -- the D5.1 host contract is (userId, clientId) and the control plane derives org membership from the user itself. Pinned by a dogfood gate that arms a DENYING gate and drives /oauth2/authorize over the cookie lane and both accepted bearer spellings, asserting the gate was actually invoked with the caller as its subject and the request refused rather than issued a code. The cookie and raw-token lanes are controls that pass on the broken build too; the signed-bearer lane is the pin. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .changeset/oidc-authorize-env-gate-bearer.md | 42 +++ .../plugins/plugin-auth/src/auth-manager.ts | 59 ++-- .../oidc-authorize-env-gate.dogfood.test.ts | 312 ++++++++++++++++++ 3 files changed, 376 insertions(+), 37 deletions(-) create mode 100644 .changeset/oidc-authorize-env-gate-bearer.md create mode 100644 packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts diff --git a/.changeset/oidc-authorize-env-gate-bearer.md b/.changeset/oidc-authorize-env-gate-bearer.md new file mode 100644 index 0000000000..786b03cee1 --- /dev/null +++ b/.changeset/oidc-authorize-env-gate-bearer.md @@ -0,0 +1,42 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): the D5.1 `/oauth2/authorize` env-access gate now runs for a signed bearer credential (#8102) + +ADR-0069 D5.1's cloud-as-IdP gate (`oidcAuthorizeGate`) is what enforces +org-membership / app-assignment before the OP issues an authorization code. It +resolved its subject with an **inline copy** of the shared `resolveActor` — +line for line the same logic, in a second place — and the two diverged the +moment one of them was fixed. + +`#8049` taught `resolveActor` that a bearer credential must have its signature +stripped before lookup: `bearer()` hands clients the signed form in the +`set-auth-token` response header (the documented API-lane credential) and +accepts it back, while `session.token` stores the **unsigned** value. The copy +guarding `/oauth2/authorize` kept looking the signed credential up verbatim and +so resolved nothing. + +**Why that is a security defect and not a lookup miss.** The unresolved case at +this endpoint is deliberately **fail-open** — an anonymous caller must fall +through so the OP can redirect them to log in. So an *authenticated* caller +holding the signed bearer was read as unauthenticated, and the env-access check +was not denied but **never evaluated at all**: the request proceeded, and +against a `skip_consent` client it was issued an authorization code that the +gate, had it run, would have refused. A declared control enforced for one +credential spelling and silently absent for the other. + +Impact is bounded: `oidcAuthorizeGate` is set only on the cloud control plane +(unset in open editions / self-host, where there is no gate at all), and the +OP's authorize endpoint is normally browser/cookie-driven — the cookie branch +always normalized and was never affected. + +**Fix.** The inline copy is deleted; the branch calls the shared +`resolveActor`, so there is one resolution site instead of two. The fail-open +default for genuinely unauthenticated callers is unchanged and deliberately +preserved. + +Pinned by a new dogfood gate that arms a **denying** gate and drives +`/oauth2/authorize` over the cookie lane and both accepted bearer spellings, +asserting on each that the gate was actually invoked with the caller as its +subject and that the request was refused rather than issued a code. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 151852bafa..bfa7ee9049 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1323,47 +1323,32 @@ export class AuthManager { // (open editions / self-host) → no gate. Unauthenticated → fall // through so the OP redirects to login; the gate runs on the // return pass (or immediately for a bearer/cookie session). + // + // [#8102] The acting subject is resolved through the shared, + // hook-order-independent {@link resolveActor} — NOT a second inline + // copy of it. This branch used to carry its own token lookup, line + // for line the same logic, and the two diverged the moment one was + // fixed: #8101 taught `resolveActor` to strip the signature from a + // bearer credential (`session.token` stores the UNSIGNED value while + // `bearer()` hands clients the SIGNED form in `set-auth-token`), and + // the copy here kept looking the signed credential up verbatim. It + // resolved nothing — and because the unresolved case is deliberately + // fail-open, an AUTHENTICATED caller on the documented API lane was + // read as unauthenticated and this env-access check never evaluated + // at all. Two resolution sites are what let them diverge, so the fix + // is to delete one, not to correct it in place. if (ctx?.path === '/oauth2/authorize' && this.config.oidcAuthorizeGate) { const clientId = ctx?.query?.client_id; if (clientId) { - let gateUserId: string | undefined; - // (a) standard resolver — handles the cookie session. - try { - const { getSessionFromCtx } = await import('better-auth/api'); - const s: any = await getSessionFromCtx(ctx as any); - gateUserId = s?.user?.id ?? s?.session?.userId; - } catch { /* fall through to explicit resolution */ } - // (b) explicit token resolution — hook-order-independent. The - // bearer plugin may convert `Authorization: Bearer` to a session - // AFTER this global before-hook, so getSessionFromCtx can miss a - // bearer (or non-default cookie) request here. Resolve the token - // (bearer or the session cookie's token part) and look it up. - if (!gateUserId) { - try { - const hdr = (k: string): string => - ((ctx?.headers?.get?.(k) ?? ctx?.request?.headers?.get?.(k)) as string) || ''; - let token: string | undefined; - const bm = /^Bearer\s+(.+)$/i.exec(hdr('authorization')); - if (bm?.[1]) token = bm[1].trim(); - if (!token) { - const cm = /(?:^|;\s*)(?:__Secure-|__Host-)?better-auth\.session_token=([^;]+)/.exec(hdr('cookie')); - if (cm?.[1]) token = decodeURIComponent(cm[1]).split('.')[0]; - } - if (token) { - const sess: any = await (ctx as any).context.adapter.findOne({ - model: 'session', - where: [{ field: 'token', value: token }], - }); - const exp = sess?.expiresAt ?? sess?.expires_at; - if (sess && (!exp || new Date(exp).getTime() > Date.now())) { - gateUserId = String(sess.userId ?? sess.user_id ?? '') || undefined; - } - } - } catch { /* unresolved → fall through, OP handles auth */ } - } - if (gateUserId) { + const actor = await this.resolveActor(ctx); + // Unauthenticated → fall through, per the contract above. Only an + // AUTHENTICATED subject is gated here. `resolveActor` also returns + // `activeOrgId`; this gate deliberately does not consume it — the + // D5.1 host contract is `(userId, clientId)` and the control plane + // derives org membership / app assignment from the user itself. + if (actor?.userId) { const allowed = await this.config.oidcAuthorizeGate({ - userId: gateUserId, + userId: actor.userId, clientId: String(clientId), }); if (!allowed) { diff --git a/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts b/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts new file mode 100644 index 0000000000..d4f8b2c6c5 --- /dev/null +++ b/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts @@ -0,0 +1,312 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8102] ADR-0069 D5.1's `/oauth2/authorize` env-access gate must run for an + * AUTHENTICATED subject on EVERY credential spelling — including the signed + * bearer the documented API lane hands out. + * + * ## The defect this pins + * + * The gate's branch resolved its subject with an INLINE COPY of `resolveActor` + * — line for line the same logic, in a second place. #8101 fixed one bug in the + * shared resolver (`session.token` stores the UNSIGNED value, while `bearer()` + * hands clients the SIGNED form in `set-auth-token` and accepts it back, so a + * bearer credential must have its signature stripped before lookup) and left + * the copy untouched. From that moment the two disagreed about what a signed + * bearer means. + * + * What makes it `security` rather than a lookup miss is the FAIL-OPEN default + * sitting immediately behind it. The unresolved case is deliberately permissive + * — *"Unauthenticated -> fall through so the OP redirects to login"* — so an + * authenticated caller holding the signed bearer was read as unauthenticated + * and the org-membership / app-assignment check never evaluated at all. Not a + * denied request: a request that was never judged, which then proceeded to be + * issued an authorization code. + * + * The fail-open default itself is CORRECT and is deliberately preserved (a + * genuinely anonymous caller must still reach the OP's login redirect). This + * file pins the authenticated case only. + * + * ## Why the fixture is shaped the way it is + * + * Three ways a test of this surface can be green while proving nothing, all + * three closed here on purpose: + * + * 1. **Driven with a cookie, it passes before and after the fix.** The inline + * copy's cookie branch always stripped the signature; only the bearer + * branch did not. So the cookie lane cannot detect this defect — it is + * carried below as the CONTROL that proves the gate is armed and the + * fixture wired, and the signed-bearer lane is the actual regression pin. + * Both bearer spellings are driven for the same reason `#8049` drives both: + * the raw sign-in token happens to match the stored column verbatim, so it + * too passes on the broken build. Exactly ONE of the three lanes flips. + * 2. **Against a gate that ALLOWS, refusal and fall-through are the same + * observation.** The gate here DENIES, so the two outcomes are distinct and + * opposite: refused (403 `ENV_ACCESS_DENIED`, no code) versus fell through + * (302 to the redirect URI carrying `code=`). The client is seeded with + * `skip_consent`, so fall-through really does mint a code for a subject the + * gate would have denied — the assertion is the security failure itself, + * not a proxy for it. + * 3. **With no gate configured at all it is vacuously green.** + * `oidcAuthorizeGate` is unset in open editions / self-host (it is a cloud + * control-plane hook), and the pre-existing + * `oidc-authorization-code-flow.dogfood.test.ts` drives this same endpoint + * with none set. This file installs one through `applyConfigPatch` — the + * same seam settings writes use — and asserts it was actually CALLED, with + * the right `userId`. A gate that never ran cannot deny anything, and the + * call recorder is what tells the two apart. + * + * The invocation assertion is the sharp one: it distinguishes "resolved the + * subject and denied it" from "resolved nothing and skipped the check", which + * a status code alone cannot do. + */ + +import { createHash } from 'node:crypto'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; + +// Must be on before the AuthPlugin builds its plugin list (kernel.use during +// bootStack), or /oauth2/authorize is not mounted and every assertion below +// would be measuring a 404. +process.env.OS_OIDC_PROVIDER_ENABLED = 'true'; + +const CLIENT_ID = 'project_envgate_8102'; +const CLIENT_SECRET = 'envgate-plaintext-secret'; +const REDIRECT_URI = 'https://env.example.com/api/v1/auth/callback/objectstack-cloud'; + +const ADMIN_EMAIL = 'admin@objectos.ai'; +const ADMIN_PASSWORD = 'admin123'; + +/** SHA-256 -> base64url (no padding) — @better-auth/oauth-provider defaultHasher. */ +function hashSecret(plaintext: string): string { + return createHash('sha256').update(plaintext) + .digest('base64') + .replace(/=+$/, '') + .replace(/\+/g, '-') + .replace(/\//g, '_'); +} + +/** Collect a response's Set-Cookie values into a single request Cookie header. */ +function cookieHeader(res: Response): string { + const jar = res.headers.getSetCookie?.() ?? []; + return jar.map((c) => c.split(';')[0]).join('; '); +} + +/** + * One authenticated transport. `credential` picks this lane's credential out of + * a sign-in response; `headers` turns it into the request headers that lane + * would send. + */ +interface Lane { + readonly name: string; + /** Does this lane's credential spelling detect the #8102 defect? */ + readonly pinsTheDefect: boolean; + credential(res: Response, body: { token?: string }): string; + headers(credential: string): Record; +} + +const LANES: Lane[] = [ + { + // CONTROL. The browser lane the OP is normally driven by, and the one the + // inline copy already normalized — green before and after the fix. It is + // here to prove the gate is armed and denying, so a red bearer lane below + // cannot be dismissed as "the fixture never worked". + name: 'cookie (control — normalized even on the broken build)', + pinsTheDefect: false, + credential: (res) => cookieHeader(res), + headers: (c) => ({ Cookie: c }), + }, + { + // THE PIN. `bearer()` hands this to clients in the `set-auth-token` + // response header; it is the documented API-lane credential and the only + // spelling that fails against the unfixed build. + name: 'bearer, signed set-auth-token (THE PIN)', + pinsTheDefect: true, + credential: (res) => res.headers.get('set-auth-token') ?? '', + headers: (c) => ({ Authorization: `Bearer ${c}` }), + }, + { + // CONTROL. The other accepted spelling: the raw session token from the + // sign-in body, which matches the stored column verbatim and so resolved + // fine even on the broken build. + name: 'bearer, raw sign-in token (control — matches the column verbatim)', + pinsTheDefect: false, + credential: (_res, body) => body.token ?? '', + headers: (c) => ({ Authorization: `Bearer ${c}` }), + }, +]; + +/** One recorded `oidcAuthorizeGate` invocation. */ +interface GateCall { + readonly userId: string; + readonly clientId: string; +} + +describe('#8102: the D5.1 /oauth2/authorize env-access gate runs on every credential spelling', () => { + let stack: VerifyStack; + let adminUserId: string; + let gateCalls: GateCall[] = []; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, {}); + + // Seed the OAuth client the way cloud's seedPlatformSsoClient does. + // `skip_consent` matters: without it a fall-through would stop at a consent + // screen and look like a refusal. With it, falling through mints a code. + const ql = await stack.kernel.getServiceAsync('objectql'); + const nowIso = new Date().toISOString(); + await ql.insert('sys_oauth_application', { + id: 'oauthc_envgate_8102', + name: 'Env Gate Pin', + client_id: CLIENT_ID, + client_secret: hashSecret(CLIENT_SECRET), + type: 'web', + redirect_uris: JSON.stringify([REDIRECT_URI]), + grant_types: JSON.stringify(['authorization_code', 'refresh_token']), + response_types: JSON.stringify(['code']), + scopes: JSON.stringify(['openid', 'email', 'profile']), + token_endpoint_auth_method: 'client_secret_basic', + require_pkce: false, + skip_consent: true, + disabled: false, + subject_type: 'public', + created_at: nowIso, + updated_at: nowIso, + }, { context: { isSystem: true } }); + + // Arm the gate. Unset (the default for open editions) there is no gate at + // all and every assertion in this file would pass on the broken build. + // DENY, so "refused" and "fell through" are opposite observations. + const auth = await stack.kernel.getServiceAsync('auth'); + auth.applyConfigPatch({ + oidcAuthorizeGate: (params: { userId: string; clientId: string }) => { + gateCalls.push({ userId: params.userId, clientId: params.clientId }); + return false; + }, + }); + + const user = (await ql.find( + 'sys_user', + { where: { email: ADMIN_EMAIL }, limit: 1 }, + { context: { isSystem: true } }, + ))[0]; + adminUserId = String(user?.id ?? ''); + expect(adminUserId, 'fixture: could not read the admin user id').toBeTruthy(); + }, 180_000); + + afterAll(async () => { await stack?.stop?.(); }); + + /** Sign in through the real route and hand back every lane's credential. */ + async function signIn() { + const res = await stack.api('/auth/sign-in/email', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email: ADMIN_EMAIL, password: ADMIN_PASSWORD }), + }); + const body = res.status === 200 ? ((await res.clone().json()) as { token?: string }) : {}; + return { res, body }; + } + + function authorizeQuery(): string { + return new URLSearchParams({ + response_type: 'code', + client_id: CLIENT_ID, + redirect_uri: REDIRECT_URI, + scope: 'openid email profile', + state: 'envgate-8102', + }).toString(); + } + + for (const lane of LANES) { + // eslint-disable-next-line vitest/valid-title + describe(`lane: ${lane.name}`, () => { + it('a denying gate REFUSES the authorize request, and is actually invoked with the caller as its subject', async () => { + const { res: signInRes, body } = await signIn(); + expect(signInRes.status, await signInRes.clone().text()).toBe(200); + const credential = lane.credential(signInRes, body); + expect(credential, `${lane.name}: sign-in yielded no credential`).toBeTruthy(); + + gateCalls = []; + const res = await stack.api(`/auth/oauth2/authorize?${authorizeQuery()}`, { + headers: lane.headers(credential), + redirect: 'manual', + }); + + // (1) The gate RAN, and resolved this caller as its subject. This is + // what separates "denied" from "never evaluated" — on the unfixed build + // the signed-bearer lane recorded zero calls here. + expect( + gateCalls.length, + `${lane.name}: oidcAuthorizeGate was never invoked — the subject did not resolve, ` + + 'so the env-access check was skipped entirely (fail-open)', + ).toBe(1); + expect(gateCalls[0].userId).toBe(adminUserId); + expect(gateCalls[0].clientId).toBe(CLIENT_ID); + + // (2) The request was REFUSED — code AND status, per the ADR-0112 + // envelope. Not "did not obviously succeed". + const text = await res.clone().text(); + expect(res.status, `${lane.name}: ${text}`).toBe(403); + const refusal: any = await res.json().catch(() => ({})); + expect(refusal?.code ?? refusal?.error?.code).toBe('ENV_ACCESS_DENIED'); + + // (3) …and refused rather than fallen through: no authorization code + // was minted for a subject the gate denied. On the unfixed build this + // was a 302 to the redirect URI carrying `code=`. + const location = res.headers.get('location') ?? ''; + expect(location, `${lane.name}: redirected instead of refusing`).toBe(''); + expect(text).not.toContain('code='); + }, 120_000); + + it('the same request with an ALLOWING gate is let through — the refusal above is the gate talking, not a broken request', async () => { + const auth = await stack.kernel.getServiceAsync('auth'); + const seen: GateCall[] = []; + auth.applyConfigPatch({ + oidcAuthorizeGate: (params: { userId: string; clientId: string }) => { + seen.push({ userId: params.userId, clientId: params.clientId }); + return true; + }, + }); + try { + const { res: signInRes, body } = await signIn(); + const credential = lane.credential(signInRes, body); + const res = await stack.api(`/auth/oauth2/authorize?${authorizeQuery()}`, { + headers: lane.headers(credential), + redirect: 'manual', + }); + + expect([302, 303]).toContain(res.status); + const location = res.headers.get('location') ?? ''; + expect(location.startsWith(REDIRECT_URI)).toBe(true); + expect(new URL(location).searchParams.get('code')).toBeTruthy(); + // The allow lane proves the endpoint works; it proves nothing about + // the DEFECT — an unresolved subject also reaches this outcome. Only + // the recorder distinguishes them, so assert it here too. + expect(seen.length, `${lane.name}: gate not invoked on the allow pass`).toBe(1); + expect(seen[0].userId).toBe(adminUserId); + } finally { + auth.applyConfigPatch({ + oidcAuthorizeGate: (params: { userId: string; clientId: string }) => { + gateCalls.push({ userId: params.userId, clientId: params.clientId }); + return false; + }, + }); + } + }, 120_000); + }); + } + + it('an UNAUTHENTICATED caller still falls through to the OP login redirect (the fail-open default is deliberate)', async () => { + gateCalls = []; + const res = await stack.api(`/auth/oauth2/authorize?${authorizeQuery()}`, { + redirect: 'manual', + }); + // Never judged, because there is no subject to judge — the OP takes over + // and sends the browser to log in. Changing THIS is a separate decision; + // the assertion exists so a future "fix" to the fail-open default cannot + // land silently under cover of this file. + expect(gateCalls.length, 'the gate must not run without a resolved subject').toBe(0); + expect(res.status).not.toBe(403); + }, 120_000); +}); From 708c665cae1eee7e39dd90cad76dc82f2b1f2ddd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 18:51:03 +0000 Subject: [PATCH 2/2] test(qa): assert the fixture's own discriminator claim in the #8102 pin The Lane.pinsTheDefect field was declared on every lane and read by nothing -- the same declared-but-unenforced shape this file exists to pin. Assert it: the suite now fails if a later edit makes a second lane the discriminator or drops the signed-bearer lane, either of which would leave the file green while measuring something other than #8102. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .../test/oidc-authorize-env-gate.dogfood.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts b/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts index d4f8b2c6c5..55bccf92a3 100644 --- a/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts +++ b/packages/qa/dogfood/test/oidc-authorize-env-gate.dogfood.test.ts @@ -297,6 +297,18 @@ describe('#8102: the D5.1 /oauth2/authorize env-access gate runs on every creden }); } + it('fixture self-check: exactly ONE lane is the discriminator', () => { + // The file's central claim is that two of the three lanes CANNOT detect + // this defect and are carried as controls. If a later edit made a second + // lane the pin — or dropped the signed-bearer lane — the suite would still + // be green while proving something else, so the claim is asserted rather + // than only written down. (This also keeps `pinsTheDefect` a field that is + // read, not merely declared.) + expect(LANES.filter((l) => l.pinsTheDefect).map((l) => l.name)).toEqual([ + 'bearer, signed set-auth-token (THE PIN)', + ]); + }); + it('an UNAUTHENTICATED caller still falls through to the OP login redirect (the fail-open default is deliberate)', async () => { gateCalls = []; const res = await stack.api(`/auth/oauth2/authorize?${authorizeQuery()}`, {