diff --git a/.changeset/24773-oauth-register-refusal-status.md b/.changeset/24773-oauth-register-refusal-status.md new file mode 100644 index 00000000000..2807da55eb4 --- /dev/null +++ b/.changeset/24773-oauth-register-refusal-status.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): a refused OAuth application registration answers the status the refusal declared, not 500 (#24773) + +Clause-②: no + +`POST /api/v1/auth/sys-oauth-application/register` answered most refusals from better-auth as `500 OAUTH_REGISTER_FAILED`. better-auth raises each refusal as an `APIError` whose HTTP status is the number `statusCode` (`400`) and the name `status` (`'BAD_REQUEST'`). The route read only a numeric `status`, so a named refusal fell through to 500. Two examples are a redirect URI the provider refuses, such as `http://localhost:4001/callback` on a `web` client (`invalid_redirect_uri`), and a request with no session (`UNAUTHORIZED`). + +- **Now.** The route answers the refusal's own status: `400` for the redirect-URI refusal and `401` for the missing session. The code stays `OAUTH_REGISTER_FAILED`, and `error.details.upstreamError` still carries the provider's discriminator (`invalid_redirect_uri`). +- **Unchanged.** A throw that declares no status, such as a storage fault while the client is persisted, still answers `500`. The body-schema refusal keeps answering `400`. Which redirect URIs a client may register is unchanged: a `web` client still needs an `https` URI on a non-loopback host. diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index d313aef1f42..03e026deaed 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -29,7 +29,7 @@ import { } from '@objectstack/platform-objects/apps'; import { SysOrganizationDetailPage, SysUserDetailPage } from '@objectstack/platform-objects/pages'; import { PLATFORM_OWNER_EMAIL_ENV, resolvePlatformOwnerEmail, resolveTenancyPosture } from '@objectstack/types'; -import { inProcessSessionReadInput } from '@objectstack/types'; +import { inProcessSessionReadInput, resolveThrownHttpError } from '@objectstack/types'; import { postureEnforcesWall, type OrgScopingEntitlement } from '@objectstack/spec/security'; import type { AuthSessionApi, IDataEngine, IEmailService, II18nService, IObjectQLEngine, ISmsService } from '@objectstack/spec/contracts'; import { @@ -3624,7 +3624,17 @@ export class AuthPlugin implements Plugin { ...inProcessSessionReadInput(c.req.raw.headers), }); } catch (err: any) { - const status = typeof err?.status === 'number' ? err.status : 500; + // [#24773] Answer the status the refusal DECLARED, by the repo's one + // rule for a thrown error's status (`resolveThrownHttpError`). + // better-call's `APIError` — every refusal better-auth raises — + // carries its status twice: as the NUMBER `statusCode` (400) and as + // the NAME `status` ('BAD_REQUEST'). This door used to read only a + // numeric `status`, so each named refusal answered 500 — an + // `invalid_redirect_uri` redirect URI, a request with no session + // (`UNAUTHORIZED`) — while the body-schema `ValidationError`, whose + // `status` is the number 400, answered 400 from the same door. A + // throw that declares no status is a server fault and stays 500. + const { status } = resolveThrownHttpError(err, 500); // better-auth's `body.error` is an arbitrary upstream string, so it // cannot be `error.code` — that field is a closed set (ADR-0112) and // an unbounded source defeats it. Keep the code ours and carry the diff --git a/packages/plugins/plugin-auth/src/oauth-register-refusal-status.test.ts b/packages/plugins/plugin-auth/src/oauth-register-refusal-status.test.ts new file mode 100644 index 00000000000..8b2336493fe --- /dev/null +++ b/packages/plugins/plugin-auth/src/oauth-register-refusal-status.test.ts @@ -0,0 +1,203 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#24773] `POST /api/v1/auth/sys-oauth-application/register` answers a + * refused registration with the status the refusal DECLARED — a client error + * is a coded 4xx (`OAUTH_REGISTER_FAILED`, ADR-0112), never 500 — and a throw + * that declares no status stays 500. + * + * The defect, measured against the installed better-auth: every refusal + * better-auth raises is better-call's `APIError`, which carries its status + * twice — the NUMBER `statusCode` (400) and the NAME `status` ('BAD_REQUEST'). + * The wrapper read only a numeric `status`, so each named refusal answered 500: + * an `http://localhost` redirect URI on a `web` client (the vendor's + * `invalid_redirect_uri`), and a request with no session (`UNAUTHORIZED`). + * The body-schema `ValidationError`, whose `status` IS the number 400, answered + * 400 from the same door — one door, two answers for the same class. + * + * This file pins the STATUS only. Which redirect URIs a client may register is + * the vendor's acceptance rule and is not changed here: the loopback refusals + * below are the vendor's own answer, used as the fixture's named-status + * refusal. + * + * Driven against REAL better-auth (the installed version, OIDC provider on) + * behind the plugin's REAL route registration on a real Hono app — the + * `in-process-session-renewal.pin.test.ts` harness: a real `AuthManager` over + * the shared in-memory engine. Each refusal asserts the ADR-0112 envelope + * (`code` + `status`), the vendor's discriminator in `details`, and that no + * `sys_oauth_application` row was written. + */ + +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import { Hono } from 'hono'; +import { envelopeViolations } from '@objectstack/spec/api'; +import type { PluginContext } from '@objectstack/core'; +import { AuthManager } from './auth-manager'; +import { AuthPlugin } from './auth-plugin'; +import { createMemoryEngine } from './impersonation-bearer-rotation.test'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const PASSWORD = 'S3cure!Passw0rd-24773'; +const EMAIL = 'owner.24773@example.com'; +const ORIGIN = 'http://localhost:3000'; +const BASE = '/api/v1/auth'; +const DOOR = '/sys-oauth-application/register'; +const CLIENTS = 'sys_oauth_application'; + +const mockCtx = (): PluginContext => + ({ + registerService: vi.fn(), + getService: vi.fn((name: string) => (name === 'manifest' ? { register: vi.fn() } : undefined)), + getServices: vi.fn(() => new Map()), + hook: vi.fn(), + trigger: vi.fn(), + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }, + getKernel: vi.fn(), + }) as unknown as PluginContext; + +let engine: ReturnType; +let manager: AuthManager; +let app: Hono; +let cookie: string; + +const clientRows = (): number => (engine.tables.get(CLIENTS) ?? []).length; + +const register = (body: unknown, withSession = true) => + app.request(`${ORIGIN}${BASE}${DOOR}`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: ORIGIN, ...(withSession ? { cookie } : {}) }, + body: JSON.stringify(body), + }); + +type Answer = { + success: boolean; + data?: { client?: { client_id?: unknown } }; + error?: { code: string; message: string; details?: { upstreamError?: string } }; +}; + +/** The refusal envelope: ADR-0112 `code` + `status`, and nothing persisted. */ +async function expectRefused(res: Response, status: number, rowsBefore: number, upstreamError?: string) { + const body = (await res.json()) as Answer; + expect(res.status, JSON.stringify(body)).toBe(status); + expect(envelopeViolations(body)).toEqual([]); + expect(body.success).toBe(false); + expect(body.error?.code).toBe('OAUTH_REGISTER_FAILED'); + if (upstreamError) expect(body.error?.details?.upstreamError).toBe(upstreamError); + expect(clientRows(), 'a refused registration wrote a client row').toBe(rowsBefore); +} + +beforeAll(async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + engine = createMemoryEngine(); + manager = new AuthManager({ + secret: SECRET, + baseUrl: ORIGIN, + dataEngine: engine, + plugins: { oidcProvider: true }, + } as unknown as ConstructorParameters[0]); + + const direct = (path: string, body: unknown) => + manager.handleRequest( + new Request(`${ORIGIN}${BASE}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }), + ); + const signUp = await direct('/sign-up/email', { email: EMAIL, password: PASSWORD, name: 'Client Owner' }); + expect(signUp.status, `sign-up: ${await signUp.clone().text()}`).toBe(200); + const signIn = await direct('/sign-in/email', { email: EMAIL, password: PASSWORD }); + expect(signIn.status, `sign-in: ${await signIn.clone().text()}`).toBe(200); + const staged = signIn.headers.getSetCookie().find((c) => /(?:^|\.)session_token=/.test(c.split(';')[0])); + if (!staged) throw new Error('sign-in staged no session cookie'); + cookie = staged.split(';')[0]; + + app = new Hono(); + const ctx = mockCtx(); + const plugin = new AuthPlugin({ secret: SECRET }); + await plugin.init(ctx); + (plugin as unknown as { authManager: AuthManager }).authManager = manager; + (plugin as unknown as { + registerAuthRoutes(server: unknown, ctx: PluginContext): void; + }).registerAuthRoutes({ getRawApp: () => app, getPort: () => 0 }, ctx); +}); + +afterAll(() => vi.restoreAllMocks()); + +describe('[#24773] precondition — the fixture refusal is a NAMED-status APIError', () => { + it('better-auth refuses a loopback redirect URI on a web client with status "BAD_REQUEST" / statusCode 400', async () => { + // Without this, a vendor that switched to a numeric `status` would let the + // pins below pass on the pre-fix wrapper too, and prove nothing. + const api = (await manager.getApi()) as unknown as { + createOAuthClient(input: unknown): Promise; + }; + const thrown = await api + .createOAuthClient({ + body: { client_name: 'precondition', redirect_uris: ['http://localhost:4001/callback'], application_type: 'web' }, + headers: new Headers({ cookie }), + }) + .then(() => undefined, (e: unknown) => e as { name?: unknown; status?: unknown; statusCode?: unknown; body?: { error?: unknown } }); + expect(thrown, 'the vendor accepted the fixture URI — nothing below is a refusal').toBeDefined(); + expect(thrown?.name).toBe('APIError'); + expect(thrown?.status).toBe('BAD_REQUEST'); + expect(thrown?.statusCode).toBe(400); + expect(thrown?.body?.error).toBe('invalid_redirect_uri'); + }); +}); + +describe('[#24773] a refusal answers the status it declared, never 500', () => { + it('http://localhost redirect URI on a web client: 400 OAUTH_REGISTER_FAILED, invalid_redirect_uri', async () => { + const before = clientRows(); + const res = await register({ name: 'Local Dev', redirectURLs: 'http://localhost:4001/callback', type: 'web' }); + await expectRefused(res, 400, before, 'invalid_redirect_uri'); + }); + + it('http://127.0.0.1 redirect URI on a web client: 400 OAUTH_REGISTER_FAILED, invalid_redirect_uri', async () => { + const before = clientRows(); + const res = await register({ name: 'Local Dev', redirectURLs: 'http://127.0.0.1:4001/callback', type: 'web' }); + await expectRefused(res, 400, before, 'invalid_redirect_uri'); + }); + + it('no session: the vendor\'s UNAUTHORIZED answers 401 OAUTH_REGISTER_FAILED', async () => { + const before = clientRows(); + const res = await register({ name: 'Anonymous', redirectURLs: 'https://app.example.com/callback' }, false); + await expectRefused(res, 401, before); + }); + + it('preserved: the body-schema refusal (numeric status) still answers 400', async () => { + const before = clientRows(); + const res = await register({ name: 'Plain HTTP', redirectURLs: 'http://app.example.com/callback' }); + await expectRefused(res, 400, before); + }); +}); + +describe('[#24773] a throw that declares no status is a server fault: 500', () => { + it('a storage fault while persisting the client answers 500, not a client error', async () => { + const before = clientRows(); + const insert = engine.insert; + engine.insert = async (name: string, data: unknown) => { + if (name === CLIENTS) throw new Error('storage fault (test)'); + return insert(name, data); + }; + try { + const res = await register({ name: 'Fault', redirectURLs: 'https://app.example.com/callback' }); + await expectRefused(res, 500, before); + } finally { + engine.insert = insert; + } + }); +}); + +describe('[#24773] control — an accepted registration', () => { + it('https redirect URI on a web client: 200 with the minted client, one row written', async () => { + const before = clientRows(); + const res = await register({ name: 'Production', redirectURLs: 'https://app.example.com/callback', type: 'web' }); + const body = (await res.json()) as Answer; + expect(res.status, JSON.stringify(body)).toBe(200); + expect(envelopeViolations(body)).toEqual([]); + expect(typeof body.data?.client?.client_id).toBe('string'); + expect(clientRows()).toBe(before + 1); + }); +});