add multi auth guard#41
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds super-admin authentication infrastructure with provider-aware session tracking across the entire Holo auth system. It introduces a dedicated admin guard/provider, creates the admins database table with password hashing, implements super-admin login/logout flows and UI pages for Next/Nuxt/SvelteKit, adds provider gating to Clerk/WorkOS logout routes, extends the user endpoint with optional guard query parameters, and updates scaffolding, model registry, docs, and tests. ChangesSuper-Admin with Provider Awareness
Estimated code review effort Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/project/scaffold/framework-renderers.ts (1)
186-209:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd error handling for provider() check in generated logout routes.
The Nuxt logout route generator (and the Next/Svelte equivalents at lines 389-408 and 677-697) calls
await provider()without error handling. Ifprovider()throws, the generated route will fail with a 500 error, preventing users from logging out. Theauth()helpers inpackages/auth/src/*/server.tshandle similar errors gracefully—the scaffolded logout routes should do the same.🛡️ Proposed fix for Nuxt logout route generator
function renderNuxtHostedAuthLogoutRoute(spec: HostedAuthProviderSpec): string { return [ 'import { provider } from \'@holo-js/auth\'', `import { ${spec.logoutFunction} } from '${spec.packageName}'`, 'import { createError, sendRedirect } from \'h3\'', '', 'export default defineEventHandler(async (event) => {', - ` if (await provider() !== '${spec.provider}') {`, - ' return await sendRedirect(event, \'/\', 303)', - ' }', + ' try {', + ` if (await provider() !== '${spec.provider}') {`, + ' return await sendRedirect(event, \'/\', 303)', + ' }', + ' } catch (error) {', + ' console.warn(\'Failed to resolve auth provider for logout.\', error)', + ' return await sendRedirect(event, \'/\', 303)', + ' }', '', ` const result = await ${spec.logoutFunction}(event)`,Apply similar changes to
renderNextHostedAuthLogoutRoute(lines 389-408) andrenderSvelteHostedAuthLogoutRoute(lines 677-697).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/project/scaffold/framework-renderers.ts` around lines 186 - 209, The generated logout routes call await provider() without error handling; wrap that call in a try/catch so a thrown error doesn't produce a 500. In renderNuxtHostedAuthLogoutRoute (and likewise update renderNextHostedAuthLogoutRoute and renderSvelteHostedAuthLogoutRoute) replace the bare `if (await provider() !== '${spec.provider}')` check with a try/catch around `await provider()` and on catch perform the same graceful behavior as the mismatch case (e.g., await sendRedirect(event, '/', 303) or equivalent), ensuring the rest of the logout logic runs only when provider() succeeds and matches spec.provider.apps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.ts (1)
5-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd error handling for provider() check.
The
provider()call on line 6 can throw if the auth system encounters an error. Without a try-catch wrapper, the route will return a 500 error, preventing users from logging out. Theauth()function inpackages/auth/src/sveltekit/server.tshandles similar failures gracefully—this route should do the same.🛡️ Proposed fix
export const POST = (async (event) => { + try { - if (await provider() !== 'workos') { - throw redirect(303, '/') - } + if (await provider() !== 'workos') { + throw redirect(303, '/') + } + } catch (error) { + console.warn('Failed to resolve auth provider for logout.', error) + throw redirect(303, '/') + } const result = await logoutWithWorkos(event)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-sveltekit/src/routes/api/auth/workos/logout/`+server.ts around lines 5 - 16, Wrap the provider() call inside POST in a try/catch to handle thrown errors and return a graceful 4xx response instead of letting the route throw a 500; specifically, catch errors from provider(), and if an error occurs return a Response.json({ ok: false, error: String(err) }, { status: 422 }) (or redirect to '/' if you prefer the same UX), then continue to call logoutWithWorkos(event) as before—update the POST handler that uses provider(), logoutWithWorkos, and satisfies RequestHandler to include this error handling.
♻️ Duplicate comments (4)
apps/blog-sveltekit/src/routes/api/auth/clerk/logout/+server.ts (1)
5-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame missing error handling as WorkOS logout route.
This route has the same issue as
apps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.ts: theprovider()call needs error handling to prevent 500 errors when logout is attempted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-sveltekit/src/routes/api/auth/clerk/logout/`+server.ts around lines 5 - 16, Wrap the initial provider() call in a try/catch inside the POST handler to avoid uncaught exceptions: call await provider() in the try block and if it returns a value not equal to 'clerk' keep the existing redirect; in the catch block return a safe error response (e.g., Response.json({ ok: false, error: err.message }, { status: 422 })) so the route returns a 4xx instead of causing a 500. Ensure this change is applied in the POST handler and does not alter the subsequent logoutWithClerk(event) logic.apps/blog-next/app/api/auth/clerk/logout/route.ts (1)
4-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame missing error handling as other logout routes.
This route needs error handling for the
provider()call, as flagged in the scaffolding generator and other logout routes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-next/app/api/auth/clerk/logout/route.ts` around lines 4 - 15, The POST route currently calls provider() (and later logoutWithClerk()) without error handling; wrap the provider() call in try/catch inside the POST function and return an appropriate error Response (e.g., JSON with a 500 status) if provider() throws or returns an unexpected value, and also guard the logoutWithClerk(request) call with try/catch to return a 500/422 JSON error on failure instead of allowing unhandled exceptions; update handling around provider(), logoutWithClerk(), and the POST function to mirror the other logout routes' error responses.apps/blog-next/app/api/auth/workos/logout/route.ts (1)
4-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame missing error handling as other logout routes.
This route needs error handling for the
provider()call, as flagged in the scaffolding generator and other logout routes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-next/app/api/auth/workos/logout/route.ts` around lines 4 - 15, The POST route calls provider() without guarding against exceptions; wrap the provider() invocation in a try/catch inside the POST function (and mirror the pattern used in other logout routes) so any thrown error is caught, logged if you have a logger, and the handler returns a safe error Response (e.g., Response.json({ error: 'Failed to determine provider' }, { status: 500 }) or a redirect to '/' consistent with other logout handlers). Ensure you still short-circuit when provider() !== 'workos' and preserve the existing logoutWithWorkos(request) flow, but handle and return a proper Response on provider() failure.apps/blog-nuxt/server/api/auth/workos/logout.post.ts (1)
5-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame missing error handling as other logout routes.
This route needs error handling for the
provider()call, as flagged in the scaffolding generator (packages/cli/src/project/scaffold/framework-renderers.ts) and the SvelteKit WorkOS logout route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/server/api/auth/workos/logout.post.ts` around lines 5 - 19, The provider() call can throw and lacks error handling; wrap the provider() invocation in a try/catch at the top of the defineEventHandler to catch exceptions and convert them into a proper createError (e.g., statusCode 500 with the error message) or a safe redirect; after a successful provider() call continue to check if it !== 'workos' and return sendRedirect as before. Specifically, modify the handler around provider() (referencing provider() and defineEventHandler) to try { const p = await provider(); if (p !== 'workos') return sendRedirect(...) } catch (err) { throw createError({ statusCode: 500, statusMessage: err?.message || 'Error resolving provider' }) }, then proceed to call logoutWithWorkos(event) and handle its result as existing code does.
🧹 Nitpick comments (6)
packages/auth/src/next/client.ts (1)
46-52: ⚡ Quick winBehavioral change in
refreshUserimplementation.The
refreshUserfunction now callsauthClientInternals.fetchCurrentUser(..., { force: true })instead of a dedicated refresh method. This fetches the complete auth state (user + provider) rather than just refreshing the user. The function still returns only the user for backward compatibility.Note that errors from
fetchCurrentUserwill propagate to the caller. Consider whether explicit error handling or logging is needed here.🛡️ Consider adding error handling
const refreshUser = useCallback(async () => { + try { const currentAuth = await authClientInternals.fetchCurrentUser(requestOptionsRef.current, { force: true, }) setCurrentProvider(currentAuth.provider) setCurrentUser(currentAuth.user) return currentAuth.user + } catch (error) { + // Handle error: log, set error state, or reset auth state + setCurrentProvider(null) + setCurrentUser(null) + throw error + } }, [])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth/src/next/client.ts` around lines 46 - 52, The new refreshUser implementation now calls authClientInternals.fetchCurrentUser(...) which can throw and also updates provider state; wrap the call in refreshUser in a try/catch around authClientInternals.fetchCurrentUser(requestOptionsRef.current, { force: true }), log the caught error (e.g. via console.error or your existing logger), still update setCurrentProvider and setCurrentUser on success, and then rethrow the error (or return a defined fallback like null if you prefer not to propagate) so callers get clear error visibility; ensure references to refreshUser, authClientInternals.fetchCurrentUser, setCurrentProvider and setCurrentUser are used so the correct code is updated.packages/db/src/model/defineModelHelpers.ts (1)
54-67: 💤 Low valueConsider making the error check more robust.
The exact string match for
error.message === 'DB facade is not configured with a ConnectionManager.'could break if the error message changes. Consider checking error properties or using error codes if available.♻️ Alternative approach
function getRuntimeSchemaTable(tableName: string, connectionName?: string): TableDefinition | undefined { try { return DB.getManager().connection(connectionName).getSchemaRegistry().get(tableName) } catch (error) { if ( error instanceof ConfigurationError - && error.message === 'DB facade is not configured with a ConnectionManager.' + && error.message.includes('not configured with a ConnectionManager') ) { return undefined } throw error } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/model/defineModelHelpers.ts` around lines 54 - 67, The current getRuntimeSchemaTable function relies on an exact error.message string which is brittle; update the catch to more robustly detect the "missing ConnectionManager" case by checking error type (ConfigurationError) plus either a stable error code (e.g., error.code === '<appropriate_code>' if available) or a substring/more tolerant match (e.g., error.message.includes('ConnectionManager') or error.message.includes('not configured')) instead of strict equality, and return undefined in that case; keep throwing the error for all other cases. Reference: getRuntimeSchemaTable and ConfigurationError.apps/blog-nuxt/app/app.vue (1)
6-7: 💤 Low valueConsider handling null/undefined provider values.
The computed properties assume
provider.valueis always a string, but it might benullorundefinedwhen not authenticated or during initialization. While thev-if="isClerkSession"will correctly evaluate tofalsein those cases, it would be more explicit to handle this.🔍 Optional: More explicit null handling
-const isClerkSession = computed(() => provider.value === 'clerk') -const isWorkosSession = computed(() => provider.value === 'workos') +const isClerkSession = computed(() => provider.value === 'clerk') +const isWorkosSession = computed(() => provider.value === 'workos')Actually, on second thought, the current implementation is fine since
null === 'clerk'andundefined === 'clerk'both returnfalse, which is the desired behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/app/app.vue` around lines 6 - 7, The computed properties isClerkSession and isWorkosSession assume provider.value is a string; make them explicitly handle null/undefined by normalizing provider.value before comparison (e.g., coerce to empty string or use a nullish fallback) so comparisons like provider.value === 'clerk' become safe and explicit even during initialization; update the computed definitions (isClerkSession, isWorkosSession) to compare against a normalized value derived from provider.value.apps/blog-nuxt/server/db/seeders/BlogSeeder.ts (1)
14-15: ⚡ Quick winAvoid fixed seeded credentials in code.
Same concern here: hardcoded seed passwords make accidental non-local seeding unsafe. Prefer env-backed values.
Suggested patch
- const userPassword = await hashPassword('secret') - const adminPassword = await hashPassword('admin-secret') + const seededUserPassword = process.env.SEED_BLOG_USER_PASSWORD ?? 'secret' + const seededAdminPassword = process.env.SEED_SUPER_ADMIN_PASSWORD ?? 'admin-secret' + const userPassword = await hashPassword(seededUserPassword) + const adminPassword = await hashPassword(seededAdminPassword)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/server/db/seeders/BlogSeeder.ts` around lines 14 - 15, Replace the hardcoded seed passwords by reading them from environment variables so non-local runs don't embed fixed secrets: in BlogSeeder replace direct calls that compute userPassword and adminPassword via hashPassword('secret') and hashPassword('admin-secret') with hashing of values loaded from process.env (e.g., process.env.SEED_USER_PASSWORD and process.env.SEED_ADMIN_PASSWORD), and ensure sensible defaults are only used for local/dev by checking NODE_ENV or adding a clear comment; update any references to userPassword and adminPassword accordingly so seeding uses env-backed values instead of literals.apps/blog-sveltekit/server/db/seeders/BlogSeeder.ts (1)
14-15: ⚡ Quick winAvoid fixed seeded credentials in code.
Using hardcoded seed passwords (
secret/admin-secret) leaves known credentials in non-local seed runs. Please source them from env vars (with local defaults if needed).Suggested patch
- const userPassword = await hashPassword('secret') - const adminPassword = await hashPassword('admin-secret') + const seededUserPassword = process.env.SEED_BLOG_USER_PASSWORD ?? 'secret' + const seededAdminPassword = process.env.SEED_SUPER_ADMIN_PASSWORD ?? 'admin-secret' + const userPassword = await hashPassword(seededUserPassword) + const adminPassword = await hashPassword(seededAdminPassword)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-sveltekit/server/db/seeders/BlogSeeder.ts` around lines 14 - 15, The seeder currently uses hardcoded passwords when calling hashPassword (userPassword and adminPassword); change it to read from environment variables (e.g., SEED_USER_PASSWORD and SEED_ADMIN_PASSWORD) with local defaults for development, then pass those values into hashPassword (preserving the async/await) inside BlogSeeder so seeded credentials are not fixed in code.apps/blog-nuxt/app/pages/super-admin/index.vue (1)
22-23: ⚡ Quick winConsider providing user feedback on logout errors.
The logout error is only logged to the console without any user-facing notification. If the logout fails (e.g., network issue or server error), the user won't know what happened, which may lead to confusion.
💬 Suggested improvement to show user feedback
You could display a toast notification or set an error ref to show in the template:
+const errorMessage = ref('') + async function logout() { if (isLoggingOut.value) { return } + errorMessage.value = '' isLoggingOut.value = true try { await $fetch('/api/super-admin/logout', { method: 'POST' }) await refreshUser() await navigateTo('/super-admin/login') } catch (error) { console.warn('Super admin logout failed.', error) + errorMessage.value = 'Failed to sign out. Please try again.' } finally { isLoggingOut.value = false } }Then display the error in the template:
<p>Signed in as {{ displayName }} through the admin guard.</p> + <p v-if="errorMessage" style="color: `#ef4444`;">{{ errorMessage }}</p> <div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/app/pages/super-admin/index.vue` around lines 22 - 23, The catch block that currently calls console.warn('Super admin logout failed.', error) should provide user-facing feedback: replace or augment that console.warn in the catch (error) handler inside apps/blog-nuxt/app/pages/super-admin/index.vue with code that sets a reactive error state (e.g., errorRef or logoutError) and/or triggers a toast (e.g., useToast()/this.$toast) showing a friendly message plus the error details; also update the component template to render the errorRef or toast so users see the failure. Ensure you reference the existing catch (error) block and the string 'Super admin logout failed.' when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/blog-next/server/db/seeders/BlogSeeder.ts`:
- Around line 14-15: The seed currently uses hardcoded passwords
('secret'/'admin-secret') via hashPassword to set userPassword and adminPassword
in BlogSeeder; change it to read passwords from environment variables (e.g.,
process.env.SEED_USER_PASSWORD and process.env.SEED_ADMIN_PASSWORD) and only
allow defaults in local/dev (NODE_ENV==='development' or similar), otherwise
throw/exit if the env vars are missing so privileged accounts cannot be seeded
with predictable credentials; update the userPassword/adminPassword assignments
to use these env values before calling hashPassword and add a clear error path
when required vars are absent.
In `@apps/blog-sveltekit/server/db/migrations/2026_04_25_044031_create_admins.ts`:
- Around line 7-9: The migration creates the admins table with nullable identity
columns; update the table schema in create_admins.ts so that
table.string('name') and table.string('email').unique() are declared
notNullable() (keep password as nullable if intended) to enforce required
identities and ensure the unique constraint applies correctly across rows; run
or regenerate the migration if your workflow requires a new migration artifact
after this change.
In `@apps/blog-sveltekit/src/routes/`+layout.svelte:
- Around line 46-58: The UI renders both a generic Logout button
(onclick={logout}) and provider-specific logout forms when auth.authenticated,
causing redundancy; choose one approach and remove the other: either remove the
provider-specific form blocks that check auth.provider === 'workos' / 'clerk'
and have the generic logout handler (logout) perform provider-aware sign-out, or
remove the generic button and keep only the provider forms
(action="/api/auth/workos/logout" and action="/api/auth/clerk/logout") with
their submit buttons; update the +layout.svelte markup accordingly and ensure
the logout function or remaining form actions handle provider-specific cleanup.
In `@packages/auth-clerk/src/index.ts`:
- Around line 1070-1073: The current return reuses a hosted session by returning
provider: 'clerk' whenever source.clerk exists, but it doesn't verify the stored
clerk provider name matches the active provider, which can leak sessions across
providers; update the condition that sets provider (the expression using source,
payload and returning 'clerk') to only return 'clerk' when source.clerk is an
object AND source.clerk.provider (the stored provider name) equals the active
provider name (e.g., payload.provider or the module's configured Clerk provider)
so wrong-provider sessions are not reused; keep authenticated.guard as-is but
tighten the provider selection logic to validate payload.clerk.provider before
reusing.
In `@packages/auth-workos/src/index.ts`:
- Around line 1067-1070: The code currently sets provider to 'workos' whenever a
stored workos session exists, but doesn't verify the stored workos.provider
matches the currently authenticated provider; change the provider selection to
only reuse the stored WorkOS session when source.workos is an object and its
provider field equals the current payload.provider (and is a string).
Concretely, in the return object where provider is chosen (near variables
source, payload, authenticated.guard), replace the unconditional check with one
that also validates source.workos.provider === payload.provider (and that
source.workos.provider is typeof 'string') so you only return 'workos' when the
hosted provider names match, otherwise fall back to payload.provider.
In `@packages/auth/src/next/server.ts`:
- Around line 84-100: The auth() function should mirror the SvelteKit
implementation by wrapping the calls that resolve user and provider in a
try-catch: keep computing guard as you do using
authRuntimeInternals.getRuntimeBindings().config.defaults.guard, then inside a
try block call either holoAuth.guard(guard).user()/provider() or
currentUser()/currentProvider(), convert the user with toClientAuthUser, and
return the normal AuthState; in the catch block log a warning (e.g.,
console.warn('Failed to resolve Next.js auth state.', error)) and return the
safe fallback AuthState { authenticated: false, guard, provider: null, user:
null } so user()/provider() failures don’t throw uncaught exceptions.
In `@packages/db/src/model/ModelRegistry.ts`:
- Around line 32-35: The fallback branch that runs when leftTableName or
rightTableName is missing currently only compares left.name and left.morphClass;
update this to also compare left.primaryKey === right.primaryKey so
registrations with different primary keys are not treated as equal. Locate the
check in ModelRegistry (the block using left, right, leftTableName,
rightTableName) and add the primaryKey equality into the returned conjunction.
---
Outside diff comments:
In `@apps/blog-sveltekit/src/routes/api/auth/workos/logout/`+server.ts:
- Around line 5-16: Wrap the provider() call inside POST in a try/catch to
handle thrown errors and return a graceful 4xx response instead of letting the
route throw a 500; specifically, catch errors from provider(), and if an error
occurs return a Response.json({ ok: false, error: String(err) }, { status: 422
}) (or redirect to '/' if you prefer the same UX), then continue to call
logoutWithWorkos(event) as before—update the POST handler that uses provider(),
logoutWithWorkos, and satisfies RequestHandler to include this error handling.
In `@packages/cli/src/project/scaffold/framework-renderers.ts`:
- Around line 186-209: The generated logout routes call await provider() without
error handling; wrap that call in a try/catch so a thrown error doesn't produce
a 500. In renderNuxtHostedAuthLogoutRoute (and likewise update
renderNextHostedAuthLogoutRoute and renderSvelteHostedAuthLogoutRoute) replace
the bare `if (await provider() !== '${spec.provider}')` check with a try/catch
around `await provider()` and on catch perform the same graceful behavior as the
mismatch case (e.g., await sendRedirect(event, '/', 303) or equivalent),
ensuring the rest of the logout logic runs only when provider() succeeds and
matches spec.provider.
---
Duplicate comments:
In `@apps/blog-next/app/api/auth/clerk/logout/route.ts`:
- Around line 4-15: The POST route currently calls provider() (and later
logoutWithClerk()) without error handling; wrap the provider() call in try/catch
inside the POST function and return an appropriate error Response (e.g., JSON
with a 500 status) if provider() throws or returns an unexpected value, and also
guard the logoutWithClerk(request) call with try/catch to return a 500/422 JSON
error on failure instead of allowing unhandled exceptions; update handling
around provider(), logoutWithClerk(), and the POST function to mirror the other
logout routes' error responses.
In `@apps/blog-next/app/api/auth/workos/logout/route.ts`:
- Around line 4-15: The POST route calls provider() without guarding against
exceptions; wrap the provider() invocation in a try/catch inside the POST
function (and mirror the pattern used in other logout routes) so any thrown
error is caught, logged if you have a logger, and the handler returns a safe
error Response (e.g., Response.json({ error: 'Failed to determine provider' }, {
status: 500 }) or a redirect to '/' consistent with other logout handlers).
Ensure you still short-circuit when provider() !== 'workos' and preserve the
existing logoutWithWorkos(request) flow, but handle and return a proper Response
on provider() failure.
In `@apps/blog-nuxt/server/api/auth/workos/logout.post.ts`:
- Around line 5-19: The provider() call can throw and lacks error handling; wrap
the provider() invocation in a try/catch at the top of the defineEventHandler to
catch exceptions and convert them into a proper createError (e.g., statusCode
500 with the error message) or a safe redirect; after a successful provider()
call continue to check if it !== 'workos' and return sendRedirect as before.
Specifically, modify the handler around provider() (referencing provider() and
defineEventHandler) to try { const p = await provider(); if (p !== 'workos')
return sendRedirect(...) } catch (err) { throw createError({ statusCode: 500,
statusMessage: err?.message || 'Error resolving provider' }) }, then proceed to
call logoutWithWorkos(event) and handle its result as existing code does.
In `@apps/blog-sveltekit/src/routes/api/auth/clerk/logout/`+server.ts:
- Around line 5-16: Wrap the initial provider() call in a try/catch inside the
POST handler to avoid uncaught exceptions: call await provider() in the try
block and if it returns a value not equal to 'clerk' keep the existing redirect;
in the catch block return a safe error response (e.g., Response.json({ ok:
false, error: err.message }, { status: 422 })) so the route returns a 4xx
instead of causing a 500. Ensure this change is applied in the POST handler and
does not alter the subsequent logoutWithClerk(event) logic.
---
Nitpick comments:
In `@apps/blog-nuxt/app/app.vue`:
- Around line 6-7: The computed properties isClerkSession and isWorkosSession
assume provider.value is a string; make them explicitly handle null/undefined by
normalizing provider.value before comparison (e.g., coerce to empty string or
use a nullish fallback) so comparisons like provider.value === 'clerk' become
safe and explicit even during initialization; update the computed definitions
(isClerkSession, isWorkosSession) to compare against a normalized value derived
from provider.value.
In `@apps/blog-nuxt/app/pages/super-admin/index.vue`:
- Around line 22-23: The catch block that currently calls console.warn('Super
admin logout failed.', error) should provide user-facing feedback: replace or
augment that console.warn in the catch (error) handler inside
apps/blog-nuxt/app/pages/super-admin/index.vue with code that sets a reactive
error state (e.g., errorRef or logoutError) and/or triggers a toast (e.g.,
useToast()/this.$toast) showing a friendly message plus the error details; also
update the component template to render the errorRef or toast so users see the
failure. Ensure you reference the existing catch (error) block and the string
'Super admin logout failed.' when making the change.
In `@apps/blog-nuxt/server/db/seeders/BlogSeeder.ts`:
- Around line 14-15: Replace the hardcoded seed passwords by reading them from
environment variables so non-local runs don't embed fixed secrets: in BlogSeeder
replace direct calls that compute userPassword and adminPassword via
hashPassword('secret') and hashPassword('admin-secret') with hashing of values
loaded from process.env (e.g., process.env.SEED_USER_PASSWORD and
process.env.SEED_ADMIN_PASSWORD), and ensure sensible defaults are only used for
local/dev by checking NODE_ENV or adding a clear comment; update any references
to userPassword and adminPassword accordingly so seeding uses env-backed values
instead of literals.
In `@apps/blog-sveltekit/server/db/seeders/BlogSeeder.ts`:
- Around line 14-15: The seeder currently uses hardcoded passwords when calling
hashPassword (userPassword and adminPassword); change it to read from
environment variables (e.g., SEED_USER_PASSWORD and SEED_ADMIN_PASSWORD) with
local defaults for development, then pass those values into hashPassword
(preserving the async/await) inside BlogSeeder so seeded credentials are not
fixed in code.
In `@packages/auth/src/next/client.ts`:
- Around line 46-52: The new refreshUser implementation now calls
authClientInternals.fetchCurrentUser(...) which can throw and also updates
provider state; wrap the call in refreshUser in a try/catch around
authClientInternals.fetchCurrentUser(requestOptionsRef.current, { force: true
}), log the caught error (e.g. via console.error or your existing logger), still
update setCurrentProvider and setCurrentUser on success, and then rethrow the
error (or return a defined fallback like null if you prefer not to propagate) so
callers get clear error visibility; ensure references to refreshUser,
authClientInternals.fetchCurrentUser, setCurrentProvider and setCurrentUser are
used so the correct code is updated.
In `@packages/db/src/model/defineModelHelpers.ts`:
- Around line 54-67: The current getRuntimeSchemaTable function relies on an
exact error.message string which is brittle; update the catch to more robustly
detect the "missing ConnectionManager" case by checking error type
(ConfigurationError) plus either a stable error code (e.g., error.code ===
'<appropriate_code>' if available) or a substring/more tolerant match (e.g.,
error.message.includes('ConnectionManager') or error.message.includes('not
configured')) instead of strict equality, and return undefined in that case;
keep throwing the error for all other cases. Reference: getRuntimeSchemaTable
and ConfigurationError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 931540b7-e0cc-4508-aa73-69371197f458
📒 Files selected for processing (79)
apps/blog-next/app/api/auth/clerk/logout/route.tsapps/blog-next/app/api/auth/user/route.tsapps/blog-next/app/api/auth/workos/logout/route.tsapps/blog-next/app/api/super-admin/login/route.tsapps/blog-next/app/api/super-admin/logout/route.tsapps/blog-next/app/auth-nav.tsxapps/blog-next/app/layout.tsxapps/blog-next/app/super-admin/login/page.tsxapps/blog-next/app/super-admin/logout-button.tsxapps/blog-next/app/super-admin/page.tsxapps/blog-next/config/auth.tsapps/blog-next/proxy.tsapps/blog-next/server/db/migrations/2026_04_25_044031_create_admins.tsapps/blog-next/server/db/seeders/BlogSeeder.tsapps/blog-next/server/holo-models.d.tsapps/blog-next/server/models/Admin.tsapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/middleware/super-admin-guest.tsapps/blog-nuxt/app/middleware/super-admin.tsapps/blog-nuxt/app/pages/super-admin/index.vueapps/blog-nuxt/app/pages/super-admin/login.vueapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/auth/clerk/logout.post.tsapps/blog-nuxt/server/api/auth/user.get.tsapps/blog-nuxt/server/api/auth/workos/logout.post.tsapps/blog-nuxt/server/api/super-admin/login.post.tsapps/blog-nuxt/server/api/super-admin/logout.post.tsapps/blog-nuxt/server/db/migrations/2026_04_25_044031_create_admins.tsapps/blog-nuxt/server/db/seeders/BlogSeeder.tsapps/blog-nuxt/server/models/Admin.tsapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/server/db/migrations/2026_04_25_044031_create_admins.tsapps/blog-sveltekit/server/db/seeders/BlogSeeder.tsapps/blog-sveltekit/server/holo-models.d.tsapps/blog-sveltekit/server/models/Admin.tsapps/blog-sveltekit/src/hooks.server.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/api/auth/clerk/logout/+server.tsapps/blog-sveltekit/src/routes/api/auth/user/+server.tsapps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.tsapps/blog-sveltekit/src/routes/api/super-admin/login/+server.tsapps/blog-sveltekit/src/routes/api/super-admin/logout/+server.tsapps/blog-sveltekit/src/routes/super-admin/+page.server.tsapps/blog-sveltekit/src/routes/super-admin/+page.svelteapps/blog-sveltekit/src/routes/super-admin/login/+page.svelteapps/docs/docs/auth/clerk.mdapps/docs/docs/auth/current-auth-client.mdapps/docs/docs/auth/workos.mdpackages/auth-clerk/src/index.tspackages/auth-clerk/tests/package.test.tspackages/auth-workos/src/index.tspackages/auth-workos/tests/package.test.tspackages/auth-workos/tests/tsconfig.jsonpackages/auth-workos/tsconfig.tests.jsonpackages/auth/src/client-runtime.tspackages/auth/src/client.tspackages/auth/src/contracts.tspackages/auth/src/index.tspackages/auth/src/next/client.tspackages/auth/src/next/server.tspackages/auth/src/nuxt.tspackages/auth/src/runtime.tspackages/auth/src/sveltekit/client.tspackages/auth/src/sveltekit/server.tspackages/auth/tests/contracts.type.test.tspackages/auth/tests/framework.test.tspackages/auth/tests/package.test.tspackages/auth/tests/tsconfig.jsonpackages/cli/src/project/discovery-helpers.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/runtime.tspackages/cli/tests/cli.test.tspackages/db/src/index.tspackages/db/src/model/ModelRegistry.tspackages/db/src/model/defineModel.tspackages/db/src/model/defineModelHelpers.tspackages/db/src/model/index.tspackages/db/tests/model-core.test.tstests/example-app-auth-flow.mjs
💤 Files with no reviewable changes (1)
- packages/cli/src/project/discovery-helpers.ts
Comment on lines
+14
to
+15
| const userPassword = await hashPassword('secret') | ||
| const adminPassword = await hashPassword('admin-secret') |
There was a problem hiding this comment.
Avoid hardcoded seeded passwords for privileged accounts.
Using fixed credentials ('secret' / 'admin-secret') creates a predictable login path if this seed data reaches non-local environments. Please source these from environment variables (or fail when missing outside local/dev).
Suggested hardening
- const userPassword = await hashPassword('secret')
- const adminPassword = await hashPassword('admin-secret')
+ const seededUserPassword = process.env.SEED_USER_PASSWORD
+ const seededAdminPassword = process.env.SEED_ADMIN_PASSWORD
+ if (!seededUserPassword || !seededAdminPassword) {
+ throw new Error('Missing SEED_USER_PASSWORD or SEED_ADMIN_PASSWORD')
+ }
+ const userPassword = await hashPassword(seededUserPassword)
+ const adminPassword = await hashPassword(seededAdminPassword)📝 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.
Suggested change
| const userPassword = await hashPassword('secret') | |
| const adminPassword = await hashPassword('admin-secret') | |
| const seededUserPassword = process.env.SEED_USER_PASSWORD | |
| const seededAdminPassword = process.env.SEED_ADMIN_PASSWORD | |
| if (!seededUserPassword || !seededAdminPassword) { | |
| throw new Error('Missing SEED_USER_PASSWORD or SEED_ADMIN_PASSWORD') | |
| } | |
| const userPassword = await hashPassword(seededUserPassword) | |
| const adminPassword = await hashPassword(seededAdminPassword) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/blog-next/server/db/seeders/BlogSeeder.ts` around lines 14 - 15, The
seed currently uses hardcoded passwords ('secret'/'admin-secret') via
hashPassword to set userPassword and adminPassword in BlogSeeder; change it to
read passwords from environment variables (e.g., process.env.SEED_USER_PASSWORD
and process.env.SEED_ADMIN_PASSWORD) and only allow defaults in local/dev
(NODE_ENV==='development' or similar), otherwise throw/exit if the env vars are
missing so privileged accounts cannot be seeded with predictable credentials;
update the userPassword/adminPassword assignments to use these env values before
calling hashPassword and add a clear error path when required vars are absent.
Comment on lines
+7
to
+9
| table.string('name') | ||
| table.string('email').unique() | ||
| table.string('password').nullable() |
There was a problem hiding this comment.
Make admin identity columns non-nullable.
Line 7 and Line 8 define name/email without notNullable(). For an auth table this allows incomplete identities, and unique() on nullable email can still permit multiple NULL rows depending on DB engine.
🔧 Proposed fix
- table.string('name')
- table.string('email').unique()
+ table.string('name').notNullable()
+ table.string('email').notNullable().unique()📝 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.
Suggested change
| table.string('name') | |
| table.string('email').unique() | |
| table.string('password').nullable() | |
| table.string('name').notNullable() | |
| table.string('email').notNullable().unique() | |
| table.string('password').nullable() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/blog-sveltekit/server/db/migrations/2026_04_25_044031_create_admins.ts`
around lines 7 - 9, The migration creates the admins table with nullable
identity columns; update the table schema in create_admins.ts so that
table.string('name') and table.string('email').unique() are declared
notNullable() (keep password as nullable if intended) to enforce required
identities and ensure the unique constraint applies correctly across rows; run
or regenerate the migration if your workflow requires a new migration artifact
after this change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/blog-next/app/api/auth/workos/logout/route.ts (1)
13-13: ⚡ Quick winAlign 500 error payloads with the existing
WorkosLogoutResultshape.The 500 responses use
{ ok: false, error }while the 422 response on Line 28 forwards theWorkosLogoutResult{ ok: false, code, message }. Clients reading both branches must handle two shapes. Consider emitting acode+messagefrom the 500 path so callers can parse failures uniformly.♻️ Proposed adjustment
- } catch (error) { - return Response.json({ ok: false, error: getErrorMessage(error) }, { status: 500 }) - } + } catch (error) { + return Response.json( + { ok: false, code: 'workos_provider_lookup_failed', message: getErrorMessage(error) }, + { status: 500 }, + ) + } @@ - } catch (error) { - return Response.json({ ok: false, error: getErrorMessage(error) }, { status: 500 }) - } + } catch (error) { + return Response.json( + { ok: false, code: 'workos_logout_failed', message: getErrorMessage(error) }, + { status: 500 }, + ) + }Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-next/app/api/auth/workos/logout/route.ts` at line 13, The 500 error responses currently return { ok: false, error: ... } which doesn't match the WorkosLogoutResult shape returned for 422; update both 500 branches (the Response.json calls that currently emit ok:false with an error field) to return { ok: false, code: 'internal_error', message: getErrorMessage(error) } (or a similar internal error code) and keep the status: 500 so callers always receive the WorkosLogoutResult { ok, code, message } shape; locate the Response.json invocations in the logout route handler to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/blog-next/app/api/auth/workos/logout/route.ts`:
- Line 13: The 500 error responses currently return { ok: false, error: ... }
which doesn't match the WorkosLogoutResult shape returned for 422; update both
500 branches (the Response.json calls that currently emit ok:false with an error
field) to return { ok: false, code: 'internal_error', message:
getErrorMessage(error) } (or a similar internal error code) and keep the status:
500 so callers always receive the WorkosLogoutResult { ok, code, message }
shape; locate the Response.json invocations in the logout route handler to make
this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9c9a38f-5ae6-4be5-9a22-315150cffa8a
📒 Files selected for processing (16)
apps/blog-next/app/api/auth/clerk/logout/route.tsapps/blog-next/app/api/auth/workos/logout/route.tsapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/pages/super-admin/index.vueapps/blog-nuxt/server/api/auth/clerk/logout.post.tsapps/blog-nuxt/server/api/auth/workos/logout.post.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/api/auth/clerk/logout/+server.tsapps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.tspackages/auth-clerk/src/index.tspackages/auth-workos/src/index.tspackages/auth/src/next/client.tspackages/auth/src/next/server.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/db/src/model/ModelRegistry.tspackages/db/src/model/defineModelHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/blog-sveltekit/src/routes/+layout.svelte
- apps/blog-sveltekit/src/routes/api/auth/workos/logout/+server.ts
- packages/db/src/model/ModelRegistry.ts
- apps/blog-nuxt/server/api/auth/workos/logout.post.ts
- apps/blog-nuxt/app/pages/super-admin/index.vue
- apps/blog-nuxt/server/api/auth/clerk/logout.post.ts
- packages/db/src/model/defineModelHelpers.ts
- packages/auth/src/next/server.ts
- apps/blog-nuxt/app/app.vue
- apps/blog-next/app/api/auth/clerk/logout/route.ts
- packages/auth/src/next/client.ts
- packages/auth-clerk/src/index.ts
- apps/blog-sveltekit/src/routes/api/auth/clerk/logout/+server.ts
- packages/cli/src/project/scaffold/framework-renderers.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests