Repository navigation
feat(core): stability hardening, system admin, and test coverage - #16
Conversation
- Added System Admin Module (Controller, Guard, Tests) - Refactored AuthGuard and Session Management to fix persistence - Fixed Lint and Type errors across API (removed 'any') - Improved API Test Coverage (Passed 22/22 suites) - Frontend: Admin Layout fixes and User Management Refactor
📝 WalkthroughWalkthroughAdds DB columns (invitation.createdAt, user.system_role), header-based session extraction/enrichment, invite acceptance and complete-invite flows, Invitations.list endpoint and invitation listing in frontend user lists, SystemAdmin guard/module/controller with admin APIs, various scripts, utilities, and tests; updates snapshots and routing/resource paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant BetterAuth
participant DB
User->>Frontend: Click invite link (/invite/accept?id=...)
Frontend->>Frontend: parse inviteId
alt authenticated
Frontend->>API: POST /invitations/accept (forward headers)
API->>BetterAuth: getSessionFromHeaders(request.headers)
BetterAuth->>DB: query session & user (token)
DB-->>BetterAuth: session + user (with systemRole)
BetterAuth-->>API: {session,user}
API->>DB: accept invitation (update)
DB-->>API: success
API-->>Frontend: 200 OK
Frontend->>Frontend: redirect /dashboard
else not authenticated
Frontend->>Frontend: redirect /signup?to=/invite/accept...&email=...
User->>Frontend: submit complete-invite
Frontend->>API: POST /auth/complete-invite
API->>BetterAuth: createUser + forceVerifyEmail
BetterAuth->>DB: insert user & set emailVerified
DB-->>BetterAuth: new user
BetterAuth->>BetterAuth: sign-in -> session + cookie
BetterAuth-->>API: {session,user,cookie}
API->>DB: accept invitation
DB-->>API: success
API-->>Frontend: 200 OK + Set-Cookie
Frontend->>Frontend: update auth state, redirect /dashboard
end
sequenceDiagram
participant Admin
participant AdminUI
participant API
participant Guard
participant BetterAuth
participant DB
Admin->>AdminUI: GET /admin/users?page=1&pageSize=10
AdminUI->>API: request
API->>Guard: canActivate(context)
Guard->>BetterAuth: getSessionFromHeaders(headers)
BetterAuth->>DB: query session & user
DB-->>BetterAuth: session + user (systemRole)
BetterAuth-->>Guard: {session,user}
Guard->>Guard: check user.systemRole === 'platform_admin'
alt allowed
API->>DB: query users (limit/offset)
DB-->>API: users + total
API-->>AdminUI: {data,total}
else denied
API-->>AdminUI: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used🧬 Code graph analysis (2)apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (3)
apps/api/src/modules/auth/identity-provider.abstract.ts (2)
🪛 Biome (2.1.2)apps/api/src/modules/system-admin/system-admin.controller.spec.ts[error] 99-99: Do not add then to an object. (lint/suspicious/noThenProperty) [error] 114-114: Do not add then to an object. (lint/suspicious/noThenProperty) [error] 136-136: Do not add then to an object. (lint/suspicious/noThenProperty) [error] 156-156: Do not add then to an object. (lint/suspicious/noThenProperty) 🔇 Additional comments (30)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/web/src/pages/admin/tenants/TenantListPage.tsx (1)
57-60: Create Tenant button has no click handler.The button is rendered but lacks an
onClickhandler or navigation logic, so it currently does nothing when clicked. If this is intentional for a future feature, consider either disabling the button or adding a placeholder/navigation.Suggested options
Option 1: Disable until implemented
- <Button> + <Button disabled title="Coming soon">Option 2: Add navigation (if route exists)
+ const navigate = useNavigate(); + // ... - <Button> + <Button onClick={() => navigate("/admin/tenants/create")}>apps/web/src/pages/LoginPage.tsx (1)
109-128: Inconsistent redirect behavior for Google OAuth login.The Google OAuth flow hardcodes
callbackURL: '/dashboard', while email/password login respects the?toparameter and role-based routing. This means:
- Platform admins using Google login always land on
/dashboardinstead of/admin- The
?toparameter is ignored for social loginsConsider aligning the behavior or documenting this as intentional.
Suggested approach to align behavior
type="button" onClick={async () => { try { + const searchParams = new URLSearchParams(window.location.search); + const toParam = searchParams.get('to'); + const isValidRedirect = toParam && toParam.startsWith('/') && !toParam.startsWith('//'); + // Role-based fallback would need to be handled in the callback handler + const callbackURL = isValidRedirect + ? `${window.location.origin}${toParam}` + : `${window.location.origin}/dashboard`; await authClient.signIn.social({ provider: "google", - callbackURL: `${window.location.origin}/dashboard`, + callbackURL,Note: Full role-based routing for OAuth may require handling in the OAuth callback endpoint since
systemRoleisn't known until after authentication.apps/web/src/pages/admin/users/UserEdit.tsx (1)
28-34: Remove duplicate comments.Lines 28-31 and 32-34 contain identical comments. This appears to be an unintentional copy-paste.
Proposed fix
// We can use a simple controlled form approach for now without react-hook-form // to avoid extra dependencies, or just plain HTML form submission. // Refine's onFinish accepts a values object. - - // We can use a simple controlled form approach for now without react-hook-form - // to avoid extra dependencies, or just plain HTML form submission. - // Refine's onFinish accepts a values object.apps/web/src/modules/users/Users.tsx (1)
42-73: Status and verification rendering don't handle all possible user states.The
statusfield inUserTableItemsupports four values:"active","pending","disabled", and"suspended"(and can be undefined). However, the Status cell only checks for'pending'and renders everything else—including"disabled"and"suspended"—as "Active." This mislabels inactive users.Additionally, the field is optional; if
statusis undefined, it will silently render as "Active."Consider handling all states explicitly:
Example fix
<TableCell> {user.status === 'pending' ? ( <Badge variant="outline" className="text-yellow-600 border-yellow-200 bg-yellow-50">Pending</Badge> ) : user.status === 'active' ? ( <Badge variant="outline" className="text-green-600 border-green-200 bg-green-50">Active</Badge> ) : user.status === 'disabled' ? ( <Badge variant="outline" className="text-red-600 border-red-200 bg-red-50">Disabled</Badge> ) : user.status === 'suspended' ? ( <Badge variant="outline" className="text-orange-600 border-orange-200 bg-orange-50">Suspended</Badge> ) : ( <Badge variant="outline" className="text-slate-600 border-slate-200 bg-slate-50">Unknown</Badge> )} </TableCell>apps/web/src/pages/SignupPage.tsx (1)
43-123: Remove password-bearing debug logs.The invite payload log includes the plaintext password; it should never be printed.
🔧 Suggested fix
- console.log("Submitting Payload:", payload); // DEBUGGINGapps/api/src/modules/invitations/invitations.controller.ts (1)
38-42: Address the TODO: Unguarded endpoint exposes invitation data.The
get()endpoint lacks authentication, allowing anyone to retrieve invitation details by ID. This could leak sensitive information (email addresses, organization context). Consider adding@UseGuards(AuthGuard)or validating that the requester has permission to view the invitation.🔒 Suggested fix
`@Get`(':id') - // We should probably guard this or validate the ID format is safe/expected + `@UseGuards`(AuthGuard) async get(`@Param`('id') id: string) { return this.invitationsService.get(id); }apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
320-362: Ensurex-inviter-idis set even when headers are provided.When the invitations controller passes request headers to this method (line 30 in invitations.controller.ts), the
||operator short-circuits and thex-inviter-idheader is never added. This breaks invite attribution since the inviter ID will be missing from the Better Auth API call.The fix correctly merges the inviter ID into the headers:
Proposed fix
- return await api.createInvitation({ + const headers = new Headers(payload.headers ?? undefined); + headers.set('x-inviter-id', payload.inviterId); + + return await api.createInvitation({ body: { email: payload.email, role: payload.role, organizationId: payload.organizationId, expiresIn: payload.expiresIn, }, - headers: - payload.headers || - new Headers({ - 'x-inviter-id': payload.inviterId, - }), + headers, });
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/auth.controller.ts`:
- Around line 42-57: The login controller currently returns the entire result
from authProvider.login including result.cookie which would serialize the
Set-Cookie value into the response body; update the login method (and any other
handlers in the same file noted around lines 82-113) to set the Set-Cookie
header from result.cookie but strip the cookie property before returning (e.g.,
destructure { cookie, ...safeResult } = result and return safeResult) so the
cookie is only sent via header and not included in the JSON body.
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 87-93: The cookie config in advanced.defaultCookieAttributes
currently hard-codes secure: false; change it to derive from environment (e.g.
secure: process.env.SESSION_COOKIE_SECURE === 'true' || process.env.NODE_ENV ===
'production') so cookies are secure in production but remain usable in local
dev; update the advanced.defaultCookieAttributes object in
better-auth.provider.ts accordingly and ensure any env var name used is
documented/loaded by your config.
In `@apps/api/src/modules/system-admin/system-admin.controller.ts`:
- Around line 20-21: Remove the temporary console.log debug statements that
print internal database objects (the two lines using console.log with this.db
and this.db?.query) from system-admin.controller.ts; either delete both
console.log calls or replace them with a proper levelled logger (e.g.,
this.logger.debug) or a NODE_ENV/dev-only guard so internal details are not
emitted in production, ensuring the change targets the debug prints in the
SystemAdminController where this.db is referenced.
In `@apps/api/src/scripts/bootstrap-admin.ts`:
- Around line 96-100: The current bootstrap-admin.ts prints the admin password
(PASSWORD) to stdout (console.log), which leaks secrets; remove the line that
prints `Password: ${PASSWORD}` and instead either print a non-sensitive hint
(e.g., "Password stored in environment variable ADMIN_PASSWORD") or a masked
value (e.g., show only last 4 chars) using the EMAIL and PASSWORD variables;
ensure no plaintext secret appears in console output and update any related
messages that referenced the removed line.
- Around line 16-20: Replace hard-coded credentials in bootstrap-admin.ts
(API_URL, EMAIL, PASSWORD, NAME) with values read from environment variables
(e.g., process.env.API_URL, process.env.ADMIN_EMAIL, process.env.ADMIN_PASSWORD,
process.env.ADMIN_NAME) and add a fail-fast check that throws or logs a clear
error and exits if any required env var is missing; update any functions or
calls that rely on those constants (e.g., the admin bootstrap routine) to use
the env-derived values instead.
In `@apps/api/src/scripts/reset-db.ts`:
- Around line 41-45: The catch block in reset-db.ts currently logs errors for
the DB truncation but returns a zero exit status; update the catch handler
around the truncation logic so that after logging the error (err) you call
process.exit(1) to return a non-zero exit code (or alternatively rethrow the
error so the process exits with failure), ensuring the finally block still
awaits client.end() on the client variable; locate the catch in the script
around the client usage in reset-db.ts and add process.exit(1) (or rethrow)
after console.error('Error resetting DB:', err).
- Around line 13-17: The reset script currently reads process.env.DATABASE_URL
into dbUrl and exits if missing but has no guard against running in production;
update the top of reset-db.ts to detect production (e.g., process.env.NODE_ENV
=== 'production' or process.env.DEPLOYMENT === 'production') and refuse to run
unless an explicit confirmation is provided (for example require a specific env
var like RESET_DB_CONFIRM set to "true" or prompt the user for a typed
confirmation via readline). Keep the existing dbUrl existence check, and ensure
the new guard logs a clear message and exits with non-zero status if
confirmation is not present or NODE_ENV indicates production so accidental
production runs are prevented.
In `@apps/web/src/layouts/AdminLayout.tsx`:
- Around line 153-154: The AdminLayout currently returns null when the user is
missing or not a platform admin, causing a blank screen; update AdminLayout to
render a small fallback UI instead of null — e.g., show an "Unauthorized"
message or a "Redirecting..." spinner and optionally trigger a redirect when
isAuthenticated is true but user is null or user.systemRole !==
'platform_admin'; locate the conditional in AdminLayout (check uses of
isAuthenticated, user, and systemRole) and replace the null return with a
concise fallback component or call a navigate/redirect helper so users see
status rather than a dead blank page.
In `@apps/web/src/lib/auth/AuthProvider.tsx`:
- Around line 25-27: Remove the sensitive console.logging around session
payloads in AuthProvider: do not log the raw `data` or full session from
`authClient.getSession()` (these may contain tokens/PII); delete or replace the
two console.log calls shown and, if you need telemetry, log only non-sensitive
status (e.g., success/failure) or use an environment-gated debug log that never
emits tokens—refer to the `authClient.getSession()` call and the `data`/`error`
variables in `AuthProvider.tsx` to locate the lines to remove/replace.
In `@apps/web/src/pages/LoginPage.tsx`:
- Around line 57-69: The redirect uses searchParams.get('to') directly creating
an open-redirect; update the logic around searchParams, redirectUrl and navigate
to validate the "to" value is a safe relative path before using it.
Specifically, when reading const to = searchParams.get('to'), ensure it is
non-empty, begins with a single '/' (not '//' which is protocol-relative), does
not contain "://" and does not attempt to navigate outside the app (e.g., no
back-path traversal like '/../'); only then set redirectUrl = to, otherwise fall
back to the role-based fallback; keep LoginUser, systemRole and navigate
unchanged but replace direct use of searchParams.get('to') with this validated
value.
- Around line 20-27: The auto-redirect useEffect in LoginPage is using the
form's "loading" state (the local submission flag) instead of the auth
initialization flag from useAuth(); update the effect to check "if (user &&
!isLoading)" and include "isLoading" in the dependency array (replace "loading"
with "isLoading") so the redirect waits for auth initialization to finish;
ensure the component is reading isLoading from useAuth() (the same hook that
provides user) and keep the local form "loading" variable unchanged for
submission state.
In `@apps/web/src/pages/public/AcceptInvitePage.tsx`:
- Around line 65-68: The constructed target URL in AcceptInvitePage (variable
target built from inviteId and searchParams.get('email')) fails to URL-encode
the email query param; update the code that builds target so the email value is
wrapped with encodeURIComponent (e.g.,
encodeURIComponent(searchParams.get('email') || '')) before concatenation so
special characters like '+' are preserved, then call navigate(target, { replace:
true }) as before.
- Around line 31-58: The silentAccept function should include authentication
when calling /invitations/accept: obtain the token from useAuth() (the hook
exposing token) and add an Authorization: `Bearer ${token}` header and
credentials: 'include' to the fetch options (still send body with invitationId).
Update the fetch call in silentAccept (and ensure inviteId and navigate usage
remains) and instead of fully swallowing errors, log the fetch response error or
non-2xx status before navigating so authentication failures are visible in
console.
🧹 Nitpick comments (19)
apps/web/vite.config.ts (1)
23-31: Make the proxy target configurable via env (optional).Hardcoding the API port makes multi-env setups brittle. Consider reading from Vite env so devs can switch targets without code edits.
Example refactor
-import { defineConfig } from 'vite' +import { defineConfig, loadEnv } from 'vite' -export default defineConfig({ +export default defineConfig(({ mode }) => { + const env = loadEnv(mode, process.cwd(), '') + const apiTarget = env.VITE_API_URL ?? 'http://127.0.0.1:3000' + return { plugins: [ react(), checker({ typescript: true, }), ], resolve: { alias: { "@": path.resolve(__dirname, "./src"), }, }, server: { proxy: { '/api': { - target: 'http://127.0.0.1:3000', + target: apiTarget, changeOrigin: true, secure: false, } } - } -}) + } + } +})apps/web/src/pages/LoginPage.tsx (1)
57-61: Move interface definition to module level.Defining
LoginUserinside the function body is unconventional and recreates the type definition on each call. Consider moving it to the top of the file or importing a shared user type if one exists.Suggested refactor
import { useAuth } from '../hooks/useAuth'; import { authClient } from '../lib/auth-client'; +interface LoginUser { + id: string; + email: string; + systemRole?: string; +} + /** * Component for the Login Page.Then remove the inline definition from
handleSubmit.apps/api/src/modules/tenants/tenant.schema.ts (1)
52-56: Consider relocating therelationsimport to the top of the file.The
relationsimport on line 56 appears after the type exports. While functional, grouping all imports at the top improves readability and follows conventional TypeScript style.Suggested reordering
-import { pgTable, text, timestamp, pgEnum, unique } from 'drizzle-orm/pg-core'; +import { pgTable, text, timestamp, pgEnum, unique } from 'drizzle-orm/pg-core'; +import { relations } from 'drizzle-orm'; import { user } from '../users/user.schema';Then remove the import from line 56.
apps/api/src/scripts/reset-db.ts (1)
28-38: Table list may become stale as schema evolves.The hardcoded table list requires manual updates when new tables are added. Consider adding a comment to remind maintainers, or dynamically querying table names (with appropriate filtering).
docs/plans/merge-invitations-proposal.md (2)
26-29: Consider documenting pagination and error handling for the combined list.The rendering strategy combines users and invitations arrays, but the plan doesn't address:
- Pagination when the combined list grows large
- Fallback behavior if the invitations fetch fails (show users only?)
- Search/filter behavior across the combined data sources
These may be out of scope for this initial implementation, but worth noting for future iterations.
31-32: Thestatusfield includes'disabled'which isn't covered in the transformation logic.The type definition includes
status?: 'active' | 'pending' | 'disabled', but the data transformation section only mentionspendingfor invitations. Consider documenting how'active'and'disabled'are derived for regular users (e.g., from a user'sbannedfield or similar).apps/api/src/modules/invitations/invitations.controller.spec.ts (1)
51-61: Strengthen the headers propagation assertion.
expect.anything()will pass even if headers are omitted; consider asserting the exact headers passed.♻️ Suggested test tightening
- const req = { user: { id: 'user-123' } } as unknown as Request & { - user: { id: string }; - }; + const req = { + user: { id: 'user-123' }, + headers: { 'x-test': '1' }, + } as unknown as Request & { + user: { id: string }; + headers: Record<string, string>; + }; ... - expect(service.create).toHaveBeenCalledWith( - dto, - 'user-123', - expect.anything(), - ); + expect(service.create).toHaveBeenCalledWith(dto, 'user-123', req.headers);apps/web/src/routes/AdminRoutes.tsx (1)
47-47: Avoid empty-string sentinel forinviteResource.Using
inviteResource=""works but is a bit opaque. Consider a clearer signal (e.g., adisableInvitesboolean) or allowinviteResource={null}with matching prop typing inUserListto make the intent explicit.docs/design/invitation-flow.md (1)
43-45: Minor wording polish (formal noun usage).Consider “invitation” instead of “invite” in Line 44 for formal tone.
✏️ Suggested tweak
-1. **Smart Redirection**: The Login/Signup pages must support a `?to=` or `?redirect=` parameter to remember the user came from an invite. +1. **Smart Redirection**: The Login/Signup pages must support a `?to=` or `?redirect=` parameter to remember the user came from an invitation.apps/web/src/pages/SignupPage.tsx (1)
20-35: Move email prefill into an effect (avoid setState during render).Setting state directly in render is non-idiomatic and can cause extra renders in Strict Mode.
🔧 Suggested fix
-import { useState } from 'react'; +import { useEffect, useState } from 'react'; ... - // Pre-fill email if provided - if (emailParam && !email) { - setEmail(emailParam); - } + // Pre-fill email if provided + useEffect(() => { + if (emailParam && !email) { + setEmail(emailParam); + } + }, [emailParam, email]);apps/api/src/modules/auth/system-admin.guard.ts (2)
18-21: Potential type mismatch when casting Express headers.Express headers can contain
string | string[]values (for repeated headers), but the cast toRecord<string, string>assumes all values are strings. This could cause unexpected behavior if multi-value headers are present.♻️ Suggested approach
- // 1. Extract Token (Similar to AuthGuard but isolated logic) - // 1. Validate Session via Headers (Correctly handles Signed Cookies) - const headers = new Headers(req.headers as Record<string, string>); + // Validate Session via Headers (Correctly handles Signed Cookies) + const headers = new Headers(); + for (const [key, value] of Object.entries(req.headers)) { + if (typeof value === 'string') { + headers.set(key, value); + } else if (Array.isArray(value)) { + headers.set(key, value.join(', ')); + } + }Also removes the duplicate "1." comments that appear to be leftover from refactoring.
28-34: Consider defining a typed interface for the user object.The inline cast to
{ systemRole?: string }works but loses type information. A dedicated interface would improve maintainability and make the expected user shape explicit.♻️ Suggested improvement
// At the top of the file or in a shared types file interface SystemUser { id: string; systemRole?: string; // Add other relevant fields } // In canActivate: const user = sessionData.user as SystemUser;apps/api/src/modules/invitations/invitations.controller.ts (1)
25-35: Avoid mutating the DTO directly.Directly assigning to
createInvitation.organizationIdmutates the validated DTO object, which can cause unexpected side effects. Create a new object instead.♻️ Suggested fix
- // Strict Enforcement: Invites are ALWAYS for the current user's organization. - // No explicit override allowed via API body. - createInvitation.organizationId = req.user.organizationId; - - // Pass headers to propagate auth context to BetterAuth client - const headers = new Headers(req.headers as Record<string, string>); - return this.invitationsService.create( - createInvitation, - req.user.id, - headers, - ); + // Strict Enforcement: Invites are ALWAYS for the current user's organization. + const invitationPayload = { + ...createInvitation, + organizationId: req.user.organizationId, + }; + + // Pass headers to propagate auth context to BetterAuth client + const headers = new Headers(); + for (const [key, value] of Object.entries(req.headers)) { + if (typeof value === 'string') { + headers.set(key, value); + } else if (Array.isArray(value)) { + headers.set(key, value.join(', ')); + } + } + return this.invitationsService.create(invitationPayload, req.user.id, headers);apps/web/src/modules/users/UserList.tsx (2)
37-43: Redundant fallback and confusing conditional logic.The
resource: inviteResource || "invitations"fallback is redundant sinceinviteResourcealready defaults to"invitations"in the destructuring. Additionally,enabled: !!inviteResourcewill always betruedue to the default value, making it impossible to disable invitation fetching without explicitly passing an empty string orundefined.If disabling invitation fetching is a valid use case, consider making the default
undefined:♻️ Suggested fix
export const UserList = ({ basePath, resource = "users", - inviteResource = "invitations" + inviteResource }: UserListProps) => { // ... // Fetch Invitations with Refine's Data Hook (Conditional) const { data: inviteData, isLoading: isLoadingInvites } = useList({ - resource: inviteResource || "invitations", // Default to satisfy hook, but we'll gate usage + resource: inviteResource ?? "invitations", queryOptions: { enabled: !!inviteResource, // Only fetch if resource provided } });This allows callers to pass
inviteResource={undefined}to disable invitation fetching.
45-52: Consider typing the invitation data for better safety.The
inviteData.datais untyped, which could lead to runtime errors if the API response shape changes. Consider defining an interface for the invitation response.♻️ Suggested improvement
interface InvitationItem { id: string | number; email: string; role?: string; } // Then use it: const { data: inviteData, isLoading: isLoadingInvites } = useList<InvitationItem>({ // ... });apps/api/src/modules/auth/identity-provider.abstract.ts (1)
43-49: PreferHeadersInit(or normalization) to avoid unsafe casting at call sites.
UsingHeadersforces Express headers to be cast as aHeadersinstance, which they aren’t. Consider widening the type and normalizing in the provider to keep typing honest.♻️ Suggested contract tweak
- abstract getSessionFromHeaders( - headers: Headers, - ): Promise<{ session: Session; user: User } | null>; + abstract getSessionFromHeaders( + headers: HeadersInit, + ): Promise<{ session: Session; user: User } | null>; abstract createInvitation(payload: { email: string; role: string; organizationId: string | null; expiresIn?: number; inviterId: string; - headers?: Headers; + headers?: HeadersInit; }): Promise<unknown>;Also applies to: 68-75
apps/api/src/modules/auth/auth.controller.ts (1)
68-80: Normalize Express headers before passing togetSessionFromHeaders.
Castingreq.headerstoHeadersassumes it’s already aHeadersinstance. Consider converting to a realHeaders(or widening the interface) to avoid runtime surprises.Also applies to: 119-135
apps/api/src/modules/system-admin/system-admin.controller.ts (1)
17-25: Add a maxpageSizeand paginate tenants.
Without a cap (and no pagination for tenants), the endpoints can pull very large datasets. Consider a server-side limit and pagination params for the tenants list as well.Also applies to: 60-79
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
141-200: Handle multipleSet-Cookieheaders robustly.
Depending on the runtime,headers.get('set-cookie')may drop additional cookies. Consider using the runtime’s multi-cookie API (e.g.,getSetCookie()orheaders.raw()in undici) with a fallback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
apps/api/drizzle/0007_special_vivisector.sqlapps/api/drizzle/0008_real_terrax.sqlapps/api/drizzle/meta/0007_snapshot.jsonapps/api/drizzle/meta/0008_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/app.module.tsapps/api/src/modules/auth/auth.controller.spec.tsapps/api/src/modules/auth/auth.controller.tsapps/api/src/modules/auth/auth.guard.spec.tsapps/api/src/modules/auth/auth.guard.tsapps/api/src/modules/auth/auth.module.tsapps/api/src/modules/auth/identity-provider.abstract.tsapps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.tsapps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsapps/api/src/modules/auth/system-admin.guard.spec.tsapps/api/src/modules/auth/system-admin.guard.tsapps/api/src/modules/invitations/invitations.controller.spec.tsapps/api/src/modules/invitations/invitations.controller.tsapps/api/src/modules/invitations/invitations.module.tsapps/api/src/modules/invitations/invitations.service.tsapps/api/src/modules/system-admin/system-admin.controller.spec.tsapps/api/src/modules/system-admin/system-admin.controller.tsapps/api/src/modules/system-admin/system-admin.module.tsapps/api/src/modules/tenants/tenant.schema.tsapps/api/src/modules/tenants/tenants.controller.spec.tsapps/api/src/modules/tenants/tenants.controller.tsapps/api/src/modules/tenants/tenants.service.spec.tsapps/api/src/modules/tenants/tenants.service.tsapps/api/src/modules/users/user.schema.tsapps/api/src/modules/users/users.validation.spec.tsapps/api/src/modules/users/users.validation.tsapps/api/src/scripts/bootstrap-admin.tsapps/api/src/scripts/reset-db.tsapps/web/src/App.tsxapps/web/src/layouts/AdminLayout.tsxapps/web/src/lib/auth-client.tsapps/web/src/lib/auth/AuthProvider.tsxapps/web/src/lib/auth/types.tsapps/web/src/modules/users/UserList.tsxapps/web/src/modules/users/Users.tsxapps/web/src/modules/users/types.tsapps/web/src/pages/LoginPage.tsxapps/web/src/pages/SignupPage.tsxapps/web/src/pages/admin/tenants/TenantListPage.tsxapps/web/src/pages/admin/users/UserEdit.tsxapps/web/src/pages/admin/users/UserShow.tsxapps/web/src/pages/public/AcceptInvitePage.tsxapps/web/src/routes/AdminRoutes.tsxapps/web/src/routes/TenantRoutes.tsxapps/web/vite.config.tsdocs/design/invitation-flow.mddocs/plans/merge-invitations-proposal.md
🧰 Additional context used
🧬 Code graph analysis (18)
apps/web/src/modules/users/Users.tsx (3)
apps/web/src/components/ui/table.tsx (2)
TableHead(116-116)TableCell(118-118)apps/api/src/modules/users/user.schema.ts (1)
user(3-16)apps/web/src/components/ui/badge.tsx (1)
Badge(37-37)
apps/api/src/modules/system-admin/system-admin.module.ts (1)
apps/api/src/app.module.ts (1)
Module(12-28)
apps/web/src/App.tsx (1)
apps/web/src/pages/public/AcceptInvitePage.tsx (1)
AcceptInvitePage(7-97)
apps/web/src/layouts/AdminLayout.tsx (1)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)
apps/api/src/modules/invitations/invitations.module.ts (2)
apps/api/src/app.module.ts (1)
Module(12-28)apps/api/src/modules/system-admin/system-admin.module.ts (1)
Module(7-11)
apps/web/src/pages/admin/tenants/TenantListPage.tsx (1)
apps/web/src/modules/tenants/types.ts (1)
TenantApiResponse(11-19)
apps/api/src/modules/users/users.validation.spec.ts (1)
apps/api/src/modules/users/users.validation.ts (1)
SignupSchema(28-36)
apps/web/src/routes/AdminRoutes.tsx (1)
apps/web/src/modules/users/UserList.tsx (1)
UserList(13-78)
apps/web/src/pages/SignupPage.tsx (2)
apps/web/src/lib/auth/context.ts (1)
useAuth(6-12)apps/web/src/hooks/useAuth.ts (1)
useAuth(1-1)
apps/web/src/pages/LoginPage.tsx (1)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)
apps/web/src/routes/TenantRoutes.tsx (1)
apps/web/src/modules/users/UserList.tsx (1)
UserList(13-78)
apps/api/src/modules/invitations/invitations.controller.ts (1)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
createInvitation(314-363)
apps/api/src/scripts/bootstrap-admin.ts (1)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (3)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)apps/api/src/modules/auth/auth.schema.ts (2)
session(4-18)Session(47-47)apps/api/src/modules/tenants/tenant.schema.ts (1)
invitation(39-50)
apps/web/src/lib/auth/AuthProvider.tsx (1)
apps/web/src/lib/auth-client.ts (1)
authClient(21-26)
apps/web/src/modules/users/UserList.tsx (4)
apps/web/src/modules/users/types.ts (1)
UserTableItem(1-8)apps/api/src/modules/users/user.schema.ts (1)
user(3-16)apps/web/src/modules/invitations/InviteMemberDialog.tsx (1)
InviteMemberDialog(47-153)apps/web/src/modules/users/Users.tsx (1)
Users(21-95)
apps/api/src/modules/auth/auth.controller.ts (3)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
login(137-201)apps/api/src/modules/auth/auth.schema.ts (2)
Session(47-47)session(4-18)apps/api/src/modules/users/users.validation.ts (1)
CompleteInvite(55-55)
apps/api/src/modules/auth/auth.module.ts (2)
apps/api/src/app.module.ts (1)
Module(12-28)apps/api/src/modules/invitations/invitations.module.ts (1)
Module(6-12)
🪛 LanguageTool
docs/design/invitation-flow.md
[style] ~44-~44: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...meter to remember the user came from an invite. 2. "Join" Mode for Signup: The Si...
(AN_INVITE)
🔇 Additional comments (69)
apps/web/src/pages/admin/tenants/TenantListPage.tsx (2)
8-13: LGTM on resource path update.The resource path change to
"admin/tenants"correctly aligns with the new admin-scoped API endpoints introduced in this PR.
30-46: LGTM on mutation resource alignment.The
useUpdateresource path is consistent with theuseTableconfiguration, ensuring both read and write operations target the same admin endpoint.apps/web/vite.config.ts (1)
23-31: VerifychangeOrigin: truedoesn’t break tenant/host routing.If the API uses the Host header (or cookie domain) for tenant resolution,
changeOrigin: truewill rewrite it to127.0.0.1:3000and can misroute requests in dev. Please confirm this is safe for your auth/tenant logic; otherwise consider keeping the original host.Potential adjustment (if Host header must be preserved)
server: { proxy: { '/api': { target: 'http://127.0.0.1:3000', - changeOrigin: true, + changeOrigin: false, secure: false, } } }apps/web/src/lib/auth-client.ts (1)
21-26: LGTM! Correct approach for cross-origin session persistence.Adding
credentials: "include"ensures cookies are sent with requests to the API, which is necessary for session management when frontend and backend are on different origins. This aligns with the session persistence fixes mentioned in the PR.The backend CORS configuration already properly supports this:
apps/api/src/main.tsdefinescredentials: trueand restricts origins to specific values from theALLOWED_ORIGINSenvironment variable (not wildcard*), which is the correct secure approach.apps/api/src/modules/tenants/tenants.controller.ts (1)
20-22: Verify AuthGuard always populatesreq.userbefore access.
findAllnow dereferencesreq.user.id; ifAuthGuardever allows a request without a user, this becomes a 500 instead of a 401. Please confirm the guard always injectsreq.user, or add an explicit check/UnauthorizedException fallback.apps/api/src/modules/tenants/tenants.service.spec.ts (2)
29-66: Mock extension forinnerJoinis consistent.The chainable
innerJoinmock aligns with the new query flow and keeps the test doubles coherent.
103-114: UpdatedfindAllForUsertest coverage looks solid.The test now exercises the join/where execution path and validates the service result.
apps/api/src/modules/tenants/tenants.controller.spec.ts (1)
2-3: Tests correctly reflect the user-scoped tenant fetch.The controller test aligns with the updated handler signature and verifies the correct service call.
Also applies to: 12-14, 45-64
apps/api/src/modules/tenants/tenants.service.ts (1)
14-31: Guard against duplicate tenant rows from the join.
innerJoincan yield duplicate organizations ifmemberpermits multiple rows per(userId, organizationId)(e.g., role history or re-invites). Please confirm a uniqueness constraint exists, or add a distinct/grouping step to guarantee one org per user.apps/api/drizzle/0008_real_terrax.sql (1)
1-1: Consider adding NOT NULL constraint for consistency.The
system_rolecolumn is nullable, but the default value'user'suggests all users should have a role. If the application logic expectssystemRoleto always be present, consider addingNOT NULL:-ALTER TABLE "user" ADD COLUMN "system_role" text DEFAULT 'user'; +ALTER TABLE "user" ADD COLUMN "system_role" text DEFAULT 'user' NOT NULL;If existing rows need to be handled, you may need a two-step migration: add with default, then alter to NOT NULL.
apps/api/src/modules/tenants/tenant.schema.ts (1)
49-49: LGTM!The
createdAtcolumn addition follows the established pattern used inorganizationandmembertables, with propernotNull()anddefaultNow()constraints for reliable audit tracking.apps/api/drizzle/0007_special_vivisector.sql (1)
1-1: LGTM!The migration correctly adds the
createdAtcolumn withNOT NULLand a sensible default, ensuring existing invitation rows receive a timestamp. This aligns with the Drizzle schema definition.apps/web/src/modules/users/types.ts (1)
7-7: LGTM!The optional
statusfield with a well-defined union type appropriately extends the interface to support merged user/invitation lists. The inclusion of"pending"alongside the organization status values ("active","disabled","suspended") aligns with the invitation flow requirements.apps/api/drizzle/meta/0007_snapshot.json (1)
1-608: Schema snapshot looks consistent.This auto-generated Drizzle snapshot correctly captures the database schema at version 7. The foreign key relationships and constraints are well-defined. One minor observation: the
invitationtable's FK toorganizationusesonDelete: "no action", which means deleting an organization will fail if invitations exist. This is a valid choice if you want to enforce cleanup before org deletion.apps/api/drizzle/meta/0008_snapshot.json (1)
1-615: Schema snapshot correctly reflects thesystem_roleaddition.The snapshot properly captures the schema evolution from 0007 to 0008, including the new
system_rolecolumn on theusertable. TheprevIdchain is correctly linked for migration tracking.apps/api/drizzle/meta/_journal.json (1)
53-67: Migration journal entries are correctly added.The two new journal entries (idx 7 and 8) are properly formatted with sequential indices and timestamps. The tags align with the corresponding migration files.
apps/api/src/modules/users/user.schema.ts (1)
12-12: NULL handling forsystemRoleis already safely implemented.The guard correctly treats
NULLvalues as non-admin (line 32:if (user.systemRole !== 'platform_admin')), and the auth provider normalizesNULLto'user'via the fallbackdbUser?.systemRole || 'user'(better-auth.provider.ts:197). Existing users withNULLwill be treated as regular users in all authorization contexts, so no backfill migration is necessary.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts (4)
50-61: LGTM! Mock database layer properly extended.The addition of
session.findFirstanduser.findFirstquery mocks aligns with the provider's implementation that now performs direct DB lookups for session validation and systemRole enrichment. This enables more realistic test coverage of the enriched session flows.
165-186: LGTM! Login test properly mocks the response chain.The refactored test correctly simulates the provider's login flow by mocking the
signInEmailresponse with the expected structure (ok,headers,json()), followed by DB lookups for session and user data. The assertions verify that bothsessionanduserare defined in the result.
335-337: URL expectation correctly reflects test environment.The change from a full URL to a relative path (
/invite/accept?id=inv-123) is appropriate sinceFRONTEND_HOSTis not set in the test environment. The comment clearly explains the reasoning.
204-225: The mock ordering concern is valid but currently works because the implementation guarantees the call sequence.The test correctly relies on this specific order:
validateSessionis called first, which triggers the firstuser.findFirst(line 225 of the implementation)getEnrichedSessionthen callsmember.findManyand subsequently the seconduser.findFirst(line 286)Since
validateSessionmust complete beforegetEnrichedSession's seconduser.findFirstexecutes, the current implementation does guarantee the expected call order. However, the test remains brittle to refactoring. Consider adding a clarifying comment above the mocks to document the expected call sequence, or usemockImplementationwith a call counter to make the test more resilient to implementation changes without losing clarity.apps/web/src/pages/admin/users/UserEdit.tsx (1)
16-22: Resource path change aligns with admin API structure.The update from
"users"to"admin/users"is consistent with the admin-scoped endpoints. Theas anycast is acknowledged via the eslint-disable comment.apps/web/src/pages/admin/users/UserShow.tsx (1)
15-17: LGTM! Resource path consistently updated.The change to
"admin/users"aligns with the admin-scoped API endpoints and is consistent withUserEdit.tsx. Navigation links correctly reference/admin/userspaths.apps/web/src/lib/auth/types.ts (1)
14-14: LGTM! System role type addition is well-scoped.The optional
systemRolefield with a restrictive union type ('platform_admin' | 'user') enables frontend authorization checks while maintaining backward compatibility. The naming distinguishes platform-level admin from tenant-level admin roles.apps/web/src/routes/TenantRoutes.tsx (1)
50-50: LGTM! Props enable invitation listing alongside users.The explicit
resourceandinviteResourceprops wire up theUserListcomponent to fetch and display pending invitations alongside users. While these match the component's default values, explicit declaration improves readability and makes the intent clear.apps/api/src/app.module.ts (1)
10-23: Wiring the admin module looks good.
No concerns with registering the new module.apps/api/src/modules/system-admin/system-admin.module.ts (1)
1-10: System admin module composition is clear.
Imports and controller registration align with the module’s purpose.apps/api/src/modules/users/users.validation.spec.ts (1)
101-109: Test aligns with optional companyName.
Good coverage for the new optional field behavior.apps/web/src/App.tsx (1)
5-22: Invite acceptance route wiring looks good.
Public route addition is clear and consistent with other public pages.apps/web/src/layouts/AdminLayout.tsx (2)
124-131: Type alignment forsystemRolelooks correct.
This keeps the auth context shape in sync with the new authorization checks.
136-146: Platform-admin gate and redirect look solid.
The guard is clear and tight for admin-only access.apps/api/src/modules/invitations/invitations.module.ts (1)
1-8: forwardRef wiring for the Auth module looks right.
This should resolve the circular dependency without changing providers/controllers.apps/api/src/modules/auth/auth.module.ts (1)
1-18: forwardRef addition for InvitationsModule looks appropriate.
This should align with the mutual module dependency.apps/api/src/modules/invitations/invitations.service.ts (2)
37-38: List delegation is straightforward.
This keeps the service aligned with the identity provider abstraction.
11-25: No changes needed. The code correctly handles headers at the appropriate layer: the controller converts the plainreq.headersobject to aHeadersinstance (line 30) before passing it to the service, which then forwards it to the identity provider. Theheaders?: Headersparameter type in the service is correct and consistent with the abstract class contract. Adding additional normalization in the service would be redundant.Likely an incorrect or invalid review comment.
apps/api/src/modules/auth/system-admin.guard.spec.ts (1)
7-93: LGTM — good coverage of authorization paths.The suite cleanly covers unauthenticated, unauthorized role, and platform_admin success cases.
apps/api/src/modules/system-admin/system-admin.controller.spec.ts (1)
1-90: LGTM — solid unit coverage for listUsers/listTenants.The tests validate pagination defaults and tenant userCount derivation as expected.
apps/web/src/routes/AdminRoutes.tsx (1)
22-32: No action required. All admin pages (UserShow, UserEdit, TenantListPage) explicitly specify the correct admin resource names ("admin/users" and "admin/tenants") in their Refine hooks, so the resource rename has been properly reflected across the codebase.apps/api/src/modules/auth/auth.guard.ts (1)
41-48: The defensive check is not necessary—token is guaranteed by the schema.The
tokenfield is defined in the database schema as.notNull(), which means the TypeScript-inferredSessiontype hastoken: stringas a required, non-optional field. The null check at lines 27–29 already guards against missing sessions; after that check passes,result.session.tokenis guaranteed to exist. No additional check is needed.apps/web/src/lib/auth/AuthProvider.tsx (3)
34-45: Enriched-session fallback looks clear and safe.Explicit Authorization header plus a warning on failure keeps the flow resilient.
90-94: No-session handling is straightforward.
115-124: Propagation ofhasTenantandsystemRoleis consistent with the new user model.Also applies to: 153-162
apps/api/src/modules/auth/auth.controller.spec.ts (2)
5-52: Test wiring for InvitationsService looks correct.
79-81: UsinggetSessionFromHeadersaligns with the new auth flow.apps/web/src/pages/public/AcceptInvitePage.tsx (2)
13-17: Legacytokenand modernidsupport is a nice compatibility touch.
74-96: Loading/invalid states look polished and clear.apps/web/src/pages/SignupPage.tsx (1)
132-193: Invite-flow UI tweaks are clear and user-friendly.apps/api/src/modules/users/users.validation.ts (2)
45-55: LGTM!The
CompleteInviteSchemais well-structured with appropriate validations: email format, password minimum length, required first/last names, and UUID validation for the invitation ID.
32-35: Remove.optional()fromcompanyNameinSignupSchemato enforce the intended requirement.The comment on line 15 explicitly states that
companyNameshould be "required on Signup", but theSignupSchemaoverride at lines 32-35 still includes.optional(), making it optional. Remove.optional()on line 35 to align with the documented intent.apps/api/src/modules/auth/system-admin.guard.ts (1)
36-40: LGTM!Explicitly not attaching tenant context for system admin requests is a good security practice to prevent accidental tenant data leakage.
apps/api/src/modules/invitations/invitations.controller.ts (1)
56-66: LGTM!The new
list()endpoint correctly scopes invitations to the user's organization and handles the edge case of missingorganizationIdgracefully.apps/web/src/modules/users/UserList.tsx (1)
54-56: LGTM!The data merging logic and combined loading state handling are clean. Placing invitations first in the combined data ensures pending invites are prominently displayed.
Also applies to: 64-67
apps/api/src/modules/auth/auth.guard.spec.ts (3)
12-22: LGTM!The mock object correctly includes
getSessionFromHeadersto align with the updatedIdentityProviderinterface.
57-94: LGTM!The test comprehensively validates the two-step authentication flow: session extraction via headers followed by session enrichment. The assertions properly verify both method calls and the resulting request state.
96-131: LGTM!Good coverage of failure scenarios: invalid session from headers and enrichment failure both correctly throw
UnauthorizedException.apps/api/src/modules/auth/identity-provider.abstract.ts (3)
30-33: Login contract update is clear.
90-93: Invitation listing hook looks fine.
101-105: Force-verify hook addition is clean.apps/api/src/modules/auth/auth.controller.ts (1)
14-40: DI wiring for InvitationsService is straightforward.apps/api/src/modules/system-admin/system-admin.controller.ts (2)
1-13: Controller/guard wiring looks good.
27-57: Users query + envelope response are consistent.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (8)
1-7: Import change is fine.
43-50: System role field wiring looks good.
69-75: Invite URL composition is clear and properly encoded.
204-251: DB-backed validation + expiry checks look solid.
253-265: Delegating header parsing to Better Auth is appropriate.
291-309: System role refresh during enrichment is a good addition.
394-409: List-invitations query is straightforward.
415-424: Force-verify update is simple and clear.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| advanced: { | ||
| defaultCookieAttributes: { | ||
| secure: false, // Force false for local dev debugging | ||
| sameSite: 'lax', | ||
| path: '/', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "better-auth.provider.ts" --exec wc -l {}Repository: pramodnarayana/nexiom
Length of output: 142
🏁 Script executed:
cat -n apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts | head -120Repository: pramodnarayana/nexiom
Length of output: 5482
🏁 Script executed:
# Check for environment-based configuration or conditional logic
rg "NODE_ENV|process\.env" apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts -B2 -A2Repository: pramodnarayana/nexiom
Length of output: 1190
🏁 Script executed:
# Search for other cookie security configurations in the codebase
rg "secure:\s*(true|false|process\.env)" apps/api/src --type ts -B2 -A2Repository: pramodnarayana/nexiom
Length of output: 589
Fix hard-coded secure: false for cookies in production.
The cookie security setting is currently hard-coded to false regardless of environment, which allows sessions to be transmitted over HTTP in production. Make it environment-dependent:
Suggested fix
advanced: {
defaultCookieAttributes: {
- secure: false, // Force false for local dev debugging
+ secure: process.env.NODE_ENV === 'production',
sameSite: 'lax',
path: '/',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| advanced: { | |
| defaultCookieAttributes: { | |
| secure: false, // Force false for local dev debugging | |
| sameSite: 'lax', | |
| path: '/', | |
| }, | |
| }, | |
| advanced: { | |
| defaultCookieAttributes: { | |
| secure: process.env.NODE_ENV === 'production', | |
| sameSite: 'lax', | |
| path: '/', | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`
around lines 87 - 93, The cookie config in advanced.defaultCookieAttributes
currently hard-codes secure: false; change it to derive from environment (e.g.
secure: process.env.SESSION_COOKIE_SECURE === 'true' || process.env.NODE_ENV ===
'production') so cookies are secure in production but remain usable in local
dev; update the advanced.defaultCookieAttributes object in
better-auth.provider.ts accordingly and ensure any env var name used is
documented/loaded by your config.
| const silentAccept = async () => { | ||
| try { | ||
| const API_URL = import.meta.env.VITE_API_URL; | ||
| // Attempt to accept using the active session. | ||
| // If already a member, backend will likely throw or return success. | ||
| // Ideally backend should be idempotent. | ||
| await fetch(`${API_URL}/invitations/accept`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| // Credentials included automatically by browser cookies (if used) or we need token | ||
| // Our AuthProvider attaches token usually, but here we fetch directly. | ||
| // We need to pass the access token if using Bearer. | ||
| // Better Auth client handles this usually. Let's use authClient? | ||
| // But authClient.invitation.acceptInvitaton might be different. | ||
| // Let's rely on browser Cookies for now as per `authClient`. | ||
| // Or better: Use the global `authClient` if possible. | ||
| }, | ||
| body: JSON.stringify({ invitationId: inviteId }), | ||
| }); | ||
| // We intentionally ignore potential errors (e.g. "Already member") | ||
| // and proceed to dashboard. | ||
| } catch (e) { | ||
| // Ignore errors (Idempotency) | ||
| console.warn("Silent accept failed or already member", e); | ||
| } finally { | ||
| navigate('/dashboard', { replace: true }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/pages/public/AcceptInvitePage.tsx | head -80Repository: pramodnarayana/nexiom
Length of output: 4341
🏁 Script executed:
fd useAuth apps/web/src -type f | head -5Repository: pramodnarayana/nexiom
Length of output: 235
🏁 Script executed:
find apps/web/src -name "*useAuth*" -o -name "*auth*" -type f | grep -i hook | head -10Repository: pramodnarayana/nexiom
Length of output: 94
🏁 Script executed:
cat -n apps/web/src/hooks/useAuth.tsRepository: pramodnarayana/nexiom
Length of output: 118
🏁 Script executed:
cat -n apps/web/src/lib/auth/context.tsxRepository: pramodnarayana/nexiom
Length of output: 130
🏁 Script executed:
find apps/web/src/lib/auth -type f | head -20Repository: pramodnarayana/nexiom
Length of output: 167
🏁 Script executed:
cat -n apps/web/src/lib/auth/context.tsRepository: pramodnarayana/nexiom
Length of output: 524
🏁 Script executed:
cat -n apps/web/src/lib/auth/types.tsRepository: pramodnarayana/nexiom
Length of output: 1146
🏁 Script executed:
cat -n apps/web/src/lib/auth/AuthProvider.tsxRepository: pramodnarayana/nexiom
Length of output: 9838
🏁 Script executed:
cat -n apps/web/src/lib/auth-client.ts | head -50Repository: pramodnarayana/nexiom
Length of output: 958
Add authentication to the invitation accept request.
The fetch call to /invitations/accept omits credentials: 'include' and the Authorization header, so the request will fail authentication. The useAuth() hook exposes a token property that should be used. Since errors are silently caught, the user is redirected to dashboard on failure, masking the problem.
🔧 Suggested fix
- const { user, isLoading } = useAuth();
+ const { user, isLoading, token } = useAuth();
...
await fetch(`${API_URL}/invitations/accept`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
+ ...(token ? { Authorization: `Bearer ${token}` } : {}),
},
+ credentials: 'include',
body: JSON.stringify({ invitationId: inviteId }),
});🤖 Prompt for AI Agents
In `@apps/web/src/pages/public/AcceptInvitePage.tsx` around lines 31 - 58, The
silentAccept function should include authentication when calling
/invitations/accept: obtain the token from useAuth() (the hook exposing token)
and add an Authorization: `Bearer ${token}` header and credentials: 'include' to
the fetch options (still send body with invitationId). Update the fetch call in
silentAccept (and ensure inviteId and navigate usage remains) and instead of
fully swallowing errors, log the fetch response error or non-2xx status before
navigating so authentication failures are visible in console.
- Addressed all CodeRabbit review feedback - Secured auth/cookie handling - Fixed potential open redirects - Removed redundant root scripts folder
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/auth.controller.ts`:
- Around line 85-119: completeInvite currently creates the user before ensuring
the invitation is still valid which can orphan accounts if invitations are
accepted/expired concurrently; before calling authProvider.createUser, call
invitationsService.findById(body.invitationId) and validate invitation.status
=== 'pending' and invitation.expiresAt is in the future (throw
BadRequestException if not), then proceed to create the user and call
invitationsService.accept; additionally, guard against accept failing by either
performing both operations in a transaction (if your DB/services support it) or
by rolling back on failure (e.g., delete the newly created user via
authProvider.deleteUser) and surface a clear error, while keeping subsequent
calls to authProvider.forceVerifyEmail and authProvider.login unchanged.
- Around line 126-141: The refreshSession method currently force-casts Express
req.headers to Web API Headers when calling
this.authProvider.getSessionFromHeaders; replace that cast with a proper
conversion that maps IncomingHttpHeaders entries into a new Headers object
(handling array values by joining with ", " and skipping undefined), e.g.,
create and use a shared utility like toWebHeaders(expressHeaders) and call
this.authProvider.getSessionFromHeaders(toWebHeaders(req.headers)); update
refreshSession to import and use toWebHeaders so the header shape is correct
without unsafe casting.
- Around line 73-76: The cast of Express req.headers to the Web API Headers in
the call to this.authProvider.getSessionFromHeaders is invalid and can break
runtime code that expects Headers methods; instead construct a proper Headers
instance from the plain object (or use a small utility like toWebHeaders) and
pass that to getSessionFromHeaders (update the call site in auth.controller.ts
where getSessionFromHeaders is invoked and any other places using req.headers),
ensuring header names/values are normalized when converting so methods like
.get() work as expected.
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 69-75: Validate and restrict the constructed invite URL to trusted
origins: in better-auth.provider.ts, when computing baseUrl (FRONTEND_URL or
first ALLOWED_ORIGINS entry) check that the chosen FRONTEND_URL (if present) is
included in the ALLOWED_ORIGINS list, and if using the fallback origin ensure it
exactly matches an entry from ALLOWED_ORIGINS (reject/make deterministic if
not). Update the logic that produces inviteUrl (using baseUrl +
/invite/accept?id=... with data.invitation.id and data.email) to throw or log an
error and abort sending if the resolved baseUrl is not in ALLOWED_ORIGINS, and
add guidance to .env.example to document that FRONTEND_URL must be set in
production or that ALLOWED_ORIGINS must contain the allowed frontend host.
- Around line 357-361: The current code sets a custom 'x-inviter-id' header on
payload.headers; instead, use Better Auth's built-in inviter mechanism by adding
inviter context to the invitation data sent to the invite plugin (e.g., populate
the invitation's data object with inviterId) and remove the custom header
assignment in the payload.headers block; update the places that construct the
invitation payload (refer to payload, headers, and invitation handlers like
sendInvitationEmail and beforeCreate) so the inviter is available via
invitation.data.inviter (or invitation.inviter) in plugin hooks and handlers
rather than relying on the x-inviter-id header.
In `@apps/api/src/modules/system-admin/system-admin.controller.ts`:
- Around line 16-23: The listUsers pagination currently only enforces a minimum
pageSize (variable limit) so a client can request arbitrarily large pages;
update the logic in listUsers to clamp pageSize to a reasonable maximum (e.g.,
const MAX_PAGE_SIZE = 100 or a config value) by using Math.min after parsing
(limit = Math.max(1, Math.min(MAX_PAGE_SIZE, parseInt(pageSize) || 10))), and
use that limited limit and offset for DB queries to prevent unbounded queries.
♻️ Duplicate comments (2)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
87-93: Cookie security setting addressed as per previous review.The
secureattribute now correctly derives fromprocess.env.NODE_ENV === 'production', ensuring cookies are transmitted securely in production while remaining usable in local development.apps/api/src/modules/auth/auth.controller.ts (1)
42-60: Cookie handling correctly addressed per previous review.The destructuring
{ cookie: loginCookie, ...result }properly strips the cookie from the response body while still setting it via header. This prevents exposing HttpOnly cookie values to JavaScript.
🧹 Nitpick comments (4)
apps/web/src/pages/LoginPage.tsx (1)
57-61: Consider moving the interface outside the function.Defining
LoginUserinsidehandleSubmitmeans it's recreated on every call. Move it to file scope or a shared types file.Suggested refactor
+interface LoginUser { + id: string; + email: string; + systemRole?: string; +} + export function LoginPage() { // ... const handleSubmit = async (e: React.FormEvent) => { // ... const data = await res.json(); setAuthState(data); - - interface LoginUser { - id: string; - email: string; - systemRole?: string; - } const searchParams = new URLSearchParams(window.location.search); const systemRole = (data.user as LoginUser).systemRole;apps/web/src/layouts/AdminLayout.tsx (1)
31-36: Consider removing unusedrolesproperty from SidebarContent props.With authorization now using
systemRole, theroles?: string[]property in SidebarContent'suserprop type appears unused. Cleaning this up would improve type accuracy.Suggested cleanup
const SidebarContent = ({ navGroups, location, user, navigate, logout }: { navGroups: { title: string, items: { label: string, href: string, icon: React.ElementType }[] }[], location: Location, - user: { name?: string; email?: string; roles?: string[] } | null, + user: { name?: string; email?: string; systemRole?: string } | null, navigate: NavigateFunction, logout: () => void }) => (apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
203-251: Direct DB session validation is solid, but consider time comparison edge cases.The expiry check at line 217 compares timestamps correctly. However, be aware that if there's clock skew between servers or if DB timestamps use a different timezone, edge cases could occur.
Consider adding a small grace period or ensuring timezone consistency:
- if (session.expiresAt < new Date()) { + // Small grace period (e.g., 5 seconds) to handle minor clock differences + const now = new Date(); + if (session.expiresAt.getTime() < now.getTime() - 5000) {This is a minor consideration and the current implementation works correctly in most scenarios.
apps/api/src/scripts/bootstrap-admin.ts (1)
78-115: Ensure DB connections close on exceptions and surface failures.
If a query/update throws afterclient.connect(), the connection may stay open andvoid bootstrap()will hide the failure. Consider atry/finallywith a top-level catch.♻️ Suggested restructuring for cleanup + error propagation
async function bootstrap() { console.log('🚀 Starting Admin Bootstrap...'); @@ console.log('2️⃣ Elevating to Platform Admin...'); await client.connect(); const db = drizzle(client, { schema }); - console.log(`Searching for: ${EMAIL}`); - const users = await db - .select() - .from(schema.user) - .where(eq(schema.user.email, EMAIL)); - - if (users.length === 0) { - console.error('❌ User not found in DB after API call.'); - await client.end(); - process.exit(1); - } - - const user = users[0]; - - await db - .update(schema.user) - .set({ systemRole: 'platform_admin' }) - .where(eq(schema.user.id, user.id)); - - console.log( - `✅ User '${user.name}' (${user.email}) is now a PLATFORM ADMIN.`, - ); + try { + console.log(`Searching for: ${EMAIL}`); + const users = await db + .select() + .from(schema.user) + .where(eq(schema.user.email, EMAIL)); + + if (users.length === 0) { + throw new Error('User not found in DB after API call.'); + } + + const user = users[0]; + + await db + .update(schema.user) + .set({ systemRole: 'platform_admin' }) + .where(eq(schema.user.id, user.id)); + + console.log( + `✅ User '${user.name}' (${user.email}) is now a PLATFORM ADMIN.`, + ); + } finally { + await client.end(); + } @@ - await client.end(); } -void bootstrap(); +bootstrap().catch((e) => { + console.error('❌ Admin bootstrap failed:', e); + process.exit(1); +});
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/modules/auth/auth.controller.tsapps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsapps/api/src/modules/system-admin/system-admin.controller.tsapps/api/src/scripts/bootstrap-admin.tsapps/api/src/scripts/reset-db.tsapps/web/src/layouts/AdminLayout.tsxapps/web/src/lib/auth/AuthProvider.tsxapps/web/src/pages/LoginPage.tsxapps/web/src/pages/public/AcceptInvitePage.tsxcoderabbit.yaml
✅ Files skipped from review due to trivial changes (1)
- coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/pages/public/AcceptInvitePage.tsx
- apps/web/src/lib/auth/AuthProvider.tsx
- apps/api/src/scripts/reset-db.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/api/src/scripts/bootstrap-admin.ts (2)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)apps/api/src/main.ts (1)
bootstrap(10-35)
apps/web/src/layouts/AdminLayout.tsx (1)
apps/api/src/modules/users/user.schema.ts (1)
user(3-16)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (3)
apps/api/src/modules/users/user.schema.ts (2)
User(18-18)user(3-16)apps/api/src/modules/auth/auth.schema.ts (2)
session(4-18)Session(47-47)apps/api/src/modules/tenants/tenant.schema.ts (1)
invitation(39-50)
🔇 Additional comments (14)
apps/web/src/pages/LoginPage.tsx (2)
17-27: LGTM! Auth loading state now correctly used for auto-redirect.The auto-redirect logic now properly uses
isLoadingfrom the auth context instead of the form submission state, ensuring the redirect waits for auth initialization to complete.
63-72: LGTM! Open redirect vulnerability properly mitigated.The validation ensuring
toParamstarts with/and doesn't start with//prevents protocol-relative URLs and absolute URL redirects. This addresses the security concern.apps/web/src/layouts/AdminLayout.tsx (3)
126-131: LGTM! Type extended to include systemRole for authorization.The type correctly reflects the backend schema where
systemRoleis'platform_admin' | 'user'.
142-146: LGTM! Authorization check now uses systemRole.The strict platform admin check using
systemRolealigns with the backend user schema and provides clear console feedback on access denial.
153-159: LGTM! Access denied UI now renders instead of blank screen.This addresses the previous concern about rendering
nullon unauthorized users, providing a visible fallback while the redirect is pending.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (6)
43-50: LGTM! System role configuration properly added.The
additionalFieldsconfiguration correctly registerssystemRoleas an optional string field, aligning with the database schema wheresystemRoledefaults to'user'.
141-200: Login flow correctly refactored with proper cookie handling.The implementation properly:
- Uses
asResponse: trueto capture native headers- Extracts the session from DB using the token
- Enriches user with
systemRolefrom DB- Returns the cookie string for header forwarding
One minor observation: the error message at line 149 includes
apiResponse.statusTextwhich is safe, but consider logging the full error server-side for debugging while returning a generic message to clients.
253-265: New header-based session retrieval looks correct.The method properly delegates to Better Auth's
api.getSessionwith headers for cookie parsing. The null check and type casting are appropriate.
291-309: System role enrichment in getEnrichedSession is consistent.The fresh DB query for
systemRoleensures the enriched session always has the latest role, which is important for permission changes to take effect without requiring re-login.
394-409: LGTM! Direct DB query for listing invitations is efficient.The query correctly filters by
organizationIdandstatus: 'pending', with proper ordering bycreatedAtdescending. This avoids unnecessary API overhead.
415-424: Force email verification bypasses normal verification flow.This is intentional for the invite acceptance flow where the email is implicitly trusted. However, ensure this method is only called from trusted internal flows (like
completeInvite) and never exposed directly to user input.The current usage appears safe since it's called after successful invitation acceptance.
apps/api/src/modules/system-admin/system-admin.controller.ts (1)
25-29: This concern does not apply to the code. Theusertable schema contains only safe, non-sensitive columns (id, name, email, emailVerified, image, createdAt, updatedAt, role, systemRole, banned, banReason, banExpires). There are no password hashes, tokens, or secrets stored in the user table that would be exposed by thefindMany()query. Sensitive authentication data (passwords, tokens) is stored in separatesessionandaccounttables, which are not queried here. The endpoint is also properly protected by the SystemAdminGuard.Likely an incorrect or invalid review comment.
apps/api/src/scripts/bootstrap-admin.ts (2)
16-29: Good: env-driven credentials with fail-fast validation.
This keeps secrets out of the repo and stops early if required values are missing.
45-56: Verify runtime support for globalfetch.
This script assumesfetchexists. If the runtime is Node < 18 or lacks a polyfill, it will fail at runtime. Please confirm the engine (or add an explicit guard/polyfill).✅ Optional guard to fail fast with a clear error
+if (typeof fetch !== 'function') { + throw new Error( + 'Global fetch is unavailable. Use Node 18+ or add a fetch polyfill.', + ); +}
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Validate Invite URL origin against ALLOWED_ORIGINS - Use native inviterId in invitation payload - Clamp pageSize in System Admin API - Document FRONTEND_URL in .env.example - Improve test coverage for better-auth provider
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 161-220: The code currently calls
apiResponse.headers.get('set-cookie') which only returns a single cookie;
replace that with apiResponse.headers.getSetCookie() to capture all Set-Cookie
values (array of strings), rename cookieHeader accordingly, update the returned
cookie field in the object returned by the login flow to be string | string[]
(or undefined) and propagate that type through the IdentityProvider.login return
type so callers (e.g., controller res.setHeader) can accept either a string or
string[]; ensure any downstream uses handle both single-string and array cases.
In `@apps/api/src/modules/system-admin/system-admin.controller.ts`:
- Around line 62-82: The listTenants method returns all tenants and loads all
member rows to compute userCount; update listTenants to accept pagination
(page/limit or skip/take) consistent with listUsers, use
this.db.query.organization.findMany with skip/take (or equivalent) and fetch
only tenant fields, obtain total via this.db.query.organization.count, and
replace loading members with an aggregate count per organization (e.g., a
members.count/subquery or a grouped count query) so userCount is computed by the
DB rather than relying on t.members.length; locate symbols listTenants,
this.db.query.organization.findMany, and t.members to implement these changes.
♻️ Duplicate comments (1)
apps/api/src/modules/system-admin/system-admin.controller.ts (1)
20-27: Page size capping is now properly implemented.The pagination logic correctly enforces bounds with
MAX_PAGE_SIZE = 100andMath.min/Math.max, addressing the previous concern about unbounded queries.Minor: Consider adding explicit radix to
parseIntcalls (e.g.,parseInt(page, 10)) for clarity, though behavior is correct for decimal strings.
🧹 Nitpick comments (2)
apps/api/.env.example (1)
19-21: Remove quotes from environment variable values.In
.envfiles, quotes around values are optional and can sometimes cause parsing issues depending on the dotenv parser. The dotenv-linter correctly flags this as a style issue. For consistency and to avoid potential parsing problems, consider removing the quotes from these three variables.♻️ Proposed fix
-FRONTEND_URL="https://app.nexiom.com" -ALLOWED_ORIGINS="https://app.nexiom.com,http://localhost:3000" -BETTER_AUTH_URL="https://api.nexiom.com" +FRONTEND_URL=https://app.nexiom.com +ALLOWED_ORIGINS=https://app.nexiom.com,http://localhost:3000 +BETTER_AUTH_URL=https://api.nexiom.comapps/api/src/modules/system-admin/system-admin.controller.ts (1)
41-59: Consider condensing verbose developer notes.Lines 41-54 read like internal deliberation rather than documentation. A single concise comment explaining the design choice would be cleaner.
🧹 Proposed cleanup
- // Return in format Refine expects (or standard API) - // Refine simple-rest expects header x-total-count usually, OR a { data, total } envelope if customized. - // Our existing data-provider handles standard REST? - // Let's stick to simple REST array or envelope. - // Looking at UserList.tsx:22 `data?.data?.map`, it expects `{ data: [...], total: ... }` envelope from Refine's hook? - // Actually Refine's `simple-rest` generally expects just generic List. - // But `UserList.tsx` accesses `data.data`. - // Let's verify `dataProvider` source later, but Envelope is safest for custom controllers. - /* - Refine's `useTable` returns { data: { data: [...], total: ... } } if the DataProvider returns { data: [...], total: ... } - Standard `simple-rest` expects array and `x-total-count` header. - BUT, we should match what the existing API does. - Let's assume the Envelope pattern for now as it's cleaner. - */ + // Return envelope format { data, total } expected by Refine's useTable hook return { - // Envelope data: users, total, };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api/.env.exampleapps/api/src/modules/auth/auth.controller.tsapps/api/src/modules/auth/identity-provider.abstract.tsapps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.tsapps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsapps/api/src/modules/system-admin/system-admin.controller.tsapps/api/src/shared/utils/headers.util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/auth/identity-provider.abstract.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/src/modules/auth/auth.controller.ts (6)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
login(157-221)apps/api/src/modules/auth/auth.schema.ts (2)
Session(47-47)session(4-18)apps/api/src/modules/users/user.schema.ts (2)
User(18-18)user(3-16)apps/api/src/shared/utils/headers.util.ts (1)
toWebHeaders(7-16)apps/api/src/modules/users/users.validation.ts (1)
CompleteInvite(55-55)apps/api/src/modules/tenants/tenant.schema.ts (1)
invitation(39-50)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (3)
apps/api/src/modules/users/user.schema.ts (2)
User(18-18)user(3-16)apps/api/src/modules/auth/auth.schema.ts (2)
session(4-18)Session(47-47)apps/api/src/modules/tenants/tenant.schema.ts (1)
invitation(39-50)
🪛 dotenv-linter (4.0.0)
apps/api/.env.example
[warning] 19-19: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 20-20: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 20-20: [UnorderedKey] The ALLOWED_ORIGINS key should go before the FRONTEND_URL key
(UnorderedKey)
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 21-21: [UnorderedKey] The BETTER_AUTH_URL key should go before the FRONTEND_URL key
(UnorderedKey)
🪛 GitHub Check: Build, Lint, and Test
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts
[warning] 208-208:
Unsafe argument of type any assigned to a parameter of type { id: string; createdAt: Date; updatedAt: Date; expiresAt: Date; token: string; ipAddress: string | null; userAgent: string | null; userId: string; impersonatedBy: string | null; } | PgRelationalQuery<...> | undefined
[warning] 194-194:
Unsafe argument of type any assigned to a parameter of type { id: string; createdAt: Date; updatedAt: Date; expiresAt: Date; token: string; ipAddress: string | null; userAgent: string | null; userId: string; impersonatedBy: string | null; } | PgRelationalQuery<...> | undefined
🔇 Additional comments (22)
apps/api/.env.example (1)
19-21: Verify that production URLs in .env.example are intentional.The
.env.examplefile contains what appear to be real production URLs (app.nexiom.com,api.nexiom.com). Typically, example environment files use placeholder values (e.g.,example.com,your-app.com) to avoid confusion or accidental use of production values.Please confirm whether these production URLs are intentional documentation or should be replaced with placeholder values.
apps/api/src/modules/system-admin/system-admin.controller.ts (1)
1-13: LGTM!Imports are appropriate, and the controller setup with class-level
SystemAdminGuardcorrectly protects all admin endpoints. Drizzle DB injection follows the established pattern.apps/api/src/shared/utils/headers.util.ts (1)
1-16: LGTM — clean, focused header conversion utility.
Handles array values and produces a standardHeadersinstance as intended.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (9)
43-50: LGTM — optional systemRole field aligns with DB schema.
69-96: Good security hardening for invite URL resolution.
Trusted-origins validation and explicit base URL checks are solid.
107-113: LGTM — cookie attributes now environment-aware.
224-271: LGTM — direct DB validation + expiry check is solid.
273-285: LGTM — header-based session extraction delegated cleanly.
311-330: LGTM — systemRole refresh in enriched session is consistent.
340-373: LGTM — inviterId + optional headers support is clear and aligned.
405-420: LGTM — DB-backed invitation listing is straightforward.
426-439: LGTM — force verify + delete utilities are concise.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts (6)
45-61: LGTM — mock DB extended appropriately for new session/user lookups.
161-186: LGTM — login test now mirrors response + DB lookup flow.
189-214: LGTM — validateSession edge cases covered (missing/expired).
229-251: LGTM — enriched session path mocks updated correctly.
264-301: LGTM — invitation payload expectations updated for inviterId.
344-363: LGTM — invite email test aligns with new URL construction.apps/api/src/modules/auth/auth.controller.ts (4)
44-61: LGTM — cookie forwarded via header without leaking in body.
73-85: LGTM — header conversion for session extraction is correct.
87-145: LGTM — invite completion flow now validates + rolls back safely.
151-167: LGTM — refresh-session uses header-based session and enrichment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Use asResponse: true to get the full response headers (including Set-Cookie) | ||
| // This allows us to forward the exact cookie Better Auth generates (signed/unsigned correctly) | ||
| const apiResponse = await this.auth.api.signInEmail({ | ||
| body: { email, password }, | ||
| asResponse: false, | ||
| asResponse: true, | ||
| }); | ||
|
|
||
| if (!apiResponse.ok) { | ||
| throw new Error('Login failed (API Error): ' + apiResponse.statusText); | ||
| } | ||
|
|
||
| const cookieHeader = apiResponse.headers.get('set-cookie'); | ||
| const result = (await apiResponse.json()) as { | ||
| token: string; | ||
| user: { | ||
| id: string; | ||
| email: string; | ||
| name: string; | ||
| image?: string | null; | ||
| emailVerified: boolean; | ||
| createdAt: string; | ||
| updatedAt: string; | ||
| }; | ||
| }; | ||
|
|
||
| // BetterAuth returns { token, user } but not always the full session object | ||
| // We need the full session object to satisfy our Interface. | ||
| // Note: 'result.token' is the session token. | ||
| if (!result.token) { | ||
| throw new Error('Login failed: No token returned'); | ||
| } | ||
|
|
||
| // Direct DB Query for Session (More reliable than self-referential API call) | ||
| const dbSession = await this.db.query.session.findFirst({ | ||
| where: eq(schema.session.token, result.token), | ||
| }); | ||
| return result as unknown as { session: schema.Session; user: schema.User }; | ||
|
|
||
| if (!dbSession) { | ||
| // Fallback or retry? If BetterAuth just created it, it should be there. | ||
| // If not found, maybe result.token IS the session ID? | ||
| // BetterAuth usually uses token as the lookup. | ||
| throw new Error('Login succeeded but session record not found in DB'); | ||
| } | ||
|
|
||
| // Enrich with System Role from DB (Explicit) | ||
| const dbUser = await this.db.query.user.findFirst({ | ||
| where: eq(schema.user.id, result.user.id), | ||
| columns: { | ||
| systemRole: true, | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| session: dbSession, | ||
| user: { | ||
| ...result.user, | ||
| systemRole: dbUser?.systemRole || 'user', | ||
| } as unknown as schema.User, | ||
| cookie: cookieHeader || undefined, // Return the native cookie string | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find where this code is used
git ls-files apps/api/src/modules/auth | head -20Repository: pramodnarayana/nexiom
Length of output: 678
🏁 Script executed:
# Search for where cookieHeader is used downstream
rg "cookieHeader" apps/api/src/modules/auth --type ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 1102
🏁 Script executed:
# Check the IdentityProvider interface/type definition
rg "IdentityProvider|interface.*Provider" apps/api/src/modules/auth --type ts -A 5Repository: pramodnarayana/nexiom
Length of output: 20994
🏁 Script executed:
# Look for Response headers usage with set-cookie
rg "set-cookie|Set-Cookie" apps/api/src/modules/auth --type ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 2388
🏁 Script executed:
# Check if there are any tests or type definitions that show Better Auth's response structure
fd -e ts -e tsx apps/api/src | xargs rg "signInEmail" --type ts -B 3 -A 3Repository: pramodnarayana/nexiom
Length of output: 3437
🏁 Script executed:
cat -n apps/api/src/modules/auth/identity-provider.abstract.tsRepository: pramodnarayana/nexiom
Length of output: 4031
🏁 Script executed:
cat -n apps/api/src/modules/auth/auth.controller.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 3772
🏁 Script executed:
cat -n apps/api/src/modules/auth/auth.controller.ts | tail -100Repository: pramodnarayana/nexiom
Length of output: 4101
🏁 Script executed:
# Check the login method in better-auth provider more carefully
sed -n '150,230p' apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts | cat -nRepository: pramodnarayana/nexiom
Length of output: 3225
🏁 Script executed:
# Check the test file to understand what Better Auth's response looks like
cat -n apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.tsRepository: pramodnarayana/nexiom
Length of output: 14299
🏁 Script executed:
# Check if there's any info about Response headers handling in the codebase
rg "getSetCookie|raw\(\)|Headers" apps/api/src --type ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 14625
🏁 Script executed:
# Look for package.json to see what version of Better Auth is being used
fd package.json -x cat {} \; | grep -A 20 -B 5 "better-auth" | head -40Repository: pramodnarayana/nexiom
Length of output: 1222
🏁 Script executed:
# Check toWebHeaders utility to understand how headers are converted
rg "toWebHeaders" apps/api/src --type ts -B 2 -A 5Repository: pramodnarayana/nexiom
Length of output: 2787
🌐 Web query:
Better Auth signInEmail response multiple Set-Cookie headers 1.4.10
💡 Result:
Short answer: forward each Set-Cookie the library returns (don't join them into one header), or use the provided helper/plugin so Better Auth's Set-Cookie headers are applied automatically.
What to do
- If you're manually proxying signInEmail, call it with returnHeaders/ asResponse to get the response object, then extract cookies with response.headers.getSetCookie() and send them as multiple Set-Cookie headers from your server (e.g. res.setHeader('Set-Cookie', cookies) or framework equivalent). This preserves multiple Set-Cookie headers instead of concatenating them. [1][2]
- If using Next.js, install/use the nextCookies plugin (plugins: [nextCookies()]) so cookies from signInEmail / signUpEmail are applied automatically. [1]
- If you're seeing infinite re-fetches / other bugs when another backend sets its own Set-Cookie alongside Better Auth, check the open issues — some clients/hooks have problems when multiple Set-Cookie headers are present and may require updating Better Auth or avoiding duplicate cookie names. See related GitHub issues. [3]
References
- Better Auth Next.js docs (nextCookies plugin). [1]
- Example / Q&A showing returnHeaders + forwarding Set-Cookie array (res.setHeader('Set-Cookie', cookies)). [2]
- Reported client bugs when multiple Set-Cookie headers cause hooks/refetch loops. [3]
If you want, tell me which framework (Next/Express/Svelte/Expo) and paste the signIn handling code and I’ll give exact code to forward the cookies.
🏁 Script executed:
# Check the full toWebHeaders implementation
cat -n apps/api/src/shared/utils/headers.util.tsRepository: pramodnarayana/nexiom
Length of output: 800
🏁 Script executed:
# Search for Response object handling and Set-Cookie
rg "Response|response\.headers" apps/api/src/modules/auth/providers/better-auth --type ts -B 3 -A 3Repository: pramodnarayana/nexiom
Length of output: 4377
🏁 Script executed:
# Check if Web API Headers is being used vs Node's native headers
rg "new Headers|apiResponse|asResponse" apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 828
🏁 Script executed:
# Look for how Better Auth actually returns the response object
rg "asResponse|signInEmail|signUpEmail" apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts -B 5 -A 10 | head -80Repository: pramodnarayana/nexiom
Length of output: 1470
🏁 Script executed:
# Check if there are any type definitions or imports that show Response type
rg "import.*Response|from.*better-auth" apps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsRepository: pramodnarayana/nexiom
Length of output: 228
🏁 Script executed:
# Check the complete login method implementation to see how cookies are handled
sed -n '150,230p' apps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsRepository: pramodnarayana/nexiom
Length of output: 2658
🏁 Script executed:
# Check if there's any usage of getSetCookie or multiple cookies elsewhere in codebase
rg "getSetCookie\|set.*cookie.*array\|Set-Cookie.*\[\]" apps/api/src --type tsRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Test if Fetch API Headers has getSetCookie method
node <<'EOF'
// Verify Fetch API Headers methods
const headers = new Headers();
headers.set('set-cookie', 'cookie1=value1');
console.log("Headers methods:", Object.getOwnPropertyNames(Object.getPrototypeOf(headers)).sort());
console.log("Has getSetCookie:", typeof headers.getSetCookie);
EOFRepository: pramodnarayana/nexiom
Length of output: 260
🏁 Script executed:
# Search for any actual Better Auth response handling or cookie tests
rg "getSetCookie|Set-Cookie.*array" --type ts -B 3 -A 3Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Verify the current Node.js/runtime supports getSetCookie on Fetch API Headers
node <<'EOF'
// Check getSetCookie behavior with multiple Set-Cookie values
const headers = new Headers();
// Simulate multiple Set-Cookie values - note: Headers.set overwrites, but let's check if the method exists
console.log("getSetCookie method exists:", typeof headers.getSetCookie);
console.log("getSetCookie is callable:", typeof headers.getSetCookie === 'function');
// Test with a real scenario
const testHeaders = new Headers([
['set-cookie', 'session=abc123; Path=/'],
['set-cookie', 'refresh=xyz789; Path=/']
]);
try {
const result = testHeaders.getSetCookie?.();
console.log("getSetCookie() returned:", result);
console.log("Type:", Array.isArray(result) ? 'array' : typeof result);
} catch (e) {
console.error("Error calling getSetCookie:", e.message);
}
// Also test .get()
console.log("get('set-cookie') returned:", testHeaders.get('set-cookie'));
EOFRepository: pramodnarayana/nexiom
Length of output: 299
🏁 Script executed:
# Check what version of Node is being used and if getSetCookie is available in runtime
grep -r "node" package.json apps/api/package.json 2>/dev/null | head -5Repository: pramodnarayana/nexiom
Length of output: 404
🏁 Script executed:
# Look for any .nvmrc or engines specification
fd -e nvmrc -e "engines" -x cat {} \;Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Check if the proposed fix's syntax would even work with the current code pattern
rg "apiResponse\.headers\." apps/api/src/modules/auth/providers/better-auth/better-auth.provider.tsRepository: pramodnarayana/nexiom
Length of output: 128
Use Headers.getSetCookie() to capture all Set-Cookie values from Better Auth.
The current code uses apiResponse.headers.get('set-cookie'), which returns only the first Set-Cookie value when multiple are present. Better Auth may emit multiple cookies (session + refresh token, etc.), causing downstream cookies to be lost. Use getSetCookie() instead, which returns an array of all Set-Cookie values and is available in Node >=18.
Update the return type to support string | string[] and adjust accordingly:
🔧 Proposed fix
- const cookieHeader = apiResponse.headers.get('set-cookie');
+ const setCookies = apiResponse.headers.getSetCookie();
+ const cookieHeader = setCookies.length ? setCookies : undefined;Also update the IdentityProvider.login return type:
- ): Promise<{ session: Session; user: User; cookie?: string }>;
+ ): Promise<{ session: Session; user: User; cookie?: string | string[] }>;The controller's res.setHeader('Set-Cookie', value) already supports both string and string[] natively.
🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`
around lines 161 - 220, The code currently calls
apiResponse.headers.get('set-cookie') which only returns a single cookie;
replace that with apiResponse.headers.getSetCookie() to capture all Set-Cookie
values (array of strings), rename cookieHeader accordingly, update the returned
cookie field in the object returned by the login flow to be string | string[]
(or undefined) and propagate that type through the IdentityProvider.login return
type so callers (e.g., controller res.setHeader) can accept either a string or
string[]; ensure any downstream uses handle both single-string and array cases.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.