🛂 server: add better auth is an authentication framework#881
🛂 server: add better auth is an authentication framework#881nfmelendez wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 128b4ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust authentication and authorization system by integrating the 'better-auth' framework. It enables advanced features such as Sign-In with Ethereum (SIWE) and comprehensive organization management, allowing users to create organizations and manage members with distinct roles. The changes span across database schema, API endpoints, middleware, and user-facing documentation, laying the groundwork for secure and scalable user and organization interactions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds BetterAuth-based SIWE authentication and organization support: new Drizzle Postgres schema and relations, exported database and authAdapter, BetterAuth configuration and routes, middleware session fallback, OpenAPI security entry, docs, and three changeset files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Auth as BetterAuth Handler
participant Server as App Server
participant DB as Database
participant Sentry as Sentry
Client->>Auth: GET /api/auth/nonce
Auth->>DB: store nonce
DB-->>Auth: nonce
Auth-->>Client: nonce
Client->>Client: sign SIWE message
Client->>Auth: POST /api/auth/verify (signed message)
Auth->>Auth: parse & verify chainId/address/signature
alt valid
Auth->>DB: create/update user & session
DB-->>Auth: session
Auth->>Server: set __Secure-better-auth.session_token
Server-->>Client: success response
else invalid
Auth->>Sentry: capture verification error
Sentry-->>Auth: ack
Auth-->>Client: unauthorized
end
Client->>Server: POST /api/auth/create-organization (with session)
Server->>DB: create organization & member
DB-->>Server: organization created
Server-->>Client: organization id/details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cruzdanilo
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
✅ All tests passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac66a631-8798-4e64-80e3-76561a0be138
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/cool-snakes-reply.md.changeset/pretty-chicken-hang.md.changeset/rare-pears-sort.mddocs/astro.config.tsdocs/src/content/docs/organization-authentication.mdserver/database/index.tsserver/database/schema.tsserver/index.tsserver/middleware/auth.tsserver/package.jsonserver/script/openapi.tsserver/utils/auth.ts
nfmelendez
force-pushed
the
better-auth
branch
2 times, most recently
from
c2defde to
c93fb20
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nfmelendez
force-pushed
the
better-auth
branch
2 times, most recently
from
6d12264 to
811d11f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/middleware/auth.ts (1)
10-19:⚠️ Potential issue | 🔴 CriticalType signature doesn't match runtime behavior—when BetterAuth session is used,
credentialIdis undefined, not the string promised by the middleware type.The middleware declares
{ out: { cookie: { credentialId: string } } }, but the BetterAuth fallback path (lines 14–17) callsnext()without settingcredentialId. All nine endpoints usingauth()destructure{ credentialId }and will receiveundefinedwhen a BetterAuth session is used.Per the design intent (noted in prior review), this fallback is for endpoints that don't require
credentialId. However, every current endpoint usingauth()does require it. Either:
- Make the type accurate:
{ credentialId?: string }and add conditional logic in endpoints to handle both auth methods- Separate concerns: create distinct middleware for credential-based vs session-based authentication
- Confirm whether the BetterAuth path is intended to be unreachable for these endpoints
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c01d4f7-1f82-4e51-b6b8-fe3ea4aeb76a
📒 Files selected for processing (8)
.changeset/rare-pears-sort.mddocs/astro.config.tsdocs/src/content/docs/organization-authentication.mdserver/database/index.tsserver/index.tsserver/middleware/auth.tsserver/script/openapi.tsserver/utils/auth.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (!credentialId) { | ||
| const session = await betterAuth.api.getSession({ headers: c.req.raw.headers }); | ||
| if (session) { | ||
| await next(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: The auth middleware allows users authenticated via betterAuth to proceed without setting credentialId in validated data, which downstream handlers expect.
Severity: MEDIUM
Suggested Fix
Modify the middleware to ensure credentialId is set from the betterAuth session before calling next(). Alternatively, use separate routing to prevent betterAuth-authenticated users from reaching handlers that require credentialId.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/middleware/auth.ts#L12-L17
Potential issue: In the `auth` middleware, if a user authenticates via the
`betterAuth.api.getSession()` path, the middleware calls `next()` without adding the
`credentialId` to the validated cookie data. Downstream route handlers that use this
middleware expect `credentialId` to be present and attempt to destructure it from
`c.req.valid("cookie")`. When it's missing, `credentialId` will be `undefined`, leading
to incorrect behavior or potential cascading errors in at least six route files that
depend on it.
Did we get this right? 👍 / 👎 to inform future reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!credentialId) { | ||
| const session = await betterAuth.api.getSession({ headers: c.req.raw.headers }); | ||
| if (session) { | ||
| await next(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Auth middleware skips setting credentialId for better-auth sessions, breaking all downstream handlers
When a user authenticates via a better-auth session (no legacy credential_id cookie), the middleware at lines 14-16 calls next() without calling c.req.addValidatedData("cookie", { credentialId }). Every downstream handler guarded by auth() immediately destructures credentialId from c.req.valid("cookie") — e.g. server/api/card.ts:199, server/api/ramp.ts:45, server/api/activity.ts:87, server/api/kyc.ts:51, server/api/pax.ts:30, server/api/passkey.ts:30 — and uses it for database lookups like where: eq(credentials.id, credentialId). With credentialId being undefined, these queries match nothing, and the handlers return 500 ("no credential") or 400 errors. The authentication appears to succeed but every protected operation silently fails.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .route("/pax", pax) | ||
| .route("/ramp", ramp); | ||
| .route("/ramp", ramp) | ||
| .on(["POST", "GET"], "/auth/*", (c) => auth.handler(c.req.raw)); |
There was a problem hiding this comment.
🚩 Wildcard /auth/* route could shadow existing auth sub-routes
The new .on(["POST", "GET"], "/auth/*", (c) => auth.handler(c.req.raw)) at server/api/index.ts:31 is registered after .route("/auth/registration", registration) and .route("/auth/authentication", authentication) at lines 23-24. In Hono's trie-based router, more specific paths typically take precedence over wildcards, so existing registration/authentication endpoints should still work. However, if better-auth registers any routes that overlap with sub-paths of /auth/registration/* or /auth/authentication/*, there could be unexpected behavior. Additionally, all HTTP methods other than POST/GET to /auth/* sub-paths will not reach better-auth (only POST and GET are handled). This is probably fine for better-auth's API surface but worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 128b4ece93
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (session) { | ||
| await next(); | ||
| return; |
There was a problem hiding this comment.
When credential_id is missing, this branch now accepts any Better Auth session and calls next() without adding validated cookie data, but downstream handlers still assume c.req.valid("cookie").credentialId exists (for example, server/api/pax.ts and other auth()-protected routes). Requests authenticated only via the new SIWE session cookie will therefore hit protected endpoints with no credential id and fail as 500/"no credential" instead of a clean 401 or mapped identity, which breaks the newly introduced auth path for existing API routes.
Useful? React with 👍 / 👎.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Security