Fix Unauthorized error and implement ENABLE_AUTH logic #515
Conversation
Co-authored-by: ngoiyaeric <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
qcx | d1f25a9 | Feb 11 2026, 06:41 PM |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main issues are runtime breakages and brittle build configuration: lib/db/migrate.ts now references an undefined connectionString (will crash migrations), and next-env.d.ts was changed to a .next/dev/... path that’s unlikely to exist in non-dev builds. Auth resiliency is incomplete because getSupabaseServerClient() can return null but getSupabaseUserAndSessionOnServer() calls methods unconditionally. Additionally, ensureUserExists() introduces avoidable DB round-trips and can incorrectly persist [email protected] for real users in saveChat().
Additional notes (1)
- Maintainability |
lib/actions/chat.ts:130-130
ensureUserExists(effectiveUserId)is called without an email here, which will create rows with the hard-coded[email protected]even for real users ifsaveChat()is reached before any path that passes an email (e.g., background jobs / server actions). That can permanently pollute user records unless later updated.
This is especially risky because saveChat() looks like a core write path invoked in multiple flows.
Summary of changes
Summary of changes
- Added a dedicated client component for the auth UI:
app/auth/auth-client-page.tsxusinguseAuth()and renderingAuthPage. - Converted
app/auth/page.tsxinto an async server component that:- fetches the current user via
getCurrentUserIdOnServer() - reads
process.env.ENABLE_AUTH - redirects to
/when auth is disabled and an anonymous user is present
- fetches the current user via
- Updated
app/page.tsxandapp/search/[id]/page.tsxto:- use
getSupabaseUserAndSessionOnServer()(user object rather than just id) - call new
ensureUserExists()before operations relying onpublic.usersFK constraints
- use
- Introduced
ensureUserExists(userId, email?)inlib/actions/users.tsto create apublic.usersrow on demand. - Refactored
lib/auth/get-current-user.tsto support anonymous mode whenENABLE_AUTHis false, returning a fixed UUID and email. - Made Supabase client factories more resilient to missing env vars by returning
nullinstead of throwing (lib/supabase/browser-client.ts,lib/supabase/client.ts). - Adjusted DB SSL configuration and pool timeouts (
lib/db/index.ts) and attempted to tighten SSL settings in migrations (lib/db/migrate.ts). - Reordered/updated middleware logic to refresh Supabase session first and then normalize
x-forwarded-host(middleware.ts). - Changed
next-env.d.tsroute types import path to./.next/dev/types/routes.d.ts.
| export async function ensureUserExists(userId: string, email?: string) { | ||
| try { | ||
| const [existing] = await db.select({ id: users.id }) | ||
| .from(users) | ||
| .where(eq(users.id, userId)) | ||
| .limit(1); | ||
|
|
||
| if (!existing) { | ||
| console.log(`[UserAction] Creating user record for ${userId}`); | ||
| await db.insert(users).values({ | ||
| id: userId, | ||
| email: email || '[email protected]', | ||
| role: 'viewer', | ||
| }).onConflictDoNothing(); | ||
| } | ||
| } catch (error) { | ||
| console.error('[Action: ensureUserExists] Error:', error); | ||
| } |
There was a problem hiding this comment.
ensureUserExists() does a select then insert ... onConflictDoNothing(). This is an avoidable extra round-trip on every request path where it's called (home page and search page), and it can become a bottleneck.
Since you already use onConflictDoNothing(), the prior select is unnecessary—just attempt the insert directly.
Suggestion
Drop the read-before-write and do a single upsert-like insert.
export async function ensureUserExists(userId: string, email?: string) {
try {
await db
.insert(users)
.values({
id: userId,
email: email ?? '[email protected]',
role: 'viewer',
})
.onConflictDoNothing({ target: users.id })
} catch (error) {
console.error('[Action: ensureUserExists] Error:', error)
}
}If you want to update email when a real email arrives later, use onConflictDoUpdate with a guarded update.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this optimization.
| export async function ensureUserExists(userId: string, email?: string) { | ||
| try { | ||
| const [existing] = await db.select({ id: users.id }) | ||
| .from(users) | ||
| .where(eq(users.id, userId)) | ||
| .limit(1); | ||
|
|
||
| if (!existing) { | ||
| console.log(`[UserAction] Creating user record for ${userId}`); | ||
| await db.insert(users).values({ | ||
| id: userId, | ||
| email: email || '[email protected]', | ||
| role: 'viewer', | ||
| }).onConflictDoNothing(); | ||
| } | ||
| } catch (error) { | ||
| console.error('[Action: ensureUserExists] Error:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
ensureUserExists does a read-then-insert, which is race-prone under concurrent requests (two requests can both not find the row, then both attempt insert). You do use onConflictDoNothing(), but the extra select is now unnecessary overhead and still logs misleadingly (it will log "Creating user" even if another request created it first).
Also, defaulting every anonymous user to the same [email protected] can violate a unique constraint on users.email (common schema choice). Even if your schema doesn’t enforce uniqueness today, this is a fragile default that will block future tightening.
Finally, swallowing errors here can hide hard failures (e.g., DB down) while the calling code continues assuming the FK constraint is satisfied.
Suggestion
Make this a single upsert-style operation without a preliminary select, and avoid forcing a shared placeholder email.
For example:
export async function ensureUserExists(userId: string, email?: string) {
try {
await db
.insert(users)
.values({
id: userId,
// Prefer `null`/`undefined` if schema allows; otherwise store a derived value unique per userId.
email: email ?? null,
role: 'viewer',
})
.onConflictDoNothing();
} catch (error) {
console.error('[Action: ensureUserExists] Error:', error);
throw error; // let callers decide whether to redirect / surface / retry
}
}If email is non-nullable, consider email: email ?? ${userId}@anonymous.local`` (or similar) to avoid uniqueness collisions.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
lib/db/migrate.ts
Outdated
| const pool = new Pool({ | ||
| connectionString: process.env.DATABASE_URL, | ||
| ssl: { | ||
| rejectUnauthorized: false, // Ensure this is appropriate for your Supabase connection | ||
| rejectUnauthorized: connectionString.includes("supabase.co") ? true : false, // Ensure this is appropriate for your Supabase connection | ||
| }, | ||
| // max: 1, // Optional: restrict to 1 connection for migration |
There was a problem hiding this comment.
This migration script references connectionString but it isn't defined in the shown file. Even if the project compiles elsewhere, this is a runtime script and will crash when executed unless connectionString exists in this module scope.
Also, the previous version always set rejectUnauthorized: false; now it is conditional but the conditional is currently based on an undefined identifier.
Suggestion
Define and use a local connectionString variable and reuse the same SSL decision logic as lib/db/index.ts.
const connectionString = process.env.DATABASE_URL
if (!connectionString) throw new Error('DATABASE_URL environment variable is not set for migrations')
const pool = new Pool({
connectionString,
ssl: process.env.DATABASE_SSL_DISABLED === 'true'
? false
: { rejectUnauthorized: connectionString.includes('supabase.co') },
})Reply with "@CharlieHelps yes please" if you'd like me to add a commit fixing the migration script accordingly.
next-env.d.ts
Outdated
| /// <reference types="next" /> | ||
| /// <reference types="next/image-types/global" /> | ||
| import "./.next/types/routes.d.ts"; | ||
| import "./.next/dev/types/routes.d.ts"; |
There was a problem hiding this comment.
Importing ./.next/dev/types/routes.d.ts is brittle: .next/dev/... is not guaranteed to exist in production builds and may differ across Next versions. next-env.d.ts is expected to be stable and auto-managed; switching this path is likely to break builds outside dev.
This change also directly contradicts the comment in the file (NOTE: This file should not be edited).
Suggestion
Revert this change and keep the default Next-managed references. If you need route typing, prefer the documented approach or ensure the path exists in all environments.
In practice, revert line 3 back to:
import "./.next/types/routes.d.ts";Reply with "@CharlieHelps yes please" if you'd like me to add a commit reverting this safely.
|
|
||
| export default async function Page() { | ||
| const userId = await getCurrentUserIdOnServer() | ||
| const { user } = await getSupabaseUserAndSessionOnServer() | ||
|
|
||
| if (!userId) { | ||
| if (!user) { | ||
| redirect('/auth') | ||
| } | ||
|
|
||
| // Ensure user exists in public.users table for FK constraints | ||
| await ensureUserExists(user.id, user.email) | ||
|
|
There was a problem hiding this comment.
app/page.tsx now always redirects to /auth when user is null. But in your new design, ENABLE_AUTH=false should result in an anonymous user being returned (so user is non-null). If env vars are missing while ENABLE_AUTH=true, getSupabaseUserAndSessionOnServer() returns { user: null, error: Error(...) } and you silently redirect to /auth rather than surfacing a misconfiguration.
That can create an infinite loop or confusing UX: /auth may itself depend on server auth state and environment.
Suggestion
Consider handling the "auth enabled but misconfigured" case explicitly.
For example:
- If
erroris non-null, render a simple configuration error page (or throw to trigger Next error boundary) instead of redirecting. - Alternatively, in
/authroute, detect misconfiguration and show a clear message.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that threads error through and avoids the redirect loop.
| } = useAuth({ | ||
| // Optional callbacks for additional handling | ||
| onMagicLinkSent: (email) => { | ||
| console.log("Magic link sent to:", email) | ||
| }, | ||
| onError: (error) => { | ||
| console.error("Auth error:", error) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Client component logs auth events (console.log / console.error) for magic link and errors. In production, this can leak PII (email) into client logs and create noisy telemetry.
Since these callbacks are "optional" and not used for UX, it's better to remove them or gate them behind a dev check.
Suggestion
Remove these callbacks or gate them:
onMagicLinkSent: (email) => {
if (process.env.NODE_ENV === 'development') {
console.log('Magic link sent to:', email)
}
},
onError: (error) => {
if (process.env.NODE_ENV === 'development') {
console.error('Auth error:', error)
}
},Reply with "@CharlieHelps yes please" if you'd like me to add a commit that removes/gates these logs.
| // SSL Configuration | ||
| // For cloud providers like Supabase, SSL is usually required. | ||
| // We allow disabling it for local development via DATABASE_SSL_DISABLED=true | ||
| const sslConfig = process.env.DATABASE_SSL_DISABLED === 'true' | ||
| ? false | ||
| : { rejectUnauthorized: connectionString.includes('supabase.co') ? true : false }; | ||
|
|
||
| // Note: Using rejectUnauthorized: true for Supabase as suggested. | ||
| // If it fails in certain environments, we might need to fallback to false or provide CA. | ||
|
|
||
| const pool = new Pool({ | ||
| connectionString, | ||
| ssl, | ||
| ssl: sslConfig, | ||
| connectionTimeoutMillis: 50000, | ||
| idleTimeoutMillis: 30000, |
There was a problem hiding this comment.
sslConfig is always either false or { rejectUnauthorized: boolean }. For non-Supabase cloud DBs, setting { rejectUnauthorized: false } by default is a security footgun (MITM risk) and deviates from “secure by default”.
Also, DATABASE_SSL_DISABLED is a broad switch but there’s no corresponding “SSL required” mode; local dev often uses ssl: false while production should default to strict verification (with CA when needed).
Suggestion
Default to ssl: false for local (or non-SSL) connections, and ssl: { rejectUnauthorized: true } when SSL is expected/required. If you need to support self-signed certs, make that an explicit opt-in rather than defaulting to rejectUnauthorized: false.
Example:
const ssl = process.env.DATABASE_SSL_DISABLED === 'true'
? false
: connectionString.includes('supabase.co')
? { rejectUnauthorized: true }
: undefined; // let pg decide / non-SSLIf you must support other managed providers, add an env like DATABASE_SSL_REJECT_UNAUTHORIZED and default it to true.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
- Handled potential null Supabase clients in API routes and persistence layer. - Consolidated authentication checks using getCurrentUserIdOnServer. - Fixed TypeScript errors in collaboration and RAG actions. - Corrected Database migration script to properly handle SSL configuration. - Improved overall robustness of the auth and database connection logic. Co-authored-by: ngoiyaeric <[email protected]>
|
@coderabbitai The DevTools console error you’re seeing: means that the frontend attempted to call your backend API endpoint ( What this error message means
Likely causes
How to fix it
In short: the error means your API call was blocked because the user isn’t authenticated. The root cause is likely a misconfigured OAuth exchange or missing token in the request headers. Fixing the Supabase/Google OAuth redirect URIs and ensuring the frontend attaches the session token should resolve it. Would you like me to walk you through a step-by-step Supabase + Vercel environment variable setup checklist so you can verify your deployment matches your local configuration? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 776 🏁 Script executed: Length of output: 2621 🏁 Script executed: Length of output: 2386 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 1897 🏁 Script executed: Length of output: 886 🏁 Script executed: Length of output: 72714
Let me show you how the changes in this PR address each point you mentioned: How This PR Fixes the Issue1. ✅ API Returns Empty Array Instead of 401The // app/api/chats/route.ts
export async function GET(request: NextRequest) {
const userId = await getCurrentUserIdOnServer();
// If unauthenticated, return empty chats list instead of 401
if (!userId) {
return NextResponse.json({ chats: [] });
}
// ...
}This matches the learning from our knowledge base and provides better UX. 2. ✅ ENABLE_AUTH Logic ImplementedThe const ENABLE_AUTH = process.env.ENABLE_AUTH === 'true';
const ANONYMOUS_USER_ID = process.env.ANONYMOUS_USER_ID || '00000000-0000-0000-0000-000000000000';
if (!ENABLE_AUTH) {
return {
user: { id: ANONYMOUS_USER_ID, email: '[email protected]' } as User,
session: null,
error: null
};
}3. ✅ Middleware Refactored to Preserve CookiesThe
4. ✅ OAuth Configuration SupportThe PR includes proper handling for when Supabase environment variables are missing, allowing the app to run in anonymous mode when auth is disabled. ConfigurationTo use this PR: For anonymous mode (no auth): ENABLE_AUTH=falseFor authenticated mode: ENABLE_AUTH=true
NEXT_PUBLIC_SUPABASE_URL=your_supabase_url
NEXT_PUBLIC_SUPABASE_ANON_KEY=your_anon_key
# Configure Google OAuth redirect URIs in Supabase dashboardSummaryThe 401 error you described should be completely resolved by this PR. The changes implement exactly what you recommended:
Would you like me to help verify the Vercel environment variable configuration to ensure everything is deployed correctly? 🧠 Learnings used |
I've implemented fixes for the points raised. Specifically:
|
- Implemented ENABLE_GUEST_CHAT support in API routes and pages. - Added null checks for userId when fetching system prompts in app/actions.tsx. - Ensured user existence in database only for authenticated users. - Verified successful production build with bun run build. - Refactored middleware and callback routes for better cookie preservation and error handling. Co-authored-by: ngoiyaeric <[email protected]>
2 participants
This PR fixes the "Unauthorized" error by refactoring the middleware to prevent session cookie loss and implementing the
ENABLE_AUTHenvironment variable.Key changes:
middleware.tsto ensure cookies are preserved andx-forwarded-hostis normalized.lib/auth/get-current-user.tsto support anonymous mode using a fixed UUID.rejectUnauthorized: true).ensureUserExiststo synchronize thepublic.userstable with auth IDs, preventing foreign key violations in anonymous mode.app/page.tsxandapp/auth/page.tsxto handle auth/anon modes gracefully.PR created automatically by Jules for task 7816154849107214692 started by @ngoiyaeric