diff --git a/.changeset/24773-oauth-application-type.md b/.changeset/24773-oauth-application-type.md index e80d2be37ad..e52106467d2 100644 --- a/.changeset/24773-oauth-application-type.md +++ b/.changeset/24773-oauth-application-type.md @@ -9,5 +9,5 @@ Clause-②: no — the declared type now takes effect under the OAuth provider's `POST /api/v1/auth/sys-oauth-application/register` (the Setup → OAuth Applications form) forwarded the form's Application Type as `type`. The provider's `/oauth2/create-client` body declares `application_type` (`web` | `native`) and no `type`, so the value was dropped and every client was judged and stored as `web`. A `native` client therefore could not register a loopback redirect URI such as `http://localhost:4001/callback`, and its `sys_oauth_application.type` read `web`. - **Now.** `web` and `native` are forwarded as `application_type`, and the provider's own rule decides. A `native` client may use http only on exactly `localhost`, `127.0.0.1` or `[::1]` (any port), and is stored as `native`. A `web` client still needs an https URI on a non-loopback host. -- **Unchanged, with a warning.** Any other type, including the form's `user-agent-based` and `public`, has no `application_type` in the provider. It keeps today's outcome: registered and judged as `web`, with a client secret. The server now logs one warning per such registration, naming the value. To use a loopback redirect URI, register the client as `native`. +- **Refused.** Any other type, including the form's `user-agent-based` and `public`, has no `application_type` in the provider. The register route refuses it with `400 INVALID_REQUEST` and registers nothing (this release's breaking entry for the route). To use a loopback redirect URI, register the client as `native`. - **Unchanged.** No type at all is `web`. Refusals keep the codes and statuses they had: `400 OAUTH_REGISTER_FAILED`, with `error.details.upstreamError` `invalid_redirect_uri` when the redirect-URI rule refuses. diff --git a/.changeset/24773-oauth-form-types.md b/.changeset/24773-oauth-form-types.md index 7112b2dbe31..2166d7c6890 100644 --- a/.changeset/24773-oauth-form-types.md +++ b/.changeset/24773-oauth-form-types.md @@ -4,12 +4,12 @@ fix(platform-objects): the OAuth application register form offers only Web and Native, and its Redirect URLs help states the rule for each (#24773) -Clause-②: no (a form's offering: the register route still accepts every type value it accepted, and nothing on the server reads the action's param options, so no accept set or served shape moves) +Clause-②: no (a form's offering: nothing on the server reads the action's param options, so this change moves no accept set or served shape; the register route's refusal of the other types is its own breaking entry in this release) -Setup › OAuth Applications › Register offered four application types: Web, Native, User-agent based and Public. The OAuth provider judges only `web` and `native`, so the register route registers the other two as `web`, with a client secret. The form now offers Web and Native, and still defaults to Web. +Setup › OAuth Applications › Register offered four application types: Web, Native, User-agent based and Public. The OAuth provider judges only `web` and `native`, and the register route refuses any other type with `400 INVALID_REQUEST`. The form now offers Web and Native, and still defaults to Web. - **Redirect URLs help.** It said "Must use https:// in production", which did not tell an admin that a local `http://localhost` callback needs the Native type. It now states the rule for each type: a Web application needs `https://` on a host other than localhost or another loopback address; a Native application may also use `http://` on exactly `localhost`, `127.0.0.1` or `[::1]`, on any port. - **Application Type help.** New: choose Native for a desktop or mobile app, or for an app in local development that redirects to `http://localhost`. -- **Unchanged.** The register route accepts the same values as before, including `user-agent-based` and `public`. The stored `sys_oauth_application.type` column still lists all four values, so a row written under an earlier provider line keeps its value. The `public` column is unchanged. +- **Unchanged.** The stored `sys_oauth_application.type` column still lists all four values, so a row written under an earlier provider line keeps its value. The `public` column is unchanged. The zh-CN, ja-JP and es-ES translations carry the new help texts. diff --git a/.changeset/24773-oauth-type-refusal.md b/.changeset/24773-oauth-type-refusal.md new file mode 100644 index 00000000000..f96ffc0a658 --- /dev/null +++ b/.changeset/24773-oauth-type-refusal.md @@ -0,0 +1,21 @@ +--- +'@objectstack/plugin-auth': minor +'@objectstack/platform-objects': patch +--- + +fix(plugin-auth)!: `POST /api/v1/auth/sys-oauth-application/register` refuses an application type the OAuth provider does not judge with `400 INVALID_REQUEST`, where it registered it as a `web` client + +Clause-②: no (narrowing) — the register door accepted any `type` value and registered every one other than `web` / `native` as a `web` client with a secret; it now refuses each of those values with 400, so it accepts exactly `web`, `native` and no type. + + + +**BREAKING** (a REST door's accept set narrows), shipped as `minor` under the launch-window convention for breaking changes. + +The OAuth provider (`@better-auth/oauth-provider`) judges two application types, `web` and `native`, and this door forwards the type the Setup → OAuth Applications form chose as the provider's `application_type`. Any other value has no provider counterpart. The door used to register such a request as a confidential `web` client, with a client secret and the web redirect-URI rule, which is a different kind of client from the one the request asked for. It now refuses the request instead. + +- **FROM:** a request whose `type` is present and is neither `web` nor `native` (`user-agent-based`, `public`, any other string including the empty string, or a value that is not a string) answered `200` and stored a `web` client, or was refused by the web rule (`OAUTH_REGISTER_FAILED`) for a loopback redirect URI. +- **TO:** the same request answers `400` with `error.code: 'INVALID_REQUEST'`. The message names the value (its first 64 characters) and the two accepted types, and says to choose `native` for an `http://localhost`, `http://127.0.0.1` or `http://[::1]` redirect URI. The provider is not called, nothing is stored, and no warning is logged. +- **Accepted:** `web` and `native` are forwarded as the provider's `application_type` (this release's Application Type entry), and the provider's own redirect-URI rule decides. +- **Unchanged:** A request with no `type`, or with `type: null`, registers a `web` client, the provider's default. The other refusals keep their codes (their statuses are this release's refusal-status entry). Clients already registered are not touched. +- **`@objectstack/platform-objects`:** only the code comment on the `create_oauth_application` action changes, to state this refusal; it ships in the package's JavaScript. The form offers only Web and Native (this release's register-form entry). +- **The fix for an integrator:** send `type: 'web'` for a client whose redirect URIs are https on a host that is not a loopback address, or `type: 'native'` for a client that also redirects to `http://localhost`, `http://127.0.0.1` or `http://[::1]`. Omitting `type` registers `web`. diff --git a/packages/platform-objects/src/identity/sys-oauth-application.object.ts b/packages/platform-objects/src/identity/sys-oauth-application.object.ts index 240ded11b61..dca7b45442d 100644 --- a/packages/platform-objects/src/identity/sys-oauth-application.object.ts +++ b/packages/platform-objects/src/identity/sys-oauth-application.object.ts @@ -151,14 +151,14 @@ export const SysOauthApplication = ObjectSchema.create({ // provider judges (`@better-auth/oauth-provider` 1.7.3 declares // `application_type` as `web` | `native`), and the redirect-URL help // states that provider's redirect-URI rule per type, which the register - // route forwards the type to. `user-agent-based` and `public` are no - // longer offered: the provider has no such application type, so the - // route registers either as `web`. The route itself still accepts both - // (refusing them narrows the door, which waits for a major version), and - // nothing on the server reads these options: the generic action door - // refuses a `type: 'api'` action before its param contract runs, so the - // route is the only judge. The stored `type` column below is a separate - // surface and is not narrowed here: that waits for the same major version. + // route forwards the type to. The route accepts only those two: in 18.x it + // refuses any other present type (`user-agent-based`, `public`, ...) with + // 400 before the provider runs; no type (absent or null) registers `web`. + // Nothing on the server reads these options: the generic action door + // refuses a `type: 'api'` action before its param contract runs. The + // stored `type` column below is not narrowed: 1.7.3 writes only `web` or + // `native` (null for a client-metadata-document client), an earlier + // provider line could write `user-agent-based`, and no in-tree reader branches on it. params: [ { name: 'name', label: 'Application Name', type: 'text', required: true }, { diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 01004ce1674..d065fc855f5 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -3647,26 +3647,35 @@ export class AuthPlugin implements Plugin { // non-loopback host, `native` may use http only on exactly `localhost`, // `127.0.0.1` or `[::1]` — and nothing here re-judges a URI. // - // Any other value (the form's `user-agent-based` and `public`, which - // have no `application_type`, or anything else) keeps the outcome it - // always had: registered and judged as `web`. Refusing it would narrow - // what this door accepts, and such a narrowing ships its loud half - // first (the standing post-GA rule, #6015 `6292005854`): one warning - // per registration, naming the value. No type at all is the - // provider's own default, `web`, and says nothing. + // The door accepts exactly those two types. Any other value present in + // the body — the form's former `user-agent-based` and `public`, which + // have no `application_type`, an empty string, a non-string, anything + // else — is refused here with the route's own 400 `INVALID_REQUEST`, + // before the provider is called, so nothing is stored. It is never + // registered as `web` and never mapped to another type: that silent + // substitution issued a confidential web client, with a secret, for a + // request that asked for something else. The refusal ships in 18.x as + // a breaking minor (the standing post-GA rule as amended, #6015 + // `6303865025`). No type at all — absent, or JSON `null` — is the + // provider's own default, `web`, and is unchanged. let applicationType: 'web' | 'native' = 'web'; if (type === 'web' || type === 'native') { applicationType = type; } else if (type !== undefined && type !== null) { - const requestedType = typeof type === 'string' ? type.slice(0, 64) : `(${typeof type})`; - ctx.logger.warn( - `[AuthPlugin] sys-oauth-application/register: application type ${JSON.stringify(requestedType)} is not ` + - "one the OAuth provider judges (it knows 'web' and 'native'), so this client is registered and judged " + - "as 'web': https redirect URIs on non-loopback hosts only, and a client secret is issued. Register it " + - "as 'web', or as 'native' to use an http://localhost, http://127.0.0.1 or http://[::1] redirect URI. " + - 'A future major version refuses this value.', - { requestedType, registeredAs: 'web' }, - ); + const shown = + typeof type === 'string' + ? JSON.stringify(type.length > 64 ? `${type.slice(0, 64)}…` : type) + : `(${typeof type})`; + return c.json({ + success: false, + error: { + code: 'INVALID_REQUEST', + message: + `type ${shown} is not an application type the OAuth provider judges: send 'web' or 'native', ` + + "or no type for 'web'. Choose 'native' for an http://localhost, http://127.0.0.1 or http://[::1] " + + "redirect URI; 'web' needs https on a host that is not a loopback address.", + }, + }, 400); } const authApi: any = await this.authManager!.getApi(); diff --git a/packages/plugins/plugin-auth/src/oauth-register-application-type.test.ts b/packages/plugins/plugin-auth/src/oauth-register-application-type.test.ts index acfc1a56194..3dd3d789eb2 100644 --- a/packages/plugins/plugin-auth/src/oauth-register-application-type.test.ts +++ b/packages/plugins/plugin-auth/src/oauth-register-application-type.test.ts @@ -17,12 +17,15 @@ * provider's own rule applies unchanged: `web` needs https on a non-loopback * host; `native` may use http only on exactly `localhost`, `127.0.0.1` or * `[::1]`. Nothing in the door re-judges a URI. - * - Any other value (the form's `user-agent-based` and `public`, which have no - * `application_type`, or anything else) keeps the outcome it always had: - * registered, judged and stored as `web`. It is NOT refused — that would - * narrow what the door accepts — and the server logs exactly ONE warning - * per registration, naming the value. - * - No type at all is the provider's default, `web`, with no warning. + * - Any other value present in the body (the form's former `user-agent-based` + * and `public`, which have no `application_type`, an unknown string, an + * empty string, a non-string) is REFUSED with the route's own + * `400 INVALID_REQUEST`, before the provider is called: nothing is stored, + * nothing is logged, and it is never registered as `web` or mapped to + * another type. Until this refusal it was registered as `web` with one + * server warning; that warning is gone. + * - No type at all — absent, or JSON `null` — is the provider's default, + * `web`, and is unchanged. * * Driven against REAL better-auth (OIDC provider on) behind the plugin's REAL * route registration on a real Hono app, over a REAL ObjectQL engine on SQLite @@ -96,6 +99,7 @@ const AUTHZ_OBJECTS = [ ]; const warn = vi.fn(); +const logError = vi.fn(); const mockCtx = (): PluginContext => ({ registerService: vi.fn(), @@ -103,7 +107,7 @@ const mockCtx = (): PluginContext => getServices: vi.fn(() => new Map()), hook: vi.fn(), trigger: vi.fn(), - logger: { info: vi.fn(), error: vi.fn(), warn, debug: vi.fn() }, + logger: { info: vi.fn(), error: logError, warn, debug: vi.fn() }, getKernel: vi.fn(), }) as unknown as PluginContext; @@ -111,6 +115,8 @@ let engine: ObjectQL; let manager: AuthManager; let app: Hono; let cookie: string; +/** A pass-through spy on the provider's `createOAuthClient`, the door's one call into it. */ +let providerCall: ReturnType; type Row = Record; @@ -136,18 +142,27 @@ type Answer = { }; /** - * One registration through the door. Returns the answer and the warnings the - * plugin logged while serving THIS request (the mock is cleared first). + * One registration through the door. Returns the answer, the warnings and + * errors the plugin logged while serving THIS request, and the bodies the door + * handed the provider's `createOAuthClient` (every mock is cleared first). */ async function register(body: Record) { warn.mockClear(); + logError.mockClear(); + providerCall.mockClear(); const res = await app.request(`${ORIGIN}${BASE}${DOOR}`, { method: 'POST', headers: { 'content-type': 'application/json', origin: ORIGIN, cookie }, body: JSON.stringify(body), }); const json = (await res.json()) as Answer; - return { status: res.status, body: json, warnings: [...warn.mock.calls] }; + return { + status: res.status, + body: json, + warnings: [...warn.mock.calls], + errors: [...logError.mock.calls], + providerBodies: providerCall.mock.calls.map(([input]) => (input as { body?: Record }).body), + }; } /** Accepted: 200, a minted client, its row stored with `type`. */ @@ -155,6 +170,9 @@ async function expectStoredAs(answer: Awaited>, stor expect(answer.status, JSON.stringify(answer.body)).toBe(200); expect(envelopeViolations(answer.body)).toEqual([]); expect(answer.body.success).toBe(true); + // Forwarded unchanged: exactly one provider call, carrying the type it stored. + expect(answer.providerBodies).toHaveLength(1); + expect(answer.providerBodies[0]?.application_type).toBe(storedType); const clientId = answer.body.data?.client?.client_id; expect(typeof clientId).toBe('string'); const row = await storedClient(clientId); @@ -207,6 +225,9 @@ beforeAll(async () => { dataEngine: engine as never, plugins: { oidcProvider: true }, } as unknown as ConstructorParameters[0]); + // `getApi()` hands out one api object per manager, so the door calls this spy. + const providerApi = (await manager.getApi()) as unknown as { createOAuthClient(input: unknown): Promise }; + providerCall = vi.spyOn(providerApi, 'createOAuthClient') as unknown as ReturnType; const direct = (path: string, body: unknown) => manager.handleRequest( @@ -307,28 +328,64 @@ describe('[#24773] web and native reach the provider as `application_type`', () await expectStoredAs(answer, 'web'); expect(answer.warnings).toEqual([]); }); + + it('type null: no type at all, so the provider\'s default, web — stored as web, no warning', async () => { + const answer = await register({ name: 'Null typed', redirectURLs: HTTPS, type: null }); + await expectStoredAs(answer, 'web'); + expect(answer.warnings).toEqual([]); + }); }); -describe('[#24773] a type the provider has no `application_type` for keeps today\'s outcome, with one warning', () => { - const UNHONOURED = ['user-agent-based', 'public', 'spa'] as const; - - it.each(UNHONOURED)('%s + https: accepted and stored as web; exactly one warning naming the value', async (type) => { - const answer = await register({ name: `Typed ${type}`, redirectURLs: HTTPS, type }); - const row = await expectStoredAs(answer, 'web'); - // The form's value is stored nowhere on the row, and no public client - // (no secret) was issued for `public`. - expect(Object.entries(row).filter(([, value]) => value === type)).toEqual([]); - expect(row.public === true || row.public === 1, 'the door registered a public client').toBe(false); - expect(row.token_endpoint_auth_method).not.toBe('none'); - expect(answer.warnings).toHaveLength(1); - const [, meta] = answer.warnings[0] as [string, Record]; - expect(meta).toEqual({ requestedType: type, registeredAs: 'web' }); +describe('[#24773] any other type is refused before the provider is called', () => { + // Every value present in the body other than `web` / `native`. Until this + // refusal each one was registered and judged as `web`, with a client secret + // and one server warning. + const REFUSED: ReadonlyArray = [ + ['user-agent-based', 'user-agent-based'], + ['public', 'public'], + ['an unknown string', 'spa'], + ['an empty string', ''], + ['a number', 42], + ['a boolean', true], + ['an object', { application_type: 'native' }], + ]; + + /** Refused by the door itself: 400 INVALID_REQUEST, no provider call, no row, nothing logged. */ + async function expectTypeRefused(answer: Awaited>, rowsBefore: number) { + expect(answer.status, JSON.stringify(answer.body)).toBe(400); + expect(envelopeViolations(answer.body)).toEqual([]); + expect(answer.body.success).toBe(false); + expect(answer.body.error?.code).toBe('INVALID_REQUEST'); + expect(answer.providerBodies, 'the door called the provider for a type it refuses').toEqual([]); + expect((await storedClients()).length, 'a refused registration wrote a client row').toBe(rowsBefore); + expect(answer.warnings, 'the door still warns instead of (or as well as) refusing').toEqual([]); + expect(answer.errors).toEqual([]); + } + + it.each(REFUSED)('%s + https: 400 INVALID_REQUEST, the provider is never called, nothing is stored', async (_label, type) => { + const before = (await storedClients()).length; + await expectTypeRefused(await register({ name: 'Typed', redirectURLs: HTTPS, type }), before); }); - it.each(UNHONOURED)('%s + http://localhost: judged as web — refused, nothing stored; exactly one warning', async (type) => { + it.each(REFUSED)('%s + http://localhost: refused for its type, before the provider\'s web rule can judge the URI', async (_label, type) => { const before = (await storedClients()).length; - const answer = await register({ name: `Local ${type}`, redirectURLs: LOOPBACK, type }); - await expectRefused(answer, before); - expect(answer.warnings).toHaveLength(1); + await expectTypeRefused(await register({ name: 'Local typed', redirectURLs: LOOPBACK, type }), before); + }); + + it('the message names the value, truncated, and both accepted types, and points a loopback redirect at native', async () => { + const long = `${'a'.repeat(64)}TAIL-NEVER-ECHOED`; + const answer = await register({ name: 'Long typed', redirectURLs: HTTPS, type: long }); + const message = answer.body.error?.message ?? ''; + expect(answer.body.error?.code).toBe('INVALID_REQUEST'); + expect(message).toContain('a'.repeat(64)); + expect(message, 'the message echoes the value past its first 64 characters').not.toContain('TAIL'); + for (const subject of ["'web'", "'native'", 'http://localhost', 'http://127.0.0.1', 'http://[::1]']) { + expect(message).toContain(subject); + } + + const short = await register({ name: 'Short typed', redirectURLs: HTTPS, type: 'user-agent-based' }); + expect(short.body.error?.message).toContain('"user-agent-based"'); + const nonString = await register({ name: 'Number typed', redirectURLs: HTTPS, type: 42 }); + expect(nonString.body.error?.message).toContain('(number)'); }); });