-
Notifications
You must be signed in to change notification settings - Fork 0
Use better-auth's universal handler instead of manual API wrappers #581
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
Use better-auth's universal handler instead of manual API wrappers #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
78e6b06
into
copilot/start-better-auth-implementation
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.
Pull request overview
This PR updates @objectstack/plugin-auth to integrate better-auth using its universal handler(Request) -> Response pattern, replacing the prior per-endpoint wrapper approach and routing everything through a single wildcard route.
Changes:
- Replace manual auth endpoint wrappers with
AuthManager.handleRequest(request)delegating directly toauth.handler(). - Switch
AuthPluginroute registration from individual endpoints to a single wildcard route (/api/v1/auth/*) with URL path rewriting before forwarding. - Update docs/examples/tests to reflect the forwarding-based integration and available better-auth endpoints.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Lockfile updates reflecting dependency resolution changes (notably drizzle-orm version). |
| packages/plugins/plugin-auth/src/auth-plugin.ts | Implements wildcard route forwarding and URL rewrite; adds runtime requirement for getRawApp(). |
| packages/plugins/plugin-auth/src/auth-plugin.test.ts | Updates tests to expect wildcard registration via getRawApp().all(...). |
| packages/plugins/plugin-auth/src/auth-manager.ts | Removes placeholder endpoint wrappers; adds handleRequest() and exposes api getter. |
| packages/plugins/plugin-auth/examples/basic-usage.ts | Updates example output to list better-auth endpoints under /api/v1/auth/*. |
| packages/plugins/plugin-auth/README.md | Updates documentation to describe wildcard forwarding and better-auth endpoints. |
| packages/plugins/plugin-auth/ARCHITECTURE.md | Adds rationale/architecture doc for direct forwarding approach. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| // Return error response | ||
| return new Response( | ||
| JSON.stringify({ | ||
| success: false, | ||
| error: err.message, |
Copilot
AI
Feb 10, 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.
The catch-all error handler returns err.message to the client. For auth endpoints this can leak internal details (e.g., adapter errors, stack-adjacent messages). Prefer returning a generic message (and/or an error code / request id) while keeping the detailed error only in logs; optionally gate detailed messages behind an explicit dev/debug flag.
| // Return error response | |
| return new Response( | |
| JSON.stringify({ | |
| success: false, | |
| error: err.message, | |
| // Return generic error response to avoid leaking internal details | |
| return new Response( | |
| JSON.stringify({ | |
| success: false, | |
| error: 'Internal authentication error', |
| expect(mockContext.getService).toHaveBeenCalledWith('http-server'); | ||
| expect(mockHttpServer.post).toHaveBeenCalled(); | ||
| expect(mockHttpServer.get).toHaveBeenCalled(); | ||
| expect(mockHttpServer.getRawApp).toHaveBeenCalled(); | ||
| expect(mockRawApp.all).toHaveBeenCalledWith('/api/v1/auth/*', expect.any(Function)); | ||
| expect(mockContext.logger.info).toHaveBeenCalledWith( |
Copilot
AI
Feb 10, 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.
Current tests only assert that the wildcard route is registered, but they don't exercise the forwarded handler logic (especially the URL/path rewrite) or the new failure mode when getRawApp() is missing. Consider capturing the handler passed to mockRawApp.all and adding assertions that it rewrites /api/v1/auth/... to the expected better-auth path, and add a test that start() throws with the intended message when getRawApp is not available.
| ### Production Ready Features | ||
| - ✅ **Email/Password Authentication** - Handled by better-auth | ||
| - ✅ **OAuth Providers** - Configured via `providers` option | ||
| - ✅ **Session Management** - Automatic session handling | ||
| - ✅ **Password Reset** - Email-based password reset flow |
Copilot
AI
Feb 10, 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.
The README labels several items as "Production Ready Features", but AuthManager.createDatabaseConfig() is still an in-memory placeholder and warns that the adapter integration isn't complete. Until persistence/session storage is implemented, describing the plugin as production-ready (especially for sessions, password reset, etc.) is misleading; consider rewording this section to clarify the current limitations and what is actually production-ready today.
| ```typescript | ||
| // All requests under /api/v1/auth/* are forwarded to better-auth | ||
| rawApp.all('/api/v1/auth/*', async (c) => { | ||
| const request = c.req.raw; // Web standard Request |
Copilot
AI
Feb 10, 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.
The Architecture snippet shows forwarding c.req.raw directly into authManager.handleRequest(request), but the actual implementation rewrites the URL path (strips basePath) before forwarding. This doc/code mismatch may confuse users trying to replicate the integration; update the snippet (and/or add a brief note explaining why the rewrite is needed when mounting under /api/v1/auth).
| const request = c.req.raw; // Web standard Request | |
| const basePath = '/api/v1/auth'; | |
| // better-auth expects paths like `/sign-in`, not `/api/v1/auth/sign-in`, | |
| // so we strip the mounted basePath before forwarding the request. | |
| const originalUrl = new URL(c.req.url); | |
| const strippedPathname = originalUrl.pathname.replace(basePath, '') || '/'; | |
| const rewrittenUrl = new URL(strippedPathname + originalUrl.search, originalUrl.origin); | |
| // Create a new Request with the rewritten URL, preserving the original request details | |
| const request = new Request(rewrittenUrl, c.req.raw); |
|
|
||
| - [Better-Auth Documentation](https://www.better-auth.com/docs) | ||
| - [PR #580](https://github.com/objectstack-ai/spec/pull/580) - Initial better-auth integration | ||
| - Analysis Document: `/tmp/better-auth-approach-analysis.md` |
Copilot
AI
Feb 10, 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.
This references an analysis document under /tmp/..., which won’t exist for readers of the repo and can’t be accessed in most environments. Consider removing this reference or replacing it with a committed document/link in the repository (issue, PR comment permalink, or a file in this package).
| - Analysis Document: `/tmp/better-auth-approach-analysis.md` | |
| - Architectural analysis: Captured in this document and in PR #580 discussion |
| // Register wildcard route to forward all auth requests to better-auth | ||
| // Better-auth expects requests at its baseURL, so we need to preserve the full path | ||
| rawApp.all(`${basePath}/*`, async (c: any) => { | ||
| try { |
Copilot
AI
Feb 10, 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.
basePath is interpolated directly into the wildcard route (rawApp.all(${basePath}/*, ...)). If a caller configures basePath with a trailing slash (e.g. /api/v1/auth/), this becomes /api/v1/auth//* and may not match requests. Consider normalizing basePath (trim trailing /, ensure leading /) once before registering the route and using that normalized value for both routing and path stripping.
PR #580 implemented manual wrapper methods for each auth endpoint. Better-auth provides a universal handler designed for this use case.
Changes
AuthManager - Replaced manual API wrappers with direct handler delegation:
login,register,logout, etc.)handleRequest(request: Request): Promise<Response>that forwards toauth.handler()apigetter for programmatic access to better-auth methodsAuthPlugin - Switched from individual routes to wildcard routing:
basePath/*/api/v1/auth/sign-in/email→/sign-in/emailfor better-auth compatibilitygetRawApp()wildcard support)Implementation
Impact
Documentation
ARCHITECTURE.mdexplaining the decision rationaleREADME.mdwith better-auth endpoint documentationOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.