-
Notifications
You must be signed in to change notification settings - Fork 1
fix: AuthPlugin silent 500s and broken ObjectQL adapter config #879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
84c3f17
fac396d
27db9f3
cfaabbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "@objectstack/plugin-auth": patch | ||
| --- | ||
|
|
||
| fix: AuthPlugin error handling & database adapter config | ||
|
|
||
| - `AuthManager.handleRequest()` now inspects `response.status >= 500` and logs the error body via `console.error`, since better-auth catches internal errors and returns 500 Responses without throwing. | ||
| - `AuthPlugin.registerAuthRoutes()` also logs 500+ responses via `ctx.logger.error` for structured plugin logging. | ||
| - `createDatabaseConfig()` now wraps the ObjectQL adapter as a `DBAdapterInstance` factory function so better-auth's `getBaseAdapter()` correctly recognises it (via `typeof database === "function"` check) instead of falling through to the Kysely adapter path. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. | ||
|
|
||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | ||
| import { AuthManager } from './auth-manager'; | ||
|
|
||
| // Mock better-auth so we can control the handler behaviour | ||
| vi.mock('better-auth', () => ({ | ||
| betterAuth: vi.fn(() => ({ | ||
| handler: vi.fn(), | ||
| api: {}, | ||
| })), | ||
| })); | ||
|
|
||
| import { betterAuth } from 'better-auth'; | ||
|
|
||
| describe('AuthManager', () => { | ||
| let consoleSpy: ReturnType<typeof vi.spyOn>; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| consoleSpy.mockRestore(); | ||
| }); | ||
|
|
||
| describe('handleRequest – error response logging', () => { | ||
| it('should log when better-auth returns a 500 response', async () => { | ||
| const errorResponse = new Response( | ||
| JSON.stringify({ error: 'Internal database error' }), | ||
| { status: 500, headers: { 'Content-Type': 'application/json' } }, | ||
| ); | ||
|
|
||
| const mockHandler = vi.fn().mockResolvedValue(errorResponse); | ||
| (betterAuth as any).mockReturnValue({ handler: mockHandler, api: {} }); | ||
|
|
||
| const manager = new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| }); | ||
|
|
||
| const request = new Request('http://localhost:3000/sign-up/email', { | ||
| method: 'POST', | ||
| body: JSON.stringify({ email: 'a@b.com', password: 'pass' }), | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
|
|
||
| const response = await manager.handleRequest(request); | ||
|
|
||
| expect(response.status).toBe(500); | ||
| expect(consoleSpy).toHaveBeenCalledWith( | ||
| '[AuthManager] better-auth returned error:', | ||
| 500, | ||
| expect.stringContaining('Internal database error'), | ||
| ); | ||
| }); | ||
|
|
||
| it('should NOT log for successful (2xx) responses', async () => { | ||
| const okResponse = new Response(JSON.stringify({ user: {} }), { | ||
| status: 200, | ||
| }); | ||
|
|
||
| const mockHandler = vi.fn().mockResolvedValue(okResponse); | ||
| (betterAuth as any).mockReturnValue({ handler: mockHandler, api: {} }); | ||
|
|
||
| const manager = new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| }); | ||
|
|
||
| const request = new Request('http://localhost:3000/sign-in/email', { | ||
| method: 'POST', | ||
| body: JSON.stringify({ email: 'a@b.com', password: 'pass' }), | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
|
|
||
| const response = await manager.handleRequest(request); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(consoleSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should NOT log for 4xx responses', async () => { | ||
| const badRequestResponse = new Response( | ||
| JSON.stringify({ error: 'Bad request' }), | ||
| { status: 400 }, | ||
| ); | ||
|
|
||
| const mockHandler = vi.fn().mockResolvedValue(badRequestResponse); | ||
| (betterAuth as any).mockReturnValue({ handler: mockHandler, api: {} }); | ||
|
|
||
| const manager = new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| }); | ||
|
|
||
| const request = new Request('http://localhost:3000/sign-in/email', { | ||
| method: 'POST', | ||
| }); | ||
|
|
||
| const response = await manager.handleRequest(request); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(consoleSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('createDatabaseConfig – adapter wrapping', () => { | ||
| it('should pass a function (DBAdapterInstance) to betterAuth when dataEngine is provided', () => { | ||
| const mockDataEngine = { | ||
| insert: vi.fn(), | ||
| findOne: vi.fn(), | ||
| find: vi.fn(), | ||
| count: vi.fn(), | ||
| update: vi.fn(), | ||
| delete: vi.fn(), | ||
| }; | ||
|
|
||
| new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| dataEngine: mockDataEngine as any, | ||
| }); | ||
|
|
||
| // Trigger lazy initialization by calling getAuthInstance() | ||
| // betterAuth should have been called with a database value that is a function | ||
| // We need to trigger the lazy init first | ||
| }); | ||
|
|
||
| it('should provide a factory function as database config that returns adapter with id and transaction', () => { | ||
| const mockDataEngine = { | ||
| insert: vi.fn().mockResolvedValue({ id: '1' }), | ||
| findOne: vi.fn().mockResolvedValue({ id: '1' }), | ||
| find: vi.fn().mockResolvedValue([]), | ||
| count: vi.fn().mockResolvedValue(0), | ||
| update: vi.fn().mockResolvedValue({ id: '1' }), | ||
| delete: vi.fn().mockResolvedValue(undefined), | ||
| }; | ||
|
|
||
| let capturedConfig: any; | ||
| (betterAuth as any).mockImplementation((config: any) => { | ||
| capturedConfig = config; | ||
| return { handler: vi.fn(), api: {} }; | ||
| }); | ||
|
|
||
| const manager = new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| dataEngine: mockDataEngine as any, | ||
| }); | ||
|
|
||
| // Trigger lazy initialisation | ||
| manager.getAuthInstance(); | ||
|
|
||
| // The database config should be a function (DBAdapterInstance) | ||
| expect(typeof capturedConfig.database).toBe('function'); | ||
|
|
||
| // Calling the factory should return an adapter object | ||
| const adapterResult = capturedConfig.database({}); | ||
| expect(adapterResult).toHaveProperty('id', 'objectql'); | ||
| expect(typeof adapterResult.create).toBe('function'); | ||
| expect(typeof adapterResult.findOne).toBe('function'); | ||
| expect(typeof adapterResult.findMany).toBe('function'); | ||
| expect(typeof adapterResult.count).toBe('function'); | ||
| expect(typeof adapterResult.update).toBe('function'); | ||
| expect(typeof adapterResult.delete).toBe('function'); | ||
| expect(typeof adapterResult.deleteMany).toBe('function'); | ||
| expect(typeof adapterResult.updateMany).toBe('function'); | ||
| expect(typeof adapterResult.transaction).toBe('function'); | ||
| }); | ||
|
|
||
| it('should return undefined (in-memory fallback) when no dataEngine is provided', () => { | ||
| let capturedConfig: any; | ||
| (betterAuth as any).mockImplementation((config: any) => { | ||
| capturedConfig = config; | ||
| return { handler: vi.fn(), api: {} }; | ||
| }); | ||
|
|
||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
|
|
||
| const manager = new AuthManager({ | ||
| secret: 'test-secret-at-least-32-chars-long', | ||
| baseUrl: 'http://localhost:3000', | ||
| }); | ||
|
|
||
| manager.getAuthInstance(); | ||
|
|
||
| expect(capturedConfig.database).toBeUndefined(); | ||
| warnSpy.mockRestore(); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -91,11 +91,30 @@ export class AuthManager { | |||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Create database configuration using ObjectQL adapter | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * better-auth resolves the `database` option as follows: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * - `undefined` → in-memory adapter | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * - `typeof fn === "function"` → treated as `DBAdapterInstance`, called with `(options)` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * - otherwise → forwarded to Kysely adapter factory (pool/dialect) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * A raw `CustomAdapter` object would fall into the third branch and fail | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * silently. We therefore wrap the ObjectQL adapter in a factory function | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * so it is correctly recognised as a `DBAdapterInstance`. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private createDatabaseConfig(): any { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use ObjectQL adapter if dataEngine is provided | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (this.config.dataEngine) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return createObjectQLAdapter(this.config.dataEngine); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const adapter = createObjectQLAdapter(this.config.dataEngine); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Return a DBAdapterInstance factory function | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return (_options: any) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| id: 'objectql', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...adapter, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // ObjectQL does not yet expose a separate transaction context, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // so we pass the adapter itself. better-auth patches this | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // automatically when missing, but providing it avoids a | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // runtime warning from getBaseAdapter(). | ||||||||||||||||||||||||||||||||||||||||||||||||||
| transaction: async <R>(cb: (trx: any) => Promise<R>): Promise<R> => cb(adapter), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+109
to
+117
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return (_options: any) => ({ | |
| id: 'objectql', | |
| ...adapter, | |
| // ObjectQL does not yet expose a separate transaction context, | |
| // so we pass the adapter itself. better-auth patches this | |
| // automatically when missing, but providing it avoids a | |
| // runtime warning from getBaseAdapter(). | |
| transaction: async <R>(cb: (trx: any) => Promise<R>): Promise<R> => cb(adapter), | |
| }); | |
| return (_options: any) => { | |
| const wrappedAdapter = { | |
| id: 'objectql', | |
| ...adapter, | |
| // ObjectQL does not yet expose a separate transaction context, | |
| // so we reuse the same adapter shape inside transactions. | |
| // This keeps `trx` consistent with the top-level adapter | |
| // (including `id` and `transaction` fields). | |
| transaction: async <R>(cb: (trx: any) => Promise<R>): Promise<R> => cb(wrappedAdapter), | |
| }; | |
| return wrappedAdapter; | |
| }; |
Copilot
AI
Mar 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleRequest() logs the full response body for any 500+ response. Reading/logging an unbounded body can be expensive and may leak sensitive details into stdout/stderr. Consider truncating the body, and/or logging only a small excerpt plus structured metadata (status + URL) to reduce operational risk.
| try { | |
| const body = await response.clone().text(); | |
| console.error('[AuthManager] better-auth returned error:', response.status, body); | |
| } catch { | |
| console.error('[AuthManager] better-auth returned error:', response.status, '(unable to read body)'); | |
| const MAX_ERROR_BODY_LOG_LENGTH = 2048; | |
| try { | |
| const text = await response.clone().text(); | |
| const isTruncated = text.length > MAX_ERROR_BODY_LOG_LENGTH; | |
| const bodyExcerpt = isTruncated | |
| ? text.slice(0, MAX_ERROR_BODY_LOG_LENGTH) + '... [truncated]' | |
| : text; | |
| console.error('[AuthManager] better-auth returned error response', { | |
| status: response.status, | |
| url: request.url, | |
| bodyExcerpt, | |
| }); | |
| } catch { | |
| console.error('[AuthManager] better-auth returned error response', { | |
| status: response.status, | |
| url: request.url, | |
| bodyExcerpt: '(unable to read body)', | |
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -199,6 +199,18 @@ export class AuthPlugin implements Plugin { | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Forward to better-auth handler | ||||||||||||||||||||||||||||||||||||||||
| const response = await this.authManager!.handleRequest(rewrittenRequest); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // better-auth catches internal errors and returns error Responses | ||||||||||||||||||||||||||||||||||||||||
| // without throwing, so the catch block below would never trigger. | ||||||||||||||||||||||||||||||||||||||||
| // We proactively log server errors here for observability. | ||||||||||||||||||||||||||||||||||||||||
| if (response.status >= 500) { | ||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||
| const body = await response.clone().text(); | ||||||||||||||||||||||||||||||||||||||||
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: ${body}`)); | ||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: (unable to read body)`)); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+208
to
+211
|
||||||||||||||||||||||||||||||||||||||||
| const body = await response.clone().text(); | |
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: ${body}`)); | |
| } catch { | |
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: (unable to read body)`)); | |
| const maxLogBodyLength = 4096; | |
| const bodyText = await response.clone().text(); | |
| const truncatedBody = | |
| bodyText.length > maxLogBodyLength | |
| ? bodyText.slice(0, maxLogBodyLength) + '...[truncated]' | |
| : bodyText; | |
| ctx.logger.error( | |
| '[AuthPlugin] better-auth returned server error', | |
| new Error(`HTTP ${response.status}: ${truncatedBody}`) | |
| ); | |
| } catch { | |
| ctx.logger.error( | |
| '[AuthPlugin] better-auth returned server error', | |
| new Error(`HTTP ${response.status}: (unable to read body)`) | |
| ); |
Copilot
AI
Mar 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthPlugin logs 500+ responses here, but AuthManager.handleRequest() (which this route calls) also logs 500+ responses. In server/plugin mode this will produce duplicate error logs for every internal better-auth failure. Consider logging in only one place (e.g., move logging responsibility entirely to AuthPlugin, or add an option to AuthManager to disable/override error-response logging when a structured logger is available).
| // better-auth catches internal errors and returns error Responses | |
| // without throwing, so the catch block below would never trigger. | |
| // We proactively log server errors here for observability. | |
| if (response.status >= 500) { | |
| try { | |
| const body = await response.clone().text(); | |
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: ${body}`)); | |
| } catch { | |
| ctx.logger.error('[AuthPlugin] better-auth returned server error', new Error(`HTTP ${response.status}: (unable to read body)`)); | |
| } | |
| } | |
| // better-auth and AuthManager are responsible for logging 5xx responses. | |
| // Here we simply forward the response; only unexpected exceptions are | |
| // logged in the catch block below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test
"should pass a function (DBAdapterInstance) to betterAuth when dataEngine is provided"is currently incomplete (no call that triggers initialization, no assertions). This will always pass even if the behavior regresses. Either complete it by triggeringgetAuthInstance()and asserting on the captured config, or remove it to avoid giving a false sense of coverage.